diff --git a/docs/docs/api/index.md b/docs/docs/api/index.md index 9a74360c85..f6d8de618b 100644 --- a/docs/docs/api/index.md +++ b/docs/docs/api/index.md @@ -8,6 +8,24 @@ Our [API](https://api.exceptionless.io) utilizes Swagger and Swashbuckle to auto To view the full API documentation, visit and click on the `API Documentation` link to be taken to the API documentation. With Swagger, you will have examples for all endpoints that include required parameters and potential responses. +## MCP OAuth access + +OAuth discovery advertises the scopes the server supports. Each OAuth application has its own allowed scopes, and each authorization grant contains only the scopes and organizations approved by the user. Discovery does not grant every advertised permission to an application. + +For the MCP resource, `mcp:read` is required to connect. `projects:read`, `stacks:read`, and `events:read` permit the corresponding read tools. `stacks:write` permits changing stack status (open, fixed, ignored, or discarded), snoozing stacks, changing whether future occurrences are critical, and adding or removing reference links. Access remains limited to the organizations in the grant and the user's current membership. + +`offline_access` permits refresh token issuance so the client can obtain another access token without a new interactive authorization. It adds no read or write permission. Access tokens default to one hour and refresh tokens to 30 days; these lifetimes are configured with `OAuthServer:AccessTokenLifetimeMinutes` and `OAuthServer:RefreshTokenLifetimeDays`. + +Dynamic registration without a scope uses `mcp:read projects:read stacks:read events:read offline_access`. It excludes `stacks:write`. An explicit registration scope is retained, so a client registered with only the four read scopes also lacks `offline_access`. + +### Resolving a scope error after registration + +If authorization reports scopes that are not allowed for the application, restart the client's authorization flow requesting an allowed subset. Keep `mcp:read` for MCP access. Changing checkboxes on a failed consent page does not validate a new request. + +Client software should register the scopes it will request. Each grant remains limited to the scopes and organizations approved by the user. Account **Applications** lists and revokes the user's grants. + +Existing access and refresh tokens do not gain additional permissions automatically. Fresh consent is required for additional access, including refresh tokens when the original grant did not include `offline_access`. + --- [Next > Getting Started](/docs/api/api-getting-started) diff --git a/src/Exceptionless.Core/Services/OAuthService.cs b/src/Exceptionless.Core/Services/OAuthService.cs index 77e7cb1446..4bd3a00885 100644 --- a/src/Exceptionless.Core/Services/OAuthService.cs +++ b/src/Exceptionless.Core/Services/OAuthService.cs @@ -361,7 +361,14 @@ public async Task ValidateAuthorizationRequestAsync(OAuth var allowedScopes = GetAllowedScopes(client); if (requestedScopes.Any(s => !allowedScopes.Contains(s, StringComparer.Ordinal))) - return OAuthValidationResult.Invalid("invalid_scope", "One or more scopes are not allowed for this client."); + { + // Only name server-supported scopes; do not reflect arbitrary request values. + var disallowedScopes = SupportedScopes.Where(s => requestedScopes.Contains(s, StringComparer.Ordinal) && !allowedScopes.Contains(s, StringComparer.Ordinal)).ToArray(); + string description = disallowedScopes.Length > 0 + ? $"Scopes not allowed for this application: {String.Join(", ", disallowedScopes)}." + : "One or more scopes are not allowed for this application."; + return OAuthValidationResult.Invalid("invalid_scope", $"{description} Restart authorization with scopes allowed for this application."); + } if (resourceDefinition.RequiredScopes.Any(s => !requestedScopes.Contains(s, StringComparer.Ordinal))) return OAuthValidationResult.Invalid("invalid_scope", "One or more required resource scopes are missing."); diff --git a/src/Exceptionless.Web/ClientApp/e2e/tests/oauth-consent.e2e.ts b/src/Exceptionless.Web/ClientApp/e2e/tests/oauth-consent.e2e.ts new file mode 100644 index 0000000000..439441bd2b --- /dev/null +++ b/src/Exceptionless.Web/ClientApp/e2e/tests/oauth-consent.e2e.ts @@ -0,0 +1,159 @@ +import { expect, test } from '@playwright/test'; + +test('AuthorizeConsent_InvalidAndRestartedRequests_UsesOnlyCurrentValidation', async ({ page }) => { + // Arrange + const errorDescription = + 'Scopes not allowed for this application: stacks:write, offline_access. Restart authorization with scopes allowed for this application.'; + let consentRequests = 0; + let authorizationRequests = 0; + let completeStaleConsent: (() => void) | undefined; + let completeSameQueryConsent: (() => void) | undefined; + let sameQueryRequests = 0; + await page.addInitScript(() => window.localStorage.setItem('satellizer_token', 'test-consent-session')); + // This browser regression exercises the production page and FetchClient with HTTP responses. + // Service policy and issuance are covered separately by OAuthEndpointTests. + await page.route('**/api/v2/**', async (route) => { + const path = new URL(route.request().url()).pathname; + if (path.endsWith('/users/me')) { + await route.fulfill({ json: { email_address: 'member@example.test', full_name: 'Member' } }); + } else if (path.endsWith('/organizations')) { + await route.fulfill({ json: [{ id: '000000000000000000000001', name: 'Test Organization' }] }); + } else if (path.endsWith('/oauth/authorize/consent')) { + consentRequests++; + const body = route.request().postDataJSON() as { client_id: string; scope: string }; + if (body.client_id === 'same-query-client') { + sameQueryRequests++; + } + if (body.client_id === 'stale-client' || (body.client_id === 'same-query-client' && sameQueryRequests === 1)) { + await new Promise((resolve) => { + if (body.client_id === 'same-query-client') { + completeSameQueryConsent = resolve; + } else { + completeStaleConsent = resolve; + } + }); + await route.fulfill({ json: { error: 'invalid_scope', error_description: 'Stale request error' }, status: 400 }); + return; + } + if (body.scope.includes('stacks:write')) { + await route.fulfill({ json: { error: 'invalid_scope', error_description: errorDescription }, status: 400 }); + } else { + await route.fulfill({ json: { client_name: 'Test Client', required_scopes: ['mcp:read'], scopes: body.scope.split(' ') } }); + } + } else if (path.endsWith('/oauth/authorize')) { + authorizationRequests++; + await route.fulfill({ json: { error: 'invalid_scope', error_description: errorDescription }, status: 400 }); + } else { + await route.abort(); + } + }); + + const parameters = new URLSearchParams({ + client_id: 'test-client', + code_challenge: 'a'.repeat(43), + code_challenge_method: 'S256', + redirect_uri: 'http://localhost/callback', + resource: 'http://localhost/mcp', + response_type: 'code', + scope: 'mcp:read projects:read stacks:read stacks:write events:read offline_access' + }); + try { + // Act + await page.goto(`/oauth/authorize?${parameters}`); + // Assert + await expect(page.getByText(errorDescription, { exact: true })).toBeVisible(); + await expect(page.getByText('Required', { exact: true })).toHaveCount(1); + await expect(page.getByRole('button', { exact: true, name: 'Approve' })).toBeDisabled(); + await expect(page.getByRole('checkbox', { name: /Stacks Write/ })).toBeDisabled(); + await expect(page.getByRole('checkbox', { name: /Offline Access/ })).toBeDisabled(); + await expect(page.getByRole('checkbox', { name: 'Test Organization' })).toBeDisabled(); + await expect(page.getByRole('button', { exact: true, name: 'Approve' })).toBeDisabled(); + expect(consentRequests).toBe(1); + expect(authorizationRequests).toBe(0); + await page.screenshot({ fullPage: true, path: test.info().outputPath('scope-error.png') }); + + // Act: restart with an allowed request. + parameters.set('scope', 'mcp:read'); + await page.goto(`/oauth/authorize?${parameters}`); + // Assert + await expect(page.getByText('Test Client', { exact: true })).toBeVisible(); + await expect(page.getByRole('checkbox', { name: 'Test Organization' })).toBeEnabled(); + await expect(page.getByRole('button', { exact: true, name: 'Approve' })).toBeEnabled(); + expect(consentRequests).toBe(2); + // Act + await page.getByRole('button', { exact: true, name: 'Approve' }).click(); + // Assert + await expect(page.getByText(errorDescription, { exact: true })).toBeVisible(); + expect(authorizationRequests).toBe(1); + + async function navigateWithQuery() { + await page.evaluate((href) => { + document.getElementById('oauth-query-link')?.remove(); + const link = document.createElement('a'); + link.id = 'oauth-query-link'; + link.href = href; + link.textContent = 'Change authorization request'; + document.body.append(link); + }, `/oauth/authorize?${parameters}`); + await page.getByRole('link', { name: 'Change authorization request' }).click(); + } + + // Act: a new query must clear the preceding request's error. + parameters.set('scope', 'mcp:read projects:read'); + await navigateWithQuery(); + await expect(page.getByRole('button', { exact: true, name: 'Approve' })).toBeEnabled(); + // Assert + await expect(page.getByText(errorDescription, { exact: true })).toHaveCount(0); + await expect(page.getByRole('checkbox', { name: /Projects Read/ })).toBeEnabled(); + + // Act: hold an obsolete request while navigating to another client. + parameters.set('client_id', 'stale-client'); + await navigateWithQuery(); + await expect.poll(() => Boolean(completeStaleConsent)).toBe(true); + // Assert + await expect(page.getByRole('checkbox', { name: /Projects Read/ })).toBeDisabled(); + await expect(page.getByRole('checkbox', { name: 'Test Organization' })).toBeDisabled(); + await expect(page.getByRole('button', { exact: true, name: 'Approve' })).toBeDisabled(); + parameters.set('client_id', 'test-client'); + await navigateWithQuery(); + await expect(page.getByRole('button', { exact: true, name: 'Approve' })).toBeEnabled(); + const staleResponse = page.waitForResponse( + (response) => response.url().endsWith('/oauth/authorize/consent') && response.request().postDataJSON().client_id === 'stale-client' + ); + // Act + completeStaleConsent?.(); + await (await staleResponse).finished(); + // Assert + await expect(page.getByText('Stale request error', { exact: true })).toHaveCount(0); + await expect(page.getByRole('button', { exact: true, name: 'Approve' })).toBeEnabled(); + + // Arrange: A1 and A2 have identical queries; URL equality cannot reject A1. + parameters.set('client_id', 'same-query-client'); + await navigateWithQuery(); + await expect.poll(() => Boolean(completeSameQueryConsent)).toBe(true); + await expect(page.getByRole('checkbox', { name: /Projects Read/ })).toBeDisabled(); + + // Act: navigate A → B → A, letting A2 validate before A1 completes. + parameters.set('client_id', 'test-client'); + await navigateWithQuery(); + await expect(page.getByRole('button', { exact: true, name: 'Approve' })).toBeEnabled(); + parameters.set('client_id', 'same-query-client'); + await navigateWithQuery(); + await expect.poll(() => sameQueryRequests).toBe(2); + await expect(page.getByRole('button', { exact: true, name: 'Approve' })).toBeEnabled(); + const sameQueryResponse = page.waitForResponse( + (response) => response.url().endsWith('/oauth/authorize/consent') && response.request().postDataJSON().client_id === 'same-query-client' + ); + completeSameQueryConsent?.(); + await (await sameQueryResponse).finished(); + + // Assert + await expect(page.getByText('Stale request error', { exact: true })).toHaveCount(0); + await expect(page.getByRole('checkbox', { name: /Projects Read/ })).toBeEnabled(); + await expect(page.getByRole('checkbox', { name: 'Test Organization' })).toBeEnabled(); + await expect(page.getByRole('button', { exact: true, name: 'Approve' })).toBeEnabled(); + } finally { + completeSameQueryConsent?.(); + completeStaleConsent?.(); + } +}); diff --git a/src/Exceptionless.Web/ClientApp/e2e/tests/oauth-live-consent.e2e.ts b/src/Exceptionless.Web/ClientApp/e2e/tests/oauth-live-consent.e2e.ts new file mode 100644 index 0000000000..af6c70ac6d --- /dev/null +++ b/src/Exceptionless.Web/ClientApp/e2e/tests/oauth-live-consent.e2e.ts @@ -0,0 +1,231 @@ +import { createHash, randomBytes } from 'node:crypto'; + +import type { OAuthApplication } from '../../src/lib/features/admin/models'; + +import { expect, test } from '../fixtures/e2e-test'; +import { getE2EEnvironment } from '../fixtures/environment'; +import { runCleanupStep, throwIfCleanupFailed } from '../support/cleanup'; + +interface OAuthTokens { + access_token: string; + refresh_token?: string; + scope: string; + token_type: string; +} + +const environment = getE2EEnvironment(); +const hostname = new URL(environment.appUrl).hostname; +test.skip( + environment.isProduction || (!['127.0.0.1', '[::1]', 'localhost'].includes(hostname) && !hostname.endsWith('.localhost')), + 'This synthetic OAuth journey requires the local development test host.' +); +test.use({ e2eUseGeneratedUser: true }); + +test('AuthorizeConsent_ExpandedClientRegistration_RequiresFreshMemberGrant', async ({ browser, e2eApi, e2eScenario, page, request }) => { + const readScopes = ['mcp:read', 'projects:read', 'stacks:read', 'events:read']; + const allScopes = [...readScopes, 'stacks:write', 'offline_access']; + const applicationName = `OAuth live ${e2eScenario.run}`; + const administratorToken = await e2eApi.login(); + const administratorHeaders = { Authorization: `Bearer ${administratorToken}` }; + const memberHeaders = { Authorization: `Bearer ${e2eScenario.userToken}` }; + const administratorContext = await browser.newContext({ baseURL: environment.appUrl, ignoreHTTPSErrors: true }); + let clientId: string | undefined; + let applicationId: string | undefined; + const issuedTokens: string[] = []; + + async function findApplication() { + const response = await request.get(`${environment.apiUrl}/admin/oauth-applications`, { + headers: administratorHeaders, + params: { criteria: clientId!, limit: 10 } + }); + expect(response.status()).toBe(200); + const applications = (await response.json()) as OAuthApplication[]; + return applications.find((application) => application.client_id === clientId); + } + + try { + // The existing organization page is the synthetic client's callback. Observe real + // browser navigation there; no OAuth, administrator or callback responses are intercepted. + const redirectUri = `${environment.appUrl}/organization/${e2eScenario.organizationId}/manage`; + const metadataResponse = await request.get(`${environment.appUrl}/.well-known/oauth-protected-resource/mcp`); + expect(metadataResponse.status()).toBe(200); + const { resource } = (await metadataResponse.json()) as { resource: string }; + + await test.step('register a restricted client and show its actual denied-scope response', async () => { + // Act + const registrationResponse = await request.post(`${environment.apiUrl}/oauth/register`, { + data: { + client_name: applicationName, + grant_types: ['authorization_code', 'refresh_token'], + redirect_uris: [redirectUri], + response_types: ['code'], + scope: readScopes.join(' '), + token_endpoint_auth_method: 'none' + } + }); + // Assert + expect(registrationResponse.status()).toBe(201); + clientId = ((await registrationResponse.json()) as { client_id: string }).client_id; + expect(clientId).toMatch(/^dcr_/); + await expect.poll(async () => (applicationId = (await findApplication())?.id)).toBeTruthy(); + + const forbidden = await request.put(`${environment.apiUrl}/admin/oauth-applications/${applicationId}`, { + data: { client_id: clientId, is_disabled: false, name: applicationName, redirect_uris: [redirectUri], scopes: allScopes }, + headers: memberHeaders + }); + expect(forbidden.status()).toBe(403); + + await page.goto(authorizationUrl(allScopes)); + await expect(page.getByText(/^Scopes not allowed for this application: stacks:write, offline_access\./)).toBeVisible(); + await expect(page.getByRole('button', { exact: true, name: 'Approve' })).toBeDisabled(); + await expect(page.getByRole('checkbox', { name: /Stacks Write/ })).toBeDisabled(); + await expect(page.getByRole('checkbox', { name: /Offline Access/ })).toBeDisabled(); + await expect(page.getByRole('checkbox', { exact: true, name: e2eScenario.organizationName })).toBeDisabled(); + }); + + await administratorContext.addInitScript((token) => window.localStorage.setItem('satellizer_token', token), administratorToken); + const administratorPage = await administratorContext.newPage(); + await test.step('find the failed registration through Not authorized', async () => { + // Act + await administratorPage.goto(`/system/oauth-applications?criteria=${encodeURIComponent(applicationName)}`); + // Assert + await expect(administratorPage.getByRole('button', { name: 'Filter by authorization' })).toHaveText('Authorized'); + await expect(administratorPage.getByRole('link', { exact: true, name: applicationName })).toHaveCount(0); + await administratorPage.getByRole('button', { name: 'Filter by authorization' }).click(); + await administratorPage.getByRole('option', { exact: true, name: 'Not authorized' }).click(); + await expect(administratorPage.getByRole('link', { exact: true, name: applicationName })).toBeVisible(); + await administratorPage.getByRole('button', { name: `Show details for ${applicationName}` }).click(); + await expect(administratorPage.getByText(clientId!, { exact: true })).toBeVisible(); + await administratorPage.getByRole('link', { exact: true, name: 'Edit application' }).click(); + await expect(administratorPage.getByLabel('Client ID', { exact: true })).toHaveValue(clientId!); + }); + + const limitedGrant = await test.step('consent to an allowed subset without offline access', async () => { + // Act + const tokens = await approveAndExchange(readScopes); + // Assert + expect(tokens.refresh_token).toBeUndefined(); + expect(tokens.scope.split(' ')).toEqual(readScopes); + return tokens; + }); + + await test.step('save the reviewed scopes in the real administrator form without replacing the client', async () => { + await expect(administratorPage.getByLabel('Client ID', { exact: true })).toHaveValue(clientId!); + for (const label of ['Stacks Write', 'Offline Access']) { + const checkbox = administratorPage.getByText(label, { exact: true }).locator('../..').getByRole('checkbox'); + await expect(checkbox).not.toBeChecked(); + await checkbox.check(); + } + const savedResponse = administratorPage.waitForResponse( + (response) => response.request().method() === 'PUT' && new URL(response.url()).pathname === `/api/v2/admin/oauth-applications/${applicationId}` + ); + await administratorPage.getByRole('button', { exact: true, name: 'Save Changes' }).click(); + expect((await savedResponse).status()).toBe(200); + await expect(administratorPage).toHaveURL((url) => url.pathname === '/system/oauth-applications'); + const savedApplication = await findApplication(); + expect(savedApplication?.client_id).toBe(clientId); + expect(savedApplication?.scopes.toSorted()).toEqual(allScopes.toSorted()); + expect(savedApplication?.organizations.map((organization) => organization.id)).toEqual([e2eScenario.organizationId]); + const oldGrantRefresh = await request.post(`${environment.apiUrl}/oauth/token`, { + form: { client_id: clientId!, grant_type: 'refresh_token', refresh_token: limitedGrant.access_token } + }); + expect(oldGrantRefresh.status()).toBe(400); + expect((await oldGrantRefresh.json()).error).toBe('invalid_grant'); + }); + + await test.step('restart with the same client and exchange and rotate the fresh consent grant', async () => { + // Act + const granted = await approveAndExchange(allScopes); + // Assert + expect(granted.scope.split(' ').toSorted()).toEqual(allScopes.toSorted()); + expect(granted.refresh_token).toEqual(expect.any(String)); + const response = await request.post(`${environment.apiUrl}/oauth/token`, { + form: { client_id: clientId!, grant_type: 'refresh_token', refresh_token: granted.refresh_token! } + }); + expect(response.status()).toBe(200); + const refreshed = (await response.json()) as OAuthTokens; + issuedTokens.push(refreshed.access_token); + expect(refreshed.access_token).not.toBe(granted.access_token); + expect(refreshed.refresh_token).toEqual(expect.any(String)); + expect(refreshed.refresh_token).not.toBe(granted.refresh_token); + expect(refreshed.scope.split(' ').toSorted()).toEqual(allScopes.toSorted()); + const resourceResponse = await request.get(resource, { headers: { Authorization: `Bearer ${refreshed.access_token}` } }); + // The stateless MCP endpoint rejects GET only after authenticating the OAuth grant. + expect(resourceResponse.status()).toBe(405); + const application = await findApplication(); + expect(application?.organizations.map((organization) => organization.id)).toEqual([e2eScenario.organizationId]); + }); + + function authorizationUrl(scopes: string[], verifier = randomBytes(32).toString('base64url')) { + return `/oauth/authorize?${new URLSearchParams({ + client_id: clientId!, + code_challenge: createHash('sha256').update(verifier).digest('base64url'), + code_challenge_method: 'S256', + redirect_uri: redirectUri, + resource, + response_type: 'code', + scope: scopes.join(' '), + state: e2eScenario.run + })}`; + } + + async function approveAndExchange(scopes: string[]) { + const verifier = randomBytes(32).toString('base64url'); + await page.goto(authorizationUrl(scopes, verifier)); + await expect(page.getByRole('checkbox', { exact: true, name: e2eScenario.organizationName })).toBeChecked(); + await expect(page.getByRole('button', { exact: true, name: 'Approve' })).toBeEnabled(); + await expect(page.getByRole('checkbox', { name: /Projects Read/ })).toBeEnabled(); + await expect(page.getByRole('checkbox', { exact: true, name: e2eScenario.organizationName })).toBeEnabled(); + const authorizationResponse = page.waitForResponse( + (response) => response.request().method() === 'POST' && new URL(response.url()).pathname === '/api/v2/oauth/authorize' + ); + await page.getByRole('button', { exact: true, name: 'Approve' }).click(); + const response = await authorizationResponse; + expect(response.status()).toBe(200); + expect(response.request().postDataJSON()).toMatchObject({ + client_id: clientId, + organization_ids: [e2eScenario.organizationId], + scope: scopes.join(' ') + }); + await expect(page).toHaveURL((url) => url.pathname === new URL(redirectUri).pathname && url.searchParams.has('code')); + const callback = new URL(page.url()); + expect(callback.searchParams.get('state')).toBe(e2eScenario.run); + const code = callback.searchParams.get('code'); + expect(code).toBeTruthy(); + const exchange = await request.post(`${environment.apiUrl}/oauth/token`, { + form: { + client_id: clientId!, + code: code!, + code_verifier: verifier, + grant_type: 'authorization_code', + redirect_uri: redirectUri, + resource + } + }); + expect(exchange.status()).toBe(200); + const tokens = (await exchange.json()) as OAuthTokens; + expect(tokens.access_token).toEqual(expect.any(String)); + expect(tokens.token_type).toBe('Bearer'); + issuedTokens.push(tokens.access_token); + return tokens; + } + } finally { + const cleanupErrors: Error[] = []; + await runCleanupStep(cleanupErrors, 'close administrator browser', () => administratorContext.close()); + for (const token of issuedTokens) { + await runCleanupStep(cleanupErrors, 'revoke synthetic OAuth grant', async () => { + const response = await request.post(`${environment.apiUrl}/oauth/revoke`, { form: { client_id: clientId!, token } }); + expect(response.status()).toBe(200); + }); + } + if (clientId) { + await runCleanupStep(cleanupErrors, 'delete synthetic OAuth application', async () => { + applicationId ??= (await findApplication())?.id; + expect(applicationId).toBeTruthy(); + const response = await request.delete(`${environment.apiUrl}/admin/oauth-applications/${applicationId}`, { headers: administratorHeaders }); + expect([204, 404]).toContain(response.status()); + }); + } + throwIfCleanupFailed(cleanupErrors); + } +}); diff --git a/src/Exceptionless.Web/ClientApp/src/lib/features/shared/validation.test.ts b/src/Exceptionless.Web/ClientApp/src/lib/features/shared/validation.test.ts index 0f810c5241..3739462a83 100644 --- a/src/Exceptionless.Web/ClientApp/src/lib/features/shared/validation.test.ts +++ b/src/Exceptionless.Web/ClientApp/src/lib/features/shared/validation.test.ts @@ -3,7 +3,40 @@ import { describe, expect, it } from 'vitest'; import { getProblemMessage, problemDetailsToFormErrors } from './validation'; describe('getProblemMessage', () => { - it('returns the title from a plain problem details payload', () => { + it('GetProblemMessage_NonProblemError_ReturnsFallback', () => { + // Arrange + const error = new Error('boom'); + + // Act + const actual = getProblemMessage(error, 'Please try again.'); + + // Assert + expect(actual).toBe('Please try again.'); + }); + + it.each([ + [{ error_description: 'OAuth description' }, 'OAuth description'], + [{ error: 'invalid_scope' }, 'invalid_scope'], + [{ error: 'invalid_scope', error_description: 'OAuth description', title: 'Bad Request' }, 'OAuth description'], + [{ detail: 'Problem detail' }, 'Problem detail'], + [{ title: 'Problem title' }, 'Problem title'], + [{ detail: 7, error: [], error_description: {}, title: false }, 'Fallback'], + [{ detail: 'Problem detail', error_description: '' }, 'Problem detail'], + [null, 'Fallback'], + ['Malformed body', 'Fallback'] + ])('GetProblemMessage_OAuthPayload_ReturnsSafeMessage: %j', (problem, expected) => { + // Arrange + const fallback = 'Fallback'; + + // Act + const actual = getProblemMessage(problem, fallback); + + // Assert + expect(actual).toBe(expected); + }); + + it('GetProblemMessage_ProblemPayload_ReturnsTitle', () => { + // Arrange const problem = { instance: 'DELETE /api/v2/organizations/6a121886ad40fc0017a40d3c', status: 400, @@ -12,48 +45,52 @@ describe('getProblemMessage', () => { type: 'https://tools.ietf.org/html/rfc9110#section-15.5.1' }; - expect(getProblemMessage(problem, 'Please try again.')).toBe('An organization cannot be deleted if it has a subscription.'); + // Act + const actual = getProblemMessage(problem, 'Please try again.'); + + // Assert + expect(actual).toBe('An organization cannot be deleted if it has a subscription.'); }); - it('prefers validation errors when present', () => { + it('GetProblemMessage_ValidationErrors_PrefersValidationMessage', () => { + // Arrange const problem = { - errors: { - general: ['The uploaded file is too large.'] - }, + errors: { general: ['The uploaded file is too large.'] }, status: 422, title: 'Validation failed.' }; - expect(getProblemMessage(problem, 'Please try again.')).toBe('The uploaded file is too large.'); - }); + // Act + const actual = getProblemMessage(problem, 'Please try again.'); - it('falls back when the error is not problem details shaped', () => { - expect(getProblemMessage(new Error('boom'), 'Please try again.')).toBe('Please try again.'); + // Assert + expect(actual).toBe('The uploaded file is too large.'); }); }); describe('problemDetailsToFormErrors', () => { it.each([{ status: 400 }, { status: 422 }, { errors: {}, status: 422 }, { errors: { version: [] }, status: 422 }])( - 'shows the problem message when there are no field errors: %j', + 'ProblemDetailsToFormErrors_NoFieldErrors_ReturnsProblemMessage: %j', (details) => { - const problem = { - ...details, - title: 'An organization cannot be deleted if it has a subscription.' - }; - - expect(problemDetailsToFormErrors(problem as never)).toEqual({ - form: 'An organization cannot be deleted if it has a subscription.' - }); + // Arrange + const problem = { ...details, title: 'An organization cannot be deleted if it has a subscription.' }; + + // Act + const actual = problemDetailsToFormErrors(problem as never); + + // Assert + expect(actual).toEqual({ form: 'An organization cannot be deleted if it has a subscription.' }); } ); - it('preserves specific field errors without adding the generic title', () => { - expect( - problemDetailsToFormErrors({ - errors: { version: ['Version is invalid.'] }, - status: 422, - title: 'Validation failed.' - } as never) - ).toEqual({ fields: { version: 'Version is invalid.' } }); + it('ProblemDetailsToFormErrors_SpecificFieldErrors_OmitsGenericTitle', () => { + // Arrange + const problem = { errors: { version: ['Version is invalid.'] }, status: 422, title: 'Validation failed.' }; + + // Act + const actual = problemDetailsToFormErrors(problem as never); + + // Assert + expect(actual).toEqual({ fields: { version: 'Version is invalid.' } }); }); }); diff --git a/src/Exceptionless.Web/ClientApp/src/lib/features/shared/validation.ts b/src/Exceptionless.Web/ClientApp/src/lib/features/shared/validation.ts index 989415d1c5..f010c51394 100644 --- a/src/Exceptionless.Web/ClientApp/src/lib/features/shared/validation.ts +++ b/src/Exceptionless.Web/ClientApp/src/lib/features/shared/validation.ts @@ -117,6 +117,16 @@ export function getFormErrorMessages(errors?: unknown[]): string | string[] | un } export function getProblemMessage(error: unknown, fallback: string): string { + // OAuth uses its own error format; FetchClient also stores these bodies in response.problem. + if (error && typeof error === 'object') { + if ('error_description' in error && isNonEmptyString(error.error_description)) { + return error.error_description; + } + if ('error' in error && isNonEmptyString(error.error)) { + return error.error; + } + } + if (!isProblemDetailsLike(error)) { return fallback; } diff --git a/src/Exceptionless.Web/ClientApp/src/routes/(auth)/oauth/authorize/+page.svelte b/src/Exceptionless.Web/ClientApp/src/routes/(auth)/oauth/authorize/+page.svelte index 307732eb24..1a937a7d26 100644 --- a/src/Exceptionless.Web/ClientApp/src/routes/(auth)/oauth/authorize/+page.svelte +++ b/src/Exceptionless.Web/ClientApp/src/routes/(auth)/oauth/authorize/+page.svelte @@ -15,6 +15,7 @@ import { clearAuthenticationSession } from '$features/auth/session.svelte'; import { getOrganizationsQuery } from '$features/organizations/api.svelte'; import { getMeQuery } from '$features/users/api.svelte'; + import { getProblemMessage } from '$shared/validation'; import { useFetchClient } from '@foundatiofx/fetchclient'; import { SvelteSet } from 'svelte/reactivity'; @@ -57,6 +58,7 @@ let isLoadingConsent = $state(false); let loadedConsentKey = $state(null); let initializedOrganizationSelectionKey = $state(null); + let consentRequestId = 0; const selectedOrganizationIds = new SvelteSet(); const selectedScopes = new SvelteSet(); @@ -85,7 +87,10 @@ const hasSelectedOrganizations = $derived(selectedOrganizationIds.size > 0); const hasSelectedResourceScope = $derived(selectedScopeValues.some((scope) => scope !== offlineAccessScope)); const hasRequiredScopes = $derived(missingRequiredScopes.length === 0 && requiredScopes.every((scope) => selectedScopes.has(scope))); - const canApprove = $derived(!isLoadingConsent && !consentErrorMessage && hasSelectedOrganizations && hasSelectedResourceScope && hasRequiredScopes); + const canSelectConsent = $derived(Boolean(consentDetails) && !isLoadingConsent && !consentErrorMessage && !isAuthorizing); + const canApprove = $derived( + Boolean(consentDetails) && !isLoadingConsent && !consentErrorMessage && hasSelectedOrganizations && hasSelectedResourceScope && hasRequiredScopes + ); $effect(() => { if (!browser || accessToken.current) { @@ -194,12 +199,7 @@ return; } - errorMessage = - response.data?.error_description || - response.data?.error || - response.problem?.detail || - response.problem?.title || - 'Unable to authorize application.'; + errorMessage = getProblemMessage(response.data, getProblemMessage(response.problem, 'Unable to authorize application.')); } function cancelAuthorization() { @@ -252,7 +252,7 @@ function getRequiredScopes(resourceValue: string): string[] { if (resourceValue.endsWith('/mcp')) { - return [mcpReadScope, offlineAccessScope]; + return [mcpReadScope]; } return []; @@ -267,7 +267,12 @@ } async function loadConsentDetails(): Promise { + // A → B → A navigation can leave an older request with the same query; only the newest response owns this state. + const requestId = ++consentRequestId; + const consentKey = page.url.search; isLoadingConsent = true; + consentDetails = null; + errorMessage = null; consentErrorMessage = null; const client = useFetchClient(); const response = await client.postJSON( @@ -278,6 +283,10 @@ } ); + if (requestId !== consentRequestId || consentKey !== page.url.search) { + return; + } + isLoadingConsent = false; if (response.ok && response.data) { consentDetails = response.data; @@ -290,12 +299,7 @@ } consentDetails = null; - consentErrorMessage = - response.data?.error_description || - response.data?.error || - response.problem?.detail || - response.problem?.title || - 'Unable to load application details.'; + consentErrorMessage = getProblemMessage(response.data, getProblemMessage(response.problem, 'Unable to load application details.')); } async function redirectToLogin(): Promise { @@ -365,6 +369,7 @@