-
Notifications
You must be signed in to change notification settings - Fork 12
fix: add additional application/json accept header to issuer client #4210
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
Conversation
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.
Good catch! That is really a problem, for Authentication Provider who enforce the OIDC specification and don't allow other accept headers.
Our tests also uses the JSON as response type and never checked for the correct ACCEPT header.
Could you add a check for the ACCEPT header into the Wiremock stub configuration for the token Endpoint in the different OidcClientTest classes? (package: sda-commons-client-jersey/src/test/java/org/sdase/commons/client/jersey/oidc/)
for example in OidcClientTest itself:
WIRE.stubFor(
post("/token")
.withHeader(ACCEPT, equalTo(APPLICATION_FORM_URLENCODED + "," + APPLICATION_JSON)) //or similar check
.willReturn(
aResponse()
.withStatus(200)
.withHeader(CONTENT_TYPE, APPLICATION_JSON)
.withBodyFile("fixtures/tokenResponse.json")));
so that it is checked, that now both headers are always send to the Authorization Provider?
51740a4 to
eaf9f4a
Compare
|
@YellowFlora thanks for the review. Please have a look again |
YellowFlora
left a comment
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.
Looks good!
Its only missing in OidcRequestFilterTest; the stub configuration is at the bottom there
eaf9f4a to
4b73a01
Compare
YellowFlora
left a comment
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 will check sonar tmr morning, not sure yet why it doesnt work)
|
Hi @YellowFlora have you already looked into the sonar issue? |
|
Hi @pathec we downgraded Sonar now, so it should work now |
4b73a01 to
ced5dc2
Compare
|
🎉 This PR is included in version 8.1.4 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
No description provided.