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

[Security] Use auth trust resolver to determine anonymous in ContextListener #18211

Closed

Conversation

wouterj
Copy link
Member

@wouterj wouterj commented Mar 17, 2016

Q A
Branch? master
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets -
License MIT
Doc PR not done yet

There is a nice class in Symfony that is used to check whether a token is anonymously: AuthenticationTrustResolver. However, its logic was still hard coded in the ContextListener, making it impossible to customize it (e.g. using another anonymous token class). I think it makes lots of sense to use the dedicated class.

@@ -15,6 +15,8 @@
use Symfony\Component\HttpKernel\Event\GetResponseEvent;
use Symfony\Component\HttpKernel\Event\FilterResponseEvent;
use Symfony\Component\HttpKernel\KernelEvents;
use Symfony\Component\Security\Core\Authentication\AuthenticationTrustResolver;
use Symfony\Component\Security\Core\Authentication\AuthenticationTrustResolverInterface;
use Symfony\Component\Security\Core\Authentication\Token\AnonymousToken;
Copy link
Member

Choose a reason for hiding this comment

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

This class is no longer used.

Copy link
Member Author

Choose a reason for hiding this comment

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

thanks, fixed

@wouterj wouterj force-pushed the security-remember_me_trust_resolver branch from c2e4d54 to a695652 Compare March 17, 2016 11:15
@@ -121,7 +124,7 @@ public function onKernelResponse(FilterResponseEvent $event)
$request = $event->getRequest();
$session = $request->getSession();

if ((null === $token = $this->tokenStorage->getToken()) || ($token instanceof AnonymousToken)) {
if ((null === $token = $this->tokenStorage->getToken()) || ($this->trustResolver->isAnonymous($token))) {
Copy link
Member

Choose a reason for hiding this comment

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

We can remove one pair of parentheses here.

Copy link
Member Author

Choose a reason for hiding this comment

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

done

@@ -58,6 +60,7 @@ public function __construct(TokenStorageInterface $tokenStorage, array $userProv
$this->sessionKey = '_security_'.$contextKey;
$this->logger = $logger;
$this->dispatcher = $dispatcher;
$this->trustResolver = $trustResolver ?: new AuthenticationTrustResolver('Symfony\Component\Security\Core\Authentication\Token\AnonymousToken', 'Symfony\Component\Security\Core\Authentication\Token\RememberMeToken');
Copy link
Member

Choose a reason for hiding this comment

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

You could use ::class constants here

Copy link
Contributor

Choose a reason for hiding this comment

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

agree

@fabpot
Copy link
Member

fabpot commented Mar 18, 2016

👍

1 similar comment
@xabbuh
Copy link
Member

xabbuh commented Mar 22, 2016

👍

@fabpot
Copy link
Member

fabpot commented Mar 23, 2016

Thanks @wouterj

@fabpot fabpot closed this Mar 23, 2016
fabpot added a commit that referenced this pull request Mar 23, 2016
…ous in ContextListener (WouterJ)

This PR was squashed before being merged into the 3.1-dev branch (closes #18211).

Discussion
----------

[Security] Use auth trust resolver to determine anonymous in ContextListener

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

There is a nice class in Symfony that is used to check whether a token is anonymously: `AuthenticationTrustResolver`. However, its logic was still hard coded in the `ContextListener`, making it impossible to customize it (e.g. using another anonymous token class). I think it makes lots of sense to use the dedicated class.

Commits
-------

ab5578e [Security] Use auth trust resolver to determine anonymous in ContextListener
nicolas-grekas added a commit that referenced this pull request Mar 23, 2016
This PR was merged into the 3.1-dev branch.

Discussion
----------

use class constants instead of FQCN strings

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #18211 (comment)
| License       | MIT
| Doc PR        |

Commits
-------

d4ec7dd use class constants instead of FQCN strings
@wouterj wouterj deleted the security-remember_me_trust_resolver branch March 23, 2016 20:19
@fabpot fabpot mentioned this pull request May 13, 2016
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