diff --git a/src/Microsoft.AspNet.Authorization/AuthorizationPolicy.cs b/src/Microsoft.AspNet.Authorization/AuthorizationPolicy.cs index 1882efb51b..3e6d0a773d 100644 --- a/src/Microsoft.AspNet.Authorization/AuthorizationPolicy.cs +++ b/src/Microsoft.AspNet.Authorization/AuthorizationPolicy.cs @@ -85,23 +85,27 @@ namespace Microsoft.AspNet.Authorization policyBuilder.Combine(policy); useDefaultPolicy = false; } - var rolesSplit = authorizeAttribute.Roles?.Split(','); + var rolesSplit = authorizeAttribute.Roles?.Split(new char[] { ',' }, StringSplitOptions.RemoveEmptyEntries); if (rolesSplit != null && rolesSplit.Any()) { for (int i = 0; i < rolesSplit.Length; ++i) - rolesSplit[i] = rolesSplit[i]?.Trim(); + { + rolesSplit[i] = rolesSplit[i].Trim(); + } - policyBuilder.RequireRole(rolesSplit); + policyBuilder.RequireRole(rolesSplit.Where(r => !string.IsNullOrWhiteSpace(r))); useDefaultPolicy = false; } - var authTypesSplit = authorizeAttribute.ActiveAuthenticationSchemes?.Split(','); + var authTypesSplit = authorizeAttribute.ActiveAuthenticationSchemes?.Split(new char[] { ',' }, StringSplitOptions.RemoveEmptyEntries); if (authTypesSplit != null && authTypesSplit.Any()) { foreach (var authType in authTypesSplit) { - if (string.IsNullOrEmpty(authType)) - continue; - policyBuilder.AuthenticationSchemes.Add(authType.Trim()); + var trimmedAuthType = authType.Trim(); + if(!string.IsNullOrWhiteSpace(trimmedAuthType)) + { + policyBuilder.AuthenticationSchemes.Add(trimmedAuthType); + } } } if (useDefaultPolicy) diff --git a/test/Microsoft.AspNet.Authorization.Test/AuthorizationPolicyFacts.cs b/test/Microsoft.AspNet.Authorization.Test/AuthorizationPolicyFacts.cs index 8807fa840f..97430a1215 100644 --- a/test/Microsoft.AspNet.Authorization.Test/AuthorizationPolicyFacts.cs +++ b/test/Microsoft.AspNet.Authorization.Test/AuthorizationPolicyFacts.cs @@ -77,8 +77,10 @@ namespace Microsoft.AspNet.Authroization.Test }; var options = new AuthorizationOptions(); + // Act var combined = AuthorizationPolicy.Combine(options, attributes); + // Assert Assert.True(combined.Requirements.Any(r => r is RolesAuthorizationRequirement)); var rolesAuthorizationRequirement = combined.Requirements.OfType().First(); Assert.Equal(2, rolesAuthorizationRequirement.AllowedRoles.Count()); @@ -95,11 +97,51 @@ namespace Microsoft.AspNet.Authroization.Test }; var options = new AuthorizationOptions(); + // Act var combined = AuthorizationPolicy.Combine(options, attributes); + // Assert Assert.Equal(2, combined.AuthenticationSchemes.Count()); Assert.True(combined.AuthenticationSchemes.Any(a => a.Equals("a1"))); Assert.True(combined.AuthenticationSchemes.Any(a => a.Equals("a2"))); } + + [Fact] + public void CombineMustIgnoreEmptyAuthenticationScheme() + { + // Arrange + var attributes = new AuthorizeAttribute[] { + new AuthorizeAttribute() { ActiveAuthenticationSchemes = "a1 , , ,,, a2" } + }; + var options = new AuthorizationOptions(); + + // Act + var combined = AuthorizationPolicy.Combine(options, attributes); + + // Assert + Assert.Equal(2, combined.AuthenticationSchemes.Count()); + Assert.True(combined.AuthenticationSchemes.Any(a => a.Equals("a1"))); + Assert.True(combined.AuthenticationSchemes.Any(a => a.Equals("a2"))); + } + + [Fact] + public void CombineMustIgnoreEmptyRoles() + { + // Arrange + var attributes = new AuthorizeAttribute[] { + new AuthorizeAttribute() { Roles = "r1 , ,, , r2" } + }; + var options = new AuthorizationOptions(); + + // Act + var combined = AuthorizationPolicy.Combine(options, attributes); + + // Assert + Assert.True(combined.Requirements.Any(r => r is RolesAuthorizationRequirement)); + var rolesAuthorizationRequirement = combined.Requirements.OfType().First(); + Assert.Equal(2, rolesAuthorizationRequirement.AllowedRoles.Count()); + Assert.True(rolesAuthorizationRequirement.AllowedRoles.Any(r => r.Equals("r1"))); + Assert.True(rolesAuthorizationRequirement.AllowedRoles.Any(r => r.Equals("r2"))); + } } } \ No newline at end of file