Remove default tasks for OIDC notifications, perform null check before any work.

[brentschmaltz]

The constructor sets no-op notifications.

public OpenIdConnectAuthenticationNotifications()
        {
            AuthenticationFailed = notification => Task.FromResult(0);
            AuthorizationCodeReceived = notification => Task.FromResult(0);
            MessageReceived = notification => Task.FromResult(0);
            SecurityTokenReceived = notification => Task.FromResult(0);
            SecurityTokenValidated = notification => Task.FromResult(0);
            RedirectToIdentityProvider = notification => Task.FromResult(0);
        }

This results in the handler doing a bunch of work for nothing. Wrap the whole thing in:
if (... != null ) { do work }
instead of:

 var messageReceivedNotification =
     new MessageReceivedNotification<OpenIdConnectMessage, OpenIdConnectAuthenticationOptions>(Context, Options)
     {
         ProtocolMessage = message
     };

 await Options.Notifications.MessageReceived(messageReceivedNotification);
 if (messageReceivedNotification.HandledResponse)
 {
     Logger.LogVerbose(Resources.OIDCH_0002_MessageReceivedNotificationHandledResponse);
     return messageReceivedNotification.AuthenticationTicket;
 }

 if (messageReceivedNotification.Skipped)
 {
     Logger.LogVerbose(Resources.OIDCH_0003_MessageReceivedNotificationSkipped);
     return null;
 }

[kevinchalet]

Hum, I'm not sure about this change: is the perf gain worth it, compared to the noise it will add to the code?

If we decide to bring back an IOpenIdConnectAuthenticationNotifications interface with good old methods, you won't have to care about null delegates in the handler.

[brentschmaltz]

What does IOpenIdConnectAuthenticationNotifications change look like?

[kevinchalet]

Exactly like the OAuth2 client middleware: https://github.com/aspnet/Security/blob/dev/src/Microsoft.AspNet.Authentication.OAuth/Notifications/IOAuthAuthenticationNotifications.cs

[brentschmaltz]

@PinpointTownes What is the reason for changing the notification model?
How does it address this performance issue?

[brentschmaltz]

@Tratcher you added a 'need-design' tag. Does that indicate some type of deliverable? If so what does it look like?

[kevinchalet]

I never said it was supposed to address a performance issue 😄

It will only be changed for consistency: #257 (comment)

[brentschmaltz]

@PinpointTownes on the performance issue, I want to be able to determine if a notification was set by the user.
Will the interface model require a specific interface for each middleware?

[kevinchalet]

Cookies, OAuth2 and Twitter already have their own interface:
https://github.com/aspnet/Security/blob/dev/src/Microsoft.AspNet.Authentication.Cookies/Notifications/ICookieAuthenticationNotifications.cs
https://github.com/aspnet/Security/blob/dev/src/Microsoft.AspNet.Authentication.OAuth/Notifications/IOAuthAuthenticationNotifications.cs
https://github.com/aspnet/Security/blob/dev/src/Microsoft.AspNet.Authentication.Twitter/Notifications/ITwitterAuthenticationNotifications.cs

Only OAuth2 bearer and OIDC will need to be updated.

[brentschmaltz]

Yes each having it's own interface was something we tried to avoid when we introduced WsFed and OIDC into Katana.

Either way, I would not set a default no-op.

[Tratcher]

needs-design means that there are open questions to answer before coding.

[brentschmaltz]

@Eilon @davidfowl
this issue is blocking our Code Flow scenario that is very important.
Can you help me out with a ruling here.
Thanks.

[kevinchalet]

I'm not sure to understand how this thing can be blocking for the code flow support.