Fixing so that we can have multiple of same claim type #66
Conversation
| /// </summary> | ||
| public class ClientPrincipal | ||
| { | ||
| #pragma warning disable CA1707, CS8618 |
There was a problem hiding this comment.
Can we add the warnings that are being disabled as a comment?
I have no idea what CA1707 and CS8618 are.
| } | ||
|
|
||
| response.ContentType = "application/json; charset=utf-8"; | ||
| var options = new JsonSerializerOptions(_serializerOptions) |
There was a problem hiding this comment.
You could make the JsonSerializerOptions a class variable instead of creating it each time.
There was a problem hiding this comment.
Thanks. Makes sense.
| /// <param name="request"><see cref="HttpRequest"/> that holds all the request information.</param> | ||
| /// <param name="response"><see cref="HttpResponse"/> that will be written to.</param> | ||
| /// <returns>Awaitable task.</returns> | ||
| public async Task Handler(HttpRequest request, HttpResponse response) |
There was a problem hiding this comment.
Just a style preference but I like to turn more complex conditionals into predicate functions.
So instead of checking all the request headers individually, a function called something like HasValidIdentityHeaders(). Also something further down like "IsValidJsonToken".
For a more simple method with a more technical role, it's perfectly ok to keep everything inline.
There was a problem hiding this comment.
I agree to this, and also revert the test to reduce nesting
Fixed
IdentityProviderEndpointto handle multiple claims with same type.IdentityProviderEndpointand setup to be more testable.IdentityProviderEndpointand setup code.