Skip to content

Commit

Permalink
feature #36574 [Security] Removed anonymous in the new security syste…
Browse files Browse the repository at this point in the history
…m (wouterj)

This PR was merged into the 5.1-dev branch.

Discussion
----------

[Security] Removed anonymous in the new security system

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

This was one of the "Future considerations" of #33558:

> Drop the AnonymousToken and AnonymousAuthenticator: Anonymous authentication has never made much sense and complicates things (e.g. the user can be a string). For access control, an anonymous user has the same meaning as an un-authenticated one (null). This require changes in the AccessListener and AuthorizationChecker and probably also a new Security attribute (to replace IS_AUTHENTICATED_ANONYMOUSLY). Related issues: #34909, #30609

This new experimental system is probably a once-in-a-lifetime change to make this change. @weaverryan and I have had some brainstorming about this. Some reasons why we think it makes 100% sense to do this change:

* From a Security perspective, **a user that is not authenticated is similar to an "unknown" user**: They both have no rights at all.
* **The higher level consequences of the AnonymousToken are confusing and inconsistent**:
  * It's hard to explain people new to Symfony Security that not being logged in still means you're authenticated within the Symfony app
  * To counter this, some higher level APIs explicitly mark anonymous tokens as not being authenticated, see e.g. the [`is_authenticated()` expression language function](https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Security/Core/Authorization/ExpressionLanguageProvider.php#L33-L37)
  * The anonymous authentication resulted in the `IS_AUTHENTICATED` security attribute being removed from #35854, as there was no clear consensus on what its meaning should be
* **Spring Security, which is where this originated from, makes Anonymous a very special case**:

  > Finally, there is an AnonymousAuthenticationFilter, which is chained after the normal authentication mechanisms and automatically adds an AnonymousAuthenticationToken to the SecurityContextHolder if there is no existing Authentication held there.
  >
  > Note that there is no real conceptual difference between a user who is “anonymously authenticated” and an unauthenticated user. Spring Security's anonymous authentication just gives you a more convenient way to configure your access-control attributes. Calls to servlet API calls such as getCallerPrincipal, for example, will still return null even though there is actually an anonymous authentication object in the SecurityContextHolder.
* Symfony uses AnonymousToken much more than "just for convience in access-control attributes". **Removing anonymous tokens allows us to move towards only allowing `UserInterface` users**: #34909

---

Removing anonymous tokens do have an impact on `AccessListener` and `AuthorizationChecker`. These currently throw an exception if there is no token in the storage, instead of treating them like "unknown users" (i.e. no roles). See #30609 on a RFC about removing this exception. We can also see e.g. the [Twig `is_granted()` function explicitly catching this exception](https://github.com/symfony/symfony/blob/master/src/Symfony/Bridge/Twig/Extension/SecurityExtension.php#L37-L52).

* **To make the changes in `AccessListener` and `AuthorizationChecker` BC, a flag has been added - default enabled - to throw an exception when no token is present** (which is automatically disabled when the new system is used). In Symfony 5.4 (or whenever the new system is no longer experimental), we can deprecate this flag and in 6.0 we can never throw the exception anymore.
* **`anonymous: lazy` has been deprecated in favor of `{ anonymous: true, lazy: true }`** This fixes the dependency on `AnonymousFactory` from the `SecurityExtension` and allows removing the `anonymous` option.
* **Introduced `PUBLIC_ACCESS` Security attribute** as alternative of `IS_AUTHENTICATED_ANONYMOUSLY`. Both work in the new system, the latter only triggers a deprecation notice (but may be usefull to allow switching back and forth between old and new system).

cc @javiereguiluz you might be interested, as I recently talked with you about this topic

Commits
-------

ac84a6c Removed AnonymousToken from the authenticator system
  • Loading branch information
fabpot committed May 3, 2020
2 parents 28bb74c + ac84a6c commit 09f9079
Show file tree
Hide file tree
Showing 14 changed files with 142 additions and 158 deletions.
18 changes: 18 additions & 0 deletions UPGRADE-5.1.md
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,24 @@ Routing
SecurityBundle
--------------

* Deprecated `anonymous: lazy` in favor of `lazy: true`

*Before*
```yaml
security:
firewalls:
main:
anonymous: lazy
```

*After*
```yaml
security:
firewalls:
main:
anonymous: true
lazy: true
```
* Marked the `AnonymousFactory`, `FormLoginFactory`, `FormLoginLdapFactory`, `GuardAuthenticationFactory`,
`HttpBasicFactory`, `HttpBasicLdapFactory`, `JsonLoginFactory`, `JsonLoginLdapFactory`, `RememberMeFactory`, `RemoteUserFactory`
and `X509Factory` as `@internal`. Instead of extending these classes, create your own implementation based on
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -197,6 +197,7 @@ private function addFirewallsSection(ArrayNodeDefinition $rootNode, array $facto
->scalarNode('entry_point')->end()
->scalarNode('provider')->end()
->booleanNode('stateless')->defaultFalse()->end()
->booleanNode('lazy')->defaultFalse()->end()
->scalarNode('context')->cannotBeEmpty()->end()
->arrayNode('logout')
->treatTrueLike([])
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
namespace Symfony\Bundle\SecurityBundle\DependencyInjection\Security\Factory;

use Symfony\Component\Config\Definition\Builder\NodeDefinition;
use Symfony\Component\Config\Definition\Exception\InvalidConfigurationException;
use Symfony\Component\DependencyInjection\ChildDefinition;
use Symfony\Component\DependencyInjection\ContainerBuilder;
use Symfony\Component\DependencyInjection\Parameter;
Expand Down Expand Up @@ -46,16 +47,7 @@ public function create(ContainerBuilder $container, $id, $config, $userProvider,

public function createAuthenticator(ContainerBuilder $container, string $firewallName, array $config, string $userProviderId): string
{
if (null === $config['secret']) {
$config['secret'] = new Parameter('container.build_hash');
}

$authenticatorId = 'security.authenticator.anonymous.'.$firewallName;
$container
->setDefinition($authenticatorId, new ChildDefinition('security.authenticator.anonymous'))
->replaceArgument(0, $config['secret']);

return $authenticatorId;
throw new InvalidConfigurationException(sprintf('The authenticator manager no longer has "anonymous" security. Please remove this option under the "%s" firewall'.($config['lazy'] ? ' and add "lazy: true"' : '').'.', $firewallName));
}

public function getPosition()
Expand All @@ -76,7 +68,7 @@ public function addConfiguration(NodeDefinition $builder)
->then(function ($v) { return ['lazy' => true]; })
->end()
->children()
->booleanNode('lazy')->defaultFalse()->end()
->booleanNode('lazy')->defaultFalse()->setDeprecated('symfony/security-bundle', '5.1', 'Using "anonymous: lazy" to make the firewall lazy is deprecated, use "lazy: true" instead.')->end()
->scalarNode('secret')->defaultNull()->end()
->end()
;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,13 @@ public function load(array $configs, ContainerBuilder $container)

if ($this->authenticatorManagerEnabled = $config['enable_authenticator_manager']) {
$loader->load('security_authenticator.xml');

// The authenticator system no longer has anonymous tokens. This makes sure AccessListener
// and AuthorizationChecker do not throw AuthenticationCredentialsNotFoundException when no
// token is available in the token storage.
$container->getDefinition('security.access_listener')->setArgument(4, false);
$container->getDefinition('security.authorization_checker')->setArgument(4, false);
$container->getDefinition('security.authorization_checker')->setArgument(5, false);
} else {
$loader->load('security_legacy.xml');
}
Expand Down Expand Up @@ -269,7 +276,8 @@ private function createFirewalls(array $config, ContainerBuilder $container)
list($matcher, $listeners, $exceptionListener, $logoutListener) = $this->createFirewall($container, $name, $firewall, $authenticationProviders, $providerIds, $configId);

$contextId = 'security.firewall.map.context.'.$name;
$context = new ChildDefinition($firewall['stateless'] || empty($firewall['anonymous']['lazy']) ? 'security.firewall.context' : 'security.firewall.lazy_context');
$isLazy = !$firewall['stateless'] && (!empty($firewall['anonymous']['lazy']) || $firewall['lazy']);
$context = new ChildDefinition($isLazy ? 'security.firewall.lazy_context' : 'security.firewall.context');
$context = $container->setDefinition($contextId, $context);
$context
->replaceArgument(0, new IteratorArgument($listeners))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -111,13 +111,6 @@
<argument type="service" id="property_accessor" on-invalid="null" />
</service>

<service id="security.authenticator.anonymous"
class="Symfony\Component\Security\Http\Authenticator\AnonymousAuthenticator"
abstract="true">
<argument type="abstract">secret</argument>
<argument type="service" id="security.untracked_token_storage" />
</service>

<service id="security.authenticator.remember_me"
class="Symfony\Component\Security\Http\Authenticator\RememberMeAuthenticator"
abstract="true">
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,8 @@ security:
firewalls:
secure:
pattern: ^/
anonymous: lazy
anonymous: ~
lazy: true
stateless: false
guard:
authenticators:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,8 @@ security:
check_path: /login_check
default_target_path: /profile
logout: ~
anonymous: lazy
anonymous: ~
lazy: true

# This firewall is here just to check its the logout functionality
second_area:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,24 +29,30 @@ class AuthorizationChecker implements AuthorizationCheckerInterface
private $accessDecisionManager;
private $authenticationManager;
private $alwaysAuthenticate;
private $exceptionOnNoToken;

public function __construct(TokenStorageInterface $tokenStorage, AuthenticationManagerInterface $authenticationManager, AccessDecisionManagerInterface $accessDecisionManager, bool $alwaysAuthenticate = false)
public function __construct(TokenStorageInterface $tokenStorage, AuthenticationManagerInterface $authenticationManager, AccessDecisionManagerInterface $accessDecisionManager, bool $alwaysAuthenticate = false, bool $exceptionOnNoToken = true)
{
$this->tokenStorage = $tokenStorage;
$this->authenticationManager = $authenticationManager;
$this->accessDecisionManager = $accessDecisionManager;
$this->alwaysAuthenticate = $alwaysAuthenticate;
$this->exceptionOnNoToken = $exceptionOnNoToken;
}

/**
* {@inheritdoc}
*
* @throws AuthenticationCredentialsNotFoundException when the token storage has no authentication token
* @throws AuthenticationCredentialsNotFoundException when the token storage has no authentication token and $exceptionOnNoToken is set to true
*/
final public function isGranted($attribute, $subject = null): bool
{
if (null === ($token = $this->tokenStorage->getToken())) {
throw new AuthenticationCredentialsNotFoundException('The token storage contains no authentication token. One possible reason may be that there is no firewall configured for this URL.');
if ($this->exceptionOnNoToken) {
throw new AuthenticationCredentialsNotFoundException('The token storage contains no authentication token. One possible reason may be that there is no firewall configured for this URL.');
}

return false;
}

if ($this->alwaysAuthenticate || !$token->isAuthenticated()) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,13 @@ public function testVoteWithoutAuthenticationToken()
$this->authorizationChecker->isGranted('ROLE_FOO');
}

public function testVoteWithoutAuthenticationTokenAndExceptionOnNoTokenIsFalse()
{
$authorizationChecker = new AuthorizationChecker($this->tokenStorage, $this->authenticationManager, $this->accessDecisionManager, false, false);

$this->assertFalse($authorizationChecker->isGranted('ROLE_FOO'));
}

/**
* @dataProvider isGrantedProvider
*/
Expand Down

This file was deleted.

49 changes: 38 additions & 11 deletions src/Symfony/Component/Security/Http/Firewall/AccessListener.php
Original file line number Diff line number Diff line change
Expand Up @@ -31,17 +31,21 @@
*/
class AccessListener extends AbstractListener
{
const PUBLIC_ACCESS = 'PUBLIC_ACCESS';

private $tokenStorage;
private $accessDecisionManager;
private $map;
private $authManager;
private $exceptionOnNoToken;

public function __construct(TokenStorageInterface $tokenStorage, AccessDecisionManagerInterface $accessDecisionManager, AccessMapInterface $map, AuthenticationManagerInterface $authManager)
public function __construct(TokenStorageInterface $tokenStorage, AccessDecisionManagerInterface $accessDecisionManager, AccessMapInterface $map, AuthenticationManagerInterface $authManager, bool $exceptionOnNoToken = true)
{
$this->tokenStorage = $tokenStorage;
$this->accessDecisionManager = $accessDecisionManager;
$this->map = $map;
$this->authManager = $authManager;
$this->exceptionOnNoToken = $exceptionOnNoToken;
}

/**
Expand All @@ -52,18 +56,18 @@ public function supports(Request $request): ?bool
[$attributes] = $this->map->getPatterns($request);
$request->attributes->set('_access_control_attributes', $attributes);

return $attributes && [AuthenticatedVoter::IS_AUTHENTICATED_ANONYMOUSLY] !== $attributes ? true : null;
return $attributes && ([AuthenticatedVoter::IS_AUTHENTICATED_ANONYMOUSLY] !== $attributes && [self::PUBLIC_ACCESS] !== $attributes) ? true : null;
}

/**
* Handles access authorization.
*
* @throws AccessDeniedException
* @throws AuthenticationCredentialsNotFoundException
* @throws AuthenticationCredentialsNotFoundException when the token storage has no authentication token and $exceptionOnNoToken is set to true
*/
public function authenticate(RequestEvent $event)
{
if (!$event instanceof LazyResponseEvent && null === $token = $this->tokenStorage->getToken()) {
if (!$event instanceof LazyResponseEvent && null === ($token = $this->tokenStorage->getToken()) && $this->exceptionOnNoToken) {
throw new AuthenticationCredentialsNotFoundException('A Token was not found in the TokenStorage.');
}

Expand All @@ -76,8 +80,26 @@ public function authenticate(RequestEvent $event)
return;
}

if ($event instanceof LazyResponseEvent && null === $token = $this->tokenStorage->getToken()) {
throw new AuthenticationCredentialsNotFoundException('A Token was not found in the TokenStorage.');
if ($event instanceof LazyResponseEvent) {
$token = $this->tokenStorage->getToken();
}

if (null === $token) {
if ($this->exceptionOnNoToken) {
throw new AuthenticationCredentialsNotFoundException('A Token was not found in the TokenStorage.');
}

if ([AuthenticatedVoter::IS_AUTHENTICATED_ANONYMOUSLY] === $attributes) {
trigger_deprecation('symfony/security-http', '5.1', 'Using "IS_AUTHENTICATED_ANONYMOUSLY" in your access_control rules when using the authenticator Security system is deprecated, use "PUBLIC_ACCESS" instead.');

return;
}

if ([self::PUBLIC_ACCESS] === $attributes) {
return;
}

throw $this->createAccessDeniedException($request, $attributes);
}

if (!$token->isAuthenticated()) {
Expand All @@ -86,11 +108,16 @@ public function authenticate(RequestEvent $event)
}

if (!$this->accessDecisionManager->decide($token, $attributes, $request, true)) {
$exception = new AccessDeniedException();
$exception->setAttributes($attributes);
$exception->setSubject($request);

throw $exception;
throw $this->createAccessDeniedException($request, $attributes);
}
}

private function createAccessDeniedException(Request $request, array $attributes)
{
$exception = new AccessDeniedException();
$exception->setAttributes($attributes);
$exception->setSubject($request);

return $exception;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,9 @@ private function handleAccessDeniedException(ExceptionEvent $event, AccessDenied

try {
$insufficientAuthenticationException = new InsufficientAuthenticationException('Full authentication is required to access this resource.', 0, $exception);
$insufficientAuthenticationException->setToken($token);
if (null !== $token) {
$insufficientAuthenticationException->setToken($token);
}

$event->setResponse($this->startAuthentication($event->getRequest(), $insufficientAuthenticationException));
} catch (\Exception $e) {
Expand Down
Loading

0 comments on commit 09f9079

Please sign in to comment.