feat: restore old behavior of automatically creating client secret

This commit is contained in:
Elias Schneider
2026-09-23 22:00:43 +02:00
parent 2075de3234
commit d259a15131
19 changed files with 240 additions and 40 deletions
+4 -2
View File
@@ -66,7 +66,8 @@ type AppConfigModel struct {
WebauthnAllowSyncedPasskeys AppConfigValue `json:"webauthnAllowSyncedPasskeys" env:"WEBAUTHN_ALLOW_SYNCED_PASSKEYS" type:"bool"`
WebauthnAuthenticatorAttachment AppConfigValue `json:"webauthnAuthenticatorAttachment" env:"WEBAUTHN_AUTHENTICATOR_ATTACHMENT"`
// OIDC
CIMDURLAllowlist AppConfigValue `json:"cimdUrlAllowlist" env:"CIMD_URL_ALLOWLIST"` // JSON-encoded array of strings
CIMDURLAllowlist AppConfigValue `json:"cimdUrlAllowlist" env:"CIMD_URL_ALLOWLIST"` // JSON-encoded array of strings
AutoCreateOIDCClientSecret AppConfigValue `json:"autoCreateOidcClientSecret" env:"AUTO_CREATE_OIDC_CLIENT_SECRET" type:"bool"`
}
// appConfigEnvName returns the explicit environment variable name for a JSON configuration field
@@ -171,7 +172,8 @@ func getDefaultConfig() *AppConfigModel {
WebauthnAllowSyncedPasskeys: "true",
WebauthnAuthenticatorAttachment: "any",
// OIDC
CIMDURLAllowlist: "[]",
CIMDURLAllowlist: "[]",
AutoCreateOIDCClientSecret: "true",
}
}
@@ -167,7 +167,7 @@ func registerRoutes(r *gin.Engine, db *gorm.DB, svc *services, rateLimitServices
rateLimitMiddleware.Add(middleware.RateLimitDeviceLoginExchange),
rateLimitMiddleware.Add(middleware.RateLimitDeviceLoginVerification),
)
controller.NewOidcController(apiGroup, authMiddleware, fileSizeLimitMiddleware, svc.oidcService)
controller.NewOidcController(apiGroup, authMiddleware, fileSizeLimitMiddleware, svc.oidcService, svc.appConfigService)
controller.NewUserController(apiGroup, authMiddleware, svc.appConfigService, svc.userService, svc.webauthnModule)
controller.NewAppConfigController(apiGroup, authMiddleware, svc.appConfigService, svc.emailModule)
svc.ldapSyncModule.RegisterRoutes(apiGroup, authMiddleware.Add())
+22 -6
View File
@@ -8,6 +8,7 @@ import (
"github.com/gin-gonic/gin"
"github.com/pocket-id/pocket-id/backend/internal/appconfig"
"github.com/pocket-id/pocket-id/backend/internal/apperror"
"github.com/pocket-id/pocket-id/backend/internal/dto"
"github.com/pocket-id/pocket-id/backend/internal/httpserver"
@@ -20,9 +21,10 @@ import (
// @Summary OIDC controller
// @Description Initializes all OIDC-related API endpoints for authentication and client management
// @Tags OIDC
func NewOidcController(group *gin.RouterGroup, authMiddleware *middleware.AuthMiddleware, fileSizeLimitMiddleware *middleware.FileSizeLimitMiddleware, oidcService *service.OidcService) {
func NewOidcController(group *gin.RouterGroup, authMiddleware *middleware.AuthMiddleware, fileSizeLimitMiddleware *middleware.FileSizeLimitMiddleware, oidcService *service.OidcService, appConfigService appconfig.AppConfigResolver) {
oc := &OidcController{
oidcService: oidcService,
oidcService: oidcService,
appConfigService: appConfigService,
}
group.GET("/oidc/clients", authMiddleware.Add(), httpserver.Handle(oc.listClientsHandler))
@@ -54,7 +56,8 @@ func NewOidcController(group *gin.RouterGroup, authMiddleware *middleware.AuthMi
}
type OidcController struct {
oidcService *service.OidcService
oidcService *service.OidcService
appConfigService appconfig.AppConfigResolver
}
// getClientMetaDataHandler godoc
@@ -155,7 +158,7 @@ func (oc *OidcController) listClientsHandler(c *gin.Context) error {
// @Accept json
// @Produce json
// @Param client body dto.OidcClientCreateDto true "Client information"
// @Success 201 {object} dto.OidcClientWithAllowedUserGroupsDto "Created client"
// @Success 201 {object} dto.OidcClientCreatedDto "Created client"
// @Failure default {object} dto.ErrorDto "Error"
// @Router /api/oidc/clients [post]
func (oc *OidcController) createClientHandler(c *gin.Context) error {
@@ -165,16 +168,29 @@ func (oc *OidcController) createClientHandler(c *gin.Context) error {
return err
}
client, err := oc.oidcService.CreateClient(c.Request.Context(), input, c.GetString("userID"))
config, err := oc.appConfigService.GetConfig(c.Request.Context())
if err != nil {
return err
}
var clientDto dto.OidcClientWithAllowedUserGroupsDto
client, createdSecret, err := oc.oidcService.CreateClient(c.Request.Context(), input, c.GetString("userID"), config.AutoCreateOIDCClientSecret.IsTrue())
if err != nil {
return err
}
var clientDto dto.OidcClientCreatedDto
err = dto.MapStruct(client, &clientDto)
if err != nil {
return err
}
if createdSecret != "" {
var secretDto dto.OidcClientSecretCreatedDto
if err := dto.MapStruct(client.Credentials.Secrets[0], &secretDto); err != nil {
return err
}
secretDto.Secret = createdSecret
clientDto.CreatedSecret = &secretDto
}
c.JSON(http.StatusCreated, clientDto)
return nil
+1
View File
@@ -61,6 +61,7 @@ type AppConfigUpdateDto struct {
EmailApiKeyExpirationEnabled string `json:"emailApiKeyExpirationEnabled" binding:"required,boolean_string"`
EmailVerificationEnabled string `json:"emailVerificationEnabled" binding:"required,boolean_string"`
CIMDURLAllowlist string `json:"cimdUrlAllowlist" binding:"omitempty,cimd_url_allowlist"`
AutoCreateOIDCClientSecret string `json:"autoCreateOidcClientSecret" binding:"required,boolean_string"`
}
func (a AppConfigUpdateDto) Validate() error {
+6
View File
@@ -38,6 +38,12 @@ type OidcClientWithAllowedUserGroupsDto struct {
AllowedUserGroups []UserGroupMinimalDto `json:"allowedUserGroups"`
}
// OidcClientCreatedDto reveals the automatically generated secret only in the create response
type OidcClientCreatedDto struct {
OidcClientWithAllowedUserGroupsDto
CreatedSecret *OidcClientSecretCreatedDto `json:"createdSecret,omitempty"`
}
type OidcClientWithAllowedGroupsDto struct {
OidcClientDto
AllowedUserGroups []UserGroupMinimalDto `json:"allowedUserGroups"`
@@ -229,6 +229,7 @@ func TestValidationResponseUsesAppConfigTypeMessages(t *testing.T) {
EmailLoginNotificationEnabled: "false",
EmailApiKeyExpirationEnabled: "false",
EmailVerificationEnabled: "false",
AutoCreateOIDCClientSecret: "true",
}
payload, err := json.Marshal(input)
require.NoError(t, err)
+43 -24
View File
@@ -152,7 +152,7 @@ func (s *OidcService) ListClients(ctx context.Context, name string, listRequestO
return clients, response, err
}
func (s *OidcService) CreateClient(ctx context.Context, input dto.OidcClientCreateDto, userID string) (model.OidcClient, error) {
func (s *OidcService) CreateClient(ctx context.Context, input dto.OidcClientCreateDto, userID string, autoCreateSecret bool) (model.OidcClient, string, error) {
client := model.OidcClient{
Base: model.Base{
ID: input.ID,
@@ -161,7 +161,18 @@ func (s *OidcService) CreateClient(ctx context.Context, input dto.OidcClientCrea
}
err := updateOIDCClientModelFromDto(&client, &input.OidcClientUpdateDto)
if err != nil {
return model.OidcClient{}, err
return model.OidcClient{}, "", err
}
// Generate the initial credential before saving so a failed generation cannot leave a client without its expected secret
var createdSecret string
if autoCreateSecret && !client.IsPublic {
secret, value, err := newOIDCClientSecret(dto.OidcClientSecretCreateDto{})
if err != nil {
return model.OidcClient{}, "", err
}
client.Credentials.Secrets = append(client.Credentials.Secrets, secret)
createdSecret = value
}
err = s.db.
@@ -170,27 +181,27 @@ func (s *OidcService) CreateClient(ctx context.Context, input dto.OidcClientCrea
Error
if err != nil {
if errors.Is(err, gorm.ErrDuplicatedKey) {
return model.OidcClient{}, apperror.ClientIDAlreadyExists()
return model.OidcClient{}, "", apperror.ClientIDAlreadyExists()
}
return model.OidcClient{}, err
return model.OidcClient{}, "", err
}
// All storage operations must be executed outside of a transaction
if input.LogoURL != nil {
err = s.downloadAndSaveLogoFromURL(ctx, client.ID, *input.LogoURL, true)
if err != nil {
return model.OidcClient{}, fmt.Errorf("failed to download logo: %w", err)
return model.OidcClient{}, "", fmt.Errorf("failed to download logo: %w", err)
}
}
if input.DarkLogoURL != nil {
err = s.downloadAndSaveLogoFromURL(ctx, client.ID, *input.DarkLogoURL, false)
if err != nil {
return model.OidcClient{}, fmt.Errorf("failed to download dark logo: %w", err)
return model.OidcClient{}, "", fmt.Errorf("failed to download dark logo: %w", err)
}
}
return client, nil
return client, createdSecret, nil
}
func (s *OidcService) UpdateClient(ctx context.Context, clientID string, input dto.OidcClientUpdateDto) (model.OidcClient, error) {
@@ -412,23 +423,9 @@ func (s *OidcService) CreateClientSecret(ctx context.Context, clientID string, i
return model.OidcClientSecret{}, "", apperror.ValidationMessage(fmt.Sprintf("A client cannot have more than %d secrets", model.MaxOidcClientSecrets))
}
// Callers may supply their own value, otherwise one with enough entropy is generated here
clientSecret := input.Secret
if clientSecret == "" {
clientSecret, err = utils.GenerateRandomAlphanumericString(32)
if err != nil {
return model.OidcClientSecret{}, "", fmt.Errorf("failed to generate client secret: %w", err)
}
}
// Only the hash and a short prefix are persisted, so this is the last time the value is available
secret := model.OidcClientSecret{
ID: uuid.NewV4().String(),
Algorithm: model.OidcClientSecretHashSHA256,
Hash: utils.CreateSha256Hash(clientSecret),
Prefix: clientSecretPrefix(clientSecret),
CreatedAt: datatype.DateTime(time.Now()),
ExpiresAt: input.ExpiresAt,
secret, clientSecret, err := newOIDCClientSecret(input)
if err != nil {
return model.OidcClientSecret{}, "", err
}
client.Credentials.Secrets = append(client.Credentials.Secrets, secret)
@@ -450,6 +447,28 @@ func (s *OidcService) CreateClientSecret(ctx context.Context, clientID string, i
return secret, clientSecret, nil
}
// newOIDCClientSecret keeps the generated value transient while persisting only its hash and prefix
func newOIDCClientSecret(input dto.OidcClientSecretCreateDto) (model.OidcClientSecret, string, error) {
clientSecret := input.Secret
if clientSecret == "" {
var err error
clientSecret, err = utils.GenerateRandomAlphanumericString(32)
if err != nil {
return model.OidcClientSecret{}, "", fmt.Errorf("failed to generate client secret: %w", err)
}
}
secret := model.OidcClientSecret{
ID: uuid.NewV4().String(),
Algorithm: model.OidcClientSecretHashSHA256,
Hash: utils.CreateSha256Hash(clientSecret),
Prefix: clientSecretPrefix(clientSecret),
CreatedAt: datatype.DateTime(time.Now()),
ExpiresAt: input.ExpiresAt,
}
return secret, clientSecret, nil
}
// DeleteClientSecret removes a single secret from a client, making it immediately unusable
func (s *OidcService) DeleteClientSecret(ctx context.Context, clientID string, secretID string) error {
tx := s.db.Begin()
+45 -3
View File
@@ -533,7 +533,7 @@ func TestOidcService_CreateClient_withDescription(t *testing.T) {
},
}
client, err := s.CreateClient(t.Context(), input, "user-id")
client, _, err := s.CreateClient(t.Context(), input, "user-id", true)
require.NoError(t, err)
var fetched model.OidcClient
@@ -556,7 +556,7 @@ func TestOidcService_CreateClient_withoutDescription(t *testing.T) {
},
}
client, err := s.CreateClient(t.Context(), input, "user-id")
client, _, err := s.CreateClient(t.Context(), input, "user-id", true)
require.NoError(t, err)
var fetched model.OidcClient
@@ -565,6 +565,48 @@ func TestOidcService_CreateClient_withoutDescription(t *testing.T) {
assert.Empty(t, fetched.Description)
}
func TestOidcService_CreateClient_initialSecret(t *testing.T) {
for _, test := range []struct {
name string
isPublic bool
autoCreateSecret bool
wantSecret bool
}{
{name: "confidential client gets a secret", autoCreateSecret: true, wantSecret: true},
{name: "automatic creation disabled", autoCreateSecret: false},
{name: "public client never gets a secret", isPublic: true, autoCreateSecret: true},
} {
t.Run(test.name, func(t *testing.T) {
db := testutils.NewDatabaseForTest(t)
s := &OidcService{db: db}
input := dto.OidcClientCreateDto{
OidcClientUpdateDto: dto.OidcClientUpdateDto{
Name: "Test Client",
IsPublic: test.isPublic,
},
}
client, value, err := s.CreateClient(t.Context(), input, "user-id", test.autoCreateSecret)
require.NoError(t, err)
var fetched model.OidcClient
require.NoError(t, db.First(&fetched, "id = ?", client.ID).Error)
if !test.wantSecret {
assert.Empty(t, value)
assert.Empty(t, fetched.Credentials.Secrets)
return
}
require.Len(t, fetched.Credentials.Secrets, 1)
require.Len(t, client.Credentials.Secrets, 1)
assert.Len(t, value, 32)
assert.Equal(t, utils.CreateSha256Hash(value), fetched.Credentials.Secrets[0].Hash)
assert.Equal(t, value[:model.OidcClientSecretPrefixLength], fetched.Credentials.Secrets[0].Prefix)
assert.Nil(t, fetched.Credentials.Secrets[0].ExpiresAt)
})
}
}
func TestOidcService_CreateClient_tokenLifetimes(t *testing.T) {
for _, test := range []struct {
name string
@@ -607,7 +649,7 @@ func TestOidcService_CreateClient_tokenLifetimes(t *testing.T) {
},
}
client, err := s.CreateClient(t.Context(), input, "user-id")
client, _, err := s.CreateClient(t.Context(), input, "user-id", true)
require.NoError(t, err)
var fetched model.OidcClient
+2
View File
@@ -647,6 +647,8 @@
"credentials": "Credentials",
"client_secrets": "Client secrets",
"client_secrets_description": "Secrets that the app uses to authenticate itself. Multiple secrets can be active at the same time, so one can be rotated without downtime.",
"auto_create_client_secret": "Automatically create client secrets",
"auto_create_client_secret_description": "Create a secret without an expiration date when a confidential OIDC client is added. The secret is shown once after creation.",
"no_client_secrets_yet": "This app has no client secrets yet.",
"public_clients_cannot_have_secrets": "Public clients cannot have client secrets.",
"add_client_secret": "Add client secret",
+2 -1
View File
@@ -7,6 +7,7 @@ import type {
InteractionStep,
OidcClient,
OidcClientCreate,
OidcClientCreated,
OidcClientMetaData,
OidcClientSecret,
OidcClientSecretCreated,
@@ -42,7 +43,7 @@ class OidcService extends APIService {
};
createClient = async (client: OidcClientCreate) =>
(await this.api.post('/oidc/clients', client)).data as OidcClient;
(await this.api.post('/oidc/clients', client)).data as OidcClientCreated;
removeClient = async (id: string) => {
await this.api.delete(`/oidc/clients/${encodeClientIdParam(id)}`);
@@ -3,6 +3,7 @@ import { writable } from 'svelte/store';
// Holds the clear-text value of the client secrets created during the current page visit, keyed by secret ID.
// The server never returns those values again, so they are shown until the user navigates away and then forgotten.
const clientSecretStore = writable<Record<string, string>>({});
export const autoCreatedSecretId = writable<string | null>(null);
const set = (secretId: string, secret: string) => {
clientSecretStore.update((secrets) => ({ ...secrets, [secretId]: secret }));
@@ -18,11 +19,18 @@ const remove = (secretId: string) => {
const clear = () => {
clientSecretStore.set({});
autoCreatedSecretId.set(null);
};
const setAutoCreated = (secretId: string, secret: string) => {
set(secretId, secret);
autoCreatedSecretId.set(secretId);
};
export default {
subscribe: clientSecretStore.subscribe,
set,
setAutoCreated,
remove,
clear
};
@@ -58,6 +58,7 @@ export type AllAppConfig = AppConfig & {
webauthnAuthenticatorAttachment: 'any' | 'platform' | 'cross-platform';
// OIDC
cimdUrlAllowlist: string[];
autoCreateOidcClientSecret: boolean;
};
export type AppConfigRawResponse = {
+4
View File
@@ -68,6 +68,10 @@ export type OidcClient = OidcClientMetaData & {
refreshTokenDurationMinutes: number;
};
export type OidcClientCreated = OidcClient & {
createdSecret?: OidcClientSecretCreated;
};
export type OidcClientTokenLifetimes = Pick<
OidcClient,
'accessTokenDurationMinutes' | 'refreshTokenDurationMinutes'
@@ -8,6 +8,7 @@
import type { AllAppConfig } from '$lib/types/application-configuration.type';
import { LucideInfo } from '@lucide/svelte';
import AppConfigDynamicClientsForm from './forms/app-config-dynamic-clients-form.svelte';
import AppConfigClientSecretsForm from './forms/app-config-client-secrets-form.svelte';
import AppConfigEmailForm from './forms/app-config-email-form.svelte';
import AppConfigGeneralForm from './forms/app-config-general-form.svelte';
import AppConfigLdapForm from './forms/app-config-ldap-form.svelte';
@@ -191,7 +192,16 @@
</Card.Root>
</Tabs.Content>
<Tabs.Content value="oidc" id="application-configuration-oidc">
<Tabs.Content value="oidc" id="application-configuration-oidc" class="flex flex-col gap-4">
<Card.Root>
<Card.Header>
<Card.Title>{m.general()}</Card.Title>
</Card.Header>
<Card.Content>
<AppConfigClientSecretsForm {appConfig} callback={updateAppConfig} />
</Card.Content>
</Card.Root>
<Card.Root>
<Card.Header>
<Card.Title>{m.client_id_metadata_documents()}</Card.Title>
@@ -0,0 +1,34 @@
<script lang="ts">
import SwitchWithLabel from '$lib/components/form/switch-with-label.svelte';
import { m } from '$lib/paraglide/messages';
import appConfigStore from '$lib/stores/application-configuration-store';
import type { AllAppConfig } from '$lib/types/application-configuration.type';
import { createForm } from '$lib/utils/form-util';
import { trackFormChanges } from '$lib/utils/unsaved-changes-util.svelte';
import { z } from 'zod/v4';
let {
appConfig,
callback
}: {
appConfig: AllAppConfig;
callback: (appConfig: Partial<AllAppConfig>) => Promise<void>;
} = $props();
const formSchema = z.object({ autoCreateOidcClientSecret: z.boolean() });
let formStore = $derived(
createForm(formSchema, { autoCreateOidcClientSecret: appConfig.autoCreateOidcClientSecret })
);
let inputs = $derived(formStore.inputs);
trackFormChanges(() => formStore, callback);
</script>
<fieldset disabled={$appConfigStore.uiConfigDisabled}>
<SwitchWithLabel
id="auto-create-oidc-client-secret"
label={m.auto_create_client_secret()}
description={m.auto_create_client_secret_description()}
bind:checked={$inputs.autoCreateOidcClientSecret.value}
/>
</fieldset>
@@ -21,6 +21,12 @@
async function createOIDCClient(client: OidcClientCreateWithLogo) {
clientSecretStore.clear();
const createdClient = await oidcService.createClient(client);
if (createdClient.createdSecret) {
clientSecretStore.setAutoCreated(
createdClient.createdSecret.id,
createdClient.createdSecret.secret
);
}
const logoPromise = client.logo
? oidcService.updateClientLogo(createdClient, client.logo, true)
@@ -30,7 +36,6 @@
: Promise.resolve();
await Promise.all([logoPromise, darkLogoPromise]);
// A new client starts without any secret: the admin creates the ones they need from the credentials tab
goto(`/settings/admin/oidc-clients/${encodeClientIdParam(createdClient.id)}`);
toast.success(m.oidc_client_created_successfully());
}
@@ -11,7 +11,7 @@
import { m } from '$lib/paraglide/messages';
import OidcService from '$lib/services/oidc-service';
import ScimService from '$lib/services/scim-service';
import clientSecretStore from '$lib/stores/client-secret-store';
import clientSecretStore, { autoCreatedSecretId } from '$lib/stores/client-secret-store';
import type {
OidcClientCreateWithLogo,
OidcClientCredentials,
@@ -251,6 +251,16 @@
</span>
</CopyToClipboard>
</div>
{#if $autoCreatedSecretId && clientSecrets.some((secret) => secret.id === $autoCreatedSecretId) && $clientSecretStore[$autoCreatedSecretId]}
<div class="mb-2 flex flex-col sm:flex-row sm:items-center">
<Field.Label class="w-52">{m.client_secret()}</Field.Label>
<CopyToClipboard value={$clientSecretStore[$autoCreatedSecretId]}>
<span class="text-muted-foreground text-sm break-all" data-testid="client-secret">
{$clientSecretStore[$autoCreatedSecretId]}
</span>
</CopyToClipboard>
</div>
{/if}
{#if showAllDetails}
<div transition:slide>
{#each Object.entries(setupDetails) as [key, value] (key)}
@@ -25,6 +25,28 @@ test('Update general configuration', async ({ page }) => {
await page.waitForURL('/settings/apps');
});
test('Disable automatic client secret creation', async ({ page }) => {
await page.getByRole('tab', { name: 'OIDC' }).click();
const autoCreateSecret = page.getByRole('switch', {
name: 'Automatically create client secrets'
});
await expect(autoCreateSecret).toBeChecked();
await autoCreateSecret.click();
await saveUnsavedChanges(page);
await page.reload();
await page.getByRole('tab', { name: 'OIDC' }).click();
await expect(autoCreateSecret).not.toBeChecked();
const response = await page.request.post('/api/oidc/clients', {
data: { name: 'No Automatic Secret' }
});
expect(response.status()).toBe(201);
const createdClient = await response.json();
expect(createdClient.createdSecret).toBeUndefined();
expect(createdClient.credentials.secrets ?? []).toHaveLength(0);
});
test('Save configuration from every editable tab together', async ({ page }) => {
await page.getByLabel('Application Name', { exact: true }).fill('Combined Settings');
await page.getByRole('tab', { name: 'User Creation' }).click();
+16
View File
@@ -40,6 +40,13 @@ test.describe('Create OIDC client', () => {
);
const resolvedClientId = (await page.getByTestId('client-id').innerText()).trim();
const createdSecret = (
await page
.getByRole('tabpanel', { name: 'General', exact: true })
.getByTestId('client-secret')
.innerText()
).trim();
expect(createdSecret).toMatch(/^\w{32}$/);
if (clientId) {
expect(resolvedClientId).toBe(clientId);
@@ -55,6 +62,15 @@ test.describe('Create OIDC client', () => {
const res = await page.request.get(`/api/oidc/clients/${resolvedClientId}/logo`);
expect(res.ok()).toBeTruthy();
// The generated value is available on the creation page and is forgotten after a reload
await page.reload();
await expect(page.getByText(createdSecret, { exact: true })).toHaveCount(0);
await page.getByRole('tab', { name: 'Credentials' }).click();
await expect(page.getByTestId('client-secret-row')).toHaveCount(1);
await expect(page.getByTestId('client-secret')).toHaveText(
`${createdSecret.slice(0, 4)}••••••••`
);
}
test('with auto-generated client ID', async ({ page }) => {