Merge pull request #1053 from Zibbp/oauth-nil-user-panic

fix: prevent oauth nil user panic

Author: Zibbp

internal/auth/auth.go

@@ -179,51 +179,60 @@ func (s *Service) OAuthUserCheck(ctx context.Context, userClaims OIDCCLaims) (*e
 
 		log.Debug().Msgf("OAuth user not found, creating user: %v", userClaims.PreferredUsername)
 
-		// Determine role from groups
-		role := utils.UserRole
-		for _, group := range userClaims.Groups {
-			if strings.HasPrefix(group, "ganymede-") {
-				groupRole := strings.TrimPrefix(group, "ganymede-")
-				if utils.IsValidRole(groupRole) {
-					log.Debug().Msgf("Found Ganymede role in user group %v", group)
-					role = utils.Role(groupRole)
-					break
-				}
-			}
+		// Determine role from groups for newly created users.
+		// If no mapped role is present, default to user.
+		role, ok := roleFromGroups(userClaims.Groups)
+		if !ok {
+			role = utils.UserRole
 		}
 
-		// Create new user
-		if _, err := s.Store.Client.User.Create().
+		createdUser, err := s.Store.Client.User.Create().
 			SetSub(userClaims.Sub).
 			SetUsername(userClaims.PreferredUsername).
 			SetRole(role).
 			SetOauth(true).
-			Save(ctx); err != nil {
+			Save(ctx)
+		if err != nil {
 			return nil, fmt.Errorf("failed to create user: %w", err)
 		}
-		return user, nil
-	}
-
-	// Determine role from groups
-	newRole := utils.UserRole
-	for _, group := range userClaims.Groups {
-		if strings.HasPrefix(group, "ganymede-") {
-			groupRole := strings.TrimPrefix(group, "ganymede-")
-			if utils.IsValidRole(groupRole) {
-				log.Debug().Msgf("Found Ganymede role in user group %v", group)
-				newRole = utils.Role(groupRole)
-				break
-			}
-		}
+		return createdUser, nil
 	}
 
-	// Update existing user
-	if _, err := s.Store.Client.User.UpdateOne(user).
-		SetUsername(userClaims.PreferredUsername).
-		SetRole(newRole).
-		Save(ctx); err != nil {
+	// Always keep username in sync with OIDC claim.
+	update := s.Store.Client.User.UpdateOne(user).SetUsername(userClaims.PreferredUsername)
+
+	// Only sync role when provider explicitly sends a ganymede-* or ganymede_* role group.
+	// This prevents manual role changes from being reset on each login.
+	if newRole, ok := roleFromGroups(userClaims.Groups); ok {
+		update = update.SetRole(newRole)
+	}
+
+	updatedUser, err := update.Save(ctx)
+	if err != nil {
 		return nil, fmt.Errorf("failed to update user: %w", err)
 	}
 
-	return user, nil
+	return updatedUser, nil
+}
+
+func roleFromGroups(groups []string) (utils.Role, bool) {
+	for _, group := range groups {
+		var groupRole string
+		switch {
+		case strings.HasPrefix(group, "ganymede-"):
+			groupRole = strings.TrimPrefix(group, "ganymede-")
+		case strings.HasPrefix(group, "ganymede_"):
+			groupRole = strings.TrimPrefix(group, "ganymede_")
+		default:
+			continue
+		}
+		if !utils.IsValidRole(groupRole) {
+			continue
+		}
+
+		log.Debug().Msgf("Found Ganymede role in user group %v", group)
+		return utils.Role(groupRole), true
+	}
+
+	return "", false
 }

internal/auth/auth_test.go

@@ -0,0 +1,34 @@
+package auth
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/zibbp/ganymede/internal/utils"
+)
+
+func TestRoleFromGroups(t *testing.T) {
+	t.Run("returns role when valid ganymede group exists", func(t *testing.T) {
+		role, ok := roleFromGroups([]string{"users", "ganymede-admin"})
+		assert.True(t, ok)
+		assert.Equal(t, utils.AdminRole, role)
+	})
+
+	t.Run("returns role when valid ganymede underscore group exists", func(t *testing.T) {
+		role, ok := roleFromGroups([]string{"users", "ganymede_editor"})
+		assert.True(t, ok)
+		assert.Equal(t, utils.EditorRole, role)
+	})
+
+	t.Run("skips invalid role groups", func(t *testing.T) {
+		role, ok := roleFromGroups([]string{"ganymede-invalid"})
+		assert.False(t, ok)
+		assert.Empty(t, role)
+	})
+
+	t.Run("returns false when no ganymede role group exists", func(t *testing.T) {
+		role, ok := roleFromGroups([]string{"users", "staff"})
+		assert.False(t, ok)
+		assert.Empty(t, role)
+	})
+}

internal/transport/http/auth.go

@@ -212,6 +212,9 @@ func (h *Handler) OAuthCallback(c echo.Context) error {
 	if err != nil {
 		return ErrorResponse(c, http.StatusInternalServerError, err.Error())
 	}
+	if user == nil {
+		return ErrorResponse(c, http.StatusInternalServerError, "oauth callback returned nil user")
+	}
 
 	h.SessionManager.Put(c.Request().Context(), "user_id", user.ID.String())