From ee9e781b9404eeaaeefaebe41d6f21bcf8c0628f Mon Sep 17 00:00:00 2001
From: agius <andrew@atevans.com>
Date: Fri, 4 Oct 2024 14:36:27 -0700
Subject: [PATCH] Group settting to notify inherited members for access token
expiry
Currently when a group or project access token is expiring, we notify
direct members of the project or group. This change adds a setting which
will default to notifying both direct and inherited members about the
expiring token, which can be disabled at the group level.
Changelog: added
---
app/controllers/concerns/groups/params.rb | 1 +
app/models/namespace.rb | 9 +++
.../groups/settings/_email_settings.html.haml | 9 +++
..._notify_inherited_members_group_setting.rb | 9 +++
db/schema_migrations/20241004213405 | 1 +
db/structure.sql | 1 +
.../namespace_setting_changes_auditor_spec.rb | 2 +-
locale/gitlab.pot | 6 ++
spec/models/namespace_spec.rb | 70 +++++++++++++++++++
spec/views/groups/edit.html.haml_spec.rb | 46 ++++++++++++
10 files changed, 153 insertions(+), 1 deletion(-)
create mode 100644 db/migrate/20241004213405_add_token_expiry_notify_inherited_members_group_setting.rb
create mode 100644 db/schema_migrations/20241004213405
diff --git a/app/controllers/concerns/groups/params.rb b/app/controllers/concerns/groups/params.rb
index 7649dc971171e..eafc24524bc3f 100644
--- a/app/controllers/concerns/groups/params.rb
+++ b/app/controllers/concerns/groups/params.rb
@@ -19,6 +19,7 @@ def group_params_attributes
:emails_disabled,
:emails_enabled,
:show_diff_preview_in_email,
+ :token_expiry_notify_inherited,
:mentions_disabled,
:lfs_enabled,
:name,
diff --git a/app/models/namespace.rb b/app/models/namespace.rb
index 55de4fac24b27..4bbb5100b156b 100644
--- a/app/models/namespace.rb
+++ b/app/models/namespace.rb
@@ -173,6 +173,7 @@ class Namespace < ApplicationRecord
to: :namespace_settings
delegate :emails_enabled, :emails_enabled=,
to: :namespace_settings, allow_nil: true
+ delegate :token_expiry_notify_inherited, :token_expiry_notify_inherited=, to: :namespace_settings
delegate :allow_runner_registration_token,
:allow_runner_registration_token=,
to: :namespace_settings
@@ -618,6 +619,14 @@ def closest_setting(name)
.try(name)
end
+ def can_modify_token_expiry_notify_inherited?
+ ancestors.all?(&:token_expiry_notify_inherited)
+ end
+
+ def token_expiry_notify_inherited?
+ self_and_ancestors.all?(&:token_expiry_notify_inherited)
+ end
+
def actual_plan
Plan.default
end
diff --git a/app/views/groups/settings/_email_settings.html.haml b/app/views/groups/settings/_email_settings.html.haml
index bed7709bba785..aa4e7f4e3d0e4 100644
--- a/app/views/groups/settings/_email_settings.html.haml
+++ b/app/views/groups/settings/_email_settings.html.haml
@@ -11,3 +11,12 @@
checkbox_options: { checked: @group.show_diff_preview_in_email? & @group.emails_enabled?,
disabled: !@group.emails_enabled? || !can_set_group_diff_preview_in_email?(@group) },
help_text: s_('GroupSettings|Emails are not encrypted. Concerned administrators may want to disable diff previews.')
+
+- if Feature.enabled?(:pat_expiry_inherited_members_notification, @group.root_ancestor)
+ - controlled_by_parent_group = !@group.can_modify_token_expiry_notify_inherited?
+ .form-group
+ = f.label :token_expiry_notify_inherited, _('Expiry notification emails about group and project access tokens within this group should be sent to:')
+ = f.gitlab_ui_radio_component :token_expiry_notify_inherited, true, _('All direct and inherited members of the group or project'), radio_options: { disabled: controlled_by_parent_group }
+ = f.gitlab_ui_radio_component :token_expiry_notify_inherited, false, _('Only direct members of the group or project'), radio_options: { disabled: controlled_by_parent_group }
+ - if controlled_by_parent_group
+ .form-text= _('A parent group has selected "Only direct members." It cannot be overridden by this group.')
diff --git a/db/migrate/20241004213405_add_token_expiry_notify_inherited_members_group_setting.rb b/db/migrate/20241004213405_add_token_expiry_notify_inherited_members_group_setting.rb
new file mode 100644
index 0000000000000..dfef9c36edfac
--- /dev/null
+++ b/db/migrate/20241004213405_add_token_expiry_notify_inherited_members_group_setting.rb
@@ -0,0 +1,9 @@
+# frozen_string_literal: true
+
+class AddTokenExpiryNotifyInheritedMembersGroupSetting < Gitlab::Database::Migration[2.2]
+ milestone '17.6'
+
+ def change
+ add_column :namespace_settings, :token_expiry_notify_inherited, :boolean, default: true, null: false
+ end
+end
diff --git a/db/schema_migrations/20241004213405 b/db/schema_migrations/20241004213405
new file mode 100644
index 0000000000000..ddad2f8a7ccab
--- /dev/null
+++ b/db/schema_migrations/20241004213405
@@ -0,0 +1 @@
+fdc6b0a8a43515c7a0dd89e650e5ebcd95d78f1911dee2294a0ad891c0e9b6ce
\ No newline at end of file
diff --git a/db/structure.sql b/db/structure.sql
index b750f04065743..d55cebdf34bcb 100644
--- a/db/structure.sql
+++ b/db/structure.sql
@@ -14612,6 +14612,7 @@ CREATE TABLE namespace_settings (
spp_repository_pipeline_access boolean,
lock_spp_repository_pipeline_access boolean DEFAULT false NOT NULL,
archived boolean DEFAULT false NOT NULL,
+ token_expiry_notify_inherited boolean DEFAULT true NOT NULL,
CONSTRAINT check_0ba93c78c7 CHECK ((char_length(default_branch_name) <= 255)),
CONSTRAINT namespace_settings_unique_project_download_limit_alertlist_size CHECK ((cardinality(unique_project_download_limit_alertlist) <= 100)),
CONSTRAINT namespace_settings_unique_project_download_limit_allowlist_size CHECK ((cardinality(unique_project_download_limit_allowlist) <= 100))
diff --git a/ee/spec/lib/audit/namespace_setting_changes_auditor_spec.rb b/ee/spec/lib/audit/namespace_setting_changes_auditor_spec.rb
index 5c7a861973808..adc6fa5f9434e 100644
--- a/ee/spec/lib/audit/namespace_setting_changes_auditor_spec.rb
+++ b/ee/spec/lib/audit/namespace_setting_changes_auditor_spec.rb
@@ -121,7 +121,7 @@
default_branch_protection_defaults allow_merge_without_pipeline auto_ban_user_on_excessive_projects_download
lock_math_rendering_limits_enabled enable_auto_assign_gitlab_duo_pro_seats early_access_program_participant
lock_duo_features_enabled allow_merge_without_pipeline only_allow_merge_if_pipeline_succeeds
- lock_spp_repository_pipeline_access spp_repository_pipeline_access archived]
+ lock_spp_repository_pipeline_access spp_repository_pipeline_access archived token_expiry_notify_inherited]
columns_to_audit = Audit::NamespaceSettingChangesAuditor::EVENT_NAME_PER_COLUMN.keys.map(&:to_s)
diff --git a/locale/gitlab.pot b/locale/gitlab.pot
index 0ec0594ffb1a6..741050dd9190c 100644
--- a/locale/gitlab.pot
+++ b/locale/gitlab.pot
@@ -2089,6 +2089,9 @@ msgstr ""
msgid "A non-confidential work item cannot have a confidential parent."
msgstr ""
+msgid "A parent group has selected \"Only direct members.\" It cannot be overridden by this group."
+msgstr ""
+
msgid "A parent must be provided when bulk updating issuables"
msgstr ""
@@ -22688,6 +22691,9 @@ msgstr ""
msgid "Expiry notification emails about group and project access tokens should be sent to:"
msgstr ""
+msgid "Expiry notification emails about group and project access tokens within this group should be sent to:"
+msgstr ""
+
msgid "Explain current vulnerability."
msgstr ""
diff --git a/spec/models/namespace_spec.rb b/spec/models/namespace_spec.rb
index 1e93f5a415599..4f0ef85fc166c 100644
--- a/spec/models/namespace_spec.rb
+++ b/spec/models/namespace_spec.rb
@@ -654,6 +654,8 @@
it { is_expected.to delegate_method(:math_rendering_limits_enabled).to(:namespace_settings) }
it { is_expected.to delegate_method(:math_rendering_limits_enabled?).to(:namespace_settings) }
it { is_expected.to delegate_method(:lock_math_rendering_limits_enabled?).to(:namespace_settings) }
+ it { is_expected.to delegate_method(:token_expiry_notify_inherited).to(:namespace_settings) }
+ it { is_expected.to delegate_method(:token_expiry_notify_inherited=).to(:namespace_settings).with_arguments(:args) }
it { is_expected.to delegate_method(:add_creator).to(:namespace_details) }
it { is_expected.to delegate_method(:pending_delete).to(:namespace_details) }
it { is_expected.to delegate_method(:pending_delete=).to(:namespace_details).with_arguments(:args) }
@@ -2221,6 +2223,74 @@
end
end
+ context 'with token_expiry_notify_inherited settings' do
+ let_it_be_with_reload(:grandparent_namespace) { create(:group) }
+ let_it_be_with_reload(:parent_namespace) { create(:group, parent: grandparent_namespace) }
+ let_it_be_with_reload(:child_namespace) { create(:group, parent: parent_namespace) }
+
+ describe '.token_expiry_notify_inherited?' do
+ subject { child_namespace.token_expiry_notify_inherited? }
+
+ # setting defaults to true for all namespace settings
+ it { is_expected.to eq(true) }
+
+ context 'when parent namespace has setting disabled' do
+ before do
+ parent_namespace.namespace_settings.update!(token_expiry_notify_inherited: false)
+ end
+
+ it { is_expected.to eq(false) }
+ end
+
+ context 'when grandparent namespace has setting disabled' do
+ before do
+ grandparent_namespace.namespace_settings.update!(token_expiry_notify_inherited: false)
+ end
+
+ it { is_expected.to eq(false) }
+ end
+
+ context 'when current token_expiry_notify_inherited is set to false' do
+ before do
+ child_namespace.namespace_settings.update!(token_expiry_notify_inherited: false)
+ end
+
+ it { is_expected.to eq(false) }
+ end
+ end
+
+ describe '.can_modify_token_expiry_notify_inherited?' do
+ subject { child_namespace.can_modify_token_expiry_notify_inherited? }
+
+ # setting defaults to true for all namespace settings
+ it { is_expected.to eq(true) }
+
+ context 'when parent namespace has setting disabled' do
+ before do
+ parent_namespace.namespace_settings.update!(token_expiry_notify_inherited: false)
+ end
+
+ it { is_expected.to eq(false) }
+ end
+
+ context 'when grandparent namespace has setting disabled' do
+ before do
+ grandparent_namespace.namespace_settings.update!(token_expiry_notify_inherited: false)
+ end
+
+ it { is_expected.to eq(false) }
+ end
+
+ context 'when current token_expiry_notify_inherited is set to false' do
+ before do
+ child_namespace.namespace_settings.update!(token_expiry_notify_inherited: false)
+ end
+
+ it { is_expected.to eq(true) }
+ end
+ end
+ end
+
describe '#any_project_with_pages_deployed?' do
it 'returns true if any project nested under the group has pages deployed' do
parent_1 = create(:group) # Three projects, one with pages
diff --git a/spec/views/groups/edit.html.haml_spec.rb b/spec/views/groups/edit.html.haml_spec.rb
index f33a4190bf8e4..029531c58449d 100644
--- a/spec/views/groups/edit.html.haml_spec.rb
+++ b/spec/views/groups/edit.html.haml_spec.rb
@@ -161,4 +161,50 @@
end
end
end
+
+ describe 'Email notifications section' do
+ let_it_be(:user) { create(:user) }
+ let_it_be_with_reload(:parent_group) { create(:group, owners: user) }
+ let_it_be_with_reload(:group) { create(:group, parent: parent_group, owners: user) }
+
+ before do
+ assign(:group, group)
+ allow(view).to receive(:current_user).and_return(user)
+ end
+
+ it 'renders fields for token_expiry_notify_inherited' do
+ render
+
+ expect(rendered).to have_content(_('Expiry notification emails about group and project access tokens within this group should be sent to:'))
+ expect(rendered).to have_selector('#group_token_expiry_notify_inherited_true:not([disabled])')
+ expect(rendered).to have_selector('#group_token_expiry_notify_inherited_false:not([disabled])')
+ expect(rendered).not_to have_content(_('A parent group has selected "Only direct members." It cannot be overridden by this group.'))
+ end
+
+ context 'when pat_expiry_inherited_members_notification FF is disabled' do
+ before do
+ stub_feature_flags(pat_expiry_inherited_members_notification: false)
+ end
+
+ it 'does not render form' do
+ render
+
+ expect(rendered).not_to have_content(_('Expiry notification emails about group and project access tokens within this group should be sent to:'))
+ end
+ end
+
+ context 'when parent group has token_expiry_notify_inherited set to false' do
+ before do
+ parent_group.namespace_settings.update!(token_expiry_notify_inherited: false)
+ end
+
+ it 'renders disabled fields' do
+ render
+
+ expect(rendered).to have_selector('#group_token_expiry_notify_inherited_false[disabled]')
+ expect(rendered).to have_selector('#group_token_expiry_notify_inherited_true[disabled]')
+ expect(rendered).to have_content(_('A parent group has selected "Only direct members." It cannot be overridden by this group.'))
+ end
+ end
+ end
end
--
GitLab