Skip to content
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

Easier Custom Authentication errors #15882

Merged
merged 1 commit into from Sep 28, 2015

Conversation

@weaverryan
Copy link
Member

commented Sep 24, 2015

Q A
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets n/a
License MIT
Doc PR not yet

This makes failing authentication with a custom message much easier:

throw CustomAuthenticationException::createWithSafeMessage(
    'That was a ridiculous username'
);

// or
$e = new CustomAuthenticationException();
$e->setSafeMessage('That was a ridiculous username');

throw $e;

Currently, to do this, you'd need to create a new sub-class of AuthenticationException, which is way more work than it needs to be. The original design was so that all messages exposed are safe, which is why I've named the methods like I have.

Thanks!

@weaverryan weaverryan force-pushed the weaverryan:custom-authentication-exception branch from 8397fc9 to 1773c01 Sep 24, 2015

*
* @author Ryan Weaver <ryan@knpuniversity.com>
*/
class CustomAuthenticationException extends AuthenticationException

This comment has been minimized.

Copy link
@fabpot

fabpot Sep 24, 2015

Member

I would convey the "safe" (not sure if safe is the right word here) notion in the class name.

This comment has been minimized.

Copy link
@fabpot

fabpot Sep 24, 2015

Member

I suppose that safe here means: can be displayed to the end user.

*
* @return CustomAuthenticationException
*/
static public function createWithSafeMessage($safeMessage = '', array $messageData = array(), $code = 0, \Exception $previous = null)

This comment has been minimized.

Copy link
@fabpot

fabpot Sep 24, 2015

Member

We do not have such static methods to create exceptions in Symfony (unlike Doctrine).

return serialize(array(
$this->messageKey,
$this->messageData,
parent::serialize(),

This comment has been minimized.

Copy link
@stof

stof Sep 24, 2015

Member

I suggest putting the parent data first. This way, it is easier to add more fields in the future while preserving backward compatibility in the future (to keep the BC layer sane, new fields should be added at the end of array, which would then mean that the parent data goes in the middle of the object data)

public function getMessageData()
{
return null !== $this->messageData ? $this->messageData : parent::getMessageData();

This comment has been minimized.

Copy link
@stof

stof Sep 24, 2015

Member

I think you should rather ensure that $this->messageData is always an array (and never null) and then get rid of the parent call returning an empty array

@weaverryan weaverryan force-pushed the weaverryan:custom-authentication-exception branch from 1773c01 to 556abe0 Sep 26, 2015

@weaverryan

This comment has been minimized.

Copy link
Member Author

commented Sep 26, 2015

I've just made all the suggested changes. To avoid having the static method call, I've made the $message constructor be the safe message. I think this is still safe because I've renamed the class to highlight that you're passing in a safe message. We could not override the constructor and force the setter to be called to - it's just a little less convenient.

*/
namespace Symfony\Component\Security\Core\Exception;
use Exception;

This comment has been minimized.

Copy link
@wouterj

wouterj Sep 26, 2015

Member

This should be inlined as a FQCN

{
parent::__construct($message, $code, $previous);
$this->setSafeMessage($message, $messageData);

This comment has been minimized.

Copy link
@wouterj

wouterj Sep 26, 2015

Member

might worth a check if $message is not an empty string?

This comment has been minimized.

Copy link
@weaverryan

weaverryan Sep 26, 2015

Author Member

good catch - i've done that - I think it's the best behavior

@wouterj

This comment has been minimized.

Copy link
Member

commented Sep 26, 2015

Status: Needs work

@weaverryan weaverryan force-pushed the weaverryan:custom-authentication-exception branch from f9a00b0 to 32d9a6a Sep 26, 2015

@weaverryan

This comment has been minimized.

Copy link
Member Author

commented Sep 26, 2015

Updated!

Status: Needs Review

if ('' !== $message) {
$this->messageKey = $message;
}

This comment has been minimized.

Copy link
@wouterj

wouterj Sep 26, 2015

Member

hmm, no you removed the complete method call. In such cases, the Symfony standard is to remove any pre-configured defaults in the property definition and just call $this->messageKey = $message; $this->messageData = $messageData;

@weaverryan weaverryan force-pushed the weaverryan:custom-authentication-exception branch from d616938 to 84dd924 Sep 26, 2015

@weaverryan

This comment has been minimized.

Copy link
Member Author

commented Sep 26, 2015

@wouterj check it out now - I just simplified things - I think this makes more sense.

*
* @author Ryan Weaver <ryan@knpuniversity.com>
*/
class SafeMessageAuthenticationException extends AuthenticationException

This comment has been minimized.

Copy link
@fabpot

fabpot Sep 27, 2015

Member

I'm really not comfortable with the naming. The idea is that the message is safe to be displayed to the end user. So, what about UserMessageAuthenticationException, or UserFacingAuthenticationException, UserOrientedAuthenticationException? You get my point; the naming should indicate that the message is "for end users". My proposals are probably not the right ones, but we need to find a better name.

This comment has been minimized.

Copy link
@javiereguiluz

javiereguiluz Sep 27, 2015

Member

More proposals: PublicAuthenticationException, ExposedAuthenticationException, VisibleAuthenticationException

This comment has been minimized.

Copy link
@weaverryan

weaverryan Sep 27, 2015

Author Member

I've just tried CustomUserMessageAuthenticationException. It's the the longest of all ideas, but it's clear to me: this is how you show a custom message... to your user. Leanna liked it better than UserFacingMessageAuthenticationException, fwiw ;)

@weaverryan weaverryan force-pushed the weaverryan:custom-authentication-exception branch 2 times, most recently from 2d16ab0 to af25c3b Sep 27, 2015

@weaverryan weaverryan force-pushed the weaverryan:custom-authentication-exception branch from af25c3b to d7c1463 Sep 27, 2015

@fabpot

This comment has been minimized.

Copy link
Member

commented Sep 28, 2015

Thank you @weaverryan.

@fabpot fabpot merged commit d7c1463 into symfony:2.8 Sep 28, 2015

1 of 3 checks passed

continuous-integration/appveyor AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build failed
Details
fabbot.io Your code looks good.
Details
fabpot added a commit that referenced this pull request Sep 28, 2015
feature #15882 Easier Custom Authentication errors (weaverryan)
This PR was merged into the 2.8 branch.

Discussion
----------

Easier Custom Authentication errors

| Q             | A
| ------------- | ---
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | n/a
| License       | MIT
| Doc PR        | not yet

This makes failing authentication with a custom message much easier:

```php
throw CustomAuthenticationException::createWithSafeMessage(
    'That was a ridiculous username'
);

// or
$e = new CustomAuthenticationException();
$e->setSafeMessage('That was a ridiculous username');

throw $e;
```

Currently, to do this, you'd need to create a new sub-class of `AuthenticationException`, which is way more work than it needs to be. The original design was so that all messages exposed are safe, which is why I've named the methods like I have.

Thanks!

Commits
-------

d7c1463 Adding a class to make it easier to set custom authentication error messages
@fabpot fabpot referenced this pull request Nov 16, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.