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

Improve logging for Global Authentication #14711

Merged
merged 1 commit into from
Apr 25, 2024

Conversation

Kehrlann
Copy link
Contributor

@Kehrlann Kehrlann commented Mar 8, 2024

Closes gh-14663

See the issue for use-case matrix.

Open questions

I'd like to draw your attention to the following:

  • Review log messages, are they appropriate?
  • Review log levels, are they appropriate?
  • The class names are too long and are truncated in the log lines. Is there something I should do?

Example log lines:

1 UserDetailsService ; INFO level:

2024-03-08T14:23:23.058+01:00  INFO 49038 --- [           main] r$InitializeUserDetailsManagerConfigurer : Global AuthenticationManager configured with UserDetailsService bean with name myUserDetailsService

2 UserDetailsService ; WARN level:

2024-03-08T14:25:11.644+01:00  WARN 49146 --- [           main] r$InitializeUserDetailsManagerConfigurer : Found 2 UserDetailsService beans, with names [myUserDetailsService, secondUserDetailsService]. Global Authentication Manager will not use a UserDetailsService for username/password login.

1 AuthenticationProvider and at least 1 UserDetailsService bean, WARN level:

2024-03-08T14:22:27.832+01:00  WARN 48979 --- [           main] r$InitializeUserDetailsManagerConfigurer : Global AuthenticationManager configured with an AuthenticationProvider bean. UserDetailsService beans will not be used for username/password login.

1 AuthenticationProvider, INFO level:

2024-03-08T14:22:27.831+01:00  INFO 48979 --- [           main] eAuthenticationProviderManagerConfigurer : Global AuthenticationManager configured with AuthenticationProvider bean with name myAuthProvider

2 AuthenticationProviders, WARN level:

2024-03-08T14:23:23.048+01:00  WARN 49038 --- [           main] eAuthenticationProviderManagerConfigurer : Found 2 AuthenticationProvider beans, with names [myAuthProvider, secondAuthProvider]. Global Authentication Manager will not be configured with AuthenticationProviders.

Interaction between the configurers

When the two configurers produce log lines, here are a few examples:

2 AuthenticationProvider, 1 UserDetailsService:

2024-03-08T14:22:27.831+01:00  INFO 48979 --- [           main] eAuthenticationProviderManagerConfigurer : Global AuthenticationManager configured with AuthenticationProvider bean with name myAuthProvider
2024-03-08T14:22:27.832+01:00  WARN 48979 --- [           main] r$InitializeUserDetailsManagerConfigurer : Global AuthenticationManager configured with an AuthenticationProvider bean. UserDetailsService beans will not be used for username/password login.

2 AuthenticationProviders, 1 UserDetailsService:

2024-03-08T14:23:23.048+01:00  WARN 49038 --- [           main] eAuthenticationProviderManagerConfigurer : Found 2 AuthenticationProvider beans, with names [myAuthProvider, secondAuthProvider]. Global Authentication Manager will not be configured with AuthenticationProviders.
2024-03-08T14:23:23.058+01:00  INFO 49038 --- [           main] r$InitializeUserDetailsManagerConfigurer : Global AuthenticationManager configured with UserDetailsService bean with name myUserDetailsService

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Mar 8, 2024
@jzheaux jzheaux self-assigned this Mar 21, 2024
@jzheaux jzheaux added in: core An issue in spring-security-core type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged labels Mar 21, 2024
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.

@Kehrlann, thank you! Aside from my inline comments, I think regarding logging levels we should consider whether it is ever correct to arrange a Spring application with two AuthenticationProvider beans. If it is, then I think DEBUG is a better level so that folks who intentionally have two don't get bugged for eternity by a needless message.

I lean towards DEBUG, but I'm open to your reflection as well.

@Kehrlann
Copy link
Contributor Author

@jzheaux thank you for the review, addressed the comments.

Thinking about it, I agree that two AuthenticationProvider beans is a valid Spring configuration and should probably not trigger a WARN log.

If a single AuthenticationProvider bean produces an INFO log, then having two beans should also produce an INFO log as well.
I'm tempted to keep it at the INFO level, as global authentication is not trivial to understand. Having a bit more logs will help users understand that this feature even exists.

@Kehrlann Kehrlann requested a review from jzheaux April 24, 2024 12:29
@jzheaux jzheaux added this to the 6.3.0 milestone Apr 25, 2024
@jzheaux jzheaux merged commit 7ddc005 into spring-projects:main Apr 25, 2024
3 checks passed
@jzheaux
Copy link
Contributor

jzheaux commented Apr 25, 2024

Thanks, @Kehrlann! This is now merged into main.

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: enhancement A general enhancement
Projects
Status: Done
3 participants