From 528dd3cc446cdef1aaefea174650341c525c2a0e Mon Sep 17 00:00:00 2001 From: Bobby Ho Date: Wed, 23 Sep 2026 08:39:57 -0700 Subject: [PATCH] feat: edit an OAuth2 app's redirect URIs as an ordered list (#29603) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Completes PLAT-619, following #29440 and #29502. - Replace the OAuth2 app’s single callback field with an editable `redirect_uris` list supporting 1–32 entries, with the first labeled “Default callback.” - Show a `+N` badge for additional redirect URIs in the applications table. - Remove `omitempty` from request `redirect_uris` so explicit empty lists reach the server. - Add tests and stories for adding, editing, removing, and requiring at least one URI. --- coderd/apidoc/docs.go | 1 + coderd/apidoc/swagger.json | 1 + coderd/oauth2provider/apps.go | 5 +- coderd/oauth2provider/provider_test.go | 42 +- codersdk/oauth2.go | 4 +- docs/admin/integrations/oauth2-provider.md | 2 +- docs/reference/api/schemas.md | 2 +- site/src/api/typesGenerated.ts | 4 + .../CreateOAuth2AppPageView.stories.tsx | 46 +- .../CreateOAuth2AppPageView.tsx | 6 +- .../EditOAuth2AppPageView.stories.tsx | 52 +- .../EditOAuth2AppPageView.tsx | 1 + .../OAuth2AppForm.stories.tsx | 1 + .../OAuth2AppForm.test.tsx | 468 ++++++++++++++++-- .../OAuth2AppsSettingsPage/OAuth2AppForm.tsx | 196 ++++++-- .../OAuth2AppsSettingsPageView.stories.tsx | 15 + .../OAuth2AppsSettingsPageView.tsx | 25 +- .../components/RedirectURIsField.test.tsx | 109 ++++ .../components/RedirectURIsField.tsx | 97 ++++ 19 files changed, 980 insertions(+), 97 deletions(-) create mode 100644 site/src/pages/DeploymentSettingsPage/OAuth2AppsSettingsPage/components/RedirectURIsField.test.tsx create mode 100644 site/src/pages/DeploymentSettingsPage/OAuth2AppsSettingsPage/components/RedirectURIsField.tsx diff --git a/coderd/apidoc/docs.go b/coderd/apidoc/docs.go index 8fe6c84632..516733510a 100644 --- a/coderd/apidoc/docs.go +++ b/coderd/apidoc/docs.go @@ -27479,6 +27479,7 @@ const docTemplate = `{ "type": "string" }, "icon": { + "description": "Icon replaces the app's stored icon. Omitting it clears the stored\nicon rather than leaving it unchanged.", "type": "string" }, "name": { diff --git a/coderd/apidoc/swagger.json b/coderd/apidoc/swagger.json index ffe2bc15a7..b3c7c43a3b 100644 --- a/coderd/apidoc/swagger.json +++ b/coderd/apidoc/swagger.json @@ -25216,6 +25216,7 @@ "type": "string" }, "icon": { + "description": "Icon replaces the app's stored icon. Omitting it clears the stored\nicon rather than leaving it unchanged.", "type": "string" }, "name": { diff --git a/coderd/oauth2provider/apps.go b/coderd/oauth2provider/apps.go index 0616009740..553439f7ec 100644 --- a/coderd/oauth2provider/apps.go +++ b/coderd/oauth2provider/apps.go @@ -80,14 +80,15 @@ func emptyRedirectURIsDetail(fromCallback string) string { // the scheme, cannot be saved until a request sends a list that passes. func validateAppRedirectURIFields(uris []string, clientType codersdk.OAuth2ClientType, fromCallback string) []codersdk.ValidationError { // A failure on the request's callback_url is reported against that field - // and without a list index, since the caller never sent a list. + // and without a row number, since the caller never sent a list. Rows are + // numbered from one to match the labels the web UI shows. invalid := func(i int, uri, detail string) []codersdk.ValidationError { if uri != "" && uri == fromCallback { return []codersdk.ValidationError{{Field: "callback_url", Detail: "callback URL " + detail}} } return []codersdk.ValidationError{{ Field: "redirect_uris", - Detail: fmt.Sprintf("redirect URI at index %d %s", i, detail), + Detail: fmt.Sprintf("redirect URI %d %s", i+1, detail), }} } if len(uris) == 0 { diff --git a/coderd/oauth2provider/provider_test.go b/coderd/oauth2provider/provider_test.go index 6fb0e457fc..3d7246a2c4 100644 --- a/coderd/oauth2provider/provider_test.go +++ b/coderd/oauth2provider/provider_test.go @@ -1038,7 +1038,7 @@ func TestOAuth2ProviderAppRedirectURIs(t *testing.T) { Name: "renamed", CallbackURL: first, }) - requireRedirectURIsValidationError(t, err, "redirect URI at index 1 must be at most 2048 bytes") + requireRedirectURIsValidationError(t, err, "redirect URI 2 must be at most 2048 bytes") stored, err := db.GetOAuth2ProviderAppByID(ctx, app.ID) require.NoError(t, err) @@ -1356,9 +1356,9 @@ func TestOAuth2ProviderAppRedirectURIs(t *testing.T) { require.Equal(t, http.StatusBadRequest, sdkErr.StatusCode()) require.Len(t, sdkErr.Validations, 1) require.Equal(t, "redirect_uris", sdkErr.Validations[0].Field) - require.Equal(t, "redirect URI at index 0 must use https scheme for non-localhost URLs", sdkErr.Validations[0].Detail) + require.Equal(t, "redirect URI 1 must use https scheme for non-localhost URLs", sdkErr.Validations[0].Detail) - // The index names the offending entry rather than the first one. + // The row number names the offending entry rather than the first one. //nolint:gocritic // OAuth2 app management requires owner permission. _, err = client.PostOAuth2ProviderApp(ctx, codersdk.PostOAuth2ProviderAppRequest{ Name: "cleartext-create-second", @@ -1367,7 +1367,7 @@ func TestOAuth2ProviderAppRedirectURIs(t *testing.T) { sdkErr = nil require.ErrorAs(t, err, &sdkErr) require.Len(t, sdkErr.Validations, 1) - require.Equal(t, "redirect URI at index 1 must use https scheme for non-localhost URLs", sdkErr.Validations[0].Detail) + require.Equal(t, "redirect URI 2 must use https scheme for non-localhost URLs", sdkErr.Validations[0].Detail) // The deprecated field is reported against its own name. //nolint:gocritic // OAuth2 app management requires owner permission. @@ -1406,7 +1406,7 @@ func TestOAuth2ProviderAppRedirectURIs(t *testing.T) { require.ErrorAs(t, err, &sdkErr) require.Len(t, sdkErr.Validations, 1) require.Equal(t, "redirect_uris", sdkErr.Validations[0].Field) - require.Equal(t, "redirect URI at index 0 must use https scheme for non-localhost URLs", sdkErr.Validations[0].Detail) + require.Equal(t, "redirect URI 1 must use https scheme for non-localhost URLs", sdkErr.Validations[0].Detail) }) t.Run("LegacyCleartextHTTPCallback", func(t *testing.T) { @@ -1434,7 +1434,7 @@ func TestOAuth2ProviderAppRedirectURIs(t *testing.T) { _, err := client.PutOAuth2ProviderApp(ctx, app.ID, codersdk.PutOAuth2ProviderAppRequest{ Name: "renamed", }) - requireRedirectURIsValidationError(t, err, "redirect URI at index 0 must use https scheme for non-localhost URLs") + requireRedirectURIsValidationError(t, err, "redirect URI 1 must use https scheme for non-localhost URLs") stored, err := db.GetOAuth2ProviderAppByID(ctx, app.ID) require.NoError(t, err) @@ -1502,6 +1502,32 @@ func TestOAuth2ProviderAppRedirectURIs(t *testing.T) { require.Equal(t, "redirect_uris was sent as an empty list, which overrides callback_url; send at least one redirect URI, or omit redirect_uris to use callback_url", sdkErr.Validations[0].Detail) }) + // The web UI only rejects a fragment for public clients, so a confidential + // app's fragment reaches the server. The row number in the message counts + // from one, matching the form labels, rather than the zero-based index. + t.Run("FragmentNamesVisibleRow", func(t *testing.T) { + t.Parallel() + + client := coderdtest.New(t, nil) + _ = coderdtest.CreateFirstUser(t, client) + ctx := testutil.Context(t, testutil.WaitLong) + + //nolint:gocritic // OAuth2 app management requires owner permission. + app, err := client.PostOAuth2ProviderApp(ctx, codersdk.PostOAuth2ProviderAppRequest{ + Name: "fragment-row", + RedirectURIs: []string{first}, + }) + require.NoError(t, err) + require.Equal(t, codersdk.OAuth2ClientTypeConfidential, app.ClientType) + + //nolint:gocritic // OAuth2 app management requires owner permission. + _, err = client.PutOAuth2ProviderApp(ctx, app.ID, codersdk.PutOAuth2ProviderAppRequest{ + Name: app.Name, + RedirectURIs: []string{first, "https://example.com/callback#state"}, + }) + requireRedirectURIsValidationError(t, err, "redirect URI 2 must not contain a fragment component") + }) + // A create that sends neither URI field is refused. t.Run("OmittedFieldsOnCreateRefused", func(t *testing.T) { t.Parallel() @@ -1522,7 +1548,7 @@ func TestOAuth2ProviderAppRedirectURIs(t *testing.T) { }) // A malformed entry sent through redirect_uris is attributed to that - // field with its index; the same value sent through callback_url is + // field with its row number; the same value sent through callback_url is // attributed to callback_url instead. t.Run("AttributionByOriginField", func(t *testing.T) { t.Parallel() @@ -1540,7 +1566,7 @@ func TestOAuth2ProviderAppRedirectURIs(t *testing.T) { require.ErrorAs(t, err, &sdkErr) require.Len(t, sdkErr.Validations, 1) require.Equal(t, "redirect_uris", sdkErr.Validations[0].Field) - require.Equal(t, "redirect URI at index 1 uses the dangerous scheme javascript", sdkErr.Validations[0].Detail) + require.Equal(t, "redirect URI 2 uses the dangerous scheme javascript", sdkErr.Validations[0].Detail) //nolint:gocritic // OAuth2 app management requires owner permission. _, err = client.PostOAuth2ProviderApp(ctx, codersdk.PostOAuth2ProviderAppRequest{ diff --git a/codersdk/oauth2.go b/codersdk/oauth2.go index 18a35e6c58..c5ab6e9df0 100644 --- a/codersdk/oauth2.go +++ b/codersdk/oauth2.go @@ -126,7 +126,9 @@ type PutOAuth2ProviderAppRequest struct { // Deprecated: send redirect_uris instead. If both are sent, callback_url // must equal the first entry of redirect_uris. CallbackURL string `json:"callback_url,omitempty" validate:"omitempty"` - Icon string `json:"icon" validate:"omitempty"` + // Icon replaces the app's stored icon. Omitting it clears the stored + // icon rather than leaving it unchanged. + Icon string `json:"icon" validate:"omitempty"` // Scope replaces the app's current allowlist. Omit to leave the existing // allowlist untouched. Set to an empty string to clear it, making the app // unrestricted. diff --git a/docs/admin/integrations/oauth2-provider.md b/docs/admin/integrations/oauth2-provider.md index 9f8f256974..d9d8a662a6 100644 --- a/docs/admin/integrations/oauth2-provider.md +++ b/docs/admin/integrations/oauth2-provider.md @@ -804,7 +804,7 @@ The current implementation has these limitations: - No device authorization grant support (RFC 8628) - Implicit grant (`response_type=token`) is not supported; OAuth 2.1 deprecated this flow due to token leakage risks, and a request for it redirects to the registered callback with `unsupported_response_type` - Limited to opaque access tokens (no JWT support) -- An application may register at most 32 redirect URIs of at most 2048 bytes each. An application that stored a longer list before this limit existed keeps working, but it cannot be saved again until the list fits. To fix it, send a `PUT` with a `redirect_uris` list that fits, as shown under [Management API](#method-2-management-api). The web UI cannot edit the list: editing **Callback URL** replaces the primary redirect URI and preserves the other registered URIs, and the application page displays only the primary. +- An application may register at most 32 redirect URIs of at most 2048 bytes each. An application that stored a longer list before this limit existed keeps working, but it cannot be saved again until the list fits. To fix it, send a `PUT` with a `redirect_uris` list that fits, as shown under [Management API](#method-2-management-api). In the web UI, open the application and remove entries from **Redirect URIs** until the list fits. - A cleartext `http://` redirect URI to a host that is not local is rejected. Earlier versions accepted one through the management API for a confidential application, although Dynamic Client Registration always refused it. An application that stored one keeps working, but it cannot be saved again until its list uses `https://` or a local host, as described under [Callback URL schemes](#callback-url-schemes). - A redirect URI with a private-use scheme must name a path or an authority, as in `com.example.app:/callback` or `com.example.app://auth/callback`. The bare form `com.example.app:callback` is rejected. Dynamic Client Registration accepted it in earlier versions. A client that registered one can re-register with one of the other two forms, or an administrator can correct it with the same `PUT`. diff --git a/docs/reference/api/schemas.md b/docs/reference/api/schemas.md index 23e36fe833..a69c4f30a2 100644 --- a/docs/reference/api/schemas.md +++ b/docs/reference/api/schemas.md @@ -13114,7 +13114,7 @@ Git clone makes use of this by parsing the URL from: 'Username for "https://gith | Name | Type | Required | Restrictions | Description | |-----------------|-----------------|----------|--------------|-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------| | `callback_url` | string | false | | Deprecated: send redirect_uris instead. If both are sent, callback_url must equal the first entry of redirect_uris. | -| `icon` | string | false | | | +| `icon` | string | false | | Icon replaces the app's stored icon. Omitting it clears the stored icon rather than leaving it unchanged. | | `name` | string | true | | | | `redirect_uris` | array of string | false | | Redirect uris is the ordered list of URIs the app may redirect to. The first entry is the primary. Omit both this and callback_url to keep the stored redirect URIs. Other fields are replaced. Sending an empty list is an error, not a way to keep the stored list. | | `scope` | string | false | | Scope replaces the app's current allowlist. Omit to leave the existing allowlist untouched. Set to an empty string to clear it, making the app unrestricted. | diff --git a/site/src/api/typesGenerated.ts b/site/src/api/typesGenerated.ts index 2e4b49ff47..e9635f8179 100644 --- a/site/src/api/typesGenerated.ts +++ b/site/src/api/typesGenerated.ts @@ -8066,6 +8066,10 @@ export interface PutOAuth2ProviderAppRequest { * must equal the first entry of redirect_uris. */ readonly callback_url?: string; + /** + * Icon replaces the app's stored icon. Omitting it clears the stored + * icon rather than leaving it unchanged. + */ readonly icon: string; /** * Scope replaces the app's current allowlist. Omit to leave the existing diff --git a/site/src/pages/DeploymentSettingsPage/OAuth2AppsSettingsPage/CreateOAuth2AppPageView.stories.tsx b/site/src/pages/DeploymentSettingsPage/OAuth2AppsSettingsPage/CreateOAuth2AppPageView.stories.tsx index a66341b7cd..b36e8fbdea 100644 --- a/site/src/pages/DeploymentSettingsPage/OAuth2AppsSettingsPage/CreateOAuth2AppPageView.stories.tsx +++ b/site/src/pages/DeploymentSettingsPage/OAuth2AppsSettingsPage/CreateOAuth2AppPageView.stories.tsx @@ -61,7 +61,7 @@ export const WithValidationError: Story = { message: "Validation failed", validations: [ { field: "name", detail: "name error" }, - { field: "callback_url", detail: "url error" }, + { field: "redirect_uris", detail: "url error" }, { field: "icon", detail: "icon error" }, ], }), @@ -71,7 +71,7 @@ export const WithValidationError: Story = { const canvas = within(canvasElement); await userEvent.type(await canvas.findByLabelText(/^name/i), "test-app"); await userEvent.type( - canvas.getByLabelText(/callback url/i), + canvas.getByLabelText(/default callback/i), "https://example.com/callback", ); await userEvent.click( @@ -87,7 +87,7 @@ export const InvalidCallbackURL: Story = { play: async ({ canvasElement }) => { const canvas = within(canvasElement); await userEvent.type(await canvas.findByLabelText(/^name/i), "test-app"); - const callbackInput = canvas.getByLabelText(/callback url/i); + const callbackInput = canvas.getByLabelText(/default callback/i); // oxlint-disable-next-line eslint/no-script-url -- Deliberately invalid input exercises callback URL rejection. await userEvent.type(callbackInput, "javascript:alert(1)"); await userEvent.tab(); @@ -102,13 +102,51 @@ export const DynamicallyRegisteredValues: Story = { "VS Code Coder Extension", ); await userEvent.type( - canvas.getByLabelText(/callback url/i), + canvas.getByLabelText(/default callback/i), "vscode://coder.coder-remote/oauth/callback", ); await userEvent.tab(); }, }; +export const MultipleRedirectURIs: Story = { + play: async ({ canvasElement }) => { + const canvas = within(canvasElement); + await userEvent.type( + await canvas.findByLabelText(/^name/i), + "VS Code Coder Extension", + ); + await userEvent.type( + canvas.getByLabelText(/default callback/i), + "vscode://coder.coder-remote/oauth/callback", + ); + await userEvent.click( + canvas.getByRole("button", { name: /add redirect uri/i }), + ); + await userEvent.type( + canvas.getByLabelText(/^redirect uri 2/i), + "https://example.com/callback", + ); + }, +}; + +export const InvalidRowState: Story = { + play: async ({ canvasElement }) => { + const canvas = within(canvasElement); + await userEvent.type(await canvas.findByLabelText(/^name/i), "test-app"); + await userEvent.click( + canvas.getByRole("button", { name: /add redirect uri/i }), + ); + // oxlint-disable-next-line eslint/no-script-url -- Deliberately invalid input exercises redirect URI rejection. + const invalidRedirectURI = "javascript:alert(1)"; + await userEvent.type( + canvas.getByLabelText(/^redirect uri 2/i), + invalidRedirectURI, + ); + await userEvent.tab(); + }, +}; + export const NoPermissions: Story = { parameters: { permissions: { diff --git a/site/src/pages/DeploymentSettingsPage/OAuth2AppsSettingsPage/CreateOAuth2AppPageView.tsx b/site/src/pages/DeploymentSettingsPage/OAuth2AppsSettingsPage/CreateOAuth2AppPageView.tsx index 411267ae6d..2a7bccd0c3 100644 --- a/site/src/pages/DeploymentSettingsPage/OAuth2AppsSettingsPage/CreateOAuth2AppPageView.tsx +++ b/site/src/pages/DeploymentSettingsPage/OAuth2AppsSettingsPage/CreateOAuth2AppPageView.tsx @@ -21,9 +21,10 @@ export const CreateOAuth2AppPageView: FC = () => { const queryClient = useQueryClient(); const postAppMutation = useMutation(postApp(queryClient)); + const callbackURL = searchParams.get("callback_url"); const defaultValues = { name: searchParams.get("name") ?? "", - callback_url: searchParams.get("callback_url") ?? "", + redirect_uris: callbackURL ? [callbackURL] : [], icon: searchParams.get("icon") ?? "", }; const [icon, setIcon] = useState(defaultValues.icon); @@ -50,6 +51,9 @@ export const CreateOAuth2AppPageView: FC = () => {
{ try { const app = await postAppMutation.mutateAsync(req); diff --git a/site/src/pages/DeploymentSettingsPage/OAuth2AppsSettingsPage/EditOAuth2AppPageView.stories.tsx b/site/src/pages/DeploymentSettingsPage/OAuth2AppsSettingsPage/EditOAuth2AppPageView.stories.tsx index 6d023691bf..e020d0bfd1 100644 --- a/site/src/pages/DeploymentSettingsPage/OAuth2AppsSettingsPage/EditOAuth2AppPageView.stories.tsx +++ b/site/src/pages/DeploymentSettingsPage/OAuth2AppsSettingsPage/EditOAuth2AppPageView.stories.tsx @@ -120,7 +120,7 @@ export const WithValidationError: Story = { message: "Validation failed", validations: [ { field: "name", detail: "name error" }, - { field: "callback_url", detail: "url error" }, + { field: "redirect_uris", detail: "url error" }, { field: "icon", detail: "icon error" }, ], }), @@ -216,6 +216,56 @@ export const PublicClient: Story = { }, }; +export const MultipleRedirectURIs: Story = { + parameters: { + queries: [ + { key: externalScopesKey, data: MockExternalAPIKeyScopes }, + { + key: oauth2ProviderAppKey(appId), + data: { + ...mockApp, + redirect_uris: [mockApp.callback_url, "https://example.com/callback"], + }, + }, + { + key: oauth2ProviderAppSecretsKey(appId), + data: MockOAuth2ProviderAppSecrets, + }, + ], + }, + play: async ({ canvasElement }) => { + const canvas = within(canvasElement); + // Waits for the query to resolve so Pixel captures the loaded state. + await canvas.findByLabelText(/^redirect uri 2/i); + }, +}; + +export const InvalidRowState: Story = { + parameters: { + queries: [ + { key: externalScopesKey, data: MockExternalAPIKeyScopes }, + { key: oauth2ProviderAppKey(appId), data: mockApp }, + { + key: oauth2ProviderAppSecretsKey(appId), + data: MockOAuth2ProviderAppSecrets, + }, + ], + }, + play: async ({ canvasElement }) => { + const canvas = within(canvasElement); + await userEvent.click( + await canvas.findByRole("button", { name: /add redirect uri/i }), + ); + // oxlint-disable-next-line eslint/no-script-url -- Deliberately invalid input exercises redirect URI rejection. + const invalidRedirectURI = "javascript:alert(1)"; + await userEvent.type( + canvas.getByLabelText(/^redirect uri 2/i), + invalidRedirectURI, + ); + await userEvent.tab(); + }, +}; + export const NoSecretPermissions: Story = { parameters: { permissions: { diff --git a/site/src/pages/DeploymentSettingsPage/OAuth2AppsSettingsPage/EditOAuth2AppPageView.tsx b/site/src/pages/DeploymentSettingsPage/OAuth2AppsSettingsPage/EditOAuth2AppPageView.tsx index 3dbd5ea78a..0a34f41162 100644 --- a/site/src/pages/DeploymentSettingsPage/OAuth2AppsSettingsPage/EditOAuth2AppPageView.tsx +++ b/site/src/pages/DeploymentSettingsPage/OAuth2AppsSettingsPage/EditOAuth2AppPageView.tsx @@ -202,6 +202,7 @@ export const EditOAuth2AppPageView: FC = () => { { try { const updated = await putAppMutation.mutateAsync({ diff --git a/site/src/pages/DeploymentSettingsPage/OAuth2AppsSettingsPage/OAuth2AppForm.stories.tsx b/site/src/pages/DeploymentSettingsPage/OAuth2AppsSettingsPage/OAuth2AppForm.stories.tsx index 573d3e6177..6157ba58e0 100644 --- a/site/src/pages/DeploymentSettingsPage/OAuth2AppsSettingsPage/OAuth2AppForm.stories.tsx +++ b/site/src/pages/DeploymentSettingsPage/OAuth2AppsSettingsPage/OAuth2AppForm.stories.tsx @@ -14,6 +14,7 @@ const meta = { title: "pages/DeploymentSettingsPage/OAuth2AppForm", component: OAuth2AppForm, args: { + clientType: "confidential", onSubmit: fn(), isUpdating: false, disabled: false, diff --git a/site/src/pages/DeploymentSettingsPage/OAuth2AppsSettingsPage/OAuth2AppForm.test.tsx b/site/src/pages/DeploymentSettingsPage/OAuth2AppsSettingsPage/OAuth2AppForm.test.tsx index 279da58f39..b4b21e589d 100644 --- a/site/src/pages/DeploymentSettingsPage/OAuth2AppsSettingsPage/OAuth2AppForm.test.tsx +++ b/site/src/pages/DeploymentSettingsPage/OAuth2AppsSettingsPage/OAuth2AppForm.test.tsx @@ -2,6 +2,8 @@ import { act, screen, waitFor } from "@testing-library/react"; import userEvent, { type UserEvent } from "@testing-library/user-event"; import { API } from "#/api/api"; import { + OAuth2RedirectURIMaxBytes, + OAuth2RedirectURIsMaxCount, OAuth2ScopeListMaxBytes, OAuth2ScopeListMaxNames, } from "#/api/typesGenerated"; @@ -37,12 +39,17 @@ describe("OAuth2AppForm", () => { const onSubmit = vi.fn(); render( - , + , ); await user.type(screen.getByLabelText(/^name/i), name); await user.type( - screen.getByLabelText(/callback url/i), + screen.getByLabelText(/default callback/i), "vscode://coder.coder-remote/oauth/callback", ); await user.click( @@ -52,7 +59,7 @@ describe("OAuth2AppForm", () => { await waitFor(() => { expect(onSubmit).toHaveBeenCalledWith({ name: "VS Code Coder Extension", - callback_url: "vscode://coder.coder-remote/oauth/callback", + redirect_uris: ["vscode://coder.coder-remote/oauth/callback"], icon: "", scope: "", }); @@ -72,6 +79,7 @@ describe("OAuth2AppForm", () => { render( { await waitFor(() => { expect(onSubmit).toHaveBeenCalledWith({ name: "Cursor MCP Extension", - callback_url: "vscode://coder.coder-remote/oauth/callback", icon: app.icon, }); }); @@ -117,11 +124,16 @@ describe("OAuth2AppForm", () => { const onSubmit = vi.fn(); render( - , + , ); await user.type(screen.getByLabelText(/^name/i), "test-app"); - const callbackURL = screen.getByLabelText(/callback url/i); + const callbackURL = screen.getByLabelText(/default callback/i); await user.type(callbackURL, callback); await user.click( screen.getByRole("button", { name: /create application/i }), @@ -135,6 +147,11 @@ describe("OAuth2AppForm", () => { " vscode://coder.coder-remote/oauth/callback", "vscode://coder.coder-remote/oauth/callback ", " vscode://coder.coder-remote/oauth/callback ", + " https://example.com/callback", + "https://example.com/callback ", + " https://example.com/callback ", + " http://localhost:3000/callback", + "http://127.0.0.1:3000/callback ", "URN:ietf:wg:oauth:2.0:oob", "com.example.app:/oauth2redirect", "cursor://anysphere.cursor-mcp/oauth/callback", @@ -142,17 +159,22 @@ describe("OAuth2AppForm", () => { const user = userEvent.setup(); const onSubmit = vi.fn(); render( - , + , ); await user.type(screen.getByLabelText(/^name/i), "OAuth App"); - await user.type(screen.getByLabelText(/callback url/i), callback); + await user.type(screen.getByLabelText(/default callback/i), callback); await user.click( screen.getByRole("button", { name: /create application/i }), ); await waitFor(() => expect(onSubmit).toHaveBeenCalledWith({ name: "OAuth App", - callback_url: callback.trim(), + redirect_uris: [callback.trim()], icon: "", scope: "", }), @@ -169,11 +191,16 @@ describe("OAuth2AppForm", () => { const user = userEvent.setup(); const onSubmit = vi.fn(); render( - , + , ); await user.type(screen.getByLabelText(/^name/i), name); await user.type( - screen.getByLabelText(/callback url/i), + screen.getByLabelText(/default callback/i), "https://example.com/callback", ); await user.click( @@ -184,7 +211,7 @@ describe("OAuth2AppForm", () => { await waitFor(() => expect(onSubmit).toHaveBeenCalledWith({ name, - callback_url: "https://example.com/callback", + redirect_uris: ["https://example.com/callback"], icon: "", scope: "", }), @@ -194,6 +221,56 @@ describe("OAuth2AppForm", () => { } }); + // Each "é" is two UTF-8 bytes but one UTF-16 code unit, so these URIs stay + // well under the limit by string length and only cross it by byte count. + const redirectURIPrefix = "https://example.com/"; + const redirectURIOfBytes = (bytes: number) => + redirectURIPrefix + + "é".repeat(Math.floor((bytes - redirectURIPrefix.length) / 2)) + + "a".repeat((bytes - redirectURIPrefix.length) % 2); + + it.each([ + { bytes: OAuth2RedirectURIMaxBytes, valid: true }, + { bytes: OAuth2RedirectURIMaxBytes + 1, valid: false }, + ])( + "enforces the UTF-8 byte limit for a $bytes byte redirect URI", + async ({ bytes, valid }) => { + const uri = redirectURIOfBytes(bytes); + expect(new TextEncoder().encode(uri)).toHaveLength(bytes); + expect(uri.length).toBeLessThan(OAuth2RedirectURIMaxBytes); + + const user = userEvent.setup(); + const onSubmit = vi.fn(); + render( + , + ); + await user.type(screen.getByLabelText(/^name/i), "OAuth App"); + await user.click(screen.getByLabelText(/default callback/i)); + await user.paste(uri); + await user.click( + screen.getByRole("button", { name: /create application/i }), + ); + await act(async () => {}); + if (valid) { + await waitFor(() => + expect(onSubmit).toHaveBeenCalledWith({ + name: "OAuth App", + redirect_uris: [uri], + icon: "", + scope: "", + }), + ); + } else { + expect(onSubmit).not.toHaveBeenCalled(); + } + }, + ); + it.each([ { callback: "mailto:a@b", valid: false }, { callback: "mailto://a@b", valid: false }, @@ -218,14 +295,15 @@ describe("OAuth2AppForm", () => { render( , ); - await user.clear(screen.getByLabelText(/callback url/i)); + await user.clear(screen.getByLabelText(/default callback/i)); await user.type( - screen.getByLabelText(/callback url/i), + screen.getByLabelText(/default callback/i), callback.replaceAll("[", "[["), ); await user.type(screen.getByLabelText(/^name/i), " updated"); @@ -237,7 +315,7 @@ describe("OAuth2AppForm", () => { await waitFor(() => expect(onSubmit).toHaveBeenCalledWith({ name: `${MockOAuth2ProviderAppPublic.name} updated`, - callback_url: callback, + redirect_uris: [callback], icon: MockOAuth2ProviderAppPublic.icon, }), ); @@ -263,6 +341,7 @@ describe("OAuth2AppForm", () => { const onSubmit = vi.fn(); render( { ); await user.type(screen.getByLabelText(/^name/i), "confidential-app"); await user.type( - screen.getByLabelText(/callback url/i), + screen.getByLabelText(/default callback/i), callback.replaceAll("[", "[["), ); await user.click( @@ -281,7 +360,7 @@ describe("OAuth2AppForm", () => { await waitFor(() => expect(onSubmit).toHaveBeenCalledWith({ name: "confidential-app", - callback_url: callback, + redirect_uris: [callback], icon: "", scope: "", }), @@ -292,18 +371,301 @@ describe("OAuth2AppForm", () => { }, ); - // Confidential apps could store a non-local http callback before the form - // checked for it. The stored value fails validation on load, so the error - // must show without the field being touched. - it("shows the error for a stored callback that fails validation", async () => { + // The stored list is left out of an unrelated save. If another admin removed + // a URI after this form loaded, resending the loaded list would restore it. + it("omits redirect_uris and callback_url on a rename", async () => { + const user = userEvent.setup(); + const onSubmit = vi.fn(); + const app = { + ...MockOAuth2ProviderApps[0], + redirect_uris: ["https://a.example.com/cb", "https://b.example.com/cb"], + }; + + render( + , + ); + + await user.clear(screen.getByLabelText(/^name/i)); + await user.type(screen.getByLabelText(/^name/i), "Renamed app"); + await user.click( + screen.getByRole("button", { name: /update application/i }), + ); + + await waitFor(() => { + const call = onSubmit.mock.calls[0][0]; + expect(call).toStrictEqual({ + name: "Renamed app", + icon: app.icon, + }); + expect(call).not.toHaveProperty("redirect_uris"); + expect(call).not.toHaveProperty("callback_url"); + }); + }); + + it("edits an entry in place, keeping its position", async () => { + const user = userEvent.setup(); + const onSubmit = vi.fn(); + const app = { + ...MockOAuth2ProviderApps[0], + redirect_uris: ["https://a.example.com/cb", "https://b.example.com/cb"], + }; + + render( + , + ); + + await user.clear(screen.getByLabelText(/default callback/i)); + await user.type( + screen.getByLabelText(/default callback/i), + "https://c.example.com/cb", + ); + await user.click( + screen.getByRole("button", { name: /update application/i }), + ); + + await waitFor(() => + expect(onSubmit).toHaveBeenCalledWith({ + name: app.name, + redirect_uris: ["https://c.example.com/cb", "https://b.example.com/cb"], + icon: app.icon, + }), + ); + }); + + it("removes an entry", async () => { + const user = userEvent.setup(); + const onSubmit = vi.fn(); + const app = { + ...MockOAuth2ProviderApps[0], + redirect_uris: ["https://a.example.com/cb", "https://b.example.com/cb"], + }; + + render( + , + ); + + await user.click( + screen.getByRole("button", { name: /remove redirect uri 2/i }), + ); + await user.click( + screen.getByRole("button", { name: /update application/i }), + ); + + await waitFor(() => + expect(onSubmit).toHaveBeenCalledWith({ + name: app.name, + redirect_uris: ["https://a.example.com/cb"], + icon: app.icon, + }), + ); + }); + + it("adds an entry", async () => { + const user = userEvent.setup(); + const onSubmit = vi.fn(); + const app = { + ...MockOAuth2ProviderApps[0], + redirect_uris: ["https://a.example.com/cb", "https://b.example.com/cb"], + }; + + render( + , + ); + + await user.click(screen.getByRole("button", { name: /add redirect uri/i })); + await user.type( + screen.getByLabelText(/^redirect uri 3/i), + "https://d.example.com/cb", + ); + await user.click( + screen.getByRole("button", { name: /update application/i }), + ); + + await waitFor(() => + expect(onSubmit).toHaveBeenCalledWith({ + name: app.name, + redirect_uris: [ + "https://a.example.com/cb", + "https://b.example.com/cb", + "https://d.example.com/cb", + ], + icon: app.icon, + }), + ); + }); + + it("disables submit after removing the only entry", async () => { + const user = userEvent.setup(); + const onSubmit = vi.fn(); + const app = { + ...MockOAuth2ProviderApps[0], + redirect_uris: ["https://a.example.com/cb"], + }; + + render( + , + ); + + await user.click( + screen.getByRole("button", { name: /remove redirect uri 1/i }), + ); + + await waitFor(() => + expect( + screen.getByRole("button", { name: /update application/i }), + ).toBeDisabled(), + ); + expect( + screen.getByText(/at least one redirect uri is required/i), + ).toBeInTheDocument(); + }); + + it("does not submit when a public app has a disallowed entry in another row", async () => { + const user = userEvent.setup(); + const onSubmit = vi.fn(); + const app = { + ...MockOAuth2ProviderAppPublic, + redirect_uris: ["vscode://coder.coder-remote/oauth/callback"], + }; + + render( + , + ); + + await user.click(screen.getByRole("button", { name: /add redirect uri/i })); + await user.type( + screen.getByLabelText(/^redirect uri 2/i), + "http://example.com/cb", + ); + await user.click( + screen.getByRole("button", { name: /update application/i }), + ); + + await act(async () => {}); + expect(onSubmit).not.toHaveBeenCalled(); + }); + + it("does not submit when a row duplicates another row", async () => { + const user = userEvent.setup(); + const onSubmit = vi.fn(); + const app = { + ...MockOAuth2ProviderApps[0], + redirect_uris: ["https://a.example.com/cb", "https://b.example.com/cb"], + }; + + render( + , + ); + + await user.clear(screen.getByLabelText(/^redirect uri 2/i)); + await user.type( + screen.getByLabelText(/^redirect uri 2/i), + "https://a.example.com/cb", + ); + await user.click( + screen.getByRole("button", { name: /update application/i }), + ); + + await act(async () => {}); + expect(onSubmit).not.toHaveBeenCalled(); + expect( + screen.getByText(/already used by another row/i), + ).toBeInTheDocument(); + }); + + it("flags every row that duplicates an earlier one", async () => { + const user = userEvent.setup(); + const onSubmit = vi.fn(); + const app = { + ...MockOAuth2ProviderApps[0], + redirect_uris: [ + "https://a.example.com/cb", + "https://b.example.com/cb", + "https://c.example.com/cb", + ], + }; + + render( + , + ); + + for (const row of [2, 3]) { + const field = screen.getByLabelText( + new RegExp(`^redirect uri ${row}`, "i"), + ); + await user.clear(field); + await user.type(field, "https://a.example.com/cb"); + } + await user.click( + screen.getByRole("button", { name: /update application/i }), + ); + + await act(async () => {}); + expect(onSubmit).not.toHaveBeenCalled(); + expect(screen.getAllByText(/already used by another row/i)).toHaveLength(2); + }); + + // Confidential apps could store a non-local http redirect URI before the + // form checked for it. The stored value fails validation on load, so the + // error must show without the field being touched. + it("shows the error for a stored redirect URI that fails validation", async () => { const user = userEvent.setup(); const onSubmit = vi.fn(); render( { ); expect( - await screen.findByText("Please enter a valid callback URL."), + await screen.findByText("Please enter a valid redirect URI."), ).toBeInTheDocument(); await user.type(screen.getByLabelText(/^name/i), " updated"); expect( screen.getByRole("button", { name: /update application/i }), ).toBeDisabled(); - await user.clear(screen.getByLabelText(/callback url/i)); + await user.clear(screen.getByLabelText(/default callback/i)); await user.type( - screen.getByLabelText(/callback url/i), + screen.getByLabelText(/default callback/i), "https://intranet.example.com/callback", ); await waitFor(() => expect( - screen.queryByText("Please enter a valid callback URL."), + screen.queryByText("Please enter a valid redirect URI."), ).not.toBeInTheDocument(), ); await user.click( @@ -334,17 +696,47 @@ describe("OAuth2AppForm", () => { await waitFor(() => expect(onSubmit).toHaveBeenCalledWith({ name: `${MockOAuth2ProviderApps[0].name} updated`, - callback_url: "https://intranet.example.com/callback", + redirect_uris: ["https://intranet.example.com/callback"], icon: MockOAuth2ProviderApps[0].icon, }), ); }); - it("keeps the callback error hidden when the stored callback is valid", async () => { + // A list over the count cap has no row to blur, so its message must also + // show on load. + it("shows the error for a stored list over the count cap", async () => { + render( + `https://alt-${i}.example.com/cb`, + ), + }} + clientType="confidential" + onSubmit={vi.fn()} + isUpdating={false} + disabled={false} + />, + ); + + expect( + await screen.findByText( + `At most ${OAuth2RedirectURIsMaxCount} redirect URIs are allowed.`, + ), + ).toBeInTheDocument(); + expect( + screen.getByRole("button", { name: /update application/i }), + ).toBeDisabled(); + }); + + it("keeps the redirect URI error hidden when the stored list is valid", async () => { const user = userEvent.setup(); render( { await user.type(screen.getByLabelText(/^name/i), " updated"); expect( - screen.queryByText("Please enter a valid callback URL."), + screen.queryByText("Please enter a valid redirect URI."), ).not.toBeInTheDocument(); expect( screen.getByRole("button", { name: /update application/i }), @@ -364,12 +756,17 @@ describe("OAuth2AppForm", () => { const onSubmit = vi.fn(); const user = userEvent.setup(); render( - , + , ); await user.type(screen.getByLabelText(/^name/i), "test-app"); await user.type( - screen.getByLabelText(/callback url/i), + screen.getByLabelText(/default callback/i), "https://example.com/callback", ); await selectScope(user, "workspace:ssh"); @@ -381,7 +778,7 @@ describe("OAuth2AppForm", () => { await waitFor(() => expect(onSubmit).toHaveBeenCalledWith({ name: "test-app", - callback_url: "https://example.com/callback", + redirect_uris: ["https://example.com/callback"], icon: "", scope: "workspace:ssh coder:all", }), @@ -394,6 +791,7 @@ describe("OAuth2AppForm", () => { render( { render( { render( { await waitFor(() => expect(onSubmit).toHaveBeenCalledTimes(1)); expect(onSubmit.mock.calls[0][0]).toStrictEqual({ name: "foo-updated", - callback_url: MockOAuth2ProviderApps[0].callback_url, icon: MockOAuth2ProviderApps[0].icon, }); }, @@ -499,6 +898,7 @@ describe("OAuth2AppForm", () => { render( { await waitFor(() => expect(onSubmit).toHaveBeenCalledTimes(1)); expect(onSubmit.mock.calls[0][0]).toStrictEqual({ name: "foo-updated", - callback_url: MockOAuth2ProviderApps[0].callback_url, icon: MockOAuth2ProviderApps[0].icon, }); }); @@ -524,6 +923,7 @@ describe("OAuth2AppForm", () => { render( void | Promise; + } + | { + app: TypesGen.OAuth2ProviderApp; + onSubmit: ( + data: TypesGen.PutOAuth2ProviderAppRequest, + ) => void | Promise; + }; -type OAuth2AppFormProps = { - app?: TypesGen.OAuth2ProviderApp; - onSubmit: (data: OAuth2AppFormRequest) => void | Promise; +type OAuth2AppFormProps = OAuth2AppFormSubmit & { + // Passed on its own because the create page has no app yet. The client + // type decides which redirect URI rules apply. + clientType: TypesGen.OAuth2ClientType; error?: unknown; isUpdating: boolean; defaultValues?: Partial; @@ -104,6 +120,50 @@ const isValidCallbackURL = ( } }; +const redirectURIsSchema = (isPublicClient: boolean) => + Yup.array() + .of( + Yup.string() + .trim() + .test( + "redirect-uri-byte-length", + `A redirect URI cannot be longer than ${OAuth2RedirectURIMaxBytes} UTF-8 bytes.`, + (value) => + new TextEncoder().encode(value).length <= OAuth2RedirectURIMaxBytes, + ) + .test( + "valid-redirect-uri", + "Please enter a valid redirect URI.", + (value) => isValidCallbackURL(value, isPublicClient), + ), + ) + // The server also deduplicates on save, so without this the form + // would submit a URI it never actually saved and mislead the user. + .test( + "unique-redirect-uri", + "This redirect URI is already used by another row.", + function (uris) { + const seen = new Set(); + const errors: Yup.ValidationError[] = []; + for (const [index, uri] of (uris ?? []).entries()) { + const trimmed = uri?.trim(); + if (!trimmed) { + continue; + } + if (seen.has(trimmed)) { + errors.push(this.createError({ path: `${this.path}[${index}]` })); + } + seen.add(trimmed); + } + return errors.length === 0 || new Yup.ValidationError(errors); + }, + ) + .min(1, "At least one redirect URI is required.") + .max( + OAuth2RedirectURIsMaxCount, + `At most ${OAuth2RedirectURIsMaxCount} redirect URIs are allowed.`, + ); + const validationSchema = (isPublicClient: boolean) => Yup.object({ name: Yup.string() @@ -115,63 +175,91 @@ const validationSchema = (isPublicClient: boolean) => (value) => new TextEncoder().encode(value).length <= OAuth2AppNameMaxBytes, ), - callback_url: Yup.string() - .trim() - .required("Please enter a callback URL.") - .test( - "valid-callback-url", - "Please enter a valid callback URL.", - (value) => isValidCallbackURL(value, isPublicClient), - ), + redirect_uris: redirectURIsSchema(isPublicClient), icon: iconValidator, }); +// app and onSubmit stay together in submit so that checking app also +// narrows the type of onSubmit. export const OAuth2AppForm: FC = ({ - app, - onSubmit, + clientType, error, isUpdating, defaultValues, disabled, onIconChange, + ...submit }) => { + const { app } = submit; const didSubmit = useRef(false); - const isPublicClient = app?.client_type === "public"; - // A stored callback that no longer passes validation disables Update on - // load. Show its error right away instead of waiting for the field to be + const isPublicClient = clientType === "public"; + // A stored list that no longer passes validation disables Update on load. + // Show its errors right away instead of waiting for the field to be // touched, so the admin can see what to correct. - const storedCallbackInvalid = - app !== undefined && !isValidCallbackURL(app.callback_url, isPublicClient); + const storedRedirectURIsInvalid = + app !== undefined && + !redirectURIsSchema(isPublicClient).isValidSync(app.redirect_uris); const form = useFormik({ initialValues: { name: app?.name ?? defaultValues?.name ?? "", - callback_url: app?.callback_url ?? defaultValues?.callback_url ?? "", + redirect_uris: app + ? [...app.redirect_uris] + : defaultValues?.redirect_uris?.length + ? [...defaultValues.redirect_uris] + : [""], icon: app?.icon ?? defaultValues?.icon ?? "", scope: app?.scope.split(" ").filter(Boolean) ?? defaultValues?.scope ?? [], }, - initialTouched: storedCallbackInvalid ? { callback_url: true } : undefined, + initialTouched: storedRedirectURIsInvalid + ? { redirect_uris: true } + : undefined, validationSchema: validationSchema(isPublicClient), validateOnMount: true, - onSubmit: async ({ scope: selectedScopes, ...values }) => { + onSubmit: async ({ + scope: selectedScopes, + redirect_uris: enteredRedirectURIs, + ...values + }) => { didSubmit.current = true; + const redirectURIs = enteredRedirectURIs.map((uri) => uri.trim()); const scope = selectedScopes.join(" "); // An untouched allowlist is left out of updates rather than echoed // back. The form cannot round-trip a stored list exactly: a whitespace // only list grants nothing but would resend as "" and lift the // restriction, and a legacy list may exceed the current size limits. const scopeChanged = !app || scope !== form.initialValues.scope.join(" "); - await onSubmit({ + const request = { ...values, name: values.name.trim(), - callback_url: values.callback_url.trim(), ...(scopeChanged ? { scope } : {}), - }); + }; + if (submit.app === undefined) { + await submit.onSubmit({ ...request, redirect_uris: redirectURIs }); + return; + } + // An untouched list is also left out, so a copy loaded before another + // admin changed the list does not overwrite their change. + const stored = submit.app.redirect_uris; + const redirectURIsChanged = + redirectURIs.length !== stored.length || + redirectURIs.some((uri, index) => uri !== stored[index]); + await submit.onSubmit( + redirectURIsChanged + ? { ...request, redirect_uris: redirectURIs } + : request, + ); }, }); const scopesQuery = useQuery(getExternalScopes()); const getFieldHelpers = getFormHelpers(form, error); const iconField = getFieldHelpers("icon"); + const redirectURIsField = getFieldHelpers("redirect_uris"); + const redirectURIsErrors = form.errors.redirect_uris; + const redirectURIsRowErrors = + form.touched.redirect_uris && Array.isArray(redirectURIsErrors) + ? redirectURIsErrors + : undefined; const formDisabled = disabled || isUpdating; const editing = Boolean(app); const submitDisabled = @@ -201,6 +289,30 @@ export const OAuth2AppForm: FC = ({ onIconChange?.(value); }; + const handleRedirectURIsChange = (values: string[]) => { + void form.setFieldValue("redirect_uris", values); + // Removing the last row disables Update without a blur event to mark + // the field touched, so the required-URI message never shows. Mark it + // touched here (skipping its own validate call, since setFieldValue + // above already triggers one) only for that case, since doing this on + // every keystroke would race with the blur-triggered validateForm below. + if (values.length === 0) { + void form.setFieldTouched("redirect_uris", true, false); + } + }; + // Touched is set on blur rather than on every keystroke. Setting it in + // handleRedirectURIsChange as well would fire a second concurrent + // validateForm per keystroke; Formik does not guarantee those resolve in + // order, so the result of an earlier keystroke can overwrite the latest. + const handleRedirectURIsBlur = () => { + void form.setFieldTouched("redirect_uris", true); + }; + // The overall-list message (min/max count, or a server validation error) + // is shown once below the rows; a per-row message is shown on its row + // instead, so this excludes the case where Yup reports one message per row. + const showRedirectURIsListError = + redirectURIsField.error && !Array.isArray(redirectURIsErrors); + return (
@@ -213,13 +325,21 @@ export const OAuth2AppForm: FC = ({ autoFocus required /> - +
+ + {showRedirectURIsListError && ( + + {redirectURIsField.helperText} + + )} +
diff --git a/site/src/pages/DeploymentSettingsPage/OAuth2AppsSettingsPage/OAuth2AppsSettingsPageView.stories.tsx b/site/src/pages/DeploymentSettingsPage/OAuth2AppsSettingsPage/OAuth2AppsSettingsPageView.stories.tsx index 0c7a2c6666..e5096aaa5a 100644 --- a/site/src/pages/DeploymentSettingsPage/OAuth2AppsSettingsPage/OAuth2AppsSettingsPageView.stories.tsx +++ b/site/src/pages/DeploymentSettingsPage/OAuth2AppsSettingsPage/OAuth2AppsSettingsPageView.stories.tsx @@ -64,6 +64,21 @@ export const Apps: Story = { }, }; +export const AppWithMultipleRedirectURIs: Story = { + args: { + isLoadingApps: false, + apps: [ + { + ...MockOAuth2ProviderApps[0], + redirect_uris: [ + MockOAuth2ProviderApps[0].callback_url, + "https://example.com/callback", + ], + }, + ], + }, +}; + export const Empty: Story = { args: { isLoadingApps: false, diff --git a/site/src/pages/DeploymentSettingsPage/OAuth2AppsSettingsPage/OAuth2AppsSettingsPageView.tsx b/site/src/pages/DeploymentSettingsPage/OAuth2AppsSettingsPage/OAuth2AppsSettingsPageView.tsx index e59c3fdb9f..8e5fea251a 100644 --- a/site/src/pages/DeploymentSettingsPage/OAuth2AppsSettingsPage/OAuth2AppsSettingsPageView.tsx +++ b/site/src/pages/DeploymentSettingsPage/OAuth2AppsSettingsPage/OAuth2AppsSettingsPageView.tsx @@ -5,6 +5,7 @@ import type * as TypesGen from "#/api/typesGenerated"; import { ErrorAlert } from "#/components/Alert/ErrorAlert"; import { Avatar } from "#/components/Avatar/Avatar"; import { AvatarData } from "#/components/Avatar/AvatarData"; +import { Badge } from "#/components/Badge/Badge"; import { Button } from "#/components/Button/Button"; import { Loader } from "#/components/Loader/Loader"; import { @@ -243,12 +244,24 @@ const OAuth2AppRow: FC = ({ app }) => { /> - - {app.callback_url} - +
+ + {app.redirect_uris[0]} + + {app.redirect_uris.length > 1 && ( + 2 ? "s" : ""}`} + > + +{app.redirect_uris.length - 1} + + )} +
diff --git a/site/src/pages/DeploymentSettingsPage/OAuth2AppsSettingsPage/components/RedirectURIsField.test.tsx b/site/src/pages/DeploymentSettingsPage/OAuth2AppsSettingsPage/components/RedirectURIsField.test.tsx new file mode 100644 index 0000000000..5aad393f65 --- /dev/null +++ b/site/src/pages/DeploymentSettingsPage/OAuth2AppsSettingsPage/components/RedirectURIsField.test.tsx @@ -0,0 +1,109 @@ +import { screen } from "@testing-library/react"; +import userEvent from "@testing-library/user-event"; +import { useState } from "react"; +import { render } from "#/testHelpers/renderHelpers"; +import { RedirectURIsField } from "./RedirectURIsField"; + +// onChange is a controlled callback, so exercising edit/remove/add needs a +// stateful wrapper to feed each call's result back in as the next `values`. +const ControlledField = ({ + initialValues, + onChange, + isPublicClient = false, +}: { + initialValues: string[]; + onChange: (values: string[]) => void; + isPublicClient?: boolean; +}) => { + const [values, setValues] = useState(initialValues); + return ( + { + setValues(next); + onChange(next); + }} + onBlur={() => {}} + /> + ); +}; + +describe("RedirectURIsField", () => { + it("calls onChange with the edited entry", async () => { + const user = userEvent.setup(); + const onChange = vi.fn(); + render( + , + ); + + await user.type(screen.getByLabelText(/default callback/i), "2"); + + expect(onChange).toHaveBeenLastCalledWith(["https://a.example.com/cb2"]); + }); + + it("calls onChange with the entry removed", async () => { + const user = userEvent.setup(); + const onChange = vi.fn(); + render( + , + ); + + await user.click( + screen.getByRole("button", { name: /remove redirect uri 2/i }), + ); + + expect(onChange).toHaveBeenCalledWith(["https://a.example.com/cb"]); + }); + + it("calls onChange with a new empty entry appended", async () => { + const user = userEvent.setup(); + const onChange = vi.fn(); + render( + , + ); + + await user.click(screen.getByRole("button", { name: /add redirect uri/i })); + + expect(onChange).toHaveBeenCalledWith(["https://a.example.com/cb", ""]); + }); + + it("allows removing the only entry, leaving the list empty", async () => { + const user = userEvent.setup(); + const onChange = vi.fn(); + render( + , + ); + + await user.click( + screen.getByRole("button", { name: /remove redirect uri 1/i }), + ); + + expect(onChange).toHaveBeenCalledWith([]); + }); + + it("disables Add redirect URI once at the max count", () => { + const values = Array.from( + { length: 32 }, + (_, i) => `https://${i}.example.com/cb`, + ); + render(); + + expect( + screen.getByRole("button", { name: /add redirect uri/i }), + ).toBeDisabled(); + }); +}); diff --git a/site/src/pages/DeploymentSettingsPage/OAuth2AppsSettingsPage/components/RedirectURIsField.tsx b/site/src/pages/DeploymentSettingsPage/OAuth2AppsSettingsPage/components/RedirectURIsField.tsx new file mode 100644 index 0000000000..b235e8b592 --- /dev/null +++ b/site/src/pages/DeploymentSettingsPage/OAuth2AppsSettingsPage/components/RedirectURIsField.tsx @@ -0,0 +1,97 @@ +import { PlusIcon, XIcon } from "lucide-react"; +import { type FC, useId } from "react"; +import { OAuth2RedirectURIsMaxCount } from "#/api/typesGenerated"; +import { Button } from "#/components/Button/Button"; +import { FormField } from "#/components/FormField/FormField"; +import type { FormHelpers } from "#/utils/formUtils"; + +type RedirectURIsFieldProps = { + values: string[]; + onChange: (values: string[]) => void; + onBlur: () => void; + disabled: boolean; + isPublicClient: boolean; + errors?: string[]; +}; + +export const RedirectURIsField: FC = ({ + values, + onChange, + onBlur, + disabled, + isPublicClient, + errors, +}) => { + const baseId = useId(); + const atMax = values.length >= OAuth2RedirectURIsMaxCount; + + const setEntry = (index: number, value: string) => { + onChange(values.map((v, i) => (i === index ? value : v))); + }; + + const removeEntry = (index: number) => { + onChange(values.filter((_, i) => i !== index)); + }; + + return ( +
+
+ Redirect URIs + + {isPublicClient + ? "URIs this application may redirect to. This app has no client secret, so http is limited to a loopback address; https, a custom scheme, and the out-of-band URN are also accepted." + : "URIs this application may redirect to after a user authorizes it."} + +
+ + {values.map((value, index) => { + const fieldHelpers: FormHelpers = { + name: `redirect_uris.${index}`, + id: `${baseId}-${index}`, + value, + error: Boolean(errors?.[index]), + helperText: errors?.[index], + onChange: (event) => setEntry(index, event.target.value), + onBlur, + }; + + return ( +
+
+ +
+ +
+ ); + })} + + +
+ ); +};