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

Add checks for security context so that layout is usable for 404 error page. #974

Merged
merged 2 commits into from
Feb 4, 2014

Conversation

Richtermeister
Copy link
Contributor

Also adding a 404 error page which extends the Sylius layout.

@@ -64,7 +64,7 @@ public function setCurrency($currency)

protected function getUser()
{
if ($this->securityContext->isGranted('IS_AUTHENTICATED_REMEMBERED')) {
if ($this->securityContext->getToken() && $this->securityContext->isGranted('IS_AUTHENTICATED_REMEMBERED')) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is wrong, it should work with old code, if not there is bug in Symfony that broke BC compatibility.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It works under normal conditions, but not in 404 pages. Check out the SF doc here: http://symfony.com/doc/current/cookbook/controller/error_pages.html (second gray box).

Copy link
Contributor

Choose a reason for hiding this comment

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

You must not use is_granted in your error pages (or layout used by your error pages), because the router runs before the firewall. If the router throws an exception (for instance, when the route does not match), then using is_granted will throw a further exception. You can use is_granted safely by saying {% if app.user and is_granted('...') %}.

?

If so, then you get this wrong, because it's only about calling that in Twig, not in code it self.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Obviously the twig code is just an extension wrapper for the security check behind it. I'm sure you understand the intention of the documentation beyond its literal text..

Copy link
Contributor

Choose a reason for hiding this comment

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

SecurityContext throws a further exception if there's no token and you call isGranted(). The docs are only speaking about Twig, because the page is about customzing the Twig error templates. But it applies fot the code too 🐤

@winzou
Copy link
Contributor

winzou commented Feb 4, 2014

👍

pjedrzejewski pushed a commit that referenced this pull request Feb 4, 2014
Add checks for security context so that layout is usable for 404 error page.
@pjedrzejewski pjedrzejewski merged commit d3d4782 into Sylius:master Feb 4, 2014
@pjedrzejewski
Copy link
Member

Thank you Daniel!

@Richtermeister Richtermeister deleted the fix-404-user branch February 4, 2014 17:14
<h2>The server returned a "404 Not Found".</h2>

<div>
Something is broken. Please e-mail us at [email] and let us know
Copy link
Contributor

Choose a reason for hiding this comment

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

i18n?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, true. I just copied the twig default template up, but good point.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants