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.

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

Merged
merged 21 commits into from
Nov 27, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
21 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
c1fa402
wip
ARADDCC002 Nov 18, 2024
71d82a2
more changes from pr comments and merged main
ARADDCC002 Nov 18, 2024
7352462
wip
ARADDCC002 Nov 19, 2024
7b41a6e
updated model service to check for user auth on both update and creat…
ARADDCC002 Nov 20, 2024
39e5525
fixed an issue with config map
ARADDCC002 Nov 20, 2024
cd0d01d
addressed more pr comments and improved some of logic
ARADDCC002 Nov 20, 2024
e244ae9
improved duplicate entity checking when manually adding collaboratores
ARADDCC002 Nov 20, 2024
d7ae9a7
updated various names of components and values for manual user access
ARADDCC002 Nov 21, 2024
a8baf9f
added migration script for removing invalid users
ARADDCC002 Nov 22, 2024
5b8ad95
added a check for new collabs
ARADDCC002 Nov 25, 2024
14a1020
changes to new user auth checking error throwing
ARADDCC002 Nov 25, 2024
549b3f8
changes to new user auth checking error throwing
ARADDCC002 Nov 25, 2024
5d5e4f7
removed uneeded unit tests
ARADDCC002 Nov 25, 2024
540b126
updated collaborator validation function name
ARADDCC002 Nov 25, 2024
72eabf5
added a check so that only users are checked by auth connector
ARADDCC002 Nov 26, 2024
2fd33d6
improved entity kind check for new collaborators
ARADDCC002 Nov 26, 2024
27697e6
readded userinformation error unit tests
ARADDCC002 Nov 26, 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
4 changes: 4 additions & 0 deletions backend/config/default.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -195,6 +195,10 @@ module.exports = {
text: '',
startTimestamp: '',
},

helpPopoverText: {
manualEntryAccess: '',
Copy link
Member

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?

},
},

connectors: {
Expand Down
44 changes: 44 additions & 0 deletions backend/src/migrations/011_find_and_remove_invalid_users.ts
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 */
}
6 changes: 6 additions & 0 deletions backend/src/models/Migration.ts
Original file line number Diff line number Diff line change
@@ -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
Expand All @@ -12,6 +17,7 @@ export type MigrationDoc = Migration & Document<any, any, Migration>
const MigrationSchema = new Schema<Migration>(
{
name: { type: String, required: true },
metadata: { type: Schema.Types.Mixed },
},
{
timestamps: true,
Expand Down
15 changes: 10 additions & 5 deletions backend/src/services/migration.ts
Original file line number Diff line number Diff line change
@@ -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({
Expand All @@ -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,
})
}
56 changes: 53 additions & 3 deletions backend/src/services/model.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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'
Expand All @@ -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) =>
Expand Down Expand Up @@ -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>) {
Expand All @@ -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) {
Copy link
Member

Choose a reason for hiding this comment

The 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

Copy link
Member

Choose a reason for hiding this comment

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

Don't forget to add tests for this! :)

await validateCollaborators(modelDiff.collaborators, model.collaborators)
}

const auth = await authorisation.model(user, model, ModelAction.Update)
if (!auth.success) {
Expand All @@ -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<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')
}
// 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)
Copy link
Member

Choose a reason for hiding this comment

The 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.

}
}),
)
}

export async function createModelCardFromSchema(
user: UserInterface,
modelId: string,
Expand Down
9 changes: 9 additions & 0 deletions backend/src/types/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,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 {
Expand Down Expand Up @@ -85,4 +90,8 @@ export interface UiConfig {
text: string
startTimestamp: string
}

helpPopoverText: {
manualEntryAccess: string
}
}
4 changes: 2 additions & 2 deletions backend/src/utils/database.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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}`)
}
Expand Down
19 changes: 19 additions & 0 deletions backend/test/services/model.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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')

Expand Down Expand Up @@ -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.',
Expand Down
2 changes: 1 addition & 1 deletion frontend/src/common/HelpDialog.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ export default function HelpDialog({ title, content }: HelpDialogProps) {
<>
<Tooltip title={title}>
<IconButton size='small' onClick={handleOpen}>
<HelpOutlineIcon />
<HelpOutlineIcon color='primary' />
</IconButton>
</Tooltip>
<Dialog open={open} onClose={handleClose} maxWidth='md' TransitionComponent={Transition}>
Expand Down
1 change: 1 addition & 0 deletions frontend/src/common/HelpPopover.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ function HelpPopover({ anchorOrigin, transformOrigin, children }: Props) {
onMouseEnter={handlePopoverOpen}
onMouseLeave={handlePopoverClose}
data-test='helpIcon'
color='primary'
/>
<Popover
id='help-popover'
Expand Down
14 changes: 13 additions & 1 deletion frontend/src/entry/settings/EntryAccessInput.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,9 @@ 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 ManualEntityInput from 'src/entry/settings/ManualEntityInput'
import MessageAlert from 'src/MessageAlert'
import { CollaboratorEntry, EntityObject, EntryKindKeys, Role } from 'types/types'
import { CollaboratorEntry, EntityKind, EntityObject, EntryKindKeys, Role } from 'types/types'
import { toSentenceCase } from 'utils/stringUtils'

type EntryAccessInputProps = {
Expand All @@ -27,6 +28,7 @@ export default function EntryAccessInput({ value, onUpdate, entryKind, entryRole
const [open, setOpen] = useState(false)
const [accessList, setAccessList] = useState<CollaboratorEntry[]>(value)
const [userListQuery, setUserListQuery] = useState('')
const [manualEntityInputErrorMessage, setManualEntityInputErrorMessage] = useState('')

const { users, isUsersLoading, isUsersError } = useListUsers(userListQuery)

Expand Down Expand Up @@ -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<Element, Event>, value: string) => {
handleInputChange(event, value)
}, 500)
Expand Down Expand Up @@ -113,6 +124,7 @@ export default function EntryAccessInput({ value, onUpdate, entryKind, entryRole
/>
)}
/>
<ManualEntityInput onAddEntityManually={handleAddEntityManually} errorMessage={manualEntityInputErrorMessage} />
<Table>
<TableHead>
<TableRow>
Expand Down
68 changes: 68 additions & 0 deletions frontend/src/entry/settings/ManualEntityInput.tsx
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()
Copy link
Member

Choose a reason for hiding this comment

The 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?

Copy link
Member Author

Choose a reason for hiding this comment

The 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>
)
}
Loading