From 0c360b09a817f85c6126fb38732a5f2e50b4fb7b Mon Sep 17 00:00:00 2001
From: Ian Anderson <ianderson@gitlab.com>
Date: Fri, 14 Feb 2025 15:25:50 +0000
Subject: [PATCH] Add feature flag for development of admin custom roles
---
.../customizable_admin_permissions_enum.rb | 2 +-
.../types/members/customizable_permission.rb | 19 ++++++++----
ee/app/models/members/member_role.rb | 4 +++
.../wip/custom_ability_read_admin_cicd.yml | 9 ------
.../custom_ability_read_admin_monitoring.yml | 9 ------
...custom_ability_read_admin_subscription.yml | 9 ------
.../feature_flags/wip/custom_admin_roles.yml | 9 ++++++
ee/spec/models/members/member_role_spec.rb | 24 +++++++++++++++
...ustomizable_permissions_shared_examples.rb | 29 +++++++++++++++----
.../custom_roles/check_docs_task_spec.rb | 9 +++---
.../docs/templates/custom_abilities.md.erb | 3 +-
11 files changed, 83 insertions(+), 43 deletions(-)
delete mode 100644 ee/config/feature_flags/wip/custom_ability_read_admin_cicd.yml
delete mode 100644 ee/config/feature_flags/wip/custom_ability_read_admin_monitoring.yml
delete mode 100644 ee/config/feature_flags/wip/custom_ability_read_admin_subscription.yml
create mode 100644 ee/config/feature_flags/wip/custom_admin_roles.yml
diff --git a/ee/app/graphql/types/members/customizable_admin_permissions_enum.rb b/ee/app/graphql/types/members/customizable_admin_permissions_enum.rb
index 7a4df2402024c..83c1a4ec5c417 100644
--- a/ee/app/graphql/types/members/customizable_admin_permissions_enum.rb
+++ b/ee/app/graphql/types/members/customizable_admin_permissions_enum.rb
@@ -9,7 +9,7 @@ class CustomizableAdminPermissionsEnum < BaseEnum
include CustomizablePermission
MemberRole.all_customizable_admin_permissions.each_pair do |key, value|
- define_permission(key, value)
+ define_permission(key, value, feature_flag: :custom_admin_roles)
end
end
end
diff --git a/ee/app/graphql/types/members/customizable_permission.rb b/ee/app/graphql/types/members/customizable_permission.rb
index d2a31b6e6b5f0..e78359e2ac374 100644
--- a/ee/app/graphql/types/members/customizable_permission.rb
+++ b/ee/app/graphql/types/members/customizable_permission.rb
@@ -6,11 +6,8 @@ module CustomizablePermission
extend ActiveSupport::Concern
included do
- # As new custom abilities are created they are implemented behind a feature flag with a standard
- # naming convention. Since these abilities depend on the feature flag being enabled, we want to mark
- # any feature flagged abilities as experimental until they are generally released.
- def self.define_permission(name, attrs)
- if ::Feature::Definition.get("custom_ability_#{name}")
+ def self.define_permission(name, attrs, feature_flag: nil)
+ if CustomizablePermission.experimental?(name, feature_flag: feature_flag)
value name.upcase, value: name, description: attrs[:description],
experiment: { milestone: attrs[:milestone] }
else
@@ -18,6 +15,18 @@ def self.define_permission(name, attrs)
end
end
end
+
+ # As new custom abilities are created they are implemented behind a feature flag with a standard
+ # naming convention. Since these abilities depend on the feature flag being enabled, we want to mark
+ # any feature flagged abilities as experimental until they are generally released.
+ #
+ # Optionally, an additional feature flag parameter can be passed to check a feature flag that is meant
+ # to mark many custom permissions as experimental at once.
+ def self.experimental?(permission, feature_flag: nil)
+ ["custom_ability_#{permission}", feature_flag].compact.any? do |ff|
+ ::Feature::Definition.get(ff)
+ end
+ end
end
end
end
diff --git a/ee/app/models/members/member_role.rb b/ee/app/models/members/member_role.rb
index ee91f1bfcd319..8fe9211bbd05f 100644
--- a/ee/app/models/members/member_role.rb
+++ b/ee/app/models/members/member_role.rb
@@ -119,6 +119,10 @@ def customizable_permissions_exempt_from_consuming_seat
end
def permission_enabled?(permission, user = nil)
+ if ::Feature.disabled?(:custom_admin_roles, user) && all_customizable_admin_permission_keys.include?(permission)
+ return false
+ end
+
return true unless ::Feature::Definition.get("custom_ability_#{permission}")
::Feature.enabled?("custom_ability_#{permission}", user)
diff --git a/ee/config/feature_flags/wip/custom_ability_read_admin_cicd.yml b/ee/config/feature_flags/wip/custom_ability_read_admin_cicd.yml
deleted file mode 100644
index bb8fa312742de..0000000000000
--- a/ee/config/feature_flags/wip/custom_ability_read_admin_cicd.yml
+++ /dev/null
@@ -1,9 +0,0 @@
----
-name: custom_ability_read_admin_cicd
-feature_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/507960
-introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/177233
-rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/512831
-milestone: '17.9'
-group: group::authorization
-type: wip
-default_enabled: false
diff --git a/ee/config/feature_flags/wip/custom_ability_read_admin_monitoring.yml b/ee/config/feature_flags/wip/custom_ability_read_admin_monitoring.yml
deleted file mode 100644
index a18039b0eef87..0000000000000
--- a/ee/config/feature_flags/wip/custom_ability_read_admin_monitoring.yml
+++ /dev/null
@@ -1,9 +0,0 @@
----
-name: custom_ability_read_admin_monitoring
-feature_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/507960
-introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/179439
-rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/515665
-milestone: '17.9'
-group: group::authorization
-type: wip
-default_enabled: false
diff --git a/ee/config/feature_flags/wip/custom_ability_read_admin_subscription.yml b/ee/config/feature_flags/wip/custom_ability_read_admin_subscription.yml
deleted file mode 100644
index f3a494188e081..0000000000000
--- a/ee/config/feature_flags/wip/custom_ability_read_admin_subscription.yml
+++ /dev/null
@@ -1,9 +0,0 @@
----
-name: custom_ability_read_admin_subscription
-feature_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/507961
-introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/178230
-rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/514810
-milestone: '17.9'
-group: group::authorization
-type: wip
-default_enabled: false
diff --git a/ee/config/feature_flags/wip/custom_admin_roles.yml b/ee/config/feature_flags/wip/custom_admin_roles.yml
new file mode 100644
index 0000000000000..0105f5eb61174
--- /dev/null
+++ b/ee/config/feature_flags/wip/custom_admin_roles.yml
@@ -0,0 +1,9 @@
+---
+name: custom_admin_roles
+feature_issue_url: https://gitlab.com/groups/gitlab-org/-/epics/15956
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/181346
+rollout_issue_url:
+milestone: '17.9'
+group: group::authorization
+type: wip
+default_enabled: false
diff --git a/ee/spec/models/members/member_role_spec.rb b/ee/spec/models/members/member_role_spec.rb
index a3dffb31345b5..068d2a17ce1d8 100644
--- a/ee/spec/models/members/member_role_spec.rb
+++ b/ee/spec/models/members/member_role_spec.rb
@@ -440,6 +440,30 @@
it { is_expected.to eq(expected_result) }
end
+
+ context 'with an admin permission' do
+ let(:ability) { :read_admin_dashboard }
+
+ before do
+ allow(described_class).to receive_messages(
+ all_customizable_permissions: {
+ read_code: { description: 'Test permission' },
+ read_admin_dashboard: { description: 'Test admin permission' }
+ },
+ all_customizable_admin_permissions: { read_admin_dashboard: { description: 'Test admin permission' } }
+ )
+ end
+
+ it { is_expected.to be true }
+
+ context 'when the custom_admin_roles feature flag is disabled' do
+ before do
+ stub_feature_flags(custom_admin_roles: false)
+ end
+
+ it { is_expected.to be false }
+ end
+ end
end
describe '#enabled_permission_items' do
diff --git a/ee/spec/support/shared_examples/graphql/types/members/customizable_permissions_shared_examples.rb b/ee/spec/support/shared_examples/graphql/types/members/customizable_permissions_shared_examples.rb
index dace73cef4d0e..fdc64a1bc9160 100644
--- a/ee/spec/support/shared_examples/graphql/types/members/customizable_permissions_shared_examples.rb
+++ b/ee/spec/support/shared_examples/graphql/types/members/customizable_permissions_shared_examples.rb
@@ -7,15 +7,34 @@
let(:permission_attrs) { { description: 'read code', milestone: '17.10' } }
describe '.define_permission' do
- subject(:define_permission) { described_class.define_permission(permission_name, permission_attrs) }
+ let(:feature_flag) { nil }
+
+ subject(:define_permission) do
+ described_class.define_permission(permission_name, permission_attrs, feature_flag: feature_flag)
+ end
context 'for feature flagged permissions' do
- before do
- allow(::Feature::Definition).to receive(:get).with("custom_ability_#{permission_name}").and_return(true)
+ context 'for a default feature flag' do
+ before do
+ allow(::Feature::Definition).to receive(:get).with("custom_ability_#{permission_name}").and_return(true)
+ end
+
+ it 'is experimental' do
+ expect(define_permission.deprecation_reason).to include('Experiment')
+ end
end
- it 'is experimental' do
- expect(define_permission.deprecation_reason).to include('Experiment')
+ context 'for a custom feature flag' do
+ let(:feature_flag) { :custom_permission_feature_in_dev }
+
+ before do
+ allow(::Feature::Definition).to receive(:get).and_call_original
+ allow(::Feature::Definition).to receive(:get).with(feature_flag).and_return(true)
+ end
+
+ it 'is experimental' do
+ expect(define_permission.deprecation_reason).to include('Experiment')
+ end
end
end
diff --git a/ee/spec/tasks/gitlab/custom_roles/check_docs_task_spec.rb b/ee/spec/tasks/gitlab/custom_roles/check_docs_task_spec.rb
index 37b0413910820..0b7b41cdce16f 100644
--- a/ee/spec/tasks/gitlab/custom_roles/check_docs_task_spec.rb
+++ b/ee/spec/tasks/gitlab/custom_roles/check_docs_task_spec.rb
@@ -9,8 +9,9 @@
let(:docs_path) { Rails.root.join(docs_dir, 'abilities.md') }
let(:template_erb_path) { Rails.root.join("tooling/custom_roles/docs/templates/custom_abilities.md.erb") }
+ # TODO convert to all_customizable_permissions when custom_admin_roles feature flag is removed
let(:stub_definitions) do
- expect(::MemberRole).to receive(:all_customizable_permissions).and_return(updated_definitions)
+ expect(::MemberRole).to receive(:all_customizable_standard_permissions).and_return(updated_definitions)
end
describe '#run' do
@@ -26,10 +27,10 @@
} }
end
- let(:added_definition) { MemberRole.all_customizable_permissions.merge(new_ability) }
- let(:removed_definition) { MemberRole.all_customizable_permissions.except(:admin_terraform_state) }
+ let(:added_definition) { MemberRole.all_customizable_standard_permissions.merge(new_ability) }
+ let(:removed_definition) { MemberRole.all_customizable_standard_permissions.except(:admin_terraform_state) }
let(:updated_definition) do
- definitions = MemberRole.all_customizable_permissions
+ definitions = MemberRole.all_customizable_standard_permissions
definitions[:read_code][:milestone] = '12.0'
definitions
diff --git a/tooling/custom_roles/docs/templates/custom_abilities.md.erb b/tooling/custom_roles/docs/templates/custom_abilities.md.erb
index c5775e6eb6ca0..225e30557effd 100644
--- a/tooling/custom_roles/docs/templates/custom_abilities.md.erb
+++ b/tooling/custom_roles/docs/templates/custom_abilities.md.erb
@@ -1,4 +1,5 @@
-<% active_custom_abilities = MemberRole.all_customizable_permissions.filter { |p| !::Feature::Definition.get("custom_ability_#{p}") } %>
+<% # change all_customizable_standard_permissions to all_customizable_permissions when custom_admin_roles feature flag is removed%>
+<% active_custom_abilities = MemberRole.all_customizable_standard_permissions.filter { |p| !::Feature::Definition.get("custom_ability_#{p}") } %>
<% custom_abilities_by_feature_category = active_custom_abilities.group_by { |_k, definition| definition[:feature_category] } %>
<% def humanize(feature_category) %>
<% case feature_category %>
--
GitLab