Skip to content

Conversation

chalasr
Copy link
Member

@chalasr chalasr commented Aug 10, 2021

Q A
Branch? 5.4
Bug fix? yes
New feature? no
Deprecations? no
Tickets -
License MIT
Doc PR -

An authenticator might override the createAuthenticatedToken() method, hence it must keep being called until that authenticator has been migrated to use createToken().

Spotted in lexik/jwt-authentication-bundle's CI.

@chalasr chalasr requested a review from wouterj as a code owner August 10, 2021 21:20
@chalasr chalasr changed the title Fix AbstractAuthenticator::createToken() BC layer Fix AbstractAuthenticator::createToken() BC layer Aug 10, 2021
@chalasr chalasr force-pushed the fix-createtoken-layer branch 2 times, most recently from 485476f to 2975e5a Compare August 10, 2021 21:22
@chalasr chalasr force-pushed the fix-createtoken-layer branch 4 times, most recently from acfb744 to 6d802d3 Compare August 10, 2021 22:53
@chalasr chalasr force-pushed the fix-createtoken-layer branch 5 times, most recently from 9333402 to a48ca00 Compare August 10, 2021 23:19
*/
public function createToken(Passport $passport, string $firewallName): TokenInterface
public function __call(string $method, array $arguments)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't it be easier if we call createAuthenticatedToken() from createToken() (with a hidden 3 parameter: $triggerDeprecation)? I think that's what we normally do

Copy link
Member Author

@chalasr chalasr Aug 12, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wish it would be as simple, the difference here is that this is an abstract class.
Take the following example (which reflects what I have in lexikjwt):

class JWTAuthenticator 
{
    public function createAuthenticatedToken(PassportInterface $passport, string $firewallName)
    {
        $token = parent::createAuthenticatedToken($passport, $firewallName);
        
        // do something
        
        return $token;    
    }
}

Problem: Making AbstractAuthenticator::createToken() calls the legacy createAuthenticatedToken() with an extra flag that disable the deprecation notice would have no effect, because createAuthenticatedToken would be called on the final implementation i.e. JWTAuthenticator::createAuthenticatedToken(), not the AbstractAuthenticator one - which means that the flag is ignored given that JWTAuthenticator does not know about it.

Also, I need to keep the bundle compatible with both 5.3 and 5.4, which requires to keep implementing the legacy createAuthenticatedToken() in addition to the new method (it's just never called on 5.4).

I hope my explanation is clear enough, I literally spent 2 hours figuring how to fix this BC layer so that everything behaves as expected.
Btw, I just force pushed to hopefully make the code simpler by moving the logic to AbstractAuthenticator only and removing the __call() method.

@carsonbot carsonbot changed the title Fix AbstractAuthenticator::createToken() BC layer [Security] Fix AbstractAuthenticator::createToken() BC layer Aug 11, 2021
@derrabus derrabus added this to the 5.4 milestone Aug 11, 2021
@chalasr chalasr force-pushed the fix-createtoken-layer branch from a48ca00 to 8b464d1 Compare August 12, 2021 18:22
@chalasr chalasr force-pushed the fix-createtoken-layer branch from 8b464d1 to 799acc5 Compare August 12, 2021 18:26
@chalasr
Copy link
Member Author

chalasr commented Aug 18, 2021

(PR ready)

@fabpot
Copy link
Member

fabpot commented Aug 18, 2021

Thank you @chalasr.

@fabpot fabpot merged commit 4822448 into symfony:5.4 Aug 18, 2021
@chalasr chalasr deleted the fix-createtoken-layer branch August 18, 2021 15:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants