-
Notifications
You must be signed in to change notification settings - Fork 11
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
Object-level permissions for hosting providers and data centers #481
Conversation
32deb62
to
671bdbf
Compare
671bdbf
to
b71ff07
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi Oliwia! This looks good!
I see no issues with the logic - my feedback is mainly about style and consistency.
There are are a couple of places where I couldn't understand why we might use one class created in this PR over a built in one (like with Permissions), but otherwise I'm happy.
We discussed the precautions we might take before merging this into production, and I think I'll need to speak to to you in off github and in Zulip about how we sequence deployment and running the migration.
Thanks again for this picking up this massive task.
@@ -46,21 +46,18 @@ class TestHostingProvider: | |||
"compensatie", | |||
), | |||
) | |||
@pytest.mark.django_db |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see you using this decorator over paassing db
into the function, and we have a mix of use throughout our test suite.
I'm open to us settling on either, but I'm not sure about the reasoning either way.
Is there a reason for your preference here? I can see some value in splitting it out as a decorator, as it lets us decorate an entire test class, that needs to hit the db, but all the new tests seem to use function name as a namespacing strategy, so it wasn't so clear to me why we'd use one over the other.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's only a matter of personal preference - I prefer to have a decorator over adding a function argument. But frankly I didn't think much of it, and I don't think it's an impactful decision for the codebase. If you have a strong preference let me know, I can change this to a function parameter
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See below - we can punt this til later. No decisions or changes needed for right now.
@@ -81,9 +77,12 @@ def test_get_queryset_staff( | |||
assert gip in qs | |||
|
|||
def test_get_queryset_non_staff( | |||
self, db, rf, hosting_provider_with_sample_user, green_ip, | |||
self, | |||
db, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we're going to use the django_db
mark in pytest passing in db
as a fixture, would you mind making the change here too so it's consistent in this PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like we use both version across our test cases. In each new test I went with the variant that is already used in other tests in the same module. Do you want to take the decision now about this and update all occurrences? I don't see it as important, but let me know if you think otherwise
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nah, not's not a priority.
Let's keep this as is and create an issue to review this where make a recorded decision and then apply it to all future PRs instead. This PR is already ginormous.
b71ff07
to
78da57d
Compare
…add migration to assign permissions to existing objects
…ulate values from existing records
…splay related objects based on object-level permissions
…cts with explicit user permission
…out the default Django perms
…licit permissions rather than group membership
…igned to the user
…at the time of the migration, and current models to access the objects to assign permissions to
24fa649
to
364b6fd
Compare
This is a draft pull request - documentation is still missing!
Closes #335.
This pull request introduces object-level permission system, which can be used to assign arbitrary number of users a permission to manage (through admin or API) an arbitrary number of hosting providers and data centers.
Scope of changes:
django-guardian
to provide a back-end for object-level permissions,manage_provider
forHostingprovider
model andmanage_datacenter
forDatacenter
model. The permissions are defined as singletons for re-using in the app (new modulepermissions.py
).GuardedModelAdminMixin
provided bydjango-guardian
so that only admins can access permission management screens, and only custom model permissions are displayed (excluding default Django permissions),Hostingprovider
s, and Users andDatacenter
s are expressed as object-level permissions,Hostingprovider.created_by
andDatacenter.created_by
, data populated for existing records based on the User.hostingprovider reference.User
toHostingprovider
,Hostingprovider
andDatacenter
have now helper properties:users
andusers_explicit_perms
that each returns a queryset of related users based on defined permissions,User
model has now helper properties:hosting_providers
anddata_centers
that each returns a queryset of related objects based on defined permissions,Hostingprovider
,Datacenter
andUser
models now include a list of related objects based on defined permissions,To do:
ProviderSharedSecretView
API, so that the provider is explicitly identified (this will be handled in a separate task),