Richard Bradley (Migrated from SEC-1888) said:
If I secure an object with a mixture of @PreAuthorize and @Secured annotations, then one or the other is silently ignored:
@PreAuthorize("isAuthenticated()")
public class FooHelper {
// you have to be logged in to do this
public Foo getFoo(int id) {
...
}
@Secured("ROLE_FOO_EDITOR")
public void saveFoo(Foo newFoo) {
...
}
}
In the above example, I wanted to use @Secured where I had a role-based restriction, as it's prettier than @PreAuthorize, but I had to use @PreAuthorize for the isAuthenticated() restriction, as that is not expressable with @Secured.
I have had a look at the code, and I believe the culprit is here: core/src/main/java/org/springframework/security/access/method/DelegatingMethodSecurityMetadataSource.java:49
// No cached value, so query the sources to find a result
Collection<ConfigAttribute> attributes = null;
for (MethodSecurityMetadataSource s : methodSecurityMetadataSources) {
attributes = s.getAttributes(method, targetClass);
if (attributes != null && !attributes.isEmpty()) {
break;
}
}
The "break" means that only the first MethodSecurityMetadataSource is considered. I think all should be considered.
I have attached my attempted fix with a unit test, however it breaks another test, and I can't immediately see why. Please could someone have a look and see if they can fix it up?
(Also, I assume that returning multiple ConfigAttributes here will AND them together, is that correct?)
Many thanks,
Rich
Comment From: spring-projects-issues
Luke Taylor said:
This isn't really a bug - it's intentional that DelegatingMethodSecurityMetadataSource only uses one source of metadata, the first which returns a non-null result (the class Javadoc makes this clear). We don't recommend mixing multiple annotation types in the same class, as it can be difficult enough to work out what the overall effect is, without having to work out how different annotations are combined. In this case you could use a "hasRole()" expression to check for the foo-editor role.
Comment From: spring-projects-issues
Richard Bradley said:
So, silently ignoring security annotations is done to avoid confusion? That seems a bit off.
I think we should do one of: - support multiple security annotations, as per the attached patch - throw an exception if multiple security annotations are found, informing the user that it is not supported
If we cannot support this, we should update the docs to make it clear that different annotations cannot be mixed together. For example, the @PreAuthorize chapter makes no mention of the fact that @PreAuthorize-annotated classes will silently ignore @Secured annotations: http://static.springsource.org/spring-security/site/docs/3.1.x/reference/el-access.html#el-pre-post-annotations -- and the "Method Security" chapter discusses both @Secured and @PreAuthorize annotations, but doesn't mention this surprising result of mixing the two: http://static.springsource.org/spring-security/site/docs/3.1.x/reference/ns-config.html#ns-method-security
Comment From: spring-projects-issues
Luke Taylor said:
The method security chapter does explicitly say that you shouldn't mix annotations in the same class:
"You can enable more than one type of annotation in the same application, but you should avoid mixing annotations types in the same interface or class to avoid confusion".
It's true it could go into more detail about why it won't work, but then that applies to all areas of the framework. There comes a point when too much detail will confuse people who are just trying to learn how to use things, and those who want an in-depth understanding should look at the Javadoc and code for more detail. Feel free to submit a pull request with improved wording or an additional footnote if you would like to see it improved.
Comment From: spring-projects-issues
Richard Bradley said:
It seems like "to avoid confusion" should be upgraded to "because it will silently ignore some of them". How about this patch.
Is there nothing we can do in the code to make this failure a bit more noisy? What if the for loop I amended in my first patch threw an exception if it found more than one annotation source?
Comment From: spring-projects-issues
Richard Bradley said:
Oh -- you asked for a pull request, not a patch. How do I do that?
Comment From: spring-projects-issues
Luke Taylor said:
You can either do it via git.springsource.org or, more conveniently, I have a repo on github at https://github.com/tekul/spring-security and can push it to the main repo from there.
Comment From: spring-projects-issues
Richard Bradley said:
OK, I've opened https://github.com/tekul/spring-security/pull/1 for the documentation clarification.
It would be nice for the code to notice when it is about to ignore security attributes and throw or warn. Do you know why DMSMS needs to use only the first metadata source? What would break if it threw an exception when there were more than one?
Comment From: spring-projects-issues
Luke Taylor said:
Applied the doc changes from github.
@Ankur, I already answered your duplicate question on stackoverflow
http://stackoverflow.com/a/9240107/241990
Please use that or the Spring forums for asking questions, rather than posting them in the issue tracker.
Comment From: spring-projects-issues
Winarto said:
Luke, would it be more helpful to developer if the Spring container could throw runtime exception during start up if it encounter a class/interface is mixing @Secured and @PreAuthorize annotation? It caused me confusion for the whole day to find out why @Secure which was initially working fine out of sudden it's not working, only to find out that I mixed both annotation.
Comment From: kdejaeger
I think this changed in the latest versions, didn't it. We upgraded to spring boot 3 and noticed both annotations are now enforced. Or maybe @Secured is being preferred now.
Comment From: kdejaeger
Did some more tests, Yea it seems both annotations in spring boot 3.1 get evaluated now ... . But to be sure I combined them into one @PreAuthorized :)