Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.6k
[Security] Return null instead of empty username to fix deprecation notice#59640
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
| @@ -45,6 +45,6 @@ protected function extractUsername(Request $request): ?string | |||
| throw new BadCredentialsException(sprintf('User key was not found: "%s".', $this->userKey)); | |||
| } | |||
| return $request->server->get($this->userKey); | |||
| return $request->server->get($this->userKey) ?: null; | |||
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.
Looking at how the method is used I wonder if we shouldn’t throw a BadCredentialsException here then.
(xabbuh's comment on previous PR)
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.
Throwing an exception would be consistent with theFormLoginAuthenticator's behaviour :
symfony/src/Symfony/Component/Security/Http/Authenticator/FormLoginAuthenticator.php
Lines 130 to 131 in0deaa1e
| if ('' ===$credentials['username']) { | |
| thrownewBadCredentialsException(\sprintf('The key "%s" must be a non-empty string.',$this->options['username_parameter'])); |
OTOH the base class' abstractextractUsername() returns a nullable string and the null check insupports() indicates this is due to being unable to extract a username. Other users who've extended the base class may expect that behaviour, so throwing an exception (in a reference implementation) may be confusing.
symfony/src/Symfony/Component/Security/Http/Authenticator/AbstractPreAuthenticatedAuthenticator.php
Line 53 in0deaa1e
| abstractprotectedfunctionextractUsername(Request$request): ?string; |
symfony/src/Symfony/Component/Security/Http/Authenticator/AbstractPreAuthenticatedAuthenticator.php
Lines 67 to 68 in0deaa1e
| if (null ===$username) { | |
| $this->logger?->debug('Skipping pre-authenticated authenticator no username could be extracted.', ['authenticator' =>static::class]); |
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.
Makes sense to me
Apologies; I ran into some trouble rebasing the commit for a test case from myprevious PR so I created a new branch/PR to be safe. |
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.
Works for me as is.
Thank you@phasdev. |
25dd52e intosymfony:6.4Uh oh!
There was an error while loading.Please reload this page.
RemoteUserAuthenticatormay return an empty string when extracting a username from the configured$_SERVERparameter (e.g.REMOTE_USER).An empty username triggers the
User Deprecated: Since symfony/security-http 7.2: Using an empty string as user identifier is deprecated and will throw an exception in Symfony 8.0.Return
nullinstead of empty username to skip authenticator when username is empty and fix Symfony 8 deprecation notice.