-
Notifications
You must be signed in to change notification settings - Fork 4.1k
[Az.Resources] Fix Get-AzRoleDefinition returning null Condition for multi-permission roles #29060
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
| Thanks for your contribution! The pull request validation has started. Please revisit this comment for updated status. |
|
Thank you for your contribution @atomassi! We will review the pull request and get back to you soon. |
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.
Pull request overview
This PR fixes a bug in Get-AzRoleDefinition where role definitions with conditions on non-first permission entries were returning incorrect data (null Condition). The fix restructures the output model to preserve the full permissions array from the Azure API instead of flattening it.
Changes:
- Restructured PSRoleDefinition to use a Permissions list instead of flattened Actions/Condition properties
- Added Condition and ConditionVersion properties to PSPermission to preserve per-permission conditions
- Updated all conversion and validation logic to work with the new structure
- Added 18 comprehensive unit tests to verify the conversion behavior
- Updated scenario tests and JSON fixtures to use the new Permissions format
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Resources/Resources/Models.Authorization/PSRoleDefinition.cs | Removed flattened properties (Actions, NotActions, DataActions, NotDataActions, Condition, ConditionVersion) and replaced with Permissions list |
| src/Resources/Resources/Models.Authorization/PSPermission.cs | Added Condition and ConditionVersion properties to preserve per-permission ABAC conditions |
| src/Resources/Resources/Models.Authorization/AuthorizationClientExtensions.cs | Updated ToPSRoleDefinition conversion to preserve full Permissions structure instead of flattening |
| src/Resources/Resources/Models.Authorization/AuthorizationClient.cs | Updated ToRoleDefinitionPermissions and ValidateRoleDefinition to work with Permissions list |
| src/Resources/Resources/ChangeLog.md | Documented the breaking changes with migration guidance |
| src/Resources/Resources.Test/UnitTests/Authorization/AuthorizationClientExtensionsTests.cs | Added comprehensive unit tests (18 tests) covering conversion scenarios including multi-permission roles with conditions |
| src/Resources/Resources.Test/ScenarioTests/RoleDefinitionTests.ps1 | Updated scenario tests to use new Permissions[n].Actions pattern instead of direct Actions property |
| src/Resources/Resources.Test/Resources/RoleDefinition.json | Updated JSON fixture to use Permissions array format |
| src/Resources/Resources.Test/Resources/NewRoleDefinition.json | Updated JSON fixture to use Permissions array format |
| src/Resources/Resources.Test/Resources/InvalidRoleDefinition.json | Updated JSON fixture to use Permissions array format |
| src/Resources/Resources.Test/Resources/DataActionsRoleDefinition.json | Updated JSON fixture to use Permissions array format |
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.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 6 comments.
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.
Pull request overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated 1 comment.
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.
Pull request overview
Copilot reviewed 21 out of 22 changed files in this pull request and generated 2 comments.
Files not reviewed (1)
- src/Resources/Resources/Properties/Resources.Designer.cs: Language not supported
- Update Get-AzRoleDefinition.md with Permissions property examples - Update New-AzRoleDefinition.md with Permissions array format and JSON sample - Update Set-AzRoleDefinition.md with Permissions structure and API limitation note - Update Remove-AzRoleDefinition.md with Permissions output description - Update ChangeLog.md with clearer issue reference formatting
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
|
To the author of the pull request, |
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
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.
Pull request overview
Copilot reviewed 23 out of 24 changed files in this pull request and generated 2 comments.
Files not reviewed (1)
- src/Resources/Resources/Properties/Resources.Designer.cs: Language not supported
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
Fixes #29058
Fixes #25940
Description
This is a bug fix -
Get-AzRoleDefinitionwas returning incorrect data (Condition: null) for roles that have conditions on non-first permission entries.The Bug
The Azure API returns role definitions with multiple permission entries, each with its own
Condition. The old code:Actions,NotActions, etc. propertiesConditionfrompermissions[0].conditionazure-powershell/src/Resources/Resources/Models.Authorization/AuthorizationClientExtensions.cs
Lines 51 to 64 in 04a37e3
For roles like "Azure Container Storage Contributor" where the ABAC condition is on
permissions[1], users calling$role.Conditiongot$null- incorrect data that didn't match the API response.API Response (actual data):
{ "properties": { "roleName": "Azure Container Storage Contributor", "type": "BuiltInRole", "description": "Lets you install Azure Container Storage and manage its storage resources", "assignableScopes": [ "/" ], "permissions": [ { "actions": [ "Microsoft.KubernetesConfiguration/extensions/write", "Microsoft.KubernetesConfiguration/extensions/read", "Microsoft.KubernetesConfiguration/extensions/delete", "Microsoft.KubernetesConfiguration/extensions/operations/read", "Microsoft.Authorization/*/read", "Microsoft.Resources/subscriptions/resourceGroups/read", "Microsoft.Resources/subscriptions/read", "Microsoft.Management/managementGroups/read", "Microsoft.Resources/deployments/*", "Microsoft.Support/*" ], "notActions": [], "dataActions": [], "notDataActions": [] }, { "actions": [ "Microsoft.Authorization/roleAssignments/write", "Microsoft.Authorization/roleAssignments/delete" ], "notActions": [], "dataActions": [], "notDataActions": [], "conditionVersion": "2.0", "condition": "((!(ActionMatches{'Microsoft.Authorization/roleAssignments/write'})) OR (@Request[Microsoft.Authorization/roleAssignments:RoleDefinitionId] ForAnyOfAnyValues:GuidEquals{08d4c71acc634ce4a9c85dd251b4d619})) AND ((!(ActionMatches{'Microsoft.Authorization/roleAssignments/delete'})) OR (@Resource[Microsoft.Authorization/roleAssignments:RoleDefinitionId] ForAnyOfAnyValues:GuidEquals{08d4c71acc634ce4a9c85dd251b4d619}))" } ], "createdOn": "2024-03-06T18:39:55.6502598Z", "updatedOn": "2024-03-28T20:02:49.6413404Z", "createdBy": null, "updatedBy": null }, "id": "/providers/Microsoft.Authorization/roleDefinitions/95dd08a6-00bd-4661-84bf-f6726f83a4d0", "type": "Microsoft.Authorization/roleDefinitions", "name": "95dd08a6-00bd-4661-84bf-f6726f83a4d0" }Old cmdlet output (incorrect):
New cmdlet output (correct):
The Fix
Permissionsstructure from the APICondition/ConditionVersiontoPSPermissionBreaking Change Note
While this is a bug fix, it does change the output/input structure. Users must update scripts:
Get-AzRoleDefinitionoutput:New-AzRoleDefinition -InputFileJSON format:{ "Name": "My Custom Role", "Description": "Custom role description", - "Actions": ["Microsoft.Storage/storageAccounts/read"], - "NotActions": [], - "DataActions": ["Microsoft.Storage/.../blobs/read"], - "NotDataActions": [], + "Permissions": [ + { + "Actions": ["Microsoft.Storage/storageAccounts/read"], + "NotActions": [], + "DataActions": ["Microsoft.Storage/.../blobs/read"], + "NotDataActions": [], + "Condition": null, + "ConditionVersion": null + } + ], "AssignableScopes": ["/subscriptions/{subscriptionId}"] }Why this is necessary: The old flattened properties were discarding data from the API response. This change exposes the correct data that was previously being lost.
Affected Cmdlets
Get-AzRoleDefinitionPSRoleDefinitionno longer hasActions,NotActions,DataActions,NotDataActions,Condition,ConditionVersionproperties. UsePermissions[n].Actionsetc. instead.New-AzRoleDefinition-InputFileJSON format changed. Must usePermissionsarray instead of flattenedActions/NotActions/DataActions/NotDataActionsproperties.Set-AzRoleDefinition-InputFileJSON format changed. Must usePermissionsarray.-Roleparameter now expectsPSRoleDefinitionwithPermissionsproperty.Remove-AzRoleDefinitionPSRoleDefinitionstructure changed (same asGet-AzRoleDefinition).Removed Properties from
PSRoleDefinitionThe following properties have been removed from the
PSRoleDefinitionoutput type:Actions→ UsePermissions[0].ActionsNotActions→ UsePermissions[0].NotActionsDataActions→ UsePermissions[0].DataActionsNotDataActions→ UsePermissions[0].NotDataActionsCondition→ UsePermissions[n].Condition(now per-permission!)ConditionVersion→ UsePermissions[n].ConditionVersion(now per-permission!)Migration Examples
Reading role definition properties:
Creating a custom role with
-InputFile:{ "Name": "My Custom Role", "Description": "Custom role description", - "Actions": ["Microsoft.Storage/storageAccounts/read"], - "NotActions": [], - "DataActions": ["Microsoft.Storage/storageAccounts/blobServices/containers/blobs/read"], - "NotDataActions": [], + "Permissions": [ + { + "Actions": ["Microsoft.Storage/storageAccounts/read"], + "NotActions": [], + "DataActions": ["Microsoft.Storage/storageAccounts/blobServices/containers/blobs/read"], + "NotDataActions": [], + "Condition": null, + "ConditionVersion": null + } + ], "AssignableScopes": ["/subscriptions/{subscriptionId}"] }Updating a role definition:
Why This Breaking Change Is Necessary
The old flattened properties were discarding data from the API response:
Conditionvalues lost because onlypermissions[0].Conditionwas readnullforConditioneven when the role had conditions definedThis change exposes the correct data that was previously being lost.
Mandatory Checklist
Please choose the target release of Azure PowerShell. (⚠️ Target release is a different concept from API readiness. Please click below links for details.)
Check this box to confirm: I have read the Submitting Changes section of
CONTRIBUTING.mdand reviewed the following information:ChangeLog.mdfile(s) appropriatelysrc/{{SERVICE}}/{{SERVICE}}/ChangeLog.md.## Upcoming Releaseheader in the past tense.ChangeLog.mdif no new release is required, such as fixing test case only.