Closed spring-projects-issues closed 8 years ago
spring-projects-issues
commented
11 years ago Rob Winch said:
Rather than getting the name from the principal, we can just check to see if the Principals are equal. For example:
if ((principal instanceof String) && currentUser.getName().equals(principal)) {
return false;
}
if(principal != null && principal.equals(currentUser.getPrincipal())) {
return false;
}Rob Winch said:
Fixed as outlined in my previous comment.
spring-projects-issues
commented
11 years ago Henrik Baastrup said:
I do not think calling the equals method direct on the Principal is a good idea, that should be last resort, using getName is more correct as it must be override, and as a programmer that is the method I would call to control a Principal. So maybe the code should look like this:
if (principal==null) return true;
if (principal instanceof Principal) {
if (currentUser.getPrincipal()!=null) {
if (currentUser.getPrincipal().getName().equals(principal.getName()) return false;
return !currentUser.getPrincipal().equals(principal);
}
return !currentUser.getName().equals(((Principal)principal).toString());
}
return !currentUser.getName().equals(principal.toString());This code allow the programmer to use three methods around the Principal: getName (that must be override as it is part of the interface), equals and toString. Also the principal can be any kind of Object now.
spring-projects-issues
commented
11 years ago Rob Winch said:
I disagree that using getName() is more correct. Do you have any evidence or concrete examples of why you believe getName() is "more correct"?
The only time that getName() should be used to determine equality is when you are trying to compare the Principal to a String representation (which is already accounted for). Otherwise, the equals method should be used. This is the definition of the equals method after all.
spring-projects-issues
commented
11 years ago Henrik Baastrup said:
I could override toString if I want to compare two Principals as strings and then, if I follow your argumentation, the getName has no sense.
The Principal interface has only one method (property); the getName, this has sense, after all the principal represent an entity, and as such has a name, two principals with the same name, are in this sense, equal they represent the same entity. When you have two principals the only value you can be sure to compare is the name, because that is the only method the programmer is force to write, it is not given that the equals method is overwritten and for this reason your code will fail even if the two principals where thought equals, as they had the same name. A principal is not a string, as the original code expected, but is a principal.
Last, it is not given you override the equals method, I believe, if you take a look at your own code, you have only a very little part of your objects, there actual override this method, even you compare properties within the objects. And what does it means that two instances of an object are equals? That depend of the programmer understanding of this. I strongly advice, that you use the getName method, as the code in question, is used as a framework by others and as such, you should write code, for what you could expect, and you can not expect that the equals method is overwritten.
spring-projects-issues
commented
11 years ago Rob Winch said:
In reply to comment #5:
I could override toString if I want to compare two Principals as strings and then, if I follow your argumentation, the getName has no sense.
Overriding toString() has nothing to do with the equality of the two objects. This is just one way a human would visualize the Object.
The Principal interface has only one method (property); the getName, this has sense, after all the principal represent an entity, and as such has a name, two principals with the same name, are in this sense, equal they represent the same entity. When you have two principals the only value you can be sure to compare is the name, because that is the only method the programmer is force to write, it is not given that the equals method is overwritten and for this reason your code will fail even if the two principals where thought equals, as they had the same name. A principal is not a string, as the original code expected, but is a principal.
Actually the getPreAuthenticatedPrincipal method allows for Object to be returned (not just Principal). Additionally, the User object accepts an Object as the principal that is injected into it. Since everything is an Object and Object defines the equals method, then everything (despite if it is a Princpal or not) will have an equals method.
Last, it is not given you override the equals method, I believe, if you take a look at your own code, you have only a very little part of your objects, there actual override this method, even you compare properties within the objects.
It is very frequent that I override the equals (and hashCode) methods. You must override the equals method if you are using standard Java Collections, an ORM like Hibernate, etc. This is a very common requirement.
And what does it means that two instances of an object are equals? That depend of the programmer understanding of this.
Precisely why the equals method should be used. If it is not used, then the behavior cannot be changed by developers.
I strongly advice, that you use the getName method, as the code in question, is used as a framework by others and as such, you should write code, for what you could expect, and you can not expect that the equals method is overwritten.
As mentioned previously, there are many other reasons that you must override the equals method. The equals method is a standard Java method that must be overridden if you want to determine equality (this is documented on the equals method as well).
spring-projects-issues
commented
11 years ago Henrik Baastrup said:
I do follow your way of thinking, and I do agree with you, that you have the equals and hasCode methods to override when needed. The problem is; that object is not just objects, they are thought to have behaviors, and the Principal class has a behavior around its name, see the "JAAS Reference Guide" and "JAAS and Java GSS-API Tutorials" authorization in Java, as I understand it, is based on Principal names and personal I have only seen the name property used when two Principals are compared.
I advice you again to use the name property to compare the two Principals, as that is, as I understand it, the natural behavior of the Principal object.
If you look at the code I suggest, it takes into account that the Principal might have a special behavior around its name and for this reason use the equals method. It also takes into account that the object has overwritten the toString method such as the String object. I do believe this code will be more useful for the framework than yours, but I cannot, and do not want to, force you to change your code, so I will start to override the equals method, even I think it is not a natural behavior, on my Principals when use the SPRING framework. You should document this someplace too.
The "Single Sign-on Using Kerberos in Java" is an other very interesting paper on the subject
spring-projects-issues
commented
11 years ago Rob Winch said:
Resolving as fixed. In summary, the fix will use the equals method for the following reasons:
spring-projects-issues
commented
11 years ago Henrik Baastrup said:
Having taken a look of the Spring code again, I think it comes down to what you want to trig the re-authentication on: Do you want to trig on difference in objects, from the getPreAuthenticatedPrincipal method and what the currentUser object contains, then use equals, or do you want trig on change in entity, then use getName. Please see at my code below
class SimplePrincipal implements Principal {
String name;
public SimplePrincipal(String name) {
this.name = name;
}
@Override
public String getName() {
return name;
}
}
class ComplexPrincipal implements Principal {
String person;
int age;
ComplexPrincipal(String person, int age) {
this.person = person;
this.age = age;
}
@Override
public String getName() {
return person;
}
@Override
public boolean equals(Object obj) {
if (!(obj instanceof ComplexPrincipal)) return false;
if (!((ComplexPrincipal)obj).person.equals(this.person)) return false;
if (((ComplexPrincipal)obj).age!=this.age) return false;
return true;
}
}
SimplePrincipal sp = new SimplePrincipal("Henrik Baastrup");
ComplexPrincipal cp = new ComplexPrincipal("Henrik Baastrup", 28); //That is not my age ;-)
if (sp.getName().equals(cp.getName())) System.out.println("The two principals present the same entity but Spring does not understand that!");
if (sp.equals(cp)) System.out.println("This should not be possibly as the two principals are of different types");
else System.out.println("Sping has failed to detect that the two principals contain the same entity as they are of different type");Do not forget that it might be very likely that the objects are different as they come from different part of the code, and for this reason you should trig on change in entity. My opinion is that you have not solved the bug, but only added complexity to the code, as it does not handle components from the java.security package correct.
spring-projects-issues
commented
11 years ago Rob Winch said:
If ComplexPrincipal and Principal are not equal, then it should trigger re-authentication. The detection determines if the Principal has changed, it is not intended to determine if the Principal name has changed or not (although this may be reflected in the equals of your Principal Object in which case it has the same effect as detecting changes in getName). If you want the two to be treated as equal, then you need to make your equals methods reflect that for both objects. Again this is no different than if you were using Java Collections, Hibernate, etc.
spring-projects-issues
commented
11 years ago Henrik Baastrup said:
I don't feel you understand the idea behind principals. They are objects there present entities and can be of different types, even they present the same entity. This is especial valid in the current case, as it is not the same code there set the principals for the current user, as that there is use for pri-authentication, so you risk different principal types presenting the same entity and you can not use equals. If I want to use Authenticator A (could be one of the Authenticators there comes with Spring) and have my pri-authentication object B (written by me), then A might use one type of principals and B an other, and your code is not able to deistic if the entity has changed. Equals is normal used on the same type of objects: here is a good article on the subject: http://www.a2ztechguide.com/2011/12/java-how-to-override-equals-and.html
spring-projects-issues
commented
11 years ago Rob Winch said:
I do see your points, but I am hesitant to use the proposed solution because I am concerned when another user logs a JIRA stating they have two different Principal objects that return the same value for getName() but are different Principals. Going back to your JAAS reference, consider the Subject API which allows for multiple Principal objects to be placed on it. One Principal may represent the username and a second Principal may represent a Group. In this instance, the two Principals may have the same result for getName(), but not be equal. This is only one scenario where it makes sense to have getName() return the same value but represent different Principals, but I am sure there are others that I am missing.
Using the equals method solves this issue because the user can determine what it means to be equal. However, as I mentioned I still see your points. With that said, I propose that we provide a method that can be overridden in AbstractPreAuthenticatedProcessingFilter that allows users to customize when the principal changed. The default behavior of this method will be the original implementation. The method would look like:
protected boolean principalChanged(HttpServletRequest request, Authentication currentAuthentication) {
Object principal = getPreAuthenticatedPrincipal(request);
if (currentAuthentication.getName().equals(principal)) {
return false;
}
logger.debug("Pre-authenticated principal has changed to " + principal + " and will be reauthenticated");
return true;
}This would allow you to solve your situation with very minimal effort without making any assumptions. It would also account for the future use case.
spring-projects-issues
commented
11 years ago Henrik Baastrup said:
I think this is an acceptable solution. Making the principalChanged protected and this way let the programmer choses an alternative solution for trigging re-authentication can also help in other situations. We could even begin to discuss if it should be abstract, but that would properly brake the architecture around pre-authentication. In the favor of using equals can be said; Sprint does no implement JAAS at pre-authentication level, so if you want to use the javax.security package, override principalChanged.
spring-projects-issues
commented
11 years ago Henrik Baastrup said:
I now see (again) that the currentUser in the requiresAuthentication method implementing the java.security.Principal interface for this reason the right solution is something like my first proposal:
if (principal instanceof Principal) {
return !currentUser.getName().equals(((Principal)principal).getName());+
}
else {
return !currentUser.getName().equals(principal.toString());
}The output from the getName method is always a String, and as the String only can compare other String objects, you can not use the equals method, but must use the toString method on not java.security.Principal objects and should use the getName on java.security.Principal objects. Sorry that I did not noticed this before, I think I forgot it a long the long discussion we had.
spring-projects-issues
commented
11 years ago Rob Winch said:
The toString() method may allow you to compare the two objects but it is not intended for checking equality. If you wish to modify the default behavior, then you will be able to override the principalChanged method that is to be added.
spring-projects-issues
commented
11 years ago Henrik Baastrup said:
The problem is that the code you have put in the principalChanged method is useless, or has the same bug as the original code. It expect the pre-authentication principal to be a String object, while the currentAuthentication must be a Principal (Authentication) object, you are comparing pears with apples. You main problem is that the getPreAuthenticatedPrincipal method is allowed to return everything (Object), but you want to compare it with a Principal in String representation (currentAuthentication.getName()). The only method there present an Object as a String is the toString method.
After my opinion you have tow possibilities: Either you trow in the towel, and declare the principalChanged method abstract, this way you export the full responsibility to the programmer, I think this is a good idea. Or you try to do something intelligent, where you take into account that you might have some programmers there use Principals (like me), other using String (like the Spring examples) and maybe some programmers would like to use something else and would like to override the principalChanged method. The minimum change is to compare like this:
currentUser.getName().equals(principal.toString())as the output from getName is a String and it is in noway given that the principal is a String. As Spring do use the java.security.Principal such as the Authentication interface, you should also take that into account and we are back to my code.
In short do an intelligent hack or be abstract.
spring-projects-issues
commented
11 years ago Henrik Baastrup said:
There also exist the possibility to re-declare the getPreAuthenticatedPrincipal methods like this:
protected abstract Principal getPreAuthenticatedPrincipal(HttpServletRequest request);I use Principal and not String as we want to compare the output with an Principal currentAuthentication, and we want to do it as the java.security framework expect us to do it like this:
if (currentAuthentication.getName().equals(principal.getName())...Rob Winch said:
We cannot make th proposed changes to the signature because they would be non-passive
spring-projects-issues
commented
11 years ago Henrik Baastrup said:
I believe that the problem is to change the return type for the getPreAuthenticatedPrincipal method and declare the principalChanged method abstract. Then try to do something intelligent, as I wrote in my note "10/Dec/12 1:11 AM", you solution does not hold and you have not fixed the bug after my opinion.
spring-projects-issues
commented
11 years ago Rob Winch said:
For passivity reasons, we cannot:
The problem with your proposed solution is that it does not take into account a user that wants to support a more complex scenario where the getName() does not determine equality.
Spring Security will not ever solve every problem. Instead it should support scenarios with little to no effort. Overriding a single method to support (what I consider to be an edge case) is a very simple change. I consider this an edge case as the code has been around for quite some time and there have not been other reports. Therefore, it is unlikely many have the same requirements you do.
Again the changes I proposed allow for overriding a single method and still allow for more complex comparisons. Therefore I think it is a very reasonable solution.
spring-projects-issues
commented
11 years ago Henrik Baastrup said:
I do understand Spring can't solve any case, and I'm very aware of the code change is in a marginal part of the Spring code, it seems that the pre-authentication feature is not much in use, but it is a nice feature in a SSo scenario, and the possibility to do re-authentication make the whole idea very useful.
But what I do not understand is; when you change the bit of code, at least change it, so it is going to be useful. The core of the piece of code is that you want to compare something against a java.secure.Principal, so at least change the code in the way Java tells you to do, I think that have been my argument trough this whole discussion. This is equal to "scenarios with little to no effort", what you suggest will neither support simple nor complex scenario, as it does not respect the excepted behavior of a java.secure.Principal that is the base object for the re-authentication decision. It is a good idea to allow programmers to override the principalChanged method, but if he decides not to do so, it should at least do the right stuff. As is, it can only handle String from the getPreAuthenticatedPrincipal method and that is, after my opinion a non expected limitation.
If this code is marginal, at least listen to a user there actual use it, if you think it should be changed.
spring-projects-issues
commented
11 years ago Rob Winch said:
In reply to comment #21:
I do understand Spring can't solve any case, and I'm very aware of the code change is in a marginal part of the Spring code, it seems that the pre-authentication feature is not much in use, but it is a nice feature in a SSo
Pre Authentication is fairly common. One of the main use cases is to support SiteMinder integration (and other similar solutions)
But what I do not understand is; when you change the bit of code, at least change it, so it is going to be useful.
The enhancement seems very useful for the situation you are in. You can now easily override the method to customize detecting when a principal changes. Beforehand you had to rewrite the entire Filter.
The core of the piece of code is that you want to compare something against a java.secure.Principal, so at least change the code in the way Java tells you to do, I think that have been my argument trough this whole discussion.
I disagree that the core piece of code is to compare something against a Principal. The code has been in place for years with no complaints. Additionally, all four concrete implementations of AbstractPreAuthenticatedProcessingFilter#getPreAuthenticatedPrincipal that are included with Spring Security return a String.
Perhaps you can help me understand why you are using a Principal instead of a String (i.e. principal.getName())?
It is a good idea to allow programmers to override the principalChanged method, but if he decides not to do so, it should at least do the right stuff.
I agree, but the concern is that you are not the only one that uses the framework. So what happens when someone returns two different Principals that are different types (similar to my example in one of my previous comments)? Users may not want to treat getName() comparison as equality between the two types. In fact, I think not wanting to use getName() for determination of equality an excellent reason to return a richer object like a Principal making this a very real use case. If we use your proposed changes, then we may have made an invalid assumption for the aforementioned use case.
If this code is marginal, at least listen to a user there actual use it, if you think it should be changed.
There are other users we must consider. If I make the proposed changes, there is just as likely a chance that I have made a bad assumption for users who do not want to treat getName() as equal. For this reason, it is best to allow users who have a more unique situation to customize the strategy themselves.
spring-projects-issues
commented
11 years ago Henrik Baastrup said:
I believe that my case is marginal because, this bug would have be found before if not. The system function perfectly as long you do not use the "check for principal change" flag, and even so, you might not notice that you do re-authentication when not necessary or your getPreAuthenticatedPrincipa might return a String object. Also not everybody might be so hard headed as me ;-)
The core of the code compare against the currentAuthentication there is a Principal, and for that reason use the getName to extract its value to check if the entity, it present, is different from something there should present an entity.
I use principals because I use the same objects in my Authentication Provider and my pre-authentication code and this way re-use my code. I could of cause just return the getName in my getPreAuthenticatedPrincipal method, but that is not logic because it expect an Object not a String. In general I'm using the java.security package in my code, my code also implement JAAS and respect the expected behavior of the classes here, a Principal is the object there present an entity here. If you want to compare two principals (entities) you should use the getName method, this way you can compare tow different type of principals there might present the same entity, remember you want to do re-authentication when the entity has changed not when the object has changed. E.g. I want to do re-authentication when the entity change from Bob to Alise not when the object change from type A to type B but the entity remain Bob, am I right?
Why I object so strongly to your suggesting is; you have just move the bug down in some code I can override, not solved the bug. As long you compare (use equals) with a String object you can only return Strings from the getPreAuthenticatedPrincipal method. If that is what you want, you should change the return type for the method to String.
My sensation is, I might be wrong, that you like to use String to present entities, this is not necessary wrong, but Java tells us to use Principals, and compare them at String level using the getName method, we can always discuss if this is a good solution, but after all that is what Java use and also what Spring use for the Authentication interface, so why not continue to do that? I think it is a good idea to allow people to override your method, if they do not use Strings or Principals. But as is, you can only use Strings for principals, so all existing code so fare must have returned Strings, if they pass this piece of code. That is to pure, especial when Java tell us the right thing is to use Principals, and the abstract method allow Objects.
Last: What I suggest in comment "07/Dec/12 9:48 AM" (I can't see the number) will function with all existing code, plus Principal and non String objects, not too bad I think. Put it in in you new method and people can then override if necessary, but the code now have expected behavior.
spring-projects-issues
commented
11 years ago Rob Winch said:
If you want to compare two principals (entities) you should use the getName method, this way you can compare tow different type of principals there might present the same entity, remember you want to do re-authentication when the entity has changed not when the object has changed. E.g. I want to do re-authentication when the entity change from Bob to Alise not when the object change from type A to type B but the entity remain Bob, am I right?
This may be true in some systems, but Spring Security does not know the semantics of your Principal. As I outlined previously, a Principal could represent a Group and a User. We do not want to treat these as the same. Another example is that the Principal may have changed its strength (i.e. perhaps it only used on factor to authenticate and then changed to two factors). We want to ensure that triggers authentication too. In short, allowing a system to use getName() for comparison can be dangerous in a system that we are not familiar with. However, equals can accommodate both problems.
As long you compare (use equals) with a String object you can only return Strings from the getPreAuthenticatedPrincipal method. If that is what you want, you should change the return type for the method to String.
Again we cannot change the method signature as this is non-passive. The originally proposed code will allow returning any Object and the comparison would be done using equals.
My sensation is, I might be wrong, that you like to use String to present entities, this is not necessary wrong, but Java tells us to use Principals, and compare them at String level using the getName method, we can always discuss if this is a good solution, but after all that is what Java use and also what Spring use for the Authentication interface, so why not continue to do that?
The method signature is Object, so we must abide by that.
Last: What I suggest in comment "07/Dec/12 9:48 AM" (I can't see the number) will function with all existing code, plus Principal and non String objects, not too bad I think. Put it in in you new method and people can then override if necessary, but the code now have expected behavior.
It will function as expected for you. Again we don't know others use-cases and I would rather error on the side of making the method return not equals (require re-authentication) than let something slip by accidentally. Worst case scenario for your proposal is a Security breach...worst case scenario for my proposals is a performance issue.
spring-projects-issues
commented
9 years ago Kathryn Spinks said:
Hi Rob, your proposal to provide a method which subclasses can override will work perfectly for me, without compromising other subclasses that already rely on this behaviour (like J2eePreAuthenticatedProcessingFilter which returns a String and NOT a principal when getPreAuthenticatedPrincipal(request) is called).
spring-projects-issues
commented
9 years ago Kathryn Spinks said:
Hi Rob, are you able to provide me an idea of which, ir any, release this change might be added to please? The solution that you have proposed would be ideal for the situation that I am facing. Thanks Kathryn
spring-projects-issues
commented
9 years ago Henrik Baastrup said:
Hi Kathryn I don't know which of Rob's changes you looking for, but as Rob and me did not agree I thought the bug was suspended in some way, and I did a workaround. I don't know if you could do the same, but the main idea is to overwrite the equals method in the principal, so the Spring pre-authentication comes out right, below is my equals method, the principal the userId property is a String:
class IdP4JAuthenticationPrincipal implements Principal,Serializable {
.
.
@Override
public boolean equals(Object obj) {
if (userId!=null) {
if (obj instanceof String) return userId.equals(obj);
else if (obj instanceof IdP4JAuthenticationPrincipal) return userId.equals(((IdP4JAuthenticationPrincipal)obj).getUserId());
else return userId.equals(obj.toString());
}
else return super.equals(obj);
}
*Kathryn Spinks said:
Hi Henrik, The change of Rob's that I was referring to was
protected boolean principalChanged(HttpServletRequest request, Authentication currentAuthentication) {
Object principal = getPreAuthenticatedPrincipal(request);
if (currentAuthentication.getName().equals(principal)) {
return false;
}
logger.debug("Pre-authenticated principal has changed to " + principal + " and will be reauthenticated");
return true;
}
This change will enable existing implementation of the abstract class to work, whilst enabling me to override the default behaviour. I use both the J2eePreAuthenticatedProcessingFilter which relies on the existing behaviour, and I need to implement something else with different behaviour, and this change suits both.
With regards to your example above with the equals method, whilst I can see why this might seem an ideal solution, it just isn't practical for me as the principal is supplied via Tomcat and not my application and so I do not have easy (or any?) access to amend the equals method.
The workaround I have implemented is to add Rob's method to the class - and it is working perfectly for me in both situations.
spring-projects-issues
commented
9 years ago Henrik Baastrup said:
Hi Kathryn,
Sorry the above example was not my workaround, it so long time I did this stuff, but by digging a bit, I found my real workaround. My setup is that I have a Tomcat application there use an external authentication provider. This provider pass a token to my application that I later can use to verify the validation of a session. To workaround the problem described in this issue, I let the pre-authentication fails in AbstractPreAuthenticatedProcessingFilter as my overriding class getPreAuthenticatedPrincipal method always returns a Principal not a String, then I handle this in my AuthenticationProvider authenticate method. There I check the type of Authentication there is passed as parameter, if it is of the same type I returned from getPreAuthenticatedPrincipal and contains valid values, I accept it as a valid Authentication and create a new Authentication of a different type than was passed and return this. The problem with this solution is, you always run your authentication, with all the cost this has, and the whole idea with preauthentication seems a bit empty.
NOTE: The Spring Authentication class implements java.security.Principal and for this reason the existing code is so absurd that it only can accept Strings and for the same reason I can not agree with Rob's solution.
I don't know is this can help you but for me it function.
spring-projects-issues
commented
9 years ago Kathryn Spinks said:
Hi Henrik, I agree that the current implementation doesn't make much sense, however I believe that your equals method would not work for me even if I was able to implement it. This is because the Object returned by SecurityContextHolder.getContext().getAuthentication().getPrincipal(); isn't a 'Principal' in my application - its an Object that's an implementation of the UserDetails interface. The solution that Rob suggests is a pragmatic solution that allows existing implementations to work, and allows for different 'Objects' to be returned by the getPrincipal() method (perhaps this method should be renamed as its quite misleading!).
spring-projects-issues
commented
9 years ago Rob Winch said:
Thanks for the feedback. I have reapplied the changes I made to allow overriding the method in 3.2.6 and 4.0.0.RC2 which will be available later this week.
Henrik Baastrup (Migrated from SEC-2078) said:
The problem occurs when using pre-authentication with "check for principal change" set and the class there extends org.springframework.security.web.authentication.preauth.AbstractPreAuthenticatedProcessingFilter using non String principals but e.g. java.security.Principal. The problem is that the authentication manager will always authenticate even the principal has no changed, this can give problems with the authentication provider, and performance in the code.
The error is in line 145 in the AbstractPreAuthenticatedProcessingFilter class, the code:
if (currentUser.getName().equals(principal)) { return false; }
should be changed to something like:
if (principal instanceof Principal) { return !currentUser.getName().equals(((Principal)principal).getName()); } else { return !currentUser.getName().equals(principal.toString()); }
The original code will only function when the passed principal parameter is of the type String. The code suggested will function for all type of objects there either implements the java.security.Principal interface or override the toString method.