Skip to content

Fix how to safely use is_granted in error pages #2359

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 2 commits into from
Mar 31, 2013

Conversation

ftassi
Copy link
Contributor

@ftassi ftassi commented Mar 26, 2013

Q A
Doc fix? yes
New docs? no
Applies to 2.1+
Fixed tickets #2078

See df85a35#L0R59

@@ -56,7 +56,7 @@ end-user, create a new template located at
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.security and is_granted('...') %}``.
can use ``is_granted`` safely by saying ``{% if app.user and is_granted('...') %}``.
Copy link
Member

Choose a reason for hiding this comment

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

as said by @stof, app.user does always exists, you should check if app.user is not null. ( #2078 (comment) )

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@wouterj 👍 you're right

Copy link
Member

Choose a reason for hiding this comment

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

@wouterj {% if app.user %} is not the same than {% if app.user is defined %}. The first one returns false for falsy values (if (null) is false) and explode on undefined variables (when using strict variables).

Copy link
Member

Choose a reason for hiding this comment

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

@stof I think it's exactly what I said?

Copy link
Member

Choose a reason for hiding this comment

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

@wouterj No. You said it always exist and the check should be changed. Always being defined is indeed true, but it is irrelevant in this contest as {% if app.user %} is not checking if it is defined but if it is truthy (which is what we need).

The only difference between {% if app.user %} and {% if app.user is not null %} is that the first one means if ($app->getUser()) whereas the second one means if (null !== $app->getUser()). But as $app->getUser() returns either null or a UserInterface instance, it is strictly equivalent.
So I would document {% if app.user %} for the only reason that it is shorter.

Btw, another way would be to check {% if app.security.token %} (and it would be the right one to be strict about what we check).

app.user can be null in 3 cases:

  • app.security is null (happens when SecurityBundle is not registered in the project, in which case you have a bigger issue as using is_granted forbids you to compile the template as it also comes from SecurityBundle)
  • app.security.token is null (not behind a firewall, which can happen on the error page when the error was in the routing, and is what we want to guard against)
  • the user is anonymous (in which case the is_granted check would probably return false anyway, which is why using app.user can be considered a good way to make it shorter)

weaverryan added a commit that referenced this pull request Mar 31, 2013
Fix how to safely use is_granted in error pages
@weaverryan weaverryan merged commit 1b25c86 into symfony:2.1 Mar 31, 2013
weaverryan added a commit that referenced this pull request Mar 31, 2013
@weaverryan
Copy link
Member

Hi Francesco!

This is an excellent improvement! I've merged this in, but changed it to use the shorter syntax ({% if app.user and ... %}) as @stof pointed out.

Thanks!

@wouterj
Copy link
Member

wouterj commented Apr 1, 2013

I suggest to change it to app.security.token, as Stof suggested:

Btw, another way would be to check {% if app.security.token %} (and it would be the right one to be strict about what we check).

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.

4 participants