Change HttpLoginSecurityFilter to return 401 when the client responds by sending a different scheme

[stanculescu]

@dpwspoon can you please review my PR? Another fix I was thinking is calling the method HttpLoginSecurityFilter.loginMissingToken when the schemes are different.

[dpwspoon]

Another fix I was thinking is calling the method HttpLoginSecurityFilter.loginMissingToken when the schemes are different.

If that works, that is ok.

@stanculescu this looks ok, to me but it is hard to tell without tests.

Please add or check that at a minimum we have tests for:

1. scheme are case insensitive
2. 401 for pure HttpAcceptor on missing scheme (maybe we already have)
3. 401 for pure HttpAcceptor on scheme is not of correct type
4. 401 on multiple http realms where the second scheme differs

These should all run in the http.transport module.

If test "4. 401 on multiple http realms where the second scheme differs" works before your change, consider moving the logic back out of the scheduled Thread or combining your logic with what is invoking it. If it does not work, then consider optimizing the initial case by having generic logic somewhere where both use cases check (don't perform the check twice!). This optimization does not need to be done, but if you can implement it in a clean way do it, though I'd rather have it implemented cleanly than optimized for a slight speed up on initial connect (1 time hit).

[dpwspoon]

@stanculescu, thank you. One nit on the comment, and then my apologies. I wasn't clear enough in the comments above, it would be better if the tests were in the transport.http module. This is what I was trying to say by adding tests on the "pure HttpAcceptor". Making this change improves your work because code changes made in transport.http (like you are doing) are tested in the same module. Instead, you added the tests to ws transport. Functionally, I don't see any issues with your implementations or tests (other than minor nit on the log). But I would still like to aim at getting the tests in the correct (Better) location. Thanks!

@danibusu will you please review after the last update (feel free to merge too!) Thanks

[stanculescu]

@dpwspoon thanks for the review, I moved the tests in transport.http as you suggested. The test "4. 401 on multiple http realms where the second scheme differs" didn't work before my change. As an optimization I was thinking to call a method that perform the check out of the scheduled Thread for the first realm and inside the HttpLoginSecurityFilter.login for the realms that haven't been logged yet successfully by adding a new parameter in login method signature "realmStartAt" and comparing with the current index of realm. If realmStartAt > realmIndex do nothing, otherwise call the check method. But I don't know if this is a good solution or just makes the code harder to read.

[dpwspoon]

As an optimization ...

I don't think it is worth doing the optimization here as it adds complexity and if the strategy of going to a scheduled thread doesn't perform that well then we already have a bigger issue with the whole login process. Additionally, if we were spending the time to optimize it, it would be better to measure before and after. I only originally brought it up because I noticed the change, but the change makes perfect sense as we now support multiple realms.

Please don't implement it, but if we were to do it, a perfectly reasonable approach would be to have the check in both places without special casing the second check. Meaning the scheme gets checked twice for the first realm, but the second check always succeed. I think this is better than the index approach because the index is trying to couple code in 2 different spots that are not programmatically coupled. Thus, it seems like the index approach is asking for trouble in refactoring as there is nothing implicitly tieing the code together. Moreover, this is a security related part of the product so we should be even less inclined to add special cases for it. Also String.equalsIgnore case vs index !== 0 seems to be the performance change in this alternative approach.

[dpwspoon]

LGTM @danibusu please merge if you are happy with the changes! Thanks @stanculescu / @danibusu