From 8af233b78d3c82cd6b89e4a9e28a9ad46a78cacd Mon Sep 17 00:00:00 2001 From: Stavros Date: Sun, 25 Jan 2026 18:32:14 +0200 Subject: [PATCH] fix: oidc review comments --- internal/bootstrap/service_bootstrap.go | 1 + internal/controller/oidc_controller.go | 34 +++++++++++++------------ internal/service/oidc_service.go | 23 ++++++++++++++--- 3 files changed, 38 insertions(+), 20 deletions(-) diff --git a/internal/bootstrap/service_bootstrap.go b/internal/bootstrap/service_bootstrap.go index b592a62..36ff821 100644 --- a/internal/bootstrap/service_bootstrap.go +++ b/internal/bootstrap/service_bootstrap.go @@ -94,6 +94,7 @@ func (app *BootstrapApp) initServices(queries *repository.Queries) (Services, er PrivateKeyPath: app.config.OIDC.PrivateKeyPath, PublicKeyPath: app.config.OIDC.PublicKeyPath, Issuer: app.config.AppURL, + SessionExpiry: app.config.Auth.SessionExpiry, }, queries) err = oidcService.Init() diff --git a/internal/controller/oidc_controller.go b/internal/controller/oidc_controller.go index f9f86a3..cb705cf 100644 --- a/internal/controller/oidc_controller.go +++ b/internal/controller/oidc_controller.go @@ -148,13 +148,15 @@ func (controller *OIDCController) Authorize(c *gin.Context) { return } - // We also need a snapshot of the user that authorized this - err = controller.oidc.StoreUserinfo(c, sub, userContext, req) + // We also need a snapshot of the user that authorized this (skip if no openid scope) + if slices.Contains(strings.Split(req.Scope, " "), "openid") { + err = controller.oidc.StoreUserinfo(c, sub, userContext, req) - if err != nil { - tlog.App.Error().Err(err).Msg("Failed to insert user info into database") - controller.authorizeError(c, err, "Failed to store user info", "Failed to store user info", req.RedirectURI, "server_error", req.State) - return + if err != nil { + tlog.App.Error().Err(err).Msg("Failed to insert user info into database") + controller.authorizeError(c, err, "Failed to store user info", "Failed to store user info", req.RedirectURI, "server_error", req.State) + return + } } queries, err := query.Values(AuthorizeCallback{ @@ -315,6 +317,15 @@ func (controller *OIDCController) Userinfo(c *gin.Context) { return } + // If we don't have the openid scope, return an error + if !slices.Contains(strings.Split(entry.Scope, ","), "openid") { + tlog.App.Warn().Msg("OIDC userinfo accessed without openid scope") + c.JSON(401, gin.H{ + "error": "invalid_request", + }) + return + } + user, err := controller.oidc.GetUserinfo(c, entry.Sub) if err != nil { @@ -325,15 +336,6 @@ func (controller *OIDCController) Userinfo(c *gin.Context) { return } - // If we don't have the openid scope, return an error - if !slices.Contains(strings.Split(entry.Scope, ","), "openid") { - tlog.App.Warn().Msg("OIDC userinfo accessed without openid scope") - c.JSON(401, gin.H{ - "error": "invalid_request", - }) - return - } - c.JSON(200, controller.oidc.CompileUserinfo(user, entry.Scope)) } @@ -362,7 +364,7 @@ func (controller *OIDCController) authorizeError(c *gin.Context, err error, reas c.JSON(200, gin.H{ "status": 200, - "redirect_uri": fmt.Sprintf("%s/?%s", callback, queries.Encode()), + "redirect_uri": fmt.Sprintf("%s?%s", callback, queries.Encode()), }) return } diff --git a/internal/service/oidc_service.go b/internal/service/oidc_service.go index d10186d..18ca566 100644 --- a/internal/service/oidc_service.go +++ b/internal/service/oidc_service.go @@ -69,6 +69,7 @@ type OIDCServiceConfig struct { PrivateKeyPath string PublicKeyPath string Issuer string + SessionExpiry int } type OIDCService struct { @@ -123,6 +124,9 @@ func (service *OIDCService) Init() error { return err } der := x509.MarshalPKCS1PrivateKey(privateKey) + if der == nil { + return errors.New("failed to marshal private key") + } encoded := pem.EncodeToMemory(&pem.Block{ Type: "RSA PRIVATE KEY", Bytes: der, @@ -134,6 +138,9 @@ func (service *OIDCService) Init() error { service.privateKey = privateKey } else { block, _ := pem.Decode(fprivateKey) + if block == nil { + return errors.New("failed to decode private key") + } privateKey, err = x509.ParsePKCS1PrivateKey(block.Bytes) if err != nil { return err @@ -150,6 +157,9 @@ func (service *OIDCService) Init() error { if errors.Is(err, os.ErrNotExist) { publicKey := service.privateKey.Public() der := x509.MarshalPKCS1PublicKey(publicKey.(*rsa.PublicKey)) + if der == nil { + return errors.New("failed to marshal public key") + } encoded := pem.EncodeToMemory(&pem.Block{ Type: "RSA PUBLIC KEY", Bytes: der, @@ -161,6 +171,9 @@ func (service *OIDCService) Init() error { service.publicKey = publicKey } else { block, _ := pem.Decode(fpublicKey) + if block == nil { + return errors.New("failed to decode public key") + } publicKey, err := x509.ParsePKCS1PublicKey(block.Bytes) if err != nil { return err @@ -316,9 +329,7 @@ func (service *OIDCService) GetCodeEntry(c *gin.Context, codeHash string) (repos func (service *OIDCService) generateIDToken(client config.OIDCClientConfig, sub string) (string, error) { createdAt := time.Now().Unix() - - // TODO: This should probably be user-configured if refresh logic does not exist - expiresAt := time.Now().Add(time.Duration(1) * time.Hour).Unix() + expiresAt := time.Now().Add(time.Duration(service.config.SessionExpiry) * time.Second).Unix() claims := jws.ClaimSet{ Iss: service.issuer, @@ -432,7 +443,11 @@ func (service *OIDCService) CompileUserinfo(user repository.OidcUserinfo, scope } if slices.Contains(scopes, "groups") { - userInfo.Groups = strings.Split(user.Groups, ",") + if user.Groups != "" { + userInfo.Groups = strings.Split(user.Groups, ",") + } else { + userInfo.Groups = []string{} + } } return userInfo