API Review cleanup #1

- Remove constructor defaults
- Make service properties internal
- Add Logging/HttpContextAccessor services which are required by
identity

src/Microsoft.AspNet.Identity/IdentityServiceCollectionExtensions.cs

@@ -2,6 +2,7 @@
 // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
 
 using System;
+using Microsoft.AspNet.Hosting;
 using Microsoft.AspNet.Http;
 using Microsoft.AspNet.Identity;
 using Microsoft.AspNet.Security;
@@ -52,6 +53,8 @@ namespace Microsoft.Framework.DependencyInjection
             // Services used by identity
             services.AddOptions(identityConfig);
             services.AddDataProtection(identityConfig);
+            services.AddLogging(identityConfig);
+            services.TryAdd(describe.Singleton<IHttpContextAccessor, HttpContextAccessor>());
 
             // Identity services
             services.TryAdd(describe.Transient<IUserValidator<TUser>, UserValidator<TUser>>());

src/Microsoft.AspNet.Identity/Properties/AssemblyInfo.cs

@@ -3,4 +3,7 @@
 
 using System.Runtime.CompilerServices;
 
-[assembly: InternalsVisibleTo("Microsoft.AspNet.Identity.Test")]
+[assembly: InternalsVisibleTo("Microsoft.AspNet.Identity.Test")]
+[assembly: InternalsVisibleTo("Microsoft.AspNet.Identity.EntityFramework.Test")]
+[assembly: InternalsVisibleTo("Microsoft.AspNet.Identity.EntityFramework.InMemory.Test")]
+[assembly: InternalsVisibleTo("Microsoft.AspNet.Identity.InMemory.Test")]

src/Microsoft.AspNet.Identity/RoleManager.cs

@@ -28,11 +28,11 @@ namespace Microsoft.AspNet.Identity
         /// <param name="store">The IRoleStore commits changes via the UpdateAsync/CreateAsync methods</param>
         /// <param name="roleValidator"></param>
         public RoleManager(IRoleStore<TRole> store,
-            IEnumerable<IRoleValidator<TRole>> roleValidators = null,
+            IEnumerable<IRoleValidator<TRole>> roleValidators,
-            ILookupNormalizer keyNormalizer = null,
+            ILookupNormalizer keyNormalizer,
-            IdentityErrorDescriber errors = null,
+            IdentityErrorDescriber errors,
-            ILogger<RoleManager<TRole>> logger = null,
+            ILogger<RoleManager<TRole>> logger,
-            IHttpContextAccessor contextAccessor = null)
+            IHttpContextAccessor contextAccessor)
         {
             if (store == null)
             {
@@ -62,22 +62,22 @@ namespace Microsoft.AspNet.Identity
         /// <summary>
         ///     Used to validate roles before persisting changes
         /// </summary>
-        public IList<IRoleValidator<TRole>> RoleValidators { get; } = new List<IRoleValidator<TRole>>();
+        internal IList<IRoleValidator<TRole>> RoleValidators { get; } = new List<IRoleValidator<TRole>>();
 
         /// <summary>
         ///     Used to generate public API error messages
         /// </summary>
-        public IdentityErrorDescriber ErrorDescriber { get; set; }
+        internal IdentityErrorDescriber ErrorDescriber { get; set; }
 
         /// <summary>
         ///     Used to log results
         /// </summary>
-        public ILogger<RoleManager<TRole>> Logger { get; set; }
+        internal ILogger<RoleManager<TRole>> Logger { get; set; }
 
         /// <summary>
         ///     Used to normalize user names, role names, emails for uniqueness
         /// </summary>
-        public ILookupNormalizer KeyNormalizer { get; set; }
+        internal ILookupNormalizer KeyNormalizer { get; set; }
 
         /// <summary>
         ///     Returns an IQueryable of roles if the store is an IQueryableRoleStore

src/Microsoft.AspNet.Identity/UserManager.cs

@@ -29,8 +29,6 @@ namespace Microsoft.AspNet.Identity
 
         private TimeSpan _defaultLockout = TimeSpan.Zero;
         private bool _disposed;
-        private IPasswordHasher<TUser> _passwordHasher;
-        private IdentityOptions _options;
         private HttpContext _context;
 
         /// <summary>
@@ -47,16 +45,16 @@ namespace Microsoft.AspNet.Identity
         /// <param name="msgProviders"></param>
         /// <param name="loggerFactory"></param>
         public UserManager(IUserStore<TUser> store,
-            IOptions<IdentityOptions> optionsAccessor = null,
+            IOptions<IdentityOptions> optionsAccessor,
-            IPasswordHasher<TUser> passwordHasher = null,
+            IPasswordHasher<TUser> passwordHasher,
-            IEnumerable<IUserValidator<TUser>> userValidators = null,
+            IEnumerable<IUserValidator<TUser>> userValidators,
-            IEnumerable<IPasswordValidator<TUser>> passwordValidators = null,
+            IEnumerable<IPasswordValidator<TUser>> passwordValidators,
-            ILookupNormalizer keyNormalizer = null,
+            ILookupNormalizer keyNormalizer,
-            IdentityErrorDescriber errors = null,
+            IdentityErrorDescriber errors,
-            IEnumerable<IUserTokenProvider<TUser>> tokenProviders = null,
+            IEnumerable<IUserTokenProvider<TUser>> tokenProviders,
-            IEnumerable<IIdentityMessageProvider> msgProviders = null,
+            IEnumerable<IIdentityMessageProvider> msgProviders,
-            ILogger<UserManager<TUser>> logger = null,
+            ILogger<UserManager<TUser>> logger,
-            IHttpContextAccessor contextAccessor = null)
+            IHttpContextAccessor contextAccessor)
         {
             if (store == null)
             {
@@ -64,10 +62,10 @@ namespace Microsoft.AspNet.Identity
             }
             Store = store;
             Options = optionsAccessor?.Options ?? new IdentityOptions();
-            PasswordHasher = passwordHasher ?? new PasswordHasher<TUser>();
-            KeyNormalizer = keyNormalizer ?? new UpperInvariantLookupNormalizer();
-            ErrorDescriber = errors ?? new IdentityErrorDescriber();
             _context = contextAccessor?.Value;
+            PasswordHasher = passwordHasher;
+            KeyNormalizer = keyNormalizer;
+            ErrorDescriber = errors;
 
             if (userValidators != null)
             {
@@ -93,7 +91,6 @@ namespace Microsoft.AspNet.Identity
                     RegisterTokenProvider(tokenProvider);
                 }
             }
-
             if (msgProviders != null)
             {
                 foreach (var msgProvider in msgProviders)
@@ -101,7 +98,6 @@ namespace Microsoft.AspNet.Identity
                     RegisterMessageProvider(msgProvider);
                 }
             }
-
         }
 
         /// <summary>
@@ -109,69 +105,19 @@ namespace Microsoft.AspNet.Identity
         /// </summary>
         protected internal IUserStore<TUser> Store { get; set; }
 
-        /// <summary>
+        internal IPasswordHasher<TUser> PasswordHasher { get; set; }
-        ///     Used to hash/verify passwords
-        /// </summary>
-        public IPasswordHasher<TUser> PasswordHasher
-        {
-            get
-            {
-                ThrowIfDisposed();
-                return _passwordHasher;
-            }
-            set
-            {
-                ThrowIfDisposed();
-                if (value == null)
-                {
-                    throw new ArgumentNullException("value");
-                }
-                _passwordHasher = value;
-            }
-        }
 
-        /// <summary>
+        internal IList<IUserValidator<TUser>> UserValidators { get; } = new List<IUserValidator<TUser>>();
-        ///     Used to validate users before persisting changes
-        /// </summary>
-        public IList<IUserValidator<TUser>> UserValidators { get; } = new List<IUserValidator<TUser>>();
 
-        /// <summary>
+        internal IList<IPasswordValidator<TUser>> PasswordValidators { get; } = new List<IPasswordValidator<TUser>>();
-        ///     Used to validate passwords before persisting changes
-        /// </summary>
-        public IList<IPasswordValidator<TUser>> PasswordValidators { get; } = new List<IPasswordValidator<TUser>>();
 
-        /// <summary>
+        internal ILookupNormalizer KeyNormalizer { get; set; }
-        ///     Used to normalize user names and emails for uniqueness
-        /// </summary>
-        public ILookupNormalizer KeyNormalizer { get; set; }
 
-        /// <summary>
+        internal IdentityErrorDescriber ErrorDescriber { get; set; }
-        ///     Used to generate public API error messages
-        /// </summary>
-        public IdentityErrorDescriber ErrorDescriber { get; set; }
 
-        /// <summary>
+        internal ILogger<UserManager<TUser>> Logger { get; set; }
-        ///     Used to log IdentityResult
-        /// </summary>
-        public ILogger<UserManager<TUser>> Logger { get; set; }
 
-        public IdentityOptions Options
+        internal IdentityOptions Options { get; set; }
-        {
-            get
-            {
-                ThrowIfDisposed();
-                return _options;
-            }
-            set
-            {
-                ThrowIfDisposed();
-                if (value == null)
-                {
-                    throw new ArgumentNullException("value");
-                }
-                _options = value;
-            }
-        }
 
         /// <summary>
         ///     Returns true if the store is an IUserTwoFactorStore

test/Microsoft.AspNet.Identity.EntityFramework.InMemory.Test/RoleStoreTest.cs

@@ -19,8 +19,8 @@ namespace Microsoft.AspNet.Identity.EntityFramework.InMemory.Test
             var services = new ServiceCollection();
             services.AddEntityFramework().AddInMemoryStore();
             var store = new RoleStore<IdentityRole>(new InMemoryContext());
-            services.AddIdentity();
             services.AddInstance<IRoleStore<IdentityRole>>(store);
+            services.AddIdentity();
             var provider = services.BuildServiceProvider();
             var manager = provider.GetRequiredService<RoleManager<IdentityRole>>();
             Assert.NotNull(manager);
@@ -33,7 +33,7 @@ namespace Microsoft.AspNet.Identity.EntityFramework.InMemory.Test
             services.AddEntityFramework().AddInMemoryStore();
             services.AddTransient<InMemoryContext>();
             services.AddTransient<IRoleStore<IdentityRole>, RoleStore<IdentityRole, InMemoryContext>>();
-            services.AddTransient<IRoleValidator<IdentityRole>, RoleValidator<IdentityRole>>();
+            services.AddIdentity();
             services.AddSingleton<RoleManager<IdentityRole>>();
             var provider = services.BuildServiceProvider();
             var manager = provider.GetRequiredService<RoleManager<IdentityRole>>();

test/Microsoft.AspNet.Identity.Test/IdentityBuilderTest.cs

@@ -237,13 +237,13 @@ namespace Microsoft.AspNet.Identity.Test
 
         private class MyUserManager : UserManager<TestUser>
         {
-            public MyUserManager(IUserStore<TestUser> store) : base(store) { }
+            public MyUserManager(IUserStore<TestUser> store) : base(store, null, null, null, null, null, null, null, null, null, null) { }
         }
 
         private class MyRoleManager : RoleManager<TestRole>
         {
             public MyRoleManager(IRoleStore<TestRole> store,
-                IEnumerable<IRoleValidator<TestRole>> roleValidators) : base(store)
+                IEnumerable<IRoleValidator<TestRole>> roleValidators) : base(store, null, null, null, null, null)
             {
 
             }

test/Microsoft.AspNet.Identity.Test/RoleManagerTest.cs

@@ -124,7 +124,7 @@ namespace Microsoft.AspNet.Identity.Test
         public async Task RoleManagerPublicNullChecks()
         {
             Assert.Throws<ArgumentNullException>("store",
-                () => new RoleManager<TestRole>(null, null, null));
+                () => new RoleManager<TestRole>(null, null, null, null, null, null));
             var manager = CreateRoleManager(new NotImplementedStore());
             await Assert.ThrowsAsync<ArgumentNullException>("role", async () => await manager.CreateAsync(null));
             await Assert.ThrowsAsync<ArgumentNullException>("role", async () => await manager.UpdateAsync(null));
@@ -148,9 +148,7 @@ namespace Microsoft.AspNet.Identity.Test
 
         private static RoleManager<TestRole> CreateRoleManager(IRoleStore<TestRole> roleStore)
         {
-            var v = new List<IRoleValidator<TestRole>>();
+            return MockHelpers.TestRoleManager(roleStore);
-            v.Add(new RoleValidator<TestRole>());
-            return new RoleManager<TestRole>(roleStore);
         }
 
         private class NotImplementedStore : IRoleStore<TestRole>

test/Microsoft.AspNet.Identity.Test/RoleValidatorTest.cs

@@ -14,7 +14,7 @@ namespace Microsoft.AspNet.Identity.Test
         {
             // Setup
             var validator = new RoleValidator<TestRole>();
-            var manager = new RoleManager<TestRole>(new NoopRoleStore());
+            var manager = MockHelpers.TestRoleManager<TestRole>();
 
             // Act
             // Assert
@@ -29,7 +29,7 @@ namespace Microsoft.AspNet.Identity.Test
         {
             // Setup
             var validator = new RoleValidator<TestRole>();
-            var manager = new RoleManager<TestRole>(new NoopRoleStore());
+            var manager = MockHelpers.TestRoleManager<TestRole>();
             var user = new TestRole {Name = input};
 
             // Act

test/Microsoft.AspNet.Identity.Test/UserManagerTest.cs

@@ -16,23 +16,15 @@ namespace Microsoft.AspNet.Identity.Test
 {
     public class UserManagerTest
     {
-        private class TestManager : UserManager<TestUser>
-        {
-            public IUserStore<TestUser> StorePublic { get { return Store; } }
-
-            public TestManager(IUserStore<TestUser> store) : base(store) { }
-        }
-
         [Fact]
         public void EnsureDefaultServicesDefaultsWithStoreWorks()
         {
             var services = new ServiceCollection()
-                    .AddTransient<IUserStore<TestUser>, NoopUserStore>()
+                    .AddTransient<IUserStore<TestUser>, NoopUserStore>();
-                    .AddTransient<TestManager>();
             services.AddIdentity<TestUser, IdentityRole>();
-            var manager = services.BuildServiceProvider().GetRequiredService<TestManager>();
+            var manager = services.BuildServiceProvider().GetRequiredService<UserManager<TestUser>>();
             Assert.NotNull(manager.PasswordHasher);
-            Assert.NotNull(manager.StorePublic);
+            Assert.NotNull(manager.Store);
             Assert.NotNull(manager.Options);
         }
 
@@ -639,15 +631,11 @@ namespace Microsoft.AspNet.Identity.Test
         [Fact]
         public async Task ManagerPublicNullChecks()
         {
-            var store = new NotImplementedStore();
-
             Assert.Throws<ArgumentNullException>("store",
-                () => new UserManager<TestUser>(null, null));
+                () => new UserManager<TestUser>(null, null, null, null, null, null, null, null, null, null, null));
 
-            var manager = new UserManager<TestUser>(store);
+            var manager = MockHelpers.TestUserManager(new NotImplementedStore());
 
-            Assert.Throws<ArgumentNullException>("value", () => manager.PasswordHasher = null);
-            Assert.Throws<ArgumentNullException>("value", () => manager.Options = null);
             await Assert.ThrowsAsync<ArgumentNullException>("user", async () => await manager.CreateAsync(null));
             await Assert.ThrowsAsync<ArgumentNullException>("user", async () => await manager.CreateAsync(null, null));
             await

test/Shared/MockHelpers.cs

@@ -8,6 +8,9 @@ using System.Threading.Tasks;
 using System.Text;
 using Microsoft.Framework.Logging;
 using Moq;
+using Microsoft.Framework.OptionsModel;
+using System.Linq;
+using Microsoft.AspNet.Hosting;
 
 namespace Microsoft.AspNet.Identity.Test
 {
@@ -49,10 +52,20 @@ namespace Microsoft.AspNet.Identity.Test
         public static UserManager<TUser> TestUserManager<TUser>(IUserStore<TUser> store = null) where TUser : class
         {
             store = store ?? new Mock<IUserStore<TUser>>().Object;
+            var options = new Mock<IOptions<IdentityOptions>>();
+            var idOptions = new IdentityOptions();
+            options.Setup(o => o.Options).Returns(idOptions);
+            var userValidators = new List<IUserValidator<TUser>>();
             var validator = new Mock<IUserValidator<TUser>>();
-            var userManager = new UserManager<TUser>(store);
+            userValidators.Add(validator.Object);
-            userManager.UserValidators.Add(validator.Object);
+            var pwdValidators = new List<PasswordValidator<TUser>>();
-            userManager.PasswordValidators.Add(new PasswordValidator<TUser>());
+            pwdValidators.Add(new PasswordValidator<TUser>());
+            var userManager = new UserManager<TUser>(store, options.Object, new PasswordHasher<TUser>(),
+                userValidators, pwdValidators, new UpperInvariantLookupNormalizer(),
+                new IdentityErrorDescriber(), Enumerable.Empty<IUserTokenProvider<TUser>>(),
+                Enumerable.Empty<IIdentityMessageProvider>(), 
+                new Logger<UserManager<TUser>>(new LoggerFactory()),
+                null);
             validator.Setup(v => v.ValidateAsync(userManager, It.IsAny<TUser>()))
                 .Returns(Task.FromResult(IdentityResult.Success)).Verifiable();
             return userManager;
@@ -63,7 +76,11 @@ namespace Microsoft.AspNet.Identity.Test
             store = store ?? new Mock<IRoleStore<TRole>>().Object;
             var roles = new List<IRoleValidator<TRole>>();
             roles.Add(new RoleValidator<TRole>());
-            return new RoleManager<TRole>(store, roles);
+            return new RoleManager<TRole>(store, roles, 
+                new UpperInvariantLookupNormalizer(), 
+                new IdentityErrorDescriber(),
+                new Logger<RoleManager<TRole>>(new LoggerFactory()),
+                null);
         }
 
     }