kobotoolbox / kpi / 18598022134

81%

master: 76%

Build Type

push

github

Committed by web-flow

Commit Message

fix(dataCollectors): remove assets from groups if owners lose permission DEV-1150 (#6357)

### 📣 Summary
Ensure data collectors can no longer access projects if the group owner
has lost permission.


### 📖 Description
Data collector group owners can only add an asset to their group if they
have the manage-asset permission. If that permission is removed, the
asset should no longer be assigned to the group and all the associated
data collector enketo links should be removed.

### Notes
The big problem that needed to be solved with this PR was the situation
where, as part of recalculating permissions, we delete all old ones
before possibly adding them back. To make matters even more complicated,
we do this every time we save an asset.

The approach this PR takes is to refactor the recalculating of
permissions to only delete the old permissions that we know we want to
delete, rather than deleting and re-adding the ones we care about,
except in the case where `return_instead_of_creating` is passed to the
recalculation method. In that case, it's the responsibility of the
caller to handle whatever problems may arise from removing the
manage_asset permission. This approach has a few benefits:
1. no new db calls. this actually reduces the number of db calls because
we aren't re-creating the same permissions that already exist
2. no data collector-specific code outside of the data collector app
3. we can enforce the removal of assets at the model level, no matter if
the permission was removed permissions from the DCG owner via the
expected endpoint(s) or a different path

Other approaches considered:
1. Updating the bulk delete endpoint to call `post_remove_perm` for each
permission removed and using that as the signal instead of
ObjectPermission.post_delete. Rejected because it would add at least one
database call to the endpoint (to get all existing permissions so we
knew which ones we were removing) and because it's endpoint-specific and
not future-proof aga... (continued)

Run Details

7061 of 11107 branches covered (63.57%)

20 of 22 new or added lines in 2 files covered. (90.91%)

1 existing line in 1 file now uncovered.

27276 of 33555 relevant lines covered (81.29%)

1.6 hits per line