Revoke all access tokens when suspending a user #2308
Conversation
There was a problem hiding this comment.
Hi @mike29736
Apologies it's taken so long for me to finish reviewing this PR. I had quite a lot to learn about how things work at the moment to understand the effect of this change!
If tokens aren't revoked, a suspended user technically retains its
authorised access (potentially for years, because TTL is quite long).
It's unclear what could be achieved maliciously using that access
without any permissions, but the consensus is that it's safer and
cleaner to revoke the tokens of suspended accounts.
I've added a comment to the Trello card that demonstrates how an API Token for a suspended API User can be used to upload files to Asset Manager. So I think it depends on whether the API is checking that the API User has specific permissions, or whether (as I believe to be the case with Asset Manager) the "signin" permission allows that API User to interact fully with the API.
The implementation is simpler if we just perform this step
for suspended Users more generally, and it seems safe to do that...
but I could be missing something here! Any idea how I could check that?
I agree that it seems safe to do this as Doorkeeper already handles the case of revoked Access Tokens. When a normal user signs in to one of the publishing apps (e.g. Whitehall) then they'll end up with an Access Token in the oauth_access_tokens table of Signon. If they sign out and then back in while that Access Token is active (i.e. not expired and not revoked) then they'll continue to use that same Access Token. If they sign out and that Access Token is revoked then the next time they sign in a new Access Token will be created.
Can you add a migration to this PR to revoke the Access Tokens for suspended users currently in the system. It might worth notifying 2ndline once we're ready to deploy this on the off chance that something is relying on using an API token of a suspended user.
While reviewing this I noticed that the InactiveUsersSuspender#suspend method duplicates some of the logic in Suspendable#suspend. At the moment this means that any users (presumably including API users) that are suspended due to inactivity won't have their Access Tokens revoked. For consistency I think we should revoke the Access Tokens irrespective of what triggered the suspension. Can you update the PR to do that, please?
I've asked for two changes based on my current understanding of the world. Let me know if you disagree and/or want to chat about it in person :-)
mike29736
force-pushed
the
revoke-api-user-token-when-suspending
branch
from
abcc41b
to
6f725c6
Compare
|
Thanks for the review, @chrisroos! I've updated that first commit message to remove some of the doubt and reflect your comments. And I've added the same step to the It's not obvious to me how to de-duplicate those two competing user-suspending methods ( I'll respond to your Asset Manager discovery in the Trello card |
app/models/user.rb
Outdated
| @@ -159,6 +159,10 @@ def authorised_applications | |||
| end | |||
| alias_method :applications_used, :authorised_applications | |||
|
|
|||
| def revoke_all_authorisations | |||
| authorisations.each(&:revoke) | |||
There was a problem hiding this comment.
Hi @mike29736 - I've just realised that this will update the revoked_at timestamp for all tokens, including those that are already revoked. I think it should probably only update those that are not currently revoked so that we preserve the revoked_at timestamp of any tokens that have already been revoked. What do you think?
There was a problem hiding this comment.
Thanks, good catch
mike29736
force-pushed
the
revoke-api-user-token-when-suspending
branch
from
6f725c6
to
ccd188e
Compare
We're going to use this method when suspending users
Until now a suspended ApiUser is flagged as suspended in the database, which results in Signon telling applications that the user has no permissions. Here we add the extra step of revoking the user's access tokens, too. If tokens aren't revoked, a suspended user technically retains its authorised access (potentially for years, because TTL is quite long). Despite the user also losing all of their permissions, this does present a security concern. The intended target of this change is `ApiUser`s (i.e. applications and not human users). The implementation is simpler if we just perform this step for suspended `User`s more generally and it's safe to do that because in the case of normal, human users, new tokens are created automatically on signin if expired or revoked.
Similarly to the change made to `User#suspend`, this fixes the issue of suspended users (especially API users, whose tokens have much longer TTLs) having active valid tokens which potentially allow them to access publishing apps. It seems as though `InactiveUsersSuspender` duplicates the logic from `User#suspend` and `Suspension#save` because it's avoiding performing validations on save.
We've updated our user suspension logic to revoke all of a user's access tokens. This migration backfills that fix for existing, currently suspended users. This is important because tokens can have long TTLs
mike29736
force-pushed
the
revoke-api-user-token-when-suspending
branch
from
ccd188e
to
3305799
Compare
https://trello.com/c/UTcTxQKG/142-suspending-api-users-on-signon-doesnt-disable-their-tokens
Until now a suspended ApiUser is flagged as suspended in the database,
which results in Signon telling applications that the user has no
permissions. Here we add the extra step of revoking the user's access
tokens, too.
If tokens aren't revoked, a suspended user technically retains its
authorised access (potentially for years, because TTL is quite long).
It's unclear what could be achieved maliciously using that access
without any permissions, but the consensus is that it's safer and
cleaner to revoke the tokens of suspended accounts.
The intended target of this change is
ApiUsers (i.e. applications and nothuman users). The implementation is simpler if we just perform this step
for suspended
Users more generally, and it seems safe to do that...but I could be missing something here! Any idea how I could check that?