From 0fbe01cd22ea498084068ae607f46ce3195e5640 Mon Sep 17 00:00:00 2001 From: Andy Butland Date: Wed, 18 Nov 2020 16:12:42 +0100 Subject: [PATCH 1/5] Created new abstraction IUmbracoWebsiteSecurity and migrated controller's using it (replacing MembershipHelper) to Umbraco.Web.Website. --- .../Models/{ => Security}/LoginModel.cs | 4 +- .../{ => Security}/PostRedirectModel.cs | 2 +- .../Models/Security}/ProfileModel.cs | 39 +----------- .../Models/Security}/RegisterModel.cs | 21 +------ .../PublishedContentExtensions.cs | 2 - .../Security/IUmbracoWebsiteSecurity.cs | 20 ++++++ .../Security/RegisterMemberStatus.cs | 13 ++++ .../Security/UpdateMemberProfileStatus.cs | 8 +++ .../Controllers/AuthenticationController.cs | 10 +-- .../Controllers/SurfaceController.cs | 26 ++++---- .../Controllers/UmbLoginController.cs | 42 ++++++------- .../Controllers/UmbLoginStatusController.cs | 37 +++++------ .../Controllers/UmbProfileController.cs | 63 +++++++++++++++++++ .../Controllers/UmbRegisterController.cs | 59 +++++++---------- ...racoWebstiteServiceCollectionExtensions.cs | 5 ++ .../Security/UmbracoWebsiteSecurity.cs | 33 ++++++++++ .../Umbraco.Web.Website.csproj | 2 +- .../Controllers/UmbProfileController.cs | 61 ------------------ src/Umbraco.Web/Security/MembershipHelper.cs | 13 ++-- src/Umbraco.Web/Umbraco.Web.csproj | 8 +-- 20 files changed, 235 insertions(+), 233 deletions(-) rename src/Umbraco.Core/Models/{ => Security}/LoginModel.cs (82%) rename src/Umbraco.Core/Models/{ => Security}/PostRedirectModel.cs (92%) rename src/{Umbraco.Web/Models => Umbraco.Core/Models/Security}/ProfileModel.cs (62%) rename src/{Umbraco.Web/Models => Umbraco.Core/Models/Security}/RegisterModel.cs (79%) create mode 100644 src/Umbraco.Core/Security/IUmbracoWebsiteSecurity.cs create mode 100644 src/Umbraco.Core/Security/RegisterMemberStatus.cs create mode 100644 src/Umbraco.Core/Security/UpdateMemberProfileStatus.cs rename src/{Umbraco.Web => Umbraco.Web.Website}/Controllers/UmbLoginController.cs (52%) rename src/{Umbraco.Web => Umbraco.Web.Website}/Controllers/UmbLoginStatusController.cs (54%) create mode 100644 src/Umbraco.Web.Website/Controllers/UmbProfileController.cs rename src/{Umbraco.Web => Umbraco.Web.Website}/Controllers/UmbRegisterController.cs (60%) create mode 100644 src/Umbraco.Web.Website/Security/UmbracoWebsiteSecurity.cs delete mode 100644 src/Umbraco.Web/Controllers/UmbProfileController.cs diff --git a/src/Umbraco.Core/Models/LoginModel.cs b/src/Umbraco.Core/Models/Security/LoginModel.cs similarity index 82% rename from src/Umbraco.Core/Models/LoginModel.cs rename to src/Umbraco.Core/Models/Security/LoginModel.cs index f44b274ffba4..98c9d23cff39 100644 --- a/src/Umbraco.Core/Models/LoginModel.cs +++ b/src/Umbraco.Core/Models/Security/LoginModel.cs @@ -1,7 +1,7 @@ using System.ComponentModel.DataAnnotations; using System.Runtime.Serialization; -namespace Umbraco.Web.Models +namespace Umbraco.Core.Models.Security { public class LoginModel : PostRedirectModel { @@ -11,7 +11,7 @@ public class LoginModel : PostRedirectModel [Required] [DataMember(Name = "password", IsRequired = true)] - [StringLength(maximumLength:256)] + [StringLength(maximumLength: 256)] public string Password { get; set; } } diff --git a/src/Umbraco.Core/Models/PostRedirectModel.cs b/src/Umbraco.Core/Models/Security/PostRedirectModel.cs similarity index 92% rename from src/Umbraco.Core/Models/PostRedirectModel.cs rename to src/Umbraco.Core/Models/Security/PostRedirectModel.cs index e04584704d64..3a87cdcbe5a5 100644 --- a/src/Umbraco.Core/Models/PostRedirectModel.cs +++ b/src/Umbraco.Core/Models/Security/PostRedirectModel.cs @@ -1,4 +1,4 @@ -namespace Umbraco.Web.Models +namespace Umbraco.Core.Models.Security { /// /// A base model containing a value to indicate to Umbraco where to redirect to after Posting if diff --git a/src/Umbraco.Web/Models/ProfileModel.cs b/src/Umbraco.Core/Models/Security/ProfileModel.cs similarity index 62% rename from src/Umbraco.Web/Models/ProfileModel.cs rename to src/Umbraco.Core/Models/Security/ProfileModel.cs index 0397f3484999..8493a5f5a9a4 100644 --- a/src/Umbraco.Web/Models/ProfileModel.cs +++ b/src/Umbraco.Core/Models/Security/ProfileModel.cs @@ -2,36 +2,15 @@ using System.Collections.Generic; using System.ComponentModel; using System.ComponentModel.DataAnnotations; -using System.Web.Mvc; -using Current = Umbraco.Web.Composing.Current; +using Umbraco.Web.Models; -namespace Umbraco.Web.Models +namespace Umbraco.Core.Models.Security { /// /// A readonly member profile model /// - [ModelBinder(typeof(ProfileModelBinder))] public class ProfileModel : PostRedirectModel { - - public static ProfileModel CreateModel() - { - var model = new ProfileModel(false); - return model; - } - - private ProfileModel(bool doLookup) - { - MemberProperties = new List(); - if (doLookup && Current.UmbracoContext != null) - { - var helper = Current.MembershipHelper; - var model = helper.GetCurrentMemberProfileModel(); - MemberProperties = model.MemberProperties; - } - } - - [Required] [RegularExpression(@"[a-z0-9!#$%&'*+/=?^_`{|}~-]+(?:\.[a-z0-9!#$%&'*+/=?^_`{|}~-]+)*@(?:[a-z0-9](?:[a-z0-9-]*[a-z0-9])?\.)+[a-z0-9](?:[a-z0-9-]*[a-z0-9])?", ErrorMessage = "Please enter a valid e-mail address")] @@ -81,18 +60,6 @@ private ProfileModel(bool doLookup) /// /// Adding items to this list on the front-end will not add properties to the member in the database. /// - public List MemberProperties { get; set; } - - /// - /// A custom model binder for MVC because the default ctor performs a lookup! - /// - internal class ProfileModelBinder : DefaultModelBinder - { - protected override object CreateModel(ControllerContext controllerContext, ModelBindingContext bindingContext, Type modelType) - { - return ProfileModel.CreateModel(); - } - - } + public List MemberProperties { get; set; } = new List(); } } diff --git a/src/Umbraco.Web/Models/RegisterModel.cs b/src/Umbraco.Core/Models/Security/RegisterModel.cs similarity index 79% rename from src/Umbraco.Web/Models/RegisterModel.cs rename to src/Umbraco.Core/Models/Security/RegisterModel.cs index 1ee9307969eb..fca749703de9 100644 --- a/src/Umbraco.Web/Models/RegisterModel.cs +++ b/src/Umbraco.Core/Models/Security/RegisterModel.cs @@ -1,12 +1,9 @@ -using System; -using System.Collections.Generic; +using System.Collections.Generic; using System.ComponentModel.DataAnnotations; -using System.Web.Mvc; -using Umbraco.Core; +using Umbraco.Web.Models; -namespace Umbraco.Web.Models +namespace Umbraco.Core.Models.Security { - [ModelBinder(typeof(RegisterModelBinder))] public class RegisterModel : PostRedirectModel { /// @@ -27,7 +24,6 @@ private RegisterModel() CreatePersistentLoginCookie = true; } - [Required] [RegularExpression(@"[A-Za-z0-9!#$%&'*+/=?^_`{|}~-]+(?:\.[A-Za-z0-9!#$%&'*+/=?^_`{|}~-]+)*@(?:[A-Za-z0-9](?:[A-Za-z0-9-]*[A-Za-z0-9])?\.)+[A-Za-z0-9](?:[A-Za-z0-9-]*[A-Za-z0-9])?", ErrorMessage = "Please enter a valid e-mail address")] @@ -74,16 +70,5 @@ private RegisterModel() /// Default is true to create a persistent cookie if LoginOnSuccess is true /// public bool CreatePersistentLoginCookie { get; set; } - - /// - /// A custom model binder for MVC because the default ctor performs a lookup! - /// - internal class RegisterModelBinder : DefaultModelBinder - { - protected override object CreateModel(ControllerContext controllerContext, ModelBindingContext bindingContext, Type modelType) - { - return RegisterModel.CreateModel(); - } - } } } diff --git a/src/Umbraco.Core/PublishedContentExtensions.cs b/src/Umbraco.Core/PublishedContentExtensions.cs index 1b7c460c8ba6..fd353f95cd9c 100644 --- a/src/Umbraco.Core/PublishedContentExtensions.cs +++ b/src/Umbraco.Core/PublishedContentExtensions.cs @@ -2,10 +2,8 @@ using System.Collections.Generic; using System.Linq; using Umbraco.Core.Configuration.Models; -using Umbraco.Core.Configuration.UmbracoSettings; using Umbraco.Core.Models.PublishedContent; using Umbraco.Core.Services; -using Umbraco.Web; using Umbraco.Web.PublishedCache; using Umbraco.Web.Routing; diff --git a/src/Umbraco.Core/Security/IUmbracoWebsiteSecurity.cs b/src/Umbraco.Core/Security/IUmbracoWebsiteSecurity.cs new file mode 100644 index 000000000000..f14adae1746a --- /dev/null +++ b/src/Umbraco.Core/Security/IUmbracoWebsiteSecurity.cs @@ -0,0 +1,20 @@ +using Umbraco.Core.Models.Security; + +namespace Umbraco.Core.Security +{ + public interface IUmbracoWebsiteSecurity + { + // TODO: this should return the member, but in what form? MembershipUser is in place on MembershipHelper, but + // isn't appropriate for when we're using ASP.NET Identity. + void RegisterMember(RegisterModel model, out RegisterMemberStatus status, bool logMemberIn = true); + + // TODO: again, should this return the member? + void UpdateMemberProfile(ProfileModel model, out UpdateMemberProfileStatus status, out string errorMessage); + + bool Login(string username, string password); + + bool IsLoggedIn(); + + void LogOut(); + } +} diff --git a/src/Umbraco.Core/Security/RegisterMemberStatus.cs b/src/Umbraco.Core/Security/RegisterMemberStatus.cs new file mode 100644 index 000000000000..1cbeae38d1fc --- /dev/null +++ b/src/Umbraco.Core/Security/RegisterMemberStatus.cs @@ -0,0 +1,13 @@ +namespace Umbraco.Core.Security +{ + public enum RegisterMemberStatus + { + Success, + InvalidUserName, + InvalidPassword, + InvalidEmail, + DuplicateUserName, + DuplicateEmail, + Error, + } +} diff --git a/src/Umbraco.Core/Security/UpdateMemberProfileStatus.cs b/src/Umbraco.Core/Security/UpdateMemberProfileStatus.cs new file mode 100644 index 000000000000..33dfe4d4867c --- /dev/null +++ b/src/Umbraco.Core/Security/UpdateMemberProfileStatus.cs @@ -0,0 +1,8 @@ +namespace Umbraco.Core.Security +{ + public enum UpdateMemberProfileStatus + { + Success, + Error, + } +} diff --git a/src/Umbraco.Web.BackOffice/Controllers/AuthenticationController.cs b/src/Umbraco.Web.BackOffice/Controllers/AuthenticationController.cs index 4441d1b2f307..1b414279a4ab 100644 --- a/src/Umbraco.Web.BackOffice/Controllers/AuthenticationController.cs +++ b/src/Umbraco.Web.BackOffice/Controllers/AuthenticationController.cs @@ -3,17 +3,19 @@ using System.Linq; using System.Net; using System.Threading.Tasks; -using Microsoft.AspNetCore.Routing; using Microsoft.AspNetCore.Authentication; +using Microsoft.AspNetCore.Identity; using Microsoft.AspNetCore.Mvc; -using Microsoft.Extensions.Options; +using Microsoft.AspNetCore.Routing; using Microsoft.Extensions.Logging; +using Microsoft.Extensions.Options; using Umbraco.Core; using Umbraco.Core.BackOffice; using Umbraco.Core.Configuration.Models; using Umbraco.Core.Mapping; using Umbraco.Core.Models; using Umbraco.Core.Models.Membership; +using Umbraco.Core.Models.Security; using Umbraco.Core.Security; using Umbraco.Core.Services; using Umbraco.Extensions; @@ -25,12 +27,10 @@ using Umbraco.Web.Common.Exceptions; using Umbraco.Web.Common.Filters; using Umbraco.Web.Common.Security; +using Umbraco.Web.Editors.Filters; using Umbraco.Web.Models; using Umbraco.Web.Models.ContentEditing; -using Umbraco.Web.Security; using Constants = Umbraco.Core.Constants; -using Microsoft.AspNetCore.Identity; -using Umbraco.Web.Editors.Filters; namespace Umbraco.Web.BackOffice.Controllers { diff --git a/src/Umbraco.Web.Website/Controllers/SurfaceController.cs b/src/Umbraco.Web.Website/Controllers/SurfaceController.cs index 8125923ee4f7..4e0517754cab 100644 --- a/src/Umbraco.Web.Website/Controllers/SurfaceController.cs +++ b/src/Umbraco.Web.Website/Controllers/SurfaceController.cs @@ -21,7 +21,13 @@ namespace Umbraco.Web.Website.Controllers // [MergeParentContextViewData] public abstract class SurfaceController : PluginController { - private readonly IPublishedUrlProvider _publishedUrlProvider; + protected SurfaceController(IUmbracoContextAccessor umbracoContextAccessor, IUmbracoDatabaseFactory databaseFactory, ServiceContext services, AppCaches appCaches, IProfilingLogger profilingLogger, IPublishedUrlProvider publishedUrlProvider) + : base(umbracoContextAccessor, databaseFactory, services, appCaches, profilingLogger) + { + PublishedUrlProvider = publishedUrlProvider; + } + + protected IPublishedUrlProvider PublishedUrlProvider { get; } /// /// Gets the current page. @@ -39,12 +45,6 @@ protected virtual IPublishedContent CurrentPage } } - protected SurfaceController(IUmbracoContextAccessor umbracoContextAccessor, IUmbracoDatabaseFactory databaseFactory, ServiceContext services, AppCaches appCaches, IProfilingLogger profilingLogger, IPublishedUrlProvider publishedUrlProvider) - : base(umbracoContextAccessor, databaseFactory, services, appCaches, profilingLogger) - { - _publishedUrlProvider = publishedUrlProvider; - } - /// /// Redirects to the Umbraco page with the given id /// @@ -52,7 +52,7 @@ protected SurfaceController(IUmbracoContextAccessor umbracoContextAccessor, IUmb /// protected RedirectToUmbracoPageResult RedirectToUmbracoPage(Guid contentKey) { - return new RedirectToUmbracoPageResult(contentKey, _publishedUrlProvider, UmbracoContextAccessor); + return new RedirectToUmbracoPageResult(contentKey, PublishedUrlProvider, UmbracoContextAccessor); } /// @@ -63,7 +63,7 @@ protected RedirectToUmbracoPageResult RedirectToUmbracoPage(Guid contentKey) /// protected RedirectToUmbracoPageResult RedirectToUmbracoPage(Guid contentKey, QueryString queryString) { - return new RedirectToUmbracoPageResult(contentKey, queryString, _publishedUrlProvider, UmbracoContextAccessor); + return new RedirectToUmbracoPageResult(contentKey, queryString, PublishedUrlProvider, UmbracoContextAccessor); } /// @@ -73,7 +73,7 @@ protected RedirectToUmbracoPageResult RedirectToUmbracoPage(Guid contentKey, Que /// protected RedirectToUmbracoPageResult RedirectToUmbracoPage(IPublishedContent publishedContent) { - return new RedirectToUmbracoPageResult(publishedContent, _publishedUrlProvider, UmbracoContextAccessor); + return new RedirectToUmbracoPageResult(publishedContent, PublishedUrlProvider, UmbracoContextAccessor); } /// @@ -84,7 +84,7 @@ protected RedirectToUmbracoPageResult RedirectToUmbracoPage(IPublishedContent pu /// protected RedirectToUmbracoPageResult RedirectToUmbracoPage(IPublishedContent publishedContent, QueryString queryString) { - return new RedirectToUmbracoPageResult(publishedContent, queryString, _publishedUrlProvider, UmbracoContextAccessor); + return new RedirectToUmbracoPageResult(publishedContent, queryString, PublishedUrlProvider, UmbracoContextAccessor); } /// @@ -93,7 +93,7 @@ protected RedirectToUmbracoPageResult RedirectToUmbracoPage(IPublishedContent pu /// protected RedirectToUmbracoPageResult RedirectToCurrentUmbracoPage() { - return new RedirectToUmbracoPageResult(CurrentPage, _publishedUrlProvider, UmbracoContextAccessor); + return new RedirectToUmbracoPageResult(CurrentPage, PublishedUrlProvider, UmbracoContextAccessor); } /// @@ -103,7 +103,7 @@ protected RedirectToUmbracoPageResult RedirectToCurrentUmbracoPage() /// protected RedirectToUmbracoPageResult RedirectToCurrentUmbracoPage(QueryString queryString) { - return new RedirectToUmbracoPageResult(CurrentPage, queryString, _publishedUrlProvider, UmbracoContextAccessor); + return new RedirectToUmbracoPageResult(CurrentPage, queryString, PublishedUrlProvider, UmbracoContextAccessor); } /// diff --git a/src/Umbraco.Web/Controllers/UmbLoginController.cs b/src/Umbraco.Web.Website/Controllers/UmbLoginController.cs similarity index 52% rename from src/Umbraco.Web/Controllers/UmbLoginController.cs rename to src/Umbraco.Web.Website/Controllers/UmbLoginController.cs index 1f3faf7a431d..a986cef77e6c 100644 --- a/src/Umbraco.Web/Controllers/UmbLoginController.cs +++ b/src/Umbraco.Web.Website/Controllers/UmbLoginController.cs @@ -1,62 +1,58 @@ -using System.Web.Mvc; -using Umbraco.Web.Models; -using Umbraco.Web.Mvc; +using Microsoft.AspNetCore.Mvc; using Umbraco.Core; using Umbraco.Core.Cache; using Umbraco.Core.Logging; +using Umbraco.Core.Models.Security; using Umbraco.Core.Persistence; +using Umbraco.Core.Security; using Umbraco.Core.Services; -using Umbraco.Web.Security; +using Umbraco.Web.Common.Filters; +using Umbraco.Web.Routing; -namespace Umbraco.Web.Controllers +namespace Umbraco.Web.Website.Controllers { public class UmbLoginController : SurfaceController { - private readonly MembershipHelper _membershipHelper; - - public UmbLoginController() - { - } + private readonly IUmbracoWebsiteSecurity _websiteSecurity; public UmbLoginController(IUmbracoContextAccessor umbracoContextAccessor, IUmbracoDatabaseFactory databaseFactory, - ServiceContext services, AppCaches appCaches, IProfilingLogger profilingLogger, - MembershipHelper membershipHelper) - : base(umbracoContextAccessor, databaseFactory, services, appCaches, profilingLogger) + ServiceContext services, AppCaches appCaches, IProfilingLogger profilingLogger, IPublishedUrlProvider publishedUrlProvider, + IUmbracoWebsiteSecurity websiteSecurity) + : base(umbracoContextAccessor, databaseFactory, services, appCaches, profilingLogger, publishedUrlProvider) { - _membershipHelper = membershipHelper; + _websiteSecurity = websiteSecurity; } [HttpPost] [ValidateAntiForgeryToken] [ValidateUmbracoFormRouteString] - public ActionResult HandleLogin([Bind(Prefix = "loginModel")]LoginModel model) + public IActionResult HandleLogin([Bind(Prefix = "loginModel")]LoginModel model) { if (ModelState.IsValid == false) { return CurrentUmbracoPage(); } - if (_membershipHelper.Login(model.Username, model.Password) == false) + if (_websiteSecurity.Login(model.Username, model.Password) == false) { - //don't add a field level error, just model level + // Don't add a field level error, just model level. ModelState.AddModelError("loginModel", "Invalid username or password"); return CurrentUmbracoPage(); } TempData["LoginSuccess"] = true; - //if there is a specified path to redirect to then use it + // If there is a specified path to redirect to then use it. if (model.RedirectUrl.IsNullOrWhiteSpace() == false) { - // validate the redirect url - // if it's not a local url we'll redirect to the root of the current site + // Validate the redirect url. + // If it's not a local url we'll redirect to the root of the current site. return Redirect(Url.IsLocalUrl(model.RedirectUrl) ? model.RedirectUrl - : CurrentPage.AncestorOrSelf(1).Url()); + : CurrentPage.AncestorOrSelf(1).Url(PublishedUrlProvider)); } - //redirect to current page by default - + // Redirect to current page by default. return RedirectToCurrentUmbracoPage(); } } diff --git a/src/Umbraco.Web/Controllers/UmbLoginStatusController.cs b/src/Umbraco.Web.Website/Controllers/UmbLoginStatusController.cs similarity index 54% rename from src/Umbraco.Web/Controllers/UmbLoginStatusController.cs rename to src/Umbraco.Web.Website/Controllers/UmbLoginStatusController.cs index d7a86018381a..1c026c7b46f0 100644 --- a/src/Umbraco.Web/Controllers/UmbLoginStatusController.cs +++ b/src/Umbraco.Web.Website/Controllers/UmbLoginStatusController.cs @@ -1,58 +1,53 @@ -using System.Web.Mvc; -using System.Web.Security; -using Umbraco.Web.Models; -using Umbraco.Web.Mvc; +using Microsoft.AspNetCore.Mvc; using Umbraco.Core; using Umbraco.Core.Cache; using Umbraco.Core.Logging; +using Umbraco.Core.Models.Security; using Umbraco.Core.Persistence; +using Umbraco.Core.Security; using Umbraco.Core.Services; -using Umbraco.Web.Security; +using Umbraco.Web.Common.Filters; +using Umbraco.Web.Routing; -namespace Umbraco.Web.Controllers +namespace Umbraco.Web.Website.Controllers { - [MemberAuthorize] + // TOOO: reinstate [MemberAuthorize] public class UmbLoginStatusController : SurfaceController { - private readonly MembershipHelper _membershipHelper; - - public UmbLoginStatusController() - { - } + private readonly IUmbracoWebsiteSecurity _websiteSecurity; public UmbLoginStatusController(IUmbracoContextAccessor umbracoContextAccessor, IUmbracoDatabaseFactory databaseFactory, ServiceContext services, AppCaches appCaches, - IProfilingLogger profilingLogger, MembershipHelper membershipHelper) - : base(umbracoContextAccessor, databaseFactory, services, appCaches, profilingLogger) + IProfilingLogger profilingLogger, IPublishedUrlProvider publishedUrlProvider, IUmbracoWebsiteSecurity websiteSecurity) + : base(umbracoContextAccessor, databaseFactory, services, appCaches, profilingLogger, publishedUrlProvider) { - _membershipHelper = membershipHelper; + _websiteSecurity = websiteSecurity; } [HttpPost] [ValidateAntiForgeryToken] [ValidateUmbracoFormRouteString] - public ActionResult HandleLogout([Bind(Prefix = "logoutModel")]PostRedirectModel model) + public IActionResult HandleLogout([Bind(Prefix = "logoutModel")]PostRedirectModel model) { if (ModelState.IsValid == false) { return CurrentUmbracoPage(); } - if (_membershipHelper.IsLoggedIn()) + if (_websiteSecurity.IsLoggedIn()) { - FormsAuthentication.SignOut(); + _websiteSecurity.LogOut(); } TempData["LogoutSuccess"] = true; - //if there is a specified path to redirect to then use it + // If there is a specified path to redirect to then use it. if (model.RedirectUrl.IsNullOrWhiteSpace() == false) { return Redirect(model.RedirectUrl); } - //redirect to current page by default - + // Redirect to current page by default. return RedirectToCurrentUmbracoPage(); } } diff --git a/src/Umbraco.Web.Website/Controllers/UmbProfileController.cs b/src/Umbraco.Web.Website/Controllers/UmbProfileController.cs new file mode 100644 index 000000000000..e91266f396cf --- /dev/null +++ b/src/Umbraco.Web.Website/Controllers/UmbProfileController.cs @@ -0,0 +1,63 @@ +using System; +using Microsoft.AspNetCore.Mvc; +using Umbraco.Core; +using Umbraco.Core.Cache; +using Umbraco.Core.Logging; +using Umbraco.Core.Models.Security; +using Umbraco.Core.Persistence; +using Umbraco.Core.Security; +using Umbraco.Core.Services; +using Umbraco.Web.Common.Filters; +using Umbraco.Web.Routing; + +namespace Umbraco.Web.Website.Controllers +{ + // TOOO: reinstate [MemberAuthorize] + public class UmbProfileController : SurfaceController + { + private readonly IUmbracoWebsiteSecurity _websiteSecurity; + + public UmbProfileController(IUmbracoContextAccessor umbracoContextAccessor, IUmbracoDatabaseFactory databaseFactory, + ServiceContext services, AppCaches appCaches, IProfilingLogger profilingLogger, + IPublishedUrlProvider publishedUrlProvider, IUmbracoWebsiteSecurity websiteSecurity) + : base(umbracoContextAccessor, databaseFactory, services, appCaches, profilingLogger, publishedUrlProvider) + { + _websiteSecurity = websiteSecurity; + } + + [HttpPost] + [ValidateAntiForgeryToken] + [ValidateUmbracoFormRouteString] + public IActionResult HandleUpdateProfile([Bind(Prefix = "profileModel")] ProfileModel model) + { + if (ModelState.IsValid == false) + { + return CurrentUmbracoPage(); + } + + _websiteSecurity.UpdateMemberProfile(model, out var status, out var errorMessage); + switch(status) + { + case UpdateMemberProfileStatus.Success: + break; + case UpdateMemberProfileStatus.Error: + // Don't add a field level error, just model level. + ModelState.AddModelError("profileModel", errorMessage); + return CurrentUmbracoPage(); + default: + throw new ArgumentOutOfRangeException(); + } + + TempData["ProfileUpdateSuccess"] = true; + + // If there is a specified path to redirect to then use it. + if (model.RedirectUrl.IsNullOrWhiteSpace() == false) + { + return Redirect(model.RedirectUrl); + } + + // Redirect to current page by default. + return RedirectToCurrentUmbracoPage(); + } + } +} diff --git a/src/Umbraco.Web/Controllers/UmbRegisterController.cs b/src/Umbraco.Web.Website/Controllers/UmbRegisterController.cs similarity index 60% rename from src/Umbraco.Web/Controllers/UmbRegisterController.cs rename to src/Umbraco.Web.Website/Controllers/UmbRegisterController.cs index 30720eb776f7..e20cb88715a9 100644 --- a/src/Umbraco.Web/Controllers/UmbRegisterController.cs +++ b/src/Umbraco.Web.Website/Controllers/UmbRegisterController.cs @@ -1,37 +1,33 @@ using System; -using System.Web.Mvc; -using System.Web.Security; +using Microsoft.AspNetCore.Mvc; using Umbraco.Core; using Umbraco.Core.Cache; using Umbraco.Core.Logging; +using Umbraco.Core.Models.Security; using Umbraco.Core.Persistence; +using Umbraco.Core.Security; using Umbraco.Core.Services; -using Umbraco.Web.Models; -using Umbraco.Web.Mvc; -using Umbraco.Web.Security; +using Umbraco.Web.Common.Filters; +using Umbraco.Web.Routing; -namespace Umbraco.Web.Controllers +namespace Umbraco.Web.Website.Controllers { public class UmbRegisterController : SurfaceController { - private readonly MembershipHelper _membershipHelper; - - public UmbRegisterController() - { - } + private readonly IUmbracoWebsiteSecurity _websiteSecurity; public UmbRegisterController(IUmbracoContextAccessor umbracoContextAccessor, IUmbracoDatabaseFactory databaseFactory, ServiceContext services, AppCaches appCaches, - IProfilingLogger profilingLogger, MembershipHelper membershipHelper) - : base(umbracoContextAccessor, databaseFactory, services, appCaches, profilingLogger) + IProfilingLogger profilingLogger, IPublishedUrlProvider publishedUrlProvider, IUmbracoWebsiteSecurity websiteSecurity) + : base(umbracoContextAccessor, databaseFactory, services, appCaches, profilingLogger, publishedUrlProvider) { - _membershipHelper = membershipHelper; + _websiteSecurity = websiteSecurity; } [HttpPost] [ValidateAntiForgeryToken] [ValidateUmbracoFormRouteString] - public ActionResult HandleRegisterMember([Bind(Prefix = "registerModel")]RegisterModel model) + public IActionResult HandleRegisterMember([Bind(Prefix = "registerModel")]RegisterModel model) { if (ModelState.IsValid == false) { @@ -39,59 +35,51 @@ public ActionResult HandleRegisterMember([Bind(Prefix = "registerModel")]Registe } // U4-10762 Server error with "Register Member" snippet (Cannot save member with empty name) - // If name field is empty, add the email address instead + // If name field is empty, add the email address instead. if (string.IsNullOrEmpty(model.Name) && string.IsNullOrEmpty(model.Email) == false) { model.Name = model.Email; } - MembershipCreateStatus status; - var member = _membershipHelper.RegisterMember(model, out status, model.LoginOnSuccess); + _websiteSecurity.RegisterMember(model, out var status, model.LoginOnSuccess); switch (status) { - case MembershipCreateStatus.Success: + case RegisterMemberStatus.Success: TempData["FormSuccess"] = true; - //if there is a specified path to redirect to then use it + // If there is a specified path to redirect to then use it. if (model.RedirectUrl.IsNullOrWhiteSpace() == false) { return Redirect(model.RedirectUrl); } - //redirect to current page by default + // Redirect to current page by default. return RedirectToCurrentUmbracoPage(); - case MembershipCreateStatus.InvalidUserName: + case RegisterMemberStatus.InvalidUserName: ModelState.AddModelError((model.UsernameIsEmail || model.Username == null) ? "registerModel.Email" : "registerModel.Username", "Username is not valid"); break; - case MembershipCreateStatus.InvalidPassword: + case RegisterMemberStatus.InvalidPassword: ModelState.AddModelError("registerModel.Password", "The password is not strong enough"); break; - case MembershipCreateStatus.InvalidQuestion: - case MembershipCreateStatus.InvalidAnswer: - // TODO: Support q/a http://issues.umbraco.org/issue/U4-3213 - throw new NotImplementedException(status.ToString()); - case MembershipCreateStatus.InvalidEmail: + case RegisterMemberStatus.InvalidEmail: ModelState.AddModelError("registerModel.Email", "Email is invalid"); break; - case MembershipCreateStatus.DuplicateUserName: + case RegisterMemberStatus.DuplicateUserName: ModelState.AddModelError((model.UsernameIsEmail || model.Username == null) ? "registerModel.Email" : "registerModel.Username", "A member with this username already exists."); break; - case MembershipCreateStatus.DuplicateEmail: + case RegisterMemberStatus.DuplicateEmail: ModelState.AddModelError("registerModel.Email", "A member with this e-mail address already exists"); break; - case MembershipCreateStatus.UserRejected: - case MembershipCreateStatus.InvalidProviderUserKey: - case MembershipCreateStatus.DuplicateProviderUserKey: - case MembershipCreateStatus.ProviderError: - //don't add a field level error, just model level + case RegisterMemberStatus.Error: + // Don't add a field level error, just model level. ModelState.AddModelError("registerModel", "An error occurred creating the member: " + status); break; default: @@ -100,6 +88,5 @@ public ActionResult HandleRegisterMember([Bind(Prefix = "registerModel")]Registe return CurrentUmbracoPage(); } - } } diff --git a/src/Umbraco.Web.Website/Extensions/UmbracoWebstiteServiceCollectionExtensions.cs b/src/Umbraco.Web.Website/Extensions/UmbracoWebstiteServiceCollectionExtensions.cs index 317afedc7a41..12efe811383b 100644 --- a/src/Umbraco.Web.Website/Extensions/UmbracoWebstiteServiceCollectionExtensions.cs +++ b/src/Umbraco.Web.Website/Extensions/UmbracoWebstiteServiceCollectionExtensions.cs @@ -1,6 +1,8 @@ using Microsoft.AspNetCore.Mvc; using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.Options; +using Umbraco.Core.Security; +using Umbraco.Web.Website.Security; using Umbraco.Web.Website.ViewEngines; namespace Umbraco.Extensions @@ -21,6 +23,9 @@ public static void AddUmbracoWebsite(this IServiceCollection services) //TODO figure out if we need more to work on load balanced setups services.AddDataProtection(); + + // Website security + services.AddSingleton(); } } } diff --git a/src/Umbraco.Web.Website/Security/UmbracoWebsiteSecurity.cs b/src/Umbraco.Web.Website/Security/UmbracoWebsiteSecurity.cs new file mode 100644 index 000000000000..cd1d667dc9d0 --- /dev/null +++ b/src/Umbraco.Web.Website/Security/UmbracoWebsiteSecurity.cs @@ -0,0 +1,33 @@ +using Umbraco.Core.Models.Security; +using Umbraco.Core.Security; + +namespace Umbraco.Web.Website.Security +{ + public class UmbracoWebsiteSecurity : IUmbracoWebsiteSecurity + { + public void RegisterMember(RegisterModel model, out RegisterMemberStatus status, bool logMemberIn = true) + { + throw new System.NotImplementedException(); + } + + public void UpdateMemberProfile(ProfileModel model, out UpdateMemberProfileStatus status, out string errorMessage) + { + throw new System.NotImplementedException(); + } + + public bool IsLoggedIn() + { + throw new System.NotImplementedException(); + } + + public bool Login(string username, string password) + { + throw new System.NotImplementedException(); + } + + public void LogOut() + { + throw new System.NotImplementedException(); + } + } +} diff --git a/src/Umbraco.Web.Website/Umbraco.Web.Website.csproj b/src/Umbraco.Web.Website/Umbraco.Web.Website.csproj index daba18d51bd8..55cfa3e44db3 100644 --- a/src/Umbraco.Web.Website/Umbraco.Web.Website.csproj +++ b/src/Umbraco.Web.Website/Umbraco.Web.Website.csproj @@ -1,4 +1,4 @@ - + netcoreapp3.1 diff --git a/src/Umbraco.Web/Controllers/UmbProfileController.cs b/src/Umbraco.Web/Controllers/UmbProfileController.cs deleted file mode 100644 index 88edee26c827..000000000000 --- a/src/Umbraco.Web/Controllers/UmbProfileController.cs +++ /dev/null @@ -1,61 +0,0 @@ -using System; -using System.Web.Mvc; -using Umbraco.Web.Models; -using Umbraco.Web.Mvc; -using Umbraco.Core.Security; -using Umbraco.Core; -using Umbraco.Core.Cache; -using Umbraco.Core.Logging; -using Umbraco.Core.Persistence; -using Umbraco.Core.Services; -using Umbraco.Web.Security; - -namespace Umbraco.Web.Controllers -{ - [MemberAuthorize] - public class UmbProfileController : SurfaceController - { - private readonly MembershipHelper _membershipHelper; - - public UmbProfileController() - { } - - public UmbProfileController(IUmbracoContextAccessor umbracoContextAccessor, IUmbracoDatabaseFactory databaseFactory, - ServiceContext services, AppCaches appCaches, IProfilingLogger profilingLogger, - MembershipHelper membershipHelper) - : base(umbracoContextAccessor, databaseFactory, services, appCaches, profilingLogger) - { - _membershipHelper = membershipHelper; - } - - [HttpPost] - [ValidateAntiForgeryToken] - [ValidateUmbracoFormRouteString] - public ActionResult HandleUpdateProfile([Bind(Prefix = "profileModel")] ProfileModel model) - { - if (ModelState.IsValid == false) - { - return CurrentUmbracoPage(); - } - - var updateAttempt = _membershipHelper.UpdateMemberProfile(model); - if (updateAttempt.Success == false) - { - //don't add a field level error, just model level - ModelState.AddModelError("profileModel", updateAttempt.Exception.Message); - return CurrentUmbracoPage(); - } - - TempData["ProfileUpdateSuccess"] = true; - - //if there is a specified path to redirect to then use it - if (model.RedirectUrl.IsNullOrWhiteSpace() == false) - { - return Redirect(model.RedirectUrl); - } - - //redirect to current page by default - return RedirectToCurrentUmbracoPage(); - } - } -} diff --git a/src/Umbraco.Web/Security/MembershipHelper.cs b/src/Umbraco.Web/Security/MembershipHelper.cs index 6d762afcd98d..1e26782d4ab0 100644 --- a/src/Umbraco.Web/Security/MembershipHelper.cs +++ b/src/Umbraco.Web/Security/MembershipHelper.cs @@ -1,20 +1,20 @@ using System; using System.Collections.Generic; +using System.ComponentModel.DataAnnotations; using System.Linq; using System.Web.Security; using Microsoft.Extensions.Logging; using Umbraco.Core; -using Umbraco.Core.Models; -using Umbraco.Web.Models; -using Umbraco.Web.PublishedCache; using Umbraco.Core.Cache; -using Umbraco.Web.Composing; +using Umbraco.Core.Models; using Umbraco.Core.Models.PublishedContent; +using Umbraco.Core.Models.Security; using Umbraco.Core.Services; using Umbraco.Core.Strings; using Umbraco.Web.Editors; +using Umbraco.Web.Models; +using Umbraco.Web.PublishedCache; using Umbraco.Web.Security.Providers; -using System.ComponentModel.DataAnnotations; namespace Umbraco.Web.Security { @@ -443,7 +443,7 @@ public virtual ProfileModel GetCurrentMemberProfileModel() return null; } - var model = ProfileModel.CreateModel(); + var model = new ProfileModel(); model.Name = member.Name; model.MemberTypeAlias = member.ContentTypeAlias; @@ -458,7 +458,6 @@ public virtual ProfileModel GetCurrentMemberProfileModel() model.LastActivityDate = membershipUser.LastActivityDate; model.LastPasswordChangedDate = membershipUser.LastPasswordChangedDate; - var memberType = _memberTypeService.Get(member.ContentTypeId); var builtIns = ConventionsHelper.GetStandardPropertyTypeStubs(_shortStringHelper).Select(x => x.Key).ToArray(); diff --git a/src/Umbraco.Web/Umbraco.Web.csproj b/src/Umbraco.Web/Umbraco.Web.csproj index b54df888552e..8157a9071548 100644 --- a/src/Umbraco.Web/Umbraco.Web.csproj +++ b/src/Umbraco.Web/Umbraco.Web.csproj @@ -142,6 +142,7 @@ + @@ -220,8 +221,6 @@ - - @@ -236,13 +235,10 @@ - - - @@ -253,10 +249,8 @@ - - From d8ef341854c9f753f3e7ba209ec7788df53e4eef Mon Sep 17 00:00:00 2001 From: Andy Butland Date: Wed, 18 Nov 2020 16:52:40 +0100 Subject: [PATCH 2/5] Refactored to async where appropriate. Added call to new abstraction in member authorize attribute. --- .../Security/IUmbracoWebsiteSecurity.cs | 47 +++++++++++++++---- .../Security/UpdateMemberProfileResult.cs | 24 ++++++++++ .../Filters/UmbracoMemberAuthorizeFilter.cs | 28 ++++++++--- .../Controllers/UmbLoginController.cs | 7 +-- .../Controllers/UmbLoginStatusController.cs | 9 ++-- .../Controllers/UmbProfileController.cs | 11 +++-- .../Controllers/UmbRegisterController.cs | 9 ++-- .../Security/UmbracoWebsiteSecurity.cs | 36 +++++++++++--- 8 files changed, 134 insertions(+), 37 deletions(-) create mode 100644 src/Umbraco.Core/Security/UpdateMemberProfileResult.cs diff --git a/src/Umbraco.Core/Security/IUmbracoWebsiteSecurity.cs b/src/Umbraco.Core/Security/IUmbracoWebsiteSecurity.cs index f14adae1746a..2f35a83dc217 100644 --- a/src/Umbraco.Core/Security/IUmbracoWebsiteSecurity.cs +++ b/src/Umbraco.Core/Security/IUmbracoWebsiteSecurity.cs @@ -1,20 +1,51 @@ -using Umbraco.Core.Models.Security; +using System.Collections.Generic; +using System.Threading.Tasks; +using Umbraco.Core.Models.Security; namespace Umbraco.Core.Security { public interface IUmbracoWebsiteSecurity { - // TODO: this should return the member, but in what form? MembershipUser is in place on MembershipHelper, but - // isn't appropriate for when we're using ASP.NET Identity. - void RegisterMember(RegisterModel model, out RegisterMemberStatus status, bool logMemberIn = true); + /// + /// Registers a new member. + /// + /// Register member model. + /// Flag for whether to log the member in upon successful registration. + /// Result of registration operation. + Task RegisterMemberAsync(RegisterModel model, bool logMemberIn = true); - // TODO: again, should this return the member? - void UpdateMemberProfile(ProfileModel model, out UpdateMemberProfileStatus status, out string errorMessage); + /// + /// Updates the currently logged in member's profile. + /// + /// Update member profile model. + /// Result of update profile operation. + Task UpdateMemberProfileAsync(ProfileModel model); - bool Login(string username, string password); + /// + /// A helper method to perform the validation and logging in of a member. + /// + /// The username. + /// The password. + /// Result of login operation. + Task LoginAsync(string username, string password); bool IsLoggedIn(); - void LogOut(); + /// + /// Logs out the current member. + /// + Task LogOutAsync(); + + /// + /// Checks if the current member is authorized based on the parameters provided. + /// + /// Allowed types. + /// Allowed groups. + /// Allowed individual members. + /// True or false if the currently logged in member is authorized + bool IsMemberAuthorized( + IEnumerable allowTypes = null, + IEnumerable allowGroups = null, + IEnumerable allowMembers = null); } } diff --git a/src/Umbraco.Core/Security/UpdateMemberProfileResult.cs b/src/Umbraco.Core/Security/UpdateMemberProfileResult.cs new file mode 100644 index 000000000000..1d435378a6dd --- /dev/null +++ b/src/Umbraco.Core/Security/UpdateMemberProfileResult.cs @@ -0,0 +1,24 @@ +namespace Umbraco.Core.Security +{ + public class UpdateMemberProfileResult + { + private UpdateMemberProfileResult() + { + } + + public UpdateMemberProfileStatus Status { get; private set; } + + public string ErrorMessage { get; private set; } + + public static UpdateMemberProfileResult Success() + { + return new UpdateMemberProfileResult { Status = UpdateMemberProfileStatus.Success }; + } + + public static UpdateMemberProfileResult Error(string message) + { + return new UpdateMemberProfileResult { Status = UpdateMemberProfileStatus.Error, ErrorMessage = message }; + } + } + +} diff --git a/src/Umbraco.Web.Common/Filters/UmbracoMemberAuthorizeFilter.cs b/src/Umbraco.Web.Common/Filters/UmbracoMemberAuthorizeFilter.cs index 27c292263740..4019f462eb74 100644 --- a/src/Umbraco.Web.Common/Filters/UmbracoMemberAuthorizeFilter.cs +++ b/src/Umbraco.Web.Common/Filters/UmbracoMemberAuthorizeFilter.cs @@ -2,6 +2,7 @@ using Microsoft.AspNetCore.Mvc.Filters; using System.Collections.Generic; using Umbraco.Core; +using Umbraco.Core.Security; using Umbraco.Extensions; namespace Umbraco.Web.Common.Filters @@ -12,6 +13,13 @@ namespace Umbraco.Web.Common.Filters /// public class UmbracoMemberAuthorizeFilter : IAuthorizationFilter { + private readonly IUmbracoWebsiteSecurity _websiteSecurity; + + public UmbracoMemberAuthorizeFilter(IUmbracoWebsiteSecurity websiteSecurity) + { + _websiteSecurity = websiteSecurity; + } + /// /// Comma delimited list of allowed member types /// @@ -27,9 +35,7 @@ public class UmbracoMemberAuthorizeFilter : IAuthorizationFilter /// public string AllowMembers { get; private set; } - - private UmbracoMemberAuthorizeFilter( - string allowType, string allowGroup, string allowMembers) + private UmbracoMemberAuthorizeFilter(string allowType, string allowGroup, string allowMembers) { AllowType = allowType; AllowGroup = allowGroup; @@ -48,11 +54,19 @@ public void OnAuthorization(AuthorizationFilterContext context) private bool IsAuthorized() { if (AllowMembers.IsNullOrWhiteSpace()) - AllowMembers = ""; + { + AllowMembers = string.Empty; + } + if (AllowGroup.IsNullOrWhiteSpace()) - AllowGroup = ""; + { + AllowGroup = string.Empty; + } + if (AllowType.IsNullOrWhiteSpace()) - AllowType = ""; + { + AllowType = string.Empty; + } var members = new List(); foreach (var s in AllowMembers.Split(',')) @@ -63,7 +77,7 @@ private bool IsAuthorized() } } - return false;// TODO reintroduce when members are implemented: _memberHelper.IsMemberAuthorized(AllowType.Split(','), AllowGroup.Split(','), members); + return _websiteSecurity.IsMemberAuthorized(AllowType.Split(','), AllowGroup.Split(','), members); } } } diff --git a/src/Umbraco.Web.Website/Controllers/UmbLoginController.cs b/src/Umbraco.Web.Website/Controllers/UmbLoginController.cs index a986cef77e6c..51938f00f509 100644 --- a/src/Umbraco.Web.Website/Controllers/UmbLoginController.cs +++ b/src/Umbraco.Web.Website/Controllers/UmbLoginController.cs @@ -1,4 +1,5 @@ -using Microsoft.AspNetCore.Mvc; +using System.Threading.Tasks; +using Microsoft.AspNetCore.Mvc; using Umbraco.Core; using Umbraco.Core.Cache; using Umbraco.Core.Logging; @@ -26,14 +27,14 @@ public class UmbLoginController : SurfaceController [HttpPost] [ValidateAntiForgeryToken] [ValidateUmbracoFormRouteString] - public IActionResult HandleLogin([Bind(Prefix = "loginModel")]LoginModel model) + public async Task HandleLogin([Bind(Prefix = "loginModel")]LoginModel model) { if (ModelState.IsValid == false) { return CurrentUmbracoPage(); } - if (_websiteSecurity.Login(model.Username, model.Password) == false) + if (await _websiteSecurity.LoginAsync(model.Username, model.Password) == false) { // Don't add a field level error, just model level. ModelState.AddModelError("loginModel", "Invalid username or password"); diff --git a/src/Umbraco.Web.Website/Controllers/UmbLoginStatusController.cs b/src/Umbraco.Web.Website/Controllers/UmbLoginStatusController.cs index 1c026c7b46f0..3da1f34282d3 100644 --- a/src/Umbraco.Web.Website/Controllers/UmbLoginStatusController.cs +++ b/src/Umbraco.Web.Website/Controllers/UmbLoginStatusController.cs @@ -1,4 +1,5 @@ -using Microsoft.AspNetCore.Mvc; +using System.Threading.Tasks; +using Microsoft.AspNetCore.Mvc; using Umbraco.Core; using Umbraco.Core.Cache; using Umbraco.Core.Logging; @@ -11,7 +12,7 @@ namespace Umbraco.Web.Website.Controllers { - // TOOO: reinstate [MemberAuthorize] + [UmbracoMemberAuthorize] public class UmbLoginStatusController : SurfaceController { private readonly IUmbracoWebsiteSecurity _websiteSecurity; @@ -27,7 +28,7 @@ public class UmbLoginStatusController : SurfaceController [HttpPost] [ValidateAntiForgeryToken] [ValidateUmbracoFormRouteString] - public IActionResult HandleLogout([Bind(Prefix = "logoutModel")]PostRedirectModel model) + public async Task HandleLogout([Bind(Prefix = "logoutModel")]PostRedirectModel model) { if (ModelState.IsValid == false) { @@ -36,7 +37,7 @@ public IActionResult HandleLogout([Bind(Prefix = "logoutModel")]PostRedirectMode if (_websiteSecurity.IsLoggedIn()) { - _websiteSecurity.LogOut(); + await _websiteSecurity.LogOutAsync(); } TempData["LogoutSuccess"] = true; diff --git a/src/Umbraco.Web.Website/Controllers/UmbProfileController.cs b/src/Umbraco.Web.Website/Controllers/UmbProfileController.cs index e91266f396cf..69bf77981ea7 100644 --- a/src/Umbraco.Web.Website/Controllers/UmbProfileController.cs +++ b/src/Umbraco.Web.Website/Controllers/UmbProfileController.cs @@ -1,4 +1,5 @@ using System; +using System.Threading.Tasks; using Microsoft.AspNetCore.Mvc; using Umbraco.Core; using Umbraco.Core.Cache; @@ -12,7 +13,7 @@ namespace Umbraco.Web.Website.Controllers { - // TOOO: reinstate [MemberAuthorize] + [UmbracoMemberAuthorize] public class UmbProfileController : SurfaceController { private readonly IUmbracoWebsiteSecurity _websiteSecurity; @@ -28,21 +29,21 @@ public class UmbProfileController : SurfaceController [HttpPost] [ValidateAntiForgeryToken] [ValidateUmbracoFormRouteString] - public IActionResult HandleUpdateProfile([Bind(Prefix = "profileModel")] ProfileModel model) + public async Task HandleUpdateProfile([Bind(Prefix = "profileModel")] ProfileModel model) { if (ModelState.IsValid == false) { return CurrentUmbracoPage(); } - _websiteSecurity.UpdateMemberProfile(model, out var status, out var errorMessage); - switch(status) + var result = await _websiteSecurity.UpdateMemberProfileAsync(model); + switch (result.Status) { case UpdateMemberProfileStatus.Success: break; case UpdateMemberProfileStatus.Error: // Don't add a field level error, just model level. - ModelState.AddModelError("profileModel", errorMessage); + ModelState.AddModelError("profileModel", result.ErrorMessage); return CurrentUmbracoPage(); default: throw new ArgumentOutOfRangeException(); diff --git a/src/Umbraco.Web.Website/Controllers/UmbRegisterController.cs b/src/Umbraco.Web.Website/Controllers/UmbRegisterController.cs index e20cb88715a9..8af215702232 100644 --- a/src/Umbraco.Web.Website/Controllers/UmbRegisterController.cs +++ b/src/Umbraco.Web.Website/Controllers/UmbRegisterController.cs @@ -1,4 +1,5 @@ using System; +using System.Threading.Tasks; using Microsoft.AspNetCore.Mvc; using Umbraco.Core; using Umbraco.Core.Cache; @@ -27,7 +28,7 @@ public class UmbRegisterController : SurfaceController [HttpPost] [ValidateAntiForgeryToken] [ValidateUmbracoFormRouteString] - public IActionResult HandleRegisterMember([Bind(Prefix = "registerModel")]RegisterModel model) + public async Task HandleRegisterMember([Bind(Prefix = "registerModel")]RegisterModel model) { if (ModelState.IsValid == false) { @@ -41,9 +42,9 @@ public IActionResult HandleRegisterMember([Bind(Prefix = "registerModel")]Regist model.Name = model.Email; } - _websiteSecurity.RegisterMember(model, out var status, model.LoginOnSuccess); + var result = await _websiteSecurity.RegisterMemberAsync(model, model.LoginOnSuccess); - switch (status) + switch (result) { case RegisterMemberStatus.Success: @@ -80,7 +81,7 @@ public IActionResult HandleRegisterMember([Bind(Prefix = "registerModel")]Regist break; case RegisterMemberStatus.Error: // Don't add a field level error, just model level. - ModelState.AddModelError("registerModel", "An error occurred creating the member: " + status); + ModelState.AddModelError("registerModel", $"An error occurred creating the member: {result}"); break; default: throw new ArgumentOutOfRangeException(); diff --git a/src/Umbraco.Web.Website/Security/UmbracoWebsiteSecurity.cs b/src/Umbraco.Web.Website/Security/UmbracoWebsiteSecurity.cs index cd1d667dc9d0..90e80537ec35 100644 --- a/src/Umbraco.Web.Website/Security/UmbracoWebsiteSecurity.cs +++ b/src/Umbraco.Web.Website/Security/UmbracoWebsiteSecurity.cs @@ -1,31 +1,55 @@ -using Umbraco.Core.Models.Security; +using System.Collections.Generic; +using System.Threading.Tasks; +using Microsoft.AspNetCore.Authentication; +using Microsoft.AspNetCore.Authentication.Cookies; +using Microsoft.AspNetCore.Http; +using Umbraco.Core.Models.Security; using Umbraco.Core.Security; namespace Umbraco.Web.Website.Security { public class UmbracoWebsiteSecurity : IUmbracoWebsiteSecurity { - public void RegisterMember(RegisterModel model, out RegisterMemberStatus status, bool logMemberIn = true) + private readonly IHttpContextAccessor _httpContextAccessor; + + public UmbracoWebsiteSecurity(IHttpContextAccessor httpContextAccessor) + { + _httpContextAccessor = httpContextAccessor; + } + + /// + public Task RegisterMemberAsync(RegisterModel model, bool logMemberIn = true) { throw new System.NotImplementedException(); } - public void UpdateMemberProfile(ProfileModel model, out UpdateMemberProfileStatus status, out string errorMessage) + /// + public Task UpdateMemberProfileAsync(ProfileModel model) { throw new System.NotImplementedException(); } + /// public bool IsLoggedIn() { - throw new System.NotImplementedException(); + var httpContext = _httpContextAccessor.HttpContext; + return httpContext?.User != null && httpContext.User.Identity.IsAuthenticated; } - public bool Login(string username, string password) + /// + public Task LoginAsync(string username, string password) { throw new System.NotImplementedException(); } - public void LogOut() + /// + public async Task LogOutAsync() + { + await _httpContextAccessor.HttpContext.SignOutAsync(CookieAuthenticationDefaults.AuthenticationScheme); + } + + /// + public bool IsMemberAuthorized(IEnumerable allowTypes = null, IEnumerable allowGroups = null, IEnumerable allowMembers = null) { throw new System.NotImplementedException(); } From c971a2d23dd9edb2c9f42d373f6e1a3d4ce2f00f Mon Sep 17 00:00:00 2001 From: Andy Butland Date: Wed, 18 Nov 2020 17:37:31 +0100 Subject: [PATCH 3/5] Introduced website security accessor to ensure members aren't shared between sessions. --- .../HybridBackofficeSecurityAccessor.cs | 9 ++---- .../HybridUmbracoWebsiteSecurityAccessor.cs | 28 +++++++++++++++++++ .../IUmbracoWebsiteSecurityAccessor.cs | 7 +++++ .../Runtime/AspNetCoreComposer.cs | 3 ++ .../Controllers/UmbLoginController.cs | 8 +++--- .../Controllers/UmbLoginStatusController.cs | 10 +++---- .../Controllers/UmbProfileController.cs | 8 +++--- .../Controllers/UmbRegisterController.cs | 8 +++--- ...racoWebstiteServiceCollectionExtensions.cs | 3 -- 9 files changed, 58 insertions(+), 26 deletions(-) rename src/Umbraco.Core/{ => Security}/HybridBackofficeSecurityAccessor.cs (72%) create mode 100644 src/Umbraco.Core/Security/HybridUmbracoWebsiteSecurityAccessor.cs create mode 100644 src/Umbraco.Core/Security/IUmbracoWebsiteSecurityAccessor.cs diff --git a/src/Umbraco.Core/HybridBackofficeSecurityAccessor.cs b/src/Umbraco.Core/Security/HybridBackofficeSecurityAccessor.cs similarity index 72% rename from src/Umbraco.Core/HybridBackofficeSecurityAccessor.cs rename to src/Umbraco.Core/Security/HybridBackofficeSecurityAccessor.cs index 4549227c897e..eb4be355f41a 100644 --- a/src/Umbraco.Core/HybridBackofficeSecurityAccessor.cs +++ b/src/Umbraco.Core/Security/HybridBackofficeSecurityAccessor.cs @@ -1,15 +1,12 @@ using Umbraco.Core.Cache; -using Umbraco.Core.Security; using Umbraco.Web; -using Umbraco.Web.Security; -namespace Umbraco.Core +namespace Umbraco.Core.Security { - public class HybridBackofficeSecurityAccessor : HybridAccessorBase, IBackOfficeSecurityAccessor { /// - /// Initializes a new instance of the class. + /// Initializes a new instance of the class. /// public HybridBackofficeSecurityAccessor(IRequestCache requestCache) : base(requestCache) @@ -19,7 +16,7 @@ public HybridBackofficeSecurityAccessor(IRequestCache requestCache) protected override string ItemKey => "Umbraco.Web.HybridBackofficeSecurityAccessor"; /// - /// Gets or sets the object. + /// Gets or sets the object. /// public IBackOfficeSecurity BackOfficeSecurity { diff --git a/src/Umbraco.Core/Security/HybridUmbracoWebsiteSecurityAccessor.cs b/src/Umbraco.Core/Security/HybridUmbracoWebsiteSecurityAccessor.cs new file mode 100644 index 000000000000..09a7ab5d1b98 --- /dev/null +++ b/src/Umbraco.Core/Security/HybridUmbracoWebsiteSecurityAccessor.cs @@ -0,0 +1,28 @@ +using Umbraco.Core.Cache; +using Umbraco.Web; + +namespace Umbraco.Core.Security +{ + + public class HybridUmbracoWebsiteSecurityAccessor : HybridAccessorBase, IUmbracoWebsiteSecurityAccessor + { + /// + /// Initializes a new instance of the class. + /// + public HybridUmbracoWebsiteSecurityAccessor(IRequestCache requestCache) + : base(requestCache) + { } + + /// + protected override string ItemKey => "Umbraco.Web.HybridUmbracoWebsiteSecurityAccessor"; + + /// + /// Gets or sets the object. + /// + public IUmbracoWebsiteSecurity WebsiteSecurity + { + get => Value; + set => Value = value; + } + } +} diff --git a/src/Umbraco.Core/Security/IUmbracoWebsiteSecurityAccessor.cs b/src/Umbraco.Core/Security/IUmbracoWebsiteSecurityAccessor.cs new file mode 100644 index 000000000000..618aeb71464b --- /dev/null +++ b/src/Umbraco.Core/Security/IUmbracoWebsiteSecurityAccessor.cs @@ -0,0 +1,7 @@ +namespace Umbraco.Core.Security +{ + public interface IUmbracoWebsiteSecurityAccessor + { + IUmbracoWebsiteSecurity WebsiteSecurity { get; set; } + } +} diff --git a/src/Umbraco.Web.Common/Runtime/AspNetCoreComposer.cs b/src/Umbraco.Web.Common/Runtime/AspNetCoreComposer.cs index 8acea232890b..82fc2701a105 100644 --- a/src/Umbraco.Web.Common/Runtime/AspNetCoreComposer.cs +++ b/src/Umbraco.Web.Common/Runtime/AspNetCoreComposer.cs @@ -74,9 +74,12 @@ public override void Compose(Composition composition) // register the umbraco context factory composition.Services.AddUnique(); + composition.Services.AddUnique(); composition.Services.AddUnique(); + composition.Services.AddUnique(); + //register the install components composition.ComposeInstaller(); diff --git a/src/Umbraco.Web.Website/Controllers/UmbLoginController.cs b/src/Umbraco.Web.Website/Controllers/UmbLoginController.cs index 51938f00f509..6ba0f582c8d4 100644 --- a/src/Umbraco.Web.Website/Controllers/UmbLoginController.cs +++ b/src/Umbraco.Web.Website/Controllers/UmbLoginController.cs @@ -14,14 +14,14 @@ namespace Umbraco.Web.Website.Controllers { public class UmbLoginController : SurfaceController { - private readonly IUmbracoWebsiteSecurity _websiteSecurity; + private readonly IUmbracoWebsiteSecurityAccessor _websiteSecurityAccessor; public UmbLoginController(IUmbracoContextAccessor umbracoContextAccessor, IUmbracoDatabaseFactory databaseFactory, ServiceContext services, AppCaches appCaches, IProfilingLogger profilingLogger, IPublishedUrlProvider publishedUrlProvider, - IUmbracoWebsiteSecurity websiteSecurity) + IUmbracoWebsiteSecurityAccessor websiteSecurityAccessor) : base(umbracoContextAccessor, databaseFactory, services, appCaches, profilingLogger, publishedUrlProvider) { - _websiteSecurity = websiteSecurity; + _websiteSecurityAccessor = websiteSecurityAccessor; } [HttpPost] @@ -34,7 +34,7 @@ public async Task HandleLogin([Bind(Prefix = "loginModel")]LoginM return CurrentUmbracoPage(); } - if (await _websiteSecurity.LoginAsync(model.Username, model.Password) == false) + if (await _websiteSecurityAccessor.WebsiteSecurity.LoginAsync(model.Username, model.Password) == false) { // Don't add a field level error, just model level. ModelState.AddModelError("loginModel", "Invalid username or password"); diff --git a/src/Umbraco.Web.Website/Controllers/UmbLoginStatusController.cs b/src/Umbraco.Web.Website/Controllers/UmbLoginStatusController.cs index 3da1f34282d3..e9bf164eb335 100644 --- a/src/Umbraco.Web.Website/Controllers/UmbLoginStatusController.cs +++ b/src/Umbraco.Web.Website/Controllers/UmbLoginStatusController.cs @@ -15,14 +15,14 @@ namespace Umbraco.Web.Website.Controllers [UmbracoMemberAuthorize] public class UmbLoginStatusController : SurfaceController { - private readonly IUmbracoWebsiteSecurity _websiteSecurity; + private readonly IUmbracoWebsiteSecurityAccessor _websiteSecurityAccessor; public UmbLoginStatusController(IUmbracoContextAccessor umbracoContextAccessor, IUmbracoDatabaseFactory databaseFactory, ServiceContext services, AppCaches appCaches, - IProfilingLogger profilingLogger, IPublishedUrlProvider publishedUrlProvider, IUmbracoWebsiteSecurity websiteSecurity) + IProfilingLogger profilingLogger, IPublishedUrlProvider publishedUrlProvider, IUmbracoWebsiteSecurityAccessor websiteSecurityAccessor) : base(umbracoContextAccessor, databaseFactory, services, appCaches, profilingLogger, publishedUrlProvider) { - _websiteSecurity = websiteSecurity; + _websiteSecurityAccessor = websiteSecurityAccessor; } [HttpPost] @@ -35,9 +35,9 @@ public async Task HandleLogout([Bind(Prefix = "logoutModel")]Post return CurrentUmbracoPage(); } - if (_websiteSecurity.IsLoggedIn()) + if (_websiteSecurityAccessor.WebsiteSecurity.IsLoggedIn()) { - await _websiteSecurity.LogOutAsync(); + await _websiteSecurityAccessor.WebsiteSecurity.LogOutAsync(); } TempData["LogoutSuccess"] = true; diff --git a/src/Umbraco.Web.Website/Controllers/UmbProfileController.cs b/src/Umbraco.Web.Website/Controllers/UmbProfileController.cs index 69bf77981ea7..cc23786c4b35 100644 --- a/src/Umbraco.Web.Website/Controllers/UmbProfileController.cs +++ b/src/Umbraco.Web.Website/Controllers/UmbProfileController.cs @@ -16,14 +16,14 @@ namespace Umbraco.Web.Website.Controllers [UmbracoMemberAuthorize] public class UmbProfileController : SurfaceController { - private readonly IUmbracoWebsiteSecurity _websiteSecurity; + private readonly IUmbracoWebsiteSecurityAccessor _websiteSecurityAccessor; public UmbProfileController(IUmbracoContextAccessor umbracoContextAccessor, IUmbracoDatabaseFactory databaseFactory, ServiceContext services, AppCaches appCaches, IProfilingLogger profilingLogger, - IPublishedUrlProvider publishedUrlProvider, IUmbracoWebsiteSecurity websiteSecurity) + IPublishedUrlProvider publishedUrlProvider, IUmbracoWebsiteSecurityAccessor websiteSecurityAccessor) : base(umbracoContextAccessor, databaseFactory, services, appCaches, profilingLogger, publishedUrlProvider) { - _websiteSecurity = websiteSecurity; + _websiteSecurityAccessor = websiteSecurityAccessor; } [HttpPost] @@ -36,7 +36,7 @@ public async Task HandleUpdateProfile([Bind(Prefix = "profileMode return CurrentUmbracoPage(); } - var result = await _websiteSecurity.UpdateMemberProfileAsync(model); + var result = await _websiteSecurityAccessor.WebsiteSecurity.UpdateMemberProfileAsync(model); switch (result.Status) { case UpdateMemberProfileStatus.Success: diff --git a/src/Umbraco.Web.Website/Controllers/UmbRegisterController.cs b/src/Umbraco.Web.Website/Controllers/UmbRegisterController.cs index 8af215702232..9542a2bf759b 100644 --- a/src/Umbraco.Web.Website/Controllers/UmbRegisterController.cs +++ b/src/Umbraco.Web.Website/Controllers/UmbRegisterController.cs @@ -15,14 +15,14 @@ namespace Umbraco.Web.Website.Controllers { public class UmbRegisterController : SurfaceController { - private readonly IUmbracoWebsiteSecurity _websiteSecurity; + private readonly IUmbracoWebsiteSecurityAccessor _websiteSecurityAccessor; public UmbRegisterController(IUmbracoContextAccessor umbracoContextAccessor, IUmbracoDatabaseFactory databaseFactory, ServiceContext services, AppCaches appCaches, - IProfilingLogger profilingLogger, IPublishedUrlProvider publishedUrlProvider, IUmbracoWebsiteSecurity websiteSecurity) + IProfilingLogger profilingLogger, IPublishedUrlProvider publishedUrlProvider, IUmbracoWebsiteSecurityAccessor websiteSecurityAccessor) : base(umbracoContextAccessor, databaseFactory, services, appCaches, profilingLogger, publishedUrlProvider) { - _websiteSecurity = websiteSecurity; + _websiteSecurityAccessor = websiteSecurityAccessor; } [HttpPost] @@ -42,7 +42,7 @@ public async Task HandleRegisterMember([Bind(Prefix = "registerMo model.Name = model.Email; } - var result = await _websiteSecurity.RegisterMemberAsync(model, model.LoginOnSuccess); + var result = await _websiteSecurityAccessor.WebsiteSecurity.RegisterMemberAsync(model, model.LoginOnSuccess); switch (result) { diff --git a/src/Umbraco.Web.Website/Extensions/UmbracoWebstiteServiceCollectionExtensions.cs b/src/Umbraco.Web.Website/Extensions/UmbracoWebstiteServiceCollectionExtensions.cs index 12efe811383b..5737cb10300a 100644 --- a/src/Umbraco.Web.Website/Extensions/UmbracoWebstiteServiceCollectionExtensions.cs +++ b/src/Umbraco.Web.Website/Extensions/UmbracoWebstiteServiceCollectionExtensions.cs @@ -23,9 +23,6 @@ public static void AddUmbracoWebsite(this IServiceCollection services) //TODO figure out if we need more to work on load balanced setups services.AddDataProtection(); - - // Website security - services.AddSingleton(); } } } From 024b37e0e61da606a9c13d6fd0e08ccd3e782231 Mon Sep 17 00:00:00 2001 From: Andy Butland Date: Wed, 18 Nov 2020 17:41:10 +0100 Subject: [PATCH 4/5] Added further method header comment. --- src/Umbraco.Core/Security/IUmbracoWebsiteSecurity.cs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/Umbraco.Core/Security/IUmbracoWebsiteSecurity.cs b/src/Umbraco.Core/Security/IUmbracoWebsiteSecurity.cs index 2f35a83dc217..c302d45354b7 100644 --- a/src/Umbraco.Core/Security/IUmbracoWebsiteSecurity.cs +++ b/src/Umbraco.Core/Security/IUmbracoWebsiteSecurity.cs @@ -29,6 +29,10 @@ public interface IUmbracoWebsiteSecurity /// Result of login operation. Task LoginAsync(string username, string password); + /// + /// Check if a member is logged in + /// + /// True if logged in, false if not. bool IsLoggedIn(); /// From 09cfe5f0554461eb96bba90ff37cd3a1ab015c0d Mon Sep 17 00:00:00 2001 From: Andy Butland Date: Sun, 22 Nov 2020 09:51:57 +0100 Subject: [PATCH 5/5] Fixed typo in class name. --- ...nsions.cs => UmbracoWebsiteServiceCollectionExtensions.cs} | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) rename src/Umbraco.Web.Website/Extensions/{UmbracoWebstiteServiceCollectionExtensions.cs => UmbracoWebsiteServiceCollectionExtensions.cs} (89%) diff --git a/src/Umbraco.Web.Website/Extensions/UmbracoWebstiteServiceCollectionExtensions.cs b/src/Umbraco.Web.Website/Extensions/UmbracoWebsiteServiceCollectionExtensions.cs similarity index 89% rename from src/Umbraco.Web.Website/Extensions/UmbracoWebstiteServiceCollectionExtensions.cs rename to src/Umbraco.Web.Website/Extensions/UmbracoWebsiteServiceCollectionExtensions.cs index 5737cb10300a..036d66128dff 100644 --- a/src/Umbraco.Web.Website/Extensions/UmbracoWebstiteServiceCollectionExtensions.cs +++ b/src/Umbraco.Web.Website/Extensions/UmbracoWebsiteServiceCollectionExtensions.cs @@ -1,13 +1,11 @@ using Microsoft.AspNetCore.Mvc; using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.Options; -using Umbraco.Core.Security; -using Umbraco.Web.Website.Security; using Umbraco.Web.Website.ViewEngines; namespace Umbraco.Extensions { - public static class UmbracoWebstiteServiceCollectionExtensions + public static class UmbracoWebsiteServiceCollectionExtensions { public static void AddUmbracoWebsite(this IServiceCollection services) {