diff --git a/frontend/src/pages/authorize-page.tsx b/frontend/src/pages/authorize-page.tsx index 17aa3df7e..730e5f27b 100644 --- a/frontend/src/pages/authorize-page.tsx +++ b/frontend/src/pages/authorize-page.tsx @@ -190,8 +190,7 @@ export const AuthorizePage = () => { diff --git a/internal/assets/migrations/postgres/000003_oidc_consent.down.sql b/internal/assets/migrations/postgres/000003_oidc_consent.down.sql new file mode 100644 index 000000000..f3d144d87 --- /dev/null +++ b/internal/assets/migrations/postgres/000003_oidc_consent.down.sql @@ -0,0 +1 @@ +DROP TABLE IF EXISTS "oidc_consents"; \ No newline at end of file diff --git a/internal/assets/migrations/postgres/000003_oidc_consent.up.sql b/internal/assets/migrations/postgres/000003_oidc_consent.up.sql new file mode 100644 index 000000000..a6ff3b65f --- /dev/null +++ b/internal/assets/migrations/postgres/000003_oidc_consent.up.sql @@ -0,0 +1,7 @@ +CREATE TABLE IF NOT EXISTS "oidc_consents" ( + "username" TEXT NOT NULL, + "client_id" TEXT NOT NULL, + "scope" TEXT NOT NULL, + "created_at" BIGINT NOT NULL, + PRIMARY KEY ("username", "client_id") +); diff --git a/internal/assets/migrations/sqlite/000011_oidc_consent.down.sql b/internal/assets/migrations/sqlite/000011_oidc_consent.down.sql new file mode 100644 index 000000000..f3d144d87 --- /dev/null +++ b/internal/assets/migrations/sqlite/000011_oidc_consent.down.sql @@ -0,0 +1 @@ +DROP TABLE IF EXISTS "oidc_consents"; \ No newline at end of file diff --git a/internal/assets/migrations/sqlite/000011_oidc_consent.up.sql b/internal/assets/migrations/sqlite/000011_oidc_consent.up.sql new file mode 100644 index 000000000..6d67a89cc --- /dev/null +++ b/internal/assets/migrations/sqlite/000011_oidc_consent.up.sql @@ -0,0 +1,7 @@ +CREATE TABLE IF NOT EXISTS "oidc_consents" ( + "username" TEXT NOT NULL, + "client_id" TEXT NOT NULL, + "scope" TEXT NOT NULL, + "created_at" INTEGER NOT NULL, + PRIMARY KEY ("username", "client_id") +); diff --git a/internal/bootstrap/app_bootstrap.go b/internal/bootstrap/app_bootstrap.go index 5762b97a8..6fc954080 100644 --- a/internal/bootstrap/app_bootstrap.go +++ b/internal/bootstrap/app_bootstrap.go @@ -179,8 +179,6 @@ func (app *BootstrapApp) Setup() error { cookieId := strings.Split(app.runtime.UUID, "-")[0] // first 8 characters of the uuid should be good enough app.runtime.SessionCookieName = fmt.Sprintf("%s-%s", model.SessionCookieName, cookieId) - app.runtime.CSRFCookieName = fmt.Sprintf("%s-%s", model.CSRFCookieName, cookieId) - app.runtime.RedirectCookieName = fmt.Sprintf("%s-%s", model.RedirectCookieName, cookieId) app.runtime.OAuthSessionCookieName = fmt.Sprintf("%s-%s", model.OAuthSessionCookieName, cookieId) // database diff --git a/internal/controller/oidc_controller.go b/internal/controller/oidc_controller.go index 59856244d..9064135f5 100644 --- a/internal/controller/oidc_controller.go +++ b/internal/controller/oidc_controller.go @@ -242,6 +242,16 @@ func (controller *OIDCController) authorize(c *gin.Context) { } } + if userContext != nil && userContext.Authenticated && values.OIDCPrompt != service.OIDCPromptLogin { + consent, err := controller.oidc.GetOIDCConsent(c, userContext.GetUsername(), req.ClientID) + + if err != nil { + controller.log.App.Warn().Err(err).Msg("Failed to get OIDC consent") + } else if consent != nil && scopesGranted(consent.Scope, req.Scope) { + values.OIDCPrompt = service.OIDCPromptNone + } + } + queries, err := query.Values(values) if err != nil { @@ -320,6 +330,19 @@ func (controller *OIDCController) authorizeComplete(c *gin.Context) { return } + // Get the client + client, ok := controller.oidc.GetClient(authorizeReq.ClientID) + + if !ok { + controller.authorizeError(c, authorizeErrorParams{ + err: errors.New("client not found"), + reason: "Client not found", + reasonPublic: "The client is not configured", + json: true, + }) + return + } + // We no longer need the ticket controller.oidc.DeleteAuthorizeRequestTicket(req.Ticket) @@ -356,6 +379,11 @@ func (controller *OIDCController) authorizeComplete(c *gin.Context) { return } + // Store the consent granted by the user for this client + if _, err := controller.oidc.UpsertOIDCConsent(c, userContext.GetUsername(), authorizeReq.Scope, client.ClientID); err != nil { + controller.log.App.Warn().Err(err).Msg("Failed to store OIDC consent") + } + q := cu.Query() q.Set("code", code) @@ -756,3 +784,20 @@ func (controller *OIDCController) resolveNormalParams(c *gin.Context) (*service. return &req, nil } + +// scopesGranted reports whether every scope in requested is present in the +// space-separated granted scope string. +func scopesGranted(granted, requested string) bool { + grantedScopes := strings.Split(granted, " ") + + for _, scope := range strings.Split(requested, " ") { + if scope == "" { + continue + } + if !slices.Contains(grantedScopes, scope) { + return false + } + } + + return true +} diff --git a/internal/controller/oidc_controller_test.go b/internal/controller/oidc_controller_test.go index b22ddc547..d73ef34f2 100644 --- a/internal/controller/oidc_controller_test.go +++ b/internal/controller/oidc_controller_test.go @@ -170,6 +170,102 @@ func TestOIDCController(t *testing.T) { assert.Contains(t, location, "oidc_name="+url.QueryEscape("Test Client")) }, }, + { + description: "Authorize skips the consent screen when all requested scopes were already granted", + middlewares: []gin.HandlerFunc{authedUser}, + run: func(t *testing.T, router *gin.Engine, recorder *httptest.ResponseRecorder) { + _, err := store.UpsertOIDCConsent(ctx, repository.UpsertOIDCConsentParams{ + Username: "testuser", ClientID: "some-client-id", + Scope: "openid profile", CreatedAt: time.Now().Unix(), + }) + require.NoError(t, err) + + q := url.Values{} + q.Set("scope", "openid profile") + q.Set("response_type", "code") + q.Set("client_id", "some-client-id") + q.Set("redirect_uri", "https://test.example.com/callback") + + req := httptest.NewRequest("GET", "/authorize?"+q.Encode(), nil) + router.ServeHTTP(recorder, req) + + assert.Equal(t, http.StatusFound, recorder.Code) + location := recorder.Header().Get("Location") + assert.True(t, strings.HasPrefix(location, oidcService.GetIssuer()+"/oidc/authorize?")) + assert.Contains(t, location, "oidc_prompt=none") + }, + }, + { + description: "Authorize shows the consent screen when a new scope is requested", + middlewares: []gin.HandlerFunc{authedUser}, + run: func(t *testing.T, router *gin.Engine, recorder *httptest.ResponseRecorder) { + _, err := store.UpsertOIDCConsent(ctx, repository.UpsertOIDCConsentParams{ + Username: "testuser", ClientID: "some-client-id", + Scope: "openid", CreatedAt: time.Now().Unix(), + }) + require.NoError(t, err) + + q := url.Values{} + q.Set("scope", "openid profile") + q.Set("response_type", "code") + q.Set("client_id", "some-client-id") + q.Set("redirect_uri", "https://test.example.com/callback") + + req := httptest.NewRequest("GET", "/authorize?"+q.Encode(), nil) + router.ServeHTTP(recorder, req) + + assert.Equal(t, http.StatusFound, recorder.Code) + location := recorder.Header().Get("Location") + assert.True(t, strings.HasPrefix(location, oidcService.GetIssuer()+"/oidc/authorize?")) + assert.NotContains(t, location, "oidc_prompt=none") + }, + }, + { + description: "Authorize skips the consent screen for a subset of already granted scopes", + middlewares: []gin.HandlerFunc{authedUser}, + run: func(t *testing.T, router *gin.Engine, recorder *httptest.ResponseRecorder) { + _, err := store.UpsertOIDCConsent(ctx, repository.UpsertOIDCConsentParams{ + Username: "testuser", ClientID: "some-client-id", + Scope: "openid profile email", CreatedAt: time.Now().Unix(), + }) + require.NoError(t, err) + + q := url.Values{} + q.Set("scope", "openid profile") + q.Set("response_type", "code") + q.Set("client_id", "some-client-id") + q.Set("redirect_uri", "https://test.example.com/callback") + + req := httptest.NewRequest("GET", "/authorize?"+q.Encode(), nil) + router.ServeHTTP(recorder, req) + + assert.Equal(t, http.StatusFound, recorder.Code) + location := recorder.Header().Get("Location") + assert.True(t, strings.HasPrefix(location, oidcService.GetIssuer()+"/oidc/authorize?")) + assert.Contains(t, location, "oidc_prompt=none") + }, + }, + { + description: "Authorize shows the consent screen when no consent was granted yet", + middlewares: []gin.HandlerFunc{authedUser}, + run: func(t *testing.T, router *gin.Engine, recorder *httptest.ResponseRecorder) { + require.NoError(t, store.DeleteOIDCConsentByClientID(ctx, "some-client-id")) + + q := url.Values{} + q.Set("scope", "openid profile") + q.Set("response_type", "code") + q.Set("client_id", "some-client-id") + q.Set("redirect_uri", "https://test.example.com/callback") + + req := httptest.NewRequest("GET", "/authorize?"+q.Encode(), nil) + router.ServeHTTP(recorder, req) + + assert.Equal(t, http.StatusFound, recorder.Code) + location := recorder.Header().Get("Location") + assert.True(t, strings.HasPrefix(location, oidcService.GetIssuer()+"/oidc/authorize?")) + assert.NotContains(t, location, "oidc_prompt=none") + }, + }, { description: "Authorize redirects to error screen when the request object is invalid", run: func(t *testing.T, router *gin.Engine, recorder *httptest.ResponseRecorder) { diff --git a/internal/model/constants.go b/internal/model/constants.go index ff44a7299..7a743d981 100644 --- a/internal/model/constants.go +++ b/internal/model/constants.go @@ -20,8 +20,6 @@ var OverrideProviders = map[string]string{ var ReservedProviderNames = []string{"local", "ldap", "tailscale"} const SessionCookieName = "tinyauth-session" -const CSRFCookieName = "tinyauth-csrf" -const RedirectCookieName = "tinyauth-redirect" const OAuthSessionCookieName = "tinyauth-oauth" const GracefulShutdownTimeout = 5 // seconds diff --git a/internal/model/runtime.go b/internal/model/runtime.go index 920002655..668119528 100644 --- a/internal/model/runtime.go +++ b/internal/model/runtime.go @@ -5,8 +5,6 @@ type RuntimeConfig struct { UUID string CookieDomain string SessionCookieName string - CSRFCookieName string - RedirectCookieName string OAuthSessionCookieName string LocalUsers []LocalUser OAuthProviders map[string]OAuthServiceConfig diff --git a/internal/repository/memory/memory_test.go b/internal/repository/memory/memory_test.go index 558ed234f..c2ae4ca8d 100644 --- a/internal/repository/memory/memory_test.go +++ b/internal/repository/memory/memory_test.go @@ -277,6 +277,80 @@ func TestMemoryStore(t *testing.T) { assert.NoError(t, err) }, }, + { + description: "Upsert creates a consent for each user+client pair", + run: func(t *testing.T, s repository.Store) { + _, err := s.UpsertOIDCConsent(ctx, repository.UpsertOIDCConsentParams{ + Username: "alice", ClientID: "client-a", Scope: "openid profile", CreatedAt: 1, + }) + require.NoError(t, err) + _, err = s.UpsertOIDCConsent(ctx, repository.UpsertOIDCConsentParams{ + Username: "alice", ClientID: "client-b", Scope: "openid email", CreatedAt: 2, + }) + require.NoError(t, err) + + consents, err := s.ListOIDCConsents(ctx) + require.NoError(t, err) + assert.Len(t, consents, 2) + + gotA, err := s.GetOIDCConsentByUsernameAndClientID(ctx, repository.GetOIDCConsentByUsernameAndClientIDParams{Username: "alice", ClientID: "client-a"}) + require.NoError(t, err) + assert.Equal(t, "openid profile", gotA.Scope) + + gotB, err := s.GetOIDCConsentByUsernameAndClientID(ctx, repository.GetOIDCConsentByUsernameAndClientIDParams{Username: "alice", ClientID: "client-b"}) + require.NoError(t, err) + assert.Equal(t, "openid email", gotB.Scope) + }, + }, + { + description: "Upsert overwrites the same consent row", + run: func(t *testing.T, s repository.Store) { + _, err := s.UpsertOIDCConsent(ctx, repository.UpsertOIDCConsentParams{ + Username: "alice", ClientID: "client-a", Scope: "openid", CreatedAt: 1, + }) + require.NoError(t, err) + + _, err = s.UpsertOIDCConsent(ctx, repository.UpsertOIDCConsentParams{ + Username: "alice", ClientID: "client-a", Scope: "openid email", CreatedAt: 2, + }) + require.NoError(t, err) + + consents, err := s.ListOIDCConsents(ctx) + require.NoError(t, err) + assert.Len(t, consents, 1) + + got, err := s.GetOIDCConsentByUsernameAndClientID(ctx, repository.GetOIDCConsentByUsernameAndClientIDParams{Username: "alice", ClientID: "client-a"}) + require.NoError(t, err) + assert.Equal(t, "openid email", got.Scope) + }, + }, + { + description: "Get consent by username and client not found", + run: func(t *testing.T, s repository.Store) { + _, err := s.GetOIDCConsentByUsernameAndClientID(ctx, repository.GetOIDCConsentByUsernameAndClientIDParams{Username: "alice", ClientID: "client-a"}) + assert.ErrorIs(t, err, repository.ErrNotFound) + }, + }, + { + description: "Delete consent by client id", + run: func(t *testing.T, s repository.Store) { + _, err := s.UpsertOIDCConsent(ctx, repository.UpsertOIDCConsentParams{ + Username: "alice", ClientID: "client-a", Scope: "openid", CreatedAt: 1, + }) + require.NoError(t, err) + _, err = s.UpsertOIDCConsent(ctx, repository.UpsertOIDCConsentParams{ + Username: "alice", ClientID: "client-b", Scope: "openid", CreatedAt: 2, + }) + require.NoError(t, err) + + require.NoError(t, s.DeleteOIDCConsentByClientID(ctx, "client-a")) + + consents, err := s.ListOIDCConsents(ctx) + require.NoError(t, err) + assert.Len(t, consents, 1) + assert.Equal(t, "client-b", consents[0].ClientID) + }, + }, } for _, test := range tests { diff --git a/internal/repository/memory/oidc_queries.go b/internal/repository/memory/oidc_queries.go index 1ee81c8bf..e06a4346c 100644 --- a/internal/repository/memory/oidc_queries.go +++ b/internal/repository/memory/oidc_queries.go @@ -94,3 +94,46 @@ func (s *Store) DeleteExpiredOIDCSessions(_ context.Context, arg repository.Dele } return nil } + +func consentKey(username, clientID string) string { + return username + "\x00" + clientID +} + +func (s *Store) UpsertOIDCConsent(_ context.Context, arg repository.UpsertOIDCConsentParams) (repository.OidcConsent, error) { + s.mu.Lock() + defer s.mu.Unlock() + oc := repository.OidcConsent(arg) + s.oidcConsents[consentKey(arg.Username, arg.ClientID)] = oc + return oc, nil +} + +func (s *Store) GetOIDCConsentByUsernameAndClientID(_ context.Context, arg repository.GetOIDCConsentByUsernameAndClientIDParams) (repository.OidcConsent, error) { + s.mu.RLock() + defer s.mu.RUnlock() + oc, ok := s.oidcConsents[consentKey(arg.Username, arg.ClientID)] + if !ok { + return repository.OidcConsent{}, repository.ErrNotFound + } + return oc, nil +} + +func (s *Store) DeleteOIDCConsentByClientID(_ context.Context, clientID string) error { + s.mu.Lock() + defer s.mu.Unlock() + for key, oc := range s.oidcConsents { + if oc.ClientID == clientID { + delete(s.oidcConsents, key) + } + } + return nil +} + +func (s *Store) ListOIDCConsents(_ context.Context) ([]repository.OidcConsent, error) { + s.mu.RLock() + defer s.mu.RUnlock() + out := make([]repository.OidcConsent, 0, len(s.oidcConsents)) + for _, oc := range s.oidcConsents { + out = append(out, oc) + } + return out, nil +} diff --git a/internal/repository/memory/store.go b/internal/repository/memory/store.go index 684ddeb3e..5aa4750b4 100644 --- a/internal/repository/memory/store.go +++ b/internal/repository/memory/store.go @@ -12,6 +12,7 @@ type Store struct { mu sync.RWMutex sessions map[string]repository.Session oidcSessions map[string]repository.OidcSession + oidcConsents map[string]repository.OidcConsent } // New returns a new empty in-memory Store. @@ -19,5 +20,6 @@ func New() repository.Store { return &Store{ sessions: make(map[string]repository.Session), oidcSessions: make(map[string]repository.OidcSession), + oidcConsents: make(map[string]repository.OidcConsent), } } diff --git a/internal/repository/models.go b/internal/repository/models.go index 39538a000..9e356680a 100644 --- a/internal/repository/models.go +++ b/internal/repository/models.go @@ -84,3 +84,22 @@ type DeleteExpiredOIDCSessionsParams struct { TokenExpiresAt int64 RefreshTokenExpiresAt int64 } + +type OidcConsent struct { + Username string + ClientID string + Scope string + CreatedAt int64 +} + +type UpsertOIDCConsentParams struct { + Username string + ClientID string + Scope string + CreatedAt int64 +} + +type GetOIDCConsentByUsernameAndClientIDParams struct { + Username string + ClientID string +} diff --git a/internal/repository/postgres/models.go b/internal/repository/postgres/models.go index f957e1fde..ccf7ce62a 100644 --- a/internal/repository/postgres/models.go +++ b/internal/repository/postgres/models.go @@ -4,6 +4,13 @@ package postgres +type OidcConsent struct { + Username string + ClientID string + Scope string + CreatedAt int64 +} + type OidcSession struct { Sub string AccessTokenHash string diff --git a/internal/repository/postgres/oidc_queries.sql.go b/internal/repository/postgres/oidc_queries.sql.go index b5b9789c9..a2bb96314 100644 --- a/internal/repository/postgres/oidc_queries.sql.go +++ b/internal/repository/postgres/oidc_queries.sql.go @@ -80,6 +80,16 @@ func (q *Queries) DeleteExpiredOIDCSessions(ctx context.Context, arg DeleteExpir return err } +const deleteOIDCConsentByClientID = `-- name: DeleteOIDCConsentByClientID :exec +DELETE FROM "oidc_consents" +WHERE "client_id" = $1 +` + +func (q *Queries) DeleteOIDCConsentByClientID(ctx context.Context, clientID string) error { + _, err := q.db.ExecContext(ctx, deleteOIDCConsentByClientID, clientID) + return err +} + const deleteOIDCSessionBySub = `-- name: DeleteOIDCSessionBySub :exec DELETE FROM "oidc_sessions" WHERE "sub" = $1 @@ -90,6 +100,28 @@ func (q *Queries) DeleteOIDCSessionBySub(ctx context.Context, sub string) error return err } +const getOIDCConsentByUsernameAndClientID = `-- name: GetOIDCConsentByUsernameAndClientID :one +SELECT username, client_id, scope, created_at FROM "oidc_consents" +WHERE "username" = $1 AND "client_id" = $2 +` + +type GetOIDCConsentByUsernameAndClientIDParams struct { + Username string + ClientID string +} + +func (q *Queries) GetOIDCConsentByUsernameAndClientID(ctx context.Context, arg GetOIDCConsentByUsernameAndClientIDParams) (OidcConsent, error) { + row := q.db.QueryRowContext(ctx, getOIDCConsentByUsernameAndClientID, arg.Username, arg.ClientID) + var i OidcConsent + err := row.Scan( + &i.Username, + &i.ClientID, + &i.Scope, + &i.CreatedAt, + ) + return i, err +} + const getOIDCSessionByAccessTokenHash = `-- name: GetOIDCSessionByAccessTokenHash :one SELECT sub, access_token_hash, refresh_token_hash, scope, client_id, token_expires_at, refresh_token_expires_at, nonce, userinfo_json FROM "oidc_sessions" WHERE "access_token_hash" = $1 @@ -156,6 +188,38 @@ func (q *Queries) GetOIDCSessionBySub(ctx context.Context, sub string) (OidcSess return i, err } +const listOIDCConsents = `-- name: ListOIDCConsents :many +SELECT username, client_id, scope, created_at FROM "oidc_consents" +` + +func (q *Queries) ListOIDCConsents(ctx context.Context) ([]OidcConsent, error) { + rows, err := q.db.QueryContext(ctx, listOIDCConsents) + if err != nil { + return nil, err + } + defer rows.Close() + var items []OidcConsent + for rows.Next() { + var i OidcConsent + if err := rows.Scan( + &i.Username, + &i.ClientID, + &i.Scope, + &i.CreatedAt, + ); err != nil { + return nil, err + } + items = append(items, i) + } + if err := rows.Close(); err != nil { + return nil, err + } + if err := rows.Err(); err != nil { + return nil, err + } + return items, nil +} + const updateOIDCSession = `-- name: UpdateOIDCSession :one UPDATE "oidc_sessions" SET "access_token_hash" = $1, @@ -208,3 +272,43 @@ func (q *Queries) UpdateOIDCSession(ctx context.Context, arg UpdateOIDCSessionPa ) return i, err } + +const upsertOIDCConsent = `-- name: UpsertOIDCConsent :one +INSERT INTO "oidc_consents" ( + "username", + "client_id", + "scope", + "created_at" +) VALUES ( + $1, $2, $3, $4 +) +ON CONFLICT ("username", "client_id") +DO UPDATE SET + "scope" = excluded.scope, + "created_at" = excluded.created_at +RETURNING username, client_id, scope, created_at +` + +type UpsertOIDCConsentParams struct { + Username string + ClientID string + Scope string + CreatedAt int64 +} + +func (q *Queries) UpsertOIDCConsent(ctx context.Context, arg UpsertOIDCConsentParams) (OidcConsent, error) { + row := q.db.QueryRowContext(ctx, upsertOIDCConsent, + arg.Username, + arg.ClientID, + arg.Scope, + arg.CreatedAt, + ) + var i OidcConsent + err := row.Scan( + &i.Username, + &i.ClientID, + &i.Scope, + &i.CreatedAt, + ) + return i, err +} diff --git a/internal/repository/postgres/store.go b/internal/repository/postgres/store.go index b3e79c803..1857016a2 100644 --- a/internal/repository/postgres/store.go +++ b/internal/repository/postgres/store.go @@ -56,6 +56,10 @@ func (s *Store) DeleteExpiredSessions(ctx context.Context, expiry int64) error { return mapErr(s.q.DeleteExpiredSessions(ctx, expiry)) } +func (s *Store) DeleteOIDCConsentByClientID(ctx context.Context, clientID string) error { + return mapErr(s.q.DeleteOIDCConsentByClientID(ctx, clientID)) +} + func (s *Store) DeleteOIDCSessionBySub(ctx context.Context, sub string) error { return mapErr(s.q.DeleteOIDCSessionBySub(ctx, sub)) } @@ -64,6 +68,14 @@ func (s *Store) DeleteSession(ctx context.Context, uuid string) error { return mapErr(s.q.DeleteSession(ctx, uuid)) } +func (s *Store) GetOIDCConsentByUsernameAndClientID(ctx context.Context, arg repository.GetOIDCConsentByUsernameAndClientIDParams) (repository.OidcConsent, error) { + r, err := s.q.GetOIDCConsentByUsernameAndClientID(ctx, GetOIDCConsentByUsernameAndClientIDParams(arg)) + if err != nil { + return repository.OidcConsent{}, mapErr(err) + } + return repository.OidcConsent(r), nil +} + func (s *Store) GetOIDCSessionByAccessTokenHash(ctx context.Context, accessTokenHash string) (repository.OidcSession, error) { r, err := s.q.GetOIDCSessionByAccessTokenHash(ctx, accessTokenHash) if err != nil { @@ -96,6 +108,18 @@ func (s *Store) GetSession(ctx context.Context, uuid string) (repository.Session return repository.Session(r), nil } +func (s *Store) ListOIDCConsents(ctx context.Context) ([]repository.OidcConsent, error) { + rows, err := s.q.ListOIDCConsents(ctx) + if err != nil { + return nil, mapErr(err) + } + out := make([]repository.OidcConsent, len(rows)) + for i, row := range rows { + out[i] = repository.OidcConsent(row) + } + return out, nil +} + func (s *Store) UpdateOIDCSession(ctx context.Context, arg repository.UpdateOIDCSessionParams) (repository.OidcSession, error) { r, err := s.q.UpdateOIDCSession(ctx, UpdateOIDCSessionParams(arg)) if err != nil { @@ -111,3 +135,11 @@ func (s *Store) UpdateSession(ctx context.Context, arg repository.UpdateSessionP } return repository.Session(r), nil } + +func (s *Store) UpsertOIDCConsent(ctx context.Context, arg repository.UpsertOIDCConsentParams) (repository.OidcConsent, error) { + r, err := s.q.UpsertOIDCConsent(ctx, UpsertOIDCConsentParams(arg)) + if err != nil { + return repository.OidcConsent{}, mapErr(err) + } + return repository.OidcConsent(r), nil +} diff --git a/internal/repository/sqlite/models.go b/internal/repository/sqlite/models.go index 2ced8a2b3..f30ae6726 100644 --- a/internal/repository/sqlite/models.go +++ b/internal/repository/sqlite/models.go @@ -4,6 +4,13 @@ package sqlite +type OidcConsent struct { + Username string + ClientID string + Scope string + CreatedAt int64 +} + type OidcSession struct { Sub string AccessTokenHash string diff --git a/internal/repository/sqlite/oidc_queries.sql.go b/internal/repository/sqlite/oidc_queries.sql.go index a5aa08a8f..e574197a8 100644 --- a/internal/repository/sqlite/oidc_queries.sql.go +++ b/internal/repository/sqlite/oidc_queries.sql.go @@ -80,6 +80,16 @@ func (q *Queries) DeleteExpiredOIDCSessions(ctx context.Context, arg DeleteExpir return err } +const deleteOIDCConsentByClientID = `-- name: DeleteOIDCConsentByClientID :exec +DELETE FROM "oidc_consents" +WHERE "client_id" = ? +` + +func (q *Queries) DeleteOIDCConsentByClientID(ctx context.Context, clientID string) error { + _, err := q.db.ExecContext(ctx, deleteOIDCConsentByClientID, clientID) + return err +} + const deleteOIDCSessionBySub = `-- name: DeleteOIDCSessionBySub :exec DELETE FROM "oidc_sessions" WHERE "sub" = ? @@ -90,6 +100,28 @@ func (q *Queries) DeleteOIDCSessionBySub(ctx context.Context, sub string) error return err } +const getOIDCConsentByUsernameAndClientID = `-- name: GetOIDCConsentByUsernameAndClientID :one +SELECT username, client_id, scope, created_at FROM "oidc_consents" +WHERE "username" = ? AND "client_id" = ? +` + +type GetOIDCConsentByUsernameAndClientIDParams struct { + Username string + ClientID string +} + +func (q *Queries) GetOIDCConsentByUsernameAndClientID(ctx context.Context, arg GetOIDCConsentByUsernameAndClientIDParams) (OidcConsent, error) { + row := q.db.QueryRowContext(ctx, getOIDCConsentByUsernameAndClientID, arg.Username, arg.ClientID) + var i OidcConsent + err := row.Scan( + &i.Username, + &i.ClientID, + &i.Scope, + &i.CreatedAt, + ) + return i, err +} + const getOIDCSessionByAccessTokenHash = `-- name: GetOIDCSessionByAccessTokenHash :one SELECT sub, access_token_hash, refresh_token_hash, scope, client_id, token_expires_at, refresh_token_expires_at, nonce, userinfo_json FROM "oidc_sessions" WHERE "access_token_hash" = ? @@ -156,6 +188,38 @@ func (q *Queries) GetOIDCSessionBySub(ctx context.Context, sub string) (OidcSess return i, err } +const listOIDCConsents = `-- name: ListOIDCConsents :many +SELECT username, client_id, scope, created_at FROM "oidc_consents" +` + +func (q *Queries) ListOIDCConsents(ctx context.Context) ([]OidcConsent, error) { + rows, err := q.db.QueryContext(ctx, listOIDCConsents) + if err != nil { + return nil, err + } + defer rows.Close() + var items []OidcConsent + for rows.Next() { + var i OidcConsent + if err := rows.Scan( + &i.Username, + &i.ClientID, + &i.Scope, + &i.CreatedAt, + ); err != nil { + return nil, err + } + items = append(items, i) + } + if err := rows.Close(); err != nil { + return nil, err + } + if err := rows.Err(); err != nil { + return nil, err + } + return items, nil +} + const updateOIDCSession = `-- name: UpdateOIDCSession :one UPDATE "oidc_sessions" SET "access_token_hash" = ?, @@ -208,3 +272,43 @@ func (q *Queries) UpdateOIDCSession(ctx context.Context, arg UpdateOIDCSessionPa ) return i, err } + +const upsertOIDCConsent = `-- name: UpsertOIDCConsent :one +INSERT INTO "oidc_consents" ( + "username", + "client_id", + "scope", + "created_at" +) VALUES ( + ?, ?, ?, ? +) +ON CONFLICT ("username", "client_id") +DO UPDATE SET + "scope" = excluded.scope, + "created_at" = excluded.created_at +RETURNING username, client_id, scope, created_at +` + +type UpsertOIDCConsentParams struct { + Username string + ClientID string + Scope string + CreatedAt int64 +} + +func (q *Queries) UpsertOIDCConsent(ctx context.Context, arg UpsertOIDCConsentParams) (OidcConsent, error) { + row := q.db.QueryRowContext(ctx, upsertOIDCConsent, + arg.Username, + arg.ClientID, + arg.Scope, + arg.CreatedAt, + ) + var i OidcConsent + err := row.Scan( + &i.Username, + &i.ClientID, + &i.Scope, + &i.CreatedAt, + ) + return i, err +} diff --git a/internal/repository/sqlite/store.go b/internal/repository/sqlite/store.go index a567c8718..3c0d8a78c 100644 --- a/internal/repository/sqlite/store.go +++ b/internal/repository/sqlite/store.go @@ -56,6 +56,10 @@ func (s *Store) DeleteExpiredSessions(ctx context.Context, expiry int64) error { return mapErr(s.q.DeleteExpiredSessions(ctx, expiry)) } +func (s *Store) DeleteOIDCConsentByClientID(ctx context.Context, clientID string) error { + return mapErr(s.q.DeleteOIDCConsentByClientID(ctx, clientID)) +} + func (s *Store) DeleteOIDCSessionBySub(ctx context.Context, sub string) error { return mapErr(s.q.DeleteOIDCSessionBySub(ctx, sub)) } @@ -64,6 +68,14 @@ func (s *Store) DeleteSession(ctx context.Context, uuid string) error { return mapErr(s.q.DeleteSession(ctx, uuid)) } +func (s *Store) GetOIDCConsentByUsernameAndClientID(ctx context.Context, arg repository.GetOIDCConsentByUsernameAndClientIDParams) (repository.OidcConsent, error) { + r, err := s.q.GetOIDCConsentByUsernameAndClientID(ctx, GetOIDCConsentByUsernameAndClientIDParams(arg)) + if err != nil { + return repository.OidcConsent{}, mapErr(err) + } + return repository.OidcConsent(r), nil +} + func (s *Store) GetOIDCSessionByAccessTokenHash(ctx context.Context, accessTokenHash string) (repository.OidcSession, error) { r, err := s.q.GetOIDCSessionByAccessTokenHash(ctx, accessTokenHash) if err != nil { @@ -96,6 +108,18 @@ func (s *Store) GetSession(ctx context.Context, uuid string) (repository.Session return repository.Session(r), nil } +func (s *Store) ListOIDCConsents(ctx context.Context) ([]repository.OidcConsent, error) { + rows, err := s.q.ListOIDCConsents(ctx) + if err != nil { + return nil, mapErr(err) + } + out := make([]repository.OidcConsent, len(rows)) + for i, row := range rows { + out[i] = repository.OidcConsent(row) + } + return out, nil +} + func (s *Store) UpdateOIDCSession(ctx context.Context, arg repository.UpdateOIDCSessionParams) (repository.OidcSession, error) { r, err := s.q.UpdateOIDCSession(ctx, UpdateOIDCSessionParams(arg)) if err != nil { @@ -111,3 +135,11 @@ func (s *Store) UpdateSession(ctx context.Context, arg repository.UpdateSessionP } return repository.Session(r), nil } + +func (s *Store) UpsertOIDCConsent(ctx context.Context, arg repository.UpsertOIDCConsentParams) (repository.OidcConsent, error) { + r, err := s.q.UpsertOIDCConsent(ctx, UpsertOIDCConsentParams(arg)) + if err != nil { + return repository.OidcConsent{}, mapErr(err) + } + return repository.OidcConsent(r), nil +} diff --git a/internal/repository/store.go b/internal/repository/store.go index abd70bd34..4291cd468 100644 --- a/internal/repository/store.go +++ b/internal/repository/store.go @@ -27,4 +27,10 @@ type Store interface { GetOIDCSessionByRefreshTokenHash(ctx context.Context, refreshTokenHash string) (OidcSession, error) GetOIDCSessionBySub(ctx context.Context, sub string) (OidcSession, error) UpdateOIDCSession(ctx context.Context, arg UpdateOIDCSessionParams) (OidcSession, error) + + // OIDC Consents + UpsertOIDCConsent(ctx context.Context, arg UpsertOIDCConsentParams) (OidcConsent, error) + GetOIDCConsentByUsernameAndClientID(ctx context.Context, arg GetOIDCConsentByUsernameAndClientIDParams) (OidcConsent, error) + DeleteOIDCConsentByClientID(ctx context.Context, clientID string) error + ListOIDCConsents(ctx context.Context) ([]OidcConsent, error) } diff --git a/internal/service/oidc_service.go b/internal/service/oidc_service.go index a95b6bc53..3848a6c9c 100644 --- a/internal/service/oidc_service.go +++ b/internal/service/oidc_service.go @@ -16,6 +16,7 @@ import ( "os" "path/filepath" "strings" + "sync" "time" "slices" @@ -163,6 +164,10 @@ type OIDCService struct { usedCode *cache.CacheStore[UsedCodeEntry] authorize *cache.CacheStore[AuthorizeRequest] } + + mus struct { + consent sync.RWMutex + } } type OIDCServiceInput struct { @@ -336,6 +341,11 @@ func NewOIDCService(i OIDCServiceInput) (*OIDCService, error) { issuer: issuer, } + // Remove consents for clients that are no longer configured + if err := service.reconcileOIDCConsents(context.Background()); err != nil { + i.Log.App.Warn().Err(err).Msg("Failed to reconcile OIDC consents") + } + // Start cleanup routine i.Ding.Go(service.cleanupRoutine, ding.RingMinor) @@ -920,7 +930,7 @@ func (service *OIDCService) DeleteAuthorizeRequestTicket(ticket string) { service.caches.authorize.Delete(ticket) } -// TODO: support signed request objects in the future +// DecodeAuthorizeJWT TODO: support signed request objects in the future func (service *OIDCService) DecodeAuthorizeJWT(tokenString string) (*AuthorizeRequest, error) { var claims jwt.MapClaims @@ -970,3 +980,114 @@ func (service *OIDCService) GetPrompt(prompt string) []OIDCPrompt { return parsedPromps } + +func (service *OIDCService) getOIDCConsentUnsafe(ctx context.Context, username, clientId string) (*repository.OidcConsent, error) { + entry, err := service.queries.GetOIDCConsentByUsernameAndClientID(ctx, repository.GetOIDCConsentByUsernameAndClientIDParams{ + Username: username, + ClientID: clientId, + }) + + if err != nil { + if errors.Is(err, repository.ErrNotFound) { + return nil, nil + } + return nil, fmt.Errorf("failed to get oidc consent: %w", err) + } + + return &entry, nil +} + +func (service *OIDCService) GetOIDCConsent(ctx context.Context, username, clientId string) (*repository.OidcConsent, error) { + service.mus.consent.RLock() + defer service.mus.consent.RUnlock() + return service.getOIDCConsentUnsafe(ctx, username, clientId) +} + +func (service *OIDCService) UpsertOIDCConsent(ctx context.Context, username, scope, clientId string) (repository.OidcConsent, error) { + service.mus.consent.Lock() + defer service.mus.consent.Unlock() + + existing, err := service.getOIDCConsentUnsafe(ctx, username, clientId) + + if err != nil { + return repository.OidcConsent{}, err + } + + merged := scope + + if existing != nil { + merged = mergeScopes(existing.Scope, scope) + } + + entry := repository.UpsertOIDCConsentParams{ + Username: username, + Scope: merged, + ClientID: clientId, + CreatedAt: time.Now().Unix(), + } + + consent, err := service.queries.UpsertOIDCConsent(ctx, entry) + + if err != nil { + service.log.App.Error().Err(err).Msg("Failed to upsert OIDC consent") + return repository.OidcConsent{}, err + } + + return consent, nil +} + +func (service *OIDCService) reconcileOIDCConsents(ctx context.Context) error { + consents, err := service.queries.ListOIDCConsents(ctx) + + if err != nil { + return fmt.Errorf("failed to list oidc consents: %w", err) + } + + cleaned := make(map[string]struct{}) + + for _, consent := range consents { + if _, ok := cleaned[consent.ClientID]; ok { + continue + } + + cleaned[consent.ClientID] = struct{}{} + + if _, ok := service.clients[consent.ClientID]; ok { + continue + } + + service.log.App.Info().Str("clientId", consent.ClientID).Msg("Removed OIDC client no longer in configuration, deleting its consents") + + if err := service.queries.DeleteOIDCConsentByClientID(ctx, consent.ClientID); err != nil { + service.log.App.Warn().Err(err).Str("clientId", consent.ClientID).Msg("Failed to delete OIDC consents for removed client") + } + } + + return nil +} + +func mergeScopes(existing, requested string) string { + set := make(map[string]struct{}) + + for _, scope := range strings.Split(existing, " ") { + if scope != "" { + set[scope] = struct{}{} + } + } + + for _, scope := range strings.Split(requested, " ") { + if scope != "" { + set[scope] = struct{}{} + } + } + + scopes := make([]string, 0, len(set)) + + for scope := range set { + scopes = append(scopes, scope) + } + + slices.Sort(scopes) + + return strings.Join(scopes, " ") +} diff --git a/sql/postgres/oidc_queries.sql b/sql/postgres/oidc_queries.sql index 3cd5ff99f..12d2cf503 100644 --- a/sql/postgres/oidc_queries.sql +++ b/sql/postgres/oidc_queries.sql @@ -46,3 +46,29 @@ UPDATE "oidc_sessions" SET "userinfo_json" = $8 WHERE "sub" = $9 RETURNING *; + +-- name: UpsertOIDCConsent :one +INSERT INTO "oidc_consents" ( + "username", + "client_id", + "scope", + "created_at" +) VALUES ( + $1, $2, $3, $4 +) +ON CONFLICT ("username", "client_id") +DO UPDATE SET + "scope" = excluded.scope, + "created_at" = excluded.created_at +RETURNING *; + +-- name: GetOIDCConsentByUsernameAndClientID :one +SELECT * FROM "oidc_consents" +WHERE "username" = $1 AND "client_id" = $2; + +-- name: DeleteOIDCConsentByClientID :exec +DELETE FROM "oidc_consents" +WHERE "client_id" = $1; + +-- name: ListOIDCConsents :many +SELECT * FROM "oidc_consents"; diff --git a/sql/postgres/oidc_schemas.sql b/sql/postgres/oidc_schemas.sql index 2376c1d4a..dad3d332d 100644 --- a/sql/postgres/oidc_schemas.sql +++ b/sql/postgres/oidc_schemas.sql @@ -9,3 +9,12 @@ CREATE TABLE IF NOT EXISTS "oidc_sessions" ( "nonce" TEXT NOT NULL DEFAULT '', "userinfo_json" TEXT NOT NULL ); + +CREATE TABLE IF NOT EXISTS "oidc_consents" ( + "username" TEXT NOT NULL, + "client_id" TEXT NOT NULL, + "scope" TEXT NOT NULL, + "created_at" BIGINT NOT NULL, + PRIMARY KEY ("username", "client_id") +); + diff --git a/sql/sqlite/oidc_queries.sql b/sql/sqlite/oidc_queries.sql index 49b33cff7..ee4528ea2 100644 --- a/sql/sqlite/oidc_queries.sql +++ b/sql/sqlite/oidc_queries.sql @@ -46,3 +46,29 @@ UPDATE "oidc_sessions" SET "userinfo_json" = ? WHERE "sub" = ? RETURNING *; + +-- name: UpsertOIDCConsent :one +INSERT INTO "oidc_consents" ( + "username", + "client_id", + "scope", + "created_at" +) VALUES ( + ?, ?, ?, ? +) +ON CONFLICT ("username", "client_id") +DO UPDATE SET + "scope" = excluded.scope, + "created_at" = excluded.created_at +RETURNING *; + +-- name: GetOIDCConsentByUsernameAndClientID :one +SELECT * FROM "oidc_consents" +WHERE "username" = ? AND "client_id" = ?; + +-- name: DeleteOIDCConsentByClientID :exec +DELETE FROM "oidc_consents" +WHERE "client_id" = ?; + +-- name: ListOIDCConsents :many +SELECT * FROM "oidc_consents"; diff --git a/sql/sqlite/oidc_schemas.sql b/sql/sqlite/oidc_schemas.sql index 5a851033a..aac3aeacf 100644 --- a/sql/sqlite/oidc_schemas.sql +++ b/sql/sqlite/oidc_schemas.sql @@ -9,3 +9,11 @@ CREATE TABLE IF NOT EXISTS "oidc_sessions" ( "nonce" TEXT NOT NULL DEFAULT "", "userinfo_json" TEXT NOT NULL ); + +CREATE TABLE IF NOT EXISTS "oidc_consents" ( + "username" TEXT NOT NULL, + "client_id" TEXT NOT NULL, + "scope" TEXT NOT NULL, + "created_at" INTEGER NOT NULL, + PRIMARY KEY ("username", "client_id") +);