8000 merged branch asm89/issue-837 (PR #4935) · pilot/symfony@3e99f4e · GitHub
[go: up one dir, main page]
Skip to content

Commit 3e99f4e

Browse files
fabpotfabpot
committed
merged branch asm89/issue-837 (PR symfony#4935)
This PR was merged into the master branch. Commits ------- 73db84f [Security] Move translations file to 'security' domain 324703a [Security] Switch to English messages as message keys aa74769 [Security] Fix CS + unreachable code 2d7a7ba [Security] Fix `AuthenticationException` serialization 50d5724 [Security] Introduced `UsernameNotFoundException#get/setUsername` 39da27a [Security] Removed `get/setExtraInformation`, added `get/set(Token|User)` 837ae15 [Security] Add note about changed constructor to changelog d6c57cf [FrameworkBundle] Register security exception translations d7129b9 [Security] Fix exception constructors called in `UserChecker` 0038fbb [Security] Add initial translations for AccountStatusException childs 50e2cfc [Security] Add custom `getMessageKey` AccountStatusException childs 1147977 [Security] Fix InsufficientAuthenticationException constructor calls 79430b8 [Security] Fix AuthenticationServiceException constructor calls 42cced4 [Security] Fix AuthenticationException constructor calls 963a1d7 [Security] Add initial translations for the exceptions ed6eed4 [Security] Add `getMessageKey` and `getMessageData` to auth exceptions 694c47c [Security] Change signature of `AuthenticationException` to match `\Exception` Discussion ---------- [2.2][Security] AuthenticationException enhancements Bug fix: semi Feature addition: yes Backwa 8000 rds compatibility break: yes Symfony2 tests pass: [![Build Status](https://secure.travis-ci.org/asm89/symfony.png?branch=issue-837)](http://travis-ci.org/asm89/symfony) Fixes the following tickets: symfony#837 License of the code: MIT This PR adds the functionality discussed in symfony#837 and changes the constructor of the `AuthenticationException` to match that of `\Exception`. This PR will allow developers to show a translated (save) authentication exception message to the user. :) *Todo:* - Add some functional test to check that the exceptions can indeed be translated? - Get feedback on the current English messages --------------------------------------------------------------------------- by asm89 at 2012-07-15T14:04:11Z ping @schmittjoh --------------------------------------------------------------------------- by schmittjoh at 2012-07-15T14:57:32Z Looks good to me. While you are at the exceptions, I think we can also get rid of the "extra information" thing and replace it by explicit getters/setters. Mostly that will mean adding set/getToken, set/getUser, set/getUsername. Bundles might add custom exceptions which have other data. This will make it a bit more useful and predictable. --------------------------------------------------------------------------- by asm89 at 2012-07-15T15:40:45Z @schmittjoh I removed the `get/setExtraInformation` and added the more explicit getters/setters as you suggested. --------------------------------------------------------------------------- by asm89 at 2012-07-15T19:33:15Z @fabpot Did you reschedule this for 2.2? Why? It was originally a 2.1 ticket. I think it is an important one because at the moment there is no reliable way to show users the cause of an `AuthenticationException` without the threat of exposing sensitive information. This issue has been around for a while, see the original issue this PR refers to, or for example [this TODO comment in FOSUB](https://github.com/FriendsOfSymfony/FOSUserBundle/blob/master/Controller/SecurityController.php#L37). The PR itself is ready to merge now. My only question that remains is about whether the actual translations should be functional tested? --------------------------------------------------------------------------- by fabpot at 2012-07-15T19:43:19Z We need to stop at some point. If not, we never release anything. beta3 was scheduled for today and I don't plan any other one before the first RC and I won't have time to review this PR next week. So, if you, @schmittjoh, @vicb, @stof, and a few other core devs "validate" this PR, I might consider merging it before 2.1. --------------------------------------------------------------------------- by asm89 at 2012-07-15T19:46:09Z @fabpot I totally agree with your point of view. I just have been trying to pickup some security issues that were still open. :) --------------------------------------------------------------------------- by stof at 2012-07-15T19:50:29Z This looks good to me --------------------------------------------------------------------------- by asm89 at 2012-08-12T09:06:24Z Since the beta period is over I assume the window was missed to get this security related PR in 2.1. If I have feedback from @fabpot I'll still try to make it mergeable asap though. --------------------------------------------------------------------------- by fabpot at 2012-08-13T10:10:32Z @asm89 This would indeed be considered for merging in 2.2. --------------------------------------------------------------------------- by Antek88 at 2012-10-03T10:30:46Z +1 --------------------------------------------------------------------------- by stof at 2012-10-04T21:27:15Z @asm89 could you rebase this PR ? It conflicts with master --------------------------------------------------------------------------- by fabpot at 2012-10-05T17:16:44Z What's the status of this PR? @asm89 Have you taken all the feedback into account? --------------------------------------------------------------------------- by stof at 2012-10-13T17:48:48Z @asm89 ping --------------------------------------------------------------------------- by fabpot at 2012-10-29T09:48:40Z @asm89 If you don't have time, I can finish the work on this PR, but can you just tell me what's left? --------------------------------------------------------------------------- by asm89 at 2012-10-29T10:02:22Z I can pick this up, but I have two outstanding questions: - One about adding `::create()`? symfony#4935 (comment) - And what is the final verdict on the messages? symfony#4935 (comment) The initial idea was that the exception itself have an exception message which is plain english and informative for the developer. If you want to display the 'safe' user messages you have the optional dependency on the translator. There is a comparison made with the Validator component, but in my opinion that's a different case because the violations always contain the message directed at the user and have no plain english message for the developer. Apart from that the Validator component contains it's own code for replacing `{{ }}` variables in messages (duplication? not as flexible as the translator). Concluding I'd opt for: optional dependency on translator component if you want to show 'safe' user messages + message keys. @schmittjoh Any things to add? --------------------------------------------------------------------------- by schmittjoh at 2012-10-29T10:14:09Z Message keys sound good to me. I wouldn't add the ``create`` method for now. On Mon, Oct 29, 2012 at 11:02 AM, Alexander <notifications@github.com>wrote: > I can pick this up, but I have two outstanding questions: > > - One about adding ::create()? symfony#4935<symfony#4935 (comment)> > - And what is the final verdict on the messages? symfony#4935<https://github.com/symfony/symfony/issues/4935#discussion_r1165701>The initial idea was that the exception itself have an exception message > which is plain english and informative for the developer. If you want to > display the 'safe' user messages you have the optional dependency on the > translator. There is a comparison made with the Validator component, but in > my opinion that's a different case because the violations always contain > the message directed at the user and have no plain english message for the > developer. Apart from that the Validator component contains it's own code > for replacing {{ }} variables in messages (duplication? not as > flexible as the translator). Concluding I'd opt for: optional dependency on > translator component if you want to show 'safe' user messages + message > keys. > > @schmittjoh <https://github.com/schmittjoh> Any things to add? > > — > Reply to this email directly or view it on GitHub<symfony#4935 (comment)>. > > --------------------------------------------------------------------------- by fabpot at 2012-10-29T10:27:37Z As I said in the discussion about the translations, I'm -1 for the message keys to be consistent with how we manage translations everywhere else in the framework. --------------------------------------------------------------------------- by stof at 2012-10-29T10:30:50Z @fabpot When we changed the English translation for the validation errors in 2.1, we had to tag the commit as a BC rbeak as it was changing the source for all other translations. And if you look at the state of the files now, you will see that we are *not* using the English as source anymore in some places as some validation errors have a pluralized translation but the source has not been changed. So I think using a key is more future-proof. --------------------------------------------------------------------------- by asm89 at 2012-10-30T19:44:49Z Any final decision on this? On one hand I have @stof and @schmittjoh +1 on message keys, on the other @fabpot -1. I guess it's your call @fabpot. Edit: also @vicb seemed to be +1 on message keys earlier on. --------------------------------------------------------------------------- by drak at 2012-11-01T20:19:00Z I am also -1, I agree with @fabpot --------------------------------------------------------------------------- by asm89 at 2012-11-12T09:38:51Z @fabpot Can you please give a definite answer on this? I personally think @stof and @vicb have good points to do message keys, but with all these different people +1 and -1'ing the PR I'm lost on what it should actually do. --------------------------------------------------------------------------- by asm89 at 2012-11-14T09:59:06Z ping @fabpot --------------------------------------------------------------------------- by asm89 at 2012-11-26T10:01:27Z ping @fabpot We talked about this in Berlin. Any final thoughts on the PR? :) One idea was to do message keys + opt depend on the translator component if you want to use them, or use your own implementation. --------------------------------------------------------------------------- by fabpot at 2012-11-26T14:01:37Z The conclusion is: keep using plain English. On Mon, Nov 26, 2012 at 11:01 AM, Alexander <notifications@github.com>wrote: > ping @fabpot <https://github.com/fabpot> We talked about this in Berlin. > Any final thoughts on the PR? :) One idea was to do message keys + opt > depend on the translator component if you want to use them, or use your own > implementation. > > — > Reply to this email directly or view it on GitHub<symfony#4935 (comment)>. > > --------------------------------------------------------------------------- by Inori at 2012-11-26T15:00:22Z is this final? if not, then +1 for message keys --------------------------------------------------------------------------- by vicb at 2012-11-27T22:33:47Z @fabpot I can't understand why we keep discussing this for months as this implementation use *both* keys and plain Englis, ie using keys is optional ( if it was not it would not be an issue according to symfony#6129) --------------------------------------------------------------------------- by asm89 at 2013-01-02T21:43:46Z @fabpot @vicb I'll rebase this PR, fix the comments and refactor the message keys to use plain English + {{ }} syntax for the placeholders. --------------------------------------------------------------------------- by asm89 at 2013-01-07T15:00:58Z @fabpot If I fix this tonight, will it make the beta? --------------------------------------------------------------------------- by fabpot at 2013-01-07T15:53:00Z yes, definitely. --------------------------------------------------------------------------- by asm89 at 2013-01-07T20:13:38Z @fabpot I switched the implementation to English messages instead of message keys and fixed the final comments + rebased. Anything you want me to do after this? Still happy with `getMessageKey()`?
2 parents b981a6f + 73db84f commit 3e99f4e

31 files changed

+372
-33
lines changed

‎src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -527,6 +527,11 @@ private function registerTranslatorConfiguration(array $config, ContainerBuilder
527527

528528
$dirs[] = dirname($r->getFilename()).'/Resources/translations';
529529
}
530+
if (class_exists('Symfony\Component\Security\Core\Exception\AuthenticationException')) {
531+
$r = new \ReflectionClass('Symfony\Component\Security\Core\Exception\AuthenticationException');
532+
533+
$dirs[] = dirname($r->getFilename()).'/../../Resources/translations';
534+
}
530535
$overridePath = $container->getParameter('kernel.root_dir').'/Resources/%s/translations';
531536
foreach ($container->getParameter('kernel.bundles') as $bundle => $class) {
532537
$reflection = new \ReflectionClass($class);

‎src/Symfony/Bundle/FrameworkBundle/Tests/DependencyInjection/FrameworkExtensionTest.php

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -205,6 +205,11 @@ public function testTranslator()
205205
$files,
206206
'->registerTranslatorConfiguration() finds Form translation resources'
207207
);
208+
$this->assertContains(
209+
'Symfony/Component/Security/Resources/translations/security.en.xlf',
210+
$files,
211+
'->registerTranslatorConfiguration() finds Security translation resources'
212+
);
208213

209214
$calls = $container->getDefinition('translator.default')->getMethodCalls();
210215
$this->assertEquals('fr', $calls[0][1][0]);

‎src/Symfony/Bundle/FrameworkBundle/composer.json

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,8 @@
2929
"doctrine/common": ">=2.2,<2.4-dev"
3030
},
3131
"require-dev": {
32-
"symfony/finder": "2.2.*"
32+
"symfony/finder": "2.2.*",
33+
"symfony/security": "2.2.*"
3334
},
3435
"suggest": {
3536
"symfony/console": "2.2.*",

‎src/Symfony/Component/Security/CHANGELOG.md

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -34,3 +34,8 @@ CHANGELOG
3434
`AbstractAuthenticationListener` has changed.
3535
* [BC BREAK] moved the default logout success handling to a separate class. The
3636
order of arguments in the constructor of `LogoutListener` has changed.
37+
* [BC BREAK] The constructor of `AuthenticationException` and all child
38+
classes now matches the constructor of `\Exception`. The extra information
39+
getters and setters are removed. There are now dedicated getters/setters for
40+
token (`AuthenticationException'), user (`AccountStatusException`) and
41+
username (`UsernameNotFoundException`).

‎src/Symfony/Component/Security/Core/Authentication/AuthenticationProviderManager.php

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -77,7 +77,7 @@ public function authenticate(TokenInterface $token)
7777
break;
7878
}
7979
} catch (AccountStatusException $e) {
80-
$e->setExtraInformation($token);
80+
$e->setToken($token);
8181

8282
throw $e;
8383
} catch (AuthenticationException $e) {
@@ -105,7 +105,7 @@ public function authenticate(TokenInterface $token)
105105
$this->eventDispatcher->dispatch(AuthenticationEvents::AUTHENTICATION_FAILURE, new AuthenticationFailureEvent($token, $lastException));
106106
}
107107

108-
$lastException->setExtraInformation($token);
108+
$lastException->setToken($token);
109109

110110
throw $lastException;
111111
}

‎src/Symfony/Component/Security/Core/Authentication/Provider/DaoAuthenticationProvider.php

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -88,9 +88,12 @@ protected function retrieveUser($username, UsernamePasswordToken $token)
8888

8989
return $user;
9090
} catch (UsernameNotFoundException $notFound) {
91+
$notFound->setUsername($username);
9192
throw $notFound;
9293
} catch (\Exception $repositoryProblem) {
93-
throw new AuthenticationServiceException($repositoryProblem->getMessage(), $token, 0, $repositoryProblem);
94+
$ex = new AuthenticationServiceException($repositoryProblem->getMessage(), 0, $repositoryProblem);
95+
$ex->setToken($token);
96+
throw $ex;
9497
}
9598
}
9699
}

‎src/Symfony/Component/Security/Core/Authentication/Provider/UserAuthenticationProvider.php

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -71,6 +71,7 @@ public function authenticate(TokenInterface $token)
7171
if ($this->hideUserNotFoundExceptions) {
7272
throw new BadCredentialsException('Bad credentials', 0, $notFound);
7373
}
74+
$notFound->setUsername($username);
7475

7576
throw $notFound;
7677
}

‎src/Symfony/Component/Security/Core/Exception/AccountExpiredException.php

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,15 @@
1515
* AccountExpiredException is thrown when the user account has expired.
1616
*
1717
* @author Fabien Potencier <fabien@symfony.com>
18+
* @author Alexander <iam.asm89@gmail.com>
1819
*/
1920
class AccountExpiredException extends AccountStatusException
2021
{
22+
/**
23+
* {@inheritDoc}
24+
*/
25+
public function getMessageKey()
26+
{
27+
return 'Account has expired.';
28+
}
2129
}

‎src/Symfony/Component/Security/Core/Exception/AccountStatusException.php

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,12 +11,57 @@
1111

1212
namespace Symfony\Component\Security\Core\Exception;
1313

14+
use Symfony\Component\Security\Core\User\UserInterface;
15+
1416
/**
1517
* AccountStatusException is the base class for authentication exceptions
1618
* caused by the user account status.
1719
*
1820
* @author Fabien Potencier <fabien@symfony.com>
21+
* @author Alexander <iam.asm89@gmail.com>
1922
*/
2023
abstract class AccountStatusException extends AuthenticationException
2124
{
25+
private $user;
26+
27+
/**
28+
* Get the user.
29+
*
30+
* @return UserInterface
31+
*/
32+
public function getUser()
33+
{
34+
return $this->user;
35+
}
36+
37+
/**
38+
* Set the user.
39+
*
40+
* @param UserInterface $user
41+
*/
42+
public function setUser(UserInterface $user)
43+
{
44+
$this->user = $user;
45+
}
46+
47+
/**
48+
* {@inheritDoc}
49+
*/
50+
public function serialize()
51+
{
52+
return serialize(array(
53+
$this->user,
54+
parent::serialize(),
55+
));
56+
}
57+
58+
/**
59+
* {@inheritDoc}
60+
*/
61+
public function unserialize($str)
62+
{
63+
list($this->user, $parentData) = unserialize($str);
64+
65+
parent::unserialize($parentData);
66+
}
2267
}

‎src/Symfony/Component/Security/Core/Exception/AuthenticationCredentialsNotFoundException.php

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,15 @@
1616
* because no Token is available.
1717
*
1818
* @author Fabien Potencier <fabien@symfony.com>
19+
* @author Alexander <iam.asm89@gmail.com>
1920
*/
2021
class AuthenticationCredentialsNotFoundException extends AuthenticationException
2122
{
23+
/**
24+
* {@inheritDoc}
25+
*/
26+
public function getMessageKey()
27+
{
28+
return 'Authentication credentials could not be found.';
29+
}
2230
}

0 commit comments

Comments
 (0)
0