Skip to content

Commit cd0acd7

Browse files
HaoKHaoK
committed
API Review cleanup #1
- Remove constructor defaults - Make service properties internal - Add Logging/HttpContextAccessor services which are required by identity
1 parent 597e2b3 commit cd0acd7

File tree

10 files changed

+70
-115
lines changed

10 files changed

+70
-115
lines changed

‎src/Microsoft.AspNet.Identity/IdentityServiceCollectionExtensions.cs

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
33

44
using System;
5+
using Microsoft.AspNet.Hosting;
56
using Microsoft.AspNet.Http;
67
using Microsoft.AspNet.Identity;
78
using Microsoft.AspNet.Security;
@@ -52,6 +53,8 @@ public static IdentityBuilder AddIdentity<TUser, TRole>(
5253
// Services used by identity
5354
services.AddOptions(identityConfig);
5455
services.AddDataProtection(identityConfig);
56+
services.AddLogging(identityConfig);
57+
services.TryAdd(describe.Singleton<IHttpContextAccessor, HttpContextAccessor>());
5558

5659
// Identity services
5760
services.TryAdd(describe.Transient<IUserValidator<TUser>, UserValidator<TUser>>());

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

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,4 +3,7 @@
33

44
using System.Runtime.CompilerServices;
55

6-
[assembly: InternalsVisibleTo("Microsoft.AspNet.Identity.Test")]
6+
[assembly: InternalsVisibleTo("Microsoft.AspNet.Identity.Test")]
7+
[assembly: InternalsVisibleTo("Microsoft.AspNet.Identity.EntityFramework.Test")]
8+
[assembly: InternalsVisibleTo("Microsoft.AspNet.Identity.EntityFramework.InMemory.Test")]
9+
[assembly: InternalsVisibleTo("Microsoft.AspNet.Identity.InMemory.Test")]

‎src/Microsoft.AspNet.Identity/RoleManager.cs

Lines changed: 9 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -28,11 +28,11 @@ public class RoleManager<TRole> : IDisposable where TRole : class
2828
/// <param name="store">The IRoleStore commits changes via the UpdateAsync/CreateAsync methods</param>
2929
/// <param name="roleValidator"></param>
3030
public RoleManager(IRoleStore<TRole> store,
31-
IEnumerable<IRoleValidator<TRole>> roleValidators = null,
32-
ILookupNormalizer keyNormalizer = null,
33-
IdentityErrorDescriber errors = null,
34-
ILogger<RoleManager<TRole>> logger = null,
35-
IHttpContextAccessor contextAccessor = null)
31+
IEnumerable<IRoleValidator<TRole>> roleValidators,
32+
ILookupNormalizer keyNormalizer,
33+
IdentityErrorDescriber errors,
34+
ILogger<RoleManager<TRole>> logger,
35+
IHttpContextAccessor contextAccessor)
3636
{
3737
if (store == null)
3838
{
@@ -62,22 +62,22 @@ public RoleManager(IRoleStore<TRole> store,
6262
/// <summary>
6363
/// Used to validate roles before persisting changes
6464
/// </summary>
65-
public IList<IRoleValidator<TRole>> RoleValidators { get; } = new List<IRoleValidator<TRole>>();
65+
internal IList<IRoleValidator<TRole>> RoleValidators { get; } = new List<IRoleValidator<TRole>>();
6666

6767
/// <summary>
6868
/// Used to generate public API error messages
6969
/// </summary>
70-
public IdentityErrorDescriber ErrorDescriber { get; set; }
70+
internal IdentityErrorDescriber ErrorDescriber { get; set; }
7171

7272
/// <summary>
7373
/// Used to log results
7474
/// </summary>
75-
public ILogger<RoleManager<TRole>> Logger { get; set; }
75+
internal ILogger<RoleManager<TRole>> Logger { get; set; }
7676

7777
/// <summary>
7878
/// Used to normalize user names, role names, emails for uniqueness
7979
/// </summary>
80-
public ILookupNormalizer KeyNormalizer { get; set; }
80+
internal ILookupNormalizer KeyNormalizer { get; set; }
8181

8282
/// <summary>
8383
/// Returns an IQueryable of roles if the store is an IQueryableRoleStore

‎src/Microsoft.AspNet.Identity/UserManager.cs

Lines changed: 20 additions & 74 deletions
Original file line numberDiff line numberDiff line change
@@ -29,8 +29,6 @@ public class UserManager<TUser> : IDisposable where TUser : class
2929

3030
private TimeSpan _defaultLockout = TimeSpan.Zero;
3131
private bool _disposed;
32-
private IPasswordHasher<TUser> _passwordHasher;
33-
private IdentityOptions _options;
3432
private HttpContext _context;
3533

3634
/// <summary>
@@ -47,27 +45,27 @@ public class UserManager<TUser> : IDisposable where TUser : class
4745
/// <param name="msgProviders"></param>
4846
/// <param name="loggerFactory"></param>
4947
public UserManager(IUserStore<TUser> store,
50-
IOptions<IdentityOptions> optionsAccessor = null,
51-
IPasswordHasher<TUser> passwordHasher = null,
52-
IEnumerable<IUserValidator<TUser>> userValidators = null,
53-
IEnumerable<IPasswordValidator<TUser>> passwordValidators = null,
54-
ILookupNormalizer keyNormalizer = null,
55-
IdentityErrorDescriber errors = null,
56-
IEnumerable<IUserTokenProvider<TUser>> tokenProviders = null,
57-
IEnumerable<IIdentityMessageProvider> msgProviders = null,
58-
ILogger<UserManager<TUser>> logger = null,
59-
IHttpContextAccessor contextAccessor = null)
48+
IOptions<IdentityOptions> optionsAccessor,
49+
IPasswordHasher<TUser> passwordHasher,
50+
IEnumerable<IUserValidator<TUser>> userValidators,
51+
IEnumerable<IPasswordValidator<TUser>> passwordValidators,
52+
ILookupNormalizer keyNormalizer,
53+
IdentityErrorDescriber errors,
54+
IEnumerable<IUserTokenProvider<TUser>> tokenProviders,
55+
IEnumerable<IIdentityMessageProvider> msgProviders,
56+
ILogger<UserManager<TUser>> logger,
57+
IHttpContextAccessor contextAccessor)
6058
{
6159
if (store == null)
6260
{
6361
throw new ArgumentNullException(nameof(store));
6462
}
6563
Store = store;
6664
Options = optionsAccessor?.Options ?? new IdentityOptions();
67-
PasswordHasher = passwordHasher ?? new PasswordHasher<TUser>();
68-
KeyNormalizer = keyNormalizer ?? new UpperInvariantLookupNormalizer();
69-
ErrorDescriber = errors ?? new IdentityErrorDescriber();
7065
_context = contextAccessor?.Value;
66+
PasswordHasher = passwordHasher;
67+
KeyNormalizer = keyNormalizer;
68+
ErrorDescriber = errors;
7169

7270
if (userValidators != null)
7371
{
@@ -93,85 +91,33 @@ public UserManager(IUserStore<TUser> store,
9391
RegisterTokenProvider(tokenProvider);
9492
}
9593
}
96-
9794
if (msgProviders != null)
9895
{
9996
foreach (var msgProvider in msgProviders)
10097
{
10198
RegisterMessageProvider(msgProvider);
10299
}
103100
}
104-
105101
}
106102

107103
/// <summary>
108104
/// Persistence abstraction that the Manager operates against
109105
/// </summary>
110106
protected internal IUserStore<TUser> Store { get; set; }
111107

112-
/// <summary>
113-
/// Used to hash/verify passwords
114-
/// </summary>
115-
public IPasswordHasher<TUser> PasswordHasher
116-
{
117-
get
118-
{
119-
ThrowIfDisposed();
120-
return _passwordHasher;
121-
}
122-
set
123-
{
124-
ThrowIfDisposed();
125-
if (value == null)
126-
{
127-
throw new ArgumentNullException("value");
128-
}
129-
_passwordHasher = value;
130-
}
131-
}
108+
internal IPasswordHasher<TUser> PasswordHasher { get; set; }
132109

133-
/// <summary>
134-
/// Used to validate users before persisting changes
135-
/// </summary>
136-
public IList<IUserValidator<TUser>> UserValidators { get; } = new List<IUserValidator<TUser>>();
110+
internal IList<IUserValidator<TUser>> UserValidators { get; } = new List<IUserValidator<TUser>>();
137111

138-
/// <summary>
139-
/// Used to validate passwords before persisting changes
140-
/// </summary>
141-
public IList<IPasswordValidator<TUser>> PasswordValidators { get; } = new List<IPasswordValidator<TUser>>();
112+
internal IList<IPasswordValidator<TUser>> PasswordValidators { get; } = new List<IPasswordValidator<TUser>>();
142113

143-
/// <summary>
144-
/// Used to normalize user names and emails for uniqueness
145-
/// </summary>
146-
public ILookupNormalizer KeyNormalizer { get; set; }
114+
internal ILookupNormalizer KeyNormalizer { get; set; }
147115

148-
/// <summary>
149-
/// Used to generate public API error messages
150-
/// </summary>
151-
public IdentityErrorDescriber ErrorDescriber { get; set; }
116+
internal IdentityErrorDescriber ErrorDescriber { get; set; }
152117

153-
/// <summary>
154-
/// Used to log IdentityResult
155-
/// </summary>
156-
public ILogger<UserManager<TUser>> Logger { get; set; }
118+
internal ILogger<UserManager<TUser>> Logger { get; set; }
157119

158-
public IdentityOptions Options
159-
{
160-
get
161-
{
162-
ThrowIfDisposed();
163-
return _options;
164-
}
165-
set
166-
{
167-
ThrowIfDisposed();
168-
if (value == null)
169-
{
170-
throw new ArgumentNullException("value");
171-
}
172-
_options = value;
173-
}
174-
}
120+
internal IdentityOptions Options { get; set; }
175121

176122
/// <summary>
177123
/// Returns true if the store is an IUserTwoFactorStore

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -19,8 +19,8 @@ public async Task CanCreateUsingAddRoleManager()
1919
var services = new ServiceCollection();
2020
services.AddEntityFramework().AddInMemoryStore();
2121
var store = new RoleStore<IdentityRole>(new InMemoryContext());
22-
services.AddIdentity();
2322
services.AddInstance<IRoleStore<IdentityRole>>(store);
23+
services.AddIdentity();
2424
var provider = services.BuildServiceProvider();
2525
var manager = provider.GetRequiredService<RoleManager<IdentityRole>>();
2626
Assert.NotNull(manager);
@@ -33,7 +33,7 @@ public async Task CanCreateRoleWithSingletonManager()
3333
services.AddEntityFramework().AddInMemoryStore();
3434
services.AddTransient<InMemoryContext>();
3535
services.AddTransient<IRoleStore<IdentityRole>, RoleStore<IdentityRole, InMemoryContext>>();
36-
services.AddTransient<IRoleValidator<IdentityRole>, RoleValidator<IdentityRole>>();
36+
services.AddIdentity();
3737
services.AddSingleton<RoleManager<IdentityRole>>();
3838
var provider = services.BuildServiceProvider();
3939
var manager = provider.GetRequiredService<RoleManager<IdentityRole>>();

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -237,13 +237,13 @@ Task<IdentityRole> IRoleStore<IdentityRole>.FindByNameAsync(string roleName, Can
237237

238238
private class MyUserManager : UserManager<TestUser>
239239
{
240-
public MyUserManager(IUserStore<TestUser> store) : base(store) { }
240+
public MyUserManager(IUserStore<TestUser> store) : base(store, null, null, null, null, null, null, null, null, null, null) { }
241241
}
242242

243243
private class MyRoleManager : RoleManager<TestRole>
244244
{
245245
public MyRoleManager(IRoleStore<TestRole> store,
246-
IEnumerable<IRoleValidator<TestRole>> roleValidators) : base(store)
246+
IEnumerable<IRoleValidator<TestRole>> roleValidators) : base(store, null, null, null, null, null)
247247
{
248248

249249
}

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

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -124,7 +124,7 @@ public void DisposeAfterDisposeDoesNotThrow()
124124
public async Task RoleManagerPublicNullChecks()
125125
{
126126
Assert.Throws<ArgumentNullException>("store",
127-
() => new RoleManager<TestRole>(null, null, null));
127+
() => new RoleManager<TestRole>(null, null, null, null, null, null));
128128
var manager = CreateRoleManager(new NotImplementedStore());
129129
await Assert.ThrowsAsync<ArgumentNullException>("role", async () => await manager.CreateAsync(null));
130130
await Assert.ThrowsAsync<ArgumentNullException>("role", async () => await manager.UpdateAsync(null));
@@ -148,9 +148,7 @@ public async Task RoleStoreMethodsThrowWhenDisposed()
148148

149149
private static RoleManager<TestRole> CreateRoleManager(IRoleStore<TestRole> roleStore)
150150
{
151-
var v = new List<IRoleValidator<TestRole>>();
152-
v.Add(new RoleValidator<TestRole>());
153-
return new RoleManager<TestRole>(roleStore);
151+
return MockHelpers.TestRoleManager(roleStore);
154152
}
155153

156154
private class NotImplementedStore : IRoleStore<TestRole>

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@ public async Task ValidateThrowsWithNull()
1414
{
1515
// Setup
1616
var validator = new RoleValidator<TestRole>();
17-
var manager = new RoleManager<TestRole>(new NoopRoleStore());
17+
var manager = MockHelpers.TestRoleManager<TestRole>();
1818

1919
// Act
2020
// Assert
@@ -29,7 +29,7 @@ public async Task ValidateFailsWithTooShortRoleName(string input)
2929
{
3030
// Setup
3131
var validator = new RoleValidator<TestRole>();
32-
var manager = new RoleManager<TestRole>(new NoopRoleStore());
32+
var manager = MockHelpers.TestRoleManager<TestRole>();
3333
var user = new TestRole {Name = input};
3434

3535
// Act

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

Lines changed: 5 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -16,23 +16,15 @@ namespace Microsoft.AspNet.Identity.Test
1616
{
1717
public class UserManagerTest
1818
{
19-
private class TestManager : UserManager<TestUser>
20-
{
21-
public IUserStore<TestUser> StorePublic { get { return Store; } }
22-
23-
public TestManager(IUserStore<TestUser> store) : base(store) { }
24-
}
25-
2619
[Fact]
2720
public void EnsureDefaultServicesDefaultsWithStoreWorks()
2821
{
2922
var services = new ServiceCollection()
30-
.AddTransient<IUserStore<TestUser>, NoopUserStore>()
31-
.AddTransient<TestManager>();
23+
.AddTransient<IUserStore<TestUser>, NoopUserStore>();
3224
services.AddIdentity<TestUser, IdentityRole>();
33-
var manager = services.BuildServiceProvider().GetRequiredService<TestManager>();
25+
var manager = services.BuildServiceProvider().GetRequiredService<UserManager<TestUser>>();
3426
Assert.NotNull(manager.PasswordHasher);
35-
Assert.NotNull(manager.StorePublic);
27+
Assert.NotNull(manager.Store);
3628
Assert.NotNull(manager.Options);
3729
}
3830

@@ -639,15 +631,11 @@ public async Task PasswordValidatorBlocksCreate()
639631
[Fact]
640632
public async Task ManagerPublicNullChecks()
641633
{
642-
var store = new NotImplementedStore();
643-
644634
Assert.Throws<ArgumentNullException>("store",
645-
() => new UserManager<TestUser>(null, null));
635+
() => new UserManager<TestUser>(null, null, null, null, null, null, null, null, null, null, null));
646636

647-
var manager = new UserManager<TestUser>(store);
637+
var manager = MockHelpers.TestUserManager(new NotImplementedStore());
648638

649-
Assert.Throws<ArgumentNullException>("value", () => manager.PasswordHasher = null);
650-
Assert.Throws<ArgumentNullException>("value", () => manager.Options = null);
651639
await Assert.ThrowsAsync<ArgumentNullException>("user", async () => await manager.CreateAsync(null));
652640
await Assert.ThrowsAsync<ArgumentNullException>("user", async () => await manager.CreateAsync(null, null));
653641
await

‎test/Shared/MockHelpers.cs

Lines changed: 21 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,9 @@
88
using System.Text;
99
using Microsoft.Framework.Logging;
1010
using Moq;
11+
using Microsoft.Framework.OptionsModel;
12+
using System.Linq;
13+
using Microsoft.AspNet.Hosting;
1114

1215
namespace Microsoft.AspNet.Identity.Test
1316
{
@@ -49,10 +52,20 @@ public static Mock<ILogger<T>> MockILogger<T>(StringBuilder logStore = null) whe
4952
public static UserManager<TUser> TestUserManager<TUser>(IUserStore<TUser> store = null) where TUser : class
5053
{
5154
store = store ?? new Mock<IUserStore<TUser>>().Object;
55+
var options = new Mock<IOptions<IdentityOptions>>();
56+
var idOptions = new IdentityOptions();
57+
options.Setup(o => o.Options).Returns(idOptions);
58+
var userValidators = new List<IUserValidator<TUser>>();
5259
var validator = new Mock<IUserValidator<TUser>>();
53-
var userManager = new UserManager<TUser>(store);
54-
userManager.UserValidators.Add(validator.Object);
55-
userManager.PasswordValidators.Add(new PasswordValidator<TUser>());
60+
userValidators.Add(validator.Object);
61+
var pwdValidators = new List<PasswordValidator<TUser>>();
62+
pwdValidators.Add(new PasswordValidator<TUser>());
63+
var userManager = new UserManager<TUser>(store, options.Object, new PasswordHasher<TUser>(),
64+
userValidators, pwdValidators, new UpperInvariantLookupNormalizer(),
65+
new IdentityErrorDescriber(), Enumerable.Empty<IUserTokenProvider<TUser>>(),
66+
Enumerable.Empty<IIdentityMessageProvider>(),
67+
new Logger<UserManager<TUser>>(new LoggerFactory()),
68+
null);
5669
validator.Setup(v => v.ValidateAsync(userManager, It.IsAny<TUser>()))
5770
.Returns(Task.FromResult(IdentityResult.Success)).Verifiable();
5871
return userManager;
@@ -63,7 +76,11 @@ public static RoleManager<TRole> TestRoleManager<TRole>(IRoleStore<TRole> store
6376
store = store ?? new Mock<IRoleStore<TRole>>().Object;
6477
var roles = new List<IRoleValidator<TRole>>();
6578
roles.Add(new RoleValidator<TRole>());
66-
return new RoleManager<TRole>(store, roles);
79+
return new RoleManager<TRole>(store, roles,
80+
new UpperInvariantLookupNormalizer(),
81+
new IdentityErrorDescriber(),
82+
new Logger<RoleManager<TRole>>(new LoggerFactory()),
83+
null);
6784
}
6885

6986
}

0 commit comments

Comments
 (0)