Fix the anonymization script issue for access grants #10926
Conversation
|
@thewca-bot deploy staging |
danieljames-dj
force-pushed
the
anonymization-fix
branch
from
4bdae51 to
2a72d99
Compare
|
I tested in staging and this is working as expected. |
danieljames-dj
force-pushed
the
anonymization-fix
branch
from
2a72d99 to
9b845a5
Compare
app/models/user.rb
Outdated
| @@ -1502,7 +1502,22 @@ def location | |||
| end | |||
|
|
|||
| def anonymization_checks_with_message_args | |||
| access_grants = oauth_access_grants&.select { |access_grant| !access_grant.revoked_at.nil? } | |||
| access_grants = oauth_access_grants | |||
| .select { |access_grant| !access_grant.revoked_at.nil? } | |||
There was a problem hiding this comment.
Just curious: Why do you limit this to access grants where revoked_at is NOT nil?
This means you will only see grants which have revoked_at present, i.e. ones which have already been revoked.
There was a problem hiding this comment.
On that note, the select can be a tad bit easier to parse if you use access_grant.revoked_at.present? because you can skip the ! in that case.
There was a problem hiding this comment.
Why do you limit this to access grants where revoked_at is NOT nil?
TBH I don't know. This was evolved from some long discussions which I'm not aware of. I checked Jacob's initial doc but according to that doc, only the applications where resource_owner_id is the user needs to be cleared - but that actually doesn't come under 'anonymization of the user'. So at some point it was changed to every oauth_access_grants.
The select came in after one of your comments: #6086 (comment) and I don't have much context on it.
IMO, this should be access_grants without select. By any chance do you remember any context on this?
There was a problem hiding this comment.
Huh. To be honest I don't remember that I originally gave this comment to Jacob. Was really surprised to see my own reviews there 😅😅
My guess (and I really mean guess here!) is that you only need to care about grants which have already been revoked, because the user requesting Anon might not be aware of them anymore.
All currently active grants (ie those where revoked_at is nil) can be taken care of by the user themselves.
With that being said, the approach with filtering for not-nil obviously worked fine for many years. So it might be worth checking on WRT-internal channels if anybody knows anything about this procedure.
If necessary, you can also try reaching out to Jacob in retirement and see if he still remembers.
There was a problem hiding this comment.
... the select can be a tad bit easier to parse ...
I just figured out oauth_access_grants.where.not(revoked_at: nil) might also be a good option. I'm not sure if it'll work fine in staging. I'll test in staging once and confirm.
Re decision on whether we need all access grants or just the ones with revoked_at value as nil or just the ones with revoked_at value as not nil - I will check with the WRT and if change is required, I'll make it in a later PR. Is it fine to approve this PR as it is now so that it will allow WRT to anonymize in case some request arise? Currently the UI is breaking when access_grants is available for a user.
There was a problem hiding this comment.
I'm not sure if it'll work fine in staging. I'll test in staging once and confirm.
I've now confirmed that this code is working fine in staging.
|
@thewca-bot deploy staging |
danieljames-dj
force-pushed
the
anonymization-fix
branch
from
da90c07 to
ea93713
Compare
|
@thewca-bot deploy staging |
This PR is to fix the issue where the anonymization script UI breaks as it was trying to access
applicationofaccess_grants(code link).I am not sure if this fix will work, I couldn't test it in local as I don't have any idea how to populate access grants.