force input parameter in NewAzureRoleAssignmentCommand to follow vali… #4019
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,62 @@ | ||
| using Microsoft.Azure.Commands.Resources.Models.Authorization; | ||
| using Microsoft.WindowsAzure.Commands.ScenarioTest; | ||
| using System; | ||
| using System.Collections.Generic; | ||
| using Xunit; | ||
|
|
||
| namespace Microsoft.Azure.Commands.Resources.Test { | ||
| public class RoleAssignmentUnitTests | ||
| { | ||
| private void VerifyInvalidScope(string scope, string error) | ||
| { | ||
| try | ||
| { | ||
| AuthorizationClient.ValidateScope(scope); | ||
| Assert.True(false); | ||
| } | ||
| catch(ArgumentException ex) | ||
| { | ||
| Assert.Equal(ex.Message, error); | ||
| } | ||
| } | ||
|
|
||
| private void VerifyValidScope(string scope) | ||
| { | ||
| AuthorizationClient.ValidateScope(scope); | ||
| } | ||
|
|
||
| [Fact] | ||
| [Trait(Category.AcceptanceType, Category.CheckIn)] | ||
| public void VerifyInvalidScopes() | ||
| { | ||
| Dictionary<string, string> scopeAndErrors = new Dictionary<string, string>(); | ||
| scopeAndErrors.Add("test", "Scope 'test' should begin with '/subscriptions' or '/providers'."); | ||
| scopeAndErrors.Add("/subscriptions/e9ee799d-6ab2-4084-b952-e7c86344bbab/Should be 'ResourceGroups'/any group name", "Scope '/subscriptions/e9ee799d-6ab2-4084-b952-e7c86344bbab/Should be 'ResourceGroups'/any group name' should begin with '/subscriptions/<subid>/resourceGroups'."); | ||
| scopeAndErrors.Add("/subscriptions/e9ee799d-6ab2-4084-b952-e7c86344bbab/ResourceGroups", "Scope '/subscriptions/e9ee799d-6ab2-4084-b952-e7c86344bbab/ResourceGroups' should have even number of parts."); | ||
| scopeAndErrors.Add("/subscriptions/e9ee799d-6ab2-4084-b952-e7c86344bbab/ResourceGroups/", "Scope '/subscriptions/e9ee799d-6ab2-4084-b952-e7c86344bbab/ResourceGroups/' should not have any empty part."); | ||
| scopeAndErrors.Add("/subscriptions/e9ee799d-6ab2-4084-b952-e7c86344bbab/ResourceGroups/groupname/Should be 'Providers'/any provider name", "Scope '/subscriptions/e9ee799d-6ab2-4084-b952-e7c86344bbab/ResourceGroups/groupname/Should be 'Providers'/any provider name' should begin with '/subscriptions/<subid>/resourceGroups/<groupname>/providers'."); | ||
| scopeAndErrors.Add("/subscriptions/e9ee799d-6ab2-4084-b952-e7c86344bbab/ResourceGroups/groupname/Providers/providername", "Scope '/subscriptions/e9ee799d-6ab2-4084-b952-e7c86344bbab/ResourceGroups/groupname/Providers/providername' should have at least one pair of resource type and resource name. e.g. '/subscriptions/<subid>/resourceGroups/<groupname>/providers/<providername>/<resourcetype>/<resourcename>'."); | ||
| foreach (var kvp in scopeAndErrors) | ||
| { | ||
| VerifyInvalidScope(kvp.Key, kvp.Value); | ||
| } | ||
| } | ||
|
|
||
| [Fact] | ||
| [Trait(Category.AcceptanceType, Category.CheckIn)] | ||
| public void VerifyValidScopes() | ||
| { | ||
| List<string> validScopes = new List<string>(); | ||
| validScopes.Add("/"); | ||
| validScopes.Add("/subscriptions/e9ee799d-6ab2-4084-b952-e7c86344bbab"); | ||
| validScopes.Add("/providers/providername"); | ||
| validScopes.Add("/subscriptions/e9ee799d-6ab2-4084-b952-e7c86344bbab/ResourceGroups/groupname"); | ||
| validScopes.Add("/subscriptions/e9ee799d-6ab2-4084-b952-e7c86344bbab/ResourceGroups/groupname/Providers/providername/type/typename"); | ||
|
|
||
| foreach (var scope in validScopes) | ||
| { | ||
| VerifyValidScope(scope); | ||
| } | ||
| } | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -157,9 +157,11 @@ public PSRoleAssignment CreateRoleAssignment(FilterRoleAssignmentsOptions parame | |
| { | ||
| Guid principalId = ActiveDirectoryClient.GetObjectId(parameters.ADObjectFilter); | ||
| Guid roleAssignmentId = RoleAssignmentNames.Count == 0 ? Guid.NewGuid() : RoleAssignmentNames.Dequeue(); | ||
| string scope = parameters.Scope; | ||
| ValidateScope(scope); | ||
|
There was a problem hiding this comment.
just a afterthought,we can use the same function for other operations also correct to validate the scope(maybe a change as part of a different CR)? There was a problem hiding this comment. |
||
| string roleDefinitionId = !string.IsNullOrEmpty(parameters.RoleDefinitionName) | ||
| ? AuthorizationHelper.ConstructFullyQualifiedRoleDefinitionIdFromScopeAndIdAsGuid(scope, GetSingleRoleDefinitionByName(parameters.RoleDefinitionName, scope).Id) | ||
| : AuthorizationHelper.ConstructFullyQualifiedRoleDefinitionIdFromScopeAndIdAsGuid(scope, parameters.RoleDefinitionId); | ||
|
|
||
| RoleAssignmentCreateParameters createParameters = new RoleAssignmentCreateParameters | ||
| { | ||
|
|
@@ -498,5 +500,66 @@ private static void ValidateRoleDefinition(PSRoleDefinition roleDefinition) | |
| throw new ArgumentException(ProjectResources.InvalidActions); | ||
| } | ||
| } | ||
|
|
||
| public static void ValidateScope(string scope) | ||
|
There was a problem hiding this comment. So I am assuming that this is just the scope and you'll append /providers/Microsoft.Authorization/roleAssignments after this #Resolved There was a problem hiding this comment. |
||
| { | ||
|
|
||
| if (scope.Length == 0 || !scope.StartsWith("/")) | ||
| { | ||
| throw new ArgumentException(string.Format(ProjectResources.ScopeShouldBeginWithSubscriptionsOrProviders, scope)); | ||
| } | ||
|
|
||
| // "/" is a valid scope. | ||
| if (string.Compare(scope, "/") == 0) | ||
|
There was a problem hiding this comment. Is "/" really a valid scope .. for tenant admins I suppose ? Would a /providers/Microsoft.Authorization/roleAssignments create an assignment ? #ByDesign There was a problem hiding this comment. yes "/" is a valid scope. everything starts from "providers/Microsoft.Authorization" is not part of the scope. In reply to: 120757288 [](ancestors = 120757288) |
||
| { | ||
| return; | ||
| } | ||
|
|
||
| var parts = scope.Substring(1).Split('/'); // Skip the leading '/' | ||
|
|
||
| if (parts.Contains(string.Empty)) | ||
|
There was a problem hiding this comment.
Aren't the double slashes "//" ignored by the service. I thought that should be the case (It'll be if the scope is derived from scope.) #ByDesign There was a problem hiding this comment. ARM does ignore the extra slash, but it is not clear to end user, and i don't think there is no benefit for end user to type double slash in the scope. In reply to: 119178261 [](ancestors = 119178261) |
||
| { | ||
| throw new ArgumentException(string.Format(ProjectResources.ScopeShouldHaveNoEmptyPart, scope)); | ||
| } | ||
|
|
||
| int count = parts.Count(); | ||
|
|
||
| if (count % 2 != 0) | ||
| { | ||
| throw new ArgumentException(string.Format(ProjectResources.ScopeShouldHaveEvenNumberOfParts, scope)); | ||
| } | ||
|
|
||
| // For Scope that on tenant level, without subscriptions, should start with "providers" | ||
| // in this case we only ensure that the number of parts is even. | ||
| if (string.Compare(parts[0], "providers", true) == 0) | ||
| { | ||
| return; | ||
| } | ||
| else if (string.Compare(parts[0], "subscriptions", true) != 0) | ||
| { | ||
| throw new ArgumentException(string.Format(ProjectResources.ScopeShouldBeginWithSubscriptionsOrProviders, scope)); | ||
| } | ||
|
|
||
| // Now check the case that scope begins with '/subscriptions' | ||
|
|
||
| if (count >= 4 && string.Compare(parts[2], "resourceGroups", true) != 0) | ||
|
There was a problem hiding this comment. All these rules make sense to me. Is there a way to make sure that are there any current role assignments which are at scope which don't adhere to these rules. At the end of the day you guys are the master of role assignments and the best judge for what the valid scope is and what is not. One way would be to add the same validation in the RP as well for the scope and make sure that both the validations in service and at client side matches. #ByDesign There was a problem hiding this comment. You may also do a dry run with these validations at the service side #ByDesign There was a problem hiding this comment. on RP side we do have similar check. i've checked all the role assignments (~1.6M) in microsoft tenant that all scopes in these assignments passed this check. In reply to: 120758308 [](ancestors = 120758308) There was a problem hiding this comment. |
||
| { | ||
| throw new ArgumentException(string.Format(ProjectResources.ScopeShouldBeginWithSubscriptionsAndResourceGroups, scope)); | ||
| } | ||
|
|
||
| if (count >= 6) | ||
| { | ||
| if (string.Compare(parts[4], "providers", true) != 0) | ||
| { | ||
| throw new ArgumentException(string.Format(ProjectResources.ScopeShouldBeginWithSubscriptionsAndResourceGroupsAndProviders, scope)); | ||
| } | ||
|
|
||
| if (count < 8) | ||
| { | ||
| throw new ArgumentException(string.Format(ProjectResources.ScopeShouldHaveAtLeastOnePairOfResourceTypeAndName, scope)); | ||
| } | ||
| } | ||
|
|
||
| } | ||
| } | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is necessary, but not sufficient. Please include some unit tests of just the validation method. This will allow you to easily cover all that code without requiring a powershell invocation. Make sure to cover each of the negative and positive outcomes [/, tenant resources, subscription, resource group, top-level resource, child resource, grandchild resource] #Resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unit test added, but basically it is doing same thing as the powershell scripts:)
In reply to: 118623381 [](ancestors = 118623381)
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@markcowl #Resolved