U4-10369 - Make UmbracoDefaultOwinStartup more flexible with nicer cross cutting

Created by Shannon Deminick 31 Aug 2017, 04:32:35 Updated by Sebastiaan Janssen 17 Oct 2017, 13:20:05

Tags: Up For Grabs Unscheduled PR Community Contrib

Relates to: U4-9081

Currently there are 2 overridable methods on UmbracoDefaultOwinStartup:

  • ConfigureMiddleware
  • ConfigureServices

which mimic how ASP.NET Core does startup and that is fine, however in some cases it requires that a developer re-implement much of what is going on inside of these methods and it would be nicer for better cross cutting concerns than to override a whole block.

Comments

Shannon Deminick 31 Aug 2017, 04:33:24

PR for external identities: https://github.com/umbraco/Umbraco-CMS/pull/2162/files


Shannon Deminick 31 Aug 2017, 04:33:45

Quote from previous issue:

After trying to figure out why Facebook/Google OAuth providers would appear and the OAuth calls occurred but Umbraco would never link or allow logins, I realized that the configuration for the OAuth providers needs to happen after ConfigureMiddleware runs. It doesn't seem to me that ConfigureMiddleware is a logical place for users to be overriding for this purpose, so I've put together a PR at https://github.com/umbraco/Umbraco-CMS/pull/2162 that adds a new ConfigureExternalLogins method that runs after ConfigureMiddleware . Users can configure OAuth providers by overriding that method.

The question becomes what to do with UmbracoIdentityExtensions. In order to get the benefit of the restructuring to make things more sensible, we'd need to jump the dependency requirements all the way forward to now. What are your thoughts?


Shannon Deminick 31 Aug 2017, 04:45:41

@bitmapped

  1. Not sure why overriding ConfigureMiddleware is not a logical place? When you are configuring external login providers you are configuring Middleware.

The base methods of ConfigureMiddleware does quite a lot of things:

//Ensure owin is configured for Umbraco back office authentication. If you have any front-end OWIN
// cookie configuration, this must be declared after it.
app
    .UseUmbracoBackOfficeCookieAuthentication(ApplicationContext, PipelineStage.Authenticate)
    .UseUmbracoBackOfficeExternalCookieAuthentication(ApplicationContext, PipelineStage.Authenticate)
    .UseUmbracoPreviewAuthentication(ApplicationContext, PipelineStage.Authorize)
    .UseSignalR()
    .FinalizeMiddlewareConfiguration();

These are all important and need to be done in that order - and the last part FinalizeMiddlewareConfiguration is a bit of undocumented stuff ... this actually emits an event that you could subscribe to instead of swapping out your own Owin startup: UmbracoDefaultOwinStartup.MiddlewareConfigured (can't remember when this was added but definitely after 7.3). You have the option to use this event too if you wanted.

I'd prefer to not have a method called ConfigureExternalLogins which is very specific. If you really want to have a method to override that is not ConfigureMiddleware then it should be more like ConfigureAuthentication and you could either:

Override that and ensure you call base.ConfigureAuthentication first since it would then make sense to put the Core's authentication middleware setup in that method which might actually be handy anyways so that there is only one method to call to configure all of Umbraco's normal authentication middlware OR

we have this as an empty method (probably called ConfigureCustomAuthentication) which executes before UseSignalR

...I'd prefer option 1 if we go down this path

  1. Yes, UmbracoIdentityExtensions will need to be released as a new major version when it's code snippets are updated to support the new cross cutting methods.


Brian Powell 31 Aug 2017, 06:00:32

@Shandem I'm sorry about the confusion with ConfigureMiddleware versus ConfigureServices. I've been poking around with ASP.NET Core 2.0 and it uses ConfigureServices for OAuth config.

Today was the first time I configured OAuth with Umbraco. I banged my head against the wall getting things to work because I had the config out of order, so I tried to look at dummyproofing the OAuth config process to make it easy for users. If things are so sensitive that they have to be in just the right order for it all to work, it seems to me that it's worth considering a placeholder method like I proposed in my pull request. It eliminates the chance that a user will call methods out of order, like I did, or fail to call something, like some commenters in U4-9081 did.

I can understand your concern about adding another method to the codebase. You're the core developer. It's your call.


Shannon Deminick 31 Aug 2017, 06:16:16

I like the option #1 I listed above. This means Core's ConfigureMiddleware methods would look like:

ConfigureUmbracoAuthentication(app, ApplicationContext);
app.UseSignalR()
   .FinalizeMiddlewareConfiguration();

and the ConfigureUmbracoAuthentication would be:

app
    .UseUmbracoBackOfficeCookieAuthentication(ApplicationContext, PipelineStage.Authenticate)
    .UseUmbracoBackOfficeExternalCookieAuthentication(ApplicationContext, PipelineStage.Authenticate)
    .UseUmbracoPreviewAuthentication(ApplicationContext, PipelineStage.Authorize);

Then you can override ConfigureUmbracoAuthentication (and ensure you call the base.ConfigureUmbracoAuthentication first)

Does that work for you (and make sense)? If so can you update your PR that way?


Brian Powell 04 Sep 2017, 12:56:48

@Shandem The changes make sense. I've been traveling this weekend (it's Labor Day weekend here in the US). I'll submit an updated PR tomorrow.


Brian Powell 06 Sep 2017, 04:47:31

@Shandem I've updated the pull request. https://github.com/umbraco/Umbraco-CMS/pull/2162

If this looks good, I'll work on updating UmbracoIdentity.


Shannon Deminick 09 Oct 2017, 04:51:00

Looks good, have merged in :)


Priority: Normal

Type: Bug

State: Fixed

Assignee:

Difficulty: Normal

Category:

Backwards Compatible: True

Fix Submitted:

Affected versions:

Due in version: 7.7.3, 7.6.10

Sprint: Sprint 69

Story Points: 0.5

Cycle: 5