-
-
Notifications
You must be signed in to change notification settings - Fork 184
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
feat(ProjectHistoryLogs): create ph logs for permissions changes TASK-944 #5297
Conversation
5a181da
to
d0c0d94
Compare
7c0f25f
to
8be6200
Compare
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.
LGTM, although I haven't done an in-depth review. I leave that to Guillermo.
Just left some aesthetic comments.
kobo/apps/audit_log/models.py
Outdated
# remove each permission as it is logged so we can see if there | ||
# are any unusual ones left over | ||
for combination, action in ANONYMOUS_USER_PERMISSION_ACTIONS.items(): | ||
# ANONYMOUS_USER_PERMIISSION_ACTIONS has tuples as keys |
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.
typo ;-)
@receiver(post_assign_perm, sender=Asset) | ||
def add_assigned_perms(sender, instance, user, codename, deny, **kwargs): | ||
request = initialize_request() | ||
if not request or instance.asset_type != ASSET_TYPE_SURVEY or deny: | ||
return | ||
request.permissions_added[user.username].add(codename) | ||
request.permissions_removed[user.username].discard(codename) | ||
|
||
|
||
@receiver(post_remove_perm, sender=Asset) | ||
def add_removed_perms(sender, instance, user, codename, **kwargs): | ||
request = initialize_request() | ||
if not request or instance.asset_type != ASSET_TYPE_SURVEY: | ||
return | ||
request.permissions_removed[user.username].add(codename) | ||
request.permissions_added[user.username].discard(codename) | ||
|
||
|
||
@receiver(post_assign_partial_perm, sender=Asset) | ||
def add_assigned_partial_perms(sender, instance, user, perms, **kwargs): | ||
request = initialize_request() | ||
if not request or instance.asset_type != ASSET_TYPE_SURVEY: | ||
return | ||
perms_as_list_of_dicts = [{'code': k, 'filters': v} for k, v in perms.items()] | ||
# partial permissions are replaced rather than added | ||
request.partial_permissions_added[user.username] = perms_as_list_of_dicts | ||
|
||
|
||
@receiver(post_remove_partial_perms, sender=Asset) | ||
def remove_partial_perms(sender, instance, user, **kwargs): | ||
request = initialize_request() | ||
if not request or instance.asset_type != ASSET_TYPE_SURVEY: | ||
return | ||
request.partial_permissions_added[user.username] = [] | ||
# in case we somehow deleted partial permissions | ||
# without deleting 'partial_submissions', take care of that now since | ||
# we can't have one without the other | ||
request.permissions_added[user.username].discard(PERM_PARTIAL_SUBMISSIONS) | ||
request.permissions_removed[user.username].add(PERM_PARTIAL_SUBMISSIONS) |
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.
A -> Z 🙏
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.
I'm going to alphabetize everything in one big PR after all the functionality is done
sender=Asset, | ||
instance=self.asset, | ||
user=self.user, | ||
codename=PERM_VIEW_ASSET, |
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.
Should it be PERM_PARTIAL_SUBMISSIONS
? or you did it in purpose because of your test?
If it is the latter, you should add a comment because PERM_VIEW_ASSET
should never be added with partial perms.
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.
In this case it doesn't matter since I'm just testing the signal, not actually adding permissions to anything.
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.
Made some minor suggestions via chat, the rest of functionality and code looks great, and it works like a charm.
🗒️ Checklist
<type>(<scope>)<!>: <title> TASK-1234
frontend
orbackend
unless it's global📣 Summary
Create project history logs when users are assigned permissions or have permissions revoked for a project.
👀 Preview steps
Note: for this test, all PH logs should have
metadata['log_subtype']='permission'
andmetadata['asset_uid']=<the uid of the project you're updating>
/api/v2/audit-logs/?q=log_type:project-history AND metadata__asset_uid:<Project1.uid>
and
tmp.json
file (unless you really want to type the whole json into the command line):curl -v -X POST -H "Content-Type: application/json" -H "Authorization: Token <your token>" http://localhost/api/v2/assets/<asset_uid>/permission-assignments/bulk/?format=json -d @/tmp.json
💭 Notes
This PR does a lot:
Additional notes:
Partial permissions are handled a bit differently than normal ones since they are not additive. Every time we set partial permissions, we're wiping away any old partial permissions.
I tried to be thorough in the unit tests but there are a LOT of different ways permissions can change, especially with a bulk request.
This PR purposely leaves out changes that result from collection actions. Those will be dealt with later. Similarly, the PR does not handle the v1 endpoint or requests to transfer ownership.