Improve logging #1467
Improve logging #1467
Conversation
spring-projects-issues
added
the
status: waiting-for-triage
| 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); |
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
@leshalv I would be open to enhancing the trace logging if that works for you?
There was a problem hiding this comment.
@leshalv I would be open to enhancing the
tracelogging 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.
There was a problem hiding this comment.
@leshalv I would be open to enhancing the
tracelogging 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.
There was a problem hiding this comment.
@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.
jgrandja
added
status: declined
jgrandja
changed the title
jgrandja
added
type: enhancement
leshalv
changed the title
|
@jgrandja I made logging enhancements in some key places. |
|
@leshalv Apologies I didn't have time to review the changes. I'm off for the holidays and back Jan 8. I will review it the week I'm back. Enjoy the holidays! |
|
Thanks for the updates @leshalv. This is now merged. FYI, I added a polish commit with some minor updates. |
#1240