Clean up notification extensibility model about security HOT 11 CLOSED

@PinpointTownes does your PR address everything here?

from security.

@HaoK not really. This ticket was about unifying I*AuthenticationProvider and *AuthenticationNotifications, but I*AuthenticationProvider has since been replaced by I*AuthenticationNotifications.

The question is now: do we want to have...

• an I*AuthenticationNotifications interface and a default *AuthenticationNotifications class exposing Func properties like the OAuth2 client middleware https://github.com/aspnet/Security/blob/dev/src/Microsoft.AspNet.Authentication.OAuth/Notifications/IOAuthAuthenticationNotifications.cs and https://github.com/aspnet/Security/blob/dev/src/Microsoft.AspNet.Authentication.OAuth/Notifications/OAuthAuthenticationNotifications.cs
• or a single *AuthenticationNotifications class that doesn't have any method and simply exposes Func properties, like the OIDC client middleware: https://github.com/aspnet/Security/blob/dev/src/Microsoft.AspNet.Authentication.OpenIdConnect/OpenIdConnectAuthenticationNotifications.cs

The first approach is definitely less DRY but seems more flexible and allows advanced scenarios like interface interception via Dynamic Proxy.

from security.

I recommend the second approach. It's much cleaner and I have never seen anyone utilize the extensibility provided by the first approach.

from security.

@Tratcher here's a sample of subclassing a default *AuthenticationProvider class: https://github.com/aspnet-contrib/AspNet.Security.OpenIdConnect.Server/blob/vNext/samples/Mvc/Mvc.Server/Providers/AuthorizationProvider.cs#L11

Overriding a method is much cleaner than assigning a delegate.

from security.

@HaoK We need to do design on this. @glennc can assign an existing design meeting slot for this.

from security.

Do we finally have an agreement on the definitive notification pattern?

(related topic: IdentityServer/IdentityServer3#1437)

from security.

Nope...

from security.

Notes:

• Rename *Notifications -> Events
• Change OIDC notifications -> use the I*Notifications pattern (see cookies)
• Move all of the OIDC base notification objects from Authentication/Notifications to ODIC/Notifications, squash the generic. Rename to *Context.
• Rename OAuthBearer to OAuthJWTBearer or something similar? Its not generic bearer, it’s JWT IdentityModel bearer.

I’ll file separate bugs for these.

from security.

Filed #413 for OAuthBearer. The others are all interrelated and can be handled together as part of this #.

from security.

Nice! 👍

from security.

Related: #307

from security.