From 6850e3b3b65d0ed80e65c62f51d0654ba44bc1f4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Vincent=20Lain=C3=A9?= Date: Mon, 28 Dec 2015 14:47:15 +0100 Subject: [PATCH 1/4] Fix missing Trim in Roles and Schemes split --- .../AuthorizationPolicy.cs | 7 +++- .../AuthorizationPolicyFacts.cs | 34 +++++++++++++++++++ 2 files changed, 40 insertions(+), 1 deletion(-) diff --git a/src/Microsoft.AspNet.Authorization/AuthorizationPolicy.cs b/src/Microsoft.AspNet.Authorization/AuthorizationPolicy.cs index 096caeecd9..1882efb51b 100644 --- a/src/Microsoft.AspNet.Authorization/AuthorizationPolicy.cs +++ b/src/Microsoft.AspNet.Authorization/AuthorizationPolicy.cs @@ -88,6 +88,9 @@ namespace Microsoft.AspNet.Authorization var rolesSplit = authorizeAttribute.Roles?.Split(','); if (rolesSplit != null && rolesSplit.Any()) { + for (int i = 0; i < rolesSplit.Length; ++i) + rolesSplit[i] = rolesSplit[i]?.Trim(); + policyBuilder.RequireRole(rolesSplit); useDefaultPolicy = false; } @@ -96,7 +99,9 @@ namespace Microsoft.AspNet.Authorization { foreach (var authType in authTypesSplit) { - policyBuilder.AuthenticationSchemes.Add(authType); + if (string.IsNullOrEmpty(authType)) + continue; + policyBuilder.AuthenticationSchemes.Add(authType.Trim()); } } if (useDefaultPolicy) diff --git a/test/Microsoft.AspNet.Authorization.Test/AuthorizationPolicyFacts.cs b/test/Microsoft.AspNet.Authorization.Test/AuthorizationPolicyFacts.cs index 03eccd2a62..487888d1f4 100644 --- a/test/Microsoft.AspNet.Authorization.Test/AuthorizationPolicyFacts.cs +++ b/test/Microsoft.AspNet.Authorization.Test/AuthorizationPolicyFacts.cs @@ -67,5 +67,39 @@ namespace Microsoft.AspNet.Authroization.Test Assert.False(combined.Requirements.Any(r => r is DenyAnonymousAuthorizationRequirement)); Assert.Equal(2, combined.Requirements.OfType().Count()); } + + [Fact] + public void CombineMustTrimRoles() + { + // Arrange + var attributes = new AuthorizeAttribute[] { + new AuthorizeAttribute("2") { Roles = "r1 , r2" } + }; + var options = new AuthorizationOptions(); + + var combined = AuthorizationPolicy.Combine(options, attributes); + + 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"))); + } + + [Fact] + public void CombineMustTrimAuthenticationScheme() + { + // Arrange + var attributes = new AuthorizeAttribute[] { + new AuthorizeAttribute("2") { ActiveAuthenticationSchemes = "a1 , a2" } + }; + var options = new AuthorizationOptions(); + + var combined = AuthorizationPolicy.Combine(options, attributes); + + Assert.Equal(2, combined.AuthenticationSchemes.Count()); + Assert.True(combined.AuthenticationSchemes.Any(a => a.Equals("a1"))); + Assert.True(combined.AuthenticationSchemes.Any(a => a.Equals("a2"))); + } } } \ No newline at end of file From ee6a57e9a2f4b7fa3c19ecaff637bf277ee690a1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Vincent=20Lain=C3=A9?= Date: Mon, 28 Dec 2015 14:55:13 +0100 Subject: [PATCH 2/4] Fix unit tests --- .../AuthorizationPolicyFacts.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/Microsoft.AspNet.Authorization.Test/AuthorizationPolicyFacts.cs b/test/Microsoft.AspNet.Authorization.Test/AuthorizationPolicyFacts.cs index 487888d1f4..8807fa840f 100644 --- a/test/Microsoft.AspNet.Authorization.Test/AuthorizationPolicyFacts.cs +++ b/test/Microsoft.AspNet.Authorization.Test/AuthorizationPolicyFacts.cs @@ -73,7 +73,7 @@ namespace Microsoft.AspNet.Authroization.Test { // Arrange var attributes = new AuthorizeAttribute[] { - new AuthorizeAttribute("2") { Roles = "r1 , r2" } + new AuthorizeAttribute() { Roles = "r1 , r2" } }; var options = new AuthorizationOptions(); @@ -91,7 +91,7 @@ namespace Microsoft.AspNet.Authroization.Test { // Arrange var attributes = new AuthorizeAttribute[] { - new AuthorizeAttribute("2") { ActiveAuthenticationSchemes = "a1 , a2" } + new AuthorizeAttribute() { ActiveAuthenticationSchemes = "a1 , a2" } }; var options = new AuthorizationOptions(); From 9a5da5861b1f16f96e5bf1ff96de7a787c25b974 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Vincent=20Lain=C3=A9?= Date: Mon, 28 Dec 2015 23:57:42 +0100 Subject: [PATCH 3/4] Fix coding style and handle case where empty roles & schemes are empty --- .../AuthorizationPolicy.cs | 18 ++++---- .../AuthorizationPolicyFacts.cs | 42 +++++++++++++++++++ 2 files changed, 53 insertions(+), 7 deletions(-) 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 From 9bf861307cd7d0edbebebd578a4ba7eab7096ef6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Vincent=20Lain=C3=A9?= Date: Wed, 30 Dec 2015 12:04:00 +0100 Subject: [PATCH 4/4] Rework the empty or space only filtering in Roles and Schemes --- .../AuthorizationPolicy.cs | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-) diff --git a/src/Microsoft.AspNet.Authorization/AuthorizationPolicy.cs b/src/Microsoft.AspNet.Authorization/AuthorizationPolicy.cs index 3e6d0a773d..befcb3c277 100644 --- a/src/Microsoft.AspNet.Authorization/AuthorizationPolicy.cs +++ b/src/Microsoft.AspNet.Authorization/AuthorizationPolicy.cs @@ -85,26 +85,22 @@ namespace Microsoft.AspNet.Authorization policyBuilder.Combine(policy); useDefaultPolicy = false; } - var rolesSplit = authorizeAttribute.Roles?.Split(new char[] { ',' }, StringSplitOptions.RemoveEmptyEntries); + var rolesSplit = authorizeAttribute.Roles?.Split(','); if (rolesSplit != null && rolesSplit.Any()) { - for (int i = 0; i < rolesSplit.Length; ++i) - { - rolesSplit[i] = rolesSplit[i].Trim(); - } + var trimmedRolesSplit = rolesSplit.Where(r => !string.IsNullOrWhiteSpace(r)).Select(r => r.Trim()); - policyBuilder.RequireRole(rolesSplit.Where(r => !string.IsNullOrWhiteSpace(r))); + policyBuilder.RequireRole(trimmedRolesSplit); useDefaultPolicy = false; } - var authTypesSplit = authorizeAttribute.ActiveAuthenticationSchemes?.Split(new char[] { ',' }, StringSplitOptions.RemoveEmptyEntries); + var authTypesSplit = authorizeAttribute.ActiveAuthenticationSchemes?.Split(','); if (authTypesSplit != null && authTypesSplit.Any()) { foreach (var authType in authTypesSplit) { - var trimmedAuthType = authType.Trim(); - if(!string.IsNullOrWhiteSpace(trimmedAuthType)) + if (!string.IsNullOrWhiteSpace(authType)) { - policyBuilder.AuthenticationSchemes.Add(trimmedAuthType); + policyBuilder.AuthenticationSchemes.Add(authType.Trim()); } } }