[Fix] Verify Azure DevOps Entra credentials and explain rejected ones - #888
Merged
Conversation
Both Microsoft Entra modes request the `.default` scope, so an app registration that was never granted the Azure DevOps API permissions still mints a token: the config saved cleanly and every later call answered 401 with no hint about what to fix. - name the required API permissions (Code, Graph, User Delegation / Impersonation) and the save-then-consent step in the setup and Settings Entra surfaces, and in the docs - probe one real Azure DevOps call before persisting entra and delegated config, so a permission-less registration fails the save with remediation instead of the first sync - carry the HTTP status on Azure DevOps API errors and replace a rejected-credential status with mode-specific guidance during sync Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
|
No code issues found. See task
Reviewed 9c5f3fd |
Verified against a real Azure DevOps organization: `/_apis/connectionData`
answers `400 Bad Request` at api-version 7.1 ("the -preview flag must be
supplied"), so the credential probe could only ever return `unknown` for a
working credential — it reported success without verifying anything. The
rejected-credential case only looked right because Azure DevOps evaluates
auth (401) before api-version (400).
Probe `/_apis/git/repositories?$top=1` instead: it is the exact call the
sync makes, it is not preview-gated, and it exercises the Code access the
sync needs. This also makes the pre-existing PAT validation functional for
the first time (confirmed `valid` against a real token).
Azure DevOps names the specific problem in the response body — a service
principal that was never added to the organization answers `TF401444:
Please sign-in at least once as ...`. Capture that message on the API
error and lead the guidance with it instead of discarding it.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Caught by driving the real Settings form: saving Azure DevOps config in Entra mode returned success in 24ms without making a single Azure call. The form submits an empty string for every field already satisfied by a runtime env var, and these resolutions used `??`, which only falls back on null/undefined. So `ADO_ORGANIZATION` resolved to `''` and the organization guard returned early — skipping the entire probe. A deployment configured by env vars, which is the self-hosted default, got no validation at all. Use `||` so a blank submitted value falls through to the runtime or persisted one. The same save now takes ~1.4s and is rejected with the Azure DevOps explanation. Regression test fails with `??` and passes with `||`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
CodeQL flagged js/incomplete-url-substring-sanitization (high): the test mock decided which hop it was answering with a substring check, which also matches a URL that merely contains the tenant host in its path or query. Compare the parsed hostname instead. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The rejection message surfaces as a toast, and it was carrying the whole add-save-consent walkthrough (~300 chars after the provider quote). That walkthrough is already permanently visible in the setup/Settings guidance right above the form, and in the docs. Keep the toast to what failed, Azure DevOps' own diagnosis, and one compact pointer: the permission names, "(saved and admin-consented)", and organization membership. Also tightens the captured provider-message cap from 300 to 200 chars. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ssage Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Review feedback: the toast was carrying the whole story. Split it by altitude instead: - toast: one line of diagnosis (likely missing API permissions or not added to the organization) plus Azure DevOps' own explanation with GUID chains collapsed to an ellipsis — TF401444 embeds tenant\tenant\objectId, which dominated the toast without helping anyone read it (the raw message stays on AdoApiError for logs) - Settings inline hint: one sentence naming the three permissions and the save/consent gotcha, linking to the docs setup guide - setup wizard + docs: the full walkthrough, unchanged Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Review feedback: the toast buried the quote under 148 characters of lead-in. Now it is a six-word lead-in, the provider's explanation, and a six-word action. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Roomote review: both validators converted every token-acquisition failure to invalid, so a Microsoft outage, timeout, or network blip during the client-credentials request or the delegated refresh would block saving. That contradicts the probe contract, where only a definitive rejection fails the save. Token acquisition now throws a typed error carrying whether the failure was definitive (the token endpoint answered 400 or 401, or the account needs reconnecting) and the validators map everything else to unknown, which saves. The delegated refresh request also honors the validation timeout. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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.
Problem
The Azure DevOps Microsoft Entra paths (service principal and delegated) had no guidance about required API permissions and no validation. Both modes request the
.defaultscope, so Entra issues a token for whatever the app registration was already consented for: a registration with no Azure DevOps access authenticates fine, saves cleanly, and then every call fails at repository sync with a bare401 Unauthorized.What this PR does
Validates credentials at save time. Saving
entraordelegatedconfig makes one real Azure DevOps call (the same repository listing the sync uses) before anything is persisted. A credential that cannot reach the organization fails the save with actionable text; nothing is written. PAT validation goes through the same probe. Only definitive rejections (203/401/403) fail the save; an unverifiable result such as a network error still saves, matching existing PAT behavior.Explains rejections. Azure DevOps API errors now carry their HTTP status and the provider's own explanation from the response body. A rejected credential surfaces as a short toast: a brief lead-in, the provider's message with GUID chains collapsed, then one action sentence. The same message replaces the raw
401 Unauthorizedon sync failures.Names the required permissions. Both Entra modes in setup and in Settings now name the required Azure DevOps API permissions (Code, Graph, and User Delegation / Impersonation) plus the save and admin-consent steps, with a link to a new "Required API permissions" section in the docs. The permission list lives in one shared constant so UI and docs cannot drift.
Screenshots
Service principal mode: permissions hint with a Setup guide link
Delegated mode: connected account, same hint
Rejected save: the provider's own diagnosis plus one action
Testing
packages/adoand the source-control command tests, including a regression test for deployments configured through runtime env vars, where the form submits blank values.pnpm lint,pnpm check-types, and the fullpnpm testsuite pass.🤖 Generated with Claude Code