[GLASSFISH-20588] Principal Classes have serious problems Created: 29/May/13  Updated: 24/Apr/14

Status: Open
Project: glassfish
Component/s: security
Affects Version/s: None
Fix Version/s: future release

Type: Bug Priority: Major
Reporter: Byron Nevins Assignee: Craig Perez
Resolution: Unresolved Votes: 1
Labels: None
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified


 Description   

Ref: Joshua Bloch, Effective Java, 2nd edition, item #8
Mr. Bloch does an excellent job of describing precisely the problem in these classes:
(all in common-util)
common/common-util/src/main/java/org/glassfish/security/common/Role.java
common/common-util/src/main/java/org/glassfish/security/common/Group.java
common/common-util/src/main/java/org/glassfish/security/common/Principal.java
3 problems:
(1) These problems are the only low level FindBugs issues in all of common-util.
(2) The equals/hash methods are a mess. Look at the FB results for common-utils. Especially look at the way the base class equals() method chedcks to see if the other object is a Group. Note how it doesn't check if it is a Role (why?). This is quite brittle. The base class ought to know nothing about subclasses!
(3) Even though all three classes implement the Principal interface, the outside calling code does NOT use it. Instead, it uses PrincipalImpl. Why bother with an interface if it isn't used ? Should either use them as a class hierarchy OR as references to interfaces.
I attempted to fix this using Bloch's recommendation which is to use composition instead of inheritance. I.e. I made all three implement the interface directly. Then Role and Group stopped being subclasses. This failed because outside code is looking for PrincipalImpl objects - not Principal objects.
PrincipalImpl pi = new PrincipalImpl("foo");
Group g = new Group("foo");
Role r = new Role("foo");
pi.equals(r) --> true
r.equals(pi) --> false






Generated at Fri Feb 12 05:51:48 UTC 2016 using JIRA 6.2.3#6260-sha1:63ef1d6dac3f4f4d7db4c1effd405ba38ccdc558.