From d3f69fd34ae4a8e5f7dd8a0a9b5c1b965e27a0c5 Mon Sep 17 00:00:00 2001 From: Ieva Date: Mon, 9 Oct 2023 10:32:44 +0100 Subject: [PATCH] Chore: legacy access control cleanup for frontend team pages (#75005) * clean up legacy access control code for teams * remove editorsCanAdmin config from the frontend * add editorsCanAdmin config option back for the frontend --- .betterer.results | 6 - public/app/features/teams/TeamList.test.tsx | 23 +-- public/app/features/teams/TeamList.tsx | 30 +--- .../app/features/teams/TeamMemberRow.test.tsx | 78 --------- public/app/features/teams/TeamMemberRow.tsx | 115 -------------- .../app/features/teams/TeamMembers.test.tsx | 68 -------- public/app/features/teams/TeamMembers.tsx | 149 ------------------ public/app/features/teams/TeamPages.test.tsx | 47 +----- public/app/features/teams/TeamPages.tsx | 62 ++------ public/app/features/teams/TeamSettings.tsx | 12 +- .../app/features/teams/__mocks__/teamMocks.ts | 19 --- .../features/teams/state/selectors.test.ts | 115 +------------- public/app/features/teams/state/selectors.ts | 41 +---- 13 files changed, 35 insertions(+), 730 deletions(-) delete mode 100644 public/app/features/teams/TeamMemberRow.test.tsx delete mode 100644 public/app/features/teams/TeamMemberRow.tsx delete mode 100644 public/app/features/teams/TeamMembers.test.tsx delete mode 100644 public/app/features/teams/TeamMembers.tsx diff --git a/.betterer.results b/.betterer.results index 3ba9605375d..be66d2848e2 100644 --- a/.betterer.results +++ b/.betterer.results @@ -5179,12 +5179,6 @@ exports[`better eslint`] = { [0, 0, 0, "Unexpected any. Specify a different type.", "0"], [0, 0, 0, "Unexpected any. Specify a different type.", "1"] ], - "public/app/features/teams/TeamMemberRow.tsx:5381": [ - [0, 0, 0, "Do not use any type assertions.", "0"] - ], - "public/app/features/teams/TeamMembers.tsx:5381": [ - [0, 0, 0, "Unexpected any. Specify a different type.", "0"] - ], "public/app/features/teams/state/reducers.ts:5381": [ [0, 0, 0, "Do not use any type assertions.", "0"] ], diff --git a/public/app/features/teams/TeamList.test.tsx b/public/app/features/teams/TeamList.test.tsx index 4be97e0cf8f..76dd014644a 100644 --- a/public/app/features/teams/TeamList.test.tsx +++ b/public/app/features/teams/TeamList.test.tsx @@ -3,9 +3,9 @@ import userEvent from '@testing-library/user-event'; import React from 'react'; import { TestProvider } from 'test/helpers/TestProvider'; -import { contextSrv, User } from 'app/core/services/context_srv'; +import { contextSrv } from 'app/core/services/context_srv'; -import { OrgRole, Team } from '../../types'; +import { Team } from '../../types'; import { Props, TeamList } from './TeamList'; import { getMockTeam, getMultipleMockTeams } from './__mocks__/teamMocks'; @@ -30,18 +30,11 @@ const setup = (propOverrides?: object) => { totalPages: 0, page: 0, hasFetched: false, - editorsCanAdmin: false, perPage: 10, - signedInUser: { - id: 1, - orgRole: OrgRole.Viewer, - } as User, }; Object.assign(props, propOverrides); - contextSrv.user = props.signedInUser; - render( @@ -62,11 +55,6 @@ describe('TeamList', () => { teams: getMultipleMockTeams(1), totalCount: 1, hasFetched: true, - editorsCanAdmin: true, - signedInUser: { - id: 1, - orgRole: OrgRole.Editor, - } as User, }); expect(screen.getByRole('link', { name: /new team/i })).not.toHaveStyle('pointer-events: none'); @@ -80,11 +68,6 @@ describe('TeamList', () => { teams: getMultipleMockTeams(1), totalCount: 1, hasFetched: true, - editorsCanAdmin: true, - signedInUser: { - id: 1, - orgRole: OrgRole.Viewer, - } as User, }); expect(screen.getByRole('link', { name: /new team/i })).toHaveStyle('pointer-events: none'); @@ -95,7 +78,7 @@ describe('TeamList', () => { it('should call delete team', async () => { const mockDelete = jest.fn(); const mockTeam = getMockTeam(); - jest.spyOn(contextSrv, 'hasAccessInMetadata').mockReturnValue(true); + jest.spyOn(contextSrv, 'hasPermissionInMetadata').mockReturnValue(true); setup({ deleteTeam: mockDelete, teams: [mockTeam], totalCount: 1, hasFetched: true }); await userEvent.click(screen.getByRole('button', { name: `Delete team ${mockTeam.name}` })); await userEvent.click(screen.getByRole('button', { name: 'Delete' })); diff --git a/public/app/features/teams/TeamList.tsx b/public/app/features/teams/TeamList.tsx index 8c2f8ce7b2b..341fce8f419 100644 --- a/public/app/features/teams/TeamList.tsx +++ b/public/app/features/teams/TeamList.tsx @@ -18,7 +18,6 @@ import { import EmptyListCTA from 'app/core/components/EmptyListCTA/EmptyListCTA'; import { Page } from 'app/core/components/Page/Page'; import { fetchRoleOptions } from 'app/core/components/RolePicker/api'; -import { config } from 'app/core/config'; import { contextSrv } from 'app/core/services/context_srv'; import { AccessControlAction, Role, StoreState, Team } from 'app/types'; @@ -26,7 +25,6 @@ import { TeamRolePicker } from '../../core/components/RolePicker/TeamRolePicker' import { Avatar } from '../admin/Users/Avatar'; import { deleteTeam, loadTeams, changePage, changeQuery, changeSort } from './state/actions'; -import { isPermissionTeamAdmin } from './state/selectors'; type Cell = CellProps; export interface OwnProps {} @@ -44,8 +42,6 @@ export const TeamList = ({ deleteTeam, changeQuery, totalPages, - signedInUser, - editorsCanAdmin, page, changePage, changeSort, @@ -110,16 +106,7 @@ export const TeamList = ({ id: 'edit', header: '', cell: ({ row: { original } }: Cell) => { - const isTeamAdmin = isPermissionTeamAdmin({ - permission: original.permission, - editorsCanAdmin, - signedInUser, - }); - const canReadTeam = contextSrv.hasAccessInMetadata( - AccessControlAction.ActionTeamsRead, - original, - isTeamAdmin - ); + const canReadTeam = contextSrv.hasPermissionInMetadata(AccessControlAction.ActionTeamsRead, original); return canReadTeam ? ( @@ -133,16 +120,7 @@ export const TeamList = ({ id: 'delete', header: '', cell: ({ row: { original } }: Cell) => { - const isTeamAdmin = isPermissionTeamAdmin({ - permission: original.permission, - editorsCanAdmin, - signedInUser, - }); - const canDelete = contextSrv.hasAccessInMetadata( - AccessControlAction.ActionTeamsDelete, - original, - isTeamAdmin - ); + const canDelete = contextSrv.hasPermissionInMetadata(AccessControlAction.ActionTeamsDelete, original); return ( { - const props: Props = { - member: getMockTeamMember(), - syncEnabled: false, - editorsCanAdmin: false, - signedInUserIsTeamAdmin: false, - updateTeamMember: jest.fn(), - removeTeamMember: jest.fn(), - }; - - Object.assign(props, propOverrides); - - render( - - - - -
- ); -}; - -describe('Render', () => { - it('should render team member labels when sync enabled', () => { - const member = getMockTeamMember(); - member.labels = ['LDAP']; - setup({ member, syncEnabled: true }); - expect(screen.getByText('LDAP')).toBeInTheDocument(); - }); - - describe('when feature toggle editorsCanAdmin is turned on', () => { - it('should render permissions select if user is team admin', () => { - const member = getMockTeamMember(); - setup({ editorsCanAdmin: true, signedInUserIsTeamAdmin: true, member }); - expect(screen.getByLabelText(`Select member's ${member.name} permission level`)).toBeInTheDocument(); - }); - }); - - describe('when feature toggle editorsCanAdmin is turned off', () => { - it('should not render permissions', () => { - const member = getMockTeamMember(); - setup({ editorsCanAdmin: false, signedInUserIsTeamAdmin: true, member }); - expect(screen.queryByLabelText(`Select member's ${member.name} permission level`)).not.toBeInTheDocument(); - }); - }); -}); - -describe('Functions', () => { - it('should remove member on remove button click', async () => { - const member = getMockTeamMember(); - const mockRemove = jest.fn(); - setup({ member, removeTeamMember: mockRemove, editorsCanAdmin: true, signedInUserIsTeamAdmin: true }); - await userEvent.click(screen.getByRole('button', { name: `Remove team member ${member.name}` })); - await userEvent.click(screen.getByRole('button', { name: 'Delete' })); - - expect(mockRemove).toHaveBeenCalledWith(member.userId); - }); - - it('should update permission for user in team', async () => { - const member = getMockTeamMember(); - const mockUpdate = jest.fn(); - setup({ member, editorsCanAdmin: true, signedInUserIsTeamAdmin: true, updateTeamMember: mockUpdate }); - const permission = TeamPermissionLevel.Admin; - const expectedTeamMember = { ...member, permission }; - await userEvent.click(screen.getByLabelText(`Select member's ${member.name} permission level`)); - await userEvent.click(screen.getByText('Admin')); - - expect(mockUpdate).toHaveBeenCalledWith(expectedTeamMember); - }); -}); diff --git a/public/app/features/teams/TeamMemberRow.tsx b/public/app/features/teams/TeamMemberRow.tsx deleted file mode 100644 index e3c0c58b201..00000000000 --- a/public/app/features/teams/TeamMemberRow.tsx +++ /dev/null @@ -1,115 +0,0 @@ -import React, { PureComponent } from 'react'; -import { connect, ConnectedProps } from 'react-redux'; - -import { SelectableValue } from '@grafana/data'; -import { Select, DeleteButton } from '@grafana/ui'; -import { TagBadge } from 'app/core/components/TagFilter/TagBadge'; -import { WithFeatureToggle } from 'app/core/components/WithFeatureToggle'; -import { TeamMember, teamsPermissionLevels, TeamPermissionLevel } from 'app/types'; - -import { updateTeamMember, removeTeamMember } from './state/actions'; - -const mapDispatchToProps = { - removeTeamMember, - updateTeamMember, -}; - -const connector = connect(null, mapDispatchToProps); - -interface OwnProps { - member: TeamMember; - syncEnabled: boolean; - editorsCanAdmin: boolean; - signedInUserIsTeamAdmin: boolean; -} -export type Props = ConnectedProps & OwnProps; - -export class TeamMemberRow extends PureComponent { - constructor(props: Props) { - super(props); - this.renderLabels = this.renderLabels.bind(this); - this.renderPermissions = this.renderPermissions.bind(this); - } - - onRemoveMember(member: TeamMember) { - this.props.removeTeamMember(member.userId); - } - - onPermissionChange = (item: SelectableValue, member: TeamMember) => { - const permission = item.value; - const updatedTeamMember: TeamMember = { - ...member, - permission: permission as number, - }; - - this.props.updateTeamMember(updatedTeamMember); - }; - - renderPermissions(member: TeamMember) { - const { editorsCanAdmin, signedInUserIsTeamAdmin } = this.props; - const value = teamsPermissionLevels.find((dp) => dp.value === member.permission)!; - - return ( - - - {signedInUserIsTeamAdmin ? ( - - {contextSrv.licensedAccessControlEnabled() && ( + {contextSrv.licensedAccessControlEnabled() && canListRoles && ( { }; }; -export const getMockTeamMembers = (amount: number, teamAdminId: number): TeamMember[] => { - const teamMembers: TeamMember[] = []; - - for (let i = 1; i <= amount; i++) { - teamMembers.push({ - userId: i, - teamId: 1, - avatarUrl: 'some/url/', - email: 'test@test.com', - name: 'testName', - login: `testUser-${i}`, - labels: ['label 1', 'label 2'], - permission: i === teamAdminId ? TeamPermissionLevel.Admin : TeamPermissionLevel.Member, - }); - } - - return teamMembers; -}; - export const getMockTeamMember = (): TeamMember => { return { userId: 1, diff --git a/public/app/features/teams/state/selectors.test.ts b/public/app/features/teams/state/selectors.test.ts index 2d86f980cff..ee7664586c5 100644 --- a/public/app/features/teams/state/selectors.test.ts +++ b/public/app/features/teams/state/selectors.test.ts @@ -1,9 +1,7 @@ -import { User } from 'app/core/services/context_srv'; +import { TeamState } from '../../../types'; +import { getMockTeam } from '../__mocks__/teamMocks'; -import { Team, TeamGroup, TeamState, OrgRole } from '../../../types'; -import { getMockTeam, getMockTeamMembers } from '../__mocks__/teamMocks'; - -import { getTeam, getTeamMembers, isSignedInUserTeamAdmin, Config } from './selectors'; +import { getTeam } from './selectors'; describe('Team selectors', () => { describe('Get team', () => { @@ -21,111 +19,4 @@ describe('Team selectors', () => { expect(team).toEqual(mockTeam); }); }); - - describe('Get members', () => { - const mockTeamMembers = getMockTeamMembers(5, 5); - - it('should return team members', () => { - const mockState: TeamState = { - team: {} as Team, - searchMemberQuery: '', - members: mockTeamMembers, - groups: [] as TeamGroup[], - }; - - const members = getTeamMembers(mockState); - expect(members).toEqual(mockTeamMembers); - }); - }); -}); - -const signedInUserId = 1; - -const setup = (configOverrides?: Partial) => { - const defaultConfig: Config = { - editorsCanAdmin: false, - members: getMockTeamMembers(5, 5), - signedInUser: { - id: signedInUserId, - isGrafanaAdmin: false, - orgRole: OrgRole.Viewer, - } as User, - }; - - return { ...defaultConfig, ...configOverrides }; -}; - -describe('isSignedInUserTeamAdmin', () => { - describe('when feature toggle editorsCanAdmin is turned off', () => { - it('should return true', () => { - const config = setup(); - - const result = isSignedInUserTeamAdmin(config); - - expect(result).toBe(true); - }); - }); - - describe('when feature toggle editorsCanAdmin is turned on', () => { - it('should return true if signed in user is grafanaAdmin', () => { - const config = setup({ - editorsCanAdmin: true, - signedInUser: { - id: signedInUserId, - isGrafanaAdmin: true, - orgRole: OrgRole.Viewer, - } as User, - }); - - const result = isSignedInUserTeamAdmin(config); - - expect(result).toBe(true); - }); - - it('should return true if signed in user is org admin', () => { - const config = setup({ - editorsCanAdmin: true, - signedInUser: { - id: signedInUserId, - isGrafanaAdmin: false, - orgRole: OrgRole.Admin, - } as User, - }); - - const result = isSignedInUserTeamAdmin(config); - - expect(result).toBe(true); - }); - - it('should return true if signed in user is team admin', () => { - const config = setup({ - members: getMockTeamMembers(5, signedInUserId), - editorsCanAdmin: true, - signedInUser: { - id: signedInUserId, - isGrafanaAdmin: false, - orgRole: OrgRole.Viewer, - } as User, - }); - - const result = isSignedInUserTeamAdmin(config); - - expect(result).toBe(true); - }); - - it('should return false if signed in user is not grafanaAdmin, org admin or team admin', () => { - const config = setup({ - editorsCanAdmin: true, - signedInUser: { - id: signedInUserId, - isGrafanaAdmin: false, - orgRole: OrgRole.Viewer, - } as User, - }); - - const result = isSignedInUserTeamAdmin(config); - - expect(result).toBe(false); - }); - }); }); diff --git a/public/app/features/teams/state/selectors.ts b/public/app/features/teams/state/selectors.ts index eb20ed2e1a3..5caa33469f1 100644 --- a/public/app/features/teams/state/selectors.ts +++ b/public/app/features/teams/state/selectors.ts @@ -1,7 +1,5 @@ -import { User } from 'app/core/services/context_srv'; -import { Team, TeamState, TeamMember, OrgRole, TeamPermissionLevel } from 'app/types'; +import { Team, TeamState } from 'app/types'; -export const getSearchMemberQuery = (state: TeamState) => state.searchMemberQuery; export const getTeamGroups = (state: TeamState) => state.groups; export const getTeam = (state: TeamState, currentTeamId: any): Team | null => { @@ -11,40 +9,3 @@ export const getTeam = (state: TeamState, currentTeamId: any): Team | null => { return null; }; - -export const getTeamMembers = (state: TeamState) => { - const regex = RegExp(state.searchMemberQuery, 'i'); - - return state.members.filter((member) => { - return regex.test(member.login) || regex.test(member.email) || regex.test(member.name); - }); -}; - -export interface Config { - members: TeamMember[]; - editorsCanAdmin: boolean; - signedInUser: User; -} - -export const isSignedInUserTeamAdmin = (config: Config): boolean => { - const { members, signedInUser, editorsCanAdmin } = config; - const userInMembers = members.find((m) => m.userId === signedInUser.id); - const permission = userInMembers ? userInMembers.permission : TeamPermissionLevel.Member; - - return isPermissionTeamAdmin({ permission, signedInUser, editorsCanAdmin }); -}; - -export interface PermissionConfig { - permission: TeamPermissionLevel; - editorsCanAdmin: boolean; - signedInUser: User; -} - -export const isPermissionTeamAdmin = (config: PermissionConfig): boolean => { - const { permission, signedInUser, editorsCanAdmin } = config; - const isAdmin = signedInUser.isGrafanaAdmin || signedInUser.orgRole === OrgRole.Admin; - const userIsTeamAdmin = permission === TeamPermissionLevel.Admin; - const isSignedInUserTeamAdmin = isAdmin || userIsTeamAdmin; - - return isSignedInUserTeamAdmin || !editorsCanAdmin; -};