Auth revoke on profile update

+++ b/simple_oauth.module
@@ -31,10 +31,15 @@ function simple_oauth_cron() {
+    $pass = $entity->get('pass')->getValue()[0]['value'];

Can be written shorter like:

$pass = $entity->pass->value;

I am pretty sure it's not only on password change that tokens should be revoked.

How will this handle:
1. Email address change, when https://www.drupal.org/project/email_registration is enabled.
2. Will it handle correctly the case of account being deactivated or deleted. Should it?
3. Any other cases I am not thinking of now.

Thank you for the contributions to the issue queue.

In this case @ndobromirov is correct. We need to revoke/reset tokens on user update for all the reasons stated above plus others (for instance a user may be stripped of an important role after the free trial period ends).

I agree this is not obvious when you first think about it. Having an issue on this will help clarify for future users.

My team has recently faced the same issue and I tend to agree with @m.abdulqader's approach. Let me explain the issue we've experienced and why we believe that this behavior should change.

We do have decoupled web app (React.js + Drupal 8). On the Drupal side we use Group module for managing user permissions to certain content within the system. Every time a manager adds or removes a user from a group, user save is getting triggered from the Group module (for sake of flushing necessary caches, rebuilding perms, etc etc). This behavior causes the user who was added or removed from the group having to log out and log in again in order to become authenticated again.

The problem of the given use case is that frontend doesn't know when an action happens on the backend, so I can't think of any good way to request a token from the frontend after it was revoked from the backend, apart from adding a leading token refresh request before every request to the backend, which seems like a big overhead.

I've been thinking about this issue for a while, and the conclusion is that the token should be expired only for security reasons, like:

1. When a password has been changed (replicating core's behavior)
2. When user account got blocked (obviously, no more access to the system)
3. When roles have changed (a good example in #10)

In all other scenarios the tokens should be left along. The preference here would be to adjust the cases to revoke the token when we know there are more, rather than swatting a fly with a sledgehammer. So unless it's a potential security issue, frontend should have an ability to request a new token in a controlled way after an action had been taken.

This could be a public API of the module to expose hooks / events / plugins to offload the invalidation to 3rd party modules.

This way email_gregistration for example will be able to handle this on it's own.

The module can provide 1 generic (kill all) plugin as it's doing now with flush on any update on user and many other smaller plugins that would allow for a more targeted invalidation.

This is like best of both worlds - module will work by default, but anyone could tune it to fit for his needs.

+++ b/simple_oauth.module
@@ -34,7 +34,23 @@ function simple_oauth_entity_update(EntityInterface $entity) {
+    if ($password_changed || $account_blocked || $roles_changed) {
+      $collector->deleteMultipleTokens($collector->collectForAccount($entity));
+    }

I like the idea, but not the implementation details. There is still no way of 3rd party modules extending or narrowing the conditions listed in the code.

I think that we are looking at it from a narrow perspective. There are multitude of scenarios that we are not taking into account, in particular all the unknown scenarios that can crop up from a combination of contribs. Limiting the revoke to a given set of actions will leave some important scenarios out, leaving the site exposed to a security thread.

This is a big problem.
When a user change his nick_name, or upload avatar image, after then, client app redirect to login page, shit.

I think the right way is that, we should not revoke the tokens until we can revoke them at the right time.
we can‘t revoke tokens rudely when user entity was updated.

This one is a separate feature request. #15 I think you are referring to #2946882: Auth revoke on profile update.

My mistake - same issue...

I think the right way is that, we should not revoke the tokens until we can revoke them at the right time.
we can‘t revoke tokens rudely when user entity was updated.

@164713332@qq.com I'm open to review patch suggestions. Just take the security considerations of #14 into account.

Isn't it the case that the approach taken in this module is currently far more restrictive to that of core's session authentication. Is there a reason why this needs to be the case? If it would be insecure for simple_oauth not to invalidate logins on any account save then does that not mean that core is likewise insecure?

I would have though that the best solution would be for core to have a login invalidation API and simple_oauth could have some kind of login system plugin and as such respond events that cause invalidations in contrib without the contrib modules having to write code specifically for simple_oauth.

Or more simply there could be a hook that gets invoked in core's SessionManager::delete(). That simple_oauth could implement and invalidate logins That should work wherever core invalidates a user and would actually be more complete because it would also respond to events that are not account saves. Come to think about it SessionManager is a service so I think I'm right in saying that simple_oauth could add a higher priority service extending SessionManager, override that function and invalidate the tokens before calling parent::destroy(). I don't know if it's cool for contrib modules to take over services, but it's a thought.

In the mean time, I have to agree that the current regime renders this module fairly unusable without patching.

I see your concerns.

#16

When a user change his nick_name, or upload avatar image, after then, client app redirect to login page, shit.

#18

In the mean time, I have to agree that the current regime renders this module fairly unusable without patching.

I actually share them, but I don't want to deal with a security issue if it comes :-P

I'm open to the changes as long as you can get someone from the security team to validate the patch in #11.

> When a user change his nick_name, or upload avatar image, after then, client app redirect to login page, shit.

Actually, if you have that behavior then you are not properly implementing refresh tokens in your app. Those do not expire, only the auth tokens do, so all you have to do is get a new auth token using the refresh token.

Auth tokens are meant to be short-lived anyway, like a few minutes and you should then regularily refresh them as they are actually meant to usable without any further validation by anyone who trusts the site that genrates them.

That said, in that regard, when those tokens are used correctly (as short-lived things that need to be frequently revalidated) invalidating them is actually kinda pointless *but* we should look into invalidating the refresh tokens if really necessary (e.g. on password change).

#20 I totally agree that client code should be able to deal with unexpected revokes using the refresh token grant. However, from a back-end perspective I can agree that an avatar update does not necessarily qualify as a reason to revoke existing tokens.

My point is that some properties in the user object do qualify as a reason to revoke tokens. Contributed modules add enough variability that it makes me uneasy having a closed list of properties that we should check for revokability. That's why I pushed back on this feature.

My comment in #19 acknowledges that I don't know the constraints all clients may have, and that the claims that some properties should not require a revoke have a point. But on the other hand I also don't know that a closed list of properties is secure enough.

@Berdir do you have any opinion about how secure #11 would be?

*but* we should look into invalidating the refresh tokens if really necessary (e.g. on password change).

That's another very good point. I think this has some overlap with the logout feature in #2945273: [PP-1] Revoke refresh tokens and #2974963: Link refresh tokens to the user they will identidy for query filtering.

Edit: which I realize you already mentioned in https://www.drupal.org/project/simple_oauth/issues/2974963#comment-12626961 :-D

I have also been dealing with this a little while now.

I have came to the same conclusion as Spleshka in #11. Updating the user profile picture does not qualify to revoke the token.

Now the solution in frontend is to ask request a new token right away after updating user data (because we know the token will be expired). Also my preference is that we do it like this for now than dealing with a some kind of security issue. :)

I have came to the same conclusion as Spleshka in #11. Updating the user profile picture does not qualify to revoke the token.

Do you suggest to check for the updated fields and see if there only was a change in the profile picture to skip revoking the token? That seems very ad-hoc and error prone, but I'm open to review a patch in that direction if you think it's best.

Another option we introduced on a project was fully make the API hidden behind oauth. At that point all requests needed tokens. Once you get 403 on your normal call - you are triggering the refresh token. It's very transparent from clients side, as they are integrating that on communication level primitive to refresh the token and repeat the previously 403 call.

I got the same problem.

When I update my user, the access token is being revoked and the refresh token can't be used.

As you can see it occurs this error when use refresh token, that's sadness.

if ($this->refreshTokenRepository->isRefreshTokenRevoked($refreshTokenData['refresh_token_id']) === true) {
    throw OAuthServerException::invalidRefreshToken('Token has been revoked');
}

@nginex what would be your ideal output given all the context in this issue?

At the moment, we have a situation where we delete related access tokens on user updates.

A refresh token will not work when access token was deleted.

I propose to revoke access tokens instead of delete them (tested on my local app).

Example of the method:

/**
 * Revokes multiple tokens based on ID.
 *
 * @param \Drupal\simple_oauth\Entity\Oauth2TokenInterface[] $tokens
 *   The token entity IDs.
 */
public function revokeMultipleTokens(array $tokens = []) {
  foreach ($tokens as $token) {
    $token->revoke();
    $token->save();
  }
}

In that case, existing access tokens will not work but it will be possible to use a related refresh token to get a new access token.

I think, situation like changing a user password should be handled by the app which use oauth. It can be simple re-login with new credentials to get new refresh and access tokens.

Probably, it makes sense to remove all tokens when a user gets blocked.

Besides, I've noticed when generate a new access token the old refresh token will not work (error message is Token has been revoked).
So it makes sense to remove all old refresh tokens (user related) after generating a new one.

I could provide a patch for that.

@e0ipso what do you think?

@e0ipso what do you think?

I know I'm repeating myself here. But you only seem to mention updates to the user of things that core does. Drupal can be extended in many ways, and the password is not the only sensitive & identifying piece of information possible.

In that case, existing access tokens will not work but it will be possible to use a related refresh token to get a new access token.

I'm fuzzy about the implementation, it's been a while.

Will that bypass safety guards? Imagine this scenario: user A is a power user with lots of privileges and has a refresh token. An admin updates to user A to take away some of those privileges. User A is in possession of a refresh token and gets an access token. Will this access token grant more access than what he would have using the password to generate an access token?

I'm pretty sure that the refresh token still worked for me, mostly because they are not assigned to the user and can't be found. Maybe you are using some patches to change that, which causes the refresh tokens to be deleted too?

Unfortunately, I stumbled upon this as well. Had to overcome by "decorating" the call to hook by custom conditions.

WARNING, hacky approach below (requires diff module):

/**
 * Implements hook_module_implements_alter().
 *
 * @internal
 */
function HOOK_module_implements_alter(array &$implementations, string $hook): void {
  if ($hook === 'entity_update') {
    // Remove explicit invocation of the "simple_oauth_entity_update()"
    // because it unconditionally removes access tokens for the saved
    // account.
    unset($implementations['simple_oauth']);
  }
}

/**
 * Implements hook_entity_update().
 *
 * @internal
 */
function HOOK_entity_update(\Drupal\Core\Entity\EntityInterface $entity): void {
  // Make call to "simple_oauth_entity_update()" conditional.
  if (empty($entity->original) || !$entity instanceof \Drupal\user\UserInterface) {
    simple_oauth_entity_update($entity);
  }
  else {
    $safe_field_names = [
      'field_pending_checks',
    ];

    foreach (\Drupal::service('diff.entity_comparison')->compareRevisions($entity, $entity->original) as $key => $item) {
      // The key has the following format: "ID:ENTITY_TYPE:FIELD_NAME".
      /* @see \Drupal\diff\DiffEntityParser::parseEntity() */
      list(, $field_name) = explode('.', $key, 2);

      if (!in_array($field_name, $safe_field_names, TRUE)) {
        // Remove user's access tokens once at least single *sensitive*
        // field has been updated.
        simple_oauth_entity_update($entity);
        break;
      }
    }
  }
}

This approach brings these benefits to me:

1. I don't need to wait in fear the time when a patch from this thread will stop applying.
2. I can build my own logic around the simple_oauth_entity_update().

Nevertheless, I don't like such crutches :(

I ran into the same issue having the same scenario than the one described in #11. I ended up with a solution like the one proposed in #24 which seems cleaner to me.
Specifically, I created a route_enhancer with a low priority to be able to modify the jsonapi route options and requirements. Ideally, I'd have liked jsonapi to provide different access options or an extendable solution.
I think removing access token on any Account update is a reasonable approach for the different reasons pointed by @e0ipso. The refresh_token is a good backup.

I personally think, that patch #12 is the best solution. We are using it for quite a long time and it covers all our need we have for security because it cover the cases, that really imply on the user right.

Thanks for the feedback @ayalon. I agree with your comment, depending on the specifics of the site (maybe most of the sites) the patch will solve this problem. However, I also want to caution against the optimism of #32. While it is true that the patch will help in some scenarios, it will not be secure in all scenarios. Make sure you understand the implications before applying the patch to your site.

I added to the patch the actual check "whether the user's email has been changed?". Please check, maybe it will be relevant. Thanks.

@e0ipso: If I understood you correctly there won't be a new release because of the security implications? So the only way to address the issue is to apply a patch if one is ready to make a trade off with security.

That's correct TipiT.

Is this discussion at a point where it would make sense for me to pester the security team to take a look at the patch in #11?

I also think #34 adds some value to #11.

Diff between the two included.

Is this discussion at a point where it would make sense for me to pester the security team to take a look at the patch in #11?

I'd love to have someone from the security team show a personal opinion here.

Issue submitted to the security team on Aug. 14.

Marking as needs work and bumping version; I think postponed/maintainer isn't quite accurate given the breadth of the issue; input from the security team is only one component and I think we can work this out here.

I came upon this ticket in testing a native app that utilizes OAuth with Drupal. Like some users have noted above, I've found that a user who is blocked can use a previously-granted refresh token to obtain a new access (and, refresh) token. This is because ExpiredCollector::collectForAccount() only collects access tokens, not refresh tokens, to invalidate. However, the resulting access token later fails validation in SimpleOauthAuthenticationProvider::authenticate(), which limits the security impact. The issue #2978041: Blocked user shouldn't get access token nor refresh token is related in so far it properly identifies this issue however I think it would be better addressed by revoking the refresh token on the account lifecycle (e.g., when it's blocked) rather than adding additional complexity by checking the user status at runtime. That way, the revocation logic is all handled in one place - the entity update hook - rather than during token validation.

I might submit that the most secure but also flexible solution here would be to make the default behaviour what it is currently - tokens are revoked on account update - but use a whitelist instead of blacklist. We can ship a sensible default based on Drupal's core functionality, and then site owners can expand the whitelist. To accomplish this, there is work on a core dirty fields API at #2862574: Add ability to track an entity object's dirty fields (and see if it has changed) which looks promising.

As it stands now I am not really sure why refresh tokens are excluded from the collection for invalidation. If the goal is presumably to lock the user out from consumers after a profile (e.g., password) change, leaving the refresh tokens valid would mean no such experience on many clients, which will take a 401 response as a prompt to refresh. The only situation in which the client would subsequently fail would be if the user was blocked - and only when attempting to use the access token.

I'm attaching a patch and test to include the refresh tokens in the collection - and I think the maintainer can weigh in on the whitelisting idea?

File uploader was being wonky. Adding full patch here.

Adding a flag to avoid BC break on collecting; you can now opt-in to collecting refresh tokens as well, which is what we do in the entity lifecycle hook. Let's see how this fares.

Hi bradjones1,

I've made the changes proposed in #42 in the simple_oauth 3.16 manually (right now I need to use 3.x version, 4.x is not an option).
I've found this doesn't work the same way as #2978041: Blocked users may use refresh tokens.

While #18 patch from #2978041 issue patch avoids creation of new access and refresh tokens using password grant_type (with user credentials), #40 patch removes all user access and refresh tokens when the user is updated, but it keeps generating access and refresh tokens using password grant_type. It is true that this tokens can not be used to get access to other api endpoints, but the response on those endpoints is an 500 server error. This makes it difficult to determine, for a consumer app, if the 500 it's caused by a server error or by invalid token.

I'm answering #40.

I came upon this ticket in testing a native app that utilizes OAuth with Drupal. Like some users have noted above, I've found that a user who is blocked can use a previously-granted refresh token to obtain a new access (and, refresh) token. This is because ExpiredCollector::collectForAccount() only collects access tokens, not refresh tokens, to invalidate. However, the resulting access token later fails validation in SimpleOauthAuthenticationProvider::authenticate(), which limits the security impact. The issue #2978041: Blocked users may use refresh tokens is related in so far it properly identifies this issue however I think it would be better addressed by revoking the refresh token on the account lifecycle (e.g., when it's blocked) rather than adding additional complexity by checking the user status at runtime. That way, the revocation logic is all handled in one place - the entity update hook - rather than during token validation.

The #42 patch does is to remove all access and refresh token when the profile is updated. In my project what I'm doing is to remove all the user tokens just if the user is updated and its status is changed from active to bloqued. In that situation I remove the tokens, but I don not remove the tokens if the user updates any other field. Right now #42 remove all the access and refresh tokens when the user updates any custom field (like favorite color).

As it stands now I am not really sure why refresh tokens are excluded from the collection for invalidation. If the goal is presumably to lock the user out from consumers after a profile (e.g., password) change, leaving the refresh tokens valid would mean no such experience on many clients, which will take a 401 response as a prompt to refresh. The only situation in which the client would subsequently fail would be if the user was blocked - and only when attempting to use the access token.

If all the refresh tokens are removed when the user is updated (for instanve changing its favorite color), the user will need to introduce user and password again. So it is not user friendly.
If the refresh tokens are not removed it allows to:

• Get new access and refresh token with an old refresh token
• Have the same user logged in different devices:
  • When the password grant_type is used it generates an access and a refresh token
  • When the password grant_type is used again it generates a new access and a new refresh token. The old refresh token is not invalidated.
  • The refresh tokens are invalidated when the refresh_token grant is used.

I think that the objective of this issue is not easy to do, because there are many scenarios to take into account. I also think that #2978041 issue is not a duplicate of this issue.

Long post...

bradjones1, what do you think?

If all the refresh tokens are removed when the user is updated (for instanve changing its favorite color), the user will need to introduce user and password again. So it is not user friendly.

@bradjones1 I'm curious about your input on this quote from #43.

Thanks everyone for evolving the conversation here.

@e0ipso - Gladly.

I think the goal of this issue should be to effectively fix the incomplete logic that's already in the module today regarding revocation. As it stands, the hook intends to revoke tokens for a user whose account has changed. The patch I've proposed broadens this to include refresh tokens, as revoking access tokens alone does not achieve what I believe to be the goal of doing the invalidation at all.

Furthermore, in the case of a user being blocked, the current code revokes access but not refresh tokens; this is explored in #2978041: Blocked user shouldn't get access token nor refresh token, which is why I marked it as a duplicate.

It is certainly a worthwhile endeavor to seek to limit the conditions on which an account update triggers token invalidation. @espurnes's point about updating a favorite color field is well-taken, but I think it is a good follow-up vs. blocking this issue. Basically I think this issue should be narrowly focused on following through on the intent of the module's current behavior. Then, something like a backwards-compatible whitelist/blacklist option (as I discuss in #40) could be implemented as a new feature.

I think this note from the related issue helps illustrate why these are actually duplicates:

I think this is not a duplicate issue. This avoids the generation of access and refresh tokens for a blocked user, the other issue removes existing access and refresh tokens on user update.

It's true that on refresh, there is no check on account status; that's the genesis of the issue. However if we revoke all tokens on account block, there's nothing to refresh. We don't need to add special handling if we effectively force re-auth in this ticket. There would simply be no valid pre-block refresh tokens left to use.

I think we could hash this out today/tomorrow at DrupalCon sprints?

Hi @bradjones1,

Thank you so much for your commits and dedication to solve this issue :) . But I'm not agree with the fact that the two issues are duplicates.

The 2978041 - Blocked users may use refresh tokens until some days ago was called 2978041 - OAuth Token gerenrated for Blocked User in Drupal System. I think the previous title match better the aim of the issue.

The problem is that a blocked user can generate access an refresh tokens using user and password (grant_type password).

If I use '/oauth/token' endpoint to generate the oauth token for the blocked user in Drupal system with grant type as 'password', the oauth token is generated for the user which should not be the case.

So as I mentioned before the current issue does not solve this behaviour. The current issue is focused on remove access (and maybe refresh) tokens when the user profile is updated. But without user status validation on the the /src/Controller/Oauth2Token.php, if a blocked user use the password grant_type will get new access and refresh tokens.

So I'm still believe that these two issues are not duplicates.

The white/black list seem a good aproach to deep in. But I think the current patch can't be commited without this implementation.

thanks :)

@bradjones1, @e0ipso what do you think?

I tried the patch #42 from @bradjones1 which seems to also delete refresh tokens, even though $includeRefresh = FALSE should avoid that in my understanding.

I don't understand why the solution from @Spleshka in #11 is not acceptable, because everybody seems to agree that the cases of revoking the tokens are if ($password_changed || $account_blocked || $roles_changed), which are clearly seen in the patch. The solution is also very simple in this case.

So would it be enough, or get as closer to the solution, if the security team would agree on the cases of revoking tokens?

I'll try to take a look at this, this week, as time allows. For what it's worth, from my prior conversations the security team doesn't really wade into this; it's up to us as the subject-matter experts/module contributors to figure out what's best. I think this is use-case specific enough that this would not prompt/has not yet prompted a security release, so.

What's the process with this? If I update [add] a value on a custom field, auth is revoked - which is very annoying.

Hi, sorry for my bad english.

I have created a patch with a portion of #2946882-42: Auth revoke on profile update.
This portion is very useful for reusing the ExpiredCollector service without reinventing the wheel and I think could be committed because doesn't change current behavior of simple_oauth

The interdiff_42-50 is the portion of #2946882-42: Auth revoke on profile update removed. I think that we shouldn't change the default behavior of a stable release

Maybe we could add a configuration for the "opt-in", but the original behavior must be maintained by default or we risk to add a breaking change on a stable product.

Totally agree with @TipiT. We were encountering this issue with simple profile field updates. A user could make updates only once and then they would have to refresh their page to save again. We are currently using #11 patch as it solves our issue enabling the issue to save as many times as needed while editing their profile. We also tried patch from #50 but this did nothing for us on our end.

Hello everyone,

I allow myself to add my small contribution, and the context of use in my case.

Current project context: I develop an e-commerce, in full headless (angular), and users are not created from the front, but from a webservice that takes care of creating or updating a user. User profiles are not editable from the front. Everything has to go through a customer service. So it's not possible for me to trigger the request for a new token.

Updates can take place at any time, potentially several times a day. It is therefore inconceivable, that a shopping cart of a product triggers for example, an authentication.

Couldn't we have a configuration page, which would allow us to select the "fields" to be tested to detect a possible change, thus triggering a revocation of the tokens? By default, all fields would be unchecked, implying a test. The ones we check would be ignored by the test and therefore would not invalidate the tokens.

Finally, as of today, the only solution is to add an patch in the composer.json, for security reasons I can understand?

If I can bring you a vision, or help, it will be with pleasure.

Sincerely,
Sébastien

#11 seems right. We work with group and have same issue. His patch work correctly.

I have a new use case similar to @sebyogaf's in #52 which means I'm not sure the "everybody seems to agree" statement from @TipiT is necessarily true:

I don't understand why the solution from @Spleshka in #11 is not acceptable, because everybody seems to agree that the cases of revoking the tokens are if ($password_changed || $account_blocked || $roles_changed), which are clearly seen in the patch. The solution is also very simple in this case.

The use case is a de-coupled / headless implementation where devices only have the authenticated role. Once they subscribe (via a 3rd party) we eventually receive a callback push notification confirmation which tiggers the role change from just authenticated to subscribed.

I would not expect the end user to re-authenicate once this subscription has been confirmed and can think of other scenarios where the role changes behind the scenes (subscriptions upgrades? promotions based on a conditions? etc).

Maybe we should have a whitelist for what could trigger token invalidations? Any other ideas on how to handle this much appreciated.

I am open to add a hook/event to the module that can be implemented in custom code. This hook would help determine if the tokens should be invalidated or not.

Would that be acceptable to this audience?

+1, Thanks @e0ipso, definitely would seem a big improvement from my point of view.

+1, thanks @e0ipso, it would be helpful for custom implementations that use the module, I think that the module could provide an admin interface where you choose in which cases you want revoke users tokens on accounts updates. This would cover perfectly the scenario mentioned in #54

+1

+1

Thanks!

Can anyone confirm the patch still works for D9.2x and module v 5.x? Composer fails to apply the patch / or is already patched but I still have the issue.

https://www.drupal.org/files/issues/2020-06-10/interdiff_42-50.txt (Prevents auth revoke on profile update)
   Could not apply patch! Skipping. The error was: Cannot apply patch https://www.drupal.org/files/issues/2020-06-10/interdiff_42-50.txt

Access token has been revoked in Drupal\simple_oauth\Authentication\Provider\SimpleOauthAuthenticationProvider->authenticate() (line 81

I agree that revoking tokens anytime a user updates some account field is a terrible UX.

I applied @Sleshka #11 patch, which covers the main security concerns, and could be the default behavior, leaving devs free to implement more strict or laxer policy if they have to.

@e0ipso hook proposal would be a good solution, then, since there are uncountable different cases. For instance #54 example, where a user is given a new role after subscribing, is very common.

Thanks all for the amazing job done !

Ran into this myself today and tried to synthesize the thrust of #11 which everyone likes, with the idea for an event to determine this by business rules in #55. We also need to preserve the current behavior, just in case someone is depending on it.

Reviews welcome.

MR is green which means no regressions... need test coverage to test the "access characteristics" determination and event subscriber.

https://git.drupalcode.org/project/simple_oauth/-/pipelines/6515/test_re...

The current state of the MR doesn't invalidate refresh tokens after a user's roles have changed. Should I commit that to the issue branch or do you think that's a BC break? If so, we should create follow up issues to change those things.

This event is quite unusable ATM since the updated user object is not passed with the event object. I'll add that to the MR.

Disagree that it's unusable without the user passed (you may only care about changing the default behavior) but agree that including the user entity is a positive change.

Still needs tests.

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!

Never mind, just saw that the issue is for v5, but I am using v6.

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

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.

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? Looks like none apply against 6.x.

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: Blocked user shouldn't get access token nor refresh token and #2945273: [PP-1] Revoke refresh tokens 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 :)

It looks like https://git.drupalcode.org/project/simple_oauth/-/merge_requests/99.diff is not intended to change the behavior, but rather provide a mechanism to override the default behavior. You'd need to implement an event subscriber like this:

namespace Drupal\gg_pedal\EventSubscriber;

use Drupal\simple_oauth\Event\UserUpdateTokenInvalidationEvent;
use Symfony\Component\EventDispatcher\EventSubscriberInterface;

/**
 * Event subscriber for user token invalidation events.
 */
class UserTokenInvalidationSubscriber implements EventSubscriberInterface {

  /**
   * {@inheritdoc}
   */
  public static function getSubscribedEvents(): array {
    return [
      UserUpdateTokenInvalidationEvent::class => 'onUserUpdateTokenInvalidation',
    ];
  }

  /**
   * Reacts to the UserUpdateTokenInvalidationEvent.
   *
   * @param \Drupal\simple_oauth\Event\UserUpdateTokenInvalidationEvent $event
   *   The event object.
   */
  public function onUserUpdateTokenInvalidation(UserUpdateTokenInvalidationEvent $event): void {
    // Get the user entity from the event.
    $user = $event->getUser();

    // Set the invalidation flag based on whether access characteristics have changed.
    $shouldInvalidate = $event->haveUserAccessCharacteristicsChanged();
    $event->setInvalidateAccessTokens($shouldInvalidate);

    if ($shouldInvalidate) {
      // Optionally, you can also invalidate refresh tokens when access characteristics change.
      // Uncomment the next line if you want to invalidate refresh tokens as well.
      // $event->setInvalidateRefreshTokens(TRUE);
      \Drupal::logger('gg_pedal')->notice('Tokens invalidated for user @uid due to access characteristic changes.', ['@uid' => $user->id()]);
    }
    else {
      \Drupal::logger('gg_pedal')->info('No token invalidation needed for user @uid.', ['@uid' => $user->id()]);
    }
  }

}

@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 + 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

Error:
php TypeError: in_array(): Argument #2 ($haystack) must be of type array, null given in in_array() (line 58 of /code/docroot/modules/contrib/simple_oauth/src/TokenExpiryTriggerHandler.php) #0 /code/docroot/modules/contrib/simple_oauth/src/TokenExpiryTriggerHandler.php(58): in_array('user_update', NULL, true)
#1 /code/docroot/modules/contrib/simple_oauth/simple_oauth.module(34): Drupal\simple_oauth\TokenExpiryTriggerHandler->handleUserUpdate(Object(Drupal\user\Entity\User))
#2 [internal function]: simple_oauth_user_update(Object(Drupal\user\Entity\User))
...

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

Thanks all for the work on this! @bojan_dev Any preference for type of test/where? I believe just test coverage that saving AccountInterface or ConsumerInterface triggers the token deletion right, but the idea that a service can be overridden would not need testing, is that your expectation?

Moving back to needs work per #111

For those stumbling across, for now we just did a module implements alter and unset simple oauth implementation to roll our own logic matching that of the project

if ($hook === 'entity_update') {
  unset($implementations['simple_oauth']);
}

Thanks all for the work on this! @bojan_dev Any preference for type of test/where? I believe just test coverage that saving AccountInterface or ConsumerInterface triggers the token deletion right, but the idea that a service can be overridden would not need testing, is that your expectation?

There is a new service introduced TokenExpiryTriggerHandler, all the methods need to have test coverage, checking if they are triggered on specific events and have the right parameters. For example: handleConsumerUpdate() is triggered on consumer update and provides the associated consumer object.

MR164 worked for me. I tried MR138 first but that didn't work. Didn't look into why. Sorry lol, was like why does it do this me! Took a bit to find this issue!

Rewriting the patch.
Providing a patch for version 5.2.x for those who wish to use MR-164 with an older version.

Thanks.

I like both approaches (MR138 and MR164) I think the intent is very similar. MR164 tries to be a bit more helpful out of the box.

If I have to choose between the two approaches, I'd go with MR138. I think it's less prescriptive, which IMO in this scenario is good (we can't possibly cover all use cases).

My preferred way wold be to have MR138 merged in, and then a separate contrib `simple_oauth_user_updates` that decorates the new service to implement MR164 using form alters and such. If we go this way, we could update MR138 to include a link in the settings form, pointing to the `simple_oauth_user_updates` project page to surface it's existence to users.

Does that sound like a good compromise?

We went with MR138. If you need token invalidation to be triggered under different conditions, you can replace the TokenExpiryTriggerHandler service.

Hi @bojan_dev! Friendly reminder, seems like you forgot to update the credits in the contribution record.

This has been a long-standing discussion with multiple MRs, so it’s hard to determine credit fairly. I’ve updated the credits and tried my best to be fair. If you believe you should be credited, please don’t hesitate to reach out.