HP-2319 | Check allowed data fields when querying profiles #494

nicobav · 2024-04-29T06:13:51Z

Adds middleware for checking allowed_data_fields for service when querying profiles
Adds feature flag for the feature.

refs HP-2319

terovirtanen · 2024-04-29T07:05:42Z

HELSINKI-PROFILE-API branch is deployed to platta: https://helsinki-profile-pr494.api.dev.hel.ninja 🚀🚀🚀

terovirtanen · 2024-04-29T10:11:43Z

HELSINKI-PROFILE-API branch is deployed to platta: https://helsinki-profile-pr494.api.dev.hel.ninja 🚀🚀🚀

terovirtanen · 2024-04-29T11:12:31Z

HELSINKI-PROFILE-API branch is deployed to platta: https://helsinki-profile-pr494.api.dev.hel.ninja 🚀🚀🚀

terovirtanen · 2024-04-30T09:58:21Z

HELSINKI-PROFILE-API branch is deployed to platta: https://helsinki-profile-pr494.api.dev.hel.ninja 🚀🚀🚀

danipran

The code itself seems fine, just a bit confused about the documentation.

danipran · 2024-05-03T12:09:12Z

profiles/schema.py

+    `allowed_data_fields_map` is a dictionary where the key is the `field_name` of the allowed data field
+    `allowed_data_fields.json` and the value is an iterable of django model's field names that the `field_name`


Did you mean:

`allowed_data_fields_map` is a dictionary where the key is the `field_name` of the allowed data field -> in <- `allowed_data_fields.json` and the value is an iterable of django model's field names that the `field_name`

danipran · 2024-05-03T12:35:59Z

profiles/schema.py

+    Mixin class for checking allowed data fields per service.
+
+    `allowed_data_fields_map` is a dictionary where the key is the `field_name` of the allowed data field
+    `allowed_data_fields.json` and the value is an iterable of django model's field names that the `field_name`
+    describes. For example, if the `field_name` is `name`, the value could be `("first_name", "last_name")`.
+    e.g:
+    allowed_data_fields_map = {
+        "name": ("first_name", "last_name", "nickname"),
+        "personalidentitycode": ("national_identification_number",),
+        "address": ("address", "postal_code", "city", "country_code")
+    }
+
+    `always_allow_fields`: Since connections are not defined in `allowed_data_fields.json` they should be
+    defined here. If the field is connection and the node does not inherit this mixin the data will be available
+    to all services.


So... what's allowed_data_fields.json? 😅 No mention of it in README. It's a bit confusing for me at least.

The docstring doesn't quite click for me; I understand there's a mapping from some file's field name to model fields, but I'm not quite sure what this does. Checks for allowed data fields, which means...? Judging by the tests, it's some sort of an enforced filter for data? Yet there's no allowed_data_fields.json anywhere, just AllowedDataField instances. What role does allowed_data_fields.json play in this?

So... what's allowed_data_fields.json? 😅 No mention of it in README. It's a bit confusing for me at least.

Oh that's actually a good question in this github context. Our documentation elsewhere guides about using json file named like that to import the allowed data fields using command set_allowed_data_fields. Which then creates the AllowedDataField model instances mapped in the json file.

Checks for allowed data fields, which means...?

We need to check from the AllowedDataField instances linked to Service to which fields the service is allowed to access.

open_city_profile/graphene.py

profiles/tests/test_gql_my_profile_query.py

nicobav · 2024-05-07T11:16:31Z

I made some changes based on the comments and added the feature to be inside a feature flag as discussed irl.

codecov-commenter · 2024-05-07T11:26:39Z

Codecov Report

Attention: Patch coverage is 93.72549% with 16 lines in your changes are missing coverage. Please review.

Project coverage is 95.42%. Comparing base (f505b24) to head (41d0b49).
Report is 36 commits behind head on main.

❗ Current head 41d0b49 differs from pull request most recent head 95193fe

Please upload reports for the commit 95193fe to get more accurate results.

Files	Patch %	Lines
open_city_profile/graphene.py	74.19%	7 Missing and 1 partial ⚠️
open_city_profile/settings.py	0.00%	1 Missing and 1 partial ⚠️
open_city_profile/tests/graphql_test_helpers.py	66.66%	1 Missing and 1 partial ⚠️
profiles/schema.py	88.23%	1 Missing and 1 partial ⚠️
profiles/tests/test_gql_my_profile_query.py	97.95%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #494      +/-   ##
==========================================
- Coverage   95.56%   95.42%   -0.14%     
==========================================
  Files         207      214       +7     
  Lines        8222     8616     +394     
  Branches      991     1037      +46     
==========================================
+ Hits         7857     8222     +365     
- Misses        279      301      +22     
- Partials       86       93       +7

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

terovirtanen · 2024-05-07T11:55:36Z

HELSINKI-PROFILE-API branch is deployed to platta: https://helsinki-profile-pr494.api.dev.hel.ninja 🚀🚀🚀

open_city_profile/graphene.py

profiles/schema.py

terovirtanen · 2024-05-10T12:17:26Z

HELSINKI-PROFILE-API branch is deployed to platta: https://helsinki-profile-pr494.api.dev.hel.ninja 🚀🚀🚀

terovirtanen · 2024-05-13T06:57:51Z

HELSINKI-PROFILE-API branch is deployed to platta: https://helsinki-profile-pr494.api.dev.hel.ninja 🚀🚀🚀

open_city_profile/graphene.py

terovirtanen · 2024-05-22T06:22:37Z

HELSINKI-PROFILE-API branch is deployed to platta: https://helsinki-profile-pr494.api.dev.hel.ninja 🚀🚀🚀

terovirtanen · 2024-05-22T07:36:29Z

HELSINKI-PROFILE-API branch is deployed to platta: https://helsinki-profile-pr494.api.dev.hel.ninja 🚀🚀🚀

profiles/models.py

open_city_profile/graphene.py

Refs HP-2319

When calling profile any kind there should be checked to which fields the service has access rights by using the allowed data fields. This adds middleware and mixin class for Profile model and VerifiedPersonalInfo models for checking that the queried fields are allowed for the service. Refs HP-2319

Adds setting for allowed data field check named as: ENABLE_ALLOWED_DATA_FIELDS_RESTRICTION The setting defaults False. When False, app logs a warning message when there is a query trying to access a field which is not in service's allowed_data_fields Refs HP-2319

sonarqubecloud · 2024-05-23T13:29:41Z

Quality Gate passed

Issues
3 New issues
0 Accepted issues

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

See analysis details on SonarCloud

terovirtanen · 2024-05-23T13:57:17Z

HELSINKI-PROFILE-API branch is deployed to platta: https://helsinki-profile-pr494.api.dev.hel.ninja 🚀🚀🚀

charn

Good work! LGTM! 👍

nicobav force-pushed the HP-2319/restrict-queryable-myprofile-fields branch 2 times, most recently from 4b5d514 to 9f33558 Compare April 29, 2024 06:55

nicobav force-pushed the HP-2319/restrict-queryable-myprofile-fields branch from 9f33558 to 09201eb Compare April 29, 2024 10:01

nicobav requested a review from a team April 29, 2024 10:56

nicobav force-pushed the HP-2319/restrict-queryable-myprofile-fields branch from 09201eb to 46b3a26 Compare April 29, 2024 11:00

nicobav marked this pull request as ready for review April 29, 2024 11:01

nicobav changed the title ~~DRAFT: HP-2319 | Check myProfile query fields for allowed data fields~~ HP-2319 | Check myProfile query fields for allowed data fields Apr 29, 2024

nicobav force-pushed the HP-2319/restrict-queryable-myprofile-fields branch from 46b3a26 to 4bbdc66 Compare April 30, 2024 09:48

danipran reviewed May 3, 2024

View reviewed changes

charn reviewed May 3, 2024

View reviewed changes

open_city_profile/graphene.py Outdated Show resolved Hide resolved

charn reviewed May 3, 2024

View reviewed changes

profiles/tests/test_gql_my_profile_query.py Show resolved Hide resolved

nicobav force-pushed the HP-2319/restrict-queryable-myprofile-fields branch 2 times, most recently from c94f001 to b631a9f Compare May 7, 2024 11:15

nicobav force-pushed the HP-2319/restrict-queryable-myprofile-fields branch from b631a9f to 41d0b49 Compare May 7, 2024 11:21

charn reviewed May 8, 2024

View reviewed changes

open_city_profile/graphene.py Outdated Show resolved Hide resolved

charn reviewed May 8, 2024

View reviewed changes

open_city_profile/graphene.py Outdated Show resolved Hide resolved

charn reviewed May 8, 2024

View reviewed changes

open_city_profile/graphene.py Outdated Show resolved Hide resolved

charn reviewed May 8, 2024

View reviewed changes

open_city_profile/graphene.py Outdated Show resolved Hide resolved

charn reviewed May 8, 2024

View reviewed changes

profiles/schema.py Outdated Show resolved Hide resolved

nicobav force-pushed the HP-2319/restrict-queryable-myprofile-fields branch 2 times, most recently from f4b6ca7 to 2e5f256 Compare May 10, 2024 11:42

nicobav force-pushed the HP-2319/restrict-queryable-myprofile-fields branch from 2e5f256 to 1f4fd99 Compare May 13, 2024 06:23

charn reviewed May 15, 2024

View reviewed changes

open_city_profile/graphene.py Outdated Show resolved Hide resolved

nicobav force-pushed the HP-2319/restrict-queryable-myprofile-fields branch from 1f4fd99 to 193c0e7 Compare May 22, 2024 05:48

nicobav changed the title ~~HP-2319 | Check myProfile query fields for allowed data fields~~ HP-2319 | Check allowed data fields when querying profiles May 22, 2024

nicobav force-pushed the HP-2319/restrict-queryable-myprofile-fields branch from 193c0e7 to 4284797 Compare May 22, 2024 07:01

charn reviewed May 22, 2024

View reviewed changes

profiles/models.py Outdated Show resolved Hide resolved

charn reviewed May 22, 2024

View reviewed changes

open_city_profile/graphene.py Outdated Show resolved Hide resolved

feat: add FieldNotAllowedError

da711f1

Refs HP-2319

nicobav force-pushed the HP-2319/restrict-queryable-myprofile-fields branch from 4284797 to d063e0a Compare May 23, 2024 13:18

nicobav added 2 commits May 23, 2024 16:21

nicobav force-pushed the HP-2319/restrict-queryable-myprofile-fields branch from d063e0a to 95193fe Compare May 23, 2024 13:22

charn approved these changes May 23, 2024

View reviewed changes

nicobav merged commit 3b18fbc into main May 28, 2024
26 checks passed

nicobav deleted the HP-2319/restrict-queryable-myprofile-fields branch May 28, 2024 12:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HP-2319 | Check allowed data fields when querying profiles #494

HP-2319 | Check allowed data fields when querying profiles #494

nicobav commented Apr 29, 2024 •

edited

Loading

terovirtanen commented Apr 29, 2024

terovirtanen commented Apr 29, 2024

terovirtanen commented Apr 29, 2024

terovirtanen commented Apr 30, 2024

danipran left a comment

danipran May 3, 2024

danipran May 3, 2024

nicobav May 6, 2024 •

edited

Loading

nicobav commented May 7, 2024

codecov-commenter commented May 7, 2024 •

edited

Loading

terovirtanen commented May 7, 2024

terovirtanen commented May 10, 2024

terovirtanen commented May 13, 2024

terovirtanen commented May 22, 2024

terovirtanen commented May 22, 2024

sonarqubecloud bot commented May 23, 2024

terovirtanen commented May 23, 2024

charn left a comment

		`allowed_data_fields_map` is a dictionary where the key is the `field_name` of the allowed data field
		`allowed_data_fields.json` and the value is an iterable of django model's field names that the `field_name`

HP-2319 | Check allowed data fields when querying profiles #494

HP-2319 | Check allowed data fields when querying profiles #494

Conversation

nicobav commented Apr 29, 2024 • edited Loading

terovirtanen commented Apr 29, 2024

terovirtanen commented Apr 29, 2024

terovirtanen commented Apr 29, 2024

terovirtanen commented Apr 30, 2024

danipran left a comment

Choose a reason for hiding this comment

danipran May 3, 2024

Choose a reason for hiding this comment

danipran May 3, 2024

Choose a reason for hiding this comment

nicobav May 6, 2024 • edited Loading

Choose a reason for hiding this comment

nicobav commented May 7, 2024

codecov-commenter commented May 7, 2024 • edited Loading

Codecov Report

terovirtanen commented May 7, 2024

terovirtanen commented May 10, 2024

terovirtanen commented May 13, 2024

terovirtanen commented May 22, 2024

terovirtanen commented May 22, 2024

sonarqubecloud bot commented May 23, 2024

Quality Gate passed

terovirtanen commented May 23, 2024

charn left a comment

Choose a reason for hiding this comment

nicobav commented Apr 29, 2024 •

edited

Loading

nicobav May 6, 2024 •

edited

Loading

codecov-commenter commented May 7, 2024 •

edited

Loading