Add compatibility with league/oauth2-server:^9

[ajgarlag]

No description provided.

[wmouwen]

Would love to see this PR to be released when stable! 🚀

[chalasr]

@ajgarlag would you appreciate some help finishing this? thanks for the PR!

[ajgarlag]

@ajgarlag would you appreciate some help finishing this? thanks for the PR!

@chalasr Sure! Any help is welcome!

I think that there is a supported use case that we should deprecate: calling the authorization controller without any authenticated user (in other PR). WDYT?

[chalasr]

I think that there is a supported use case that we should deprecate: calling the authorization controller without any authenticated user (in other PR). WDYT?

I agree. We should also disallow passing null to UserConverterInterface::toLeague() isn't it?

[chalasr]

Sure! Any help is welcome!

Is there anything missing actually? I just see WIP in the title but this looks good

[ajgarlag]

Sure! Any help is welcome!

Is there anything missing actually? I just see WIP in the title but this looks good

My main concern is with the last commit, but I think this will affect only a minimal number of users.

[ajgarlag]

I think that there is a supported use case that we should deprecate: calling the authorization controller without any authenticated user (in other PR). WDYT?

I agree. We should also disallow passing null to UserConverterInterface::toLeague() isn't it?

I would merge this PR and disallow passing null to UserConverterInterface::toLeague() in a new PR before tagging a new version. WDYT?

Does Symfony allow an empty string as a user identifier?

[chalasr]

I would merge this PR and disallow passing null to UserConverterInterface::toLeague() in a new PR before tagging a new version. WDYT?

Works for me 👍

Does Symfony allow an empty string as a user identifier?

No, built-in authenticators forbid it https://github.com/symfony/symfony/blob/dd9fa153d344593c7fe12eb761158558377d98a4/src/Symfony/Component/Security/Http/Authenticator/JsonLoginAuthenticator.php#L149

[chalasr]

Can you please remind me under which flow do we expect anonymous users? Wording-wise the anonymous concept has been removed in Symfony but I feel like this one is different and specific to this bundle, what's the case I'm missing?

[ajgarlag]

The AuthorizationController does not require authentication (although it is highly recommended), so when the AuthorizationRequestResolveEvent is dispatched, there is a valid use case where the AuthorizationRequestUserResolvingListener cannot set the user instance in the event:

oauth2-server-bundle/src/EventListener/AuthorizationRequestUserResolvingListenerTrait.php

Lines 12 to 18 in ec2c2d3

	public function onAuthorizationRequest(AuthorizationRequestResolveEvent $event): void
	{
	$user = $this->security->getUser();
	if ($user instanceof UserInterface) {
	$event->setUser($user);
	}
	}

So the $event->getUser() call can return null with a resolved authorization

oauth2-server-bundle/src/Controller/AuthorizationController.php

Line 104 in ec2c2d3

$authRequest->setUser($this->userConverter->toLeague($event->getUser()));

This is the use case I think we should disallow.

[ajgarlag]

I would merge this PR and disallow passing null to UserConverterInterface::toLeague() in a new PR before tagging a new version. WDYT?

Works for me 👍

Does Symfony allow an empty string as a user identifier?

No, built-in authenticators forbid it https://github.com/symfony/symfony/blob/dd9fa153d344593c7fe12eb761158558377d98a4/src/Symfony/Component/Security/Http/Authenticator/JsonLoginAuthenticator.php#L149

But the interface allows it. Maybe a /** @return non-empty-string */ annotation should be added to the UserInterface. I'll submit a PR to deprecate this behavior in Symfony 7

[ajgarlag]

But the interface allows it. Maybe a /** @return non-empty-string */ annotation should be added to the UserInterface. I'll submit a PR to deprecate this behavior in Symfony 7

See symfony/symfony#58007

[chalasr]

Given the low potential of breaking people's code as you said + the fact we're still in 0.x version (not for long I think!), I'm fine doing this change although it's a bit aggressive.

[chalasr]

Thank you @ajgarlag.