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

Disable UI starting dataset creating jobs for unprivileged users #7753

Merged

Conversation

MichaelBuessemeyer
Copy link
Contributor

@MichaelBuessemeyer MichaelBuessemeyer commented Apr 15, 2024

This PR disables the UI elements allowing the creation of a new dataset as an output of a job for normal unprivileged users.

URL of deployed dev instance (used for testing):

Steps to test:

  • On the dev instance log in as the [email protected] user and try to access UI allowing you to start a job creating a dataset:
  • <host-ulr-part>/datasets/upload should now show a 403 error page
    • For this, an improved icon might be preferable that follows wks drawing style
  • In tracing view:
    • The "materialize merger mode annotation" button should not be rendered
    • The AI Analysis Button should not be rendered
    • The option "Merge this volume annotation with its fallback layer" of a segmentation layer with fallback data (e.g. l4_sample) should not be rendered

Open question:

  • Is there a UI element I missed while scanning the codebase?

TODOs:

  • Confirm whether keeping the 403 page is ok
    • Get a better image for the forbidden page

Issues:


(Please delete unneeded items, merge only when none are left open)

@MichaelBuessemeyer MichaelBuessemeyer changed the title Disable UI starting dataset creating jobs for non-admin/datasetmanagers Disable UI starting dataset creating jobs for unprivileged users Apr 16, 2024
@MichaelBuessemeyer MichaelBuessemeyer marked this pull request as ready for review April 16, 2024 09:23
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I formatted this page with prettier, as biome currently does not support formatting css.

See: biomejs/biome#1285

Copy link
Member

@philippotto philippotto left a comment

Choose a reason for hiding this comment

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

Beautiful :) See my one comment, though, as this should be added to lots of other routes, too, as far as I see.

@@ -428,6 +428,7 @@ class ReactRouter extends React.Component<Props> {
<SecuredRouteWithErrorBoundary
isAuthenticated={isAuthenticated}
path="/datasets/upload"
requiresAdminOrManagerRole
Copy link
Member

Choose a reason for hiding this comment

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

I think, this prop can be added to a few other components:

path="/users/:userId/details"
path="/users"
path="/teams"
path="/timetracking"
path="/reports/projectProgress"
path="/reports/availableTasks"
path="/tasks"
path="/tasks/create"
path="/tasks/:taskId/edit"
path="/tasks/:taskId"
path="/projects"
path="/projects/create"
path="/projects/:projectId/tasks"
path="/projects/:projectId/edit"
path="/datasets/:organizationName/:datasetName/import"
path="/datasets/:organizationName/:datasetName/edit"
path="/taskTypes"
path="/taskTypes/create"
path="/taskTypes/:taskTypeId/edit"
path="/taskTypes/:taskTypeId/tasks"
path="/taskTypes/:taskTypeId/projects"
path="/scripts/create"
path="/scripts/:scriptId/edit"
path="/scripts"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for listing all other components 👍

Initially, I though that this should be added in a separate PR but doing it already here is also fine to me and might also save some time.

I'll test each route as admin and non-admin later :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll test each route as admin and non-admin later :)

Done, worked out as expected 👍. All pages/view listed above are available to [email protected] but not to [email protected]

Copy link
Member

@philippotto philippotto left a comment

Choose a reason for hiding this comment

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

Excellente 🥖

@MichaelBuessemeyer MichaelBuessemeyer merged commit 7af1a4c into master May 2, 2024
2 checks passed
@MichaelBuessemeyer MichaelBuessemeyer deleted the only-allow-privilegde-user-dataset-creation-jobs branch May 2, 2024 12:01
dieknolle3333 pushed a commit that referenced this pull request May 8, 2024
* disable ui starting dataset creating jobs for non-admin/ datasetmanager users

* hide materialize merger mode button instead of disabling it

* check for is team manager as well (and not only admin or dataset manager roles)

* re-allow merge with fallback layer for team managers

* add changelog entry

* add improved image to permission enforcer view / forbidden view

* add manager/admin permissions required view to privileged views
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Adapt UI to show dataset-creating Jobs only to Privileged Users
2 participants