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

Add initial RBAC support #1062

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Add initial RBAC support #1062

wants to merge 1 commit into from

Conversation

maggu
Copy link

@maggu maggu commented Apr 5, 2024

Add RBAC support for APT repositories. Partially fixes #860.

@maggu maggu force-pushed the main branch 3 times, most recently from bb25db7 to b76d7e6 Compare April 6, 2024 14:30
@hstct
Copy link
Contributor

hstct commented Apr 8, 2024

The Deb CI / test / test (docs) (pull_request) pipeline is running endless had to cancel it. I think you may need to rebase your branch onto the latest main branch.

@maggu
Copy link
Author

maggu commented Apr 8, 2024

@hstct Okay. Done now.

@quba42
Copy link
Collaborator

quba42 commented Apr 23, 2024

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

@maggu
Copy link
Author

maggu commented Apr 25, 2024

@quba42 Absolutely. I'll delegate that task to colleagues who have a clearer view of our use cases than I do. Thanks.

@quba42 quba42 added the .feature CHANGES/<issue_number>.feature label May 13, 2024
Copy link

@gerrod3 gerrod3 left a comment

Choose a reason for hiding this comment

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

This PR looks pretty good. Are you thinking of adding any tests?

pulp_deb/app/viewsets/content.py Show resolved Hide resolved
pulp_deb/app/viewsets/content.py Show resolved Hide resolved
pulp_deb/app/viewsets/publication.py Show resolved Hide resolved
pulp_deb/app/viewsets/repository.py Outdated Show resolved Hide resolved
@maggu
Copy link
Author

maggu commented Jul 3, 2024

This PR looks pretty good. Are you thinking of adding any tests?

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.

@maggu
Copy link
Author

maggu commented Jul 3, 2024

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.

@maggu
Copy link
Author

maggu commented Sep 12, 2024

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 view_debrepository instead of view_aptrepository in viewsets/publication.py by the way. It's corrected now.)

Copy link

@gerrod3 gerrod3 left a comment

Choose a reason for hiding this comment

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

One little line that doesn't need to be there (it does nothing if left in), but overall looks good with the tests! Sorry I am late to reviewing this, so if you want you can merge now and we can change it later.

"effect": "allow",
"condition": [
"has_model_or_domain_perms:deb.add_verbatimpublication",
"has_publication_param_model_or_domain_or_obj_perms:deb.view_aptpublication",
Copy link

Choose a reason for hiding this comment

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

Suggested change
"has_publication_param_model_or_domain_or_obj_perms:deb.view_aptpublication",

This check doesn't make sense for creating a publication.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
.feature CHANGES/<issue_number>.feature no-issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Domain and/or RBAC (multi-tenancy) support
4 participants