[ENG-11829] Revoke ORCID trusted party access during GDPR delete - Part 1 - #11885
Conversation
292149b to
4c48222
Compare
f757b97 to
eebddd6
Compare
cslzchen
left a comment
cslzchen
left a comment
There was a problem hiding this comment.
3rd pass done 🌟
This is the main PR now, please update ticket title.
| sentry.log_message( | ||
| f'CAS response ORCID attributes: user=[{user._id}], orcidId=[{orcid_id}], ' | ||
| f'orcidAccessToken=[{"present" if access_token else "missing"}]', | ||
| level=logging.WARNING, | ||
| ) |
There was a problem hiding this comment.
Future (new ticket), change this to debug log and only log empty access token error after we dev tested on staging1.
| sentry.log_message( | ||
| f'ORCID token stored on external_identity_tokens: user=[{user._id}], ' | ||
| f'provider_id=[{orcid_id}], access_token=[{"present" if access_token else "missing"}]', | ||
| level=logging.INFO, | ||
| ) |
f5aa332 to
667cca7
Compare
Vlad0n20
changed the title
173ac2b to
c7b653f
Compare
| for service in user.external_identity: | ||
| for service_id in user.external_identity[service].keys(): | ||
| if not ( | ||
| service_id in self.external_identity.get(service, '') and | ||
| self.external_identity[service][service_id] == 'VERIFIED' | ||
| ): | ||
| # Prevent 'CREATE', merging user has already been created. | ||
| external = user.external_identity[service][service_id] | ||
| status = 'VERIFIED' if external == 'VERIFIED' else 'LINK' | ||
| if self.external_identity.get(service): | ||
| self.external_identity[service].update( | ||
| {service_id: status} | ||
| ) | ||
| else: | ||
| self.external_identity[service] = { | ||
| service_id: status | ||
| } | ||
|
|
||
| token_entry = user.external_identity_tokens.get(service, {}).get(service_id) | ||
| if token_entry: | ||
| self.external_identity_tokens.setdefault(service, {})[service_id] = token_entry |
There was a problem hiding this comment.
Not blocking the merge, but I have a question on how external identity is handled during merge?
e.g. two user have different ORCiD, it seems this will create two ORCiD entries, and I think this is the same behavior for tokens
There was a problem hiding this comment.
Checked — confirmed, this is real. Tracing it: if self already has ORCID id A VERIFIED and the merged-away user has a different id B also VERIFIED, the if not (...) guard is true for B (it's not yet in self.external_identity['ORCID']), so it falls into self.external_identity[service].update({service_id: status}), which adds B alongside A instead of replacing it. Same for the new token line (setdefault(service, {})[service_id] = token_entry) — it adds a second key. So a merge can leave self.external_identity['ORCID'] and self.external_identity_tokens['ORCID'] each with two entries.
This loop itself is pre-2016 generic multi-provider merge logic (git blame points to the original commit), so it's not something this PR introduced — the new token line just mirrors the same existing per-service_id loop. But it does mean the "a user has at most one ORCID identity" assumption _clear_identifying_information relies on (next(iter(...)), grabs one arbitrary entry) doesn't hold for merged accounts: if a user merges in an account with a different verified ORCID, GDPR delete will only revoke one of the two tokens and silently leave the other with live trusted-party access to ORCID.
Agreed this shouldn't block this PR. Want this tracked as a separate ticket, or should I fix _clear_identifying_information now to iterate/revoke all ORCID token entries instead of just the first one?
| 'auth_user_external_id_access_token': user_dict.get('external_id_access_token'), | ||
| 'auth_user_external_id_refresh_token': user_dict.get('external_id_refresh_token'), |
There was a problem hiding this comment.
Out of the scope of PR but is fine to have it here.
| external_id_access_token = session.get('auth_user_external_id_access_token', None) | ||
| external_id_refresh_token = session.get('auth_user_external_id_refresh_token', None) |
There was a problem hiding this comment.
Ditto, out of scope but it's ok
| campaign=None, | ||
| accepted_terms_of_service=accepted_terms_of_service | ||
| ) | ||
| cas.save_orcid_access_token_to_user(user, external_id, external_id_access_token, external_id_refresh_token) |
cslzchen
changed the title
51fb2fa
into
CenterForOpenScience:feature/gdpr-delete-orcid-rewrite
2 participants
Ticket
Purpose
Revoke ORCID trusted party access during GDPR delete - Part 1
Changes
Side Effects
QE Notes
CE Notes
Documentation