Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Back office user updates to support OAuth and 8.9 features #9470

Merged
merged 23 commits into from
Dec 3, 2020

Conversation

Shazwazza
Copy link
Contributor

@Shazwazza Shazwazza commented Nov 30, 2020

Once we merged 8.9 up, we then have to reimplement most things because it's all changed in aspnetcore.

So this re-implements OAuth along with the 8.9 features that didn't automatically merge among other things:

  • Update gulp build to not build to 2x different places, so it's faster and no longer outputs to the old web project, also fixes watching files (i think)
  • Introduce interface IBackOfficeSignInManager
  • Don't rely on IBackOfficeSecurity unnecessarily especially when Authentication is not yet guaranteed like in IsAuthenticated, PostLogout and GetRemainingTimeoutSeconds, instead rely on the standard aspnetcore authentication and ClaimsPrincipal methods.
  • Remove unnecessary methods of IBackOfficeSecurity and add TODOs about how we can reduce it more and get rid of it
  • Smiplifies AuthenticationController & fixes AuthN for unathorized endpoints
  • Simplifies BackOfficeController & fixes AuthN for unathorized endpoints
    • Fix Xsrf handling with BackOfficeSignInManager
    • Move most of the external login support to the BackOfficeSignInManager including auto linking
  • Introduce IBackOfficeTwoFactorOptions and the default NoopBackOfficeTwoFactorOptions to allow for end users to be able to supply an angular view to show the 2FA dialog to the back office user (just like in v8). This is simpler than in v8 where the interface was required to be implemented by the user manager, now it's just it's own service that can be replaced.
  • Do not implement IUserTwoFactorStore because we don't support that store OOTB, just like in v8. For 2FA support (just like v8) it will require the user to replace our store, implement the required IUserTwoFactorStore, the DB changes necessary and override the UpdateAsync method to persist it all properly. It will be slightly easier to replace in netcore than in v8 due to better DI support and no OWIN and also not requiring replacing the user manager, but will still require most of the principles and code found in this project https://github.com/Offroadcode/Umbraco-2FA. Some time in the future if required the CMS codebase can fully implement 2FA which now in aspnetcore requires a lot more implementation logic, for now it will be entirely up to the end user (just like v8)
  • Ports user manager auditing events to netcore
  • Don't specify a default (global) authentication scheme, just add our own and ensure we are always using our own for the back office
  • Adds ability to configure external login providers for the back office as part of the IUmbracoBuilder.
    • This must be part of the IUmbracoBuilder phase because we need to associate our own metadata for external login providers (BackOfficeExternalLoginProvider) with identity's external login provider configuration. There is no built in way to do this so we need to wrap those calls and use our own BackOfficeAuthenticationBuilder instance to do the association of BackOfficeExternalLoginProviderOptions with the corresponding external login Scheme which registers a BackOfficeExternalLoginProvider to be resolved later. Then ensure that it's OAuth options are always configured correctly for the back office (SignInScheme)
  • Moves BackOfficeUserManager to the back office netcore project since we need access to netcore services like IHttpContextAccessor. (it could be in the common project but not sure, it's back office specific, i guess we need to agree on a standard approach of what exists in common vs back office)

Testing

In the UI.NetCore project you can install these packages for testing:

  • Microsoft.AspNetCore.Authentication.Google
  • Microsoft.Identity.Web
  • Microsoft.AspNetCore.Authentication.OpenIdConnect

In Startup your ConfigureServices could look like the below. This enables a Google and Azure AD provider. Ensure you fill in your own details. You can basically follow these instructions for Azure Ad https://shazwazza.com/post/configuring-azure-active-directory-login-with-umbraco/ EXCEPT that the redirect url must be what is below: /umbraco-signin-oidc instead of /umbraco. For Google you just follow this https://developers.google.com/identity/protocols/oauth2 and use the below redirect path: /umbraco-google-signin

Then you can play around with those options, like: denyLocalLogin which will mean it won't let you login locally, autoRedirectLoginToExternalProvider which will auto redirect to that provider when you hit the login page, specifying ExternalSignInAutoLinkOptions(true) (true) to enable auto-linking which will automatically create a local user and link them, you can breakpoint in the callbacks to make sure they are working. I've tested all this and it works on my machine.

NOTE: the weirdness with the Azure Ad stuff, that is because the package Microsoft.Identity.Web tries to do too much and there's no way around that. It will try to AddCookie authentication when we already have it. It's pretty silly and I have a GH issue to open about that, for now the work around is to use a "Fake" scheme which will just add a fake cookie auth scheme that isn't used since we redirect this using options to the back office scheme.

public void ConfigureServices(IServiceCollection services)
{
    services.AddUmbraco(_env, _config)
        .AddAllBackOfficeComponents()
        .AddUmbracoWebsite()
        .AddBackOfficeExternalLogins(logins =>
        {
            var loginProviderOptions = new BackOfficeExternalLoginProviderOptions(
                "btn-google", "fa-google",
                new ExternalSignInAutoLinkOptions(true)
                {
                    OnAutoLinking = (user, login) =>
                    {

                    },
                    OnExternalLogin = (user, login) =>
                    {
                        return true;
                    }
                },
                denyLocalLogin: false,
                autoRedirectLoginToExternalProvider: false);

            logins.AddBackOfficeLogin(
                loginProviderOptions,
                auth =>
                {
                    auth.AddGoogle(
                        // The scheme must be set with this method to work for the back office
                        auth.SchemeForBackOffice("Google"),
                        options =>
                        {
                            //  By default this is '/signin-google' but it needs to be changed to this
                            options.CallbackPath = "/umbraco-google-signin";
                            options.ClientId = "YOURCLIENTID";
                            options.ClientSecret = "YOURCLIENTSECRET";
                        });

                    // NOTE: Adding additional providers here is possible via the API but
                    // it will mean that the same BackOfficeExternalLoginProviderOptions will be registered
                    // for them. In some weird cases maybe people would want that?
                });

            logins.AddBackOfficeLogin(
                new BackOfficeExternalLoginProviderOptions("btn-microsoft", "fa-windows"),
                auth =>
                {
                    auth.AddMicrosoftIdentityWebApp(options =>
                    {
                        //  By default this is '/signin-oidc' but it needs to be changed to this
                        options.CallbackPath = "/umbraco-signin-oidc";
                        options.Instance = "https://login.microsoftonline.com/";
                        options.TenantId = "YOURTENANTID";
                        options.ClientId = "YOURCLIENTID";
                        options.ClientSecret = "YOURCLIENTSECRET";
                    },
                    // The scheme must be set with this method to work for the back office
                    openIdConnectScheme: auth.SchemeForBackOffice("AzureAD"),
                    // This is unfortunate but required, I have a follow up report to send the MS team about this provider
                    cookieScheme: "Fake");
                });
        })
        .Build();

}

New tasks

  • Deal with UpdateExternalAuthenticationTokensAsync in BackOfficeController.ExternalLinkLoginCallback to persist external login information. This is built into aspnetcore identity and it wasn't before. Things like UmbracoId need to hack around this but now that it's built in we should automatically handle it. See TODO.
  • Kill OverrideAuthorization, this requires either: handling authorization in controllers that have this differently. Either: more restrictive authz at the action level, lesser at the controller, but this means being very careful about any new actions added, OR new controllers for these lesser restrictive authz and updating angular, etc...
    • Split ContentTypeController & MediaTypeController apart, see TODO
  • Remove UmbracoBackOfficeIdentity entirely and rely only on ClaimsIdentity, see TODO (this is optional but would be very helpful and adhere to best practices moving forward)
  • Move aspnetcore back office stuff that doesn't need to exist in common to the back office, see TODOs

bergmania and others added 7 commits November 27, 2020 13:33
…ice login providers""

Signed-off-by: Bjarke Berg <mail@bergmania.dk>
…ers and debug why the styles aren't working""

Signed-off-by: Bjarke Berg <mail@bergmania.dk>
…er""

Signed-off-by: Bjarke Berg <mail@bergmania.dk>
…cated under the back office scheme""

Signed-off-by: Bjarke Berg <mail@bergmania.dk>
…Controller for endpoints that aren't authorized (and simplifies)""

Signed-off-by: Bjarke Berg <mail@bergmania.dk>
Signed-off-by: Bjarke Berg <mail@bergmania.dk>
@Shazwazza Shazwazza changed the title Back office user updates to support OAuth and 8.9 features WIP - Back office user updates to support OAuth and 8.9 features Nov 30, 2020
@Shazwazza Shazwazza changed the title WIP - Back office user updates to support OAuth and 8.9 features Back office user updates to support OAuth and 8.9 features Dec 2, 2020
/// </summary>
/// <param name="throwExceptions">set to true if you want exceptions to be thrown if failed</param>
/// <returns></returns>
ValidateRequestAttempt AuthorizeRequest(bool throwExceptions = false);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No longer needed, the more we can remove from this the better

…ature/2FAuth

# Conflicts:
#	src/Umbraco.Tests.Integration/TestServerTest/TestAuthHandler.cs
#	src/Umbraco.Web.BackOffice/Controllers/AuthenticationController.cs
#	src/Umbraco.Web.BackOffice/Controllers/BackOfficeController.cs
#	src/Umbraco.Web.BackOffice/Controllers/ContentController.cs
#	src/Umbraco.Web.BackOffice/Controllers/ContentTypeController.cs
#	src/Umbraco.Web.BackOffice/Controllers/MediaTypeController.cs
#	src/Umbraco.Web.BackOffice/Controllers/UsersController.cs
#	src/Umbraco.Web.BackOffice/Filters/DenyLocalLoginAuthorizationAttribute.cs
#	src/Umbraco.Web.BackOffice/Filters/OverrideAuthorizationFilterProvider.cs
#	src/Umbraco.Web.Common/Filters/UmbracoBackOfficeAuthorizeAttribute.cs
#	src/Umbraco.Web.Common/Filters/UmbracoBackOfficeAuthorizeFilter.cs
…ith the fix of returning the task instance from within MoveViewsAndRegenerateJS
@Shazwazza
Copy link
Contributor Author

This was reviewed and approved by @bergmania who's authorized me to merge :P

@Shazwazza Shazwazza merged commit 7a71995 into netcore/netcore Dec 3, 2020
@Shazwazza Shazwazza deleted the netcore/feature/2FAuth branch December 3, 2020 07:54
@bergmania
Copy link
Member

For the record, I reviewed this, but we discussed over zoom :)

@p-m-j
Copy link
Contributor

p-m-j commented Dec 3, 2020

By default this is '/signin-google' but it needs to be changed to this
options.CallbackPath = "/umbraco-google-signin";

Interesting what was wrong with the default?

cookieScheme: "Fake"

Something sounds off here, shouldn't each auth provider have their own cookie used for the authentication flow?
My memory may be warped by how IdentityServer4 is configured vs Identity

@Shazwazza
Copy link
Contributor Author

Interesting what was wrong with the default?

Nothing, it's just that you can have one for the front end members and one for the back end users, they cannot share the same path because they have different configurations. By convention - though it's not a rule is to prefix the path with "umbraco-". This could be anything you want, just so long as it doesn't overlap with another.

Something sounds off here, shouldn't each auth provider have their own cookie used for the authentication flow?
My memory may be warped by how IdentityServer4 is configured vs Identity

What is off here is that this library https://github.com/AzureAD/microsoft-identity-web/ tries to do way to much. It automatically tries to configure all of your auth including your default cookie auth. I have an issue to raise and/or comment on but it's also related to

There should be an option to not configure any cookie authentication, just like normal OpenIdConnect, Google, etc... providers. This library is trying to be too 'simple' and just do everything for you.

See

Sure we could avoid this hack but it would mean duplicating almost all of their code here https://github.com/AzureAD/microsoft-identity-web/blob/b9de3cc12a58864f24e03538279d5577185e6788/src/Microsoft.Identity.Web/WebAppExtensions/MicrosoftIdentityWebAppAuthenticationBuilderExtensions.cs#L255

For this test, not specifying anything for the "cookieScheme" would work fine, we set the SignInScheme to be the umbraco back office one here https://github.com/umbraco/Umbraco-CMS/pull/9470/files#diff-655b7862f1cff88520b0766af2912c6eadcf0448565c839404f907a6e58fe0aeR59 , however if you had normal cookie authentication enabled for your own front end then their method would throw an exception since it's trying to configure 2x the same cookie auth. Hence this work around.

@p-m-j
Copy link
Contributor

p-m-j commented Dec 3, 2020

heh I don't think microsoft-identity-web existed last time I integrated azure ad, guessing it would have been MSAL, sorry for your pain

@thomashdk
Copy link
Contributor

What am I missing here ?

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants