Add location-based row level security to group and individual #121

weilu · 2024-09-06T21:22:24Z

Below is a full list of GQL queries and mutations. @sniedzielski do I need to add row security to all of them or are there ones not used by the frontend or not relevant in this context?

to facilitate row security

Aka GroupIndividual

Also refactor individual mutation & query test to extract setup code

sniedzielski · 2024-09-24T14:47:20Z

I wonder about adding this row security in 'group' like:
"add_individual_to_group +
edit_individual_in_group + create_group_and_move_individual" to ensure we can add the individual only related to user's specific location. @delcroip What do you think about it and the whole changes? Should we include it in upcoming release?

weilu · 2024-10-01T14:37:17Z

@sniedzielski I'm marking it as ready for review as this PR is already a bit too big. It covers row-level security changes for the models and corresponding CRUD operations. Kindly review please.

I'll make separate PRs for the rest of the changes, tentatively one for csv import/export and another for enrollment.

delcroip

I added some comments, please check them and give me you thoughts

delcroip · 2024-10-03T13:26:52Z

individual/models.py

@@ -13,12 +15,50 @@ class Individual(HistoryModel):
    #TODO WHY the HistoryModel json_ext was not enough
    json_ext = models.JSONField(db_column="Json_ext", blank=True, default=dict)

+    village = models.ForeignKey(


I wonder if we should not have location instead, related names should be 'individuals'

This is an interesting point. I thought about this long and hard – the pro is the flexibility, but the con is also the flexibility. Say we go with location and allow it to be defined at any of the 4 geo levels, on the UI for creating & updating an individual/group, we will have the user select the location type, followed by the name. For individual import, we'd have two columns: location_type and location_name, instead of one village. These are all ok.

Using location also allows individuals and groups to have different levels of granularity defined for them. Would this be good for data consistency from a record keeping point of view? e.g. one might end up with a database of 20k individuals' locations at district level while another 10k at village level – would this serve program administration well? @andreamartin @malike @amschel-de-r would be good to have your thoughts on this.

Another minor-ish point to consider – What would be the validation logic on the relationship between the individual.location and individual.group.location when they are both present? Should they match exactly? One contains another? Would there be enrollment implications if group and individual locations do not match exactly?

yeah the concept of village as a geolocation is not available in all countries. The best approach will be to have labels which can be named at implementation level but tbh I don't know if its possible with the current openIMIS as the level of effort might be too big.

@malike the term "village" can be customized through translation on the UI. The decision point here is to either pin it down to the lowest geo level out of the 4 (most specific) or leave it flexible to allow it to be defined at any geo level for each individual/group.

I think it would be good practice that every instance has one geographic level that is required for all individuals, i.e. no location_type flexibility. Having columns in data upload for location_type and location_name would add unnecessary confusion for users for all the countries I can think of. I would also be very surprised if there was a country that didn't have at least one geo level that all individuals have data on.

What I would say, is that it might be good for the level the mandatory field is at (i.e. geo_4, geo_3, geo_5) to be customizable in one place, maybe in core_moduleConfigurations or something. This would help the system fit both a larger country - where they may have complete data down to geo_6 level and users that need row-level security at geo_3 level - as well as a small island country - where they may only have 3 geo levels.

If we're talking edge cases, e.g. migrants, refugees and IDPs, I think we can encourage a practice where they allow for a catch-all value. E.g. if the geo levels are Region, District, Municipality and Village, and refugees/IDPs are handled at the Region level, then for each region you would create e.g. R1D_NA - no district, R1M_NA - no municipality, R1V_NA no village. Then in the data upload, you could set all the refugees/migrants in district 1 to have R1V_NA and they would be successfully handled (if I understand the feature).

As a side note, having either village or location as the mandatory column name would add confusion either way - do we have somewhere where we can customize a mapping between mandatory model fields and csv upload column names?

Open to comments - also have run this past Astrid as well

delcroip · 2024-10-03T13:30:31Z

individual/models.py

+                prefix='village__parent__parent',
+                loc_types=['D']


this will work fine but we might change it to support all locations to just prefix='location and no loc_types'

Suggested change

prefix='village__parent__parent',

loc_types=['D']

delcroip · 2024-10-03T13:31:23Z

individual/models.py

+                prefix='groupindividual__group__village__parent__parent',
+                loc_types=['D']


Suggested change

prefix='groupindividual__group__village__parent__parent',

loc_types=['D']

prefix='groupindividual__group__location'

delcroip · 2024-10-03T13:33:15Z

individual/models.py

+    village = models.ForeignKey(
+        Location,
+        models.DO_NOTHING,
+        blank=True,
+        null=True
+    )


Suggested change

village = models.ForeignKey(

Location,

models.DO_NOTHING,

blank=True,

null=True

)

location = models.ForeignKey(

Location,

models.DO_NOTHING,

blank=True,

null=True,

related_name="groups",

)

delcroip · 2024-10-03T13:33:51Z

individual/models.py

+    village = models.ForeignKey(
+        Location,
+        models.DO_NOTHING,
+        blank=True,
+        null=True
+    )


Suggested change

village = models.ForeignKey(

Location,

models.DO_NOTHING,

blank=True,

null=True

)

location = models.ForeignKey(

Location,

models.DO_NOTHING,

blank=True,

null=True,

related_name="individuals",

)

delcroip · 2024-10-03T13:43:18Z

individual/gql_mutations.py

@@ -140,6 +152,15 @@ def _validate_mutation(cls, user, **data):
                IndividualConfig.gql_individual_delete_perms):
            raise ValidationError("mutation.authentication_required")

+        villages_qs = Location.objects.filter(individual__id__in=data['ids'], type='V')


not sure what you are doing here filtering Location with individual_id (for id you need only one _)

It's for checking that the user has sufficient (geo) permission to delete the individual. See test case IndividualGQLMutationTest.test_delete_individual_row_security.

Good spotting on the double _. Fixed.

delcroip · 2024-10-03T13:43:37Z

individual/gql_mutations.py

@@ -55,6 +58,7 @@ def resolve_recipient_type(self, info):
 class CreateGroupInputType(OpenIMISMutation.Input):
    code = graphene.String(required=True)
    individuals_data = graphene.List(CreateGroupIndividualInputTypeInputObjectType, required=False)
+    village_id = graphene.Int(required=False)


see comment on locations

delcroip · 2024-10-03T13:50:06Z

individual/gql_mutations.py

+        villages_qs = Location.objects.filter(individual__id__in=data['ids'], type='V')
+        # must first check if villages_qs exists in case none of the individuals has location
+        if villages_qs.exists():
+            allowed_loc_ids = Location.get_queryset(None, user).values('id')
+            not_in_allowed = villages_qs.exclude(id__in=Subquery(allowed_loc_ids))
+            # all individuals' villages must be within permission for the given user
+            if not allowed_loc_ids.exists() or not_in_allowed.exists():
+                raise ValidationError("mutation.authentication_required")


to be tested but you get the idea,

Suggested change

villages_qs = Location.objects.filter(individual__id__in=data['ids'], type='V')

# must first check if villages_qs exists in case none of the individuals has location

if villages_qs.exists():

allowed_loc_ids = Location.get_queryset(None, user).values('id')

not_in_allowed = villages_qs.exclude(id__in=Subquery(allowed_loc_ids))

# all individuals' villages must be within permission for the given user

if not allowed_loc_ids.exists() or not_in_allowed.exists():

raise ValidationError("mutation.authentication_required")

individual_not_allowed = Individual.objects.filters(

id__in=data['ids'],

Q(~location__in=LocationManager().allowed(user=user))

)

if individual_not_allowed:

raise ValidationError("mutation.authentication_required")

delcroip · 2024-10-03T13:51:13Z

individual/gql_mutations.py

@@ -175,6 +196,15 @@ def _validate_mutation(cls, user, **data):
                IndividualConfig.gql_individual_undo_delete_perms):
            raise ValidationError("mutation.authentication_required")

+        villages_qs = Location.objects.filter(individual__id__in=data['ids'], type='V')


Fixed unnecessary __

delcroip · 2024-10-03T13:52:25Z

individual/gql_mutations.py

@@ -232,6 +266,9 @@ def _validate_mutation(cls, user, **data):
        if not user.has_perms(
                IndividualConfig.gql_group_update_perms):
            raise ValidationError("mutation.authentication_required")
+        village = Group.objects.get(id=data['id']).village
+        if village and village not in Location.get_queryset(None, user):


TBC id allowed won't be better

What do you mean?

weilu · 2024-10-19T20:18:03Z

@delcroip I see you merged these changes into 24.10 release branch. Should 24.10 be merged to develop and this PR be discarded?

weilu · 2024-10-19T20:28:24Z

Closing this as the change set is included in #130

weilu added 3 commits September 5, 2024 11:35

Add village to individual and group model

80997e4

to facilitate row security

Unit test existing individual GQL permission control

e6db8b5

Enable row level security on group and individual list queries

d4cd3ae

weilu marked this pull request as draft September 6, 2024 21:22

weilu linked an issue Sep 6, 2024 that may be closed by this pull request

Add row-level security to individual and group weilu/openimis-be-individual_py#1

Closed

weilu added 10 commits September 9, 2024 13:56

Row-level security for individual history query

af44440

Row-level security for group history query

3269d6b

Row-level security for group membership query

48df524

Aka GroupIndividual

Row-level security for individual group membership history

93000ab

Add row level security to individual creation query

e01ebe2

Add row-level security to individual update query

8cd6b76

Add row-level security to individual delete query

27a5d33

Add row-level security to individual undo delete query

9b814cb

Add basic create group gql mutation test

364ec1c

Also refactor individual mutation & query test to extract setup code

Add row level security to group create

c90e48b

weilu added 7 commits September 24, 2024 23:53

Row level security for group update

fc96164

Add row level security to group delete

d9664a6

Add row level security to adding individual to group

80b9ba0

Verify that both individual_id and group_id are required by service

5cb938b

Add row level security to editing individual in group

2cbda50

Add row level security to removing individuals from group

c7556bb

Make sure individual group move doesn’t create and delete unnecessarily

93b8d54

weilu marked this pull request as ready for review October 1, 2024 14:33

delcroip self-requested a review October 3, 2024 10:00

delcroip reviewed Oct 3, 2024

View reviewed changes

Remove unnecessary location-individual relationship reversal in query

629486f

weilu mentioned this pull request Oct 19, 2024

Merge release branch 24.10 to develop #130

Merged

weilu closed this Oct 19, 2024

weilu deleted the row-security branch October 19, 2024 20:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add location-based row level security to group and individual #121

Add location-based row level security to group and individual #121

weilu commented Sep 6, 2024 •

edited

Loading

sniedzielski commented Sep 24, 2024

weilu commented Oct 1, 2024

delcroip left a comment

delcroip Oct 3, 2024

weilu Oct 3, 2024

malike Oct 4, 2024

weilu Oct 4, 2024

amschel-de-r Oct 8, 2024 •

edited

Loading

delcroip Oct 3, 2024

delcroip Oct 3, 2024

delcroip Oct 3, 2024

delcroip Oct 3, 2024

delcroip Oct 3, 2024

weilu Oct 4, 2024 •

edited

Loading

delcroip Oct 3, 2024

delcroip Oct 3, 2024

delcroip Oct 3, 2024

weilu Oct 4, 2024

delcroip Oct 3, 2024

weilu Oct 4, 2024

weilu commented Oct 19, 2024

weilu commented Oct 19, 2024

		prefix='groupindividual__group__village__parent__parent',
		loc_types=['D']

	prefix='groupindividual__group__village__parent__parent',
	loc_types=['D']
	prefix='groupindividual__group__location'

Add location-based row level security to group and individual #121

Add location-based row level security to group and individual #121

Conversation

weilu commented Sep 6, 2024 • edited Loading

sniedzielski commented Sep 24, 2024

weilu commented Oct 1, 2024

delcroip left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amschel-de-r Oct 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weilu Oct 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weilu commented Oct 19, 2024

weilu commented Oct 19, 2024

weilu commented Sep 6, 2024 •

edited

Loading

amschel-de-r Oct 8, 2024 •

edited

Loading

weilu Oct 4, 2024 •

edited

Loading