From 1117440a04a0a90fc59c3af3e649acedbcbbcac6 Mon Sep 17 00:00:00 2001 From: Rick Butterfield Date: Sat, 6 Nov 2021 11:40:05 +0000 Subject: [PATCH 1/6] Fix for #11591 --- src/Umbraco.Web.BackOffice/Security/BackOfficeSignInManager.cs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/Umbraco.Web.BackOffice/Security/BackOfficeSignInManager.cs b/src/Umbraco.Web.BackOffice/Security/BackOfficeSignInManager.cs index 4b970e4b72..c043cf73d2 100644 --- a/src/Umbraco.Web.BackOffice/Security/BackOfficeSignInManager.cs +++ b/src/Umbraco.Web.BackOffice/Security/BackOfficeSignInManager.cs @@ -78,6 +78,7 @@ namespace Umbraco.Cms.Web.BackOffice.Security if (shouldSignIn == false) { Logger.LogWarning("The AutoLinkOptions of the external authentication provider '{LoginProvider}' have refused the login based on the OnExternalLogin method. Affected user id: '{UserId}'", loginInfo.LoginProvider, user.Id); + return SignInResult.NotAllowed; } } From b00e686936e47160fd8a56d2a0f4f9f64771c844 Mon Sep 17 00:00:00 2001 From: Rick Butterfield Date: Mon, 8 Nov 2021 12:44:01 +0000 Subject: [PATCH 2/6] Fixes #11591 --- .../Security/BackOfficeSignInManager.cs | 22 ++++++++++++++++++- 1 file changed, 21 insertions(+), 1 deletion(-) diff --git a/src/Umbraco.Web.BackOffice/Security/BackOfficeSignInManager.cs b/src/Umbraco.Web.BackOffice/Security/BackOfficeSignInManager.cs index c043cf73d2..96c98d80de 100644 --- a/src/Umbraco.Web.BackOffice/Security/BackOfficeSignInManager.cs +++ b/src/Umbraco.Web.BackOffice/Security/BackOfficeSignInManager.cs @@ -193,6 +193,17 @@ namespace Umbraco.Cms.Web.BackOffice.Security return AutoLinkSignInResult.FailedException(ex.Message); } + var shouldSignIn = autoLinkOptions.OnExternalLogin(autoLinkUser, loginInfo); + if (shouldSignIn == false) + { + Logger.LogWarning("The AutoLinkOptions of the external authentication provider '{LoginProvider}' have refused the login based on the OnExternalLogin method. Affected user id: '{UserId}'", loginInfo.LoginProvider, autoLinkUser.Id); + return SignInResult.NotAllowed; + } + else + { + return await LinkUser(autoLinkUser, loginInfo); + } + return await LinkUser(autoLinkUser, loginInfo); } else @@ -226,7 +237,16 @@ namespace Umbraco.Cms.Web.BackOffice.Security } else { - return await LinkUser(autoLinkUser, loginInfo); + var shouldSignIn = autoLinkOptions.OnExternalLogin(autoLinkUser, loginInfo); + if (shouldSignIn == false) + { + Logger.LogWarning("The AutoLinkOptions of the external authentication provider '{LoginProvider}' have refused the login based on the OnExternalLogin method. Affected user id: '{UserId}'", loginInfo.LoginProvider, autoLinkUser.Id); + return SignInResult.NotAllowed; + } + else + { + return await LinkUser(autoLinkUser, loginInfo); + } } } } From 709c2498fec80496eee1f6272a1adefdd2e366d6 Mon Sep 17 00:00:00 2001 From: Rick Butterfield Date: Mon, 8 Nov 2021 12:46:04 +0000 Subject: [PATCH 3/6] Remove unreachable code --- src/Umbraco.Web.BackOffice/Security/BackOfficeSignInManager.cs | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/Umbraco.Web.BackOffice/Security/BackOfficeSignInManager.cs b/src/Umbraco.Web.BackOffice/Security/BackOfficeSignInManager.cs index 96c98d80de..11f77a34e9 100644 --- a/src/Umbraco.Web.BackOffice/Security/BackOfficeSignInManager.cs +++ b/src/Umbraco.Web.BackOffice/Security/BackOfficeSignInManager.cs @@ -203,8 +203,6 @@ namespace Umbraco.Cms.Web.BackOffice.Security { return await LinkUser(autoLinkUser, loginInfo); } - - return await LinkUser(autoLinkUser, loginInfo); } else { From 3d438159816ad157083f640030689809e54fcfb9 Mon Sep 17 00:00:00 2001 From: Andy Butland Date: Mon, 15 Nov 2021 11:17:47 +0100 Subject: [PATCH 4/6] Added suggestions from code review. --- .../Controllers/BackOfficeController.cs | 5 +++++ .../Security/BackOfficeSignInManager.cs | 15 +++++++++------ .../Security/ExternalLoginSignInResult.cs | 15 +++++++++++++++ 3 files changed, 29 insertions(+), 6 deletions(-) create mode 100644 src/Umbraco.Web.BackOffice/Security/ExternalLoginSignInResult.cs diff --git a/src/Umbraco.Web.BackOffice/Controllers/BackOfficeController.cs b/src/Umbraco.Web.BackOffice/Controllers/BackOfficeController.cs index b6df2ebf4a..aa5fc83e1e 100644 --- a/src/Umbraco.Web.BackOffice/Controllers/BackOfficeController.cs +++ b/src/Umbraco.Web.BackOffice/Controllers/BackOfficeController.cs @@ -517,6 +517,11 @@ namespace Umbraco.Cms.Web.BackOffice.Controllers // Failed only occurs when the user does not exist errors.Add("The requested provider (" + loginInfo.LoginProvider + ") has not been linked to an account, the provider must be linked from the back office."); } + else if (result == ExternalLoginSignInResult.NotAllowed) + { + // This occurs when the external provider has approved the login but custom logic in OnExternalLogin has denined it. + errors.Add($"The user {loginInfo.Principal.Identity.Name} for the external provider {loginInfo.ProviderDisplayName} has not been accepted and cannot sign in."); + } else if (result == AutoLinkSignInResult.FailedNotLinked) { errors.Add("The requested provider (" + loginInfo.LoginProvider + ") has not been linked to an account, the provider must be linked from the back office."); diff --git a/src/Umbraco.Web.BackOffice/Security/BackOfficeSignInManager.cs b/src/Umbraco.Web.BackOffice/Security/BackOfficeSignInManager.cs index 11f77a34e9..4426e07e22 100644 --- a/src/Umbraco.Web.BackOffice/Security/BackOfficeSignInManager.cs +++ b/src/Umbraco.Web.BackOffice/Security/BackOfficeSignInManager.cs @@ -77,8 +77,8 @@ namespace Umbraco.Cms.Web.BackOffice.Security var shouldSignIn = autoLinkOptions.OnExternalLogin(user, loginInfo); if (shouldSignIn == false) { - Logger.LogWarning("The AutoLinkOptions of the external authentication provider '{LoginProvider}' have refused the login based on the OnExternalLogin method. Affected user id: '{UserId}'", loginInfo.LoginProvider, user.Id); - return SignInResult.NotAllowed; + LogFailedExternalLogin(loginInfo, user); + return ExternalLoginSignInResult.NotAllowed; } } @@ -196,8 +196,8 @@ namespace Umbraco.Cms.Web.BackOffice.Security var shouldSignIn = autoLinkOptions.OnExternalLogin(autoLinkUser, loginInfo); if (shouldSignIn == false) { - Logger.LogWarning("The AutoLinkOptions of the external authentication provider '{LoginProvider}' have refused the login based on the OnExternalLogin method. Affected user id: '{UserId}'", loginInfo.LoginProvider, autoLinkUser.Id); - return SignInResult.NotAllowed; + LogFailedExternalLogin(loginInfo, autoLinkUser); + return ExternalLoginSignInResult.NotAllowed; } else { @@ -238,8 +238,8 @@ namespace Umbraco.Cms.Web.BackOffice.Security var shouldSignIn = autoLinkOptions.OnExternalLogin(autoLinkUser, loginInfo); if (shouldSignIn == false) { - Logger.LogWarning("The AutoLinkOptions of the external authentication provider '{LoginProvider}' have refused the login based on the OnExternalLogin method. Affected user id: '{UserId}'", loginInfo.LoginProvider, autoLinkUser.Id); - return SignInResult.NotAllowed; + LogFailedExternalLogin(loginInfo, autoLinkUser); + return ExternalLoginSignInResult.NotAllowed; } else { @@ -283,5 +283,8 @@ namespace Umbraco.Cms.Web.BackOffice.Security return AutoLinkSignInResult.FailedLinkingUser(errors); } } + + private void LogFailedExternalLogin(ExternalLoginInfo loginInfo, BackOfficeIdentityUser user) => + Logger.LogWarning("The AutoLinkOptions of the external authentication provider '{LoginProvider}' have refused the login based on the OnExternalLogin method. Affected user id: '{UserId}'", loginInfo.LoginProvider, user.Id); } } diff --git a/src/Umbraco.Web.BackOffice/Security/ExternalLoginSignInResult.cs b/src/Umbraco.Web.BackOffice/Security/ExternalLoginSignInResult.cs new file mode 100644 index 0000000000..188961b2ac --- /dev/null +++ b/src/Umbraco.Web.BackOffice/Security/ExternalLoginSignInResult.cs @@ -0,0 +1,15 @@ +using Microsoft.AspNetCore.Identity; + +namespace Umbraco.Cms.Web.BackOffice.Security +{ + /// + /// Result returned from signing in when external logins are used. + /// + public class ExternalLoginSignInResult : SignInResult + { + public static ExternalLoginSignInResult NotAllowed { get; } = new ExternalLoginSignInResult() + { + Succeeded = false + }; + } +} From c6176a0ae700d9075fbf9088b18d29f133ba1df5 Mon Sep 17 00:00:00 2001 From: Andy Butland Date: Mon, 15 Nov 2021 11:42:04 +0100 Subject: [PATCH 5/6] Added null check for external login invocation when linking users. --- .../Security/BackOfficeSignInManager.cs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/Umbraco.Web.BackOffice/Security/BackOfficeSignInManager.cs b/src/Umbraco.Web.BackOffice/Security/BackOfficeSignInManager.cs index 4426e07e22..e32a556c1b 100644 --- a/src/Umbraco.Web.BackOffice/Security/BackOfficeSignInManager.cs +++ b/src/Umbraco.Web.BackOffice/Security/BackOfficeSignInManager.cs @@ -193,8 +193,8 @@ namespace Umbraco.Cms.Web.BackOffice.Security return AutoLinkSignInResult.FailedException(ex.Message); } - var shouldSignIn = autoLinkOptions.OnExternalLogin(autoLinkUser, loginInfo); - if (shouldSignIn == false) + var shouldLinkUser = autoLinkOptions.OnExternalLogin == null || autoLinkOptions.OnExternalLogin(autoLinkUser, loginInfo); + if (shouldLinkUser == false) { LogFailedExternalLogin(loginInfo, autoLinkUser); return ExternalLoginSignInResult.NotAllowed; @@ -235,8 +235,8 @@ namespace Umbraco.Cms.Web.BackOffice.Security } else { - var shouldSignIn = autoLinkOptions.OnExternalLogin(autoLinkUser, loginInfo); - if (shouldSignIn == false) + var shouldLinkUser = autoLinkOptions.OnExternalLogin == null || autoLinkOptions.OnExternalLogin(autoLinkUser, loginInfo); + if (shouldLinkUser == false) { LogFailedExternalLogin(loginInfo, autoLinkUser); return ExternalLoginSignInResult.NotAllowed; From fe396ba0cf5c51ea27b5ce145e62f5537c1bed8d Mon Sep 17 00:00:00 2001 From: Andy Butland Date: Mon, 15 Nov 2021 11:45:00 +0100 Subject: [PATCH 6/6] Inverted conditional on linking user for clarity. --- .../Security/BackOfficeSignInManager.cs | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/src/Umbraco.Web.BackOffice/Security/BackOfficeSignInManager.cs b/src/Umbraco.Web.BackOffice/Security/BackOfficeSignInManager.cs index e32a556c1b..ec0d273b56 100644 --- a/src/Umbraco.Web.BackOffice/Security/BackOfficeSignInManager.cs +++ b/src/Umbraco.Web.BackOffice/Security/BackOfficeSignInManager.cs @@ -194,14 +194,14 @@ namespace Umbraco.Cms.Web.BackOffice.Security } var shouldLinkUser = autoLinkOptions.OnExternalLogin == null || autoLinkOptions.OnExternalLogin(autoLinkUser, loginInfo); - if (shouldLinkUser == false) + if (shouldLinkUser) { - LogFailedExternalLogin(loginInfo, autoLinkUser); - return ExternalLoginSignInResult.NotAllowed; + return await LinkUser(autoLinkUser, loginInfo); } else { - return await LinkUser(autoLinkUser, loginInfo); + LogFailedExternalLogin(loginInfo, autoLinkUser); + return ExternalLoginSignInResult.NotAllowed; } } else @@ -236,14 +236,14 @@ namespace Umbraco.Cms.Web.BackOffice.Security else { var shouldLinkUser = autoLinkOptions.OnExternalLogin == null || autoLinkOptions.OnExternalLogin(autoLinkUser, loginInfo); - if (shouldLinkUser == false) + if (shouldLinkUser) { - LogFailedExternalLogin(loginInfo, autoLinkUser); - return ExternalLoginSignInResult.NotAllowed; + return await LinkUser(autoLinkUser, loginInfo); } else { - return await LinkUser(autoLinkUser, loginInfo); + LogFailedExternalLogin(loginInfo, autoLinkUser); + return ExternalLoginSignInResult.NotAllowed; } } }