From a4509ce4cfd2eaf3c348237303d50fddb032dcc1 Mon Sep 17 00:00:00 2001 From: araddcc002 Date: Wed, 13 Nov 2024 14:08:47 +0000 Subject: [PATCH 01/20] wip, added manual entity input --- .../src/entry/settings/EntryAccessInput.tsx | 62 ++++++++++++++++++- 1 file changed, 60 insertions(+), 2 deletions(-) diff --git a/frontend/src/entry/settings/EntryAccessInput.tsx b/frontend/src/entry/settings/EntryAccessInput.tsx index 1e3c5f412..870138180 100644 --- a/frontend/src/entry/settings/EntryAccessInput.tsx +++ b/frontend/src/entry/settings/EntryAccessInput.tsx @@ -1,10 +1,29 @@ -import { Autocomplete, Stack, Table, TableBody, TableCell, TableHead, TableRow, TextField } from '@mui/material' +import ExpandMoreIcon from '@mui/icons-material/ExpandMore' +import { + Accordion, + AccordionDetails, + AccordionSummary, + Autocomplete, + Button, + FormControl, + InputLabel, + MenuItem, + Select, + Stack, + Table, + TableBody, + TableCell, + TableHead, + TableRow, + TextField, + Typography, +} from '@mui/material' import { useListUsers } from 'actions/user' import { debounce } from 'lodash-es' import { SyntheticEvent, useCallback, useEffect, useMemo, useState } from 'react' import EntityItem from 'src/entry/settings/EntityItem' import MessageAlert from 'src/MessageAlert' -import { CollaboratorEntry, EntityObject, EntryKindKeys, Role } from 'types/types' +import { CollaboratorEntry, EntityKind, EntityObject, EntryKindKeys, Role } from 'types/types' import { toSentenceCase } from 'utils/stringUtils' type EntryAccessInputProps = { @@ -27,6 +46,8 @@ export default function EntryAccessInput({ value, onUpdate, entryKind, entryRole const [open, setOpen] = useState(false) const [accessList, setAccessList] = useState(value) const [userListQuery, setUserListQuery] = useState('') + const [manualEntityKind, setManualEntityKind] = useState(EntityKind.USER) + const [manualEntityName, setManualEntityName] = useState('') const { users, isUsersLoading, isUsersError } = useListUsers(userListQuery) @@ -113,6 +134,43 @@ export default function EntryAccessInput({ value, onUpdate, entryKind, entryRole /> )} /> + + } + aria-controls='manual-user-add-content' + id='manual-user-add-header' + > + Trouble finding user / group? + + + + + User or group + + + setManualEntityName(e.target.value)} + /> + + + + From 4f1836c6f2d71124c42363054b0e3986fbcbbb13 Mon Sep 17 00:00:00 2001 From: araddcc002 Date: Wed, 13 Nov 2024 16:41:32 +0000 Subject: [PATCH 02/20] added a new function for user info look up as non-hook. added ability to manually add users to model access --- frontend/actions/user.ts | 10 +++- .../src/entry/settings/EntryAccessInput.tsx | 50 +++++++++++-------- 2 files changed, 36 insertions(+), 24 deletions(-) diff --git a/frontend/actions/user.ts b/frontend/actions/user.ts index 46d2f9291..3439f35ec 100644 --- a/frontend/actions/user.ts +++ b/frontend/actions/user.ts @@ -49,9 +49,9 @@ interface UserInformationResponse { entity: UserInformation } -export function useGetUserInformation(dn: string) { +export function useGetUserInformation(dn: string, kind?: string) { const { data, isLoading, error, mutate } = useSWR( - `/api/v2/entity/${dn}/lookup`, + `/api/v2/entity/${dn}/lookup${kind ? `?kind=${kind}` : ''}`, fetcher, ) @@ -63,6 +63,12 @@ export function useGetUserInformation(dn: string) { } } +export function getUserInformation(dn: string) { + return fetch(`/api/v2/entity/${dn}/lookup`, { + headers: { 'Content-Type': 'application/json' }, + }) +} + interface GetUserTokensResponse { tokens: TokenInterface[] } diff --git a/frontend/src/entry/settings/EntryAccessInput.tsx b/frontend/src/entry/settings/EntryAccessInput.tsx index 870138180..c3192e3c1 100644 --- a/frontend/src/entry/settings/EntryAccessInput.tsx +++ b/frontend/src/entry/settings/EntryAccessInput.tsx @@ -5,10 +5,6 @@ import { AccordionSummary, Autocomplete, Button, - FormControl, - InputLabel, - MenuItem, - Select, Stack, Table, TableBody, @@ -18,12 +14,13 @@ import { TextField, Typography, } from '@mui/material' -import { useListUsers } from 'actions/user' +import { getUserInformation, useListUsers } from 'actions/user' import { debounce } from 'lodash-es' import { SyntheticEvent, useCallback, useEffect, useMemo, useState } from 'react' import EntityItem from 'src/entry/settings/EntityItem' import MessageAlert from 'src/MessageAlert' import { CollaboratorEntry, EntityKind, EntityObject, EntryKindKeys, Role } from 'types/types' +import { getErrorMessage } from 'utils/fetcher' import { toSentenceCase } from 'utils/stringUtils' type EntryAccessInputProps = { @@ -46,8 +43,8 @@ export default function EntryAccessInput({ value, onUpdate, entryKind, entryRole const [open, setOpen] = useState(false) const [accessList, setAccessList] = useState(value) const [userListQuery, setUserListQuery] = useState('') - const [manualEntityKind, setManualEntityKind] = useState(EntityKind.USER) const [manualEntityName, setManualEntityName] = useState('') + const [errorMessage, setErrorMessage] = useState('') const { users, isUsersLoading, isUsersError } = useListUsers(userListQuery) @@ -96,6 +93,23 @@ export default function EntryAccessInput({ value, onUpdate, entryKind, entryRole handleInputChange(event, value) }, 500) + const handleAddEntityManuallyOnClick = useCallback(async () => { + setErrorMessage('') + if (manualEntityName !== undefined && manualEntityName !== '') { + if (accessList.find((collaborator) => collaborator.entity === `${EntityKind.USER}:${manualEntityName}`)) { + return setErrorMessage(`The requested user has already been added below.`) + } + const response = await getUserInformation(manualEntityName) + if (!response.ok) { + return setErrorMessage(await getErrorMessage(response)) + } + const updatedAccessList = [...accessList] + const newAccess = { entity: `${EntityKind.USER}:${manualEntityName}`, roles: [] } + updatedAccessList.push(newAccess) + setAccessList(updatedAccessList) + } + }, [accessList, manualEntityName]) + const noOptionsText = useMemo(() => { if (userListQuery.length < 3) return 'Please enter at least three characters' if (isUsersError?.status === 413) return 'Too many results, please refine your search' @@ -141,34 +155,26 @@ export default function EntryAccessInput({ value, onUpdate, entryKind, entryRole aria-controls='manual-user-add-content' id='manual-user-add-header' > - Trouble finding user / group? + + Trouble finding a user? Click here to add them manually + - - User or group - - setManualEntityName(e.target.value)} /> - + +
From d58028162305097e04860ce6c0a8e597fb75679b Mon Sep 17 00:00:00 2001 From: araddcc002 Date: Wed, 13 Nov 2024 17:10:32 +0000 Subject: [PATCH 03/20] updated backend to check for invalid users --- backend/src/services/model.ts | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/backend/src/services/model.ts b/backend/src/services/model.ts index b953d38c1..f6b4a0637 100644 --- a/backend/src/services/model.ts +++ b/backend/src/services/model.ts @@ -305,7 +305,10 @@ export async function updateModelCard( return revision } -export type UpdateModelParams = Pick & { +export type UpdateModelParams = Pick< + ModelInterface, + 'name' | 'teamId' | 'description' | 'visibility' | 'collaborators' +> & { settings: Partial } export async function updateModel(user: UserInterface, modelId: string, modelDiff: Partial) { @@ -319,6 +322,18 @@ export async function updateModel(user: UserInterface, modelId: string, modelDif if (modelDiff.settings?.mirror?.destinationModelId && modelDiff.settings?.mirror?.sourceModelId) { throw BadReq('You cannot select both mirror settings simultaneously.') } + if (modelDiff.collaborators) { + const invalidUsers = [] + modelDiff.collaborators.forEach(async (collaborator) => { + const userInformation = await authentication.getUserInformation(collaborator.entity) + if (!userInformation) { + invalidUsers.push(userInformation) + } + }) + if (invalidUsers.length > 0) { + throw BadReq('One or more user is invalid') + } + } const auth = await authorisation.model(user, model, ModelAction.Update) if (!auth.success) { From 40131db4af3190556dc8a526c99f24bd6cdf21a4 Mon Sep 17 00:00:00 2001 From: araddcc002 Date: Thu, 14 Nov 2024 14:47:32 +0000 Subject: [PATCH 04/20] moved manual user access form to its own component --- .../src/entry/settings/EntryAccessInput.tsx | 72 ++----------------- .../src/entry/settings/ManualEntryAccess.tsx | 67 +++++++++++++++++ 2 files changed, 72 insertions(+), 67 deletions(-) create mode 100644 frontend/src/entry/settings/ManualEntryAccess.tsx diff --git a/frontend/src/entry/settings/EntryAccessInput.tsx b/frontend/src/entry/settings/EntryAccessInput.tsx index c3192e3c1..a2475d830 100644 --- a/frontend/src/entry/settings/EntryAccessInput.tsx +++ b/frontend/src/entry/settings/EntryAccessInput.tsx @@ -1,26 +1,11 @@ -import ExpandMoreIcon from '@mui/icons-material/ExpandMore' -import { - Accordion, - AccordionDetails, - AccordionSummary, - Autocomplete, - Button, - Stack, - Table, - TableBody, - TableCell, - TableHead, - TableRow, - TextField, - Typography, -} from '@mui/material' -import { getUserInformation, useListUsers } from 'actions/user' +import { Autocomplete, Stack, Table, TableBody, TableCell, TableHead, TableRow, TextField } from '@mui/material' +import { useListUsers } from 'actions/user' import { debounce } from 'lodash-es' import { SyntheticEvent, useCallback, useEffect, useMemo, useState } from 'react' import EntityItem from 'src/entry/settings/EntityItem' +import ManualEntryAccess from 'src/entry/settings/ManualEntryAccess' import MessageAlert from 'src/MessageAlert' -import { CollaboratorEntry, EntityKind, EntityObject, EntryKindKeys, Role } from 'types/types' -import { getErrorMessage } from 'utils/fetcher' +import { CollaboratorEntry, EntityObject, EntryKindKeys, Role } from 'types/types' import { toSentenceCase } from 'utils/stringUtils' type EntryAccessInputProps = { @@ -43,8 +28,6 @@ export default function EntryAccessInput({ value, onUpdate, entryKind, entryRole const [open, setOpen] = useState(false) const [accessList, setAccessList] = useState(value) const [userListQuery, setUserListQuery] = useState('') - const [manualEntityName, setManualEntityName] = useState('') - const [errorMessage, setErrorMessage] = useState('') const { users, isUsersLoading, isUsersError } = useListUsers(userListQuery) @@ -93,23 +76,6 @@ export default function EntryAccessInput({ value, onUpdate, entryKind, entryRole handleInputChange(event, value) }, 500) - const handleAddEntityManuallyOnClick = useCallback(async () => { - setErrorMessage('') - if (manualEntityName !== undefined && manualEntityName !== '') { - if (accessList.find((collaborator) => collaborator.entity === `${EntityKind.USER}:${manualEntityName}`)) { - return setErrorMessage(`The requested user has already been added below.`) - } - const response = await getUserInformation(manualEntityName) - if (!response.ok) { - return setErrorMessage(await getErrorMessage(response)) - } - const updatedAccessList = [...accessList] - const newAccess = { entity: `${EntityKind.USER}:${manualEntityName}`, roles: [] } - updatedAccessList.push(newAccess) - setAccessList(updatedAccessList) - } - }, [accessList, manualEntityName]) - const noOptionsText = useMemo(() => { if (userListQuery.length < 3) return 'Please enter at least three characters' if (isUsersError?.status === 413) return 'Too many results, please refine your search' @@ -148,35 +114,7 @@ export default function EntryAccessInput({ value, onUpdate, entryKind, entryRole /> )} /> - - } - aria-controls='manual-user-add-content' - id='manual-user-add-header' - > - - Trouble finding a user? Click here to add them manually - - - - - setManualEntityName(e.target.value)} - /> - - - - - +
diff --git a/frontend/src/entry/settings/ManualEntryAccess.tsx b/frontend/src/entry/settings/ManualEntryAccess.tsx new file mode 100644 index 000000000..6d5f7e426 --- /dev/null +++ b/frontend/src/entry/settings/ManualEntryAccess.tsx @@ -0,0 +1,67 @@ +import ExpandMoreIcon from '@mui/icons-material/ExpandMore' +import { Accordion, AccordionDetails, AccordionSummary, Button, Stack, TextField, Typography } from '@mui/material' +import { getUserInformation } from 'actions/user' +import { useState } from 'react' +import MessageAlert from 'src/MessageAlert' +import { CollaboratorEntry, EntityKind } from 'types/types' +import { getErrorMessage } from 'utils/fetcher' + +interface ManualEntryAccessProps { + accessList: CollaboratorEntry[] + setAccessList: (accessList: CollaboratorEntry[]) => void +} + +export default function ManualEntryAccess({ accessList, setAccessList }: ManualEntryAccessProps) { + const [manualEntityName, setManualEntityName] = useState('') + const [errorMessage, setErrorMessage] = useState('') + + const handleAddEntityManuallyOnClick = async () => { + setErrorMessage('') + if (manualEntityName !== undefined && manualEntityName !== '') { + if (accessList.find((collaborator) => collaborator.entity === `${EntityKind.USER}:${manualEntityName}`)) { + return setErrorMessage(`The requested user has already been added below.`) + } + const response = await getUserInformation(manualEntityName) + if (!response.ok) { + return setErrorMessage(await getErrorMessage(response)) + } + const updatedAccessList = [...accessList] + const newAccess = { entity: `${EntityKind.USER}:${manualEntityName}`, roles: [] } + updatedAccessList.push(newAccess) + setAccessList(updatedAccessList) + setManualEntityName('') + } + } + + return ( + + } + aria-controls='manual-user-add-content' + id='manual-user-add-header' + > + + Trouble finding a user? Click here to add them manually + + + + + setManualEntityName(e.target.value)} + /> + + + + + + ) +} From c1fa4022b20f74a644adf29081acafc3fb3d3d3b Mon Sep 17 00:00:00 2001 From: araddcc002 Date: Mon, 18 Nov 2024 09:55:15 +0000 Subject: [PATCH 05/20] wip --- backend/src/services/model.ts | 19 +++++++++---------- frontend/actions/user.ts | 4 ++-- 2 files changed, 11 insertions(+), 12 deletions(-) diff --git a/backend/src/services/model.ts b/backend/src/services/model.ts index f6b4a0637..18687b6ad 100644 --- a/backend/src/services/model.ts +++ b/backend/src/services/model.ts @@ -305,10 +305,7 @@ export async function updateModelCard( return revision } -export type UpdateModelParams = Pick< - ModelInterface, - 'name' | 'teamId' | 'description' | 'visibility' | 'collaborators' -> & { +export type UpdateModelParams = Pick & { settings: Partial } export async function updateModel(user: UserInterface, modelId: string, modelDiff: Partial) { @@ -324,12 +321,14 @@ export async function updateModel(user: UserInterface, modelId: string, modelDif } if (modelDiff.collaborators) { const invalidUsers = [] - modelDiff.collaborators.forEach(async (collaborator) => { - const userInformation = await authentication.getUserInformation(collaborator.entity) - if (!userInformation) { - invalidUsers.push(userInformation) - } - }) + await Promise.all( + modelDiff.collaborators.map(async (collaborator) => { + const userInformation = await authentication.getUserInformation(collaborator.entity) + if (!userInformation) { + invalidUsers.push(userInformation) + } + }), + ) if (invalidUsers.length > 0) { throw BadReq('One or more user is invalid') } diff --git a/frontend/actions/user.ts b/frontend/actions/user.ts index 3439f35ec..985bc2f02 100644 --- a/frontend/actions/user.ts +++ b/frontend/actions/user.ts @@ -49,9 +49,9 @@ interface UserInformationResponse { entity: UserInformation } -export function useGetUserInformation(dn: string, kind?: string) { +export function useGetUserInformation(dn: string) { const { data, isLoading, error, mutate } = useSWR( - `/api/v2/entity/${dn}/lookup${kind ? `?kind=${kind}` : ''}`, + `/api/v2/entity/${dn}/lookup`, fetcher, ) From 7352462e1a3a9b2915c861fafaa1ba93d733bc9e Mon Sep 17 00:00:00 2001 From: araddcc002 Date: Tue, 19 Nov 2024 13:39:03 +0000 Subject: [PATCH 06/20] wip --- backend/config/default.cjs | 4 ++ backend/src/services/model.ts | 26 +++++------ backend/src/types/types.ts | 4 ++ backend/test/services/model.spec.ts | 1 + frontend/src/common/HelpDialog.tsx | 2 +- frontend/src/common/HelpPopover.tsx | 1 + .../src/entry/settings/ManualEntryAccess.tsx | 46 ++++++++++++------- frontend/types/types.ts | 4 ++ .../templates/bailo/bailo.configmap.yaml | 4 ++ infrastructure/helm/bailo/values.yaml | 3 ++ 10 files changed, 64 insertions(+), 31 deletions(-) diff --git a/backend/config/default.cjs b/backend/config/default.cjs index 457fb3ecd..7b140a568 100644 --- a/backend/config/default.cjs +++ b/backend/config/default.cjs @@ -195,6 +195,10 @@ module.exports = { text: '', startTimestamp: '', }, + + helpPopoverText: { + manualEntryAccess: '', + }, }, connectors: { diff --git a/backend/src/services/model.ts b/backend/src/services/model.ts index 765d70ef8..adbc51753 100644 --- a/backend/src/services/model.ts +++ b/backend/src/services/model.ts @@ -318,20 +318,7 @@ export async function updateModel(user: UserInterface, modelId: string, modelDif throw BadReq('You cannot select both mirror settings simultaneously.') } if (modelDiff.collaborators) { - const invalidEntities: string[] = [] - await Promise.all( - modelDiff.collaborators.map(async (collaborator) => { - const userInformation = await authentication.getUserInformation(collaborator.entity) - if (!userInformation) { - invalidEntities.push(collaborator.entity) - } - }), - ) - if (invalidEntities.length > 0) { - throw BadReq( - `Failed to update ${`model.kind`}. Invalid ${invalidEntities.length === 1 ? 'entity' : 'entities'}: ${invalidEntities.join(', ')}`, - ) - } + await checkCollaboratorAuthorisation(modelDiff.collaborators) } const auth = await authorisation.model(user, model, ModelAction.Update) @@ -351,6 +338,17 @@ export async function updateModel(user: UserInterface, modelId: string, modelDif return model } +async function checkCollaboratorAuthorisation(collaborators: CollaboratorEntry[]) { + await Promise.all( + collaborators.map(async (collaborator) => { + const userInformation = await authentication.getUserInformation(collaborator.entity) + if (!userInformation) { + throw BadReq(`Unable to find user ${collaborator.entity}`) + } + }), + ) +} + export async function createModelCardFromSchema( user: UserInterface, modelId: string, diff --git a/backend/src/types/types.ts b/backend/src/types/types.ts index bd7125bd5..8a8fe89f8 100644 --- a/backend/src/types/types.ts +++ b/backend/src/types/types.ts @@ -85,4 +85,8 @@ export interface UiConfig { text: string startTimestamp: string } + + helpPopoverText: { + manualEntryAccess: string + } } diff --git a/backend/test/services/model.spec.ts b/backend/test/services/model.spec.ts index 1a6999d2b..496776d18 100644 --- a/backend/test/services/model.spec.ts +++ b/backend/test/services/model.spec.ts @@ -86,6 +86,7 @@ vi.mock('../../src/models/Model.js', () => ({ default: modelMocks })) const authenticationMocks = vi.hoisted(() => ({ getEntities: vi.fn(() => ['user']), + getUserInformation: vi.fn(() => ({ name: 'user', email: 'user@example.com' })), })) vi.mock('../../src/connectors/authentication/index.js', async () => ({ default: authenticationMocks, diff --git a/frontend/src/common/HelpDialog.tsx b/frontend/src/common/HelpDialog.tsx index e43260a3b..14a43ce7b 100644 --- a/frontend/src/common/HelpDialog.tsx +++ b/frontend/src/common/HelpDialog.tsx @@ -22,7 +22,7 @@ export default function HelpDialog({ title, content }: HelpDialogProps) { <> - + diff --git a/frontend/src/common/HelpPopover.tsx b/frontend/src/common/HelpPopover.tsx index 395722e5c..28fa94db9 100644 --- a/frontend/src/common/HelpPopover.tsx +++ b/frontend/src/common/HelpPopover.tsx @@ -29,6 +29,7 @@ function HelpPopover({ anchorOrigin, transformOrigin, children }: Props) { onMouseEnter={handlePopoverOpen} onMouseLeave={handlePopoverClose} data-test='helpIcon' + color='primary' /> { setErrorMessage('') if (manualEntityName !== undefined && manualEntityName !== '') { @@ -24,6 +29,10 @@ export default function ManualEntryAccess({ accessList, setAccessList }: ManualE } } + if (isUiConfigError) { + return + } + return ( - - - setManualEntityName(e.target.value)} - /> - - - + {isUiConfigLoading && } + {!isUiConfigLoading && uiConfig && ( + + + setManualEntityName(e.target.value)} + /> + {uiConfig.helpPopoverText.manualEntryAccess && ( + {uiConfig.helpPopoverText.manualEntryAccess} + )} + + + + )} diff --git a/frontend/types/types.ts b/frontend/types/types.ts index eaaaf1436..f391e090a 100644 --- a/frontend/types/types.ts +++ b/frontend/types/types.ts @@ -60,6 +60,10 @@ export interface UiConfig { text: string startTimestamp: string } + + helpPopoverText: { + manualEntryAccess: string + } } export interface FileInterface { diff --git a/infrastructure/helm/bailo/templates/bailo/bailo.configmap.yaml b/infrastructure/helm/bailo/templates/bailo/bailo.configmap.yaml index 34d2274bd..76a27abf5 100644 --- a/infrastructure/helm/bailo/templates/bailo/bailo.configmap.yaml +++ b/infrastructure/helm/bailo/templates/bailo/bailo.configmap.yaml @@ -179,6 +179,10 @@ data: disclaimer: '{{ .Values.modelMirror.export.disclaimer }}' } }, + + helpPopoverText: { + manualEntryAccess: {{ .Values.helpPopoverText.manualEntryAccess }} + } }, connectors: { diff --git a/infrastructure/helm/bailo/values.yaml b/infrastructure/helm/bailo/values.yaml index b6657b62d..f0dac1160 100644 --- a/infrastructure/helm/bailo/values.yaml +++ b/infrastructure/helm/bailo/values.yaml @@ -228,6 +228,9 @@ config: text: '' startTimestamp: '' + helpPopoverText: + manualEntryAccess: '' + smtp: #host: 'mail' #service name port: 1025 From 7b41a6e90563453c94d015b2d1da8c70d9dc954a Mon Sep 17 00:00:00 2001 From: araddcc002 Date: Wed, 20 Nov 2024 10:36:13 +0000 Subject: [PATCH 07/20] updated model service to check for user auth on both update and create and added unit tests --- backend/src/services/model.ts | 14 +++++++++++--- backend/test/services/model.spec.ts | 18 ++++++++++++++++++ 2 files changed, 29 insertions(+), 3 deletions(-) diff --git a/backend/src/services/model.ts b/backend/src/services/model.ts index adbc51753..18e0c32dd 100644 --- a/backend/src/services/model.ts +++ b/backend/src/services/model.ts @@ -33,6 +33,10 @@ export type CreateModelParams = Pick< export async function createModel(user: UserInterface, modelParams: CreateModelParams) { const modelId = convertStringToId(modelParams.name) + if (modelParams.collaborators) { + await checkCollaboratorAuthorisation(modelParams.collaborators) + } + let collaborators: CollaboratorEntry[] = [] if (modelParams.collaborators && modelParams.collaborators.length > 0) { const collaboratorListContainsOwner = modelParams.collaborators.some((collaborator) => @@ -341,9 +345,13 @@ export async function updateModel(user: UserInterface, modelId: string, modelDif async function checkCollaboratorAuthorisation(collaborators: CollaboratorEntry[]) { await Promise.all( collaborators.map(async (collaborator) => { - const userInformation = await authentication.getUserInformation(collaborator.entity) - if (!userInformation) { - throw BadReq(`Unable to find user ${collaborator.entity}`) + if (collaborator.entity === '') { + throw BadReq('Collaborator name must be a valid string') + } + try { + await authentication.getUserInformation(collaborator.entity) + } catch (err) { + throw InternalError(`Unable to find user ${collaborator.entity}`, { err }) } }), ) diff --git a/backend/test/services/model.spec.ts b/backend/test/services/model.spec.ts index 496776d18..3757cba2d 100644 --- a/backend/test/services/model.spec.ts +++ b/backend/test/services/model.spec.ts @@ -107,6 +107,15 @@ describe('services > model', () => { expect(modelMocks.save).not.toBeCalled() }) + test('createModel > should throw an internal error if getUserInformation fails due to invalid user', async () => { + authenticationMocks.getUserInformation.mockImplementation(() => { + throw new Error('Could not find user') + }) + expect(() => + createModel({} as any, { collaborators: [{ entity: 'user:unknown_user', roles: [] }] } as any), + ).rejects.toThrowError(/^Unable to find user user:unknown_user/) + }) + test('createModel > bad request is thrown when attempting to set both source and destinationModelId simultaneously', async () => { vi.mocked(authorisation.model).mockResolvedValueOnce({ info: 'You cannot select both settings simultaneously.', @@ -327,6 +336,15 @@ describe('services > model', () => { ).rejects.toThrowError(/^You cannot select both mirror settings simultaneously./) }) + test('updateModel > should throw an internal error if getUserInformation fails due to invalid user', async () => { + authenticationMocks.getUserInformation.mockImplementation(() => { + throw new Error('Could not find user') + }) + expect(() => + updateModel({} as any, '123', { collaborators: [{ entity: 'user:unknown_user', roles: [] }] }), + ).rejects.toThrowError(/^Unable to find user user:unknown_user/) + }) + test('createModelcardFromSchema > should throw an error when attempting to change a model from mirrored to standard', async () => { vi.mocked(authorisation.model).mockResolvedValue({ info: 'Cannot alter a mirrored model.', From 39e552571d24408ae703e4551d402bf15a5f2647 Mon Sep 17 00:00:00 2001 From: araddcc002 Date: Wed, 20 Nov 2024 14:41:55 +0000 Subject: [PATCH 08/20] fixed an issue with config map --- infrastructure/helm/bailo/templates/bailo/bailo.configmap.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/infrastructure/helm/bailo/templates/bailo/bailo.configmap.yaml b/infrastructure/helm/bailo/templates/bailo/bailo.configmap.yaml index 76a27abf5..c4f19bfb7 100644 --- a/infrastructure/helm/bailo/templates/bailo/bailo.configmap.yaml +++ b/infrastructure/helm/bailo/templates/bailo/bailo.configmap.yaml @@ -181,7 +181,7 @@ data: }, helpPopoverText: { - manualEntryAccess: {{ .Values.helpPopoverText.manualEntryAccess }} + manualEntryAccess: {{ .Values.config.ui.helpPopoverText.manualEntryAccess }} } }, From cd0d01d90426bad230401fdf4d38cd017768a80c Mon Sep 17 00:00:00 2001 From: araddcc002 Date: Wed, 20 Nov 2024 17:19:06 +0000 Subject: [PATCH 09/20] addressed more pr comments and improved some of logic --- backend/src/services/model.ts | 11 ++++++++++ .../src/entry/settings/EntryAccessInput.tsx | 22 ++++++++++++++++--- ...lEntryAccess.tsx => ManualEntityInput.tsx} | 17 +++++--------- 3 files changed, 35 insertions(+), 15 deletions(-) rename frontend/src/entry/settings/{ManualEntryAccess.tsx => ManualEntityInput.tsx} (75%) diff --git a/backend/src/services/model.ts b/backend/src/services/model.ts index 18e0c32dd..c9889ef3e 100644 --- a/backend/src/services/model.ts +++ b/backend/src/services/model.ts @@ -343,6 +343,17 @@ export async function updateModel(user: UserInterface, modelId: string, modelDif } async function checkCollaboratorAuthorisation(collaborators: CollaboratorEntry[]) { + const duplicateEntityNames: string[] = [] + collaborators.forEach((collaborator) => { + if (collaborators[collaborator.entity]) { + duplicateEntityNames.push(collaborator.entity) + } else { + collaborators[collaborator.entity] = true + } + }) + if (duplicateEntityNames.length > 0) { + throw BadReq(`The following duplicate collaborators have been found: ${duplicateEntityNames.join(', ')}`) + } await Promise.all( collaborators.map(async (collaborator) => { if (collaborator.entity === '') { diff --git a/frontend/src/entry/settings/EntryAccessInput.tsx b/frontend/src/entry/settings/EntryAccessInput.tsx index a2475d830..e16e319c6 100644 --- a/frontend/src/entry/settings/EntryAccessInput.tsx +++ b/frontend/src/entry/settings/EntryAccessInput.tsx @@ -3,9 +3,9 @@ import { useListUsers } from 'actions/user' import { debounce } from 'lodash-es' import { SyntheticEvent, useCallback, useEffect, useMemo, useState } from 'react' import EntityItem from 'src/entry/settings/EntityItem' -import ManualEntryAccess from 'src/entry/settings/ManualEntryAccess' +import ManualEntityInput from 'src/entry/settings/ManualEntityInput' import MessageAlert from 'src/MessageAlert' -import { CollaboratorEntry, EntityObject, EntryKindKeys, Role } from 'types/types' +import { CollaboratorEntry, EntityKind, EntityObject, EntryKindKeys, Role } from 'types/types' import { toSentenceCase } from 'utils/stringUtils' type EntryAccessInputProps = { @@ -28,6 +28,7 @@ export default function EntryAccessInput({ value, onUpdate, entryKind, entryRole const [open, setOpen] = useState(false) const [accessList, setAccessList] = useState(value) const [userListQuery, setUserListQuery] = useState('') + const [manualEntityEntryErrorMessage, setManualEntityEntryErrorMessage] = useState('') const { users, isUsersLoading, isUsersError } = useListUsers(userListQuery) @@ -72,6 +73,18 @@ export default function EntryAccessInput({ value, onUpdate, entryKind, entryRole setUserListQuery(value) }, []) + const handleNewManualEntityEntry = useCallback( + (manualEntityName: string) => { + setManualEntityEntryErrorMessage('') + if (accessList.find((collaborator) => collaborator.entity === `${EntityKind.USER}:${manualEntityName}`)) { + setManualEntityEntryErrorMessage('User has already been added below.') + } else { + setAccessList([...accessList, { entity: `${EntityKind.USER}:${manualEntityName}`, roles: [] }]) + } + }, + [accessList], + ) + const debounceOnInputChange = debounce((event: SyntheticEvent, value: string) => { handleInputChange(event, value) }, 500) @@ -114,7 +127,10 @@ export default function EntryAccessInput({ value, onUpdate, entryKind, entryRole /> )} /> - +
diff --git a/frontend/src/entry/settings/ManualEntryAccess.tsx b/frontend/src/entry/settings/ManualEntityInput.tsx similarity index 75% rename from frontend/src/entry/settings/ManualEntryAccess.tsx rename to frontend/src/entry/settings/ManualEntityInput.tsx index 454fef737..144bd2fe7 100644 --- a/frontend/src/entry/settings/ManualEntryAccess.tsx +++ b/frontend/src/entry/settings/ManualEntityInput.tsx @@ -5,27 +5,21 @@ import { useState } from 'react' import HelpPopover from 'src/common/HelpPopover' import Loading from 'src/common/Loading' import MessageAlert from 'src/MessageAlert' -import { CollaboratorEntry, EntityKind } from 'types/types' -interface ManualEntryAccessProps { - accessList: CollaboratorEntry[] - setAccessList: (accessList: CollaboratorEntry[]) => void +interface ManualEntityInputProps { + onAddEntityManually: (entityName: string) => void + errorMessage: string } -export default function ManualEntryAccess({ accessList, setAccessList }: ManualEntryAccessProps) { +export default function ManualEntityInput({ onAddEntityManually, errorMessage }: ManualEntityInputProps) { const [manualEntityName, setManualEntityName] = useState('') - const [errorMessage, setErrorMessage] = useState('') const { uiConfig, isUiConfigLoading, isUiConfigError } = useGetUiConfig() const handleAddEntityManuallyOnClick = async () => { - setErrorMessage('') if (manualEntityName !== undefined && manualEntityName !== '') { - if (accessList.find((collaborator) => collaborator.entity === `${EntityKind.USER}:${manualEntityName}`)) { - return setErrorMessage(`The requested user has already been added below.`) - } - setAccessList([...accessList, { entity: `${EntityKind.USER}:${manualEntityName}`, roles: [] }]) setManualEntityName('') + onAddEntityManually(manualEntityName) } } @@ -51,7 +45,6 @@ export default function ManualEntryAccess({ accessList, setAccessList }: ManualE Date: Wed, 20 Nov 2024 18:08:53 +0000 Subject: [PATCH 10/20] improved duplicate entity checking when manually adding collaboratores --- backend/src/services/model.ts | 27 +++++++++++++++++---------- 1 file changed, 17 insertions(+), 10 deletions(-) diff --git a/backend/src/services/model.ts b/backend/src/services/model.ts index c9889ef3e..1a89c9542 100644 --- a/backend/src/services/model.ts +++ b/backend/src/services/model.ts @@ -343,16 +343,23 @@ export async function updateModel(user: UserInterface, modelId: string, modelDif } async function checkCollaboratorAuthorisation(collaborators: CollaboratorEntry[]) { - const duplicateEntityNames: string[] = [] - collaborators.forEach((collaborator) => { - if (collaborators[collaborator.entity]) { - duplicateEntityNames.push(collaborator.entity) - } else { - collaborators[collaborator.entity] = true - } - }) - if (duplicateEntityNames.length > 0) { - throw BadReq(`The following duplicate collaborators have been found: ${duplicateEntityNames.join(', ')}`) + const duplicateEntities = collaborators.reduce( + (duplicates, currentCollaborator, currentCollaboratorIndex) => { + if ( + collaborators.find( + (collaborator, index) => + index !== currentCollaboratorIndex && collaborator.entity === currentCollaborator.entity, + ) && + !duplicates.includes(currentCollaborator.entity) + ) { + duplicates.push(currentCollaborator.entity) + } + return duplicates + }, + [], + ) + if (duplicateEntities.length > 0) { + throw BadReq(`The following duplicate collaborators have been found: ${duplicateEntities.join(', ')}`) } await Promise.all( collaborators.map(async (collaborator) => { From d7ae9a77932862ae520bfa5ff40ae1949fc382e6 Mon Sep 17 00:00:00 2001 From: araddcc002 Date: Thu, 21 Nov 2024 10:12:10 +0000 Subject: [PATCH 11/20] updated various names of components and values for manual user access --- .../src/entry/settings/EntryAccessInput.tsx | 26 +++++++------------ .../src/entry/settings/ManualEntityInput.tsx | 2 +- 2 files changed, 11 insertions(+), 17 deletions(-) diff --git a/frontend/src/entry/settings/EntryAccessInput.tsx b/frontend/src/entry/settings/EntryAccessInput.tsx index e16e319c6..f5a106818 100644 --- a/frontend/src/entry/settings/EntryAccessInput.tsx +++ b/frontend/src/entry/settings/EntryAccessInput.tsx @@ -28,7 +28,7 @@ export default function EntryAccessInput({ value, onUpdate, entryKind, entryRole const [open, setOpen] = useState(false) const [accessList, setAccessList] = useState(value) const [userListQuery, setUserListQuery] = useState('') - const [manualEntityEntryErrorMessage, setManualEntityEntryErrorMessage] = useState('') + const [manualEntityInputErrorMessage, setManualEntityInputErrorMessage] = useState('') const { users, isUsersLoading, isUsersError } = useListUsers(userListQuery) @@ -73,17 +73,14 @@ export default function EntryAccessInput({ value, onUpdate, entryKind, entryRole setUserListQuery(value) }, []) - const handleNewManualEntityEntry = useCallback( - (manualEntityName: string) => { - setManualEntityEntryErrorMessage('') - if (accessList.find((collaborator) => collaborator.entity === `${EntityKind.USER}:${manualEntityName}`)) { - setManualEntityEntryErrorMessage('User has already been added below.') - } else { - setAccessList([...accessList, { entity: `${EntityKind.USER}:${manualEntityName}`, roles: [] }]) - } - }, - [accessList], - ) + const handleAddEntityManually = (manualEntityName: string) => { + setManualEntityInputErrorMessage('') + if (accessList.find((collaborator) => collaborator.entity === `${EntityKind.USER}:${manualEntityName}`)) { + setManualEntityInputErrorMessage('User has already been added below.') + } else { + setAccessList([...accessList, { entity: `${EntityKind.USER}:${manualEntityName}`, roles: [] }]) + } + } const debounceOnInputChange = debounce((event: SyntheticEvent, value: string) => { handleInputChange(event, value) @@ -127,10 +124,7 @@ export default function EntryAccessInput({ value, onUpdate, entryKind, entryRole /> )} /> - +
diff --git a/frontend/src/entry/settings/ManualEntityInput.tsx b/frontend/src/entry/settings/ManualEntityInput.tsx index 144bd2fe7..ce551486e 100644 --- a/frontend/src/entry/settings/ManualEntityInput.tsx +++ b/frontend/src/entry/settings/ManualEntityInput.tsx @@ -16,7 +16,7 @@ export default function ManualEntityInput({ onAddEntityManually, errorMessage }: const { uiConfig, isUiConfigLoading, isUiConfigError } = useGetUiConfig() - const handleAddEntityManuallyOnClick = async () => { + const handleAddEntityManuallyOnClick = () => { if (manualEntityName !== undefined && manualEntityName !== '') { setManualEntityName('') onAddEntityManually(manualEntityName) From a8baf9fc8ea2fd39820118ea515441beca1cfa7f Mon Sep 17 00:00:00 2001 From: araddcc002 Date: Fri, 22 Nov 2024 13:55:42 +0000 Subject: [PATCH 12/20] added migration script for removing invalid users --- .../011_find_and_remove_invalid_users.ts | 44 +++++++++++++++++++ backend/src/models/Migration.ts | 6 +++ backend/src/services/migration.ts | 15 ++++--- backend/src/services/model.ts | 2 +- backend/src/utils/database.ts | 4 +- .../src/entry/settings/ManualEntityInput.tsx | 5 ++- 6 files changed, 66 insertions(+), 10 deletions(-) create mode 100644 backend/src/migrations/011_find_and_remove_invalid_users.ts diff --git a/backend/src/migrations/011_find_and_remove_invalid_users.ts b/backend/src/migrations/011_find_and_remove_invalid_users.ts new file mode 100644 index 000000000..a0fa6970d --- /dev/null +++ b/backend/src/migrations/011_find_and_remove_invalid_users.ts @@ -0,0 +1,44 @@ +import authentication from '../connectors/authentication/index.js' +import { MigrationMetadata } from '../models/Migration.js' +import ModelModel from '../models/Model.js' + +/** + * As we now do backend validation for users being added to model access lists, we + * added this script to find and remove all existing users that do not pass the + * "getUserInformation" call in the authentication connector. You can find a + * list of removed users for all affected models by looking at the "metadata" + * property of this migration's database object. + **/ + +export async function up() { + const models = await ModelModel.find({}) + const metadata: MigrationMetadata[] = [] + for (const model of models) { + const invalidUsers: string[] = [] + await Promise.all( + model.collaborators.map(async (collaborator) => { + if (collaborator.entity !== '') { + try { + await authentication.getUserInformation(collaborator.entity) + } catch (err) { + invalidUsers.push(collaborator.entity) + } + } + }), + ) + if (invalidUsers.length > 0) { + const invalidUsersForModel = { modelId: model.id, invalidUsers: invalidUsers } + const invalidUsersRemoved = model.collaborators.filter( + (collaborator) => !invalidUsers.includes(collaborator.entity), + ) + model.collaborators = invalidUsersRemoved + await model.save() + metadata.push(invalidUsersForModel) + } + } + return metadata +} + +export async function down() { + /* NOOP */ +} diff --git a/backend/src/models/Migration.ts b/backend/src/models/Migration.ts index af634ebd7..8f83d8b31 100644 --- a/backend/src/models/Migration.ts +++ b/backend/src/models/Migration.ts @@ -1,7 +1,12 @@ import { Document, model, Schema } from 'mongoose' +export interface MigrationMetadata { + [key: string]: any +} + export interface Migration { name: string + metadata?: MigrationMetadata createdAt: Date updatedAt: Date @@ -12,6 +17,7 @@ export type MigrationDoc = Migration & Document const MigrationSchema = new Schema( { name: { type: String, required: true }, + metadata: { type: Schema.Types.Mixed }, }, { timestamps: true, diff --git a/backend/src/services/migration.ts b/backend/src/services/migration.ts index 9c248a44e..9cb8b4d2b 100644 --- a/backend/src/services/migration.ts +++ b/backend/src/services/migration.ts @@ -1,4 +1,4 @@ -import MigrationModel from '../models/Migration.js' +import MigrationModel, { MigrationMetadata } from '../models/Migration.js' export async function doesMigrationExist(name: string) { const migration = await MigrationModel.findOne({ @@ -12,8 +12,13 @@ export async function doesMigrationExist(name: string) { return true } -export async function markMigrationComplete(name: string) { - await MigrationModel.create({ - name, - }) +export async function markMigrationComplete(name: string, metadata: MigrationMetadata | undefined) { + metadata + ? await MigrationModel.create({ + name, + metadata, + }) + : await MigrationModel.create({ + name, + }) } diff --git a/backend/src/services/model.ts b/backend/src/services/model.ts index 1a89c9542..adf36408a 100644 --- a/backend/src/services/model.ts +++ b/backend/src/services/model.ts @@ -342,7 +342,7 @@ export async function updateModel(user: UserInterface, modelId: string, modelDif return model } -async function checkCollaboratorAuthorisation(collaborators: CollaboratorEntry[]) { +export async function checkCollaboratorAuthorisation(collaborators: CollaboratorEntry[]) { const duplicateEntities = collaborators.reduce( (duplicates, currentCollaborator, currentCollaboratorIndex) => { if ( diff --git a/backend/src/utils/database.ts b/backend/src/utils/database.ts index aae04ddc6..f26cc6200 100644 --- a/backend/src/utils/database.ts +++ b/backend/src/utils/database.ts @@ -57,9 +57,9 @@ export async function runMigrations() { // run migration const migration = await import(join(base, file)) - await migration.up() + const runMigration = await migration.up() - await markMigrationComplete(file) + await markMigrationComplete(file, runMigration) log.info({ file }, `Finished migration ${file}`) } diff --git a/frontend/src/entry/settings/ManualEntityInput.tsx b/frontend/src/entry/settings/ManualEntityInput.tsx index ce551486e..1295f29df 100644 --- a/frontend/src/entry/settings/ManualEntityInput.tsx +++ b/frontend/src/entry/settings/ManualEntityInput.tsx @@ -1,7 +1,7 @@ import ExpandMoreIcon from '@mui/icons-material/ExpandMore' import { Accordion, AccordionDetails, AccordionSummary, Box, Button, Stack, TextField, Typography } from '@mui/material' import { useGetUiConfig } from 'actions/uiConfig' -import { useState } from 'react' +import { FormEvent, useState } from 'react' import HelpPopover from 'src/common/HelpPopover' import Loading from 'src/common/Loading' import MessageAlert from 'src/MessageAlert' @@ -16,7 +16,8 @@ export default function ManualEntityInput({ onAddEntityManually, errorMessage }: const { uiConfig, isUiConfigLoading, isUiConfigError } = useGetUiConfig() - const handleAddEntityManuallyOnClick = () => { + const handleAddEntityManuallyOnClick = (event: FormEvent) => { + event.preventDefault() if (manualEntityName !== undefined && manualEntityName !== '') { setManualEntityName('') onAddEntityManually(manualEntityName) From 5b8ad951c455868b54d9d681d402e7aaa3a4dad6 Mon Sep 17 00:00:00 2001 From: araddcc002 Date: Mon, 25 Nov 2024 10:21:16 +0000 Subject: [PATCH 13/20] added a check for new collabs --- backend/src/services/model.ts | 30 ++++++++++++++++++++---------- 1 file changed, 20 insertions(+), 10 deletions(-) diff --git a/backend/src/services/model.ts b/backend/src/services/model.ts index adf36408a..f4cbc6f1a 100644 --- a/backend/src/services/model.ts +++ b/backend/src/services/model.ts @@ -322,7 +322,7 @@ export async function updateModel(user: UserInterface, modelId: string, modelDif throw BadReq('You cannot select both mirror settings simultaneously.') } if (modelDiff.collaborators) { - await checkCollaboratorAuthorisation(modelDiff.collaborators) + await checkCollaboratorAuthorisation(modelDiff.collaborators, model.collaborators) } const auth = await authorisation.model(user, model, ModelAction.Update) @@ -342,11 +342,15 @@ export async function updateModel(user: UserInterface, modelId: string, modelDif return model } -export async function checkCollaboratorAuthorisation(collaborators: CollaboratorEntry[]) { - const duplicateEntities = collaborators.reduce( +export async function checkCollaboratorAuthorisation( + updatedCollaborators: CollaboratorEntry[], + previousCollaborators: CollaboratorEntry[] = [], +) { + const previousCollaboratorEntities: string[] = previousCollaborators.map((collaborator) => collaborator.entity) + const duplicates = updatedCollaborators.reduce( (duplicates, currentCollaborator, currentCollaboratorIndex) => { if ( - collaborators.find( + updatedCollaborators.find( (collaborator, index) => index !== currentCollaboratorIndex && collaborator.entity === currentCollaborator.entity, ) && @@ -358,18 +362,24 @@ export async function checkCollaboratorAuthorisation(collaborators: Collaborator }, [], ) - if (duplicateEntities.length > 0) { - throw BadReq(`The following duplicate collaborators have been found: ${duplicateEntities.join(', ')}`) + if (duplicates.length > 0) { + throw BadReq(`The following duplicate collaborators have been found: ${duplicates.join(', ')}`) } + const newCollaborators = updatedCollaborators.reduce((acc, currentCollaborator) => { + if (!previousCollaboratorEntities.includes(currentCollaborator.entity)) { + acc.push(currentCollaborator.entity) + } + return acc + }, []) await Promise.all( - collaborators.map(async (collaborator) => { - if (collaborator.entity === '') { + newCollaborators.map(async (collaborator) => { + if (collaborator === '') { throw BadReq('Collaborator name must be a valid string') } try { - await authentication.getUserInformation(collaborator.entity) + await authentication.getUserInformation(collaborator) } catch (err) { - throw InternalError(`Unable to find user ${collaborator.entity}`, { err }) + throw InternalError(`Unable to find user ${collaborator}`, { err }) } }), ) From 14a10207102e8ca9cc7197cc49f9009f3bf771df Mon Sep 17 00:00:00 2001 From: araddcc002 Date: Mon, 25 Nov 2024 10:35:37 +0000 Subject: [PATCH 14/20] changes to new user auth checking error throwing --- backend/src/services/model.ts | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/backend/src/services/model.ts b/backend/src/services/model.ts index f4cbc6f1a..3ed47751b 100644 --- a/backend/src/services/model.ts +++ b/backend/src/services/model.ts @@ -379,7 +379,11 @@ export async function checkCollaboratorAuthorisation( try { await authentication.getUserInformation(collaborator) } catch (err) { - throw InternalError(`Unable to find user ${collaborator}`, { err }) + if (err instanceof Error && err.name === 'Registry Error') { + throw NotFound(`Unable to find user ${collaborator}`, { err }) + } else { + throw err + } } }), ) From 549b3f82f68ceca5f24c78206b11e2dc83eb2c9c Mon Sep 17 00:00:00 2001 From: araddcc002 Date: Mon, 25 Nov 2024 10:39:34 +0000 Subject: [PATCH 15/20] changes to new user auth checking error throwing --- backend/src/services/model.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/backend/src/services/model.ts b/backend/src/services/model.ts index 3ed47751b..f096ca8fc 100644 --- a/backend/src/services/model.ts +++ b/backend/src/services/model.ts @@ -379,7 +379,7 @@ export async function checkCollaboratorAuthorisation( try { await authentication.getUserInformation(collaborator) } catch (err) { - if (err instanceof Error && err.name === 'Registry Error') { + if (err instanceof Error && err.name === 'Not Found') { throw NotFound(`Unable to find user ${collaborator}`, { err }) } else { throw err From 5d5e4f7a1857edfd42873597e2baa721a5ea41f1 Mon Sep 17 00:00:00 2001 From: araddcc002 Date: Mon, 25 Nov 2024 12:30:38 +0000 Subject: [PATCH 16/20] removed uneeded unit tests --- backend/src/services/model.ts | 10 +--------- backend/test/services/model.spec.ts | 18 ------------------ 2 files changed, 1 insertion(+), 27 deletions(-) diff --git a/backend/src/services/model.ts b/backend/src/services/model.ts index f096ca8fc..094e8e36f 100644 --- a/backend/src/services/model.ts +++ b/backend/src/services/model.ts @@ -376,15 +376,7 @@ export async function checkCollaboratorAuthorisation( if (collaborator === '') { throw BadReq('Collaborator name must be a valid string') } - try { - await authentication.getUserInformation(collaborator) - } catch (err) { - if (err instanceof Error && err.name === 'Not Found') { - throw NotFound(`Unable to find user ${collaborator}`, { err }) - } else { - throw err - } - } + await authentication.getUserInformation(collaborator) }), ) } diff --git a/backend/test/services/model.spec.ts b/backend/test/services/model.spec.ts index 3757cba2d..496776d18 100644 --- a/backend/test/services/model.spec.ts +++ b/backend/test/services/model.spec.ts @@ -107,15 +107,6 @@ describe('services > model', () => { expect(modelMocks.save).not.toBeCalled() }) - test('createModel > should throw an internal error if getUserInformation fails due to invalid user', async () => { - authenticationMocks.getUserInformation.mockImplementation(() => { - throw new Error('Could not find user') - }) - expect(() => - createModel({} as any, { collaborators: [{ entity: 'user:unknown_user', roles: [] }] } as any), - ).rejects.toThrowError(/^Unable to find user user:unknown_user/) - }) - test('createModel > bad request is thrown when attempting to set both source and destinationModelId simultaneously', async () => { vi.mocked(authorisation.model).mockResolvedValueOnce({ info: 'You cannot select both settings simultaneously.', @@ -336,15 +327,6 @@ describe('services > model', () => { ).rejects.toThrowError(/^You cannot select both mirror settings simultaneously./) }) - test('updateModel > should throw an internal error if getUserInformation fails due to invalid user', async () => { - authenticationMocks.getUserInformation.mockImplementation(() => { - throw new Error('Could not find user') - }) - expect(() => - updateModel({} as any, '123', { collaborators: [{ entity: 'user:unknown_user', roles: [] }] }), - ).rejects.toThrowError(/^Unable to find user user:unknown_user/) - }) - test('createModelcardFromSchema > should throw an error when attempting to change a model from mirrored to standard', async () => { vi.mocked(authorisation.model).mockResolvedValue({ info: 'Cannot alter a mirrored model.', From 540b1269b178c80cbbb73dc66435c201fa51c0d2 Mon Sep 17 00:00:00 2001 From: araddcc002 Date: Mon, 25 Nov 2024 14:55:47 +0000 Subject: [PATCH 17/20] updated collaborator validation function name --- backend/src/services/model.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/backend/src/services/model.ts b/backend/src/services/model.ts index 094e8e36f..41520d895 100644 --- a/backend/src/services/model.ts +++ b/backend/src/services/model.ts @@ -34,7 +34,7 @@ export async function createModel(user: UserInterface, modelParams: CreateModelP const modelId = convertStringToId(modelParams.name) if (modelParams.collaborators) { - await checkCollaboratorAuthorisation(modelParams.collaborators) + await validateCollaborators(modelParams.collaborators) } let collaborators: CollaboratorEntry[] = [] @@ -322,7 +322,7 @@ export async function updateModel(user: UserInterface, modelId: string, modelDif throw BadReq('You cannot select both mirror settings simultaneously.') } if (modelDiff.collaborators) { - await checkCollaboratorAuthorisation(modelDiff.collaborators, model.collaborators) + await validateCollaborators(modelDiff.collaborators, model.collaborators) } const auth = await authorisation.model(user, model, ModelAction.Update) @@ -342,7 +342,7 @@ export async function updateModel(user: UserInterface, modelId: string, modelDif return model } -export async function checkCollaboratorAuthorisation( +async function validateCollaborators( updatedCollaborators: CollaboratorEntry[], previousCollaborators: CollaboratorEntry[] = [], ) { From 72eabf527d16342ad306d2ec2e419cab599cbf9d Mon Sep 17 00:00:00 2001 From: araddcc002 Date: Tue, 26 Nov 2024 10:24:19 +0000 Subject: [PATCH 18/20] added a check so that only users are checked by auth connector --- backend/src/services/model.ts | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/backend/src/services/model.ts b/backend/src/services/model.ts index 41520d895..967535423 100644 --- a/backend/src/services/model.ts +++ b/backend/src/services/model.ts @@ -376,7 +376,10 @@ async function validateCollaborators( if (collaborator === '') { throw BadReq('Collaborator name must be a valid string') } - await authentication.getUserInformation(collaborator) + // TODO we currently only check for users, we should consider how we want to handle groups + if (collaborator.startsWith('user:')) { + await authentication.getUserInformation(collaborator) + } }), ) } From 2fd33d63baa6266c359db176e16204ac546d2753 Mon Sep 17 00:00:00 2001 From: araddcc002 Date: Tue, 26 Nov 2024 14:00:52 +0000 Subject: [PATCH 19/20] improved entity kind check for new collaborators --- backend/src/services/model.ts | 7 ++++--- backend/src/types/types.ts | 5 +++++ 2 files changed, 9 insertions(+), 3 deletions(-) diff --git a/backend/src/services/model.ts b/backend/src/services/model.ts index 967535423..7ea0ac3b3 100644 --- a/backend/src/services/model.ts +++ b/backend/src/services/model.ts @@ -12,9 +12,9 @@ import ModelCardRevisionModel, { } from '../models/ModelCardRevision.js' import { UserInterface } from '../models/User.js' import { GetModelCardVersionOptions, GetModelCardVersionOptionsKeys, GetModelFiltersKeys } from '../types/enums.js' -import { EntryUserPermissions } from '../types/types.js' +import { EntityKind, EntryUserPermissions } from '../types/types.js' import { isValidatorResultError } from '../types/ValidatorResultError.js' -import { toEntity } from '../utils/entity.js' +import { fromEntity, toEntity } from '../utils/entity.js' import { BadReq, Forbidden, InternalError, NotFound } from '../utils/error.js' import { convertStringToId } from '../utils/id.js' import { authResponseToUserPermission } from '../utils/permissions.js' @@ -377,7 +377,8 @@ async function validateCollaborators( throw BadReq('Collaborator name must be a valid string') } // TODO we currently only check for users, we should consider how we want to handle groups - if (collaborator.startsWith('user:')) { + const { kind } = fromEntity(collaborator) + if (kind === EntityKind.USER) { await authentication.getUserInformation(collaborator) } }), diff --git a/backend/src/types/types.ts b/backend/src/types/types.ts index 8a8fe89f8..56b3a413e 100644 --- a/backend/src/types/types.ts +++ b/backend/src/types/types.ts @@ -3,6 +3,11 @@ export const RoleKind = { SCHEMA: 'schema', } as const +export enum EntityKind { + USER = 'user', + GROUP = 'group', +} + export type RoleKindKeys = (typeof RoleKind)[keyof typeof RoleKind] export interface Role { From 27697e684b78c8a703e6291ed03fdaadc3f4289f Mon Sep 17 00:00:00 2001 From: araddcc002 Date: Tue, 26 Nov 2024 15:44:15 +0000 Subject: [PATCH 20/20] readded userinformation error unit tests --- backend/test/services/model.spec.ts | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/backend/test/services/model.spec.ts b/backend/test/services/model.spec.ts index 496776d18..c003c5a79 100644 --- a/backend/test/services/model.spec.ts +++ b/backend/test/services/model.spec.ts @@ -120,6 +120,15 @@ describe('services > model', () => { expect(modelMocks.save).not.toBeCalled() }) + test('createModel > should throw an internal error if getUserInformation fails due to invalid user', async () => { + authenticationMocks.getUserInformation.mockImplementation(() => { + throw new Error('Unable to find user user:unknown_user') + }) + expect(() => + createModel({} as any, { collaborators: [{ entity: 'user:unknown_user', roles: [] }] } as any), + ).rejects.toThrowError(/^Unable to find user user:unknown_user/) + }) + test('getModelById > good', async () => { modelMocks.findOne.mockResolvedValueOnce('mocked') @@ -327,6 +336,15 @@ describe('services > model', () => { ).rejects.toThrowError(/^You cannot select both mirror settings simultaneously./) }) + test('updateModel > should throw an internal error if getUserInformation fails due to invalid user', async () => { + authenticationMocks.getUserInformation.mockImplementation(() => { + throw new Error('Unable to find user user:unknown_user') + }) + expect(() => + updateModel({} as any, '123', { collaborators: [{ entity: 'user:unknown_user', roles: [] }] }), + ).rejects.toThrowError(/^Unable to find user user:unknown_user/) + }) + test('createModelcardFromSchema > should throw an error when attempting to change a model from mirrored to standard', async () => { vi.mocked(authorisation.model).mockResolvedValue({ info: 'Cannot alter a mirrored model.',