Skip to content

[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.

Sign up for GitHub

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

Merged
nicolas-grekas merged 1 commit into from Feb 5, 2025
Merged

[Security] Return null instead of empty username to fix deprecation notice #59640

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/Symfony/Component/Security/Http/Authenticator/RemoteUserAuthenticator.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Copy link
Author

@ghost ghost Jan 28, 2025 •
edited by ghost
Loading

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)

Copy link
Author

@ghost ghost Jan 28, 2025 •
edited by ghost
Loading

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 the FormLoginAuthenticator's behaviour :

symfony/src/Symfony/Component/Security/Http/Authenticator/FormLoginAuthenticator.php

Lines 130 to 131 in 0deaa1e

if ('' === $credentials['username']) {
throw new BadCredentialsException(\sprintf('The key "%s" must be a non-empty string.', $this->options['username_parameter']));

OTOH the base class' abstract extractUsername() returns a nullable string and the null check in supports() 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 in 0deaa1e

abstract protected function extractUsername(Request $request): ?string;

symfony/src/Symfony/Component/Security/Http/Authenticator/AbstractPreAuthenticatedAuthenticator.php

Lines 67 to 68 in 0deaa1e

if (null === $username) {
$this->logger?->debug('Skipping pre-authenticated authenticator no username could be extracted.', ['authenticator' => static::class]);

Copy link
Member

@chalasr chalasr Feb 4, 2025

Choose a reason for hiding this comment

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

Makes sense to me

}
}
1 change: 1 addition & 0 deletions src/Symfony/Component/Security/Http/Tests/Authenticator/RemoteUserAuthenticatorTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ public function testSupportNoUser()
$authenticator = new RemoteUserAuthenticator(new InMemoryUserProvider(), new TokenStorage(), 'main');

$this->assertFalse($authenticator->supports($this->createRequest([])));
$this->assertFalse($authenticator->supports($this->createRequest(['REMOTE_USER' => ''])));
}

public function testSupportTokenStorageWithToken()
Expand Down