Handle Default Authentication using events

[MikeAlhayek]

By utilizing middleware instead of controllers for performance optimization, we can keep the login code streamlined and free from excessive external authentication logic.

[Piedone]

How does using a middleware instead of a controller optimize performance?

[kevinchalet]

I agree with @Piedone, there's likely no gain at all to expect from a performance perspective. On the other hand, moving that to a middleware introduces new risks like responses collision (you're not checking whether the response has already been sent or not so you could potentially end up in a situation where your custom middleware will end up calling SignOutAsync() on an HTTP response whose headers have already been returned, which isn't going to work with hosts that use streamed responses).

If performance was your main motivation for that, I'd say it's not worth the risk 😃

[MikeAlhayek]

@Piedone

How does using a middleware instead of a controller optimize performance?

Quicker response time since it's done earlier on "before routing to a controller, and much less resource to create during the login request

@kevinchalet main goal isn't performance here since this is only handled by one action. Main goal is to clean up the code so that the external authentication logic isn't coupled with the code in the Users feature. Also, to reduce the amount of code we used for the default login logic.

[kevinchalet]

Quicker response time since it's done earlier on "before routing to a controller, and much less resource to create during the login request

Did you measure it? 😄

It's worth noting that middleware aren't really free. And unlike the existing approach, the price will now be paid for basically all requests (e.g you'll have to pay for the async state machine whether the request path matches or not the /login path). I really doubt you'll win anything substantial by converting the existing controller to a middleware.

@kevinchalet main goal isn't performance here since this is only handled by one action. Main goal is to clean up the code so that the external authentication logic isn't coupled with the code in the Users feature. Also, to reduce the amount of code we used for the default login logic.

In this case, why not an event? It's the good old way of doing things in Orchard 😃

[MikeAlhayek]

Did you measure it?

No. Again, the idea behind this PR isn't optimization is more of decoupling the code. But, optimization and reducing allocations would be nice too.

In this case, why not an event? It's the good old way of doing things in Orchard

I am open to ideas. How would we use handlers in this case challenge the request from an action?

[MikeAlhayek]

@kevinchalet I am now using event to handle this which simplifies the code and we won't need a middleware.

[MikeAlhayek]

@Piedone anything else to add here?

[kevinchalet]

[Piedone]

I see there's no middleware anymore, so good enough for me :). Can't comment otherwise.

[sebastienros]

We had a discussion about base classes during triage and decided (we forced Mike) to remove the base class. We think that we should accept base classes in these conditions:

• We can provide a single implementation of a base class if it adds some value (no empty base class)
• We provide 2 or more implementations to show there is a behavior to choose from
• We don't implementation default interface methods, unless to prevent a breaking change (adding a new method to the interface)
• Base classes are documented with the reason when to use it (ideally any consumable implementation of an interface)