save_refresh_token may delete valid refresh_token and break refresh functionality

[drzraf]

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';
        });

2. Connect
3. 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:

[timnolte]

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

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'])) { ... }

[ammojamo]

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.