diff --git a/backend/internal/common/errors.go b/backend/internal/common/errors.go index c570bd32..4f0608bc 100644 --- a/backend/internal/common/errors.go +++ b/backend/internal/common/errors.go @@ -368,3 +368,13 @@ func (e *OpenSignupDisabledError) Error() string { func (e *OpenSignupDisabledError) HttpStatusCode() int { return http.StatusForbidden } + +type ClientIdAlreadyExistsError struct{} + +func (e *ClientIdAlreadyExistsError) Error() string { + return "Client ID already in use" +} + +func (e *ClientIdAlreadyExistsError) HttpStatusCode() int { + return http.StatusBadRequest +} diff --git a/backend/internal/controller/oidc_controller.go b/backend/internal/controller/oidc_controller.go index 2f8a3ea3..82317b6c 100644 --- a/backend/internal/controller/oidc_controller.go +++ b/backend/internal/controller/oidc_controller.go @@ -492,11 +492,11 @@ func (oc *OidcController) deleteClientHandler(c *gin.Context) { // @Accept json // @Produce json // @Param id path string true "Client ID" -// @Param client body dto.OidcClientCreateDto true "Client information" +// @Param client body dto.OidcClientUpdateDto true "Client information" // @Success 200 {object} dto.OidcClientWithAllowedUserGroupsDto "Updated client" // @Router /api/oidc/clients/{id} [put] func (oc *OidcController) updateClientHandler(c *gin.Context) { - var input dto.OidcClientCreateDto + var input dto.OidcClientUpdateDto if err := c.ShouldBindJSON(&input); err != nil { _ = c.Error(err) return diff --git a/backend/internal/dto/oidc_dto.go b/backend/internal/dto/oidc_dto.go index c76b495d..6d95ee2d 100644 --- a/backend/internal/dto/oidc_dto.go +++ b/backend/internal/dto/oidc_dto.go @@ -29,7 +29,7 @@ type OidcClientWithAllowedGroupsCountDto struct { AllowedUserGroupsCount int64 `json:"allowedUserGroupsCount"` } -type OidcClientCreateDto struct { +type OidcClientUpdateDto struct { Name string `json:"name" binding:"required,max=50" unorm:"nfc"` CallbackURLs []string `json:"callbackURLs"` LogoutCallbackURLs []string `json:"logoutCallbackURLs"` @@ -40,6 +40,11 @@ type OidcClientCreateDto struct { LaunchURL *string `json:"launchURL" binding:"omitempty,url"` } +type OidcClientCreateDto struct { + OidcClientUpdateDto + ID string `json:"id" binding:"omitempty,client_id,min=2,max=128"` +} + type OidcClientCredentialsDto struct { FederatedIdentities []OidcClientFederatedIdentityDto `json:"federatedIdentities,omitempty"` } diff --git a/backend/internal/dto/validations.go b/backend/internal/dto/validations.go index 4db3695d..80c3ea56 100644 --- a/backend/internal/dto/validations.go +++ b/backend/internal/dto/validations.go @@ -18,6 +18,8 @@ func init() { // [a-zA-Z0-9]$ : The username must end with an alphanumeric character var validateUsernameRegex = regexp.MustCompile("^[a-zA-Z0-9][a-zA-Z0-9_.@-]*[a-zA-Z0-9]$") + var validateClientIDRegex = regexp.MustCompile("^[a-zA-Z0-9._-]+$") + // Maximum allowed value for TTLs const maxTTL = 31 * 24 * time.Hour @@ -28,6 +30,14 @@ func init() { if err != nil { panic("Failed to register custom validation for username: " + err.Error()) } + + err = v.RegisterValidation("client_id", func(fl validator.FieldLevel) bool { + return validateClientIDRegex.MatchString(fl.Field().String()) + }) + if err != nil { + panic("Failed to register custom validation for client_id: " + err.Error()) + } + err = v.RegisterValidation("ttl", func(fl validator.FieldLevel) bool { ttl, ok := fl.Field().Interface().(utils.JSONDuration) if !ok { diff --git a/backend/internal/service/oidc_service.go b/backend/internal/service/oidc_service.go index 7649e07f..197d0bcd 100644 --- a/backend/internal/service/oidc_service.go +++ b/backend/internal/service/oidc_service.go @@ -670,22 +670,28 @@ func (s *OidcService) ListClients(ctx context.Context, name string, sortedPagina func (s *OidcService) CreateClient(ctx context.Context, input dto.OidcClientCreateDto, userID string) (model.OidcClient, error) { client := model.OidcClient{ + Base: model.Base{ + ID: input.ID, + }, CreatedByID: utils.Ptr(userID), } - updateOIDCClientModelFromDto(&client, &input) + updateOIDCClientModelFromDto(&client, &input.OidcClientUpdateDto) err := s.db. WithContext(ctx). Create(&client). Error if err != nil { + if errors.Is(err, gorm.ErrDuplicatedKey) { + return model.OidcClient{}, &common.ClientIdAlreadyExistsError{} + } return model.OidcClient{}, err } return client, nil } -func (s *OidcService) UpdateClient(ctx context.Context, clientID string, input dto.OidcClientCreateDto) (model.OidcClient, error) { +func (s *OidcService) UpdateClient(ctx context.Context, clientID string, input dto.OidcClientUpdateDto) (model.OidcClient, error) { tx := s.db.Begin() defer func() { tx.Rollback() @@ -719,7 +725,7 @@ func (s *OidcService) UpdateClient(ctx context.Context, clientID string, input d return client, nil } -func updateOIDCClientModelFromDto(client *model.OidcClient, input *dto.OidcClientCreateDto) { +func updateOIDCClientModelFromDto(client *model.OidcClient, input *dto.OidcClientUpdateDto) { // Base fields client.Name = input.Name client.CallbackURLs = input.CallbackURLs diff --git a/backend/internal/service/oidc_service_test.go b/backend/internal/service/oidc_service_test.go index 86448cad..fce54eef 100644 --- a/backend/internal/service/oidc_service_test.go +++ b/backend/internal/service/oidc_service_test.go @@ -171,8 +171,10 @@ func TestOidcService_verifyClientCredentialsInternal(t *testing.T) { // Create the test clients // 1. Confidential client confidentialClient, err := s.CreateClient(t.Context(), dto.OidcClientCreateDto{ - Name: "Confidential Client", - CallbackURLs: []string{"https://example.com/callback"}, + OidcClientUpdateDto: dto.OidcClientUpdateDto{ + Name: "Confidential Client", + CallbackURLs: []string{"https://example.com/callback"}, + }, }, "test-user-id") require.NoError(t, err) @@ -182,20 +184,24 @@ func TestOidcService_verifyClientCredentialsInternal(t *testing.T) { // 2. Public client publicClient, err := s.CreateClient(t.Context(), dto.OidcClientCreateDto{ - Name: "Public Client", - CallbackURLs: []string{"https://example.com/callback"}, - IsPublic: true, + OidcClientUpdateDto: dto.OidcClientUpdateDto{ + Name: "Public Client", + CallbackURLs: []string{"https://example.com/callback"}, + IsPublic: true, + }, }, "test-user-id") require.NoError(t, err) // 3. Confidential client with federated identity federatedClient, err := s.CreateClient(t.Context(), dto.OidcClientCreateDto{ - Name: "Federated Client", - CallbackURLs: []string{"https://example.com/callback"}, + OidcClientUpdateDto: dto.OidcClientUpdateDto{ + Name: "Federated Client", + CallbackURLs: []string{"https://example.com/callback"}, + }, }, "test-user-id") require.NoError(t, err) - federatedClient, err = s.UpdateClient(t.Context(), federatedClient.ID, dto.OidcClientCreateDto{ + federatedClient, err = s.UpdateClient(t.Context(), federatedClient.ID, dto.OidcClientUpdateDto{ Name: federatedClient.Name, CallbackURLs: federatedClient.CallbackURLs, Credentials: dto.OidcClientCredentialsDto{ diff --git a/frontend/messages/en.json b/frontend/messages/en.json index 919ad851..61c68003 100644 --- a/frontend/messages/en.json +++ b/frontend/messages/en.json @@ -437,5 +437,8 @@ "revoke_access": "Revoke Access", "revoke_access_description": "Revoke access to {clientName}. {clientName} will no longer be able to access your account information.", "revoke_access_successful": "The access to {clientName} has been successfully revoked.", - "last_signed_in_ago": "Last signed in {time} ago" + "last_signed_in_ago": "Last signed in {time} ago", + "invalid_client_id": "Client ID can only contain letters, numbers, underscores, and hyphens", + "custom_client_id_description": "Set a custom client ID if this is required by your application. Otherwise, leave it blank to generate a random one.", + "generated": "Generated" } diff --git a/frontend/src/lib/services/oidc-service.ts b/frontend/src/lib/services/oidc-service.ts index 3a1c101a..d36bec09 100644 --- a/frontend/src/lib/services/oidc-service.ts +++ b/frontend/src/lib/services/oidc-service.ts @@ -4,6 +4,7 @@ import type { OidcClient, OidcClientCreate, OidcClientMetaData, + OidcClientUpdate, OidcClientWithAllowedUserGroups, OidcClientWithAllowedUserGroupsCount, OidcDeviceCodeInfo @@ -67,7 +68,7 @@ class OidcService extends APIService { return (await this.api.get(`/oidc/clients/${id}/meta`)).data as OidcClientMetaData; } - async updateClient(id: string, client: OidcClientCreate) { + async updateClient(id: string, client: OidcClientUpdate) { return (await this.api.put(`/oidc/clients/${id}`, client)).data as OidcClient; } diff --git a/frontend/src/lib/types/oidc.type.ts b/frontend/src/lib/types/oidc.type.ts index 470796cd..f46a1f36 100644 --- a/frontend/src/lib/types/oidc.type.ts +++ b/frontend/src/lib/types/oidc.type.ts @@ -37,7 +37,13 @@ export type OidcClientWithAllowedUserGroupsCount = OidcClient & { allowedUserGroupsCount: number; }; -export type OidcClientCreate = Omit; +export type OidcClientUpdate = Omit; +export type OidcClientCreate = OidcClientUpdate & { + id?: string; +}; +export type OidcClientUpdateWithLogo = OidcClientUpdate & { + logo: File | null | undefined; +}; export type OidcClientCreateWithLogo = OidcClientCreate & { logo: File | null | undefined; diff --git a/frontend/src/lib/utils/zod-util.ts b/frontend/src/lib/utils/zod-util.ts index 0a304415..9806106d 100644 --- a/frontend/src/lib/utils/zod-util.ts +++ b/frontend/src/lib/utils/zod-util.ts @@ -1,9 +1,7 @@ import z from 'zod/v4'; -export const optionalString = z - .string() - .transform((v) => (v === '' ? undefined : v)) - .optional(); +export const emptyToUndefined = (validation: z.ZodType) => + z.preprocess((v) => (v === '' ? undefined : v), validation); export const optionalUrl = z .url() diff --git a/frontend/src/routes/settings/admin/api-keys/api-key-form.svelte b/frontend/src/routes/settings/admin/api-keys/api-key-form.svelte index b65af4ec..09518f2b 100644 --- a/frontend/src/routes/settings/admin/api-keys/api-key-form.svelte +++ b/frontend/src/routes/settings/admin/api-keys/api-key-form.svelte @@ -5,7 +5,7 @@ import type { ApiKeyCreate } from '$lib/types/api-key.type'; import { preventDefault } from '$lib/utils/event-util'; import { createForm } from '$lib/utils/form-util'; - import { optionalString } from '$lib/utils/zod-util'; + import { emptyToUndefined } from '$lib/utils/zod-util'; import { z } from 'zod/v4'; let { @@ -28,7 +28,7 @@ const formSchema = z.object({ name: z.string().min(3).max(50), - description: optionalString, + description: emptyToUndefined(z.string().optional()), expiresAt: z.date().min(new Date(), m.expiration_date_must_be_in_the_future()) }); diff --git a/frontend/src/routes/settings/admin/oidc-clients/+page.svelte b/frontend/src/routes/settings/admin/oidc-clients/+page.svelte index 65053932..67875646 100644 --- a/frontend/src/routes/settings/admin/oidc-clients/+page.svelte +++ b/frontend/src/routes/settings/admin/oidc-clients/+page.svelte @@ -70,7 +70,7 @@ {#if expandAddClient}
- +
{/if} diff --git a/frontend/src/routes/settings/admin/oidc-clients/[id]/+page.svelte b/frontend/src/routes/settings/admin/oidc-clients/[id]/+page.svelte index 085c8666..dda8ca6e 100644 --- a/frontend/src/routes/settings/admin/oidc-clients/[id]/+page.svelte +++ b/frontend/src/routes/settings/admin/oidc-clients/[id]/+page.svelte @@ -179,7 +179,7 @@ - + Promise; + callback: (client: OidcClientCreateWithLogo | OidcClientUpdateWithLogo) => Promise; + mode: 'create' | 'update'; } = $props(); let isLoading = $state(false); @@ -34,6 +40,7 @@ ); const client = { + id: '', name: existingClient?.name || '', callbackURLs: existingClient?.callbackURLs || [], logoutCallbackURLs: existingClient?.logoutCallbackURLs || [], @@ -47,6 +54,16 @@ }; const formSchema = z.object({ + id: emptyToUndefined( + z + .string() + .min(2) + .max(128) + .regex(/^[a-zA-Z0-9_-]+$/, { + message: m.invalid_client_id() + }) + .optional() + ), name: z.string().min(2).max(50), callbackURLs: z.array(z.string().nonempty()).default([]), logoutCallbackURLs: z.array(z.string().nonempty()), @@ -185,7 +202,16 @@ {#if showAdvancedOptions} -
+
+ {#if mode == 'create'} + + {/if} await cleanupBackend()); -test('Create OIDC client', async ({ page }) => { - await page.goto('/settings/admin/oidc-clients'); - const oidcClient = oidcClients.pingvinShare; +test.describe('Create OIDC client', () => { + async function createClientTest(page: Page, clientId?: string) { + const oidcClient = oidcClients.pingvinShare; + await page.goto('/settings/admin/oidc-clients'); + await page.getByRole('button', { name: 'Add OIDC Client' }).click(); - await page.getByRole('button', { name: 'Add OIDC Client' }).click(); - await page.getByLabel('Name').fill(oidcClient.name); + await page.getByLabel('Name').fill(oidcClient.name); + await page.getByLabel('Client Launch URL').fill(oidcClient.launchURL); - await page.getByLabel('Client Launch URL').fill(oidcClient.launchURL); + await page.getByRole('button', { name: 'Add' }).first().click(); + await page.getByTestId('callback-url-1').fill(oidcClient.callbackUrl); - await page.getByRole('button', { name: 'Add' }).nth(1).click(); - await page.getByTestId('callback-url-1').fill(oidcClient.callbackUrl); - await page.getByRole('button', { name: 'Add another' }).click(); - await page.getByTestId('callback-url-2').fill(oidcClient.secondCallbackUrl!); + await page.getByRole('button', { name: 'Add another' }).click(); + await page.getByTestId('callback-url-2').fill(oidcClient.secondCallbackUrl); - await page.getByLabel('logo').setInputFiles('assets/pingvin-share-logo.png'); - await page.getByRole('button', { name: 'Save' }).click(); + await page.getByLabel('logo').setInputFiles('assets/pingvin-share-logo.png'); - const clientId = await page.getByTestId('client-id').textContent(); + if (clientId) { + await page.getByRole('button', { name: 'Show Advanced Options' }).click(); + await page.getByLabel('Client ID').fill(clientId); + } - await expect(page.locator('[data-type="success"]')).toHaveText( - 'OIDC client created successfully' - ); - expect(clientId?.length).toBe(36); - expect((await page.getByTestId('client-secret').textContent())?.length).toBe(32); - await expect(page.getByLabel('Name')).toHaveValue(oidcClient.name); - await expect(page.getByTestId('callback-url-1')).toHaveValue(oidcClient.callbackUrl); - await expect(page.getByTestId('callback-url-2')).toHaveValue(oidcClient.secondCallbackUrl!); - await expect(page.getByRole('img', { name: `${oidcClient.name} logo` })).toBeVisible(); - await page.request - .get(`/api/oidc/clients/${clientId}/logo`) - .then((res) => expect.soft(res.status()).toBe(200)); + await page.getByRole('button', { name: 'Save' }).click(); + + await expect(page.locator('[data-type="success"]')).toHaveText( + 'OIDC client created successfully' + ); + + const resolvedClientId = (await page.getByTestId('client-id').innerText()).trim(); + const clientSecret = (await page.getByTestId('client-secret').innerText()).trim(); + + if (clientId) { + expect(resolvedClientId).toBe(clientId); + } else { + expect(resolvedClientId).toMatch(/^[\w-]{36}$/); + } + + expect(clientSecret).toMatch(/^\w{32}$/); + + await expect(page.getByLabel('Name')).toHaveValue(oidcClient.name); + await expect(page.getByTestId('callback-url-1')).toHaveValue(oidcClient.callbackUrl); + await expect(page.getByTestId('callback-url-2')).toHaveValue(oidcClient.secondCallbackUrl); + await expect(page.getByRole('img', { name: `${oidcClient.name} logo` })).toBeVisible(); + + const res = await page.request.get(`/api/oidc/clients/${resolvedClientId}/logo`); + expect(res.ok()).toBeTruthy(); + } + + test('with auto-generated client ID', async ({ page }) => { + await createClientTest(page); + }); + + test('with custom client ID', async ({ page }) => { + await createClientTest(page, '123e4567-e89b-12d3-a456-426614174000'); + }); }); test('Edit OIDC client', async ({ page }) => {