From 9dd7d1ae08e429b806020f7800b2dad9b3738825 Mon Sep 17 00:00:00 2001 From: Wesley Yarde Date: Mon, 27 Oct 2025 12:11:33 +0000 Subject: [PATCH 1/9] Remove warning since only human user accounts can be enterprise users --- doc/user/ssh.md | 6 ------ 1 file changed, 6 deletions(-) diff --git a/doc/user/ssh.md b/doc/user/ssh.md index 93d6b9207cd79f..7e44d53a23ae20 100644 --- a/doc/user/ssh.md +++ b/doc/user/ssh.md @@ -586,12 +586,6 @@ Disabling the SSH Keys of a group's [enterprise users](enterprise_user/_index.md even if an enterprise user is also an administrator of the group. - Disables the existing SSH Keys of the enterprise users. -{{< alert type="warning" >}} - -Disabling SSH Keys for enterprise users does not disable deployment keys for [service accounts](profile/service_accounts.md). - -{{< /alert >}} - To disable the enterprise users' SSH Keys: 1. On the left sidebar, select **Search or go to** and find your group. -- GitLab From ba62df1f7c5f1751226c22564c751fd726535e7c Mon Sep 17 00:00:00 2001 From: Wesley Yarde Date: Tue, 28 Oct 2025 11:21:15 +0000 Subject: [PATCH 2/9] Remove confusing line on deleting an enterprise account --- doc/user/ssh.md | 2 -- 1 file changed, 2 deletions(-) diff --git a/doc/user/ssh.md b/doc/user/ssh.md index 7e44d53a23ae20..94f04454198790 100644 --- a/doc/user/ssh.md +++ b/doc/user/ssh.md @@ -594,8 +594,6 @@ To disable the enterprise users' SSH Keys: 1. Under **Enterprise users**, select **Disable SSH Keys**. 1. Select **Save changes**. -When you delete or block an enterprise user account, their personal SSH Keys are automatically revoked. - ## Overriding SSH settings on the GitLab server GitLab integrates with the system-installed SSH daemon and designates a user -- GitLab From f64e17291e2ec23b562ca3456a2b1177230087f1 Mon Sep 17 00:00:00 2001 From: Wesley Yarde Date: Mon, 27 Oct 2025 13:14:47 +0000 Subject: [PATCH 3/9] Ensure every action is guarded by the SSH key check --- ee/app/controllers/ee/user_settings/ssh_keys_controller.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ee/app/controllers/ee/user_settings/ssh_keys_controller.rb b/ee/app/controllers/ee/user_settings/ssh_keys_controller.rb index 68e7bd6943d6aa..777bb75d2998c0 100644 --- a/ee/app/controllers/ee/user_settings/ssh_keys_controller.rb +++ b/ee/app/controllers/ee/user_settings/ssh_keys_controller.rb @@ -6,7 +6,7 @@ module SshKeysController extend ActiveSupport::Concern prepended do - before_action :check_ssh_keys_enabled, only: [:index, :show, :create, :destroy] + before_action :check_ssh_keys_enabled end private -- GitLab From 3019e5c0333636a96a2826212437ba15eba1c5ed Mon Sep 17 00:00:00 2001 From: Wesley Yarde Date: Mon, 27 Oct 2025 14:01:48 +0000 Subject: [PATCH 4/9] Remove redundant guard --- ee/lib/ee/gitlab/git_access_project.rb | 1 - 1 file changed, 1 deletion(-) diff --git a/ee/lib/ee/gitlab/git_access_project.rb b/ee/lib/ee/gitlab/git_access_project.rb index 2a74202cbfb698..e135d1fb68f528 100644 --- a/ee/lib/ee/gitlab/git_access_project.rb +++ b/ee/lib/ee/gitlab/git_access_project.rb @@ -52,7 +52,6 @@ def allowed_access_namespace? # Deploy keys are allowed anyway def ssh_keys_disabled_for_user? return false unless actor.instance_of?(::Key) - return false unless user&.enterprise_user? return false unless user.enterprise_group&.disable_ssh_keys? user.human? -- GitLab From 4f806b49f896b0dc61cc279c1010e90d0aac5995 Mon Sep 17 00:00:00 2001 From: Wesley Yarde Date: Tue, 28 Oct 2025 11:07:47 +0000 Subject: [PATCH 5/9] Move check for SSH Keys enabled into user.rb to enable reuse By moving the functionality to check if a user can authenticate with an SSH key into user.rb, we can reuse the same method in other places. --- .../ee/user_settings/ssh_keys_controller.rb | 2 +- ee/app/models/ee/user.rb | 5 + ee/lib/ee/gitlab/git_access.rb | 10 ++ ee/lib/ee/gitlab/git_access_project.rb | 13 --- .../user_settings/menus/ssh_keys_menu.rb | 2 +- .../lib/ee/gitlab/git_access_project_spec.rb | 58 ----------- ee/spec/lib/gitlab/git_access_spec.rb | 25 +++++ ee/spec/models/ee/user_spec.rb | 95 +++++++++++++++++++ 8 files changed, 137 insertions(+), 73 deletions(-) diff --git a/ee/app/controllers/ee/user_settings/ssh_keys_controller.rb b/ee/app/controllers/ee/user_settings/ssh_keys_controller.rb index 777bb75d2998c0..cf45efe09a08ba 100644 --- a/ee/app/controllers/ee/user_settings/ssh_keys_controller.rb +++ b/ee/app/controllers/ee/user_settings/ssh_keys_controller.rb @@ -12,7 +12,7 @@ module SshKeysController private def check_ssh_keys_enabled - return unless current_user.enterprise_user? && current_user.enterprise_group.disable_ssh_keys? + return unless current_user.ssh_keys_disabled? render_404 end diff --git a/ee/app/models/ee/user.rb b/ee/app/models/ee/user.rb index e7148e8cd05367..5c425faa0bc30b 100644 --- a/ee/app/models/ee/user.rb +++ b/ee/app/models/ee/user.rb @@ -731,6 +731,11 @@ def skip_enterprise_user_email_change_restrictions? @skip_enterprise_user_email_change_restrictions end + def ssh_keys_disabled? + # Only human users can be linked to Enterprise groups; see `Groups::EnterpriseUsers::Associable`. + !!enterprise_group&.disable_ssh_keys? + end + def contributed_epic_groups contributed_group_ids = ::Event.select(:group_id) .epic_contributions diff --git a/ee/lib/ee/gitlab/git_access.rb b/ee/lib/ee/gitlab/git_access.rb index 4ff78ba9050ca3..bdafda1cfe62bd 100644 --- a/ee/lib/ee/gitlab/git_access.rb +++ b/ee/lib/ee/gitlab/git_access.rb @@ -22,6 +22,16 @@ def check(cmd, changes) super end + override :check_valid_actor! + def check_valid_actor! + super + + return unless key? + return unless actor.user&.ssh_keys_disabled? + + raise ::Gitlab::GitAccess::ForbiddenError, 'SSH keys are disabled for this user' + end + override :can_read_project? def can_read_project? return true if geo? diff --git a/ee/lib/ee/gitlab/git_access_project.rb b/ee/lib/ee/gitlab/git_access_project.rb index e135d1fb68f528..b0b5e7373951fd 100644 --- a/ee/lib/ee/gitlab/git_access_project.rb +++ b/ee/lib/ee/gitlab/git_access_project.rb @@ -32,9 +32,6 @@ def allowed_access_namespace? # Verify namespace access only on initial call from Gitlab Shell and Workhorse return true unless changes == ::Gitlab::GitAccess::ANY - # Check if SSH keys are disabled for enterprise users - return false if ssh_keys_disabled_for_user? - # Return early if ssh certificate feature is not enabled for namespace # If allowed_namespace_path is passed anyway, we return false # It may happen, when a user authenticates via SSH certificate and tries accessing to personal namespace @@ -47,16 +44,6 @@ def allowed_access_namespace? allowed_namespace.present? && namespace.root_ancestor.id == allowed_namespace.id end - # Verify that SSH keys are disabled for enterprise users and the - # actor is a Key - # Deploy keys are allowed anyway - def ssh_keys_disabled_for_user? - return false unless actor.instance_of?(::Key) - return false unless user.enterprise_group&.disable_ssh_keys? - - user.human? - end - # Verify that enabled_git_access_protocol is ssh_certificates and the # actor is either User or Key # Deploy keys are allowed anyway diff --git a/ee/lib/ee/sidebars/user_settings/menus/ssh_keys_menu.rb b/ee/lib/ee/sidebars/user_settings/menus/ssh_keys_menu.rb index 03ab35e9c21099..7ef2210216d2d8 100644 --- a/ee/lib/ee/sidebars/user_settings/menus/ssh_keys_menu.rb +++ b/ee/lib/ee/sidebars/user_settings/menus/ssh_keys_menu.rb @@ -9,7 +9,7 @@ module SshKeysMenu override :render? def render? - return false if context.current_user&.enterprise_group&.disable_ssh_keys? + return false if context.current_user&.ssh_keys_disabled? super end diff --git a/ee/spec/lib/ee/gitlab/git_access_project_spec.rb b/ee/spec/lib/ee/gitlab/git_access_project_spec.rb index 158282e2278d9c..16d5e013575df1 100644 --- a/ee/spec/lib/ee/gitlab/git_access_project_spec.rb +++ b/ee/spec/lib/ee/gitlab/git_access_project_spec.rb @@ -387,62 +387,4 @@ def simulate_quarantine_size(repository, size) end end end - - describe 'SSH key access control', feature_category: :system_access do - let_it_be_with_reload(:enterprise_group) { create(:group) } - let_it_be(:project) { create(:project, :repository, namespace: enterprise_group) } - let_it_be_with_reload(:enterprise_user) { create(:user, enterprise_group_id: enterprise_group.id) } - let_it_be(:key) { create(:key, user: enterprise_user) } - - before do - stub_licensed_features(disable_ssh_keys: true) - stub_saas_features(disable_ssh_keys: true) - stub_feature_flags(enterprise_disable_ssh_keys: true) - project.add_developer(enterprise_user) - enterprise_group.namespace_settings.update!(disable_ssh_keys: true) - end - - def git_access_with_key(user_key) - described_class.new( - user_key, - project, - 'ssh', - authentication_abilities: %i[read_project download_code push_code], - repository_path: "#{project.full_path}.git" - ) - end - - it 'blocks SSH access for enterprise users' do - access = git_access_with_key(key) - - expect { access.check('git-upload-pack', '_any') }.to raise_error( - Gitlab::GitAccess::ForbiddenError, - 'You are not allowed to access projects in this namespace.' - ) - end - - it 'allows deploy key access' do - deploy_key = create(:deploy_key) - project.deploy_keys << deploy_key - access = git_access_with_key(deploy_key) - - expect { access.check('git-upload-pack', '_any') }.not_to raise_error - end - - it 'allows non-enterprise users' do - non_enterprise_user = create(:user) - non_enterprise_key = create(:key, user: non_enterprise_user) - project.add_developer(non_enterprise_user) - access = git_access_with_key(non_enterprise_key) - - expect { access.check('git-upload-pack', '_any') }.not_to raise_error - end - - it 'allows access when group setting is disabled' do - enterprise_group.namespace_settings.update!(disable_ssh_keys: false) - access = git_access_with_key(key) - - expect { access.check('git-upload-pack', '_any') }.not_to raise_error - end - end end diff --git a/ee/spec/lib/gitlab/git_access_spec.rb b/ee/spec/lib/gitlab/git_access_spec.rb index 24bd28ccda9880..a1fe6bbf1c485d 100644 --- a/ee/spec/lib/gitlab/git_access_spec.rb +++ b/ee/spec/lib/gitlab/git_access_spec.rb @@ -49,6 +49,31 @@ def download_ability deploy_key.deploy_keys_projects.create!(project: project, can_push: true) end + context 'SSH keys disabled for enterprise user' do + let_it_be(:group) { create(:group) } + let_it_be(:enterprise_user) { create(:user, enterprise_group_id: group.id) } + let_it_be(:key) { create(:key, user: enterprise_user) } + let_it_be(:project) { create(:project, :repository, namespace: group) } + + let(:actor) { key } + let(:protocol) { 'ssh' } + + before do + stub_licensed_features(disable_ssh_keys: true) + stub_saas_features(disable_ssh_keys: true) + stub_feature_flags(enterprise_disable_ssh_keys: true) + group.namespace_settings.update!(disable_ssh_keys: true) + end + + it 'rejects git pull with an informative message' do + expect { pull_changes }.to raise_forbidden('SSH keys are disabled for this user') + end + + it 'rejects git push with an informative message' do + expect { push_changes }.to raise_forbidden('SSH keys are disabled for this user') + end + end + context 'with ip restriction' do before do allow(Gitlab::IpAddressState).to receive(:current).and_return('192.168.0.2') diff --git a/ee/spec/models/ee/user_spec.rb b/ee/spec/models/ee/user_spec.rb index f2397917e33dad..3c6bee0e619e18 100644 --- a/ee/spec/models/ee/user_spec.rb +++ b/ee/spec/models/ee/user_spec.rb @@ -4228,4 +4228,99 @@ end end end + + describe '#ssh_keys_disabled?' do + let_it_be(:enterprise_group) { create(:group) } + + context 'when user is not associated with an enterprise group' do + let(:user) { build(:user) } + + it 'allows SSH keys' do + expect(user.ssh_keys_disabled?).to be(false) + end + end + + context 'when user belongs to an enterprise group' do + let(:user) { build(:enterprise_user, enterprise_group: enterprise_group) } + + context 'when group does not disable SSH keys' do + before do + allow(enterprise_group).to receive(:disable_ssh_keys?).and_return(false) + end + + it 'allows SSH keys' do + expect(user.ssh_keys_disabled?).to be(false) + end + end + + context 'when group disables SSH keys' do + before do + allow(enterprise_group).to receive(:disable_ssh_keys?).and_return(true) + end + + it 'blocks SSH keys for human users' do + expect(user.ssh_keys_disabled?).to be(true) + end + end + end + end + + describe 'SSH key access control', feature_category: :system_access do + let_it_be_with_reload(:enterprise_group) { create(:group) } + let_it_be(:project) { create(:project, :repository, namespace: enterprise_group) } + let_it_be_with_reload(:enterprise_user) { create(:user, enterprise_group_id: enterprise_group.id) } + let_it_be(:key) { create(:key, user: enterprise_user) } + + before do + stub_licensed_features(disable_ssh_keys: true) + stub_saas_features(disable_ssh_keys: true) + stub_feature_flags(enterprise_disable_ssh_keys: true) + + project.add_developer(enterprise_user) + enterprise_group.namespace_settings.update!(disable_ssh_keys: true) + end + + def git_access_with_key(key) + ::Gitlab::GitAccessProject.new( + key, + project, + 'ssh', + authentication_abilities: %i[read_project download_code push_code], + repository_path: "#{project.full_path}.git" + ) + end + + it 'blocks SSH access for enterprise users when disabled' do + access = git_access_with_key(key) + + expect { access.check('git-upload-pack', '_any') }.to raise_error( + ::Gitlab::GitAccess::ForbiddenError, + 'SSH keys are disabled for this user' + ) + end + + it 'allows deploy key access' do + deploy_key = create(:deploy_key) + project.deploy_keys << deploy_key + access = git_access_with_key(deploy_key) + + expect { access.check('git-upload-pack', '_any') }.not_to raise_error + end + + it 'allows non-enterprise users' do + non_enterprise_user = create(:user) + non_enterprise_key = create(:key, user: non_enterprise_user) + project.add_developer(non_enterprise_user) + access = git_access_with_key(non_enterprise_key) + + expect { access.check('git-upload-pack', '_any') }.not_to raise_error + end + + it 'allows access when group setting is disabled' do + enterprise_group.namespace_settings.update!(disable_ssh_keys: false) + access = git_access_with_key(key) + + expect { access.check('git-upload-pack', '_any') }.not_to raise_error + end + end end -- GitLab From 9fb8ed21ff8d42d824b2cc8952a3e60745bce08f Mon Sep 17 00:00:00 2001 From: Wesley Yarde Date: Wed, 29 Oct 2025 13:35:42 +0000 Subject: [PATCH 6/9] Restrict usage of SSH keys for internal APIs Guard internal APIs to ensure SSH keys cannot be used for authorization when they are disabled. --- ee/lib/ee/api/internal/base.rb | 11 ++++++++++ ee/spec/requests/api/internal/base_spec.rb | 25 ++++++++++++++++++++++ spec/requests/api/internal/base_spec.rb | 2 +- 3 files changed, 37 insertions(+), 1 deletion(-) diff --git a/ee/lib/ee/api/internal/base.rb b/ee/lib/ee/api/internal/base.rb index 3eab6c37586bd5..b6729655248399 100644 --- a/ee/lib/ee/api/internal/base.rb +++ b/ee/lib/ee/api/internal/base.rb @@ -15,6 +15,17 @@ def lfs_authentication_url(container) container.lfs_http_url_to_repo(params[:operation]) end + override :validate_actor + def validate_actor(actor) + message = super + + return message if message + + return unless actor.key && actor.user&.ssh_keys_disabled? + + 'SSH keys are disabled for this user' + end + override :check_allowed def check_allowed(params) ip = params.fetch(:check_ip, nil) diff --git a/ee/spec/requests/api/internal/base_spec.rb b/ee/spec/requests/api/internal/base_spec.rb index 6e7890307749eb..80ecf16b46c192 100644 --- a/ee/spec/requests/api/internal/base_spec.rb +++ b/ee/spec/requests/api/internal/base_spec.rb @@ -963,6 +963,31 @@ def lfs_auth_user(user_id, project) end end + describe 'POST /internal/two_factor_recovery_codes' do + let_it_be(:enterprise_group) { create(:group) } + let_it_be(:enterprise_user) { create(:user, enterprise_group_id: enterprise_group.id) } + let_it_be(:enterprise_key) { create(:key, user: enterprise_user) } + + context 'when SSH keys are disabled for the enterprise user' do + before do + stub_licensed_features(disable_ssh_keys: true) + stub_saas_features(disable_ssh_keys: true) + stub_feature_flags(enterprise_disable_ssh_keys: true) + allow(enterprise_user).to receive(:two_factor_enabled?).and_return(true) + enterprise_group.namespace_settings.update!(disable_ssh_keys: true) + end + + it 'returns an error stating SSH keys are disabled' do + post api('/internal/two_factor_recovery_codes'), + params: { key_id: enterprise_key.id }, + headers: gitlab_shell_internal_api_request_header + + expect(json_response['success']).to be_falsey + expect(json_response['message']).to eq('SSH keys are disabled for this user') + end + end + end + describe 'POST /internal/two_factor_manual_otp_check' do let_it_be(:key) { create(:key, user: user) } diff --git a/spec/requests/api/internal/base_spec.rb b/spec/requests/api/internal/base_spec.rb index f92bd06bff3f07..15a9954a999bd8 100644 --- a/spec/requests/api/internal/base_spec.rb +++ b/spec/requests/api/internal/base_spec.rb @@ -85,7 +85,7 @@ def perform_request(headers: gitlab_shell_internal_api_request_header) end end - describe 'GET /internal/two_factor_recovery_codes' do + describe 'POST /internal/two_factor_recovery_codes' do let(:key_id) { key.id } subject do -- GitLab From 0ac4015b535f2a5c25cf9279ddc84fb5949cd18a Mon Sep 17 00:00:00 2001 From: Wesley Yarde Date: Fri, 31 Oct 2025 16:12:39 +0000 Subject: [PATCH 7/9] Restrict /user/keys API when SSH keys are disabled Disallow interacting with keys via /user/keys API when SSH keys are disabled for the user --- ee/lib/ee/api/users.rb | 25 +++++++++++ ee/spec/requests/api/internal/base_spec.rb | 5 ++- ee/spec/requests/api/users_spec.rb | 48 ++++++++++++++++++++++ lib/api/users.rb | 22 ++++++++++ 4 files changed, 99 insertions(+), 1 deletion(-) create mode 100644 ee/lib/ee/api/users.rb diff --git a/ee/lib/ee/api/users.rb b/ee/lib/ee/api/users.rb new file mode 100644 index 00000000000000..23144b7abe3314 --- /dev/null +++ b/ee/lib/ee/api/users.rb @@ -0,0 +1,25 @@ +# frozen_string_literal: true + +module EE + module API + module Users + extend ActiveSupport::Concern + + prepended do + helpers do + extend ::Gitlab::Utils::Override + + override :ensure_ssh_keys_visible! + def ensure_ssh_keys_visible!(user) + not_found! if user.ssh_keys_disabled? + end + + override :ensure_ssh_keys_mutable! + def ensure_ssh_keys_mutable!(user) + render_api_error!('SSH keys are disabled for this user', 403) if user.ssh_keys_disabled? + end + end + end + end + end +end diff --git a/ee/spec/requests/api/internal/base_spec.rb b/ee/spec/requests/api/internal/base_spec.rb index 80ecf16b46c192..978deba794c665 100644 --- a/ee/spec/requests/api/internal/base_spec.rb +++ b/ee/spec/requests/api/internal/base_spec.rb @@ -973,7 +973,10 @@ def lfs_auth_user(user_id, project) stub_licensed_features(disable_ssh_keys: true) stub_saas_features(disable_ssh_keys: true) stub_feature_flags(enterprise_disable_ssh_keys: true) - allow(enterprise_user).to receive(:two_factor_enabled?).and_return(true) + + allow_next_instance_of(User) do |enterprise_user| + allow(enterprise_user).to receive(:two_factor_enabled?).and_return(true) + end enterprise_group.namespace_settings.update!(disable_ssh_keys: true) end diff --git a/ee/spec/requests/api/users_spec.rb b/ee/spec/requests/api/users_spec.rb index a0e4b4d6809af4..f120b564bdd695 100644 --- a/ee/spec/requests/api/users_spec.rb +++ b/ee/spec/requests/api/users_spec.rb @@ -126,6 +126,54 @@ end end + context 'SSH keys' do + describe 'when disabled for enterprise users' do + let_it_be(:group) { create(:group) } + let_it_be(:enterprise_user) { create(:enterprise_user, enterprise_group: group) } + let!(:key) { create(:key, user: enterprise_user) } + + before do + allow_next_found_instance_of(User) do |user| + allow(user).to receive(:ssh_keys_disabled?).and_return(true) + end + end + + describe 'GET /user/keys' do + it 'returns 404' do + get api('/user/keys', enterprise_user) + + expect(response).to have_gitlab_http_status(:not_found) + end + end + + describe 'GET /user/keys/:key_id' do + it 'returns 404' do + get api("/user/keys/#{key.id}", enterprise_user) + + expect(response).to have_gitlab_http_status(:not_found) + end + end + + describe 'POST /user/keys' do + it 'returns 403 with message' do + post api('/user/keys', enterprise_user), params: attributes_for(:key) + + expect(response).to have_gitlab_http_status(:forbidden) + expect(response.body).to include('SSH keys are disabled for this user') + end + end + + describe 'DELETE /user/keys/:key_id' do + it 'returns 403 with message' do + delete api("/user/keys/#{key.id}", enterprise_user) + + expect(response).to have_gitlab_http_status(:forbidden) + expect(response.body).to include('SSH keys are disabled for this user') + end + end + end + end + context 'shared_runners_minutes_limit' do describe "PUT /users/:id" do context 'when user is an admin' do diff --git a/lib/api/users.rb b/lib/api/users.rb index 467416e8ee8e34..e98f5f898a9ed7 100644 --- a/lib/api/users.rb +++ b/lib/api/users.rb @@ -45,6 +45,12 @@ def custom_order_by_or_sort? params[:order_by].present? || params[:sort].present? end + # overwritten in EE + def ensure_ssh_keys_visible!(_user); end + + # overwritten in EE + def ensure_ssh_keys_mutable!(_user); end + # rubocop: disable CodeReuse/ActiveRecord def reorder_users(users) # Users#search orders by exact matches and handles pagination, @@ -547,6 +553,8 @@ def reorder_users(users) user = User.find_by(id: params.delete(:user_id)) not_found!('User') unless user + ensure_ssh_keys_mutable!(user) + key = ::Keys::CreateService.new(current_user, declared_params(include_missing: false).merge(user: user, organization: Current.organization)).execute if key.persisted? @@ -570,6 +578,8 @@ def reorder_users(users) user = find_user(params[:user_id]) not_found!('User') unless user && can?(current_user, :read_user, user) + ensure_ssh_keys_visible!(user) + keys = user.keys.preload_users present paginate(keys), with: Entities::SSHKey end @@ -587,6 +597,8 @@ def reorder_users(users) user = find_user(params[:id]) not_found!('User') unless user && can?(current_user, :read_user, user) + ensure_ssh_keys_visible!(user) + key = user.keys.find_by(id: params[:key_id]) # rubocop: disable CodeReuse/ActiveRecord not_found!('Key') unless key @@ -607,6 +619,8 @@ def reorder_users(users) user = User.find_by(id: params[:id]) not_found!('User') unless user + ensure_ssh_keys_mutable!(user) + key = user.keys.find_by(id: params[:key_id]) not_found!('Key') unless key @@ -1185,6 +1199,8 @@ def set_user_status(include_missing_params:) use :pagination end get "keys", feature_category: :system_access do + ensure_ssh_keys_visible!(current_user) + keys = current_user.keys.preload_users present paginate(keys), with: Entities::SSHKey @@ -1198,6 +1214,8 @@ def set_user_status(include_missing_params:) end # rubocop: disable CodeReuse/ActiveRecord get "keys/:key_id", feature_category: :system_access do + ensure_ssh_keys_visible!(current_user) + key = current_user.keys.find_by(id: params[:key_id]) not_found!('Key') unless key @@ -1216,6 +1234,8 @@ def set_user_status(include_missing_params:) desc: 'Scope of usage for the SSH key' end post "keys", feature_category: :system_access do + ensure_ssh_keys_mutable!(current_user) + key = ::Keys::CreateService.new(current_user, declared_params(include_missing: false)).execute if key.persisted? @@ -1233,6 +1253,8 @@ def set_user_status(include_missing_params:) end # rubocop: disable CodeReuse/ActiveRecord delete "keys/:key_id", feature_category: :system_access do + ensure_ssh_keys_mutable!(current_user) + key = current_user.keys.find_by(id: params[:key_id]) not_found!('Key') unless key -- GitLab From 9c021baf0c1863a3f742ac5376f7393b0f0bd62f Mon Sep 17 00:00:00 2001 From: Wesley Yarde Date: Mon, 3 Nov 2025 19:28:18 +0000 Subject: [PATCH 8/9] WIP - Disable SSH key notifications and todos when disabled Add guards to disable notifications and todos related to SSH keys when they are disabled --- .../keys/expiry_notification_service.rb | 2 + app/services/notification_service.rb | 6 +++ app/services/todo_service.rb | 1 + .../ee/keys/expiry_notification_service.rb | 14 ++++++ ee/app/services/ee/notification_service.rb | 21 +++++++++ ee/app/services/ee/todo_service.rb | 9 ++++ .../keys/expiry_notification_service_spec.rb | 44 +++++++++++++++++++ .../services/ee/notification_service_spec.rb | 42 ++++++++++++++++++ ee/spec/services/todo_service_spec.rb | 17 +++++++ 9 files changed, 156 insertions(+) create mode 100644 ee/app/services/ee/keys/expiry_notification_service.rb create mode 100644 ee/spec/services/ee/keys/expiry_notification_service_spec.rb diff --git a/app/services/keys/expiry_notification_service.rb b/app/services/keys/expiry_notification_service.rb index 2ab395b0e0146a..44e505837e4475 100644 --- a/app/services/keys/expiry_notification_service.rb +++ b/app/services/keys/expiry_notification_service.rb @@ -50,3 +50,5 @@ def create_expired_todos end end end + +Keys::ExpiryNotificationService.prepend_mod diff --git a/app/services/notification_service.rb b/app/services/notification_service.rb index 31f5c301d73259..3bf7c04e48f72c 100644 --- a/app/services/notification_service.rb +++ b/app/services/notification_service.rb @@ -81,6 +81,8 @@ def disabled_two_factor(user, type = :two_factor, options = {}) # even if user disabled notifications. However, # it won't be sent to internal users like the # ghost user or the EE support bot. + # + # Method overridden in EE def new_key(key) if key.user&.can?(:receive_notifications) mailer.new_ssh_key_email(key.id).deliver_later @@ -163,6 +165,8 @@ def deploy_token_about_to_expire(user, token_name, project, params = {}) end # Notify the user when at least one of their ssh key has expired today + # + # Method overridden in EE def ssh_key_expired(user, fingerprints) return unless user.can?(:receive_notifications) @@ -170,6 +174,8 @@ def ssh_key_expired(user, fingerprints) end # Notify the user when at least one of their ssh key is expiring soon + # + # Method overridden in EE def ssh_key_expiring_soon(user, fingerprints) return unless user.can?(:receive_notifications) diff --git a/app/services/todo_service.rb b/app/services/todo_service.rb index 170a4252296f26..8ce0e4f165c29b 100644 --- a/app/services/todo_service.rb +++ b/app/services/todo_service.rb @@ -439,6 +439,7 @@ def create_unmergeable_todo(merge_request, todo_author) create_todos(todo_author, attributes, project.namespace, project) end + # Method overridden in EE def create_ssh_key_todos(ssh_keys, action) ssh_keys.each do |ssh_key| user = ssh_key.user diff --git a/ee/app/services/ee/keys/expiry_notification_service.rb b/ee/app/services/ee/keys/expiry_notification_service.rb new file mode 100644 index 00000000000000..788ab8e4e0a656 --- /dev/null +++ b/ee/app/services/ee/keys/expiry_notification_service.rb @@ -0,0 +1,14 @@ +# frozen_string_literal: true + +module EE + module Keys # rubocop:disable Gitlab/BoundedContexts -- existing non-EE module is not bounded + module ExpiryNotificationService + extend ::Gitlab::Utils::Override + + override :allowed? + def allowed? + super && !user.ssh_keys_disabled? + end + end + end +end diff --git a/ee/app/services/ee/notification_service.rb b/ee/app/services/ee/notification_service.rb index 604de8bd9ba069..db1558bcc0ad8e 100644 --- a/ee/app/services/ee/notification_service.rb +++ b/ee/app/services/ee/notification_service.rb @@ -5,6 +5,27 @@ module NotificationService include ::Gitlab::Utils::UsageData extend ::Gitlab::Utils::Override + override :new_key + def new_key(key) + return if key.user&.ssh_keys_disabled? + + super + end + + override :ssh_key_expired + def ssh_key_expired(user, fingerprints) + return if user.ssh_keys_disabled? + + super + end + + override :ssh_key_expiring_soon + def ssh_key_expiring_soon(user, fingerprints) + return if user.ssh_keys_disabled? + + super + end + def mirror_was_hard_failed(project) return if project.emails_disabled? diff --git a/ee/app/services/ee/todo_service.rb b/ee/app/services/ee/todo_service.rb index 4713512f71491e..d3ec6059729ca2 100644 --- a/ee/app/services/ee/todo_service.rb +++ b/ee/app/services/ee/todo_service.rb @@ -40,6 +40,15 @@ def duo_enterprise_access_granted(user) create_todos(user, attributes, nil, nil) end + override :create_ssh_key_todos + def create_ssh_key_todos(ssh_keys, action) + filtered_keys = Array(ssh_keys).reject { |key| key.user&.ssh_keys_disabled? } + + return if filtered_keys.empty? + + super(filtered_keys, action) + end + def update_epic(epic, current_user, skip_users = []) update_issuable(epic, current_user, skip_users) end diff --git a/ee/spec/services/ee/keys/expiry_notification_service_spec.rb b/ee/spec/services/ee/keys/expiry_notification_service_spec.rb new file mode 100644 index 00000000000000..6d8540ee4f46d9 --- /dev/null +++ b/ee/spec/services/ee/keys/expiry_notification_service_spec.rb @@ -0,0 +1,44 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Keys::ExpiryNotificationService, feature_category: :source_code_management do + let_it_be(:user) { create(:user) } + let_it_be(:expiring_key) { create(:key, expires_at: 3.days.from_now, user: user) } + let_it_be(:expired_key) { create(:key, :expired_today, user: user) } + + subject(:service) { described_class.new(user, params) } + + before do + allow(user).to receive(:ssh_keys_disabled?).and_return(true) + end + + shared_examples 'suppresses notifications and todos' do |column| + it 'does not send emails, create todos, or update notification timestamps' do + expect(NotificationService).not_to receive(:new) + expect(TodoService).not_to receive(:new) + + expect do + service.execute + end.not_to change { Todo.count } + + expect(keys.first.reload.public_send(column)).to be_nil + end + end + + describe '#execute' do + context 'when keys are expiring soon' do + let(:params) { { keys: user.keys.where(id: expiring_key), expiring_soon: true } } + let(:keys) { [expiring_key] } + + it_behaves_like 'suppresses notifications and todos', :before_expiry_notification_delivered_at + end + + context 'when keys have expired' do + let(:params) { { keys: user.keys.where(id: expired_key), expiring_soon: false } } + let(:keys) { [expired_key] } + + it_behaves_like 'suppresses notifications and todos', :expiry_notification_delivered_at + end + end +end diff --git a/ee/spec/services/ee/notification_service_spec.rb b/ee/spec/services/ee/notification_service_spec.rb index 09510255863ea0..a69a8f9fc64708 100644 --- a/ee/spec/services/ee/notification_service_spec.rb +++ b/ee/spec/services/ee/notification_service_spec.rb @@ -20,6 +20,48 @@ end end + describe 'SSH keys' do + let(:fingerprints) { ['aa:bb:cc'] } + + describe '#new_key' do + let(:key) { build_stubbed(:personal_key, user: build_stubbed(:user)) } + + before do + allow(key.user).to receive(:ssh_keys_disabled?).and_return(true) + end + + it 'does not send email when SSH keys are disabled' do + expect { subject.new_key(key) }.not_to have_enqueued_email(key.id, mail: 'new_ssh_key_email') + end + end + + describe '#ssh_key_expired' do + let(:user) { build_stubbed(:user) } + + before do + allow(user).to receive(:ssh_keys_disabled?).and_return(true) + end + + it 'does not send email when SSH keys are disabled' do + expect { subject.ssh_key_expired(user, fingerprints) } + .not_to have_enqueued_email(user, fingerprints, mail: 'ssh_key_expired_email') + end + end + + describe '#ssh_key_expiring_soon' do + let(:user) { build_stubbed(:user) } + + before do + allow(user).to receive(:ssh_keys_disabled?).and_return(true) + end + + it 'does not send email when SSH keys are disabled' do + expect { subject.ssh_key_expiring_soon(user, fingerprints) } + .not_to have_enqueued_email(user, fingerprints, mail: 'ssh_key_expiring_soon_email') + end + end + end + context 'new review' do let(:project) { create(:project, :repository) } let(:user) { create(:user) } diff --git a/ee/spec/services/todo_service_spec.rb b/ee/spec/services/todo_service_spec.rb index 46604233e2becb..f04a5165e6462f 100644 --- a/ee/spec/services/todo_service_spec.rb +++ b/ee/spec/services/todo_service_spec.rb @@ -15,6 +15,23 @@ let(:skip_users) { [skipped] } let(:service) { described_class.new } + describe 'SSH keys' do + let_it_be(:enterprise_user) { create(:user) } + let_it_be(:ssh_key) { create(:key, user: enterprise_user) } + + before do + allow(enterprise_user).to receive(:ssh_keys_disabled?).and_return(true) + end + + it 'does not create todos for expiring soon keys' do + expect { service.ssh_key_expiring_soon(ssh_key) }.not_to change { Todo.count } + end + + it 'does not create todos for expired keys' do + expect { service.ssh_key_expired(ssh_key) }.not_to change { Todo.count } + end + end + describe 'Epics' do let(:users) { [author, non_member, member, guest, admin, john_doe, skipped] } let(:mentions) { users.map(&:to_reference).join(' ') } -- GitLab From 0a1e0a0910fb7bf09db46f43d223bc6840af1aec Mon Sep 17 00:00:00 2001 From: Bogdan Denkovych Date: Mon, 8 Dec 2025 13:40:27 +0200 Subject: [PATCH 9/9] Bogdan's suggestions Related to https://gitlab.com/gitlab-org/gitlab/-/issues/30343#note_2940146057 --- app/models/key.rb | 10 +- app/services/notification_service.rb | 6 - app/services/todo_service.rb | 1 - doc/api/user_keys.md | 8 +- .../ee/user_settings/ssh_keys_controller.rb | 4 +- ee/app/models/ee/key.rb | 12 +- ee/app/models/ee/user.rb | 10 +- .../ee/keys/expiry_notification_service.rb | 6 +- ee/app/services/ee/notification_service.rb | 21 -- ee/app/services/ee/todo_service.rb | 9 - ee/lib/ee/api/internal/base.rb | 8 +- ee/lib/ee/api/users.rb | 25 -- ee/lib/ee/gitlab/git_access.rb | 21 +- .../features/groups/group_settings_spec.rb | 33 +++ .../user_settings/menus/ssh_keys_menu_spec.rb | 45 ++++ ee/spec/lib/gitlab/git_access_spec.rb | 136 ++++++++--- .../namespace_setting_changes_auditor_spec.rb | 1 + ee/spec/models/ee/group_spec.rb | 65 ++++-- ee/spec/models/ee/key_spec.rb | 85 ++++++- ee/spec/models/ee/user_spec.rb | 85 +++---- ee/spec/requests/api/internal/base_spec.rb | 221 ++++++++++++++++-- ee/spec/requests/api/users_spec.rb | 94 +++++--- .../user_settings/ssh_keys_controller_spec.rb | 152 +++++++++--- ee/spec/requests/groups_controller_spec.rb | 32 ++- .../keys/expiry_notification_service_spec.rb | 109 +++++++-- .../services/ee/notification_service_spec.rb | 42 ---- ee/spec/services/todo_service_spec.rb | 17 -- lib/api/users.rb | 22 -- lib/gitlab/git_access.rb | 16 +- spec/lib/gitlab/git_access_spec.rb | 8 +- spec/models/key_spec.rb | 51 +++- spec/requests/api/users_spec.rb | 8 +- 32 files changed, 945 insertions(+), 418 deletions(-) delete mode 100644 ee/lib/ee/api/users.rb create mode 100644 ee/spec/lib/ee/sidebars/user_settings/menus/ssh_keys_menu_spec.rb diff --git a/app/models/key.rb b/app/models/key.rb index 78c4518b46c331..d372ae30de894c 100644 --- a/app/models/key.rb +++ b/app/models/key.rb @@ -70,7 +70,11 @@ class Key < ApplicationRecord scope :expires_after, ->(date) { where(arel_table[:expires_at].gteq(date)) } def self.regular_keys - where(type: ['Key', nil]) + where(type: regular_key_types) + end + + def self.regular_key_types + [nil, 'Key'] end def key=(value) @@ -159,6 +163,10 @@ def to_reference fingerprint end + def regular_key? + type.in?(self.class.regular_key_types) + end + private def generate_fingerprint diff --git a/app/services/notification_service.rb b/app/services/notification_service.rb index 3bf7c04e48f72c..31f5c301d73259 100644 --- a/app/services/notification_service.rb +++ b/app/services/notification_service.rb @@ -81,8 +81,6 @@ def disabled_two_factor(user, type = :two_factor, options = {}) # even if user disabled notifications. However, # it won't be sent to internal users like the # ghost user or the EE support bot. - # - # Method overridden in EE def new_key(key) if key.user&.can?(:receive_notifications) mailer.new_ssh_key_email(key.id).deliver_later @@ -165,8 +163,6 @@ def deploy_token_about_to_expire(user, token_name, project, params = {}) end # Notify the user when at least one of their ssh key has expired today - # - # Method overridden in EE def ssh_key_expired(user, fingerprints) return unless user.can?(:receive_notifications) @@ -174,8 +170,6 @@ def ssh_key_expired(user, fingerprints) end # Notify the user when at least one of their ssh key is expiring soon - # - # Method overridden in EE def ssh_key_expiring_soon(user, fingerprints) return unless user.can?(:receive_notifications) diff --git a/app/services/todo_service.rb b/app/services/todo_service.rb index 8ce0e4f165c29b..170a4252296f26 100644 --- a/app/services/todo_service.rb +++ b/app/services/todo_service.rb @@ -439,7 +439,6 @@ def create_unmergeable_todo(merge_request, todo_author) create_todos(todo_author, attributes, project.namespace, project) end - # Method overridden in EE def create_ssh_key_todos(ssh_keys, action) ssh_keys.each do |ssh_key| user = ssh_key.user diff --git a/doc/api/user_keys.md b/doc/api/user_keys.md index e0198fc32a1010..6f38e9563a31bf 100644 --- a/doc/api/user_keys.md +++ b/doc/api/user_keys.md @@ -79,7 +79,11 @@ curl --header "PRIVATE-TOKEN: " \ ## Get an SSH key -Gets an SSH key for your user account. This endpoint does not require authentication. +Gets an SSH key for your user account. + +Prerequisites: + +- You must be authenticated. ```plaintext GET /user/keys/:key_id @@ -122,7 +126,7 @@ Supported attributes: | Attribute | Type | Required | Description | |:----------|:--------|:---------|:------------| -| `id` | integer | yes | ID of user account | +| `id` | integer | yes | ID or username of user account | | `key_id` | integer | yes | ID of existing key | Example request: diff --git a/ee/app/controllers/ee/user_settings/ssh_keys_controller.rb b/ee/app/controllers/ee/user_settings/ssh_keys_controller.rb index cf45efe09a08ba..66aa9a876bac60 100644 --- a/ee/app/controllers/ee/user_settings/ssh_keys_controller.rb +++ b/ee/app/controllers/ee/user_settings/ssh_keys_controller.rb @@ -12,9 +12,7 @@ module SshKeysController private def check_ssh_keys_enabled - return unless current_user.ssh_keys_disabled? - - render_404 + render_404 if current_user.ssh_keys_disabled? end end end diff --git a/ee/app/models/ee/key.rb b/ee/app/models/ee/key.rb index e8b7bc65a268c9..0488766ab2e5da 100644 --- a/ee/app/models/ee/key.rb +++ b/ee/app/models/ee/key.rb @@ -16,6 +16,10 @@ module Key validate :validate_expires_at_before_max_expiry_date end + validate :ensure_ssh_keys_enabled, on: :create + + private + def validate_expires_at_before_max_expiry_date return errors.add(:key, message: 'has no expiration date but an expiration date is required for SSH keys on this instance. Contact the instance administrator.') if expires_at.blank? @@ -23,11 +27,15 @@ def validate_expires_at_before_max_expiry_date max_expiry_date = (created_at.presence || Time.current) + ::Gitlab::CurrentSettings.max_ssh_key_lifetime.days errors.add(:key, message: 'has an invalid expiration date. Set a shorter lifetime for the key or contact the instance administrator.') if expires_at > max_expiry_date end + + def ensure_ssh_keys_enabled + errors.add(:base, 'SSH keys are disabled for this user') if regular_key? && user && user.ssh_keys_disabled? + end end class_methods do - def regular_keys - where(type: ['LDAPKey', 'Key', nil]) + def regular_key_types + super.push('LDAPKey') end end diff --git a/ee/app/models/ee/user.rb b/ee/app/models/ee/user.rb index 5c425faa0bc30b..e16d0120cb9b8c 100644 --- a/ee/app/models/ee/user.rb +++ b/ee/app/models/ee/user.rb @@ -732,8 +732,14 @@ def skip_enterprise_user_email_change_restrictions? end def ssh_keys_disabled? - # Only human users can be linked to Enterprise groups; see `Groups::EnterpriseUsers::Associable`. - !!enterprise_group&.disable_ssh_keys? + enterprise_user? && enterprise_group.disable_ssh_keys? + end + + override :require_ssh_key? + def require_ssh_key? + return false if ssh_keys_disabled? + + super end def contributed_epic_groups diff --git a/ee/app/services/ee/keys/expiry_notification_service.rb b/ee/app/services/ee/keys/expiry_notification_service.rb index 788ab8e4e0a656..5306c08afd8ebd 100644 --- a/ee/app/services/ee/keys/expiry_notification_service.rb +++ b/ee/app/services/ee/keys/expiry_notification_service.rb @@ -1,13 +1,15 @@ # frozen_string_literal: true module EE - module Keys # rubocop:disable Gitlab/BoundedContexts -- existing non-EE module is not bounded + module Keys # rubocop:disable Gitlab/BoundedContexts -- prepended class is not inside a bounded context namespace module ExpiryNotificationService extend ::Gitlab::Utils::Override override :allowed? def allowed? - super && !user.ssh_keys_disabled? + return false if user.ssh_keys_disabled? + + super end end end diff --git a/ee/app/services/ee/notification_service.rb b/ee/app/services/ee/notification_service.rb index db1558bcc0ad8e..604de8bd9ba069 100644 --- a/ee/app/services/ee/notification_service.rb +++ b/ee/app/services/ee/notification_service.rb @@ -5,27 +5,6 @@ module NotificationService include ::Gitlab::Utils::UsageData extend ::Gitlab::Utils::Override - override :new_key - def new_key(key) - return if key.user&.ssh_keys_disabled? - - super - end - - override :ssh_key_expired - def ssh_key_expired(user, fingerprints) - return if user.ssh_keys_disabled? - - super - end - - override :ssh_key_expiring_soon - def ssh_key_expiring_soon(user, fingerprints) - return if user.ssh_keys_disabled? - - super - end - def mirror_was_hard_failed(project) return if project.emails_disabled? diff --git a/ee/app/services/ee/todo_service.rb b/ee/app/services/ee/todo_service.rb index d3ec6059729ca2..4713512f71491e 100644 --- a/ee/app/services/ee/todo_service.rb +++ b/ee/app/services/ee/todo_service.rb @@ -40,15 +40,6 @@ def duo_enterprise_access_granted(user) create_todos(user, attributes, nil, nil) end - override :create_ssh_key_todos - def create_ssh_key_todos(ssh_keys, action) - filtered_keys = Array(ssh_keys).reject { |key| key.user&.ssh_keys_disabled? } - - return if filtered_keys.empty? - - super(filtered_keys, action) - end - def update_epic(epic, current_user, skip_users = []) update_issuable(epic, current_user, skip_users) end diff --git a/ee/lib/ee/api/internal/base.rb b/ee/lib/ee/api/internal/base.rb index b6729655248399..6df4d0debeb409 100644 --- a/ee/lib/ee/api/internal/base.rb +++ b/ee/lib/ee/api/internal/base.rb @@ -17,13 +17,11 @@ def lfs_authentication_url(container) override :validate_actor def validate_actor(actor) - message = super + error_message = super - return message if message + return error_message if error_message - return unless actor.key && actor.user&.ssh_keys_disabled? - - 'SSH keys are disabled for this user' + 'SSH keys are disabled for this user' if actor.user.ssh_keys_disabled? end override :check_allowed diff --git a/ee/lib/ee/api/users.rb b/ee/lib/ee/api/users.rb deleted file mode 100644 index 23144b7abe3314..00000000000000 --- a/ee/lib/ee/api/users.rb +++ /dev/null @@ -1,25 +0,0 @@ -# frozen_string_literal: true - -module EE - module API - module Users - extend ActiveSupport::Concern - - prepended do - helpers do - extend ::Gitlab::Utils::Override - - override :ensure_ssh_keys_visible! - def ensure_ssh_keys_visible!(user) - not_found! if user.ssh_keys_disabled? - end - - override :ensure_ssh_keys_mutable! - def ensure_ssh_keys_mutable!(user) - render_api_error!('SSH keys are disabled for this user', 403) if user.ssh_keys_disabled? - end - end - end - end - end -end diff --git a/ee/lib/ee/gitlab/git_access.rb b/ee/lib/ee/gitlab/git_access.rb index bdafda1cfe62bd..39bd260e5a16e0 100644 --- a/ee/lib/ee/gitlab/git_access.rb +++ b/ee/lib/ee/gitlab/git_access.rb @@ -22,16 +22,6 @@ def check(cmd, changes) super end - override :check_valid_actor! - def check_valid_actor! - super - - return unless key? - return unless actor.user&.ssh_keys_disabled? - - raise ::Gitlab::GitAccess::ForbiddenError, 'SSH keys are disabled for this user' - end - override :can_read_project? def can_read_project? return true if geo? @@ -111,6 +101,15 @@ def check_protocol! super end + override :check_valid_actor! + def check_valid_actor! + super + + if key? && actor.user.ssh_keys_disabled? + raise ::Gitlab::GitAccess::ForbiddenError, 'SSH keys are disabled for this user.' + end + end + override :check_change_access! def check_change_access! check_free_user_cap_over_limit! # order matters here, this needs to come before size check for storage limits @@ -158,7 +157,7 @@ def check_otp_session! return unless ::License.feature_available?(:git_two_factor_enforcement) return unless ::Feature.enabled?(:two_factor_for_cli) return unless ssh? - return if !key? || deploy_key? + return unless key? return unless user.two_factor_enabled? if ::Gitlab::Auth::Otp::SessionEnforcer.new(actor).access_restricted? diff --git a/ee/spec/features/groups/group_settings_spec.rb b/ee/spec/features/groups/group_settings_spec.rb index 2e42b879de42d3..83a2bf350aa5b2 100644 --- a/ee/spec/features/groups/group_settings_spec.rb +++ b/ee/spec/features/groups/group_settings_spec.rb @@ -530,6 +530,39 @@ def service_access_token_expiration_enforced_selector end end + context 'for disable SSH Keys setting' do + before do + stub_licensed_features(disable_ssh_keys: true) + end + + context 'when on self-managed' do + it 'does not render disable SSH Keys setting' do + visit edit_group_path(group) + wait_for_all_requests + + within(permissions_selector) do + expect(page).not_to have_content(s_('GroupSettings|Disable SSH Keys')) + end + end + end + + context 'for SaaS', :saas do + before do + stub_licensed_features(domain_verification: true, disable_ssh_keys: true) + stub_saas_features(disable_ssh_keys: true) + end + + it 'renders disable SSH Keys setting' do + visit edit_group_path(group) + wait_for_all_requests + + within(permissions_selector) do + expect(page).to have_content(s_('GroupSettings|Disable SSH Keys')) + end + end + end + end + context 'for disable_invite_members setting' do before do stub_licensed_features(disable_invite_members: true) diff --git a/ee/spec/lib/ee/sidebars/user_settings/menus/ssh_keys_menu_spec.rb b/ee/spec/lib/ee/sidebars/user_settings/menus/ssh_keys_menu_spec.rb new file mode 100644 index 00000000000000..1a84d344068b80 --- /dev/null +++ b/ee/spec/lib/ee/sidebars/user_settings/menus/ssh_keys_menu_spec.rb @@ -0,0 +1,45 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Sidebars::UserSettings::Menus::SshKeysMenu, feature_category: :navigation do + subject(:sidebar_item) { described_class.new(context) } + + describe '#render?' do + context 'for enterprise users', :saas do + before do + stub_licensed_features(disable_ssh_keys: true) + stub_saas_features(disable_ssh_keys: true) + end + + let_it_be(:group) { create(:group) } + let_it_be(:user) { create(:enterprise_user, enterprise_group: group) } + + context 'when user is logged in' do + let(:context) { Sidebars::Context.new(current_user: user, container: nil) } + + it 'renders' do + expect(sidebar_item.render?).to be true + end + + context 'when SSH Keys are disabled by the group' do + before do + group.namespace_settings.update!(disable_ssh_keys: true) + end + + it 'does not render' do + expect(sidebar_item.render?).to be false + end + end + end + + context 'when user is not logged in' do + let(:context) { Sidebars::Context.new(current_user: nil, container: nil) } + + it 'does not render' do + expect(sidebar_item.render?).to be false + end + end + end + end +end diff --git a/ee/spec/lib/gitlab/git_access_spec.rb b/ee/spec/lib/gitlab/git_access_spec.rb index a1fe6bbf1c485d..42019851a772da 100644 --- a/ee/spec/lib/gitlab/git_access_spec.rb +++ b/ee/spec/lib/gitlab/git_access_spec.rb @@ -49,31 +49,6 @@ def download_ability deploy_key.deploy_keys_projects.create!(project: project, can_push: true) end - context 'SSH keys disabled for enterprise user' do - let_it_be(:group) { create(:group) } - let_it_be(:enterprise_user) { create(:user, enterprise_group_id: group.id) } - let_it_be(:key) { create(:key, user: enterprise_user) } - let_it_be(:project) { create(:project, :repository, namespace: group) } - - let(:actor) { key } - let(:protocol) { 'ssh' } - - before do - stub_licensed_features(disable_ssh_keys: true) - stub_saas_features(disable_ssh_keys: true) - stub_feature_flags(enterprise_disable_ssh_keys: true) - group.namespace_settings.update!(disable_ssh_keys: true) - end - - it 'rejects git pull with an informative message' do - expect { pull_changes }.to raise_forbidden('SSH keys are disabled for this user') - end - - it 'rejects git push with an informative message' do - expect { push_changes }.to raise_forbidden('SSH keys are disabled for this user') - end - end - context 'with ip restriction' do before do allow(Gitlab::IpAddressState).to receive(:current).and_return('192.168.0.2') @@ -693,7 +668,7 @@ def download_ability end # This case should be fully tested elsewhere. It's only here as a point of comparison with :geo actor. - context 'when the actor is a key' do + context 'when the actor is a DeployKey' do let_it_be(:deploy_key) { create(:deploy_key, user: user) } let(:actor) { deploy_key } @@ -1173,7 +1148,7 @@ def stub_redis end end - context 'when actor is not an SSH key' do + context 'when actor is a DeployKey' do let(:deploy_key) { create(:deploy_key, user: user) } let(:actor) { deploy_key } @@ -1375,12 +1350,109 @@ def push_access_check end describe '#check_valid_actor!' do - context 'key expiration is enforced' do - let(:actor) { build(:key, expires_at: 2.days.ago) } + before do + project.add_maintainer(user) + end + + context 'for LDAPKey' do + let(:actor) { build(:ldap_key, user: user) } + + context 'key is valid' do + it 'allows pull and push access', :aggregate_failures do + expect { pull_changes }.not_to raise_error + expect { push_changes }.not_to raise_error + end + end + + context 'key is expired' do + before do + actor.expires_at = 2.days.ago + end + + it 'does not allow pull and push access', :aggregate_failures do + expect { pull_changes }.to raise_forbidden('Your SSH key has expired.') + expect { push_changes }.to raise_forbidden('Your SSH key has expired.') + end + end + + context 'key is too small' do + before do + stub_application_setting(rsa_key_restriction: 4096) + end + + it 'does not allow pull and push access', :aggregate_failures do + expect(actor).not_to be_valid + expect { pull_changes }.to raise_forbidden('Your SSH key must be at least 4096 bits.') + expect { push_changes }.to raise_forbidden('Your SSH key must be at least 4096 bits.') + end + end + + context 'key type is not allowed' do + before do + stub_application_setting(rsa_key_restriction: ApplicationSetting::FORBIDDEN_KEY_VALUE) + end - it 'does not allow expired keys', :aggregate_failures do - expect { push_changes }.to raise_forbidden('Your SSH key has expired.') - expect { pull_changes }.to raise_forbidden('Your SSH key has expired.') + it 'does not allow pull and push access', :aggregate_failures do + expect(actor).not_to be_valid + expect { pull_changes }.to raise_forbidden(/Your SSH key type is forbidden/) + expect { push_changes }.to raise_forbidden(/Your SSH key type is forbidden/) + end + end + end + + context 'for enterprise users', :saas do + before do + stub_licensed_features(disable_ssh_keys: true) + stub_saas_features(disable_ssh_keys: true) + end + + let_it_be(:group) { create(:group) } + let_it_be(:project) { create(:project, :repository, group: group) } + let_it_be(:user) { create(:enterprise_user, enterprise_group: group) } + + context "when actor is the enterprise user's SSH key" do + let_it_be(:actor) { create(:key, user: user) } + + it 'allows pull and push access', :aggregate_failures do + expect { pull_changes }.not_to raise_error + expect { push_changes }.not_to raise_error + end + + context 'when SSH Keys are disabled by the group' do + before do + group.namespace_settings.update!(disable_ssh_keys: true) + end + + it 'does not allow pull and push access', :aggregate_failures do + expect { pull_changes }.to raise_forbidden('SSH keys are disabled for this user.') + expect { push_changes }.to raise_forbidden('SSH keys are disabled for this user.') + end + end + end + + # See https://gitlab.com/gitlab-org/gitlab/-/issues/30343#note_2940146057 + context 'when actor is a DeployKey tied to the enterprise user' do + let_it_be(:actor) { create(:deploy_key, user: user) } + + before_all do + actor.deploy_keys_projects.create!(project: project, can_push: true) + end + + it 'allows pull and push access', :aggregate_failures do + expect { pull_changes }.not_to raise_error + expect { push_changes }.not_to raise_error + end + + context 'when SSH Keys are disabled by the group' do + before do + group.namespace_settings.update!(disable_ssh_keys: true) + end + + it 'allows pull and push access', :aggregate_failures do + expect { pull_changes }.not_to raise_error + expect { push_changes }.not_to raise_error + end + end end end end diff --git a/ee/spec/lib/namespaces/namespace_setting_changes_auditor_spec.rb b/ee/spec/lib/namespaces/namespace_setting_changes_auditor_spec.rb index 48b3b2c7cc50c6..b30b30c2941528 100644 --- a/ee/spec/lib/namespaces/namespace_setting_changes_auditor_spec.rb +++ b/ee/spec/lib/namespaces/namespace_setting_changes_auditor_spec.rb @@ -69,6 +69,7 @@ :service_access_tokens_expiration_enforced | false | true :enforce_ssh_certificates | false | true :disable_personal_access_tokens | false | true + :disable_ssh_keys | false | true :remove_dormant_members | false | true :remove_dormant_members_period | 90 | 100 :prevent_sharing_groups_outside_hierarchy | false | true diff --git a/ee/spec/models/ee/group_spec.rb b/ee/spec/models/ee/group_spec.rb index 452870d159230f..6c56764e9200ff 100644 --- a/ee/spec/models/ee/group_spec.rb +++ b/ee/spec/models/ee/group_spec.rb @@ -3144,13 +3144,22 @@ def webhook_headers context 'on SaaS', :saas do before do stub_saas_features(disable_ssh_keys: true) - stub_feature_flags(enterprise_disable_ssh_keys: true) end it 'returns true' do expect(group.disable_ssh_keys?).to be_truthy end + context 'when :enterprise_disable_ssh_keys FF is disabled' do + before do + stub_feature_flags(enterprise_disable_ssh_keys: false) + end + + it 'returns false' do + expect(group.disable_ssh_keys?).to be_falsey + end + end + context 'for a subgroup' do let(:subgroup) { create(:group, parent: group) } @@ -3165,39 +3174,47 @@ def webhook_headers end describe '#disable_ssh_keys_available?' do - context 'when all requirements met (licensed, SaaS, root group)', :saas do + context 'when not licensed' do + it 'returns false' do + expect(group.disable_ssh_keys_available?).to be_falsey + end + end + + context 'when licensed' do before do stub_licensed_features(disable_ssh_keys: true) - stub_saas_features(disable_ssh_keys: true) - stub_feature_flags(enterprise_disable_ssh_keys: true) end - it 'returns true for root group' do - expect(group.disable_ssh_keys_available?).to be_truthy + it 'returns false' do + expect(group.disable_ssh_keys_available?).to be_falsey end - it 'returns false for subgroup' do - subgroup = create(:group, parent: group) - expect(subgroup.disable_ssh_keys_available?).to be_falsey - end - end + context 'on SaaS', :saas do + before do + stub_saas_features(disable_ssh_keys: true) + end - context 'when requirements not met' do - it 'returns false when not licensed' do - expect(group.disable_ssh_keys_available?).to be_falsey - end + it 'returns true' do + expect(group.disable_ssh_keys_available?).to be_truthy + end - it 'returns false when licensed but not SaaS' do - stub_licensed_features(disable_ssh_keys: true) - expect(group.disable_ssh_keys_available?).to be_falsey - end + context 'when :enterprise_disable_ssh_keys FF is disabled' do + before do + stub_feature_flags(enterprise_disable_ssh_keys: false) + end - it 'returns false when feature flag is disabled', :saas do - stub_licensed_features(disable_ssh_keys: true) - stub_saas_features(disable_ssh_keys: true) - stub_feature_flags(enterprise_disable_ssh_keys: false) + it 'returns false' do + expect(group.disable_ssh_keys_available?).to be_falsey + end + end - expect(group.disable_ssh_keys_available?).to be_falsey + context 'for a subgroup' do + let(:subgroup) { create(:group, parent: group) } + + it 'returns false' do + expect(subgroup.disable_ssh_keys_available?).to be_falsey + end + end end end end diff --git a/ee/spec/models/ee/key_spec.rb b/ee/spec/models/ee/key_spec.rb index 4ac8a5ab842da3..faa3874f01fe1a 100644 --- a/ee/spec/models/ee/key_spec.rb +++ b/ee/spec/models/ee/key_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe Key do +RSpec.describe Key, feature_category: :system_acces do describe 'validations' do describe 'expiration' do using RSpec::Parameterized::TableSyntax @@ -68,6 +68,65 @@ end end end + + describe '#ensure_ssh_keys_enabled' do + context 'for enterprise users', :saas do + before do + stub_licensed_features(disable_ssh_keys: true) + stub_saas_features(disable_ssh_keys: true) + end + + let_it_be_with_reload(:group) { create(:group) } + let_it_be_with_reload(:user) { create(:enterprise_user, enterprise_group: group) } + + context 'for regular keys' do + it 'ensures SSH Keys enabled for the user', :aggregate_failures do + described_class.regular_key_types.each do |regular_key_type| + key = build(:key, type: regular_key_type, user: user) + expect(key.valid?).to be(true) + end + end + + context 'when the key is not associated with any user' do + it 'does not apply the validation' do + key = build(:key, type: described_class.regular_key_types.sample, user: nil) + expect(key.valid?).to be(true) + end + end + end + + context 'when SSH Keys are disabled by the group' do + before do + group.namespace_settings.update!(disable_ssh_keys: true) + end + + context 'for regular keys' do + it 'ensures SSH Keys enabled for the user', :aggregate_failures do + described_class.regular_key_types.each do |regular_key_type| + key = build(:key, type: regular_key_type, user: user) + expect(key.valid?).to be(false) + expect(key.errors[:base]).to include('SSH keys are disabled for this user') + end + end + + context 'when the key is not associated with any user' do + it 'does not apply the validation' do + key = build(:key, type: described_class.regular_key_types.sample, user: nil) + expect(key.valid?).to be(true) + end + end + end + + # See https://gitlab.com/gitlab-org/gitlab/-/issues/30343#note_2940146057 + context 'for deploy keys' do + it 'does not apply the validation' do + key = build(:deploy_key, user: user) + expect(key.valid?).to be(true) + end + end + end + end + end end describe '#audit_details' do @@ -76,4 +135,28 @@ expect(key.audit_details).to eq(key.title) end end + + describe 'scopes' do + describe '.regular_keys' do + let_it_be(:ldap_key) { create(:ldap_key) } + + it "includes keys with 'LDAPKey' type" do + expect(described_class.regular_keys).to include(ldap_key) + end + end + end + + describe '.regular_key_types' do + it "includes 'LDAPKey'" do + expect(described_class.regular_key_types).to include('LDAPKey') + end + end + + describe '#regular_key?' do + context "when type is 'LDAPKey'" do + it 'returns true' do + expect(build(:ldap_key).regular_key?).to be(true) + end + end + end end diff --git a/ee/spec/models/ee/user_spec.rb b/ee/spec/models/ee/user_spec.rb index 3c6bee0e619e18..1b27dd99d30802 100644 --- a/ee/spec/models/ee/user_spec.rb +++ b/ee/spec/models/ee/user_spec.rb @@ -4235,12 +4235,12 @@ context 'when user is not associated with an enterprise group' do let(:user) { build(:user) } - it 'allows SSH keys' do + it 'returns false' do expect(user.ssh_keys_disabled?).to be(false) end end - context 'when user belongs to an enterprise group' do + context 'when user is associated with an enterprise group' do let(:user) { build(:enterprise_user, enterprise_group: enterprise_group) } context 'when group does not disable SSH keys' do @@ -4248,7 +4248,7 @@ allow(enterprise_group).to receive(:disable_ssh_keys?).and_return(false) end - it 'allows SSH keys' do + it 'returns false' do expect(user.ssh_keys_disabled?).to be(false) end end @@ -4258,69 +4258,48 @@ allow(enterprise_group).to receive(:disable_ssh_keys?).and_return(true) end - it 'blocks SSH keys for human users' do + it 'returns true' do expect(user.ssh_keys_disabled?).to be(true) end end end end - describe 'SSH key access control', feature_category: :system_access do - let_it_be_with_reload(:enterprise_group) { create(:group) } - let_it_be(:project) { create(:project, :repository, namespace: enterprise_group) } - let_it_be_with_reload(:enterprise_user) { create(:user, enterprise_group_id: enterprise_group.id) } - let_it_be(:key) { create(:key, user: enterprise_user) } - - before do - stub_licensed_features(disable_ssh_keys: true) - stub_saas_features(disable_ssh_keys: true) - stub_feature_flags(enterprise_disable_ssh_keys: true) - - project.add_developer(enterprise_user) - enterprise_group.namespace_settings.update!(disable_ssh_keys: true) - end - - def git_access_with_key(key) - ::Gitlab::GitAccessProject.new( - key, - project, - 'ssh', - authentication_abilities: %i[read_project download_code push_code], - repository_path: "#{project.full_path}.git" - ) - end - - it 'blocks SSH access for enterprise users when disabled' do - access = git_access_with_key(key) + describe '#require_ssh_key?' do + let_it_be(:enterprise_group) { create(:group) } - expect { access.check('git-upload-pack', '_any') }.to raise_error( - ::Gitlab::GitAccess::ForbiddenError, - 'SSH keys are disabled for this user' - ) - end + context 'when user does not have SSH keys' do + context 'when user is not associated with an enterprise group' do + let(:user) { build(:user) } - it 'allows deploy key access' do - deploy_key = create(:deploy_key) - project.deploy_keys << deploy_key - access = git_access_with_key(deploy_key) + it 'returns true' do + expect(user.require_ssh_key?).to be(true) + end + end - expect { access.check('git-upload-pack', '_any') }.not_to raise_error - end + context 'when user is associated with an enterprise group' do + let(:user) { build(:enterprise_user, enterprise_group: enterprise_group) } - it 'allows non-enterprise users' do - non_enterprise_user = create(:user) - non_enterprise_key = create(:key, user: non_enterprise_user) - project.add_developer(non_enterprise_user) - access = git_access_with_key(non_enterprise_key) + context 'when group does not disable SSH keys' do + before do + allow(enterprise_group).to receive(:disable_ssh_keys?).and_return(false) + end - expect { access.check('git-upload-pack', '_any') }.not_to raise_error - end + it 'returns true' do + expect(user.require_ssh_key?).to be(true) + end + end - it 'allows access when group setting is disabled' do - enterprise_group.namespace_settings.update!(disable_ssh_keys: false) - access = git_access_with_key(key) + context 'when group disables SSH keys' do + before do + allow(enterprise_group).to receive(:disable_ssh_keys?).and_return(true) + end - expect { access.check('git-upload-pack', '_any') }.not_to raise_error + it 'returns false' do + expect(user.require_ssh_key?).to be(false) + end + end + end end end end diff --git a/ee/spec/requests/api/internal/base_spec.rb b/ee/spec/requests/api/internal/base_spec.rb index 978deba794c665..1d33ce7702ec53 100644 --- a/ee/spec/requests/api/internal/base_spec.rb +++ b/ee/spec/requests/api/internal/base_spec.rb @@ -911,6 +911,88 @@ def lfs_auth_user(user_id, project) end end + describe 'POST /internal/two_factor_recovery_codes' do + subject(:request_two_factor_recovery_codes) do + post api('/internal/two_factor_recovery_codes'), + params: { key_id: key.id }, + headers: gitlab_shell_internal_api_request_header + end + + context 'for enterprise users', :saas do + before do + stub_licensed_features(disable_ssh_keys: true) + stub_saas_features(disable_ssh_keys: true) + end + + let_it_be_with_reload(:group) { create(:group) } + + context 'when two-factor is enabled' do + let_it_be(:user) { create(:enterprise_user, :two_factor, enterprise_group: group) } + let_it_be(:key) { create(:key, user: user) } + + it 'returns new recovery codes' do + request_two_factor_recovery_codes + + expect(json_response['success']).to be_truthy + end + + context 'when SSH Keys are disabled by the group' do + before do + group.namespace_settings.update!(disable_ssh_keys: true) + end + + it 'returns an error message', :aggregate_failures do + request_two_factor_recovery_codes + + expect(json_response['success']).to be_falsey + expect(json_response['message']).to eq('SSH keys are disabled for this user') + end + end + end + end + end + + describe 'POST /internal/two_factor_config' do + subject(:request_two_factor_config) do + post api('/internal/two_factor_config'), + params: { key_id: key.id }, + headers: gitlab_shell_internal_api_request_header + end + + context 'for enterprise users', :saas do + before do + stub_licensed_features(disable_ssh_keys: true) + stub_saas_features(disable_ssh_keys: true) + end + + let_it_be_with_reload(:group) { create(:group) } + + context 'when two-factor is enabled' do + let_it_be(:user) { create(:enterprise_user, :two_factor, enterprise_group: group) } + let_it_be(:key) { create(:key, user: user) } + + it 'returns user two factor config' do + request_two_factor_config + + expect(json_response['success']).to be_truthy + end + + context 'when SSH Keys are disabled by the group' do + before do + group.namespace_settings.update!(disable_ssh_keys: true) + end + + it 'returns an error message', :aggregate_failures do + request_two_factor_config + + expect(json_response['success']).to be_falsey + expect(json_response['message']).to eq('SSH keys are disabled for this user') + end + end + end + end + end + describe 'POST /internal/personal_access_token', :system_access, :with_current_organization do let_it_be(:key) { create(:key, user: user) } @@ -961,38 +1043,55 @@ def lfs_auth_user(user_id, project) end end end - end - - describe 'POST /internal/two_factor_recovery_codes' do - let_it_be(:enterprise_group) { create(:group) } - let_it_be(:enterprise_user) { create(:user, enterprise_group_id: enterprise_group.id) } - let_it_be(:enterprise_key) { create(:key, user: enterprise_user) } - context 'when SSH keys are disabled for the enterprise user' do + context 'for enterprise users', :saas do before do stub_licensed_features(disable_ssh_keys: true) stub_saas_features(disable_ssh_keys: true) - stub_feature_flags(enterprise_disable_ssh_keys: true) - - allow_next_instance_of(User) do |enterprise_user| - allow(enterprise_user).to receive(:two_factor_enabled?).and_return(true) - end - enterprise_group.namespace_settings.update!(disable_ssh_keys: true) end - it 'returns an error stating SSH keys are disabled' do - post api('/internal/two_factor_recovery_codes'), - params: { key_id: enterprise_key.id }, + let_it_be_with_reload(:group) { create(:group) } + + let_it_be(:user) { create(:enterprise_user, enterprise_group: group) } + let_it_be(:key) { create(:key, user: user) } + + it 'returns new token' do + post api('/internal/personal_access_token'), + params: { + key_id: key.id, + name: 'newtoken', + scopes: %w[read_api read_repository], + expires_at: Date.tomorrow + }, headers: gitlab_shell_internal_api_request_header - expect(json_response['success']).to be_falsey - expect(json_response['message']).to eq('SSH keys are disabled for this user') + expect(json_response['success']).to be_truthy + end + + context 'when SSH Keys are disabled by the group' do + before do + group.namespace_settings.update!(disable_ssh_keys: true) + end + + it 'returns an error message', :aggregate_failures do + post api('/internal/personal_access_token'), + params: { + key_id: key.id, + name: 'newtoken', + scopes: %w[read_api read_repository], + expires_at: Date.tomorrow + }, + headers: gitlab_shell_internal_api_request_header + + expect(json_response['success']).to be_falsey + expect(json_response['message']).to eq('SSH keys are disabled for this user') + end end end end describe 'POST /internal/two_factor_manual_otp_check' do - let_it_be(:key) { create(:key, user: user) } + let(:key) { create(:key, user: user) } let(:key_id) { key.id } let(:otp) { '123456' } @@ -1040,6 +1139,47 @@ def lfs_auth_user(user_id, project) expect(json_response['success']).to be_truthy end + + context 'for enterprise users', :saas do + before do + stub_licensed_features(git_two_factor_enforcement: true, disable_ssh_keys: true) + stub_saas_features(disable_ssh_keys: true) + end + + let_it_be_with_reload(:group) { create(:group) } + + let_it_be(:user) { create(:enterprise_user, :two_factor, enterprise_group: group) } + let_it_be(:key) { create(:key, user: user) } + + it 'registers a new OTP session and returns success' do + allow_next_instance_of(Users::ValidateManualOtpService) do |service| + allow(service).to receive(:execute).with(otp).and_return(status: :success) + end + + expect_next_instance_of(::Gitlab::Auth::Otp::SessionEnforcer) do |session_enforcer| + expect(session_enforcer).to receive(:update_session).once + end + + subject + + expect(json_response['success']).to be_truthy + end + + context 'when SSH Keys are disabled by the group' do + before do + group.namespace_settings.update!(disable_ssh_keys: true) + end + + it 'does not register a new OTP session and return an error message', :aggregate_failures do + expect(Users::ValidateManualOtpService).not_to receive(:new) + + subject + + expect(json_response['success']).to be_falsey + expect(json_response['message']).to eq('SSH keys are disabled for this user') + end + end + end end context 'when the OTP is invalid' do @@ -1143,7 +1283,7 @@ def lfs_auth_user(user_id, project) end describe 'POST /internal/two_factor_push_otp_check' do - let_it_be(:key) { create(:key, user: user) } + let(:key) { create(:key, user: user) } let(:key_id) { key.id } let(:otp) { '123456' } @@ -1191,6 +1331,47 @@ def lfs_auth_user(user_id, project) expect(json_response['success']).to be_truthy end + + context 'for enterprise users', :saas do + before do + stub_licensed_features(git_two_factor_enforcement: true, disable_ssh_keys: true) + stub_saas_features(disable_ssh_keys: true) + end + + let_it_be_with_reload(:group) { create(:group) } + + let_it_be(:user) { create(:enterprise_user, :two_factor, enterprise_group: group) } + let_it_be(:key) { create(:key, user: user) } + + it 'registers a new OTP session and returns success' do + allow_next_instance_of(Users::ValidatePushOtpService) do |service| + allow(service).to receive(:execute).and_return(status: :success) + end + + expect_next_instance_of(::Gitlab::Auth::Otp::SessionEnforcer) do |session_enforcer| + expect(session_enforcer).to receive(:update_session).once + end + + subject + + expect(json_response['success']).to be_truthy + end + + context 'when SSH Keys are disabled by the group' do + before do + group.namespace_settings.update!(disable_ssh_keys: true) + end + + it 'does not register a new OTP session and return an error message', :aggregate_failures do + expect(Users::ValidatePushOtpService).not_to receive(:new) + + subject + + expect(json_response['success']).to be_falsey + expect(json_response['message']).to eq('SSH keys are disabled for this user') + end + end + end end context 'when the OTP is invalid' do diff --git a/ee/spec/requests/api/users_spec.rb b/ee/spec/requests/api/users_spec.rb index f120b564bdd695..14aa256bab4d42 100644 --- a/ee/spec/requests/api/users_spec.rb +++ b/ee/spec/requests/api/users_spec.rb @@ -126,49 +126,85 @@ end end - context 'SSH keys' do - describe 'when disabled for enterprise users' do - let_it_be(:group) { create(:group) } - let_it_be(:enterprise_user) { create(:enterprise_user, enterprise_group: group) } - let!(:key) { create(:key, user: enterprise_user) } + describe 'SSH Keys' do + describe 'POST /user/keys' do + let(:path) { '/user/keys' } - before do - allow_next_found_instance_of(User) do |user| - allow(user).to receive(:ssh_keys_disabled?).and_return(true) + context 'for enterprise users', :saas do + before do + stub_licensed_features(disable_ssh_keys: true) + stub_saas_features(disable_ssh_keys: true) end - end - describe 'GET /user/keys' do - it 'returns 404' do - get api('/user/keys', enterprise_user) + let_it_be_with_reload(:group) { create(:group) } + let_it_be_with_reload(:user) { create(:enterprise_user, enterprise_group: group) } + + it 'creates ssh key' do + key_attrs = attributes_for(:key) - expect(response).to have_gitlab_http_status(:not_found) + expect do + post api(path, user), params: key_attrs + end.to change { user.keys.count }.by(1) + + expect(response).to have_gitlab_http_status(:created) end - end - describe 'GET /user/keys/:key_id' do - it 'returns 404' do - get api("/user/keys/#{key.id}", enterprise_user) + context 'when SSH Keys are disabled by the group' do + before do + group.namespace_settings.update!(disable_ssh_keys: true) + end + + it 'does create ssh key' do + key_attrs = attributes_for(:key) + + expect do + post api(path, user), params: key_attrs + end.not_to change { user.keys.count } - expect(response).to have_gitlab_http_status(:not_found) + expect(response).to have_gitlab_http_status(:bad_request) + expect(json_response['message']['base']).to include('SSH keys are disabled for this user') + end end end + end + + describe 'POST /users/:id/keys' do + let(:path) { "/users/#{user.id}/keys" } - describe 'POST /user/keys' do - it 'returns 403 with message' do - post api('/user/keys', enterprise_user), params: attributes_for(:key) + context 'for enterprise users', :saas do + before do + stub_licensed_features(disable_ssh_keys: true) + stub_saas_features(disable_ssh_keys: true) + end - expect(response).to have_gitlab_http_status(:forbidden) - expect(response.body).to include('SSH keys are disabled for this user') + let_it_be_with_reload(:group) { create(:group) } + let_it_be_with_reload(:user) { create(:enterprise_user, enterprise_group: group) } + + it 'creates ssh key' do + key_attrs = attributes_for(:key) + + expect do + post api(path, admin, admin_mode: true), params: key_attrs + end.to change { user.keys.count }.by(1) + + expect(response).to have_gitlab_http_status(:created) end - end - describe 'DELETE /user/keys/:key_id' do - it 'returns 403 with message' do - delete api("/user/keys/#{key.id}", enterprise_user) + context 'when SSH Keys are disabled by the group' do + before do + group.namespace_settings.update!(disable_ssh_keys: true) + end - expect(response).to have_gitlab_http_status(:forbidden) - expect(response.body).to include('SSH keys are disabled for this user') + it 'does create ssh key' do + key_attrs = attributes_for(:key) + + expect do + post api(path, admin, admin_mode: true), params: key_attrs + end.not_to change { user.keys.count } + + expect(response).to have_gitlab_http_status(:bad_request) + expect(json_response['message']['base']).to include('SSH keys are disabled for this user') + end end end end diff --git a/ee/spec/requests/ee/user_settings/ssh_keys_controller_spec.rb b/ee/spec/requests/ee/user_settings/ssh_keys_controller_spec.rb index d8958703e75059..145b522635da5e 100644 --- a/ee/spec/requests/ee/user_settings/ssh_keys_controller_spec.rb +++ b/ee/spec/requests/ee/user_settings/ssh_keys_controller_spec.rb @@ -3,47 +3,141 @@ require 'spec_helper' RSpec.describe UserSettings::SshKeysController, feature_category: :source_code_management do - let_it_be(:group) { create(:group) } - let_it_be(:enterprise_user) { create(:user, enterprise_group_id: group.id) } - let_it_be(:regular_user) { create(:user, :with_namespace) } - - before do - stub_licensed_features(disable_ssh_keys: true) - stub_saas_features(disable_ssh_keys: true) - stub_feature_flags(enterprise_disable_ssh_keys: true) - end + context 'for enterprise users', :saas do + before do + stub_licensed_features(disable_ssh_keys: true) + stub_saas_features(disable_ssh_keys: true) + + login_as(user) + end + + let_it_be_with_reload(:group) { create(:group) } + let_it_be_with_reload(:user) { create(:enterprise_user, :with_namespace, enterprise_group: group) } + + let_it_be(:ssh_key) { create(:key, user: user) } + + describe 'GET #index' do + subject(:request_index) { get user_settings_ssh_keys_path } - describe 'SSH key disabling for enterprise users' do - context 'when group has SSH keys disabled' do - before do - group.namespace_settings.update!(disable_ssh_keys: true) + it 'renders page' do + request_index + + expect(response).to have_gitlab_http_status(:success) end - it 'blocks access to SSH key management for enterprise users' do - login_as(enterprise_user) + context 'when SSH Keys are disabled by the group' do + before do + group.namespace_settings.update!(disable_ssh_keys: true) + end - get user_settings_ssh_keys_path - expect(response).to have_gitlab_http_status(:not_found) + it 'renders 404' do + request_index - post user_settings_ssh_keys_path, params: { key: build(:key).attributes } - expect(response).to have_gitlab_http_status(:not_found) + expect(response).to have_gitlab_http_status(:not_found) + end end + end + + describe 'GET #show' do + subject(:request_show) { get user_settings_ssh_key_path(ssh_key) } - it 'does not block non-enterprise users' do - login_as(regular_user) + it 'renders page' do + request_show + + expect(response).to have_gitlab_http_status(:success) + end - component_double = instance_double(Namespaces::Storage::NamespaceLimit::PreEnforcementAlertComponent, - render?: false, - render_in: nil) - allow(Namespaces::Storage::NamespaceLimit::PreEnforcementAlertComponent) - .to receive(:new).and_return(component_double) + context 'when SSH Keys are disabled by the group' do + before do + group.namespace_settings.update!(disable_ssh_keys: true) + end - get user_settings_ssh_keys_path - expect(response).to have_gitlab_http_status(:ok) + it 'renders 404' do + request_show + + expect(response).to have_gitlab_http_status(:not_found) + end + end + end + + describe 'POST #create' do + subject(:request_create) do + post user_settings_ssh_keys_path, params: { key: { title: 'SSH Key', key: build(:key).key } } + end + + it 'creates the key' do + expect do + request_create + end.to change { user.reload.keys.count }.by(1) - post user_settings_ssh_keys_path, params: { key: build(:key).attributes } expect(response).to have_gitlab_http_status(:found) end + + context 'when SSH Keys are disabled by the group' do + before do + group.namespace_settings.update!(disable_ssh_keys: true) + end + + it 'does not create the key' do + expect do + request_create + end.not_to change { user.reload.keys.count } + + expect(response).to have_gitlab_http_status(:not_found) + end + end + end + + describe 'DELETE #destroy' do + subject(:request_destroy) { delete user_settings_ssh_key_path(ssh_key) } + + it 'destroys the key' do + expect do + request_destroy + end.to change { user.reload.keys.count }.by(-1) + + expect(response).to have_gitlab_http_status(:found) + end + + context 'when SSH Keys are disabled by the group' do + before do + group.namespace_settings.update!(disable_ssh_keys: true) + end + + it 'does not destroy the key' do + expect do + request_destroy + end.not_to change { user.reload.keys.count } + + expect(response).to have_gitlab_http_status(:not_found) + end + end + end + + describe 'DELETE #revoke' do + subject(:request_revoke) { delete revoke_user_settings_ssh_key_path(ssh_key) } + + it 'revokes the key' do + expect do + request_revoke + end.to change { user.reload.keys.count }.by(-1) + + expect(response).to have_gitlab_http_status(:found) + end + + context 'when SSH Keys are disabled by the group' do + before do + group.namespace_settings.update!(disable_ssh_keys: true) + end + + it 'does not revoke the key' do + expect do + request_revoke + end.not_to change { user.reload.keys.count } + + expect(response).to have_gitlab_http_status(:not_found) + end + end end end end diff --git a/ee/spec/requests/groups_controller_spec.rb b/ee/spec/requests/groups_controller_spec.rb index 86aa9bd37a3522..5157275b6d2c1c 100644 --- a/ee/spec/requests/groups_controller_spec.rb +++ b/ee/spec/requests/groups_controller_spec.rb @@ -618,40 +618,36 @@ end end - context 'setting disable_ssh_keys', :saas do + context 'setting disable_ssh_keys' do let(:params) { { group: { disable_ssh_keys: true } } } before do stub_licensed_features(disable_ssh_keys: true) - stub_saas_features(disable_ssh_keys: true) - stub_feature_flags(enterprise_disable_ssh_keys: true) end - context 'when user is a group owner' do - before do - group.add_owner(user) - end + context 'when on self-managed' do + it 'does not change the setting' do + expect(group.namespace_settings.disable_ssh_keys).to be_falsey - it 'successfully updates the setting' do - expect { request }.to change { - group.reload.namespace_settings.disable_ssh_keys? - }.from(false).to(true) + request expect(response).to have_gitlab_http_status(:found) + expect(group.namespace_settings.reload.disable_ssh_keys).to be_falsey end end - context 'when user is not a group owner' do + context 'when on SaaS', :saas do before do - group.add_maintainer(user) + stub_saas_features(disable_ssh_keys: true) end - it 'does not change the setting and returns not found' do - expect { request }.not_to change { - group.reload.namespace_settings.disable_ssh_keys? - }.from(false) + it 'changes the setting' do + expect(group.namespace_settings.disable_ssh_keys).to be_falsey - expect(response).to have_gitlab_http_status(:not_found) + request + + expect(response).to have_gitlab_http_status(:found) + expect(group.namespace_settings.reload.disable_ssh_keys).to be_truthy end end end diff --git a/ee/spec/services/ee/keys/expiry_notification_service_spec.rb b/ee/spec/services/ee/keys/expiry_notification_service_spec.rb index 6d8540ee4f46d9..3c0954818a525f 100644 --- a/ee/spec/services/ee/keys/expiry_notification_service_spec.rb +++ b/ee/spec/services/ee/keys/expiry_notification_service_spec.rb @@ -3,42 +3,109 @@ require 'spec_helper' RSpec.describe Keys::ExpiryNotificationService, feature_category: :source_code_management do - let_it_be(:user) { create(:user) } - let_it_be(:expiring_key) { create(:key, expires_at: 3.days.from_now, user: user) } - let_it_be(:expired_key) { create(:key, :expired_today, user: user) } + let(:params) { { keys: user.keys, expiring_soon: expiring_soon } } - subject(:service) { described_class.new(user, params) } + subject { described_class.new(user, params) } - before do - allow(user).to receive(:ssh_keys_disabled?).and_return(true) + shared_examples 'sends a notification' do |notification_method, notified_column| + it 'sends email to the user', :aggregate_failures do + expect(NotificationService).to receive(:new).and_call_original + + perform_enqueued_jobs do + subject.execute + end + + should_email(user) + end + + it 'uses notification service to send email to the user' do + expect_next_instance_of(NotificationService) do |notification_service| + expect(notification_service).to receive(notification_method).with(key.user, [key.fingerprint]) + end + + subject.execute + end + + it 'creates todo' do + expect(TodoService).to receive(:new).and_call_original + + perform_enqueued_jobs do + expect { subject.execute }.to change { user.todos.count }.by(1) + end + end + + it 'updates notified column' do + expect { subject.execute }.to change { key.reload.public_send(notified_column) } + end end - shared_examples 'suppresses notifications and todos' do |column| - it 'does not send emails, create todos, or update notification timestamps' do + shared_examples 'does not send notification' do |notified_column| + it 'does not send email to the user', :aggregate_failures do expect(NotificationService).not_to receive(:new) + + perform_enqueued_jobs do + subject.execute + end + + should_not_email(user) + end + + it 'does not create todo', :aggregate_failures do expect(TodoService).not_to receive(:new) - expect do - service.execute - end.not_to change { Todo.count } + perform_enqueued_jobs do + expect { subject.execute }.not_to change { user.todos.count } + end + end - expect(keys.first.reload.public_send(column)).to be_nil + it 'does not update notified column' do + expect { subject.execute }.not_to change { key.reload.public_send(notified_column) } end end - describe '#execute' do - context 'when keys are expiring soon' do - let(:params) { { keys: user.keys.where(id: expiring_key), expiring_soon: true } } - let(:keys) { [expiring_key] } + context 'for enterprise users', :saas do + before do + stub_licensed_features(disable_ssh_keys: true) + stub_saas_features(disable_ssh_keys: true) + end + + let_it_be_with_reload(:group) { create(:group) } + let_it_be_with_reload(:user) { create(:enterprise_user, enterprise_group: group) } + + context 'with key expiring today', :mailer do + let_it_be_with_reload(:key) { create(:key, :expired_today, user: user) } - it_behaves_like 'suppresses notifications and todos', :before_expiry_notification_delivered_at + let(:expiring_soon) { false } + + context 'when user has permission to receive notification' do + it_behaves_like 'sends a notification', :ssh_key_expired, :expiry_notification_delivered_at + + context 'when SSH Keys are disabled by the group' do + before do + group.namespace_settings.update!(disable_ssh_keys: true) + end + + it_behaves_like 'does not send notification', :expiry_notification_delivered_at + end + end end - context 'when keys have expired' do - let(:params) { { keys: user.keys.where(id: expired_key), expiring_soon: false } } - let(:keys) { [expired_key] } + context 'with key expiring soon', :mailer do + let_it_be_with_reload(:key) { create(:key, expires_at: 3.days.from_now, user: user) } + + let(:expiring_soon) { true } + + context 'when user has permission to receive notification' do + it_behaves_like 'sends a notification', :ssh_key_expiring_soon, :before_expiry_notification_delivered_at + + context 'when SSH Keys are disabled by the group' do + before do + group.namespace_settings.update!(disable_ssh_keys: true) + end - it_behaves_like 'suppresses notifications and todos', :expiry_notification_delivered_at + it_behaves_like 'does not send notification', :before_expiry_notification_delivered_at + end + end end end end diff --git a/ee/spec/services/ee/notification_service_spec.rb b/ee/spec/services/ee/notification_service_spec.rb index a69a8f9fc64708..09510255863ea0 100644 --- a/ee/spec/services/ee/notification_service_spec.rb +++ b/ee/spec/services/ee/notification_service_spec.rb @@ -20,48 +20,6 @@ end end - describe 'SSH keys' do - let(:fingerprints) { ['aa:bb:cc'] } - - describe '#new_key' do - let(:key) { build_stubbed(:personal_key, user: build_stubbed(:user)) } - - before do - allow(key.user).to receive(:ssh_keys_disabled?).and_return(true) - end - - it 'does not send email when SSH keys are disabled' do - expect { subject.new_key(key) }.not_to have_enqueued_email(key.id, mail: 'new_ssh_key_email') - end - end - - describe '#ssh_key_expired' do - let(:user) { build_stubbed(:user) } - - before do - allow(user).to receive(:ssh_keys_disabled?).and_return(true) - end - - it 'does not send email when SSH keys are disabled' do - expect { subject.ssh_key_expired(user, fingerprints) } - .not_to have_enqueued_email(user, fingerprints, mail: 'ssh_key_expired_email') - end - end - - describe '#ssh_key_expiring_soon' do - let(:user) { build_stubbed(:user) } - - before do - allow(user).to receive(:ssh_keys_disabled?).and_return(true) - end - - it 'does not send email when SSH keys are disabled' do - expect { subject.ssh_key_expiring_soon(user, fingerprints) } - .not_to have_enqueued_email(user, fingerprints, mail: 'ssh_key_expiring_soon_email') - end - end - end - context 'new review' do let(:project) { create(:project, :repository) } let(:user) { create(:user) } diff --git a/ee/spec/services/todo_service_spec.rb b/ee/spec/services/todo_service_spec.rb index f04a5165e6462f..46604233e2becb 100644 --- a/ee/spec/services/todo_service_spec.rb +++ b/ee/spec/services/todo_service_spec.rb @@ -15,23 +15,6 @@ let(:skip_users) { [skipped] } let(:service) { described_class.new } - describe 'SSH keys' do - let_it_be(:enterprise_user) { create(:user) } - let_it_be(:ssh_key) { create(:key, user: enterprise_user) } - - before do - allow(enterprise_user).to receive(:ssh_keys_disabled?).and_return(true) - end - - it 'does not create todos for expiring soon keys' do - expect { service.ssh_key_expiring_soon(ssh_key) }.not_to change { Todo.count } - end - - it 'does not create todos for expired keys' do - expect { service.ssh_key_expired(ssh_key) }.not_to change { Todo.count } - end - end - describe 'Epics' do let(:users) { [author, non_member, member, guest, admin, john_doe, skipped] } let(:mentions) { users.map(&:to_reference).join(' ') } diff --git a/lib/api/users.rb b/lib/api/users.rb index e98f5f898a9ed7..467416e8ee8e34 100644 --- a/lib/api/users.rb +++ b/lib/api/users.rb @@ -45,12 +45,6 @@ def custom_order_by_or_sort? params[:order_by].present? || params[:sort].present? end - # overwritten in EE - def ensure_ssh_keys_visible!(_user); end - - # overwritten in EE - def ensure_ssh_keys_mutable!(_user); end - # rubocop: disable CodeReuse/ActiveRecord def reorder_users(users) # Users#search orders by exact matches and handles pagination, @@ -553,8 +547,6 @@ def reorder_users(users) user = User.find_by(id: params.delete(:user_id)) not_found!('User') unless user - ensure_ssh_keys_mutable!(user) - key = ::Keys::CreateService.new(current_user, declared_params(include_missing: false).merge(user: user, organization: Current.organization)).execute if key.persisted? @@ -578,8 +570,6 @@ def reorder_users(users) user = find_user(params[:user_id]) not_found!('User') unless user && can?(current_user, :read_user, user) - ensure_ssh_keys_visible!(user) - keys = user.keys.preload_users present paginate(keys), with: Entities::SSHKey end @@ -597,8 +587,6 @@ def reorder_users(users) user = find_user(params[:id]) not_found!('User') unless user && can?(current_user, :read_user, user) - ensure_ssh_keys_visible!(user) - key = user.keys.find_by(id: params[:key_id]) # rubocop: disable CodeReuse/ActiveRecord not_found!('Key') unless key @@ -619,8 +607,6 @@ def reorder_users(users) user = User.find_by(id: params[:id]) not_found!('User') unless user - ensure_ssh_keys_mutable!(user) - key = user.keys.find_by(id: params[:key_id]) not_found!('Key') unless key @@ -1199,8 +1185,6 @@ def set_user_status(include_missing_params:) use :pagination end get "keys", feature_category: :system_access do - ensure_ssh_keys_visible!(current_user) - keys = current_user.keys.preload_users present paginate(keys), with: Entities::SSHKey @@ -1214,8 +1198,6 @@ def set_user_status(include_missing_params:) end # rubocop: disable CodeReuse/ActiveRecord get "keys/:key_id", feature_category: :system_access do - ensure_ssh_keys_visible!(current_user) - key = current_user.keys.find_by(id: params[:key_id]) not_found!('Key') unless key @@ -1234,8 +1216,6 @@ def set_user_status(include_missing_params:) desc: 'Scope of usage for the SSH key' end post "keys", feature_category: :system_access do - ensure_ssh_keys_mutable!(current_user) - key = ::Keys::CreateService.new(current_user, declared_params(include_missing: false)).execute if key.persisted? @@ -1253,8 +1233,6 @@ def set_user_status(include_missing_params:) end # rubocop: disable CodeReuse/ActiveRecord delete "keys/:key_id", feature_category: :system_access do - ensure_ssh_keys_mutable!(current_user) - key = current_user.keys.find_by(id: params[:key_id]) not_found!('Key') unless key diff --git a/lib/gitlab/git_access.rb b/lib/gitlab/git_access.rb index 977deb56ccffc4..5031788ad02322 100644 --- a/lib/gitlab/git_access.rb +++ b/lib/gitlab/git_access.rb @@ -189,7 +189,7 @@ def check_custom_ssh_action! end def check_for_console_messages - return console_messages unless key? + return console_messages unless key? || deploy_key? key_status = Gitlab::Auth::KeyStatusChecker.new(actor) @@ -205,12 +205,12 @@ def console_messages end def check_valid_actor! - return unless key? - - if !actor.valid? - raise ForbiddenError, "Your SSH key #{actor.errors[:key].first}." - elsif actor.expired? - raise ForbiddenError, "Your SSH key has expired." + if key? || deploy_key? + if !actor.valid? + raise ForbiddenError, "Your SSH key #{actor.errors[:key].first}." + elsif actor.expired? + raise ForbiddenError, "Your SSH key has expired." + end end end @@ -414,7 +414,7 @@ def ci? end def key? - actor.is_a?(Key) + actor.is_a?(Key) && actor.regular_key? end def can_read_project? diff --git a/spec/lib/gitlab/git_access_spec.rb b/spec/lib/gitlab/git_access_spec.rb index 4929f4ed5db104..28ac14529d4b77 100644 --- a/spec/lib/gitlab/git_access_spec.rb +++ b/spec/lib/gitlab/git_access_spec.rb @@ -255,7 +255,9 @@ def disable_protocol(protocol) end context 'key is expired' do - let(:actor) { create(:deploy_key, :expired) } + before do + actor.expires_at = 2.days.ago + end it 'does not allow expired keys' do expect { pull_access_check }.to raise_forbidden('Your SSH key has expired.') @@ -280,7 +282,7 @@ def disable_protocol(protocol) stub_application_setting(rsa_key_restriction: ApplicationSetting::FORBIDDEN_KEY_VALUE) end - it 'does not allow keys which are too small' do + it 'does not allow keys which are forbidden' do expect(actor).not_to be_valid expect { pull_access_check }.to raise_forbidden(/Your SSH key type is forbidden/) expect { push_access_check }.to raise_forbidden(/Your SSH key type is forbidden/) @@ -289,7 +291,7 @@ def disable_protocol(protocol) end it_behaves_like '#check with a key that is not valid' do - let(:actor) { build(:deploy_key, user: user) } + let(:actor) { build(:rsa_key_2048, user: user) } end it_behaves_like '#check with a key that is not valid' do diff --git a/spec/models/key_spec.rb b/spec/models/key_spec.rb index c38c511de42c62..f8ac04df0b076f 100644 --- a/spec/models/key_spec.rb +++ b/spec/models/key_spec.rb @@ -296,14 +296,35 @@ end describe '.regular_keys' do - let_it_be(:personal_key) { create(:personal_key) } - let_it_be(:key) { create(:key) } + let_it_be(:personal_key) { create(:personal_key, type: nil) } + let_it_be(:key) { create(:key, type: 'Key') } let_it_be(:deploy_key) { create(:deploy_key) } - it 'does not return keys of type DeployKey' do - expect(described_class.all).to match_array([personal_key, key, deploy_key]) - expect(described_class.regular_keys).to match_array([personal_key, key]) + it 'includes keys with nil type' do + expect(described_class.regular_keys).to include(personal_key) end + + it "includes keys with 'Key' type" do + expect(described_class.regular_keys).to include(key) + end + + it "does not include keys with 'DeployKey' type" do + expect(described_class.regular_keys).not_to include(deploy_key) + end + end + end + + describe '.regular_key_types' do + it 'includes nil' do + expect(described_class.regular_key_types).to include(nil) + end + + it "includes 'Key'" do + expect(described_class.regular_key_types).to include('Key') + end + + it "does not include 'DeployKey'" do + expect(described_class.regular_key_types).not_to include('DeployKey') end end @@ -528,4 +549,24 @@ expect(build(:key, usage_type: :auth)).not_to be_signing end end + + describe '#regular_key?' do + context 'when type is nil' do + it 'returns true' do + expect(build(:key, type: nil).regular_key?).to be(true) + end + end + + context "when type is 'Key'" do + it 'returns true' do + expect(build(:key, type: 'Key').regular_key?).to be(true) + end + end + + context "when type is 'DeployKey'" do + it 'returns false' do + expect(build(:deploy_key).regular_key?).to be(false) + end + end + end end diff --git a/spec/requests/api/users_spec.rb b/spec/requests/api/users_spec.rb index f108e73c9791dc..632b7ae94e2291 100644 --- a/spec/requests/api/users_spec.rb +++ b/spec/requests/api/users_spec.rb @@ -2594,7 +2594,7 @@ def update_password(user, admin, password = User.random_password) end end - describe 'GET /user/:id/keys' do + describe 'GET /users/:id/keys' do subject(:request) { get api(path) } let(:path) { "/users/#{user.id}/keys" } @@ -2679,7 +2679,7 @@ def request_with_second_scope end end - describe 'GET /user/:user_id/keys' do + describe 'GET /users/:username/keys' do let(:path) { "/users/#{user.username}/keys" } before do @@ -2717,7 +2717,7 @@ def request_with_second_scope end end - describe 'GET /user/:id/keys/:key_id' do + describe 'GET /users/:id/keys/:key_id' do let(:path) { "/users/#{user.id}/keys/#{key.id}" } it 'gets existing key' do @@ -2777,7 +2777,7 @@ def request_with_second_scope end end - describe 'DELETE /user/:id/keys/:key_id' do + describe 'DELETE /users/:id/keys/:key_id' do let(:path) { "/users/#{user.id}/keys/#{key.id}" } it_behaves_like 'DELETE request permissions for admin mode' -- GitLab