diff options
-rw-r--r-- | app/assets/javascripts/members/components/avatars/user_avatar.vue | 8 | ||||
-rw-r--r-- | app/assets/javascripts/members/utils.js | 4 | ||||
-rw-r--r-- | app/helpers/projects/project_members_helper.rb | 4 | ||||
-rw-r--r-- | changelogs/unreleased/24908-user-2fa-status-should-not-be-publicly-shown.yml | 5 | ||||
-rw-r--r-- | doc/user/permissions.md | 2 | ||||
-rw-r--r-- | spec/features/groups/members/list_members_spec.rb | 42 | ||||
-rw-r--r-- | spec/features/projects/members/list_spec.rb | 44 | ||||
-rw-r--r-- | spec/frontend/members/components/avatars/user_avatar_spec.js | 26 | ||||
-rw-r--r-- | spec/frontend/members/mock_data.js | 2 | ||||
-rw-r--r-- | spec/frontend/members/utils_spec.js | 37 | ||||
-rw-r--r-- | spec/helpers/projects/project_members_helper_spec.rb | 4 |
11 files changed, 159 insertions, 19 deletions
diff --git a/app/assets/javascripts/members/components/avatars/user_avatar.vue b/app/assets/javascripts/members/components/avatars/user_avatar.vue index 79dda3c1379..658fb43cecb 100644 --- a/app/assets/javascripts/members/components/avatars/user_avatar.vue +++ b/app/assets/javascripts/members/components/avatars/user_avatar.vue @@ -5,6 +5,7 @@ import { GlBadge, GlSafeHtmlDirective as SafeHtml, } from '@gitlab/ui'; +import { mapState } from 'vuex'; import { generateBadges } from 'ee_else_ce/members/utils'; import { glEmojiTag } from '~/emoji'; import { __ } from '~/locale'; @@ -34,11 +35,16 @@ export default { }, }, computed: { + ...mapState(['canManageMembers']), user() { return this.member.user; }, badges() { - return generateBadges(this.member, this.isCurrentUser).filter((badge) => badge.show); + return generateBadges({ + member: this.member, + isCurrentUser: this.isCurrentUser, + canManageMembers: this.canManageMembers, + }).filter((badge) => badge.show); }, statusEmoji() { return this.user?.status?.emoji; diff --git a/app/assets/javascripts/members/utils.js b/app/assets/javascripts/members/utils.js index 4de2dadb490..2bf30dd7b6e 100644 --- a/app/assets/javascripts/members/utils.js +++ b/app/assets/javascripts/members/utils.js @@ -13,7 +13,7 @@ import { GROUP_LINK_ACCESS_LEVEL_PROPERTY_NAME, } from './constants'; -export const generateBadges = (member, isCurrentUser) => [ +export const generateBadges = ({ member, isCurrentUser, canManageMembers }) => [ { show: isCurrentUser, text: __("It's you"), @@ -25,7 +25,7 @@ export const generateBadges = (member, isCurrentUser) => [ variant: 'danger', }, { - show: member.user?.twoFactorEnabled, + show: member.user?.twoFactorEnabled && (canManageMembers || isCurrentUser), text: __('2FA'), variant: 'info', }, diff --git a/app/helpers/projects/project_members_helper.rb b/app/helpers/projects/project_members_helper.rb index 99c1b742da4..662afbcfd25 100644 --- a/app/helpers/projects/project_members_helper.rb +++ b/app/helpers/projects/project_members_helper.rb @@ -40,7 +40,7 @@ module Projects::ProjectMembersHelper members: project_members_data_json(project, members), member_path: project_project_member_path(project, ':id'), source_id: project.id, - can_manage_members: can_manage_project_members?(project) + can_manage_members: can_manage_project_members?(project).to_s } end @@ -49,7 +49,7 @@ module Projects::ProjectMembersHelper members: project_group_links_data_json(group_links), member_path: project_group_link_path(project, ':id'), source_id: project.id, - can_manage_members: can_manage_project_members?(project) + can_manage_members: can_manage_project_members?(project).to_s } end end diff --git a/changelogs/unreleased/24908-user-2fa-status-should-not-be-publicly-shown.yml b/changelogs/unreleased/24908-user-2fa-status-should-not-be-publicly-shown.yml new file mode 100644 index 00000000000..b47577d3e69 --- /dev/null +++ b/changelogs/unreleased/24908-user-2fa-status-should-not-be-publicly-shown.yml @@ -0,0 +1,5 @@ +--- +title: Only show 2FA badge to project maintainers and group owners +merge_request: 54646 +author: +type: changed diff --git a/doc/user/permissions.md b/doc/user/permissions.md index 67b18340486..6a05d355fcc 100644 --- a/doc/user/permissions.md +++ b/doc/user/permissions.md @@ -173,6 +173,7 @@ The following table depicts the various user permission levels in a project. | View project Audit Events | | | ✓ (*12*) | ✓ | ✓ | | Manage [push rules](../push_rules/push_rules.md) | | | | ✓ | ✓ | | Manage [project access tokens](project/settings/project_access_tokens.md) **(FREE SELF)** | | | | ✓ | ✓ | +| View 2FA status of members | | | | ✓ | ✓ | | Switch visibility level | | | | | ✓ | | Transfer project to another namespace | | | | | ✓ | | Rename project | | | | | ✓ | @@ -293,6 +294,7 @@ group. | View Value Stream analytics | ✓ | ✓ | ✓ | ✓ | ✓ | | View Billing **(FREE SAAS)** | | | | | ✓ (4) | | View Usage Quotas **(FREE SAAS)** | | | | | ✓ (4) | +| View 2FA status of members | | | | | ✓ | | Filter members by 2FA status | | | | | ✓ | | Administer project compliance frameworks | | | | | ✓ | diff --git a/spec/features/groups/members/list_members_spec.rb b/spec/features/groups/members/list_members_spec.rb index b0a896ec8cb..b81949da85d 100644 --- a/spec/features/groups/members/list_members_spec.rb +++ b/spec/features/groups/members/list_members_spec.rb @@ -47,4 +47,46 @@ RSpec.describe 'Groups > Members > List members', :js do expect(first_row).to have_selector('gl-emoji[data-name="smirk"]') end end + + describe 'when user has 2FA enabled' do + let_it_be(:admin) { create(:admin) } + let_it_be(:user_with_2fa) { create(:user, :two_factor_via_otp) } + + before do + group.add_guest(user_with_2fa) + end + + it 'shows 2FA badge to user with "Owner" access level' do + group.add_owner(user1) + + visit group_group_members_path(group) + + expect(find_member_row(user_with_2fa)).to have_content('2FA') + end + + it 'shows 2FA badge to admins' do + sign_in(admin) + gitlab_enable_admin_mode_sign_in(admin) + + visit group_group_members_path(group) + + expect(find_member_row(user_with_2fa)).to have_content('2FA') + end + + it 'does not show 2FA badge to users with access level below "Owner"' do + group.add_maintainer(user1) + + visit group_group_members_path(group) + + expect(find_member_row(user_with_2fa)).not_to have_content('2FA') + end + + it 'shows 2FA badge to themselves' do + sign_in(user_with_2fa) + + visit group_group_members_path(group) + + expect(find_member_row(user_with_2fa)).to have_content('2FA') + end + end end diff --git a/spec/features/projects/members/list_spec.rb b/spec/features/projects/members/list_spec.rb index b0fe5b9c48a..7cace8e08dd 100644 --- a/spec/features/projects/members/list_spec.rb +++ b/spec/features/projects/members/list_spec.rb @@ -8,7 +8,7 @@ RSpec.describe 'Project members list' do let(:user1) { create(:user, name: 'John Doe') } let(:user2) { create(:user, name: 'Mary Jane') } let(:group) { create(:group) } - let(:project) { create(:project, namespace: group) } + let(:project) { create(:project, :internal, namespace: group) } before do stub_feature_flags(invite_members_group_modal: false) @@ -117,6 +117,48 @@ RSpec.describe 'Project members list' do end end end + + describe 'when user has 2FA enabled' do + let_it_be(:admin) { create(:admin) } + let_it_be(:user_with_2fa) { create(:user, :two_factor_via_otp) } + + before do + project.add_guest(user_with_2fa) + end + + it 'shows 2FA badge to user with "Maintainer" access level' do + project.add_maintainer(user1) + + visit_members_page + + expect(find_member_row(user_with_2fa)).to have_content('2FA') + end + + it 'shows 2FA badge to admins' do + sign_in(admin) + gitlab_enable_admin_mode_sign_in(admin) + + visit_members_page + + expect(find_member_row(user_with_2fa)).to have_content('2FA') + end + + it 'does not show 2FA badge to users with access level below "Maintainer"' do + group.add_developer(user1) + + visit_members_page + + expect(find_member_row(user_with_2fa)).not_to have_content('2FA') + end + + it 'shows 2FA badge to themselves' do + sign_in(user_with_2fa) + + visit_members_page + + expect(find_member_row(user_with_2fa)).to have_content('2FA') + end + end end context 'when `vue_project_members_list` feature flag is disabled' do diff --git a/spec/frontend/members/components/avatars/user_avatar_spec.js b/spec/frontend/members/components/avatars/user_avatar_spec.js index 303c82582a3..3f4d9155c5d 100644 --- a/spec/frontend/members/components/avatars/user_avatar_spec.js +++ b/spec/frontend/members/components/avatars/user_avatar_spec.js @@ -1,21 +1,31 @@ import { GlAvatarLink, GlBadge } from '@gitlab/ui'; import { within } from '@testing-library/dom'; import { mount, createWrapper } from '@vue/test-utils'; +import Vue from 'vue'; +import Vuex from 'vuex'; import UserAvatar from '~/members/components/avatars/user_avatar.vue'; -import { member as memberMock, orphanedMember } from '../../mock_data'; +import { member as memberMock, member2faEnabled, orphanedMember } from '../../mock_data'; + +Vue.use(Vuex); describe('UserAvatar', () => { let wrapper; const { user } = memberMock; - const createComponent = (propsData = {}) => { + const createComponent = (propsData = {}, state = {}) => { wrapper = mount(UserAvatar, { propsData: { member: memberMock, isCurrentUser: false, ...propsData, }, + store: new Vuex.Store({ + state: { + canManageMembers: true, + ...state, + }, + }), }); }; @@ -69,9 +79,9 @@ describe('UserAvatar', () => { describe('badges', () => { it.each` - member | badgeText - ${{ ...memberMock, user: { ...memberMock.user, blocked: true } }} | ${'Blocked'} - ${{ ...memberMock, user: { ...memberMock.user, twoFactorEnabled: true } }} | ${'2FA'} + member | badgeText + ${{ ...memberMock, user: { ...memberMock.user, blocked: true } }} | ${'Blocked'} + ${member2faEnabled} | ${'2FA'} `('renders the "$badgeText" badge', ({ member, badgeText }) => { createComponent({ member }); @@ -83,6 +93,12 @@ describe('UserAvatar', () => { expect(getByText("It's you").exists()).toBe(true); }); + + it('does not render 2FA badge when `canManageMembers` is `false`', () => { + createComponent({ member: member2faEnabled }, { canManageMembers: false }); + + expect(within(wrapper.element).queryByText('2FA')).toBe(null); + }); }); describe('user status', () => { diff --git a/spec/frontend/members/mock_data.js b/spec/frontend/members/mock_data.js index fa324ce1cf9..6a73b2fcf8c 100644 --- a/spec/frontend/members/mock_data.js +++ b/spec/frontend/members/mock_data.js @@ -75,3 +75,5 @@ export const membersJsonString = JSON.stringify(members); export const directMember = { ...member, isDirectMember: true }; export const inheritedMember = { ...member, isDirectMember: false }; + +export const member2faEnabled = { ...member, user: { ...member.user, twoFactorEnabled: true } }; diff --git a/spec/frontend/members/utils_spec.js b/spec/frontend/members/utils_spec.js index f447a4c4ee9..bfb5a4bc7d3 100644 --- a/spec/frontend/members/utils_spec.js +++ b/spec/frontend/members/utils_spec.js @@ -17,6 +17,7 @@ import { member as memberMock, directMember, inheritedMember, + member2faEnabled, group, invite, membersJsonString, @@ -30,7 +31,11 @@ const URL_HOST = 'https://localhost/'; describe('Members Utils', () => { describe('generateBadges', () => { it('has correct properties for each badge', () => { - const badges = generateBadges(memberMock, true); + const badges = generateBadges({ + member: memberMock, + isCurrentUser: true, + canManageMembers: true, + }); badges.forEach((badge) => { expect(badge).toEqual( @@ -44,12 +49,32 @@ describe('Members Utils', () => { }); it.each` - member | expected - ${memberMock} | ${{ show: true, text: "It's you", variant: 'success' }} - ${{ ...memberMock, user: { ...memberMock.user, blocked: true } }} | ${{ show: true, text: 'Blocked', variant: 'danger' }} - ${{ ...memberMock, user: { ...memberMock.user, twoFactorEnabled: true } }} | ${{ show: true, text: '2FA', variant: 'info' }} + member | expected + ${memberMock} | ${{ show: true, text: "It's you", variant: 'success' }} + ${{ ...memberMock, user: { ...memberMock.user, blocked: true } }} | ${{ show: true, text: 'Blocked', variant: 'danger' }} + ${member2faEnabled} | ${{ show: true, text: '2FA', variant: 'info' }} `('returns expected output for "$expected.text" badge', ({ member, expected }) => { - expect(generateBadges(member, true)).toContainEqual(expect.objectContaining(expected)); + expect( + generateBadges({ member, isCurrentUser: true, canManageMembers: true }), + ).toContainEqual(expect.objectContaining(expected)); + }); + + describe('when `canManageMembers` argument is `false`', () => { + describe.each` + description | memberIsCurrentUser | expectedBadgeToBeShown + ${'is not the current user'} | ${false} | ${false} + ${'is the current user'} | ${true} | ${true} + `('when member is $description', ({ memberIsCurrentUser, expectedBadgeToBeShown }) => { + it(`sets 'show' to '${expectedBadgeToBeShown}' for 2FA badge`, () => { + const badges = generateBadges({ + member: member2faEnabled, + isCurrentUser: memberIsCurrentUser, + canManageMembers: false, + }); + + expect(badges.find((badge) => badge.text === '2FA').show).toBe(expectedBadgeToBeShown); + }); + }); }); }); diff --git a/spec/helpers/projects/project_members_helper_spec.rb b/spec/helpers/projects/project_members_helper_spec.rb index 5e0b4df7f7f..1a55840a58a 100644 --- a/spec/helpers/projects/project_members_helper_spec.rb +++ b/spec/helpers/projects/project_members_helper_spec.rb @@ -166,7 +166,7 @@ RSpec.describe Projects::ProjectMembersHelper do members: helper.project_members_data_json(project, present_members(project_members)), member_path: '/foo-bar/-/project_members/:id', source_id: project.id, - can_manage_members: true + can_manage_members: 'true' }) end end @@ -193,7 +193,7 @@ RSpec.describe Projects::ProjectMembersHelper do members: helper.project_group_links_data_json(project_group_links), member_path: '/foo-bar/-/group_links/:id', source_id: project.id, - can_manage_members: true + can_manage_members: 'true' }) end end |