Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Feature/bai 1485 manual user access #1624

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Feature/bai 1485 manual user access #1624

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a4509ce
wip, added manual entity input
ARADDCC002 Nov 13, 2024
4f1836c
added a new function for user info look up as non-hook. added ability…
ARADDCC002 Nov 13, 2024
d580281
updated backend to check for invalid users
ARADDCC002 Nov 13, 2024
40131db
moved manual user access form to its own component
ARADDCC002 Nov 14, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 16 additions & 1 deletion backend/src/services/model.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -305,7 +305,10 @@ export async function updateModelCard(
return revision
}

export type UpdateModelParams = Pick<ModelInterface, 'name' | 'teamId' | 'description' | 'visibility'> & {
export type UpdateModelParams = Pick<
ModelInterface,
'name' | 'teamId' | 'description' | 'visibility' | 'collaborators'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

teamId isn't a valid property anymore, so it looks like this was missed from your other PR. It's a little concerning that this isn't causing TS errors, so can we double check that there's not any more that we've missed?

> & {
settings: Partial<ModelInterface['settings']>
}
export async function updateModel(user: UserInterface, modelId: string, modelDiff: Partial<UpdateModelParams>) {
Expand All @@ -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 = []
Copy link
Member

@ARADDCC012 ARADDCC012 Nov 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we type this array please?

It's worth noting that not all entities are users, so we should probably use the word "entity" instead of "user". At some point I'd like to get around to fixing this across the app, but it would be nice to at least not add to the tech debt.

I also think we should change this to store the collaboration.entity so that we can inform users of what has failed by returning exactly what they provided.

const invalidEntities: string[] = []

modelDiff.collaborators.forEach(async (collaborator) => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can't await the promises from this async callback when using .forEach() as it doesn't return anything. Instead we can use .map() like so:

await Promise.all(modelDiff.collaborators.map(async (collaborator) => { ... }))

const userInformation = await authentication.getUserInformation(collaborator.entity)
if (!userInformation) {
invalidUsers.push(userInformation)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As mentioned in another comment (https://github.com/gchq/Bailo/pull/1624/files#r1843611009) I think it's better if we store the entity string that's invalid, so we can return that to the user.

invalidEntities.push(collaborator.entity)

}
})
if (invalidUsers.length > 0) {
throw BadReq('One or more user is invalid')
Copy link
Member

@ARADDCC012 ARADDCC012 Nov 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could tell the user which users are invalid by doing something like:

// We probably need to have the entry kind label formatting logic from the frontend to make this a nice error.
throw BadReq(
  `Failed to update ${`model.kind`}. Invalid ${invalidEntities.length === 1 ? 'entity' : 'entities'}: `${invalidEntities.join(', ')}`
  )

}
}

const auth = await authorisation.model(user, model, ModelAction.Update)
if (!auth.success) {
Expand Down
10 changes: 8 additions & 2 deletions frontend/actions/user.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,9 +49,9 @@ interface UserInformationResponse {
entity: UserInformation
}

export function useGetUserInformation(dn: string) {
export function useGetUserInformation(dn: string, kind?: string) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we give kind a more specific type? I'm assuming this can be 'user' or 'group' but I'm not sure just by looking at it.

const { data, isLoading, error, mutate } = useSWR<UserInformationResponse, ErrorInfo>(
`/api/v2/entity/${dn}/lookup`,
`/api/v2/entity/${dn}/lookup${kind ? `?kind=${kind}` : ''}`,
fetcher,
)

Expand All @@ -63,6 +63,12 @@ export function useGetUserInformation(dn: string) {
}
}

export function getUserInformation(dn: string) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This won't be needed if we let the backend handle all of the validation.

return fetch(`/api/v2/entity/${dn}/lookup`, {
headers: { 'Content-Type': 'application/json' },
})
}

interface GetUserTokensResponse {
tokens: TokenInterface[]
}
Expand Down
2 changes: 2 additions & 0 deletions frontend/src/entry/settings/EntryAccessInput.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ 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, EntityObject, EntryKindKeys, Role } from 'types/types'
import { toSentenceCase } from 'utils/stringUtils'
Expand Down Expand Up @@ -113,6 +114,7 @@ export default function EntryAccessInput({ value, onUpdate, entryKind, entryRole
/>
)}
/>
<ManualEntryAccess accessList={accessList} setAccessList={setAccessList} />
<Table>
<TableHead>
<TableRow>
Expand Down
67 changes: 67 additions & 0 deletions frontend/src/entry/settings/ManualEntryAccess.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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[]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This component doesn't have to care about the access list. It can probably have a single prop onAddEntityManually: (entityName: string) => void. We can let the main EntryAccessInput component manage the access list logic, though I also think the checks we're doing within handleAddEntityManuallyOnClick can and should be handled by the backend.

setAccessList: (accessList: CollaboratorEntry[]) => void
}

export default function ManualEntryAccess({ accessList, setAccessList }: ManualEntryAccessProps) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider a name change to something like ManualEntityInput as this component isn't responsible for managing entry access in itself.

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.`)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The backend should handle this, not the frontend. An API user can bypass this check.

}
const response = await getUserInformation(manualEntityName)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be handled by the backend

if (!response.ok) {
return setErrorMessage(await getErrorMessage(response))
}
const updatedAccessList = [...accessList]
const newAccess = { entity: `${EntityKind.USER}:${manualEntityName}`, roles: [] }
updatedAccessList.push(newAccess)
setAccessList(updatedAccessList)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider condensing this like so:

setAccessList([...accessList, { entity: `${EntityKind.USER}:${manualEntityName}`, roles: [] }])

It turns 4 lines into 1 and I think it's also easier to read.

setManualEntityName('')
}
}

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 }}>
<Stack spacing={2} direction={{ xs: 'column', sm: 'row' }}>
<TextField
id='manual-entity-name-select'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't look like this id is needed

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good spot, I think this was left over from when I was using a FormControl to add a label to it .

placeholder='Joe Bloggs'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct me if I'm wrong here, but aren't we expecting users to provide a dn here? I'm not sure this placeholder is useful if that's the case.

Depending on how intuitive it is to know what information should be provided by a user, we may benefit from using a help icon that explains what can be provided.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I appreciate the placeholder might not be that useful. A help icon with more information could be the way to go but I'd imagine we'd need to make it configurable.

size='small'
fullWidth
label='User'
value={manualEntityName}
onChange={(e) => setManualEntityName(e.target.value)}
/>
<Button variant='contained' onClick={handleAddEntityManuallyOnClick} disabled={manualEntityName === ''}>
Add
</Button>
</Stack>
<MessageAlert message={errorMessage} severity='error' />
</AccordionDetails>
</Accordion>
)
}