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 review .

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).