HTTPCLIENT-2414 - Fix Basic auth cache scoping across path prefixes#802
HTTPCLIENT-2414 - Fix Basic auth cache scoping across path prefixes#802arturobernalg wants to merge 1 commit intoapache:masterfrom
Conversation
rPraml
left a comment
rPraml
left a comment
There was a problem hiding this comment.
Test looks good and the fix makes sense to me
(But I’m not really deep enough into the topic to know if it could have any other side effects.)
| }); | ||
| } | ||
|
|
||
| Assertions.assertEquals(Arrays.asList(401, 200, 401, 200), responseCodes); |
| this.state = State.UNCHALLENGED; | ||
| this.authOptions = null; | ||
| this.authScheme = null; | ||
| this.pathPrefix = null; |
There was a problem hiding this comment.
When I debugged the issue in my code, I come to exactly this place, where the previously set path prefix was cleared.
So I think, not setting pathPrefix = null here will fix the issue.
ok2c
left a comment
ok2c
left a comment
There was a problem hiding this comment.
@arturobernalg It is good that we a have a fix for the defect, but as a side effect the AuthExchange#reset method now kind of resets the state but not quite. This is wrong. conceptually the fix is all right but it need to be moved from the #reset method to the protocol layer.
arturobernalg
force-pushed
the
HTTPCLIENT-2414
branch
2 times, most recently
from
3f06fa0 to
31f81ae
Compare
Preserve AuthExchange pathPrefix on reset to avoid preemptive Authorization reuse outside the protection space.
arturobernalg
force-pushed
the
HTTPCLIENT-2414
branch
from
d9afadb to
b1cbadc
Compare
|
| responseQueue.stream().map(HttpResponse::getCode).collect(Collectors.toList())); | ||
| assertThat( | ||
| responseQueue.stream().map(HttpResponse::getCode).collect(Collectors.toList()), | ||
| CoreMatchers.equalTo(Arrays.asList(200, 401, 200, 401, 200, 401, 200))); |
There was a problem hiding this comment.
@arturobernalg This worries me a little. I want to understand why the behavior has changed.
There was a problem hiding this comment.
@arturobernalg This worries me a little. I want to understand why the behavior has changed.
The expectation in that assertion was effectively relying on the client reusing Basic auth preemptively across unrelated path prefixes within the same context (so it behaved almost host-wide). With my change we now keep preemptive auth scoped to the authenticated path prefix, so when the request moves from /yada/ back to /blah/ it no longer sends Authorization blindly and you see a fresh 401/200 again.
There was a problem hiding this comment.
@arturobernalg I see. My test case was wrong from the very beginning. The path prefixes of the first and the third requests are different. I overlooked it. Your fix is correct. Please just remove unnecessary code at line 166 in ProtocolExec and line 162 in AsyncProtocolExec
if (targetAuthExchange.getPathPrefix() == null) {
targetAuthExchange.setPathPrefix(pathPrefix);
}
3 participants
Preserve AuthExchange pathPrefix on reset to avoid preemptive Authorization reuse outside the protection space.