Revert default role assignment for New-AzureRmADServicePrincipal #6272

cormacpayne · 2018-05-22T18:44:21Z

Description

Revert the change to New-AzureRmADServicePrincipal that gave "Contributor" permissions to the current subscription by default if Role and Scope parameters weren't provided. The behavior now goes back to before the 6.0.0 release where no permissions are assigned to the service principal if Role and Scope aren't provided, but use the values provided (or default values) if one (or both) of the parameters are provided.
Update the examples for New-AzureRmADServicePrincipal to reflect these changes, as well as update the descriptions for the cmdlet and the affected parameters

Checklist

I have read the Submitting Changes section of CONTRIBUTING.md
The title of the PR is clear and informative
The appropriate change log has been updated
The PR does not introduce breaking changes
If applicable, the changes made in the PR have proper test coverage
For public API changes to cmdlets:
- the changes have gone through a cmdlet design review
- the cmdlet markdown files were generated using the platyPS module

…te help with examples

cormacpayne · 2018-05-23T00:32:46Z

On-demand: https://azuresdkci.westus2.cloudapp.azure.com/job/powershell-demand/869/
Sign job: https://azuresdkci.westus2.cloudapp.azure.com/job/ps-sign/644/

darshanhs90 · 2018-05-23T00:32:57Z

src/ResourceManager/Resources/ChangeLog.md

+* Revert change to `New-AzureRmADServicePrincipal` that gave service principals "Contributor" permissions over the current subscription if no values were provided for the `Role` or `Scope` parameters
+    - If no values are provided for `Role` or `Scope`, the service principal is created with no permissions
+    - If a `Role` is provided, but no `Scope`, the service principal is created with the specified `Role` permissions over the current subscription
+    - If a `Scope` is provided, but no `Contributor`, the service principal is created with `Contributor` permissions over the specified `Scope`


Contributor [](start = 40, length = 11)

role

darshanhs90 · 2018-05-23T00:36:30Z

            Role = "Contributor";

why are we defaulting to contributor role,when a role isnt provided?

Refers to: src/ResourceManager/Resources/Commands.Resources/ActiveDirectory/NewAzureADServicePrincipalCommand.cs:281 in 0acb1cf. [](commit_id = 0acb1cf, deletion_comment = False)

markcowl

How easy would it be to add a test covering each of the two parameter sets? If possible we should add before checking in. Otherwise, we should verify each scenario manually and file an issue to record the new tests this sprint.

markcowl · 2018-05-23T01:17:50Z

@darshanhs90 Because that is our best guess at what the user would want in this case - since no previous versions of the cmdlet have role or scope as a parameter, there is no chance of an existing script hitting unexpected behavior in this case, as they have to provide at least 'Scope'

twitchax

LGTM. Great work, @cormacpayne.

twitchax · 2018-05-23T03:25:36Z

@darshanhs90, the new behavior is meant to match the Azure CLI behavior, so the default Role is Contributor, as it is in CLI.

…cope

cormacpayne · 2018-05-23T20:55:35Z

Sign job: https://azuresdkci.westus2.cloudapp.azure.com/job/ps-sign/659/

cormacpayne · 2018-05-23T20:56:14Z

@darshanhs90 @markcowl comments addressed and tests added

…te-new-sp # Conflicts: # src/Stack.sln

cormacpayne · 2018-05-24T15:28:52Z

On-demand: https://azuresdkci.westus2.cloudapp.azure.com/job/powershell-demand/877/
Sign job: https://azuresdkci.westus2.cloudapp.azure.com/job/ps-sign/663/

darshanhs90 · 2018-05-24T19:33:00Z

ChangeLog.md

+* Revert change to `New-AzureRmADServicePrincipal` that gave service principals `Contributor` permissions over the current subscription if no values were provided for the `Role` or `Scope` parameters
+    - If no values are provided for `Role` or `Scope`, the service principal is created with no permissions
+    - If a `Role` is provided, but no `Scope`, the service principal is created with the specified `Role` permissions over the current subscription
+    - If a `Scope` is provided, but no `Scope`, the service principal is created with `Contributor` permissions over the specified `Scope`


@darshanhs90 oops, thanks. Fixed this in all occurrences.

darshanhs90 · 2018-05-24T19:33:43Z

src/ResourceManager/Resources/ChangeLog.md

+* Revert change to `New-AzureRmADServicePrincipal` that gave service principals `Contributor` permissions over the current subscription if no values were provided for the `Role` or `Scope` parameters
+    - If no values are provided for `Role` or `Scope`, the service principal is created with no permissions
+    - If a `Role` is provided, but no `Scope`, the service principal is created with the specified `Role` permissions over the current subscription
+    - If a `Scope` is provided, but no `Scope`, the service principal is created with `Contributor` permissions over the specified `Scope`


darshanhs90 · 2018-05-24T19:34:45Z

tools/AzureRM/AzureRM.psd1

+* Revert change to `New-AzureRmADServicePrincipal` that gave service principals `Contributor` permissions over the current subscription if no values were provided for the `Role` or `Scope` parameters
+    - If no values are provided for `Role` or `Scope`, the service principal is created with no permissions
+    - If a `Role` is provided, but no `Scope`, the service principal is created with the specified `Role` permissions over the current subscription
+    - If a `Scope` is provided, but no `Scope`, the service principal is created with `Contributor` permissions over the specified `Scope`


darshanhs90 · 2018-05-24T19:36:00Z

src/ResourceManager/Resources/Commands.Resources.Test/ScenarioTests/ActiveDirectoryTests.ps1

+#>
+function Test-NewADServicePrincipalWithReaderRole
+{
+	# Setup


can we add another test where in we dont specify any role or scope and make sure that no roleassignment change happens before and after running the command

@darshanhs90 added a check in Test-NewADServicePrincipalWithoutApp that no role assignment was created for the new service principal

cormacpayne · 2018-05-24T20:51:25Z

Sign job: https://azuresdkci.westus2.cloudapp.azure.com/job/ps-sign/668/

azuresdkci added the needs-review label May 22, 2018

maddieclayton assigned maddieclayton and unassigned maddieclayton May 22, 2018

Revert default role assignment in New-AzureRmADServicePrincipal, upda…

0acb1cf

…te help with examples

cormacpayne force-pushed the update-new-sp branch from 87d1902 to 0acb1cf Compare May 23, 2018 00:20

cormacpayne changed the title ~~Update documentation and messages displayed for New-AzureRmADServicePrincipal~~ Revert default role assignment for New-AzureRmADServicePrincipal May 23, 2018

cormacpayne assigned markcowl and darshanhs90 May 23, 2018

darshanhs90 reviewed May 23, 2018

View reviewed changes

markcowl reviewed May 23, 2018

View reviewed changes

twitchax previously approved these changes May 23, 2018

View reviewed changes

markcowl assigned cormacpayne and unassigned markcowl May 23, 2018

markcowl added needs-revision and removed needs-review labels May 23, 2018

Fix sign job, add tests for service principal creation with role or s…

574e505

…cope

cormacpayne dismissed twitchax’s stale review via 574e505 May 23, 2018 20:54

cormacpayne assigned markcowl and unassigned cormacpayne May 23, 2018

cormacpayne added needs-review and removed needs-revision labels May 23, 2018

cormacpayne added 2 commits May 24, 2018 08:15

Merge branch 'preview' of github.com:Azure/azure-powershell into upda…

a7b2839

…te-new-sp # Conflicts: # src/Stack.sln

Update versions for release of AzureRM.Resources

215888e

darshanhs90 reviewed May 24, 2018

View reviewed changes

Add test coverage for no role assignment in default SP creation scenario

25fb72a

markcowl approved these changes May 25, 2018

View reviewed changes

markcowl unassigned markcowl and darshanhs90 May 25, 2018

markcowl removed the needs-review label May 25, 2018

cormacpayne changed the base branch from preview to release-6.1.1 May 25, 2018 22:41

cormacpayne changed the base branch from release-6.1.1 to preview May 25, 2018 22:42

cormacpayne changed the base branch from preview to release-6.1.1 May 25, 2018 22:43

twitchax approved these changes May 25, 2018

View reviewed changes

cormacpayne merged commit 33da488 into Azure:release-6.1.1 May 29, 2018

cormacpayne deleted the update-new-sp branch January 16, 2019 21:35

Revert default role assignment for New-AzureRmADServicePrincipal #6272

Revert default role assignment for New-AzureRmADServicePrincipal #6272

Uh oh!

Conversation

cormacpayne commented May 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Checklist

Uh oh!

cormacpayne commented May 23, 2018

Uh oh!

darshanhs90 May 23, 2018

Choose a reason for hiding this comment

Uh oh!

darshanhs90 commented May 23, 2018

Uh oh!

markcowl left a comment

Choose a reason for hiding this comment

Uh oh!

markcowl commented May 23, 2018

Uh oh!

twitchax left a comment

Choose a reason for hiding this comment

Uh oh!

twitchax commented May 23, 2018

Uh oh!

cormacpayne commented May 23, 2018

Uh oh!

cormacpayne commented May 23, 2018

Uh oh!

cormacpayne commented May 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

darshanhs90 May 24, 2018

Choose a reason for hiding this comment

Uh oh!

cormacpayne May 24, 2018

Choose a reason for hiding this comment

Uh oh!

darshanhs90 May 24, 2018

Choose a reason for hiding this comment

Uh oh!

darshanhs90 May 24, 2018

Choose a reason for hiding this comment

Uh oh!

darshanhs90 May 24, 2018

Choose a reason for hiding this comment

Uh oh!

cormacpayne May 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cormacpayne commented May 24, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

cormacpayne commented May 22, 2018 •

edited

Loading

cormacpayne commented May 24, 2018 •

edited

Loading

cormacpayne May 24, 2018 •

edited

Loading