Reuse failed auth results in AuthN #43269
Conversation
|
Thanks for your PR, @Kahbazi. Someone from the team will get assigned to your PR shortly and we'll get it reviewed. |
davidfowl
left a comment
There was a problem hiding this comment.
Keep these changes coming 👍🏾
davidfowl
left a comment
There was a problem hiding this comment.
Turns out you can mutate these results 😢
I can't see how! Could you please elaborate? Is it because of the |
There was a problem hiding this comment.
Can you change AuthenticateResult.NoResult to cache?
There's a theoretical issue with caching AuthenticateResult, it can reference mutable structures like ClaimsPrincipal, AuthenticationProerties, Exception, etc.. NoResult, Skipped, and Handled might be the only cases truly safe to cache because they don't hold any mutable objects. That might be worth overlooking for extremely common scenarios. Are any of these failures especially common?
edit Oh, already discussed above 😁.
(Merged since the feedback was trivial and we're at the rc1 deadline)
|
|
||
| internal static class AuthenticateResults | ||
| { | ||
| internal static AuthenticateResult NoSelfSigned = AuthenticateResult.Fail("Options do not allow self signed certificates."); |
There was a problem hiding this comment.
Style: Why define these in separate classes if they're only used in one class?
| /// </summary> | ||
| public class HandleRequestResult : AuthenticateResult | ||
| { | ||
| private static readonly HandleRequestResult _noResult = new() { None = true }; |
There was a problem hiding this comment.
Style: I think we use "NoResult" for class statics, and "_noResult" for class fields.
|
Hi @Kahbazi. It looks like you just commented on a closed PR. The team will most probably miss it. If you'd like to bring something important up to their attention, consider filing a new issue and add enough details to build context. |
No description provided.