Service Configuration: Use UserConverterInterface on UserRepository definition

[yvalentin]

Hi !

On the service configuration (oauth2-bundle/Resources/config/services.xml:48), the UserRepository service is define.
And it use ClientManagerInterface and EventDispatcherInterface and UserConverter

Can you change this configuration to use UserConverterInterface ?

As UserConverter class is define as the default class for this interface (oauth2-bundle/Resources/config/services.xml:233), we can easy overload the interface definition if we when to use another class converter.

With this way, we can overload the converter if we want to change with another property than username ('email' for example)

# oauth2-bundle/Converter/UserConverter.php
$userEntity->setIdentifier($user->getUsername());

Thanks

[jorissteyn]

I believe this is important because the default behaviour could be considered a security vulnerability: if a user with an active oauth token changes his username, and another user changes his username to the first users username, he is effectively hijacking the oauth token.

Being able to change the default behaviour is important, as well as documenting the default behaviour.

The bundle defines the class name as alias:

        <service id="Trikoder\Bundle\OAuth2Bundle\Converter\UserConverter" />
        <service id="trikoder.oauth2.converter.user_converter" alias="Trikoder\Bundle\OAuth2Bundle\Converter\UserConverter">
            <deprecated>The "%alias_id%" service alias is deprecated and will be removed in v4.</deprecated>
        </service>

But internally still uses the classname as ID making overriding the user converter hard. I believe we should use the new service id in services.xml.

[jorissteyn]

Also, DoctrineCredentialsRevoker::revokeCredentialsForUser() makes the assumption that the user identifier is the username, which is incorrect if using the default UserConverter. The method should take an Trikoder\Bundle\OAuth2Bundle\League\Entity\UserEntityInterface as argument to make sure tokens are revoked.

[jorissteyn]

I give up: the assumption that the username is the user identifier is too hard-coded. Overriding the UserConverter is not practically possible. To prevent the described security vulnerability, you must make sure to invalidate oauth tokens after a username change.