-
-
Notifications
You must be signed in to change notification settings - Fork 9.7k
[Security] Fix AbstractAuthenticator::createToken()
BC layer
#42472
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
Conversation
AbstractAuthenticator::createToken()
BC layer
485476f
to
2975e5a
Compare
acfb744
to
6d802d3
Compare
9333402
to
a48ca00
Compare
*/ | ||
public function createToken(Passport $passport, string $firewallName): TokenInterface | ||
public function __call(string $method, array $arguments) |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
AbstractAuthenticator::createToken()
BC layerAbstractAuthenticator::createToken()
BC layer
a48ca00
to
8b464d1
Compare
8b464d1
to
799acc5
Compare
(PR ready) |
Thank you @chalasr. |
An authenticator might override the
createAuthenticatedToken()
method, hence it must keep being called until that authenticator has been migrated to usecreateToken()
.Spotted in lexik/jwt-authentication-bundle's CI.