Account created on 24 January 2014, about 11 years ago
  • Freelance Drupal developer at DICTU 
#

Merge Requests

More

Recent comments

🇳🇱Netherlands bojan_dev

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.

🇳🇱Netherlands bojan_dev

I find it not clear how to reproduce your issue, so it's hard for me to figure this one out. The hook_update: simple_oauth_update_8604 tries to migrate existing (old) scope settings from your consumers to the new dynamic scopes, so if this didn't work you could always just manually configure your dynamic scopes and consumers.

🇳🇱Netherlands bojan_dev

@yobottehg could you please check if the fix in MR 177 resolves the issue?

🇳🇱Netherlands bojan_dev

I think we should add some extra checks in that hook:

  • If role is not NULL.
  • If scope already exists.
  • If scopes are already referenced on the associated consumer.

This way we can rerun the hook_update.

🇳🇱Netherlands bojan_dev

Could you please check on which line the id() is giving null?

🇳🇱Netherlands bojan_dev

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

🇳🇱Netherlands bojan_dev

It appears the following change in thephpleague/oauth2-server is the reason for the BC: https://github.com/thephpleague/oauth2-server/pull/1094.

finalizeScopes is now being called in the RefreshTokenGrant, which does not set the $userIdentifier, this means that no scopes are being returned. This has impact on all grant types that support refresh tokens. Sorry that I missed this, also I have read your comments @m.stenta on 🌱 Move to thephpleague/oauth2-server 9.0 Active , those are valid points I will definitely make a major version release next time.

Merging MR !176 and will roll out a new stable release.

🇳🇱Netherlands bojan_dev

I was assuming you were only using the grant types that are provided by simple_oauth, based on that assumption that would mean that only the Authorization code grant type supports refresh tokens. But I see you are using the Password grant, which is not supported by simple_oauth: 6.0 due to the following reason: https://datatracker.ietf.org/doc/html/draft-ietf-oauth-security-topics#s...

You could take a look in 5.2 which still supports the password grant.

🇳🇱Netherlands bojan_dev

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

🇳🇱Netherlands bojan_dev

@drunken monkey thank you for you input and you have some valid points, I could just create a custom plugin extend the "search_api_solr_suggester" and make those adjustment like you said. But trimming by default spaces in the "search_api_solr_suggester" plugin feels to me like the query is specific for some Solr lookup implementations, vs just allowing spaces and having the query to the Solr suggester raw as possible.

If you still find this an edge case, we can close both issue's and I will do a project specific implementation for it.

🇳🇱Netherlands bojan_dev

Do you mean it's missing from /oauth/userinfo route/endpoint?
Did you try to update to beta10?

Take a look on the following KernelTest, which illustrates how to retrieve user info from a token: https://git.drupalcode.org/project/simple_oauth/-/blob/6.0.0-beta10/test...

🇳🇱Netherlands bojan_dev

Closing issue because simple_oauth 8.x-4.x is EOL.

🇳🇱Netherlands bojan_dev

I can't reproduce this, it's not possible to refresh the token for any grant type, it is only applicable for the "Authorization Code".
Can you please provide more info about the configured consumer, scope and authorize request?

By the way: please don't set the issue status to reviewed, a peer review should not be done on your own work.

🇳🇱Netherlands bojan_dev

I agree with e0ipso this hook_alter comes from anonymous_login and makes no sense for people that don't use the anonymous_login module. That hook should be documented in the anonymous_login, for people that use both simple_oauth + anonymous_login modules.

🇳🇱Netherlands bojan_dev

In 6.0 we migrate some tests to Kernel tests, but some are still needed in browser, e.g: when dealing with Authorization Code Grant there is a case where a grant form is presented in the authorization flow.

🇳🇱Netherlands bojan_dev

No activity for a long period, closing issue for now.

🇳🇱Netherlands bojan_dev

There is for a long period no activity on this feature request, closing it for now.

🇳🇱Netherlands bojan_dev

Closing issue: task is pretty old and no patch was provided.

🇳🇱Netherlands bojan_dev

@skyejohnson I made a new release for this composer fix 6.0.0-beta10, so you should be able to do a regular update (without patching), "drupal/simple_oauth": "^6.0" should be set as requirement in your project composer.json.

🇳🇱Netherlands bojan_dev

I wonder if we really need that dependency specified, because it's already a dependency in: steverhoades/oauth2-openid-connect-server, see: https://github.com/steverhoades/oauth2-openid-connect-server/blob/v3.0.1....

composer update drupal/simple_oauth -W should do the trick or am I missing something?

🇳🇱Netherlands bojan_dev

You need to update simple_oauth with dependencies, thephpleague/oauth2-server needs to be 9.2, for more info see: 🌱 Move to thephpleague/oauth2-server 9.0 Active and https://git.drupalcode.org/project/simple_oauth/-/jobs/4456948.

🇳🇱Netherlands bojan_dev

How did you verify that the tokens are invalided for those specific entity updates?

In hook simple_oauth_entity_update the tokens get deleted if the consumer or user entity get's updated. In 📌 Auth revoke on profile update Needs work we are trying to improve this logic with configurations.

🇳🇱Netherlands bojan_dev

This can only occur if the config "simple_oauth.settings" or the setting inside "token_cron_batch_size" isn't available. I wouldn't make the limit argument nullable, I rather see it addressed in the if statement on simple_oauth.module:30.

🇳🇱Netherlands bojan_dev

If this still is occuring on the latest version, feel free to reopen the issue, closing it for now.

🇳🇱Netherlands bojan_dev

Thanks for sharing this @chfoidl, I dropped some comments and I wonder if it's even possible to have test coverage for this.

🇳🇱Netherlands bojan_dev

To be in line with the OAuth2 spec: https://datatracker.ietf.org/doc/html/rfc6749#section-3.3
I have made the token endpoint require the token scope parameter when there is no default scopes available on the client/consumer when dealing with the client_credentials.

Could you please review my last commit?

🇳🇱Netherlands bojan_dev

If there are no scopes requested and no scopes are set on the consumer with the client credentials, in 5.2 it will fallback on the default user (set on the consumer), with 6.0 you won't have any access. We definitely need to address this in 6.0, I think we should grant all scopes that support client credentials if no scopes are set on the request and consumer.

Regarding the tiny module, it would be interesting to have some testing/debug submodule available that does this. I would be open to add this to the module.

🇳🇱Netherlands bojan_dev

bojan_dev made their first commit to this issue’s fork.

🇳🇱Netherlands bojan_dev

The password credentials grant is no longer supported in 6.0, see: https://www.drupal.org/node/3315400
Did you mean simple_oauth 5.2?

🇳🇱Netherlands bojan_dev

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.

🇳🇱Netherlands bojan_dev

bojan_dev made their first commit to this issue’s fork.

🇳🇱Netherlands bojan_dev

Nice work idebr! I merged the one which fixes the cspell job.

🇳🇱Netherlands bojan_dev

I went ahead and made the default scopes limit the allowed scopes per consumer. Can somebody please review?

🇳🇱Netherlands bojan_dev

I have no feedback, the implementation looks great, you have thought well about the upgrade path, merged ;)

🇳🇱Netherlands bojan_dev

This is a valid point, we should definitely reintroduce this feature. The question is how? We could add an extra responsibility on the default scopes, so it limits the scopes per consumer or we can introduce an additional field where we can specify allowed scopes. I’m open for suggestions.

🇳🇱Netherlands bojan_dev

I had a quick peek and let me first say that it is great work what you did here @tstoeckler. Unfortunately I jammed with work, so an extensive review I can't give in the short term. Any dev's that what to speed up the process, feel free to review.

🇳🇱Netherlands bojan_dev

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

Production build 0.71.5 2024