[Hangyi (HangyiWang)]

Make namespace link removal use read-only, advisory permission checks after an expanded warning. LinkedResourceDeleted and Failed skip the check; Succeeded checks read-grant evidence for the configured outbound identity at the linked resource's resource group, falling back to its subscription when the group is confirmed deleted.

Missing access or malformed/unavailable checks warn and continue without changing role assignments; --skip-rbac-check bypasses only this advisory step, and ADR remains authoritative. ETag protection, single-attempt conditional PUTs, and asynchronous polling with retryable reads remain intact.

[Copilot]

🟡 Changes recommended

Non-string target IDs can escape the advisory error handler and block unlink.

1 open finding

• Handle non-string resource IDs without aborting unlink · New

What changed in this PR

Makes ADR namespace unlink permission checks advisory and read-only while preserving concurrency protection and service error reporting.

Changes:

• Adds state-aware read-grant checks with resource-group deletion fallback.
• Introduces --skip-permission-check and clearer warnings.
• Updates documentation and regression coverage.

File	Description
README.md	Summarizes advisory unlink behavior.
HISTORY.rst	Records changes and removes obsolete grant guidance.
azext_iot/​tests/​test_workflow_results_unit.py	Updates integration-test discovery expectations.
azext_iot/​tests/​adr/​test_adr_registration_unit.py	Checks confirmation and flag registration.
azext_iot/​tests/​adr/​test_adr_link_rbac_unit.py	Tests read-grant evidence and scope fallback.
azext_iot/​tests/​adr/​test_adr_link_delete_unit.py	Tests advisory failures, state gating, and safeguards.
azext_iot/​tests/​adr/​test_adr_link_delete_int.py	Adds live advisory-check coverage.
azext_iot/​tests/​adr/​conftest.py	Registers the new integration scenario.
azext_iot/​adr/​rbac.py	Replaces grant creation with read-only checks.
azext_iot/​adr/​providers/​link.py	Applies state-aware advisory preflight.
azext_iot/​adr/​params.py	Registers the skip flag.
azext_iot/​adr/​commands_link.py	Forwards the skip flag.
azext_iot/​adr/​command_map.py	Expands unlink confirmation.
azext_iot/​adr/​_help.py	Documents checks and independent flags.

🧠 Review effort: Balanced

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

🟡 Changes recommended

Malformed identity metadata can still abort the advisory preflight and prevent unlink submission.

1 open finding

• Support the promised --skip-permission-check CLI option · New

1 resolved since last review

• Handle non-string resource IDs without aborting unlink

Previously missed (1)

In code that hasn't changed since last review

Handle malformed identity metadata in advisory checks

azext_iot/​adr/​providers/​link.py:275

For a Succeeded endpoint, malformed identity metadata can still block this advisory check. A non-string outboundIdentity.userAssignedIdentity raises AttributeError in _normalized_id(), and a non-dict attached UAMI projection can raise it when reading principalId. Neither reaches the warning handler, so no namespace PUT is submitted. Convert shape errors from the identity resolver into a handled advisory error without changing add/update behavior, and add regression tests for both cases.

🧠 Review effort: Balanced

[ZZ (zzcodework)]

Azure SRE Agent - automated review

Reviewing full diff (head 2122cc0)

This replaces LinkRbacManager.ensure_unlink_reader (which auto-created a Reader role assignment and waited 30s for propagation before unlink) with check_unlink_read_access, a read-only advisory check: LinkedResourceDeleted/Failed endpoints skip it entirely, Succeeded endpoints look for an existing Reader/Contributor/Owner grant (including inherited) at the linked resource's resource group — falling back to the subscription only if the group is confirmed ResourceGroupNotFound — and any failure to confirm access just warns and continues rather than blocking the unlink. --skip-rbac-check bypasses only this advisory step (not confirmation, not ETag validation).

This is a real behavior change worth flagging explicitly for anyone relying on the old auto-grant: the CLI no longer creates the Reader assignment needed for ADR's own backend to verify the linked resource is gone, so a removal that previously "just worked" by self-granting now depends on the permission already existing, or on ADR's server-side check tolerating its absence. That tradeoff is called out in HISTORY.rst/README.md/_help.py and is exercised end-to-end by the new _assert_live_target_removal_rejected int-test helper, so I'm treating it as intentional rather than a defect.

Both open Copilot findings on this PR look incorrect — verified against the code:

1. "Non-string resourceId reaches the shared parser's .strip() and raises an uncaught AttributeError" — not reachable. _check_unlink_permissions (azext_iot/adr/providers/link.py) does if not isinstance(resource_id, str): raise AzureResponseError(...) before calling _parse_hub_resource_id/_parse_dps_resource_id/_parse_su_resource_id, and that AzureResponseError is itself caught by the surrounding except (CLIError, HttpResponseError, ServiceRequestError, ServiceResponseError) (confirmed AzureResponseError → UserFault → AzCLIError → knack.util.CLIError in azure-cli-core/azure/cli/core/azclierror.py), so it warns and continues exactly like any other advisory-check failure. The PR's own new test, test_delete_real_sdk_non_string_resource_id_does_not_block_unlink, parametrizes resourceId over int/list/dict/bool and asserts the PUT still happens — i.e. this exact scenario is covered and passes.
2. "PR description promises --skip-permission-check, but only --skip-rbac-check is registered" — the PR description, HISTORY.rst, README.md, _help.py and params.py all consistently use --skip-rbac-check only; I could not find --skip-permission-check promised anywhere in this PR. The new parametrize case Copilot is likely reacting to — adding "--skip-permission-check" to test_delete_rejects_composite_and_wait_options's option list — asserts the opposite of what the comment claims: it's a regression test confirming that --skip-permission-check is correctly rejected (SystemExit code 2) as an unregistered option, guarding against that plausible-but-wrong spelling ever being accepted by mistake.

Checked and clean

• resource["identity"] = sanitize_arm_identity(namespace["identity"]) is now correctly guarded by if "identity" in namespace: — needed because the state-gated permission check can skip resolving the outbound identity, so a namespace without an identity key no longer raises KeyError on the PUT body.
• UNLINK_READ_ROLE_IDS's Owner id 8e3af657-a8ff-443c-a75c-2fe8c4bcb635 matches the well-known Azure built-in Owner role definition id.
• _assignment_scope_applies(..., inherited_at_scope=True) (pre-existing, unchanged) correctly treats both a parent subscription scope and a management-group ancestor scope as covering the target resource group, which is what the new inherited-grant test parametrization exercises.
• The conflict-retry path (test_delete_real_sdk_conflict_retries_with_updated_resource equivalent) still calls check_unlink_read_access once per attempt and doesn't regress the existing If-Match/412 handling.

No blocking issues found.

---

Automated review; may be incomplete.

[Copilot]

🟡 Changes recommended

Malformed identity data and SDK deserialization failures can still escape the advisory handler and block unlink.

1 open finding

• Malformed advisory data aborts unlink before namespace removal · New

1 resolved since last review

• Support the promised --skip-permission-check CLI option

🧠 Review effort: Balanced

[Copilot]

🔵 Needs a closer look

Malformed identity containers can still abort unlink before submission despite the advisory continuation warning.

0 open findings

1 resolved since last review

• Malformed advisory data aborts unlink before namespace removal

Previously missed (1)

In code that hasn't changed since last review

Handle non-iterable user-assigned identities without TypeError

azext_iot/​adr/​providers/​link.py:224

If identity.userAssignedIdentities is a non-iterable value such as 123, the advisory check warns that unlink will continue, but this call then raises TypeError: sanitize_arm_identity iterates that value in azext_iot/common/arm.py:67-70. No namespace PUT is sent, and --skip-rbac-check hits the same failure. Handle malformed identity containers when building the writable payload without silently dropping attached identities, and add regression cases for both normal and skipped preflight.

🧠 Review effort: Balanced

[ZZ (zzcodework)]

Azure SRE Agent - automated review

No blockers. 0 new suggestions on b9a813a9.

This commit resolves Copilot's inline finding (4289561224, not restating) that malformed outboundIdentity/userAssignedIdentities data could raise an uncaught AttributeError via _normalized_id, and that a DeserializationError from the resource-group lookup in check_unlink_read_access fell outside the advisory catch.

Verified: link.py's new pre-validation guards every malformed shape the tests cover (non-dict/list/int/bool at each level), short-circuits before .items() so a non-dict userAssignedIdentities can't crash, and the DeserializationError catch is scoped to _check_unlink_permissions only — _remove_endpoint's PUT/412 handling is untouched and still propagates.

_{Automated review, unverified. 0 clean checks recorded in state.}

[Copilot]

🔵 Needs a closer look

Falsy malformed outbound-identity metadata can select the system identity and produce misleading permission-check results.

No open issues 🎉

1 previously missed issue

In code that hasn't changed since last review

Preserve falsy malformed outboundIdentity values for validation

azext_iot/​adr/​providers/​link.py:284

For a Succeeded endpoint, falsy non-object outboundIdentity values ([], false, 0, or "") become {} before validation. If the namespace has a system-assigned identity, the resolver then selects that identity and the check can report read-grant evidence instead of warning about malformed metadata. Default only missing/null values to {} so other malformed values reach the existing advisory warning path. Add these falsy values to the malformed-outbound-identity tests.

_{Review effort: Balanced}

[ZZ (zzcodework)]

Azure SRE Agent - automated review

No blockers. 0 new suggestions on 05b4c34f (changes since b9a813a9, 1 commit, 4 files).

Not restating — this is the direct fix for cheatsheet1999's inline comment (confirmed by HangyiWang): sanitize_arm_identity() assumed identity/identity.userAssignedIdentities are mappings. The commit now raises AzureResponseError before preflight when identity isn't a dict, or userAssignedIdentities isn't a dict with string keys — matching both flagged cases (identity: 123, userAssignedIdentities: 123). Verified via test_delete_real_sdk_rejects_malformed_identity_containers_before_preflight and test_delete_rejects_non_string_attached_identity_keys: GET-only, no PUT, source untouched, preflight/check_unlink_read_access never called.

Also added: resourceId must resolve to a subscription+RG+name-scoped ARM id via parse_resource_id (still advisory, not a hard block) — consistent with existing unscoped-id test coverage.

_{Automated review, unverified. 1 clean check recorded in state.}

[Copilot]

🟢 Approval recommended

No blocking issues were identified; focused regression coverage supports advisory behavior while preserving existing write safeguards.

No open issues 🎉

_{Review effort: Balanced}

[Paymaun (digimaun)]

This warning makes deleting the linked resource a prerequisite for every removal, which is broader than what ADR requires and still incomplete. ADR removes a Failed endpoint without the linked resource being deleted, so a user cleaning up a failed link is told to delete a resource that may still be in use. For a Hub endpoint, including a Failed one, ADR also returns 400 EndpointUsedByNestedDevices before starting the update while any Registry Device still has an IoT Hub capability that references the endpoint, and deleting the IoT Hub does not clear those references. The warning and the hub remove help should limit the deletion step to Succeeded links and add the device reference prerequisite for Hubs, for example deleting the referencing devices with az iot adr ns device delete.

[Paymaun (digimaun)]

The roles link add grants the namespace identity are scoped to the target itself, so once the target is deleted ADR can reject removing a Succeeded endpoint for missing read access. The old path created that Reader grant, and this warning now describes the gap without a runnable fix. Could it print the Reader command for the checked scope through _manual_commands, the way _ensure_assignments already does, when no read grant is found? The remove help and README could also say that ADR confirms a deletion by reading the target, which needs read access on a scope that outlives it, such as the resource group, or the subscription when the group is deleted.

[Paymaun (digimaun)]

Could we add an opt-in next to this flag, such as --grant-reader, for users who want the removal to set up the read access itself? When the check finds no read grant, it would create Reader for the namespace outbound identity on the linked resource's resource group through _ensure_assignments, the same path link add uses, which already confirms the caller can assign roles and otherwise prints the exact commands. The default stays read only, and a grant left behind after a 412 is then one the user asked for. It would apply only to Succeeded endpoints, reject combination with --skip-rbac-check, and print the subscription command rather than granting it when the resource group is deleted, because a subscription grant reaches every resource in it. Its help and the message after a grant should also say that a new assignment can take several minutes to take effect, so if ADR rejects the removal right after the grant, rerunning the command later can succeed. The link update help already notes that assignment visibility does not guarantee the service honors access.