Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

save_refresh_token may delete valid refresh_token and break refresh functionality #404

Open
3 tasks done
drzraf opened this issue May 18, 2022 · 3 comments · May be fixed by #406
Open
3 tasks done

save_refresh_token may delete valid refresh_token and break refresh functionality #404

drzraf opened this issue May 18, 2022 · 3 comments · May be fixed by #406
Assignees
Labels
bug enhancement Issues & PRs related to new features. needs analysis Issues needing further investigation to the cause and/or change required.
Milestone

Comments

@drzraf
Copy link
Contributor

drzraf commented May 18, 2022

Describe the bug
save_refresh_token() is called after the initial request. In our case of interest, the payload bring a request_token field (in the case of Google, this happens if access_type=offline.
Such refresh_token may not have an expiration time.

The problem is that when a refresh is requested, in ensure_tokens_still_fresh(), save_refresh_token() is called again with the new response. But the new response may not provide a new refresh_token.
Google states

Your application should store both tokens in a secure, long-lived location that is accessible between different invocations of your application.

save_refresh_token() disregard the existing and still valid refresh_token and replace it with false (since the response to a renewal does not contain it)

To Reproduce
Steps to reproduce the behavior:

  1. Use Google OIDC with refresh token
        add_filter( 'openid-connect-generic-auth-url', function( string $url ) {
            return $url . '&access_type=offline&prompt=consent';
        });
  1. Connect
  2. See from your logs that you're disconnect after 1h (the first call torefresh_token)

Expected behavior
I think Google OIDC should work out of the box

Isolating the problem (mark completed items with an [x]):

  • I have deactivated other plugins and confirmed this bug occurs when only this plugin is active.
  • This bug happens with a default WordPress theme active.
  • I can reproduce this bug consistently using the steps above.

WordPress Environment

  • PHP Version: 7.4
  • WordPress Version: 3.9
  • Plugin Version: 3.9.0
  • Identity Provider: Google
  • Relevant Plugin Settings:
@drzraf drzraf added the bug label May 18, 2022
@timnolte timnolte added needs analysis Issues needing further investigation to the cause and/or change required. enhancement Issues & PRs related to new features. labels May 18, 2022
@timnolte timnolte self-assigned this May 18, 2022
@timnolte
Copy link
Collaborator

@drzraf I think this is probably both a big and an enhancement, given them the plugin wasn't designed to handle the offline login method being used. However, replacing a valid token with a false/invalid one is not a good idea. Beyond what Google states I want to review the official OIDC specs as ultimately the desire is to refrain from implementing IDP specific functionality. Thanks for the report!

@drzraf
Copy link
Contributor Author

drzraf commented May 19, 2022

I'll try to suggest a patch, my rough idea on the topic would be to encapsulate the $session[ $this->cookie_token_refresh_key ] block inside save_refresh_token() with a if (isset($token_response['refresh_token'])) { ... }

@timnolte timnolte added this to the 3.9.2 milestone May 11, 2023
@timnolte timnolte modified the milestones: 3.9.2, 3.10.0 May 25, 2023
@ammojamo
Copy link

I also encountered this problem with Salesforce as an Identity Provider.

I think Google and Salesforce are within spec here, as it is optional for the Authorization Server to issue a new refresh token when using an existing refresh token: https://datatracker.ietf.org/doc/html/rfc6749#section-6

The authorization server MAY issue a new refresh token, in which case
the client MUST discard the old refresh token and replace it with the
new refresh token.

This is from the OAuth spec which is referenced directly by the OIDC spec, and the OIDC spec does not include any additional requirement that saying that a refresh response must include a new refresh token.

@timnolte timnolte modified the milestones: 3.10.0, 3.10.1 Apr 25, 2024
@timnolte timnolte removed this from the 3.10.1 milestone May 7, 2024
@timnolte timnolte added this to the 3.10.1 milestone May 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug enhancement Issues & PRs related to new features. needs analysis Issues needing further investigation to the cause and/or change required.
Projects
3 participants