Spring Security
  1. Spring Security
  2. SEC-1900

sec:authorize ifAllGranted does not work if Authorities are deprecated GrantedAuthorityImpl

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Complete
    • Affects Version/s: 3.1.0
    • Fix Version/s: 3.1.1
    • Component/s: Taglibs
    • Labels:
      None

      Description

      <sec:authorize ifAllGranted="ROLE_ADMIN">

      did not work for me after upgrading to 3.1.0

      reason: i added some role manually in my own UserDetails like this:

      GrantedAuthority granted = new GrantedAuthorityImpl(rolle.toString());
      authorities.add(granted);

      GrantedAuthorityImpl is deprecated and ifAllGranted, too. But i think it is still a bug.

      You can fix it by using simpleGrantedAuthority

      GrantedAuthority granted = new SimpleGrantedAuthority(rolle.toString());
      authorities.add(granted);

      the bug is somewhere in the Collection class in an equals method. It checks if the containing element is of class SimpleGrantedAuthority.

        Activity

        Hide
        Erik Jensen added a comment -

        I ran into this same bug tonight while upgrading to Spring 3.1.0. This issue prevents the following tag from working when using the SwitchUserFilter

        <security:authorize ifAllGranted="ROLE_PREVIOUS_ADMINISTRATOR">

        We unfortunately use this heavily in our JSP files. I had to change to using ifAnyGranted which did work for most scenarios in our code base.

        This bug is on line 135 of AbstractAuthorizeTag which looks like

        if (!granted.containsAll(toAuthorities(getIfAllGranted()))) {

        The problem is the "equals()" method being used on the GrantedAuthority instances. When comparing GrantedAuthorityImpl or SwitchUserGrantedAuthority to a SimpleGrantedAuthority, it will fail every time. The code likely should be comparing role names instead.

        Show
        Erik Jensen added a comment - I ran into this same bug tonight while upgrading to Spring 3.1.0. This issue prevents the following tag from working when using the SwitchUserFilter <security:authorize ifAllGranted="ROLE_PREVIOUS_ADMINISTRATOR"> We unfortunately use this heavily in our JSP files. I had to change to using ifAnyGranted which did work for most scenarios in our code base. This bug is on line 135 of AbstractAuthorizeTag which looks like if (!granted.containsAll(toAuthorities(getIfAllGranted()))) { The problem is the "equals()" method being used on the GrantedAuthority instances. When comparing GrantedAuthorityImpl or SwitchUserGrantedAuthority to a SimpleGrantedAuthority, it will fail every time. The code likely should be comparing role names instead.
        Hide
        Christian Hilmersson added a comment -

        Just ran into this same problem, but when using a JPA based user service with home grown entities extending UserDetails and GrantedAuthority.

        I also found that the problem is in SimpleGrantedAuthority.equals(Object obj)

        public boolean equals(Object obj) {
        if (this == obj)

        { return true; }

        if (obj instanceof SimpleGrantedAuthority)

        { return role.equals(((SimpleGrantedAuthority) obj).role); }

        return false;
        }

        IMHO this code:

        if (obj instanceof SimpleGrantedAuthority)

        { return role.equals(((SimpleGrantedAuthority) obj).role); }

        should have been something like...

        if (obj instanceof GrantedAuthority)

        { return role.equals(((GrantedAuthority) obj).getAuthority()); }

        so it will be possible to compare with something else than SimpleGrantedAuthority.

        If that is not possible for some reason, the containsAll on AbstractAuthorizeTag need to be fixed in another fashion as Erik says.

        I think I would personally go for the equals fix in SimpleGrantedAuthority.

        Show
        Christian Hilmersson added a comment - Just ran into this same problem, but when using a JPA based user service with home grown entities extending UserDetails and GrantedAuthority. I also found that the problem is in SimpleGrantedAuthority.equals(Object obj) public boolean equals(Object obj) { if (this == obj) { return true; } if (obj instanceof SimpleGrantedAuthority) { return role.equals(((SimpleGrantedAuthority) obj).role); } return false; } IMHO this code: if (obj instanceof SimpleGrantedAuthority) { return role.equals(((SimpleGrantedAuthority) obj).role); } should have been something like... if (obj instanceof GrantedAuthority) { return role.equals(((GrantedAuthority) obj).getAuthority()); } so it will be possible to compare with something else than SimpleGrantedAuthority. If that is not possible for some reason, the containsAll on AbstractAuthorizeTag need to be fixed in another fashion as Erik says. I think I would personally go for the equals fix in SimpleGrantedAuthority.
        Hide
        Christian Hilmersson added a comment -

        Another maybe even simpler fix could be to just flip the order of the collections on AbstractAuthorizeTag:135

        if (!granted.containsAll(toAuthorities(getIfAllGranted()))) {

        to

        if (!toAuthorities(getIfAllGranted()).containsAll(granted)) {

        in that way one can implement the needed logic in the objects of the granted array, which you have control over.
        Would probably give a bigger impact on existing aplpicaitons though.

        Show
        Christian Hilmersson added a comment - Another maybe even simpler fix could be to just flip the order of the collections on AbstractAuthorizeTag:135 if (!granted.containsAll(toAuthorities(getIfAllGranted()))) { to if (!toAuthorities(getIfAllGranted()).containsAll(granted)) { in that way one can implement the needed logic in the objects of the granted array, which you have control over. Would probably give a bigger impact on existing aplpicaitons though.
        Hide
        Christian Hilmersson added a comment -

        The idea in the last comment will not work since it also changes the semantics.
        I realized that A.containsAll(B) is NOT semantically equivalent to B.containsAll(A)

        So probably it would still be interesting to consider changing the equals method of SimpleGrantedAuthority. That method makes no sense if SimpleGrantedAuthority is to seen as/compared as a GrantedAuthority and used in conjunction with other GrantedAuthority implementations.

        Show
        Christian Hilmersson added a comment - The idea in the last comment will not work since it also changes the semantics. I realized that A.containsAll(B) is NOT semantically equivalent to B.containsAll(A) So probably it would still be interesting to consider changing the equals method of SimpleGrantedAuthority. That method makes no sense if SimpleGrantedAuthority is to seen as/compared as a GrantedAuthority and used in conjunction with other GrantedAuthority implementations.
        Hide
        Christian Hilmersson added a comment -

        Sorry for spamming the thread but this solution seems to work pretty well and is consistent with the solution for the two other attributes ifAnyGranted and ifNotGranted already in AbstractAuthorizeTag:

        /* Original code from AbstractAuthorizeTag:134

        if (hasTextAllGranted) {
        if (!granted.containsAll(toAuthorities(getIfAllGranted())))

        { return false; }
        }

        */
        // Changed code
        if (hasTextAllGranted) {
        Set<String> grantedRoles = authoritiesToRoles(granted);
        Set<String> requiredRoles = authoritiesToRoles(toAuthorities(getIfAllGranted()));
        if (!grantedRoles.containsAll(requiredRoles)) { return false; }

        }
        // End of changed code

        Show
        Christian Hilmersson added a comment - Sorry for spamming the thread but this solution seems to work pretty well and is consistent with the solution for the two other attributes ifAnyGranted and ifNotGranted already in AbstractAuthorizeTag: /* Original code from AbstractAuthorizeTag:134 if (hasTextAllGranted) { if (!granted.containsAll(toAuthorities(getIfAllGranted()))) { return false; } } */ // Changed code if (hasTextAllGranted) { Set<String> grantedRoles = authoritiesToRoles(granted); Set<String> requiredRoles = authoritiesToRoles(toAuthorities(getIfAllGranted())); if (!grantedRoles.containsAll(requiredRoles)) { return false; } } // End of changed code
        Hide
        Christian Hilmersson added a comment -

        Added a pull request for solving this issue (https://github.com/SpringSource/spring-security/pulls)
        Also added a separate JIRA issue (SEC-1921) for the equals method in SimpleGrantedAuthority.

        Show
        Christian Hilmersson added a comment - Added a pull request for solving this issue ( https://github.com/SpringSource/spring-security/pulls ) Also added a separate JIRA issue ( SEC-1921 ) for the equals method in SimpleGrantedAuthority.
        Hide
        Burkhard Graves added a comment -

        The priority of this task should be 'Major' at least.

        Show
        Burkhard Graves added a comment - The priority of this task should be 'Major' at least.
        Hide
        Rob Winch added a comment -

        Thanks for the bug submission. I have fixed this in master so you can give it a try if you like.

        Show
        Rob Winch added a comment - Thanks for the bug submission. I have fixed this in master so you can give it a try if you like.

          People

          • Assignee:
            Rob Winch
            Reporter:
            Janning Vygen
          • Votes:
            5 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: