From d1e0cc8329b4bf146a833e4ad7f5341cac693161 Mon Sep 17 00:00:00 2001 From: smriti Date: Wed, 13 Nov 2024 11:07:04 +0530 Subject: [PATCH 1/3] Removed feature flag With this MR we are removing FF allow_top_level_group_owners_to_create_service_accounts FF is enabled on production environment for some considerable time now Changelog: removed --- .../settings/account_and_limit_settings.md | 1 + ee/app/policies/ee/group_policy.rb | 6 +-- ...roup_owners_to_create_service_accounts.yml | 9 ---- ee/spec/policies/group_policy_spec.rb | 28 +---------- .../api/group_service_accounts_spec.rb | 37 +------------- .../service_accounts/create_service_spec.rb | 48 ++++++------------- .../service_accounts/delete_service_spec.rb | 42 +++++----------- 7 files changed, 31 insertions(+), 140 deletions(-) delete mode 100644 ee/config/feature_flags/gitlab_com_derisk/allow_top_level_group_owners_to_create_service_accounts.yml diff --git a/doc/administration/settings/account_and_limit_settings.md b/doc/administration/settings/account_and_limit_settings.md index cbd6b0f6a4bdca..fb0ff9d4a563b0 100644 --- a/doc/administration/settings/account_and_limit_settings.md +++ b/doc/administration/settings/account_and_limit_settings.md @@ -248,6 +248,7 @@ DETAILS: **Offering:** Self-managed > - [Introduced](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/163726) in GitLab 17.5 [with a feature flag](../feature_flags.md) named `allow_top_level_group_owners_to_create_service_accounts` for GitLab self-managed. Disabled by default. +> - Feature flags `allow_top_level_group_owners_to_create_service_accounts` [removed](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/125835) in GitLab 17.6. FLAG: On GitLab self-managed, by default this feature is not available. To make it available, an administrator can [enable the feature flag](../feature_flags.md) named `allow_top_level_group_owners_to_create_service_accounts`. On GitLab.com, this feature is available. diff --git a/ee/app/policies/ee/group_policy.rb b/ee/app/policies/ee/group_policy.rb index a5e5f29b945562..b9e6b5a892c58e 100644 --- a/ee/app/policies/ee/group_policy.rb +++ b/ee/app/policies/ee/group_policy.rb @@ -221,11 +221,7 @@ module GroupPolicy end condition(:allow_top_level_group_owners_to_create_service_accounts, scope: :global) do - if ::Feature.enabled?(:allow_top_level_group_owners_to_create_service_accounts, @user) - ::Gitlab::CurrentSettings.current_application_settings.allow_top_level_group_owners_to_create_service_accounts? - else - is_gitlab_com? - end + ::Gitlab::CurrentSettings.current_application_settings.allow_top_level_group_owners_to_create_service_accounts? end MemberRole.all_customizable_group_permissions.each do |ability| diff --git a/ee/config/feature_flags/gitlab_com_derisk/allow_top_level_group_owners_to_create_service_accounts.yml b/ee/config/feature_flags/gitlab_com_derisk/allow_top_level_group_owners_to_create_service_accounts.yml deleted file mode 100644 index e079ffd813fd3e..00000000000000 --- a/ee/config/feature_flags/gitlab_com_derisk/allow_top_level_group_owners_to_create_service_accounts.yml +++ /dev/null @@ -1,9 +0,0 @@ ---- -name: allow_top_level_group_owners_to_create_service_accounts -feature_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/468806 -introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/163726 -rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/482400 -milestone: '17.5' -group: group::authentication -type: gitlab_com_derisk -default_enabled: false diff --git a/ee/spec/policies/group_policy_spec.rb b/ee/spec/policies/group_policy_spec.rb index 3581ef5502de74..3a711aebafa474 100644 --- a/ee/spec/policies/group_policy_spec.rb +++ b/ee/spec/policies/group_policy_spec.rb @@ -3328,9 +3328,9 @@ def expect_private_group_permissions_as_if_non_member context 'when the user is an owner' do let(:current_user) { owner } - context 'when allow_top_level_group_owners_to_create_service_accounts is disabled' do + context 'when application setting is disabled' do before do - stub_feature_flags(allow_top_level_group_owners_to_create_service_accounts: false) + stub_ee_application_setting(allow_top_level_group_owners_to_create_service_accounts: false) end it { is_expected.to be_allowed(:admin_service_accounts) } @@ -3339,34 +3339,10 @@ def expect_private_group_permissions_as_if_non_member it { is_expected.to be_disallowed(:delete_service_account) } context 'when saas', :saas do - it { is_expected.to be_allowed(:admin_service_accounts) } - it { is_expected.to be_allowed(:admin_service_account_member) } - it { is_expected.to be_allowed(:create_service_account) } - it { is_expected.to be_allowed(:delete_service_account) } - end - end - - context 'when allow_top_level_group_owners_to_create_service_accounts FF is enabled' do - before do - stub_feature_flags(allow_top_level_group_owners_to_create_service_accounts: true) - end - - context 'when application setting is disabled' do - before do - stub_ee_application_setting(allow_top_level_group_owners_to_create_service_accounts: false) - end - it { is_expected.to be_allowed(:admin_service_accounts) } it { is_expected.to be_allowed(:admin_service_account_member) } it { is_expected.to be_disallowed(:create_service_account) } it { is_expected.to be_disallowed(:delete_service_account) } - - context 'when saas', :saas do - it { is_expected.to be_allowed(:admin_service_accounts) } - it { is_expected.to be_allowed(:admin_service_account_member) } - it { is_expected.to be_disallowed(:create_service_account) } - it { is_expected.to be_disallowed(:delete_service_account) } - end end context 'when application setting is enabled ' do diff --git a/ee/spec/requests/api/group_service_accounts_spec.rb b/ee/spec/requests/api/group_service_accounts_spec.rb index ef5554e393a5a2..0407e064e4e4f8 100644 --- a/ee/spec/requests/api/group_service_accounts_spec.rb +++ b/ee/spec/requests/api/group_service_accounts_spec.rb @@ -235,52 +235,17 @@ it_behaves_like "service account user creation" end - context 'when current user is an owner and feature is activated and allow top level setting activated' do + context 'when current user is an owner and allow top level setting activated' do let_it_be(:user) { create(:user) } before do group.add_owner(user) stub_ee_application_setting(allow_top_level_group_owners_to_create_service_accounts: true) - stub_feature_flags(allow_top_level_group_owners_to_create_service_accounts: true) end it_behaves_like "service account user creation" end - context 'when current user is an owner and feature is deactivated in GitLab.com', :saas do - let_it_be(:user) { create(:user) } - let(:hosted_plan) { create(:ultimate_plan) } - - before do - group.add_owner(user) - stub_feature_flags(allow_top_level_group_owners_to_create_service_accounts: false) - create(:gitlab_subscription, namespace: group, hosted_plan: hosted_plan) - stub_application_setting(check_namespace_plan: true) - end - - it_behaves_like "service account user creation" - end - - context 'when current user is an owner and feature is deactivated and allow top level setting activated' do - let_it_be(:user) { create(:user) } - let(:group_id) { group.id } - - before do - group.add_owner(user) - stub_ee_application_setting(allow_top_level_group_owners_to_create_service_accounts: true) - stub_feature_flags(allow_top_level_group_owners_to_create_service_accounts: false) - end - - it 'returns error' do - perform_request - - expect(response).to have_gitlab_http_status(:bad_request) - expect(json_response['message']).to include( - s_('ServiceAccount|User does not have permission to create a service account in this namespace.') - ) - end - end - context 'when current user is an owner and allow top level setting deactivated' do let_it_be(:user) { create(:user) } let(:group_id) { group.id } diff --git a/ee/spec/services/namespaces/service_accounts/create_service_spec.rb b/ee/spec/services/namespaces/service_accounts/create_service_spec.rb index bc82bcc5e81fc6..79a4fa14516797 100644 --- a/ee/spec/services/namespaces/service_accounts/create_service_spec.rb +++ b/ee/spec/services/namespaces/service_accounts/create_service_spec.rb @@ -129,61 +129,41 @@ context 'when group owner' do let_it_be(:current_user) { create(:user, owner_of: group) } - context 'when allow_top_level_group_owners_to_create_service_accounts FF is disabled' do + context 'when application setting is disabled' do before do - stub_feature_flags(allow_top_level_group_owners_to_create_service_accounts: false) + stub_ee_application_setting(allow_top_level_group_owners_to_create_service_accounts: false) end it_behaves_like 'service account creation failure' context 'when saas', :saas do - it_behaves_like 'service account creation success' do - let(:username_prefix) { "service_account_group_#{group.id}" } - end + it_behaves_like 'service account creation failure' end end - context 'when allow_top_level_group_owners_to_create_service_accounts FF is enabled' do + context 'when application setting is enabled' do before do - stub_feature_flags(allow_top_level_group_owners_to_create_service_accounts: true) + stub_ee_application_setting(allow_top_level_group_owners_to_create_service_accounts: true) end - context 'when application setting is disabled' do - before do - stub_ee_application_setting(allow_top_level_group_owners_to_create_service_accounts: false) - end - - it_behaves_like 'service account creation failure' - - context 'when saas', :saas do - it_behaves_like 'service account creation failure' - end + it_behaves_like 'service account creation success' do + let(:username_prefix) { "service_account_group_#{group.id}" } end - context 'when application setting is enabled' do - before do - stub_ee_application_setting(allow_top_level_group_owners_to_create_service_accounts: true) - end - + context 'when saas', :saas do it_behaves_like 'service account creation success' do let(:username_prefix) { "service_account_group_#{group.id}" } end + end - context 'when saas', :saas do - it_behaves_like 'service account creation success' do - let(:username_prefix) { "service_account_group_#{group.id}" } - end - end + # setting is only applicable for top level group + context 'when the group is subgroup' do + let(:namespace_id) { subgroup.id } - # setting is only applicable for top level group - context 'when the group is subgroup' do - let(:namespace_id) { subgroup.id } + it_behaves_like 'service account creation failure' + context 'when saas', :saas do it_behaves_like 'service account creation failure' - - context 'when saas', :saas do - it_behaves_like 'service account creation failure' - end end end end diff --git a/ee/spec/services/namespaces/service_accounts/delete_service_spec.rb b/ee/spec/services/namespaces/service_accounts/delete_service_spec.rb index a573ebe3055696..972fb4ead32c94 100644 --- a/ee/spec/services/namespaces/service_accounts/delete_service_spec.rb +++ b/ee/spec/services/namespaces/service_accounts/delete_service_spec.rb @@ -63,52 +63,34 @@ context 'when group owner' do let_it_be(:current_user) { create(:user, owner_of: group) } - context 'when allow_top_level_group_owners_to_create_service_accounts FF is disabled' do + context 'when setting is off' do before do - stub_feature_flags(allow_top_level_group_owners_to_create_service_accounts: false) + stub_ee_application_setting(allow_top_level_group_owners_to_create_service_accounts: false) end it_behaves_like 'service account deletion failure' context 'when saas', :saas do - it_behaves_like 'service account deletion is success' + it_behaves_like 'service account deletion failure' end end - context 'when allow_top_level_group_owners_to_create_service_accounts is enabled' do + context 'when setting is on' do before do - stub_feature_flags(allow_top_level_group_owners_to_create_service_accounts: true) + stub_ee_application_setting(allow_top_level_group_owners_to_create_service_accounts: true) end - context 'when setting is off' do - before do - stub_ee_application_setting(allow_top_level_group_owners_to_create_service_accounts: false) - end - - it_behaves_like 'service account deletion failure' - - context 'when saas', :saas do - it_behaves_like 'service account deletion failure' - end - end - - context 'when setting is on' do - before do - stub_ee_application_setting(allow_top_level_group_owners_to_create_service_accounts: true) - end + it_behaves_like 'service account deletion is success' + context 'when saas', :saas do it_behaves_like 'service account deletion is success' + end - context 'when saas', :saas do - it_behaves_like 'service account deletion is success' - end - - context 'when its a subgroup' do - let_it_be(:group) { create(:group, :private, parent: create(:group)) } - let_it_be(:delete_user) { create(:service_account, provisioned_by_group: group) } + context 'when its a subgroup' do + let_it_be(:group) { create(:group, :private, parent: create(:group)) } + let_it_be(:delete_user) { create(:service_account, provisioned_by_group: group) } - it_behaves_like 'service account deletion failure' - end + it_behaves_like 'service account deletion failure' end end end -- GitLab From 2ee8fa5b2ebcee280a80dfb16422ad573da3dd27 Mon Sep 17 00:00:00 2001 From: smriti Date: Wed, 13 Nov 2024 17:11:40 +0530 Subject: [PATCH 2/3] Spec failures resolved --- ee/spec/policies/group_policy_spec.rb | 74 +++++++++--------- .../api/group_service_accounts_spec.rb | 78 +++++++------------ .../auth/two_factor_auth_verifier_spec.rb | 2 +- 3 files changed, 68 insertions(+), 86 deletions(-) diff --git a/ee/spec/policies/group_policy_spec.rb b/ee/spec/policies/group_policy_spec.rb index 3a711aebafa474..43764daf501a5d 100644 --- a/ee/spec/policies/group_policy_spec.rb +++ b/ee/spec/policies/group_policy_spec.rb @@ -3328,7 +3328,7 @@ def expect_private_group_permissions_as_if_non_member context 'when the user is an owner' do let(:current_user) { owner } - context 'when application setting is disabled' do + context 'when application setting allow_top_level_group_owners_to_create_service_accounts is disabled' do before do stub_ee_application_setting(allow_top_level_group_owners_to_create_service_accounts: false) end @@ -3344,56 +3344,56 @@ def expect_private_group_permissions_as_if_non_member it { is_expected.to be_disallowed(:create_service_account) } it { is_expected.to be_disallowed(:delete_service_account) } end + end - context 'when application setting is enabled ' do - before do - stub_ee_application_setting(allow_top_level_group_owners_to_create_service_accounts: true) - end + context 'when application setting allow_top_level_group_owners_to_create_service_accounts is enabled ' do + before do + stub_ee_application_setting(allow_top_level_group_owners_to_create_service_accounts: true) + end + it { is_expected.to be_allowed(:admin_service_accounts) } + it { is_expected.to be_allowed(:admin_service_account_member) } + it { is_expected.to be_allowed(:create_service_account) } + it { is_expected.to be_allowed(:delete_service_account) } + + context 'when saas', :saas do it { is_expected.to be_allowed(:admin_service_accounts) } it { is_expected.to be_allowed(:admin_service_account_member) } it { is_expected.to be_allowed(:create_service_account) } it { is_expected.to be_allowed(:delete_service_account) } - context 'when saas', :saas do - it { is_expected.to be_allowed(:admin_service_accounts) } - it { is_expected.to be_allowed(:admin_service_account_member) } - it { is_expected.to be_allowed(:create_service_account) } - it { is_expected.to be_allowed(:delete_service_account) } - - context 'when trial is active' do - before do - allow(group).to receive_messages(trial_active?: true) - end - - it { is_expected.to be_disallowed(:admin_service_accounts) } - it { is_expected.to be_disallowed(:admin_service_account_member) } - it { is_expected.to be_disallowed(:create_service_account) } - it { is_expected.to be_disallowed(:delete_service_account) } - end - end - - context 'when a trial is active' do + context 'when trial is active' do before do - allow(group).to receive_messages(gitlab_subscription: nil) + allow(group).to receive_messages(trial_active?: true) end - it { is_expected.to be_allowed(:admin_service_accounts) } - it { is_expected.to be_allowed(:admin_service_account_member) } - it { is_expected.to be_allowed(:create_service_account) } - it { is_expected.to be_allowed(:delete_service_account) } + it { is_expected.to be_disallowed(:admin_service_accounts) } + it { is_expected.to be_disallowed(:admin_service_account_member) } + it { is_expected.to be_disallowed(:create_service_account) } + it { is_expected.to be_disallowed(:delete_service_account) } + end + end + + context 'when a trial is active' do + before do + allow(group).to receive_messages(gitlab_subscription: nil) end - context 'for subgroup' do - let_it_be(:subgroup) { create(:group, :private, parent: group) } + it { is_expected.to be_allowed(:admin_service_accounts) } + it { is_expected.to be_allowed(:admin_service_account_member) } + it { is_expected.to be_allowed(:create_service_account) } + it { is_expected.to be_allowed(:delete_service_account) } + end - subject { described_class.new(current_user, subgroup) } + context 'for subgroup' do + let_it_be(:subgroup) { create(:group, :private, parent: group) } - it { is_expected.to be_allowed(:admin_service_accounts) } - it { is_expected.to be_allowed(:admin_service_account_member) } - it { is_expected.to be_disallowed(:create_service_account) } - it { is_expected.to be_disallowed(:delete_service_account) } - end + subject { described_class.new(current_user, subgroup) } + + it { is_expected.to be_allowed(:admin_service_accounts) } + it { is_expected.to be_allowed(:admin_service_account_member) } + it { is_expected.to be_disallowed(:create_service_account) } + it { is_expected.to be_disallowed(:delete_service_account) } end end diff --git a/ee/spec/requests/api/group_service_accounts_spec.rb b/ee/spec/requests/api/group_service_accounts_spec.rb index 0407e064e4e4f8..7ba8fcef2a18f2 100644 --- a/ee/spec/requests/api/group_service_accounts_spec.rb +++ b/ee/spec/requests/api/group_service_accounts_spec.rb @@ -235,33 +235,47 @@ it_behaves_like "service account user creation" end - context 'when current user is an owner and allow top level setting activated' do + context 'when current user is an owner' do let_it_be(:user) { create(:user) } before do group.add_owner(user) - stub_ee_application_setting(allow_top_level_group_owners_to_create_service_accounts: true) end - it_behaves_like "service account user creation" - end + context 'when allow top level setting is activated' do + before do + stub_ee_application_setting(allow_top_level_group_owners_to_create_service_accounts: true) + end - context 'when current user is an owner and allow top level setting deactivated' do - let_it_be(:user) { create(:user) } - let(:group_id) { group.id } + it_behaves_like "service account user creation" - before do - group.add_owner(user) - stub_ee_application_setting(allow_top_level_group_owners_to_create_service_accounts: false) + context 'when in GitLab.com', :saas do + let(:hosted_plan) { create(:ultimate_plan) } + + before do + create(:gitlab_subscription, namespace: group, hosted_plan: hosted_plan) + stub_application_setting(check_namespace_plan: true) + end + + it_behaves_like "service account user creation" + end end - it 'returns error' do - perform_request + context 'when allow top level setting is deactivated' do + let(:group_id) { group.id } - expect(response).to have_gitlab_http_status(:bad_request) - expect(json_response['message']).to include( - s_('ServiceAccount|User does not have permission to create a service account in this namespace.') - ) + before do + stub_ee_application_setting(allow_top_level_group_owners_to_create_service_accounts: false) + end + + it 'returns error' do + perform_request + + expect(response).to have_gitlab_http_status(:bad_request) + expect(json_response['message']).to include( + s_('ServiceAccount|User does not have permission to create a service account in this namespace.') + ) + end end end end @@ -414,41 +428,10 @@ it_behaves_like "service account user deletion" - it "is not available for group owners when allow top level group owners feature is disabled on Self Managed", - :sidekiq_inline do - group.add_owner(user) - - stub_feature_flags(allow_top_level_group_owners_to_create_service_accounts: false) - perform_enqueued_jobs { delete api(path, user) } - expect(response).to have_gitlab_http_status(:no_content) - expect(Users::GhostUserMigration.where(user: service_account_user, initiator_user: user)).not_to exist - end - - it "is not available for group owners when allow top level group owners feature is disabled", :sidekiq_inline do - group.add_owner(user) - - stub_feature_flags(allow_top_level_group_owners_to_create_service_accounts: true) - stub_ee_application_setting(allow_top_level_group_owners_to_create_service_accounts: false) - perform_enqueued_jobs { delete api(path, user) } - expect(response).to have_gitlab_http_status(:no_content) - expect(Users::GhostUserMigration.where(user: service_account_user, initiator_user: user)).not_to exist - end - - it "is available for group owners when allow top level group owners feature is disabled on GitLab.com", - :sidekiq_inline, :saas do - group.add_owner(user) - - stub_feature_flags(allow_top_level_group_owners_to_create_service_accounts: false) - perform_enqueued_jobs { delete api(path, user) } - expect(response).to have_gitlab_http_status(:no_content) - expect(Users::GhostUserMigration.where(user: service_account_user, initiator_user: user)).to exist - end - it "is available for group owners when allow top level group owners application setting is enabled", :sidekiq_inline, :saas do group.add_owner(user) - stub_feature_flags(allow_top_level_group_owners_to_create_service_accounts: true) stub_ee_application_setting(allow_top_level_group_owners_to_create_service_accounts: true) perform_enqueued_jobs { delete api(path, user) } expect(response).to have_gitlab_http_status(:no_content) @@ -457,7 +440,6 @@ it "is not available to non group owners" do group.add_maintainer(user) - stub_feature_flags(allow_top_level_group_owners_to_create_service_accounts: true) stub_ee_application_setting(allow_top_level_group_owners_to_create_service_accounts: true) perform_enqueued_jobs { delete api(path, user) } diff --git a/spec/lib/gitlab/auth/two_factor_auth_verifier_spec.rb b/spec/lib/gitlab/auth/two_factor_auth_verifier_spec.rb index bccddaa50a0d4b..9fd57f80cbf604 100644 --- a/spec/lib/gitlab/auth/two_factor_auth_verifier_spec.rb +++ b/spec/lib/gitlab/auth/two_factor_auth_verifier_spec.rb @@ -86,7 +86,7 @@ with_them do before do allow(request).to receive(:session).and_return(session) - stub_feature_flags(bypass_two_factor: false) + stub_feature_flags(by_pass_two_factor_for_current_session: false) stub_application_setting(require_two_factor_authentication: instance_level_enabled) allow(user).to receive(:require_two_factor_authentication_from_group?).and_return(group_level_enabled) session[:provider_2FA] = provider_2FA -- GitLab From ac29e6a2b7fb3e4bc3ece1263329f10b42b461e4 Mon Sep 17 00:00:00 2001 From: Smriti Garg Date: Wed, 13 Nov 2024 17:35:51 +0000 Subject: [PATCH 3/3] Doc suggestion applid --- doc/administration/settings/account_and_limit_settings.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc/administration/settings/account_and_limit_settings.md b/doc/administration/settings/account_and_limit_settings.md index fb0ff9d4a563b0..f4daddc58ed73e 100644 --- a/doc/administration/settings/account_and_limit_settings.md +++ b/doc/administration/settings/account_and_limit_settings.md @@ -248,7 +248,7 @@ DETAILS: **Offering:** Self-managed > - [Introduced](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/163726) in GitLab 17.5 [with a feature flag](../feature_flags.md) named `allow_top_level_group_owners_to_create_service_accounts` for GitLab self-managed. Disabled by default. -> - Feature flags `allow_top_level_group_owners_to_create_service_accounts` [removed](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/125835) in GitLab 17.6. +> - [Generally available](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/125835) in GitLab 17.6. Feature flag `allow_top_level_group_owners_to_create_service_accounts` removed. FLAG: On GitLab self-managed, by default this feature is not available. To make it available, an administrator can [enable the feature flag](../feature_flags.md) named `allow_top_level_group_owners_to_create_service_accounts`. On GitLab.com, this feature is available. -- GitLab