Skip to content

[Fix] Verify Azure DevOps Entra credentials and explain rejected ones - #888

Merged
daniel-lxs merged 11 commits into
developfrom
claude/vibrant-carson-9f6366
Jul 29, 2026
Merged

[Fix] Verify Azure DevOps Entra credentials and explain rejected ones#888
daniel-lxs merged 11 commits into
developfrom
claude/vibrant-carson-9f6366

Conversation

@daniel-lxs

@daniel-lxs daniel-lxs commented Jul 29, 2026

Copy link
Copy Markdown
Member

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 .default scope, 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 bare 401 Unauthorized.

What this PR does

Validates credentials at save time. Saving entra or delegated config 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 Unauthorized on 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

Settings source control section in Entra service principal mode showing the required API permissions hint and Setup guide link

Delegated mode: connected account, same hint

Settings source control section in delegated mode showing the connected Azure DevOps account panel

Rejected save: the provider's own diagnosis plus one action

Toast showing Azure DevOps rejected the Microsoft Entra credential with the TF401444 explanation and a short action

Testing

  • Verified against a live Azure DevOps organization: a service principal without organization access is rejected at save with the TF401444 explanation and nothing persisted; the delegated flow (connect, probe, save, sync, service hooks) succeeds as a Basic-access user; the PAT probe returns valid.
  • Unit coverage in packages/ado and 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 full pnpm test suite pass.

🤖 Generated with Claude Code

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>
@roomote-roomote

roomote-roomote Bot commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

No code issues found. See task

  • packages/ado/src/api.ts:1027 Transient Microsoft Entra token-endpoint failures still block saving: this catch converts network, timeout, and 5xx failures to invalid, which assertValidSourceControlConfigInput rejects. Return unknown for unverifiable failures, reserving invalid for definitive credential rejection.
  • packages/ado/src/api.ts:1079 A transient delegated-token refresh failure also blocks saving: resolveAdoDelegatedToken throws for token-endpoint network and 5xx failures, and this catch converts every error to invalid. Return unknown for unverifiable refresh failures (and apply the validation timeout to the refresh request), reserving invalid for a definitive rejected or expired delegated connection.

Reviewed 9c5f3fd

Comment thread packages/ado/src/__tests__/api.test.ts Fixed
daniel-lxs and others added 10 commits July 29, 2026 13:52
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>
@daniel-lxs
daniel-lxs merged commit 5afaf85 into develop Jul 29, 2026
17 checks passed
@daniel-lxs
daniel-lxs deleted the claude/vibrant-carson-9f6366 branch July 29, 2026 21:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants