-
Notifications
You must be signed in to change notification settings - Fork 4
CDD-3173: add the base code for the DB permission filtering of privat… #3201
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
base: task/CDD-3171-update-getpages-endpoint-for-non-public-ian
Are you sure you want to change the base?
Changes from all commits
034223b
4d6a4ac
b927176
f6fef6f
28ec2ac
4184430
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,11 +4,13 @@ | |
| from django.http import FileResponse | ||
| from drf_spectacular.utils import OpenApiExample, extend_schema | ||
| from rest_framework import permissions | ||
| from rest_framework.authentication import SessionAuthentication | ||
| from rest_framework.response import Response | ||
| from rest_framework.views import APIView | ||
|
|
||
| import config | ||
| from caching.private_api.decorators import cache_response | ||
| from common.auth.cognito_jwt import JSONWebTokenAuthentication | ||
| from metrics.api.decorators.auth import require_authorisation | ||
| from metrics.api.enums import AppMode | ||
| from metrics.api.serializers import ChartsSerializer | ||
|
|
@@ -218,6 +220,7 @@ def post(cls, request, *args, **kwargs): | |
|
|
||
|
|
||
| class EncodedChartsView(APIView): | ||
| authentication_classes = [SessionAuthentication, JSONWebTokenAuthentication] | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should not be being set. |
||
| permission_classes = [] | ||
|
|
||
| @classmethod | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -15,6 +15,9 @@ | |
| from metrics.api.permissions.fluent_permissions import ( | ||
| validate_permissions_for_non_public, | ||
| ) | ||
| from metrics.utils.permissions import ( | ||
| check_any_permissions_allow_access, | ||
| ) | ||
|
|
||
|
|
||
| class CoreHeadlineQuerySet(models.QuerySet): | ||
|
|
@@ -320,21 +323,22 @@ | |
| return CoreHeadlineQuerySet(model=self.model, using=self.db) | ||
|
|
||
| def query_for_data( | ||
| self, | ||
| *, | ||
| topic: str, | ||
| metric: str, | ||
| fields_to_export: list[str], | ||
| geography: str = "England", | ||
| geography_type: str = "Nation", | ||
| geography_code: str = "", | ||
| stratum: str = "", | ||
| sex: str = "", | ||
| age: str = "", | ||
| theme: str = "", | ||
| sub_theme: str = "", | ||
| rbac_permissions: Iterable["RBACPermission"] | None = None, | ||
| jwt_permissions: dict | None = None, | ||
| **kwargs, | ||
|
Check warning on line 341 in metrics/data/managers/core_models/headline.py
|
||
| ): | ||
| """Filters for a N-item list of dicts by the given params if `fields_to_export` is used. | ||
|
|
||
|
|
@@ -373,6 +377,10 @@ | |
| rbac_permissions: The RBAC permissions available | ||
| to the given request. This dictates whether the given | ||
| request is permitted access to non-public data or not. | ||
| The new JWT-based authorization below takes precedence | ||
| over RBAC permissions, which is not in use anymore. | ||
| jwt_permissions: The JWT permissions extracted from the Cognito token. | ||
| Contains 'permission_set_hierarchy' (list) and 'has_global_access' (bool). | ||
|
|
||
| Returns: | ||
| Queryset of (x_axis, y_axis) where x_axis represents the variable on the x_axis | ||
|
|
@@ -382,16 +390,38 @@ | |
| Examples: | ||
| <CoreHeadlineQuerySet [{'age__name': '01-04', 'metric_value': Decimal('534.0000')}]> | ||
| """ | ||
|
|
||
| rbac_permissions = rbac_permissions or [] | ||
| has_access_to_non_public_data: bool = validate_permissions_for_non_public( | ||
| theme=theme, | ||
| sub_theme=sub_theme, | ||
| topic=topic, | ||
| metric=metric, | ||
| geography=geography, | ||
| geography_type=geography_type, | ||
| rbac_permissions=rbac_permissions, | ||
| ) | ||
|
|
||
| has_access_to_non_public_data: bool | ||
|
|
||
| if jwt_permissions: | ||
| # Check JWT permissions first (new authorization takes precedence) | ||
| has_global_access = jwt_permissions.get("has_global_access", False) | ||
|
|
||
| if has_global_access: | ||
| has_access_to_non_public_data = True | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could simplify this |
||
| else: | ||
| has_access_to_non_public_data = check_any_permissions_allow_access( | ||
| jwt_permissions=jwt_permissions, | ||
| theme=theme, | ||
| sub_theme=sub_theme, | ||
| topic=topic, | ||
| metric=metric, | ||
| geography_type=geography_type, | ||
| geography=geography, | ||
| ) | ||
| else: | ||
| # Legacy RBAC permissions (not in use) (to be removed in a future release) | ||
| has_access_to_non_public_data = validate_permissions_for_non_public( | ||
| theme=theme, | ||
| sub_theme=sub_theme, | ||
| topic=topic, | ||
| metric=metric, | ||
| geography=geography, | ||
| geography_type=geography_type, | ||
| rbac_permissions=rbac_permissions, | ||
| ) | ||
|
|
||
| if has_access_to_non_public_data: | ||
| queryset = self.get_queryset().get_all_headlines_released_from_embargo( | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,6 +7,7 @@ | |
| from django.db.models import Manager, QuerySet | ||
| from pydantic import BaseModel | ||
|
|
||
| from common.auth.cognito_jwt.user_manager import extract_jwt_permissions | ||
| from metrics.api.settings import auth | ||
| from metrics.data.models.core_models import CoreTimeSeries, Topic | ||
| from metrics.domain.common.utils import ChartAxisFields | ||
|
|
@@ -162,7 +163,9 @@ def get_queryset_from_core_model_manager( | |
| plot_params["fields_to_export"].append("lower_confidence") | ||
|
|
||
| return self.core_model_manager.query_for_data( | ||
| **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 | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. as with previous comment, you should be able to just use something like: |
||
| ) | ||
|
|
||
| def build_plot_data_from_parameters_with_complete_queryset( | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -21,6 +21,54 @@ | |
| ) | ||
|
|
||
|
|
||
| def convert_permission_set_into_hierarchy(raw_permission_sets: dict) -> dict: | ||
| """ | ||
| Convert a "permission_set" back into a "permission_set_hierarchy" again | ||
| (the NormalizedPermission class does it the other way round) | ||
|
|
||
| @param {dict} raw_permission_sets, eg: | ||
| { | ||
| "permission_sets": [ | ||
| { | ||
| "theme": {"id": "100", "name": "immunisation"}, | ||
| "sub_theme": {"id": "200", "name": "childhood-vaccines"}, | ||
| "topic": {"id": "215", "name": "MMR1"}, | ||
| } | ||
| ], | ||
| "summary": { | ||
| "has_global_access": False | ||
| }, | ||
| } | ||
|
|
||
| @return {dict}, eg: | ||
| { | ||
| "permission_set_hierarchy": [ | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What's the value in this over using the raw permissions?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @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. |
||
| { | ||
| "theme": {"id": "100", "name": "immunisation"}, | ||
| "sub_theme": {"id": "200", "name": "childhood-vaccines"}, | ||
| "topic": {"id": "215", "name": "MMR1"}, | ||
| } | ||
| ], | ||
| "has_global_access": False, | ||
| } | ||
| """ | ||
|
|
||
| permission_set_hierarchy = raw_permission_sets.get("permission_set_hierarchy") | ||
| if permission_set_hierarchy is None: | ||
| permission_set_hierarchy = raw_permission_sets.get("permission_sets", []) | ||
|
|
||
| has_global_access = raw_permission_sets.get("has_global_access") | ||
| if has_global_access is None: | ||
| has_global_access = raw_permission_sets.get("summary", {}).get( | ||
| "has_global_access", False | ||
| ) | ||
|
|
||
| return { | ||
| "permission_set_hierarchy": permission_set_hierarchy, | ||
| "has_global_access": bool(has_global_access), | ||
| } | ||
|
|
||
|
|
||
| @dataclass | ||
| class NormalizedPermission: | ||
| """ | ||
|
|
||
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.
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
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.
@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.