diff options
author | GitLab Release Tools Bot <delivery-team+release-tools@gitlab.com> | 2020-09-02 12:48:21 +0000 |
---|---|---|
committer | GitLab Release Tools Bot <delivery-team+release-tools@gitlab.com> | 2020-09-02 12:48:21 +0000 |
commit | cb6968910528d6752bf4ae07f36c8802cf49f728 (patch) | |
tree | bda0459ddfed8730e6939a62598ce4d2505df468 | |
parent | 2f99285841c974cd688afbd8b74b26c8dfb7c937 (diff) | |
parent | c4e5ddc61fadd842693a4f5dcb1732df1fbb5df2 (diff) | |
download | gitlab-ce-cb6968910528d6752bf4ae07f36c8802cf49f728.tar.gz |
Merge remote-tracking branch 'dev/13-1-stable' into 13-1-stable
85 files changed, 1403 insertions, 229 deletions
diff --git a/CHANGELOG.md b/CHANGELOG.md index 707b5a1746e..d8d5d739b19 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,35 @@ documentation](doc/development/changelog.md) for instructions on adding your own entry. +## 13.1.9 (2020-09-02) + +### Security (23 changes, 1 of them is from the community) + +- Check validity of project's import_url before mirroring repository. +- Show on two-factor authentication setup page groups that are the cause of this requirement. +- Prevent interrupted 2FA sign-in from signing-in incorrect user. +- Create new 2FA code each time user is entering 2FA setup page. +- Remove all sessions but current while enabling 2FA. +- Invalidate two factor sign-in when user password changes. +- Delete members invites created by users being deleted. +- Prevent OmniAuth from rendering arbitrary error messages. +- Prevent not-2fa authenticated users that are supposed to use it to consume api via session. +- Invalidate remember me when an active session is revoked. +- Add rate limit on webhooks testing feature. +- Add scope presence validation to OAuth Application creation. +- Allow only running job tokens for API authentication. +- Prevent Deploy Tokens to read project resources when repository is disabled. +- Change conan api to use proper workhorse validation. +- Ensure global ID is of Snippet type in GraphQL destroy mutation. +- Fix Improper Access Control on Deploy-Key. +- Set maximum limit for profile events. +- Persist EKS External ID before presenting it to the user. +- Prevent project maintainers from editing group badges. +- Upgrade jquery to v3.5. +- Update websocket-extensions gem to 0.1.5. (Vitor Meireles De Sousa) +- Update GitLab Runner Helm Chart to 0.18.3. + + ## 13.1.8 (2020-08-18) - No changes. diff --git a/GITALY_SERVER_VERSION b/GITALY_SERVER_VERSION index 8761dcbecbb..fefe0f3c920 100644 --- a/GITALY_SERVER_VERSION +++ b/GITALY_SERVER_VERSION @@ -1 +1 @@ -13.1.8 +13.1.9
\ No newline at end of file diff --git a/Gemfile.lock b/Gemfile.lock index 69bd5235472..6201be00000 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -1138,7 +1138,7 @@ GEM railties (>= 3.2.0) websocket-driver (0.7.1) websocket-extensions (>= 0.1.0) - websocket-extensions (0.1.4) + websocket-extensions (0.1.5) wikicloth (0.8.1) builder expression_parser @@ -1 +1 @@ -13.1.8 +13.1.9 diff --git a/app/controllers/clusters/clusters_controller.rb b/app/controllers/clusters/clusters_controller.rb index 46dec5f3287..e0b65fec94e 100644 --- a/app/controllers/clusters/clusters_controller.rb +++ b/app/controllers/clusters/clusters_controller.rb @@ -36,8 +36,7 @@ class Clusters::ClustersController < Clusters::BaseController def new if params[:provider] == 'aws' - @aws_role = current_user.aws_role || Aws::Role.new - @aws_role.ensure_role_external_id! + @aws_role = Aws::Role.create_or_find_by!(user: current_user) @instance_types = load_instance_types.to_json elsif params[:provider] == 'gcp' @@ -271,7 +270,7 @@ class Clusters::ClustersController < Clusters::BaseController end def aws_role_params - params.require(:cluster).permit(:role_arn, :role_external_id) + params.require(:cluster).permit(:role_arn) end def generate_gcp_authorize_url diff --git a/app/controllers/concerns/authenticates_with_two_factor.rb b/app/controllers/concerns/authenticates_with_two_factor.rb index b885e55f902..d9e4a3a3aea 100644 --- a/app/controllers/concerns/authenticates_with_two_factor.rb +++ b/app/controllers/concerns/authenticates_with_two_factor.rb @@ -22,6 +22,8 @@ module AuthenticatesWithTwoFactor return handle_locked_user(user) unless user.can?(:log_in) session[:otp_user_id] = user.id + session[:user_updated_at] = user.updated_at + setup_u2f_authentication(user) render 'devise/sessions/two_factor' end @@ -41,6 +43,7 @@ module AuthenticatesWithTwoFactor def authenticate_with_two_factor user = self.resource = find_user return handle_locked_user(user) unless user.can?(:log_in) + return handle_changed_user(user) if user_changed?(user) if user_params[:otp_attempt].present? && session[:otp_user_id] authenticate_with_two_factor_via_otp(user) @@ -59,12 +62,14 @@ module AuthenticatesWithTwoFactor def clear_two_factor_attempt! session.delete(:otp_user_id) + session.delete(:user_updated_at) + session.delete(:challenge) end def authenticate_with_two_factor_via_otp(user) if valid_otp_attempt?(user) # Remove any lingering user data from login - session.delete(:otp_user_id) + clear_two_factor_attempt! remember_me(user) if user_params[:remember_me] == '1' user.save! @@ -81,8 +86,7 @@ module AuthenticatesWithTwoFactor def authenticate_with_two_factor_via_u2f(user) if U2fRegistration.authenticate(user, u2f_app_id, user_params[:device_response], session[:challenge]) # Remove any lingering user data from login - session.delete(:otp_user_id) - session.delete(:challenge) + clear_two_factor_attempt! remember_me(user) if user_params[:remember_me] == '1' sign_in(user, message: :two_factor_authenticated, event: :authentication) @@ -109,4 +113,18 @@ module AuthenticatesWithTwoFactor end end # rubocop: enable CodeReuse/ActiveRecord + + def handle_changed_user(user) + clear_two_factor_attempt! + + redirect_to new_user_session_path, alert: _('An error occurred. Please sign in again.') + end + + # If user has been updated since we validated the password, + # the password might have changed. + def user_changed?(user) + return false unless session[:user_updated_at] + + user.updated_at != session[:user_updated_at] + end end diff --git a/app/controllers/concerns/enforces_two_factor_authentication.rb b/app/controllers/concerns/enforces_two_factor_authentication.rb index f1dd46648f1..02082f81598 100644 --- a/app/controllers/concerns/enforces_two_factor_authentication.rb +++ b/app/controllers/concerns/enforces_two_factor_authentication.rb @@ -29,12 +29,11 @@ module EnforcesTwoFactorAuthentication end def two_factor_authentication_required? - Gitlab::CurrentSettings.require_two_factor_authentication? || - current_user.try(:require_two_factor_authentication_from_group?) + two_factor_verifier.two_factor_authentication_required? end def current_user_requires_two_factor? - current_user && !current_user.temp_oauth_email? && !current_user.two_factor_enabled? && !skip_two_factor? + two_factor_verifier.current_user_needs_to_setup_two_factor? && !skip_two_factor? end # rubocop: disable CodeReuse/ActiveRecord @@ -43,7 +42,7 @@ module EnforcesTwoFactorAuthentication if Gitlab::CurrentSettings.require_two_factor_authentication? global.call else - groups = current_user.expanded_groups_requiring_two_factor_authentication.reorder(name: :asc) + groups = current_user.source_groups_of_two_factor_authentication_requirement.reorder(name: :asc) group.call(groups) end end @@ -51,14 +50,11 @@ module EnforcesTwoFactorAuthentication # rubocop: enable CodeReuse/ActiveRecord def two_factor_grace_period - periods = [Gitlab::CurrentSettings.two_factor_grace_period] - periods << current_user.two_factor_grace_period if current_user.try(:require_two_factor_authentication_from_group?) - periods.min + two_factor_verifier.two_factor_grace_period end def two_factor_grace_period_expired? - date = current_user.otp_grace_period_started_at - date && (date + two_factor_grace_period.hours) < Time.current + two_factor_verifier.two_factor_grace_period_expired? end def two_factor_skippable? @@ -70,6 +66,10 @@ module EnforcesTwoFactorAuthentication def skip_two_factor? session[:skip_two_factor] && session[:skip_two_factor] > Time.current end + + def two_factor_verifier + @two_factor_verifier ||= Gitlab::Auth::TwoFactorAuthVerifier.new(current_user) # rubocop:disable Gitlab/ModuleWithInstanceVariables + end end EnforcesTwoFactorAuthentication.prepend_if_ee('EE::EnforcesTwoFactorAuthentication') diff --git a/app/controllers/concerns/hooks_execution.rb b/app/controllers/concerns/hooks_execution.rb index e8add1f4055..ad1f8341109 100644 --- a/app/controllers/concerns/hooks_execution.rb +++ b/app/controllers/concerns/hooks_execution.rb @@ -17,4 +17,16 @@ module HooksExecution flash[:alert] = "Hook execution failed: #{message}" end end + + def create_rate_limit(key, scope) + if rate_limiter.throttled?(key, scope: [scope, current_user]) + rate_limiter.log_request(request, "#{key}_request_limit".to_sym, current_user) + + render plain: _('This endpoint has been requested too many times. Try again later.'), status: :too_many_requests + end + end + + def rate_limiter + ::Gitlab::ApplicationRateLimiter + end end diff --git a/app/controllers/omniauth_callbacks_controller.rb b/app/controllers/omniauth_callbacks_controller.rb index 4c595313cb6..7a8c8cff913 100644 --- a/app/controllers/omniauth_callbacks_controller.rb +++ b/app/controllers/omniauth_callbacks_controller.rb @@ -49,12 +49,6 @@ class OmniauthCallbacksController < Devise::OmniauthCallbacksController redirect_unverified_saml_initiation end - def omniauth_error - @provider = params[:provider] - @error = params[:error] - render 'errors/omniauth_error', layout: "oauth_error", status: :unprocessable_entity - end - def cas3 ticket = params['ticket'] if ticket @@ -198,9 +192,10 @@ class OmniauthCallbacksController < Devise::OmniauthCallbacksController end def fail_login(user) - error_message = user.errors.full_messages.to_sentence + @provider = Gitlab::Auth::OAuth::Provider.label_for(params[:action]) + @error = user.errors.full_messages.to_sentence - return redirect_to omniauth_error_path(oauth['provider'], error: error_message) + render 'errors/omniauth_error', layout: "oauth_error", status: :unprocessable_entity end def fail_auth0_login diff --git a/app/controllers/profiles/active_sessions_controller.rb b/app/controllers/profiles/active_sessions_controller.rb index f409193aefc..d9ec3195fd1 100644 --- a/app/controllers/profiles/active_sessions_controller.rb +++ b/app/controllers/profiles/active_sessions_controller.rb @@ -7,6 +7,7 @@ class Profiles::ActiveSessionsController < Profiles::ApplicationController def destroy ActiveSession.destroy_with_public_id(current_user, params[:id]) + current_user.forget_me! respond_to do |format| format.html { redirect_to profile_active_sessions_url, status: :found } diff --git a/app/controllers/profiles/two_factor_auths_controller.rb b/app/controllers/profiles/two_factor_auths_controller.rb index 95b9344c551..50fbf8146e5 100644 --- a/app/controllers/profiles/two_factor_auths_controller.rb +++ b/app/controllers/profiles/two_factor_auths_controller.rb @@ -4,7 +4,7 @@ class Profiles::TwoFactorAuthsController < Profiles::ApplicationController skip_before_action :check_two_factor_requirement def show - unless current_user.otp_secret + unless current_user.two_factor_enabled? current_user.otp_secret = User.generate_otp_secret(32) end @@ -38,6 +38,8 @@ class Profiles::TwoFactorAuthsController < Profiles::ApplicationController def create if current_user.validate_and_consume_otp!(params[:pin_code]) + ActiveSession.destroy_all_but_current(current_user, session) + Users::UpdateService.new(current_user, user: current_user, otp_required_for_login: true).execute! do |user| @codes = user.generate_otp_backup_codes! end diff --git a/app/controllers/projects/hooks_controller.rb b/app/controllers/projects/hooks_controller.rb index 097a357889f..2f4dc1ddc3a 100644 --- a/app/controllers/projects/hooks_controller.rb +++ b/app/controllers/projects/hooks_controller.rb @@ -6,6 +6,7 @@ class Projects::HooksController < Projects::ApplicationController # Authorize before_action :authorize_admin_project! before_action :hook_logs, only: :edit + before_action -> { create_rate_limit(:project_testing_hook, @project) }, only: :test respond_to :html diff --git a/app/controllers/projects/tags_controller.rb b/app/controllers/projects/tags_controller.rb index df20daa8f7e..475ca8894e4 100644 --- a/app/controllers/projects/tags_controller.rb +++ b/app/controllers/projects/tags_controller.rb @@ -92,7 +92,7 @@ class Projects::TagsController < Projects::ApplicationController end format.js do - render status: :unprocessable_entity + render status: :ok end end end diff --git a/app/controllers/sessions_controller.rb b/app/controllers/sessions_controller.rb index 9e8075d4bcc..4e1228dec39 100644 --- a/app/controllers/sessions_controller.rb +++ b/app/controllers/sessions_controller.rb @@ -8,6 +8,7 @@ class SessionsController < Devise::SessionsController include Recaptcha::Verify include RendersLdapServers include KnownSignIn + include Gitlab::Utils::StrongMemoize skip_before_action :check_two_factor_requirement, only: [:destroy] # replaced with :require_no_authentication_without_flash @@ -197,10 +198,14 @@ class SessionsController < Devise::SessionsController end def find_user - if session[:otp_user_id] - User.find(session[:otp_user_id]) - elsif user_params[:login] - User.by_login(user_params[:login]) + strong_memoize(:find_user) do + if session[:otp_user_id] && user_params[:login] + User.by_id_and_login(session[:otp_user_id], user_params[:login]).first + elsif session[:otp_user_id] + User.find(session[:otp_user_id]) + elsif user_params[:login] + User.by_login(user_params[:login]) + end end end diff --git a/app/finders/ci/auth_job_finder.rb b/app/finders/ci/auth_job_finder.rb new file mode 100644 index 00000000000..aee7dd16341 --- /dev/null +++ b/app/finders/ci/auth_job_finder.rb @@ -0,0 +1,56 @@ +# frozen_string_literal: true +module Ci + class AuthJobFinder + AuthError = Class.new(StandardError) + NotRunningJobError = Class.new(AuthError) + ErasedJobError = Class.new(AuthError) + DeletedProjectError = Class.new(AuthError) + + def initialize(token:) + @token = token + end + + def execute! + find_job_by_token.tap do |job| + next unless job + + validate_job!(job) + end + end + + def execute + execute! + rescue AuthError + end + + private + + attr_reader :token, :require_running, :raise_on_missing + + def find_job_by_token + ::Ci::Build.find_by_token(token) + end + + def validate_job!(job) + validate_running_job!(job) + validate_job_not_erased!(job) + validate_project_presence!(job) + + true + end + + def validate_running_job!(job) + raise NotRunningJobError, 'Job is not running' unless job.running? + end + + def validate_job_not_erased!(job) + raise ErasedJobError, 'Job has been erased!' if job.erased? + end + + def validate_project_presence!(job) + if job.project.nil? || job.project.pending_delete? + raise DeletedProjectError, 'Project has been deleted!' + end + end + end +end diff --git a/app/finders/user_recent_events_finder.rb b/app/finders/user_recent_events_finder.rb index 3f2e813d381..f376b26ab9c 100644 --- a/app/finders/user_recent_events_finder.rb +++ b/app/finders/user_recent_events_finder.rb @@ -6,6 +6,7 @@ # WARNING: does not consider project feature visibility! # - user: The user for which to load the events # - params: +# - limit: Number of items that to be returned. Defaults to 20 and limited to 100. # - offset: The page of events to return class UserRecentEventsFinder prepend FinderWithCrossProjectAccess @@ -16,7 +17,8 @@ class UserRecentEventsFinder attr_reader :current_user, :target_user, :params - LIMIT = 20 + DEFAULT_LIMIT = 20 + MAX_LIMIT = 100 def initialize(current_user, target_user, params = {}) @current_user = current_user @@ -31,7 +33,7 @@ class UserRecentEventsFinder recent_events(params[:offset] || 0) .joins(:project) .with_associations - .limit_recent(params[:limit].presence || LIMIT, params[:offset]) + .limit_recent(limit, params[:offset]) end # rubocop: enable CodeReuse/ActiveRecord @@ -59,4 +61,10 @@ class UserRecentEventsFinder def projects target_user.project_interactions.to_sql end + + def limit + return DEFAULT_LIMIT unless params[:limit].present? + + [params[:limit].to_i, MAX_LIMIT].min + end end diff --git a/app/graphql/mutations/snippets/base.rb b/app/graphql/mutations/snippets/base.rb index c8cc721b2e0..023f876d035 100644 --- a/app/graphql/mutations/snippets/base.rb +++ b/app/graphql/mutations/snippets/base.rb @@ -11,7 +11,7 @@ module Mutations private def find_object(id:) - GitlabSchema.object_from_id(id) + GitlabSchema.object_from_id(id, expected_type: ::Snippet) end def authorized_resource?(snippet) diff --git a/app/models/active_session.rb b/app/models/active_session.rb index a23190cc8b3..f5e8382f945 100644 --- a/app/models/active_session.rb +++ b/app/models/active_session.rb @@ -102,6 +102,19 @@ class ActiveSession end end + def self.destroy_all_but_current(user, current_session) + session_ids = not_impersonated(user) + session_ids.reject! { |session| session.current?(current_session) } if current_session + + Gitlab::Redis::SharedState.with do |redis| + destroy_sessions(redis, user, session_ids.map(&:session_id)) if session_ids.any? + end + end + + def self.not_impersonated(user) + list(user).reject(&:is_impersonated) + end + def self.key_name(user_id, session_id = '*') "#{Gitlab::Redis::SharedState::USER_SESSIONS_NAMESPACE}:#{user_id}:#{session_id}" end diff --git a/app/models/aws/role.rb b/app/models/aws/role.rb index 54132be749d..7d34665082d 100644 --- a/app/models/aws/role.rb +++ b/app/models/aws/role.rb @@ -9,6 +9,7 @@ module Aws validates :role_external_id, uniqueness: true, length: { in: 1..64 } validates :role_arn, length: 1..2048, + allow_nil: true, format: { with: Gitlab::Regex.aws_arn_regex, message: Gitlab::Regex.aws_arn_regex_message diff --git a/app/models/clusters/applications/runner.rb b/app/models/clusters/applications/runner.rb index f0050b18204..5548651ebe0 100644 --- a/app/models/clusters/applications/runner.rb +++ b/app/models/clusters/applications/runner.rb @@ -3,7 +3,7 @@ module Clusters module Applications class Runner < ApplicationRecord - VERSION = '0.17.2' + VERSION = '0.18.3' self.table_name = 'clusters_applications_runners' diff --git a/app/models/member.rb b/app/models/member.rb index f2926d32d47..16108979f2e 100644 --- a/app/models/member.rb +++ b/app/models/member.rb @@ -71,6 +71,8 @@ class Member < ApplicationRecord scope :request, -> { where.not(requested_at: nil) } scope :non_request, -> { where(requested_at: nil) } + scope :not_accepted_invitations_by_user, -> (user) { invite.where(invite_accepted_at: nil, created_by: user) } + scope :has_access, -> { active.where('access_level > 0') } scope :guests, -> { active.where(access_level: GUEST) } diff --git a/app/models/user.rb b/app/models/user.rb index 431a5b3a5b7..56bf12db1a7 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -353,6 +353,7 @@ class User < ApplicationRecord scope :order_oldest_sign_in, -> { reorder(Gitlab::Database.nulls_last_order('current_sign_in_at', 'ASC')) } scope :order_recent_last_activity, -> { reorder(Gitlab::Database.nulls_last_order('last_activity_on', 'DESC')) } scope :order_oldest_last_activity, -> { reorder(Gitlab::Database.nulls_first_order('last_activity_on', 'ASC')) } + scope :by_id_and_login, ->(id, login) { where(id: id).where('username = LOWER(:login) OR email = LOWER(:login)', login: login) } def active_for_authentication? super && can?(:log_in) @@ -858,6 +859,12 @@ class User < ApplicationRecord all_expanded_groups.where(require_two_factor_authentication: true) end + def source_groups_of_two_factor_authentication_requirement + Gitlab::ObjectHierarchy.new(expanded_groups_requiring_two_factor_authentication) + .all_objects + .where(id: groups) + end + # rubocop: disable CodeReuse/ServiceClass def refresh_authorized_projects Users::RefreshAuthorizedProjectsService.new(self).execute diff --git a/app/services/applications/create_service.rb b/app/services/applications/create_service.rb index 500db1e172a..92500fbc254 100644 --- a/app/services/applications/create_service.rb +++ b/app/services/applications/create_service.rb @@ -11,7 +11,16 @@ module Applications # EE would override and use `request` arg def execute(request) - Doorkeeper::Application.create(params) + @application = Doorkeeper::Application.new(params) + + unless params[:scopes].present? + @application.errors.add(:base, _("Scopes can't be blank")) + + return @application + end + + @application.save + @application end end end diff --git a/app/services/auth/container_registry_authentication_service.rb b/app/services/auth/container_registry_authentication_service.rb index 44a434f4402..831a25a637e 100644 --- a/app/services/auth/container_registry_authentication_service.rb +++ b/app/services/auth/container_registry_authentication_service.rb @@ -132,6 +132,7 @@ module Auth def can_access?(requested_project, requested_action) return false unless requested_project.container_registry_enabled? + return false if requested_project.repository_access_level == ::ProjectFeature::DISABLED case requested_action when 'pull' diff --git a/app/services/ci/pipeline_trigger_service.rb b/app/services/ci/pipeline_trigger_service.rb index 37b9b4c362c..d9f41b7040e 100644 --- a/app/services/ci/pipeline_trigger_service.rb +++ b/app/services/ci/pipeline_trigger_service.rb @@ -10,6 +10,9 @@ module Ci elsif job_from_token create_pipeline_from_job(job_from_token) end + + rescue Ci::AuthJobFinder::AuthError => e + error(e.message, 401) end private @@ -41,8 +44,6 @@ module Ci # this check is to not leak the presence of the project if user cannot read it return unless can?(job.user, :read_project, project) - return error("400 Job has to be running", 400) unless job.running? - pipeline = Ci::CreatePipelineService.new(project, job.user, ref: params[:ref]) .execute(:pipeline, ignore_skip_ci: true) do |pipeline| source = job.sourced_pipelines.build( @@ -64,7 +65,7 @@ module Ci def job_from_token strong_memoize(:job) do - Ci::Build.find_by_token(params[:token].to_s) + Ci::AuthJobFinder.new(token: params[:token].to_s).execute! end end diff --git a/app/services/clusters/aws/authorize_role_service.rb b/app/services/clusters/aws/authorize_role_service.rb index 6eafce0597e..73e31c417ed 100644 --- a/app/services/clusters/aws/authorize_role_service.rb +++ b/app/services/clusters/aws/authorize_role_service.rb @@ -9,6 +9,7 @@ module Clusters ERRORS = [ ActiveRecord::RecordInvalid, + ActiveRecord::RecordNotFound, Clusters::Aws::FetchCredentialsService::MissingRoleError, ::Aws::Errors::MissingCredentialsError, ::Aws::STS::Errors::ServiceError @@ -20,7 +21,8 @@ module Clusters end def execute - @role = create_or_update_role! + ensure_role_exists! + update_role_arn! Response.new(:ok, credentials) rescue *ERRORS @@ -31,14 +33,12 @@ module Clusters attr_reader :role, :params - def create_or_update_role! - if role = user.aws_role - role.update!(params) + def ensure_role_exists! + @role = ::Aws::Role.find_by_user_id!(user.id) + end - role - else - user.create_aws_role!(params) - end + def update_role_arn! + role.update!(params) end def credentials diff --git a/app/services/members/destroy_service.rb b/app/services/members/destroy_service.rb index 20f64a99ad7..e2bc0e59f26 100644 --- a/app/services/members/destroy_service.rb +++ b/app/services/members/destroy_service.rb @@ -18,6 +18,7 @@ module Members end delete_subresources(member) unless skip_subresources + delete_project_invitations_by(member) unless skip_subresources enqueue_delete_todos(member) after_execute(member: member) @@ -32,24 +33,48 @@ module Members delete_project_members(member) delete_subgroup_members(member) + delete_invited_members(member) end def delete_project_members(member) groups = member.group.self_and_descendants - ProjectMember.in_namespaces(groups).with_user(member.user).each do |project_member| - self.class.new(current_user).execute(project_member, skip_authorization: @skip_auth) - end + destroy_project_members(ProjectMember.in_namespaces(groups).with_user(member.user)) end def delete_subgroup_members(member) groups = member.group.descendants - GroupMember.of_groups(groups).with_user(member.user).each do |group_member| + destroy_group_members(GroupMember.of_groups(groups).with_user(member.user)) + end + + def delete_invited_members(member) + groups = member.group.self_and_descendants + + destroy_group_members(GroupMember.of_groups(groups).not_accepted_invitations_by_user(member.user)) + + destroy_project_members(ProjectMember.in_namespaces(groups).not_accepted_invitations_by_user(member.user)) + end + + def destroy_project_members(members) + members.each do |project_member| + self.class.new(current_user).execute(project_member, skip_authorization: @skip_auth) + end + end + + def destroy_group_members(members) + members.each do |group_member| self.class.new(current_user).execute(group_member, skip_authorization: @skip_auth, skip_subresources: true) end end + def delete_project_invitations_by(member) + return unless member.is_a?(ProjectMember) && member.user && member.project + + members_to_delete = member.project.members.not_accepted_invitations_by_user(member.user) + destroy_project_members(members_to_delete) + end + def can_destroy_member?(member) can?(current_user, destroy_member_permission(member), member) end diff --git a/app/services/projects/update_remote_mirror_service.rb b/app/services/projects/update_remote_mirror_service.rb index 5f8ef75a8d7..05eca148c97 100644 --- a/app/services/projects/update_remote_mirror_service.rb +++ b/app/services/projects/update_remote_mirror_service.rb @@ -7,6 +7,10 @@ module Projects def execute(remote_mirror, tries) return success unless remote_mirror.enabled? + if Gitlab::UrlBlocker.blocked_url?(CGI.unescape(Gitlab::UrlSanitizer.sanitize(remote_mirror.url))) + return error("The remote mirror URL is invalid.") + end + update_mirror(remote_mirror) success diff --git a/app/views/profiles/active_sessions/_active_session.html.haml b/app/views/profiles/active_sessions/_active_session.html.haml index f3ad0c4c8ad..19097ff73b6 100644 --- a/app/views/profiles/active_sessions/_active_session.html.haml +++ b/app/views/profiles/active_sessions/_active_session.html.haml @@ -27,6 +27,6 @@ - unless is_current_session .float-right - = link_to profile_active_session_path(active_session.public_id), data: { confirm: _('Are you sure? The device will be signed out of GitLab.') }, method: :delete, class: "btn btn-danger prepend-left-10" do + = link_to profile_active_session_path(active_session.public_id), data: { confirm: _('Are you sure? The device will be signed out of GitLab and all remember me tokens revoked.') }, method: :delete, class: "btn btn-danger prepend-left-10" do %span.sr-only= _('Revoke') = _('Revoke') diff --git a/config/routes/user.rb b/config/routes/user.rb index 9db3a71a270..63329277e33 100644 --- a/config/routes/user.rb +++ b/config/routes/user.rb @@ -25,7 +25,6 @@ devise_for :users, controllers: { omniauth_callbacks: :omniauth_callbacks, confirmations: :confirmations } devise_scope :user do - get '/users/auth/:provider/omniauth_error' => 'omniauth_callbacks#omniauth_error', as: :omniauth_error get '/users/almost_there' => 'confirmations#almost_there' end diff --git a/db/migrate/20200717040735_change_aws_roles_role_arn_null.rb b/db/migrate/20200717040735_change_aws_roles_role_arn_null.rb new file mode 100644 index 00000000000..707cfe96a7c --- /dev/null +++ b/db/migrate/20200717040735_change_aws_roles_role_arn_null.rb @@ -0,0 +1,15 @@ +# frozen_string_literal: true + +class ChangeAwsRolesRoleArnNull < ActiveRecord::Migration[6.0] + DOWNTIME = false + EXAMPLE_ARN = 'arn:aws:iam::000000000000:role/example-role' + + def up + change_column_null :aws_roles, :role_arn, true + end + + def down + # Records may now exist with nulls, so we must fill them with a dummy value + change_column_null :aws_roles, :role_arn, false, EXAMPLE_ARN + end +end diff --git a/db/structure.sql b/db/structure.sql index fbcbcfc0f16..f7c1737c20c 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -733,7 +733,7 @@ CREATE TABLE public.aws_roles ( user_id integer NOT NULL, created_at timestamp with time zone NOT NULL, updated_at timestamp with time zone NOT NULL, - role_arn character varying(2048) NOT NULL, + role_arn character varying(2048), role_external_id character varying(64) NOT NULL ); @@ -13999,5 +13999,6 @@ COPY "schema_migrations" (version) FROM STDIN; 20200615121217 20200615123055 20200615232735 +20200717040735 \. diff --git a/doc/user/profile/active_sessions.md b/doc/user/profile/active_sessions.md index 4dbb11b581d..a5b15a7880c 100644 --- a/doc/user/profile/active_sessions.md +++ b/doc/user/profile/active_sessions.md @@ -29,6 +29,11 @@ exceeds 100, the oldest ones are deleted. 1. Use the previous steps to navigate to **Active Sessions**. 1. Click on **Revoke** besides a session. The current session cannot be revoked, as this would sign you out of GitLab. +NOTE: **Note:** +When any session is revoked all **Remember me** tokens for all +devices will be revoked. See ['Why do I keep getting signed out?'](index.md#why-do-i-keep-getting-signed-out) +for more information about the **Remember me** feature. + <!-- ## Troubleshooting Include any troubleshooting steps that you can foresee. If you know beforehand what issues diff --git a/doc/user/profile/index.md b/doc/user/profile/index.md index 663a2888ee7..71ab88ecda1 100644 --- a/doc/user/profile/index.md +++ b/doc/user/profile/index.md @@ -255,6 +255,12 @@ to get you a new `_gitlab_session` and keep you signed in through browser restar After your `remember_user_token` expires and your `_gitlab_session` is cleared/expired, you will be asked to sign in again to verify your identity for security reasons. +NOTE: **Note:** +When any session is signed out, or when a session is revoked +via [Active Sessions](active_sessions.md), all **Remember me** tokens are revoked. +While other sessions will remain active, the **Remember me** feature will not restore +a session if the browser is closed or the existing session expires. + ### Increased sign-in time > [Introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/20340) in GitLab 13.1. @@ -264,7 +270,7 @@ The `remember_user_token` lifetime of a cookie can now extend beyond the deadlin GitLab uses both session and persistent cookies: - Session cookie: Session cookies are normally removed at the end of the browser session when the browser is closed. The `_gitlab_session` cookie has no expiration date. -- Persistent cookie: The `remember_me_token` is a cookie with an expiration date of two weeks. GitLab activates this cookie if you click Remember Me when you sign in. +- Persistent cookie: The `remember_user_token` is a cookie with an expiration date of two weeks. GitLab activates this cookie if you click Remember Me when you sign in. By default, the server sets a time-to-live (TTL) of 1-week on any session that is used. diff --git a/lib/api/api_guard.rb b/lib/api/api_guard.rb index c6557fce541..576a39d6a4e 100644 --- a/lib/api/api_guard.rb +++ b/lib/api/api_guard.rb @@ -68,7 +68,7 @@ module API deploy_token_from_request || find_user_from_access_token || find_user_from_job_token || - find_user_from_warden + user_from_warden end private @@ -101,6 +101,25 @@ module API def user_allowed_or_deploy_token?(user) Gitlab::UserAccess.new(user).allowed? || user.is_a?(DeployToken) end + + def user_from_warden + user = find_user_from_warden + + return unless user + return if two_factor_required_but_not_setup?(user) + + user + end + + def two_factor_required_but_not_setup?(user) + verifier = Gitlab::Auth::TwoFactorAuthVerifier.new(user) + + if verifier.two_factor_authentication_required? && verifier.current_user_needs_to_setup_two_factor? + verifier.two_factor_grace_period_expired? + else + false + end + end end class_methods do diff --git a/lib/api/badges.rb b/lib/api/badges.rb index d2152fad07b..f04621cdb4c 100644 --- a/lib/api/badges.rb +++ b/lib/api/badges.rb @@ -109,9 +109,10 @@ module API end put ":id/badges/:badge_id" do source = find_source_if_admin(source_type) + badge = find_badge(source) badge = ::Badges::UpdateService.new(declared_params(include_missing: false)) - .execute(find_badge(source)) + .execute(badge) if badge.valid? present_badges(source, badge) @@ -130,10 +131,6 @@ module API source = find_source_if_admin(source_type) badge = find_badge(source) - if badge.is_a?(GroupBadge) && source.is_a?(Project) - error!('To delete a Group badge please use the Group endpoint', 403) - end - destroy_conditionally!(badge) end end diff --git a/lib/api/helpers/badges_helpers.rb b/lib/api/helpers/badges_helpers.rb index 46ce5b4e7b5..f402c603c87 100644 --- a/lib/api/helpers/badges_helpers.rb +++ b/lib/api/helpers/badges_helpers.rb @@ -6,7 +6,13 @@ module API include ::API::Helpers::MembersHelpers def find_badge(source) - source.badges.find(params[:badge_id]) + badge_id = params[:badge_id] + + if source.is_a?(Project) + source.project_badges.find(badge_id) + else + source.badges.find(badge_id) + end end def present_badges(source, records, options = {}) diff --git a/lib/gitlab/application_rate_limiter.rb b/lib/gitlab/application_rate_limiter.rb index 3277ddd9f49..476529f818c 100644 --- a/lib/gitlab/application_rate_limiter.rb +++ b/lib/gitlab/application_rate_limiter.rb @@ -25,11 +25,13 @@ module Gitlab project_repositories_archive: { threshold: 5, interval: 1.minute }, project_generate_new_export: { threshold: 30, interval: 5.minutes }, project_import: { threshold: 30, interval: 5.minutes }, + project_testing_hook: { threshold: 5, interval: 1.minute }, play_pipeline_schedule: { threshold: 1, interval: 1.minute }, show_raw_controller: { threshold: -> { Gitlab::CurrentSettings.current_application_settings.raw_blob_request_limit }, interval: 1.minute }, group_export: { threshold: 30, interval: 5.minutes }, group_download_export: { threshold: 10, interval: 10.minutes }, - group_import: { threshold: 30, interval: 5.minutes } + group_import: { threshold: 30, interval: 5.minutes }, + group_testing_hook: { threshold: 5, interval: 1.minute } }.freeze end diff --git a/lib/gitlab/auth.rb b/lib/gitlab/auth.rb index 1a23814959d..3068d40d5ba 100644 --- a/lib/gitlab/auth.rb +++ b/lib/gitlab/auth.rb @@ -220,6 +220,8 @@ module Gitlab # Registry access (with jwt) does not have access to project return if project && !token.has_access_to?(project) + # When repository is disabled, no resources are accessible via Deploy Token + return if project&.repository_access_level == ::ProjectFeature::DISABLED scopes = abilities_for_scopes(token.scopes) diff --git a/lib/gitlab/auth/auth_finders.rb b/lib/gitlab/auth/auth_finders.rb index 93342fbad51..9bb655d3e07 100644 --- a/lib/gitlab/auth/auth_finders.rb +++ b/lib/gitlab/auth/auth_finders.rb @@ -63,9 +63,7 @@ module Gitlab current_request.env[JOB_TOKEN_HEADER].presence return unless token - job = ::Ci::Build.find_by_token(token) - raise UnauthorizedError unless job - + job = find_valid_running_job_by_token!(token) @current_authenticated_job = job # rubocop:disable Gitlab/ModuleWithInstanceVariables job.user @@ -78,9 +76,7 @@ module Gitlab return unless login.present? && password.present? return unless ::Ci::Build::CI_REGISTRY_USER == login - job = ::Ci::Build.find_by_token(password) - raise UnauthorizedError unless job - + job = find_valid_running_job_by_token!(password) job.user end @@ -268,6 +264,12 @@ module Gitlab def blob_request? current_request.path.include?('/raw/') end + + def find_valid_running_job_by_token!(token) + ::Ci::AuthJobFinder.new(token: token).execute.tap do |job| + raise UnauthorizedError unless job + end + end end end end diff --git a/lib/gitlab/auth/two_factor_auth_verifier.rb b/lib/gitlab/auth/two_factor_auth_verifier.rb new file mode 100644 index 00000000000..86552ef1267 --- /dev/null +++ b/lib/gitlab/auth/two_factor_auth_verifier.rb @@ -0,0 +1,36 @@ +# frozen_string_literal: true + +module Gitlab + module Auth + class TwoFactorAuthVerifier + attr_reader :current_user + + def initialize(current_user) + @current_user = current_user + end + + def two_factor_authentication_required? + Gitlab::CurrentSettings.require_two_factor_authentication? || + current_user&.require_two_factor_authentication_from_group? + end + + def current_user_needs_to_setup_two_factor? + current_user && !current_user.temp_oauth_email? && !current_user.two_factor_enabled? + end + + def two_factor_grace_period + periods = [Gitlab::CurrentSettings.two_factor_grace_period] + periods << current_user.two_factor_grace_period if current_user&.require_two_factor_authentication_from_group? + periods.min + end + + def two_factor_grace_period_expired? + time = current_user&.otp_grace_period_started_at + + return false unless time + + two_factor_grace_period.hours.since(time) < Time.current + end + end + end +end diff --git a/lib/gitlab/git_access.rb b/lib/gitlab/git_access.rb index 37e3da984d6..3ae9927e7c4 100644 --- a/lib/gitlab/git_access.rb +++ b/lib/gitlab/git_access.rb @@ -91,6 +91,13 @@ module Gitlab Guest.can?(:download_code, project) end + def deploy_key_can_download_code? + authentication_abilities.include?(:download_code) && + deploy_key? && + deploy_key.has_access_to?(container) && + project&.repository_access_level != ::Featurable::DISABLED + end + def user_can_download_code? authentication_abilities.include?(:download_code) && user_access.can_do_action?(:download_code) end @@ -235,7 +242,7 @@ module Gitlab end def check_download_access! - passed = deploy_key? || + passed = deploy_key_can_download_code? || deploy_token? || user_can_download_code? || build_can_download_code? || diff --git a/lib/gitlab/regex.rb b/lib/gitlab/regex.rb index 4caff8ae679..bc6cced2bf0 100644 --- a/lib/gitlab/regex.rb +++ b/lib/gitlab/regex.rb @@ -6,11 +6,6 @@ module Gitlab CONAN_RECIPE_FILES = %w[conanfile.py conanmanifest.txt].freeze CONAN_PACKAGE_FILES = %w[conaninfo.txt conanmanifest.txt conan_package.tgz].freeze - def conan_file_name_regex - @conan_file_name_regex ||= - %r{\A#{(CONAN_RECIPE_FILES + CONAN_PACKAGE_FILES).join("|")}\z}.freeze - end - def conan_package_reference_regex @conan_package_reference_regex ||= %r{\A[A-Za-z0-9]+\z}.freeze end diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 5198f084af3..95d528e5e2c 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -2491,6 +2491,9 @@ msgstr "" msgid "An error occurred while validating username" msgstr "" +msgid "An error occurred. Please sign in again." +msgstr "" + msgid "An error occurred. Please try again." msgstr "" @@ -2898,7 +2901,7 @@ msgstr "" msgid "Are you sure? Removing this GPG key does not affect already signed commits." msgstr "" -msgid "Are you sure? The device will be signed out of GitLab." +msgid "Are you sure? The device will be signed out of GitLab and all remember me tokens revoked." msgstr "" msgid "Are you sure? This will invalidate your registered applications and U2F devices." diff --git a/package.json b/package.json index de9d845cc9f..7e4a079985f 100644 --- a/package.json +++ b/package.json @@ -92,7 +92,7 @@ "imports-loader": "^0.8.0", "jed": "^1.1.1", "jest-transform-graphql": "^2.1.0", - "jquery": "^3.4.1", + "jquery": "^3.5.0", "jquery-ujs": "1.2.2", "jquery.caret": "^0.3.1", "jquery.waitforimages": "^2.2.0", diff --git a/spec/controllers/admin/applications_controller_spec.rb b/spec/controllers/admin/applications_controller_spec.rb index 732d20666cb..6c423097e70 100644 --- a/spec/controllers/admin/applications_controller_spec.rb +++ b/spec/controllers/admin/applications_controller_spec.rb @@ -40,7 +40,7 @@ RSpec.describe Admin::ApplicationsController do describe 'POST #create' do it 'creates the application' do - create_params = attributes_for(:application, trusted: true, confidential: false) + create_params = attributes_for(:application, trusted: true, confidential: false, scopes: ['api']) expect do post :create, params: { doorkeeper_application: create_params } @@ -63,7 +63,7 @@ RSpec.describe Admin::ApplicationsController do context 'when the params are for a confidential application' do it 'creates a confidential application' do - create_params = attributes_for(:application, confidential: true) + create_params = attributes_for(:application, confidential: true, scopes: ['read_user']) expect do post :create, params: { doorkeeper_application: create_params } @@ -75,6 +75,18 @@ RSpec.describe Admin::ApplicationsController do expect(application).to have_attributes(create_params.except(:uid, :owner_type)) end end + + context 'when scopes are not present' do + it 'renders the application form on errors' do + create_params = attributes_for(:application, trusted: true, confidential: false) + + expect do + post :create, params: { doorkeeper_application: create_params } + end.not_to change { Doorkeeper::Application.count } + + expect(response).to render_template :new + end + end end describe 'PATCH #update' do diff --git a/spec/controllers/admin/clusters_controller_spec.rb b/spec/controllers/admin/clusters_controller_spec.rb index d899e86ae5f..805f0e047bc 100644 --- a/spec/controllers/admin/clusters_controller_spec.rb +++ b/spec/controllers/admin/clusters_controller_spec.rb @@ -99,7 +99,9 @@ RSpec.describe Admin::ClustersController do end describe 'GET #new' do - def get_new(provider: 'gcp') + let(:user) { admin } + + def go(provider: 'gcp') get :new, params: { provider: provider } end @@ -112,7 +114,7 @@ RSpec.describe Admin::ClustersController do context 'when selected provider is gke and no valid gcp token exists' do it 'redirects to gcp authorize_url' do - get_new + go expect(response).to redirect_to(assigns(:authorize_url)) end @@ -125,7 +127,7 @@ RSpec.describe Admin::ClustersController do end it 'does not have authorize_url' do - get_new + go expect(assigns(:authorize_url)).to be_nil end @@ -137,7 +139,7 @@ RSpec.describe Admin::ClustersController do end it 'has new object' do - get_new + go expect(assigns(:gcp_cluster)).to be_an_instance_of(Clusters::ClusterPresenter) end @@ -158,16 +160,18 @@ RSpec.describe Admin::ClustersController do describe 'functionality for existing cluster' do it 'has new object' do - get_new + go expect(assigns(:user_cluster)).to be_an_instance_of(Clusters::ClusterPresenter) end end + include_examples 'GET new cluster shared examples' + describe 'security' do - it { expect { get_new }.to be_allowed_for(:admin) } - it { expect { get_new }.to be_denied_for(:user) } - it { expect { get_new }.to be_denied_for(:external) } + it { expect { go }.to be_allowed_for(:admin) } + it { expect { go }.to be_denied_for(:user) } + it { expect { go }.to be_denied_for(:external) } end end @@ -391,14 +395,13 @@ RSpec.describe Admin::ClustersController do end describe 'POST authorize AWS role for EKS cluster' do - let(:role_arn) { 'arn:aws:iam::123456789012:role/role-name' } - let(:role_external_id) { '12345' } + let!(:role) { create(:aws_role, user: admin) } + let(:role_arn) { 'arn:new-role' } let(:params) do { cluster: { - role_arn: role_arn, - role_external_id: role_external_id + role_arn: role_arn } } end @@ -412,28 +415,32 @@ RSpec.describe Admin::ClustersController do .and_return(double(execute: double)) end - it 'creates an Aws::Role record' do - expect { go }.to change { Aws::Role.count } + it 'updates the associated role with the supplied ARN' do + go expect(response).to have_gitlab_http_status(:ok) - - role = Aws::Role.last - expect(role.user).to eq admin - expect(role.role_arn).to eq role_arn - expect(role.role_external_id).to eq role_external_id + expect(role.reload.role_arn).to eq(role_arn) end - context 'role cannot be created' do + context 'supplied role is invalid' do let(:role_arn) { 'invalid-role' } - it 'does not create a record' do - expect { go }.not_to change { Aws::Role.count } + it 'does not update the associated role' do + expect { go }.not_to change { role.role_arn } expect(response).to have_gitlab_http_status(:unprocessable_entity) end end describe 'security' do + before do + allow_next_instance_of(Clusters::Aws::AuthorizeRoleService) do |service| + response = double(status: :ok, body: double) + + allow(service).to receive(:execute).and_return(response) + end + end + it { expect { go }.to be_allowed_for(:admin) } it { expect { go }.to be_denied_for(:user) } it { expect { go }.to be_denied_for(:external) } diff --git a/spec/controllers/application_controller_spec.rb b/spec/controllers/application_controller_spec.rb index 4002b7aca63..ce8eeb7a3d1 100644 --- a/spec/controllers/application_controller_spec.rb +++ b/spec/controllers/application_controller_spec.rb @@ -239,6 +239,7 @@ RSpec.describe ApplicationController do it 'does not redirect if 2FA is not required' do allow(controller).to receive(:two_factor_authentication_required?).and_return(false) + allow(controller).to receive(:current_user).and_return(create(:user)) expect(controller).not_to receive(:redirect_to) @@ -356,13 +357,17 @@ RSpec.describe ApplicationController do let(:user) { create :user, otp_grace_period_started_at: 2.hours.ago } it 'returns true if the grace period has expired' do - allow(controller).to receive(:two_factor_grace_period).and_return(1) + allow_next_instance_of(Gitlab::Auth::TwoFactorAuthVerifier) do |verifier| + allow(verifier).to receive(:two_factor_grace_period).and_return(2) + end expect(subject).to be_truthy end it 'returns false if the grace period is still active' do - allow(controller).to receive(:two_factor_grace_period).and_return(3) + allow_next_instance_of(Gitlab::Auth::TwoFactorAuthVerifier) do |verifier| + allow(verifier).to receive(:two_factor_grace_period).and_return(3) + end expect(subject).to be_falsey end diff --git a/spec/controllers/groups/clusters_controller_spec.rb b/spec/controllers/groups/clusters_controller_spec.rb index 6765cf0990a..d8f592e5a75 100644 --- a/spec/controllers/groups/clusters_controller_spec.rb +++ b/spec/controllers/groups/clusters_controller_spec.rb @@ -180,6 +180,8 @@ RSpec.describe Groups::ClustersController do end end + include_examples 'GET new cluster shared examples' + describe 'security' do it { expect { go }.to be_allowed_for(:admin) } it { expect { go }.to be_allowed_for(:owner).of(group) } @@ -453,14 +455,13 @@ RSpec.describe Groups::ClustersController do end describe 'POST authorize AWS role for EKS cluster' do - let(:role_arn) { 'arn:aws:iam::123456789012:role/role-name' } - let(:role_external_id) { '12345' } + let!(:role) { create(:aws_role, user: user) } + let(:role_arn) { 'arn:new-role' } let(:params) do { cluster: { - role_arn: role_arn, - role_external_id: role_external_id + role_arn: role_arn } } end @@ -474,28 +475,32 @@ RSpec.describe Groups::ClustersController do .and_return(double(execute: double)) end - it 'creates an Aws::Role record' do - expect { go }.to change { Aws::Role.count } + it 'updates the associated role with the supplied ARN' do + go expect(response).to have_gitlab_http_status(:ok) - - role = Aws::Role.last - expect(role.user).to eq user - expect(role.role_arn).to eq role_arn - expect(role.role_external_id).to eq role_external_id + expect(role.reload.role_arn).to eq(role_arn) end - context 'role cannot be created' do + context 'supplied role is invalid' do let(:role_arn) { 'invalid-role' } - it 'does not create a record' do - expect { go }.not_to change { Aws::Role.count } + it 'does not update the associated role' do + expect { go }.not_to change { role.role_arn } expect(response).to have_gitlab_http_status(:unprocessable_entity) end end describe 'security' do + before do + allow_next_instance_of(Clusters::Aws::AuthorizeRoleService) do |service| + response = double(status: :ok, body: double) + + allow(service).to receive(:execute).and_return(response) + end + end + it { expect { go }.to be_allowed_for(:admin) } it { expect { go }.to be_allowed_for(:owner).of(group) } it { expect { go }.to be_allowed_for(:maintainer).of(group) } diff --git a/spec/controllers/oauth/applications_controller_spec.rb b/spec/controllers/oauth/applications_controller_spec.rb index 33349037260..42c56d7fe64 100644 --- a/spec/controllers/oauth/applications_controller_spec.rb +++ b/spec/controllers/oauth/applications_controller_spec.rb @@ -123,7 +123,8 @@ RSpec.describe Oauth::ApplicationsController do invalid_uri_params = { doorkeeper_application: { name: 'foo', - redirect_uri: 'javascript://alert()' + redirect_uri: 'javascript://alert()', + scopes: ['api'] } } @@ -133,6 +134,23 @@ RSpec.describe Oauth::ApplicationsController do end end + context 'when scopes are not present' do + render_views + + it 'shows an error for blank scopes' do + invalid_uri_params = { + doorkeeper_application: { + name: 'foo', + redirect_uri: 'http://example.org' + } + } + + post :create, params: invalid_uri_params + + expect(response.body).to include 'Scopes can't be blank' + end + end + it_behaves_like 'redirects to login page when the user is not signed in' it_behaves_like 'redirects to 2fa setup page when the user requires it' end @@ -156,7 +174,8 @@ RSpec.describe Oauth::ApplicationsController do { doorkeeper_application: { name: 'foo', - redirect_uri: 'http://example.org' + redirect_uri: 'http://example.org', + scopes: ['api'] } } end diff --git a/spec/controllers/omniauth_callbacks_controller_spec.rb b/spec/controllers/omniauth_callbacks_controller_spec.rb index 0b99f28f79b..86c08e3162f 100644 --- a/spec/controllers/omniauth_callbacks_controller_spec.rb +++ b/spec/controllers/omniauth_callbacks_controller_spec.rb @@ -40,6 +40,22 @@ RSpec.describe OmniauthCallbacksController, type: :controller do end end + context 'when sign in is not valid' do + let(:provider) { :github } + let(:extern_uid) { 'my-uid' } + + it 'renders omniauth error page' do + allow_next_instance_of(Gitlab::Auth::OAuth::User) do |instance| + allow(instance).to receive(:valid_sign_in?).and_return(false) + end + + post provider + + expect(response).to render_template("errors/omniauth_error") + expect(response).to have_gitlab_http_status(:unprocessable_entity) + end + end + context 'when the user is on the last sign in attempt' do let(:extern_uid) { 'my-uid' } diff --git a/spec/controllers/profiles/active_sessions_controller_spec.rb b/spec/controllers/profiles/active_sessions_controller_spec.rb new file mode 100644 index 00000000000..f54f69d853d --- /dev/null +++ b/spec/controllers/profiles/active_sessions_controller_spec.rb @@ -0,0 +1,23 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Profiles::ActiveSessionsController do + describe 'DELETE destroy' do + let_it_be(:user) { create(:user) } + + before do + sign_in(user) + end + + it 'invalidates all remember user tokens' do + ActiveSession.set(user, request) + session_id = request.session.id.public_id + user.remember_me! + + delete :destroy, params: { id: session_id } + + expect(user.reload.remember_created_at).to be_nil + end + end +end diff --git a/spec/controllers/profiles/two_factor_auths_controller_spec.rb b/spec/controllers/profiles/two_factor_auths_controller_spec.rb index f645081219a..1fb0b18622b 100644 --- a/spec/controllers/profiles/two_factor_auths_controller_spec.rb +++ b/spec/controllers/profiles/two_factor_auths_controller_spec.rb @@ -14,10 +14,9 @@ RSpec.describe Profiles::TwoFactorAuthsController do let(:user) { create(:user) } it 'generates otp_secret for user' do - expect(User).to receive(:generate_otp_secret).with(32).and_return('secret').once + expect(User).to receive(:generate_otp_secret).with(32).and_call_original.once get :show - get :show # Second hit shouldn't re-generate it end it 'assigns qr_code' do @@ -27,6 +26,14 @@ RSpec.describe Profiles::TwoFactorAuthsController do get :show expect(assigns[:qr_code]).to eq code end + + it 'generates a unique otp_secret every time the page is loaded' do + expect(User).to receive(:generate_otp_secret).with(32).and_call_original.twice + + 2.times do + get :show + end + end end describe 'POST create' do @@ -57,6 +64,12 @@ RSpec.describe Profiles::TwoFactorAuthsController do expect(assigns[:codes]).to match_array %w(a b c) end + it 'calls to delete other sessions' do + expect(ActiveSession).to receive(:destroy_all_but_current) + + go + end + it 'renders create' do go expect(response).to render_template(:create) diff --git a/spec/controllers/projects/clusters_controller_spec.rb b/spec/controllers/projects/clusters_controller_spec.rb index 5645e25b741..71f78edb083 100644 --- a/spec/controllers/projects/clusters_controller_spec.rb +++ b/spec/controllers/projects/clusters_controller_spec.rb @@ -183,6 +183,8 @@ RSpec.describe Projects::ClustersController do end end + include_examples 'GET new cluster shared examples' + describe 'security' do it 'is allowed for admin when admin mode enabled', :enable_admin_mode do expect { go }.to be_allowed_for(:admin) @@ -479,14 +481,13 @@ RSpec.describe Projects::ClustersController do end describe 'POST authorize AWS role for EKS cluster' do - let(:role_arn) { 'arn:aws:iam::123456789012:role/role-name' } - let(:role_external_id) { '12345' } + let!(:role) { create(:aws_role, user: user) } + let(:role_arn) { 'arn:new-role' } let(:params) do { cluster: { - role_arn: role_arn, - role_external_id: role_external_id + role_arn: role_arn } } end @@ -500,28 +501,32 @@ RSpec.describe Projects::ClustersController do .and_return(double(execute: double)) end - it 'creates an Aws::Role record' do - expect { go }.to change { Aws::Role.count } + it 'updates the associated role with the supplied ARN' do + go expect(response).to have_gitlab_http_status(:ok) - - role = Aws::Role.last - expect(role.user).to eq user - expect(role.role_arn).to eq role_arn - expect(role.role_external_id).to eq role_external_id + expect(role.reload.role_arn).to eq(role_arn) end - context 'role cannot be created' do + context 'supplied role is invalid' do let(:role_arn) { 'invalid-role' } - it 'does not create a record' do - expect { go }.not_to change { Aws::Role.count } + it 'does not update the associated role' do + expect { go }.not_to change { role.role_arn } expect(response).to have_gitlab_http_status(:unprocessable_entity) end end describe 'security' do + before do + allow_next_instance_of(Clusters::Aws::AuthorizeRoleService) do |service| + response = double(status: :ok, body: double) + + allow(service).to receive(:execute).and_return(response) + end + end + it 'is allowed for admin when admin mode enabled', :enable_admin_mode do expect { go }.to be_allowed_for(:admin) end diff --git a/spec/controllers/projects/hooks_controller_spec.rb b/spec/controllers/projects/hooks_controller_spec.rb index 440e6b2a74c..68e91fa9c1f 100644 --- a/spec/controllers/projects/hooks_controller_spec.rb +++ b/spec/controllers/projects/hooks_controller_spec.rb @@ -46,4 +46,26 @@ RSpec.describe Projects::HooksController do expect(ProjectHook.first).to have_attributes(hook_params) end end + + describe '#test' do + let(:hook) { create(:project_hook, project: project) } + + context 'when the endpoint receives requests above the limit' do + before do + allow(Gitlab::ApplicationRateLimiter).to receive(:rate_limits) + .and_return(project_testing_hook: { threshold: 1, interval: 1.minute }) + end + + it 'prevents making test requests' do + expect_next_instance_of(TestHooks::ProjectService) do |service| + expect(service).to receive(:execute).and_return(http_status: 200) + end + + 2.times { post :test, params: { namespace_id: project.namespace, project_id: project, id: hook } } + + expect(response.body).to eq(_('This endpoint has been requested too many times. Try again later.')) + expect(response).to have_gitlab_http_status(:too_many_requests) + end + end + end end diff --git a/spec/controllers/sessions_controller_spec.rb b/spec/controllers/sessions_controller_spec.rb index 16a58112479..f45ea58b1f7 100644 --- a/spec/controllers/sessions_controller_spec.rb +++ b/spec/controllers/sessions_controller_spec.rb @@ -288,8 +288,8 @@ RSpec.describe SessionsController do context 'when using two-factor authentication via OTP' do let(:user) { create(:user, :two_factor) } - def authenticate_2fa(user_params) - post(:create, params: { user: user_params }, session: { otp_user_id: user.id }) + def authenticate_2fa(user_params, otp_user_id: user.id) + post(:create, params: { user: user_params }, session: { otp_user_id: otp_user_id }) end context 'remember_me field' do @@ -326,8 +326,22 @@ RSpec.describe SessionsController do end end + # See issue gitlab-org/gitlab#20302. + context 'when otp_user_id is stale' do + render_views + + it 'favors login over otp_user_id when password is present and does not authenticate the user' do + authenticate_2fa( + { login: 'random_username', password: user.password }, + otp_user_id: user.id + ) + + expect(response).to set_flash.now[:alert].to /Invalid Login or password/ + end + end + ## - # See #14900 issue + # See issue gitlab-org/gitlab-foss#14900 # context 'when authenticating with login and OTP of another user' do context 'when another user has 2FA enabled' do @@ -413,18 +427,6 @@ RSpec.describe SessionsController do end end end - - context 'when another user does not have 2FA enabled' do - let(:another_user) { create(:user) } - - it 'does not leak that 2FA is disabled for another user' do - authenticate_2fa(login: another_user.username, - otp_attempt: 'invalid') - - expect(response).to set_flash.now[:alert] - .to /Invalid two-factor code/ - end - end end end diff --git a/spec/features/admin/admin_manage_applications_spec.rb b/spec/features/admin/admin_manage_applications_spec.rb index 3f3d71e842c..7a9a6f2ccb8 100644 --- a/spec/features/admin/admin_manage_applications_spec.rb +++ b/spec/features/admin/admin_manage_applications_spec.rb @@ -7,7 +7,7 @@ RSpec.describe 'admin manage applications' do sign_in(create(:admin)) end - it do + it 'creates new oauth application' do visit admin_applications_path click_on 'New application' @@ -16,6 +16,7 @@ RSpec.describe 'admin manage applications' do fill_in :doorkeeper_application_name, with: 'test' fill_in :doorkeeper_application_redirect_uri, with: 'https://test.com' check :doorkeeper_application_trusted + check :doorkeeper_application_scopes_read_user click_on 'Submit' expect(page).to have_content('Application: test') expect(page).to have_content('Application ID') @@ -43,4 +44,19 @@ RSpec.describe 'admin manage applications' do end expect(page.find('.oauth-applications')).not_to have_content('test_changed') end + + context 'when scopes are blank' do + it 'returns an error' do + visit admin_applications_path + + click_on 'New application' + expect(page).to have_content('New application') + + fill_in :doorkeeper_application_name, with: 'test' + fill_in :doorkeeper_application_redirect_uri, with: 'https://test.com' + click_on 'Submit' + + expect(page).to have_content("Scopes can't be blank") + end + end end diff --git a/spec/features/profiles/user_manages_applications_spec.rb b/spec/features/profiles/user_manages_applications_spec.rb index d65365db880..22eed748c00 100644 --- a/spec/features/profiles/user_manages_applications_spec.rb +++ b/spec/features/profiles/user_manages_applications_spec.rb @@ -15,6 +15,7 @@ RSpec.describe 'User manages applications' do fill_in :doorkeeper_application_name, with: 'test' fill_in :doorkeeper_application_redirect_uri, with: 'https://test.com' + check :doorkeeper_application_scopes_read_user click_on 'Save application' expect(page).to have_content 'Application: test' @@ -41,4 +42,16 @@ RSpec.describe 'User manages applications' do end expect(page.find('.oauth-applications')).not_to have_content 'test_changed' end + + context 'when scopes are blank' do + it 'returns an error' do + expect(page).to have_content 'Add new application' + + fill_in :doorkeeper_application_name, with: 'test' + fill_in :doorkeeper_application_redirect_uri, with: 'https://test.com' + click_on 'Save application' + + expect(page).to have_content("Scopes can't be blank") + end + end end diff --git a/spec/features/users/login_spec.rb b/spec/features/users/login_spec.rb index 7ba663d08d4..c791fa5c0a5 100644 --- a/spec/features/users/login_spec.rb +++ b/spec/features/users/login_spec.rb @@ -177,6 +177,14 @@ RSpec.describe 'Login' do expect(page).not_to have_content(I18n.t('devise.failure.already_authenticated')) end + it 'does not allow sign-in if the user password is updated before entering a one-time code' do + user.update!(password: 'new_password') + + enter_code(user.current_otp) + + expect(page).to have_content('An error occurred. Please sign in again.') + end + context 'using one-time code' do it 'allows login with valid code' do expect(authentication_metrics) @@ -232,7 +240,7 @@ RSpec.describe 'Login' do expect(codes.size).to eq 10 # Ensure the generated codes get saved - user.save + user.save(touch: false) end context 'with valid code' do @@ -290,7 +298,7 @@ RSpec.describe 'Login' do code = codes.sample expect(user.invalidate_otp_backup_code!(code)).to eq true - user.save! + user.save!(touch: false) expect(user.reload.otp_backup_codes.size).to eq 9 enter_code(code) diff --git a/spec/finders/ci/auth_job_finder_spec.rb b/spec/finders/ci/auth_job_finder_spec.rb new file mode 100644 index 00000000000..6cd58f5cd01 --- /dev/null +++ b/spec/finders/ci/auth_job_finder_spec.rb @@ -0,0 +1,64 @@ +# frozen_string_literal: true +require 'spec_helper' + +RSpec.describe Ci::AuthJobFinder do + let_it_be(:job, reload: true) { create(:ci_build, status: :running) } + + let(:token) { job.token } + + subject(:finder) do + described_class.new(token: token) + end + + describe '#execute!' do + subject(:execute) { finder.execute! } + + it { is_expected.to eq(job) } + + it 'raises error if the job is not running' do + job.success! + + expect { execute }.to raise_error described_class::NotRunningJobError, 'Job is not running' + end + + it 'raises error if the job is erased' do + expect(::Ci::Build).to receive(:find_by_token).with(job.token).and_return(job) + expect(job).to receive(:erased?).and_return(true) + + expect { execute }.to raise_error described_class::ErasedJobError, 'Job has been erased!' + end + + it 'raises error if the the project is missing' do + expect(::Ci::Build).to receive(:find_by_token).with(job.token).and_return(job) + expect(job).to receive(:project).and_return(nil) + + expect { execute }.to raise_error described_class::DeletedProjectError, 'Project has been deleted!' + end + + it 'raises error if the the project is being removed' do + project = double(Project) + + expect(::Ci::Build).to receive(:find_by_token).with(job.token).and_return(job) + expect(job).to receive(:project).twice.and_return(project) + expect(project).to receive(:pending_delete?).and_return(true) + + expect { execute }.to raise_error described_class::DeletedProjectError, 'Project has been deleted!' + end + + context 'with wrong job token' do + let(:token) { 'missing' } + + it { is_expected.to be_nil } + end + end + + describe '#execute' do + subject(:execute) { finder.execute } + + before do + job.success! + end + + it { is_expected.to be_nil } + end +end diff --git a/spec/finders/user_recent_events_finder_spec.rb b/spec/finders/user_recent_events_finder_spec.rb index 04ba05c68e4..c32a0b06b0f 100644 --- a/spec/finders/user_recent_events_finder_spec.rb +++ b/spec/finders/user_recent_events_finder_spec.rb @@ -11,8 +11,10 @@ RSpec.describe UserRecentEventsFinder do let!(:private_event) { create(:event, project: private_project, author: project_owner) } let!(:internal_event) { create(:event, project: internal_project, author: project_owner) } let!(:public_event) { create(:event, project: public_project, author: project_owner) } + let(:limit) { nil } + let(:params) { { limit: limit } } - subject(:finder) { described_class.new(current_user, project_owner) } + subject(:finder) { described_class.new(current_user, project_owner, params) } describe '#execute' do context 'when profile is public' do @@ -36,5 +38,38 @@ RSpec.describe UserRecentEventsFinder do expect(finder.execute).to be_empty end + + context 'limits' do + before do + stub_const("#{described_class}::DEFAULT_LIMIT", 1) + stub_const("#{described_class}::MAX_LIMIT", 3) + end + + context 'when limit is not set' do + it 'returns events limited to DEFAULT_LIMIT' do + expect(finder.execute.size).to eq(described_class::DEFAULT_LIMIT) + end + end + + context 'when limit is set' do + let(:limit) { 2 } + + it 'returns events limited to specified limit' do + expect(finder.execute.size).to eq(limit) + end + end + + context 'when limit is set to a number that exceeds maximum limit' do + let(:limit) { 4 } + + before do + create(:event, project: public_project, author: project_owner) + end + + it 'returns events limited to MAX_LIMIT' do + expect(finder.execute.size).to eq(described_class::MAX_LIMIT) + end + end + end end end diff --git a/spec/graphql/gitlab_schema_spec.rb b/spec/graphql/gitlab_schema_spec.rb index 5d6aa863994..e52f60a2c02 100644 --- a/spec/graphql/gitlab_schema_spec.rb +++ b/spec/graphql/gitlab_schema_spec.rb @@ -212,6 +212,55 @@ RSpec.describe GitlabSchema do end end + describe '.parse_gid' do + let_it_be(:global_id) { 'gid://gitlab/TestOne/2147483647' } + + before do + test_base = Class.new + test_one = Class.new(test_base) + test_two = Class.new(test_base) + + stub_const('TestBase', test_base) + stub_const('TestOne', test_one) + stub_const('TestTwo', test_two) + end + + it 'parses the gid' do + gid = described_class.parse_gid(global_id) + + expect(gid.model_id).to eq '2147483647' + expect(gid.model_class).to eq TestOne + end + + context 'when gid is malformed' do + let_it_be(:global_id) { 'malformed://gitlab/TestOne/2147483647' } + + it 'raises an error' do + expect { described_class.parse_gid(global_id) } + .to raise_error(Gitlab::Graphql::Errors::ArgumentError, "#{global_id} is not a valid GitLab id.") + end + end + + context 'when using expected_type' do + it 'accepts a single type' do + gid = described_class.parse_gid(global_id, expected_type: TestOne) + + expect(gid.model_class).to eq TestOne + end + + it 'accepts an ancestor type' do + gid = described_class.parse_gid(global_id, expected_type: TestBase) + + expect(gid.model_class).to eq TestOne + end + + it 'rejects an unknown type' do + expect { described_class.parse_gid(global_id, expected_type: TestTwo) } + .to raise_error(Gitlab::Graphql::Errors::ArgumentError, "#{global_id} is not a valid id for TestTwo.") + end + end + end + def field_instrumenters described_class.instrumenters[:field] + described_class.instrumenters[:field_after_built_ins] end diff --git a/spec/lib/gitlab/auth/auth_finders_spec.rb b/spec/lib/gitlab/auth/auth_finders_spec.rb index 2aef206c7fd..4b6c928edcc 100644 --- a/spec/lib/gitlab/auth/auth_finders_spec.rb +++ b/spec/lib/gitlab/auth/auth_finders_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -describe Gitlab::Auth::AuthFinders do +RSpec.describe Gitlab::Auth::AuthFinders do include described_class include HttpBasicAuthHelpers @@ -499,7 +499,7 @@ describe Gitlab::Auth::AuthFinders do context 'with CI username' do let(:username) { ::Ci::Build::CI_REGISTRY_USER } let(:user) { create(:user) } - let(:build) { create(:ci_build, user: user) } + let(:build) { create(:ci_build, user: user, status: :running) } it 'returns nil without password' do set_basic_auth_header(username, nil) @@ -518,6 +518,13 @@ describe Gitlab::Auth::AuthFinders do expect { subject }.to raise_error(Gitlab::Auth::UnauthorizedError) end + + it 'returns exception if the job is not running' do + set_basic_auth_header(username, build.token) + build.success! + + expect { subject }.to raise_error(Gitlab::Auth::UnauthorizedError) + end end end @@ -567,7 +574,7 @@ describe Gitlab::Auth::AuthFinders do end describe '#find_user_from_job_token' do - let(:job) { create(:ci_build, user: user) } + let(:job) { create(:ci_build, user: user, status: :running) } let(:route_authentication_setting) { { job_token_allowed: true } } subject { find_user_from_job_token } @@ -592,6 +599,13 @@ describe Gitlab::Auth::AuthFinders do expect { subject }.to raise_error(Gitlab::Auth::UnauthorizedError) end + it 'returns exception if the job is not running' do + set_header(described_class::JOB_TOKEN_HEADER, job.token) + job.success! + + expect { subject }.to raise_error(Gitlab::Auth::UnauthorizedError) + end + context 'when route is not allowed to be authenticated' do let(:route_authentication_setting) { { job_token_allowed: false } } diff --git a/spec/lib/gitlab/auth/request_authenticator_spec.rb b/spec/lib/gitlab/auth/request_authenticator_spec.rb index 87c96803c3a..0e6a6e836fd 100644 --- a/spec/lib/gitlab/auth/request_authenticator_spec.rb +++ b/spec/lib/gitlab/auth/request_authenticator_spec.rb @@ -87,7 +87,7 @@ describe Gitlab::Auth::RequestAuthenticator do describe '#find_user_from_job_token' do let!(:user) { build(:user) } - let!(:job) { build(:ci_build, user: user) } + let!(:job) { build(:ci_build, user: user, status: :running) } before do env[Gitlab::Auth::AuthFinders::JOB_TOKEN_HEADER] = 'token' @@ -96,13 +96,18 @@ describe Gitlab::Auth::RequestAuthenticator do context 'with API requests' do before do env['SCRIPT_NAME'] = '/api/endpoint' + expect(::Ci::Build).to receive(:find_by_token).with('token').and_return(job) end it 'tries to find the user' do - expect(::Ci::Build).to receive(:find_by_token).and_return(job) - expect(subject.find_sessionless_user([:api])).to eq user end + + it 'returns nil if the job is not running' do + job.status = :success + + expect(subject.find_sessionless_user([:api])).to be_blank + end end context 'without API requests' do diff --git a/spec/lib/gitlab/auth/two_factor_auth_verifier_spec.rb b/spec/lib/gitlab/auth/two_factor_auth_verifier_spec.rb new file mode 100644 index 00000000000..f906870195a --- /dev/null +++ b/spec/lib/gitlab/auth/two_factor_auth_verifier_spec.rb @@ -0,0 +1,112 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Gitlab::Auth::TwoFactorAuthVerifier do + let(:user) { create(:user) } + + subject { described_class.new(user) } + + describe '#two_factor_authentication_required?' do + describe 'when it is required on application level' do + it 'returns true' do + stub_application_setting require_two_factor_authentication: true + + expect(subject.two_factor_authentication_required?).to be_truthy + end + end + + describe 'when it is required on group level' do + it 'returns true' do + allow(user).to receive(:require_two_factor_authentication_from_group?).and_return(true) + + expect(subject.two_factor_authentication_required?).to be_truthy + end + end + + describe 'when it is not required' do + it 'returns false when not required on group level' do + allow(user).to receive(:require_two_factor_authentication_from_group?).and_return(false) + + expect(subject.two_factor_authentication_required?).to be_falsey + end + end + end + + describe '#current_user_needs_to_setup_two_factor?' do + it 'returns false when current_user is nil' do + expect(described_class.new(nil).current_user_needs_to_setup_two_factor?).to be_falsey + end + + it 'returns false when current_user does not have temp email' do + allow(user).to receive(:two_factor_enabled?).and_return(false) + allow(user).to receive(:temp_oauth_email?).and_return(true) + + expect(subject.current_user_needs_to_setup_two_factor?).to be_falsey + end + + it 'returns false when current_user has 2fa disabled' do + allow(user).to receive(:temp_oauth_email?).and_return(false) + allow(user).to receive(:two_factor_enabled?).and_return(true) + + expect(subject.current_user_needs_to_setup_two_factor?).to be_falsey + end + + it 'returns true when user requires 2fa authentication' do + allow(user).to receive(:two_factor_enabled?).and_return(false) + allow(user).to receive(:temp_oauth_email?).and_return(false) + + expect(subject.current_user_needs_to_setup_two_factor?).to be_truthy + end + end + + describe '#two_factor_grace_period' do + it 'returns grace period from settings if there is no period from groups' do + stub_application_setting two_factor_grace_period: 2 + allow(user).to receive(:require_two_factor_authentication_from_group?).and_return(false) + + expect(subject.two_factor_grace_period).to eq(2) + end + + it 'returns grace period from groups if there is no period from settings' do + allow(user).to receive(:require_two_factor_authentication_from_group?).and_return(true) + allow(user).to receive(:two_factor_grace_period).and_return(3) + + expect(subject.two_factor_grace_period).to eq(3) + end + + it 'returns minimal grace period if there is grace period from settings and from group' do + allow(user).to receive(:require_two_factor_authentication_from_group?).and_return(true) + allow(user).to receive(:two_factor_grace_period).and_return(3) + stub_application_setting two_factor_grace_period: 2 + + expect(subject.two_factor_grace_period).to eq(2) + end + end + + describe '#two_factor_grace_period_expired?' do + before do + allow(user).to receive(:otp_grace_period_started_at).and_return(4.hours.ago) + end + + it 'returns true if the grace period has expired' do + allow(subject).to receive(:two_factor_grace_period).and_return(2) + + expect(subject.two_factor_grace_period_expired?).to be_truthy + end + + it 'returns false if the grace period has not expired' do + allow(subject).to receive(:two_factor_grace_period).and_return(6) + + expect(subject.two_factor_grace_period_expired?).to be_falsey + end + + context 'when otp_grace_period_started_at is nil' do + it 'returns false' do + allow(user).to receive(:otp_grace_period_started_at).and_return(nil) + + expect(subject.two_factor_grace_period_expired?).to be_falsey + end + end + end +end diff --git a/spec/lib/gitlab/auth_spec.rb b/spec/lib/gitlab/auth_spec.rb index 21767af9abf..19708333731 100644 --- a/spec/lib/gitlab/auth_spec.rb +++ b/spec/lib/gitlab/auth_spec.rb @@ -431,7 +431,7 @@ describe Gitlab::Auth, :use_clean_rails_memory_store_caching do end end - shared_examples 'deploy token with disabled registry' do + shared_examples 'deploy token with disabled feature' do context 'when registry disabled' do before do stub_container_registry_config(enabled: false) @@ -442,6 +442,15 @@ describe Gitlab::Auth, :use_clean_rails_memory_store_caching do .to eq(auth_failure) end end + + context 'when repository is disabled' do + let(:project) { create(:project, :repository_disabled) } + + it 'fails when login and token are valid' do + expect(gl_auth.find_for_git_client(login, deploy_token.token, project: project, ip: 'ip')) + .to eq(auth_failure) + end + end end context 'when deploy token and user have the same username' do @@ -594,7 +603,7 @@ describe Gitlab::Auth, :use_clean_rails_memory_store_caching do it_behaves_like 'registry token scope' end - it_behaves_like 'deploy token with disabled registry' + it_behaves_like 'deploy token with disabled feature' end context 'when the deploy token has write_registry as a scope' do @@ -616,7 +625,7 @@ describe Gitlab::Auth, :use_clean_rails_memory_store_caching do it_behaves_like 'registry token scope' end - it_behaves_like 'deploy token with disabled registry' + it_behaves_like 'deploy token with disabled feature' end end end diff --git a/spec/lib/gitlab/git_access_spec.rb b/spec/lib/gitlab/git_access_spec.rb index 7c09fc5cc79..0a6db18ad76 100644 --- a/spec/lib/gitlab/git_access_spec.rb +++ b/spec/lib/gitlab/git_access_spec.rb @@ -487,31 +487,135 @@ describe Gitlab::GitAccess do let(:actor) { key } context 'pull code' do - context 'when project is authorized' do - before do - key.projects << project + context 'when project is public' do + let(:project) { create(:project, :public, :repository, *options) } + + context 'when deploy key exists in the project' do + before do + key.projects << project + end + + context 'when the repository is public' do + let(:options) { %i[repository_enabled] } + + it { expect { pull_access_check }.not_to raise_error } + end + + context 'when the repository is private' do + let(:options) { %i[repository_private] } + + it { expect { pull_access_check }.not_to raise_error } + end + + context 'when the repository is disabled' do + let(:options) { %i[repository_disabled] } + + it { expect { pull_access_check }.to raise_error('You are not allowed to download code from this project.') } + end end - it { expect { pull_access_check }.not_to raise_error } + context 'when deploy key does not exist in the project' do + context 'when the repository is public' do + let(:options) { %i[repository_enabled] } + + it { expect { pull_access_check }.not_to raise_error } + end + + context 'when the repository is private' do + let(:options) { %i[repository_private] } + + it { expect { pull_access_check }.to raise_error('You are not allowed to download code from this project.') } + end + + context 'when the repository is disabled' do + let(:options) { %i[repository_disabled] } + + it { expect { pull_access_check }.to raise_error('You are not allowed to download code from this project.') } + end + end end - context 'when unauthorized' do - context 'from public project' do - let(:project) { create(:project, :public, :repository) } + context 'when project is internal' do + let(:project) { create(:project, :internal, :repository, *options) } - it { expect { pull_access_check }.not_to raise_error } + context 'when deploy key exists in the project' do + before do + key.projects << project + end + + context 'when the repository is public' do + let(:options) { %i[repository_enabled] } + + it { expect { pull_access_check }.not_to raise_error } + end + + context 'when the repository is private' do + let(:options) { %i[repository_private] } + + it { expect { pull_access_check }.not_to raise_error } + end + + context 'when the repository is disabled' do + let(:options) { %i[repository_disabled] } + + it { expect { pull_access_check }.to raise_error('You are not allowed to download code from this project.') } + end end - context 'from internal project' do - let(:project) { create(:project, :internal, :repository) } + context 'when deploy key does not exist in the project' do + context 'when the repository is public' do + let(:options) { %i[repository_enabled] } - it { expect { pull_access_check }.to raise_not_found } + it { expect { pull_access_check }.to raise_error('The project you were looking for could not be found.') } + end + + context 'when the repository is private' do + let(:options) { %i[repository_private] } + + it { expect { pull_access_check }.to raise_error('The project you were looking for could not be found.') } + end + + context 'when the repository is disabled' do + let(:options) { %i[repository_disabled] } + + it { expect { pull_access_check }.to raise_error('The project you were looking for could not be found.') } + end end + end - context 'from private project' do - let(:project) { create(:project, :private, :repository) } + context 'when project is private' do + let(:project) { create(:project, :private, :repository, *options) } - it { expect { pull_access_check }.to raise_not_found } + context 'when deploy key exists in the project' do + before do + key.projects << project + end + + context 'when the repository is private' do + let(:options) { %i[repository_private] } + + it { expect { pull_access_check }.not_to raise_error } + end + + context 'when the repository is disabled' do + let(:options) { %i[repository_disabled] } + + it { expect { pull_access_check }.to raise_error('You are not allowed to download code from this project.') } + end + end + + context 'when deploy key does not exist in the project' do + context 'when the repository is private' do + let(:options) { %i[repository_private] } + + it { expect { pull_access_check }.to raise_error('The project you were looking for could not be found.') } + end + + context 'when the repository is disabled' do + let(:options) { %i[repository_disabled] } + + it { expect { pull_access_check }.to raise_error('The project you were looking for could not be found.') } + end end end end diff --git a/spec/lib/gitlab/regex_spec.rb b/spec/lib/gitlab/regex_spec.rb index 2f220272651..340ede6e36c 100644 --- a/spec/lib/gitlab/regex_spec.rb +++ b/spec/lib/gitlab/regex_spec.rb @@ -164,15 +164,6 @@ describe Gitlab::Regex do it { is_expected.not_to match('foo/bar') } end - describe '.conan_file_name_regex' do - subject { described_class.conan_file_name_regex } - - it { is_expected.to match('conanfile.py') } - it { is_expected.to match('conan_package.tgz') } - it { is_expected.not_to match('foo.txt') } - it { is_expected.not_to match('!!()()') } - end - describe '.conan_package_reference_regex' do subject { described_class.conan_package_reference_regex } diff --git a/spec/models/active_session_spec.rb b/spec/models/active_session_spec.rb index 24b47be3c69..de39c8c7c5c 100644 --- a/spec/models/active_session_spec.rb +++ b/spec/models/active_session_spec.rb @@ -296,6 +296,59 @@ RSpec.describe ActiveSession, :clean_gitlab_redis_shared_state do end end + describe '.destroy_all_but_current' do + it 'gracefully handles a nil session ID' do + expect(described_class).not_to receive(:destroy_sessions) + + ActiveSession.destroy_all_but_current(user, nil) + end + + context 'with user sessions' do + let(:current_session_id) { '6919a6f1bb119dd7396fadc38fd18d0d' } + + before do + Gitlab::Redis::SharedState.with do |redis| + redis.set(described_class.key_name(user.id, current_session_id), + Marshal.dump(ActiveSession.new(session_id: Rack::Session::SessionId.new(current_session_id)))) + redis.set(described_class.key_name(user.id, '59822c7d9fcdfa03725eff41782ad97d'), + Marshal.dump(ActiveSession.new(session_id: Rack::Session::SessionId.new('59822c7d9fcdfa03725eff41782ad97d')))) + redis.set(described_class.key_name(9999, '5c8611e4f9c69645ad1a1492f4131358'), + Marshal.dump(ActiveSession.new(session_id: Rack::Session::SessionId.new('5c8611e4f9c69645ad1a1492f4131358')))) + redis.sadd(described_class.lookup_key_name(user.id), '59822c7d9fcdfa03725eff41782ad97d') + redis.sadd(described_class.lookup_key_name(user.id), current_session_id) + redis.sadd(described_class.lookup_key_name(9999), '5c8611e4f9c69645ad1a1492f4131358') + end + end + + it 'removes the entry associated with the all user sessions but current' do + expect { ActiveSession.destroy_all_but_current(user, request.session) }.to change { ActiveSession.session_ids_for_user(user.id).size }.from(2).to(1) + + expect(ActiveSession.session_ids_for_user(9999).size).to eq(1) + end + + it 'removes the lookup entry of deleted sessions' do + ActiveSession.destroy_all_but_current(user, request.session) + + Gitlab::Redis::SharedState.with do |redis| + expect(redis.smembers(described_class.lookup_key_name(user.id))).to eq [current_session_id] + end + end + + it 'does not remove impersonated sessions' do + impersonated_session_id = '6919a6f1bb119dd7396fadc38fd18eee' + Gitlab::Redis::SharedState.with do |redis| + redis.set(described_class.key_name(user.id, impersonated_session_id), + Marshal.dump(ActiveSession.new(session_id: Rack::Session::SessionId.new(impersonated_session_id), is_impersonated: true))) + redis.sadd(described_class.lookup_key_name(user.id), impersonated_session_id) + end + + expect { ActiveSession.destroy_all_but_current(user, request.session) }.to change { ActiveSession.session_ids_for_user(user.id).size }.from(3).to(2) + + expect(ActiveSession.session_ids_for_user(9999).size).to eq(1) + end + end + end + describe '.cleanup' do before do stub_const("ActiveSession::ALLOWED_NUMBER_OF_ACTIVE_SESSIONS", 5) diff --git a/spec/models/aws/role_spec.rb b/spec/models/aws/role_spec.rb index d4165567146..bb1a22f5060 100644 --- a/spec/models/aws/role_spec.rb +++ b/spec/models/aws/role_spec.rb @@ -29,6 +29,12 @@ describe Aws::Role do it { is_expected.to be_truthy } end + + context 'ARN is nil' do + let(:role_arn) { } + + it { is_expected.to be_truthy } + end end end diff --git a/spec/models/member_spec.rb b/spec/models/member_spec.rb index 7c40bb24b56..2879b325663 100644 --- a/spec/models/member_spec.rb +++ b/spec/models/member_spec.rb @@ -91,9 +91,10 @@ describe Member do end describe 'Scopes & finders' do + let(:project) { create(:project, :public) } + let(:group) { create(:group) } + before do - project = create(:project, :public) - group = create(:group) @owner_user = create(:user).tap { |u| group.add_owner(u) } @owner = group.members.find_by(user_id: @owner_user.id) @@ -172,6 +173,19 @@ describe Member do it { expect(described_class.non_request).to include @accepted_request_member } end + describe '.not_accepted_invitations_by_user' do + let(:invited_by_user) { create(:project_member, :invited, project: project, created_by: @owner_user) } + + before do + create(:project_member, :invited, invite_email: 'test@test.com', project: project, created_by: @owner_user, invite_accepted_at: Time.zone.now) + create(:project_member, :invited, invite_email: 'test2@test.com', project: project, created_by: @maintainer_user) + end + + subject { described_class.not_accepted_invitations_by_user(@owner_user) } + + it { is_expected.to contain_exactly(invited_by_user) } + end + describe '.search_invite_email' do it 'returns only members the matching e-mail' do create(:group_member, :invited) diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index dd4b174a38f..40f3bd20382 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -893,6 +893,20 @@ describe User do expect(described_class.without_ghosts).to match_array([user1, user2]) end end + + describe '.by_id_and_login' do + let_it_be(:user) { create(:user) } + + it 'finds a user regardless of case' do + expect(described_class.by_id_and_login(user.id, user.username.upcase)) + .to contain_exactly(user) + end + + it 'finds a user when login is an email address regardless of case' do + expect(described_class.by_id_and_login(user.id, user.email.upcase)) + .to contain_exactly(user) + end + end end describe "Respond to" do @@ -3578,6 +3592,42 @@ describe User do end end + describe '#source_groups_of_two_factor_authentication_requirement' do + let_it_be(:group_not_requiring_2FA) { create :group } + let(:user) { create :user } + + before do + group.add_user(user, GroupMember::OWNER) + group_not_requiring_2FA.add_user(user, GroupMember::OWNER) + end + + context 'when user is direct member of group requiring 2FA' do + let_it_be(:group) { create :group, require_two_factor_authentication: true } + + it 'returns group requiring 2FA' do + expect(user.source_groups_of_two_factor_authentication_requirement).to contain_exactly(group) + end + end + + context 'when user is member of group which parent requires 2FA' do + let_it_be(:parent_group) { create :group, require_two_factor_authentication: true } + let_it_be(:group) { create :group, parent: parent_group } + + it 'returns group requiring 2FA' do + expect(user.source_groups_of_two_factor_authentication_requirement).to contain_exactly(group) + end + end + + context 'when user is member of group which child requires 2FA' do + let_it_be(:group) { create :group } + let_it_be(:child_group) { create :group, require_two_factor_authentication: true, parent: group } + + it 'returns group requiring 2FA' do + expect(user.source_groups_of_two_factor_authentication_requirement).to contain_exactly(group) + end + end + end + describe '.active' do before do described_class.ghost diff --git a/spec/requests/api/badges_spec.rb b/spec/requests/api/badges_spec.rb index d7f9b7d010b..8a7b429d7f2 100644 --- a/spec/requests/api/badges_spec.rb +++ b/spec/requests/api/badges_spec.rb @@ -332,10 +332,32 @@ describe API::Badges do context 'when deleting a badge' do context 'and the source is a project' do + let(:badge) { project.group.badges.first } + it 'cannot delete badges owned by the project group' do - delete api("/projects/#{project.id}/badges/#{project_group.badges.first.id}", maintainer) + expect do + delete api("/projects/#{project.id}/badges/#{badge.id}", maintainer) + + expect(response).to have_gitlab_http_status(:not_found) + end.not_to change { badge.reload.persisted? } + end + end + end + + context 'when updating a badge' do + context 'and the source is a project' do + let(:badge) { project.group.badges.first } + let(:example_name) { 'BadgeName' } + let(:example_url) { 'http://www.example.com' } + let(:example_url2) { 'http://www.example1.com' } + + it 'cannot update badges owned by the project group' do + expect do + put api("/projects/#{project.id}/badges/#{badge.id}", maintainer), + params: { name: example_name, link_url: example_url, image_url: example_url2 } - expect(response).to have_gitlab_http_status(:forbidden) + expect(response).to have_gitlab_http_status(:not_found) + end.not_to change { badge.reload.updated_at } end end end diff --git a/spec/requests/api/graphql/mutations/snippets/destroy_spec.rb b/spec/requests/api/graphql/mutations/snippets/destroy_spec.rb index cb9aeea74b2..0c0e52fde53 100644 --- a/spec/requests/api/graphql/mutations/snippets/destroy_spec.rb +++ b/spec/requests/api/graphql/mutations/snippets/destroy_spec.rb @@ -46,6 +46,31 @@ describe 'Destroying a Snippet' do expect(mutation_response).to have_key('snippet') expect(mutation_response['snippet']).to be_nil end + + context 'when a bad gid is given' do + let!(:project) { create(:project, :private) } + let!(:snippet) { create(:project_snippet, :private, project: project, author: create(:user)) } + let!(:snippet_gid) { project.to_gid.to_s } + + it 'returns an error' do + post_graphql_mutation(mutation, current_user: current_user) + + expect(graphql_errors) + .to include(a_hash_including('message' => "#{snippet_gid} is not a valid id for Snippet.")) + end + + it 'does not destroy the Snippet' do + expect do + post_graphql_mutation(mutation, current_user: current_user) + end.not_to change { Snippet.count } + end + + it 'does not destroy the Project' do + expect do + post_graphql_mutation(mutation, current_user: current_user) + end.not_to change { Project.count } + end + end end end diff --git a/spec/requests/api/helpers_spec.rb b/spec/requests/api/helpers_spec.rb index d65c89f48ea..872d7252d0f 100644 --- a/spec/requests/api/helpers_spec.rb +++ b/spec/requests/api/helpers_spec.rb @@ -84,6 +84,27 @@ describe API::Helpers do end it { is_expected.to eq(user) } + + context 'when user should have 2fa enabled' do + before do + allow(user).to receive(:require_two_factor_authentication_from_group?).and_return(true) + allow_next_instance_of(Gitlab::Auth::TwoFactorAuthVerifier) do |verifier| + allow(verifier).to receive(:two_factor_grace_period_expired?).and_return(true) + end + end + + context 'when 2fa is not enabled' do + it { is_expected.to be_nil } + end + + context 'when 2fa is enabled' do + before do + allow(user).to receive(:two_factor_enabled?).and_return(true) + end + + it { is_expected.to eq(user) } + end + end end context "PUT request" do diff --git a/spec/requests/api/releases_spec.rb b/spec/requests/api/releases_spec.rb index f4cb7f25990..4b7013a56b7 100644 --- a/spec/requests/api/releases_spec.rb +++ b/spec/requests/api/releases_spec.rb @@ -653,12 +653,20 @@ describe API::Releases do end context 'when a valid token is provided' do - it 'creates the release' do + it 'creates the release for a running job' do + job.update!(status: :running) post api("/projects/#{project.id}/releases"), params: params.merge(job_token: job.token) expect(response).to have_gitlab_http_status(:created) expect(project.releases.last.description).to eq('Another nice release') end + + it 'returns an :unauthorized error for a completed job' do + job.success! + post api("/projects/#{project.id}/releases"), params: params.merge(job_token: job.token) + + expect(response).to have_gitlab_http_status(:unauthorized) + end end end diff --git a/spec/requests/api/terraform/state_spec.rb b/spec/requests/api/terraform/state_spec.rb index ec9db5566e3..b1fbf67b84b 100644 --- a/spec/requests/api/terraform/state_spec.rb +++ b/spec/requests/api/terraform/state_spec.rb @@ -71,7 +71,7 @@ describe API::Terraform::State do let(:auth_header) { job_basic_auth_header(job) } context 'with maintainer permissions' do - let(:job) { create(:ci_build, project: project, user: maintainer) } + let(:job) { create(:ci_build, status: :running, project: project, user: maintainer) } it 'returns terraform state belonging to a project of given state name' do request @@ -80,6 +80,13 @@ describe API::Terraform::State do expect(response.body).to eq(state.file.read) end + it 'returns unauthorized if the the job is not running' do + job.update!(status: :failed) + request + + expect(response).to have_gitlab_http_status(:unauthorized) + end + context 'for a project that does not exist' do let(:project_id) { '0000' } @@ -92,7 +99,7 @@ describe API::Terraform::State do end context 'with developer permissions' do - let(:job) { create(:ci_build, project: project, user: developer) } + let(:job) { create(:ci_build, status: :running, project: project, user: developer) } it 'returns forbidden if the user cannot access the state' do request diff --git a/spec/services/applications/create_service_spec.rb b/spec/services/applications/create_service_spec.rb index c8134087fa1..a34c5b10457 100644 --- a/spec/services/applications/create_service_spec.rb +++ b/spec/services/applications/create_service_spec.rb @@ -6,9 +6,24 @@ describe ::Applications::CreateService do include TestRequestHelpers let(:user) { create(:user) } - let(:params) { attributes_for(:application) } subject { described_class.new(user, params) } - it { expect { subject.execute(test_request) }.to change { Doorkeeper::Application.count }.by(1) } + context 'when scopes are present' do + let(:params) { attributes_for(:application, scopes: ['read_user']) } + + it { expect { subject.execute(test_request) }.to change { Doorkeeper::Application.count }.by(1) } + end + + context 'when scopes are missing' do + let(:params) { attributes_for(:application) } + + it { expect { subject.execute(test_request) }.not_to change { Doorkeeper::Application.count } } + + it 'includes blank scopes error message' do + application = subject.execute(test_request) + + expect(application.errors.full_messages).to include "Scopes can't be blank" + end + end end diff --git a/spec/services/auth/container_registry_authentication_service_spec.rb b/spec/services/auth/container_registry_authentication_service_spec.rb index 70eb35f0826..e0ea661d15d 100644 --- a/spec/services/auth/container_registry_authentication_service_spec.rb +++ b/spec/services/auth/container_registry_authentication_service_spec.rb @@ -654,6 +654,19 @@ describe Auth::ContainerRegistryAuthenticationService do it_behaves_like 'not a container repository factory' end end + + context 'for project that disables repository' do + let(:project) { create(:project, :public, :repository_disabled) } + + context 'disallow when pulling' do + let(:current_params) do + { scopes: ["repository:#{project.full_path}:pull"] } + end + + it_behaves_like 'an inaccessible' + it_behaves_like 'not a container repository factory' + end + end end context 'registry catalog browsing authorized as admin' do diff --git a/spec/services/ci/pipeline_trigger_service_spec.rb b/spec/services/ci/pipeline_trigger_service_spec.rb index 44ce1ff699b..4792c8eca8d 100644 --- a/spec/services/ci/pipeline_trigger_service_spec.rb +++ b/spec/services/ci/pipeline_trigger_service_spec.rb @@ -106,9 +106,23 @@ describe Ci::PipelineTriggerService do let(:params) { { token: job.token, ref: 'master', variables: nil } } let(:job) { create(:ci_build, :success, pipeline: pipeline, user: user) } - it 'does nothing' do + it 'does nothing', :aggregate_failures do + expect { result }.not_to change { Ci::Pipeline.count } + expect(result[:message]).to eq('Job is not running') + expect(result[:http_status]).to eq(401) + end + end + + context 'when job does not have a project' do + let(:params) { { token: job.token, ref: 'master', variables: nil } } + let(:job) { create(:ci_build, status: :running, pipeline: pipeline, user: user) } + + it 'does nothing', :aggregate_failures do + job.update!(project: nil) + expect { result }.not_to change { Ci::Pipeline.count } - expect(result[:message]).to eq('400 Job has to be running') + expect(result[:message]).to eq('Project has been deleted!') + expect(result[:http_status]).to eq(401) end end diff --git a/spec/services/clusters/aws/authorize_role_service_spec.rb b/spec/services/clusters/aws/authorize_role_service_spec.rb index 3ef332558a2..5033076e192 100644 --- a/spec/services/clusters/aws/authorize_role_service_spec.rb +++ b/spec/services/clusters/aws/authorize_role_service_spec.rb @@ -3,47 +3,34 @@ require 'spec_helper' describe Clusters::Aws::AuthorizeRoleService do - let(:user) { create(:user) } + subject { described_class.new(user, params: params).execute } + + let(:role) { create(:aws_role) } + let(:user) { role.user } let(:credentials) { instance_double(Aws::Credentials) } let(:credentials_service) { instance_double(Clusters::Aws::FetchCredentialsService, execute: credentials) } + let(:role_arn) { 'arn:my-role' } let(:params) do params = ActionController::Parameters.new({ cluster: { - role_arn: 'arn:my-role', - role_external_id: 'external-id' + role_arn: role_arn } }) - params.require(:cluster).permit(:role_arn, :role_external_id) + params.require(:cluster).permit(:role_arn) end - subject { described_class.new(user, params: params).execute } - before do allow(Clusters::Aws::FetchCredentialsService).to receive(:new) .with(instance_of(Aws::Role)).and_return(credentials_service) end - context 'role does not exist' do - it 'creates an Aws::Role record and returns a set of credentials' do - expect(user).to receive(:create_aws_role!) - .with(params).and_call_original - - expect(subject.status).to eq(:ok) - expect(subject.body).to eq(credentials) - end - end - - context 'role already exists' do - let(:role) { create(:aws_role, user: user) } - + context 'role exists' do it 'updates the existing Aws::Role record and returns a set of credentials' do - expect(role).to receive(:update!) - .with(params).and_call_original - expect(subject.status).to eq(:ok) expect(subject.body).to eq(credentials) + expect(role.reload.role_arn).to eq(role_arn) end end @@ -55,11 +42,14 @@ describe Clusters::Aws::AuthorizeRoleService do end end - context 'cannot create role' do - before do - allow(user).to receive(:create_aws_role!) - .and_raise(ActiveRecord::RecordInvalid.new(user)) - end + context 'role does not exist' do + let(:user) { create(:user) } + + include_examples 'bad request' + end + + context 'supplied ARN is invalid' do + let(:role_arn) { 'invalid' } include_examples 'bad request' end diff --git a/spec/services/members/destroy_service_spec.rb b/spec/services/members/destroy_service_spec.rb index 73ac0bd7716..99e60318c42 100644 --- a/spec/services/members/destroy_service_spec.rb +++ b/spec/services/members/destroy_service_spec.rb @@ -249,6 +249,10 @@ describe Members::DestroyService do before do create(:group_member, :developer, group: subsubgroup, user: member_user) + create(:project_member, :invited, project: group_project, created_by: member_user) + create(:group_member, :invited, group: group, created_by: member_user) + create(:project_member, :invited, project: subsubproject, created_by: member_user) + create(:group_member, :invited, group: subgroup, created_by: member_user) subsubproject.add_developer(member_user) control_project.add_maintainer(user) @@ -282,5 +286,41 @@ describe Members::DestroyService do it 'does not remove the user from the control project' do expect(control_project.members.map(&:user)).to include(user) end + + it 'removes group members invited by deleted user' do + expect(group.members.not_accepted_invitations_by_user(member_user)).to be_empty + end + + it 'removes project members invited by deleted user' do + expect(group_project.members.not_accepted_invitations_by_user(member_user)).to be_empty + end + + it 'removes subgroup members invited by deleted user' do + expect(subgroup.members.not_accepted_invitations_by_user(member_user)).to be_empty + end + + it 'removes subproject members invited by deleted user' do + expect(subsubproject.members.not_accepted_invitations_by_user(member_user)).to be_empty + end + end + + context 'deletion of invitations created by deleted project member' do + let(:user) { project.owner } + let(:member_user) { create(:user) } + let(:opts) { {} } + + let(:project) { create(:project) } + + before do + create(:project_member, :invited, project: project, created_by: member_user) + + project_member = create(:project_member, :maintainer, user: member_user, project: project) + + described_class.new(user).execute(project_member, opts) + end + + it 'removes project members invited by deleted user' do + expect(project.members.not_accepted_invitations_by_user(member_user)).to be_empty + end end end diff --git a/spec/services/projects/update_remote_mirror_service_spec.rb b/spec/services/projects/update_remote_mirror_service_spec.rb index 418973fb0a6..b817c4b2ec0 100644 --- a/spec/services/projects/update_remote_mirror_service_spec.rb +++ b/spec/services/projects/update_remote_mirror_service_spec.rb @@ -65,6 +65,40 @@ describe Projects::UpdateRemoteMirrorService do expect(remote_mirror.last_error).to include('Badly broken') end + context 'when the URL is blocked' do + before do + allow(Gitlab::UrlBlocker).to receive(:blocked_url?).and_return(true) + end + + it 'fails and returns error status' do + expect(execute!).to eq(status: :error, message: 'The remote mirror URL is invalid.') + end + end + + context "when given URLs containing escaped elements" do + using RSpec::Parameterized::TableSyntax + + where(:url, :result_status) do + "https://user:0a%23@test.example.com/project.git" | :success + "https://git.example.com:1%2F%2F@source.developers.google.com/project.git" | :success + CGI.escape("git://localhost:1234/some-path?some-query=some-val\#@example.com/") | :error + CGI.escape(CGI.escape("https://user:0a%23@test.example.com/project.git")) | :error + end + + with_them do + before do + allow(remote_mirror).to receive(:url).and_return(url) + allow(service).to receive(:update_mirror).with(remote_mirror).and_return(true) + end + + it "returns expected status" do + result = execute! + + expect(result[:status]).to eq(result_status) + end + end + end + context 'when the update fails because of a `Gitlab::Git::CommandError`' do before do allow(project.repository).to receive(:fetch_remote).and_raise(Gitlab::Git::CommandError.new('fetch failed')) diff --git a/spec/support/shared_examples/controllers/clusters_controller_shared_examples.rb b/spec/support/shared_examples/controllers/clusters_controller_shared_examples.rb new file mode 100644 index 00000000000..aa17e72d08e --- /dev/null +++ b/spec/support/shared_examples/controllers/clusters_controller_shared_examples.rb @@ -0,0 +1,29 @@ +# frozen_string_literal: true + +RSpec.shared_examples 'GET new cluster shared examples' do + describe 'EKS cluster' do + context 'user already has an associated AWS role' do + let!(:role) { create(:aws_role, user: user) } + + it 'does not create an Aws::Role record' do + expect { go(provider: 'aws') }.not_to change { Aws::Role.count } + + expect(response).to have_gitlab_http_status(:ok) + expect(assigns(:aws_role)).to eq(role) + end + end + + context 'user does not have an associated AWS role' do + it 'creates an Aws::Role record' do + expect { go(provider: 'aws') }.to change { Aws::Role.count } + + expect(response).to have_gitlab_http_status(:ok) + + role = assigns(:aws_role) + expect(role.user).to eq(user) + expect(role.role_arn).to be_nil + expect(role.role_external_id).to be_present + end + end + end +end diff --git a/yarn.lock b/yarn.lock index 5a6ab803b70..e3d90363b1c 100644 --- a/yarn.lock +++ b/yarn.lock @@ -7065,10 +7065,10 @@ jquery.waitforimages@^2.2.0: resolved "https://registry.yarnpkg.com/jquery.waitforimages/-/jquery.waitforimages-2.2.0.tgz#63f23131055a1b060dc913e6d874bcc9b9e6b16b" integrity sha1-Y/IxMQVaGwYNyRPm2HS8ybnmsWs= -"jquery@>= 1.9.1", jquery@>=1.8.0, jquery@^3.4.1: - version "3.4.1" - resolved "https://registry.yarnpkg.com/jquery/-/jquery-3.4.1.tgz#714f1f8d9dde4bdfa55764ba37ef214630d80ef2" - integrity sha512-36+AdBzCL+y6qjw5Tx7HgzeGCzC81MDDgaUP8ld2zhx58HdqXGoBd+tHdrBMiyjGQs0Hxs/MLZTu/eHNJJuWPw== +"jquery@>= 1.9.1", jquery@>=1.8.0, jquery@^3.5.0: + version "3.5.1" + resolved "https://registry.yarnpkg.com/jquery/-/jquery-3.5.1.tgz#d7b4d08e1bfdb86ad2f1a3d039ea17304717abb5" + integrity sha512-XwIBPqcMn57FxfT+Go5pzySnm4KWkT1Tv7gjrpT1srtf8Weynl6R273VJ5GjkRb51IzMp5nbaPjJXMWeju2MKg== js-base64@^2.1.8: version "2.5.1" |