From 8ad6fc6f6b771aa32d1f1a29e39f31183642189e Mon Sep 17 00:00:00 2001 From: daniel-lxs Date: Wed, 29 Jul 2026 13:29:03 -0500 Subject: [PATCH 01/11] [Fix] Verify Azure DevOps Entra credentials and explain rejected ones 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 --- .../providers/source-control/azure-devops.mdx | 20 ++ .../setup/AdoSourceControlConfig.tsx | 17 + .../setup/StepSourceControlConnect.tsx | 20 +- .../settings/SourceControlConfigForm.tsx | 17 +- .../commands/source-control/index.test.ts | 100 ++++++ .../src/trpc/commands/source-control/index.ts | 89 ++++- packages/ado/src/__tests__/api.test.ts | 173 +++++++++ packages/ado/src/api.ts | 328 +++++++++++++++--- packages/types/src/source-control.ts | 11 + 9 files changed, 710 insertions(+), 65 deletions(-) diff --git a/apps/docs/providers/source-control/azure-devops.mdx b/apps/docs/providers/source-control/azure-devops.mdx index 4e9af57e2..8ed5ca968 100644 --- a/apps/docs/providers/source-control/azure-devops.mdx +++ b/apps/docs/providers/source-control/azure-devops.mdx @@ -61,6 +61,19 @@ Service hook webhook secrets are generated automatically when Roomote configures service hooks. Advanced values remain editable later in Settings → Source Control. +#### Required API permissions + +Under the app registration's **API permissions**, add the Azure DevOps +permissions **Code**, **Graph**, and **User Delegation / Impersonation**, save +them, and grant admin consent for the tenant. + +Roomote requests the `.default` scope, so Microsoft Entra issues a token +covering whatever the app registration was already consented for. An app +registration missing these permissions still authenticates, and Azure DevOps +then rejects every API call with `401 Unauthorized`. Permissions added in the +Azure portal but not saved behave the same way, so re-check that the change was +saved before retrying a sync. + ### Connect with your Microsoft account Choose **Connect with your Microsoft account** in setup or Settings to authorize the Azure @@ -69,6 +82,9 @@ linked account reference with the deployment and refreshes its short-lived Entra access token as needed. The Entra client ID, client secret, and tenant ID are still required so Roomote can refresh the connection in background jobs. +This mode uses the same app registration, so it needs the same +[API permissions](#required-api-permissions) as the service principal. + The linked account can be reconnected or replaced later in Settings. Switching to PAT or service-principal mode removes the inactive credential path from the deployment configuration. @@ -85,6 +101,10 @@ https://dev.azure.com//_apis/git/repositories?api-version=7.1 Roomote stores the results as Azure DevOps repository rows. +If the sync reports that Azure DevOps rejected the credential, the usual cause +in either Entra mode is a missing or unsaved +[API permission](#required-api-permissions) on the app registration. + Azure DevOps-backed tasks clone from the synced repository row, so sync must run before launching an Azure DevOps-backed task. Worker tasks write host- and clone-path-scoped Azure DevOps credentials into the file-backed Git diff --git a/apps/web/src/app/(onboarding)/setup/AdoSourceControlConfig.tsx b/apps/web/src/app/(onboarding)/setup/AdoSourceControlConfig.tsx index 9c22131a4..a0742d228 100644 --- a/apps/web/src/app/(onboarding)/setup/AdoSourceControlConfig.tsx +++ b/apps/web/src/app/(onboarding)/setup/AdoSourceControlConfig.tsx @@ -1,4 +1,6 @@ import { Blocks, Cog, PlugZap, UserCog, UserKey } from 'lucide-react'; +import { ADO_ENTRA_REQUIRED_API_PERMISSIONS_TEXT } from '@roomote/types'; + import { InstructionUrl } from './ProviderSetupInstructions'; type AdoAuthMode = 'pat' | 'entra' | 'delegated'; @@ -114,6 +116,21 @@ export function AdoSourceControlInstructions({ → New registration → Create an app in the Microsoft tenant that can access your Azure DevOps organization.

+

+ Open the app's{' '} + API permissions → + Add a permission → Azure DevOps, and grant{' '} + + {ADO_ENTRA_REQUIRED_API_PERMISSIONS_TEXT} + + . Then{' '} + + save the permissions + {' '} + and grant admin consent for the tenant — Azure does not apply + permissions until they are saved, and a missing one only shows up later + as a 401 when Roomote syncs repositories. +

{authMode === 'delegated' && ( <>

diff --git a/apps/web/src/app/(onboarding)/setup/StepSourceControlConnect.tsx b/apps/web/src/app/(onboarding)/setup/StepSourceControlConnect.tsx index d128a61db..9dbb40644 100644 --- a/apps/web/src/app/(onboarding)/setup/StepSourceControlConnect.tsx +++ b/apps/web/src/app/(onboarding)/setup/StepSourceControlConnect.tsx @@ -303,13 +303,19 @@ export function StepSourceControlConnect({ adoAuthMode === 'delegated' && adoLinkedAccount.data?.account ) { - await saveAdoLinkedAccount.mutateAsync({ - provider: 'ado', - values: { - ADO_AUTH_MODE: 'delegated', - ADO_LINKED_ACCOUNT_ID: adoLinkedAccount.data.account.accountId, - }, - }); + try { + await saveAdoLinkedAccount.mutateAsync({ + provider: 'ado', + values: { + ADO_AUTH_MODE: 'delegated', + ADO_LINKED_ACCOUNT_ID: adoLinkedAccount.data.account.accountId, + }, + }); + } catch { + // The save verifies the connection against Azure DevOps, so a failure + // here (already surfaced by onError) means the sync would fail too. + return; + } } syncRepositories.mutate(); diff --git a/apps/web/src/components/settings/SourceControlConfigForm.tsx b/apps/web/src/components/settings/SourceControlConfigForm.tsx index 3b417454f..4276d7175 100644 --- a/apps/web/src/components/settings/SourceControlConfigForm.tsx +++ b/apps/web/src/components/settings/SourceControlConfigForm.tsx @@ -3,7 +3,10 @@ import { useEffect, useMemo, useState } from 'react'; import { useMutation, useQueryClient } from '@tanstack/react-query'; import { toast } from 'sonner'; -import type { SetupSourceControlStatus } from '@roomote/types'; +import { + ADO_ENTRA_REQUIRED_API_PERMISSIONS_TEXT, + type SetupSourceControlStatus, +} from '@roomote/types'; import { useTRPC } from '@/trpc/client'; import { Button, Check, Input, Spinner } from '@/components/system'; @@ -274,6 +277,18 @@ export function SourceControlConfigForm({ ) : null} + {isAdo && adoAuthMode !== 'pat' ? ( +

+ The Microsoft Entra app registration needs the Azure DevOps API + permissions{' '} + + {ADO_ENTRA_REQUIRED_API_PERMISSIONS_TEXT} + + , saved in the Azure portal and admin-consented for the tenant. + Permissions that were added but never saved do not apply, and Azure + DevOps answers every call with a 401. +

+ ) : null} {isAdo && adoAuthMode === 'delegated' ? (

diff --git a/apps/web/src/trpc/commands/source-control/index.test.ts b/apps/web/src/trpc/commands/source-control/index.test.ts index 4cf7e287c..8c474c48a 100644 --- a/apps/web/src/trpc/commands/source-control/index.test.ts +++ b/apps/web/src/trpc/commands/source-control/index.test.ts @@ -15,6 +15,9 @@ const { mockUpsertDeploymentEnvironmentVariables, mockResolveAdoOrganization, mockValidateAdoToken, + mockValidateAdoEntraCredentials, + mockValidateAdoDelegatedCredentials, + mockDescribeAdoApiError, mockValidateGiteaToken, mockEnv, } = vi.hoisted(() => ({ @@ -32,6 +35,9 @@ const { mockUpsertDeploymentEnvironmentVariables: vi.fn(), mockResolveAdoOrganization: vi.fn(), mockValidateAdoToken: vi.fn(), + mockValidateAdoEntraCredentials: vi.fn(), + mockValidateAdoDelegatedCredentials: vi.fn(), + mockDescribeAdoApiError: vi.fn(), mockValidateGiteaToken: vi.fn(), mockEnv: { R_APP_URL: 'https://roomote.example.com', @@ -41,12 +47,15 @@ const { })); vi.mock('@roomote/ado', () => ({ + describeAdoApiError: mockDescribeAdoApiError, ensureAdoServiceHooksForRepositories: mockEnsureAdoServiceHooksForRepositories, removeAdoServiceHooksForRepositories: mockRemoveAdoServiceHooksForRepositories, resolveAdoOrganization: mockResolveAdoOrganization, syncAdoRepositories: mockSyncAdoRepositories, + validateAdoDelegatedCredentials: mockValidateAdoDelegatedCredentials, + validateAdoEntraCredentials: mockValidateAdoEntraCredentials, validateAdoToken: mockValidateAdoToken, })); @@ -174,6 +183,11 @@ describe('source-control commands', () => { mockValidateGiteaToken.mockResolvedValue({ status: 'valid' }); mockResolveAdoOrganization.mockResolvedValue(null); mockValidateAdoToken.mockResolvedValue({ status: 'valid' }); + mockValidateAdoEntraCredentials.mockResolvedValue({ status: 'valid' }); + mockValidateAdoDelegatedCredentials.mockResolvedValue({ status: 'valid' }); + mockDescribeAdoApiError.mockImplementation(async (error: unknown) => + error instanceof Error ? error.message : String(error), + ); }); it('creates GitLab webhooks during the OAuth-triggered repository sync', async () => { @@ -525,6 +539,92 @@ describe('source-control commands', () => { expect(mockValidateAdoToken).not.toHaveBeenCalled(); }); + it('probes Microsoft Entra service-principal credentials before saving them', async () => { + mockValidateAdoEntraCredentials.mockResolvedValue({ + status: 'invalid', + error: 'Azure DevOps rejected the Microsoft Entra credential.', + }); + + await expect( + assertValidSourceControlConfigInput({ + provider: 'ado', + values: { + ADO_ORGANIZATION: 'acme', + ADO_AUTH_MODE: 'entra', + ADO_CLIENT_ID: 'client-id', + ADO_CLIENT_SECRET: 'client-secret', + ADO_TENANT_ID: 'tenant-id', + }, + }), + ).rejects.toThrow('Azure DevOps rejected the Microsoft Entra credential.'); + + expect(mockValidateAdoEntraCredentials).toHaveBeenCalledWith({ + clientId: 'client-id', + clientSecret: 'client-secret', + tenantId: 'tenant-id', + organization: 'acme', + baseUrl: undefined, + }); + expect(mockValidateAdoToken).not.toHaveBeenCalled(); + }); + + it('probes the delegated Azure DevOps account once it is linked', async () => { + await assertValidSourceControlConfigInput({ + provider: 'ado', + values: { + ADO_ORGANIZATION: 'acme', + ADO_AUTH_MODE: 'delegated', + ADO_CLIENT_ID: 'client-id', + ADO_CLIENT_SECRET: 'client-secret', + ADO_TENANT_ID: 'tenant-id', + ADO_LINKED_ACCOUNT_ID: 'ado-user@example.com', + }, + }); + + expect(mockValidateAdoDelegatedCredentials).toHaveBeenCalledWith({ + linkedAccountId: 'ado-user@example.com', + clientId: 'client-id', + clientSecret: 'client-secret', + tenantId: 'tenant-id', + organization: 'acme', + baseUrl: undefined, + }); + }); + + it('skips the delegated probe before the Microsoft account is connected', async () => { + await assertValidSourceControlConfigInput({ + provider: 'ado', + allowIncompleteDelegated: true, + values: { + ADO_ORGANIZATION: 'acme', + ADO_AUTH_MODE: 'delegated', + ADO_CLIENT_ID: 'client-id', + ADO_CLIENT_SECRET: 'client-secret', + ADO_TENANT_ID: 'tenant-id', + ADO_LINKED_ACCOUNT_ID: '', + }, + }); + + expect(mockValidateAdoDelegatedCredentials).not.toHaveBeenCalled(); + }); + + it('explains a rejected Azure DevOps credential instead of echoing the sync status', async () => { + mockSyncAdoRepositories.mockRejectedValue( + new Error('Azure DevOps API request failed: 401 Unauthorized'), + ); + mockDescribeAdoApiError.mockResolvedValue( + 'Azure DevOps rejected the Microsoft Entra credential (status 401). Add the API permissions.', + ); + + await expect( + syncRepositoriesCommand(buildMockAuth(), { provider: 'ado' }), + ).resolves.toEqual({ + success: false, + error: + 'Azure DevOps rejected the Microsoft Entra credential (status 401). Add the API permissions.', + }); + }); + it('requires Gitea OAuth credentials during config validation', async () => { await expect( assertValidSourceControlConfigInput({ diff --git a/apps/web/src/trpc/commands/source-control/index.ts b/apps/web/src/trpc/commands/source-control/index.ts index 85ced8af5..86f3d1355 100644 --- a/apps/web/src/trpc/commands/source-control/index.ts +++ b/apps/web/src/trpc/commands/source-control/index.ts @@ -586,9 +586,9 @@ export async function syncRepositoriesCommand( auth.userId, syncResult.repositories, ).catch( - (error: unknown): AdoWebhookSetupSummary => ({ + async (error: unknown): Promise => ({ status: 'skipped', - reason: error instanceof Error ? error.message : String(error), + reason: await Ado.describeAdoApiError(error), }), ); @@ -603,7 +603,15 @@ export async function syncRepositoriesCommand( return { success: false as const, - error: error instanceof Error ? error.message : String(error), + // A rejected Azure DevOps credential reaches the admin as a toast, so + // replace the bare status with what to fix (usually missing or unsaved + // Entra API permissions) instead of echoing "401 Unauthorized". + error: + input.provider === 'ado' + ? await Ado.describeAdoApiError(error) + : error instanceof Error + ? error.message + : String(error), }; } } @@ -853,24 +861,75 @@ export async function assertValidSourceControlConfigInput(params: { throw new Error('Configure the Bitbucket OAuth consumer ID and secret.'); } - if (nextAdoToken) { - const nextAdoOrganization = - params.values?.['ADO_ORGANIZATION']?.trim() ?? - (await Ado.resolveAdoOrganization()); + if (params.provider !== 'ado') { + return; + } - if (!nextAdoOrganization) { - return; - } + const nextAdoOrganization = + params.values?.['ADO_ORGANIZATION']?.trim() ?? + (await Ado.resolveAdoOrganization()); - const validation = await Ado.validateAdoToken({ - token: nextAdoToken, - organization: nextAdoOrganization, - baseUrl: params.values?.['ADO_BASE_URL']?.trim() || undefined, - }); + // Without an organization there is nothing to make a real API call against; + // the config-first setup step saves credentials before the org is known. + if (!nextAdoOrganization) { + return; + } + const nextAdoBaseUrl = params.values?.['ADO_BASE_URL']?.trim() || undefined; + const assertValid = (validation: Ado.AdoTokenValidationResult) => { if (validation.status === 'invalid') { throw new Error(validation.error); } + }; + + // Every mode is verified with one real Azure DevOps call before the values + // are persisted. Both Entra modes request the `.default` scope, so a token + // is issued regardless of which API permissions the app registration was + // granted — without this probe a permission-less registration saves cleanly + // and 401s on every later call. + if (nextAdoAuthMode === 'pat') { + if (nextAdoToken) { + assertValid( + await Ado.validateAdoToken({ + token: nextAdoToken, + organization: nextAdoOrganization, + baseUrl: nextAdoBaseUrl, + }), + ); + } + + return; + } + + if (nextAdoAuthMode === 'delegated') { + // The linked account only exists after the Microsoft sign-in round trip, + // so the credentials-first save has nothing to probe yet. + if (nextAdoLinkedAccountId) { + assertValid( + await Ado.validateAdoDelegatedCredentials({ + linkedAccountId: nextAdoLinkedAccountId, + clientId: nextAdoClientId ?? undefined, + clientSecret: nextAdoClientSecret ?? undefined, + tenantId: nextAdoTenantId ?? undefined, + organization: nextAdoOrganization, + baseUrl: nextAdoBaseUrl, + }), + ); + } + + return; + } + + if (nextAdoClientId && nextAdoClientSecret && nextAdoTenantId) { + assertValid( + await Ado.validateAdoEntraCredentials({ + clientId: nextAdoClientId, + clientSecret: nextAdoClientSecret, + tenantId: nextAdoTenantId, + organization: nextAdoOrganization, + baseUrl: nextAdoBaseUrl, + }), + ); } } diff --git a/packages/ado/src/__tests__/api.test.ts b/packages/ado/src/__tests__/api.test.ts index 0036890c3..eb3352ba1 100644 --- a/packages/ado/src/__tests__/api.test.ts +++ b/packages/ado/src/__tests__/api.test.ts @@ -74,12 +74,14 @@ vi.mock('@roomote/db/encryption', () => ({ })); import { + AdoApiError, buildAdoOrganizationApiBaseUrl, buildAdoRepositoryValues, clearAdoDeploymentUserCache, clearAdoEntraTokenCache, createAdoPullRequestComment, createTaskRunAdoCredentials, + describeAdoApiError, ensureAdoServiceHooksForRepositories, removeAdoServiceHooksForRepositories, getAdoBuildFailureEvidence, @@ -89,10 +91,35 @@ import { normalizeAdoLinkedAccountKey, resolveAdoToken, selectInnermostFailedAdoTimelineRecords, + validateAdoDelegatedCredentials, + validateAdoEntraCredentials, validateAdoToken, type AdoRepository, } from '../api'; +const ENTRA_PERMISSION_GUIDANCE = + 'add Code, Graph, and User Delegation / Impersonation'; + +/** + * Mocks the two hops an Entra credential makes: the Microsoft tenant token + * request, then the Azure DevOps call that the token is used for. + */ +function mockEntraThenAdo(adoResponse: () => Response) { + return vi.fn().mockImplementation(async (url) => { + if (String(url).includes('login.microsoftonline.com')) { + return new Response( + JSON.stringify({ + access_token: 'header.payload.signature', + expires_in: 3600, + }), + { status: 200 }, + ); + } + + return adoResponse(); + }); +} + function makeTaskRun(payload: TaskRun['payload']): TaskRun { return { id: 123, @@ -457,6 +484,152 @@ describe('Azure DevOps API helpers', () => { }); }); + it('rejects a Microsoft Entra app registration that has no Azure DevOps API permissions', async () => { + // The `.default` scope issues a token regardless of what was consented, + // so only the Azure DevOps call reveals the missing permissions. + const fetchMock = mockEntraThenAdo( + () => new Response('{}', { status: 401 }), + ); + + const result = await validateAdoEntraCredentials({ + clientId: 'client-id', + clientSecret: 'client-secret', + tenantId: 'tenant-id', + organization: 'acme', + baseUrl: 'https://dev.azure.com', + fetchImpl: fetchMock, + }); + + expect(result.status).toBe('invalid'); + expect(result.status === 'invalid' && result.error).toContain( + ENTRA_PERMISSION_GUIDANCE, + ); + expect(String(fetchMock.mock.calls[1]?.[0])).toBe( + 'https://dev.azure.com/acme/_apis/connectionData?api-version=7.1', + ); + }); + + it('accepts a Microsoft Entra service principal that can reach Azure DevOps', async () => { + const fetchMock = mockEntraThenAdo( + () => + new Response( + JSON.stringify({ + authenticatedUser: { + id: 'user-guid', + providerDisplayName: 'Roomote Service Principal', + }, + }), + { status: 200 }, + ), + ); + + await expect( + validateAdoEntraCredentials({ + clientId: 'client-id', + clientSecret: 'client-secret', + tenantId: 'tenant-id', + organization: 'acme', + baseUrl: 'https://dev.azure.com', + fetchImpl: fetchMock, + }), + ).resolves.toEqual({ + status: 'valid', + displayName: 'Roomote Service Principal', + }); + expect(fetchMock.mock.calls[1]?.[1]).toMatchObject({ + headers: expect.objectContaining({ + Authorization: 'Bearer header.payload.signature', + }), + }); + }); + + it('rejects Microsoft Entra credentials the tenant will not issue a token for', async () => { + const fetchMock = vi + .fn() + .mockResolvedValue(new Response('{}', { status: 401 })); + + const result = await validateAdoEntraCredentials({ + clientId: 'client-id', + clientSecret: 'wrong-secret', + tenantId: 'tenant-id', + organization: 'acme', + baseUrl: 'https://dev.azure.com', + fetchImpl: fetchMock, + }); + + expect(result).toEqual({ + status: 'invalid', + error: expect.stringContaining( + 'Microsoft Entra did not issue an Azure DevOps token', + ), + }); + expect(fetchMock).toHaveBeenCalledTimes(1); + }); + + it('validates the delegated account with its stored access token', async () => { + mockAuthAccountsFindFirst.mockResolvedValue({ + id: 'account-1', + accountId: 'ado-user@example.com', + accessToken: 'header.payload.signature', + refreshToken: 'refresh-token', + accessTokenExpiresAt: new Date(Date.now() + 3_600_000), + }); + const fetchMock = vi + .fn() + .mockResolvedValue(new Response('{}', { status: 401 })); + + const result = await validateAdoDelegatedCredentials({ + linkedAccountId: 'ado-user@example.com', + organization: 'acme', + baseUrl: 'https://dev.azure.com', + fetchImpl: fetchMock, + }); + + expect(result.status).toBe('invalid'); + expect(result.status === 'invalid' && result.error).toContain( + ENTRA_PERMISSION_GUIDANCE, + ); + }); + + it('reports a delegated connection that was never completed', async () => { + mockAuthAccountsFindFirst.mockResolvedValue(null); + + await expect( + validateAdoDelegatedCredentials({ + linkedAccountId: 'ado-user@example.com', + organization: 'acme', + baseUrl: 'https://dev.azure.com', + }), + ).resolves.toEqual({ + status: 'invalid', + error: + 'No Azure DevOps account is connected. Connect with Microsoft again, then save.', + }); + }); + + it('explains a rejected Entra credential instead of echoing the raw status', async () => { + delete process.env.ADO_TOKEN; + process.env.ADO_AUTH_MODE = 'entra'; + + const message = await describeAdoApiError( + new AdoApiError(401, 'Unauthorized'), + ); + + expect(message).toContain('(status 401)'); + expect(message).toContain(ENTRA_PERMISSION_GUIDANCE); + }); + + it('keeps non-authorization Azure DevOps failures verbatim', async () => { + await expect( + describeAdoApiError(new AdoApiError(500, 'Internal Server Error')), + ).resolves.toBe( + 'Azure DevOps API request failed: 500 Internal Server Error', + ); + await expect( + describeAdoApiError(new Error('socket hang up')), + ).resolves.toBe('socket hang up'); + }); + it('returns unknown when Azure DevOps token validation cannot complete', async () => { const fetchMock = vi .fn() diff --git a/packages/ado/src/api.ts b/packages/ado/src/api.ts index 72a77385a..49da00d5e 100644 --- a/packages/ado/src/api.ts +++ b/packages/ado/src/api.ts @@ -1,6 +1,7 @@ import { z } from 'zod'; import { + ADO_ENTRA_REQUIRED_API_PERMISSIONS_TEXT, ALL_REPOSITORIES, buildRepositoryCloneUrl, stripCloneUrlUserInfo, @@ -236,35 +237,38 @@ function normalizeOrganization(organization: string): string { return trimmed; } -export async function resolveAdoToken(): Promise { - const authMode = await resolveDeploymentEnvVar('ADO_AUTH_MODE'); - if (authMode === 'delegated') { - return resolveAdoDelegatedToken(); - } +type AdoAuthMode = 'pat' | 'entra' | 'delegated'; - const pat = await resolveDeploymentEnvVar('ADO_TOKEN'); - if (pat?.trim() && authMode !== 'entra') { - return pat; - } - - const clientId = await resolveDeploymentEnvVar('ADO_CLIENT_ID'); - const clientSecret = await resolveDeploymentEnvVar('ADO_CLIENT_SECRET'); - const tenantId = - (await resolveDeploymentEnvVar('ADO_TENANT_ID')) ?? - (await resolveDeploymentEnvVar('R_MICROSOFT_TENANT_ID')); +/** + * Effective deployment auth mode. `ADO_AUTH_MODE` is only written by the setup + * and Settings forms, so deployments configured from raw env vars fall back to + * the same inference {@link resolveAdoToken} uses: a PAT when one is present, + * otherwise the Entra service principal. + */ +async function resolveAdoAuthMode(): Promise { + const authMode = (await resolveDeploymentEnvVar('ADO_AUTH_MODE'))?.trim(); - if (!clientId?.trim() || !clientSecret?.trim() || !tenantId?.trim()) { - return null; + if (authMode === 'pat' || authMode === 'entra' || authMode === 'delegated') { + return authMode; } - if ( - cachedAdoEntraToken && - cachedAdoEntraToken.expiresAt > Date.now() + ADO_ENTRA_TOKEN_EXPIRY_SKEW_MS - ) { - return cachedAdoEntraToken.token; - } + return (await resolveDeploymentEnvVar('ADO_TOKEN'))?.trim() ? 'pat' : 'entra'; +} - const response = await fetch( +async function requestAdoEntraClientCredentialsToken({ + clientId, + clientSecret, + tenantId, + fetchImpl = fetch, + timeoutMs, +}: { + clientId: string; + clientSecret: string; + tenantId: string; + fetchImpl?: typeof fetch; + timeoutMs?: number; +}): Promise<{ token: string; expiresIn: number }> { + const response = await fetchImpl( `https://login.microsoftonline.com/${encodeURIComponent(tenantId.trim())}/oauth2/v2.0/token`, { method: 'POST', @@ -275,6 +279,9 @@ export async function resolveAdoToken(): Promise { scope: ADO_ENTRA_TOKEN_SCOPE, grant_type: 'client_credentials', }), + ...(timeoutMs === undefined + ? {} + : { signal: AbortSignal.timeout(timeoutMs) }), }, ); @@ -290,8 +297,6 @@ export async function resolveAdoToken(): Promise { }; const token = typeof payload.access_token === 'string' ? payload.access_token : null; - const expiresIn = - typeof payload.expires_in === 'number' ? payload.expires_in : 3600; if (!token) { throw new Error( @@ -299,6 +304,47 @@ export async function resolveAdoToken(): Promise { ); } + return { + token, + expiresIn: + typeof payload.expires_in === 'number' ? payload.expires_in : 3600, + }; +} + +export async function resolveAdoToken(): Promise { + const authMode = await resolveDeploymentEnvVar('ADO_AUTH_MODE'); + if (authMode === 'delegated') { + return resolveAdoDelegatedToken(); + } + + const pat = await resolveDeploymentEnvVar('ADO_TOKEN'); + if (pat?.trim() && authMode !== 'entra') { + return pat; + } + + const clientId = await resolveDeploymentEnvVar('ADO_CLIENT_ID'); + const clientSecret = await resolveDeploymentEnvVar('ADO_CLIENT_SECRET'); + const tenantId = + (await resolveDeploymentEnvVar('ADO_TENANT_ID')) ?? + (await resolveDeploymentEnvVar('R_MICROSOFT_TENANT_ID')); + + if (!clientId?.trim() || !clientSecret?.trim() || !tenantId?.trim()) { + return null; + } + + if ( + cachedAdoEntraToken && + cachedAdoEntraToken.expiresAt > Date.now() + ADO_ENTRA_TOKEN_EXPIRY_SKEW_MS + ) { + return cachedAdoEntraToken.token; + } + + const { token, expiresIn } = await requestAdoEntraClientCredentialsToken({ + clientId, + clientSecret, + tenantId, + }); + cachedAdoEntraToken = { token, expiresAt: Date.now() + expiresIn * 1000, @@ -307,13 +353,23 @@ export async function resolveAdoToken(): Promise { return token; } -async function resolveAdoDelegatedToken(): Promise { - const linkedAccountId = await resolveDeploymentEnvVar( - 'ADO_LINKED_ACCOUNT_ID', - ); - const clientId = await resolveDeploymentEnvVar('ADO_CLIENT_ID'); - const clientSecret = await resolveDeploymentEnvVar('ADO_CLIENT_SECRET'); +async function resolveAdoDelegatedToken(overrides?: { + linkedAccountId?: string; + clientId?: string; + clientSecret?: string; + tenantId?: string; + fetchImpl?: typeof fetch; +}): Promise { + const linkedAccountId = + overrides?.linkedAccountId ?? + (await resolveDeploymentEnvVar('ADO_LINKED_ACCOUNT_ID')); + const clientId = + overrides?.clientId ?? (await resolveDeploymentEnvVar('ADO_CLIENT_ID')); + const clientSecret = + overrides?.clientSecret ?? + (await resolveDeploymentEnvVar('ADO_CLIENT_SECRET')); const tenantId = + overrides?.tenantId ?? (await resolveDeploymentEnvVar('ADO_TENANT_ID')) ?? (await resolveDeploymentEnvVar('R_MICROSOFT_TENANT_ID')); @@ -369,7 +425,7 @@ async function resolveAdoDelegatedToken(): Promise { ); } - const response = await fetch( + const response = await (overrides?.fetchImpl ?? fetch)( `https://login.microsoftonline.com/${encodeURIComponent(tenantId.trim())}/oauth2/v2.0/token`, { method: 'POST', @@ -496,6 +552,70 @@ function buildAdoBasicAuthHeader(token: string): string { return `Basic ${Buffer.from(`:${token}`, 'utf8').toString('base64')}`; } +/** + * Carries the HTTP status alongside the message so callers can tell a rejected + * credential apart from any other failure and replace the raw status with + * actionable text (see {@link describeAdoApiError}). + */ +export class AdoApiError extends Error { + readonly status: number; + + constructor(status: number, statusText: string) { + super(`Azure DevOps API request failed: ${status} ${statusText}`); + this.name = 'AdoApiError'; + this.status = status; + } +} + +/** + * Azure DevOps answers a rejected credential with a 203 sign-in page as often + * as it answers 401/403, so all three mean "this credential cannot do that". + */ +function isAdoAuthorizationFailureStatus(status: number): boolean { + return status === 203 || status === 401 || status === 403; +} + +/** + * What an admin should actually do about a rejected credential. Entra tokens + * are minted from the `.default` scope, so an app registration that was never + * granted the Azure DevOps API permissions — or whose newly added permissions + * were never saved in the Azure portal — authenticates fine and fails on every + * API call instead. + */ +function buildAdoCredentialRejectionMessage({ + authMode, + status, +}: { + authMode: AdoAuthMode; + status?: number; +}): string { + const suffix = status === undefined ? '' : ` (status ${status})`; + + if (authMode === 'pat') { + return `Azure DevOps rejected the access token${suffix}. Confirm it is active, belongs to the organization, and has Code read access.`; + } + + return `Azure DevOps rejected the Microsoft Entra credential${suffix}. The app registration is most likely missing Azure DevOps API permissions: add ${ADO_ENTRA_REQUIRED_API_PERMISSIONS_TEXT}, save them in the Azure portal (newly added permissions do not apply until they are saved), grant admin consent, then try again.`; +} + +/** + * Message to surface for a failed Azure DevOps call. Rejected credentials get + * mode-specific remediation; everything else keeps its original message. + */ +export async function describeAdoApiError(error: unknown): Promise { + if ( + !(error instanceof AdoApiError) || + !isAdoAuthorizationFailureStatus(error.status) + ) { + return error instanceof Error ? error.message : String(error); + } + + return buildAdoCredentialRejectionMessage({ + authMode: await resolveAdoAuthMode(), + status: error.status, + }); +} + async function requestAdoJson({ organizationApiBaseUrl, fetchImpl = fetch, @@ -529,9 +649,7 @@ async function requestAdoJson({ ); if (![200, 201].includes(response.status)) { - throw new Error( - `Azure DevOps API request failed: ${response.status} ${response.statusText}`, - ); + throw new AdoApiError(response.status, response.statusText); } return { @@ -719,14 +837,22 @@ export type AdoTokenValidationResult = | { status: 'invalid'; error: string } | { status: 'unknown'; error: string }; -export async function validateAdoToken({ +/** + * Single real Azure DevOps call every credential must be able to make. Used to + * verify a credential before it is persisted, so a PAT without Code access or + * an Entra app registration without API permissions fails at save time with + * remediation instead of at the first sync with a bare 401. + */ +async function probeAdoConnectionData({ token, + authMode, organization, baseUrl, fetchImpl = fetch, timeoutMs = ADO_TOKEN_VALIDATION_TIMEOUT_MS, }: { token: string; + authMode: AdoAuthMode; organization: string; baseUrl?: string; fetchImpl?: typeof fetch; @@ -754,13 +880,10 @@ export async function validateAdoToken({ }, ); - // Azure DevOps answers rejected PATs with a 203 sign-in page instead of - // a 401, so treat that status as a definitive rejection too. - if ([203, 401, 403].includes(response.status)) { + if (isAdoAuthorizationFailureStatus(response.status)) { return { status: 'invalid', - error: - 'Azure DevOps rejected the access token. Confirm it is active, belongs to the organization, and has Code read access.', + error: buildAdoCredentialRejectionMessage({ authMode }), }; } @@ -793,6 +916,127 @@ export async function validateAdoToken({ } } +export async function validateAdoToken(params: { + token: string; + organization: string; + baseUrl?: string; + fetchImpl?: typeof fetch; + timeoutMs?: number; +}): Promise { + return probeAdoConnectionData({ ...params, authMode: 'pat' }); +} + +/** + * Verifies a Microsoft Entra service principal end to end: the tenant issues a + * client-credentials token, then that token makes a real Azure DevOps call. + * The token request only proves the client id/secret/tenant are right — the + * `.default` scope succeeds no matter which API permissions were consented, so + * the second step is the one that catches a permission-less app registration. + */ +export async function validateAdoEntraCredentials({ + clientId, + clientSecret, + tenantId, + organization, + baseUrl, + fetchImpl, + timeoutMs = ADO_TOKEN_VALIDATION_TIMEOUT_MS, +}: { + clientId: string; + clientSecret: string; + tenantId: string; + organization: string; + baseUrl?: string; + fetchImpl?: typeof fetch; + timeoutMs?: number; +}): Promise { + let token: string; + + try { + ({ token } = await requestAdoEntraClientCredentialsToken({ + clientId, + clientSecret, + tenantId, + fetchImpl, + timeoutMs, + })); + } catch (error) { + return { + status: 'invalid', + error: `Microsoft Entra did not issue an Azure DevOps token. Confirm the client ID, client secret, and tenant ID are correct and the client secret has not expired. (${ + error instanceof Error ? error.message : String(error) + })`, + }; + } + + return probeAdoConnectionData({ + token, + authMode: 'entra', + organization, + baseUrl, + fetchImpl, + timeoutMs, + }); +} + +/** + * Verifies the Azure DevOps account linked through delegated sign-in, using + * the stored access token (refreshed first when it has expired). + */ +export async function validateAdoDelegatedCredentials({ + linkedAccountId, + clientId, + clientSecret, + tenantId, + organization, + baseUrl, + fetchImpl, + timeoutMs = ADO_TOKEN_VALIDATION_TIMEOUT_MS, +}: { + linkedAccountId: string; + clientId?: string; + clientSecret?: string; + tenantId?: string; + organization: string; + baseUrl?: string; + fetchImpl?: typeof fetch; + timeoutMs?: number; +}): Promise { + let token: string | null; + + try { + token = await resolveAdoDelegatedToken({ + linkedAccountId, + clientId, + clientSecret, + tenantId, + fetchImpl, + }); + } catch (error) { + return { + status: 'invalid', + error: error instanceof Error ? error.message : String(error), + }; + } + + if (!token) { + return { + status: 'invalid', + error: + 'No Azure DevOps account is connected. Connect with Microsoft again, then save.', + }; + } + + return probeAdoConnectionData({ + token, + authMode: 'delegated', + organization, + baseUrl, + fetchImpl, + timeoutMs, + }); +} + function normalizeAdoParentCommentId( parentCommentId: string | number | undefined, ): number { diff --git a/packages/types/src/source-control.ts b/packages/types/src/source-control.ts index a0194bf79..218658f8e 100644 --- a/packages/types/src/source-control.ts +++ b/packages/types/src/source-control.ts @@ -108,6 +108,17 @@ export const sourceControlProviderDescriptors = { } >; +/** + * Azure DevOps API permissions a Microsoft Entra app registration must be + * granted — and admin-consented — before Roomote can read repositories and + * manage service hooks. Both Entra modes request the `.default` scope, so a + * registration with none of these still mints a token and every subsequent + * Azure DevOps call answers 401. Shared so the setup instructions and the + * rejected-credential guidance name the same list. + */ +export const ADO_ENTRA_REQUIRED_API_PERMISSIONS_TEXT = + 'Code, Graph, and User Delegation / Impersonation'; + export const sourceControlTokenBackedProviders = sourceControlProviders.filter( (provider): provider is SourceControlTokenBackedProvider => sourceControlProviderDescriptors[provider].connectionMode === 'token', From 4cec7be1d44b81411c67f7043c96f19d3ab282f0 Mon Sep 17 00:00:00 2001 From: daniel-lxs Date: Wed, 29 Jul 2026 13:52:28 -0500 Subject: [PATCH 02/11] [Fix] Probe the repository listing, not the preview-only connectionData MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- packages/ado/src/__tests__/api.test.ts | 88 ++++++++++++-------- packages/ado/src/api.ts | 107 ++++++++++++++++++------- 2 files changed, 130 insertions(+), 65 deletions(-) diff --git a/packages/ado/src/__tests__/api.test.ts b/packages/ado/src/__tests__/api.test.ts index eb3352ba1..b537e76c9 100644 --- a/packages/ado/src/__tests__/api.test.ts +++ b/packages/ado/src/__tests__/api.test.ts @@ -98,7 +98,7 @@ import { } from '../api'; const ENTRA_PERMISSION_GUIDANCE = - 'add Code, Graph, and User Delegation / Impersonation'; + 'Azure DevOps API permissions Code, Graph, and User Delegation / Impersonation'; /** * Mocks the two hops an Entra credential makes: the Microsoft tenant token @@ -430,19 +430,15 @@ describe('Azure DevOps API helpers', () => { }); }); - it('validates Azure DevOps tokens against the connection data API', async () => { - const fetchMock = vi.fn().mockResolvedValue( - new Response( - JSON.stringify({ - authenticatedUser: { - id: 'user-guid', - uniqueName: 'roomote-bot@acme.example', - providerDisplayName: 'Roomote Bot', - }, - }), - { status: 200 }, - ), - ); + it('validates Azure DevOps tokens against the repository listing the sync uses', async () => { + // Not /_apis/connectionData: that resource is preview-only and answers 400 + // unless the api-version carries `-preview`, which made every valid + // credential look unverifiable. + const fetchMock = vi + .fn() + .mockResolvedValue( + new Response(JSON.stringify({ count: 1, value: [] }), { status: 200 }), + ); await expect( validateAdoToken({ @@ -451,10 +447,10 @@ describe('Azure DevOps API helpers', () => { baseUrl: 'https://dev.azure.com', fetchImpl: fetchMock, }), - ).resolves.toEqual({ status: 'valid', displayName: 'Roomote Bot' }); + ).resolves.toEqual({ status: 'valid' }); expect(fetchMock).toHaveBeenCalledWith( - 'https://dev.azure.com/acme/_apis/connectionData?api-version=7.1', + 'https://dev.azure.com/acme/_apis/git/repositories?api-version=7.1&%24top=1', expect.objectContaining({ method: 'GET', headers: expect.objectContaining({ @@ -484,11 +480,19 @@ describe('Azure DevOps API helpers', () => { }); }); - it('rejects a Microsoft Entra app registration that has no Azure DevOps API permissions', async () => { + it('rejects a Microsoft Entra app registration that cannot reach Azure DevOps', async () => { // The `.default` scope issues a token regardless of what was consented, - // so only the Azure DevOps call reveals the missing permissions. + // so only the Azure DevOps call reveals the problem. Azure DevOps names it + // in the body, which is more specific than anything the status implies. const fetchMock = mockEntraThenAdo( - () => new Response('{}', { status: 401 }), + () => + new Response( + JSON.stringify({ + message: + 'TF401444: Please sign-in at least once as tenant\\tenant\\object-id in a web browser to enable access.', + }), + { status: 401 }, + ), ); const result = await validateAdoEntraCredentials({ @@ -501,26 +505,19 @@ describe('Azure DevOps API helpers', () => { }); expect(result.status).toBe('invalid'); + expect(result.status === 'invalid' && result.error).toContain('TF401444'); expect(result.status === 'invalid' && result.error).toContain( ENTRA_PERMISSION_GUIDANCE, ); expect(String(fetchMock.mock.calls[1]?.[0])).toBe( - 'https://dev.azure.com/acme/_apis/connectionData?api-version=7.1', + 'https://dev.azure.com/acme/_apis/git/repositories?api-version=7.1&%24top=1', ); }); it('accepts a Microsoft Entra service principal that can reach Azure DevOps', async () => { const fetchMock = mockEntraThenAdo( () => - new Response( - JSON.stringify({ - authenticatedUser: { - id: 'user-guid', - providerDisplayName: 'Roomote Service Principal', - }, - }), - { status: 200 }, - ), + new Response(JSON.stringify({ count: 1, value: [] }), { status: 200 }), ); await expect( @@ -532,10 +529,7 @@ describe('Azure DevOps API helpers', () => { baseUrl: 'https://dev.azure.com', fetchImpl: fetchMock, }), - ).resolves.toEqual({ - status: 'valid', - displayName: 'Roomote Service Principal', - }); + ).resolves.toEqual({ status: 'valid' }); expect(fetchMock.mock.calls[1]?.[1]).toMatchObject({ headers: expect.objectContaining({ Authorization: 'Bearer header.payload.signature', @@ -619,6 +613,31 @@ describe('Azure DevOps API helpers', () => { expect(message).toContain(ENTRA_PERMISSION_GUIDANCE); }); + it('surfaces the Azure DevOps explanation captured from a failed request', async () => { + delete process.env.ADO_TOKEN; + process.env.ADO_AUTH_MODE = 'entra'; + const fetchMock = vi.fn().mockResolvedValue( + new Response( + JSON.stringify({ + message: + 'TF401444: Please sign-in at least once as tenant\\tenant\\object-id in a web browser to enable access.', + }), + { status: 401, statusText: 'Unauthorized' }, + ), + ); + + // The repository sync fails here, so this is the error an admin sees. + const error = await listAdoRepositories({ + token: 'header.payload.signature', + organization: 'acme', + baseUrl: 'https://dev.azure.com', + fetchImpl: fetchMock, + }).catch((thrown: unknown) => thrown); + + expect(error).toBeInstanceOf(AdoApiError); + await expect(describeAdoApiError(error)).resolves.toContain('TF401444'); + }); + it('keeps non-authorization Azure DevOps failures verbatim', async () => { await expect( describeAdoApiError(new AdoApiError(500, 'Internal Server Error')), @@ -644,7 +663,8 @@ describe('Azure DevOps API helpers', () => { }), ).resolves.toEqual({ status: 'unknown', - error: 'Could not verify the Azure DevOps token: network unreachable', + error: + 'Could not verify the Azure DevOps credential: network unreachable', }); }); diff --git a/packages/ado/src/api.ts b/packages/ado/src/api.ts index 49da00d5e..ccb708ecd 100644 --- a/packages/ado/src/api.ts +++ b/packages/ado/src/api.ts @@ -25,6 +25,7 @@ const ADO_PROVIDER = 'ado' satisfies SourceControlProvider; const DEFAULT_ADO_BASE_URL = 'https://dev.azure.com'; export const ADO_API_VERSION = '7.1'; const ADO_TOKEN_VALIDATION_TIMEOUT_MS = 10_000; +const ADO_ERROR_MESSAGE_MAX_CHARS = 300; const ADO_ENTRA_TOKEN_SCOPE = 'https://app.vssps.visualstudio.com/.default'; const ADO_ENTRA_RESOURCE_SCOPE = '499b84ac-1321-427f-aa17-267ca6975798/.default'; @@ -559,11 +560,18 @@ function buildAdoBasicAuthHeader(token: string): string { */ export class AdoApiError extends Error { readonly status: number; + /** Azure DevOps' own explanation, when the response body carried one. */ + readonly providerMessage: string | null; - constructor(status: number, statusText: string) { + constructor( + status: number, + statusText: string, + providerMessage: string | null = null, + ) { super(`Azure DevOps API request failed: ${status} ${statusText}`); this.name = 'AdoApiError'; this.status = status; + this.providerMessage = providerMessage; } } @@ -579,23 +587,31 @@ function isAdoAuthorizationFailureStatus(status: number): boolean { * What an admin should actually do about a rejected credential. Entra tokens * are minted from the `.default` scope, so an app registration that was never * granted the Azure DevOps API permissions — or whose newly added permissions - * were never saved in the Azure portal — authenticates fine and fails on every - * API call instead. + * were never saved in the Azure portal, or that was never added to the + * organization — authenticates fine and fails on every API call instead. + * + * Azure DevOps usually names the specific problem in the response body, so + * that leads when present and the remediation follows it. */ function buildAdoCredentialRejectionMessage({ authMode, status, + providerMessage, }: { authMode: AdoAuthMode; status?: number; + providerMessage?: string | null; }): string { const suffix = status === undefined ? '' : ` (status ${status})`; + const detail = providerMessage + ? ` Azure DevOps said: “${providerMessage}”` + : ''; if (authMode === 'pat') { - return `Azure DevOps rejected the access token${suffix}. Confirm it is active, belongs to the organization, and has Code read access.`; + return `Azure DevOps rejected the access token${suffix}.${detail} Confirm it is active, belongs to the organization, and has Code read access.`; } - return `Azure DevOps rejected the Microsoft Entra credential${suffix}. The app registration is most likely missing Azure DevOps API permissions: add ${ADO_ENTRA_REQUIRED_API_PERMISSIONS_TEXT}, save them in the Azure portal (newly added permissions do not apply until they are saved), grant admin consent, then try again.`; + return `Azure DevOps rejected the Microsoft Entra credential${suffix}.${detail} Check that the app registration has the Azure DevOps API permissions ${ADO_ENTRA_REQUIRED_API_PERMISSIONS_TEXT} — added, saved in the Azure portal (newly added permissions do not apply until they are saved) and admin-consented — and that its service principal was added to the Azure DevOps organization.`; } /** @@ -613,6 +629,7 @@ export async function describeAdoApiError(error: unknown): Promise { return buildAdoCredentialRejectionMessage({ authMode: await resolveAdoAuthMode(), status: error.status, + providerMessage: error.providerMessage, }); } @@ -649,7 +666,11 @@ async function requestAdoJson({ ); if (![200, 201].includes(response.status)) { - throw new AdoApiError(response.status, response.statusText); + throw new AdoApiError( + response.status, + response.statusText, + readAdoErrorMessage(await response.text().catch(() => '')), + ); } return { @@ -833,17 +854,48 @@ export async function getAdoPullRequest({ } export type AdoTokenValidationResult = - | { status: 'valid'; displayName: string } + | { status: 'valid' } | { status: 'invalid'; error: string } | { status: 'unknown'; error: string }; /** - * Single real Azure DevOps call every credential must be able to make. Used to - * verify a credential before it is persisted, so a PAT without Code access or - * an Entra app registration without API permissions fails at save time with - * remediation instead of at the first sync with a bare 401. + * Azure DevOps explains most rejections in the response body + * (`{"message": "TF401444: Please sign-in at least once as …"}`), which is + * more specific than anything we can infer from the status alone. Pull it out + * so the admin sees the provider's own diagnosis next to the remediation. + */ +function readAdoErrorMessage(body: string): string | null { + try { + const parsed: unknown = JSON.parse(body); + const message = + typeof parsed === 'object' && parsed !== null + ? (parsed as { message?: unknown }).message + : null; + + if (typeof message !== 'string' || !message.trim()) { + return null; + } + + const collapsed = message.trim().replace(/\s+/g, ' '); + return collapsed.length > ADO_ERROR_MESSAGE_MAX_CHARS + ? `${collapsed.slice(0, ADO_ERROR_MESSAGE_MAX_CHARS)}…` + : collapsed; + } catch { + return null; + } +} + +/** + * Single real Azure DevOps call every credential must be able to make. This is + * the same repository listing the sync performs, so a PAT without Code access + * or an Entra app registration that cannot reach the organization fails at + * save time with remediation instead of at the first sync with a bare 401. + * + * Deliberately not `/_apis/connectionData`: that resource is preview-only and + * answers 400 unless the api-version carries a `-preview` suffix, which would + * make a working credential look unverifiable rather than valid. */ -async function probeAdoConnectionData({ +async function probeAdoCredential({ token, authMode, organization, @@ -867,8 +919,9 @@ async function probeAdoConnectionData({ organization, }); const response = await fetchImpl( - buildAdoApiUrl(organizationApiBaseUrl, '/_apis/connectionData', { + buildAdoApiUrl(organizationApiBaseUrl, '/_apis/git/repositories', { 'api-version': ADO_API_VERSION, + $top: 1, }), { method: 'GET', @@ -883,33 +936,25 @@ async function probeAdoConnectionData({ if (isAdoAuthorizationFailureStatus(response.status)) { return { status: 'invalid', - error: buildAdoCredentialRejectionMessage({ authMode }), + error: buildAdoCredentialRejectionMessage({ + authMode, + providerMessage: readAdoErrorMessage(await response.text()), + }), }; } if (response.status !== 200) { return { status: 'unknown', - error: `Could not verify the Azure DevOps token: ${response.status} ${response.statusText}`, + error: `Could not verify the Azure DevOps credential: ${response.status} ${response.statusText}`, }; } - const { authenticatedUser } = adoConnectionDataSchema.parse( - await response.json(), - ); - - return { - status: 'valid', - displayName: - authenticatedUser.providerDisplayName ?? - authenticatedUser.displayName ?? - authenticatedUser.uniqueName ?? - authenticatedUser.id, - }; + return { status: 'valid' }; } catch (error) { return { status: 'unknown', - error: `Could not verify the Azure DevOps token: ${ + error: `Could not verify the Azure DevOps credential: ${ error instanceof Error ? error.message : String(error) }`, }; @@ -923,7 +968,7 @@ export async function validateAdoToken(params: { fetchImpl?: typeof fetch; timeoutMs?: number; }): Promise { - return probeAdoConnectionData({ ...params, authMode: 'pat' }); + return probeAdoCredential({ ...params, authMode: 'pat' }); } /** @@ -969,7 +1014,7 @@ export async function validateAdoEntraCredentials({ }; } - return probeAdoConnectionData({ + return probeAdoCredential({ token, authMode: 'entra', organization, @@ -1027,7 +1072,7 @@ export async function validateAdoDelegatedCredentials({ }; } - return probeAdoConnectionData({ + return probeAdoCredential({ token, authMode: 'delegated', organization, From f70f1b8fb10461fc994c3e9080f21fb8ed6f84ff Mon Sep 17 00:00:00 2001 From: daniel-lxs Date: Wed, 29 Jul 2026 14:01:51 -0500 Subject: [PATCH 03/11] [Fix] Fall back past the blank values the config form submits MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .../commands/source-control/index.test.ts | 42 +++++++++++++++++++ .../src/trpc/commands/source-control/index.ts | 34 ++++++++------- 2 files changed, 61 insertions(+), 15 deletions(-) diff --git a/apps/web/src/trpc/commands/source-control/index.test.ts b/apps/web/src/trpc/commands/source-control/index.test.ts index 8c474c48a..bf4c1be18 100644 --- a/apps/web/src/trpc/commands/source-control/index.test.ts +++ b/apps/web/src/trpc/commands/source-control/index.test.ts @@ -568,6 +568,48 @@ describe('source-control commands', () => { expect(mockValidateAdoToken).not.toHaveBeenCalled(); }); + it('probes runtime-configured Entra credentials the form submits as blank', async () => { + // The Settings form sends an empty string for every field already + // satisfied by a runtime env var. Falling back on those blanks is what + // makes the probe run at all — otherwise the whole check is skipped and + // an unusable credential saves cleanly. + mockResolveAdoOrganization.mockResolvedValue('acme'); + mockResolveDeploymentEnvVar.mockImplementation( + async (name: string) => + ({ + ADO_CLIENT_ID: 'runtime-client-id', + ADO_CLIENT_SECRET: 'runtime-client-secret', + ADO_TENANT_ID: 'runtime-tenant-id', + })[name] ?? null, + ); + mockValidateAdoEntraCredentials.mockResolvedValue({ + status: 'invalid', + error: 'Azure DevOps rejected the Microsoft Entra credential.', + }); + + await expect( + assertValidSourceControlConfigInput({ + provider: 'ado', + values: { + ADO_ORGANIZATION: '', + ADO_CLIENT_ID: '', + ADO_CLIENT_SECRET: '', + ADO_TENANT_ID: '', + ADO_AUTH_MODE: 'entra', + ADO_LINKED_ACCOUNT_ID: '', + }, + }), + ).rejects.toThrow('Azure DevOps rejected the Microsoft Entra credential.'); + + expect(mockValidateAdoEntraCredentials).toHaveBeenCalledWith({ + clientId: 'runtime-client-id', + clientSecret: 'runtime-client-secret', + tenantId: 'runtime-tenant-id', + organization: 'acme', + baseUrl: undefined, + }); + }); + it('probes the delegated Azure DevOps account once it is linked', async () => { await assertValidSourceControlConfigInput({ provider: 'ado', diff --git a/apps/web/src/trpc/commands/source-control/index.ts b/apps/web/src/trpc/commands/source-control/index.ts index 86f3d1355..753388dae 100644 --- a/apps/web/src/trpc/commands/source-control/index.ts +++ b/apps/web/src/trpc/commands/source-control/index.ts @@ -791,35 +791,35 @@ export async function assertValidSourceControlConfigInput(params: { : undefined; const nextAdoToken = params.provider === 'ado' - ? (params.values?.['ADO_TOKEN']?.trim() ?? - (await resolveDeploymentEnvVar('ADO_TOKEN'))) + ? params.values?.['ADO_TOKEN']?.trim() || + (await resolveDeploymentEnvVar('ADO_TOKEN')) : undefined; const nextAdoClientId = params.provider === 'ado' - ? (params.values?.['ADO_CLIENT_ID']?.trim() ?? - (await resolveDeploymentEnvVar('ADO_CLIENT_ID'))) + ? params.values?.['ADO_CLIENT_ID']?.trim() || + (await resolveDeploymentEnvVar('ADO_CLIENT_ID')) : undefined; const nextAdoClientSecret = params.provider === 'ado' - ? (params.values?.['ADO_CLIENT_SECRET']?.trim() ?? - (await resolveDeploymentEnvVar('ADO_CLIENT_SECRET'))) + ? params.values?.['ADO_CLIENT_SECRET']?.trim() || + (await resolveDeploymentEnvVar('ADO_CLIENT_SECRET')) : undefined; const nextAdoTenantId = params.provider === 'ado' - ? (params.values?.['ADO_TENANT_ID']?.trim() ?? - (await resolveDeploymentEnvVar('ADO_TENANT_ID')) ?? - (await resolveDeploymentEnvVar('R_MICROSOFT_TENANT_ID'))) + ? params.values?.['ADO_TENANT_ID']?.trim() || + (await resolveDeploymentEnvVar('ADO_TENANT_ID')) || + (await resolveDeploymentEnvVar('R_MICROSOFT_TENANT_ID')) : undefined; const nextAdoAuthMode = params.provider === 'ado' - ? (params.values?.['ADO_AUTH_MODE']?.trim() ?? - (await resolveDeploymentEnvVar('ADO_AUTH_MODE')) ?? - (nextAdoToken ? 'pat' : 'entra')) + ? params.values?.['ADO_AUTH_MODE']?.trim() || + (await resolveDeploymentEnvVar('ADO_AUTH_MODE')) || + (nextAdoToken ? 'pat' : 'entra') : undefined; const nextAdoLinkedAccountId = params.provider === 'ado' - ? (params.values?.['ADO_LINKED_ACCOUNT_ID']?.trim() ?? - (await resolveDeploymentEnvVar('ADO_LINKED_ACCOUNT_ID'))) + ? params.values?.['ADO_LINKED_ACCOUNT_ID']?.trim() || + (await resolveDeploymentEnvVar('ADO_LINKED_ACCOUNT_ID')) : undefined; if ( @@ -865,8 +865,12 @@ export async function assertValidSourceControlConfigInput(params: { return; } + // `||`, not `??`, on every resolution above and below: the Settings form + // submits an empty string for each field already satisfied by a runtime env + // var, and `??` would keep that empty string instead of falling back to the + // configured value — which silently skipped the whole probe. const nextAdoOrganization = - params.values?.['ADO_ORGANIZATION']?.trim() ?? + params.values?.['ADO_ORGANIZATION']?.trim() || (await Ado.resolveAdoOrganization()); // Without an organization there is nothing to make a real API call against; From c60f5c437b8f6b4b10ad8bf66dad5263d40fd007 Mon Sep 17 00:00:00 2001 From: daniel-lxs Date: Wed, 29 Jul 2026 14:11:22 -0500 Subject: [PATCH 04/11] [Fix] Match the token host exactly in the Entra fetch mock 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 --- packages/ado/src/__tests__/api.test.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/packages/ado/src/__tests__/api.test.ts b/packages/ado/src/__tests__/api.test.ts index b537e76c9..899d17687 100644 --- a/packages/ado/src/__tests__/api.test.ts +++ b/packages/ado/src/__tests__/api.test.ts @@ -106,7 +106,9 @@ const ENTRA_PERMISSION_GUIDANCE = */ function mockEntraThenAdo(adoResponse: () => Response) { return vi.fn().mockImplementation(async (url) => { - if (String(url).includes('login.microsoftonline.com')) { + // Match on the exact host: a substring check would also match a URL that + // merely contains the tenant host somewhere in its path or query. + if (new URL(String(url)).hostname === 'login.microsoftonline.com') { return new Response( JSON.stringify({ access_token: 'header.payload.signature', From 6af99ec2cebe84a9889063188d4379c2c072d271 Mon Sep 17 00:00:00 2001 From: daniel-lxs Date: Wed, 29 Jul 2026 15:33:39 -0500 Subject: [PATCH 05/11] [Improve] Trim the rejected-credential toast to one remediation sentence 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 --- packages/ado/src/__tests__/api.test.ts | 2 +- packages/ado/src/api.ts | 8 +++++--- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/packages/ado/src/__tests__/api.test.ts b/packages/ado/src/__tests__/api.test.ts index 899d17687..c6b45cd1d 100644 --- a/packages/ado/src/__tests__/api.test.ts +++ b/packages/ado/src/__tests__/api.test.ts @@ -98,7 +98,7 @@ import { } from '../api'; const ENTRA_PERMISSION_GUIDANCE = - 'Azure DevOps API permissions Code, Graph, and User Delegation / Impersonation'; + 'Code, Graph, and User Delegation / Impersonation API permissions (saved and admin-consented)'; /** * Mocks the two hops an Entra credential makes: the Microsoft tenant token diff --git a/packages/ado/src/api.ts b/packages/ado/src/api.ts index ccb708ecd..90a2d980c 100644 --- a/packages/ado/src/api.ts +++ b/packages/ado/src/api.ts @@ -25,7 +25,7 @@ const ADO_PROVIDER = 'ado' satisfies SourceControlProvider; const DEFAULT_ADO_BASE_URL = 'https://dev.azure.com'; export const ADO_API_VERSION = '7.1'; const ADO_TOKEN_VALIDATION_TIMEOUT_MS = 10_000; -const ADO_ERROR_MESSAGE_MAX_CHARS = 300; +const ADO_ERROR_MESSAGE_MAX_CHARS = 200; const ADO_ENTRA_TOKEN_SCOPE = 'https://app.vssps.visualstudio.com/.default'; const ADO_ENTRA_RESOURCE_SCOPE = '499b84ac-1321-427f-aa17-267ca6975798/.default'; @@ -591,7 +591,9 @@ function isAdoAuthorizationFailureStatus(status: number): boolean { * organization — authenticates fine and fails on every API call instead. * * Azure DevOps usually names the specific problem in the response body, so - * that leads when present and the remediation follows it. + * that leads when present and one compact remediation sentence follows it. + * This surfaces as a toast, so the full add-save-consent walkthrough stays in + * the setup/Settings guidance and the docs rather than being repeated here. */ function buildAdoCredentialRejectionMessage({ authMode, @@ -611,7 +613,7 @@ function buildAdoCredentialRejectionMessage({ return `Azure DevOps rejected the access token${suffix}.${detail} Confirm it is active, belongs to the organization, and has Code read access.`; } - return `Azure DevOps rejected the Microsoft Entra credential${suffix}.${detail} Check that the app registration has the Azure DevOps API permissions ${ADO_ENTRA_REQUIRED_API_PERMISSIONS_TEXT} — added, saved in the Azure portal (newly added permissions do not apply until they are saved) and admin-consented — and that its service principal was added to the Azure DevOps organization.`; + return `Azure DevOps rejected the Microsoft Entra credential${suffix}.${detail} Check that the app registration has the ${ADO_ENTRA_REQUIRED_API_PERMISSIONS_TEXT} API permissions (saved and admin-consented) and that its service principal was added to the Azure DevOps organization.`; } /** From b16644a93c02ad4a29ed0c4ee464cdc74258d4be Mon Sep 17 00:00:00 2001 From: daniel-lxs Date: Wed, 29 Jul 2026 15:36:03 -0500 Subject: [PATCH 06/11] [Improve] Fit the whole TF401444 sentence in the captured provider message Co-Authored-By: Claude Opus 5 --- packages/ado/src/api.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/packages/ado/src/api.ts b/packages/ado/src/api.ts index 90a2d980c..705b446c4 100644 --- a/packages/ado/src/api.ts +++ b/packages/ado/src/api.ts @@ -25,7 +25,9 @@ const ADO_PROVIDER = 'ado' satisfies SourceControlProvider; const DEFAULT_ADO_BASE_URL = 'https://dev.azure.com'; export const ADO_API_VERSION = '7.1'; const ADO_TOKEN_VALIDATION_TIMEOUT_MS = 10_000; -const ADO_ERROR_MESSAGE_MAX_CHARS = 200; +// Fits the whole TF401444 sentence (~204 chars with its three GUIDs) while +// still keeping pathological provider messages toast-sized. +const ADO_ERROR_MESSAGE_MAX_CHARS = 240; const ADO_ENTRA_TOKEN_SCOPE = 'https://app.vssps.visualstudio.com/.default'; const ADO_ENTRA_RESOURCE_SCOPE = '499b84ac-1321-427f-aa17-267ca6975798/.default'; From 15d2447b39918326553a391d35ca6fbd149890d2 Mon Sep 17 00:00:00 2001 From: daniel-lxs Date: Wed, 29 Jul 2026 15:43:21 -0500 Subject: [PATCH 07/11] [Improve] Split the Entra guidance across toast, inline hint, and docs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .../settings/SourceControlConfigForm.tsx | 17 +++++++---- packages/ado/src/__tests__/api.test.ts | 12 ++++++-- packages/ado/src/api.ts | 28 +++++++++++++++---- 3 files changed, 43 insertions(+), 14 deletions(-) diff --git a/apps/web/src/components/settings/SourceControlConfigForm.tsx b/apps/web/src/components/settings/SourceControlConfigForm.tsx index 4276d7175..8bbc1a111 100644 --- a/apps/web/src/components/settings/SourceControlConfigForm.tsx +++ b/apps/web/src/components/settings/SourceControlConfigForm.tsx @@ -279,14 +279,19 @@ export function SourceControlConfigForm({ ) : null} {isAdo && adoAuthMode !== 'pat' ? (

- The Microsoft Entra app registration needs the Azure DevOps API - permissions{' '} + The Microsoft Entra app registration needs the{' '} {ADO_ENTRA_REQUIRED_API_PERMISSIONS_TEXT} - - , saved in the Azure portal and admin-consented for the tenant. - Permissions that were added but never saved do not apply, and Azure - DevOps answers every call with a 401. + {' '} + API permissions, saved and admin-consented.{' '} + + Setup guide +

) : null} {isAdo && adoAuthMode === 'delegated' ? ( diff --git a/packages/ado/src/__tests__/api.test.ts b/packages/ado/src/__tests__/api.test.ts index c6b45cd1d..e106535d0 100644 --- a/packages/ado/src/__tests__/api.test.ts +++ b/packages/ado/src/__tests__/api.test.ts @@ -98,7 +98,7 @@ import { } from '../api'; const ENTRA_PERMISSION_GUIDANCE = - 'Code, Graph, and User Delegation / Impersonation API permissions (saved and admin-consented)'; + 'likely missing API permissions or was not added to the organization'; /** * Mocks the two hops an Entra credential makes: the Microsoft tenant token @@ -491,7 +491,7 @@ describe('Azure DevOps API helpers', () => { new Response( JSON.stringify({ message: - 'TF401444: Please sign-in at least once as tenant\\tenant\\object-id in a web browser to enable access.', + 'TF401444: Please sign-in at least once as 11111111-2222-3333-4444-555555555555\\\\11111111-2222-3333-4444-555555555555\\\\66666666-7777-8888-9999-000000000000 in a web browser to enable access.', }), { status: 401 }, ), @@ -511,6 +511,14 @@ describe('Azure DevOps API helpers', () => { expect(result.status === 'invalid' && result.error).toContain( ENTRA_PERMISSION_GUIDANCE, ); + // The GUID chain is collapsed for the toast; the raw message stays on + // the thrown AdoApiError for logs. + expect(result.status === 'invalid' && result.error).not.toContain( + '11111111-2222-3333-4444-555555555555', + ); + expect(result.status === 'invalid' && result.error).toContain( + 'sign-in at least once as …', + ); expect(String(fetchMock.mock.calls[1]?.[0])).toBe( 'https://dev.azure.com/acme/_apis/git/repositories?api-version=7.1&%24top=1', ); diff --git a/packages/ado/src/api.ts b/packages/ado/src/api.ts index 705b446c4..6933f3e10 100644 --- a/packages/ado/src/api.ts +++ b/packages/ado/src/api.ts @@ -585,6 +585,22 @@ function isAdoAuthorizationFailureStatus(status: number): boolean { return status === 203 || status === 401 || status === 403; } +const GUID_PATTERN = + /[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}/gi; + +/** + * Provider messages like TF401444 embed `tenant\tenant\objectId` GUID chains + * that dominate a toast without helping anyone read it. Collapse them to a + * single ellipsis for display; the untrimmed message stays on the thrown + * {@link AdoApiError} for logs. + */ +function compactAdoProviderMessage(message: string): string { + return message + .replace(GUID_PATTERN, '…') + .replace(/…(\\+…)+/g, '…') + .replace(/\s{2,}/g, ' '); +} + /** * What an admin should actually do about a rejected credential. Entra tokens * are minted from the `.default` scope, so an app registration that was never @@ -592,10 +608,10 @@ function isAdoAuthorizationFailureStatus(status: number): boolean { * were never saved in the Azure portal, or that was never added to the * organization — authenticates fine and fails on every API call instead. * - * Azure DevOps usually names the specific problem in the response body, so - * that leads when present and one compact remediation sentence follows it. - * This surfaces as a toast, so the full add-save-consent walkthrough stays in - * the setup/Settings guidance and the docs rather than being repeated here. + * This surfaces as a toast, so it stays at one line of diagnosis plus Azure + * DevOps' own (GUID-compacted) explanation. The permission names and the + * add-save-consent walkthrough live in the setup/Settings guidance and the + * docs, which the guidance links to. */ function buildAdoCredentialRejectionMessage({ authMode, @@ -608,14 +624,14 @@ function buildAdoCredentialRejectionMessage({ }): string { const suffix = status === undefined ? '' : ` (status ${status})`; const detail = providerMessage - ? ` Azure DevOps said: “${providerMessage}”` + ? ` Azure DevOps said: “${compactAdoProviderMessage(providerMessage)}”` : ''; if (authMode === 'pat') { return `Azure DevOps rejected the access token${suffix}.${detail} Confirm it is active, belongs to the organization, and has Code read access.`; } - return `Azure DevOps rejected the Microsoft Entra credential${suffix}.${detail} Check that the app registration has the ${ADO_ENTRA_REQUIRED_API_PERMISSIONS_TEXT} API permissions (saved and admin-consented) and that its service principal was added to the Azure DevOps organization.`; + return `Azure DevOps rejected the Microsoft Entra credential${suffix} — the app registration is likely missing API permissions or was not added to the organization.${detail}`; } /** From 01292094f973816a7d682bbbbe3a9901b67be567 Mon Sep 17 00:00:00 2001 From: daniel-lxs Date: Wed, 29 Jul 2026 15:43:54 -0500 Subject: [PATCH 08/11] [Fix] Drop the permissions-list import the toast no longer uses Co-Authored-By: Claude Opus 5 --- packages/ado/src/api.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/ado/src/api.ts b/packages/ado/src/api.ts index 6933f3e10..1f6cc0a7e 100644 --- a/packages/ado/src/api.ts +++ b/packages/ado/src/api.ts @@ -1,7 +1,6 @@ import { z } from 'zod'; import { - ADO_ENTRA_REQUIRED_API_PERMISSIONS_TEXT, ALL_REPOSITORIES, buildRepositoryCloneUrl, stripCloneUrlUserInfo, From 213224173cd93f15518c7f323521b03623858d69 Mon Sep 17 00:00:00 2001 From: daniel-lxs Date: Wed, 29 Jul 2026 15:50:25 -0500 Subject: [PATCH 09/11] [Improve] Lead the rejection toast with Azure DevOps' own explanation 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 --- packages/ado/src/__tests__/api.test.ts | 2 +- packages/ado/src/api.ts | 18 +++++++++--------- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/packages/ado/src/__tests__/api.test.ts b/packages/ado/src/__tests__/api.test.ts index e106535d0..c65d858bb 100644 --- a/packages/ado/src/__tests__/api.test.ts +++ b/packages/ado/src/__tests__/api.test.ts @@ -98,7 +98,7 @@ import { } from '../api'; const ENTRA_PERMISSION_GUIDANCE = - 'likely missing API permissions or was not added to the organization'; + 'Check API permissions and organization membership.'; /** * Mocks the two hops an Entra credential makes: the Microsoft tenant token diff --git a/packages/ado/src/api.ts b/packages/ado/src/api.ts index 1f6cc0a7e..39551f4eb 100644 --- a/packages/ado/src/api.ts +++ b/packages/ado/src/api.ts @@ -607,10 +607,10 @@ function compactAdoProviderMessage(message: string): string { * were never saved in the Azure portal, or that was never added to the * organization — authenticates fine and fails on every API call instead. * - * This surfaces as a toast, so it stays at one line of diagnosis plus Azure - * DevOps' own (GUID-compacted) explanation. The permission names and the - * add-save-consent walkthrough live in the setup/Settings guidance and the - * docs, which the guidance links to. + * This surfaces as a toast: a short lead-in, then Azure DevOps' own + * (GUID-compacted) explanation, then one clipped action. The permission names + * and the add-save-consent walkthrough live in the setup/Settings guidance + * and the docs, which the guidance links to. */ function buildAdoCredentialRejectionMessage({ authMode, @@ -622,15 +622,15 @@ function buildAdoCredentialRejectionMessage({ providerMessage?: string | null; }): string { const suffix = status === undefined ? '' : ` (status ${status})`; - const detail = providerMessage - ? ` Azure DevOps said: “${compactAdoProviderMessage(providerMessage)}”` - : ''; + const quote = providerMessage + ? `: “${compactAdoProviderMessage(providerMessage)}”` + : '.'; if (authMode === 'pat') { - return `Azure DevOps rejected the access token${suffix}.${detail} Confirm it is active, belongs to the organization, and has Code read access.`; + return `Azure DevOps rejected the access token${suffix}${quote} Confirm it is active, belongs to the organization, and has Code read access.`; } - return `Azure DevOps rejected the Microsoft Entra credential${suffix} — the app registration is likely missing API permissions or was not added to the organization.${detail}`; + return `Azure DevOps rejected the Microsoft Entra credential${suffix}${quote} Check API permissions and organization membership.`; } /** From c29e98b901d09c91d1f8cf1cec4024327e0f6d59 Mon Sep 17 00:00:00 2001 From: daniel-lxs Date: Wed, 29 Jul 2026 15:51:42 -0500 Subject: [PATCH 10/11] [Improve] Drop em dashes from the copy this branch added Co-Authored-By: Claude Opus 5 --- .../src/app/(onboarding)/setup/AdoSourceControlConfig.tsx | 6 +++--- apps/web/src/trpc/commands/source-control/index.test.ts | 2 +- apps/web/src/trpc/commands/source-control/index.ts | 4 ++-- packages/ado/src/api.ts | 6 +++--- packages/types/src/source-control.ts | 2 +- 5 files changed, 10 insertions(+), 10 deletions(-) diff --git a/apps/web/src/app/(onboarding)/setup/AdoSourceControlConfig.tsx b/apps/web/src/app/(onboarding)/setup/AdoSourceControlConfig.tsx index a0742d228..92e701809 100644 --- a/apps/web/src/app/(onboarding)/setup/AdoSourceControlConfig.tsx +++ b/apps/web/src/app/(onboarding)/setup/AdoSourceControlConfig.tsx @@ -127,9 +127,9 @@ export function AdoSourceControlInstructions({ save the permissions {' '} - and grant admin consent for the tenant — Azure does not apply - permissions until they are saved, and a missing one only shows up later - as a 401 when Roomote syncs repositories. + and grant admin consent for the tenant. Azure does not apply permissions + until they are saved, and a missing one only shows up later as a 401 + when Roomote syncs repositories.

{authMode === 'delegated' && ( <> diff --git a/apps/web/src/trpc/commands/source-control/index.test.ts b/apps/web/src/trpc/commands/source-control/index.test.ts index bf4c1be18..aaa719cfc 100644 --- a/apps/web/src/trpc/commands/source-control/index.test.ts +++ b/apps/web/src/trpc/commands/source-control/index.test.ts @@ -571,7 +571,7 @@ describe('source-control commands', () => { it('probes runtime-configured Entra credentials the form submits as blank', async () => { // The Settings form sends an empty string for every field already // satisfied by a runtime env var. Falling back on those blanks is what - // makes the probe run at all — otherwise the whole check is skipped and + // makes the probe run at all; otherwise the whole check is skipped and // an unusable credential saves cleanly. mockResolveAdoOrganization.mockResolvedValue('acme'); mockResolveDeploymentEnvVar.mockImplementation( diff --git a/apps/web/src/trpc/commands/source-control/index.ts b/apps/web/src/trpc/commands/source-control/index.ts index 753388dae..ef100a049 100644 --- a/apps/web/src/trpc/commands/source-control/index.ts +++ b/apps/web/src/trpc/commands/source-control/index.ts @@ -868,7 +868,7 @@ export async function assertValidSourceControlConfigInput(params: { // `||`, not `??`, on every resolution above and below: the Settings form // submits an empty string for each field already satisfied by a runtime env // var, and `??` would keep that empty string instead of falling back to the - // configured value — which silently skipped the whole probe. + // configured value, which silently skipped the whole probe. const nextAdoOrganization = params.values?.['ADO_ORGANIZATION']?.trim() || (await Ado.resolveAdoOrganization()); @@ -889,7 +889,7 @@ export async function assertValidSourceControlConfigInput(params: { // Every mode is verified with one real Azure DevOps call before the values // are persisted. Both Entra modes request the `.default` scope, so a token // is issued regardless of which API permissions the app registration was - // granted — without this probe a permission-less registration saves cleanly + // granted. Without this probe a permission-less registration saves cleanly // and 401s on every later call. if (nextAdoAuthMode === 'pat') { if (nextAdoToken) { diff --git a/packages/ado/src/api.ts b/packages/ado/src/api.ts index 39551f4eb..f01119cd8 100644 --- a/packages/ado/src/api.ts +++ b/packages/ado/src/api.ts @@ -603,9 +603,9 @@ function compactAdoProviderMessage(message: string): string { /** * What an admin should actually do about a rejected credential. Entra tokens * are minted from the `.default` scope, so an app registration that was never - * granted the Azure DevOps API permissions — or whose newly added permissions + * granted the Azure DevOps API permissions, or whose newly added permissions * were never saved in the Azure portal, or that was never added to the - * organization — authenticates fine and fails on every API call instead. + * organization, authenticates fine and fails on every API call instead. * * This surfaces as a toast: a short lead-in, then Azure DevOps' own * (GUID-compacted) explanation, then one clipped action. The permission names @@ -993,7 +993,7 @@ export async function validateAdoToken(params: { /** * Verifies a Microsoft Entra service principal end to end: the tenant issues a * client-credentials token, then that token makes a real Azure DevOps call. - * The token request only proves the client id/secret/tenant are right — the + * The token request only proves the client id/secret/tenant are right; the * `.default` scope succeeds no matter which API permissions were consented, so * the second step is the one that catches a permission-less app registration. */ diff --git a/packages/types/src/source-control.ts b/packages/types/src/source-control.ts index 218658f8e..63caa5f1e 100644 --- a/packages/types/src/source-control.ts +++ b/packages/types/src/source-control.ts @@ -110,7 +110,7 @@ export const sourceControlProviderDescriptors = { /** * Azure DevOps API permissions a Microsoft Entra app registration must be - * granted — and admin-consented — before Roomote can read repositories and + * granted and admin-consented before Roomote can read repositories and * manage service hooks. Both Entra modes request the `.default` scope, so a * registration with none of these still mints a token and every subsequent * Azure DevOps call answers 401. Shared so the setup instructions and the From 9c5f3fd44ba47b8c2fd7008d380cc870afb47c40 Mon Sep 17 00:00:00 2001 From: daniel-lxs Date: Wed, 29 Jul 2026 16:12:45 -0500 Subject: [PATCH 11/11] [Fix] Only definitive token-endpoint rejections block saving 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 --- packages/ado/src/__tests__/api.test.ts | 69 ++++++++++++++++++++++++++ packages/ado/src/api.ts | 69 +++++++++++++++++++++++--- 2 files changed, 130 insertions(+), 8 deletions(-) diff --git a/packages/ado/src/__tests__/api.test.ts b/packages/ado/src/__tests__/api.test.ts index c65d858bb..6a74e034a 100644 --- a/packages/ado/src/__tests__/api.test.ts +++ b/packages/ado/src/__tests__/api.test.ts @@ -570,6 +570,75 @@ describe('Azure DevOps API helpers', () => { expect(fetchMock).toHaveBeenCalledTimes(1); }); + it('returns unknown when the Entra token endpoint is unreachable or failing', async () => { + // A Microsoft outage or network blip must not block saving; only a + // definitive rejection may. + const outage = await validateAdoEntraCredentials({ + clientId: 'client-id', + clientSecret: 'client-secret', + tenantId: 'tenant-id', + organization: 'acme', + baseUrl: 'https://dev.azure.com', + fetchImpl: vi + .fn() + .mockResolvedValue(new Response('{}', { status: 503 })), + }); + expect(outage.status).toBe('unknown'); + + const network = await validateAdoEntraCredentials({ + clientId: 'client-id', + clientSecret: 'client-secret', + tenantId: 'tenant-id', + organization: 'acme', + baseUrl: 'https://dev.azure.com', + fetchImpl: vi + .fn() + .mockRejectedValue(new TypeError('fetch failed')), + }); + expect(network.status).toBe('unknown'); + }); + + it('rejects a delegated connection whose refresh grant was refused', async () => { + mockAuthAccountsFindFirst.mockResolvedValue({ + id: 'account-1', + accountId: 'ado-user@example.com', + accessToken: 'expired.token.value', + refreshToken: 'refresh-token', + accessTokenExpiresAt: new Date(Date.now() - 60_000), + }); + + // 400 invalid_grant: the refresh token is expired or revoked. + const refused = await validateAdoDelegatedCredentials({ + linkedAccountId: 'ado-user@example.com', + clientId: 'client-id', + clientSecret: 'client-secret', + tenantId: 'tenant-id', + organization: 'acme', + baseUrl: 'https://dev.azure.com', + fetchImpl: vi + .fn() + .mockResolvedValue(new Response('{}', { status: 400 })), + }); + expect(refused).toEqual({ + status: 'invalid', + error: 'Azure DevOps delegated token refresh failed: 400 ', + }); + + // A refresh blocked by a network failure is unverifiable, not invalid. + const network = await validateAdoDelegatedCredentials({ + linkedAccountId: 'ado-user@example.com', + clientId: 'client-id', + clientSecret: 'client-secret', + tenantId: 'tenant-id', + organization: 'acme', + baseUrl: 'https://dev.azure.com', + fetchImpl: vi + .fn() + .mockRejectedValue(new TypeError('fetch failed')), + }); + expect(network.status).toBe('unknown'); + }); + it('validates the delegated account with its stored access token', async () => { mockAuthAccountsFindFirst.mockResolvedValue({ id: 'account-1', diff --git a/packages/ado/src/api.ts b/packages/ado/src/api.ts index f01119cd8..19d4a6755 100644 --- a/packages/ado/src/api.ts +++ b/packages/ado/src/api.ts @@ -257,6 +257,27 @@ async function resolveAdoAuthMode(): Promise { return (await resolveDeploymentEnvVar('ADO_TOKEN'))?.trim() ? 'pat' : 'entra'; } +/** + * Thrown while acquiring a Microsoft Entra token. `definitive` separates a + * rejected credential (bad client id/secret/tenant, revoked refresh token: + * the token endpoint answers 400/401) from an unverifiable failure (network, + * timeout, 5xx). The validate* helpers map definitive failures to `invalid`, + * which blocks saving, and everything else to `unknown`, which does not. + */ +class AdoTokenAcquisitionError extends Error { + readonly definitive: boolean; + + constructor(message: string, definitive: boolean) { + super(message); + this.name = 'AdoTokenAcquisitionError'; + this.definitive = definitive; + } +} + +function isDefinitiveTokenEndpointStatus(status: number): boolean { + return status === 400 || status === 401; +} + async function requestAdoEntraClientCredentialsToken({ clientId, clientSecret, @@ -288,8 +309,9 @@ async function requestAdoEntraClientCredentialsToken({ ); if (!response.ok) { - throw new Error( + throw new AdoTokenAcquisitionError( `Azure DevOps Microsoft Entra token request failed: ${response.status} ${response.statusText}`, + isDefinitiveTokenEndpointStatus(response.status), ); } @@ -361,6 +383,7 @@ async function resolveAdoDelegatedToken(overrides?: { clientSecret?: string; tenantId?: string; fetchImpl?: typeof fetch; + timeoutMs?: number; }): Promise { const linkedAccountId = overrides?.linkedAccountId ?? @@ -422,8 +445,9 @@ async function resolveAdoDelegatedToken(overrides?: { !clientSecret?.trim() || !tenantId?.trim() ) { - throw new Error( + throw new AdoTokenAcquisitionError( 'Azure DevOps delegated connection needs to be reconnected. Open Settings and connect with Microsoft again.', + true, ); } @@ -439,12 +463,18 @@ async function resolveAdoDelegatedToken(overrides?: { refresh_token: account.refreshToken, scope: ADO_ENTRA_RESOURCE_SCOPE, }), + ...(overrides?.timeoutMs === undefined + ? {} + : { signal: AbortSignal.timeout(overrides.timeoutMs) }), }, ); if (!response.ok) { - throw new Error( + // 400 means Entra rejected the grant itself (expired or revoked refresh + // token, bad client secret); anything else is unverifiable. + throw new AdoTokenAcquisitionError( `Azure DevOps delegated token refresh failed: ${response.status} ${response.statusText}`, + isDefinitiveTokenEndpointStatus(response.status), ); } @@ -1025,11 +1055,21 @@ export async function validateAdoEntraCredentials({ timeoutMs, })); } catch (error) { + // Only a definitive rejection by the token endpoint blocks saving. + // Network trouble, timeouts, and 5xx answers are unverifiable, matching + // how an unreachable Azure DevOps is treated below. + if (error instanceof AdoTokenAcquisitionError && error.definitive) { + return { + status: 'invalid', + error: `Microsoft Entra did not issue an Azure DevOps token. Confirm the client ID, client secret, and tenant ID are correct and the client secret has not expired. (${error.message})`, + }; + } + return { - status: 'invalid', - error: `Microsoft Entra did not issue an Azure DevOps token. Confirm the client ID, client secret, and tenant ID are correct and the client secret has not expired. (${ + status: 'unknown', + error: `Could not verify the Microsoft Entra credential: ${ error instanceof Error ? error.message : String(error) - })`, + }`, }; } @@ -1075,11 +1115,24 @@ export async function validateAdoDelegatedCredentials({ clientSecret, tenantId, fetchImpl, + timeoutMs, }); } catch (error) { + // Only a definitive rejection blocks saving: a missing refresh token + // (reconnect needed) or the token endpoint refusing the grant. A network + // failure or 5xx during refresh is unverifiable. + if (error instanceof AdoTokenAcquisitionError && error.definitive) { + return { + status: 'invalid', + error: error.message, + }; + } + return { - status: 'invalid', - error: error instanceof Error ? error.message : String(error), + status: 'unknown', + error: `Could not verify the delegated Azure DevOps connection: ${ + error instanceof Error ? error.message : String(error) + }`, }; }