8000 [Security] Fix UserNotFoundException is not thrown by damienfa · Pull Request #45452 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[Security] Fix UserNotFoundException is not thrown #45452

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

Jump to bottom
Merged
merged 1 commit into from
Mar 27, 2022
Merged

[Security] Fix UserNotFoundException is not thrown #45452

merged 1 commit into from
Mar 27, 2022

Conversation

damienfa
Copy link
Contributor
@damienfa damienfa commented Feb 17, 2022 •
edited by nicolas-grekas
Loading
Q A
Branch? 5.4
Bug fix? yes
New feature? no
Deprecations? no
Tickets Fix #45070
License MIT
Doc PR N/A

@carsonbot carsonbot added this to the 6.1 milestone Feb 17, 2022
@damienfa damienfa mentioned this pull request Feb 17, 2022
Closed
@carsonbot carsonbot changed the title Fix #45070 : UserNotFoundException is not thrown [Security] Fix #45070 : UserNotFoundException is not thrown Feb 17, 2022
@nicolas-grekas nicolas-grekas changed the title [Security] Fix #45070 : UserNotFoundException is not thrown [Security] Fix UserNotFoundException is not thrown Feb 17, 2022
@carsonbot
Copy link

Hey!

I think @IonBazan has recently worked with this code. Maybe they can help review this?

Cheers!

Carsonbot

nicolas-grekas
nicolas-grekas reviewed Feb 18, 2022
View reviewed changes
Copy link
Member
@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

Can you please rebase to target 5.4, and add a test case?
8000

src/Symfony/Component/Security/Http/Authenticator/Passport/Badge/UserBadge.php Outdated
@@ -66,6 +67,13 @@ public function getUser(): UserInterface
}

$user = ($this->userLoader)($this->userIdentifier);

// No user has been found via the $this->userLoader callback.
if (is_null($user)) {
Copy link
Member

Choose a reason for hiding this comment

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

if (null === $user) {

@nicolas-grekas nicolas-grekas modified the milestones: 6.1, 5.4 Feb 18, 2022
@nicolas-grekas
Copy link
Member

Can you please add a test case, and rebase + target 5.4 also?

@kaznovac
Copy link
Contributor

don't have time to thoroughly check the pr and how does the UserNotFoundException propagate, pay attention not to reintroduce the user enumerati 8000 on
also take in consideration the hideUserNotFoundExceptions parameter

@fabpot
Copy link
Member
fabpot commented Mar 26, 2022

@wouterj Can you have a look at this PR?

wouterj
wouterj approved these changes Mar 26, 2022
View reviewed changes
Copy link
Member
@wouterj wouterj left a comment

Choose a reason for hiding this comment

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

With the suggested CS changes, this change looks good to me (for 5.4+).

This code is within the user enumeration control of the authenticator manager - so nothing to worry about concerning that.

src/Symfony/Component/Security/Http/Authenticator/Passport/Badge/UserBadge.php Outdated
Comment on lines 72 to 76
if (is_null($user)) {
($exception = new UserNotFoundException())->setUserIdentifier($this->userIdentifier);
throw $exception;
Copy link
Member
@wouterj wouterj Mar 26, 2022

Choose a reason for hiding this comment

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

Suggested change
if (is_null($user)) {
($exception = new UserNotFoundException())->setUserIdentifier($this->userIdentifier);
throw $exception;
if (null === $user) {
$exception = new UserNotFoundException();
$exception->setUserIdentifier($this->userIdentifier);
throw $exception;

chalasr
chalasr approved these changes Mar 27, 2022
View reviewed changes
Copy link
Member
@chalasr chalasr left a comment

Choose a reason for hiding this comment

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

Once CS issues have been adressed

@fabpot
Copy link
Member
fabpot commented Mar 27, 2022

Thank you @damienfa.

@fabpot fabpot merged commit e6e7efd into symfony:6.1 Mar 27, 2022
@Nessnkr
Copy link
Nessnkr commented Apr 15, 2022

Hello @fabpot ,
Shouldn't it be merged into 5.4?

nicolas-grekas pushed a commit that referenced this pull request Apr 15, 2022
@fabpot @nicolas-grekas
bug #45452 [Security] Fix UserNotFoundException is not thrown (damienfa)
3ed91c4 
This PR was merged into the 5.4 branch.

Discussion
----------

[Security] Fix UserNotFoundException is not thrown

| Q             | A
| ------------- | ---
| Branch?       |  5.4
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Tickets       | Fix #45070
| License       | MIT
| Doc PR        |  N/A

Commits
-------

7e0ed85 Fix issue 45070 :  UserNotFoundException is not thrown
@nicolas-grekas
Copy link
Member

Cherry-picked on 5.4 as 3ed91c4
Thanks for the notice @Nessnkr
It'd be great to have a test case by the way.
Anyone up to send one?

nicolas-grekas added a commit that referenced this pull request Apr 15, 2022
@nicolas-grekas
Merge branch '5.4' into 6.0
5102746 
* 5.4:
  cs fix
  bug #45452 [Security] Fix UserNotFoundException is not thrown (damienfa)
nicolas-grekas added a commit that referenced this pull request Apr 15, 2022
@nicolas-grekas
Merge branch '6.0' into 6.1
c711512 
* 6.0:
  cs fix
  bug #45452 [Security] Fix UserNotFoundException is not thrown (damienfa)
@chalasr chalasr mentioned this pull request Apr 15, 2022
Merged
@chalasr
Copy link
Member
chalasr commented Apr 15, 2022

See #46063 for the missing test case

fabpot added a commit that referenced this pull request Apr 16, 2022
@fabpot
minor #46063 [Security] Add test case for user not found (chalasr)
686c704 
This PR was merged into the 5.4 branch.

Discussion
----------

[Security] Add test case for user not found

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

Covering the changes made in #45452.

Commits
-------

4a75e98 [Security] Add test case for user not found
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nicolas-grekas nicolas-grekas nicolas-grekas left review comments

@fabpot fabpot fabpot approved these changes

@wouterj wouterj wouterj approved these changes

@chalasr chalasr chalasr approved these changes

Assignees
No one assigned
Labels
Bug Security Status: Reviewed
Projects
None yet
Milestone
5.4
Development

Successfully merging this pull request may close these issues.

Security - Custom login authenticator
8 participants
@damienfa @carsonbot @nicolas-grekas @kaznovac @fabpot @Nessnkr @chalasr @wouterj
0