8000 [security] Fetching current stored context when not explicitly specified by jaytaph · Pull Request #13342 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[security] Fetching current stored context when not explicitly specified #13342

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

Closed
wants to merge 3 commits into from
Closed

[security] Fetching current stored context when not explicitly specified #13342

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3fb5b35
Fetching current stored context when not explicitly specified
jaytaph Jan 9, 2015
f57a992
Removed default argument value on the helpers
jaytaph Jan 13, 2015
55704cf
Fixed casing
jaytaph Jan 17, 2015
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
Next Next commit
Fetching current stored context when not explicitly specified
  • Loading branch information
@jaytaph
jaytaph committed Jan 16, 2015
commit 3fb5b35f67de89a6c55db8c134c105e4bb2887c4
3 changes: 2 additions & 1 deletion src/Symfony/Bundle/SecurityBundle/Resources/config/templating_php.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,9 @@
<services>
<service id="templating.helper.logout_url" class="%templating.helper.logout_url.class%">
<tag name="templating.helper" alias="logout_url" />
<argument type="service" id="service_container" />
<argument type="service" id="request_stack" />
<argument type="service" id="router" />
<argument type="service" id="security.token_storage" />
</service>

<service id="templating.helper.security" class="%templating.helper.security.class%">
Expand Down
51 changes: 39 additions & 12 deletions src/Symfony/Bundle/SecurityBundle/Templating/Helper/LogoutUrlHelper.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,9 @@
use Symfony\Component\DependencyInjection\ContainerInterface;
use Symfony\Component\Form\Extension\Csrf\CsrfProvider\CsrfProviderAdapter;
use Symfony\Component\Form\Extension\Csrf\CsrfProvider\CsrfProviderInterface;
use Symfony\Component\HttpFoundation\RequestStack;
use Symfony\Component\Routing\Generator\UrlGeneratorInterface;
use Symfony\Component\Security\Core\Authentication\Token\Storage\TokenStorageInterface;
use Symfony\Component\Security\Csrf\CsrfTokenManagerInterface;
use Symfony\Component\Templating\Helper\Helper;

Expand All @@ -25,20 +27,33 @@
*/
class LogoutUrlHelper extends Helper
{
private $container;
private $requestStack;
private $listeners = array();
private $router;
private $tokenStorage;

/**
* Constructor.
*
* @param ContainerInterface $container A ContainerInterface instance
* @param UrlGeneratorInterface $router A Router instance
* @param ContainerInterface|RequestStack $requestStack A ContainerInterface instance or RequestStack
* @param UrlGeneratorInterface $router The router service
* @param TokenStorageInterface|null $tokenStorage The token storage service
*
* @deprecated Passing a ContainerInterface as a first argument is deprecated since 2.7 and will be removed in 3.0.
*/
public function __construct(ContainerInterface $container, UrlGeneratorInterface $router)
public function __construct($requestStack, UrlGeneratorInterface $router, TokenStorageInterface $tokenStorage = null)
{
$this->container = $container;
if ($requestStack instanceof ContainerInterface) {
$this->requestStack = $requestStack->get('request_stack');
trigger_error('The '.__CLASS__.' constructor will require a RequestStack instead of a containerInterface instance in 3.0.', E_USER_DEPRECATED);
Copy link
Contributor

Choose a reason for hiding this comment

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

/containerInterface/ContainerInterface/

} elseif ($requestStack instanceof RequestStack) {
$this->requestStack = $requestStack;
} else {
throw new \InvalidArgumentException(sprintf('%s takes either a RequestStack or a ContainerInterface object as its first argument.', __METHOD__));
}

$this->router = $router;
$this->tokenStorage = $tokenStorage;
}

/**
Expand All @@ -64,39 +79,51 @@ public function registerListener($key, $logoutPath, $csrfTokenId, $csrfParameter
/**
* Generates the absolute logout path for the firewall.
*
* @param string $key The firewall key
* @param string|null $key The firewall key or null to use the current firewall key
*
* @return string The logout path
*/
public function getLogoutPath($key)
public function getLogoutPath($key = null)
Copy link
Contributor

Choose a reason for hiding this comment

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

phpdoc needs update with string|null and description what it means

Copy link
Member

Choose a reason for hiding this comment

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

and this change must be documented in the UPGRADE file

Copy link
Contributor

Choose a reason for hiding this comment

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

why?

Copy link
Member

Choose a reason for hiding this comment

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

People extending the class will have to update their method signatures. That's why the BC promise requires this change.

8000
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@xabbuh The getLogoutPath() and getLogoutUrl() from the helper can indeed used with a default argument and do not have to change.

Only the getLogoutPath() and getLogoutUrl() from the LogoutUrlExtension must have a default value, which according to the BC promise is allowed. The only question I have, in which UPGRADE file must I add this information?

{
return $this->generateLogoutUrl($key, UrlGeneratorInterface::ABSOLUTE_PATH);
}

/**
* Generates the absolute logout URL for the firewall.
*
* @param string $key The firewall key
* @param string|null $key The firewall key or null to use the current firewall key
*
* @return string The logout URL
*/
public function getLogoutUrl($key)
public function getLogoutUrl($key = null)
Copy link
Contributor

Choose a reason for hiding this comment

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

phpdoc update

{
return $this->generateLogoutUrl($key, UrlGeneratorInterface::ABSOLUTE_URL);
}

/**
* Generates the logout URL for the firewall.
*
* @param string $key The firewall key
* @param string|null $key The firewall key or null to use the current firewall key
* @param bool|string $referenceType The type of reference (one of the constants in UrlGeneratorInterface)
*
* @return string The logout URL
*
* @throws \InvalidArgumentException if no LogoutListener is registered for the key
* @throws \InvalidArgumentException if no LogoutListener is registered for the key or the key could not be found automatically.
*/
private function generateLogoutUrl($key, $referenceType)
Copy link
Contributor

Choose a reason for hiding this comment

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

phpdoc update

{
// Fetch the current provider key from token, if possible
if (null === $key && null !== $this->tokenStorage) {
$token = $this->tokenStorage->getToken();
if (null !== $token && method_exists($token, 'getProviderKey')) {
$key = $token->getProviderKey();
}
Copy link
Member

Choose a reason for hiding this comment

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

what if it cannot be retrieved ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You mean the token? I can add an additional check for this.

}

if (null === $key) {
throw new \InvalidArgumentException('Unable to find the current firewall LogoutListener, please provide the provider key manually.');
}

if (!array_key_exists($key, $this->listeners)) {
throw new \InvalidArgumentException(sprintf('No LogoutListener found for firewall key "%s".', $key));
Copy link
Member

Choose a reason for hiding this comment

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

Would it be better to throw a specific exception when key is null? Probably in the condition above when we are not able to find the corresponding key? Something like "Unable to find the current firewall LogoutListener, please specify the provider key."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@fabpot the feature ia about handling the current context. By specifying null, it will use the current provider key so we can reuse a twig template with logout_url and/or not dependable anymore of the firewall name. If a default key cannot be found, the invalidargumentexception on the lines below will be triggered as a fallback anyway.

Copy link
Member

Choose a reason for hiding this comment

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

I do understand the change, but if the getProviderKey() method does not exist, the $key will still be null here, in which case, the error message is not the right one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, I'll see if I can find an exception that can be added.

}
Expand All @@ -106,7 +133,7 @@ private function generateLogoutUrl($key, $referenceType)
$parameters = null !== $csrfTokenManager ? array($csrfParameter => (string) $csrfTokenManager->getToken($csrfTokenId)) : array();

if ('/' === $logoutPath[0]) {
$request = $this->container->get('request_stack')->getCurrentRequest();
$request = $this->requestStack->getCurrentRequest();

$url = UrlGeneratorInterface::ABSOLUTE_URL === $referenceType ? $request->getUriForPath($logoutPath) : $request->getBasePath().$logoutPath;

Expand Down
10 changes: 10 additions & 0 deletions ...undle/Tests/Functional/Bundle/FormLoginBundle/Resources/views/Login/after_login.html.twig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,4 +3,14 @@
{% block body %}
Hello {{ app.user.username }}!<br /><br />
You're browsing to path "{{ app.request.pathInfo }}".

<a href="{{ logout_path('default') }}">Log out</a>.
<a href="{{ logout_url('default') }}">Log out</a>.

<a href="{{ logout_path('second_area') }}">Log out</a>.
<a href="{{ logout_url('second_area') }}">Log out</a>.

<a href="{{ logout_path() }}">Log out</a>.
<a href="{{ logout_url() }}">Log out</a>.

{% endblock %}
34 changes: 34 additions & 0 deletions src/Symfony/Bundle/SecurityBundle/Tests/Functional/FormLoginTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,40 @@ public function testFormLogin($config)
$this->assertContains('You\'re browsing to path "/profile".', $text);
}

/**
* @dataProvider getConfigs
*/
public function testFormLogout($config)
{
$client = $this->createClient(array('test_case' => 'StandardFormLogin', 'root_config' => $config));
$client->insulate();

$form = $client->request('GET', '/login')->selectButton('login')->form();
$form['_username'] = 'johannes';
$form['_password'] = 'test';
$client->submit($form);

$this->assertRedirect($client->getResponse(), '/profile');

$crawler = $client->followRedirect();
$text = $crawler->text();

$this->assertContains('Hello johannes!', $text);
$this->assertContains('You\'re browsing to path "/profile".', $text);

$logoutLinks = $crawler->selectLink('Log out')->links();
$this->assertCount(6, $logoutLinks);
$this->assertSame($logoutLinks[0]->getUri(), $logoutLinks[1]->getUri());
$this->assertSame($logoutLinks[2]->getUri(), $logoutLinks[3]->getUri());
$this->assertSame($logoutLinks[4]->getUri(), $logoutLinks[5]->getUri());

$this->assertNotSame($logoutLinks[0]->getUri(), $logoutLinks[2]->getUri());
$this->assertNotSame($logoutLinks[1]->getUri(), $logoutLinks[3]->getUri());

$this->assertSame($logoutLinks[0]->getUri(), $logoutLinks[4]->getUri());
$this->assertSame($logoutLinks[1]->getUri(), $logoutLinks[5]->getUri());
}

/**
* @dataProvider getConfigs
*/
Expand Down
9 changes: 9 additions & 0 deletions src/Symfony/Bundle/SecurityBundle/Tests/Functional/app/StandardFormLogin/config.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,17 @@ security:
form_login:
check_path: /login_check
default_target_path: /profile
logout: ~
anonymous: ~

# This firewall is here just to check its the logout functionality
second_area:
http_basic: ~
anonymous: ~
logout:
target: /second/target
path: /second/logout

access_control:
- { path: ^/unprotected_resource$, roles: IS_AUTHENTICATED_ANONYMOUSLY }
- { path: ^/secure-but-not-covered-by-access-control$, roles: IS_AUTHENTICATED_ANONYMOUSLY }
Expand Down
8 changes: 4 additions & 4 deletions src/Symfony/Bundle/SecurityBundle/Twig/Extension/LogoutUrlExtension.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,23 +41,23 @@ public function getFunctions()
/**
* Generates the relative logout URL for the firewall.
*
* @param string $key The firewall key
* @param string|null $key The firewall key or null to use the current firewall key
*
* @return string The relative logout URL
*/
public function getLogoutPath($key)
public function getLogoutPath($key = null)
{
return $this->helper->getLogoutPath($key);
}

/**
* Generates the absolute logout URL for the firewall.
*
* @param string $key The firewall key
* @param string|null $key The firewall key or null to use the current firewall key
*
* @return string The absolute logout URL
*/
public function getLogoutUrl($key)
public function getLogoutUrl($key = null)
{
return $this->helper->getLogoutUrl($key);
}
Expand Down
0