-
Notifications
You must be signed in to change notification settings - Fork 15
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Feature/bai 1485 manual user access #1624
Changes from 16 commits
a4509ce
4f1836c
d580281
40131db
c1fa402
71d82a2
7352462
7b41a6e
39e5525
cd0d01d
e244ae9
d7ae9a7
a8baf9f
5b8ad95
14a1020
549b3f8
5d5e4f7
540b126
72eabf5
2fd33d6
27697e6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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 */ | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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) => | ||
|
@@ -303,7 +307,7 @@ export async function updateModelCard( | |
return revision | ||
} | ||
|
||
export type UpdateModelParams = Pick<ModelInterface, 'name' | 'description' | 'visibility'> & { | ||
export type UpdateModelParams = Pick<ModelInterface, 'name' | 'description' | 'visibility' | 'collaborators'> & { | ||
settings: Partial<ModelInterface['settings']> | ||
} | ||
export async function updateModel(user: UserInterface, modelId: string, modelDiff: Partial<UpdateModelParams>) { | ||
|
@@ -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) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you pull this functionality out into its own function? It would be useful to reuse this logic in create model as well There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Don't forget to add tests for this! :) |
||
await checkCollaboratorAuthorisation(modelDiff.collaborators, model.collaborators) | ||
} | ||
|
||
const auth = await authorisation.model(user, model, ModelAction.Update) | ||
if (!auth.success) { | ||
|
@@ -335,6 +342,53 @@ export async function updateModel(user: UserInterface, modelId: string, modelDif | |
return model | ||
} | ||
|
||
export async function checkCollaboratorAuthorisation( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This function doesn't check authorisation, could we change the name to something like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also, you shouldn't need to export it as it should only be used from within this file? |
||
updatedCollaborators: CollaboratorEntry[], | ||
previousCollaborators: CollaboratorEntry[] = [], | ||
) { | ||
const previousCollaboratorEntities: string[] = previousCollaborators.map((collaborator) => collaborator.entity) | ||
const duplicates = updatedCollaborators.reduce<string[]>( | ||
(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<string[]>((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') | ||
} | ||
try { | ||
await authentication.getUserInformation(collaborator) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This check will only work for users. If the new UI functionality only works for users and not groups, I'm happy that checking the validity of groups is out of scope for this. However, the collaborator you are checking here must be a user. |
||
} catch (err) { | ||
if (err instanceof Error && err.name === 'Not Found') { | ||
throw NotFound(`Unable to find user ${collaborator}`, { err }) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think there is any point in catching a not found, then throwing a not found. I thought we might want to catch a not found and then throw a bad request. If we do want to throw a not found here, I don't think this catch is required and whatever gets thrown by |
||
} else { | ||
throw err | ||
} | ||
} | ||
}), | ||
) | ||
} | ||
|
||
export async function createModelCardFromSchema( | ||
user: UserInterface, | ||
modelId: string, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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: '[email protected]' })), | ||
})) | ||
vi.mock('../../src/connectors/authentication/index.js', async () => ({ | ||
default: authenticationMocks, | ||
|
@@ -106,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.', | ||
|
@@ -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('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.', | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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<HTMLFormElement>) => { | ||
event.preventDefault() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's not always clear what behaviour we're trying to prevent when doing this sort of thing. Can we add a brief comment to explain? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This stops the page from refreshing when you submit the form |
||
if (manualEntityName !== undefined && manualEntityName !== '') { | ||
setManualEntityName('') | ||
onAddEntityManually(manualEntityName) | ||
} | ||
} | ||
|
||
if (isUiConfigError) { | ||
return <MessageAlert message={isUiConfigError.info.message} severity='error' /> | ||
} | ||
|
||
return ( | ||
<Accordion sx={{ borderTop: 'none' }}> | ||
<AccordionSummary | ||
sx={{ pl: 0, borderTop: 'none' }} | ||
expandIcon={<ExpandMoreIcon />} | ||
aria-controls='manual-user-add-content' | ||
id='manual-user-add-header' | ||
> | ||
<Typography sx={{ mr: 1 }} component='caption'> | ||
Trouble finding a user? Click here to add them manually | ||
</Typography> | ||
</AccordionSummary> | ||
<AccordionDetails sx={{ p: 0 }}> | ||
{isUiConfigLoading && <Loading />} | ||
{!isUiConfigLoading && uiConfig && ( | ||
<Box component='form' onSubmit={handleAddEntityManuallyOnClick}> | ||
<Stack spacing={2} direction={{ xs: 'column', sm: 'row' }} alignItems='center'> | ||
<TextField | ||
size='small' | ||
fullWidth | ||
label='User' | ||
value={manualEntityName} | ||
onChange={(e) => setManualEntityName(e.target.value)} | ||
/> | ||
{uiConfig.helpPopoverText.manualEntryAccess && ( | ||
<HelpPopover>{uiConfig.helpPopoverText.manualEntryAccess}</HelpPopover> | ||
)} | ||
<Button variant='contained' type='submit' disabled={manualEntityName === ''}> | ||
Add | ||
</Button> | ||
</Stack> | ||
</Box> | ||
)} | ||
<MessageAlert message={errorMessage} severity='error' /> | ||
</AccordionDetails> | ||
</Accordion> | ||
) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there some generic info that could provide some help?