Spring Security
  1. Spring Security
  2. SEC-1275

security.authorize() tag library does not work when ifNotGranted is used.

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Incomplete
    • Affects Version/s: 3.0.0 RC1
    • Fix Version/s: 3.0.0.RC2
    • Component/s: Taglibs
    • Labels:
      None

      Description

      Something has broken from M2 to RC1 with the authorize tag library.

          <@security.authorize ifNotGranted="ROLE_USER">
              <a id="loginLink" href="/myapp/rest/login"><img src="/myapp/images/login.gif" alt="Login"/>Login</a>
          </@security.authorize>
      

      The above code used to work if the user was not logged in... hence this code would execute because a non-authenticated user would not have this role. Now the code does not execute unless a principal is logged in... which defeats the purpose of this code entirely.

      Suggestion: If this is really how you want this tag library to work, I suggest created a new tag that works like this:

      <@security.ifAuthenticated>
      <a id="loginLink" href="/myapp/rest/login"><img src="/myapp/images/login.gif" alt="Login"/>Login</a>
      </@security.ifAuthenticated>

      There are countless situations where I actually don't care what their role is or any of that - I just want to make sure the principal exists. If it does, then show the code. If it doesn't? Then don't render it. Pretty simple.

      Having said that, was the breakage of the old code from M2 deliberate?

        Activity

        Hide
        Ken Egervari added a comment -

        Actually, the above new snippet should say IfNotAuthenticated, but I'm sure you get what I mean. Two small tags for ifAuthenticated and ifNotAuthenticated would solve the problem just fine for a lot of people if you intend to break how the other tag works.

        Show
        Ken Egervari added a comment - Actually, the above new snippet should say IfNotAuthenticated, but I'm sure you get what I mean. Two small tags for ifAuthenticated and ifNotAuthenticated would solve the problem just fine for a lot of people if you intend to break how the other tag works.
        Hide
        Ken Egervari added a comment -

        Also, this happens on the /rest/login screen that is specified in the application context xml. It works fine in other places. I dunno why I didn't check other pages

        I guess you guys would know why the login page would be different.

        Show
        Ken Egervari added a comment - Also, this happens on the /rest/login screen that is specified in the application context xml. It works fine in other places. I dunno why I didn't check other pages I guess you guys would know why the login page would be different.
        Hide
        Luke Taylor added a comment -

        I don't really understand what you're saying here. Please explain clearly what you think works and what doesn't, preferably with a sample configuration illustrating the configuration differences in each case.

        Show
        Luke Taylor added a comment - I don't really understand what you're saying here. Please explain clearly what you think works and what doesn't, preferably with a sample configuration illustrating the configuration differences in each case.
        Hide
        Luke Taylor added a comment -

        Resolving as "incomplete" in the absence of further information.

        Show
        Luke Taylor added a comment - Resolving as "incomplete" in the absence of further information.
        Hide
        Andy Chapman added a comment -

        Running into the same issue here. In 2.0.x I used code like this to show a logged in or logged out menu:

        <security:authorize ifNotGranted="ROLE_USER">
        <%@ include file="static-menu.jsp" %>
        </security:authorize>
        <security:authorize ifAllGranted="ROLE_USER">
        <%@ include file="context-menu.jsp" %>
        </security:authorize>

        This no longer works. I think it is due to the following code snippet from the authorize:

        public int doStartTag() throws JspException {
        Authentication currentUser = SecurityContextHolder.getContext().getAuthentication();

        if (currentUser == null)

        { return SKIP_BODY; }

        if (access != null && access.length() > 0)

        { return authorizeUsingAccessExpression(currentUser); }

        else if (url != null && url.length() > 0)

        { return authorizeUsingUrlCheck(currentUser); }

        return super.doStartTag();
        }

        Because in 3.0.x the authentication doesn't exist for an anonymous user this code drops out without evaluating "return super.doStartTag();" so ifNotGranted never gets evaluated and the body is ignored. There is a connected issue with Authentication.getPrincipal() returning a String rather than a UserDetails object for an anonymous user.

        Of course, I may just have missed a configuration option that tells Spring 3.0.x to always generate an authentication and principal even for anonymous users.

        Show
        Andy Chapman added a comment - Running into the same issue here. In 2.0.x I used code like this to show a logged in or logged out menu: <security:authorize ifNotGranted="ROLE_USER"> <%@ include file="static-menu.jsp" %> </security:authorize> <security:authorize ifAllGranted="ROLE_USER"> <%@ include file="context-menu.jsp" %> </security:authorize> This no longer works. I think it is due to the following code snippet from the authorize: public int doStartTag() throws JspException { Authentication currentUser = SecurityContextHolder.getContext().getAuthentication(); if (currentUser == null) { return SKIP_BODY; } if (access != null && access.length() > 0) { return authorizeUsingAccessExpression(currentUser); } else if (url != null && url.length() > 0) { return authorizeUsingUrlCheck(currentUser); } return super.doStartTag(); } Because in 3.0.x the authentication doesn't exist for an anonymous user this code drops out without evaluating "return super.doStartTag();" so ifNotGranted never gets evaluated and the body is ignored. There is a connected issue with Authentication.getPrincipal() returning a String rather than a UserDetails object for an anonymous user. Of course, I may just have missed a configuration option that tells Spring 3.0.x to always generate an authentication and principal even for anonymous users.
        Hide
        Andy Chapman added a comment -

        After further investigation, this code snippet is only failing in a very limited situation. It works fine for general anonymous access but I have a login panel in an non-secure page that posts to a secure page. If the login fails, I display a full secure login page. It is on this full login page that the problem occurs. Taking a flying guess, it's something to do with the session not being transferred from the non-secure to the secure page.

        Show
        Andy Chapman added a comment - After further investigation, this code snippet is only failing in a very limited situation. It works fine for general anonymous access but I have a login panel in an non-secure page that posts to a secure page. If the login fails, I display a full secure login page. It is on this full login page that the problem occurs. Taking a flying guess, it's something to do with the session not being transferred from the non-secure to the secure page.
        Hide
        Andy Chapman added a comment -

        ...and finally. It looks like this is working perfectly, but a workaround for problems in previous version ( adding filters="none" to the login action ) was stopping the authentication on the SecurityContextHolder being populated. I removed it (the filters="none" attribute) and all is now wonderful. Hopefully these comments will be useful to someone else though.

        Show
        Andy Chapman added a comment - ...and finally. It looks like this is working perfectly, but a workaround for problems in previous version ( adding filters="none" to the login action ) was stopping the authentication on the SecurityContextHolder being populated. I removed it (the filters="none" attribute) and all is now wonderful. Hopefully these comments will be useful to someone else though.

          People

          • Assignee:
            Luke Taylor
            Reporter:
            Ken Egervari
          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: