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

Return value of IdentityProviderInterface::getUserEntityByIdentifier() unclear/undocumented #7

Closed
Berdir opened this issue Feb 15, 2018 · 3 comments
Milestone

Comments

@Berdir
Copy link

Berdir commented Feb 15, 2018

Looking into using this but I'm a bit confused about the IdentityProviderInterface.

The method getUserEntityByIdentifier does not document/specify what exactly it should return.

IdTokenResponse::getExtraParams() defines that it should be UserEntityInterface object, but then goes on to call getClaims() which does not seem to be defined on any interface?

Maybe this should define something like a UserEntityWithClaimsInterface?

@steverhoades
Copy link
Owner

@Berdir Thanks for the feedback.

The method getUserEntityByIdentifier should be defined on your Provider (or Repository) and return your user entity by the passed in identifier.

As an example:

class UserProvider implements IdentityProviderInterface
{
    protected $connection;

    public function __construct(\PDO $connection)
    {
        $this->connection = $connection;
    }

    public function getUserEntityByIdentifier($identifier)
    {
            $stmt = $this->connection->prepare("SELECT * FROM oauth_users WHERE unique_id = ?");
            $stmt->execute([$identifier]);

            $result = $stmt->fetch(\PDO::FETCH_ASSOC);
            $entity = null;
            if ($result) {
                $entity = $this->hydrateEntity($result);
            }

            return $entity;
    }
}

I can see now that there absolutely should be an interface for getClaims and that interface should be checked before it's called in the IdTokenResponse method. Look for an update soon.

@Berdir
Copy link
Author

Berdir commented Feb 15, 2018

Thanks for the quick response. Yes, I was able to implement something and got it working.

Since existing implementations would not yet implement that new interface, you might want to consider just using this as documentation and not enforce it, or maybe just do a BC warning. You could of course, since this is not yet marked as stable.

Btw, if you are interested, I'm looking into this with the goal of implementing a contributed Drupal module on top of https://www.drupal.org/project/simple_oauth, which already uses league/oauth2.

@steverhoades steverhoades added this to the v1.0.0 milestone Mar 16, 2018
@steverhoades
Copy link
Owner

@Berdir I've just pushed changes which add an interface requirement to IdTokenResponse. This will be a BC and as such will be included in the 1.0 release. If your feeling brave take a look and let me know what you think.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants