diff --git a/rbac/management/access/view.py b/rbac/management/access/view.py index f8e868907..6a41ac510 100644 --- a/rbac/management/access/view.py +++ b/rbac/management/access/view.py @@ -23,6 +23,7 @@ from management.role.serializer import AccessSerializer from management.utils import ( APPLICATION_KEY, + deduplicate_access_queryset, get_principal_from_request, validate_and_get_key, validate_limit_and_offset, @@ -121,7 +122,9 @@ def get(self, request): access_policy = cache.get_policy(principal.uuid, sub_key) if access_policy is None: queryset = self.get_queryset(ordering) - access_policy = self.serializer_class(queryset, many=True).data + access_policy = self.serializer_class( + deduplicate_access_queryset(queryset), many=True + ).data cache.save_policy(principal.uuid, sub_key, access_policy) page = self.paginate_queryset(access_policy) diff --git a/rbac/management/permission/model.py b/rbac/management/permission/model.py index 29dd20342..0e16b4757 100644 --- a/rbac/management/permission/model.py +++ b/rbac/management/permission/model.py @@ -31,6 +31,20 @@ class Permission(TenantAwareModel): description = models.TextField(default="") permissions = models.ManyToManyField("self", symmetrical=False, related_name="requiring_permissions") + def __eq__(self, other): + return self.permission == other.permission + + def __contains__(self, other): + # Note that the sense of 'contains' is opposite to the sense of 'in': + # A is in B if B contains A. + if self.permission == other.permission: + return True + if not (self.application == other.application or self.application == '*'): + return False + if not (self.resource_type == other.resource_type or self.resource_type == '*'): + return False + return (self.verb == other.verb or self.verb == '*' or (other.verb == 'read' and self.verb == 'write')) + def save(self, *args, **kwargs): """Populate the application, resource_type and verb field before saving.""" context = self.permission.split(":") diff --git a/rbac/management/utils.py b/rbac/management/utils.py index b1cd82d0c..5253c54ab 100644 --- a/rbac/management/utils.py +++ b/rbac/management/utils.py @@ -192,6 +192,45 @@ def filter_queryset_by_tenant(queryset, tenant): return queryset.filter(tenant=tenant) +def deduplicate_access_queryset(queryset): + """ + Deduplicate the access queryset. + + Takes a queryset on the Access model, and returns a list of the minimal + permissions described in the original queryset. These deduplications are + performed: + + 1: two exactly matching permissions are combined - e.g. 'app:*:read' and + 'app:*:read' - including joining together their resourceDefinitions. + + Others may be added later, such as combining permissions that wholly + include others. For now let's stick with what's simple. + """ + # Since the ordering does not matter, we record these in a dict by the + # permission, for ease of access. + deduplicated_access = dict() + + def matching_perms(this_access, sought_access): + """Find all access objects that match the app and resource given.""" + tap = this_access.permission + sap = sought_access.permission + return tap.app == sap.app and tap.resource_type == sap.resource_type + + for access in queryset: + if not access.permission: + # Cannot emit an access permission if it doesn't have one + continue + if access.permission.permission in deduplicated_access: + # Combine this access object's resource definitions into that + # already stored. Note that at this stage we DO NOT attempt to + # merge attributeFilter structures. + deduplicated_access[access.permission.permission].resourceDefinitions.extend( + access.resourceDefinitions + ) + # Return access object list in order by permission for testing consistency + return (deduplicated_access[perm] for perm in sorted(deduplicated_access.keys())) + + def validate_and_get_key(params, query_key, valid_values, default_value=None, required=True): """Validate the key.""" value = params.get(query_key, default_value) diff --git a/tests/management/test_utils.py b/tests/management/test_utils.py index ffb8a31b2..30e64ea9a 100644 --- a/tests/management/test_utils.py +++ b/tests/management/test_utils.py @@ -23,6 +23,7 @@ policies_for_principal, roles_for_principal, account_id_for_tenant, + deduplicate_access_queryset, ) from tests.identity_request import IdentityRequest @@ -130,3 +131,29 @@ def test_account_number_from_tenant_name(self): """Test that we get the expected account number from a tenant name.""" tenant = Tenant.objects.create(tenant_name="acct1234") self.assertEqual(account_id_for_tenant(tenant), "1234") + + def test_deduplicate_access_queryset(self): + """ + Test a variety of scenarios where we need to deduplicate the data + """ + ih_hosts_read = Permission.objects.create(permission='inventory:hosts:read', tenant=self.tenant) + ihhr_access1 = Access.objects.create(permission=ih_hosts_read, role=self.roleA, tenant=self.tenant) + groups_rdef2 = ResourceDefinition.objects.create(access=ihhr_access1, attributeFilter={ + 'key': 'group.id', 'operation': 'in', 'value': [ + '78e3dc30-cec3-4b49-be2d-37482c74a9a1', + '78e3dc30-cec3-4b49-be2d-37482c74a9b1' + ] + }) + ihhr_access2 = Access.objects.create(permission=ih_hosts_read, role=self.roleA, tenant=self.tenant) + groups_rdef2 = ResourceDefinition.objects.create(access=ihhr_access2, attributeFilter={ + 'key': 'group.id', 'operation': 'in', 'value': [ + '78e3dc30-cec3-4b49-be2d-37482c74a9a2', + '78e3dc30-cec3-4b49-be2d-37482c74a9b2' + ] + }) + + # First basic test: what do these get merged into? + queryset = Access.objects.filter(tenant=self.tenant) + deduped = deduplicate_access_queryset(queryset) + print("deduped:", deduped) + self.assertEqual(len(deduped), 1)