thephpleague / oauth2-server

A spec compliant, secure by default PHP OAuth 2.0 Server
https://oauth2.thephpleague.com
MIT License
6.49k stars 1.12k forks source link

Why setUserIdentifier, not setUser? #1376

Closed DarthLegiON closed 7 months ago

DarthLegiON commented 9 months ago

Hello!

I'm using this package with Symfony and making AccessToken and AuthCode entities with Doctrine ORM. All relations are good, except of one: TokenInterface has setUserIdentifier method that accepts id. In Doctrine, making associations you should pass entire entity inside assiciated entity to make it work. And my new token entity unfortunately doesn't allow it, because it accepts user id.

The only place it's set is AbstractGrant:501:

$authCode->setUserIdentifier($userIdentifier);

This value is passed from AuthenticationServer:363:

$authorizationRequest->getUser()->getIdentifier(),

An UserEntityInterface entity may be simply passed there without resolving the id, and everything would be nice. Why?

The same situation is with getNewToken method of AccessTokenRepositoryInterface. This just confusing me so much, and makes to seek for ugly solutions to make Doctrine relations work. Maybe you could provide me a workaround? Thanks!

Sephster commented 9 months ago

Have you looked at the bundle for Symfony? It might be easier to integrate with your application using it

DarthLegiON commented 9 months ago

@Sephster Unfortunately, we cannot use it as we have old Symfony version and not updated yet. But it's a good idea to look there for a workaround, thanks.

DarthLegiON commented 9 months ago

@Sephster I see that the Symfony bundle doesn't make any foreign keys in the persistense entities and just fills DB with schema initialization code. It doesn't suit our requirements, we should make database manipulations with migrations.

I just can't understand why in this particular field you don't operate interface and use id. It doesn't make any sense for me.

Sephster commented 9 months ago

I can't speak as to why the code is like that as I didn't write the lines you are referencing. I am not sure why this would have such a bearing on your application code though. Are you able to share example code?

I believe people have successfully implemented this lib using Doctrine as the backend in the past as this issue hasn't been raised before so I'm hoping the fix can be done in userland rather than this codebase if possible

DarthLegiON commented 9 months ago

Of course, I already found a workaround, but it looks a bit ugly.

sgoranov commented 9 months ago

I also wonder why the User entity is not passed instead the identifier . This one is used only here:

vendor/league/oauth2-server/src/Grant/AbstractGrant.php

    protected function issueAuthCode(
        DateInterval $authCodeTTL,
        ClientEntityInterface $client,
        $userIdentifier,
        $redirectUri,
        array $scopes = []
    ) {
        $maxGenerationAttempts = self::MAX_RANDOM_TOKEN_GENERATION_ATTEMPTS;

        $authCode = $this->authCodeRepository->getNewAuthCode();
        $authCode->setExpiryDateTime((new DateTimeImmutable())->add($authCodeTTL));
        $authCode->setClient($client);
        $authCode->setUserIdentifier($userIdentifier);

vendor/league/oauth2-server/src/Grant/AuthCodeGrant.php

        if ($authorizationRequest->isAuthorizationApproved() === true) {
            $authCode = $this->issueAuthCode(
                $this->authCodeTTL,
                $authorizationRequest->getClient(),
                $authorizationRequest->getUser()->getIdentifier(),
                $authorizationRequest->getRedirectUri(),
                $authorizationRequest->getScopes()

And it looks like there is no problem to pass the whole User entity. On the other hand the current behavior, i.e. using the identifier allows loading the users from somewhere else which I guess is the main idea behind this.

DarthLegiON commented 9 months ago

@sgoranov yes, I'm talking about the same: always when we're going to link accessToken/authCode to a user, we can pass User entity either and loose nothing. But looks like the code base already cannot be adapted for this, and it will break backward compatibility.

I only wanted to know what decision brought developers to this and understand the solution.

Sephster commented 8 months ago

The original developer of this is no longer involved in the project so I can't speak as to why it was implemented that way. Do you have a link to the Doctrine behaviour stating you must pass an object to create a relation? Thanks

Sephster commented 7 months ago

Closing due to lack of response