8000 [DX][SecurityBundle] Introduce a FirewallConfig class accessible from FirewallContext by chalasr · Pull Request #19398 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[DX][SecurityBundle] Introduce a FirewallConfig class accessible from FirewallContext #19398

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.

Sign up for GitHub

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
Nov 2, 2016
Merged

[DX][SecurityBundle] Introduce a FirewallConfig class accessible from FirewallContext #19398

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
49 changes: 47 additions & 2 deletions src/Symfony/Bundle/SecurityBundle/DependencyInjection/SecurityExtension.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,7 @@ public function load(array $configs, ContainerBuilder $container)
'Symfony\Component\Security\Core\Authorization\AccessDecisionManager',
'Symfony\Component\Security\Core\Authorization\AuthorizationChecker',
'Symfony\Component\Security\Core\Authorization\Voter\VoterInterface',
'Symfony\Bundle\SecurityBundle\Security\FirewallConfig',
'Symfony\Bundle\SecurityBundle\Security\FirewallMap',
'Symfony\Bundle\SecurityBundle\Security\FirewallContext',
'Symfony\Component\HttpFoundation\RequestMatcher',
Expand Down Expand Up @@ -236,14 +237,18 @@ private function createFirewalls($config, ContainerBuilder $container)
$mapDef = $container->getDefinition('security.firewall.map');
$map = $authenticationProviders = array();
foreach ($firewalls as $name => $firewall) {
list($matcher, $listeners, $exceptionListener) = $this->createFirewall($container, $name, $firewall, $authenticationProviders, $providerIds);
$configId = 'security.firewall.map.config.'.$name;

list($matcher, $listeners, $exceptionListener) = $this->createFirewall($container, $name, $firewall, $authenticationProviders, $providerIds, $configId);

$contextId = 'security.firewall.map.context.'.$name;
$context = $container->setDefinition($contextId, new DefinitionDecorator('security.firewall.context'));
$context
->replaceArgument(0, $listeners)
->replaceArgument(1, $exceptionListener)
->replaceArgument(2, new Reference($configId))
;

$map[$contextId] = $matcher;
}
$mapDef->replaceArgument(1, $map);
Expand All @@ -258,8 +263,11 @@ private function createFirewalls($config, ContainerBuilder $container)
;
}

private function createFirewall(ContainerBuilder $container, $id, $firewall, &$authenticationProviders, $providerIds)
private function createFirewall(ContainerBuilder $container, $id, $firewall, &$authenticationProviders, $providerIds, $configId)
{
$config = $container->setDefinition($configId, new DefinitionDecorator('security.firewall.config'));
$config->replaceArgument(0, $id);

// Matcher
$matcher = null;
if (isset($firewall['request_matcher'])) {
Expand All @@ -271,20 +279,28 @@ private function createFirewall(ContainerBuilder $container, $id, $firewall, &$a
$matcher = $this->createRequestMatcher($container, $pattern, $host, $methods);
}

$config->replaceArgument(1, (string) $matcher);
$config->replaceArgument(2, $firewall['security']);

// Security disabled?
if (false === $firewall['security']) {
return array($matcher, array(), null);
}

$config->replaceArgument(3, $firewall['stateless']);

// Provider id (take the first registered provider if none defined)
if (isset($firewall['provider'])) {
$defaultProvider = $this->getUserProviderId($firewall['provider']);
} else {
$defaultProvider = reset($providerIds);
}

$config->replaceArgument(4, $defaultProvider);

// Register listeners
$listeners = array();
$listenerKeys = array();

// Channel listener
$listeners[] = new Reference('security.channel_listener');
Expand All @@ -296,11 +312,14 @@ private function createFirewall(ContainerBuilder $container, $id, $firewall, &$a
$contextKey = $firewall['context'];
}

$config->replaceArgument(5, $contextKey);

$listeners[] = new Reference($this->createContextListener($container, $contextKey));
}

// Logout listener
if (isset($firewall['logout'])) {
$listenerKeys[] = 'logout';
$listenerId = 'security.logout_listener.'.$id;
$listener = $container->setDefinition($listenerId, new DefinitionDecorator('security.logout_listener'));
$listener->replaceArgument(3, array(
Expand Down Expand Up @@ -363,10 +382,13 @@ private function createFirewall(ContainerBuilder $container, $id, $firewall, &$a
// Authentication listeners
list($authListeners, $defaultEntryPoint) = $this->createAuthenticationListeners($container, $id, $firewall, $authenticationProviders, $defaultProvider, $configuredEntryPoint);

$config->replaceArgument(6, $configuredEntryPoint ?: $defaultEntryPoint);
Copy link
Contributor

Choose a reason for hiding this comment

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

missing

if (isset($firewall['anonymous'])) {
    $listenerConfigs['anonymous'] = $firewall['anonymous'];
}

for the anonymous listener registered in createAuthenticationListeners() ?

Copy link
Member Author
@chalasr chalasr Jul 26, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Added. Thanks

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe anonymous could be a boolean parameter of the config to be exposed in the collector.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good idea.
Could you give me your opinion about the FirewallConfig::$listenerConfigs property?
I'm not sure it is useful, values cannot be the final ones so it is pretty much a dump of what is written in yaml. Plus, I really don't see how I should display them into the profiler.

Copy link
Contributor
@ogizanagi ogizanagi Jul 30, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

As it is a native listener, I'll be in favor of adding a has/authorizes/acceptsAnonymous method indeed.

Could you give me your opinion about the FirewallConfig::$listenerConfigs property?

IMHO, as we've discussed about, as this is almost the raw configuration for listeners, we should probably not expose it but instead simply list enabled listeners for the given firewall. Thus you can simply show in the profiler the list of enabled listeners.

8000

Copy link
Contributor

Choose a reason for hiding this comment

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

As it is a native listener, I'll be in favor of adding a has/authorizes/acceptsAnonymous method indeed.

allowsAnonymous considering https://github.com/symfony/symfony/pull/19490/files/acd4ceac12c252adf7e8399f08d23ad6d1f46fee#r72892574

Copy link
Member Author

Choose a reason for hiding this comment

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

we should probably not expose it but instead simply list enabled listeners for the given firewall. Thus you can simply show in the profiler the list of enabled listeners.

I think you're right. Some sensible data such as passphrase, secret (i.e. anonymous), ... should probably not be displayed in the profiler (#19490).

Would you totally avoid storing the configuration of each registered listener and only store their names instead? Or keep it as is and just list array_keys($listenerConfigs) in #19490?

Copy link
Contributor
@ogizanagi ogizanagi Aug 1, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

I would totally avoid storing this config. Having the listeners config like this is not reliable. It'll only be if each listener were forced to register their own processed config themself in a FirewallListenerConfig instance. And use-cases are probably too marginal.

So, keeping the list of registered listeners name should be enough to me.


$listeners = array_merge($listeners, $authListeners);

// Switch user listener
if (isset($firewall['switch_user'])) {
$listenerKeys[] = 'switch_user';
$listeners[] = new Reference($this->createSwitchUserListener($container, $id, $firewall['switch_user'], $defaultProvider));
}

Expand All @@ -376,7 +398,30 @@ private function createFirewall(ContainerBuilder $container, $id, $firewall, &$a
// Exception listener
$exceptionListener = new Reference($this->createExceptionListener($container, $firewall, $id, $configuredEntryPoint ?: $defaultEntryPoint, $firewall['stateless']));

if (isset($firewall['access_denied_handler'])) {
$config->replaceArgument(7, $firewall['access_denied_handler']);
}
if (isset($firewall['access_denied_url'])) {
$config->replaceArgument(8, $firewall['access_denied_url']);
}

$container->setAlias(new Alias('security.user_checker.'.$id, false), $firewall['user_checker']);
$config->replaceArgument(9, $firewall['user_checker']);

foreach ($this->factories as $position) {
foreach ($position as $factory) {
$key = str_replace('-', '_', $factory->getKey());
if (array_key_exists($key, $firewall)) {
$listenerKeys[] = $key;
}
}
}

if (isset($firewall['anonymous'])) {
$listenerKeys[] = 'anonymous';
}

$config->replaceArgument(10, $listenerKeys);

return array($matcher, $listeners, $exceptionListener);
}
Expand Down
16 changes: 15 additions & 1 deletion src/Symfony/Bundle/SecurityBundle/Resources/config/security.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,21 @@
<service id="security.firewall.context" class="Symfony\Bundle\SecurityBundle\Security\FirewallContext" abstract="true">
<argument type="collection" />
<argument type="service" id="security.exception_listener" />
<argument /> <!-- FirewallConfig -->
</service>

<service id="security.firewall.config" class="Symfony\Bundle\SecurityBundle\Security\FirewallConfig" abstract="true" public="false">
<argument /> <!-- name -->
<argument /> <!-- request_matcher -->
<argument /> <!-- security enabled -->
<argument /> <!-- stateless -->
<argument /> <!-- provider -->
<argument /> <!-- context -->
<argument /> <!-- entry_point -->
<argument /> <!-- user_checker -->
<argument /> <!-- access_denied_handler -->
<argument /> <!-- access_denied_url -->
<argument type="collection" /> <!-- listeners -->
</service>

<service id="security.logout_url_generator" class="Symfony\Component\Security\Http\Logout\LogoutUrlGenerator" public="false">
Expand All @@ -119,7 +134,6 @@
<argument type="service" id="security.token_storage" on-invalid="null" />
</service>


<!-- Provisioning -->
<service id="security.user.provider.in_memory" class="Symfony\Component\Security\Core\User\InMemoryUserProvider" abstract="true" public="false" />
<service id="security.user.provider.in_memory.user" class="Symfony\Component\Security\Core\User\User" abstract="true" public="false" />
Expand Down
126 changes: 126 additions & 0 deletions src/Symfony/Bundle/SecurityBundle/Security/FirewallConfig.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,126 @@
<?php

/*
* This file is part of the Symfony package.
*
* (c) Fabien Potencier <fabien@symfony.com>
*
* For the full copyright and license information, please view the LICENSE
* file that was distributed with this source code.
*/

namespace Symfony\Bundle\SecurityBundle\Security;

/**
* @author Robin Chalas <robin.chalas@gmail.com>
*/
class FirewallConfig
{
private $name;
private $requestMatcher;
private $securityEnabled;
private $stateless;
private $provider;
private $context;
private $entryPoint;
private $accessDeniedHandler;
private $accessDeniedUrl;
private $userChecker;
private $listeners;

public function __construct($name, $requestMatcher, $securityEnabled = true, $stateless = false, $provider = null, $context = null, $entryPoint = null, $accessDeniedHandler = null, $accessDeniedUrl = null, $userChecker = null, $listeners = array())
{
$this->name = $name;
$this->requestMatcher = $requestMatcher;
$this->securityEnabled = $securityEnabled;
$this->stateless = $stateless;
$this->provider = $provider;
$this->context = $context;
$this->entryPoint = $entryPoint;
$this->accessDeniedHandler = $accessDeniedHandler;
$this->accessDeniedUrl = $accessDeniedUrl;
$this->userChecker = $userChecker;
$this->listeners = $listeners;
}

public function getName()
{
return $this->name;
}

/**
* @return string The request matcher service id
*/
public function getRequestMatcher()
{
return $this->requestMatcher;
}

public function isSecurityEnabled()
{
return $this->securityEnabled;
}

public function allowsAnonymous()
{
return in_array('anonymous', $this->listeners, true);
}

public function isStateless()
{
return $this->stateless;
}

/**
* @return string The provider service id
*/
public function getProvider()
{
return $this->provider;
}

/**
* @return string The context key
*/
public function getContext()
{
return $this->context;
}

/**
* @return string The entry_point service id
*/
public function getEntryPoint()
{
return $this->entryPoint;
}

/**
* @return string The user_checker service id
*/
public function getUserChecker()
{
return $this->userChecker;
}

/**
* @return string The access_denied_handler service id
*/
public function getAccessDeniedHandler()
{
return $this->accessDeniedHandler;
}

public function getAccessDeniedUrl()
{
return $this->accessDeniedUrl;
}

/**
* @return array An array of listener keys
*/
public function getListeners()
{
return $this->listeners;
}
}
13 changes: 12 additions & 1 deletion src/Symfony/Bundle/SecurityBundle/Security/FirewallContext.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,11 +23,22 @@ class FirewallContext
{
private $listeners;
private $exceptionListener;
private $config;

public function __construct(array $listeners, ExceptionListener $exceptionListener = null)
public function __construct(array $listeners, ExceptionListener $exceptionListener = null, FirewallConfig $config = null)
{
if (null === $config) {
@trigger_error(sprintf('"%s()" expects an instance of "%s" as third argument since version 3.2 and will trigger an error in 4.0 if not provided.', __METHOD__, FirewallConfig::class), E_USER_DEPRECATED);
}

$this->listeners = $listeners;
$this->exceptionListener = $exceptionListener;
$this->config = $config;
}

public function getConfig()
{
return $this->config;
}

public function getContext()
Expand Down
32 changes: 29 additions & 3 deletions src/Symfony/Bundle/SecurityBundle/Security/FirewallMap.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,18 +35,44 @@ public function __construct(ContainerInterface $container, array $map)
$this->contexts = new \SplObjectStorage();
}

/**
* {@inheritdoc}
*/
public function getListeners(Request $request)
{
$context = $this->getFirewallContext($request);

if (null === $context) {
return array(array(), null);
}

return $context->getContext();
}

/**
* @return FirewallConfig|null
*/
public function getFirewallConfig(Request $request)
{
$context = $this->getFirewallContext($request);

if (null === $context) {
return;
}

return $context->getConfig();
}

private function getFirewallContext(Request $request)
{
if ($this->contexts->contains($request)) {
return $this->contexts[$request];
}

foreach ($this->map as $contextId => $requestMatcher) {
if (null === $requestMatcher || $requestMatcher->matches($request)) {
return $this->contexts[$request] = $this->container->get($contextId)->getContext();
return $this->contexts[$request] = $this->container->get($contextId);
}
}

return array(array(), null);
}
}
Loading
0