From af66fe161186373c50ab9ad1547bb7f022f32d41 Mon Sep 17 00:00:00 2001 From: Stephen Halter Date: Tue, 10 Jun 2014 16:37:49 -0700 Subject: [PATCH] Inject all dependencies directly into RoleManager and UserManager - All dependencies for RoleManager and UserManager should be required - The old calls to GetService inside the constructors will throw instead of returning null if left in after PR aspnet/DependencyInjection#87 is merged --- src/Microsoft.AspNet.Identity/RoleManager.cs | 10 ++--- src/Microsoft.AspNet.Identity/UserManager.cs | 37 ++++++++++++++----- .../InMemoryUserStoreTest.cs | 2 +- .../RoleStoreTest.cs | 1 + .../InMemoryStoreTest.cs | 9 +---- .../RoleManagerTest.cs | 19 ++++++---- .../RoleValidatorTest.cs | 4 +- .../UserManagerTest.cs | 35 +++++++++++++----- test/Shared/IdentityConfig.cs | 8 +++- test/Shared/MockHelpers.cs | 5 +-- 10 files changed, 83 insertions(+), 47 deletions(-) diff --git a/src/Microsoft.AspNet.Identity/RoleManager.cs b/src/Microsoft.AspNet.Identity/RoleManager.cs index 9a5f4790b6..9d0b338294 100644 --- a/src/Microsoft.AspNet.Identity/RoleManager.cs +++ b/src/Microsoft.AspNet.Identity/RoleManager.cs @@ -21,19 +21,19 @@ namespace Microsoft.AspNet.Identity /// /// Constructor /// - /// /// The IRoleStore is responsible for commiting changes via the UpdateAsync/CreateAsync methods - public RoleManager(IServiceProvider services, IRoleStore store) + /// + public RoleManager(IRoleStore store, IRoleValidator roleValidator) { if (store == null) { throw new ArgumentNullException("store"); } - if (services == null) + if (roleValidator == null) { - throw new ArgumentNullException("services"); + throw new ArgumentNullException("roleValidator"); } - RoleValidator = services.GetService>() ?? new RoleValidator(); + RoleValidator = roleValidator; Store = store; } diff --git a/src/Microsoft.AspNet.Identity/UserManager.cs b/src/Microsoft.AspNet.Identity/UserManager.cs index 91289e0b61..cac492f9cb 100644 --- a/src/Microsoft.AspNet.Identity/UserManager.cs +++ b/src/Microsoft.AspNet.Identity/UserManager.cs @@ -32,15 +32,16 @@ namespace Microsoft.AspNet.Identity /// /// Constructor which takes a service provider and user store /// - /// /// /// - public UserManager(IServiceProvider serviceProvider, IUserStore store, IOptionsAccessor optionsAccessor) + /// + /// + /// + /// + public UserManager(IUserStore store, IOptionsAccessor optionsAccessor, + IPasswordHasher passwordHasher, IUserValidator userValidator, + IPasswordValidator passwordValidator, IClaimsIdentityFactory claimsIdentityFactory) { - if (serviceProvider == null) - { - throw new ArgumentNullException("serviceProvider"); - } if (store == null) { throw new ArgumentNullException("store"); @@ -49,12 +50,28 @@ namespace Microsoft.AspNet.Identity { throw new ArgumentNullException("optionsAccessor"); } + if (passwordHasher == null) + { + throw new ArgumentNullException("passwordHasher"); + } + if (userValidator == null) + { + throw new ArgumentNullException("userValidator"); + } + if (passwordValidator == null) + { + throw new ArgumentNullException("passwordValidator"); + } + if (claimsIdentityFactory == null) + { + throw new ArgumentNullException("claimsIdentityFactory"); + } Store = store; Options = optionsAccessor.Options; - PasswordHasher = serviceProvider.GetService() ?? new PasswordHasher(); - UserValidator = serviceProvider.GetService>() ?? new UserValidator(); - PasswordValidator = serviceProvider.GetService>() ?? new PasswordValidator(); - ClaimsIdentityFactory = serviceProvider.GetService>() ?? new ClaimsIdentityFactory(); + PasswordHasher = passwordHasher; + UserValidator = userValidator; + PasswordValidator = passwordValidator; + ClaimsIdentityFactory = claimsIdentityFactory; // TODO: Email/Sms/Token services } diff --git a/test/Microsoft.AspNet.Identity.Entity.Test/InMemoryUserStoreTest.cs b/test/Microsoft.AspNet.Identity.Entity.Test/InMemoryUserStoreTest.cs index 356a91f0ed..b8e332a3e1 100644 --- a/test/Microsoft.AspNet.Identity.Entity.Test/InMemoryUserStoreTest.cs +++ b/test/Microsoft.AspNet.Identity.Entity.Test/InMemoryUserStoreTest.cs @@ -25,10 +25,10 @@ namespace Microsoft.AspNet.Identity.Entity.Test { var services = new ServiceCollection(); services.AddEntityFramework().AddInMemoryStore(); + services.AddIdentity(); services.AddSingleton, OptionsAccessor>(); services.AddInstance(new IdentityContext()); services.AddTransient, InMemoryUserStore>(); - services.AddSingleton>(); var provider = services.BuildServiceProvider(); var manager = provider.GetService>(); Assert.NotNull(manager); diff --git a/test/Microsoft.AspNet.Identity.Entity.Test/RoleStoreTest.cs b/test/Microsoft.AspNet.Identity.Entity.Test/RoleStoreTest.cs index 119f4724f2..1819ca812f 100644 --- a/test/Microsoft.AspNet.Identity.Entity.Test/RoleStoreTest.cs +++ b/test/Microsoft.AspNet.Identity.Entity.Test/RoleStoreTest.cs @@ -38,6 +38,7 @@ namespace Microsoft.AspNet.Identity.Entity.Test services.AddEntityFramework().AddInMemoryStore(); services.AddTransient(); services.AddTransient, EntityRoleStore>(); + services.AddTransient, RoleValidator>(); services.AddSingleton>(); var provider = services.BuildServiceProvider(); var manager = provider.GetService>(); diff --git a/test/Microsoft.AspNet.Identity.InMemory.Test/InMemoryStoreTest.cs b/test/Microsoft.AspNet.Identity.InMemory.Test/InMemoryStoreTest.cs index 6c259ad15d..a5fa6a4d03 100644 --- a/test/Microsoft.AspNet.Identity.InMemory.Test/InMemoryStoreTest.cs +++ b/test/Microsoft.AspNet.Identity.InMemory.Test/InMemoryStoreTest.cs @@ -14,10 +14,7 @@ namespace Microsoft.AspNet.Identity.InMemory.Test { var services = new ServiceCollection(); services.Add(OptionsServices.GetDefaultServices()); - services.AddTransient, UserValidator>(); - services.AddTransient, PasswordValidator>(); - services.AddSingleton, InMemoryUserStore>(); - services.AddSingleton>(); + services.AddIdentity().AddInMemory(); services.SetupOptions(options => { options.Password.RequireDigit = false; @@ -32,9 +29,7 @@ namespace Microsoft.AspNet.Identity.InMemory.Test protected override RoleManager CreateRoleManager() { var services = new ServiceCollection(); - services.AddTransient, RoleValidator>(); - services.AddInstance>(new InMemoryRoleStore()); - services.AddSingleton>(); + services.AddIdentity().AddInMemory(); return services.BuildServiceProvider().GetService>(); } } diff --git a/test/Microsoft.AspNet.Identity.Test/RoleManagerTest.cs b/test/Microsoft.AspNet.Identity.Test/RoleManagerTest.cs index 8cf9769a29..44de5a5727 100644 --- a/test/Microsoft.AspNet.Identity.Test/RoleManagerTest.cs +++ b/test/Microsoft.AspNet.Identity.Test/RoleManagerTest.cs @@ -16,7 +16,7 @@ namespace Microsoft.AspNet.Identity.Test [Fact] public void RolesQueryableFailWhenStoreNotImplemented() { - var manager = new RoleManager(new ServiceCollection().BuildServiceProvider(), new NoopRoleStore()); + var manager = CreateRoleManager(new NoopRoleStore()); Assert.False(manager.SupportsQueryableRoles); Assert.Throws(() => manager.Roles.Count()); } @@ -24,7 +24,7 @@ namespace Microsoft.AspNet.Identity.Test [Fact] public void DisposeAfterDisposeDoesNotThrow() { - var manager = new RoleManager(new ServiceCollection().BuildServiceProvider(), new NoopRoleStore()); + var manager = CreateRoleManager(new NoopRoleStore()); manager.Dispose(); manager.Dispose(); } @@ -34,10 +34,10 @@ namespace Microsoft.AspNet.Identity.Test { var provider = new ServiceCollection().BuildServiceProvider(); Assert.Throws("store", - () => new RoleManager(provider, null)); - Assert.Throws("services", - () => new RoleManager(null, new NotImplementedStore())); - var manager = new RoleManager(provider, new NotImplementedStore()); + () => new RoleManager(null, new RoleValidator())); + Assert.Throws("roleValidator", + () => new RoleManager(new NotImplementedStore(), null)); + var manager = CreateRoleManager(new NotImplementedStore()); await Assert.ThrowsAsync("role", async () => await manager.CreateAsync(null)); await Assert.ThrowsAsync("role", async () => await manager.UpdateAsync(null)); await Assert.ThrowsAsync("role", async () => await manager.DeleteAsync(null)); @@ -48,7 +48,7 @@ namespace Microsoft.AspNet.Identity.Test [Fact] public async Task RoleStoreMethodsThrowWhenDisposed() { - var manager = new RoleManager(new ServiceCollection().BuildServiceProvider(), new NoopRoleStore()); + var manager = CreateRoleManager(new NoopRoleStore()); manager.Dispose(); await Assert.ThrowsAsync(() => manager.FindByIdAsync(null)); await Assert.ThrowsAsync(() => manager.FindByNameAsync(null)); @@ -58,6 +58,11 @@ namespace Microsoft.AspNet.Identity.Test await Assert.ThrowsAsync(() => manager.DeleteAsync(null)); } + private static RoleManager CreateRoleManager(IRoleStore roleStore) + { + return new RoleManager(roleStore, new RoleValidator()); + } + private class NotImplementedStore : IRoleStore { public Task CreateAsync(TestRole role, CancellationToken cancellationToken = default(CancellationToken)) diff --git a/test/Microsoft.AspNet.Identity.Test/RoleValidatorTest.cs b/test/Microsoft.AspNet.Identity.Test/RoleValidatorTest.cs index db572b8835..809dd4c31d 100644 --- a/test/Microsoft.AspNet.Identity.Test/RoleValidatorTest.cs +++ b/test/Microsoft.AspNet.Identity.Test/RoleValidatorTest.cs @@ -15,8 +15,8 @@ namespace Microsoft.AspNet.Identity.Test public async Task ValidateThrowsWithNull() { // Setup - var manager = new RoleManager(new ServiceCollection().BuildServiceProvider(), new NoopRoleStore()); var validator = new RoleValidator(); + var manager = new RoleManager(new NoopRoleStore(), validator); // Act // Assert @@ -30,8 +30,8 @@ namespace Microsoft.AspNet.Identity.Test public async Task ValidateFailsWithTooShortRoleName(string input) { // Setup - var manager = new RoleManager(new ServiceCollection().BuildServiceProvider(), new NoopRoleStore()); var validator = new RoleValidator(); + var manager = new RoleManager(new NoopRoleStore(), validator); var user = new TestRole {Name = input}; // Act diff --git a/test/Microsoft.AspNet.Identity.Test/UserManagerTest.cs b/test/Microsoft.AspNet.Identity.Test/UserManagerTest.cs index a6e9b6ada2..f6c549ce73 100644 --- a/test/Microsoft.AspNet.Identity.Test/UserManagerTest.cs +++ b/test/Microsoft.AspNet.Identity.Test/UserManagerTest.cs @@ -21,7 +21,10 @@ namespace Microsoft.AspNet.Identity.Test { public IUserStore StorePublic { get { return Store; } } - public TestManager(IServiceProvider provider, IUserStore store, IOptionsAccessor options) : base(provider, store, options) { } + public TestManager(IUserStore store, IOptionsAccessor optionsAccessor, + IPasswordHasher passwordHasher, IUserValidator userValidator, + IPasswordValidator passwordValidator, IClaimsIdentityFactory claimsIdentityFactory) + : base(store, optionsAccessor, passwordHasher, userValidator, passwordValidator, claimsIdentityFactory) { } } [Fact] @@ -47,7 +50,6 @@ namespace Microsoft.AspNet.Identity.Test // Setup var store = new Mock>(); var user = new TestUser { UserName = "Foo" }; - var options = new OptionsAccessor(null); store.Setup(s => s.CreateAsync(user, CancellationToken.None)).Returns(Task.FromResult(0)).Verifiable(); var userManager = MockHelpers.TestUserManager(store.Object); @@ -65,9 +67,8 @@ namespace Microsoft.AspNet.Identity.Test // Setup var store = new Mock>(); var user = new TestUser { UserName = "Foo" }; - var options = new OptionsAccessor(null); store.Setup(s => s.DeleteAsync(user, CancellationToken.None)).Returns(Task.FromResult(0)).Verifiable(); - var userManager = new UserManager(new ServiceCollection().BuildServiceProvider(), store.Object, options); + var userManager = MockHelpers.TestUserManager(store.Object); // Act var result = await userManager.DeleteAsync(user); @@ -420,14 +421,28 @@ namespace Microsoft.AspNet.Identity.Test [Fact] public async Task ManagerPublicNullChecks() { - var provider = new ServiceCollection().BuildServiceProvider(); - Assert.Throws("serviceProvider", - () => new UserManager(null, null, null)); + var store = new NotImplementedStore(); + var optionsAccessor = new OptionsAccessor(null); + var passwordHasher = new PasswordHasher(); + var userValidator = new UserValidator(); + var passwordValidator = new PasswordValidator(); + var claimsIdentityFactory = new ClaimsIdentityFactory(); + Assert.Throws("store", - () => new UserManager(provider, null, null)); + () => new UserManager(null, null, null, null, null, null)); Assert.Throws("optionsAccessor", - () => new UserManager(provider, new NotImplementedStore(), null)); - var manager = new UserManager(provider, new NotImplementedStore(), new OptionsAccessor(null)); + () => new UserManager(store, null, null, null, null, null)); + Assert.Throws("passwordHasher", + () => new UserManager(store, optionsAccessor, null, null, null, null)); + Assert.Throws("userValidator", + () => new UserManager(store, optionsAccessor, passwordHasher, null, null, null)); + Assert.Throws("passwordValidator", + () => new UserManager(store, optionsAccessor, passwordHasher, userValidator, null, null)); + Assert.Throws("claimsIdentityFactory", + () => new UserManager(store, optionsAccessor, passwordHasher, userValidator, passwordValidator, null)); + + var manager = new UserManager(store, optionsAccessor, passwordHasher, userValidator, passwordValidator, claimsIdentityFactory); + Assert.Throws("value", () => manager.ClaimsIdentityFactory = null); Assert.Throws("value", () => manager.PasswordHasher = null); Assert.Throws("value", () => manager.Options = null); diff --git a/test/Shared/IdentityConfig.cs b/test/Shared/IdentityConfig.cs index 8658131af8..d5efb8ed40 100644 --- a/test/Shared/IdentityConfig.cs +++ b/test/Shared/IdentityConfig.cs @@ -8,12 +8,16 @@ namespace Microsoft.AspNet.Identity.Test { public class ApplicationUserManager : UserManager { - public ApplicationUserManager(IServiceProvider services, IUserStore store, IOptionsAccessor options) : base(services, store, options) { } + public ApplicationUserManager(IUserStore store, IOptionsAccessor options, + IPasswordHasher passwordHasher, IUserValidator userValidator, + IPasswordValidator passwordValidator, IClaimsIdentityFactory claimsIdentityFactory) + : base(store, options, passwordHasher, userValidator, passwordValidator, claimsIdentityFactory) { } } public class ApplicationRoleManager : RoleManager { - public ApplicationRoleManager(IServiceProvider services, IRoleStore store) : base(services, store) { } + public ApplicationRoleManager(IRoleStore store, IRoleValidator roleValidator) + : base(store, roleValidator) { } } public class ApplicationUser : IdentityUser diff --git a/test/Shared/MockHelpers.cs b/test/Shared/MockHelpers.cs index c6e85bba19..f7f353d70d 100644 --- a/test/Shared/MockHelpers.cs +++ b/test/Shared/MockHelpers.cs @@ -33,7 +33,7 @@ namespace Microsoft.AspNet.Identity.Test { var store = new Mock>(); var options = new OptionsAccessor(null); - return new Mock>(new ServiceCollection().BuildServiceProvider(), store.Object, options); + return new Mock>(store.Object, options, new PasswordHasher(), new UserValidator(), new PasswordValidator(), new ClaimsIdentityFactory()); } public static UserManager TestUserManager() where TUser : class @@ -45,9 +45,8 @@ namespace Microsoft.AspNet.Identity.Test { var options = new OptionsAccessor(null); var validator = new Mock>(); - var userManager = new UserManager(new ServiceCollection().BuildServiceProvider(), store, options); + var userManager = new UserManager(store, options, new PasswordHasher(), validator.Object, new PasswordValidator(), new ClaimsIdentityFactory()); validator.Setup(v => v.ValidateAsync(userManager, It.IsAny(), CancellationToken.None)).Returns(Task.FromResult(IdentityResult.Success)).Verifiable(); - userManager.UserValidator = validator.Object; return userManager; } }