From 155a1fcba09fa09262af24d712f8da39d16836e6 Mon Sep 17 00:00:00 2001 From: "Alessandro (Ale) Segala" <43508+ItalyPaleAle@users.noreply.github.com> Date: Mon, 10 Aug 2026 17:54:52 -0700 Subject: [PATCH] feat: support multiple client secrets per OIDC client (#1679) --- .../internal/controller/oidc_controller.go | 99 +++++++-- backend/internal/dto/oidc_dto.go | 22 ++ backend/internal/dto/oidc_dto_test.go | 52 +++++ backend/internal/model/oidc.go | 68 +++++- backend/internal/oidc/client.go | 22 +- backend/internal/oidc/client_secret_hasher.go | 49 +++++ .../oidc/client_secret_hasher_test.go | 99 +++++++++ backend/internal/oidc/client_test.go | 60 ++++++ backend/internal/oidc/provider.go | 1 + backend/internal/oidc/token_handler_test.go | 157 ++++++++++++-- backend/internal/service/e2etest_service.go | 34 ++- .../oidc_client_secrets_migration_test.go | 88 ++++++++ backend/internal/service/oidc_service.go | 102 +++++++-- backend/internal/service/oidc_service_test.go | 123 ++++++++++- ...0000_oidc_client_multiple_secrets.down.sql | 16 ++ ...120000_oidc_client_multiple_secrets.up.sql | 14 ++ ...0000_oidc_client_multiple_secrets.down.sql | 28 +++ ...120000_oidc_client_multiple_secrets.up.sql | 31 +++ frontend/messages/en.json | 16 +- frontend/src/lib/services/oidc-service.ts | 18 +- .../src/lib/stores/client-secret-store.ts | 19 +- frontend/src/lib/types/oidc.type.ts | 15 ++ .../settings/admin/oidc-clients/+page.svelte | 5 +- .../admin/oidc-clients/[id]/+page.svelte | 68 ++---- ...c-client-federated-credentials-card.svelte | 6 +- .../[id]/oidc-client-secrets-card.svelte | 195 ++++++++++++++++++ .../oidc-clients/oidc-client-form.svelte | 2 +- tests/resources/export/database.json | 24 +-- tests/specs/oidc-client-settings.spec.ts | 85 +++++++- 29 files changed, 1353 insertions(+), 165 deletions(-) create mode 100644 backend/internal/oidc/client_secret_hasher.go create mode 100644 backend/internal/oidc/client_secret_hasher_test.go create mode 100644 backend/internal/service/oidc_client_secrets_migration_test.go create mode 100644 backend/resources/migrations/postgres/20260807120000_oidc_client_multiple_secrets.down.sql create mode 100644 backend/resources/migrations/postgres/20260807120000_oidc_client_multiple_secrets.up.sql create mode 100644 backend/resources/migrations/sqlite/20260807120000_oidc_client_multiple_secrets.down.sql create mode 100644 backend/resources/migrations/sqlite/20260807120000_oidc_client_multiple_secrets.up.sql create mode 100644 frontend/src/routes/settings/admin/oidc-clients/[id]/oidc-client-secrets-card.svelte diff --git a/backend/internal/controller/oidc_controller.go b/backend/internal/controller/oidc_controller.go index 66a25569..ef9b06ae 100644 --- a/backend/internal/controller/oidc_controller.go +++ b/backend/internal/controller/oidc_controller.go @@ -34,7 +34,9 @@ func NewOidcController(group *gin.RouterGroup, authMiddleware *middleware.AuthMi group.DELETE("/oidc/clients/:id", authMiddleware.Add(), httpserver.Handle(oc.deleteClientHandler)) group.PUT("/oidc/clients/:id/allowed-user-groups", authMiddleware.Add(), httpserver.Handle(oc.updateAllowedUserGroupsHandler)) - group.POST("/oidc/clients/:id/secret", authMiddleware.Add(), httpserver.Handle(oc.createClientSecretHandler)) + group.GET("/oidc/clients/:id/secrets", authMiddleware.Add(), httpserver.Handle(oc.listClientSecretsHandler)) + group.POST("/oidc/clients/:id/secrets", authMiddleware.Add(), httpserver.Handle(oc.createClientSecretHandler)) + group.DELETE("/oidc/clients/:id/secrets/:secretId", authMiddleware.Add(), httpserver.Handle(oc.deleteClientSecretHandler)) group.GET("/oidc/clients/:id/logo", httpserver.Handle(oc.getClientLogoHandler)) group.DELETE("/oidc/clients/:id/logo", authMiddleware.Add(), httpserver.Handle(oc.deleteClientLogoHandler)) @@ -161,7 +163,8 @@ func (oc *OidcController) listClientsHandler(c *gin.Context) error { // @Router /api/oidc/clients [post] func (oc *OidcController) createClientHandler(c *gin.Context) error { var input dto.OidcClientCreateDto - if err := httpserver.BindJSON(c, &input); err != nil { + err := httpserver.BindJSON(c, &input) + if err != nil { return err } @@ -171,7 +174,8 @@ func (oc *OidcController) createClientHandler(c *gin.Context) error { } var clientDto dto.OidcClientWithAllowedUserGroupsDto - if err := dto.MapStruct(client, &clientDto); err != nil { + err = dto.MapStruct(client, &clientDto) + if err != nil { return err } @@ -208,7 +212,8 @@ func (oc *OidcController) deleteClientHandler(c *gin.Context) error { // @Router /api/oidc/clients/{id} [put] func (oc *OidcController) updateClientHandler(c *gin.Context) error { var input dto.OidcClientUpdateDto - if err := httpserver.BindJSON(c, &input); err != nil { + err := httpserver.BindJSON(c, &input) + if err != nil { return err } @@ -218,7 +223,8 @@ func (oc *OidcController) updateClientHandler(c *gin.Context) error { } var clientDto dto.OidcClientWithAllowedUserGroupsDto - if err := dto.MapStruct(client, &clientDto); err != nil { + err = dto.MapStruct(client, &clientDto) + if err != nil { return err } @@ -241,7 +247,8 @@ func (oc *OidcController) refreshClientMetadataHandler(c *gin.Context) error { } var clientDto dto.OidcClientWithAllowedUserGroupsDto - if err := dto.MapStruct(client, &clientDto); err != nil { + err = dto.MapStruct(client, &clientDto) + if err != nil { return err } @@ -250,28 +257,78 @@ func (oc *OidcController) refreshClientMetadataHandler(c *gin.Context) error { return nil } -// createClientSecretHandler godoc -// @Summary Create client secret -// @Description Set or generate a new secret for an OIDC client +// listClientSecretsHandler godoc +// @Summary List client secrets +// @Description List the secrets of an OIDC client, without disclosing their values // @Tags OIDC -// @Accept json // @Produce json // @Param id path string true "Client ID" -// @Param payload body dto.OidcClientSecretDto false "Client secret" -// @Success 200 {object} object "{ \"secret\": \"string\" }" -// @Router /api/oidc/clients/{id}/secret [post] -func (oc *OidcController) createClientSecretHandler(c *gin.Context) error { - var input dto.OidcClientSecretDto - if err := httpserver.BindOptionalJSON(c, &input); err != nil { - return err - } - - secret, err := oc.oidcService.CreateClientSecret(c.Request.Context(), c.Param("id"), input) +// @Success 200 {array} dto.OidcClientSecretDto "Client secrets" +// @Router /api/oidc/clients/{id}/secrets [get] +func (oc *OidcController) listClientSecretsHandler(c *gin.Context) error { + secrets, err := oc.oidcService.ListClientSecrets(c.Request.Context(), c.Param("id")) if err != nil { return err } - c.JSON(http.StatusOK, gin.H{"secret": secret}) + var secretsDto []dto.OidcClientSecretDto + err = dto.MapStructList(secrets, &secretsDto) + if err != nil { + return err + } + + c.JSON(http.StatusOK, secretsDto) + return nil +} + +// createClientSecretHandler godoc +// @Summary Create client secret +// @Description Add a new secret to an OIDC client, leaving the existing ones usable. The value is only returned by this endpoint and cannot be retrieved later. +// @Tags OIDC +// @Accept json +// @Produce json +// @Param id path string true "Client ID" +// @Param payload body dto.OidcClientSecretCreateDto false "Client secret" +// @Success 201 {object} dto.OidcClientSecretCreatedDto "Created client secret" +// @Router /api/oidc/clients/{id}/secrets [post] +func (oc *OidcController) createClientSecretHandler(c *gin.Context) error { + var input dto.OidcClientSecretCreateDto + err := httpserver.BindOptionalJSON(c, &input) + if err != nil { + return err + } + + created, secret, err := oc.oidcService.CreateClientSecret(c.Request.Context(), c.Param("id"), input) + if err != nil { + return err + } + + var secretDto dto.OidcClientSecretCreatedDto + err = dto.MapStruct(created, &secretDto) + if err != nil { + return err + } + secretDto.Secret = secret + + c.JSON(http.StatusCreated, secretDto) + return nil +} + +// deleteClientSecretHandler godoc +// @Summary Delete client secret +// @Description Delete a single secret of an OIDC client, making it immediately unusable +// @Tags OIDC +// @Param id path string true "Client ID" +// @Param secretId path string true "Client secret ID" +// @Success 204 "No content" +// @Router /api/oidc/clients/{id}/secrets/{secretId} [delete] +func (oc *OidcController) deleteClientSecretHandler(c *gin.Context) error { + err := oc.oidcService.DeleteClientSecret(c.Request.Context(), c.Param("id"), c.Param("secretId")) + if err != nil { + return err + } + + c.Status(http.StatusNoContent) return nil } diff --git a/backend/internal/dto/oidc_dto.go b/backend/internal/dto/oidc_dto.go index 53249868..9ad737eb 100644 --- a/backend/internal/dto/oidc_dto.go +++ b/backend/internal/dto/oidc_dto.go @@ -64,12 +64,34 @@ type OidcClientCreateDto struct { ID string `json:"id" binding:"omitempty,client_id,min=2,max=128"` } +// OidcClientSecretDto describes a client secret without disclosing its value, which is only ever returned right after the secret is created type OidcClientSecretDto struct { + ID string `json:"id"` + // Prefix holds the first few characters of the secret in clear text, and is empty for secrets migrated from the single-secret column + Prefix string `json:"prefix"` + CreatedAt datatype.DateTime `json:"createdAt"` + ExpiresAt *datatype.DateTime `json:"expiresAt"` + IsActive bool `json:"isActive"` +} + +// OidcClientSecretCreateDto is the request body for creating a new client secret +type OidcClientSecretCreateDto struct { + // Secret allows callers to supply their own value instead of having Pocket ID generate one Secret string `json:"secret" binding:"omitempty,min=16,printascii"` + // ExpiresAt makes the secret unusable after the given time (if nil, secrets don't expire) + ExpiresAt *datatype.DateTime `json:"expiresAt"` +} + +// OidcClientSecretCreatedDto is returned when a secret is created, and is the only response that contains the secret's value +type OidcClientSecretCreatedDto struct { + OidcClientSecretDto + Secret string `json:"secret"` } 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"` } type OidcClientFederatedIdentityDto struct { diff --git a/backend/internal/dto/oidc_dto_test.go b/backend/internal/dto/oidc_dto_test.go index 4d257f5c..2e9596bc 100644 --- a/backend/internal/dto/oidc_dto_test.go +++ b/backend/internal/dto/oidc_dto_test.go @@ -3,10 +3,14 @@ package dto import ( "encoding/json" "testing" + "time" "github.com/gin-gonic/gin/binding" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + + "github.com/pocket-id/pocket-id/backend/internal/model" + datatype "github.com/pocket-id/pocket-id/backend/internal/model/types" ) func TestOidcClientUpdateDto_tokenLifetimes(t *testing.T) { @@ -70,3 +74,51 @@ func TestOidcClientUpdateDto_tokenLifetimes(t *testing.T) { }) } } + +func TestOidcClientDto_secrets(t *testing.T) { + expired := datatype.DateTime(time.Now().Add(-time.Hour)) + valid := datatype.DateTime(time.Now().Add(time.Hour)) + client := model.OidcClient{ + Base: model.Base{ID: "client-id"}, + Name: "Test Client", + Credentials: model.OidcClientCredentials{ + Secrets: []model.OidcClientSecret{ + {ID: "active", Algorithm: model.OidcClientSecretHashSHA256, Hash: "hash-1", Prefix: "abcd"}, + {ID: "expiring", Algorithm: model.OidcClientSecretHashSHA256, Hash: "hash-2", Prefix: "efgh", ExpiresAt: &valid}, + {ID: "expired", Algorithm: model.OidcClientSecretHashSHA256, Hash: "hash-3", Prefix: "ijkl", ExpiresAt: &expired}, + }, + }, + } + + var clientDto OidcClientDto + require.NoError(t, MapStruct(client, &clientDto)) + require.Len(t, clientDto.Credentials.Secrets, 3) + + assert.Equal(t, "active", clientDto.Credentials.Secrets[0].ID) + assert.Equal(t, "abcd", clientDto.Credentials.Secrets[0].Prefix) + assert.True(t, clientDto.Credentials.Secrets[0].IsActive) + assert.True(t, clientDto.Credentials.Secrets[1].IsActive) + assert.False(t, clientDto.Credentials.Secrets[2].IsActive) + + // Serializing the client must never disclose the hashes of its secrets + serialized, err := json.Marshal(clientDto) + require.NoError(t, err) + 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 1c6e7d38..0024c1ed 100644 --- a/backend/internal/model/oidc.go +++ b/backend/internal/model/oidc.go @@ -3,6 +3,8 @@ package model import ( "database/sql/driver" "encoding/json" + "slices" + "time" datatype "github.com/pocket-id/pocket-id/backend/internal/model/types" "github.com/pocket-id/pocket-id/backend/internal/utils" @@ -34,6 +36,11 @@ const ( MinTokenDurationMinutes int64 = 1 // MaxTokenDurationMinutes is the longest configurable token lifetime MaxTokenDurationMinutes int64 = 365 * 24 * 60 + + // MaxOidcClientSecrets is the number of secrets a single client can have at the same time, including expired ones + MaxOidcClientSecrets = 20 + // OidcClientSecretPrefixLength is how many leading characters of a client secret are kept in clear text so admins can tell secrets apart + OidcClientSecretPrefixLength = 4 ) type OidcClient struct { @@ -41,7 +48,6 @@ type OidcClient struct { Name string `sortable:"true"` Description string - Secret string CallbackURLs datatype.StringList LogoutCallbackURLs datatype.StringList ImageType *string @@ -88,6 +94,66 @@ func (c OidcClient) IsMetadataDocument() bool { type OidcClientCredentials struct { //nolint:recvcheck FederatedIdentities []OidcClientFederatedIdentity `json:"federatedIdentities,omitempty"` + Secrets []OidcClientSecret `json:"secrets,omitempty"` +} + +// OidcClientSecretHashAlgorithm identifies how the hash of a client secret was computed +type OidcClientSecretHashAlgorithm string + +const ( + // OidcClientSecretHashSHA256 is used by every client secret generated by Pocket ID + // A plain hash is enough because client secrets are generated with enough entropy that they cannot be brute-forced + OidcClientSecretHashSHA256 OidcClientSecretHashAlgorithm = "sha256" + + // OidcClientSecretHashBcrypt is only found on secrets migrated from the single-secret column that Pocket ID used before it supported multiple secrets + // Those hashes cannot be converted to SHA-256, so they are kept as-is and verified with bcrypt until the admin rotates the secret + OidcClientSecretHashBcrypt OidcClientSecretHashAlgorithm = "bcrypt" +) + +// OidcClientSecret is a single client secret of an OIDC client, stored hashed in the credentials JSON document +type OidcClientSecret struct { + ID string `json:"id"` + Algorithm OidcClientSecretHashAlgorithm `json:"alg"` + 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"` +} + +// IsActive reports whether the secret can still be used to authenticate the client +func (s OidcClientSecret) IsActive() bool { + return !s.IsExpiredAt(time.Now()) +} + +// IsExpiredAt reports whether the secret is expired at the given time +// Secrets without an expiration date are always valid +func (s OidcClientSecret) IsExpiredAt(now time.Time) bool { + return s.ExpiresAt != nil && !now.Before(s.ExpiresAt.ToTime()) +} + +// EncodedHash returns the secret's hash with its algorithm prepended, in the format ":" +// Fosite compares hashes without any other context, so the algorithm has to travel alongside the hash +func (s OidcClientSecret) EncodedHash() []byte { + return []byte(string(s.Algorithm) + ":" + s.Hash) +} + +// ActiveSecrets returns the secrets that have not expired yet, ordered from the most recently created to the oldest +func (occ OidcClientCredentials) ActiveSecrets() []OidcClientSecret { + now := time.Now() + active := make([]OidcClientSecret, 0, len(occ.Secrets)) + for _, secret := range occ.Secrets { + if !secret.IsExpiredAt(now) { + active = append(active, secret) + } + } + + // Sort the copy so the primary hash is always the most recently created active secret + slices.SortStableFunc(active, func(a, b OidcClientSecret) int { + return b.CreatedAt.ToTime().Compare(a.CreatedAt.ToTime()) + }) + + return active } type OidcClientFederatedIdentity struct { diff --git a/backend/internal/oidc/client.go b/backend/internal/oidc/client.go index 2344b8d6..9fc57466 100644 --- a/backend/internal/oidc/client.go +++ b/backend/internal/oidc/client.go @@ -19,8 +19,28 @@ func (c Client) GetID() string { return c.ID } +// GetHashedSecret returns the hash of the client's most recently created secret that is still active +// Fosite checks this one first and then falls back to GetRotatedHashes for the others func (c Client) GetHashedSecret() []byte { - return []byte(c.Secret) + active := c.Credentials.ActiveSecrets() + if len(active) == 0 { + return nil + } + return active[0].EncodedHash() +} + +// GetRotatedHashes returns the hashes of every active secret except the one returned by GetHashedSecret +func (c Client) GetRotatedHashes() [][]byte { + active := c.Credentials.ActiveSecrets() + if len(active) <= 1 { + return nil + } + + hashes := make([][]byte, len(active)-1) + for i, secret := range active[1:] { + hashes[i] = secret.EncodedHash() + } + return hashes } func (c Client) GetRedirectURIs() []string { diff --git a/backend/internal/oidc/client_secret_hasher.go b/backend/internal/oidc/client_secret_hasher.go new file mode 100644 index 00000000..c4cb62c9 --- /dev/null +++ b/backend/internal/oidc/client_secret_hasher.go @@ -0,0 +1,49 @@ +package oidc + +import ( + "context" + "crypto/sha256" + "crypto/subtle" + "encoding/hex" + "errors" + "strings" + + "golang.org/x/crypto/bcrypt" + + "github.com/pocket-id/pocket-id/backend/internal/model" +) + +// errUnsupportedHashAlgorithm is returned when a stored hash carries an algorithm this build cannot verify +var errUnsupportedHashAlgorithm = errors.New("unsupported client secret hash algorithm") + +// clientSecretHasher verifies client secrets against the hashes stored in the client's credentials document +// Fosite hands it the encoded hashes produced by model.OidcClientSecret.EncodedHash, which carry the algorithm they were computed with +type clientSecretHasher struct{} + +// Compare checks a presented client secret against a stored, algorithm-prefixed hash +func (clientSecretHasher) Compare(_ context.Context, hash []byte, data []byte) error { + algorithm, encoded, found := strings.Cut(string(hash), ":") + if !found { + return errUnsupportedHashAlgorithm + } + + switch model.OidcClientSecretHashAlgorithm(algorithm) { + case model.OidcClientSecretHashSHA256: + sum := sha256.Sum256(data) + if subtle.ConstantTimeCompare([]byte(hex.EncodeToString(sum[:])), []byte(encoded)) != 1 { + return errors.New("client secret does not match") + } + return nil + case model.OidcClientSecretHashBcrypt: + // Legacy hashes migrated from the single-secret column, which cannot be re-hashed because the secret's value is not recoverable + return bcrypt.CompareHashAndPassword([]byte(encoded), data) + default: + return errUnsupportedHashAlgorithm + } +} + +// Hash is required by the fosite.Hasher interface but is never called, because Pocket ID hashes secrets when it stores them +func (clientSecretHasher) Hash(_ context.Context, data []byte) ([]byte, error) { + sum := sha256.Sum256(data) + return []byte(string(model.OidcClientSecretHashSHA256) + ":" + hex.EncodeToString(sum[:])), nil +} diff --git a/backend/internal/oidc/client_secret_hasher_test.go b/backend/internal/oidc/client_secret_hasher_test.go new file mode 100644 index 00000000..db67199b --- /dev/null +++ b/backend/internal/oidc/client_secret_hasher_test.go @@ -0,0 +1,99 @@ +package oidc + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "golang.org/x/crypto/bcrypt" + + "github.com/pocket-id/pocket-id/backend/internal/model" + "github.com/pocket-id/pocket-id/backend/internal/utils" +) + +func TestClientSecretHasherCompare(t *testing.T) { + const value = "client-secret-value" + + bcryptHash, err := bcrypt.GenerateFromPassword([]byte(value), bcrypt.MinCost) + require.NoError(t, err) + + for _, test := range []struct { + name string + hash string + data string + wantErr bool + }{ + { + name: "matching SHA-256 hash", + hash: "sha256:" + utils.CreateSha256Hash(value), + data: value, + }, + { + name: "SHA-256 hash of another secret", + hash: "sha256:" + utils.CreateSha256Hash(value), + data: "some-other-value", + wantErr: true, + }, + { + name: "bcrypt hash migrated from the single-secret column", + hash: "bcrypt:" + string(bcryptHash), + data: value, + }, + { + name: "bcrypt hash of another secret", + hash: "bcrypt:" + string(bcryptHash), + data: "some-other-value", + wantErr: true, + }, + { + name: "hash without an algorithm", + hash: utils.CreateSha256Hash(value), + data: value, + wantErr: true, + }, + { + name: "unknown algorithm", + hash: "md5:" + utils.CreateSha256Hash(value), + data: value, + wantErr: true, + }, + { + name: "empty hash, as returned for a client without secrets", + hash: "", + data: value, + wantErr: true, + }, + } { + t.Run(test.name, func(t *testing.T) { + err := clientSecretHasher{}.Compare(t.Context(), []byte(test.hash), []byte(test.data)) + if test.wantErr { + require.Error(t, err) + } else { + require.NoError(t, err) + } + }) + } +} + +func TestClientSecretHasherHash(t *testing.T) { + const value = "client-secret-value" + + hash, err := clientSecretHasher{}.Hash(t.Context(), []byte(value)) + require.NoError(t, err) + assert.Equal(t, "sha256:"+utils.CreateSha256Hash(value), string(hash)) + + // A hash produced by the hasher is accepted by its own comparison + err = clientSecretHasher{}.Compare(t.Context(), hash, []byte(value)) + require.NoError(t, err) +} + +func TestClientSecretHasherAcceptsModelEncoding(t *testing.T) { + const value = "client-secret-value" + + secret := model.OidcClientSecret{ + Algorithm: model.OidcClientSecretHashSHA256, + Hash: utils.CreateSha256Hash(value), + } + err := clientSecretHasher{}.Compare(t.Context(), secret.EncodedHash(), []byte(value)) + require.NoError(t, err) +} diff --git a/backend/internal/oidc/client_test.go b/backend/internal/oidc/client_test.go index 5279134d..1f23da50 100644 --- a/backend/internal/oidc/client_test.go +++ b/backend/internal/oidc/client_test.go @@ -1,13 +1,17 @@ package oidc import ( + "strconv" "testing" "time" "github.com/ory/fosite" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "github.com/pocket-id/pocket-id/backend/internal/model" + datatype "github.com/pocket-id/pocket-id/backend/internal/model/types" + "github.com/pocket-id/pocket-id/backend/internal/utils" ) // Interface assertions @@ -15,8 +19,26 @@ var ( _ fosite.Client = (*Client)(nil) _ fosite.ResponseModeClient = (*Client)(nil) _ fosite.ClientWithCustomTokenLifespans = (*Client)(nil) + _ fosite.ClientWithSecretRotation = (*Client)(nil) ) +// testClientCredentials returns credentials with a single never-expiring secret with the given value +func testClientCredentials(values ...string) model.OidcClientCredentials { + credentials := model.OidcClientCredentials{ + Secrets: make([]model.OidcClientSecret, len(values)), + } + for i, value := range values { + credentials.Secrets[i] = model.OidcClientSecret{ + ID: "secret-" + strconv.Itoa(i), + Algorithm: model.OidcClientSecretHashSHA256, + Hash: utils.CreateSha256Hash(value), + Prefix: value[:model.OidcClientSecretPrefixLength], + CreatedAt: datatype.DateTime(time.Now()), + } + } + return credentials +} + func TestClientGetEffectiveLifespan(t *testing.T) { client := Client{OidcClient: model.OidcClient{ AccessTokenDurationMinutes: 2 * 60, @@ -51,3 +73,41 @@ func TestClientGetEffectiveLifespan(t *testing.T) { require.Equal(t, fallback, client.GetEffectiveLifespan(fosite.GrantTypeAuthorizationCode, fosite.AccessToken, fallback)) require.Equal(t, fallback, client.GetEffectiveLifespan(fosite.GrantTypeAuthorizationCode, fosite.RefreshToken, fallback)) } + +func TestClientSecretHashes(t *testing.T) { + past := datatype.DateTime(time.Now().Add(-time.Hour)) + future := datatype.DateTime(time.Now().Add(time.Hour)) + older := datatype.DateTime(time.Now().Add(-48 * time.Hour)) + newer := datatype.DateTime(time.Now().Add(-time.Minute)) + + t.Run("client without secrets", func(t *testing.T) { + client := Client{} + assert.Nil(t, client.GetHashedSecret()) + assert.Empty(t, client.GetRotatedHashes()) + }) + + t.Run("expired secrets are never returned", func(t *testing.T) { + client := Client{OidcClient: model.OidcClient{Credentials: model.OidcClientCredentials{ + Secrets: []model.OidcClientSecret{ + {ID: "expired", Algorithm: model.OidcClientSecretHashSHA256, Hash: "expired-hash", ExpiresAt: &past}, + }, + }}} + assert.Nil(t, client.GetHashedSecret()) + assert.Empty(t, client.GetRotatedHashes()) + }) + + t.Run("the most recent active secret comes first", func(t *testing.T) { + client := Client{OidcClient: model.OidcClient{Credentials: model.OidcClientCredentials{ + Secrets: []model.OidcClientSecret{ + {ID: "older", Algorithm: model.OidcClientSecretHashSHA256, Hash: "older-hash", CreatedAt: older}, + {ID: "expired", Algorithm: model.OidcClientSecretHashSHA256, Hash: "expired-hash", CreatedAt: newer, ExpiresAt: &past}, + {ID: "newer", Algorithm: model.OidcClientSecretHashSHA256, Hash: "newer-hash", CreatedAt: newer, ExpiresAt: &future}, + }, + }}} + + assert.Equal(t, "sha256:newer-hash", string(client.GetHashedSecret())) + rotated := client.GetRotatedHashes() + require.Len(t, rotated, 1) + assert.Equal(t, "sha256:older-hash", string(rotated[0])) + }) +} diff --git a/backend/internal/oidc/provider.go b/backend/internal/oidc/provider.go index 4a83f95e..86049670 100644 --- a/backend/internal/oidc/provider.go +++ b/backend/internal/oidc/provider.go @@ -57,6 +57,7 @@ func newProvider(store *Store, authenticator *federatedClientAuthenticator, sign GlobalSecret: secret, JWTScopeClaimKey: jwt.JWTScopeFieldBoth, ClientResolver: clientResolver, + ClientSecretsHasher: clientSecretHasher{}, } keyGetter := func(context.Context) (interface{}, error) { diff --git a/backend/internal/oidc/token_handler_test.go b/backend/internal/oidc/token_handler_test.go index 20867504..4ba0bd96 100644 --- a/backend/internal/oidc/token_handler_test.go +++ b/backend/internal/oidc/token_handler_test.go @@ -20,10 +20,10 @@ import ( fositeoauth2 "github.com/ory/fosite/handler/oauth2" fositejwt "github.com/ory/fosite/token/jwt" "github.com/stretchr/testify/require" - "golang.org/x/crypto/bcrypt" "gorm.io/gorm" "github.com/pocket-id/pocket-id/backend/internal/model" + datatype "github.com/pocket-id/pocket-id/backend/internal/model/types" testutils "github.com/pocket-id/pocket-id/backend/internal/utils/testing" ) @@ -49,12 +49,10 @@ func TestTokenHandlerClientCredentialsGrant(t *testing.T) { key, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader) require.NoError(t, err) - hashed, err := bcrypt.GenerateFromPassword([]byte(clientPlain), bcrypt.DefaultCost) - require.NoError(t, err) require.NoError(t, db.Create(&model.OidcClient{ Base: model.Base{ID: clientID}, Name: "Client Credentials Client", - Secret: string(hashed), + Credentials: testClientCredentials(clientPlain), IsPublic: false, AccessTokenDurationMinutes: 2 * 60, }).Error) @@ -111,13 +109,11 @@ func TestTokenHandlerClientCredentialsDropsIdentityScopes(t *testing.T) { key, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader) require.NoError(t, err) - hashed, err := bcrypt.GenerateFromPassword([]byte(clientPlain), bcrypt.DefaultCost) - require.NoError(t, err) require.NoError(t, db.Create(&model.OidcClient{ - Base: model.Base{ID: clientID}, - Name: "Client Credentials Client", - Secret: string(hashed), - IsPublic: false, + Base: model.Base{ID: clientID}, + Name: "Client Credentials Client", + Credentials: testClientCredentials(clientPlain), + IsPublic: false, }).Error) provider, err := newProvider(NewStore(db, nil), nil, testTokenSigner{key: key}, Config{ @@ -166,13 +162,11 @@ func TestTokenHandlerClientCredentialsUsesClientSubjectGrants(t *testing.T) { key, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader) require.NoError(t, err) - hashed, err := bcrypt.GenerateFromPassword([]byte(clientPlain), bcrypt.DefaultCost) - require.NoError(t, err) require.NoError(t, db.Create(&model.OidcClient{ - Base: model.Base{ID: clientID}, - Name: "Client Credentials Client", - Secret: string(hashed), - IsPublic: false, + Base: model.Base{ID: clientID}, + Name: "Client Credentials Client", + Credentials: testClientCredentials(clientPlain), + IsPublic: false, }).Error) apiAccess := fakeAPIAccess{allowed: map[string]map[SubjectType][]string{ @@ -239,13 +233,11 @@ func TestTokenHandlerClientCredentialsDefaultsResourceScopes(t *testing.T) { key, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader) require.NoError(t, err) - hashed, err := bcrypt.GenerateFromPassword([]byte(clientPlain), bcrypt.DefaultCost) - require.NoError(t, err) require.NoError(t, db.Create(&model.OidcClient{ - Base: model.Base{ID: clientID}, - Name: "Client Credentials Client", - Secret: string(hashed), - IsPublic: false, + Base: model.Base{ID: clientID}, + Name: "Client Credentials Client", + Credentials: testClientCredentials(clientPlain), + IsPublic: false, }).Error) apiAccess := fakeAPIAccess{allowed: map[string]map[SubjectType][]string{ @@ -731,3 +723,124 @@ func TestTokenHandlerRefreshGrantPreservesAudienceAndScope(t *testing.T) { require.Equal(t, "invalid_scope", body["error"]) }) } + +// TestTokenHandlerClientSecretRotation drives the token endpoint with each of a client's secrets, +// to check that every active secret authenticates the client while expired ones are rejected. +func TestTokenHandlerClientSecretRotation(t *testing.T) { + gin.SetMode(gin.TestMode) + + const ( + baseURL = "https://issuer.example.com" + secret = "test-secret" + clientID = "rotation-client" + currentPlain = "current-secret-value" + olderPlain = "older-secret-value" + expiredPlain = "expired-secret-value" + ) + + db := testutils.NewDatabaseForTest(t) + key, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader) + require.NoError(t, err) + + credentials := testClientCredentials(currentPlain, olderPlain, expiredPlain) + expiredAt := datatype.DateTime(time.Now().Add(-time.Hour)) + credentials.Secrets[2].ExpiresAt = &expiredAt + + require.NoError(t, db.Create(&model.OidcClient{ + Base: model.Base{ID: clientID}, + Name: "Secret Rotation Client", + Credentials: credentials, + IsPublic: false, + }).Error) + + provider, err := newProvider(NewStore(db, nil), nil, testTokenSigner{key: key}, Config{ + BaseURL: baseURL, + TokenBaseURL: baseURL, + Secret: []byte(secret), + }, nil) + require.NoError(t, err) + handler := newTokenHandler(provider, newClaimsService(db, nil, baseURL, nil), nil) + + requestToken := func(t *testing.T, clientSecret string) map[string]any { + t.Helper() + + form := url.Values{"grant_type": {"client_credentials"}} + req := httptest.NewRequestWithContext(t.Context(), http.MethodPost, "/api/oidc/token", strings.NewReader(form.Encode())) + req.Header.Set("Content-Type", "application/x-www-form-urlencoded") + req.SetBasicAuth(clientID, clientSecret) + + rec := httptest.NewRecorder() + c, _ := gin.CreateTestContext(rec) + c.Request = req + handler.token(c) + + var body map[string]any + require.NoError(t, json.Unmarshal(rec.Body.Bytes(), &body)) + return body + } + + for _, test := range []struct { + name string + clientSecret string + wantToken bool + }{ + {name: "most recently created secret", clientSecret: currentPlain, wantToken: true}, + {name: "secret being rotated out", clientSecret: olderPlain, wantToken: true}, + {name: "expired secret", clientSecret: expiredPlain}, + {name: "secret that was never configured", clientSecret: "not-a-secret"}, + } { + t.Run(test.name, func(t *testing.T) { + body := requestToken(t, test.clientSecret) + if test.wantToken { + require.NotEmpty(t, body["access_token"], "expected a token, got error: %v", body["error"]) + } else { + require.Empty(t, body["access_token"]) + require.Equal(t, "invalid_client", body["error"]) + } + }) + } +} + +// TestTokenHandlerClientWithoutSecrets checks that a confidential client that has no secrets at all cannot authenticate. +func TestTokenHandlerClientWithoutSecrets(t *testing.T) { + gin.SetMode(gin.TestMode) + + const ( + baseURL = "https://issuer.example.com" + secret = "test-secret" + clientID = "no-secrets-client" + ) + + db := testutils.NewDatabaseForTest(t) + key, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader) + require.NoError(t, err) + + require.NoError(t, db.Create(&model.OidcClient{ + Base: model.Base{ID: clientID}, + Name: "Client Without Secrets", + IsPublic: false, + }).Error) + + provider, err := newProvider(NewStore(db, nil), nil, testTokenSigner{key: key}, Config{ + BaseURL: baseURL, + TokenBaseURL: baseURL, + Secret: []byte(secret), + }, nil) + require.NoError(t, err) + handler := newTokenHandler(provider, newClaimsService(db, nil, baseURL, nil), nil) + + form := url.Values{"grant_type": {"client_credentials"}} + req := httptest.NewRequestWithContext(t.Context(), http.MethodPost, "/api/oidc/token", strings.NewReader(form.Encode())) + req.Header.Set("Content-Type", "application/x-www-form-urlencoded") + req.SetBasicAuth(clientID, "") + + rec := httptest.NewRecorder() + c, _ := gin.CreateTestContext(rec) + c.Request = req + handler.token(c) + + var body map[string]any + require.NoError(t, json.Unmarshal(rec.Body.Bytes(), &body)) + require.Empty(t, body["access_token"]) + require.Equal(t, "invalid_client", body["error"]) +} diff --git a/backend/internal/service/e2etest_service.go b/backend/internal/service/e2etest_service.go index 37e3cfa0..c41fb186 100644 --- a/backend/internal/service/e2etest_service.go +++ b/backend/internal/service/e2etest_service.go @@ -103,6 +103,26 @@ func (s *TestService) initExternalIdP() error { return nil } +// seededClientCredentials returns credentials holding a single client secret with the given value, which E2E tests use to authenticate as the client +func seededClientCredentials(secretID string, value string) model.OidcClientCredentials { + return model.OidcClientCredentials{ + Secrets: seededClientSecrets(secretID, value), + } +} + +// seededClientSecrets returns a single never-expiring client secret with the given value +func seededClientSecrets(secretID string, value string) []model.OidcClientSecret { + return []model.OidcClientSecret{ + { + ID: secretID, + Algorithm: model.OidcClientSecretHashSHA256, + Hash: utils.CreateSha256Hash(value), + Prefix: value[:model.OidcClientSecretPrefixLength], + CreatedAt: datatype.DateTime(time.Now()), + }, + } +} + //nolint:gocognit func (s *TestService) SeedDatabase(baseURL string) error { err := s.db.Transaction(func(tx *gorm.DB) error { @@ -181,7 +201,7 @@ func (s *TestService) SeedDatabase(baseURL string) error { Name: "Nextcloud", Description: "This is an example description for Nextcloud", LaunchURL: new("https://nextcloud.local"), - Secret: "$2a$10$9dypwot8nGuCjT6wQWWpJOckZfRprhe2EkwpKizxS/fpVHrOLEJHC", // w2mUeZISmEvIDMEDvpY0PnxQIpj1m3zY + Credentials: seededClientCredentials("2f1b8f1a-1d3e-4f0c-9c1a-000000000001", "w2mUeZISmEvIDMEDvpY0PnxQIpj1m3zY"), CallbackURLs: datatype.StringList{"http://nextcloud.localhost/auth/callback"}, LogoutCallbackURLs: datatype.StringList{"http://nextcloud.localhost/auth/logout/callback"}, ImageType: new("png"), @@ -193,7 +213,7 @@ func (s *TestService) SeedDatabase(baseURL string) error { }, Name: "Immich", LaunchURL: new("https://immich.local"), - Secret: "$2a$10$Ak.FP8riD1ssy2AGGbG.gOpnp/rBpymd74j0nxNMtW0GG1Lb4gzxe", // PYjrE9u4v9GVqXKi52eur0eb2Ci4kc0x + Credentials: seededClientCredentials("2f1b8f1a-1d3e-4f0c-9c1a-000000000002", "PYjrE9u4v9GVqXKi52eur0eb2Ci4kc0x"), CallbackURLs: datatype.StringList{"http://immich.localhost/auth/callback"}, CreatedByID: new(users[1].ID), IsGroupRestricted: true, @@ -206,7 +226,7 @@ func (s *TestService) SeedDatabase(baseURL string) error { ID: "7c21a609-96b5-4011-9900-272b8d31a9d1", }, Name: "Tailscale", - Secret: "$2a$10$xcRReBsvkI1XI6FG8xu/pOgzeF00bH5Wy4d/NThwcdi3ZBpVq/B9a", // n4VfQeXlTzA6yKpWbR9uJcMdSx2qH0Lo + Credentials: seededClientCredentials("2f1b8f1a-1d3e-4f0c-9c1a-000000000003", "n4VfQeXlTzA6yKpWbR9uJcMdSx2qH0Lo"), CallbackURLs: datatype.StringList{"http://tailscale.localhost/auth/callback"}, LogoutCallbackURLs: datatype.StringList{"http://tailscale.localhost/auth/logout/callback"}, IsGroupRestricted: true, @@ -220,11 +240,11 @@ func (s *TestService) SeedDatabase(baseURL string) error { ID: "c48232ff-ff65-45ed-ae96-7afa8a9b443b", }, Name: "Federated", - Secret: "$2a$10$Ak.FP8riD1ssy2AGGbG.gOpnp/rBpymd74j0nxNMtW0GG1Lb4gzxe", // PYjrE9u4v9GVqXKi52eur0eb2Ci4kc0x CallbackURLs: datatype.StringList{"http://federated.localhost/auth/callback"}, CreatedByID: new(users[1].ID), AllowedUserGroups: []model.UserGroup{}, Credentials: model.OidcClientCredentials{ + Secrets: seededClientSecrets("2f1b8f1a-1d3e-4f0c-9c1a-000000000004", "PYjrE9u4v9GVqXKi52eur0eb2Ci4kc0x"), FederatedIdentities: []model.OidcClientFederatedIdentity{ { Issuer: "https://external-idp.local", @@ -240,7 +260,7 @@ func (s *TestService) SeedDatabase(baseURL string) error { ID: "c46d2090-37a0-4f2b-8748-6aa53b0c1afa", }, Name: "SCIM Client", - Secret: "$2a$10$h4wfa8gI7zavDAxwzSq1sOwYU4e8DwK1XZ8ZweNnY5KzlJ3Iz.qdK", // nQbiuMRG7FpdK2EnDd5MBivWQeKFXohn + Credentials: seededClientCredentials("2f1b8f1a-1d3e-4f0c-9c1a-000000000005", "nQbiuMRG7FpdK2EnDd5MBivWQeKFXohn"), CallbackURLs: datatype.StringList{"http://scimclient.localhost/auth/callback"}, CreatedByID: new(users[0].ID), IsGroupRestricted: true, @@ -254,7 +274,7 @@ func (s *TestService) SeedDatabase(baseURL string) error { ID: "a1b2c3d4-e5f6-7890-abcd-ef0000000001", }, Name: "PAR Test Client", - Secret: "$2a$10$9dypwot8nGuCjT6wQWWpJOckZfRprhe2EkwpKizxS/fpVHrOLEJHC", // w2mUeZISmEvIDMEDvpY0PnxQIpj1m3zY + Credentials: seededClientCredentials("2f1b8f1a-1d3e-4f0c-9c1a-000000000006", "w2mUeZISmEvIDMEDvpY0PnxQIpj1m3zY"), CallbackURLs: datatype.StringList{"http://par-client.localhost/auth/callback"}, CreatedByID: new(users[0].ID), }, @@ -263,7 +283,7 @@ func (s *TestService) SeedDatabase(baseURL string) error { ID: "e1f2a3b4-c5d6-7890-abcd-ef0000000002", }, Name: "Skip Consent Client", - Secret: "$2a$10$9dypwot8nGuCjT6wQWWpJOckZfRprhe2EkwpKizxS/fpVHrOLEJHC", // w2mUeZISmEvIDMEDvpY0PnxQIpj1m3zY + Credentials: seededClientCredentials("2f1b8f1a-1d3e-4f0c-9c1a-000000000007", "w2mUeZISmEvIDMEDvpY0PnxQIpj1m3zY"), CallbackURLs: datatype.StringList{"http://skip-consent.localhost/auth/callback"}, CreatedByID: new(users[0].ID), // Trusted client that bypasses the consent screen by default diff --git a/backend/internal/service/oidc_client_secrets_migration_test.go b/backend/internal/service/oidc_client_secrets_migration_test.go new file mode 100644 index 00000000..ff4a8b52 --- /dev/null +++ b/backend/internal/service/oidc_client_secrets_migration_test.go @@ -0,0 +1,88 @@ +package service + +import ( + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "golang.org/x/crypto/bcrypt" + "gorm.io/gorm" + + "github.com/pocket-id/pocket-id/backend/internal/model" + testutils "github.com/pocket-id/pocket-id/backend/internal/utils/testing" +) + +// versionBeforeMultipleClientSecrets is the migration version right before client secrets moved into the credentials document +const versionBeforeMultipleClientSecrets = 20260802120000 + +// TestMigrateClientSecretsToCredentials checks that the secret of every existing client is preserved as an entry of the credentials document +func TestMigrateClientSecretsToCredentials(t *testing.T) { + const legacySecret = "legacy-client-secret" + + legacyHash, err := bcrypt.GenerateFromPassword([]byte(legacySecret), bcrypt.MinCost) + require.NoError(t, err) + + createdAt := time.Now().Add(-72 * time.Hour).Truncate(time.Second) + + db := testutils.NewDatabaseForTestWithMigrationSeed(t, versionBeforeMultipleClientSecrets, func(t *testing.T, db *gorm.DB) { + // A client with a secret and no other credentials + err := db.Exec( + `INSERT INTO oidc_clients (id, created_at, name, secret, callback_urls, is_public, pkce_enabled) VALUES (?, ?, ?, ?, ?, ?, ?)`, + "client-with-secret", createdAt.Unix(), "With Secret", string(legacyHash), `["https://example.com/callback"]`, false, false, + ).Error + require.NoError(t, err) + + // A client whose credentials document already holds a federated identity, which the migration must preserve + err = db.Exec( + `INSERT INTO oidc_clients (id, created_at, name, secret, callback_urls, is_public, pkce_enabled, credentials) VALUES (?, ?, ?, ?, ?, ?, ?, ?)`, + "client-with-federated-identity", createdAt.Unix(), "With Federated Identity", string(legacyHash), `["https://example.com/callback"]`, false, false, + `{"federatedIdentities":[{"issuer":"https://issuer.example.com"}]}`, + ).Error + require.NoError(t, err) + + // A public client, which never had a secret + err = db.Exec( + `INSERT INTO oidc_clients (id, created_at, name, secret, callback_urls, is_public, pkce_enabled) VALUES (?, ?, ?, ?, ?, ?, ?)`, + "public-client", createdAt.Unix(), "Public", "", `["https://example.com/callback"]`, true, true, + ).Error + require.NoError(t, err) + }) + + // The legacy column is gone once the migration has run + ok := db.Migrator().HasColumn(&model.OidcClient{}, "secret") + assert.False(t, ok) + + var clients []model.OidcClient + err = db.Find(&clients).Error + require.NoError(t, err) + byID := make(map[string]model.OidcClient, len(clients)) + for _, client := range clients { + byID[client.ID] = client + } + require.Len(t, byID, 3) + + // The migrated secret keeps its bcrypt hash, has no expiration, and carries no prefix because the value was never stored + migrated := byID["client-with-secret"].Credentials.Secrets + require.Len(t, migrated, 1) + assert.NotEmpty(t, migrated[0].ID) + assert.Equal(t, model.OidcClientSecretHashBcrypt, migrated[0].Algorithm) + assert.Equal(t, string(legacyHash), migrated[0].Hash) + assert.Empty(t, migrated[0].Prefix) + assert.Nil(t, migrated[0].ExpiresAt) + assert.True(t, migrated[0].IsActive()) + assert.Equal(t, createdAt.UTC(), migrated[0].CreatedAt.UTC()) + + // Existing federated identities survive the migration alongside the new secret + withFederated := byID["client-with-federated-identity"].Credentials + require.Len(t, withFederated.Secrets, 1) + assert.Equal(t, string(legacyHash), withFederated.Secrets[0].Hash) + require.Len(t, withFederated.FederatedIdentities, 1) + assert.Equal(t, "https://issuer.example.com", withFederated.FederatedIdentities[0].Issuer) + + // Clients that never had a secret do not get an empty one + assert.Empty(t, byID["public-client"].Credentials.Secrets) + + // Every migrated secret gets its own identifier + assert.NotEqual(t, migrated[0].ID, withFederated.Secrets[0].ID) +} diff --git a/backend/internal/service/oidc_service.go b/backend/internal/service/oidc_service.go index 2e5664da..1a788ad5 100644 --- a/backend/internal/service/oidc_service.go +++ b/backend/internal/service/oidc_service.go @@ -10,10 +10,11 @@ import ( "net/http" "net/url" "path" + "slices" "strings" "time" - "golang.org/x/crypto/bcrypt" + "github.com/google/uuid" "gorm.io/gorm" "gorm.io/gorm/clause" @@ -320,7 +321,23 @@ func (s *OidcService) DeleteClient(ctx context.Context, clientID string) error { return nil } -func (s *OidcService) CreateClientSecret(ctx context.Context, clientID string, input dto.OidcClientSecretDto) (string, error) { +// ListClientSecrets returns all secrets configured for a client, including the expired ones +func (s *OidcService) ListClientSecrets(ctx context.Context, clientID string) ([]model.OidcClientSecret, error) { + client, err := s.getClientInternal(ctx, clientID, s.db, false) + if err != nil { + return nil, err + } + + return client.Credentials.Secrets, nil +} + +// CreateClientSecret adds a new secret to a client and returns both the stored record and the secret's value, which is not recoverable afterwards +func (s *OidcService) CreateClientSecret(ctx context.Context, clientID string, input dto.OidcClientSecretCreateDto) (model.OidcClientSecret, string, error) { + // An expiration date in the past would create a secret that can never be used + if input.ExpiresAt != nil && !input.ExpiresAt.ToTime().After(time.Now()) { + return model.OidcClientSecret{}, "", apperror.ValidationMessage("The expiration date of a client secret must be in the future") + } + tx := s.db.Begin() defer func() { tx.Rollback() @@ -328,41 +345,100 @@ func (s *OidcService) CreateClientSecret(ctx context.Context, clientID string, i client, err := s.getClientInternal(ctx, clientID, tx, true) if err != nil { - return "", err + return model.OidcClientSecret{}, "", fmt.Errorf("error retrieving OIDC client: %w", err) } if client.IsPublic { - return "", apperror.ValidationMessage("Cannot create a secret for a public client") + return model.OidcClientSecret{}, "", apperror.ValidationMessage("Cannot create a secret for a public client") } + if len(client.Credentials.Secrets) >= model.MaxOidcClientSecrets { + 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 "", err + return model.OidcClientSecret{}, "", fmt.Errorf("failed to generate client secret: %w", err) } } - hashedSecret, err := bcrypt.GenerateFromPassword([]byte(clientSecret), bcrypt.DefaultCost) - if err != nil { - return "", 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.New().String(), + Algorithm: model.OidcClientSecretHashSHA256, + Hash: utils.CreateSha256Hash(clientSecret), + Prefix: clientSecretPrefix(clientSecret), + CreatedAt: datatype.DateTime(time.Now()), + ExpiresAt: input.ExpiresAt, } + client.Credentials.Secrets = append(client.Credentials.Secrets, secret) - client.Secret = string(hashedSecret) err = tx. WithContext(ctx). - Save(&client). + Model(&client). + Select("Credentials"). + Updates(&client). Error if err != nil { - return "", err + return model.OidcClientSecret{}, "", fmt.Errorf("failed to update OIDC client: %w", err) } err = tx.Commit().Error if err != nil { - return "", err + return model.OidcClientSecret{}, "", fmt.Errorf("failed to commit transaction: %w", err) } - return clientSecret, nil + 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() + defer func() { + tx.Rollback() + }() + + client, err := s.getClientInternal(ctx, clientID, tx, true) + if err != nil { + return fmt.Errorf("error retrieving OIDC client: %w", err) + } + + countBefore := len(client.Credentials.Secrets) + client.Credentials.Secrets = slices.DeleteFunc(client.Credentials.Secrets, func(secret model.OidcClientSecret) bool { + return secret.ID == secretID + }) + if len(client.Credentials.Secrets) == countBefore { + return apperror.NotFound("Client secret") + } + + err = tx. + WithContext(ctx). + Model(&client). + Select("Credentials"). + Updates(&client). + Error + if err != nil { + return fmt.Errorf("failed to update OIDC client: %w", err) + } + + err = tx.Commit().Error + if err != nil { + return fmt.Errorf("failed to commit transaction: %w", err) + } + + return nil +} + +// clientSecretPrefix returns the leading characters of a secret that are stored in clear text to help admins tell secrets apart +func clientSecretPrefix(clientSecret string) string { + if len(clientSecret) <= model.OidcClientSecretPrefixLength { + return "" + } + + return clientSecret[:model.OidcClientSecretPrefixLength] } func (s *OidcService) GetClientLogo(ctx context.Context, clientID string, light bool) (io.ReadCloser, int64, string, error) { diff --git a/backend/internal/service/oidc_service_test.go b/backend/internal/service/oidc_service_test.go index e3acc047..156042ca 100644 --- a/backend/internal/service/oidc_service_test.go +++ b/backend/internal/service/oidc_service_test.go @@ -6,8 +6,7 @@ import ( "strconv" "strings" "testing" - - "golang.org/x/crypto/bcrypt" + "time" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -668,16 +667,130 @@ func TestOidcService_CreateClientSecret_withCustomSecret(t *testing.T) { require.NoError(t, err) customSecret := "custom-client-secret-with-a-minimum-length" - input := dto.OidcClientSecretDto{Secret: customSecret} + input := dto.OidcClientSecretCreateDto{Secret: customSecret} - secret, err := s.CreateClientSecret(t.Context(), client.ID, input) + created, secret, err := s.CreateClientSecret(t.Context(), client.ID, input) require.NoError(t, err) assert.Equal(t, customSecret, secret) + assert.Equal(t, "cust", created.Prefix) + assert.Nil(t, created.ExpiresAt) + assert.True(t, created.IsActive()) var fetched model.OidcClient err = db.First(&fetched, "id = ?", client.ID).Error require.NoError(t, err) - require.NoError(t, bcrypt.CompareHashAndPassword([]byte(fetched.Secret), []byte(customSecret))) + require.Len(t, fetched.Credentials.Secrets, 1) + assert.Equal(t, created.ID, fetched.Credentials.Secrets[0].ID) + assert.Equal(t, model.OidcClientSecretHashSHA256, fetched.Credentials.Secrets[0].Algorithm) + assert.Equal(t, utils.CreateSha256Hash(customSecret), fetched.Credentials.Secrets[0].Hash) +} + +func TestOidcService_CreateClientSecret_multipleSecrets(t *testing.T) { + db := testutils.NewDatabaseForTest(t) + + s, err := NewOidcService(db, nil, nil, nil, nil, nil, nil) + require.NoError(t, err) + + client := model.OidcClient{Name: "Test Client"} + err = db.Create(&client).Error + require.NoError(t, err) + + // Adding a second secret leaves the first one in place + first, _, err := s.CreateClientSecret(t.Context(), client.ID, dto.OidcClientSecretCreateDto{}) + require.NoError(t, err) + expiresAt := datatype.DateTime(time.Now().Add(24 * time.Hour)) + second, _, err := s.CreateClientSecret(t.Context(), client.ID, dto.OidcClientSecretCreateDto{ExpiresAt: &expiresAt}) + require.NoError(t, err) + + secrets, err := s.ListClientSecrets(t.Context(), client.ID) + require.NoError(t, err) + require.Len(t, secrets, 2) + assert.Equal(t, first.ID, secrets[0].ID) + assert.Equal(t, second.ID, secrets[1].ID) + assert.Equal(t, expiresAt.ToTime().Unix(), secrets[1].ExpiresAt.ToTime().Unix()) + + // Deleting one secret keeps the other usable + err = s.DeleteClientSecret(t.Context(), client.ID, first.ID) + require.NoError(t, err) + + secrets, err = s.ListClientSecrets(t.Context(), client.ID) + require.NoError(t, err) + require.Len(t, secrets, 1) + assert.Equal(t, second.ID, secrets[0].ID) + + // Deleting an unknown secret is reported as not found + err = s.DeleteClientSecret(t.Context(), client.ID, first.ID) + require.Error(t, err) +} + +func TestOidcService_CreateClientSecret_expirationInThePast(t *testing.T) { + db := testutils.NewDatabaseForTest(t) + + s, err := NewOidcService(db, nil, nil, nil, nil, nil, nil) + require.NoError(t, err) + + client := model.OidcClient{Name: "Test Client"} + err = db.Create(&client).Error + require.NoError(t, err) + + expiresAt := datatype.DateTime(time.Now().Add(-time.Minute)) + _, _, err = s.CreateClientSecret(t.Context(), client.ID, dto.OidcClientSecretCreateDto{ExpiresAt: &expiresAt}) + require.Error(t, err) +} + +func TestOidcService_CreateClientSecret_limit(t *testing.T) { + db := testutils.NewDatabaseForTest(t) + + s, err := NewOidcService(db, nil, nil, nil, nil, nil, nil) + require.NoError(t, err) + + client := model.OidcClient{Name: "Test Client"} + err = db.Create(&client).Error + require.NoError(t, err) + + for range model.MaxOidcClientSecrets { + _, _, err = s.CreateClientSecret(t.Context(), client.ID, dto.OidcClientSecretCreateDto{}) + require.NoError(t, err) + } + + _, _, err = s.CreateClientSecret(t.Context(), client.ID, dto.OidcClientSecretCreateDto{}) + require.Error(t, err) +} + +func TestOidcService_CreateClientSecret_preservesFederatedIdentities(t *testing.T) { + db := testutils.NewDatabaseForTest(t) + + s, err := NewOidcService(db, nil, nil, nil, nil, nil, nil) + require.NoError(t, err) + + client := model.OidcClient{ + Name: "Test Client", + CallbackURLs: datatype.StringList{"https://example.com/callback"}, + Credentials: model.OidcClientCredentials{ + FederatedIdentities: []model.OidcClientFederatedIdentity{{Issuer: "https://issuer.example.com"}}, + }, + } + err = db.Create(&client).Error + require.NoError(t, err) + + _, _, err = s.CreateClientSecret(t.Context(), client.ID, dto.OidcClientSecretCreateDto{}) + require.NoError(t, err) + + // Updating the client must not drop the secrets managed by the dedicated endpoints + _, err = s.UpdateClient(t.Context(), client.ID, dto.OidcClientUpdateDto{ + Name: "Test Client", + CallbackURLs: []string{"https://example.com/callback"}, + Credentials: dto.OidcClientCredentialsDto{ + FederatedIdentities: []dto.OidcClientFederatedIdentityDto{{Issuer: "https://other.example.com"}}, + }, + }) + require.NoError(t, err) + + var fetched model.OidcClient + require.NoError(t, db.First(&fetched, "id = ?", client.ID).Error) + require.Len(t, fetched.Credentials.Secrets, 1) + require.Len(t, fetched.Credentials.FederatedIdentities, 1) + assert.Equal(t, "https://other.example.com", fetched.Credentials.FederatedIdentities[0].Issuer) } func TestOidcService_UpdateClient_description(t *testing.T) { diff --git a/backend/resources/migrations/postgres/20260807120000_oidc_client_multiple_secrets.down.sql b/backend/resources/migrations/postgres/20260807120000_oidc_client_multiple_secrets.down.sql new file mode 100644 index 00000000..5ae40d3a --- /dev/null +++ b/backend/resources/migrations/postgres/20260807120000_oidc_client_multiple_secrets.down.sql @@ -0,0 +1,16 @@ +ALTER TABLE oidc_clients ADD COLUMN secret TEXT; + +-- Only secrets that were migrated up from this same column can be restored, since the old column stores a bcrypt hash +-- Secrets created while multiple secrets were supported are hashed with SHA-256 and are dropped here +UPDATE oidc_clients +SET secret = ( + SELECT secret_entry ->> 'hash' + FROM jsonb_array_elements(credentials -> 'secrets') AS secret_entry + WHERE secret_entry ->> 'alg' = 'bcrypt' + LIMIT 1 +) +WHERE jsonb_typeof(credentials -> 'secrets') = 'array'; + +UPDATE oidc_clients +SET credentials = credentials - 'secrets' +WHERE jsonb_typeof(credentials -> 'secrets') = 'array'; diff --git a/backend/resources/migrations/postgres/20260807120000_oidc_client_multiple_secrets.up.sql b/backend/resources/migrations/postgres/20260807120000_oidc_client_multiple_secrets.up.sql new file mode 100644 index 00000000..12042035 --- /dev/null +++ b/backend/resources/migrations/postgres/20260807120000_oidc_client_multiple_secrets.up.sql @@ -0,0 +1,14 @@ +-- Move the single client secret into the credentials document, as the only entry of the new "secrets" array +-- Migrated secrets keep their bcrypt hash because the secret's value is not recoverable, and they never expire so that existing integrations keep working +UPDATE oidc_clients +SET credentials = COALESCE(credentials, '{}'::jsonb) || jsonb_build_object( + 'secrets', jsonb_build_array(jsonb_build_object( + 'id', gen_random_uuid()::text, + 'alg', 'bcrypt', + 'hash', secret, + 'createdAt', to_char(COALESCE(created_at, now()) AT TIME ZONE 'UTC', 'YYYY-MM-DD"T"HH24:MI:SS"Z"') + )) +) +WHERE secret IS NOT NULL AND secret <> ''; + +ALTER TABLE oidc_clients DROP COLUMN secret; diff --git a/backend/resources/migrations/sqlite/20260807120000_oidc_client_multiple_secrets.down.sql b/backend/resources/migrations/sqlite/20260807120000_oidc_client_multiple_secrets.down.sql new file mode 100644 index 00000000..e0ef3446 --- /dev/null +++ b/backend/resources/migrations/sqlite/20260807120000_oidc_client_multiple_secrets.down.sql @@ -0,0 +1,28 @@ +PRAGMA foreign_keys= OFF; +BEGIN; + +ALTER TABLE oidc_clients ADD COLUMN secret TEXT; + +-- Only secrets that were migrated up from this same column can be restored, since the old column stores a bcrypt hash +-- Secrets created while multiple secrets were supported are hashed with SHA-256 and are dropped here +UPDATE oidc_clients +SET secret = ( + SELECT json_extract(value, '$.hash') + FROM json_each(json_extract(credentials, '$.secrets')) + WHERE json_extract(value, '$.alg') = 'bcrypt' + LIMIT 1 +) +WHERE credentials IS NOT NULL + AND credentials != '' + AND json_valid(credentials) + AND json_type(credentials, '$.secrets') = 'array'; + +UPDATE oidc_clients +SET credentials = json_remove(credentials, '$.secrets') +WHERE credentials IS NOT NULL + AND credentials != '' + AND json_valid(credentials) + AND json_type(credentials, '$.secrets') = 'array'; + +COMMIT; +PRAGMA foreign_keys= ON; diff --git a/backend/resources/migrations/sqlite/20260807120000_oidc_client_multiple_secrets.up.sql b/backend/resources/migrations/sqlite/20260807120000_oidc_client_multiple_secrets.up.sql new file mode 100644 index 00000000..8af7223e --- /dev/null +++ b/backend/resources/migrations/sqlite/20260807120000_oidc_client_multiple_secrets.up.sql @@ -0,0 +1,31 @@ +PRAGMA foreign_keys= OFF; +BEGIN; + +-- Move the single client secret into the credentials document, as the only entry of the new "secrets" array +-- Migrated secrets keep their bcrypt hash because the secret's value is not recoverable, and they never expire so that existing integrations keep working +UPDATE oidc_clients +SET credentials = json_set( + COALESCE(NULLIF(credentials, ''), '{}'), + '$.secrets', + json_array(json_object( + -- Generate a UUID + 'id', lower( + hex(randomblob(4)) || '-' || + hex(randomblob(2)) || '-4' || + substr(hex(randomblob(2)), 2) || '-' || + substr('89ab', abs(random()) % 4 + 1, 1) || substr(hex(randomblob(2)), 2) || '-' || + hex(randomblob(6)) + ), + 'alg', 'bcrypt', + 'hash', secret, + 'createdAt', strftime('%Y-%m-%dT%H:%M:%SZ', COALESCE(created_at, strftime('%s', 'now')), 'unixepoch') + )) +) +WHERE secret IS NOT NULL + AND secret != '' + AND (credentials IS NULL OR credentials = '' OR json_valid(credentials)); + +ALTER TABLE oidc_clients DROP COLUMN secret; + +COMMIT; +PRAGMA foreign_keys= ON; diff --git a/frontend/messages/en.json b/frontend/messages/en.json index f4d46cf6..f4fd9142 100644 --- a/frontend/messages/en.json +++ b/frontend/messages/en.json @@ -331,8 +331,6 @@ "cimd_url_allowlist_description": "Restrict which client ID metadata document URLs are accepted. {#link href=|https://pocket-id.org/docs/advanced/callback-url-wildcards|}Wildcards{/link} are supported. An empty list blocks all URLs.", "refresh": "Refresh", "oidc_client_metadata_refreshed_successfully": "Client metadata document refreshed successfully", - "create_new_client_secret": "Create new client secret", - "are_you_sure_you_want_to_create_a_new_client_secret": "Are you sure you want to create a new client secret? The old one will be invalidated.", "generate": "Generate", "new_client_secret_created_successfully": "New client secret created successfully", "oidc_client_name": "OIDC Client {name}", @@ -615,5 +613,17 @@ "or": "or", "visit_and_enter": "Visit {url} and enter:", "oidc": "OIDC", - "device_login_request_expired": "Your QR-Code has expired. Please start the sign-in process again." + "device_login_request_expired": "Your QR-Code has expired. Please start the sign-in process again.", + "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.", + "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", + "no_expiration": "No expiration", + "days_count": "{count} days", + "client_secrets_limit_reached": "An app cannot have more than 20 client secrets.", + "delete_client_secret": "Delete client secret", + "are_you_sure_you_want_to_delete_this_client_secret": "Are you sure you want to delete this client secret? Apps using it will no longer be able to authenticate.", + "client_secret_deleted_successfully": "Client secret deleted successfully" } diff --git a/frontend/src/lib/services/oidc-service.ts b/frontend/src/lib/services/oidc-service.ts index 5234b85a..97103a39 100644 --- a/frontend/src/lib/services/oidc-service.ts +++ b/frontend/src/lib/services/oidc-service.ts @@ -8,6 +8,8 @@ import type { OidcClient, OidcClientCreate, OidcClientMetaData, + OidcClientSecret, + OidcClientSecretCreated, OidcClientUpdate, OidcClientWithAllowedUserGroups, OidcClientWithAllowedUserGroupsCount, @@ -88,8 +90,20 @@ class OidcService extends APIService { cachedOidcClientLogo.bustCache(id, light); }; - createClientSecret = async (id: string) => - (await this.api.post(`/oidc/clients/${encodeClientIdParam(id)}/secret`)).data.secret as string; + listClientSecrets = async (id: string) => + (await this.api.get(`/oidc/clients/${encodeClientIdParam(id)}/secrets`)) + .data as OidcClientSecret[]; + + createClientSecret = async (id: string, expiresAt: Date | null) => + ( + await this.api.post(`/oidc/clients/${encodeClientIdParam(id)}/secrets`, { + expiresAt: expiresAt?.toISOString() ?? null + }) + ).data as OidcClientSecretCreated; + + deleteClientSecret = async (id: string, secretId: string) => { + await this.api.delete(`/oidc/clients/${encodeClientIdParam(id)}/secrets/${secretId}`); + }; updateAllowedUserGroups = async (id: string, userGroupIds: string[]) => { const res = await this.api.put(`/oidc/clients/${encodeClientIdParam(id)}/allowed-user-groups`, { diff --git a/frontend/src/lib/stores/client-secret-store.ts b/frontend/src/lib/stores/client-secret-store.ts index f860c258..162eda5d 100644 --- a/frontend/src/lib/stores/client-secret-store.ts +++ b/frontend/src/lib/stores/client-secret-store.ts @@ -1,17 +1,28 @@ import { writable } from 'svelte/store'; -const clientSecretStore = writable(null); +// 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>({}); -const set = (user: string) => { - clientSecretStore.set(user); +const set = (secretId: string, secret: string) => { + clientSecretStore.update((secrets) => ({ ...secrets, [secretId]: secret })); +}; + +const remove = (secretId: string) => { + clientSecretStore.update((secrets) => { + const remaining = { ...secrets }; + delete remaining[secretId]; + return remaining; + }); }; const clear = () => { - clientSecretStore.set(null); + clientSecretStore.set({}); }; export default { subscribe: clientSecretStore.subscribe, set, + remove, clear }; diff --git a/frontend/src/lib/types/oidc.type.ts b/frontend/src/lib/types/oidc.type.ts index f7463454..4df0dacc 100644 --- a/frontend/src/lib/types/oidc.type.ts +++ b/frontend/src/lib/types/oidc.type.ts @@ -21,8 +21,23 @@ export type OidcClientFederatedIdentity = { replayProtection: boolean; }; +export type OidcClientSecret = { + id: string; + // The first characters of the secret, empty for secrets created before Pocket ID supported multiple secrets + prefix: string; + createdAt: string; + expiresAt: string | null; + isActive: boolean; +}; + +// The clear-text value of a secret is only returned when it is created and cannot be retrieved afterwards +export type OidcClientSecretCreated = OidcClientSecret & { + secret: string; +}; + export type OidcClientCredentials = { federatedIdentities: OidcClientFederatedIdentity[]; + secrets: OidcClientSecret[]; }; export type OidcDiscoveryConfiguration = { diff --git a/frontend/src/routes/settings/admin/oidc-clients/+page.svelte b/frontend/src/routes/settings/admin/oidc-clients/+page.svelte index e75c0ecd..31fefa17 100644 --- a/frontend/src/routes/settings/admin/oidc-clients/+page.svelte +++ b/frontend/src/routes/settings/admin/oidc-clients/+page.svelte @@ -32,10 +32,7 @@ : Promise.resolve(); await Promise.all([logoPromise, darkLogoPromise]); - if (!createdClient.isPublic) { - const clientSecret = await oidcService.createClientSecret(createdClient.id); - clientSecretStore.set(clientSecret); - } + // 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()); return true; 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 92ca5bfd..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,17 +16,14 @@ import type { OidcClientCreateWithLogo, OidcClientCredentials, + OidcClientFederatedIdentity, + OidcClientSecret, OidcClientTokenLifetimes } from '$lib/types/oidc.type'; import type { ScimServiceProviderCreate } from '$lib/types/scim.type'; import { cachedOidcClientLogo } from '$lib/utils/cached-image-util'; import { axiosErrorToast } from '$lib/utils/error-util'; - import { - LucideChevronLeft, - LucideInfo, - LucideRefreshCcw, - LucideTriangleAlert - } from '@lucide/svelte'; + import { LucideChevronLeft, LucideInfo, LucideTriangleAlert } from '@lucide/svelte'; import { toast } from 'svelte-sonner'; import { slide } from 'svelte/transition'; import { backNavigate } from '../../users/navigate-back-util'; @@ -34,6 +31,7 @@ import OidcClientPreviewModal from '../oidc-client-preview-modal.svelte'; import ApiAccessCard from './api-access-card.svelte'; import OidcClientFederatedCredentialsCard from './oidc-client-federated-credentials-card.svelte'; + import OidcClientSecretsCard from './oidc-client-secrets-card.svelte'; import OidcClientTokenLifetimesCard from './oidc-client-token-lifetimes-card.svelte'; import ScimResourceProviderForm from './scim-resource-provider-form.svelte'; @@ -43,6 +41,9 @@ allowedUserGroupIds: data.client.allowedUserGroups.map((g) => g.id) }); + // Secrets are managed by their own endpoints, so they are kept out of the client object that the forms below submit + let clientSecrets = $state(data.client.credentials?.secrets ?? []); + let scimServiceProvider = $state(data.scimServiceProvider); let showAllDetails = $state(false); let showPreview = $state(false); @@ -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; @@ -174,26 +177,6 @@ }); } - async function createClientSecret() { - openConfirmDialog({ - title: m.create_new_client_secret(), - message: m.are_you_sure_you_want_to_create_a_new_client_secret(), - confirm: { - label: m.generate(), - destructive: true, - action: async () => { - try { - const clientSecret = await oidcService.createClientSecret(client.id); - clientSecretStore.set(clientSecret); - toast.success(m.new_client_secret_created_successfully()); - } catch (e) { - axiosErrorToast(e); - } - } - } - }); - } - async function updateUserGroupClients(allowedGroups: string[]) { await oidcService .updateAllowedUserGroups(client.id, allowedGroups) @@ -281,6 +264,7 @@ {/if} + {m.credentials()} {m.api_access()} {m.scim_provisioning()} {m.oidc_data_preview()} @@ -302,32 +286,6 @@ - {#if !client.isPublic} -
- {m.client_secret()} - {#if $clientSecretStore} - - - {$clientSecretStore} - - - {:else} -
- •••••••••••••••••••••••••••••••• - -
- {/if} -
- {/if} {#if showAllDetails}
{#each Object.entries(setupDetails) as [key, value] (key)} @@ -359,6 +317,10 @@ + + + + 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/[id]/oidc-client-secrets-card.svelte b/frontend/src/routes/settings/admin/oidc-clients/[id]/oidc-client-secrets-card.svelte new file mode 100644 index 00000000..c054f082 --- /dev/null +++ b/frontend/src/routes/settings/admin/oidc-clients/[id]/oidc-client-secrets-card.svelte @@ -0,0 +1,195 @@ + + + + + {m.client_secrets()} + {m.client_secrets_description()} + + + {#if client.isPublic} +

{m.public_clients_cannot_have_secrets()}

+ {:else} + {#if secrets.length > 0} + + + + {m.client_secret()} + {m.created()} + {m.expires_at()} + + + + + {#each secrets as secret (secret.id)} + {@const revealed = $clientSecretStore[secret.id]} + + + {#if revealed} + + + {revealed} + + + {:else} + {secret.prefix}{'•'.repeat(8)} + {/if} + {#if !secret.isActive} + {m.expired()} + {/if} + + {formatDate(secret.createdAt)} + {formatDate(secret.expiresAt)} + + + + + {m.toggle_menu()} + + + deleteSecret(secret)}> + + {m.delete()} + + + + + + {/each} + + + {:else} +

{m.no_client_secrets_yet()}

+ {/if} + +
+ + + {selectedExpirationLabel} + + + {#each expirationOptions as option (option.value)} + {option.label} + {/each} + + + +
+ {#if limitReached} +

+ {m.client_secrets_limit_reached()} +

+ {/if} + {/if} + + 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, diff --git a/tests/resources/export/database.json b/tests/resources/export/database.json index b27915c8..f96fad02 100644 --- a/tests/resources/export/database.json +++ b/tests/resources/export/database.json @@ -1,6 +1,6 @@ { "provider": "sqlite", - "version": 20260802120000, + "version": 20260807120000, "tableOrder": [ "users", "user_groups", @@ -92,7 +92,7 @@ "client_type": "standard", "created_at": "2025-11-25T12:39:02Z", "created_by_id": "f4b89dc2-62fb-46bf-9f5f-c34f4eafe93e", - "credentials": "e30=", + "credentials": "eyJzZWNyZXRzIjpbeyJpZCI6IjJmMWI4ZjFhLTFkM2UtNGYwYy05YzFhLTAwMDAwMDAwMDAwMSIsImFsZyI6InNoYTI1NiIsImhhc2giOiJkZjMzMWMxOGRhMTM0NDMwOTRkYzJjODQ5NzFkNWNlZmIzNDY0ZTk3OTk5YzM5YmMwMzg0NWVhY2NjNjE3MGEyIiwicHJlZml4IjoidzJtVSIsImNyZWF0ZWRBdCI6IjIwMjUtMTEtMjVUMTI6Mzk6MDJaIn1dfQ==", "dark_image_type": null, "id": "3654a746-35d4-4321-ac61-0bdcff2b4055", "image_type": "png", @@ -109,7 +109,6 @@ "requires_pushed_authorization_requests": false, "requires_reauthentication": false, "refresh_token_duration_minutes": 43200, - "secret": "$2a$10$9dypwot8nGuCjT6wQWWpJOckZfRprhe2EkwpKizxS/fpVHrOLEJHC", "skip_consent": false }, { @@ -118,7 +117,7 @@ "client_type": "standard", "created_at": "2025-11-25T12:39:02Z", "created_by_id": "1cd19686-f9a6-43f4-a41f-14a0bf5b4036", - "credentials": "e30=", + "credentials": "eyJzZWNyZXRzIjpbeyJpZCI6IjJmMWI4ZjFhLTFkM2UtNGYwYy05YzFhLTAwMDAwMDAwMDAwMiIsImFsZyI6InNoYTI1NiIsImhhc2giOiI4ODNiNzFjZjk1MWZlODk3ZTEwMjAxMTdiMTZjOGE2ZTY0MjllMWI5NGViZWY3ZGQ5NjM0OGJjZjc4ZThhMjI0IiwicHJlZml4IjoiUFlqciIsImNyZWF0ZWRBdCI6IjIwMjUtMTEtMjVUMTI6Mzk6MDJaIn1dfQ==", "dark_image_type": null, "id": "606c7782-f2b1-49e5-8ea9-26eb1b06d018", "image_type": null, @@ -135,7 +134,6 @@ "requires_pushed_authorization_requests": false, "requires_reauthentication": false, "refresh_token_duration_minutes": 43200, - "secret": "$2a$10$Ak.FP8riD1ssy2AGGbG.gOpnp/rBpymd74j0nxNMtW0GG1Lb4gzxe", "skip_consent": false }, { @@ -144,7 +142,7 @@ "client_type": "standard", "created_at": "2025-11-25T12:39:02Z", "created_by_id": "f4b89dc2-62fb-46bf-9f5f-c34f4eafe93e", - "credentials": "e30=", + "credentials": "eyJzZWNyZXRzIjpbeyJpZCI6IjJmMWI4ZjFhLTFkM2UtNGYwYy05YzFhLTAwMDAwMDAwMDAwMyIsImFsZyI6InNoYTI1NiIsImhhc2giOiIzZWY5MzU5YWVlNmRiYjNlNGNhOTYxMTQxZGY3ZjZiYTA2ZGJiOTE5MDBiMGM4MWZhNjk5NjZlNDU4ZjA3MGQ2IiwicHJlZml4IjoibjRWZiIsImNyZWF0ZWRBdCI6IjIwMjUtMTEtMjVUMTI6Mzk6MDJaIn1dfQ==", "dark_image_type": null, "id": "7c21a609-96b5-4011-9900-272b8d31a9d1", "image_type": null, @@ -161,7 +159,6 @@ "requires_pushed_authorization_requests": false, "requires_reauthentication": false, "refresh_token_duration_minutes": 43200, - "secret": "$2a$10$xcRReBsvkI1XI6FG8xu/pOgzeF00bH5Wy4d/NThwcdi3ZBpVq/B9a", "skip_consent": false }, { @@ -170,7 +167,7 @@ "client_type": "standard", "created_at": "2025-11-25T12:39:02Z", "created_by_id": "1cd19686-f9a6-43f4-a41f-14a0bf5b4036", - "credentials": "eyJmZWRlcmF0ZWRJZGVudGl0aWVzIjpbeyJpc3N1ZXIiOiJodHRwczovL2V4dGVybmFsLWlkcC5sb2NhbCIsInN1YmplY3QiOiJjNDgyMzJmZi1mZjY1LTQ1ZWQtYWU5Ni03YWZhOGE5YjQ0M2IiLCJhdWRpZW5jZSI6ImFwaTovL1BvY2tldElEIiwiandrcyI6Imh0dHA6Ly9sb2NhbGhvc3Q6MTQxMS9hcGkvZXh0ZXJuYWxpZHAvandrcy5qc29uIn1dfQ==", + "credentials": "eyJmZWRlcmF0ZWRJZGVudGl0aWVzIjpbeyJpc3N1ZXIiOiJodHRwczovL2V4dGVybmFsLWlkcC5sb2NhbCIsInN1YmplY3QiOiJjNDgyMzJmZi1mZjY1LTQ1ZWQtYWU5Ni03YWZhOGE5YjQ0M2IiLCJhdWRpZW5jZSI6ImFwaTovL1BvY2tldElEIiwiandrcyI6Imh0dHA6Ly9sb2NhbGhvc3Q6MTQxMS9hcGkvZXh0ZXJuYWxpZHAvandrcy5qc29uIn1dLCJzZWNyZXRzIjpbeyJpZCI6IjJmMWI4ZjFhLTFkM2UtNGYwYy05YzFhLTAwMDAwMDAwMDAwNCIsImFsZyI6InNoYTI1NiIsImhhc2giOiI4ODNiNzFjZjk1MWZlODk3ZTEwMjAxMTdiMTZjOGE2ZTY0MjllMWI5NGViZWY3ZGQ5NjM0OGJjZjc4ZThhMjI0IiwicHJlZml4IjoiUFlqciIsImNyZWF0ZWRBdCI6IjIwMjUtMTEtMjVUMTI6Mzk6MDJaIn1dfQ==", "dark_image_type": null, "id": "c48232ff-ff65-45ed-ae96-7afa8a9b443b", "image_type": null, @@ -187,7 +184,6 @@ "requires_pushed_authorization_requests": false, "requires_reauthentication": false, "refresh_token_duration_minutes": 43200, - "secret": "$2a$10$Ak.FP8riD1ssy2AGGbG.gOpnp/rBpymd74j0nxNMtW0GG1Lb4gzxe", "skip_consent": false }, { @@ -212,8 +208,8 @@ "requires_pushed_authorization_requests": false, "requires_reauthentication": false, "refresh_token_duration_minutes": 43200, - "secret": "$2a$10$h4wfa8gI7zavDAxwzSq1sOwYU4e8DwK1XZ8ZweNnY5KzlJ3Iz.qdK", - "skip_consent": false + "skip_consent": false, + "credentials": "eyJzZWNyZXRzIjpbeyJpZCI6IjJmMWI4ZjFhLTFkM2UtNGYwYy05YzFhLTAwMDAwMDAwMDAwNSIsImFsZyI6InNoYTI1NiIsImhhc2giOiJiZWM3ZjA4YTI4MDhlZmNkYmU2ZmIwYjFmM2EwZjFhMDFjNjYwMGQxMTc1MjYyOTNjMTlhOWRlZDNkOGVjMGZiIiwicHJlZml4IjoiblFiaSIsImNyZWF0ZWRBdCI6IjIwMjUtMTEtMjVUMTI6Mzk6MDJaIn1dfQ==" }, { "access_token_duration_minutes": 60, @@ -221,7 +217,7 @@ "client_type": "standard", "created_at": "2025-11-25T12:39:02Z", "created_by_id": "f4b89dc2-62fb-46bf-9f5f-c34f4eafe93e", - "credentials": "e30=", + "credentials": "eyJzZWNyZXRzIjpbeyJpZCI6IjJmMWI4ZjFhLTFkM2UtNGYwYy05YzFhLTAwMDAwMDAwMDAwNiIsImFsZyI6InNoYTI1NiIsImhhc2giOiJkZjMzMWMxOGRhMTM0NDMwOTRkYzJjODQ5NzFkNWNlZmIzNDY0ZTk3OTk5YzM5YmMwMzg0NWVhY2NjNjE3MGEyIiwicHJlZml4IjoidzJtVSIsImNyZWF0ZWRBdCI6IjIwMjUtMTEtMjVUMTI6Mzk6MDJaIn1dfQ==", "dark_image_type": null, "id": "a1b2c3d4-e5f6-7890-abcd-ef0000000001", "image_type": null, @@ -238,7 +234,6 @@ "requires_pushed_authorization_requests": false, "requires_reauthentication": false, "refresh_token_duration_minutes": 43200, - "secret": "$2a$10$9dypwot8nGuCjT6wQWWpJOckZfRprhe2EkwpKizxS/fpVHrOLEJHC", "skip_consent": false }, { @@ -247,7 +242,7 @@ "client_type": "standard", "created_at": "2025-11-25T12:39:02Z", "created_by_id": "f4b89dc2-62fb-46bf-9f5f-c34f4eafe93e", - "credentials": "e30=", + "credentials": "eyJzZWNyZXRzIjpbeyJpZCI6IjJmMWI4ZjFhLTFkM2UtNGYwYy05YzFhLTAwMDAwMDAwMDAwNyIsImFsZyI6InNoYTI1NiIsImhhc2giOiJkZjMzMWMxOGRhMTM0NDMwOTRkYzJjODQ5NzFkNWNlZmIzNDY0ZTk3OTk5YzM5YmMwMzg0NWVhY2NjNjE3MGEyIiwicHJlZml4IjoidzJtVSIsImNyZWF0ZWRBdCI6IjIwMjUtMTEtMjVUMTI6Mzk6MDJaIn1dfQ==", "dark_image_type": null, "id": "e1f2a3b4-c5d6-7890-abcd-ef0000000002", "image_type": null, @@ -264,7 +259,6 @@ "requires_pushed_authorization_requests": false, "requires_reauthentication": false, "refresh_token_duration_minutes": 43200, - "secret": "$2a$10$9dypwot8nGuCjT6wQWWpJOckZfRprhe2EkwpKizxS/fpVHrOLEJHC", "skip_consent": true } ], diff --git a/tests/specs/oidc-client-settings.spec.ts b/tests/specs/oidc-client-settings.spec.ts index af4fe6d0..c3c1e1fd 100644 --- a/tests/specs/oidc-client-settings.spec.ts +++ b/tests/specs/oidc-client-settings.spec.ts @@ -1,6 +1,7 @@ import test, { expect, Page } from '@playwright/test'; import { oidcClients, userGroups } from '../data'; import { cleanupBackend } from '../utils/cleanup.util'; +import * as oidcUtil from '../utils/oidc.util'; test.beforeEach(async () => await cleanupBackend()); @@ -37,7 +38,6 @@ test.describe('Create OIDC client', () => { ); const resolvedClientId = (await page.getByTestId('client-id').innerText()).trim(); - const clientSecret = (await page.getByTestId('client-secret').innerText()).trim(); if (clientId) { expect(resolvedClientId).toBe(clientId); @@ -45,8 +45,6 @@ test.describe('Create OIDC client', () => { expect(resolvedClientId).toMatch(/^[\w-]{36}$/); } - expect(clientSecret).toMatch(/^\w{32}$/); - await expect(page.getByLabel('Name')).toHaveValue(oidcClient.name); await expect(page.getByLabel('Description')).toHaveValue(oidcClient.description); await expect(page.getByTestId('callback-url-1')).toHaveValue(oidcClient.callbackUrl); @@ -166,7 +164,7 @@ test('Update OIDC client token lifetimes', async ({ page }) => { test('Update OIDC client federated credentials', async ({ page }) => { const client = oidcClients.nextcloud; - await page.goto(`/settings/admin/oidc-clients/${client.id}`); + await page.goto(`/settings/admin/oidc-clients/${client.id}#credentials`); const card = page.getByTestId('federated-credentials-card'); await card.getByRole('button', { name: 'Create', exact: true }).click(); @@ -188,6 +186,7 @@ test('Update OIDC client federated credentials', async ({ page }) => { await expect(card.getByLabel('Audience')).toHaveValue('https://pocket-id.example.com'); // Saving the main client form must preserve credentials managed by the separate card + await page.locator('[role="tab"][data-value="general"]').click(); const description = page.getByLabel('Description'); await description.fill('Updated without replacing federated credentials'); const clientForm = description.locator('xpath=ancestor::form'); @@ -199,21 +198,87 @@ test('Update OIDC client federated credentials', async ({ page }) => { await clientForm.getByRole('button', { name: 'Save' }).click(); expect((await formUpdate).ok()).toBeTruthy(); - await page.reload(); + await page.goto(`/settings/admin/oidc-clients/${client.id}#credentials`); await expect(card.getByLabel('Issuer')).toHaveValue('https://issuer.example.com'); }); -test('Create new OIDC client secret', async ({ page }) => { +test('Create and delete OIDC client secrets', async ({ page }) => { const oidcClient = oidcClients.nextcloud; - await page.goto(`/settings/admin/oidc-clients/${oidcClient.id}`); + await page.goto(`/settings/admin/oidc-clients/${oidcClient.id}#credentials`); - await page.getByLabel('Create new client secret').click(); - await page.getByRole('button', { name: 'Generate' }).click(); + const card = page.getByTestId('client-secrets-card'); + // The seeded client already has the secret the other tests authenticate with + await expect(card.getByTestId('client-secret-row')).toHaveCount(1); + + await card.getByRole('button', { name: 'Add client secret' }).click(); + await expect(page.locator('[data-type="success"]')).toHaveText( + 'New client secret created successfully' + ); + + // The new secret is the only one whose value is shown in full, and only until the page is left + await expect(card.getByTestId('client-secret-row')).toHaveCount(2); + const createdSecret = (await card.getByTestId('client-secret').nth(1).innerText()).trim(); + expect(createdSecret).toMatch(/^\w{32}$/); + + // Both secrets authenticate the client, so it can be rotated without downtime + for (const secret of [oidcClient.secret, createdSecret]) { + const res = await oidcUtil.exchangeCode(page, { + grant_type: 'client_credentials', + client_id: oidcClient.id, + client_secret: secret + }); + expect(res.access_token).toBeTruthy(); + } + + // After a reload only the stored prefix is left + await page.reload(); + await expect(card.getByTestId('client-secret').nth(1)).toHaveText( + `${createdSecret.substring(0, 4)}••••••••` + ); + + await card + .getByTestId('client-secret-row') + .nth(1) + .getByRole('button', { name: 'Toggle menu' }) + .click(); + await page.getByRole('menuitem', { name: 'Delete' }).click(); + await page.getByRole('button', { name: 'Delete' }).click(); + await expect(page.locator('[data-type="success"]')).toHaveText( + 'Client secret deleted successfully' + ); + await expect(card.getByTestId('client-secret-row')).toHaveCount(1); + + // The deleted secret can no longer authenticate the client + const res = await oidcUtil.exchangeCode(page, { + grant_type: 'client_credentials', + client_id: oidcClient.id, + client_secret: createdSecret + }); + expect(res.access_token).toBeFalsy(); +}); + +test('Client secrets can be created with an expiration', async ({ page }) => { + const oidcClient = oidcClients.nextcloud; + await page.goto(`/settings/admin/oidc-clients/${oidcClient.id}#credentials`); + + const card = page.getByTestId('client-secrets-card'); + await card.getByRole('button', { name: 'Expiration' }).click(); + await page.getByRole('option', { name: '90 days' }).click(); + await card.getByRole('button', { name: 'Add client secret' }).click(); await expect(page.locator('[data-type="success"]')).toHaveText( 'New client secret created successfully' ); - expect((await page.getByTestId('client-secret').textContent())?.length).toBe(32); + + const secrets = await page.request + .get(`/api/oidc/clients/${oidcClient.id}/secrets`) + .then((r) => r.json()); + expect(secrets).toHaveLength(2); + + const expiresAt = new Date(secrets[1].expiresAt).getTime(); + const expected = Date.now() + 90 * 24 * 60 * 60 * 1000; + expect(Math.abs(expiresAt - expected)).toBeLessThan(5 * 60 * 1000); + expect(secrets[1].isActive).toBe(true); }); test('Delete OIDC client', async ({ page }) => {