#563 Allow to use multiple authentication methods #575
Conversation
|
Oh interesting, thanks - will give a draft review by monday! |
|
Just a quick thought: For basic auth to work with browsers as a fallback we would need to make sure that the response generated by the BasicAuthenticationConcern ist returned to the client. 🤔 Or we declare this as unsupported for the sake of simplicity. |
Currently it will work if the basic auth is added last. Maybe we can force add basic auth to the end of the collection. As IMO if somebody wants to mix auth methods, it does not make sense that basic auth wouldn't be last as it requires user interaction with the browser while all other methods can be evaluate beforehand. What do you think? With the current way MultiConcernBuilder is laid out, we can easily decide the ordering, we can also prevent from adding basic auth twice or we can disallow passing BasicAuthenticationKnownUsersBuilder completely. |
|
Multiple basic authentications could be a use case (even if an extremely rare one) - I think it's fine to leave it as it is currently implemented and state in the documentation that basic auth works with challenges if added last. Less code is always better. Regarding the documentation on the website - I can write the docs for this feature, so you don't have to. Just tell me how you would like to have it. |
|
If you can contribute the documentation, that would be great! I can finalize the tests and we can ship it. 👍 |
|
@Kaliumhexacyanoferrat I think this version could be final. Though let me know your thoughts on the acceptace tests. I did not wanted to test all combinations of all existing auth methods. On the other hand with the current structure of tests and concerns in general, it is not possible to easily test the concern logic in isolation. So let me know if this approach is ok or if you'd suggest some more increments. |
There was a problem hiding this comment.
Looks good to me - thank you very much for the PR, just some minor notes on nullability and an unused method.
Regarding the tests: I think they are fine and you covered the important cases. I actually like to do those real-world kind of E2E-tests, as I discovered a lot of cases during development of the server that I would not have catched with unit tests only. Nevertheless there is nothing stopping us from adding unit tests (there are some in the test project) or mocking the IRequest and directly test the concern logic.
| public MultiAuthenticationConcern(IHandler content, IConcern[] delegatingConcerns) | ||
| { | ||
| Content = content; | ||
| _delegatingConcerns = delegatingConcerns ?? []; |
There was a problem hiding this comment.
With nullable enabled the ?? [] part cannot happen, no?
There was a problem hiding this comment.
Hmm I'm using nullable disabled in all my project as I didn't get used to this feature yet. So I'm used to a bit of a defensive coding style. So please bear with me. 😆 I'll review and update.
There is probably one question to you - developers can still use nullable disabled in their projects, while they reference genhttp that uses nullable enabled. This in practice means, that they can pass null to genhttp methods where null is not expected. This compiles and runs just fine, and you can still get nullreferenceexception inside genhttp. So the question is - do you want to be defensive for this scenario or not? (I guess probably not as that's how the rest of genhttp is written).
There was a problem hiding this comment.
Yes, I think I agree on this one - but only for the public API surface - so the main entry point should be MultiAuthentication and builder and not the concern (which could be internal theoretically).
I feel nullable support is kind of messy in C# - if I enable it, the CLR should automatically check for nulls and gate my non-nullable methods instead of just passings down null values. Seems to be implemented as a language feature and not a runtime CLR feature.
Nevertheless I think this should be decided on project level (see #576) rather than on PR level, so I would prefer removing the null checks for now.
TODO