-
Notifications
You must be signed in to change notification settings - Fork 75
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
feat(jans-auth-server): log httpresponse body configurated by httpLoggingResponseBodyContent #349 #4417
feat(jans-auth-server): log httpresponse body configurated by httpLoggingResponseBodyContent #349 #4417
Conversation
Error: Hi @jmunozherbas, You did not reference an open issue in your PR. I attempted to create an issue for you. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See inline comment
if (appConfiguration.getHttpLoggingResponseBodyContent()) { | ||
httpResponse.setStatus(((ReadableResponseWrapper) responseWrapper).getStatus()); | ||
httpResponse.setHeaders(((ReadableResponseWrapper) responseWrapper).getHeaders()); | ||
httpResponse.setBody(((ReadableResponseWrapper) responseWrapper).readBodyValue()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jmunozherbas would you please extensively test this change and confirm it works good ? Because this line sounds like we read from writer and close it. So down the chain filters will not be able to process it. As result it can cause not returing response out of the server. It's similar problem as reading input stream in request which can make it unavailable further.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have applied some changes.
I have tested with jans-auth-client heap of tests, and results are similar than before the change.
Clarification.- The changes and new flows just be applied if the parameter "httpLoggingResponseBodyContent" is enabled, if this parameter doesn´t exist or is false, then the flow is executed as usual.
The code to read body from response is based on next example:
search "Logging requests and responses" in next page:
https://spaces.at.internet2.edu/display/Grouper/Grouper+Web+Services
code repository:
https://github.com/Internet2/grouper/tree/master/grouper-ws/grouper-ws/src/grouper-ws/edu/internet2/middleware/grouper/ws/j2ee
maven repository of this code library:
https://mvnrepository.com/artifact/edu.internet2.middleware.grouper/grouper-ws/5.0.3
There is an example to read request body too, maybe could be implemented.
The log printting in just one line (request and response) could be implemented too, similar to the example code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If all tests are passed with httpLoggingResponseBodyContent=true
then I guess we are good to merge.
[Jans authentication server parent] SonarCloud Quality Gate failed. |
Lets correct sonar reports before merge. |
[jans-cli] Kudos, SonarCloud Quality Gate passed! |
[jans-linux-setup] Kudos, SonarCloud Quality Gate passed! |
[agama parent] Kudos, SonarCloud Quality Gate passed! |
[jans-core] Kudos, SonarCloud Quality Gate passed! |
[jans-pycloudlib] Kudos, SonarCloud Quality Gate passed! |
[notify] Kudos, SonarCloud Quality Gate passed! |
[jans-config-api-parent] Kudos, SonarCloud Quality Gate passed! |
[SCIM API] Kudos, SonarCloud Quality Gate passed! |
Prepare
Description
A new AppConfiguration param for logging response body.
Target issue
#349
closes #349
Implementation Details
The response body is logged if param "httpLoggingResponseBodyContent" is true in jans-auth AppConfiguration.
(ConfDynamic)
If param not exist or is false, then response body is not logged.
Test and Document the changes
Closes #4418,