diff --git a/doc/api/groups.md b/doc/api/groups.md index cb81e7f24e2471e94098c72ce67b04f073bdc50e..d7a9e89985acce7156e20922c500d7bd637b8831 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/app/policies/ee/group_policy.rb b/ee/app/policies/ee/group_policy.rb index f6351e2218d79d4b6c7363b93ec86350e2874014..b10b6b065874a5288a0dc67c85371de3d08724b5 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 }.policy do + enable :read_saml_user + end end override :lookup_access_level! diff --git a/ee/lib/ee/api/groups.rb b/ee/lib/ee/api/groups.rb index a6067f3732eb96be3e4e22872ac6903e0cfd93d9..57b6b2380d56b2e70b714bc40e36e94d09f17400 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_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 422006045edf30b931508b67ba4db38ae3bbf3da..c3481efc775054795bf3f4980526a16b0929c940 100644 --- a/ee/spec/policies/group_policy_spec.rb +++ b/ee/spec/policies/group_policy_spec.rb @@ -3031,6 +3031,69 @@ def expect_private_group_permissions_as_if_non_member end end + describe ':read_saml_user' do + let_it_be(:user) { non_group_member } + let_it_be(:subgroup) { create(:group, :private, parent: group) } + + subject(:policy) { described_class.new(user, the_group) } + + context 'when a SAML provider does not exist' do + let_it_be(:the_group) { subgroup } + + before do + stub_licensed_features(group_saml: true) + the_group.add_member(user, Gitlab::Access::OWNER) + end + + it { is_expected.to be_disallowed(:read_saml_user) } + end + + context 'when a SAML provider exists' do + 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 + + ref(:group) | true | true | false | Gitlab::Access::OWNER | false + ref(:group) | true | true | false | Gitlab::Access::MAINTAINER | false + + ref(:group) | true | true | true | Gitlab::Access::OWNER | true + ref(:group) | true | true | true | Gitlab::Access::MAINTAINER | false + + ref(:subgroup) | false | false | false | Gitlab::Access::OWNER | false + ref(:subgroup) | 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(: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 + + 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 + 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 091019f76b81a0e64ac3b602212422f755ae5996..3977169d9a108a83259a024c8f7c4185523e2c7a 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_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) } 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 @@ -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