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

Logout ely 2534 #2249

Open
wants to merge 14 commits into
base: 2.x
Choose a base branch
from
Open

Logout ely 2534 #2249

wants to merge 14 commits into from

Conversation

rsearls
Copy link
Contributor

@rsearls rsearls commented Jan 13, 2025

@rsearls rsearls marked this pull request as ready for review January 13, 2025 21:25
Copy link
Contributor

@pedro-hos pedro-hos left a comment

Choose a reason for hiding this comment

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

LGTM overall! One small improvement could be adjusting the debug message to match the format of other log statements. For instance, instead of:

log.debug("evaluateRequest uri: " + request.getRequestURI().toString());

You might consider using log.debugf for consistency:

log.debugf("The evaluated request URI is [%s]", request.getRequestURI().toString());

This keeps the style aligned with existing logs, like:

log.debugf("Ignoring request for path [%s] from mechanism [%s]. No client configuration context found." ...);

That said, this is a minor improvement — everything else looks great to me!

private static final String POST_LOGOUT_REDIRECT_URI_PARAM = "post_logout_redirect_uri";
private static final String ID_TOKEN_HINT_PARAM = "id_token_hint";
private static final String LOGOUT_TOKEN_PARAM = "logout_token";
private static final String LOGOUT_TOKEN_TYPE = "Logout";
Copy link
Contributor

Choose a reason for hiding this comment

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

"Logout" with the first capital letter is the expected value?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes "Logout" is expected it looks to be a key for claim types. similarly "Bearer" is capitalized as well.

I changed the log.debug ref to log.debugf only in the code I added. I prefer to leave pre-existing code as is.

Thanks for the code review and the suggestions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants