diff --git a/db/migrate/20231030205756_index_user_details_on_enterprise_group_id_and_user_id.rb b/db/migrate/20231030205756_index_user_details_on_enterprise_group_id_and_user_id.rb new file mode 100644 index 0000000000000000000000000000000000000000..a993944e152be84317078fbf0de90904b28977c5 --- /dev/null +++ b/db/migrate/20231030205756_index_user_details_on_enterprise_group_id_and_user_id.rb @@ -0,0 +1,22 @@ +# frozen_string_literal: true + +class IndexUserDetailsOnEnterpriseGroupIdAndUserId < Gitlab::Database::Migration[2.2] + milestone '16.6' + + disable_ddl_transaction! + + INDEX_NAME = 'index_user_details_on_enterprise_group_id_and_user_id' + INDEX_NAME_TO_REMOVE = 'index_user_details_on_enterprise_group_id' + + def up + add_concurrent_index(:user_details, [:enterprise_group_id, :user_id], name: INDEX_NAME) + + remove_concurrent_index_by_name :user_details, INDEX_NAME_TO_REMOVE + end + + def down + remove_concurrent_index_by_name :user_details, INDEX_NAME + + add_concurrent_index :user_details, :enterprise_group_id, name: INDEX_NAME_TO_REMOVE + end +end diff --git a/db/schema_migrations/20231030205756 b/db/schema_migrations/20231030205756 new file mode 100644 index 0000000000000000000000000000000000000000..3923ee6dbd0bc8169776ae119153005c618b3c5f --- /dev/null +++ b/db/schema_migrations/20231030205756 @@ -0,0 +1 @@ +fd45299e8376db582461fa4b714b3718c4f589bc087d73465ac51d04437e07c3 \ No newline at end of file diff --git a/db/structure.sql b/db/structure.sql index 043dd201d5a17cf0de88caa00bf670acb1a6523b..dab8a151fdab62d704373293d5955a822ce8635d 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -34553,7 +34553,7 @@ CREATE INDEX index_user_custom_attributes_on_key_and_value ON user_custom_attrib CREATE UNIQUE INDEX index_user_custom_attributes_on_user_id_and_key ON user_custom_attributes USING btree (user_id, key); -CREATE INDEX index_user_details_on_enterprise_group_id ON user_details USING btree (enterprise_group_id); +CREATE INDEX index_user_details_on_enterprise_group_id_and_user_id ON user_details USING btree (enterprise_group_id, user_id); CREATE INDEX index_user_details_on_password_last_changed_at ON user_details USING btree (password_last_changed_at); diff --git a/doc/user/group/index.md b/doc/user/group/index.md index 484fd8c533bbff4f8638465d1a8c20a34790d14a..ab8cba48dc44a8f70fcb53cd2e99dfbfc6d34143 100644 --- a/doc/user/group/index.md +++ b/doc/user/group/index.md @@ -219,7 +219,7 @@ Filter a group to find members. By default, all members in the group and subgrou In lists of group members, entries can display the following badges: - **SAML**, to indicate the member has a [SAML account](saml_sso/index.md) connected to them. -- **Enterprise**, to indicate that the member is an [enterprise user](../enterprise_user/index.md). +- **Enterprise**, to indicate that the member of the top-level group is an [enterprise user](../enterprise_user/index.md). 1. On the left sidebar, select **Search or go to** and find your group. 1. Select **Manage > Members**. @@ -227,7 +227,7 @@ In lists of group members, entries can display the following badges: - To view members in the group only, select **Membership = Direct**. - To view members of the group and its subgroups, select **Membership = Inherited**. - To view members with two-factor authentication enabled or disabled, select **2FA = Enabled** or **Disabled**. - - [In GitLab 14.0 and later](https://gitlab.com/gitlab-org/gitlab/-/issues/349887), to view GitLab users created by [SAML SSO](saml_sso/index.md) or [SCIM provisioning](saml_sso/scim_setup.md) select **Enterprise = true**. + - To view members of the top-level group who are [enterprise users](../enterprise_user/index.md), select **Enterprise = true**. ### Search a group diff --git a/ee/app/assets/javascripts/members/utils.js b/ee/app/assets/javascripts/members/utils.js index 737790ba4edd77904c3d83991e45524718c92844..5e5cab34ce747c8529573cec9b85ca905a243330 100644 --- a/ee/app/assets/javascripts/members/utils.js +++ b/ee/app/assets/javascripts/members/utils.js @@ -34,7 +34,7 @@ export const generateBadges = ({ member, isCurrentUser, canManageMembers }) => [ variant: 'info', }, { - show: member.provisionedByThisGroup, + show: member.enterpriseUserOfThisGroup, text: __('Enterprise'), variant: 'info', }, diff --git a/ee/app/finders/ee/group_members_finder.rb b/ee/app/finders/ee/group_members_finder.rb index 329827362a566eb5a86f2b82a6377b5cc72f3172..6fde395940e1814dd59c36c87a2e5e2ab72528ed 100644 --- a/ee/app/finders/ee/group_members_finder.rb +++ b/ee/app/finders/ee/group_members_finder.rb @@ -56,7 +56,7 @@ def filter_by_enterprise_users(members) end def can_filter_by_enterprise? - can_manage_members && group.root_ancestor.saml_enabled? + group.domain_verification_available? && can_manage_members end override :static_roles_only? diff --git a/ee/app/helpers/ee/groups/group_members_helper.rb b/ee/app/helpers/ee/groups/group_members_helper.rb index 2de3aa408051c3cf7ff96dfd7018ed9e0a1c7061..30dbed426d5342108077df54394a1bdf40032c2d 100644 --- a/ee/app/helpers/ee/groups/group_members_helper.rb +++ b/ee/app/helpers/ee/groups/group_members_helper.rb @@ -16,7 +16,7 @@ def group_members_app_data(group, members:, invited:, access_requests:, banned:, super.merge!({ can_export_members: can?(current_user, :export_group_memberships, group), export_csv_path: export_csv_group_group_members_path(group), - can_filter_by_enterprise: can?(current_user, :admin_group_member, group) && group.root_ancestor.saml_enabled?, + can_filter_by_enterprise: group.domain_verification_available? && can?(current_user, :admin_group_member, group), banned: group_members_list_data(group, banned) }) end diff --git a/ee/app/models/ee/group_member.rb b/ee/app/models/ee/group_member.rb index bcc1d987cd1613399904e1024edd109a09b70375..1c7a8cc684ab3b0ec49c0d9e649f0430701bf03d 100644 --- a/ee/app/models/ee/group_member.rb +++ b/ee/app/models/ee/group_member.rb @@ -48,7 +48,7 @@ def member_of_group?(group, user) def filter_by_enterprise_users(value) subquery = ::UserDetail.where( - ::UserDetail.arel_table[:provisioned_by_group_id].eq(arel_table[:source_id]).and( + ::UserDetail.arel_table[:enterprise_group_id].eq(arel_table[:source_id]).and( ::UserDetail.arel_table[:user_id].eq(arel_table[:user_id])) ) @@ -64,6 +64,10 @@ def provisioned_by_this_group? user&.user_detail&.provisioned_by_group_id == source_id end + def enterprise_user_of_this_group? + user&.user_detail&.enterprise_group_id == source_id + end + private override :access_level_inclusion diff --git a/ee/app/models/ee/project_member.rb b/ee/app/models/ee/project_member.rb index 3d18bf904cb0860f8efb3b2e4bbaa4e09f675c42..22690c5a3513a0d8cca93c2e2ea3cd7789e542d6 100644 --- a/ee/app/models/ee/project_member.rb +++ b/ee/app/models/ee/project_member.rb @@ -57,6 +57,10 @@ def provisioned_by_this_group? false end + def enterprise_user_of_this_group? + false + end + def group_saml_identity(root_ancestor: false) return unless group diff --git a/ee/app/models/ee/user.rb b/ee/app/models/ee/user.rb index 47676d405a479671e5923110e6dc58405ddcef32..ebd2e86bf8e951b746a45afb9eb7d33fd8ff60f9 100644 --- a/ee/app/models/ee/user.rb +++ b/ee/app/models/ee/user.rb @@ -543,12 +543,10 @@ def namespace_ban_for(namespace) namespace_bans.find_by!(namespace: namespace) end - def can_group_owner_disable_two_factor?(group, user) - return false unless group && user + def can_group_owner_disable_two_factor?(group, current_user) + return false unless group && current_user - group.root? && - group_provisioned_user?(group) && - group.owned_by?(user) + group.domain_verification_available? && enterprise_user_of_group?(group) && group.owned_by?(current_user) end def third_party_ai_features_enabled? @@ -645,10 +643,6 @@ def gitlab_com_member? end strong_memoize_attr :gitlab_com_member? - def group_provisioned_user?(group) - self.provisioned_by_group_id == group.id - end - def block_auto_created_users? if ldap_user? provider = ldap_identity.provider diff --git a/ee/app/serializers/ee/member_entity.rb b/ee/app/serializers/ee/member_entity.rb index e6ab8cf3924fd8bc81effc137387a36c26385c1e..563c8458625bb3f854710bc4081ac695f1d4a1ba 100644 --- a/ee/app/serializers/ee/member_entity.rb +++ b/ee/app/serializers/ee/member_entity.rb @@ -19,7 +19,7 @@ module MemberEntity expose :override, as: :is_overridden - expose :provisioned_by_this_group?, as: :provisioned_by_this_group + expose :enterprise_user_of_this_group?, as: :enterprise_user_of_this_group expose :can_ban?, as: :can_ban expose :can_unban?, as: :can_unban diff --git a/ee/spec/features/groups/members/manage_members_spec.rb b/ee/spec/features/groups/members/manage_members_spec.rb index 108fbab4ae4437470432e375025eaec88389a57e..4f035177e860d04f473d9e0b3d8646e5ca7d7af7 100644 --- a/ee/spec/features/groups/members/manage_members_spec.rb +++ b/ee/spec/features/groups/members/manage_members_spec.rb @@ -12,7 +12,12 @@ let_it_be(:user1) { create(:user, name: 'John Doe') } let_it_be(:user2) { create(:user, name: 'Mary Jane') } let_it_be(:user3) { create(:user, name: 'Peter Parker') } - let_it_be(:enterprise_user) { create(:user, :two_factor, provisioned_by_group_id: group.id) } + let_it_be(:enterprise_user) do + create(:user, :two_factor).tap do |user| + user.user_detail.update!(enterprise_group_id: group.id) + end + end + let_it_be(:ultimate_plan, reload: true) { create(:ultimate_plan) } before do @@ -226,26 +231,32 @@ def add_user_by_email(role) sign_in(user1) end - it 'can disable two-factor authentication', :js do - group.add_owner(user1) - group.add_developer(enterprise_user) + context 'when domain_verification feature is available for the group' do + before do + stub_licensed_features(domain_verification: true) + end - visit group_group_members_path(group) + it 'can disable two-factor authentication', :js do + group.add_owner(user1) + group.add_developer(enterprise_user) - page.within find_member_row(enterprise_user) do - show_actions - click_button s_('Members|Disable two-factor authentication') - end + visit group_group_members_path(group) - within_modal do - click_button _('Disable') - end + page.within find_member_row(enterprise_user) do + show_actions + click_button s_('Members|Disable two-factor authentication') + end - wait_for_requests + within_modal do + click_button _('Disable') + end - page.within find_member_row(enterprise_user) do - show_actions - expect(page).not_to have_button(s_('Members|Disable two-factor authentication')) + wait_for_requests + + page.within find_member_row(enterprise_user) do + show_actions + expect(page).not_to have_button(s_('Members|Disable two-factor authentication')) + end end end end diff --git a/ee/spec/finders/ee/group_members_finder_spec.rb b/ee/spec/finders/ee/group_members_finder_spec.rb index a7ab1d903f1db6d6f4b178896669401179e04f03..ae1cbecbb2a11c2bedfe8762765350cd17423970 100644 --- a/ee/spec/finders/ee/group_members_finder_spec.rb +++ b/ee/spec/finders/ee/group_members_finder_spec.rb @@ -190,22 +190,25 @@ end end - context 'filter by enterprise users' do - let_it_be(:saml_provider) { create(:saml_provider, group: group) } - let_it_be(:enterprise_member_1_of_root_group) { group.add_developer(create(:user, provisioned_by_group_id: group.id)) } - let_it_be(:enterprise_member_2_of_root_group) { group.add_developer(create(:user, provisioned_by_group_id: group.id)) } + context 'filter by enterprise users', :saas do + let_it_be(:enterprise_user_member_1_of_root_group) { group.add_developer(create(:user_detail, enterprise_group_id: group.id).user) } + let_it_be(:enterprise_user_member_2_of_root_group) { group.add_developer(create(:user_detail, enterprise_group_id: group.id).user) } let(:all_members) do [ group_owner_membership, group_member_membership, dedicated_member_account_membership, - enterprise_member_1_of_root_group, - enterprise_member_2_of_root_group + enterprise_user_member_1_of_root_group, + enterprise_user_member_2_of_root_group ] end - context 'the group has SAML enabled' do + context 'when domain_verification feature is available for the group' do + before do + stub_licensed_features(domain_verification: true) + end + context 'when requested by owner' do let(:current_user) { group_owner_membership.user } @@ -213,7 +216,7 @@ it 'returns Enterprise members when the filter is `true`' do result = described_class.new(group, current_user, params: { enterprise: 'true' }).execute - expect(result.to_a).to match_array([enterprise_member_1_of_root_group, enterprise_member_2_of_root_group]) + expect(result.to_a).to match_array([enterprise_user_member_1_of_root_group, enterprise_user_member_2_of_root_group]) end it 'returns members that are not Enterprise members when the filter is `false`' do @@ -234,30 +237,6 @@ expect(result.to_a).to match_array(all_members) end end - - context 'inherited members of the group' do - let_it_be(:subgroup) { create(:group, parent: group) } - let_it_be(:subgroup_member_membership) { subgroup.add_developer(create(:user)) } - - it 'returns all members including inherited members, that are Enterprise members, when the filter is `true`' do - result = described_class.new(subgroup, current_user, params: { enterprise: 'true' }).execute - - expect(result.to_a).to match_array([enterprise_member_1_of_root_group, enterprise_member_2_of_root_group]) - end - - it 'returns all members including inherited members, that are not Enterprise members, when the filter is `false`' do - result = described_class.new(subgroup, current_user, params: { enterprise: 'false' }).execute - - expect(result.to_a).to match_array( - [ - group_owner_membership, - group_member_membership, - dedicated_member_account_membership, - subgroup_member_membership - ] - ) - end - end end context 'when requested by non-owner' do @@ -271,15 +250,15 @@ end end - context 'the group does not have SAML enabled' do + context 'when domain_verification feature is not available for the group' do before do - group.saml_provider.destroy! + stub_licensed_features(domain_verification: false) end context 'when requested by owner' do let(:current_user) { group_owner_membership.user } - it 'returns all members, because `Enterprise` filter can only be applied on groups that have SAML enabled' do + it 'returns all members, because `Enterprise` filter can only be applied on a paid top-level group with domain_verification feature available' do result = described_class.new(group, current_user, params: { enterprise: 'true' }).execute expect(result.to_a).to match_array(all_members) diff --git a/ee/spec/fixtures/api/schemas/entities/member.json b/ee/spec/fixtures/api/schemas/entities/member.json index 3064619d863dbddc69c9725bcf02ef69f6b36c48..17f21ec91bcc5576f1303573b3f3774a16221229 100644 --- a/ee/spec/fixtures/api/schemas/entities/member.json +++ b/ee/spec/fixtures/api/schemas/entities/member.json @@ -1,7 +1,9 @@ { "type": "object", "allOf": [ - { "$ref": "../../../../../../spec/fixtures/api/schemas/entities/member.json" }, + { + "$ref": "../../../../../../spec/fixtures/api/schemas/entities/member.json" + }, { "required": [ "using_license", @@ -9,15 +11,42 @@ "group_managed_account", "can_override", "is_overridden", - "provisioned_by_this_group" + "enterprise_user_of_this_group" ], "properties": { - "using_license": { "type": ["boolean", "null"] }, - "group_sso": { "type": ["boolean", "null"] }, - "group_managed_account": { "type": ["boolean", "null"] }, - "can_override": { "type": ["boolean"] }, - "is_overridden": { "type": ["boolean"] }, - "provisioned_by_this_group": { "type": ["boolean"] } + "using_license": { + "type": [ + "boolean", + "null" + ] + }, + "group_sso": { + "type": [ + "boolean", + "null" + ] + }, + "group_managed_account": { + "type": [ + "boolean", + "null" + ] + }, + "can_override": { + "type": [ + "boolean" + ] + }, + "is_overridden": { + "type": [ + "boolean" + ] + }, + "enterprise_user_of_this_group": { + "type": [ + "boolean" + ] + } } } ] diff --git a/ee/spec/frontend/members/utils_spec.js b/ee/spec/frontend/members/utils_spec.js index 4a4334c558ce890a391db7a1726ecef54adc7d54..6454071a6f92f7853d4d7444b8555864de555dc7 100644 --- a/ee/spec/frontend/members/utils_spec.js +++ b/ee/spec/frontend/members/utils_spec.js @@ -22,12 +22,12 @@ describe('Members Utils', () => { }); it.each` - member | expected - ${{ ...memberMock, usingLicense: true }} | ${{ show: true, text: 'Is using seat', variant: 'neutral' }} - ${{ ...memberMock, groupSso: true }} | ${{ show: true, text: 'SAML', variant: 'info' }} - ${{ ...memberMock, groupManagedAccount: true }} | ${{ show: true, text: 'Managed Account', variant: 'info' }} - ${{ ...memberMock, canOverride: true }} | ${{ show: true, text: 'LDAP', variant: 'info' }} - ${{ ...memberMock, provisionedByThisGroup: true }} | ${{ show: true, text: 'Enterprise', variant: 'info' }} + member | expected + ${{ ...memberMock, usingLicense: true }} | ${{ show: true, text: 'Is using seat', variant: 'neutral' }} + ${{ ...memberMock, groupSso: true }} | ${{ show: true, text: 'SAML', variant: 'info' }} + ${{ ...memberMock, groupManagedAccount: true }} | ${{ show: true, text: 'Managed Account', variant: 'info' }} + ${{ ...memberMock, canOverride: true }} | ${{ show: true, text: 'LDAP', variant: 'info' }} + ${{ ...memberMock, enterpriseUserOfThisGroup: true }} | ${{ show: true, text: 'Enterprise', variant: 'info' }} `('returns expected output for "$expected.text" badge', ({ member, expected }) => { expect( generateBadges({ member, isCurrentUser: true, canManageMembers: true }), diff --git a/ee/spec/helpers/ee/groups/group_members_helper_spec.rb b/ee/spec/helpers/ee/groups/group_members_helper_spec.rb index 68b1ac78c043944b8ab7ec2d24dca54edea286e0..7767326c65018c052cbe50f6a40f4b1ed13f48d5 100644 --- a/ee/spec/helpers/ee/groups/group_members_helper_spec.rb +++ b/ee/spec/helpers/ee/groups/group_members_helper_spec.rb @@ -48,9 +48,24 @@ expect(subject[:export_csv_path]).not_to be_nil end - it 'adds `can_filter_by_enterprise`' do - allow(group.root_ancestor).to receive(:saml_enabled?).and_return(true) - expect(subject[:can_filter_by_enterprise]).to eq(true) + describe '`can_filter_by_enterprise`', :saas do + where(:domain_verification_availabe_for_group, :can_admin_group_member, :expected_value) do + true | true | true + true | false | false + false | true | false + false | false | false + end + + with_them do + before do + stub_licensed_features(domain_verification: domain_verification_availabe_for_group) + allow(helper).to receive(:can?).with(current_user, :admin_group_member, group).and_return(can_admin_group_member) + end + + it "is set to #{params[:expected_value]}" do + expect(subject[:can_filter_by_enterprise]).to eq(expected_value) + end + end end context 'banned members' do diff --git a/ee/spec/models/ee/user_spec.rb b/ee/spec/models/ee/user_spec.rb index d03048a0991459efedf67d951208260250fc62cc..efd94263eb429f4c02f3a724ecceb9a8cb74c74a 100644 --- a/ee/spec/models/ee/user_spec.rb +++ b/ee/spec/models/ee/user_spec.rb @@ -2608,66 +2608,58 @@ end end - describe '#can_group_owner_disable_two_factor?' do + describe '#can_group_owner_disable_two_factor?', :saas do let_it_be(:group) { create(:group) } - let_it_be(:owner) { create(:user) } + let_it_be(:current_user) { create(:user) } - context 'when current_user is a group owner' do - before do - group.add_owner(owner) - end - - context 'when user is provisioned by group' do - let_it_be(:user) { create(:user, :two_factor, provisioned_by_group_id: group.id) } + using RSpec::Parameterized::TableSyntax - context 'when group is root group' do - it 'returns true' do - expect(user.can_group_owner_disable_two_factor?(group, owner)).to eq true - end - end + where(:domain_verification_availabe_for_group, :user_is_enterprise_user_of_the_group, :current_user_is_group_owner, :expected_value) do + false | false | false | false + false | false | true | false + false | true | false | false + false | true | true | false + true | false | false | false + true | false | true | false + true | true | false | false + true | true | true | true + end - context 'when group is not root group' do - let(:parent) { build(:group) } + with_them do + before do + stub_licensed_features(domain_verification: domain_verification_availabe_for_group) - before do - group.parent = parent - parent.add_owner(create(:user)) - end + user.user_detail.enterprise_group_id = user_is_enterprise_user_of_the_group ? group.id : -42 - it 'returns false' do - expect(user.can_group_owner_disable_two_factor?(group, owner)).to eq false - end + if current_user_is_group_owner + group.add_owner(current_user) + else + group.add_maintainer(current_user) end end - context 'when user is not provisioned by group' do - let_it_be(:user) { create(:user) } - - it 'returns false' do - expect(user.can_group_owner_disable_two_factor?(group, owner)).to eq false - end + it "returns #{params[:expected_value]}" do + expect(user.can_group_owner_disable_two_factor?(group, current_user)).to eq expected_value end end - context 'when current_user is not a group owner' do - let_it_be(:user) { create(:user, :two_factor, provisioned_by_group_id: group.id) } - - before do - group.add_maintainer(owner) - end - + context 'when group passed is nil' do it 'returns false' do - expect(user.can_group_owner_disable_two_factor?(group, owner)).to eq false + expect(user.can_group_owner_disable_two_factor?(nil, current_user)).to eq false end end context 'when current_user passed is nil' do - let_it_be(:user) { create(:user, :two_factor, provisioned_by_group_id: group.id) } - it 'returns false' do expect(user.can_group_owner_disable_two_factor?(group, nil)).to eq false end end + + context 'when group and current_user passed are nil' do + it 'returns false' do + expect(user.can_group_owner_disable_two_factor?(nil, nil)).to eq false + end + end end describe '#third_party_ai_features_enabled' do diff --git a/ee/spec/models/group_member_spec.rb b/ee/spec/models/group_member_spec.rb index 0749cacca58ad66d43acdc62027c989533b0af2a..e5f4245a4cdfa5d75433fe61ca9a230e9d8b091a 100644 --- a/ee/spec/models/group_member_spec.rb +++ b/ee/spec/models/group_member_spec.rb @@ -158,20 +158,20 @@ describe '.filter_by_enterprise_users' do let_it_be(:group) { create(:group) } - let_it_be(:provisioned_member_1_of_group) { group.add_developer(create(:user, provisioned_by_group_id: group.id)) } - let_it_be(:provisioned_member_2_of_group) { group.add_developer(create(:user, provisioned_by_group_id: group.id)) } - let_it_be(:normal_group_member) { group.add_developer(create(:user)) } + let_it_be(:enterprise_user_member_1_of_group) { group.add_developer(create(:user_detail, enterprise_group_id: group.id).user) } + let_it_be(:enterprise_user_member_2_of_group) { group.add_developer(create(:user_detail, enterprise_group_id: group.id).user) } + let_it_be(:non_enterprise_user_member_of_group) { group.add_developer(create(:user)) } - it 'returns members that are provisioned by a group when the filter is `true`' do + it 'returns members that are enterprise users of a group when the filter is `true`' do result = described_class.filter_by_enterprise_users(true) - expect(result.to_a).to match_array([provisioned_member_1_of_group, provisioned_member_2_of_group]) + expect(result.to_a).to match_array([enterprise_user_member_1_of_group, enterprise_user_member_2_of_group]) end - it 'returns members that are not provisioned by a group when the filter is `false`' do + it 'returns members that are not enterprise users of a group when the filter is `false`' do result = described_class.filter_by_enterprise_users(false) - expect(result.to_a).to match_array([normal_group_member]) + expect(result.to_a).to match_array([non_enterprise_user_member_of_group]) end end @@ -414,6 +414,32 @@ end end + describe 'enterprise_user_of_this_group?' do + let_it_be(:group) { create(:group) } + + let(:user) { build(:user) } + let(:member) { build(:group_member, group: group, user: user) } + let(:invited) { build(:group_member, :invited, group: group) } + + subject { member.enterprise_user_of_this_group? } + + context 'when member is an enterprise user of this group' do + let!(:user_detail) { build(:user_detail, user: user, enterprise_group_id: group.id) } + + it { is_expected.to eq(true) } + end + + context 'when member is not an enterprise user of this group' do + it { is_expected.to eq(false) } + end + + context 'when member does not have a related user (invited member)' do + let(:member) { invited } + + it { is_expected.to eq(false) } + end + end + def webhook_data(group_member, event) { headers: { 'Content-Type' => 'application/json', 'User-Agent' => "GitLab/#{Gitlab::VERSION}", 'X-Gitlab-Event' => 'Member Hook' }, diff --git a/ee/spec/models/project_member_spec.rb b/ee/spec/models/project_member_spec.rb index 4cc3364edf41e5ba78d6ea861f8306ebba564175..ac831392d26b8ee3043b9fe485d550396c1e7b05 100644 --- a/ee/spec/models/project_member_spec.rb +++ b/ee/spec/models/project_member_spec.rb @@ -92,6 +92,14 @@ it { is_expected.to eq(false) } end + describe '#enterprise_user_of_this_group?' do + let_it_be(:member) { build(:project_member) } + + subject { member.enterprise_user_of_this_group? } + + it { is_expected.to eq(false) } + end + describe '#state' do let!(:group) { create(:group) } let!(:project) { create(:project, group: group) } diff --git a/ee/spec/requests/groups/two_factor_auths_controller_spec.rb b/ee/spec/requests/groups/two_factor_auths_controller_spec.rb index 27041191f4388eba279a6f773e1b6fcb4a6fefb1..e88f632166e8967007b7b3a3514ab69394420904 100644 --- a/ee/spec/requests/groups/two_factor_auths_controller_spec.rb +++ b/ee/spec/requests/groups/two_factor_auths_controller_spec.rb @@ -14,111 +14,133 @@ sign_in(current_user) end - describe 'DELETE #destroy' do - context 'when signed in user is a group owner' do + describe 'DELETE #destroy', :saas do + context 'when domain_verification feature is available for the group' do before do - group.add_owner(current_user) + stub_licensed_features(domain_verification: true) end - context 'when the user has 2FA enabled' do - let(:user) { create(:user, :two_factor, provisioned_by_group_id: provisioned_by_group_id) } + context 'when signed in user is a group owner' do + before do + group.add_owner(current_user) + end - context 'when the user is provisioned by the current group' do - let_it_be(:provisioned_by_group_id) { group.id } + context 'when the user has 2FA enabled' do + let(:user) { create(:user, :two_factor) } - it 'successfully disables 2FA and redirects with a success notice', :aggregate_failures do - delete_two_factor + context 'when user is an enterprise user of the current group' do + before do + user.user_detail.update!(enterprise_group_id: group.id) + end - expect(user.reload.two_factor_enabled?).to eq(false) - expect(response).to redirect_to(group_group_members_path(group)) - expect(flash[:notice]) - .to eq format(_("Two-factor authentication has been disabled successfully for %{username}!"), - { username: user.username }) - end + it 'successfully disables 2FA and redirects with a success notice', :aggregate_failures do + delete_two_factor - it 'returns not found for nil user_id' do - delete group_two_factor_auth_path(group_id: group.path, user_id: nil) + expect(user.reload.two_factor_enabled?).to eq(false) + expect(response).to redirect_to(group_group_members_path(group)) + expect(flash[:notice]) + .to eq format(_("Two-factor authentication has been disabled successfully for %{username}!"), + { username: user.username }) + end - expect(response).to have_gitlab_http_status(:not_found) - end + it 'returns not found for nil user_id' do + delete group_two_factor_auth_path(group_id: group.path, user_id: nil) - it 'returns not found for non-existent user_id' do - delete group_two_factor_auth_path(group_id: group.path, user_id: non_existing_record_id) + expect(response).to have_gitlab_http_status(:not_found) + end - expect(response).to have_gitlab_http_status(:not_found) - end + it 'returns not found for non-existent user_id' do + delete group_two_factor_auth_path(group_id: group.path, user_id: non_existing_record_id) - it 'shows unauthorized error when group is not a root group', :aggregate_failures do - parent = create(:group) - subgroup = create(:group, parent: parent) - subgroup.add_owner(current_user) + expect(response).to have_gitlab_http_status(:not_found) + end - delete group_two_factor_auth_path(group_id: subgroup.full_path, user_id: user.id) + it 'shows unauthorized error when group is not a root group', :aggregate_failures do + parent = create(:group) + subgroup = create(:group, parent: parent) + subgroup.add_owner(current_user) - expect(response).to redirect_to(group_group_members_path(subgroup)) - expect(flash[:alert]) - .to eq format(_("You are not authorized to perform this action")) + delete group_two_factor_auth_path(group_id: subgroup.full_path, user_id: user.id) + + expect(response).to redirect_to(group_group_members_path(subgroup)) + expect(flash[:alert]) + .to eq format(_("You are not authorized to perform this action")) + end end - end - context 'when user is not provisioned by current group' do - let_it_be(:provisioned_by_group_id) { create(:group) } + context 'when user is not an enterprise user of the current group' do + before do + user.user_detail.update!(enterprise_group_id: -42) + end - it 'fails with unauthorized error', :aggregate_failures do - delete_two_factor + it 'fails with unauthorized error', :aggregate_failures do + delete_two_factor - expect(response).to redirect_to(group_group_members_path(group)) - expect(flash[:alert]) - .to eq format(_("You are not authorized to perform this action")) + expect(response).to redirect_to(group_group_members_path(group)) + expect(flash[:alert]) + .to eq format(_("You are not authorized to perform this action")) + end end end - end - context 'when the user does not have 2FA enabled' do - let_it_be(:user) { create(:user, provisioned_by_group_id: group.id) } + context 'when the user does not have 2FA enabled' do + let(:user) { create(:user) } - it 'redirects to group_group_members_path' do - delete_two_factor + context 'when user is an enterprise user of the current group' do + before do + user.user_detail.update!(enterprise_group_id: group.id) + end - expect(response).to redirect_to(group_group_members_path(group)) - end + it 'redirects to group_group_members_path' do + delete_two_factor - it 'displays an alert on failure' do - delete_two_factor + expect(response).to redirect_to(group_group_members_path(group)) + end - expect(flash[:alert]) - .to eq _('Two-factor authentication is not enabled for this user') + it 'displays an alert on failure' do + delete_two_factor + + expect(flash[:alert]) + .to eq _('Two-factor authentication is not enabled for this user') + end + end end end - end - context 'when signed in user is not a group owner' do - context 'when the user has 2FA enabled' do - let(:user) { create(:user, :two_factor, provisioned_by_group_id: group.id) } + context 'when signed in user is not a group owner' do + context 'when the user has 2FA enabled' do + let(:user) { create(:user, :two_factor) } - it 'responds with a 404' do - group.add_maintainer(current_user) + context 'when user is an enterprise user of the current group' do + before do + user.user_detail.update!(enterprise_group_id: group.id) + end - delete_two_factor + it 'responds with a 404' do + group.add_maintainer(current_user) - expect(response).to have_gitlab_http_status(:not_found) - end + delete_two_factor - context 'when signed in user is not a group member' do - it 'responds with a 404' do - delete_two_factor + expect(response).to have_gitlab_http_status(:not_found) + end - expect(response).to have_gitlab_http_status(:not_found) - end - end + context 'when signed in user is not a group member' do + it 'responds with a 404' do + delete_two_factor - context "when invalid group id is passed" do - def delete_two_factor_invalid_group - delete group_two_factor_auth_path(group_id: 'non_existent_group', user_id: user.id) - end + expect(response).to have_gitlab_http_status(:not_found) + end + end + + context "when invalid group id is passed" do + def delete_two_factor_invalid_group + delete group_two_factor_auth_path(group_id: 'non_existent_group', user_id: user.id) + end - it 'throws routing error' do - expect { delete_two_factor_invalid_group }.to raise_error(ActionController::RoutingError) + it 'throws routing error' do + expect { delete_two_factor_invalid_group }.to raise_error(ActionController::RoutingError) + end + end end end end diff --git a/ee/spec/serializers/member_entity_spec.rb b/ee/spec/serializers/member_entity_spec.rb index 20460a6a6204fe86c4dab391748fd126d54dbfe9..8dd182ef62c04e96eb2b563e0e1c6dc6218ea6aa 100644 --- a/ee/spec/serializers/member_entity_spec.rb +++ b/ee/spec/serializers/member_entity_spec.rb @@ -38,10 +38,10 @@ expect(entity_hash[:can_override]).to be(true) end - it 'correctly exposes `provisioned_by_this_group`' do - allow(member).to receive(:provisioned_by_this_group?).and_return(true) + it 'correctly exposes `enterprise_user_of_this_group`' do + allow(member).to receive(:enterprise_user_of_this_group?).and_return(true) - expect(entity_hash[:provisioned_by_this_group]).to be(true) + expect(entity_hash[:enterprise_user_of_this_group]).to be(true) end it 'correctly exposes `banned`' do diff --git a/ee/spec/services/ee/two_factor/destroy_service_spec.rb b/ee/spec/services/ee/two_factor/destroy_service_spec.rb index 68ca62315a8b5515ae065edbc45f3825a505a37c..4839d1144a445d38f369a0c7d9244f987e6888a8 100644 --- a/ee/spec/services/ee/two_factor/destroy_service_spec.rb +++ b/ee/spec/services/ee/two_factor/destroy_service_spec.rb @@ -5,44 +5,41 @@ RSpec.describe TwoFactor::DestroyService, feature_category: :system_access do let_it_be(:current_user) { create(:user, :two_factor) } let_it_be(:group) { create(:group) } - let_it_be(:user) { create(:user, :two_factor, provisioned_by_group_id: group.id) } + let_it_be(:user, reload: true) { create(:user, :two_factor) } subject(:disable_2fa_with_group) { described_class.new(current_user, user: user, group: group).execute } - shared_examples_for 'throws unauthorized error' do - it 'returns error' do - expect(disable_2fa_with_group).to eq( - { - status: :error, - message: 'You are not authorized to perform this action' - } - ) - end - end - - context "when current user is a group owner" do - before do - group.add_owner(current_user) - end + describe 'disabling two-factor authentication with group', :saas do + shared_examples 'does not disable two-factor authentication' do + it 'returns error' do + expect(disable_2fa_with_group).to eq( + { + status: :error, + message: 'You are not authorized to perform this action' + } + ) + end - context 'when unlicensed' do - before do - stub_licensed_features(admin_audit_log: false, audit_events: false, extended_audit_events: false) + it 'does not disable the two-factor authentication of the user' do + expect { disable_2fa_with_group }.not_to change { user.reload.two_factor_enabled? }.from(true) end - it 'does not track audit event' do - expect { disable_2fa_with_group }.not_to change { AuditEvent.count } + it 'does not create an audit event' do + expect { disable_2fa_with_group }.not_to change(AuditEvent, :count) end end - context "when licensed" do - before do - stub_licensed_features(admin_audit_log: true, audit_events: true, extended_audit_events: true) + shared_examples 'disables two-factor authentication' do + it 'returns success' do + expect(disable_2fa_with_group).to eq({ status: :success }) + end + + it 'disables the two-factor authentication of the user' do + expect { disable_2fa_with_group }.to change { user.reload.two_factor_enabled? }.from(true).to(false) end it 'creates an audit event', :aggregate_failures do expect { disable_2fa_with_group }.to change(AuditEvent, :count).by(1) - .and change { user.reload.two_factor_enabled? }.from(true).to(false) expect(AuditEvent.last).to have_attributes( author: current_user, @@ -53,59 +50,45 @@ details: include(custom_message: 'Disabled two-factor authentication') ) end - - context "when user is not provisioned by current group" do - let(:new_group) { create(:group) } - let(:user) { create(:user, :two_factor, provisioned_by_group_id: new_group.id) } - - it_behaves_like 'throws unauthorized error' - end - - context "when group is non root" do - let(:parent) { build(:group) } - - before do - group.parent = parent - parent.add_owner(create(:user)) - end - - it_behaves_like 'throws unauthorized error' - end - - context "when user is not provisioned by group" do - let(:user) { create(:user, :two_factor) } - - it_behaves_like 'throws unauthorized error' - end end - end - - context "when user is not a group owner" do - it_behaves_like 'throws unauthorized error' - context "when group is nil" do - let(:group) { nil } - - it_behaves_like 'throws unauthorized error' + using RSpec::Parameterized::TableSyntax + + where( + :domain_verification_availabe_for_group, + :user_is_enterprise_user_of_the_group, + :current_user_is_group_owner, + :shared_examples + ) do + false | false | false | 'does not disable two-factor authentication' + false | false | true | 'does not disable two-factor authentication' + false | true | false | 'does not disable two-factor authentication' + false | true | true | 'does not disable two-factor authentication' + true | false | false | 'does not disable two-factor authentication' + true | false | true | 'does not disable two-factor authentication' + true | true | false | 'does not disable two-factor authentication' + true | true | true | 'disables two-factor authentication' end - end - context "when user passed is nil" do - let(:user) { nil } + with_them do + before do + stub_licensed_features( + domain_verification: domain_verification_availabe_for_group, + admin_audit_log: true, + audit_events: true, + extended_audit_events: true + ) - it_behaves_like 'throws unauthorized error' - end + user.user_detail.update!(enterprise_group_id: user_is_enterprise_user_of_the_group ? group.id : -42) - context 'when disabling two-factor authentication fails' do - before do - allow_next_instance_of(Users::UpdateService) do |instance| - allow(instance).to receive(:execute) - .and_return({ status: :error }) + if current_user_is_group_owner + group.add_owner(current_user) + else + group.add_maintainer(current_user) + end end - end - it 'does not create an audit event' do - expect { subject }.not_to change(AuditEvent, :count) + include_examples params[:shared_examples] end end end diff --git a/spec/frontend/members/mock_data.js b/spec/frontend/members/mock_data.js index 161e96c0c488c17b8982cc876f7643001c0c2510..67528aef1719ff721648b7e8ebbada80e8a6a907 100644 --- a/spec/frontend/members/mock_data.js +++ b/spec/frontend/members/mock_data.js @@ -41,7 +41,7 @@ export const member = { usingLicense: false, groupSso: false, groupManagedAccount: false, - provisionedByThisGroup: false, + enterpriseUserOfThisGroup: false, validRoles: { Guest: 10, Reporter: 20,