Skip to content

[Security/Core] Add CustomUserMessageAccountStatusException #36656

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

Merged
merged 1 commit into from
May 5, 2020

Conversation

VincentLanglet
Copy link
Contributor

Q A
Branch? master
Bug fix? no
New feature? yes
Deprecations? no
License MIT
Doc PR Not really needed

When implementing the UserCheckerInterface, we can throw AccountStatusException. Similar to CustomUserMessageAuthenticationException, this exception allow to throw an AccountStatusException with a custom message.

@VincentLanglet VincentLanglet force-pushed the accountStatusException branch from 826df35 to 9233efb Compare May 1, 2020 17:30
@nicolas-grekas nicolas-grekas added this to the next milestone May 1, 2020
@nicolas-grekas nicolas-grekas changed the title Add CustomUserMessageAccountStatusException [Security/Core] Add CustomUserMessageAccountStatusException May 1, 2020
@dbrumann
Copy link
Contributor

dbrumann commented May 4, 2020

Code looks good to me, but I am wondering what this is needed for and if this is the correct way of doing it?

There are a few exceptions that already extend the AccountStatusException, e.g. LockedException, AccountExpiredException or DisabledException. These would not benefit from your change. Is that intended?

That brings me to my other question, these Exceptions already communicate what the status problem was and a custom message could be provided, e.g. via translations. Do you have a use case for when you need a fully customizable message? Maybe that would help me understand better what this is needed for.

@VincentLanglet
Copy link
Contributor Author

Code looks good to me, but I am wondering what this is needed for and if this is the correct way of doing it?

There are a few exceptions that already extend the AccountStatusException, e.g. LockedException, AccountExpiredException or DisabledException. These would not benefit from your change. Is that intended?

That brings me to my other question, these Exceptions already communicate what the status problem was and a custom message could be provided, e.g. via translations. Do you have a use case for when you need a fully customizable message? Maybe that would help me understand better what this is needed for.

Yes, it's intended.

In the same way, there are multiple AuthenticationException, like BadCredentialsException, AuthenticationCredentialsNotFoundException, UsernameNotFoundException, etc... BUT symfony provide a CustomUserMessageAuthenticationException if a custom message is needed. This avoid every developper to create a new custom Exception.

For UserChecker, there are indeed LockedException, AccountExpiredException, DisabledException, etc... but you can't customize the message. Symfony won't be able to cover every possible case, so providing a CustomUserMessage sounds like a nice idea to me.

You can indeed override the error message Account is locked. of, let's say, a LockedException but the exception are no taking messageData. So If my user is temporary locked until some date, I can't pass this date to the LockedException and display it in the error message.

Plus, If you override an error message like Account is disabled., I am maybe wrong but you'll can only have one override. If you have multiple reason to disabled the account, you may want different exception and error message.

@xabbuh xabbuh added the Security label May 4, 2020
@dbrumann
Copy link
Contributor

dbrumann commented May 4, 2020

Thanks for the clarification.

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.

I think this makes sense, given AccountStatusExceptions are handled differently by the Security system (see e.g. authentication provider manager and exception listener)

}

/**
* Set a message that will be shown to the user.
Copy link
Member

Choose a reason for hiding this comment

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

Sets

@fabpot
Copy link
Member

fabpot commented May 5, 2020

Thank you @VincentLanglet.

@fabpot fabpot merged commit a0c2dd8 into symfony:master May 5, 2020
@nicolas-grekas nicolas-grekas modified the milestones: next, 5.1 May 5, 2020
@fabpot fabpot mentioned this pull request May 5, 2020
@VincentLanglet VincentLanglet deleted the accountStatusException branch June 21, 2021 08:31
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.

7 participants