From 056ad384119079c93ad7a8e724d14381a00b77b8 Mon Sep 17 00:00:00 2001
From: Smriti Garg <sgarg@gitlab.com>
Date: Tue, 4 Mar 2025 16:58:34 +0000
Subject: [PATCH] Added new column and controller setting
New column with name dpop_check_for_manage_api_endpoints is added
to namespace_settings. This setting will control whether we need
to enforce dpop authentication on /manage endpoints or not
Changelog: added
EE: true
Added new column and controller setting
---
...d_require_dpop_for_manage_api_endpoints.rb | 12 ++
db/schema_migrations/20250227044611 | 1 +
db/structure.sql | 1 +
.../controllers/concerns/ee/groups/params.rb | 5 +
ee/app/models/ee/group.rb | 2 +
.../namespace_setting_changes_auditor_spec.rb | 2 +-
ee/spec/models/ee/group_spec.rb | 3 +
ee/spec/models/namespace_setting_spec.rb | 22 ++++
ee/spec/requests/groups_controller_spec.rb | 110 ++++++++++++------
9 files changed, 124 insertions(+), 34 deletions(-)
create mode 100644 db/migrate/20250227044611_add_require_dpop_for_manage_api_endpoints.rb
create mode 100644 db/schema_migrations/20250227044611
diff --git a/db/migrate/20250227044611_add_require_dpop_for_manage_api_endpoints.rb b/db/migrate/20250227044611_add_require_dpop_for_manage_api_endpoints.rb
new file mode 100644
index 0000000000000..31a6d54334ad9
--- /dev/null
+++ b/db/migrate/20250227044611_add_require_dpop_for_manage_api_endpoints.rb
@@ -0,0 +1,12 @@
+# frozen_string_literal: true
+
+# See https://docs.gitlab.com/ee/development/migration_style_guide.html
+# for more information on how to write migrations for GitLab.
+
+class AddRequireDpopForManageApiEndpoints < Gitlab::Database::Migration[2.2]
+ milestone '17.10'
+
+ def change
+ add_column :namespace_settings, :require_dpop_for_manage_api_endpoints, :boolean, null: false, default: true
+ end
+end
diff --git a/db/schema_migrations/20250227044611 b/db/schema_migrations/20250227044611
new file mode 100644
index 0000000000000..bcaa3664b8ebb
--- /dev/null
+++ b/db/schema_migrations/20250227044611
@@ -0,0 +1 @@
+1a7161f562a83a1453dff1c15ba73cce126dca54ae217754617556ae78c48bc4
\ No newline at end of file
diff --git a/db/structure.sql b/db/structure.sql
index 56db66feda0a2..b237086a5a70f 100644
--- a/db/structure.sql
+++ b/db/structure.sql
@@ -17067,6 +17067,7 @@ CREATE TABLE namespace_settings (
extended_grat_expiry_webhooks_execute boolean DEFAULT false NOT NULL,
jwt_ci_cd_job_token_enabled boolean DEFAULT false NOT NULL,
jwt_ci_cd_job_token_opted_out boolean DEFAULT false NOT NULL,
+ require_dpop_for_manage_api_endpoints 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/app/controllers/concerns/ee/groups/params.rb b/ee/app/controllers/concerns/ee/groups/params.rb
index 89fcd7d7b227b..f9fecab0d2679 100644
--- a/ee/app/controllers/concerns/ee/groups/params.rb
+++ b/ee/app/controllers/concerns/ee/groups/params.rb
@@ -85,6 +85,11 @@ def group_params_ee
current_group.licensed_feature_available?(:group_webhooks)
params_ee << :extended_grat_expiry_webhooks_execute
end
+
+ if ::Feature.enabled?(:manage_pat_by_group_owners_ready, current_group) &&
+ can?(current_user, :admin_group, current_group)
+ params_ee << :require_dpop_for_manage_api_endpoints
+ end
end
end
# rubocop:enable Metrics/CyclomaticComplexity, Metrics/PerceivedComplexity, Metrics/AbcSize
diff --git a/ee/app/models/ee/group.rb b/ee/app/models/ee/group.rb
index da876ae5595e9..df16407b6eef0 100644
--- a/ee/app/models/ee/group.rb
+++ b/ee/app/models/ee/group.rb
@@ -118,6 +118,8 @@ module Group
delegate :enable_auto_assign_gitlab_duo_pro_seats, :enable_auto_assign_gitlab_duo_pro_seats=, :enable_auto_assign_gitlab_duo_pro_seats_human_readable, :enable_auto_assign_gitlab_duo_pro_seats_human_readable=, to: :namespace_settings, allow_nil: true
delegate :extended_grat_expiry_webhooks_execute, :extended_grat_expiry_webhooks_execute=, to: :namespace_settings
+ delegate :require_dpop_for_manage_api_endpoints, :require_dpop_for_manage_api_endpoints=, to: :namespace_settings
+ delegate :require_dpop_for_manage_api_endpoints?, to: :namespace_settings
# Use +checked_file_template_project+ instead, which implements important
# visibility checks
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 d4e74df24cf94..c509ddcd3d2e4 100644
--- a/ee/spec/lib/namespaces/namespace_setting_changes_auditor_spec.rb
+++ b/ee/spec/lib/namespaces/namespace_setting_changes_auditor_spec.rb
@@ -124,7 +124,7 @@
lock_spp_repository_pipeline_access spp_repository_pipeline_access archived
resource_access_token_notify_inherited lock_resource_access_token_notify_inherited
pipeline_variables_default_role extended_grat_expiry_webhooks_execute force_pages_access_control
- jwt_ci_cd_job_token_enabled jwt_ci_cd_job_token_opted_out]
+ jwt_ci_cd_job_token_enabled jwt_ci_cd_job_token_opted_out require_dpop_for_manage_api_endpoints]
columns_to_audit = Namespaces::NamespaceSettingChangesAuditor::EVENT_NAME_PER_COLUMN.keys.map(&:to_s)
diff --git a/ee/spec/models/ee/group_spec.rb b/ee/spec/models/ee/group_spec.rb
index f5c1c4f068bd2..940e5d672e27d 100644
--- a/ee/spec/models/ee/group_spec.rb
+++ b/ee/spec/models/ee/group_spec.rb
@@ -360,6 +360,9 @@
it { is_expected.to delegate_method(:duo_availability=).to(:namespace_settings).with_arguments(:args) }
it { is_expected.to delegate_method(:experiment_settings_allowed?).to(:namespace_settings) }
it { is_expected.to delegate_method(:user_cap_enabled?).to(:namespace_settings) }
+ it { is_expected.to delegate_method(:require_dpop_for_manage_api_endpoints?).to(:namespace_settings) }
+ it { is_expected.to delegate_method(:require_dpop_for_manage_api_endpoints).to(:namespace_settings) }
+ it { is_expected.to delegate_method(:require_dpop_for_manage_api_endpoints=).to(:namespace_settings).with_arguments(:args) }
it { is_expected.to delegate_method(:enterprise_users_extensions_marketplace_enabled=).to(:namespace_settings).with_arguments(:args) }
end
diff --git a/ee/spec/models/namespace_setting_spec.rb b/ee/spec/models/namespace_setting_spec.rb
index 747f3666ef968..a84cf07b405f3 100644
--- a/ee/spec/models/namespace_setting_spec.rb
+++ b/ee/spec/models/namespace_setting_spec.rb
@@ -329,6 +329,28 @@
end
end
+ describe '#require_dpop_for_manage_api_endpoints?' do
+ context 'when require_dpop_for_manage_api_endpoints = true' do
+ before do
+ setting.require_dpop_for_manage_api_endpoints = true
+ end
+
+ it 'returns true' do
+ expect(setting.require_dpop_for_manage_api_endpoints?).to eq(true)
+ end
+ end
+
+ context 'when require_dpop_for_manage_api_endpoints = false' do
+ before do
+ setting.require_dpop_for_manage_api_endpoints = false
+ end
+
+ it 'returns false' do
+ expect(setting.require_dpop_for_manage_api_endpoints?).to eq(false)
+ end
+ end
+ end
+
describe '#user_cap_enabled?', feature_category: :consumables_cost_management do
where(:seat_control, :new_user_signups_cap, :root_namespace, :expectation) do
:off | nil | false | false
diff --git a/ee/spec/requests/groups_controller_spec.rb b/ee/spec/requests/groups_controller_spec.rb
index b4769698f4792..7f9b664a5faf0 100644
--- a/ee/spec/requests/groups_controller_spec.rb
+++ b/ee/spec/requests/groups_controller_spec.rb
@@ -12,7 +12,7 @@
login_as(user)
end
- subject do
+ subject(:request) do
put(group_path(group), params: params)
end
@@ -31,7 +31,7 @@
context 'valid param' do
shared_examples 'creates ip restrictions' do
it 'creates ip restrictions' do
- expect { subject }
+ expect { request }
.to change { group.reload.ip_restrictions.map(&:range) }
.from([]).to(contain_exactly(*range.split(',')))
expect(response).to have_gitlab_http_status(:found)
@@ -55,7 +55,7 @@
let(:range) { 'boom!' }
it 'adds error message' do
- expect { subject }
+ expect { request }
.not_to(change { group.reload.ip_restrictions.count }.from(0))
expect(response).to have_gitlab_http_status(:ok)
expect(response.body).to include('Ip restrictions range is an invalid IP address range')
@@ -71,7 +71,7 @@
context 'valid param' do
shared_examples 'updates ip restrictions' do
it 'updates ip restrictions' do
- expect { subject }
+ expect { request }
.to change { group.reload.ip_restrictions.map(&:range) }
.from(['10.0.0.0/8']).to(contain_exactly(*range.split(',')))
expect(response).to have_gitlab_http_status(:found)
@@ -102,13 +102,13 @@
context 'invalid param' do
shared_examples 'does not update existing ip restrictions' do
it 'does not change ip restriction records' do
- expect { subject }
+ expect { request }
.not_to(change { group.reload.ip_restrictions.map(&:range) }
.from(['10.0.0.0/8']))
end
it 'adds error message' do
- subject
+ request
expect(response).to have_gitlab_http_status(:ok)
expect(response.body).to include('Ip restrictions range is an invalid IP address range')
@@ -135,7 +135,7 @@
let(:range) { '' }
it 'deletes ip restriction' do
- expect { subject }
+ expect { request }
.to(change { group.reload.ip_restrictions.count }.to(0))
expect(response).to have_gitlab_http_status(:found)
end
@@ -147,7 +147,7 @@
let(:group) { create(:group, :nested) }
it 'does not create ip restriction' do
- expect { subject }
+ expect { request }
.not_to change { group.reload.ip_restrictions.count }.from(0)
expect(response).to have_gitlab_http_status(:ok)
expect(response.body).to include('Ip restrictions IP subnet restriction only allowed for top-level groups')
@@ -161,13 +161,13 @@
end
it 'updates group setting' do
- expect { subject }
+ expect { request }
.to change { group.reload.two_factor_grace_period }.from(48).to(42)
expect(response).to have_gitlab_http_status(:found)
end
it 'does not create ip restriction' do
- expect { subject }.not_to change { IpRestriction.count }
+ expect { request }.not_to change { IpRestriction.count }
end
end
@@ -177,7 +177,7 @@
end
it 'does not create ip restriction' do
- expect { subject }
+ expect { request }
.not_to change { group.reload.ip_restrictions.count }.from(0)
expect(response).to have_gitlab_http_status(:found)
end
@@ -196,7 +196,7 @@
context 'valid param' do
shared_examples 'creates email domain restrictions' do
it 'creates email domain restrictions' do
- subject
+ request
expect(response).to have_gitlab_http_status(:found)
expect(group.reload.allowed_email_domains.domain_names).to match_array(domains.split(","))
@@ -220,7 +220,7 @@
let(:domains) { 'boom!' }
it 'adds error message' do
- expect { subject }
+ expect { request }
.not_to(change { group.reload.allowed_email_domains.count }.from(0))
expect(response).to have_gitlab_http_status(:ok)
expect(response.body).to include('The domain you entered is misformatted')
@@ -235,7 +235,7 @@
context 'valid param' do
shared_examples 'updates allowed email domain restrictions' do
it 'updates allowed email domain restrictions' do
- subject
+ request
expect(response).to have_gitlab_http_status(:found)
expect(group.reload.allowed_email_domains.domain_names).to match_array(domains.split(","))
@@ -266,13 +266,13 @@
context 'invalid param' do
shared_examples 'does not update existing email domain restrictions' do
it 'does not change allowed_email_domains records' do
- expect { subject }
+ expect { request }
.not_to(change { group.reload.allowed_email_domains.domain_names }
.from(['gitlab.com']))
end
it 'adds error message' do
- subject
+ request
expect(response).to have_gitlab_http_status(:ok)
expect(response.body).to include('The domain you entered is misformatted')
@@ -299,7 +299,7 @@
let(:domains) { '' }
it 'deletes all email domain restrictions' do
- expect { subject }
+ expect { request }
.to(change { group.reload.allowed_email_domains.count }.to(0))
expect(response).to have_gitlab_http_status(:found)
end
@@ -312,7 +312,7 @@
let(:domains) { 'gitlab.com' }
it 'does not create email domain restriction' do
- expect { subject }
+ expect { request }
.not_to change { group.reload.allowed_email_domains.count }.from(0)
expect(response).to have_gitlab_http_status(:ok)
expect(response.body).to include('Allowed email domain restriction only permitted for top-level groups')
@@ -327,7 +327,7 @@
end
it 'does not create email domain restrictions' do
- expect { subject }
+ expect { request }
.not_to change { group.reload.allowed_email_domains.count }.from(0)
expect(response).to have_gitlab_http_status(:found)
end
@@ -338,7 +338,7 @@
let(:params) { { group: { enforce_ssh_certificates: true } } }
it 'does not change the column' do
- expect { subject }.not_to change { group.reload.enforce_ssh_certificates? }
+ expect { request }.not_to change { group.reload.enforce_ssh_certificates? }
expect(response).to have_gitlab_http_status(:found)
end
@@ -348,7 +348,7 @@
end
it 'successfully changes the column' do
- expect { subject }.to change { group.reload.enforce_ssh_certificates? }
+ expect { request }.to change { group.reload.enforce_ssh_certificates? }
expect(response).to have_gitlab_http_status(:found)
end
@@ -356,13 +356,57 @@
let(:group) { create(:group, :nested) }
it 'does not change the column' do
- expect { subject }.not_to change { group.reload.enforce_ssh_certificates? }
+ expect { request }.not_to change { group.reload.enforce_ssh_certificates? }
expect(response).to have_gitlab_http_status(:found)
end
end
end
end
+ context 'when settings require_dpop_for_manage_api_endpoints' do
+ let(:params) { { group: { require_dpop_for_manage_api_endpoints: false } } }
+
+ context 'when feature flag is enabled' do
+ before do
+ stub_feature_flags(manage_pat_by_group_owners_ready: true)
+ end
+
+ context 'when not group owner' do
+ let(:user) { create(:user) }
+
+ before do
+ group.add_developer(user)
+ login_as(user)
+ end
+
+ it 'does not change the column and returns not_found' do
+ expect(group.require_dpop_for_manage_api_endpoints?).to be(true)
+
+ request
+
+ expect(response).to have_gitlab_http_status(:not_found)
+ expect(group.reload.require_dpop_for_manage_api_endpoints?).to be(true)
+ end
+ end
+
+ it 'successfully changes the column' do
+ expect { request }.to change { group.reload.require_dpop_for_manage_api_endpoints? }
+ expect(response).to have_gitlab_http_status(:found)
+ end
+ end
+
+ context 'when feature flag is disabled' do
+ before do
+ stub_feature_flags(manage_pat_by_group_owners_ready: false)
+ end
+
+ it 'does not change the column' do
+ expect { request }.not_to change { group.reload.require_dpop_for_manage_api_endpoints? }
+ expect(response).to have_gitlab_http_status(:found)
+ end
+ end
+ end
+
context 'when settings extended_grat_expiry_webhooks_execute' do
let(:params) { { group: { extended_grat_expiry_webhooks_execute: true } } }
@@ -372,7 +416,7 @@
end
it 'does not change the column' do
- expect { subject }.not_to change { group.reload.extended_grat_expiry_webhooks_execute? }
+ expect { request }.not_to change { group.reload.extended_grat_expiry_webhooks_execute? }
expect(response).to have_gitlab_http_status(:found)
end
end
@@ -383,7 +427,7 @@
end
it 'successfully changes the column' do
- expect { subject }.to change { group.reload.extended_grat_expiry_webhooks_execute? }
+ expect { request }.to change { group.reload.extended_grat_expiry_webhooks_execute? }
expect(response).to have_gitlab_http_status(:found)
end
@@ -393,7 +437,7 @@
end
it 'does not change the column' do
- expect { subject }.not_to change { group.reload.extended_grat_expiry_webhooks_execute? }
+ expect { request }.not_to change { group.reload.extended_grat_expiry_webhooks_execute? }
expect(response).to have_gitlab_http_status(:found)
end
end
@@ -411,7 +455,7 @@
end
it 'does not change the column' do
- expect { subject }.not_to change { group.reload.enterprise_users_extensions_marketplace_enabled? }
+ expect { request }.not_to change { group.reload.enterprise_users_extensions_marketplace_enabled? }
expect(response).to have_gitlab_http_status(:found)
end
@@ -421,7 +465,7 @@
end
it 'successfully changes the column' do
- expect { subject }.to change { group.reload.enterprise_users_extensions_marketplace_enabled? }
+ expect { request }.to change { group.reload.enterprise_users_extensions_marketplace_enabled? }
expect(response).to have_gitlab_http_status(:found)
end
end
@@ -439,7 +483,7 @@
it 'does not change the column' do
expect(group.enable_auto_assign_gitlab_duo_pro_seats?).to be_falsy
- subject
+ request
expect(response).to have_gitlab_http_status(:found)
expect(group.reload.enable_auto_assign_gitlab_duo_pro_seats?).to be_falsy
@@ -460,7 +504,7 @@
it 'does not change the column' do
expect(group.enable_auto_assign_gitlab_duo_pro_seats?).to be_falsy
- subject
+ request
expect(response).to have_gitlab_http_status(:found)
expect(group.reload.enable_auto_assign_gitlab_duo_pro_seats?).to be_falsy
@@ -475,7 +519,7 @@
it 'does not change the column' do
expect(group.enable_auto_assign_gitlab_duo_pro_seats?).to be_falsy
- subject
+ request
expect(response).to have_gitlab_http_status(:found)
expect(group.reload.enable_auto_assign_gitlab_duo_pro_seats?).to be_falsy
@@ -493,7 +537,7 @@
it 'does not change the column and returns not_found' do
expect(group.enable_auto_assign_gitlab_duo_pro_seats?).to be_falsy
- subject
+ request
expect(response).to have_gitlab_http_status(:not_found)
expect(group.reload.enable_auto_assign_gitlab_duo_pro_seats?).to be_falsy
@@ -508,7 +552,7 @@
it 'does not change the column' do
expect(group.enable_auto_assign_gitlab_duo_pro_seats?).to be_falsy
- subject
+ request
expect(response).to have_gitlab_http_status(:found)
expect(group.reload.enable_auto_assign_gitlab_duo_pro_seats?).to be_falsy
@@ -524,7 +568,7 @@
it 'successfully updates the column' do
expect(group.enable_auto_assign_gitlab_duo_pro_seats?).to be_falsy
- subject
+ request
expect(response).to have_gitlab_http_status(:found)
expect(group.reload.enable_auto_assign_gitlab_duo_pro_seats?).to be_truthy
--
GitLab