Feature/bai 1485 manual user access #1624

ARADDCC002 · 2024-11-13T16:42:27Z

No description provided.

… to manually add users to model access

frontend/src/entry/settings/EntryAccessInput.tsx

ARADDCC012 · 2024-11-15T10:26:58Z

frontend/src/entry/settings/ManualEntryAccess.tsx

+import { getErrorMessage } from 'utils/fetcher'
+
+interface ManualEntryAccessProps {
+  accessList: CollaboratorEntry[]


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.

ARADDCC012 · 2024-11-15T10:28:24Z

frontend/src/entry/settings/ManualEntryAccess.tsx

+  setAccessList: (accessList: CollaboratorEntry[]) => void
+}
+
+export default function ManualEntryAccess({ accessList, setAccessList }: ManualEntryAccessProps) {


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

ARADDCC012 · 2024-11-15T10:29:31Z

frontend/src/entry/settings/ManualEntryAccess.tsx

+      <AccordionDetails sx={{ p: 0 }}>
+        <Stack spacing={2} direction={{ xs: 'column', sm: 'row' }}>
+          <TextField
+            id='manual-entity-name-select'


Doesn't look like this id is needed

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

ARADDCC012 · 2024-11-15T10:50:07Z

frontend/src/entry/settings/ManualEntryAccess.tsx

+        <Stack spacing={2} direction={{ xs: 'column', sm: 'row' }}>
+          <TextField
+            id='manual-entity-name-select'
+            placeholder='Joe Bloggs'


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.

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.

ARADDCC012 · 2024-11-15T10:57:18Z

frontend/actions/user.ts

@@ -49,9 +49,9 @@ interface UserInformationResponse {
  entity: UserInformation
 }

-export function useGetUserInformation(dn: string) {
+export function useGetUserInformation(dn: string, kind?: string) {


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.

ARADDCC012 · 2024-11-15T11:01:56Z

backend/src/services/model.ts

@@ -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) => {


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) => { ... }))

ARADDCC012 · 2024-11-15T11:06:21Z

backend/src/services/model.ts

-export type UpdateModelParams = Pick<ModelInterface, 'name' | 'teamId' | 'description' | 'visibility'> & {
+export type UpdateModelParams = Pick<
+  ModelInterface,
+  'name' | 'teamId' | 'description' | 'visibility' | 'collaborators'


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?

ARADDCC012 · 2024-11-15T11:14:51Z

backend/src/services/model.ts

@@ -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 = []


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[] = []

ARADDCC012 · 2024-11-15T11:18:54Z

backend/src/services/model.ts

+    modelDiff.collaborators.forEach(async (collaborator) => {
+      const userInformation = await authentication.getUserInformation(collaborator.entity)
+      if (!userInformation) {
+        invalidUsers.push(userInformation)


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)

ARADDCC012 · 2024-11-15T11:25:58Z

backend/src/services/model.ts

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


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(', ')}` )

ARADDCC012 · 2024-11-15T11:28:54Z

frontend/actions/user.ts

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

+export function getUserInformation(dn: string) {


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

ARADDCC012 · 2024-11-15T11:29:59Z

frontend/src/entry/settings/ManualEntryAccess.tsx

+    setErrorMessage('')
+    if (manualEntityName !== undefined && manualEntityName !== '') {
+      if (accessList.find((collaborator) => collaborator.entity === `${EntityKind.USER}:${manualEntityName}`)) {
+        return setErrorMessage(`The requested user has already been added below.`)


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

ARADDCC012 · 2024-11-15T11:33:59Z

frontend/src/entry/settings/ManualEntryAccess.tsx

+      if (accessList.find((collaborator) => collaborator.entity === `${EntityKind.USER}:${manualEntityName}`)) {
+        return setErrorMessage(`The requested user has already been added below.`)
+      }
+      const response = await getUserInformation(manualEntityName)


This should be handled by the backend

ARADDCC012 · 2024-11-15T11:35:44Z

frontend/src/entry/settings/ManualEntryAccess.tsx

+      const updatedAccessList = [...accessList]
+      const newAccess = { entity: `${EntityKind.USER}:${manualEntityName}`, roles: [] }
+      updatedAccessList.push(newAccess)
+      setAccessList(updatedAccessList)


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.

ARADDCC002 added 2 commits November 13, 2024 14:08

wip, added manual entity input

a4509ce

added a new function for user info look up as non-hook. added ability…

4f1836c

… to manually add users to model access

ARADDCC012 reviewed Nov 13, 2024

View reviewed changes

frontend/src/entry/settings/EntryAccessInput.tsx Outdated Show resolved Hide resolved

ARADDCC012 reviewed Nov 13, 2024

View reviewed changes

frontend/src/entry/settings/EntryAccessInput.tsx Outdated Show resolved Hide resolved

ARADDCC002 added 2 commits November 13, 2024 17:10

updated backend to check for invalid users

d580281

moved manual user access form to its own component

40131db

ARADDCC012 reviewed Nov 15, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/bai 1485 manual user access #1624

Feature/bai 1485 manual user access #1624

ARADDCC002 commented Nov 13, 2024

ARADDCC012 Nov 15, 2024

ARADDCC012 Nov 15, 2024

ARADDCC012 Nov 15, 2024

ARADDCC002 Nov 15, 2024

ARADDCC012 Nov 15, 2024

ARADDCC002 Nov 15, 2024

ARADDCC012 Nov 15, 2024

ARADDCC012 Nov 15, 2024

ARADDCC012 Nov 15, 2024

ARADDCC012 Nov 15, 2024 •

edited

Loading

ARADDCC012 Nov 15, 2024

ARADDCC012 Nov 15, 2024 •

edited

Loading

ARADDCC012 Nov 15, 2024

ARADDCC012 Nov 15, 2024

ARADDCC012 Nov 15, 2024

ARADDCC012 Nov 15, 2024

Feature/bai 1485 manual user access #1624

Are you sure you want to change the base?

Feature/bai 1485 manual user access #1624

Conversation

ARADDCC002 commented Nov 13, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ARADDCC012 Nov 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ARADDCC012 Nov 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ARADDCC012 Nov 15, 2024 •

edited

Loading

ARADDCC012 Nov 15, 2024 •

edited

Loading