Fix LogInDetails() to check for external auth before falling through to local auth config#8004
Merged
bwateratmsft merged 5 commits intomainfrom May 1, 2026
Merged
Conversation
Agent-Logs-Url: https://github.com/Azure/azure-dev/sessions/84c64f74-2291-4d35-9140-34e98182b0d3 Co-authored-by: bwateratmsft <36966225+bwateratmsft@users.noreply.github.com>
Copilot
AI
changed the title
[WIP] Fix LogInDetails to check UseExternalAuth for external tokens
Fix LogInDetails() to check for external auth before falling through to local auth config
May 1, 2026
bwateratmsft
requested review from
JeffreyCA,
hemarina,
jongio,
rajeshkamal5050,
tg-msft,
vhvb1989,
wbreza and
weikanglim
as code owners
📋 Prioritization NoteThanks for the contribution! The linked issue isn't in the current milestone yet. |
Contributor
|
@copilot apply changes based on the comments in this thread |
…y account name Agent-Logs-Url: https://github.com/Azure/azure-dev/sessions/ea7b51fa-0517-4597-a29d-e9e2cd889eea Co-authored-by: bwateratmsft <36966225+bwateratmsft@users.noreply.github.com>
This comment was marked as resolved.
This comment was marked as resolved.
wbreza
reviewed
May 1, 2026
Contributor
wbreza
left a comment
wbreza
left a comment
There was a problem hiding this comment.
Code Review — PR #8004
Fix LogInDetails() to check for external auth before falling through to local auth config
✅ Clean bug fix — follows established UseExternalAuth() patterns, resolves a real user-facing issue. No blockers.
Findings
| # | Priority | Finding |
|---|---|---|
| 1 | Medium | Empty DisplayUsername() not guarded — DisplayUsername() returns "" if the token lacks both preferred_username and unique_name. The current diff returns Account: claims.DisplayUsername() without an empty check. Callers assume non-empty Account on success. Suggest: validate non-empty and return ErrNoCurrentUser if empty. (If a subsequent commit addresses this, disregard.) |
| 2 | Medium | EmailLoginType hardcoded — External auth could theoretically serve SP tokens, but always returns EmailLoginType. CurrentPrincipalType() maps this to UserType. Consistent with existing architecture (GetLoggedInServicePrincipalTenantID() also returns nil for external auth), but a // TODO comment acknowledging the limitation would help. |
| 3 | Low | Missing error path tests — Only happy path tested. Error scenarios (HTTP failures, malformed JWT, empty username) not covered. Low risk since underlying functions have their own tests, but one error-path test would strengthen confidence. |
| 4 | Low | Network call in local-only path — LogInDetails() now makes HTTP calls when external auth active. Inherent to the fix and gated behind explicit env var config. Worth noting in the doc comment. |
| 5 | Nitpick | Doc comment — LogInDetails() doc doesn't mention external auth behavior. One-line addition would help future maintainers. |
Overall: Well-structured fix with good test coverage of the happy path. The two Medium items are design suggestions for hardening, not regressions.
Contributor
|
@copilot please address the feedback from @wbreza in #8004 (review), if it has not already been addressed. |
Agent-Logs-Url: https://github.com/Azure/azure-dev/sessions/0aeda872-c855-445c-bc4b-50f929838abe Co-authored-by: bwateratmsft <36966225+bwateratmsft@users.noreply.github.com>
This comment was marked as resolved.
This comment was marked as resolved.
jongio
approved these changes
May 1, 2026
Member
jongio
left a comment
jongio
left a comment
There was a problem hiding this comment.
Clean bug fix. Follows the same UseExternalAuth() short-circuit pattern already used by GetLoggedInServicePrincipalTenantID(). Good call reusing ClaimsForCurrentUser instead of duplicating the token flow, and the empty-username guard handles the edge case where the token lacks both preferred_username and unique_name.
The TODO about hardcoded EmailLoginType is fair - if external auth ever needs to serve SP tokens, this'll need revisiting. Fine for now since GetLoggedInServicePrincipalTenantID() already returns nil, nil for external auth, so the system is consistent.
One minor note: CI build/test is still in progress. Assuming those pass, this looks good to merge.
Contributor
|
@copilot can you fix the tests? This is the explanation from Opus: The failing test is Test_CLI_Auth_ExternalAuth/auth_status_shows_authenticated. The root cause is visible in the stderr output: The PR adds a new code path in LogInDetails() that calls m.ClaimsForCurrentUser() when external auth is configured — this parses the token as a JWT to extract the preferred_username claim. However, the functional test at cli/azd/test/functional/auth_test.go uses a plaintext fake token ("THIS-IS-A-FAKE-TOKEN" on line 38), which is not a valid JWT. When auth status now tries to decode it, the JWT parsing fails with "malformed access token". The fix: The functional test's mock server needs to return a properly structured JWT (with claims like preferred_username) instead of the plaintext string. The unit tests in manager_test.go already do this correctly using buildTestJWT(), but the functional test predates this PR and was never updated. The mock server response on line 69-73 should return a real (unsigned/test) JWT containing a preferred_username claim, similar to how the new unit tests construct one. Since the functional test can't easily import buildTestJWT from the auth package, it would need its own minimal JWT builder or an inline base64-encoded test JWT. |
Agent-Logs-Url: https://github.com/Azure/azure-dev/sessions/02de36fe-29ca-4aad-a437-23e98a408678 Co-authored-by: bwateratmsft <36966225+bwateratmsft@users.noreply.github.com>
Contributor
Author
Fixed in 4711488. Updated |
jongio
approved these changes
May 1, 2026
Member
jongio
left a comment
jongio
left a comment
There was a problem hiding this comment.
Incremental change since my last review: the functional test now uses a properly structured JWT so ClaimsForCurrentUser can parse claims from the token. Correct fix - the old plain string token wouldn't survive JWT parsing. Still looks good.
Collaborator
Azure Dev CLI Install InstructionsInstall scriptsMacOS/Linux
bash: pwsh: WindowsPowerShell install MSI install Standalone Binary
MSI
Documentationlearn.microsoft.com documentationtitle: Azure Developer CLI reference
|
Contributor
|
Verified E2E. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
UseExternalAuth()check at the top ofLogInDetails()incli/azd/pkg/auth/manager.goClaimsForCurrentUser(ctx, nil)claims.DisplayUsername()is non-empty; returnsErrNoCurrentUserwhen emptyLogInDetails()doc comment to mention external auth and outbound HTTP callEmailLoginTypefor external authTest_CLI_Auth_ExternalAuthfunctional test to return a valid JWT (withpreferred_username) instead of a plaintext token, so the new claims parsing inauth statussucceedsOriginal prompt