Provide error suggestions for AADSTS530084 and include auth error details when emitted as ErrorWithSuggestion in telemetry#7797
Conversation
JeffreyCA
requested review from
hemarina,
jongio,
rajeshkamal5050,
tg-msft,
vhvb1989,
wbreza and
weikanglim
as code owners
There was a problem hiding this comment.
Pull request overview
Adds core CLI handling for Entra Conditional Access “token protection” failures (AADSTS530084) so azd can surface a clearer, actionable error (with links) instead of treating it as a generic re-authentication case.
Changes:
- Introduces
TokenProtectionBlockedErrorand detection logic for AADSTS530084 to return anErrorWithSuggestioncontaining guidance + documentation links. - Ensures token-protection errors take precedence over standard “re-login required” classification.
- Adds unit tests covering the new detection, precedence, and Graph-scope-specific messaging.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| cli/azd/pkg/auth/errors.go | Adds a new auth error type + AADSTS530084 detection and suggestion/link enrichment; introduces scope check for Graph messaging. |
| cli/azd/pkg/auth/errors_test.go | Adds table-driven tests for token protection detection, precedence over relogin classification, and usesGraphScope behavior. |
Co-authored-by: Copilot <copilot@github.com>
jongio
left a comment
jongio
left a comment
There was a problem hiding this comment.
A few cross-cutting concerns I'd address before merge:
-
CI is failing on cspell -
reauthenticatingisn't in the azd dictionary. Either rephrase the comment or add the word tocli/azd/.vscode/cspell-azd-dictionary.txt. -
Three places in the codebase already special-case
*ReLoginRequiredError(gRPC status mapping, two telemetry classifiers, and the auth command stderr suppression). The new*TokenProtectionBlockedErrorwon't be caught by any of them, even though it's the same family of failure. Worth deciding whether each callsite should also recognize the new type, or whether a shared marker interface (e.g.auth.NonRetriableLoginError) would let future auth-error variants flow through without touching every site.
Details inline.
Co-authored-by: Copilot <copilot@github.com>
Co-authored-by: Copilot <copilot@github.com>
JeffreyCA
added
area/error-handling
jongio
left a comment
jongio
left a comment
There was a problem hiding this comment.
Verified the three follow-ups from my earlier review are addressed. Downstream call sites now route through the new AuthInteractionError marker interface, the stderr-suppression guards in auth_login.go and auth_status.go cover both error types, and the AAD-code precedence check in newTokenProtectionBlockedError runs before the generic switch (nicely covered by TestNewReLoginRequiredError_TokenProtectionTakesPrecedence). Sealed-interface design via unexported marker is the right call for future variants. One small naming nit inline.
wbreza
left a comment
wbreza
left a comment
There was a problem hiding this comment.
Reviewing as wbreza. Prior concerns from @jongio (cspell, gRPC codes.Unauthenticated mapping, stderr suppression, telemetry split) all verified resolved by commit 506c2a0's AuthInteractionError interface adoption. New findings below — nothing blocking; mostly test coverage gaps and a design question on gRPC code semantics.
🟠 High (1)
H1 — internal/grpcserver/server.go:278 — codes.Unauthenticated collapses two semantically distinct errors
Both ReLoginRequiredError (fixable by re-auth) and TokenProtectionBlockedError (re-auth won't help — user must contact IT admin) now route through AuthInteractionError to codes.Unauthenticated. Extensions consuming the gRPC contract can't distinguish them and may wrongly prompt users to re-login for conditional-access blocks. This is also directly relevant to #7791 (extension error suggestion propagation), which cannot solve the distinction downstream if both errors share the same status code.
Two options:
- Preferred: Override specifically for TokenProtectionBlockedError → codes.FailedPrecondition or codes.PermissionDenied (policy block, not an auth credential failure)
- Minimum: Add a comment in wrapErrorWithSuggestion documenting the limitation so #7791 authors are aware
🟡 Medium (6)
M1 — internal/cmd/errors.go:309–312 — Dead code in classifySuggestionType
classifySuggestionType is only called from the ErrorWithSuggestion branch of MapError. But MapError checks �rrors.AsType*auth.TokenProtectionBlockedError before that branch, and the outer check traverses ErrorWithSuggestion.Unwrap() — so it always matches first. The new *TokenProtectionBlockedError check in classifySuggestionType (and the pre-existing *ReLoginRequiredError one next to it) is unreachable. Remove or mark as defensive with a comment.
M2 — internal/cmd/errors.go:50–55 — Missing guidepost comment for the concrete-type exception
Design is intentionally split: 3 sites use AuthInteractionError interface (forward-extensible), while MapError/classifySuggestionType use concrete types (per-type telemetry codes). This is correct, but undocumented. A future contributor adding a 3rd AuthInteractionError (e.g., MFARequiredError) will update the 3 interface sites and miss MapError, silently misclassifying telemetry as �rror.suggestion. Suggest adding a comment above the concrete-type checks:
go // Auth errors are checked by concrete type (not the AuthInteractionError // interface) because each type has a distinct telemetry code. Adding a new // AuthInteractionError type? Add a matching branch here as well.
M3 — pkg/auth/errors.go:64–67 — Inconsistent ErrorWithSuggestion field usage in the renamed function
The pre-existing ReLoginRequiredError branch returns an ErrorWithSuggestion with only Err + Suggestion. The login message and the helpLink URL are both packed into the Suggestion string. The new TokenProtectionBlockedError branch correctly populates Message, Suggestion, and Links as separate fields. Since the function is being modified (and renamed) in this PR, it's a good moment to align both branches — per AGENTS.md guidance on populating all relevant ErrorWithSuggestion fields.
M4 — internal/grpcserver/server_test.go — Missing gRPC test for TokenProtectionBlockedError
Test_wrapErrorWithSuggestion has a case for ReLoginRequiredError with suggestion returns Unauthenticated (line ~281) but no corresponding case for TokenProtectionBlockedError. The PR's key behavioral claim — that token-protection errors return codes.Unauthenticated over gRPC — is untested at the gRPC boundary. Easy to add mirroring the existing case.
M5 — internal/cmd/errors_test.go — Missing MapError telemetry test for TokenProtectionBlockedError
WithReLoginRequiredError asserts �rrCode = "auth.login_required" but there's no WithTokenProtectionBlockedError asserting �rrCode = "auth.token_protection_blocked". This test would also cover the critical path where �rrors.AsType[*TokenProtectionBlockedError] traverses ErrorWithSuggestion.Unwrap() — important because MapError correctness depends on that traversal working.
M6 — pkg/auth/errors_test.go — No assertion that AuthInteractionError interface works through the wrapper
All 3 production callsites do �rrors.AsTypeauth.AuthInteractionError on a *ErrorWithSuggestion wrapping a *TokenProtectionBlockedError. This traversal works via ErrorWithSuggestion.Unwrap() but is never asserted in tests. A single line in TestTokenProtectionBlockedError would pin the contract:
go _, isInteractionErr := errors.AsType[AuthInteractionError](err) require.True(t, isInteractionErr, \"TokenProtectionBlockedError should satisfy AuthInteractionError through the ErrorWithSuggestion wrapper\")
🟢 Low (3)
L1 — pkg/auth/errors.go — Detection edge case when �rror_codes is absent
Detection is slices.Contains(response.ErrorCodes, 530084). If AAD ever returns AADSTS530084 only in ErrorDescription (nil ErrorCodes), the fall-through produces ReLoginRequiredError and wrongly tells users to re-login. AAD always includes �rror_codes in practice, so this is low-priority robustness — optional secondary check on ErrorDescription if you want belt-and-suspenders.
L2 — pkg/auth/errors.go — Possible gofmt misalignment in marker methods
go func (*ReLoginRequiredError) isAuthInteractionError() {} func (*TokenProtectionBlockedError) isAuthInteractionError() {}
gofmt emits exactly one space before {}. CI is green so this may already be clean in the actual file, but worth a quick gofmt -l ./pkg/auth/errors.go to confirm.
L3 — pkg/auth/errors.go — Doc comment on AuthInteractionError recommends a wrong pattern
Callers that just need to know "this is an actionable auth failure" should type-assert against this interface…
A bare type assertion would fail through *ErrorWithSuggestion. All 3 callsites correctly use �rrors.AsTypeAuthInteractionError. Suggest updating the doc comment to recommend �rrors.AsType explicitly so future contributors don't write the wrong code.
Not blocking. H1 is worth a discussion (perhaps in coordination with #7791); the rest are small cleanups / test additions.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Addressed the latest review follow-ups:
I intentionally left the |
wbreza
left a comment
wbreza
left a comment
There was a problem hiding this comment.
Thanks for the thorough response to the prior round, @JeffreyCA — the gRPC code split, ErrorLink refactor, Message population, and added telemetry/interface tests all land cleanly. Re-review covers only the delta in 5137cff.
✅ Resolved from prior review
- H1 — gRPC
Unauthenticatedcollapse:grpcAuthCode()now returnscodes.PermissionDeniedfor*TokenProtectionBlockedError, with regression tests covering both paths. helpLink→ structured*ErrorLink(URL + "Conditional Access policy troubleshooting" title).scenarioMessage()populatesErrorWithSuggestion.Message("Login expired." / "Reauthentication required.").TokenProtectionBlockedErrorinterface-satisfaction assertion +token_protection_blockedtelemetry mapping test.- Doc comment updated to recommend
errors.AsType[T].
🔴 New finding
H-new — Extension consumers don't classify PermissionDenied as auth
The new server-side mapping is correct, but neither extension-side decoder treats the new code as auth:
cli/azd/pkg/azdext/extension_error.go:130— only matchescodes.Unauthenticatedcli/azd/extensions/azure.ai.agents/internal/exterrors/errors.go:150,175— same
Result: a TokenProtectionBlockedError traversing gRPC arrives at the extension as a generic error, losing the LocalErrorCategoryAuth / auth_failed classification — partially undoing the original intent of H1. Compounding this, the gRPC server already emits codes.PermissionDenied for unrelated capability errors (framework_service.go:64, service_target_service.go:68, event_service.go:77), so extensions can't safely treat all PermissionDenied as auth.
Two viable fixes:
- Update the consumer side (in this PR or a companion — possibly #7791?) so that
PermissionDenied+ the azd auth-suggestion payload is classified as auth. - Keep both on
Unauthenticatedand let the extension distinguish viaDetails/message metadata —Unauthenticatedper gRPC spec covers any "request does not have valid authentication credentials," which includes policy-blocked tokens.
If #7791 already handles (1), feel free to mark this as out-of-scope here.
🟢 Low
- L-new-1 —
pkg/auth/errors.goscenarioMessage:strings.ToUpper(scenario[:1]) + scenario[1:]is byte-sliced. Safe today (all scenarios ASCII) but a future non-ASCII scenario string would mojibake/panic. Consider autf8.DecodeRuneInStringbased helper. - L-new-2 — Add a one-line doc on
grpcAuthCodeexplaining whyPermissionDenied(notFailedPrecondition) was chosen. Both fit per grpc.io guidance; PermissionDenied is defensible but the rationale lives only in commit context today. - L-new-3 —
"Reauthentication required."forTokenProtectionBlockedErrorreads slightly off given the doc comment notes "re-authenticating with azd won't help." Something like"Sign-in blocked by policy."would match the link title and the actual remediation (admin action). TheSuggestiontext + link carry the real guidance, so this is a polish item.
jongio
left a comment
jongio
left a comment
There was a problem hiding this comment.
Re-checked 5137cff. The naming nit from my earlier review is addressed, and @wbreza's H1 (PermissionDenied split for token protection) landed cleanly with regression tests on both gRPC paths.
No new findings from me. @wbreza's H-new about extension-side classification (extension_error.go and azure.ai.agents/internal/exterrors/errors.go still only match codes.Unauthenticated, so TokenProtectionBlockedError now arrives at extensions unclassified) is the substantive open thread. I don't have anything to add there beyond the two options outlined.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Addressed @wbreza's feedback by moving the auth subtype distinction into structured gRPC details instead of the top-level status code. Changes in this update:
This preserves broad auth detection while still allowing extensions to distinguish the remediation path. |
weikanglim
left a comment
weikanglim
left a comment
There was a problem hiding this comment.
@JeffreyCA Great work on diving deep into the issue! I left a few comments with ideas on how we could clean it up more. Happy to discuss any further on them.
JeffreyCA
changed the title
AADSTS530084 auth errorsAADSTS530084 and include auth error details when emitted as ErrorWithSuggestion in telemetry
Azure Dev CLI Install InstructionsInstall scriptsMacOS/Linux
bash: pwsh: WindowsPowerShell install MSI install Standalone Binary
MSI
Documentationlearn.microsoft.com documentationtitle: Azure Developer CLI reference
|
Resolves #7794
Resolves #7884
This PR adds support for handling
AADSTS530084token protection errors in the authentication error handling logic. Instead of introducing a new azd-specific auth error type, it preserves the original*auth.AuthFailedErrorand wraps it in*internal.ErrorWithSuggestionso users still receive clear guidance and documentation links while downstream auth classification continues to use the underlying AAD error semantics.This PR also preserves auth error details in telemetry when auth failures are wrapped in
*internal.ErrorWithSuggestion.Until #7704 is addressed, users won't be able to fix this issue by re-authenticating, so
azd auth loginas a suggestion was omitted.Simulated error message (using different error code for testing):