Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

SEC-1900: sec:authorize ifAllGranted does not work if Authorities are deprecated GrantedAuthorityImpl #2130

Closed
spring-issuemaster opened this Issue Jan 25, 2012 · 9 comments

Comments

Projects
None yet
3 participants

Janning Vygen (Migrated from SEC-1900) said:

<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.

Erik Jensen said:

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.

Christian Hilmersson said:

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.

Christian Hilmersson said:

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.

Christian Hilmersson said:

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.

Christian Hilmersson said:

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

Christian Hilmersson said:

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.

Burkhard Graves said:

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

Rob Winch said:

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

@spring-issuemaster spring-issuemaster added this to the 3.1.1 milestone Feb 5, 2016

Fire-Star commented Aug 11, 2017 edited

use "access" attribute at sec:authorize tag.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment