Feature/bai 1485 manual user access #1624
Conversation
… to manually add users to model access
| @@ -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) { | |||
There was a problem hiding this comment.
Can you pull this functionality out into its own function? It would be useful to reuse this logic in create model as well
| @@ -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) { | |||
There was a problem hiding this comment.
Don't forget to add tests for this! :)
backend/src/services/model.ts
Outdated
| if (modelDiff.collaborators) { | ||
| const invalidUsers = [] | ||
| modelDiff.collaborators.forEach(async (collaborator) => { | ||
| const userInformation = await authentication.getUserInformation(collaborator.entity) |
There was a problem hiding this comment.
I think this should throw an error if the user can't be found. It's a bit difficult as this uses our connectors and the implementation could be anything but hopefully the typing of getUserInformation() can enforce that it can't just return nothing
backend/src/services/model.ts
Outdated
| @@ -335,6 +342,39 @@ export async function updateModel(user: UserInterface, modelId: string, modelDif | |||
| return model | |||
| } | |||
|
|
|||
| async function checkCollaboratorAuthorisation(collaborators: CollaboratorEntry[]) { | |||
There was a problem hiding this comment.
Could you change the name of this to validateCollaborators as this function isn't related to authorisation?
backend/src/services/model.ts
Outdated
| throw BadReq('Collaborator name must be a valid string') | ||
| } | ||
| try { | ||
| await authentication.getUserInformation(collaborator.entity) |
There was a problem hiding this comment.
Ideally it would be good to only do this check on just the collaborators that have been added to save having to make repeated external calls for entities that have already been validated. However, this would mean having to do a DB call to get the existing collaborators array so you could diff it with this one
| @@ -16,7 +16,8 @@ export default function ManualEntityInput({ onAddEntityManually, errorMessage }: | |||
|
|
|||
| const { uiConfig, isUiConfigLoading, isUiConfigError } = useGetUiConfig() | |||
|
|
|||
| const handleAddEntityManuallyOnClick = async () => { | |||
| const handleAddEntityManuallyOnClick = (event: FormEvent<HTMLFormElement>) => { | |||
| event.preventDefault() | |||
There was a problem hiding this comment.
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.
This stops the page from refreshing when you submit the form
| @@ -195,6 +195,10 @@ module.exports = { | |||
| text: '', | |||
| startTimestamp: '', | |||
| }, | |||
|
|
|||
| helpPopoverText: { | |||
| manualEntryAccess: '', | |||
There was a problem hiding this comment.
Is there some generic info that could provide some help?
| throw BadReq('Collaborator name must be a valid string') | ||
| } | ||
| try { | ||
| await authentication.getUserInformation(collaborator) |
There was a problem hiding this comment.
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.
backend/src/services/model.ts
Outdated
| await authentication.getUserInformation(collaborator) | ||
| } 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.
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 getUserInformation() should just propagate up.
backend/src/services/model.ts
Outdated
| @@ -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.
This function doesn't check authorisation, could we change the name to something like validateCollaborators
There was a problem hiding this comment.
Also, you shouldn't need to export it as it should only be used from within this file?
No description provided.