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.

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

Fix how to safely use is_granted in error pages #2359

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
82b0009
Fix how to safely use is_granted in error pages
ftassi Mar 26, 2013
b2cd345
Fix app.user check
ftassi Mar 26, 2013
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
2 changes: 1 addition & 1 deletion cookbook/controller/error_pages.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)


.. tip::

Expand Down