From 8bc9e910f533ae8f1e0460b26910a7db559d4160 Mon Sep 17 00:00:00 2001 From: Keith Walsh Date: Thu, 9 Dec 2021 08:17:21 -0500 Subject: [PATCH 1/2] Use queryset in permission filter for `allowed_only` When we do not use the `queryset` passed into the filter, this causes an edge-case where chaining filters like `/permissions/?exclude_roles=123,456&allowed_only=true` will not behave as expected, because `exclude_roles` will filter the set, but then we query the entire collection (not the filtered queryset) on `allowed_only`, negating the previous filter(s). --- rbac/management/permission/view.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rbac/management/permission/view.py b/rbac/management/permission/view.py index f243fe9cf..0ef59d0bd 100644 --- a/rbac/management/permission/view.py +++ b/rbac/management/permission/view.py @@ -60,7 +60,7 @@ def allowed_only_filter(self, queryset, field, value): """Filter to return only permissions from roles in the ROLE_CREATE_ALLOW_LIST.""" query_field = validate_and_get_key(self.request.query_params, field, VALID_BOOLEAN_PARAM_VALS, "false") if query_field == "true": - queryset = Permission.objects.filter(application__in=settings.ROLE_CREATE_ALLOW_LIST) + queryset = queryset.filter(application__in=settings.ROLE_CREATE_ALLOW_LIST) return queryset application = filters.CharFilter(field_name="application", method="multiple_values_in") From 99e988a90d0f661ba613bb1b7b86cfae0d399f10 Mon Sep 17 00:00:00 2001 From: Keith Walsh Date: Thu, 9 Dec 2021 08:38:39 -0500 Subject: [PATCH 2/2] Test to exploit issue chaining permission filters --- tests/management/permission/test_view.py | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/tests/management/permission/test_view.py b/tests/management/permission/test_view.py index 740e75f79..726a8acca 100644 --- a/tests/management/permission/test_view.py +++ b/tests/management/permission/test_view.py @@ -421,6 +421,12 @@ def test_allowed_only_filters_any_roles_not_in_allow_list_out_when_true(self): self.assertEqual(len(response.data.get("data")), 1) self.assertCountEqual(expected, response_permissions) + def test_allowed_only_filters_any_roles_not_in_allow_list_out_when_true_in_chain(self): + """Test that we filter out any permissions not in the allow list when allowed_only=true, chained with other filters.""" + response = CLIENT.get(f"{LIST_URL}?allowed_only=true&exclude_globals=true", **self.headers) + self.assertEqual(response.status_code, status.HTTP_200_OK) + self.assertEqual(len(response.data.get("data")), 0) + def test_allowed_only_filters_no_permissions_out_when_false(self): """Test that we do not filter out any permissions not in the allow list when allowed_only=false.""" with tenant_context(self.tenant):