Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

AnnotationConfigurationException when using PreAuthorize, CGLIB and EnableMethodSecurity #13625

Closed
rawfg opened this issue Aug 7, 2023 · 1 comment
Assignees
Labels
in: core An issue in spring-security-core type: bug A general bug
Milestone

Comments

@rawfg
Copy link

rawfg commented Aug 7, 2023

Describe the bug
I perforrmed the method security migration, to replace deprecated EnableGlobalMethodSecurity annotations with the new EnableMethodSecurity annotations, and I've got an exception when using CGLIB and the PreAuthorize annotation.

To Reproduce

  1. Clone the sample repository
  2. Run the application
  3. Navigate to the http://localhost:8080/hello
  4. Provide credentials: username: user, password: pass
  5. Notice the exception:
org.springframework.core.annotation.AnnotationConfigurationException: Found more than one annotation of type interface org.springframework.security.access.prepost.PreAuthorize attributed to public org.springframework.http.ResponseEntity com.example.preauth.SayHelloController.sayHello() Please remove the duplicate annotations and publish a bean to handle your authorization logic.
	at org.springframework.security.authorization.method.AuthorizationAnnotationUtils.findUniqueAnnotation(AuthorizationAnnotationUtils.java:65) ~[spring-security-core-6.1.2.jar:6.1.2]
	at org.springframework.security.authorization.method.PreAuthorizeExpressionAttributeRegistry.findPreAuthorizeAnnotation(PreAuthorizeExpressionAttributeRegistry.java:71) ~[spring-security-core-6.1.2.jar:6.1.2]

Expected behavior

  1. Clone the sample repository
  2. Comment line 15 in the SecurityConfiguration class
  3. Uncomment line 17 in the SecurityConfiguration class
  4. Run the application
  5. Navigate to the http://localhost:8080/hello
  6. Provide credentials: username: user, password: pass
  7. Application produces hello response

Sample

sample repository

@rawfg rawfg added status: waiting-for-triage An issue we've not yet triaged type: bug A general bug labels Aug 7, 2023
@patrick-sager
Copy link

I am facing the same issue and was preparing to report it when I found this. My use case is slightly different, I am using Spring Data JPA's pattern for customizing repositories (https://docs.spring.io/spring-data/jpa/docs/current/reference/html/#repositories.custom-implementations) and attempting to add PreAuthorize on the interface for the custom repository. I have created my own sample repo here: https://github.com/patrick-sager/method-security-issue.

As @rawfg pointed out, it works with the now deprecated EnableGlobalMethodSecurity annotation.

@jzheaux jzheaux self-assigned this Dec 8, 2023
@jzheaux jzheaux added in: core An issue in spring-security-core and removed status: waiting-for-triage An issue we've not yet triaged labels Dec 8, 2023
@jzheaux jzheaux closed this as completed in be11812 Dec 9, 2023
@jzheaux jzheaux added this to the 5.8.9 milestone Dec 9, 2023
sbrannen added a commit to sbrannen/spring-security that referenced this issue Jan 5, 2024
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
jzheaux pushed a commit that referenced this issue Jan 18, 2024
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 gh-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 gh-13625
See spring-projects/spring-framework#31803
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: core An issue in spring-security-core type: bug A general bug
Projects
Status: Done
Development

No branches or pull requests

3 participants