[FIXED JENKINS-25144] Http BasicAuth broken in combination with a web Session #1427
Conversation
return authentication object instead of null if authentication is not required - otherwise valid login fails with basic authentication
|
Link to issue in Jenkins JIRA: https://issues.jenkins-ci.org/browse/JENKINS-25144 |
cschoell
changed the title
| makeRequestWithAuthAndVerify(null, "bar"); | ||
|
|
||
| // if the session cookie is valid, and basic header is set anyway login should not fail either | ||
| makeRequestWithAuthAndVerify("bar:bar", "bar"); |
There was a problem hiding this comment.
Minor comment: A direct unit test for a critical bug would be preferable
There was a problem hiding this comment.
I fully appreciate that.
However to add a proper unit Test for this code I would have to change code in the BasicHeaderRealPasswordAuthenticator to make it testable. I would prefer to leave this to the original author of the code - since he has more knowledge of the internals...
The BasicHeaderProcessorTest does fail without the fix though (if that helps?)
There was a problem hiding this comment.
So is this going to be merged? It would allow my company to use the next version of jenkins again ;)
…cessed. Previously, basic auth header was processed from two different servlet filters in a single filter chain. In case the 1st filter (ApiTokenFilter) manages to authenticate the request, the 2nd filter (BasicProcessingFilter) tries to avoid interpreting the API token as the password and failing authentication (see BasicProcessingFilter.authenticationIsRequired), but the check feels rather fragile. Although I did eventually discover that the original problem (ZD-19640) was not caused by this, I've already implemented & tested this change, and this feels like a good work to be wasted, so I'm pushing this in anyway. Refrence: ZD-19640
|
I think @kohsuke as the author of the regressing commit should evaluate this. |
|
Fair enough - if I can help in any way, let me know - it would really help us alot if this bug gets fixed soon though ;) Since anyone using a proxy with an sso solution cannot use the recent jenkins versions at all I would suggest to merge this (rather simple) fix and add an issue for @kohsuke to have a look at it. He does not seem to have the time to look at it either ... (19 days later...) ? |
|
So are you planning to merge this anytime soon? Or fix the corresponding bug otherwise? It would be very much appreciated.... |
|
Sorry for being late. IIUC, the issue here is that the request in question contains both a valid session cookie AND basic authentication header, and that path results in a failure because I think the corect fix is for The problem with having this logic in Let me adjust the fix. |
|
Yey that's exactly what I thought and I did actually prepare a fix with the authenticationIsRequired in the BasicHeaderProcessor, however I thought it might hinder the acceptance of this Bugfix, since it makes the change more complicated ;) I could have pushed my change tommorow but as I can see you added the fix already so thanks! 👍 |
return authentication object instead of null if authentication is not required - otherwise valid login fails with basic authentication