Add RBAC support #679
Add RBAC support #679
Conversation
|
I read through the docs and the claimed feature support and I believe this will work well for our needs. +1 to merge |
pulp_python/app/__init__.py
Outdated
| super().import_viewsets() | ||
| from pulp_python.app.pypi.views import PyPIView, SimpleView, UploadView, MetadataView | ||
|
|
||
| self.named_viewsets[None].extend([PyPIView, SimpleView, UploadView, MetadataView]) |
There was a problem hiding this comment.
This looks very hacky. Can you explain how this works? None as a dict key really tingles.
There was a problem hiding this comment.
It is hacky. The roles & access policies migrate hook use this dict to figure out which viewsets it needs to inspect. Both hooks use values to get the viewsets, so since the PyPI views don't have a backing model we can use None as the key. The big thing with putting non-NamedModelViewSets inside this dict is that you need to have your viewset mock one by implementing ~5 methods (the ones on the PyPIMixin) in order for other parts of Pulp to not break.
I am not that pleased with this solution, it's pretty tenuous, but we currently don't have any way to register other views with our RBAC system.
There was a problem hiding this comment.
First, we should have a better mechanism.
Can't you just create the record in the db from a migration hook here?
Would we need a special PermissionClass to find the access policy without assuming it's a NamedModelVS?
There was a problem hiding this comment.
At the very least, we need a plan how it should look better. And a comment here so we know what this is actually accomplishing and when to rework it.
There was a problem hiding this comment.
Ok, I agree the hack is too much. I removed it and copied/pasted the access policy hook from pulpcore and added a comment to revisit once we have a better mechanism in pulpcore: pulp/pulpcore#5500
d8de886 to
11d51f2
Compare
| @@ -189,13 +212,28 @@ def create_group_upload_task(self, cur_session, repository, artifact, filename, | |||
| class SimpleView(PackageUploadMixin, ViewSet): | |||
| """View for the PyPI simple API.""" | |||
|
|
|||
| permission_classes = [IsAuthenticatedOrReadOnly] | |||
There was a problem hiding this comment.
We don't need to specify the APfromDB because that's the globally configured default, right?
fixes: pulp#399
| @@ -110,6 +121,11 @@ def initial(self, request, *args, **kwargs): | |||
| else: | |||
| self.base_content_url = BASE_CONTENT_URL | |||
|
|
|||
| @classmethod | |||
| def urlpattern(cls): | |||
| """Mocking NamedModelViewSet behavior to get PyPI APIs to support RBAC access polices.""" | |||
Needs domains PR to be merged first.