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

Feature/conditionally hide UI based on roles #1377

Merged
merged 13 commits into from
Jul 10, 2024
Merged

Feature/conditionally hide UI based on roles #1377

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
31f3cb7
Added missing htmlId
ARADDCC012 Jul 1, 2024
3ae8800
Conditionally disabled part of UI based on users' roles
ARADDCC012 Jul 1, 2024
2dd7cb3
Fixed types
ARADDCC012 Jul 1, 2024
5c5af1e
Merge branch 'main' into feature/conditionally-hide-ui-based-on-roles
ARADDCC012 Jul 1, 2024
2987f27
Fixed create entry tests
ARADDCC012 Jul 1, 2024
0f4ec5f
Reverted forced typing in cypress tests
ARADDCC012 Jul 1, 2024
18add51
Reverted entry description back to single line
ARADDCC012 Jul 1, 2024
49bb0a5
Merge branch 'main' into feature/conditionally-hide-ui-based-on-roles
ARADDCC012 Jul 1, 2024
ebf7c14
Corrected TODO for delete entry functionality
ARADDCC012 Jul 1, 2024
f4796e2
Merge branch 'main' into feature/conditionally-hide-ui-based-on-roles
ARADDCC012 Jul 8, 2024
3c7b8b4
Added roles logic to ExportSettings
ARADDCC012 Jul 8, 2024
64e471b
Added roles logic to inferencing tab and settings
ARADDCC012 Jul 8, 2024
b24cde8
Minor optimisation of DangerZone disable logic
ARADDCC012 Jul 8, 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
22 changes: 18 additions & 4 deletions frontend/pages/data-card/[dataCardId].tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import { useGetModel } from 'actions/model'
import { useGetCurrentUser } from 'actions/user'
import { useRouter } from 'next/router'
import { useMemo } from 'react'
import Loading from 'src/common/Loading'
Expand All @@ -8,6 +9,7 @@ import Overview from 'src/entry/overview/Overview'
import Settings from 'src/entry/settings/Settings'
import MultipleErrorWrapper from 'src/errors/MultipleErrorWrapper'
import { EntryKind } from 'types/types'
import { getCurrentUserRoles } from 'utils/roles'

export default function DataCard() {
const router = useRouter()
Expand All @@ -17,27 +19,39 @@ export default function DataCard() {
isModelLoading: isDataCardLoading,
isModelError: isDataCardError,
} = useGetModel(dataCardId, EntryKind.DATA_CARD)
const { currentUser, isCurrentUserLoading, isCurrentUserError } = useGetCurrentUser()

const currentUserRoles = useMemo(() => getCurrentUserRoles(dataCard, currentUser), [dataCard, currentUser])

const tabs = useMemo(
() =>
dataCard
? [
{ title: 'Overview', path: 'overview', view: <Overview entry={dataCard} /> },
{ title: 'Settings', path: 'settings', view: <Settings entry={dataCard} /> },
{
title: 'Overview',
path: 'overview',
view: <Overview entry={dataCard} currentUserRoles={currentUserRoles} />,
},
{
title: 'Settings',
path: 'settings',
view: <Settings entry={dataCard} currentUserRoles={currentUserRoles} />,
},
]
: [],
[dataCard],
[currentUserRoles, dataCard],
)

const error = MultipleErrorWrapper(`Unable to load data card page`, {
isDataCardError,
isCurrentUserError,
})
if (error) return error

return (
<>
<Title text={dataCard ? dataCard.name : 'Loading...'} />
{isDataCardLoading && <Loading />}
{(isDataCardLoading || isCurrentUserLoading) && <Loading />}
{dataCard && (
<PageWithTabs
showCopyButton
Expand Down
23 changes: 14 additions & 9 deletions frontend/pages/model/[modelId].tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import Overview from 'src/entry/overview/Overview'
import Settings from 'src/entry/settings/Settings'
import MultipleErrorWrapper from 'src/errors/MultipleErrorWrapper'
import { EntryKind } from 'types/types'
import { getCurrentUserRoles } from 'utils/roles'

export default function Model() {
const router = useRouter()
Expand All @@ -22,17 +23,17 @@ export default function Model() {
const { currentUser, isCurrentUserLoading, isCurrentUserError } = useGetCurrentUser()
const { uiConfig, isUiConfigLoading, isUiConfigError } = useGetUiConfig()

const currentUserRoles = useMemo(
() =>
model?.collaborators.find((collaborator) => collaborator.entity.split(':')[1] === currentUser?.dn)?.roles || [],
[model, currentUser],
)
const currentUserRoles = useMemo(() => getCurrentUserRoles(model, currentUser), [model, currentUser])

const tabs = useMemo(
() =>
model && uiConfig
? [
{ title: 'Overview', path: 'overview', view: <Overview entry={model} /> },
{
title: 'Overview',
path: 'overview',
view: <Overview entry={model} currentUserRoles={currentUserRoles} />,
},
{
title: 'Releases',
path: 'releases',
Expand All @@ -51,15 +52,19 @@ export default function Model() {
{
title: 'Registry',
path: 'registry',
view: <ModelImages model={model} />,
view: <ModelImages model={model} currentUserRoles={currentUserRoles} />,
},
{
title: 'Inferencing',
path: 'inferencing',
view: <InferenceServices model={model} />,
view: <InferenceServices model={model} currentUserRoles={currentUserRoles} />,
hidden: !uiConfig.inference.enabled,
},
{ title: 'Settings', path: 'settings', view: <Settings entry={model} /> },
{
title: 'Settings',
path: 'settings',
view: <Settings entry={model} currentUserRoles={currentUserRoles} />,
},
Comment on lines +66 to +67
Copy link
Member

Choose a reason for hiding this comment

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

Not sure how I feel about this. In my opinion the settings tab itself should be restricted?

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 asked the same thing, but it's important for any user to be able to see the collaborators in the entry access tab. We could simplify a lot of the settings roles logic if we find another place to display that information. I'll make a ticket to explore this idea.

]
: [],
[model, uiConfig, currentUserRoles],
Expand Down
36 changes: 27 additions & 9 deletions frontend/pages/model/[modelId]/access-request/[accessRequestId].tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,11 @@
import ArrowBack from '@mui/icons-material/ArrowBack'
import { Button, Container, Divider, Paper, Stack, Typography } from '@mui/material'
import { useGetAccessRequest } from 'actions/accessRequest'
import { useGetModel } from 'actions/model'
import { useGetReviewRequestsForModel, useGetReviewRequestsForUser } from 'actions/review'
import { useGetCurrentUser } from 'actions/user'
import { useRouter } from 'next/router'
import { useState } from 'react'
import { useMemo, useState } from 'react'
import CopyToClipboardButton from 'src/common/CopyToClipboardButton'
import Loading from 'src/common/Loading'
import Title from 'src/common/Title'
Expand All @@ -12,6 +14,8 @@ import ReviewBanner from 'src/entry/model/reviews/ReviewBanner'
import MultipleErrorWrapper from 'src/errors/MultipleErrorWrapper'
import Link from 'src/Link'
import ReviewComments from 'src/reviews/ReviewComments'
import { EntryKind } from 'types/types'
import { getCurrentUserRoles, hasRole } from 'utils/roles'

export default function AccessRequest() {
const router = useRouter()
Expand All @@ -29,19 +33,29 @@ export default function AccessRequest() {
isReviewsLoading: isUserReviewsLoading,
isReviewsError: isUserReviewsError,
} = useGetReviewRequestsForUser()
const { model, isModelLoading, isModelError } = useGetModel(modelId, EntryKind.MODEL)
const { currentUser, isCurrentUserLoading, isCurrentUserError } = useGetCurrentUser()

const userCanReview =
reviews.filter((review) =>
userReviews.some(
(userReview) =>
userReview.model.id === review.model.id && userReview.accessRequestId === review.accessRequestId,
),
).length > 0
const currentUserRoles = useMemo(() => getCurrentUserRoles(model, currentUser), [model, currentUser])

const userCanReview = useMemo(
() =>
hasRole(currentUserRoles, ['msro']) &&
reviews.filter((review) =>
userReviews.some(
(userReview) =>
userReview.model.id === review.model.id && userReview.accessRequestId === review.accessRequestId,
),
).length > 0,
[currentUserRoles, reviews, userReviews],
)

const error = MultipleErrorWrapper('Unable to load access request', {
isAccessRequestError,
isReviewsError,
isUserReviewsError,
isModelError,
isCurrentUserError,
})
if (error) return error

Expand All @@ -50,7 +64,11 @@ export default function AccessRequest() {
<Title text={accessRequest ? accessRequest.metadata.overview.name : 'Loading...'} />
<Container maxWidth='lg' sx={{ my: 4 }} data-test='accessRequestContainer'>
<Paper>
{isAccessRequestLoading && isReviewsLoading && isUserReviewsLoading && <Loading />}
{(isAccessRequestLoading ||
isReviewsLoading ||
isUserReviewsLoading ||
isModelLoading ||
isCurrentUserLoading) && <Loading />}
{accessRequest && (
<>
{userCanReview && <ReviewBanner accessRequest={accessRequest} />}
Expand Down
24 changes: 18 additions & 6 deletions frontend/pages/model/[modelId]/inference/[image]/[tag]/settings.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,25 +1,37 @@
import { ArrowBack } from '@mui/icons-material'
import { Button, Card, Container, Divider, Link, Stack, Typography } from '@mui/material'
import { useGetInference } from 'actions/inferencing'
import { useGetModel } from 'actions/model'
import { useGetCurrentUser } from 'actions/user'
import { useRouter } from 'next/router'
import { useMemo } from 'react'
import Loading from 'src/common/Loading'
import Title from 'src/common/Title'
import EditableInference from 'src/entry/model/inferencing/EditableInference'
import MessageAlert from 'src/MessageAlert'
import MultipleErrorWrapper from 'src/errors/MultipleErrorWrapper'
import { EntryKind } from 'types/types'
import { getCurrentUserRoles } from 'utils/roles'

export default function InferenceSettings() {
const router = useRouter()
const { modelId, image, tag }: { modelId?: string; image?: string; tag?: string } = router.query
const { inference, isInferenceLoading, isInferenceError } = useGetInference(modelId, image, tag)
const { model, isModelLoading, isModelError } = useGetModel(modelId, EntryKind.MODEL)
const { currentUser, isCurrentUserLoading, isCurrentUserError } = useGetCurrentUser()

if (isInferenceError) {
return <MessageAlert message={isInferenceError.info.message} severity='error' />
}
const currentUserRoles = useMemo(() => getCurrentUserRoles(model, currentUser), [model, currentUser])

const error = MultipleErrorWrapper(`Unable to load inference settings page`, {
isInferenceError,
isModelError,
isCurrentUserError,
})
if (error) return error

return (
<>
<Title text={inference ? `${inference.image}:${inference.tag}` : 'Loading...'} />
{!inference || isInferenceLoading ? (
{!inference || isInferenceLoading || isModelLoading || isCurrentUserLoading ? (
<Loading />
) : (
<Container maxWidth='md'>
Expand All @@ -35,7 +47,7 @@ export default function InferenceSettings() {
{inference ? `${inference.image}:${inference.tag}` : 'Loading...'}
</Typography>
</Stack>
{inference && <EditableInference inference={inference} />}
{inference && <EditableInference inference={inference} currentUserRoles={currentUserRoles} />}
</Stack>
</Card>
</Container>
Expand Down
48 changes: 38 additions & 10 deletions frontend/pages/model/[modelId]/release/[semver].tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,11 @@
import ArrowBack from '@mui/icons-material/ArrowBack'
import { Button, Container, Divider, Paper, Stack, Typography } from '@mui/material'
import { useGetModel } from 'actions/model'
import { useGetRelease } from 'actions/release'
import { useGetReviewRequestsForModel, useGetReviewRequestsForUser } from 'actions/review'
import { useGetCurrentUser } from 'actions/user'
import { useRouter } from 'next/router'
import { useState } from 'react'
import { useMemo, useState } from 'react'
import CopyToClipboardButton from 'src/common/CopyToClipboardButton'
import Loading from 'src/common/Loading'
import Title from 'src/common/Title'
Expand All @@ -12,6 +14,8 @@ import ReviewBanner from 'src/entry/model/reviews/ReviewBanner'
import MultipleErrorWrapper from 'src/errors/MultipleErrorWrapper'
import Link from 'src/Link'
import ReviewComments from 'src/reviews/ReviewComments'
import { EntryKind } from 'types/types'
import { getCurrentUserRoles, hasRole } from 'utils/roles'

export default function Release() {
const router = useRouter()
Expand All @@ -29,24 +33,41 @@ export default function Release() {
isReviewsLoading: isUserReviewsLoading,
isReviewsError: isUserReviewsError,
} = useGetReviewRequestsForUser()
const { model, isModelLoading, isModelError } = useGetModel(modelId, EntryKind.MODEL)
const { currentUser, isCurrentUserLoading, isCurrentUserError } = useGetCurrentUser()

const userCanReview =
reviews.filter((review) =>
userReviews.some(
(userReview) =>
userReview.model.id === review.model.id && userReview.accessRequestId === review.accessRequestId,
),
).length > 0
const currentUserRoles = useMemo(() => getCurrentUserRoles(model, currentUser), [model, currentUser])

const userCanReview = useMemo(
() =>
hasRole(currentUserRoles, ['msro', 'mtr']) &&
Copy link
Member

Choose a reason for hiding this comment

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

I'm a little apprehensive about having authorisation now in both the frontend as well as the backend as it's duplication that will not be good for maintainability. Specifically here it's important to remember that in the near future that the roles for reviews will be dynamic and defined in the schema.

Copy link
Member Author

@ARADDCC012 ARADDCC012 Jul 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Agreed. I was conscious of that when writing this and have made a mental note to prepare for the amount of added effort this work will add to dynamic roles. I've tried to limit that by using the same util functions for most of this.

There's definitely some changes that will need to be made once dynamic roles come in, but I'm hoping that's as simple as updating the getCurrentUserRoles call. (At least for this current solution, so it shouldn't block dynamic roles)

As for the duplication of authorisation being in the frontend and backend, I'm not sure what we can do to solve that right now. This also leads into the read-only logic for mirrored models, which adds another layer of complexity, so I think we need to design a better solution that works for both. If we can come up with a solution that contains auth to the backend, even better.

It's worth noting that this work only expands on our existing roles implementation in the frontend, so imo we should create a ticket to address this issue for both roles and mirrored models rather than doing it in this PR.

reviews.filter((review) =>
userReviews.some(
(userReview) =>
userReview.model.id === review.model.id && userReview.accessRequestId === review.accessRequestId,
),
).length > 0,
[currentUserRoles, reviews, userReviews],
)

const error = MultipleErrorWrapper('Unable to load release', {
isReleaseError,
isReviewsError,
isUserReviewsError,
isModelError,
isCurrentUserError,
})

if (error) return error

if (!release || (isReleaseLoading && isReviewsLoading && isUserReviewsLoading)) {
if (
!release ||
isReleaseLoading ||
isReviewsLoading ||
isUserReviewsLoading ||
isModelLoading ||
isCurrentUserLoading
) {
return <Loading />
}

Expand Down Expand Up @@ -79,7 +100,14 @@ export default function Release() {
/>
</Stack>
</Stack>
{release && <EditableRelease release={release} isEdit={isEdit} onIsEditChange={setIsEdit} />}
{release && (
<EditableRelease
release={release}
currentUserRoles={currentUserRoles}
isEdit={isEdit}
onIsEditChange={setIsEdit}
/>
)}
<ReviewComments release={release} isEdit={isEdit} />
</Stack>
</>
Expand Down
39 changes: 25 additions & 14 deletions frontend/src/Form/EditableFormHeading.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { LoadingButton } from '@mui/lab'
import { Button, Stack } from '@mui/material'
import { Button, Stack, Tooltip } from '@mui/material'
import { ReactNode } from 'react'
import MessageAlert from 'src/MessageAlert'

Expand All @@ -8,6 +8,8 @@ type EditableFormHeadingProps = {
editButtonText: string
isEdit: boolean
isLoading: boolean
canUserEditOrDelete: boolean
actionButtonsTooltip: string
onEdit: () => void
onCancel: () => void
onSubmit: () => void
Expand All @@ -31,6 +33,8 @@ export default function EditableFormHeading({
deleteButtonText = 'Delete',
showDeleteButton = false,
isRegistryError = false,
canUserEditOrDelete = true,
actionButtonsTooltip = '',
}: EditableFormHeadingProps) {
return (
<Stack sx={{ pb: 2 }}>
Expand All @@ -42,22 +46,29 @@ export default function EditableFormHeading({
>
{heading}
{!isEdit && (
<Stack direction='row' spacing={1} justifyContent='flex-end' alignItems='center' sx={{ mb: { xs: 2 } }}>
<Button variant='outlined' onClick={onEdit} data-test='editFormButton' disabled={isRegistryError}>
{editButtonText}
</Button>
{showDeleteButton && (
<Tooltip title={actionButtonsTooltip}>
<Stack direction='row' spacing={1} justifyContent='flex-end' alignItems='center' sx={{ mb: { xs: 2 } }}>
<Button
variant='contained'
color='secondary'
onClick={onDelete}
data-test='deleteFormButton'
disabled={isRegistryError}
variant='outlined'
onClick={onEdit}
data-test='editFormButton'
disabled={isRegistryError || !canUserEditOrDelete}
>
{deleteButtonText}
{editButtonText}
</Button>
)}
</Stack>
{showDeleteButton && (
<Button
variant='contained'
color='secondary'
onClick={onDelete}
data-test='deleteFormButton'
disabled={isRegistryError || !canUserEditOrDelete}
>
{deleteButtonText}
</Button>
)}
</Stack>
</Tooltip>
)}
{isEdit && (
<Stack direction='row' spacing={1} justifyContent='flex-end' alignItems='center' sx={{ mb: { xs: 2 } }}>
Expand Down
Loading
Loading