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

Add logging #956

Closed
wants to merge 2 commits into from
Closed

Add logging #956

wants to merge 2 commits into from

Conversation

sjohnr
Copy link
Member

@sjohnr sjohnr commented Nov 2, 2022

For consistency with Spring Security (best effort), the logs introduced in this PR use the following keywords:

  • "Retrieved"
  • "Validated"
  • "Generated"
  • "Saved"
  • "Removed"
  • "Revoked"
  • "Authenticated"
  • "Set SecurityContextHolder"
  • "Did not ... since ..."

Note: We also use a console color highlighting plugin (GrepConsole) in IntelliJ that looks for keywords in Spring Security logs which the above could be added to. Currently we look for:

  • "Invoking"
  • "Authenticating"
  • "Authenticated"
  • "Authorizing"
  • "Authorized"
  • "Securing"
  • "Secured"
  • "Set SecurityContextHolder"

Here's an example log file with Spring Security trace enabled:

application.log

Examples:

Screen Shot 2022-11-01 at 7 45 15 PM

Screen Shot 2022-11-01 at 7 48 54 PM

Closes gh-159

@sjohnr sjohnr added this to the 0.4.0 milestone Nov 2, 2022
@sjohnr sjohnr requested a review from jgrandja November 2, 2022 00:29
@sjohnr sjohnr self-assigned this Nov 2, 2022
@sjohnr sjohnr force-pushed the gh-159-logging branch 2 times, most recently from 849601d to 9c3bd84 Compare November 2, 2022 01:11
@jgrandja jgrandja added status: duplicate A duplicate of another issue type: enhancement A general enhancement labels Nov 2, 2022
@sjohnr sjohnr changed the base branch from main to 0.4.x November 2, 2022 15:57
Copy link
Collaborator

@jgrandja jgrandja left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @sjohnr.

Please add logs to OidcClientRegistrationAuthenticationProvider and OidcClientConfigurationAuthenticationProvider.

Also, for all Filter's before calling AuthenticationManager.authenticate(), let's add the log:

if (logger.isTraceEnabled()) {
	logger.trace(LogMessage.format("Authenticating %s", authenticationRequest.getClass().getSimpleName()));
}

@sjohnr sjohnr force-pushed the gh-159-logging branch 2 times, most recently from 9b99528 to 24b8f43 Compare November 3, 2022 20:52
@sjohnr sjohnr requested a review from jgrandja November 4, 2022 15:06
@jgrandja
Copy link
Collaborator

jgrandja commented Nov 9, 2022

Thanks for the PR @sjohnr ! This is now merged.

The TRACE logging is going to be super valuable for troubleshooting issues 👍

@jgrandja jgrandja closed this Nov 9, 2022
@sjohnr sjohnr deleted the gh-159-logging branch November 14, 2022 20:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: duplicate A duplicate of another issue type: enhancement A general enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add logging
2 participants