[Authorization] Add deny assignment create/update/delete support#60195
[Authorization] Add deny assignment create/update/delete support#60195jruttle wants to merge 5 commits into
Conversation
Regenerate Azure.ResourceManager.Authorization against a new AutoRest tag (package-2022-04-01-with-deny-assignment-crud) that swaps the deny assignment input file to the 2024-07-01-preview version, adding DenyAssignments_CreateOrUpdate (PUT) and DenyAssignments_Delete (DELETE). An autorest.md directive keeps deny assignment principals on the shared RoleManagementPrincipal model to avoid retyping the existing surface to the preview-only DenyAssignmentPrincipal model. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…pat baseline Update require: to the merged Azure/azure-rest-api-specs commit 37aa1414 (was the pre-merge fork commit). Regeneration produces identical code. Add eng/apicompatbaselines suppression for the intentional deny-assignment writable-model breaking changes and expand the CHANGELOG breaking-changes notes. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
github-actions
Bot
added
the
Mgmt
Regenerates tests/Generated/Samples to match the autorest output for the new package-2022-04-01-with-deny-assignment-crud tag: - Adds CreateOrUpdate sample to Sample_DenyAssignmentCollection and Update/Delete samples to Sample_DenyAssignmentResource. - Updates example-definition path comments to the relocated Microsoft.Authorization/Authorization spec folder across all samples. Fixes the failing "Verify Generated Code" CI check. Also corrects a ResouceType -> ResourceType typo in CHANGELOG flagged by cspell. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
jruttle
requested review from
a team,
ArcturusZhang,
ArthurMa1978,
jsquire and
live1206
as code owners
|
This is now ready for review. ✅ CI is fully green — the previously-failing What the last commit (
The public API surface is unchanged ( cc mgmt codegen owners @ArcturusZhang @ArthurMa1978 @live1206 and authorization service owners @AshishGargMicrosoft @darshanhs90 — would appreciate a review when you have a moment. This unblocks the .NET deny-assignment CRUD beta (the last remaining language; Python/Java/Go/JS/PowerShell are already shipped). |
There was a problem hiding this comment.
Pull request overview
This PR updates Azure.ResourceManager.Authorization to add deny assignment create/update/delete support generated from the 2024-07-01-preview API, along with the corresponding model updates, samples, and API surface files.
Changes:
- Added CRUD entry points for deny assignments on
DenyAssignmentCollectionandDenyAssignmentResource. - Updated deny-assignment-related models to support PUT semantics (new effect type + writable request model shapes).
- Regenerated sample snippets and updated API listings / AutoRest configuration.
Reviewed changes
Copilot reviewed 6 out of 60 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| sdk/authorization/Azure.ResourceManager.Authorization/tests/Generated/Samples/Sample_TenantResourceExtensions.cs | Regenerated sample comment path. |
| sdk/authorization/Azure.ResourceManager.Authorization/tests/Generated/Samples/Sample_SubscriptionResourceExtensions.cs | Regenerated sample comment path. |
| sdk/authorization/Azure.ResourceManager.Authorization/tests/Generated/Samples/Sample_RoleManagementPolicyResource.cs | Regenerated sample comment paths. |
| sdk/authorization/Azure.ResourceManager.Authorization/tests/Generated/Samples/Sample_RoleManagementPolicyCollection.cs | Regenerated sample comment paths. |
| sdk/authorization/Azure.ResourceManager.Authorization/tests/Generated/Samples/Sample_RoleManagementPolicyAssignmentResource.cs | Regenerated sample comment paths. |
| sdk/authorization/Azure.ResourceManager.Authorization/tests/Generated/Samples/Sample_RoleManagementPolicyAssignmentCollection.cs | Regenerated sample comment paths. |
| sdk/authorization/Azure.ResourceManager.Authorization/tests/Generated/Samples/Sample_RoleEligibilityScheduleResource.cs | Regenerated sample comment path. |
| sdk/authorization/Azure.ResourceManager.Authorization/tests/Generated/Samples/Sample_RoleEligibilityScheduleRequestResource.cs | Regenerated sample comment paths. |
| sdk/authorization/Azure.ResourceManager.Authorization/tests/Generated/Samples/Sample_RoleEligibilityScheduleRequestCollection.cs | Regenerated sample comment paths. |
| sdk/authorization/Azure.ResourceManager.Authorization/tests/Generated/Samples/Sample_RoleEligibilityScheduleInstanceResource.cs | Regenerated sample comment path. |
| sdk/authorization/Azure.ResourceManager.Authorization/tests/Generated/Samples/Sample_RoleEligibilityScheduleInstanceCollection.cs | Regenerated sample comment paths. |
| sdk/authorization/Azure.ResourceManager.Authorization/tests/Generated/Samples/Sample_RoleEligibilityScheduleCollection.cs | Regenerated sample comment paths. |
| sdk/authorization/Azure.ResourceManager.Authorization/tests/Generated/Samples/Sample_RoleAssignmentScheduleResource.cs | Regenerated sample comment path. |
| sdk/authorization/Azure.ResourceManager.Authorization/tests/Generated/Samples/Sample_RoleAssignmentScheduleRequestResource.cs | Regenerated sample comment paths. |
| sdk/authorization/Azure.ResourceManager.Authorization/tests/Generated/Samples/Sample_RoleAssignmentScheduleRequestCollection.cs | Regenerated sample comment paths. |
| sdk/authorization/Azure.ResourceManager.Authorization/tests/Generated/Samples/Sample_RoleAssignmentScheduleInstanceResource.cs | Regenerated sample comment path. |
| sdk/authorization/Azure.ResourceManager.Authorization/tests/Generated/Samples/Sample_RoleAssignmentScheduleInstanceCollection.cs | Regenerated sample comment paths. |
| sdk/authorization/Azure.ResourceManager.Authorization/tests/Generated/Samples/Sample_RoleAssignmentScheduleCollection.cs | Regenerated sample comment paths. |
| sdk/authorization/Azure.ResourceManager.Authorization/tests/Generated/Samples/Sample_RoleAssignmentResource.cs | Regenerated sample comment paths. |
| sdk/authorization/Azure.ResourceManager.Authorization/tests/Generated/Samples/Sample_RoleAssignmentCollection.cs | Regenerated sample comment paths. |
| sdk/authorization/Azure.ResourceManager.Authorization/tests/Generated/Samples/Sample_ResourceGroupResourceExtensions.cs | Regenerated sample comment paths. |
| sdk/authorization/Azure.ResourceManager.Authorization/tests/Generated/Samples/Sample_DenyAssignmentResource.cs | Added/updated deny assignment CRUD samples. |
| sdk/authorization/Azure.ResourceManager.Authorization/tests/Generated/Samples/Sample_DenyAssignmentCollection.cs | Added create-or-update deny assignment sample and updated existing samples. |
| sdk/authorization/Azure.ResourceManager.Authorization/tests/Generated/Samples/Sample_AuthorizationRoleDefinitionResource.cs | Regenerated sample comment paths. |
| sdk/authorization/Azure.ResourceManager.Authorization/tests/Generated/Samples/Sample_AuthorizationRoleDefinitionCollection.cs | Regenerated sample comment paths. |
| sdk/authorization/Azure.ResourceManager.Authorization/tests/Generated/Samples/Sample_AuthorizationProviderOperationsMetadataResource.cs | Regenerated sample comment path. |
| sdk/authorization/Azure.ResourceManager.Authorization/tests/Generated/Samples/Sample_AuthorizationProviderOperationsMetadataCollection.cs | Regenerated sample comment paths. |
| sdk/authorization/Azure.ResourceManager.Authorization/tests/Generated/Samples/Sample_ArmClientExtensions.cs | Regenerated sample comment path. |
| sdk/authorization/Azure.ResourceManager.Authorization/src/Generated/RestOperations/RoleAssignmentsRestOperations.cs | Added/reshuffled list-for-scope REST operation and updated docstrings. |
| sdk/authorization/Azure.ResourceManager.Authorization/src/Generated/RestOperations/ProviderOperationsMetadataRestOperations.cs | Regenerated provider operations REST operations (list vs get shapes). |
| sdk/authorization/Azure.ResourceManager.Authorization/src/Generated/Models/RoleManagementPrincipal.cs | Made principal model constructible/writable to support request bodies. |
| sdk/authorization/Azure.ResourceManager.Authorization/src/Generated/Models/RoleAssignmentListResult.Serialization.cs | Updated list result serialization/deserialization and nextLink typing. |
| sdk/authorization/Azure.ResourceManager.Authorization/src/Generated/Models/RoleAssignmentListResult.cs | Updated list result model shape (Uri nextLink, ctor changes). |
| sdk/authorization/Azure.ResourceManager.Authorization/src/Generated/Models/DenyAssignmentPermission.Serialization.cs | Updated permission model collection typing for writable requests. |
| sdk/authorization/Azure.ResourceManager.Authorization/src/Generated/Models/DenyAssignmentPermission.cs | Made permission model constructible/writable (IList collections, setters). |
| sdk/authorization/Azure.ResourceManager.Authorization/src/Generated/Models/DenyAssignmentListResult.Serialization.cs | Updated deny assignment list result serialization/deserialization and nextLink typing. |
| sdk/authorization/Azure.ResourceManager.Authorization/src/Generated/Models/DenyAssignmentListResult.cs | Updated deny assignment list result model shape (Uri nextLink, ctor changes). |
| sdk/authorization/Azure.ResourceManager.Authorization/src/Generated/Models/DenyAssignmentEffect.cs | Added new extensible enum-like struct for deny assignment effect. |
| sdk/authorization/Azure.ResourceManager.Authorization/src/Generated/Models/AuthorizationProviderOperationsMetadataListResult.Serialization.cs | Updated list result serialization/deserialization and nextLink typing. |
| sdk/authorization/Azure.ResourceManager.Authorization/src/Generated/Models/AuthorizationProviderOperationsMetadataListResult.cs | Updated list result model shape (Uri nextLink, ctor changes). |
| sdk/authorization/Azure.ResourceManager.Authorization/src/Generated/Models/AuthorizationClassicAdministratorListResult.Serialization.cs | Updated list result serialization/deserialization and nextLink typing. |
| sdk/authorization/Azure.ResourceManager.Authorization/src/Generated/Models/AuthorizationClassicAdministratorListResult.cs | Updated list result model shape (Uri nextLink, ctor changes). |
| sdk/authorization/Azure.ResourceManager.Authorization/src/Generated/LongRunningOperation/AuthorizationArmOperation.cs | Added internal LRO wrapper used by deny assignment CRUD operations. |
| sdk/authorization/Azure.ResourceManager.Authorization/src/Generated/Extensions/MockableAuthorizationArmResource.cs | Updated default API version for deny assignments in mockable extensions. |
| sdk/authorization/Azure.ResourceManager.Authorization/src/Generated/Extensions/MockableAuthorizationArmClient.cs | Updated default API version for deny assignments in mockable extensions. |
| sdk/authorization/Azure.ResourceManager.Authorization/src/Generated/Extensions/AuthorizationExtensions.cs | Updated default API version for deny assignment extension methods. |
| sdk/authorization/Azure.ResourceManager.Authorization/src/Generated/DenyAssignmentResource.cs | Added Update/Delete operations for deny assignment resource. |
| sdk/authorization/Azure.ResourceManager.Authorization/src/Generated/DenyAssignmentData.Serialization.cs | Added serialization for new deny assignment properties and write-shape tweaks. |
| sdk/authorization/Azure.ResourceManager.Authorization/src/Generated/DenyAssignmentData.cs | Made deny assignment data writable and added new effect/condition/audit fields. |
| sdk/authorization/Azure.ResourceManager.Authorization/src/Generated/DenyAssignmentCollection.cs | Added CreateOrUpdate operations and updated deny assignment API version. |
| sdk/authorization/Azure.ResourceManager.Authorization/src/Generated/AuthorizationRoleDefinitionData.Serialization.cs | Added serialization for created/updated metadata on role definitions. |
| sdk/authorization/Azure.ResourceManager.Authorization/src/Generated/AuthorizationRoleDefinitionData.cs | Added created/updated metadata properties for role definitions. |
| sdk/authorization/Azure.ResourceManager.Authorization/src/Generated/ArmAuthorizationModelFactory.cs | Updated model factory to reflect new/changed model shapes and overloads. |
| sdk/authorization/Azure.ResourceManager.Authorization/src/autorest.md | Updated AutoRest require/tag and added directive to keep shared Principal model. |
| sdk/authorization/Azure.ResourceManager.Authorization/CHANGELOG.md | Documented new deny assignment CRUD feature and related API changes. |
| sdk/authorization/Azure.ResourceManager.Authorization/api/Azure.ResourceManager.Authorization.netstandard2.0.cs | Updated public API listing for netstandard2.0. |
| sdk/authorization/Azure.ResourceManager.Authorization/api/Azure.ResourceManager.Authorization.net8.0.cs | Updated public API listing for net8.0. |
| sdk/authorization/Azure.ResourceManager.Authorization/api/Azure.ResourceManager.Authorization.net10.0.cs | Updated public API listing for net10.0. |
| eng/apicompatbaselines/Azure.ResourceManager.Authorization.txt | Added new ApiCompat baseline entries to suppress breaking changes. |
|
For reviewers — the APIView for this PR is already generated and current (refreshed by build 6482528's "Create APIView if API has changes" task): (It doesn't show on the release-plan dashboard because the build updated the existing API revision rather than creating a new one, so no fresh APIView bot comment was posted — the revision itself is up to date.) The public API surface is also visible directly in the |
…table DenyAssignmentData.Condition and .ConditionVersion are settable (get/set), not read-only; only CreatedOn/UpdatedOn/CreatedBy/UpdatedBy are read-only. Addresses the PR review note on the changelog accuracy. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Management SDK Review Summary
- Scope:
Azure.ResourceManager.Authorization(1 package, version1.2.0-beta.1, baselineApiCompatVersion=1.1.7) - Versioning: ❌ FAIL — new
eng/apicompatbaselines/Azure.ResourceManager.Authorization.txtadded with 9 ApiCompat suppressions (see Phase 1 below) - API surface: ✅ PASS — scanner: 0 new violations (1 pre-existing
BOOL001onAreDefaultRecipientsEnabledfiltered by baseline); no other rule violations - Contextual naming: evaluated 1 new public type (
DenyAssignmentEffect), flagged 0 - ApiCompat / breaking changes: ❌ FAIL — 9 suppressions in the new baseline (same findings as Phase 1)
- CI: ⏳ In progress — CI checks are still running and cannot be evaluated yet
- Migration-specific checks: not applicable (AutoRest update, not TypeSpec migration)
Phase 1 – Versioning: REQUEST CHANGES
eng/apicompatbaselines/Azure.ResourceManager.Authorization.txt was added with nine MembersMustExist suppressions. Per the management SDK review rules, new ApiCompat baseline entries are not allowed except for MPG-migration WirePathAttribute removal diffs. Breaking changes must be mitigated through customization code or generator/spec features instead.
The nine suppressed entries fall into two categories:
Category A – Factory helper removals (2 entries, easily mitigatable)
ArmAuthorizationModelFactory.DenyAssignmentPermission(...) and ArmAuthorizationModelFactory.RoleManagementPrincipal(...) were removed. Since both underlying models now have public constructors, restore these helpers as [EditorBrowsable(EditorBrowsableState.Never)] compatibility shims in a customization partial class. This is exactly the pattern already applied in this PR for DenyAssignmentData and AuthorizationRoleDefinitionData at the bottom of ArmAuthorizationModelFactory.cs. See the inline comment for the exact code.
Once these two shims are in place, their corresponding baseline entries can be removed.
Category B – IReadOnlyList<T> → IList<T> property-type changes (7 entries, requires design decision)
| Suppressed member | Model |
|---|---|
DenyAssignmentData.Permissions.get() → IReadOnlyList<DenyAssignmentPermission> |
DenyAssignmentData |
DenyAssignmentData.ExcludePrincipals.get() → IReadOnlyList<RoleManagementPrincipal> |
DenyAssignmentData |
DenyAssignmentData.Principals.get() → IReadOnlyList<RoleManagementPrincipal> |
DenyAssignmentData |
DenyAssignmentPermission.Actions.get() → IReadOnlyList<string> |
DenyAssignmentPermission |
DenyAssignmentPermission.NotActions.get() → IReadOnlyList<string> |
DenyAssignmentPermission |
DenyAssignmentPermission.DataActions.get() → IReadOnlyList<string> |
DenyAssignmentPermission |
DenyAssignmentPermission.NotDataActions.get() → IReadOnlyList<string> |
DenyAssignmentPermission |
Changing a declared property getter return type from IReadOnlyList<T> to IList<T> is a binary-breaking change even though source compatibility is preserved (since IList<T> is assignable to IReadOnlyList<T>).
Options for mitigation (please discuss with reviewers):
-
Keep
IReadOnlyList<T>as the declared property type. The underlyingChangeTrackingList<T>already implementsIList<T>, so callers who need to populate the collection can cast:((IList<DenyAssignmentPermission>)data.Permissions).Add(perm). This preserves binary compatibility at the cost of discoverability. -
Expose a writable builder constructor. Change the public constructor to accept the collections as optional parameters (or use init-only properties), keeping the getter typed as
IReadOnlyList<T>. Callers populate via construction, not property assignment. -
Accept the break and document it thoroughly. If options 1 and 2 are rejected by the reviewers, the baseline suppressions for Category B could potentially be approved as a preview exception, but that requires explicit sign-off from
@ArcturusZhang/@ArthurMa1978. Category A suppressions should still be removed and replaced with shims.
Phase 2 – API surface: PASS
The new DenyAssignmentEffect struct (Enforced / Audit) is well named — it includes domain context (DenyAssignment) and is not a generic noise word. Verdict: OK.
All other API additions follow SDK conventions:
| Change | Assessment |
|---|---|
DenyAssignmentCollection.CreateOrUpdate(WaitUntil, string id, DenyAssignmentData) |
✅ Standard pattern for PUT-create |
DenyAssignmentResource.Update(WaitUntil, DenyAssignmentData) |
✅ Standard pattern for PUT-update on resource |
DenyAssignmentResource.Delete(WaitUntil) |
✅ Standard delete |
DenyAssignmentData.CreatedOn/UpdatedOn (DateTimeOffset?) |
✅ On suffix required for DateTimeOffset |
DenyAssignmentData.IsAppliedToChildScopes/IsSystemProtected |
✅ Is prefix required for bool |
DenyAssignmentData.DenyAssignmentEffect? property (same name as type) |
✅ Unusual but acceptable |
AuthorizationRoleDefinitionData.CreatedOn/UpdatedOn/CreatedBy/UpdatedBy |
✅ Follows naming conventions |
No scanner violations on new or changed API surface.
Phase 3 – Breaking changes: REQUEST CHANGES
No CI failures to inspect yet (CI in progress). The ApiCompat baseline file captures all known breaking changes from this PR. Per Phase 1 above, these must be mitigated — not suppressed — before the PR can merge. See inline comment on ArmAuthorizationModelFactory.cs.
Non-inline findings
[Phase 1 / BOOL001 — pre-existing, out of scope] RoleManagementPolicyNotificationRule.AreDefaultRecipientsEnabled violates BOOL001 (doesn't use an allowed verb prefix). This pre-dates this PR and is not a finding for this review.
1 inline comment posted. Analyzed by azure-sdk-mgmt-pr-review:
Analyzed by Azure .NET Management SDK PR Review: https://github.com/Azure/azure-sdk-for-net/actions/runs/28225420136
…y helpers as hidden compat shims Adds [EditorBrowsable(Never)] compat shims for the two removed ArmAuthorizationModelFactory helpers in a Customizations partial, removing 2 of the 9 ApiCompat baseline suppressions (the remaining 7 are the intended IReadOnlyList->IList preview retypes). Regenerates the api/*.cs public surface and clarifies the CHANGELOG. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Addressed the automated reviewer feedback in 65135ea:
Build is green and ApiCompat passes against 1.1.7 with the reduced baseline. All three review threads resolved. @ArcturusZhang @ArthurMa1978 @live1206 — ready for another look when you have a moment. |
There was a problem hiding this comment.
Management SDK Review Summary
- Scope:
Azure.ResourceManager.Authorization(1 package, version1.2.0-beta.1,ApiCompatVersion=1.1.7) - Versioning: ❌ FAIL — 7 new ApiCompat baseline entries for
IReadOnlyList<T>→IList<T>property-type changes (reduced from 9; 2 Category A suppressions fixed ✅) - API surface: ✅ PASS — scanner: 0 new violations (1 pre-existing
BOOL001onAreDefaultRecipientsEnabledfiltered by baseline) - Contextual naming: evaluated 1 new public type (
DenyAssignmentEffect), flagged 0 — verdict: OK (name includes domain contextDenyAssignment) - ApiCompat / breaking changes: ❌ FAIL — 7 suppressions remain (same as Versioning)
- CI: ⏳ In progress — CI checks are still running and cannot be evaluated yet
- Migration-specific checks: not applicable (AutoRest update, not TypeSpec migration)
✅ Good progress: Category A suppressions resolved
The two ArmAuthorizationModelFactory helper removals that were blocking in the previous review are now correctly mitigated:
ArmAuthorizationModelFactory.DenyAssignmentPermission(...)restored as[EditorBrowsable(Never)]shim insrc/Customizations/ArmAuthorizationModelFactory.Custom.csArmAuthorizationModelFactory.RoleManagementPrincipal(...)restored as[EditorBrowsable(Never)]shim in the same file
Both corresponding MembersMustExist entries were removed from the baseline. This is exactly the right approach. ✅
❌ Phase 1 — Versioning: REQUEST CHANGES (7 suppressions remain)
eng/apicompatbaselines/Azure.ResourceManager.Authorization.txt still contains 7 MembersMustExist suppressions:
| Suppressed member | Breaking change |
|---|---|
DenyAssignmentData.Permissions.get() |
IReadOnlyList<DenyAssignmentPermission> → IList<T> |
DenyAssignmentData.ExcludePrincipals.get() |
IReadOnlyList<RoleManagementPrincipal> → IList<T> |
DenyAssignmentData.Principals.get() |
IReadOnlyList<RoleManagementPrincipal> → IList<T> |
DenyAssignmentPermission.Actions.get() |
IReadOnlyList<string> → IList<string> |
DenyAssignmentPermission.NotActions.get() |
IReadOnlyList<string> → IList<string> |
DenyAssignmentPermission.DataActions.get() |
IReadOnlyList<string> → IList<string> |
DenyAssignmentPermission.NotDataActions.get() |
IReadOnlyList<string> → IList<string> |
Changing a declared getter return type from IReadOnlyList<T> to IList<T> is a binary breaking change. Per the management SDK review rules, ApiCompat baseline entries are not allowed to suppress new breaking changes; they must be mitigated through customization code or generator/spec features instead.
Mitigation option — keep IReadOnlyList<T> on the getter:
The ChangeTrackingList<T> that backs these properties already implements both IList<T> and IReadOnlyList<T>. Keeping the declared getter type as IReadOnlyList<T> preserves binary compatibility and removes all 7 suppressions. Callers who need to populate the collection can cast:
var data = new DenyAssignmentData();
((IList<DenyAssignmentPermission>)data.Permissions).Add(perm);This is admittedly less ergonomic than IList<T> directly, but it maintains compatibility. An SDK-side customization partial or an AutoRest directive could hold this declaration.
If the cast-based workaround is unacceptable, the decision to accept these 7 binary breaks as an intentional preview exception must come from one of the assigned owners (@ArcturusZhang / @ArthurMa1978). Once one of them explicitly approves the exception in a review comment, the REQUEST_CHANGES status can be updated.
Phase 2 — API surface: PASS
No naming violations on the new or changed API surface. New additions reviewed:
| Change | Assessment |
|---|---|
DenyAssignmentEffect struct (Enforced/Audit) |
✅ Name includes domain context; OK |
DenyAssignmentCollection.CreateOrUpdate(WaitUntil, string, DenyAssignmentData) |
✅ Standard ARM PUT-create pattern |
DenyAssignmentResource.Update(WaitUntil, DenyAssignmentData) |
✅ Standard ARM PUT-update |
DenyAssignmentResource.Delete(WaitUntil) |
✅ Standard ARM delete |
DenyAssignmentData.CreatedOn/UpdatedOn (DateTimeOffset?) |
✅ On suffix required for DateTimeOffset |
DenyAssignmentData.IsAppliedToChildScopes/IsSystemProtected |
✅ Is prefix for bool |
New DenyAssignmentData.DenyAssignmentEffect? property |
✅ Unusual self-same name but acceptable |
AuthorizationRoleDefinitionData.CreatedOn/UpdatedOn/CreatedBy/UpdatedBy |
✅ Follows conventions |
Phase 3 — Breaking changes: REQUEST CHANGES (same as Phase 1)
CI is still in progress. The 7 binary breaking changes are the same ones flagged in Phase 1 and are suppressed in the baseline. No additional breaking changes observed in the API diff beyond those already documented.
Non-inline findings
[BOOL001 — pre-existing, out of scope] RoleManagementPolicyNotificationRule.AreDefaultRecipientsEnabled violates BOOL001. This pre-dates this PR; not a finding for this review.
1 inline comment posted. Analyzed by azure-sdk-mgmt-pr-review:
Analyzed by Azure .NET Management SDK PR Review: https://github.com/Azure/azure-sdk-for-net/actions/runs/28233516394
Analyzed by Azure .NET Management SDK PR Review: https://github.com/Azure/azure-sdk-for-net/actions/runs/28233516394
| MembersMustExist : Member 'public System.Collections.Generic.IReadOnlyList<System.String> Azure.ResourceManager.Authorization.Models.DenyAssignmentPermission.Actions.get()' does not exist in the implementation but it does exist in the contract. | ||
| MembersMustExist : Member 'public System.Collections.Generic.IReadOnlyList<System.String> Azure.ResourceManager.Authorization.Models.DenyAssignmentPermission.DataActions.get()' does not exist in the implementation but it does exist in the contract. | ||
| MembersMustExist : Member 'public System.Collections.Generic.IReadOnlyList<System.String> Azure.ResourceManager.Authorization.Models.DenyAssignmentPermission.NotActions.get()' does not exist in the implementation but it does exist in the contract. | ||
| MembersMustExist : Member 'public System.Collections.Generic.IReadOnlyList<System.String> Azure.ResourceManager.Authorization.Models.DenyAssignmentPermission.NotDataActions.get()' does not exist in the implementation but it does exist in the contract. |
There was a problem hiding this comment.
[Phase 1 / Phase 3 — still blocking] The two Category A suppressions (factory helper removals) were correctly resolved by the new [EditorBrowsable(Never)] shims in src/Customizations/ArmAuthorizationModelFactory.Custom.cs. Thank you for addressing those.
The 7 remaining MembersMustExist entries (lines 10–16) cover the IReadOnlyList<T> → IList<T> getter type change on DenyAssignmentData.{Permissions,Principals,ExcludePrincipals} and DenyAssignmentPermission.{Actions,NotActions,DataActions,NotDataActions}. Per the management SDK review rules, new ApiCompat baseline entries must not be used to suppress breaking changes — they must be mitigated through customization code or generator/spec features instead.
The remaining mitigation path is to keep the declared property type as IReadOnlyList<T> on the getter while allowing population via an init constructor or via an internal ChangeTrackingList<T> cast:
// Option A — keep getter as IReadOnlyList<T>; cast to populate
// The ChangeTrackingList<T> backing already implements IList<T>.
public IReadOnlyList<DenyAssignmentPermission> Permissions { get; } // stays compatible
// caller: ((IList<DenyAssignmentPermission>)data.Permissions).Add(perm);
// Option B — expose an explicit mutable add-helper on the model via SDK customization
// (still keeps the getter IReadOnlyList<T> for compat)If neither option is acceptable for UX reasons, an explicit exception for this preview break requires sign-off from one of the assigned reviewers (@ArcturusZhang / @ArthurMa1978) before the baseline entries can be approved. Please loop them in for a decision on whether to grant the exception.
Until the 7 remaining suppressions are either mitigated or explicitly approved by the owners, this baseline file keeps the PR in REQUEST_CHANGES state.
Summary
Adds create / update / delete support for deny assignments to
Azure.ResourceManager.Authorization, generated via AutoRest from the2024-07-01-previewdeny assignment API.DenyAssignmentCollectionnow exposesCreateOrUpdate/CreateOrUpdateAsyncDenyAssignmentResourcenow exposesUpdate/UpdateAsyncandDelete/DeleteAsyncDenyAssignmentEffect(Enforced/Audit) plus read-onlyCondition,ConditionVersion,CreatedOn,UpdatedOn,CreatedBy,UpdatedByonDenyAssignmentDataSpec source
Regenerated against a new, non-default AutoRest tag
package-2022-04-01-with-deny-assignment-crudadded in the merged spec PR Azure/azure-rest-api-specs#44165. The tag is identical topackage-2022-04-01but swaps the deny assignment input file topreview/2024-07-01-preview/authorization-DenyAssignmentCalls.json(which addsDenyAssignments_CreateOrUpdate/DenyAssignments_Delete).require:pins the merged upstream commit37aa1414.An
autorest.mddirective keeps deny assignment principals on the shared common-typesPrincipalmodel (RoleManagementPrincipal) so the existing principal surface is not retyped to the preview-onlyDenyAssignmentPrincipalmodel.Breaking changes (preview
1.2.0-beta.1)Supporting PUT makes
DenyAssignmentDataand its nested models writable, which is an intentional breaking change for this preview:DenyAssignmentData.Principals/ExcludePrincipals/Permissions:IReadOnlyList<T>->IList<T>; type gains a public constructorDenyAssignmentPermissioncollections becomeIList<string>; type gains a public constructor;ArmAuthorizationModelFactory.DenyAssignmentPermission(...)removed (use the public constructor)RoleManagementPrincipalgains a public constructor;ArmAuthorizationModelFactory.RoleManagementPrincipal(...)removed (use the public constructor)These are recorded in
CHANGELOG.mdand suppressed against the1.1.7baseline viaeng/apicompatbaselines/Azure.ResourceManager.Authorization.txt.Validation
dotnet build /t:GenerateCoderegenerates cleanlydotnet build -c Releasepasses (0 warnings, 0 errors), including ApiCompat with the new baselineContext
Replaces the closed full-TypeSpec-migration attempt #59931 (see #59932) with a minimal AutoRest change that adds only the two operations.
Release Plan Details