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.

Sign up for GitHub

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

Jump to bottom

Improve Method Security logging #10279

Merged
merged 3 commits into from
Oct 8, 2021
Merged

Improve Method Security logging #10279

merged 3 commits into from
Oct 8, 2021

Conversation

marcusdacoregio
Copy link
Contributor

@marcusdacoregio marcusdacoregio commented Sep 17, 2021 •
edited
Loading

This PR removes the trace logs from the PrePostAnnotationSecurityMetadataSource which were producing too much noise in logs without adding much value.

There is already some logs that provide value when the @Pre/PostAuthorize annotations are being used with @EnableGlobalMethodSecurity:

2021-09-17 10:01:49.539 DEBUG 55564 --- [           main] .PrePostAnnotationSecurityMetadataSource : @org.springframework.security.access.prepost.PreAuthorize(value="hasRole('USER')") found on specific method: public java.lang.String example.IndexController.index()
2021-09-17 10:01:49.544 DEBUG 55564 --- [           main] m.DelegatingMethodSecurityMetadataSource : Caching method [CacheKey[example.IndexController; public java.lang.String example.IndexController.index()]] with attributes [[authorize: 'hasRole('USER')', filter: 'null', filterTarget: 'null']]

Additionally, it adds logs for when @EnableMethodSecurity is used with @Pre/PostAuthorize, @Secured and JSR-250 @PermitAll, @DenyAll, @RolesAllowed.

Closes gh-10247

@marcusdacoregio
Remove trace logs for PrePostAnnotationSecurityMetadataSource
f9170a1 
Those logs were producing too much noise on the console without adding much value.

Issue spring-projectsgh-10247
@marcusdacoregio marcusdacoregio added status: duplicate A duplicate of another issue in: core An issue in spring-security-core type: enhancement A general enhancement labels Sep 17, 2021
@marcusdacoregio marcusdacoregio added this to the 5.6.0-M3 milestone Sep 17, 2021
@marcusdacoregio marcusdacoregio self-assigned this Sep 17, 2021
@sjohnr sjohnr modified the milestones: 5.6.0-M3, 5.6.0-RC1 Sep 20, 2021
jzheaux
jzheaux requested changes Sep 22, 2021
View reviewed changes
Copy link
Contributor

@jzheaux jzheaux left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @marcusdacoregio! I've left feedback inline.

.../main/java/org/springframework/security/authorization/method/Jsr250AuthorizationManager.java Outdated Show resolved Hide resolved
...ringframework/security/authorization/method/AuthorizationManagerBeforeMethodInterceptor.java Outdated Show resolved Hide resolved
...ava/org/springframework/security/authorization/method/PostAuthorizeAuthorizationManager.java Outdated Show resolved Hide resolved
@marcusdacoregio
Copy link
Contributor Author

marcusdacoregio commented Sep 23, 2021 •
edited
Loading

Thanks very much for your review @jzheaux. I've updated the PR and replied to this comment #10279 (comment)

jzheaux
jzheaux requested changes Sep 29, 2021
View reviewed changes
Copy link
Contributor

@jzheaux jzheaux left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @marcusdacoregio! I've left some inline feedback.

.../main/java/org/springframework/security/authorization/method/Jsr250AuthorizationManager.java Outdated Show resolved Hide resolved
...ringframework/security/authorization/method/AuthorizationManagerBeforeMethodInterceptor.java Outdated Show resolved Hide resolved
...ringframework/security/authorization/method/AuthorizationManagerBeforeMethodInterceptor.java Outdated Show resolved Hide resolved
...ava/org/springframework/security/authorization/method/PostAuthorizeAuthorizationManager.java Outdated Show resolved Hide resolved
@marcusdacoregio
Copy link
Contributor Author

Thanks @jzheaux, nice suggestions. I updated the PR and it's ready for your review again.

jzheaux
jzheaux requested changes Sep 30, 2021
View reviewed changes
Copy link
Contributor

@jzheaux jzheaux left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @marcusdacoregio! I've left some feedback inline.

In addition to that feedback, will you please separate out the work you are doing with custom AuthorizationDecisions into a separate commit, resolving #9287 at the same time?

.../main/java/org/springframework/security/authorization/method/Jsr250AuthorizationManager.java Outdated Show resolved Hide resolved
@marcusdacoregio
Copy link
Contributor Author

Thanks @jzheaux, I've updated the PR based on your suggestions

jzheaux
jzheaux requested changes Oct 6, 2021
View reviewed changes
Copy link
Contributor

@jzheaux jzheaux left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @marcusdacoregio. I've left feedback inline about the classes recently introduced.

...src/main/java/org/springframework/security/authorization/AuthorityAuthorizationDecision.java Outdated Show resolved Hide resolved
...src/main/java/org/springframework/security/authorization/AuthorityAuthorizationDecision.java Outdated Show resolved Hide resolved
.../springframework/security/authorization/method/ExpressionAttributeAuthorizationDecision.java Outdated Show resolved Hide resolved
...n/java/org/springframework/security/authorization/AuthorityReactiveAuthorizationManager.java Show resolved Hide resolved
.../src/main/java/org/springframework/security/authorization/AuthorityAuthorizationManager.java Show resolved Hide resolved
@marcusdacoregio
Copy link
Contributor Author

Hi @jzheaux , thanks for the review. The code is now updated.

@marcusdacoregio marcusdacoregio merged commit 86c24da into spring-projects:main Oct 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jzheaux jzheaux jzheaux approved these changes

Assignees

@marcusdacoregio marcusdacoregio

Labels
in: core An issue in spring-security-core status: duplicate A duplicate of another issue type: enhancement A general enhancement
Projects
None yet
Milestone
5.6.0-RC1
Development

Successfully merging this pull request may close these issues.

Improve Method Security Logging
3 participants
@marcusdacoregio @jzheaux @sjohnr