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

Jobs migration: define authorization model #620

Closed
nitrosx opened this issue Jul 24, 2023 · 8 comments
Closed

Jobs migration: define authorization model #620

nitrosx opened this issue Jul 24, 2023 · 8 comments
Assignees
Labels
Release Jobs Jobs migration
Milestone

Comments

@nitrosx
Copy link
Member

nitrosx commented Jul 24, 2023

Summary

Define who can do which action on jobs endpoint and have a flexible configuration.

Output should be a permission matrix and environmental variables name to configure who can view and edit jobs

@nitrosx nitrosx added this to the Release 4.4 milestone Jul 24, 2023
@nitrosx nitrosx added the Release Jobs Jobs migration label Jul 24, 2023
@nitrosx nitrosx changed the title Define jobs authorization model Jobs migration: define authorization model Jul 24, 2023
@nitrosx nitrosx moved this to Todo in Core Release Jobs Aug 11, 2023
@ddijken ddijken moved this from Todo to Done in Core Release Jobs Oct 25, 2023
@nitrosx nitrosx moved this from Done to In Progress in Core Release Jobs Oct 25, 2023
@nitrosx
Copy link
Member Author

nitrosx commented Oct 26, 2023

Image

@nitrosx
Copy link
Member Author

nitrosx commented Oct 27, 2023

@nitrosx
Copy link
Member Author

nitrosx commented Oct 31, 2023

Implemented in PR issue-712 as I was working on the dataset authorization.

@nitrosx
Copy link
Member Author

nitrosx commented Nov 22, 2023

Jobs authorization is now in master as PR issue-712 has been approved and merged.
Once we pull master in release-jobs, we should be good.

@nitrosx nitrosx moved this from In Progress to Ready in Core Release Jobs Nov 22, 2023
@sbliven
Copy link
Member

sbliven commented Mar 19, 2024

I think we now have a good sense of the instance authorization. The documentation still needs to be updated and @despadam can verify that the logic matches the documentation.

@sbliven sbliven moved this from Ready to In Progress in Core Release Jobs Mar 19, 2024
@sbliven
Copy link
Member

sbliven commented Mar 20, 2024

I started reading about CASL trying to fix some authorization bugs and now I have questions.

  1. Why does AuthOp.enum have a different endpoint authorization for each subject (dataset_create, job_create, sample_create, etc)? Shouldn't this just be can('create', DatasetClass), can('create', JobClass), etc)?

  2. The JobsCreateAny and similar actions check ownerGroup: { $in: user.currentGroups }. Is this just copy-pasted from dataset? Jobs don't have an ownerGroup so this doesn't make sense.

  3. Should instanceAuthorization be implemented as a PolicyGuard instead of within the call? Or at least using casl abilities rather than throwing the HTTPException directly?

PS I now understand why AuthOp.enum was originally called Action.enum. It's the correct CASL term.

@despadam despadam assigned sbliven and nitrosx and unassigned nitrosx Apr 4, 2024
@nitrosx
Copy link
Member Author

nitrosx commented Apr 5, 2024

@sbliven I will try to answer your questions:

  1. When I started review the authorization, I realized that we were not able to reach the level of granularity that we were looking for by using the generic actions, like create. Back than I decided to have end-point specific actions. At the moment I am still convinced that it is the right choice, although we should review the CASL definition and see if we can simplify it. I was saving the review after we are done with the current releases.
  2. I will check but it might very well be. I'm planning to work on this next, so stay tuned
  3. Long story short: you need to do extra work to be able to access the data of the specific item that you are working on in the PolicyGuard, so I decided to implement the InstanceAuthorization within the function where the data is already loaded.

Let me know if you have more questions.

@despadam
Copy link
Member

despadam commented Oct 3, 2024

@despadam despadam closed this as completed Oct 3, 2024
@github-project-automation github-project-automation bot moved this from In Progress to Ready in Core Release Jobs Oct 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Release Jobs Jobs migration
Projects
Status: Done
Development

No branches or pull requests

6 participants