CDD-3173: add the base code for the DB permission filtering of privat… by dandammann · Pull Request #3201 · UKHSA-Internal/data-dashboard-api

dandammann · 2026-05-20T08:58:52Z

Description

This PR includes the following:

CDD-3174: Similar to the changes that were implemented for the getPages endpoint as part of https://ukhsa.atlassian.net/browse/CDD-3171, this ticket concerns endpoints that are used for displaying data / generating charts should be updated to make use of the authorisation header. If the user provides an authorization header then the data returned from the endpoint should be customised depending on the permissions that the user has been granted.

CDD-3173: The database queries will need to be updated for non-public users so that they only return non-public data and they only return data that the user is permitted to view.

When a request is made by an authenticated user, the query to the DB should be made for only the data that the user is able to see. This will be driven from the user permission hierarchy

When a request is made by an authenticated user the returned data should only be for non-public data. it should not contain public data.

Type of change

Please select the options that are relevant.

[ ] Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
[ ] Breaking change (fix or feature that would cause existing functionality to not work as expected)
[ ] Tech debt item (this is focused solely on addressing any relevant technical debt)

Checklist:

I have commented my code, particularly in hard-to-understand areas
[ ] I have made corresponding changes to the documentation
I have added tests at the right levels to prove my change is effective
[ ] I have added screenshots or screen grabs where appropriate
I have added docstrings in the correct style (google)

…e APIs (using /api/charts/v3 as the first example)

…nctions/classes are

…an' into CDD-3173-BE-Update-DB-queries-to-ensure-that-user-permissions-are-used-when-requesting-data

sonarqubecloud · 2026-05-20T12:27:23Z

Quality Gate passed

Issues
3 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

kathryn-dale · 2026-05-20T16:43:43Z

            token_validator = self.get_token_validator(request)
            jwt_payload = token_validator.validate(jwt_token)
-        except TokenError:
+        except TokenError as error:


Arguably unneccessary? It's already throwing an error and I'm worried there's a risk that some permission set info or user info could be spat out as part of the error log

@dandammann Please can we avoid including work that is not related to the ticket in the same PR.
It makes it harder to review as reviewers have to figure out which code changes are related to the ticket, and which are achieving some other unknown goal.
If you find tech debt that need to be improved In existing code (and it's a quick fix), then these should be added to a seperate PR with a description that explains it and then it can be reviewed on it own merits.
Also in this instance, all these changes within the JWT code are going to cause a big headache of conflicts once the PR for CDD-3147 gets merged in too.

kathryn-dale · 2026-05-20T16:45:05Z

+                    geography=geography,
+                )
+        else:
+            # THESE LEGACY RBAC PERMISSIONS ARE NOT IN USE AND TO BE REMOVED IN A FUTURE RELEASE


If they aren't in use in this file can we remove them as part of this work? I don't see any reason to keep them in, as you are essentially replacing what they are trying to do with the new solution :)

I mentioned that RBAC permissions need removing (at some point), but you said that's out-of-scope, because there is a ticket for that in the backlog.

For the areas of the code you are already touching, it's fine to replace the RBAC parts with the new permission_sets bits, rather than adding if/else statements for code we know is going to disappear soon.
The endpoints are going to be thoroughly tested by updated unit tests and by the QA team anyway, so there's no issues with doing it that way, it's just that we don't need to make more work by extending the scope of the ticket to cover removing all the other areas of the codebase that have RBAC bits, along with all the additional updated tests and additional QA time that would go along with that work. That part can be done a future ticket.
We want to focus on what's required for our MVP deliverable, and keeping that work as simple as possible.

kathryn-dale · 2026-05-20T16:45:42Z

+                    geography=geography,
+                )
+        else:
+            # THESE LEGACY RBAC PERMISSIONS ARE NOT IN USE AND TO BE REMOVED IN A FUTURE RELEASE


Same as above

Same reply from me too.

kathryn-dale · 2026-05-20T16:46:16Z

+
+    @return {dict} example:
+        {
+            "permission_set_hierarchy": [


What's the value in this over using the raw permissions?

@dandammann I''m not sure what this is for. I was under the impression that the permission set hierarchy was already returning the permissions sets into the JWT in the most usable format for the endpoints to consume when filtering the data.
If the permission sets are not in the right format when we come to use them for filtering for pages, DB data etc, then we should be updating the format of the permission sets before they go into the JWT, so it happens just once, rather than on 100s of requests at the point it is being consumed (although there is also a tradeoff to consider here too, of avoiding making the JWT too large).

kathryn-dale · 2026-05-20T16:47:25Z

+    @return {bool}
+    """
+
+    topic_record = None


Can we not extend the function already created in CDD-3171? I find this hard to read (could just be me though)

I agree, we want to make sure we're not duplicating the core permissions checking functions

Hi @kathryn-dale and @mattjreynolds . Please look at my comments and examples in this current function and filter_permissions() and let me know, if you think your check_permissions() can achieve that.

I myself think it is too simplistic for that, because it only checks permissions for EITHER theme, sub_theme or topic, but with the charts we need to check permissions cross-cutting (taking them all into consideration).

if you do think check_permissions() is adequate, please try it out ...

There is also that difference that with GetPages the input arguments are theme/sub_theme/topic IDS, whereas with charts they are theme/sub_theme/topic NAMES.

mattjreynolds · 2026-05-21T08:50:53Z

            token_validator = self.get_token_validator(request)
            jwt_payload = token_validator.validate(jwt_token)
-        except TokenError:
+        except TokenError as error:


@dandammann Please can we avoid including work that is not related to the ticket in the same PR.
It makes it harder to review as reviewers have to figure out which code changes are related to the ticket, and which are achieving some other unknown goal.
If you find tech debt that need to be improved In existing code (and it's a quick fix), then these should be added to a seperate PR with a description that explains it and then it can be reviewed on it own merits.
Also in this instance, all these changes within the JWT code are going to cause a big headache of conflicts once the PR for CDD-3147 gets merged in too.

mattjreynolds · 2026-05-22T09:42:02Z

+
+    @return {dict} example:
+        {
+            "permission_set_hierarchy": [


@dandammann I''m not sure what this is for. I was under the impression that the permission set hierarchy was already returning the permissions sets into the JWT in the most usable format for the endpoints to consume when filtering the data.
If the permission sets are not in the right format when we come to use them for filtering for pages, DB data etc, then we should be updating the format of the permission sets before they go into the JWT, so it happens just once, rather than on 100s of requests at the point it is being consumed (although there is also a tradeoff to consider here too, of avoiding making the JWT too large).

mattjreynolds · 2026-05-22T11:39:49Z



 class EncodedChartsView(APIView):
+    authentication_classes = [SessionAuthentication, JSONWebTokenAuthentication]


This should not be being set.
Authenticaion is already being handled for all views by the JWT token code, and the permission sets are being made available in the request object (request.user.permission_sets)

mattjreynolds · 2026-05-22T13:16:47Z

+            has_global_access = jwt_permissions.get("has_global_access", False)
+
+            if has_global_access:
+                has_access_to_non_public_data = True


Could simplify this has_global_access check to just be part of the return value from check_any_permissions_allow_access
This is how it's being done in 3171, are we not able to just reuse those functions for these endpoints too? They should be doing the same job and we don't want to have 2 different function that both check permissions

mattjreynolds · 2026-05-22T13:20:19Z

-            **plot_params, rbac_permissions=self.chart_request_params.rbac_permissions
+            **plot_params,
+            rbac_permissions=self.chart_request_params.rbac_permissions,  # legacy permissions to be removed
+            jwt_permissions=extract_jwt_permissions(request=self.chart_request_params.request),  # new permissions


as with previous comment, you should be able to just use something like:
jwt_permissions=request.user.permission_sets if request.auth else None,

mattjreynolds · 2026-05-22T13:21:27Z

+    @return {bool}
+    """
+
+    topic_record = None


I agree, we want to make sure we're not duplicating the core permissions checking functions

dandammann added 3 commits May 19, 2026 09:47

CDD-3173: add the base code for the DB permission filtering of privat…

034223b

…e APIs (using /api/charts/v3 as the first example)

CDD-3173: move permissions.py to folder where the other permission fu…

4d6a4ac

…nctions/classes are

CDD-3173: make DB permission filtering generic for all 6 fields

b927176

dandammann requested a review from a team as a code owner May 20, 2026 08:58

dandammann added 3 commits May 20, 2026 10:16

Merge branch 'task/CDD-3171-update-getpages-endpoint-for-non-public-i…

f6fef6f

…an' into CDD-3173-BE-Update-DB-queries-to-ensure-that-user-permissions-are-used-when-requesting-data

CDD-3173: remove TODO comments

28ec2ac

CDD-3173: rename and remove redundant things

4184430

kathryn-dale reviewed May 20, 2026

View reviewed changes

Comment thread common/auth/cognito_jwt/user_manager.py

kathryn-dale reviewed May 20, 2026

View reviewed changes

mattjreynolds force-pushed the task/CDD-3171-update-getpages-endpoint-for-non-public-ian branch from 2303ee5 to 5219d16 Compare May 22, 2026 07:06

mattjreynolds reviewed May 22, 2026

View reviewed changes



		class EncodedChartsView(APIView):
		authentication_classes = [SessionAuthentication, JSONWebTokenAuthentication]

Conversation

dandammann commented May 20, 2026

Description

Type of change

Checklist:

Uh oh!

sonarqubecloud Bot commented May 20, 2026

Quality Gate passed

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants