From 7cb2f27cb1d93775cd9eb2e93af99ba68ca401d5 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 11 Aug 2026 00:31:06 +0000 Subject: [PATCH] Address review feedback on client secrets Always serialize the "secrets" field in the credentials DTO, as an empty list when the client has no secrets, and make it a required property in the frontend type so consumers never have to handle a missing value. The federated credentials card now hands its callback only the federated identities it owns, and the page merges them into the client credentials, so saving them no longer drops the secrets from the local client object. Also drop the field comments on OidcClientSecret that restated the code, keeping only the note about the prefix being empty for migrated secrets. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Gcdcb75uaMU5UE1G2niL3L --- backend/internal/dto/oidc_dto.go | 3 ++- backend/internal/dto/oidc_dto_test.go | 16 ++++++++++++++++ backend/internal/model/oidc.go | 10 +++------- frontend/src/lib/types/oidc.type.ts | 2 +- .../admin/oidc-clients/[id]/+page.svelte | 5 ++++- ...oidc-client-federated-credentials-card.svelte | 6 +++--- .../admin/oidc-clients/oidc-client-form.svelte | 2 +- 7 files changed, 30 insertions(+), 14 deletions(-) diff --git a/backend/internal/dto/oidc_dto.go b/backend/internal/dto/oidc_dto.go index 3570fbf6..8fa983f2 100644 --- a/backend/internal/dto/oidc_dto.go +++ b/backend/internal/dto/oidc_dto.go @@ -91,7 +91,8 @@ type OidcClientSecretCreatedDto struct { type OidcClientCredentialsDto struct { FederatedIdentities []OidcClientFederatedIdentityDto `json:"federatedIdentities,omitempty"` // Secrets is read-only: secrets are managed through the dedicated client secret endpoints and any value sent by a client is ignored - Secrets []OidcClientSecretDto `json:"secrets,omitempty"` + // It is always serialized, as an empty list when the client has no secrets, so clients don't need to handle a missing value + Secrets []OidcClientSecretDto `json:"secrets"` } type OidcClientFederatedIdentityDto struct { diff --git a/backend/internal/dto/oidc_dto_test.go b/backend/internal/dto/oidc_dto_test.go index 52d79354..2e9596bc 100644 --- a/backend/internal/dto/oidc_dto_test.go +++ b/backend/internal/dto/oidc_dto_test.go @@ -106,3 +106,19 @@ func TestOidcClientDto_secrets(t *testing.T) { assert.NotContains(t, string(serialized), "hash-1") assert.Contains(t, string(serialized), `"isActive":true`) } + +func TestOidcClientDto_secretsAlwaysSerialized(t *testing.T) { + client := model.OidcClient{ + Base: model.Base{ID: "client-id"}, + Name: "Test Client", + } + + var clientDto OidcClientDto + require.NoError(t, MapStruct(client, &clientDto)) + assert.Empty(t, clientDto.Credentials.Secrets) + + // A client without secrets must serialize an empty list rather than omitting the field, so consumers never have to handle a missing value + serialized, err := json.Marshal(clientDto) + require.NoError(t, err) + assert.Contains(t, string(serialized), `"secrets":[]`) +} diff --git a/backend/internal/model/oidc.go b/backend/internal/model/oidc.go index ae674fa2..0024c1ed 100644 --- a/backend/internal/model/oidc.go +++ b/backend/internal/model/oidc.go @@ -112,14 +112,10 @@ const ( // OidcClientSecret is a single client secret of an OIDC client, stored hashed in the credentials JSON document type OidcClientSecret struct { - // ID uniquely identifies the secret within the client, so it can be deleted without knowing its value - ID string `json:"id"` - // Algorithm used to compute Hash + ID string `json:"id"` Algorithm OidcClientSecretHashAlgorithm `json:"alg"` - // Hash of the secret's value, hex-encoded for SHA-256 secrets and in modular crypt format for bcrypt ones - Hash string `json:"hash"` - // Prefix contains the first few characters of the secret in clear text, so admins can match a secret to the app that uses it - // It is empty for secrets migrated from the single-secret column, whose value was never stored + Hash string `json:"hash"` + // Prefix is empty for secrets migrated from the single-secret column, whose value was never stored Prefix string `json:"prefix,omitempty"` CreatedAt datatype.DateTime `json:"createdAt"` ExpiresAt *datatype.DateTime `json:"expiresAt,omitempty"` diff --git a/frontend/src/lib/types/oidc.type.ts b/frontend/src/lib/types/oidc.type.ts index 1ea4d23f..4df0dacc 100644 --- a/frontend/src/lib/types/oidc.type.ts +++ b/frontend/src/lib/types/oidc.type.ts @@ -37,7 +37,7 @@ export type OidcClientSecretCreated = OidcClientSecret & { export type OidcClientCredentials = { federatedIdentities: OidcClientFederatedIdentity[]; - secrets?: OidcClientSecret[]; + secrets: OidcClientSecret[]; }; export type OidcDiscoveryConfiguration = { 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 49235e64..cc561938 100644 --- a/frontend/src/routes/settings/admin/oidc-clients/[id]/+page.svelte +++ b/frontend/src/routes/settings/admin/oidc-clients/[id]/+page.svelte @@ -16,6 +16,7 @@ import type { OidcClientCreateWithLogo, OidcClientCredentials, + OidcClientFederatedIdentity, OidcClientSecret, OidcClientTokenLifetimes } from '$lib/types/oidc.type'; @@ -125,7 +126,9 @@ return success; } - async function updateFederatedCredentials(credentials: OidcClientCredentials) { + async function updateFederatedCredentials(federatedIdentities: OidcClientFederatedIdentity[]) { + // Secrets are read-only in this request, but they are carried over so the client object keeps matching what the server has + const credentials: OidcClientCredentials = { federatedIdentities, secrets: clientSecrets }; const success = await updateClient({ ...client, credentials }); if (success) { client.credentials = credentials; diff --git a/frontend/src/routes/settings/admin/oidc-clients/[id]/oidc-client-federated-credentials-card.svelte b/frontend/src/routes/settings/admin/oidc-clients/[id]/oidc-client-federated-credentials-card.svelte index 3a8175c3..a968569b 100644 --- a/frontend/src/routes/settings/admin/oidc-clients/[id]/oidc-client-federated-credentials-card.svelte +++ b/frontend/src/routes/settings/admin/oidc-clients/[id]/oidc-client-federated-credentials-card.svelte @@ -2,7 +2,7 @@ import { Button } from '$lib/components/ui/button'; import * as Card from '$lib/components/ui/card'; import { m } from '$lib/paraglide/messages'; - import type { OidcClient, OidcClientCredentials } from '$lib/types/oidc.type'; + import type { OidcClient, OidcClientFederatedIdentity } from '$lib/types/oidc.type'; import { preventDefault } from '$lib/utils/event-util'; import { createForm } from '$lib/utils/form-util'; import { slide } from 'svelte/transition'; @@ -14,7 +14,7 @@ callback }: { client: OidcClient; - callback: (credentials: OidcClientCredentials) => Promise; + callback: (federatedIdentities: OidcClientFederatedIdentity[]) => Promise; } = $props(); let isLoading = $state(false); @@ -72,7 +72,7 @@ if (!data) return; isLoading = true; - await callback(data.credentials).finally(() => (isLoading = false)); + await callback(data.credentials.federatedIdentities).finally(() => (isLoading = false)); } diff --git a/frontend/src/routes/settings/admin/oidc-clients/oidc-client-form.svelte b/frontend/src/routes/settings/admin/oidc-clients/oidc-client-form.svelte index 16a1196c..22637788 100644 --- a/frontend/src/routes/settings/admin/oidc-clients/oidc-client-form.svelte +++ b/frontend/src/routes/settings/admin/oidc-clients/oidc-client-form.svelte @@ -109,7 +109,7 @@ const success = await callback({ ...data, - credentials: existingClient?.credentials ?? { federatedIdentities: [] }, + credentials: existingClient?.credentials ?? { federatedIdentities: [], secrets: [] }, logo: $inputs.logoUrl?.value ? undefined : logo, logoUrl: $inputs.logoUrl?.value, darkLogo: $inputs.darkLogoUrl?.value ? undefined : darkLogo,