Spring Security
  1. Spring Security
  2. SEC-1418

equals of GrantedAuthorityImpl isn't symmetric

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 3.0.2
    • Fix Version/s: 3.1.0.M2
    • Component/s: None
    • Labels:
      None
    • Environment:
      Doesn't matter.

      Description

      The implementation of equals in GrantedAuthorityImpl returns true if it sees a String with the same content as its role field. So

      new GrantedAuthorityImpl("ROLE_ADMIN").equals("ROLE_ADMIN") will return true, but "ROLE_ADMIN".equals(new GrantedAuthorityImpl("ROLE_ADMIN")) won't!

      The equals method is required to be symetric, but this implementation isn't symmetric.

      Apart from that equals should be final or test explicit for GrantedAuthorityImpl.class. Using instanceof in non-final classes with a non-final equals method is not ok.

      Please read: Joshua Bloch - Effective Java - Second Edition - Item 8 Page 33 following.

      I attached a SimpleGrantedAuthority.java which uses a very save implementation. This class is final. I know that GrantedAuthorityImpl has subclasses, but it should be consindered if the trivial sharing of the role field justify the equals problems. It always very difficult to design proper equals and hashCode for subclassable classes.

      Since GrantedAuthorityImpl is really a value class like String, it should be really final and immutable like String itself.

      1. GrantedAuthorityImplTest.java
        0.7 kB
        Alexander Kiel
      2. SimpleGrantedAuthority.java
        0.9 kB
        Alexander Kiel
      3. TestHelperTests.patch
        1 kB
        Alexander Kiel

        Activity

        Hide
        Luke Taylor added a comment -

        Do you have a real world example where this is actually causing a problem, i.e. where the asymmetry w.r.t String equality matters.

        Show
        Luke Taylor added a comment - Do you have a real world example where this is actually causing a problem, i.e. where the asymmetry w.r.t String equality matters.
        Hide
        Alexander Kiel added a comment -

        Simple test of the equals method in current GrantedAuthorityImpl.

        testEquals1 fails and testEquals2 passes.

        Show
        Alexander Kiel added a comment - Simple test of the equals method in current GrantedAuthorityImpl. testEquals1 fails and testEquals2 passes.
        Hide
        Luke Taylor added a comment -

        I understand the implications regarding collections of mixed objects stored in sets or maps. The question is, do you have a real world example where this is actually causing a problem? Granted authorities are loaded and manipulated as Collection<GrantedAuthority> instances. When would you envisage storing both Strings and GrantedAuthority instances in the same collection?

        In theory, I'd agree that GrantedAuthorityImpl should be immutable and ideally would exhibit "standard" Java equals semantics, but the logical String equality was intentionally coded that way from the start (you'll find the tests explicitly check for it) and I don't want to change it unless there is a good reason. It is also potentially useful that a basic GrantedAuthority instance behaves as much like a String as possible when performing a lookup via a String attribute into a large set of authorities.

        Show
        Luke Taylor added a comment - I understand the implications regarding collections of mixed objects stored in sets or maps. The question is, do you have a real world example where this is actually causing a problem? Granted authorities are loaded and manipulated as Collection<GrantedAuthority> instances. When would you envisage storing both Strings and GrantedAuthority instances in the same collection? In theory, I'd agree that GrantedAuthorityImpl should be immutable and ideally would exhibit "standard" Java equals semantics, but the logical String equality was intentionally coded that way from the start (you'll find the tests explicitly check for it) and I don't want to change it unless there is a good reason. It is also potentially useful that a basic GrantedAuthority instance behaves as much like a String as possible when performing a lookup via a String attribute into a large set of authorities.
        Hide
        Alexander Kiel added a comment -

        I had a look at the GrantedAuthorityImplTests class. It would be better to include explicit testing of non-symmetry there. There is a test assertEquals(new GrantedAuthorityImpl("TEST"), "TEST"); but not assertFalse("TEST".equals(new GrantedAuthorityImpl("TEST")));. Same for the MockGrantedAuthority. There is no test like: assertFalse(new MockGrantedAuthority("TEST").equals(new GrantedAuthorityImpl("TEST")));. If there would be such a test, I'm sure most people wouldn't think that the behaviour of equals is a good idea.

        Your argument, looking up a large set of GrantedAuthority instances with a String doesn't count for me. Where is the Problem building a GrantedAuthorityImpl before doing so?

        There is not even a way to implement a generic equals contract which should hold for all implementations of GrantedAuthority. You could think, one can do it like it is done in the Java Collections Framework for Set and List. A possible implementation would be:

        @Override
        public boolean equals(Object o)

        { if (this == o) return true; if (!(o instanceof GrantedAuthority)) return false; GrantedAuthority that = (GrantedAuthority) o; return getAuthority() != null && getAuthority().equals(that.getAuthority()); }

        You can't return true if both sides getAuthority() returns null, because returning null at getAuthority() means that the information in the particular GrantedAuthority is non-simple, so that two arbitrary instances returning null at getAuthority() can't be equal. But what about reflexivity? That would't work:

        class AdvancedGrantedAuthority implements GrantedAuthority {

        private String aclExp;

        public AdvancedGrantedAuthority(String aclExp)

        { this.aclExp = aclExp; }

        public String getAclExp()

        { return aclExp; }

        public String getAuthority()

        { return null; }

        [ ... same equals as above ... ]
        }

        Assert.assertEquals(new AdvancedGrantedAuthority("test"), new AdvancedGrantedAuthority("test"));

        So reflexivity won't hold on non-simple implementations.

        At the end, there is no way to specify a proper equals and hashCode for all GrantedAuthority implementations. So every implementation has to bring its own. I don't know if that would be even helpful to have such an equals on special implementations of GrantedAuthority.

        SEC-863 also gives an example, that GrantedAuthority doesn't have any valid equals semantics.

        Same problem with HierarchicalRolesTestHelper#containTheSameGrantedAuthorities(..). This simply can't work. It's only tested against GrantedAuthorityImpl generated from AuthorityUtils#createAuthorityList(..). I need only change one line in TestHelperTests and it will fail (patch comes in separate comment).

        So at the end, I would simply remove the equals method in GrantedAuthorityImpl. If I do this, I have 12 failing tests in spring-security-core. But it should be really considered, if spring-security can live with that broken equals semantic.

        Show
        Alexander Kiel added a comment - I had a look at the GrantedAuthorityImplTests class. It would be better to include explicit testing of non-symmetry there. There is a test assertEquals(new GrantedAuthorityImpl("TEST"), "TEST"); but not assertFalse("TEST".equals(new GrantedAuthorityImpl("TEST")));. Same for the MockGrantedAuthority. There is no test like: assertFalse(new MockGrantedAuthority("TEST").equals(new GrantedAuthorityImpl("TEST")));. If there would be such a test, I'm sure most people wouldn't think that the behaviour of equals is a good idea. Your argument, looking up a large set of GrantedAuthority instances with a String doesn't count for me. Where is the Problem building a GrantedAuthorityImpl before doing so? There is not even a way to implement a generic equals contract which should hold for all implementations of GrantedAuthority. You could think, one can do it like it is done in the Java Collections Framework for Set and List. A possible implementation would be: @Override public boolean equals(Object o) { if (this == o) return true; if (!(o instanceof GrantedAuthority)) return false; GrantedAuthority that = (GrantedAuthority) o; return getAuthority() != null && getAuthority().equals(that.getAuthority()); } You can't return true if both sides getAuthority() returns null, because returning null at getAuthority() means that the information in the particular GrantedAuthority is non-simple, so that two arbitrary instances returning null at getAuthority() can't be equal. But what about reflexivity? That would't work: class AdvancedGrantedAuthority implements GrantedAuthority { private String aclExp; public AdvancedGrantedAuthority(String aclExp) { this.aclExp = aclExp; } public String getAclExp() { return aclExp; } public String getAuthority() { return null; } [ ... same equals as above ... ] } Assert.assertEquals(new AdvancedGrantedAuthority("test"), new AdvancedGrantedAuthority("test")); So reflexivity won't hold on non-simple implementations. At the end, there is no way to specify a proper equals and hashCode for all GrantedAuthority implementations. So every implementation has to bring its own. I don't know if that would be even helpful to have such an equals on special implementations of GrantedAuthority. SEC-863 also gives an example, that GrantedAuthority doesn't have any valid equals semantics. Same problem with HierarchicalRolesTestHelper#containTheSameGrantedAuthorities(..). This simply can't work. It's only tested against GrantedAuthorityImpl generated from AuthorityUtils#createAuthorityList(..). I need only change one line in TestHelperTests and it will fail (patch comes in separate comment). So at the end, I would simply remove the equals method in GrantedAuthorityImpl. If I do this, I have 12 failing tests in spring-security-core. But it should be really considered, if spring-security can live with that broken equals semantic.
        Hide
        Alexander Kiel added a comment -

        Patch agains TestHelperTests, which switches the implementation of GrantedAuthority in a list from GrantedAuthorityImpl to an anonymous implementation. The result is a failing test, because HierarchicalRolesTestHelper#containTheSameGrantedAuthorities(..) depends on the equals implementation GrantedAuthority implementations.

        Show
        Alexander Kiel added a comment - Patch agains TestHelperTests, which switches the implementation of GrantedAuthority in a list from GrantedAuthorityImpl to an anonymous implementation. The result is a failing test, because HierarchicalRolesTestHelper#containTheSameGrantedAuthorities(..) depends on the equals implementation GrantedAuthority implementations.
        Hide
        Luke Taylor added a comment -

        I don't really understand why writing a test like "assertFalse("TEST".equals(new GrantedAuthorityImpl("TEST")));" is relevant. That's testing the java.lang.String equals method, which is obviously false. I don't really see the relevance of the role hierarchy test helper tests either.

        There now seem to be multiple issues here. In practice most authority implementations have a unique string representation, even if they cary additional information, and it is useful to be able to look them up (independent of the implemenation class) using the String alone. In that case I see no real problem with them using String equality and hashcode methods, and I don't really mind if they return true when compared directly to a String. There is no indication that the method should apply to all subclasses. If they return null for the getAuthority method, then they should provide a specific equals method which is only relevant to that class and its unique properties.

        I'd still like to know if you have a real world issue that has caused this issue to be raised. The fact is that the project has lived with the code as it stands for seven years without it causing major problems.

        Show
        Luke Taylor added a comment - I don't really understand why writing a test like "assertFalse("TEST".equals(new GrantedAuthorityImpl("TEST")));" is relevant. That's testing the java.lang.String equals method, which is obviously false. I don't really see the relevance of the role hierarchy test helper tests either. There now seem to be multiple issues here. In practice most authority implementations have a unique string representation, even if they cary additional information, and it is useful to be able to look them up (independent of the implemenation class) using the String alone. In that case I see no real problem with them using String equality and hashcode methods, and I don't really mind if they return true when compared directly to a String. There is no indication that the method should apply to all subclasses. If they return null for the getAuthority method, then they should provide a specific equals method which is only relevant to that class and its unique properties. I'd still like to know if you have a real world issue that has caused this issue to be raised. The fact is that the project has lived with the code as it stands for seven years without it causing major problems.
        Hide
        Alexander Kiel added a comment -

        No, I don't have a real world issue. I just saw the source code of GrantedAuthorityImpl today and thought that it would be a good idea to file this bug. So its just a code review.

        Just for joke: http://www.osnews.com/story/19266

        I actually use spring-security since 2008 in webapps and I never had a problem with it. It's fine. But as you say yourself, equals and hashCode implementations which are against the contract, carry a risk for bugs. And such bugs aren't easy to find.

        Show
        Alexander Kiel added a comment - No, I don't have a real world issue. I just saw the source code of GrantedAuthorityImpl today and thought that it would be a good idea to file this bug. So its just a code review. Just for joke: http://www.osnews.com/story/19266 I actually use spring-security since 2008 in webapps and I never had a problem with it. It's fine. But as you say yourself, equals and hashCode implementations which are against the contract, carry a risk for bugs. And such bugs aren't easy to find.
        Hide
        Luke Taylor added a comment -

        OK, on the balance of things, I've decided to make this change. I can envisage some cases in the future where we may be using more complicated authority types and I don't want something like this to cause problems down the line. GrantedAuthorityImpl is now deprecated in favour of SimpleGrantedAuthority, and the latter is used throughout the codebase.

        On the downside, this may cause problems where users are using a string as a key to perform a lookup into a collection of authorities, as this will now fail.

        Show
        Luke Taylor added a comment - OK, on the balance of things, I've decided to make this change. I can envisage some cases in the future where we may be using more complicated authority types and I don't want something like this to cause problems down the line. GrantedAuthorityImpl is now deprecated in favour of SimpleGrantedAuthority, and the latter is used throughout the codebase. On the downside, this may cause problems where users are using a string as a key to perform a lookup into a collection of authorities, as this will now fail.

          People

          • Assignee:
            Unassigned
            Reporter:
            Alexander Kiel
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: