diff --git a/backend/config/default.cjs b/backend/config/default.cjs index 91a63eccf..db97ed0fd 100644 --- a/backend/config/default.cjs +++ b/backend/config/default.cjs @@ -201,6 +201,10 @@ module.exports = { text: '', startTimestamp: '', }, + + helpPopoverText: { + manualEntryAccess: '', + }, }, connectors: { 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 e363168d0..33863e506 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' @@ -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 validateCollaborators(modelParams.collaborators) + } + let collaborators: CollaboratorEntry[] = [] if (modelParams.collaborators && modelParams.collaborators.length > 0) { const collaboratorListContainsOwner = modelParams.collaborators.some((collaborator) => @@ -303,7 +307,7 @@ export async function updateModelCard( return revision } -export type UpdateModelParams = Pick & { +export type UpdateModelParams = Pick & { settings: Partial } export async function updateModel(user: UserInterface, modelId: string, modelDiff: Partial) { @@ -317,6 +321,9 @@ 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) { + await validateCollaborators(modelDiff.collaborators, model.collaborators) + } const auth = await authorisation.model(user, model, ModelAction.Update) if (!auth.success) { @@ -335,6 +342,49 @@ export async function updateModel(user: UserInterface, modelId: string, modelDif return model } +async function validateCollaborators( + updatedCollaborators: CollaboratorEntry[], + previousCollaborators: CollaboratorEntry[] = [], +) { + const previousCollaboratorEntities: string[] = previousCollaborators.map((collaborator) => collaborator.entity) + const duplicates = updatedCollaborators.reduce( + (duplicates, currentCollaborator, currentCollaboratorIndex) => { + if ( + updatedCollaborators.find( + (collaborator, index) => + index !== currentCollaboratorIndex && collaborator.entity === currentCollaborator.entity, + ) && + !duplicates.includes(currentCollaborator.entity) + ) { + duplicates.push(currentCollaborator.entity) + } + return duplicates + }, + [], + ) + 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( + newCollaborators.map(async (collaborator) => { + if (collaborator === '') { + 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 + const { kind } = fromEntity(collaborator) + if (kind === EntityKind.USER) { + await authentication.getUserInformation(collaborator) + } + }), + ) +} + export async function createModelCardFromSchema( user: UserInterface, modelId: string, diff --git a/backend/src/types/types.ts b/backend/src/types/types.ts index 7fcb4196a..c3b8ec34c 100644 --- a/backend/src/types/types.ts +++ b/backend/src/types/types.ts @@ -9,6 +9,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 { @@ -91,4 +96,8 @@ export interface UiConfig { text: string startTimestamp: string } + + helpPopoverText: { + manualEntryAccess: string + } } 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/backend/test/services/model.spec.ts b/backend/test/services/model.spec.ts index 1a6999d2b..c003c5a79 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, @@ -119,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') @@ -326,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.', 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' /> (value) const [userListQuery, setUserListQuery] = useState('') + const [manualEntityInputErrorMessage, setManualEntityInputErrorMessage] = useState('') const { users, isUsersLoading, isUsersError } = useListUsers(userListQuery) @@ -71,6 +73,15 @@ export default function EntryAccessInput({ value, onUpdate, entryKind, entryRole setUserListQuery(value) }, []) + 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) }, 500) @@ -113,6 +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 new file mode 100644 index 000000000..1295f29df --- /dev/null +++ b/frontend/src/entry/settings/ManualEntityInput.tsx @@ -0,0 +1,68 @@ +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 { FormEvent, useState } from 'react' +import HelpPopover from 'src/common/HelpPopover' +import Loading from 'src/common/Loading' +import MessageAlert from 'src/MessageAlert' + +interface ManualEntityInputProps { + onAddEntityManually: (entityName: string) => void + errorMessage: string +} + +export default function ManualEntityInput({ onAddEntityManually, errorMessage }: ManualEntityInputProps) { + const [manualEntityName, setManualEntityName] = useState('') + + const { uiConfig, isUiConfigLoading, isUiConfigError } = useGetUiConfig() + + const handleAddEntityManuallyOnClick = (event: FormEvent) => { + event.preventDefault() + if (manualEntityName !== undefined && manualEntityName !== '') { + setManualEntityName('') + onAddEntityManually(manualEntityName) + } + } + + if (isUiConfigError) { + return + } + + return ( + + } + aria-controls='manual-user-add-content' + id='manual-user-add-header' + > + + Trouble finding a user? Click here to add them manually + + + + {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..c4f19bfb7 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.config.ui.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