fix: oidc review comments

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()

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
+	// We also need a snapshot of the user that authorized this (skip if no openid scope)
-	err = controller.oidc.StoreUserinfo(c, sub, userContext, req)
+	if slices.Contains(strings.Split(req.Scope, " "), "openid") {
+		err = controller.oidc.StoreUserinfo(c, sub, userContext, req)
 
-	if err != nil {
+		if err != nil {
-		tlog.App.Error().Err(err).Msg("Failed to insert user info into database")
+			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)
+			controller.authorizeError(c, err, "Failed to store user info", "Failed to store user info", req.RedirectURI, "server_error", req.State)
-		return
+			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
 	}

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()
-
+	expiresAt := time.Now().Add(time.Duration(service.config.SessionExpiry) * time.Second).Unix()
-	// TODO: This should probably be user-configured if refresh logic does not exist
-	expiresAt := time.Now().Add(time.Duration(1) * time.Hour).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