Revise AuthorizationAnnotationUtils
#14407
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
sbrannen
force-pushed
the
issues/AuthorizationAnnotationUtils-revision
branch
from
a3971b8
to
f7e2ddf
Compare
spring-projects-issues
added
the
status: waiting-for-triage
sbrannen
force-pushed
the
issues/AuthorizationAnnotationUtils-revision
branch
from
f7e2ddf
to
58be4b0
Compare
This commit revises AuthorizationAnnotationUtils as follows. - Removes code duplication by treating both Class and Method as AnnotatedElement. - Avoids duplicated annotation searches by processing merged annotations in a single Stream instead of first using the MergedAnnotations API to find possible duplicates and then again searching for a single annotation via AnnotationUtils (which effectively performs the same search using the MergedAnnotations API internally). - Uses `.distinct()` within the Stream to avoid the need for the workaround introduced in spring-projectsgh-13625. Note that the semantics here result in duplicate "equivalent" annotations being ignored. In other words, if @PreAuthorize("hasRole('someRole')") is present multiple times as a meta-annotation, no exception will be thrown and the first such annotation found will be used. - Improves the error message when competing annotations are found by including the competing annotations in the error message. - Updates AuthorizationAnnotationUtilsTests to cover all known, supported use cases. - Configures correct role in @RequireUserRole. Please note this commit uses `.map(MergedAnnotation::withNonMergedAttributes)` to retain backward compatibility with previous versions of Spring Security. However, that line can be deleted if the Spring Security team decides that it wishes to support merged annotation attributes via custom composed annotations. If that decision is made, the composedMergedAnnotationsAreNotSupported() test should be renamed and updated as explained in the comment in that method. See spring-projectsgh-13625 See spring-projects/spring-framework#31803
sbrannen
force-pushed
the
issues/AuthorizationAnnotationUtils-revision
branch
from
58be4b0
to
aa4e24d
Compare
marcusdacoregio
added
in: core
|
Thanks, @sbrannen! This is now merged into |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Overview
This commit revises
AuthorizationAnnotationUtilsas follows.Removes code duplication by treating both
ClassandMethodasAnnotatedElement.Avoids duplicated annotation searches by processing merged annotations in a single Stream instead of first using the
MergedAnnotationsAPI to find possible duplicates and then again searching for a single annotation viaAnnotationUtils(which effectively performs the same search using theMergedAnnotationsAPI internally).Uses
.distinct()within the Stream to avoid the need for the workaround introduced in AnnotationConfigurationException when using PreAuthorize, CGLIB and EnableMethodSecurity #13625. Note that the semantics here result in duplicate "equivalent" annotations being ignored. In other words, if@PreAuthorize("hasRole('someRole')")is present multiple times as a meta-annotation, no exception will be thrown and the first such annotation found will be used.Improves the error message when competing annotations are found by including the competing annotations in the error message.
Updates
AuthorizationAnnotationUtilsTeststo cover all known, supported use cases.Configures correct role in
@RequireUserRole.Please note this commit uses
.map(MergedAnnotation::withNonMergedAttributes)to retain backward compatibility with previous versions of Spring Security. However, that line can be deleted if the Spring Security team decides that it wishes to support merged annotation attributes via custom composed annotations. If that decision is made, thecomposedMergedAnnotationsAreNotSupported()test should be renamed and updated as explained in the comment in that method.Related Issues
MergedAnnotationsfinds duplicate annotations on method in multi-level interface hierarchy spring-framework#31803