From 70b45dbe2d61a20ef3f5505561da326407394f1f Mon Sep 17 00:00:00 2001 From: Hao Kung Date: Tue, 15 Apr 2014 11:03:18 -0700 Subject: [PATCH] AccountController identity cleanup Use SignInManager where appropriate Misc cleanup of controller actions --- .../Controllers/AccountController.cs | 147 ++++++++++++------ src/MusicStore/Startup.cs | 13 +- src/MusicStore/project.json | 1 + 3 files changed, 113 insertions(+), 48 deletions(-) diff --git a/src/MusicStore/Controllers/AccountController.cs b/src/MusicStore/Controllers/AccountController.cs index 83d1cc80ac..a7d717402b 100644 --- a/src/MusicStore/Controllers/AccountController.cs +++ b/src/MusicStore/Controllers/AccountController.cs @@ -1,5 +1,7 @@ -using Microsoft.AspNet.Abstractions.Security; +using System; +using System.Security.Claims; using Microsoft.AspNet.Identity; +using Microsoft.AspNet.Identity.Security; using Microsoft.AspNet.Mvc; using Microsoft.AspNet.Mvc.ModelBinding; using MusicStore.Models; @@ -14,19 +16,34 @@ namespace MusicStore.Controllers public AccountController() //Bug: Using an in memory store //: this(new UserManager(new UserStore(new ApplicationDbContext()))) - //: this(new UserManager(new InMemoryUserStore())) + : this(new UserManager(Startup.UserStore)) { } - //public AccountController(UserManager userManager) - //{ - // UserManager = userManager; - //} + public AccountController(UserManager userManager) + { + UserManager = userManager; + } - /// - /// TODO: Temporary ugly work around (making this static) to enable creating a static InMemory UserManager. Will go away shortly. - /// - public static UserManager UserManager { get; set; } + public UserManager UserManager { get; set; } + + private SignInManager _signInManager; + public SignInManager SignInManager { + get + { + if (_signInManager == null) + { + _signInManager = new SignInManager() + { + UserManager = UserManager, + Context = Context, + AuthenticationType = DefaultAuthenticationTypes.ApplicationCookie + }; + } + return _signInManager; + } + set { _signInManager = value; } + } // // GET: /Account/Login @@ -47,15 +64,18 @@ namespace MusicStore.Controllers { if (ModelState.IsValid == true) { - var user = await UserManager.FindByUserNamePasswordAsync(model.UserName, model.Password); - if (user != null) + var signInStatus = await SignInManager.PasswordSignInAsync(model.UserName, model.Password, model.RememberMe, shouldLockout: false); + switch (signInStatus) { - await SignIn(user, model.RememberMe); - return RedirectToLocal(returnUrl); - } - else - { - ModelState.AddModelError("", "Invalid username or password."); + case SignInStatus.Success: + return RedirectToLocal(returnUrl); + case SignInStatus.LockedOut: + ModelState.AddModelError("", "User is locked out, try again later."); + return View(model); + case SignInStatus.Failure: + default: + ModelState.AddModelError("", "Invalid username or password."); + return View(model); } } @@ -86,7 +106,7 @@ namespace MusicStore.Controllers var result = await UserManager.CreateAsync(user, model.Password); if (result.Succeeded) { - await SignIn(user, isPersistent: false); + await SignInManager.SignInAsync(user, isPersistent: false, rememberBrowser: false); //Bug: No helper methods //return RedirectToAction("Index", "Home"); return Redirect("/"); @@ -130,8 +150,8 @@ namespace MusicStore.Controllers { if (ModelState.IsValid == true) { - var user = new ApplicationUser() { UserName = this.Context.User.Identity.GetUserId() }; - IdentityResult result = await UserManager.ChangePasswordAsync(user, model.OldPassword, model.NewPassword); + var user = await GetCurrentUser(); + var result = await UserManager.ChangePasswordAsync(user, model.OldPassword, model.NewPassword); if (result.Succeeded) { //Bug: No helper method @@ -157,8 +177,8 @@ namespace MusicStore.Controllers if (ModelState.IsValid == true) { - var user = new ApplicationUser() { UserName = this.Context.User.Identity.GetUserId() }; - IdentityResult result = await UserManager.ChangePasswordAsync(user, model.OldPassword, model.NewPassword); + var user = await GetCurrentUser(); + var result = await UserManager.ChangePasswordAsync(user, model.OldPassword, model.NewPassword); if (result.Succeeded) { //Bug: No helper method @@ -181,32 +201,15 @@ namespace MusicStore.Controllers //[ValidateAntiForgeryToken] public IActionResult LogOff() { + // Bug: This should call SignInManager.SignOut() once its available this.Context.Response.SignOut(); //Bug: No helper //return RedirectToAction("Index", "Home"); return Redirect("/"); } - //Bug: Controllers need to be disposable? - protected void Dispose(bool disposing) - { - if (disposing && UserManager != null) - { - UserManager.Dispose(); - UserManager = null; - } - - //base.Dispose(disposing); - } - #region Helpers - private async Task SignIn(ApplicationUser user, bool isPersistent) - { - var identity = await UserManager.CreateIdentityAsync(user, DefaultAuthenticationTypes.ApplicationCookie); - this.Context.Response.SignIn(identity, new AuthenticationProperties() { IsPersistent = isPersistent }); - } - private void AddErrors(IdentityResult result) { foreach (var error in result.Errors) @@ -215,12 +218,17 @@ namespace MusicStore.Controllers } } + private async Task GetCurrentUser() + { + return await UserManager.FindByIdAsync(Context.User.Identity.GetUserId()); + } + private async Task HasPassword() { - var user = await UserManager.FindByNameAsync(this.Context.User.Identity.GetUserId()); + var user = await GetCurrentUser(); if (user != null) { - return user.PasswordHash != null; + return await UserManager.HasPasswordAsync(user); } return false; } @@ -255,9 +263,58 @@ namespace MusicStore.Controllers /// public static class Extensions { - public static string GetUserId(this IIdentity user) + /// + /// Return the user name using the UserNameClaimType + /// + /// + /// + public static string GetUserName(this IIdentity identity) { - return user.Name; + if (identity == null) + { + throw new ArgumentNullException("identity"); + } + var ci = identity as ClaimsIdentity; + if (ci != null) + { + return ci.FindFirstValue(ClaimsIdentity.DefaultNameClaimType); + } + return null; + } + + /// + /// Return the user id using the UserIdClaimType + /// + /// + /// + public static string GetUserId(this IIdentity identity) + { + if (identity == null) + { + throw new ArgumentNullException("identity"); + } + var ci = identity as ClaimsIdentity; + if (ci != null) + { + return ci.FindFirstValue(ClaimTypes.NameIdentifier); + } + return null; + } + + /// + /// Return the claim value for the first claim with the specified type if it exists, null otherwise + /// + /// + /// + /// + public static string FindFirstValue(this ClaimsIdentity identity, string claimType) + { + if (identity == null) + { + throw new ArgumentNullException("identity"); + } + var claim = identity.FindFirst(claimType); + return claim != null ? claim.Value : null; } } diff --git a/src/MusicStore/Startup.cs b/src/MusicStore/Startup.cs index 560b1188a6..6a4ed6a2c2 100644 --- a/src/MusicStore/Startup.cs +++ b/src/MusicStore/Startup.cs @@ -13,6 +13,7 @@ using Microsoft.AspNet.RequestContainer; using Microsoft.AspNet.Routing; using Microsoft.AspNet.Security.Cookies; using Microsoft.Net.Runtime; +using MusicStore.Controllers; using MusicStore.Logging; using MusicStore.Models; using MusicStore.Web.Models; @@ -21,6 +22,12 @@ using System.IO; public class Startup { + /// + /// TODO: Temporary ugly work around (making this static) to enable creating a static InMemory UserManager. Will go away shortly. + /// + public static InMemoryUserStore UserStore = new InMemoryUserStore(); + public static InMemoryRoleStore RoleStore = new InMemoryRoleStore(); + public void Configuration(IBuilder app) { CreateAdminUser(app.ServiceProvider); @@ -37,7 +44,7 @@ public class Startup app.UseCookieAuthentication(new CookieAuthenticationOptions() { - AuthenticationType = "Application", + AuthenticationType = DefaultAuthenticationTypes.ApplicationCookie, LoginPath = new PathString("/Account/Login") }); @@ -71,8 +78,8 @@ public class Startup string _password = configuration.Get("DefaultAdminPassword"); string _role = "Administrator"; - var userManager = new UserManager(new InMemoryUserStore()); - var roleManager = new RoleManager(new InMemoryRoleStore()); + var userManager = new UserManager(UserStore); + var roleManager = new RoleManager(RoleStore); var role = new IdentityRole(_role); var result = await roleManager.RoleExistsAsync(_role); diff --git a/src/MusicStore/project.json b/src/MusicStore/project.json index 11391916f2..0f062809fc 100644 --- a/src/MusicStore/project.json +++ b/src/MusicStore/project.json @@ -19,6 +19,7 @@ "Microsoft.AspNet.Identity": "0.1-alpha-*", "Microsoft.AspNet.Identity.Entity": "0.1-alpha-*", "Microsoft.AspNet.Identity.InMemory": "0.1-alpha-*", + "Microsoft.AspNet.Identity.Security": "0.1-alpha-*", "Microsoft.Data.Entity": "0.1-alpha-*", "Microsoft.Data.Relational": "0.1-alpha-*", "Microsoft.Data.SqlServer": "0.1-pre-*",