Oauth2-server: BC break when extending BearerTokenResponse

[curry684]

My application just broke down when upgrading to 7.3.0, working fine still when downgrading to 7.2.0.

I'm using the following override in my custom OAuth2Server class to customize the response type:

    /**
     * {@inheritdoc}
     */
    protected function getResponseType()
    {
        $this->responseType = new ExtendedBearerTokenResponse(...someparameters);

        return parent::getResponseType();
    }

The extended response class overrides the default BearerTokenResponse in getExtraParams to add some extra properties to the token. This method is apparently not called anymore, or ignored, as the tokens no longer contain the extra properties.

Have an appointment in 5 minutes so no time to debug further right now, but posting this already hoping you may have an idea what's causing this and issue a fix before I can investigate deeper tomorrow.

[curry684]

Yikes, https://github.com/thephpleague/oauth2-server/commit/d288a2ad8a574d9a1b641d529a79cbc617e03305 is the obvious cause... that commit should've never made it into a minor release, that's fundamentally breaking all extension points.

[Sephster]

Thanks @curry684. Sorry for issues caused. I will revert the name of the function later today and release a patch to fix the problem. Sorry for any inconvenience caused. @ceeram had also reported this issue so it is clearly affecting a few people

[ceeram]

This issue is not just about changing the method name, as i renamed it in our extended class as well. Also setting the privateKey and encryptionKey was moved to the constructor, which is why it still was broken after renaming the extended method.

[Sephster]

@ceeram do you have an example of your extended class you can share so I can get a better idea of the problems at hand?

[ceeram]

https://github.com/thephpleague/oauth2-server/pull/969 i have a fixed the BC without breaking the newly added tests. I think this will make both sides happy

[Sephster]

@curry684 I've reverted the interface change in release 7.3.1 which will hopefully resolve your issue. I'm leaving this issue open for now to follow up on @ceeram's issue and related PR.

[curry684]

It should be noted that we're not so much doing unexpected stuff, we're actually using a documented extension point:

    /**
     * Add custom fields to your Bearer Token response here, then override
     * AuthorizationServer::getResponseType() to pull in your version of
     * this class rather than the default.
     *
     * @param AccessTokenEntityInterface $accessToken
     *
     * @return array
     */
    protected function getExtraParams(AccessTokenEntityInterface $accessToken)
    {
        return [];
    }

I've just upgraded to 7.3.1 but still getting breakage elsewhere:

Argument 1 passed to [redacted]\OAuth2\Entity\AccessToken::convertToJWT() must be an instance of League\OAuth2\Server\CryptKey, null given, called in [redacted]/vendor/league/oauth2-server/src/ResponseTypes/BearerTokenResponse.php on line 28

This could be just an implementation detail on my end, will investigate whether this concerns this library's public API.

[ceeram]

This is also related to moving the setting of the keys. I have a PR up to move them back to where they were #969

[curry684]

My new error is still BC breakage. The old getResponseType implementation would set the keys on the fly: https://github.com/thephpleague/oauth2-server/blob/7.2.0/src/AuthorizationServer.php#L211-L214

As the new implementation does that in the constructor and clones it afterwards any custom ResponseType override "old style" will be missing the keys. https://github.com/thephpleague/oauth2-server/blob/7.3.0/src/AuthorizationServer.php#L117-L121

Imho this whole change is fundamentally wrong, as this breaks the whole point of deciding what ResponseType is needed at runtime, instead forcing us to decide during service initialization. This makes the library much less powerful.

For the time being I'm reverting to 7.2.0 again, this is not something a simple PR can fix.

edit saying the same as @ceeram 44 seconds before me 😉

[Sephster]

@curry684 are you using a similar implementation to the one @ceeram is?

class AuthorizationServer extends BaseAuthorizationServer
{
    protected function getResponseType()
    {
        $this->responseType = new OpenIdBearerTokenResponse();

        return parent::getResponseType();
    }
}

[curry684]

Pretty much identical yes, like I said it's documented to do it like this: https://github.com/thephpleague/oauth2-server/blob/master/src/ResponseTypes/BearerTokenResponse.php#L67-L69

[Sephster]

Have you tried setting the custom bearer token response in the constructor? I've asked @ceeram this as well in his PR. Just trying to establish what the blockers are to such an approach if you have tried this already.

Thank you for your continued help!

[ceeram]

Alternatively, you could add the code to set the keys in your extended method to get around this with current implementation. See https://github.com/thephpleague/oauth2-server/pull/969#issuecomment-439898961

[ceeram]

Still, this is unwanted behavior from a minor release

[curry684]

Yeah I'd recommend just yanking the 7.3.x releases and adding this to the 8.x breaking changes pile. I know there are fixes and workarounds but I shouldn't be having to use them 😉

[curry684]

Well practically yanking isn't really possible anymore after a week. Perhaps best to just ensure the old behavior still works in 7.x branch, if need be with some monkeypatch methods around the old extension points.

Right now it's a dangerous wait until applications being less actively developed hit the composer update button, we can't be the only 2 using this.

[Sephster]

Sorry that I haven't resolved this yet. I've been pretty busy in work. I suspect we will have to implement @ceeram's PR in the meantime and accept that the stateless implementations will have their keys reset unnecessarily.

I will take a closer look at this this evening and aim to get a resolution in place. Long term, I think is is preferable to set the custom response via the constructor so will look at options in version 8 to formalise this as it will require a breaking change.

[curry684]

No problem really, just sticking to 7.2.0 is an acceptable workaround for the time being as long as it doesn't take months to get 8.0 out or fix it well in a 7.3.x edition. Rather do it well than rush now that the dragon is already out. And given the lack of other outcry it would seem the broken extension point isn't used all that often.

[Sephster]

I've taken the decision to merge in @ceeram's fix. Although this will mean that people using Swoole will have their keys set multiple times needlessly, this to me is the lesser of two evils and will remove the BC change.

Longer term, I think I will make the getResponseType() method private as you can set a custom response type via the constructor. Unless someone raises a good reason not to make this change, I feel that passing in a custom class via the constructor is probably a more maintainable approach long term. This will likely come in version 8.

Thank you to you both for your assistance with this and apologies for any inconvenience it has caused you. @curry684 if you could confirm the latest version has now fixed your issue, we can close this issue down. Many thanks!

[Sephster]

Fixed in PR #969

[curry684]

Yep, I can confirm 7.3.2 is working fine again, cheers!

[Sephster]

Thanks for confirming @curry684