Auth revoke on profile update

I could not apply the https://www.drupal.org/files/issues/2020-06-10/simple_oauth-conditionall... → for new version, so i made an alternative https://www.drupal.org/files/issues/2023-04-22/simple_oauth-conditionall... → . I hope this helps someone.

Hi everyone,

Can someone explain to me what the exact reason is to invalidate the token when the user's role, email, or password, etc. changes?
If I understand correctly, changing roles, for example, should have no impact on the tokens because the Drupal access checks do check the user's permissions anyway.

Therefore, not revoking the token should not grant the user unintended access.

See src/Authentication/TokenAuthUser.php:94

Thanks in advance!

Made this feature also available for 6.0.x with MR 99. When I find some time I will try to setup some tests, which will be applicable for 5.2 + 6.0.

Hello, I think it’s worth adding here the removal of all tokens when deleting a user, to solve the problem described here: https://www.drupal.org/project/simple_oauth/issues/3402385 🐛 Delete tokens along with the user Closed: duplicate

I also have this problem.
I have custom fields on the user entity which are holding states of the user's progress. And that is not viable to revoke the token as I often need to update the user state.

Proposed solution:
What if we generate a checklist on the OAuth Settings page, mirroring the user's fields. Certain checkboxes would be disabled and set as read-only, while the administrator could select which fields should initiate a revocation.
Additionally, we could consider implementing a hook for conditional revocation of specific fields.

To keep things simple, I agree with #50 and #52 that we should have a configuration (checkbox) to disable the default revoking on user updates.

It can be a pain to *asynchronously* having to revoke and update state of a React app on every (minor) user changes !

On the other hand, of course if the user is deleted related tokens should be deleted.

This MR doesn't work. willInvalidateAccessTokens is always true.

That's expected behaviour, see the comment in UserUpdateTokenInvalidationEvent:

This is TRUE by default to maintain BC. To only invalidate when access characteristics have changed, implement an event subscriber to set this to the value of ::haveUserAccessCharacteristicsChanged().

Shouldn't it be a bool for the event dispatcher to work as intended? Not a php expert but passing an empty array for a bool check on the event class seems wrong and I can't see an array length check nor how haveAccessCharacteristicsChanged is updated?

When casted to a boolean an empty array becomes FALSE an a non-empty array becomes TRUE, so this works as intended.

Added unit test, feel free to suggest improvements

Looking at the different use case, I think we should make the implementation in the EntityUpdateHookHandler more flexible, so that the conditions for revoking the tokens can be altered with more ease. The EntityUpdateHookHandler is a final class, so it's not extendible. Making it regular/abstract class and introducing a interface would make the service more flexible and guided. We could even introduce an alterHook to simplify the alterations like e0ipso said in #55 📌 Auth revoke on profile update Needs work .

The EntityUpdateHookHandler is a final class, so it's not extendible.

It doesn't have to be, not every class needs to be extendable or be part of the public API. Conditions can be altered by listening to the event EntityUpdateHookHandler dispatches, without needing to override any services.

We could even introduce an alterHook to simplify the alterations like e0ipso said in #55.

@e0ipso was talking about a hook/event and the MR already contains a new event, so I think we're good here.

Yup, you are right, my bad, I overlooked the fact you can just subscribe on the event, add/alter conditions and make use of the setInvalidateAccessTokens method on the event.

Need to check why the tests are failing, after that we can finally merge this.

I am eagerly waiting for the new release with these updates.

Got bit by this too. Which patch is best to use now?

Is there a workaround or patch that would work on 6.0.x now?

https://git.drupalcode.org/project/simple_oauth/-/merge_requests/99.diff applies on 6.0.x.

We found this issue to be a blocker in some of our projects. Unfortunately the existing MRs were not a solution that would work for us.

Philosophy behind this simplified change
The main idea behind the proposed change is that when tokens should be invalidated will differ between projects and be highly implementation dependent. This can also be seen by the fact there there's a discussion ongoing for about 6 years.

Additionally changes to consumers and users may happen frequently depending on the application that's being built. This makes an event a possibly costly thing to process. From an ecosystem perspective it's also likely undesirable that individual modules start listening to this event and implementing logic for when they think tokens should be invalidated. If a project uses such a module and disagrees the recourse for them to change the outcome of the event may be challenging.

With that in mind there's likely going to be a single override of the logic per project. At that point the event is no longer needed and the absolute simplest implementation is moving the logic into a service that can be swapped out by whatever project that needs it.

Why not change the logic right away
In my personal opinion any change to the logic for invalidating tokens should be considered a breaking change and require a major version bump. If sites are currently relying on the logic to always invalidate and their security sensitive scenario is not included in the proposed defaults (password, roles, and status) then updating may cause them to suddenly have a security issue. Using a major version would be the proper SemVer way to communicate this. Alternatively we go for an implementation which doesn't change the logic but requires purposeful action from a developer to change the logic.

Similarly with these kinds of changes which may be security sensitive we should keep the change as small and focused as possible. This makes it easily reviewable, understandable and in worst case, revertable.

Both PR 53 and 99 suffer from trying to do more than is in scope of this issue. Code quality issues in unrelated files should not be included in a potentially security sensitive PR. Issues like [#2978041] and 💬 [PP-1] Revoke refresh tokens Needs work should also be discussed and resolved in their own issues for that same reason, rather than trying to group them into a single change.

Applied the changes from the MR in one of our projects and together with the CI feedback found I made a few typoes. Fixed those in the MR which means my proposal is ready for review; or for "needs work" in case people disagree with the direction :)

Unfortunately https://git.drupalcode.org/project/simple_oauth/-/merge_requests/99.diff applies but does not appear to work.

@kingdutch it doesn't look like your patch actually addresses this issue. can you provide an example of how we can use the changes in your patch to achieve the goal of this issue, which is to NOT revoke users' access token when their account is updated, unless their password is changed?

Any chance that we will see a merge of merge 99 on 6.x soon?

Hey @bojan_dev
updated the test to be Kernel one, please have a look.

I agree with @kingdutch points, we shouldn't change the default behavior for invalidating the tokens and I rather not introduce a new major version on the 5.x. For 6.0.x on the other hand we are still in beta release and there is more active development, we could introduce a new token invalidation behavior there. The question is if this (token invalidation) change benefits a larger target group, I believe it does, so I would propose making the token invalidation configurable via config settings per case (password change, account blocked, roles change).

I like the simplistic implementation from @kingdutch, so I have used that as base and made it configurable.
The MR 164 covers the following:
- Invalidate tokens by specific actions (multiple can be selected) via settings.
- Available actions can be altered by swapping out/decorating ```TokenExpiryTriggerHandler` service``` + ```Oauth2TokenSettingsForm```.
- Default actions are set so we don't introduce BC.

Todo: add test coverage.

Can somebody please review MR164?

@bojan_dev does one of the official maintainers need to approve MR164? This is an issue I've just stumbled across in my own situation. Thanks for working on it.

@skyejohnson no, the community can review it as well, so if you have the time please take a look.

The new MR works wonderfully for me! I visited /admin/config/people/simple_oauth, set it so that tokens won't invalidate on user save, and validated that saving the user does not invalidate the token.

There might be a bug related to the configuration and how it's saved vs how it's used. I was getting some errors on cron, and needed to make this change to the config that was exported after saving settings in simple oauth. But these changes are unmade when re-saving the form.

diff --git a/config/default/simple_oauth.settings.yml b/config/default/simple_oauth.settings.yml
index 03d6b08ab..44a5defba 100644
--- a/config/default/simple_oauth.settings.yml
+++ b/config/default/simple_oauth.settings.yml
@@ -3,11 +3,7 @@ _core:
 scope_provider: dynamic
 token_cron_batch_size: 0
 invalidate_tokens:
-  consumer_update: consumer_update
-  user_update: '0'
-  user_password_change: '0'
-  user_blocked: '0'
-  user_roles_change: '0'
+  - consumer_update
 public_key: ../keys/public.key
 private_key: ../keys/private.key
 disable_openid_connect: true

@grasmash did you run drush updb? If the config setting invalidate_tokens is giving NULL back, it probably did not run the simple_oauth_update_10001 hook_update.

We could filter those unchecked values (on submit), to make the array cleaner when retrieving values from the invalidate_tokens.

Yes I did run updb and it did execute that function.

sorry for the delay in reviewing MR164! Our part of the world was dealing with a cyclone. As per the previous comments I ran drush updb and then drush cr first.

I unchecked "User update" on here /admin/config/people/simple_oauth and confirmed the token is not revoked!!! Yippee. Well done/

I didn't get around to answering the question asked by grashmash

@kingdutch it doesn't look like your patch actually addresses this issue. can you provide an example of how we can use the changes in your patch to achieve the goal of this issue, which is to NOT revoke users' access token when their account is updated, unless their password is changed?

I did my best to elaborately explain my motivation in https://www.drupal.org/project/simple_oauth/issues/2946882#comment-15747184 📌 Auth revoke on profile update Needs work but I'll see if I can provide a TL;DR below.

The current implementation with simple_oauth is great from a library point of view, because it's absolutely safe in all scenarios. A simple CMS which has editors should assume that any user account change might have access implications and thus invalidating the tokens is safe.

The main problem is that projects that don't fit that mold have no way to change this behavior. By moving the behavior to a service, it's trivial for any project to implement its own service and decide on what the business rules are for their use-case. Any system that this module is going to implement is going to be overkill for simple applications and likely miss rules for complex applications. It's also just quite a bit of code to maintain.

If I look at https://git.drupalcode.org/project/simple_oauth/-/merge_requests/164/ then it's cool that we add a lot of configuration but that's quite a maintenance burden that this module is taking on (and I can already see it does not cover some of my own use-cases; nor would I want the module to).

My counter proposal would be to merge the PR that I proposed where the only thing this module does is provide a way to swap out the invalidation logic (through a simple service provider). More complex logic like proposed in the PR could then easily be moved to a separate contrib module that can come up with the best way to capture various use-cases or make the system smarter.

Oke this is my take on this long discussion:
The goal of this issue is to have the ability to revoke tokens in specific cases. The only 2 options that I see that could potentially land to resolve the issue are MR138 and MR164. Both MR’s are comparable; the only difference is that 164 introduces a config layer which makes it easier for the community to configure the obvious cases mentioned in this issue (without having to write code) and optionally can of course enrich it by themselves. I don’t see the maintenance burden as mentioned, as it’s not a complex implementation.

I'm pretty much up for both MR’s, the only thing that is holding me back to merge MR138: it’s missing test coverage.