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 logging #1467

Closed
wants to merge 6 commits into from
Closed

Improve logging #1467

Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
52a1e82
Improve the error log and enhancing OAuth2Error
leshalv Dec 1, 2023
38ab258
Improvement
leshalv Dec 1, 2023
b7420b4
Merge branch 'spring-projects:main' into patch-1
leshalv Dec 15, 2023
5ffa069
Improve logging in OAuth2AuthorizationCodeAuthenticationProvider
leshalv Dec 15, 2023
2624df8
Improve logging
leshalv Dec 15, 2023
3e19ace
Improve logging
leshalv Dec 15, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 9 additions & 3 deletions ...h2/server/authorization/authentication/OAuth2AuthorizationCodeAuthenticationProvider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,12 +143,17 @@ public Authentication authenticate(Authentication authentication) throws Authent
this.logger.warn(LogMessage.format("Invalidated authorization code used by registered client '%s'", registeredClient.getId()));
}
}
throw new OAuth2AuthenticationException(OAuth2ErrorCodes.INVALID_GRANT);
OAuth2Error error = new OAuth2Error(OAuth2ErrorCodes.INVALID_GRANT,"The authorization code is invalid or has expired.",ERROR_URI);
throw new OAuth2AuthenticationException(error);
}

if (StringUtils.hasText(authorizationRequest.getRedirectUri()) &&
!authorizationRequest.getRedirectUri().equals(authorizationCodeAuthentication.getRedirectUri())) {
throw new OAuth2AuthenticationException(OAuth2ErrorCodes.INVALID_GRANT);
if (this.logger.isWarnEnabled()) {
this.logger.warn(LogMessage.format("Invalidated redirect_uri used by registered client '%s'", registeredClient.getId()));
}
OAuth2Error error = new OAuth2Error(OAuth2ErrorCodes.INVALID_GRANT,"The redirect_uri does not match the redirection URI used in the authorization request.",ERROR_URI);
Copy link
Collaborator

Choose a reason for hiding this comment

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

@leshalv We do not want to reveal too much information to the caller as it can provide hints to perform other operations in order to gain access. It is intentional to return only a general error code.

At some point, we may look at addressing this in gh-1240.

I'm going to close this PR for reasons explained above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When docking integration, it is easy to have some problems, when troubleshooting problems, it is difficult to know the specific problem, here is my reference to okta back to do, which should not affect major security

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Or we could consider the PR, including its log input, masking this detail in the exception handler, which can be customized by a custom exception handler if the developer wants to return it.
@jgrandja

Copy link
Contributor Author

Choose a reason for hiding this comment

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

At present, I look at the implementation, only the server exception is returned details, in fact, we can do not return details of the INVALID_GRANT exception in the exception handler, the decision is left to the user-defined processor.
@jgrandja

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We are currently developing IAM systems based on springsecurity, and there are a lot of issues around exceptions, because there is no log print there, it is difficult to know what is going on. I have also looked at Okta, Auth0 and other SSO platforms, and I find that everyone has different wrong responses. I can also override this implementation, but considering the connection with the upstream, including subsequent upgrades, I think this piece should be synchronized to the upstream, perhaps we can optimize this piece through other ways, such as in the exception processor mask, the user through the custom processor open.
@jgrandja

Copy link
Collaborator

Choose a reason for hiding this comment

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

@leshalv I would be open to enhancing the trace logging if that works for you?

Copy link
Contributor Author

@leshalv leshalv Dec 6, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@leshalv I would be open to enhancing the trace logging if that works for you?

Enhancing trace was a good decision.
Can you consider masking described scenarios in exception handlers? Because we've done the same thing on other processors.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@leshalv I would be open to enhancing the trace logging if that works for you?

I think we can reference OAuth2ClientAuthenticationFilter processing implementation, does not return to the default, the user can customize the processor to open, believe me, it's really elegant.

image

spring-authorization-server/oauth2-authorization-server/src/main/java/org/springframework/security/oauth2/server/authorization/web/OAuth2ClientAuthenticationFilter.java

Line 201 in 068601b

OAuth2Error errorResponse = new OAuth2Error(error.getErrorCode());

Copy link
Collaborator

Choose a reason for hiding this comment

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

@leshalv I'm reopening this PR so feel free to update it with trace logging only.

Can you consider masking described scenarios in exception handlers?

OAuth2ClientAuthenticationFilter is the only Filter that does this at the moment and I'm not sure I want to follow this pattern for all Filter's. I'd rather give it some further thought and come up with a more holistic solution that we will address in gh-1240, when we schedule the time to work on it.

throw new OAuth2AuthenticationException(error);
}

if (!authorizationCode.isActive()) {
Expand All @@ -165,7 +170,8 @@ public Authentication authenticate(Authentication authentication) throws Authent
}
}
}
throw new OAuth2AuthenticationException(OAuth2ErrorCodes.INVALID_GRANT);
OAuth2Error error = new OAuth2Error(OAuth2ErrorCodes.INVALID_GRANT,"The authorization code is invalid or has expired.",ERROR_URI);
throw new OAuth2AuthenticationException(error);
}

if (this.logger.isTraceEnabled()) {
Expand Down
Loading