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

Duplicate Authorization Bearer Header Fix #36811

Closed
wants to merge 6 commits into from
Closed

Duplicate Authorization Bearer Header Fix #36811

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
ed5b427
[36810] Duplicate Authorization Bearer Header
sahuefficy Nov 1, 2023
454612d
[36810] null check with headers
sahuefficy Nov 1, 2023
7522517
[36810] Combining the flow control
sahuefficy Nov 1, 2023
4692fbc
[36810] Replaced add with putSingle
sahuefficy Nov 1, 2023
4ca667e
[36810] Duplicate Authorization Bearer Header
sahuefficy Nov 2, 2023
6cb9a4e
Merge branch '36810' of https://github.com/sahuefficy/quarkus into 36810
sahuefficy Nov 2, 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
4 changes: 3 additions & 1 deletion ...ain/java/io/quarkus/oidc/token/propagation/reactive/AccessTokenRequestReactiveFilter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,7 +124,9 @@ protected String getClientName() {

public void propagateToken(ResteasyReactiveClientRequestContext requestContext, String accessToken) {
if (accessToken != null) {
requestContext.getHeaders().add(HttpHeaders.AUTHORIZATION, BEARER_SCHEME_WITH_SPACE + accessToken);
if (requestContext.getHeaders().get(HttpHeaders.AUTHORIZATION).stream().noneMatch(h -> h.toString().startsWith(BEARER_SCHEME_WITH_SPACE))) {
requestContext.getHeaders().add(HttpHeaders.AUTHORIZATION, BEARER_SCHEME_WITH_SPACE + accessToken);
Copy link
Member

Choose a reason for hiding this comment

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

@sahuefficy Thanks, can it be simplified so that the header is not added but set ? I was expecting only change from .add to .set (or put). Your fix is correct but simply setting the header would be as cheap or even faster when the duplication situation is possible

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The problem with set/put is that it would override a non-bearer scheme authorization header. Thats why add is the right thing to do. I have indeed pushed another commit that actually checks for null before streaming.

Copy link
Member

@sberyozkin sberyozkin Nov 1, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@sahuefficy But this is what the filter's job is. One would not apply it to the REST client if this REST client wants to use one scheme for one request, then another scheme for another request. If this filter is applied to a given REST client then it is only Authorization: Bearer that must be produced

Copy link
Member

@sberyozkin sberyozkin Nov 1, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@sahuefficy Having such a code in this filter introduces an ambiguity as far as the token propagation is concerned, if the REST client suddenly has some other scheme in front of it then it is effectively an illegal state exception from this filter's point of view. IMHO this use case where a REST client instance varies the Authorization schemes is not practical, but, if it ever required, users can do a custom filter

Copy link
Contributor Author

@sahuefficy sahuefficy Nov 1, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I have got your point and have changed the check as well as add a putSingle.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks @sahuefficy, but I think we should also drop the null check, as it leaves the same ambiguity open - if the header is already present - may be it is the basic scheme - now we need to decide if we want to report an error or not - but it is really not a decision that should be taken at this filter's level. I'm sorry this simple PR which is technically correct is still being reviewed :-), I do appreciate your time and patience. Please also squash the commits, thanks

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now the PR has become pretty simple with the add replaced with putSingle.

}
} else {
LOG.debugf("Access token is null, aborting the request with HTTP 401 error");
abortRequest(requestContext);
Expand Down
4 changes: 3 additions & 1 deletion ...e/src/main/java/io/quarkus/oidc/token/propagation/runtime/AbstractTokenRequestFilter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,9 @@ public abstract class AbstractTokenRequestFilter implements ClientRequestFilter

public void propagateToken(ClientRequestContext requestContext, String token) throws IOException {
if (token != null) {
requestContext.getHeaders().add(HttpHeaders.AUTHORIZATION, BEARER_SCHEME_WITH_SPACE + token);
if (requestContext.getHeaders().get(HttpHeaders.AUTHORIZATION).stream().noneMatch(h -> h.toString().startsWith(BEARER_SCHEME_WITH_SPACE))) {
requestContext.getHeaders().add(HttpHeaders.AUTHORIZATION, BEARER_SCHEME_WITH_SPACE + token);
}
} else {
LOG.debugf("Injected access token is null, aborting the request with HTTP 401 error");
abortRequest(requestContext);
Expand Down