Duplicate Authorization Bearer Header Fix #36811
Conversation
|
Thanks for your pull request! The title of your pull request does not follow our editorial rules. Could you have a look?
This message is automatically generated by a bot. |
sahuefficy
changed the title
| @@ -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); | |||
There was a problem hiding this comment.
@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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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
There was a problem hiding this comment.
@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
There was a problem hiding this comment.
I have got your point and have changed the check as well as add a putSingle.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Now the PR has become pretty simple with the add replaced with putSingle.
There was a problem hiding this comment.
@sahuefficy Thanks, and again, apologies it took awhile to complete it, I just felt introducing the variations into the token propagation feature could complicate things.
LGTM. The only problem now, is that this PR now has 6 commits, and something did not work out during the squash, so, if you prefer, try to clean up the commit history to keep a single top level commit only but I guess at this stage it would be simplest to create a new PR with this simple update only, and once you open it then we can close this one.
|
Here you go #36835 |
Fix for #36810