8000 [Security] Require entry_point to be configured with multiple authenticators by wouterj · Pull Request #36575 · symfony/symfony · GitHub
[go: up one dir, main page]

Skip to content

[Security] Require entry_point to be configured with multiple authenticators #36575

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
Apr 30, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions UPGRADE-5.1.md
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,14 @@ Routing
* Added argument `$priority` to `RouteCollection::add()`
* Deprecated the `RouteCompiler::REGEX_DELIMITER` constant

SecurityBundle
--------------

* Marked the `AbstractFactory`, `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
`SecurityFactoryInterface`.

Security
--------

Expand Down
2 changes: 2 additions & 0 deletions src/Symfony/Bundle/SecurityBundle/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@ CHANGELOG

* Added XSD for configuration
* Added security configuration for priority-based access decision strategy
* Marked the `AbstractFactory`, `AnonymousFactory`, `FormLoginFactory`, `FormLoginLdapFactory`, `GuardAuthenticationFactory`, `HttpBasicFactory`, `HttpBasicLdapFactory`, `JsonLoginFactory`, `JsonLoginLdapFactory`, `RememberMeFactory`, `RemoteUserFactory` and `X509Factory` as `@internal`
* Renamed method `AbstractFactory#createEntryPoint()` to `AbstractFactory#createDefaultEntryPoint()`

5.0.0
-----
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@
* @author Fabien Potencier <fabien@symfony.com>
* @author Lukas Kahwe Smith <smith@pooteeweet.org>
* @author Johannes M. Schmitt <schmittjoh@gmail.com>
*
* @internal
*/
abstract class AbstractFactory implements SecurityFactoryInterface
{
Expand Down Expand Up @@ -65,7 +67,7 @@ public function create(ContainerBuilder $container, string $id, array $config, s
}

// create entry point if applicable (optional)
$entryPointId = $this->createEntryPoint($container, $id, $config, $defaultEntryPointId);
$entryPointId = $this->createDefaultEntryPoint($container, $id, $config, $defaultEntryPointId);
Copy link
Member

Choose a reason for hiding this comment

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

This is the BC break that makes my code fail now. AbstractFactory is what we use in Connect Bundle: https://github.com/symfonycorp/connect-bundle/blob/master/src/DependencyInjection/Security/Factory/ConnectFactory.php

/cc @wouterj

So, even if I can probably fix it in Connect, I fear that we won't be alone by this problem. Anyone not using Guard is probably extending AbstractFactory. That means that we cannot mark AbstractFactory as @internal.

Copy link
Member Author
@wouterj wouterj May 2, 2020

Choose a reason for hiding this comment

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

Thanks for debugging!
That also means we have to find another method name for the EntryPointInterface interface, to not conflict with the abstract method in this class. I'll do a PR later this evening. See #36661


return [$authProviderId, $listenerId, $entryPointId];
}
Expand Down Expand Up @@ -126,7 +128,7 @@ abstract protected function getListenerId();
*
* @return string|null the entry point id
*/
protected function createEntryPoint(ContainerBuilder $container, string $id, array $config, ?string $defaultEntryPointId)
protected function createDefaultEntryPoint(ContainerBuilder $container, string $id, array $config, ?string $defaultEntryPointId)
{
return $defaultEntryPointId;
}
Expand Down
EDBE
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@

/**
* @author Wouter de Jong <wouter@wouterj.nl>
*
* @internal
*/
class AnonymousFactory implements SecurityFactoryInterface, AuthenticatorFactoryInterface
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,12 @@
use Symfony\Component\Config\Definition\Builder\NodeDefinition;
use Symfony\Component\DependencyInjection\ContainerBuilder;

/**
* @author Wouter de Jong <wouter@wouterj.nl>
*
* @internal
* @experimental in Symfony 5.1
*/
class CustomAuthenticatorFactory implements AuthenticatorFactoryInterface, SecurityFactoryInterface
{
public function create(ContainerBuilder $container, string $id, array $config, string $userProvider, ?string $defaultEntryPoint)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,5 +23,5 @@ interface EntryPointFactoryInterface
/**
* Creates the entry point and returns the service ID.
*/
public function createEntryPoint(ContainerBuilder $container, string $id, array $config, ?string $defaultEntryPointId): string;
public function createEntryPoint(ContainerBuilder $container, string $id, array $config): ?string;
}
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@
*
* @author Fabien Potencier <fabien@symfony.com>
* @author Johannes M. Schmitt <schmittjoh@gmail.com>
*
* @internal
*/
class FormLoginFactory extends AbstractFactory implements AuthenticatorFactoryInterface, EntryPointFactoryInterface
{
Expand Down Expand Up @@ -90,7 +92,12 @@ protected function createListener(ContainerBuilder $container, string $id, array
return $listenerId;
}

public function createEntryPoint(ContainerBuilder $container, string $id, array $config, ?string $defaultEntryPoint): string
protected function createDefaultEntryPoint(ContainerBuilder $container, string $id, array $config, ?string $defaultEntryPointId)
{
return $this->createEntryPoint($container, $id, $config);
}

public function createEntryPoint(ContainerBuilder $container, string $id, array $config): string
{
$entryPointId = 'security.authentication.form_entry_point.'.$id;
$container
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@
*
* @author Grégoire Pineau <lyrixx@lyrixx.info>
* @author Charles Sarrazin <charles@sarraz.in>
*
* @internal
*/
class FormLoginLdapFactory extends FormLoginFactory
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@
* Configures the "guard" authentication provider key under a firewall.
*
* @author Ryan Weaver <ryan@knpuniversity.com>
*
* @internal
*/
class GuardAuthenticationFactory implements SecurityFactoryInterface, AuthenticatorFactoryInterface, EntryPointFactoryInterface
{
Expand Down Expand Up @@ -111,9 +113,15 @@ public function createAuthenticator(ContainerBuilder $container, string $firewal
return $authenticatorIds;
}

public function createEntryPoint(ContainerBuilder $container, string $id, array $config, ?string $defaultEntryPointId): string
public function createEntryPoint(ContainerBuilder $container, string $id, array $config): ?string
{
return $this->determineEntryPoint($defaultEntryPointId, $config);
try {
return $this->determineEntryPoint(null, $config);
} catch (\LogicException $e) {
// ignore the exception, the new system prefers setting "entry_point" over "guard.entry_point"
}

return null;
}

private function determineEntryPoint(?string $defaultEntryPointId, array $config): string
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,10 @@
* HttpBasicFactory creates services for HTTP basic authentication.
*
* @author Fabien Potencier <fabien@symfony.com>
*
* @internal
*/
class HttpBasicFactory implements SecurityFactoryInterface, AuthenticatorFactoryInterface
class HttpBasicFactory implements SecurityFactoryInterface, AuthenticatorFactoryInterface, EntryPointFactoryInterface
{
public function create(ContainerBuilder $container, string $id, array $config, string $userProvider, ?string $defaultEntryPoint)
{
Expand All @@ -34,7 +36,10 @@ public function create(ContainerBuilder $container, string $id, array $config, s
;

// entry point
$entryPointId = $this->createEntryPoint($container, $id, $config, $defaultEntryPoint);
$entryPointId = $defaultEntryPoint;
if (null === $entryPointId) {
$entryPointId = $this->createEntryPoint($container, $id, $config);
}

// listener
$listenerId = 'security.authentication.listener.basic.'.$id;
Expand Down Expand Up @@ -77,12 +82,8 @@ public function addConfiguration(NodeDefinition $node)
;
}

protected function createEntryPoint(ContainerBuilder $container, string $id, array $config, ?string $defaultEntryPoint)
public function createEntryPoint(ContainerBuilder $container, string $id, array $config): string
{
if (null !== $defaultEntryPoint) {
return $defaultEntryPoint;
}

$entryPointId = 'security.authentication.basic_entry_point.'.$id;
$container
->setDefinition($entryPointId, new ChildDefinition('security.authentication.basic_entry_point'))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@
* @author Fabien Potencier <fabien@symfony.com>
* @author Grégoire Pineau <lyrixx@lyrixx.info>
* @author Charles Sarrazin <charles@sarraz.in>
*
* @internal
*/
class HttpBasicLdapFactory extends HttpBasicFactory
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@
* JsonLoginFactory creates services for JSON login authentication.
*
* @author Kévin Dunglas <dunglas@gmail.com>
*
* @internal
*/
class JsonLoginFactory extends AbstractFactory implements AuthenticatorFactoryInterface
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@

/**
* JsonLoginLdapFactory creates services for json login ldap authentication.
*
* @internal
*/
class JsonLoginLdapFactory extends JsonLoginFactory
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,9 @@
use Symfony\Component\HttpFoundation\Cookie;
use Symfony\Component\Security\Http\EventListener\RememberMeLogoutListener;

/**
* @internal
*/
class RememberMeFactory implements SecurityFactoryInterface, AuthenticatorFactoryInterface
{
protected $options = [
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,8 @@
*
* @author Fabien Potencier <fabien@symfony.com>
* @author Maxime Douailin <maxime.douailin@gmail.com>
*
* @internal
*/
class RemoteUserFactory implements SecurityFactoryInterface, AuthenticatorFactoryInterface
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@
* X509Factory creates services for X509 certificate authentication.
*
* @author Fabien Potencier <fabien@symfony.com>
*
* @internal
*/
class X509Factory implements SecurityFactoryInterface, AuthenticatorFactoryInterface
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@
use Symfony\Component\Security\Core\User\ChainUserProvider;
use Symfony\Component\Security\Core\User\UserProviderInterface;
use Symfony\Component\Security\Http\Controller\UserValueResolver;
use Symfony\Component\Security\Http\EntryPoint\AuthenticationEntryPointInterface;
use Twig\Extension\AbstractExtension;

/**
Expand Down Expand Up @@ -519,6 +520,7 @@ private function createAuthenticationListeners(ContainerBuilder $container, stri
{
$listeners = [];
$hasListeners = false;
$entryPoints = [];

foreach ($this->listenerPositions as $position) {
foreach ($this->factories[$position] as $factory) {
Expand All @@ -541,8 +543,8 @@ private function createAuthenticationListeners(ContainerBuilder $container, stri
$authenticationProviders[] = $authenticators;
}

if ($factory instanceof EntryPointFactoryInterface) {
$defaultEntryPoint = $factory->createEntryPoint($container, $id, $firewall[$key], $defaultEntryPoint);
if ($factory instanceof EntryPointFactoryInterface && ($entryPoint = $factory->createEntryPoint($container, $id, $firewall[$key], null))) {
$entryPoints[$key] = $entryPoint;
}
} else {
list($provider, $listenerId, $defaultEntryPoint) = $factory->create($container, $id, $firewall[$key], $userProvider, $defaultEntryPoint);
Expand All @@ -555,6 +557,19 @@ private function createAuthenticationListeners(ContainerBuilder $container, stri
}
}

if ($entryPoints) {
// we can be sure the authenticator system is enabled
if (null !== $defaultEntryPoint) {
return $entryPoints[$defaultEntryPoint] ?? $defaultEntryPoint;
}

if (1 === \count($entryPoints)) {
return current($entryPoints);
}

throw new InvalidConfigurationException(sprintf('Because you have multiple authenticators in firewall "%s", you need to set the "entry_point" key to one of your authenticators (%s) or a service ID implementing "%s". The "entry_point" determines what should happen (e.g. redirect to "/login") when an anonymous user tries to access a protected page.', $id, implode(', ', $entryPoints), AuthenticationEntryPointInterface::class));
}

if (false === $hasListeners) {
throw new InvalidConfigurationException(sprintf('No authentication listener registered for firewall "%s".', $id));
}
Expand Down
0