Merge branch '24908-user-2fa-status-should-not-be-publicly-shown' into 'master'

Only show 2FA badge to project maintainers and group owners

See merge request gitlab-org/gitlab!54646

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;

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',
   },

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

changelogs/unreleased/24908-user-2fa-status-should-not-be-publicly-shown.yml

+---
+title: Only show 2FA badge to project maintainers and group owners
+merge_request: 54646
+author:
+type: changed

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               |       |          |           |            | ✓     |
 

ee/app/assets/javascripts/members/utils.js

@@ -14,8 +14,8 @@ export {
   canUpdate,
 } from '~/members/utils';
 
-export const generateBadges = (member, isCurrentUser) => [
-  ...CEGenerateBadges(member, isCurrentUser),
+export const generateBadges = ({ member, isCurrentUser, canManageMembers }) => [
+  ...CEGenerateBadges({ member, isCurrentUser, canManageMembers }),
   {
     show: member.usingLicense,
     text: __('Is using seat'),

ee/spec/frontend/members/components/avatars/user_avatar_spec.js

 import { GlBadge } from '@gitlab/ui';
 import { mount } from '@vue/test-utils';
+import Vue from 'vue';
+import Vuex from 'vuex';
 import { member as memberMock } from 'jest/members/mock_data';
 import UserAvatar from '~/members/components/avatars/user_avatar.vue';
 
+Vue.use(Vuex);
+
 describe('UserAvatar', () => {
   let wrapper;
 
@@ -12,6 +16,11 @@ describe('UserAvatar', () => {
         isCurrentUser: false,
         ...propsData,
       },
+      store: new Vuex.Store({
+        state: {
+          canManageMembers: true,
+        },
+      }),
     });
   };
 

ee/spec/frontend/members/utils_spec.js

@@ -10,7 +10,11 @@ import {
 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(
@@ -30,7 +34,9 @@ describe('Members Utils', () => {
       ${{ ...memberMock, groupManagedAccount: true }} | ${{ show: true, text: 'Managed Account', variant: 'info' }}
       ${{ ...memberMock, canOverride: true }}         | ${{ show: true, text: 'LDAP', 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));
     });
   });
 

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

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

spec/frontend/members/components/avatars/user_avatar_spec.js

 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', () => {

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 } };

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);
+        });
+      });
     });
   });
 

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