From 610235dd16f375f44f12cf5bcce55e2a086e6b3c Mon Sep 17 00:00:00 2001 From: Drew Blessing Date: Thu, 4 Jan 2024 15:25:01 -0600 Subject: [PATCH 1/4] Allow subgroup maintainers to query org SAML users --- ee/app/policies/ee/group_policy.rb | 11 +++++++++++ ee/lib/ee/api/groups.rb | 2 +- 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/ee/app/policies/ee/group_policy.rb b/ee/app/policies/ee/group_policy.rb index f6351e2218d79d..a810c6677720b3 100644 --- a/ee/app/policies/ee/group_policy.rb +++ b/ee/app/policies/ee/group_policy.rb @@ -91,6 +91,13 @@ module GroupPolicy ::Gitlab::Auth::GroupSaml::SessionEnforcer.new(@user, @subject).access_restricted? end + condition(:sso_enforced, scope: :subject) do + saml_provider = @subject.root_ancestor.saml_provider + next false unless saml_provider + + saml_provider.enabled? && saml_provider.enforced_sso? + end + condition(:ip_enforcement_prevents_access, scope: :subject) do !::Gitlab::IpRestriction::Enforcer.new(subject).allows_current_ip? end @@ -662,6 +669,10 @@ module GroupPolicy rule { guest }.enable :read_limit_alert rule { membership_for_chat & chat_allowed_for_group & chat_available_for_user }.enable :access_duo_chat + + rule { can?(:admin_group_member) & sso_enforced & (admin | maintainer) }.policy do + enable :read_saml_users + end end override :lookup_access_level! diff --git a/ee/lib/ee/api/groups.rb b/ee/lib/ee/api/groups.rb index a6067f3732eb96..f94e525236011a 100644 --- a/ee/lib/ee/api/groups.rb +++ b/ee/lib/ee/api/groups.rb @@ -283,7 +283,7 @@ def delete_group(group) end # rubocop: disable CodeReuse/ActiveRecord get ':id/users', feature_category: :user_management do - authorize! :read_group_member, user_group + authorize! :read_saml_users, user_group params = declared_params(include_missing: false).except!(:id) bad_request!(users_params_error) unless any_allowed_filters_present?(params) -- GitLab From 48f6e0453304f16c02e2ff4983f9eb20ab320f20 Mon Sep 17 00:00:00 2001 From: Drew Blessing Date: Mon, 29 Jan 2024 14:15:24 -0600 Subject: [PATCH 2/4] Add tests --- ee/app/policies/ee/group_policy.rb | 2 +- ee/spec/policies/group_policy_spec.rb | 31 +++++++++++++++++++++++++++ ee/spec/requests/api/groups_spec.rb | 6 +++--- 3 files changed, 35 insertions(+), 4 deletions(-) diff --git a/ee/app/policies/ee/group_policy.rb b/ee/app/policies/ee/group_policy.rb index a810c6677720b3..4253d202337605 100644 --- a/ee/app/policies/ee/group_policy.rb +++ b/ee/app/policies/ee/group_policy.rb @@ -670,7 +670,7 @@ module GroupPolicy rule { membership_for_chat & chat_allowed_for_group & chat_available_for_user }.enable :access_duo_chat - rule { can?(:admin_group_member) & sso_enforced & (admin | maintainer) }.policy do + rule { can?(:admin_group_member) & sso_enforced }.policy do enable :read_saml_users end end diff --git a/ee/spec/policies/group_policy_spec.rb b/ee/spec/policies/group_policy_spec.rb index 422006045edf30..982b5b68bfdbdf 100644 --- a/ee/spec/policies/group_policy_spec.rb +++ b/ee/spec/policies/group_policy_spec.rb @@ -3031,6 +3031,37 @@ def expect_private_group_permissions_as_if_non_member end end + describe 'read_saml_users' do + let_it_be(:saml_group) { create(:group, :private) } + let_it_be(:subgroup) { create(:group, :private, parent: saml_group) } + + let_it_be(:saml_provider) { create(:saml_provider, group: saml_group) } + + let_it_be(:group_owner) { create(:user) { |u| saml_group.add_owner(u) } } + let_it_be(:group_developer) { create(:user) { |u| saml_group.add_developer(u) } } + + subject(:policy) { described_class.new(group_owner, saml_group) } + + it { is_expected.to be_allowed(:admin_group_member) } + it { is_expected.to be_disallowed(:read_saml_users) } + + context 'when SSO is not enforced' do + it { is_expected.to be_allowed(:admin_group_member) } + it { is_expected.to be_disallowed(:read_saml_users) } + end + + context 'when SSO is enforced' do + before do + saml_provider.update!(enabled: true) + saml_provider.update!(enforced_sso: true) + stub_licensed_features(group_saml: true) + end + + it { is_expected.to be_allowed(:admin_group_member) } + it { is_expected.to be_allowed(:read_saml_users) } + end + end + context 'custom role' do let_it_be(:guest) { create(:user) } let_it_be(:parent_group) { create(:group) } diff --git a/ee/spec/requests/api/groups_spec.rb b/ee/spec/requests/api/groups_spec.rb index 091019f76b81a0..e9d62dc5a1282d 100644 --- a/ee/spec/requests/api/groups_spec.rb +++ b/ee/spec/requests/api/groups_spec.rb @@ -1521,8 +1521,8 @@ describe 'GET /groups/:id/users' do let_it_be(:group) { create(:group, :private) } let_it_be(:regular_user) { create(:user) } - let_it_be(:saml_provider) { create(:saml_provider, group: group) } - let_it_be(:group_member_user) { create(:user) { |u| group.add_developer(u) } } + let_it_be(:saml_provider) { create(:saml_provider, group: group, enforced_sso: true) } + let_it_be(:group_member_user) { create(:user) { |u| group.add_owner(u) } } let_it_be(:saml_user) { create(:user, :public_email) } let_it_be(:non_saml_user) { create(:user) { |u| group.add_maintainer(u) } } @@ -1531,7 +1531,7 @@ subject(:get_users) { get api("/groups/#{group.to_param}/users", current_user), params: params } before do - create(:saml_provider, group: group) + stub_licensed_features(group_saml: true) create(:group_saml_identity, user: group_member_user, saml_provider: saml_provider) create(:group_saml_identity, user: saml_user, saml_provider: saml_provider) end -- GitLab From 7c57c7e2d150264f66c33589a5c1d69d04274225 Mon Sep 17 00:00:00 2001 From: Drew Blessing Date: Tue, 30 Jan 2024 10:38:36 -0600 Subject: [PATCH 3/4] Add policy specs --- ee/app/policies/ee/group_policy.rb | 2 +- ee/lib/ee/api/groups.rb | 2 +- ee/spec/policies/group_policy_spec.rb | 55 ++++++++++++++++++--------- 3 files changed, 39 insertions(+), 20 deletions(-) diff --git a/ee/app/policies/ee/group_policy.rb b/ee/app/policies/ee/group_policy.rb index 4253d202337605..b10b6b065874a5 100644 --- a/ee/app/policies/ee/group_policy.rb +++ b/ee/app/policies/ee/group_policy.rb @@ -671,7 +671,7 @@ module GroupPolicy rule { membership_for_chat & chat_allowed_for_group & chat_available_for_user }.enable :access_duo_chat rule { can?(:admin_group_member) & sso_enforced }.policy do - enable :read_saml_users + enable :read_saml_user end end diff --git a/ee/lib/ee/api/groups.rb b/ee/lib/ee/api/groups.rb index f94e525236011a..57b6b2380d56b2 100644 --- a/ee/lib/ee/api/groups.rb +++ b/ee/lib/ee/api/groups.rb @@ -283,7 +283,7 @@ def delete_group(group) end # rubocop: disable CodeReuse/ActiveRecord get ':id/users', feature_category: :user_management do - authorize! :read_saml_users, user_group + authorize! :read_saml_user, user_group params = declared_params(include_missing: false).except!(:id) bad_request!(users_params_error) unless any_allowed_filters_present?(params) diff --git a/ee/spec/policies/group_policy_spec.rb b/ee/spec/policies/group_policy_spec.rb index 982b5b68bfdbdf..190a13abd47bee 100644 --- a/ee/spec/policies/group_policy_spec.rb +++ b/ee/spec/policies/group_policy_spec.rb @@ -3031,34 +3031,53 @@ def expect_private_group_permissions_as_if_non_member end end - describe 'read_saml_users' do - let_it_be(:saml_group) { create(:group, :private) } - let_it_be(:subgroup) { create(:group, :private, parent: saml_group) } + describe ':read_saml_user' do + let_it_be(:user) { non_group_member } + let_it_be(:subgroup) { create(:group, :private, parent: group) } - let_it_be(:saml_provider) { create(:saml_provider, group: saml_group) } + before_all do + create(:saml_provider, group: group) + end + + using RSpec::Parameterized::TableSyntax + + where(:the_group, :licensed, :saml_enabled, :sso_enforced, :role, :allowed) do + ref(:group) | false | false | false | Gitlab::Access::OWNER | false + ref(:group) | false | false | false | Gitlab::Access::MAINTAINER | false + + ref(:group) | true | false | false | Gitlab::Access::OWNER | false + ref(:group) | true | false | false | Gitlab::Access::MAINTAINER | false - let_it_be(:group_owner) { create(:user) { |u| saml_group.add_owner(u) } } - let_it_be(:group_developer) { create(:user) { |u| saml_group.add_developer(u) } } + ref(:group) | true | true | false | Gitlab::Access::OWNER | false + ref(:group) | true | true | false | Gitlab::Access::MAINTAINER | false - subject(:policy) { described_class.new(group_owner, saml_group) } + ref(:group) | true | true | true | Gitlab::Access::OWNER | true + ref(:group) | true | true | true | Gitlab::Access::MAINTAINER | false - it { is_expected.to be_allowed(:admin_group_member) } - it { is_expected.to be_disallowed(:read_saml_users) } + ref(:subgroup) | false | false | false | Gitlab::Access::OWNER | false + ref(:subgroup) | false | false | false | Gitlab::Access::MAINTAINER | false - context 'when SSO is not enforced' do - it { is_expected.to be_allowed(:admin_group_member) } - it { is_expected.to be_disallowed(:read_saml_users) } + ref(:subgroup) | true | false | false | Gitlab::Access::OWNER | false + ref(:subgroup) | true | false | false | Gitlab::Access::MAINTAINER | false + + ref(:subgroup) | true | true | false | Gitlab::Access::OWNER | false + ref(:subgroup) | true | true | false | Gitlab::Access::MAINTAINER | false + + ref(:subgroup) | true | true | true | Gitlab::Access::OWNER | true + ref(:subgroup) | true | true | true | Gitlab::Access::MAINTAINER | false end - context 'when SSO is enforced' do + with_them do + subject(:policy) { described_class.new(user, the_group) } + before do - saml_provider.update!(enabled: true) - saml_provider.update!(enforced_sso: true) - stub_licensed_features(group_saml: true) + stub_licensed_features(group_saml: licensed) + the_group.add_member(user, role) + the_group.root_ancestor.saml_provider.update!(enabled: saml_enabled) + the_group.root_ancestor.saml_provider.update!(enforced_sso: sso_enforced) end - it { is_expected.to be_allowed(:admin_group_member) } - it { is_expected.to be_allowed(:read_saml_users) } + it { expect(policy.allowed?(:read_saml_user)).to eq(allowed) } end end -- GitLab From a21d59e57642c9a50585712fd2d1cfb7a395d889 Mon Sep 17 00:00:00 2001 From: Drew Blessing Date: Fri, 2 Feb 2024 09:06:02 -0600 Subject: [PATCH 4/4] Add more tests --- doc/api/groups.md | 2 +- ee/spec/policies/group_policy_spec.rb | 73 ++++++++++++++++----------- ee/spec/requests/api/groups_spec.rb | 14 ++++- 3 files changed, 56 insertions(+), 33 deletions(-) diff --git a/doc/api/groups.md b/doc/api/groups.md index cb81e7f24e2471..d7a9e89985acce 100644 --- a/doc/api/groups.md +++ b/doc/api/groups.md @@ -1343,7 +1343,7 @@ by the group or subgroups. This endpoint is an [Experiment](../policy/experiment-beta-support.md) and might be changed or removed without notice. -Requires at least the Maintainer role in the group. +Requires Owner role in the group. ```plaintext GET /groups/:id/users diff --git a/ee/spec/policies/group_policy_spec.rb b/ee/spec/policies/group_policy_spec.rb index 190a13abd47bee..c3481efc775054 100644 --- a/ee/spec/policies/group_policy_spec.rb +++ b/ee/spec/policies/group_policy_spec.rb @@ -3035,49 +3035,62 @@ def expect_private_group_permissions_as_if_non_member let_it_be(:user) { non_group_member } let_it_be(:subgroup) { create(:group, :private, parent: group) } - before_all do - create(:saml_provider, group: group) - end + subject(:policy) { described_class.new(user, the_group) } - using RSpec::Parameterized::TableSyntax + context 'when a SAML provider does not exist' do + let_it_be(:the_group) { subgroup } - where(:the_group, :licensed, :saml_enabled, :sso_enforced, :role, :allowed) do - ref(:group) | false | false | false | Gitlab::Access::OWNER | false - ref(:group) | false | false | false | Gitlab::Access::MAINTAINER | false + before do + stub_licensed_features(group_saml: true) + the_group.add_member(user, Gitlab::Access::OWNER) + end - ref(:group) | true | false | false | Gitlab::Access::OWNER | false - ref(:group) | true | false | false | Gitlab::Access::MAINTAINER | false + it { is_expected.to be_disallowed(:read_saml_user) } + end - ref(:group) | true | true | false | Gitlab::Access::OWNER | false - ref(:group) | true | true | false | Gitlab::Access::MAINTAINER | false + context 'when a SAML provider exists' do + before_all do + create(:saml_provider, group: group) + end - ref(:group) | true | true | true | Gitlab::Access::OWNER | true - ref(:group) | true | true | true | Gitlab::Access::MAINTAINER | false + using RSpec::Parameterized::TableSyntax - ref(:subgroup) | false | false | false | Gitlab::Access::OWNER | false - ref(:subgroup) | false | false | false | Gitlab::Access::MAINTAINER | false + where(:the_group, :licensed, :saml_enabled, :sso_enforced, :role, :allowed) do + ref(:group) | false | false | false | Gitlab::Access::OWNER | false + ref(:group) | false | false | false | Gitlab::Access::MAINTAINER | false - ref(:subgroup) | true | false | false | Gitlab::Access::OWNER | false - ref(:subgroup) | true | false | false | Gitlab::Access::MAINTAINER | false + ref(:group) | true | false | false | Gitlab::Access::OWNER | false + ref(:group) | true | false | false | Gitlab::Access::MAINTAINER | false - ref(:subgroup) | true | true | false | Gitlab::Access::OWNER | false - ref(:subgroup) | true | true | false | Gitlab::Access::MAINTAINER | false + ref(:group) | true | true | false | Gitlab::Access::OWNER | false + ref(:group) | true | true | false | Gitlab::Access::MAINTAINER | false - ref(:subgroup) | true | true | true | Gitlab::Access::OWNER | true - ref(:subgroup) | true | true | true | Gitlab::Access::MAINTAINER | false - end + ref(:group) | true | true | true | Gitlab::Access::OWNER | true + ref(:group) | true | true | true | Gitlab::Access::MAINTAINER | false - with_them do - subject(:policy) { described_class.new(user, the_group) } + ref(:subgroup) | false | false | false | Gitlab::Access::OWNER | false + ref(:subgroup) | false | false | false | Gitlab::Access::MAINTAINER | false - before do - stub_licensed_features(group_saml: licensed) - the_group.add_member(user, role) - the_group.root_ancestor.saml_provider.update!(enabled: saml_enabled) - the_group.root_ancestor.saml_provider.update!(enforced_sso: sso_enforced) + ref(:subgroup) | true | false | false | Gitlab::Access::OWNER | false + ref(:subgroup) | true | false | false | Gitlab::Access::MAINTAINER | false + + ref(:subgroup) | true | true | false | Gitlab::Access::OWNER | false + ref(:subgroup) | true | true | false | Gitlab::Access::MAINTAINER | false + + ref(:subgroup) | true | true | true | Gitlab::Access::OWNER | true + ref(:subgroup) | true | true | true | Gitlab::Access::MAINTAINER | false end - it { expect(policy.allowed?(:read_saml_user)).to eq(allowed) } + with_them do + before do + stub_licensed_features(group_saml: licensed) + the_group.add_member(user, role) + the_group.root_ancestor.saml_provider.update!(enabled: saml_enabled) + the_group.root_ancestor.saml_provider.update!(enforced_sso: sso_enforced) + end + + it { expect(policy.allowed?(:read_saml_user)).to eq(allowed) } + end end end diff --git a/ee/spec/requests/api/groups_spec.rb b/ee/spec/requests/api/groups_spec.rb index e9d62dc5a1282d..3977169d9a108a 100644 --- a/ee/spec/requests/api/groups_spec.rb +++ b/ee/spec/requests/api/groups_spec.rb @@ -1521,7 +1521,7 @@ describe 'GET /groups/:id/users' do let_it_be(:group) { create(:group, :private) } let_it_be(:regular_user) { create(:user) } - let_it_be(:saml_provider) { create(:saml_provider, group: group, enforced_sso: true) } + let_it_be_with_refind(:saml_provider) { create(:saml_provider, group: group, enforced_sso: true) } let_it_be(:group_member_user) { create(:user) { |u| group.add_owner(u) } } let_it_be(:saml_user) { create(:user, :public_email) } @@ -1562,7 +1562,7 @@ context 'when no include params are true' do let(:params) { { include_saml_users: false } } - it 'returns 404' do + it 'returns 400' do get_users expect(response).to have_gitlab_http_status(:bad_request) @@ -1577,6 +1577,16 @@ expect(json_response.pluck('id')).to match_array([group_member_user.id, saml_user.id, service_account.id]) end + + context 'when no SAML provider exists' do + it 'returns 403' do + saml_provider.destroy! + + get_users + + expect(response).to have_gitlab_http_status(:forbidden) + end + end end context 'when only include_saml_users is true' do -- GitLab