diff --git a/apps/accounts/migrations/0056_populate_existing_permissions.py b/apps/accounts/migrations/0056_populate_existing_permissions.py index cccf73a1..069933f6 100644 --- a/apps/accounts/migrations/0056_populate_existing_permissions.py +++ b/apps/accounts/migrations/0056_populate_existing_permissions.py @@ -67,7 +67,10 @@ def _manage_permission(apps, schema_editor, manage_func): Datacenter_historical_model = apps.get_model("accounts", "Datacenter") Hostingprovider_historical_model = apps.get_model("accounts", "Hostingprovider") - # retrieve data based on queryset using historical model to access "created_by" field + # retrieve (object, user) ID pairs eligible for assigning permissions + # based on queryset of historical model - so that we can access "created_by" field + # that is present at the time of this migration. + # See more details: https://docs.djangoproject.com/en/4.2/topics/migrations/#historical-models eligible_pairs_dc = [ (dc.id, dc.created_by.id) for dc in Datacenter_historical_model.objects.using(db_alias).filter( @@ -80,13 +83,21 @@ def _manage_permission(apps, schema_editor, manage_func): created_by__isnull=False ) ] - # Get current models to retrieve the objects for assigning permissions. + + # GOTHA: django-guardian shortcut methods for assigning/removing perms + # *do not* support historical models! See details: https://github.com/django-guardian/django-guardian/issues/281#issuecomment-156264129 + # + # As a workaround, we get *current* models to retrieve the objects for assigning permissions + # based on the ID pairs we retrieved in the previous step. + # Notice that `django_apps.get_model` is used instead of `apps.get_model`. + # # This is dangerous! Future versions of django-guardian might change the schema # and this migration might fail when ran from scratch. If this happens: # - pin down the version of django-guardian to the latest working version # so that migrations work for tests # - for production / staging it's a non-issue since we always keep snapshots around # and don't run older migrations + Datacenter = django_apps.get_model("accounts", "Datacenter") Hostingprovider = django_apps.get_model("accounts", "Hostingprovider") User = get_user_model() diff --git a/apps/accounts/models/user.py b/apps/accounts/models/user.py index 15663d16..6c988de8 100644 --- a/apps/accounts/models/user.py +++ b/apps/accounts/models/user.py @@ -98,6 +98,10 @@ def hosting_providers_explicit_perms(self) -> models.QuerySet[Hostingprovider]: - superuser status This method is useful to fetch hosting providers for users that belong to the admin group. + Because the admin group has global permissions to manage all objects, the admin page + or provider portal page (where we show all objects that the users can manage) + for the admin group members would have to fetch all available objects. To avoid this, + we settled on displaying objects with explicit user-object permissions only. """ return get_objects_for_user( self, @@ -127,6 +131,10 @@ def data_centers_explicit_perms(self) -> models.QuerySet[Datacenter]: - superuser status This method is useful to fetch data centers for users that belong to the admin group. + Because the admin group has global permissions to manage all objects, the admin page + or provider portal page (where we show all objects that the users can manage) + for the admin group members would have to fetch all available objects. To avoid this, + we settled on displaying objects with explicit user-object permissions only. """ return get_objects_for_user( self, diff --git a/apps/accounts/permissions.py b/apps/accounts/permissions.py index cafb054c..8d230d44 100644 --- a/apps/accounts/permissions.py +++ b/apps/accounts/permissions.py @@ -1,13 +1,27 @@ from dataclasses import dataclass -""" -Definitions for object-level permissions for the `accounts` app. -""" - - @dataclass class Permission: + """ + This data class provides attributes and methods + for the expected representations of object-level permissions in the "accounts" app. + + It is entirely independent of django.contrib.auth.models.Permission. + + The purpose of this data class is to be used in various places in the code + (migrations, views, tests) that expect a a permission in various representations. + As an example: + - guardian.shortcuts methods assign_perm and remove_perm expect the codename of the permission as an argument + if the content type of the object can be determined, or full permission name otherwise. + We can pass `permissions.manage_provider.codename` or `permissions.manage_provider.full_name` + instead of passing strings: "manage_provider" or "accounts.manage_provider" + + The actual object-level permission (based on django.contrib.auth.models.Permission) + is created by django-guardian when it picks up the definition of the `Meta` class + of a given model. + """ + codename: str full_name: str description: str @@ -19,9 +33,13 @@ def __str__(self): return self.full_name +""" +Definitions for object-level permissions for the `accounts` app +""" + manage_provider = Permission( "manage_provider", "accounts.manage_provider", "Manage provider" ) manage_datacenter = Permission( "manage_datacenter", "accounts.manage_datacenter", "Manage datacenter" -) \ No newline at end of file +) diff --git a/apps/greencheck/tests/test_api_asn.py b/apps/greencheck/tests/test_api_asn.py index 019fbef4..e0ee7836 100644 --- a/apps/greencheck/tests/test_api_asn.py +++ b/apps/greencheck/tests/test_api_asn.py @@ -238,7 +238,7 @@ def test_can_only_destroy_asns_for_own_hosting_provider( rf = APIRequestFactory() url_path = reverse("asn-detail", kwargs={"pk": green_asn.id}) - # hosting_provider_with_sample_user.users.first() + request = rf.delete(url_path) request.user = another_host_user