diff options
| author | GitLab Bot <gitlab-bot@gitlab.com> | 2021-09-29 15:57:02 +0300 |
|---|---|---|
| committer | GitLab Bot <gitlab-bot@gitlab.com> | 2021-09-29 15:57:02 +0300 |
| commit | e0ab280b774e34fcfd6fd031616247714230ca68 (patch) | |
| tree | 472ee2dcef05f242e1b861caa47a0a5179e92f4c | |
| parent | 60b56b48afb89ed1890409b6c425f16549c4d28b (diff) | |
Add latest changes from gitlab-org/security/gitlab@14-3-stable-ee
42 files changed, 651 insertions, 252 deletions
diff --git a/app/assets/javascripts/gfm_auto_complete.js b/app/assets/javascripts/gfm_auto_complete.js index 470c785f7e4..cb63c86a4fa 100644 --- a/app/assets/javascripts/gfm_auto_complete.js +++ b/app/assets/javascripts/gfm_auto_complete.js @@ -1,6 +1,6 @@ import $ from 'jquery'; import '~/lib/utils/jquery_at_who'; -import { escape, sortBy, template } from 'lodash'; +import { escape as lodashEscape, sortBy, template } from 'lodash'; import * as Emoji from '~/emoji'; import axios from '~/lib/utils/axios_utils'; import { s__, __, sprintf } from '~/locale'; @@ -11,8 +11,21 @@ import { spriteIcon } from './lib/utils/common_utils'; import { parsePikadayDate } from './lib/utils/datetime_utility'; import glRegexp from './lib/utils/regexp'; -function sanitize(str) { - return str.replace(/<(?:.|\n)*?>/gm, ''); +/** + * Escapes user input before we pass it to at.js, which + * renders it as HTML in the autocomplete dropdown. + * + * at.js allows you to reference data using `${}` syntax + * (e.g. ${search}) which it replaces with the actual data + * before rendering it in the autocomplete dropdown. + * To prevent user input from executing this `${}` syntax, + * we also need to escape the $ character. + * + * @param string user input + * @return {string} escaped user input + */ +function escape(string) { + return lodashEscape(string).replace(/\$/g, '$'); } function createMemberSearchString(member) { @@ -44,8 +57,8 @@ export function membersBeforeSave(members) { return { username: member.username, avatarTag: autoCompleteAvatar.length === 1 ? txtAvatar : imgAvatar, - title: sanitize(title), - search: sanitize(createMemberSearchString(member)), + title, + search: createMemberSearchString(member), icon: avatarIcon, availability: member?.availability, }; @@ -366,7 +379,7 @@ class GfmAutoComplete { } return { id: i.iid, - title: sanitize(i.title), + title: i.title, reference: i.reference, search: `${i.iid} ${i.title}`, }; @@ -404,7 +417,7 @@ class GfmAutoComplete { return { id: m.iid, - title: sanitize(m.title), + title: m.title, search: m.title, expired, dueDate, @@ -456,7 +469,7 @@ class GfmAutoComplete { } return { id: m.iid, - title: sanitize(m.title), + title: m.title, reference: m.reference, search: `${m.iid} ${m.title}`, }; @@ -492,7 +505,7 @@ class GfmAutoComplete { beforeSave(merges) { if (GfmAutoComplete.isLoading(merges)) return merges; return $.map(merges, (m) => ({ - title: sanitize(m.title), + title: m.title, color: m.color, search: m.title, set: m.set, @@ -586,7 +599,7 @@ class GfmAutoComplete { } return { id: m.id, - title: sanitize(m.title), + title: m.title, search: `${m.id} ${m.title}`, }; }); diff --git a/app/controllers/admin/users_controller.rb b/app/controllers/admin/users_controller.rb index 9c556d16913..55e03503ba9 100644 --- a/app/controllers/admin/users_controller.rb +++ b/app/controllers/admin/users_controller.rb @@ -45,7 +45,7 @@ class Admin::UsersController < Admin::ApplicationController end def impersonate - if can?(user, :log_in) + if can?(user, :log_in) && !impersonation_in_progress? session[:impersonator_id] = current_user.id warden.set_user(user, scope: :user) @@ -57,7 +57,9 @@ class Admin::UsersController < Admin::ApplicationController redirect_to root_path else flash[:alert] = - if user.blocked? + if impersonation_in_progress? + _("You are already impersonating another user") + elsif user.blocked? _("You cannot impersonate a blocked user") elsif user.internal? _("You cannot impersonate an internal user") diff --git a/app/controllers/concerns/impersonation.rb b/app/controllers/concerns/impersonation.rb index a4f2c263eb4..0764fbc8eb3 100644 --- a/app/controllers/concerns/impersonation.rb +++ b/app/controllers/concerns/impersonation.rb @@ -14,7 +14,7 @@ module Impersonation protected def check_impersonation_availability - return unless session[:impersonator_id] + return unless impersonation_in_progress? unless Gitlab.config.gitlab.impersonation_enabled stop_impersonation @@ -31,6 +31,10 @@ module Impersonation current_user end + def impersonation_in_progress? + session[:impersonator_id].present? + end + def log_impersonation_event Gitlab::AppLogger.info("User #{impersonator.username} has stopped impersonating #{current_user.username}") end diff --git a/app/controllers/import/gitea_controller.rb b/app/controllers/import/gitea_controller.rb index 5a4eef352b8..32c9da67e90 100644 --- a/app/controllers/import/gitea_controller.rb +++ b/app/controllers/import/gitea_controller.rb @@ -66,11 +66,13 @@ class Import::GiteaController < Import::GithubController override :client_options def client_options - { host: provider_url, api_version: 'v1' } + verified_url, provider_hostname = verify_blocked_uri + + { host: verified_url.scheme == 'https' ? provider_url : verified_url.to_s, api_version: 'v1', hostname: provider_hostname } end def verify_blocked_uri - Gitlab::UrlBlocker.validate!( + @verified_url_and_hostname ||= Gitlab::UrlBlocker.validate!( provider_url, allow_localhost: allow_local_requests?, allow_local_network: allow_local_requests?, diff --git a/app/controllers/profiles/passwords_controller.rb b/app/controllers/profiles/passwords_controller.rb index 85e901eb3eb..c8c2dd1c7d6 100644 --- a/app/controllers/profiles/passwords_controller.rb +++ b/app/controllers/profiles/passwords_controller.rb @@ -47,6 +47,8 @@ class Profiles::PasswordsController < Profiles::ApplicationController password_attributes[:password_automatically_set] = false unless @user.password_automatically_set || @user.valid_password?(user_params[:current_password]) + handle_invalid_current_password_attempt! + redirect_to edit_profile_password_path, alert: _('You must provide a valid current password') return end @@ -85,6 +87,12 @@ class Profiles::PasswordsController < Profiles::ApplicationController render_404 unless @user.allow_password_authentication? end + def handle_invalid_current_password_attempt! + Gitlab::AppLogger.info(message: 'Invalid current password when attempting to update user password', username: @user.username, ip: request.remote_ip) + + @user.increment_failed_attempts! + end + def user_params params.require(:user).permit(:current_password, :password, :password_confirmation) end diff --git a/app/models/user.rb b/app/models/user.rb index b5f0251f639..a4c8d606911 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -1883,7 +1883,8 @@ class User < ApplicationRecord def password_expired_if_applicable? return false if bot? - return false unless password_expired? && password_automatically_set? + return false unless password_expired? + return false if password_automatically_set? return false unless allow_password_authentication? true diff --git a/app/services/projects/destroy_service.rb b/app/services/projects/destroy_service.rb index e27ea7c07e5..afa8de04fca 100644 --- a/app/services/projects/destroy_service.rb +++ b/app/services/projects/destroy_service.rb @@ -117,6 +117,7 @@ module Projects trash_relation_repositories! trash_project_repositories! destroy_web_hooks! + destroy_project_bots! # Rails attempts to load all related records into memory before # destroying: https://github.com/rails/rails/issues/22510 @@ -149,6 +150,16 @@ module Projects end end + # The project can have multiple project bots with personal access tokens generated. + # We need to remove them when a project is deleted + # rubocop: disable CodeReuse/ActiveRecord + def destroy_project_bots! + project.members.includes(:user).references(:user).merge(User.project_bot).each do |member| + Users::DestroyService.new(current_user).execute(member.user, skip_authorization: true) + end + end + # rubocop: enable CodeReuse/ActiveRecord + def remove_registry_tags return true unless Gitlab.config.registry.enabled return false unless remove_legacy_registry_tags diff --git a/app/views/projects/_export.html.haml b/app/views/projects/_export.html.haml index eb4630b84d5..97f5cdb54e5 100644 --- a/app/views/projects/_export.html.haml +++ b/app/views/projects/_export.html.haml @@ -16,6 +16,7 @@ %li= _('Job logs and artifacts') %li= _('Container registry images') %li= _('CI variables') + %li= _('Pipeline triggers') %li= _('Webhooks') %li= _('Any encrypted tokens') - if project.export_status == :finished diff --git a/config/initializers/doorkeeper.rb b/config/initializers/doorkeeper.rb index 477d419576a..25bf164c96a 100644 --- a/config/initializers/doorkeeper.rb +++ b/config/initializers/doorkeeper.rb @@ -51,6 +51,11 @@ Doorkeeper.configure do # Issue access tokens with refresh token (disabled by default) use_refresh_token + # Forbids creating/updating applications with arbitrary scopes that are + # not in configuration, i.e. `default_scopes` or `optional_scopes`. + # (disabled by default) + enforce_configured_scopes + # Forces the usage of the HTTPS protocol in non-native redirect uris (enabled # by default in non-development environments). OAuth2 delegates security in # communication to the HTTPS protocol so it is wise to keep this enabled. diff --git a/config/initializers_before_autoloader/100_patch_omniauth_oauth2.rb b/config/initializers_before_autoloader/100_patch_omniauth_oauth2.rb index 760fcba5935..1ede92609a9 100644 --- a/config/initializers_before_autoloader/100_patch_omniauth_oauth2.rb +++ b/config/initializers_before_autoloader/100_patch_omniauth_oauth2.rb @@ -1,14 +1,46 @@ # frozen_string_literal: true +# See https://github.com/omniauth/omniauth-oauth2/blob/v1.7.1/lib/omniauth/strategies/oauth2.rb#L84-L101 +# for the original version of this code. +# +# Note: We need to override `callback_phase` directly (instead of using a module with `include` or `prepend`), +# because the method has a `super` call which needs to go to the `OmniAuth::Strategy` module, +# and it also deletes `omniauth.state` from the session as a side effect. + module OmniAuth module Strategies class OAuth2 - alias_method :original_callback_phase, :callback_phase - - # Monkey patch until PR is merged and released upstream - # https://github.com/omniauth/omniauth-oauth2/pull/129 def callback_phase - original_callback_phase + error = request.params["error_reason"].presence || request.params["error"].presence + # Monkey patch #1: + # + # Swap the order of these conditions around so the `state` param is verified *first*, + # before using the error params returned by the provider. + # + # This avoids content spoofing attacks by crafting a URL with malicious messages, + # because the `state` param is only present in the session after a valid OAuth2 authentication flow. + if !options.provider_ignores_state && (request.params["state"].to_s.empty? || request.params["state"] != session.delete("omniauth.state")) + fail!(:csrf_detected, CallbackError.new(:csrf_detected, "CSRF detected")) + elsif error + fail!(error, CallbackError.new(request.params["error"], request.params["error_description"].presence || request.params["error_reason"].presence, request.params["error_uri"])) + else + self.access_token = build_access_token + self.access_token = access_token.refresh! if access_token.expired? + super + end + rescue ::OAuth2::Error, CallbackError => e + fail!(:invalid_credentials, e) + rescue ::Timeout::Error, ::Errno::ETIMEDOUT => e + fail!(:timeout, e) + rescue ::SocketError => e + fail!(:failed_to_connect, e) + # Monkey patch #2: + # + # Also catch errors from Faraday. + # See https://github.com/omniauth/omniauth-oauth2/pull/129 + # and https://github.com/oauth-xx/oauth2/issues/152 + # + # This can be removed with https://gitlab.com/gitlab-org/gitlab/-/issues/340933 rescue ::Faraday::TimeoutError, ::Faraday::ConnectionFailed => e fail!(:timeout, e) end diff --git a/db/post_migrate/20210914095310_cleanup_orphan_project_access_tokens.rb b/db/post_migrate/20210914095310_cleanup_orphan_project_access_tokens.rb new file mode 100644 index 00000000000..4756bc3dca5 --- /dev/null +++ b/db/post_migrate/20210914095310_cleanup_orphan_project_access_tokens.rb @@ -0,0 +1,54 @@ +# frozen_string_literal: true + +class CleanupOrphanProjectAccessTokens < Gitlab::Database::Migration[1.0] + disable_ddl_transaction! + + TMP_INDEX_NAME = 'idx_users_on_user_type_project_bots_batched' + + def up + users_table = define_batchable_model('users') + + add_concurrent_index(:users, :id, name: TMP_INDEX_NAME, where: 'user_type = 6') + + accumulated_orphans = [] + users_table.where(user_type: 6).each_batch(of: 500) do |relation| + orphan_ids = relation.where("not exists(select 1 from members where members.user_id = users.id)").pluck(:id) + + orphan_ids.each_slice(10) do |ids| + users_table.where(id: ids).update_all(state: 'deactivated') + end + + accumulated_orphans += orphan_ids + end + + schedule_deletion(accumulated_orphans) + ensure + remove_concurrent_index_by_name(:users, TMP_INDEX_NAME) + end + + def down + remove_concurrent_index_by_name(:users, TMP_INDEX_NAME) if index_exists_by_name?(:users, TMP_INDEX_NAME) + end + + private + + def schedule_deletion(orphan_ids) + return unless deletion_worker + + orphan_ids.each_slice(100) do |ids| + job_arguments = ids.map do |orphan_id| + [orphan_id, orphan_id, { skip_authorization: true }] + end + + deletion_worker.bulk_perform_async(job_arguments) + end + rescue StandardError + # Ignore any errors or interface changes since this part of migration is optional + end + + def deletion_worker + @deletion_worker = "DeleteUserWorker".safe_constantize unless defined?(@deletion_worker) + + @deletion_worker + end +end diff --git a/db/schema_migrations/20210914095310 b/db/schema_migrations/20210914095310 new file mode 100644 index 00000000000..fee7e0b9719 --- /dev/null +++ b/db/schema_migrations/20210914095310 @@ -0,0 +1 @@ +6fcf3ff9867df68f5e9603ae0311b29bec33aa5c5b826786b094ab0960ebcd90
\ No newline at end of file diff --git a/doc/user/project/settings/import_export.md b/doc/user/project/settings/import_export.md index 662d7e70910..69215e03f28 100644 --- a/doc/user/project/settings/import_export.md +++ b/doc/user/project/settings/import_export.md @@ -135,9 +135,11 @@ The following items are **not** exported: - Build traces and artifacts - Container registry images - CI/CD variables +- Pipeline triggers - Webhooks - Any encrypted tokens - Merge Request Approvers +- Repository size limits NOTE: For more details on the specific data persisted in a project export, see the @@ -255,13 +257,13 @@ reduce the repository size for another import attempt. git reflog expire --expire=now --all git gc --prune=now --aggressive - # Prepare recreating an importable file + # Prepare recreating an importable file git bundle create ../project.bundle smaller-tmp-main cd .. mv project/ ../"$EXPORT"-project cd .. - # Recreate an importable file + # Recreate an importable file tar -czf "$EXPORT"-smaller.tar.gz --directory="$EXPORT"/ . ``` diff --git a/lib/api/import_bitbucket_server.rb b/lib/api/import_bitbucket_server.rb index ecd78c6e6db..0f0d62dcbfb 100644 --- a/lib/api/import_bitbucket_server.rb +++ b/lib/api/import_bitbucket_server.rb @@ -4,6 +4,10 @@ module API class ImportBitbucketServer < ::API::Base feature_category :importers + before do + forbidden! unless Gitlab::CurrentSettings.import_sources&.include?('bitbucket_server') + end + helpers do def client @client ||= BitbucketServer::Client.new(credentials) diff --git a/lib/gitlab/auth.rb b/lib/gitlab/auth.rb index 1afb2eda149..0970b92723b 100644 --- a/lib/gitlab/auth.rb +++ b/lib/gitlab/auth.rb @@ -172,7 +172,11 @@ module Gitlab user = find_with_user_password(login, password) return unless user - raise Gitlab::Auth::MissingPersonalAccessTokenError if user.two_factor_enabled? + verifier = TwoFactorAuthVerifier.new(user) + + if user.two_factor_enabled? || verifier.two_factor_authentication_enforced? + raise Gitlab::Auth::MissingPersonalAccessTokenError + end Gitlab::Auth::Result.new(user, nil, :gitlab_or_ldap, full_authentication_abilities) end diff --git a/lib/gitlab/auth/two_factor_auth_verifier.rb b/lib/gitlab/auth/two_factor_auth_verifier.rb index 86552ef1267..5a203a1fe9c 100644 --- a/lib/gitlab/auth/two_factor_auth_verifier.rb +++ b/lib/gitlab/auth/two_factor_auth_verifier.rb @@ -9,6 +9,10 @@ module Gitlab @current_user = current_user end + def two_factor_authentication_enforced? + two_factor_authentication_required? && two_factor_grace_period_expired? + end + def two_factor_authentication_required? Gitlab::CurrentSettings.require_two_factor_authentication? || current_user&.require_two_factor_authentication_from_group? diff --git a/lib/gitlab/import_export/group/import_export.yml b/lib/gitlab/import_export/group/import_export.yml index 630f918a78b..f7ab1677001 100644 --- a/lib/gitlab/import_export/group/import_export.yml +++ b/lib/gitlab/import_export/group/import_export.yml @@ -37,6 +37,7 @@ excluded_attributes: - :trial_ends_on - :shared_runners_minute_limit - :extra_shared_runners_minutes_limit + - :repository_size_limit epics: - :state_id diff --git a/lib/gitlab/import_export/project/import_export.yml b/lib/gitlab/import_export/project/import_export.yml index fe0974d27a6..8046fedc4f3 100644 --- a/lib/gitlab/import_export/project/import_export.yml +++ b/lib/gitlab/import_export/project/import_export.yml @@ -88,7 +88,6 @@ tree: - :external_pull_request - :merge_request - :auto_devops - - :triggers - :pipeline_schedules - :container_expiration_policy - protected_branches: @@ -211,6 +210,7 @@ excluded_attributes: - :show_default_award_emojis - :services - :exported_protected_branches + - :repository_size_limit namespaces: - :runners_token - :runners_token_encrypted diff --git a/lib/gitlab/legacy_github_import/client.rb b/lib/gitlab/legacy_github_import/client.rb index 4482610523e..48a8e0ce6d7 100644 --- a/lib/gitlab/legacy_github_import/client.rb +++ b/lib/gitlab/legacy_github_import/client.rb @@ -8,9 +8,10 @@ module Gitlab attr_reader :access_token, :host, :api_version, :wait_for_rate_limit_reset - def initialize(access_token, host: nil, api_version: 'v3', wait_for_rate_limit_reset: true) + def initialize(access_token, host: nil, api_version: 'v3', wait_for_rate_limit_reset: true, hostname: nil) @access_token = access_token @host = host.to_s.sub(%r{/+\z}, '') + @hostname = hostname @api_version = api_version @users = {} @wait_for_rate_limit_reset = wait_for_rate_limit_reset @@ -28,7 +29,8 @@ module Gitlab # If there is no config, we're connecting to github.com and we # should verify ssl. connection_options: { - ssl: { verify: config ? config['verify_ssl'] : true } + ssl: { verify: config ? config['verify_ssl'] : true }, + headers: { host: @hostname }.compact } ) end diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 6c0b33911ca..03a26c01cd5 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -38380,6 +38380,9 @@ msgstr "" msgid "You are already a member of this %{member_source}." msgstr "" +msgid "You are already impersonating another user" +msgstr "" + msgid "You are an admin, which means granting access to %{client_name} will allow them to interact with GitLab as an admin as well. Proceed with caution." msgstr "" diff --git a/spec/controllers/admin/users_controller_spec.rb b/spec/controllers/admin/users_controller_spec.rb index 015c36c9335..e43ba6358f9 100644 --- a/spec/controllers/admin/users_controller_spec.rb +++ b/spec/controllers/admin/users_controller_spec.rb @@ -807,5 +807,20 @@ RSpec.describe Admin::UsersController do expect(response).to have_gitlab_http_status(:not_found) end end + + context 'when impersonating an admin and attempting to impersonate again' do + let(:admin2) { create(:admin) } + + before do + post :impersonate, params: { id: admin2.username } + end + + it 'does not allow double impersonation', :aggregate_failures do + post :impersonate, params: { id: user.username } + + expect(flash[:alert]).to eq(_('You are already impersonating another user')) + expect(warden.user).to eq(admin2) + end + end end end diff --git a/spec/controllers/import/gitea_controller_spec.rb b/spec/controllers/import/gitea_controller_spec.rb index 3e4b159271a..568712d29cb 100644 --- a/spec/controllers/import/gitea_controller_spec.rb +++ b/spec/controllers/import/gitea_controller_spec.rb @@ -54,6 +54,48 @@ RSpec.describe Import::GiteaController do end end end + + context 'when DNS Rebinding protection is enabled' do + let(:token) { 'gitea token' } + + let(:ip_uri) { 'http://167.99.148.217' } + let(:uri) { 'try.gitea.io' } + let(:https_uri) { "https://#{uri}" } + let(:http_uri) { "http://#{uri}" } + + before do + session[:gitea_access_token] = token + + allow(Gitlab::UrlBlocker).to receive(:validate!).with(https_uri, anything).and_return([Addressable::URI.parse(https_uri), uri]) + allow(Gitlab::UrlBlocker).to receive(:validate!).with(http_uri, anything).and_return([Addressable::URI.parse(ip_uri), uri]) + + allow(Gitlab::LegacyGithubImport::Client).to receive(:new).and_return(double('Gitlab::LegacyGithubImport::Client', repos: [], orgs: [])) + end + + context 'when provided host url is using https' do + let(:host_url) { https_uri } + + it 'uses unchanged host url to send request to Gitea' do + expect(Gitlab::LegacyGithubImport::Client).to receive(:new).with(token, host: https_uri, api_version: 'v1', hostname: 'try.gitea.io') + + get :status, format: :json + + expect(response).to have_gitlab_http_status(:ok) + end + end + + context 'when provided host url is using http' do + let(:host_url) { http_uri } + + it 'uses changed host url to send request to Gitea' do + expect(Gitlab::LegacyGithubImport::Client).to receive(:new).with(token, host: 'http://167.99.148.217', api_version: 'v1', hostname: 'try.gitea.io') + + get :status, format: :json + + expect(response).to have_gitlab_http_status(:ok) + end + end + end end end diff --git a/spec/controllers/oauth/applications_controller_spec.rb b/spec/controllers/oauth/applications_controller_spec.rb index f21ef324884..5bf3b4c48bf 100644 --- a/spec/controllers/oauth/applications_controller_spec.rb +++ b/spec/controllers/oauth/applications_controller_spec.rb @@ -98,6 +98,19 @@ RSpec.describe Oauth::ApplicationsController do end describe 'POST #create' do + let(:oauth_params) do + { + doorkeeper_application: { + name: 'foo', + redirect_uri: redirect_uri, + scopes: scopes + } + } + end + + let(:redirect_uri) { 'http://example.org' } + let(:scopes) { ['api'] } + subject { post :create, params: oauth_params } it 'creates an application' do @@ -116,38 +129,42 @@ RSpec.describe Oauth::ApplicationsController do expect(response).to redirect_to(profile_path) end - context 'redirect_uri' do + context 'when redirect_uri is invalid' do + let(:redirect_uri) { 'javascript://alert()' } + render_views it 'shows an error for a forbidden URI' do - invalid_uri_params = { - doorkeeper_application: { - name: 'foo', - redirect_uri: 'javascript://alert()', - scopes: ['api'] - } - } - - post :create, params: invalid_uri_params + subject expect(response.body).to include 'Redirect URI is forbidden by the server' + expect(response).to render_template('doorkeeper/applications/index') end end context 'when scopes are not present' do + let(:scopes) { [] } + 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 + subject expect(response.body).to include 'Scopes can't be blank' + expect(response).to render_template('doorkeeper/applications/index') + end + end + + context 'when scopes are invalid' do + let(:scopes) { %w(api foo) } + + render_views + + it 'shows an error for invalid scopes' do + subject + + expect(response.body).to include 'Scopes doesn't match configured on the server.' + expect(response).to render_template('doorkeeper/applications/index') end end @@ -185,14 +202,4 @@ RSpec.describe Oauth::ApplicationsController do def disable_user_oauth allow(Gitlab::CurrentSettings.current_application_settings).to receive(:user_oauth_applications?).and_return(false) end - - def oauth_params - { - doorkeeper_application: { - name: 'foo', - redirect_uri: 'http://example.org', - scopes: ['api'] - } - } - end end diff --git a/spec/features/profiles/password_spec.rb b/spec/features/profiles/password_spec.rb index c9059395377..893dd2c76e0 100644 --- a/spec/features/profiles/password_spec.rb +++ b/spec/features/profiles/password_spec.rb @@ -78,40 +78,80 @@ RSpec.describe 'Profile > Password' do end end - context 'Change passowrd' do + context 'Change password' do + let(:new_password) { '22233344' } + before do sign_in(user) visit(edit_profile_password_path) end - it 'does not change user passowrd without old one' do - page.within '.update-password' do - fill_passwords('22233344', '22233344') + shared_examples 'user enters an incorrect current password' do + subject do + page.within '.update-password' do + fill_in 'user_current_password', with: user_current_password + fill_passwords(new_password, new_password) + end end - page.within '.flash-container' do - expect(page).to have_content 'You must provide a valid current password' - end - end + it 'handles the invalid password attempt, and prompts the user to try again', :aggregate_failures do + expect(Gitlab::AppLogger).to receive(:info) + .with(message: 'Invalid current password when attempting to update user password', username: user.username, ip: user.current_sign_in_ip) + + subject + + user.reload - it 'does not change password with invalid old password' do - page.within '.update-password' do - fill_in 'user_current_password', with: 'invalid' - fill_passwords('password', 'confirmation') + expect(user.failed_attempts).to eq(1) + expect(user.valid_password?(new_password)).to eq(false) + expect(current_path).to eq(edit_profile_password_path) + + page.within '.flash-container' do + expect(page).to have_content('You must provide a valid current password') + end end - page.within '.flash-container' do - expect(page).to have_content 'You must provide a valid current password' + it 'locks the user account when user passes the maximum attempts threshold', :aggregate_failures do + user.update!(failed_attempts: User.maximum_attempts.pred) + + subject + + expect(current_path).to eq(new_user_session_path) + + page.within '.flash-container' do + expect(page).to have_content('Your account is locked.') + end end end - it 'changes user password' do - page.within '.update-password' do - fill_in "user_current_password", with: user.password - fill_passwords('22233344', '22233344') + context 'when current password is blank' do + let(:user_current_password) { nil } + + it_behaves_like 'user enters an incorrect current password' + end + + context 'when current password is incorrect' do + let(:user_current_password) {'invalid' } + + it_behaves_like 'user enters an incorrect current password' + end + + context 'when the password reset is successful' do + subject do + page.within '.update-password' do + fill_in "user_current_password", with: user.password + fill_passwords(new_password, new_password) + end end - expect(current_path).to eq new_user_session_path + it 'changes the password, logs the user out and prompts them to sign in again', :aggregate_failures do + expect { subject }.to change { user.reload.valid_password?(new_password) }.to(true) + expect(current_path).to eq new_user_session_path + + page.within '.flash-container' do + expect(page).to have_content('Password was successfully updated. Please sign in again.') + end + end end end diff --git a/spec/fixtures/lib/gitlab/import_export/complex/project.json b/spec/fixtures/lib/gitlab/import_export/complex/project.json index e3aeace6383..1072e63b20b 100644 --- a/spec/fixtures/lib/gitlab/import_export/complex/project.json +++ b/spec/fixtures/lib/gitlab/import_export/complex/project.json @@ -7579,23 +7579,6 @@ } } ], - "triggers": [ - { - "id": 123, - "token": "cdbfasdf44a5958c83654733449e585", - "project_id": 5, - "owner_id": 1, - "created_at": "2017-01-16T15:25:28.637Z", - "updated_at": "2017-01-16T15:25:28.637Z" - }, - { - "id": 456, - "token": "33a66349b5ad01fc00174af87804e40", - "project_id": 5, - "created_at": "2017-01-16T15:25:29.637Z", - "updated_at": "2017-01-16T15:25:29.637Z" - } - ], "pipeline_schedules": [ { "id": 1, diff --git a/spec/fixtures/lib/gitlab/import_export/complex/tree/project/triggers.ndjson b/spec/fixtures/lib/gitlab/import_export/complex/tree/project/triggers.ndjson index 93619f4fb44..2b5bda687b8 100644 --- a/spec/fixtures/lib/gitlab/import_export/complex/tree/project/triggers.ndjson +++ b/spec/fixtures/lib/gitlab/import_export/complex/tree/project/triggers.ndjson @@ -1,2 +1,2 @@ {"id":123,"token":"cdbfasdf44a5958c83654733449e585","project_id":5,"owner_id":1,"created_at":"2017-01-16T15:25:28.637Z","updated_at":"2017-01-16T15:25:28.637Z"} -{"id":456,"token":"33a66349b5ad01fc00174af87804e40","project_id":5,"created_at":"2017-01-16T15:25:29.637Z","updated_at":"2017-01-16T15:25:29.637Z"} +{"id":456,"token":"33a66349b5ad01fc00174af87804e40","project_id":5,"created_at":"2017-01-16T15:25:29.637Z","updated_at":"2017-01-16T15:25:29.637Z"}
\ No newline at end of file diff --git a/spec/frontend/gfm_auto_complete_spec.js b/spec/frontend/gfm_auto_complete_spec.js index 211ed064762..94ad7759110 100644 --- a/spec/frontend/gfm_auto_complete_spec.js +++ b/spec/frontend/gfm_auto_complete_spec.js @@ -574,6 +574,15 @@ describe('GfmAutoComplete', () => { }), ).toBe('<li><small>grp/proj#5</small> Some Issue</li>'); }); + + it('escapes title in the template as it is user input', () => { + expect( + GfmAutoComplete.Issues.templateFunction({ + id: 5, + title: '${search}<script>oh no $', // eslint-disable-line no-template-curly-in-string + }), + ).toBe('<li><small>5</small> ${search}<script>oh no $</li>'); + }); }); describe('GfmAutoComplete.Members', () => { @@ -608,16 +617,18 @@ describe('GfmAutoComplete', () => { ).toBe('<li>IMG my-group <small></small> <i class="icon"/></li>'); }); - it('should add escaped title if title is set', () => { + it('escapes title in the template as it is user input', () => { expect( GfmAutoComplete.Members.templateFunction({ avatarTag: 'IMG', username: 'my-group', - title: 'MyGroup+', + title: '${search}<script>oh no $', // eslint-disable-line no-template-curly-in-string icon: '<i class="icon"/>', availabilityStatus: '', }), - ).toBe('<li>IMG my-group <small>MyGroup+</small> <i class="icon"/></li>'); + ).toBe( + '<li>IMG my-group <small>${search}<script>oh no $</small> <i class="icon"/></li>', + ); }); it('should add user availability status if availabilityStatus is set', () => { @@ -782,6 +793,15 @@ describe('GfmAutoComplete', () => { ${'/unlabel ~'} | ${assignedLabels} `('$input shows $output.length labels', expectLabels); }); + + it('escapes title in the template as it is user input', () => { + const color = '#123456'; + const title = '${search}<script>oh no $'; // eslint-disable-line no-template-curly-in-string + + expect(GfmAutoComplete.Labels.templateFunction(color, title)).toBe( + '<li><span class="dropdown-label-box" style="background: #123456"></span> ${search}<script>oh no $</li>', + ); + }); }); describe('emoji', () => { @@ -829,4 +849,15 @@ describe('GfmAutoComplete', () => { }); }); }); + + describe('milestones', () => { + it('escapes title in the template as it is user input', () => { + const expired = false; + const title = '${search}<script>oh no $'; // eslint-disable-line no-template-curly-in-string + + expect(GfmAutoComplete.Milestones.templateFunction(title, expired)).toBe( + '<li>${search}<script>oh no $</li>', + ); + }); + }); }); diff --git a/spec/initializers/100_patch_omniauth_oauth2_spec.rb b/spec/initializers/100_patch_omniauth_oauth2_spec.rb new file mode 100644 index 00000000000..0c436e4ef45 --- /dev/null +++ b/spec/initializers/100_patch_omniauth_oauth2_spec.rb @@ -0,0 +1,56 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe 'OmniAuth::Strategies::OAuth2', type: :strategy do + let(:strategy) { [OmniAuth::Strategies::OAuth2] } + + it 'verifies the gem version' do + current_version = OmniAuth::OAuth2::VERSION + expected_version = '1.7.1' + + expect(current_version).to eq(expected_version), <<~EOF + New version #{current_version} of the `omniauth-oauth2` gem detected! + + Please check if the monkey patches in `config/initializers_before_autoloader/100_patch_omniauth_oauth2.rb` + are still needed, and either update/remove them, or bump the version in this spec. + + EOF + end + + context 'when a custom error message is passed from an OAuth2 provider' do + let(:message) { 'Please go to https://evil.com' } + let(:state) { 'secret' } + let(:callback_path) { '/users/auth/oauth2/callback' } + let(:params) { { state: state, error: 'evil_key', error_description: message } } + let(:error) { last_request.env['omniauth.error'] } + + before do + env('rack.session', { 'omniauth.state' => state }) + end + + it 'returns the custom error message if the state is valid' do + get callback_path, **params + + expect(error.message).to eq("evil_key | #{message}") + end + + it 'returns the custom `error_reason` message if the `error_description` is blank' do + get callback_path, **params.merge(error_description: ' ', error_reason: 'custom reason') + + expect(error.message).to eq('evil_key | custom reason') + end + + it 'returns a CSRF error if the state is invalid' do + get callback_path, **params.merge(state: 'invalid') + + expect(error.message).to eq('csrf_detected | CSRF detected') + end + + it 'returns a CSRF error if the state is missing' do + get callback_path, **params.without(:state) + + expect(error.message).to eq('csrf_detected | CSRF detected') + end + end +end diff --git a/spec/lib/gitlab/auth/two_factor_auth_verifier_spec.rb b/spec/lib/gitlab/auth/two_factor_auth_verifier_spec.rb index f906870195a..876c23a91bd 100644 --- a/spec/lib/gitlab/auth/two_factor_auth_verifier_spec.rb +++ b/spec/lib/gitlab/auth/two_factor_auth_verifier_spec.rb @@ -3,33 +3,50 @@ require 'spec_helper' RSpec.describe Gitlab::Auth::TwoFactorAuthVerifier do - let(:user) { create(:user) } + using RSpec::Parameterized::TableSyntax - subject { described_class.new(user) } + subject(:verifier) { 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 + let(:user) { build_stubbed(:user, otp_grace_period_started_at: Time.zone.now) } - expect(subject.two_factor_authentication_required?).to be_truthy - end - end + describe '#two_factor_authentication_enforced?' do + subject { verifier.two_factor_authentication_enforced? } - 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) + where(:instance_level_enabled, :group_level_enabled, :grace_period_expired, :should_be_enforced) do + false | false | true | false + true | false | false | false + true | false | true | true + false | true | false | false + false | true | true | true + end - expect(subject.two_factor_authentication_required?).to be_truthy + with_them do + before do + stub_application_setting(require_two_factor_authentication: instance_level_enabled) + allow(user).to receive(:require_two_factor_authentication_from_group?).and_return(group_level_enabled) + stub_application_setting(two_factor_grace_period: grace_period_expired ? 0 : 1.month.in_hours) end + + it { is_expected.to eq(should_be_enforced) } 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) + describe '#two_factor_authentication_required?' do + subject { verifier.two_factor_authentication_required? } + + where(:instance_level_enabled, :group_level_enabled, :should_be_required) do + true | false | true + false | true | true + false | false | false + end - expect(subject.two_factor_authentication_required?).to be_falsey + with_them do + before do + stub_application_setting(require_two_factor_authentication: instance_level_enabled) + allow(user).to receive(:require_two_factor_authentication_from_group?).and_return(group_level_enabled) end + + it { is_expected.to eq(should_be_required) } end end @@ -85,25 +102,21 @@ RSpec.describe Gitlab::Auth::TwoFactorAuthVerifier do 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) + stub_application_setting two_factor_grace_period: 0 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) + stub_application_setting two_factor_grace_period: 1.month.in_hours 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) + user.otp_grace_period_started_at = nil expect(subject.two_factor_grace_period_expired?).to be_falsey end diff --git a/spec/lib/gitlab/auth_spec.rb b/spec/lib/gitlab/auth_spec.rb index cc592bb8f24..5ec6e23774a 100644 --- a/spec/lib/gitlab/auth_spec.rb +++ b/spec/lib/gitlab/auth_spec.rb @@ -386,7 +386,7 @@ RSpec.describe Gitlab::Auth, :use_clean_rails_memory_store_caching do shared_examples 'with an invalid access token' do it 'fails for a non-member' do expect(gl_auth.find_for_git_client(project_bot_user.username, access_token.token, project: project, ip: 'ip')) - .to have_attributes(auth_failure ) + .to have_attributes(auth_failure) end context 'when project bot user is blocked' do @@ -396,7 +396,7 @@ RSpec.describe Gitlab::Auth, :use_clean_rails_memory_store_caching do it 'fails for a blocked project bot' do expect(gl_auth.find_for_git_client(project_bot_user.username, access_token.token, project: project, ip: 'ip')) - .to have_attributes(auth_failure ) + .to have_attributes(auth_failure) end end end @@ -466,6 +466,41 @@ RSpec.describe Gitlab::Auth, :use_clean_rails_memory_store_caching do .to have_attributes(auth_failure) end + context 'when 2fa is enabled globally' do + let_it_be(:user) do + create(:user, username: 'normal_user', password: 'my-secret', otp_grace_period_started_at: 1.day.ago) + end + + before do + stub_application_setting(require_two_factor_authentication: true) + end + + it 'fails if grace period expired' do + stub_application_setting(two_factor_grace_period: 0) + + expect { gl_auth.find_for_git_client(user.username, user.password, project: nil, ip: 'ip') } + .to raise_error(Gitlab::Auth::MissingPersonalAccessTokenError) + end + + it 'goes through if grace period is not expired yet' do + stub_application_setting(two_factor_grace_period: 72) + + expect(gl_auth.find_for_git_client(user.username, user.password, project: nil, ip: 'ip')) + .to have_attributes(actor: user, project: nil, type: :gitlab_or_ldap, authentication_abilities: described_class.full_authentication_abilities) + end + end + + context 'when 2fa is enabled personally' do + let(:user) do + create(:user, :two_factor, username: 'normal_user', password: 'my-secret', otp_grace_period_started_at: 1.day.ago) + end + + it 'fails' do + expect { gl_auth.find_for_git_client(user.username, user.password, project: nil, ip: 'ip') } + .to raise_error(Gitlab::Auth::MissingPersonalAccessTokenError) + end + end + it 'goes through lfs authentication' do user = create( :user, @@ -757,16 +792,16 @@ RSpec.describe Gitlab::Auth, :use_clean_rails_memory_store_caching do describe 'find_with_user_password' do let!(:user) do create(:user, - username: username, - password: password, - password_confirmation: password) + username: username, + password: password, + password_confirmation: password) end let(:username) { 'John' } # username isn't lowercase, test this let(:password) { 'my-secret' } it "finds user by valid login/password" do - expect( gl_auth.find_with_user_password(username, password) ).to eql user + expect(gl_auth.find_with_user_password(username, password)).to eql user end it 'finds user by valid email/password with case-insensitive email' do @@ -779,12 +814,12 @@ RSpec.describe Gitlab::Auth, :use_clean_rails_memory_store_caching do it "does not find user with invalid password" do password = 'wrong' - expect( gl_auth.find_with_user_password(username, password) ).not_to eql user + expect(gl_auth.find_with_user_password(username, password)).not_to eql user end it "does not find user with invalid login" do user = 'wrong' - expect( gl_auth.find_with_user_password(username, password) ).not_to eql user + expect(gl_auth.find_with_user_password(username, password)).not_to eql user end include_examples 'user login operation with unique ip limit' do @@ -796,13 +831,13 @@ RSpec.describe Gitlab::Auth, :use_clean_rails_memory_store_caching do it 'finds the user in deactivated state' do user.deactivate! - expect( gl_auth.find_with_user_password(username, password) ).to eql user + expect(gl_auth.find_with_user_password(username, password)).to eql user end it "does not find user in blocked state" do user.block - expect( gl_auth.find_with_user_password(username, password) ).not_to eql user + expect(gl_auth.find_with_user_password(username, password)).not_to eql user end it 'does not find user in locked state' do @@ -814,13 +849,13 @@ RSpec.describe Gitlab::Auth, :use_clean_rails_memory_store_caching do it "does not find user in ldap_blocked state" do user.ldap_block - expect( gl_auth.find_with_user_password(username, password) ).not_to eql user + expect(gl_auth.find_with_user_password(username, password)).not_to eql user end it 'does not find user in blocked_pending_approval state' do user.block_pending_approval - expect( gl_auth.find_with_user_password(username, password) ).not_to eql user + expect(gl_auth.find_with_user_password(username, password)).not_to eql user end context 'with increment_failed_attempts' do diff --git a/spec/lib/gitlab/git_access_spec.rb b/spec/lib/gitlab/git_access_spec.rb index bf682e4e4c6..bf2e3c7f5f8 100644 --- a/spec/lib/gitlab/git_access_spec.rb +++ b/spec/lib/gitlab/git_access_spec.rb @@ -435,17 +435,19 @@ RSpec.describe Gitlab::GitAccess do it 'disallows users with expired password to pull' do project.add_maintainer(user) - user.update!(password_expires_at: 2.minutes.ago, password_automatically_set: true) + user.update!(password_expires_at: 2.minutes.ago) expect { pull_access_check }.to raise_forbidden("Your password expired. Please access GitLab from a web browser to update your password.") end - it 'allows ldap users with expired password to pull' do - project.add_maintainer(user) - user.update!(password_expires_at: 2.minutes.ago) - allow(user).to receive(:ldap_user?).and_return(true) + context 'with an ldap user' do + let(:user) { create(:omniauth_user, provider: 'ldap', password_expires_at: 2.minutes.ago) } - expect { pull_access_check }.not_to raise_error + it 'allows ldap users with expired password to pull' do + project.add_maintainer(user) + + expect { pull_access_check }.not_to raise_error + end end context 'when the project repository does not exist' do @@ -987,24 +989,23 @@ RSpec.describe Gitlab::GitAccess do end it 'disallows users with expired password to push' do - user.update!(password_expires_at: 2.minutes.ago, password_automatically_set: true) + user.update!(password_expires_at: 2.minutes.ago) expect { push_access_check }.to raise_forbidden("Your password expired. Please access GitLab from a web browser to update your password.") end - it 'allows ldap users with expired password to push' do - user.update!(password_expires_at: 2.minutes.ago) - allow(user).to receive(:ldap_user?).and_return(true) + context 'with an ldap user' do + let(:user) { create(:omniauth_user, provider: 'ldap', password_expires_at: 2.minutes.ago) } - expect { push_access_check }.not_to raise_error - end + it 'allows ldap users with expired password to push' do + expect { push_access_check }.not_to raise_error + end - it 'disallows blocked ldap users with expired password to push' do - user.block - user.update!(password_expires_at: 2.minutes.ago) - allow(user).to receive(:ldap_user?).and_return(true) + it 'disallows blocked ldap users with expired password to push' do + user.block - expect { push_access_check }.to raise_forbidden("Your account has been blocked.") + expect { push_access_check }.to raise_forbidden("Your account has been blocked.") + end end it 'cleans up the files' do diff --git a/spec/lib/gitlab/import_export/project/tree_restorer_spec.rb b/spec/lib/gitlab/import_export/project/tree_restorer_spec.rb index 82f465c4f9e..518a9337826 100644 --- a/spec/lib/gitlab/import_export/project/tree_restorer_spec.rb +++ b/spec/lib/gitlab/import_export/project/tree_restorer_spec.rb @@ -445,8 +445,8 @@ RSpec.describe Gitlab::ImportExport::Project::TreeRestorer do expect(@project.merge_requests.size).to eq(9) end - it 'only restores valid triggers' do - expect(@project.triggers.size).to eq(1) + it 'does not restore triggers' do + expect(@project.triggers.size).to eq(0) end it 'has the correct number of pipelines and statuses' do diff --git a/spec/lib/gitlab/import_export/safe_model_attributes.yml b/spec/lib/gitlab/import_export/safe_model_attributes.yml index a9efa32f986..287be24d11f 100644 --- a/spec/lib/gitlab/import_export/safe_model_attributes.yml +++ b/spec/lib/gitlab/import_export/safe_model_attributes.yml @@ -401,15 +401,6 @@ Ci::Variable: - encrypted_value - encrypted_value_salt - encrypted_value_iv -Ci::Trigger: -- id -- token -- project_id -- created_at -- updated_at -- owner_id -- description -- ref Ci::PipelineSchedule: - id - description @@ -556,7 +547,6 @@ Project: - disable_overriding_approvers_per_merge_request - merge_requests_ff_only_enabled - issues_template -- repository_size_limit - sync_time - service_desk_enabled - last_repository_updated_at diff --git a/spec/lib/gitlab/legacy_github_import/client_spec.rb b/spec/lib/gitlab/legacy_github_import/client_spec.rb index 0929b90d1f4..83ba5858d81 100644 --- a/spec/lib/gitlab/legacy_github_import/client_spec.rb +++ b/spec/lib/gitlab/legacy_github_import/client_spec.rb @@ -86,6 +86,15 @@ RSpec.describe Gitlab::LegacyGithubImport::Client do it 'builds a endpoint with the given options' do expect(client.api.api_endpoint).to eq 'https://try.gitea.io/api/v3/' end + + context 'and hostname' do + subject(:client) { described_class.new(token, host: 'https://167.99.148.217/', api_version: 'v1', hostname: 'try.gitea.io') } + + it 'builds a endpoint with the given options' do + expect(client.api.connection_options.dig(:headers, :host)).to eq 'try.gitea.io' + expect(client.api.api_endpoint).to eq 'https://167.99.148.217/api/v1/' + end + end end end diff --git a/spec/lib/gitlab/lfs_token_spec.rb b/spec/lib/gitlab/lfs_token_spec.rb index a8472062f03..3bc0bd385a7 100644 --- a/spec/lib/gitlab/lfs_token_spec.rb +++ b/spec/lib/gitlab/lfs_token_spec.rb @@ -126,7 +126,7 @@ RSpec.describe Gitlab::LfsToken, :clean_gitlab_redis_shared_state do end context 'when the user password is expired' do - let(:actor) { create(:user, password_expires_at: 1.minute.ago, password_automatically_set: true) } + let(:actor) { create(:user, password_expires_at: 1.minute.ago) } it 'returns false' do expect(lfs_token.token_valid?(lfs_token.token)).to be false @@ -135,12 +135,12 @@ RSpec.describe Gitlab::LfsToken, :clean_gitlab_redis_shared_state do end context 'when the actor is an ldap user' do - before do - allow(actor).to receive(:ldap_user?).and_return(true) - end + let(:actor) { create(:omniauth_user, provider: 'ldap') } context 'when the user is blocked' do - let(:actor) { create(:user, :blocked) } + before do + actor.block! + end it 'returns false' do expect(lfs_token.token_valid?(lfs_token.token)).to be false @@ -148,7 +148,9 @@ RSpec.describe Gitlab::LfsToken, :clean_gitlab_redis_shared_state do end context 'when the user password is expired' do - let(:actor) { create(:user, password_expires_at: 1.minute.ago) } + before do + actor.update!(password_expires_at: 1.minute.ago) + end it 'returns true' do expect(lfs_token.token_valid?(lfs_token.token)).to be true diff --git a/spec/migrations/20210914095310_cleanup_orphan_project_access_tokens_spec.rb b/spec/migrations/20210914095310_cleanup_orphan_project_access_tokens_spec.rb new file mode 100644 index 00000000000..0d0f6a3df67 --- /dev/null +++ b/spec/migrations/20210914095310_cleanup_orphan_project_access_tokens_spec.rb @@ -0,0 +1,47 @@ +# frozen_string_literal: true + +require 'spec_helper' +require_migration!('cleanup_orphan_project_access_tokens') + +RSpec.describe CleanupOrphanProjectAccessTokens, :migration do + def create_user(**extra_options) + defaults = { state: 'active', projects_limit: 0, email: "#{extra_options[:username]}@example.com" } + + table(:users).create!(defaults.merge(extra_options)) + end + + def create_membership(**extra_options) + defaults = { access_level: 30, notification_level: 0, source_id: 1, source_type: 'Project' } + + table(:members).create!(defaults.merge(extra_options)) + end + + let!(:regular_user) { create_user(username: 'regular') } + let!(:orphan_bot) { create_user(username: 'orphaned_bot', user_type: 6) } + let!(:used_bot) do + create_user(username: 'used_bot', user_type: 6).tap do |bot| + create_membership(user_id: bot.id) + end + end + + it 'marks all bots without memberships as deactivated' do + expect do + migrate! + regular_user.reload + orphan_bot.reload + used_bot.reload + end.to change { + [regular_user.state, orphan_bot.state, used_bot.state] + }.from(%w[active active active]).to(%w[active deactivated active]) + end + + it 'schedules for deletion all bots without memberships' do + job_class = 'DeleteUserWorker'.safe_constantize + + if job_class + expect(job_class).to receive(:bulk_perform_async).with([[orphan_bot.id, orphan_bot.id, skip_authorization: true]]) + + migrate! + end + end +end diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 334f9b4ae30..ca4c38d4663 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -5407,43 +5407,11 @@ RSpec.describe User do end describe '#password_expired_if_applicable?' do - let(:user) { build(:user, password_expires_at: password_expires_at, password_automatically_set: set_automatically?) } + let(:user) { build(:user, password_expires_at: password_expires_at) } subject { user.password_expired_if_applicable? } - context 'when user is not ldap user' do - context 'when user has password set automatically' do - let(:set_automatically?) { true } - - context 'when password_expires_at is not set' do - let(:password_expires_at) {} - - it 'returns false' do - is_expected.to be_falsey - end - end - - context 'when password_expires_at is in the past' do - let(:password_expires_at) { 1.minute.ago } - - it 'returns true' do - is_expected.to be_truthy - end - end - - context 'when password_expires_at is in the future' do - let(:password_expires_at) { 1.minute.from_now } - - it 'returns false' do - is_expected.to be_falsey - end - end - end - end - - context 'when user has password not set automatically' do - let(:set_automatically?) { false } - + shared_examples 'password expired not applicable' do context 'when password_expires_at is not set' do let(:password_expires_at) {} @@ -5469,13 +5437,7 @@ RSpec.describe User do end end - context 'when user is ldap user' do - let(:user) { build(:user, password_expires_at: password_expires_at) } - - before do - allow(user).to receive(:ldap_user?).and_return(true) - end - + context 'with a regular user' do context 'when password_expires_at is not set' do let(:password_expires_at) {} @@ -5487,8 +5449,8 @@ RSpec.describe User do context 'when password_expires_at is in the past' do let(:password_expires_at) { 1.minute.ago } - it 'returns false' do - is_expected.to be_falsey + it 'returns true' do + is_expected.to be_truthy end end @@ -5501,32 +5463,26 @@ RSpec.describe User do end end - context 'when user is a project bot' do - let(:user) { build(:user, :project_bot, password_expires_at: password_expires_at) } - - context 'when password_expires_at is not set' do - let(:password_expires_at) {} - - it 'returns false' do - is_expected.to be_falsey - end + context 'when user is a bot' do + before do + allow(user).to receive(:bot?).and_return(true) end - context 'when password_expires_at is in the past' do - let(:password_expires_at) { 1.minute.ago } + it_behaves_like 'password expired not applicable' + end - it 'returns false' do - is_expected.to be_falsey - end - end + context 'when password_automatically_set is true' do + let(:user) { create(:omniauth_user, provider: 'ldap')} - context 'when password_expires_at is in the future' do - let(:password_expires_at) { 1.minute.from_now } + it_behaves_like 'password expired not applicable' + end - it 'returns false' do - is_expected.to be_falsey - end + context 'when allow_password_authentication is false' do + before do + allow(user).to receive(:allow_password_authentication?).and_return(false) end + + it_behaves_like 'password expired not applicable' end end diff --git a/spec/policies/global_policy_spec.rb b/spec/policies/global_policy_spec.rb index 122612df355..ca9a5b1853c 100644 --- a/spec/policies/global_policy_spec.rb +++ b/spec/policies/global_policy_spec.rb @@ -249,15 +249,13 @@ RSpec.describe GlobalPolicy do context 'user with expired password' do before do - current_user.update!(password_expires_at: 2.minutes.ago, password_automatically_set: true) + current_user.update!(password_expires_at: 2.minutes.ago) end it { is_expected.not_to be_allowed(:access_api) } context 'when user is using ldap' do - before do - allow(current_user).to receive(:ldap_user?).and_return(true) - end + let(:current_user) { create(:omniauth_user, provider: 'ldap', password_expires_at: 2.minutes.ago) } it { is_expected.to be_allowed(:access_api) } end @@ -445,15 +443,13 @@ RSpec.describe GlobalPolicy do context 'user with expired password' do before do - current_user.update!(password_expires_at: 2.minutes.ago, password_automatically_set: true) + current_user.update!(password_expires_at: 2.minutes.ago) end it { is_expected.not_to be_allowed(:access_git) } context 'when user is using ldap' do - before do - allow(current_user).to receive(:ldap_user?).and_return(true) - end + let(:current_user) { create(:omniauth_user, provider: 'ldap', password_expires_at: 2.minutes.ago) } it { is_expected.to be_allowed(:access_git) } end @@ -537,15 +533,13 @@ RSpec.describe GlobalPolicy do context 'user with expired password' do before do - current_user.update!(password_expires_at: 2.minutes.ago, password_automatically_set: true) + current_user.update!(password_expires_at: 2.minutes.ago) end it { is_expected.not_to be_allowed(:use_slash_commands) } context 'when user is using ldap' do - before do - allow(current_user).to receive(:ldap_user?).and_return(true) - end + let(:current_user) { create(:omniauth_user, provider: 'ldap', password_expires_at: 2.minutes.ago) } it { is_expected.to be_allowed(:use_slash_commands) } end diff --git a/spec/requests/api/import_bitbucket_server_spec.rb b/spec/requests/api/import_bitbucket_server_spec.rb index 2225f737f36..970416c7444 100644 --- a/spec/requests/api/import_bitbucket_server_spec.rb +++ b/spec/requests/api/import_bitbucket_server_spec.rb @@ -28,6 +28,20 @@ RSpec.describe API::ImportBitbucketServer do Grape::Endpoint.before_each nil end + it 'rejects requests when Bitbucket Server Importer is disabled' do + stub_application_setting(import_sources: nil) + + post api("/import/bitbucket_server", user), params: { + bitbucket_server_url: base_uri, + bitbucket_server_username: user, + personal_access_token: token, + bitbucket_server_project: project_key, + bitbucket_server_repo: repo_slug + } + + expect(response).to have_gitlab_http_status(:forbidden) + end + it 'returns 201 response when the project is imported successfully' do allow(Gitlab::BitbucketServerImport::ProjectCreator) .to receive(:new).with(project_key, repo_slug, anything, repo_slug, user.namespace, user, anything) diff --git a/spec/requests/git_http_spec.rb b/spec/requests/git_http_spec.rb index a16f5abf608..d2528600477 100644 --- a/spec/requests/git_http_spec.rb +++ b/spec/requests/git_http_spec.rb @@ -61,7 +61,7 @@ RSpec.describe 'Git HTTP requests' do shared_examples 'operations are not allowed with expired password' do context "when password is expired" do it "responds to downloads with status 401 Unauthorized" do - user.update!(password_expires_at: 2.days.ago, password_automatically_set: true) + user.update!(password_expires_at: 2.days.ago) download(path, user: user.username, password: user.password) do |response| expect(response).to have_gitlab_http_status(:unauthorized) @@ -69,7 +69,7 @@ RSpec.describe 'Git HTTP requests' do end it "responds to uploads with status 401 Unauthorized" do - user.update!(password_expires_at: 2.days.ago, password_automatically_set: true) + user.update!(password_expires_at: 2.days.ago) upload(path, user: user.username, password: user.password) do |response| expect(response).to have_gitlab_http_status(:unauthorized) @@ -614,7 +614,7 @@ RSpec.describe 'Git HTTP requests' do context "when password is expired" do it "responds to downloads with status 401 unauthorized" do - user.update!(password_expires_at: 2.days.ago, password_automatically_set: true) + user.update!(password_expires_at: 2.days.ago) download(path, **env) do |response| expect(response).to have_gitlab_http_status(:unauthorized) @@ -697,7 +697,7 @@ RSpec.describe 'Git HTTP requests' do context "when password is expired" do it "responds to uploads with status 401 unauthorized" do - user.update!(password_expires_at: 2.days.ago, password_automatically_set: true) + user.update!(password_expires_at: 2.days.ago) write_access_token = create(:personal_access_token, user: user, scopes: [:write_repository]) @@ -950,7 +950,7 @@ RSpec.describe 'Git HTTP requests' do context 'when users password is expired' do it 'rejects pulls with 401 unauthorized' do - user.update!(password_expires_at: 2.days.ago, password_automatically_set: true) + user.update!(password_expires_at: 2.days.ago) download(path, user: 'gitlab-ci-token', password: build.token) do |response| expect(response).to have_gitlab_http_status(:unauthorized) @@ -1245,7 +1245,7 @@ RSpec.describe 'Git HTTP requests' do context "when password is expired" do it "responds to downloads with status 401 unauthorized" do - user.update!(password_expires_at: 2.days.ago, password_automatically_set: true) + user.update!(password_expires_at: 2.days.ago) download(path, **env) do |response| expect(response).to have_gitlab_http_status(:unauthorized) @@ -1328,7 +1328,7 @@ RSpec.describe 'Git HTTP requests' do context "when password is expired" do it "responds to uploads with status 401 unauthorized" do - user.update!(password_expires_at: 2.days.ago, password_automatically_set: true) + user.update!(password_expires_at: 2.days.ago) write_access_token = create(:personal_access_token, user: user, scopes: [:write_repository]) @@ -1555,7 +1555,7 @@ RSpec.describe 'Git HTTP requests' do context 'when users password is expired' do it 'rejects pulls with 401 unauthorized' do - user.update!(password_expires_at: 2.days.ago, password_automatically_set: true) + user.update!(password_expires_at: 2.days.ago) download(path, user: 'gitlab-ci-token', password: build.token) do |response| expect(response).to have_gitlab_http_status(:unauthorized) diff --git a/spec/requests/lfs_http_spec.rb b/spec/requests/lfs_http_spec.rb index 02eb4262690..656ae744ac1 100644 --- a/spec/requests/lfs_http_spec.rb +++ b/spec/requests/lfs_http_spec.rb @@ -126,7 +126,7 @@ RSpec.describe 'Git LFS API and storage' do it_behaves_like 'LFS http 200 blob response' context 'when user password is expired' do - let_it_be(:user) { create(:user, password_expires_at: 1.minute.ago, password_automatically_set: true)} + let_it_be(:user) { create(:user, password_expires_at: 1.minute.ago)} it_behaves_like 'LFS http 401 response' end @@ -344,7 +344,7 @@ RSpec.describe 'Git LFS API and storage' do end context 'when user password is expired' do - let_it_be(:user) { create(:user, password_expires_at: 1.minute.ago, password_automatically_set: true)} + let_it_be(:user) { create(:user, password_expires_at: 1.minute.ago)} let(:role) { :reporter} @@ -958,7 +958,7 @@ RSpec.describe 'Git LFS API and storage' do it_behaves_like 'LFS http 200 workhorse response' context 'when user password is expired' do - let_it_be(:user) { create(:user, password_expires_at: 1.minute.ago, password_automatically_set: true) } + let_it_be(:user) { create(:user, password_expires_at: 1.minute.ago) } it_behaves_like 'LFS http 401 response' end diff --git a/spec/services/projects/destroy_service_spec.rb b/spec/services/projects/destroy_service_spec.rb index 4a76347ea45..27688d8c966 100644 --- a/spec/services/projects/destroy_service_spec.rb +++ b/spec/services/projects/destroy_service_spec.rb @@ -450,6 +450,16 @@ RSpec.describe Projects::DestroyService, :aggregate_failures do end end + context 'when project has project bots' do + let!(:project_bot) { create(:user, :project_bot).tap { |user| project.add_maintainer(user) } } + + it 'deletes bot user as well' do + expect do + destroy_project(project, user) + end.to change { User.find_by(id: project_bot.id) }.to(nil) + end + end + context 'error while destroying', :sidekiq_inline do let!(:pipeline) { create(:ci_pipeline, project: project) } let!(:builds) { create_list(:ci_build, 2, :artifacts, pipeline: pipeline) } |