Add initial RBAC support #1062
Add initial RBAC support #1062
Conversation
maggu
force-pushed
the
main
branch
3 times, most recently
from
bb25db7 to
b76d7e6
Compare
|
The |
|
@hstct Okay. Done now. |
|
@maggu First of all thanks for this work. Since this is an entirely new feature for pulp_deb and I don't have any experience how RBAC works for other Pulp plugins, I am somewhat unsure how to go about reviewing this. One thing that would really help us, is any amount of write up of use cases how you want to use RBAC in pulp_deb that you can do. Perhaps a post in https://discourse.pulpproject.org/c/development/8 to accompany this PR would be a good place to start. Alternatively a post on the accompanying issue might also work: #860. I am trying to understand what the goal and scope for "initial RBAC support" in pulp_deb should be. Does this request make any sense to you? |
|
@quba42 Absolutely. I'll delegate that task to colleagues who have a clearer view of our use cases than I do. Thanks. |
Thank you for review and valuable feedback! Sorry it's taken me quite some time to reply. For now my purpose has been to resolve our most urgent business needs, but tests should obviously be added and hopefully we can help out with that as well. |
|
I'm going on vacation now, so won't work on this for some time. I'm planning on making it a priority once I get back though. In the meantime, please feel free to make edits if you want to. |
|
I added a few basic functional tests as well now. Comments? They cover the cases mentioned in #860 at least, and some more. I suppose they would benefit from being further extended at some point, but perhaps it is good enough for this PR? (They made me discover that I had mistakenly written |
pulp_deb/app/viewsets/publication.py
Outdated
| "effect": "allow", | ||
| "condition": [ | ||
| "has_model_or_domain_perms:deb.add_verbatimpublication", | ||
| "has_publication_param_model_or_domain_or_obj_perms:deb.view_aptpublication", |
There was a problem hiding this comment.
| "has_publication_param_model_or_domain_or_obj_perms:deb.view_aptpublication", |
This check doesn't make sense for creating a publication.
|
Hi, sorry we didn't have time to check on this these past weeks. We introduced a change in the repository viewset that requires an action for this PR to solve a merge conflict. Apologies for the inconvenience. Very sure it's because of this line: 630c38b#diff-6683b93be00ad1cf16924a1207da4bd15f936b31299cb82ba9dad42eb89e46f7R56 Can you rebase your branch again? |
[noissue]
No problem at all. It was quite a simple change. Rebased now. Thanks for the update. |
There was a problem hiding this comment.
Ok, I finally managed to take a look at this. The changes look good to me. Glad that there are tests included. I also did some preliminary manual tests which seem to work, though I did not test every specific setting here.
To me the basic functionality of this feature seems to work and that is good enough for a merge. We will likely add some more tests for RBAC down the line at some point. Though none of this is a blocker for this PR.
LGTM. Thanks for the contribution! I know it's been a long requested feature and I'm glad we can finally support it!
|
Awesome! Very pleased to have this merged. Thanks for feedback and assistance! |
Add RBAC support for APT repositories. Partially fixes #860.