From 96cdd0f28be863d3410f83a72d4d18886e7f16a9 Mon Sep 17 00:00:00 2001 From: Christian Romeni Date: Thu, 11 Jun 2026 00:31:43 +0200 Subject: [PATCH 1/2] fix: create org membership when creating a user (#100) Users created via the system-users page were inserted with no org_membership row. On login, ResolveUserRole returned an empty org for system admins, so CreateAPIKey hit a FOREIGN KEY violation on api_keys.org_id and login failed with a 500; plain members got a 401. Every user now always belongs to an organization. CreateUser requires org_id and creates the user and membership atomically in one transaction (CreateUserWithMembership). org_admin callers may only target their own org and may not assign the org_admin role - only system admins can. User creation now writes an audit event. A defensive login guard turns the legacy empty-org case into a generic 401 (logged server-side) instead of a 500, so pre-existing org-less users from the old bug fail cleanly. The create-user dialog gains a required organization selector. Fixes #100 --- internal/api/admin/auth.go | 14 + internal/api/admin/users.go | 66 ++- internal/api/admin/users_test.go | 792 +++++++++++++++++++++++++++---- internal/db/errors.go | 9 + internal/db/users.go | 81 ++++ internal/db/users_test.go | 190 ++++++++ ui/src/hooks/useUsers.ts | 2 + ui/src/pages/SystemUsersPage.tsx | 50 +- 8 files changed, 1118 insertions(+), 86 deletions(-) diff --git a/internal/api/admin/auth.go b/internal/api/admin/auth.go index a3bec28..03ebb48 100644 --- a/internal/api/admin/auth.go +++ b/internal/api/admin/auth.go @@ -128,6 +128,20 @@ func (h *Handler) Login(c fiber.Ctx) error { return apierror.InternalError(c, "authentication failed") } + // Defense-in-depth guard for legacy/inconsistent data: under the current + // invariant every user belongs to an org, so orgID is never empty here. + // Deployments affected by the old bug may have org-less users in their DB. + // Return the same generic 401 as a wrong password to prevent enumeration — + // an attacker who already supplied the correct password must not learn that + // the account exists but has no org. The specific reason is written to the + // server log for operators. + if orgID == "" { + h.Log.WarnContext(ctx, "login: user has no organization membership", + slog.String("email", req.Email)) + h.auditLoginFailed(c, req.Email, fiber.StatusUnauthorized) + return apierror.Send(c, fiber.StatusUnauthorized, "unauthorized", "invalid email or password") + } + // Revoke previous session keys for this user so only one session exists. if err := h.DB.RevokeUserSessions(ctx, userID); err != nil { h.Log.ErrorContext(ctx, "login: revoke old sessions", slog.String("error", err.Error())) diff --git a/internal/api/admin/users.go b/internal/api/admin/users.go index 876bd23..3bf78ba 100644 --- a/internal/api/admin/users.go +++ b/internal/api/admin/users.go @@ -3,21 +3,29 @@ package admin import ( "errors" "log/slog" + "strconv" "strings" + "time" "github.com/gofiber/fiber/v3" "github.com/voidmind-io/voidllm/internal/apierror" + "github.com/voidmind-io/voidllm/internal/audit" "github.com/voidmind-io/voidllm/internal/auth" "github.com/voidmind-io/voidllm/internal/db" "golang.org/x/crypto/bcrypt" ) // createUserRequest is the JSON body accepted by CreateUser. +// OrgID is required for all users, including system admins. Every user belongs +// to exactly one organization; there are no org-less users. Role is the +// organization membership role (defaults to "member" when omitted). type createUserRequest struct { Email string `json:"email"` DisplayName string `json:"display_name"` Password string `json:"password"` IsSystemAdmin bool `json:"is_system_admin"` + OrgID string `json:"org_id"` + Role string `json:"role"` } // updateUserRequest is the JSON body accepted by UpdateUser. @@ -113,6 +121,35 @@ func (h *Handler) CreateUser(c fiber.Ctx) error { return apierror.Send(c, fiber.StatusForbidden, "forbidden", "only system admins may create system admin users") } + // Every user must belong to an org — there are no org-less users. + if req.OrgID == "" { + return apierror.BadRequest(c, "org_id is required") + } + + // Non-system-admin callers may only assign users to their own org. + if !auth.HasRole(keyInfo.Role, auth.RoleSystemAdmin) { + if req.OrgID != keyInfo.OrgID { + return apierror.Send(c, fiber.StatusForbidden, "forbidden", "org_id must match your organization") + } + } + + // Validate and default the membership role. + membershipRole := req.Role + if membershipRole == "" { + membershipRole = auth.RoleMember + } + switch membershipRole { + case auth.RoleMember, auth.RoleTeamAdmin: + // valid membership roles for all callers + case auth.RoleOrgAdmin: + // only system admins may grant the org_admin membership role + if !auth.HasRole(keyInfo.Role, auth.RoleSystemAdmin) { + return apierror.Send(c, fiber.StatusForbidden, "forbidden", "only system admins may assign the org_admin role") + } + default: + return apierror.BadRequest(c, "role must be one of: member, team_admin, org_admin") + } + hash, err := bcrypt.GenerateFromPassword([]byte(req.Password), bcrypt.DefaultCost) if err != nil { h.Log.ErrorContext(c.Context(), "create user: bcrypt", slog.String("error", err.Error())) @@ -120,21 +157,46 @@ func (h *Handler) CreateUser(c fiber.Ctx) error { } hashStr := string(hash) - user, err := h.DB.CreateUser(c.Context(), db.CreateUserParams{ + user, err := h.DB.CreateUserWithMembership(c.Context(), db.CreateUserParams{ Email: req.Email, DisplayName: req.DisplayName, PasswordHash: &hashStr, AuthProvider: "local", IsSystemAdmin: req.IsSystemAdmin, - }) + }, req.OrgID, membershipRole) if err != nil { if errors.Is(err, db.ErrConflict) { return apierror.Conflict(c, "email already in use") } + if errors.Is(err, db.ErrForeignKey) { + return apierror.BadRequest(c, "organization not found") + } h.Log.ErrorContext(c.Context(), "create user", slog.String("error", err.Error())) return apierror.InternalError(c, "failed to create user") } + if h.AuditLogger != nil { + h.AuditLogger.Log(audit.Event{ + Timestamp: time.Now().UTC(), + OrgID: req.OrgID, + ActorID: keyInfo.UserID, + ActorType: "user", + ActorKeyID: keyInfo.ID, + Action: "user_created", + ResourceType: "user", + ResourceID: user.ID, + Description: marshalDescription(map[string]string{ + "email": req.Email, + "org_id": req.OrgID, + "role": membershipRole, + "is_system_admin": strconv.FormatBool(req.IsSystemAdmin), + }), + IPAddress: c.IP(), + StatusCode: fiber.StatusCreated, + RequestID: apierror.RequestIDFromCtx(c), + }) + } + return c.Status(fiber.StatusCreated).JSON(userToResponse(user)) } diff --git a/internal/api/admin/users_test.go b/internal/api/admin/users_test.go index a5a234b..6cc46d3 100644 --- a/internal/api/admin/users_test.go +++ b/internal/api/admin/users_test.go @@ -11,6 +11,7 @@ import ( "github.com/gofiber/fiber/v3" "github.com/voidmind-io/voidllm/internal/auth" "github.com/voidmind-io/voidllm/internal/db" + "golang.org/x/crypto/bcrypt" ) // mustCreateUser inserts a user directly via the DB for test setup. @@ -37,87 +38,167 @@ func mustCreateUser(t *testing.T, database *db.DB, email, displayName string) *d func TestCreateUser(t *testing.T) { t.Parallel() - tests := []struct { - name string - role string - body any - wantStatus int - }{ - { - name: "org_admin creates user returns 201", - role: auth.RoleOrgAdmin, - body: map[string]any{ - "email": "newuser@example.com", - "display_name": "New User", - "password": "securepassword", - }, - wantStatus: fiber.StatusCreated, - }, - { - name: "missing email returns 400", - role: auth.RoleOrgAdmin, - body: map[string]any{ - "display_name": "No Email", - "password": "securepassword", - }, - wantStatus: fiber.StatusBadRequest, - }, - { - name: "missing display_name returns 400", - role: auth.RoleOrgAdmin, - body: map[string]any{ - "email": "nodisplay@example.com", - "password": "securepassword", - }, - wantStatus: fiber.StatusBadRequest, - }, - { - name: "short password returns 400", - role: auth.RoleOrgAdmin, - body: map[string]any{ - "email": "shortpw@example.com", - "display_name": "Short PW", - "password": "short", - }, - wantStatus: fiber.StatusBadRequest, - }, - { - name: "member role returns 403", - role: auth.RoleMember, - body: map[string]any{ - "email": "member@example.com", - "display_name": "Member", - "password": "securepassword", - }, - wantStatus: fiber.StatusForbidden, - }, - } + t.Run("org_admin creates user with org_id returns 201", func(t *testing.T) { + t.Parallel() - for _, tc := range tests { - t.Run(tc.name, func(t *testing.T) { - t.Parallel() + app, database, keyCache := setupTestApp(t, "file:TestCreateUser_OrgAdmin201?mode=memory&cache=private") + org := mustCreateOrg(t, database, "Create User Org", "create-user-org") + testKey := addTestKey(t, keyCache, auth.RoleOrgAdmin, org.ID) - dsn := fmt.Sprintf("file:TestCreateUser_%s?mode=memory&cache=private", - strings.ReplaceAll(tc.name, " ", "_")) - app, _, keyCache := setupTestApp(t, dsn) - testKey := addTestKey(t, keyCache, tc.role, "") + req := httptest.NewRequest("POST", "/api/v1/users", bodyJSON(t, map[string]any{ + "email": "newuser@example.com", + "display_name": "New User", + "password": "securepassword", + "org_id": org.ID, + })) + req.Header.Set("Content-Type", "application/json") + req.Header.Set("Authorization", "Bearer "+testKey) - req := httptest.NewRequest("POST", "/api/v1/users", bodyJSON(t, tc.body)) - req.Header.Set("Content-Type", "application/json") - req.Header.Set("Authorization", "Bearer "+testKey) + resp, err := app.Test(req, fiber.TestConfig{Timeout: testTimeout}) + if err != nil { + t.Fatalf("app.Test: %v", err) + } + defer resp.Body.Close() - resp, err := app.Test(req, fiber.TestConfig{Timeout: testTimeout}) - if err != nil { - t.Fatalf("app.Test: %v", err) - } - defer resp.Body.Close() + if resp.StatusCode != fiber.StatusCreated { + body, _ := io.ReadAll(resp.Body) + t.Errorf("status = %d, want 201; body: %s", resp.StatusCode, body) + } + }) - if resp.StatusCode != tc.wantStatus { - body, _ := io.ReadAll(resp.Body) - t.Errorf("status = %d, want %d; body: %s", resp.StatusCode, tc.wantStatus, body) - } - }) - } + t.Run("org_admin without org_id returns 400", func(t *testing.T) { + t.Parallel() + + app, _, keyCache := setupTestApp(t, "file:TestCreateUser_OrgAdmin400NoOrg?mode=memory&cache=private") + testKey := addTestKey(t, keyCache, auth.RoleOrgAdmin, "") + + req := httptest.NewRequest("POST", "/api/v1/users", bodyJSON(t, map[string]any{ + "email": "newuser2@example.com", + "display_name": "New User 2", + "password": "securepassword", + })) + req.Header.Set("Content-Type", "application/json") + req.Header.Set("Authorization", "Bearer "+testKey) + + resp, err := app.Test(req, fiber.TestConfig{Timeout: testTimeout}) + if err != nil { + t.Fatalf("app.Test: %v", err) + } + defer resp.Body.Close() + + if resp.StatusCode != fiber.StatusBadRequest { + body, _ := io.ReadAll(resp.Body) + t.Errorf("status = %d, want 400; body: %s", resp.StatusCode, body) + } + }) + + t.Run("missing email returns 400", func(t *testing.T) { + t.Parallel() + + app, database, keyCache := setupTestApp(t, "file:TestCreateUser_MissingEmail?mode=memory&cache=private") + org := mustCreateOrg(t, database, "Missing Email Org", "missing-email-org") + testKey := addTestKey(t, keyCache, auth.RoleOrgAdmin, org.ID) + + req := httptest.NewRequest("POST", "/api/v1/users", bodyJSON(t, map[string]any{ + "display_name": "No Email", + "password": "securepassword", + "org_id": org.ID, + })) + req.Header.Set("Content-Type", "application/json") + req.Header.Set("Authorization", "Bearer "+testKey) + + resp, err := app.Test(req, fiber.TestConfig{Timeout: testTimeout}) + if err != nil { + t.Fatalf("app.Test: %v", err) + } + defer resp.Body.Close() + + if resp.StatusCode != fiber.StatusBadRequest { + body, _ := io.ReadAll(resp.Body) + t.Errorf("status = %d, want 400; body: %s", resp.StatusCode, body) + } + }) + + t.Run("missing display_name returns 400", func(t *testing.T) { + t.Parallel() + + app, database, keyCache := setupTestApp(t, "file:TestCreateUser_MissingDisplayName?mode=memory&cache=private") + org := mustCreateOrg(t, database, "Missing Display Org", "missing-display-org") + testKey := addTestKey(t, keyCache, auth.RoleOrgAdmin, org.ID) + + req := httptest.NewRequest("POST", "/api/v1/users", bodyJSON(t, map[string]any{ + "email": "nodisplay@example.com", + "password": "securepassword", + "org_id": org.ID, + })) + req.Header.Set("Content-Type", "application/json") + req.Header.Set("Authorization", "Bearer "+testKey) + + resp, err := app.Test(req, fiber.TestConfig{Timeout: testTimeout}) + if err != nil { + t.Fatalf("app.Test: %v", err) + } + defer resp.Body.Close() + + if resp.StatusCode != fiber.StatusBadRequest { + body, _ := io.ReadAll(resp.Body) + t.Errorf("status = %d, want 400; body: %s", resp.StatusCode, body) + } + }) + + t.Run("short password returns 400", func(t *testing.T) { + t.Parallel() + + app, database, keyCache := setupTestApp(t, "file:TestCreateUser_ShortPW?mode=memory&cache=private") + org := mustCreateOrg(t, database, "Short PW Org", "short-pw-org") + testKey := addTestKey(t, keyCache, auth.RoleOrgAdmin, org.ID) + + req := httptest.NewRequest("POST", "/api/v1/users", bodyJSON(t, map[string]any{ + "email": "shortpw@example.com", + "display_name": "Short PW", + "password": "short", + "org_id": org.ID, + })) + req.Header.Set("Content-Type", "application/json") + req.Header.Set("Authorization", "Bearer "+testKey) + + resp, err := app.Test(req, fiber.TestConfig{Timeout: testTimeout}) + if err != nil { + t.Fatalf("app.Test: %v", err) + } + defer resp.Body.Close() + + if resp.StatusCode != fiber.StatusBadRequest { + body, _ := io.ReadAll(resp.Body) + t.Errorf("status = %d, want 400; body: %s", resp.StatusCode, body) + } + }) + + t.Run("member role returns 403", func(t *testing.T) { + t.Parallel() + + app, _, keyCache := setupTestApp(t, "file:TestCreateUser_MemberForbidden?mode=memory&cache=private") + testKey := addTestKey(t, keyCache, auth.RoleMember, "") + + req := httptest.NewRequest("POST", "/api/v1/users", bodyJSON(t, map[string]any{ + "email": "member@example.com", + "display_name": "Member", + "password": "securepassword", + })) + req.Header.Set("Content-Type", "application/json") + req.Header.Set("Authorization", "Bearer "+testKey) + + resp, err := app.Test(req, fiber.TestConfig{Timeout: testTimeout}) + if err != nil { + t.Fatalf("app.Test: %v", err) + } + defer resp.Body.Close() + + if resp.StatusCode != fiber.StatusForbidden { + body, _ := io.ReadAll(resp.Body) + t.Errorf("status = %d, want 403; body: %s", resp.StatusCode, body) + } + }) } func TestCreateUser_NoAuth(t *testing.T) { @@ -148,13 +229,15 @@ func TestCreateUser_DuplicateEmail(t *testing.T) { t.Parallel() app, database, keyCache := setupTestApp(t, "file:TestCreateUser_DupEmail?mode=memory&cache=private") - testKey := addTestKey(t, keyCache, auth.RoleOrgAdmin, "") + org := mustCreateOrg(t, database, "Dup Email Org", "dup-email-org") + testKey := addTestKey(t, keyCache, auth.RoleOrgAdmin, org.ID) mustCreateUser(t, database, "existing@example.com", "Existing User") req := httptest.NewRequest("POST", "/api/v1/users", bodyJSON(t, map[string]any{ "email": "existing@example.com", "display_name": "Duplicate", "password": "securepassword", + "org_id": org.ID, })) req.Header.Set("Content-Type", "application/json") req.Header.Set("Authorization", "Bearer "+testKey) @@ -198,12 +281,14 @@ func TestCreateUser_OrgAdminCannotSetSystemAdmin(t *testing.T) { } } -func TestCreateUser_SystemAdminSetsSystemAdmin(t *testing.T) { +func TestCreateUser_SystemAdminWithoutOrgReturns400(t *testing.T) { t.Parallel() - app, _, keyCache := setupTestApp(t, "file:TestCreateUser_SysAdminSetsSysAdmin?mode=memory&cache=private") + app, _, keyCache := setupTestApp(t, "file:TestCreateUser_SysAdminNoOrg400?mode=memory&cache=private") testKey := addTestKey(t, keyCache, auth.RoleSystemAdmin, "") + // org_id is intentionally omitted — every user must belong to an org, + // including system admins. req := httptest.NewRequest("POST", "/api/v1/users", bodyJSON(t, map[string]any{ "email": "sysadmin2@example.com", "display_name": "System Admin Two", @@ -219,6 +304,35 @@ func TestCreateUser_SystemAdminSetsSystemAdmin(t *testing.T) { } defer resp.Body.Close() + if resp.StatusCode != fiber.StatusBadRequest { + body, _ := io.ReadAll(resp.Body) + t.Errorf("status = %d, want 400; body: %s", resp.StatusCode, body) + } +} + +func TestCreateUser_SystemAdminSetsSystemAdmin(t *testing.T) { + t.Parallel() + + app, database, keyCache := setupTestApp(t, "file:TestCreateUser_SysAdminSetsSysAdmin?mode=memory&cache=private") + org := mustCreateOrg(t, database, "SA Org", "sa-org-sysadmin") + testKey := addTestKey(t, keyCache, auth.RoleSystemAdmin, org.ID) + + req := httptest.NewRequest("POST", "/api/v1/users", bodyJSON(t, map[string]any{ + "email": "sysadmin2@example.com", + "display_name": "System Admin Two", + "password": "securepassword", + "is_system_admin": true, + "org_id": org.ID, + })) + req.Header.Set("Content-Type", "application/json") + req.Header.Set("Authorization", "Bearer "+testKey) + + resp, err := app.Test(req, fiber.TestConfig{Timeout: testTimeout}) + if err != nil { + t.Fatalf("app.Test: %v", err) + } + defer resp.Body.Close() + if resp.StatusCode != fiber.StatusCreated { body, _ := io.ReadAll(resp.Body) t.Fatalf("status = %d, want 201; body: %s", resp.StatusCode, body) @@ -234,13 +348,15 @@ func TestCreateUser_SystemAdminSetsSystemAdmin(t *testing.T) { func TestCreateUser_ResponseHasNoPassword(t *testing.T) { t.Parallel() - app, _, keyCache := setupTestApp(t, "file:TestCreateUser_NoPwInResp?mode=memory&cache=private") - testKey := addTestKey(t, keyCache, auth.RoleOrgAdmin, "") + app, database, keyCache := setupTestApp(t, "file:TestCreateUser_NoPwInResp?mode=memory&cache=private") + org := mustCreateOrg(t, database, "No PW Org", "no-pw-org") + testKey := addTestKey(t, keyCache, auth.RoleOrgAdmin, org.ID) req := httptest.NewRequest("POST", "/api/v1/users", bodyJSON(t, map[string]any{ "email": "nopw@example.com", "display_name": "No PW", "password": "securepassword", + "org_id": org.ID, })) req.Header.Set("Content-Type", "application/json") req.Header.Set("Authorization", "Bearer "+testKey) @@ -775,3 +891,517 @@ func TestDeleteUser_ThenGetReturns404(t *testing.T) { t.Errorf("GET after DELETE status = %d, want 404; body: %s", getResp.StatusCode, body) } } + +// ---- Issue #100: CreateUserWithMembership handler tests --------------------- + +// TestCreateUser_WithOrgID_CreatesUserAndMembership covers case 5: +// POST /users with a valid org_id returns 201, the user exists, and the +// membership row exists with role=member (the default). +func TestCreateUser_WithOrgID_CreatesUserAndMembership(t *testing.T) { + t.Parallel() + + app, database, keyCache := setupTestApp(t, "file:TestCreateUser_WithOrgID_Membership?mode=memory&cache=private") + org := mustCreateOrg(t, database, "Membership Check Org", "membership-check-org") + testKey := addTestKey(t, keyCache, auth.RoleOrgAdmin, org.ID) + + req := httptest.NewRequest("POST", "/api/v1/users", bodyJSON(t, map[string]any{ + "email": "membership-check@example.com", + "display_name": "Membership Check User", + "password": "securepassword", + "org_id": org.ID, + // role omitted — defaults to "member" + })) + req.Header.Set("Content-Type", "application/json") + req.Header.Set("Authorization", "Bearer "+testKey) + + resp, err := app.Test(req, fiber.TestConfig{Timeout: testTimeout}) + if err != nil { + t.Fatalf("app.Test: %v", err) + } + defer resp.Body.Close() + + if resp.StatusCode != fiber.StatusCreated { + body, _ := io.ReadAll(resp.Body) + t.Fatalf("status = %d, want 201; body: %s", resp.StatusCode, body) + } + + var got map[string]any + decodeBody(t, resp.Body, &got) + + userID, _ := got["id"].(string) + if userID == "" { + t.Fatal("response id is empty") + } + + // Verify the membership exists and has the correct role. + role, err := database.GetUserOrgRole(context.Background(), userID, org.ID) + if err != nil { + t.Fatalf("GetUserOrgRole() after CreateUser: %v", err) + } + if role != "member" { + t.Errorf("membership role = %q, want %q", role, "member") + } +} + +// TestCreateUser_OrgIDMissing_AllCallers covers case 6: +// Any caller omitting org_id gets 400, including system_admin. +func TestCreateUser_OrgIDMissing_AllCallers(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + role string + }{ + {name: "org_admin missing org_id returns 400", role: auth.RoleOrgAdmin}, + {name: "system_admin missing org_id returns 400", role: auth.RoleSystemAdmin}, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + dsn := fmt.Sprintf("file:TestCreateUser_OrgIDMissing_%s?mode=memory&cache=private", + strings.ReplaceAll(tc.name, " ", "_")) + app, _, keyCache := setupTestApp(t, dsn) + testKey := addTestKey(t, keyCache, tc.role, "") + + req := httptest.NewRequest("POST", "/api/v1/users", bodyJSON(t, map[string]any{ + "email": "no-org-id@example.com", + "display_name": "No Org ID", + "password": "securepassword", + // org_id intentionally omitted + })) + req.Header.Set("Content-Type", "application/json") + req.Header.Set("Authorization", "Bearer "+testKey) + + resp, err := app.Test(req, fiber.TestConfig{Timeout: testTimeout}) + if err != nil { + t.Fatalf("app.Test: %v", err) + } + defer resp.Body.Close() + + if resp.StatusCode != fiber.StatusBadRequest { + body, _ := io.ReadAll(resp.Body) + t.Errorf("status = %d, want 400; body: %s", resp.StatusCode, body) + } + }) + } +} + +// TestCreateUser_NonExistentOrgID covers case 7: +// POST /users with an org_id that references no existing org returns 400 +// "organization not found" (the ErrForeignKey path). +func TestCreateUser_NonExistentOrgID(t *testing.T) { + t.Parallel() + + app, _, keyCache := setupTestApp(t, "file:TestCreateUser_NonExistentOrgID?mode=memory&cache=private") + testKey := addTestKey(t, keyCache, auth.RoleSystemAdmin, "") + + req := httptest.NewRequest("POST", "/api/v1/users", bodyJSON(t, map[string]any{ + "email": "ghost-org@example.com", + "display_name": "Ghost Org User", + "password": "securepassword", + "org_id": "00000000-0000-0000-0000-000000000000", + })) + req.Header.Set("Content-Type", "application/json") + req.Header.Set("Authorization", "Bearer "+testKey) + + resp, err := app.Test(req, fiber.TestConfig{Timeout: testTimeout}) + if err != nil { + t.Fatalf("app.Test: %v", err) + } + defer resp.Body.Close() + + if resp.StatusCode != fiber.StatusBadRequest { + body, _ := io.ReadAll(resp.Body) + t.Fatalf("status = %d, want 400; body: %s", resp.StatusCode, body) + } + + var got map[string]any + decodeBody(t, resp.Body, &got) + if errMsg(got) != "organization not found" { + t.Errorf("error.message = %q, want %q", errMsg(got), "organization not found") + } +} + +// TestCreateUser_InvalidRole covers case 8: +// POST /users with an invalid role string returns 400. +func TestCreateUser_InvalidRole(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + role string + }{ + {name: "system_admin role is invalid for membership", role: "system_admin"}, + {name: "bogus role returns 400", role: "bogus"}, + {name: "empty-but-explicit role passes as default", role: ""}, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + dsn := fmt.Sprintf("file:TestCreateUser_InvalidRole_%s?mode=memory&cache=private", + strings.ReplaceAll(tc.name, " ", "_")) + app, database, keyCache := setupTestApp(t, dsn) + org := mustCreateOrg(t, database, "Role Test Org", "role-test-org-"+strings.ReplaceAll(tc.name, " ", "-")) + testKey := addTestKey(t, keyCache, auth.RoleSystemAdmin, org.ID) + + body := map[string]any{ + "email": "invalid-role@example.com", + "display_name": "Invalid Role User", + "password": "securepassword", + "org_id": org.ID, + } + if tc.role != "" { + body["role"] = tc.role + } + + req := httptest.NewRequest("POST", "/api/v1/users", bodyJSON(t, body)) + req.Header.Set("Content-Type", "application/json") + req.Header.Set("Authorization", "Bearer "+testKey) + + resp, err := app.Test(req, fiber.TestConfig{Timeout: testTimeout}) + if err != nil { + t.Fatalf("app.Test: %v", err) + } + defer resp.Body.Close() + + wantStatus := fiber.StatusBadRequest + if tc.role == "" { + // Empty role is treated as the default "member" — should succeed. + wantStatus = fiber.StatusCreated + } + + if resp.StatusCode != wantStatus { + raw, _ := io.ReadAll(resp.Body) + t.Errorf("status = %d, want %d; body: %s", resp.StatusCode, wantStatus, raw) + } + }) + } +} + +// TestCreateUser_OrgAdminCannotAssignOrgAdminRole verifies that an org_admin +// caller receives 403 when attempting to create a user with role="org_admin", +// and that member/team_admin roles still succeed with 201. +// A system_admin caller must be allowed to assign org_admin. +func TestCreateUser_OrgAdminCannotAssignOrgAdminRole(t *testing.T) { + t.Parallel() + + t.Run("org_admin sets role=org_admin returns 403", func(t *testing.T) { + t.Parallel() + + app, database, keyCache := setupTestApp(t, "file:TestCreateUser_OrgAdminRoleOrgAdmin403?mode=memory&cache=private") + org := mustCreateOrg(t, database, "Role Assign Org", "role-assign-org-1") + testKey := addTestKey(t, keyCache, auth.RoleOrgAdmin, org.ID) + + req := httptest.NewRequest("POST", "/api/v1/users", bodyJSON(t, map[string]any{ + "email": "new-org-admin@example.com", + "display_name": "New Org Admin", + "password": "securepassword", + "org_id": org.ID, + "role": "org_admin", + })) + req.Header.Set("Content-Type", "application/json") + req.Header.Set("Authorization", "Bearer "+testKey) + + resp, err := app.Test(req, fiber.TestConfig{Timeout: testTimeout}) + if err != nil { + t.Fatalf("app.Test: %v", err) + } + defer resp.Body.Close() + + if resp.StatusCode != fiber.StatusForbidden { + body, _ := io.ReadAll(resp.Body) + t.Errorf("status = %d, want 403; body: %s", resp.StatusCode, body) + } + }) + + t.Run("org_admin sets role=member returns 201", func(t *testing.T) { + t.Parallel() + + app, database, keyCache := setupTestApp(t, "file:TestCreateUser_OrgAdminRoleMember201?mode=memory&cache=private") + org := mustCreateOrg(t, database, "Role Assign Org", "role-assign-org-2") + testKey := addTestKey(t, keyCache, auth.RoleOrgAdmin, org.ID) + + req := httptest.NewRequest("POST", "/api/v1/users", bodyJSON(t, map[string]any{ + "email": "new-member@example.com", + "display_name": "New Member", + "password": "securepassword", + "org_id": org.ID, + "role": "member", + })) + req.Header.Set("Content-Type", "application/json") + req.Header.Set("Authorization", "Bearer "+testKey) + + resp, err := app.Test(req, fiber.TestConfig{Timeout: testTimeout}) + if err != nil { + t.Fatalf("app.Test: %v", err) + } + defer resp.Body.Close() + + if resp.StatusCode != fiber.StatusCreated { + body, _ := io.ReadAll(resp.Body) + t.Errorf("status = %d, want 201; body: %s", resp.StatusCode, body) + } + }) + + t.Run("org_admin sets role=team_admin returns 201", func(t *testing.T) { + t.Parallel() + + app, database, keyCache := setupTestApp(t, "file:TestCreateUser_OrgAdminRoleTeamAdmin201?mode=memory&cache=private") + org := mustCreateOrg(t, database, "Role Assign Org", "role-assign-org-3") + testKey := addTestKey(t, keyCache, auth.RoleOrgAdmin, org.ID) + + req := httptest.NewRequest("POST", "/api/v1/users", bodyJSON(t, map[string]any{ + "email": "new-team-admin@example.com", + "display_name": "New Team Admin", + "password": "securepassword", + "org_id": org.ID, + "role": "team_admin", + })) + req.Header.Set("Content-Type", "application/json") + req.Header.Set("Authorization", "Bearer "+testKey) + + resp, err := app.Test(req, fiber.TestConfig{Timeout: testTimeout}) + if err != nil { + t.Fatalf("app.Test: %v", err) + } + defer resp.Body.Close() + + if resp.StatusCode != fiber.StatusCreated { + body, _ := io.ReadAll(resp.Body) + t.Errorf("status = %d, want 201; body: %s", resp.StatusCode, body) + } + }) + + t.Run("system_admin sets role=org_admin returns 201", func(t *testing.T) { + t.Parallel() + + app, database, keyCache := setupTestApp(t, "file:TestCreateUser_SysAdminRoleOrgAdmin201?mode=memory&cache=private") + org := mustCreateOrg(t, database, "Role Assign Org", "role-assign-org-4") + testKey := addTestKey(t, keyCache, auth.RoleSystemAdmin, org.ID) + + req := httptest.NewRequest("POST", "/api/v1/users", bodyJSON(t, map[string]any{ + "email": "new-org-admin-by-sa@example.com", + "display_name": "New Org Admin By SysAdmin", + "password": "securepassword", + "org_id": org.ID, + "role": "org_admin", + })) + req.Header.Set("Content-Type", "application/json") + req.Header.Set("Authorization", "Bearer "+testKey) + + resp, err := app.Test(req, fiber.TestConfig{Timeout: testTimeout}) + if err != nil { + t.Fatalf("app.Test: %v", err) + } + defer resp.Body.Close() + + if resp.StatusCode != fiber.StatusCreated { + body, _ := io.ReadAll(resp.Body) + t.Errorf("status = %d, want 201; body: %s", resp.StatusCode, body) + } + }) +} + +// TestCreateUser_OrgAdminCrossOrgAuthZ covers case 9: +// An org_admin setting a foreign org_id gets 403; with own org_id gets 201. +func TestCreateUser_OrgAdminCrossOrgAuthZ(t *testing.T) { + t.Parallel() + + t.Run("org_admin with foreign org_id returns 403", func(t *testing.T) { + t.Parallel() + + app, database, keyCache := setupTestApp(t, "file:TestCreateUser_OrgAdminCrossOrg403?mode=memory&cache=private") + ownOrg := mustCreateOrg(t, database, "Own Org", "own-org-cross") + foreignOrg := mustCreateOrg(t, database, "Foreign Org", "foreign-org-cross") + // Key is scoped to ownOrg, but request asks for foreignOrg. + testKey := addTestKey(t, keyCache, auth.RoleOrgAdmin, ownOrg.ID) + + req := httptest.NewRequest("POST", "/api/v1/users", bodyJSON(t, map[string]any{ + "email": "cross-org@example.com", + "display_name": "Cross Org User", + "password": "securepassword", + "org_id": foreignOrg.ID, + })) + req.Header.Set("Content-Type", "application/json") + req.Header.Set("Authorization", "Bearer "+testKey) + + resp, err := app.Test(req, fiber.TestConfig{Timeout: testTimeout}) + if err != nil { + t.Fatalf("app.Test: %v", err) + } + defer resp.Body.Close() + + if resp.StatusCode != fiber.StatusForbidden { + body, _ := io.ReadAll(resp.Body) + t.Errorf("status = %d, want 403; body: %s", resp.StatusCode, body) + } + }) + + t.Run("org_admin with own org_id returns 201", func(t *testing.T) { + t.Parallel() + + app, database, keyCache := setupTestApp(t, "file:TestCreateUser_OrgAdminOwnOrg201?mode=memory&cache=private") + ownOrg := mustCreateOrg(t, database, "Own Org", "own-org-same") + testKey := addTestKey(t, keyCache, auth.RoleOrgAdmin, ownOrg.ID) + + req := httptest.NewRequest("POST", "/api/v1/users", bodyJSON(t, map[string]any{ + "email": "own-org@example.com", + "display_name": "Own Org User", + "password": "securepassword", + "org_id": ownOrg.ID, + })) + req.Header.Set("Content-Type", "application/json") + req.Header.Set("Authorization", "Bearer "+testKey) + + resp, err := app.Test(req, fiber.TestConfig{Timeout: testTimeout}) + if err != nil { + t.Fatalf("app.Test: %v", err) + } + defer resp.Body.Close() + + if resp.StatusCode != fiber.StatusCreated { + body, _ := io.ReadAll(resp.Body) + t.Errorf("status = %d, want 201; body: %s", resp.StatusCode, body) + } + }) +} + +// TestCreateUser_ThenLogin_IssueRegression covers case 10 — the core regression +// for Issue #100. +// +// Before the fix: CreateUser did not create an org_membership, so the subsequent +// Login call hit ResolveUserRole, found no membership for a non-system-admin user, +// returned ErrNotFound, and the Login handler propagated that as HTTP 500. +// +// After the fix: CreateUserWithMembership creates both the user and the membership +// atomically, so Login succeeds with HTTP 200 and returns a valid session token. +func TestCreateUser_ThenLogin_IssueRegression(t *testing.T) { + t.Parallel() + + app, database, keyCache := setupTestApp(t, "file:TestCreateUser_ThenLogin_Issue100?mode=memory&cache=private") + org := mustCreateOrg(t, database, "Login Regression Org", "login-regression-org") + creatorKey := addTestKey(t, keyCache, auth.RoleOrgAdmin, org.ID) + + const userEmail = "regression-login@example.com" + const userPassword = "strongpassword123" + + // Step 1: create the user via the API (uses CreateUserWithMembership). + createReq := httptest.NewRequest("POST", "/api/v1/users", bodyJSON(t, map[string]any{ + "email": userEmail, + "display_name": "Regression Login User", + "password": userPassword, + "org_id": org.ID, + })) + createReq.Header.Set("Content-Type", "application/json") + createReq.Header.Set("Authorization", "Bearer "+creatorKey) + + createResp, err := app.Test(createReq, fiber.TestConfig{Timeout: testTimeout}) + if err != nil { + t.Fatalf("app.Test CreateUser: %v", err) + } + defer createResp.Body.Close() + + if createResp.StatusCode != fiber.StatusCreated { + body, _ := io.ReadAll(createResp.Body) + t.Fatalf("CreateUser status = %d, want 201; body: %s", createResp.StatusCode, body) + } + + // Step 2: log in with the newly-created user's credentials. + // On the old code (without membership) this returned HTTP 500 or a FK crash. + // On the fixed code it must return HTTP 200 with a valid session token. + loginReq := httptest.NewRequest("POST", "/api/v1/auth/login", bodyJSON(t, map[string]any{ + "email": userEmail, + "password": userPassword, + })) + loginReq.Header.Set("Content-Type", "application/json") + + loginResp, err := app.Test(loginReq, fiber.TestConfig{Timeout: testTimeout}) + if err != nil { + t.Fatalf("app.Test Login: %v", err) + } + defer loginResp.Body.Close() + + if loginResp.StatusCode != fiber.StatusOK { + body, _ := io.ReadAll(loginResp.Body) + t.Fatalf("Login status = %d, want 200 (Issue #100 regression); body: %s", loginResp.StatusCode, body) + } + + var loginBody map[string]any + decodeBody(t, loginResp.Body, &loginBody) + + token, _ := loginBody["token"].(string) + if token == "" { + t.Error("login token is empty, want a non-empty session token") + } + if !strings.HasPrefix(token, "vl_sk_") { + t.Errorf("token = %q, want prefix %q", token, "vl_sk_") + } +} + +// TestLogin_LegacyUserWithoutMembership covers case 11 — the legacy guard. +// +// A user inserted directly without an org_membership (simulating pre-fix data) +// who is NOT a system_admin must receive HTTP 403 with a clear message on login, +// not HTTP 500 or a foreign-key crash. +func TestLogin_LegacyUserWithoutMembership(t *testing.T) { + t.Parallel() + + app, database, _ := setupTestApp(t, "file:TestLogin_LegacyNoMembership?mode=memory&cache=private") + + // Insert a system_admin user directly, then strip the membership to simulate + // legacy data. ResolveUserRole for system_admin with no membership returns + // orgID="" — that is the path the guard protects. + // + // We use is_system_admin=true here because ResolveUserRole for a non-admin + // with no membership returns ErrNotFound (handled as 401), while for a + // system_admin it returns orgID="" — that is the exact legacy guard path + // introduced by the fix. + const legacyEmail = "legacy-no-membership@example.com" + const legacyPassword = "legacypassword1" + + hash, err := bcrypt.GenerateFromPassword([]byte(legacyPassword), bcrypt.MinCost) + if err != nil { + t.Fatalf("bcrypt: %v", err) + } + hashStr := string(hash) + + // Insert directly so no membership is created — simulating the bug-era state. + _, err = database.CreateUser(context.Background(), db.CreateUserParams{ + Email: legacyEmail, + DisplayName: "Legacy User", + PasswordHash: &hashStr, + IsSystemAdmin: true, // system_admin with no membership → orgID="" path + }) + if err != nil { + t.Fatalf("CreateUser legacy: %v", err) + } + + loginReq := httptest.NewRequest("POST", "/api/v1/auth/login", bodyJSON(t, map[string]any{ + "email": legacyEmail, + "password": legacyPassword, + })) + loginReq.Header.Set("Content-Type", "application/json") + + loginResp, err := app.Test(loginReq, fiber.TestConfig{Timeout: testTimeout}) + if err != nil { + t.Fatalf("app.Test Login: %v", err) + } + defer loginResp.Body.Close() + + // Must be 401 with the same generic message as a wrong password — NOT 500, + // and NOT 403 with an account-existence-revealing message. + if loginResp.StatusCode != fiber.StatusUnauthorized { + body, _ := io.ReadAll(loginResp.Body) + t.Fatalf("Login status = %d, want 401 (legacy guard must not 500 or leak account info); body: %s", loginResp.StatusCode, body) + } + + var got map[string]any + decodeBody(t, loginResp.Body, &got) + if errMsg(got) != "invalid email or password" { + t.Errorf("error.message = %q, want %q (no enumeration leak)", errMsg(got), "invalid email or password") + } +} diff --git a/internal/db/errors.go b/internal/db/errors.go index b7e63bd..3fce458 100644 --- a/internal/db/errors.go +++ b/internal/db/errors.go @@ -18,8 +18,13 @@ var ErrConflict = errors.New("conflict") // ErrNoPassword is returned when a user has no password hash (SSO-only account). var ErrNoPassword = errors.New("no password") +// ErrForeignKey is returned when an insert or update violates a foreign key constraint. +// It indicates that a referenced record (e.g. organization) does not exist. +var ErrForeignKey = errors.New("foreign key violation") + // translateError maps low-level driver errors to domain sentinels. // sql.ErrNoRows becomes ErrNotFound, UNIQUE constraint violations become ErrConflict, +// FOREIGN KEY constraint violations become ErrForeignKey, // and all other errors are returned unchanged. Both sentinel and original error // are preserved in the chain so callers can use errors.Is on either. func translateError(err error) error { @@ -34,5 +39,9 @@ func translateError(err error) error { strings.Contains(msg, "duplicate key value violates unique constraint") { return fmt.Errorf("%w: %w", ErrConflict, err) } + if strings.Contains(msg, "FOREIGN KEY constraint failed") || + strings.Contains(msg, "violates foreign key constraint") { + return fmt.Errorf("%w: %w", ErrForeignKey, err) + } return err } diff --git a/internal/db/users.go b/internal/db/users.go index dbd70e0..bc7d1cd 100644 --- a/internal/db/users.go +++ b/internal/db/users.go @@ -109,6 +109,87 @@ func (d *DB) CreateUser(ctx context.Context, params CreateUserParams) (*User, er return user, nil } +// CreateUserWithMembership creates a user and an organization membership +// atomically within a single transaction. orgID must be non-empty; every user +// belongs to exactly one organization. role is the org membership role +// (e.g. "member", "team_admin", "org_admin"). +// +// Returns ErrConflict if the email is already taken. +// Returns ErrForeignKey if orgID does not reference an existing organization, +// allowing callers to map this to a 400 "organization not found" response +// without an additional pre-flight SELECT. +func (d *DB) CreateUserWithMembership(ctx context.Context, userParams CreateUserParams, orgID, role string) (*User, error) { + userID, err := uuid.NewV7() + if err != nil { + return nil, fmt.Errorf("create user with membership: generate user id: %w", err) + } + + authProvider := userParams.AuthProvider + if authProvider == "" { + authProvider = "local" + } + + p := d.dialect.Placeholder + + insertUserQuery := "INSERT INTO users " + + "(id, email, display_name, password_hash, auth_provider, external_id, is_system_admin, created_at, updated_at) " + + "VALUES (" + + p(1) + ", " + p(2) + ", " + p(3) + ", " + p(4) + ", " + + p(5) + ", " + p(6) + ", " + p(7) + ", " + + "CURRENT_TIMESTAMP, CURRENT_TIMESTAMP)" + + selectUserQuery := "SELECT " + userSelectColumns + + " FROM users WHERE id = " + p(1) + " AND deleted_at IS NULL" + + isSystemAdminInt := 0 + if userParams.IsSystemAdmin { + isSystemAdminInt = 1 + } + + var user *User + err = d.WithTx(ctx, func(q Querier) error { + _, execErr := q.ExecContext(ctx, insertUserQuery, + userID.String(), + userParams.Email, + userParams.DisplayName, + userParams.PasswordHash, + authProvider, + userParams.ExternalID, + isSystemAdminInt, + ) + if execErr != nil { + return translateError(execErr) + } + + membershipID, idErr := uuid.NewV7() + if idErr != nil { + return fmt.Errorf("generate membership id: %w", idErr) + } + + insertMembershipQuery := "INSERT INTO org_memberships (id, org_id, user_id, role, created_at) " + + "VALUES (" + p(1) + ", " + p(2) + ", " + p(3) + ", " + p(4) + ", CURRENT_TIMESTAMP)" + + _, execErr = q.ExecContext(ctx, insertMembershipQuery, + membershipID.String(), + orgID, + userID.String(), + role, + ) + if execErr != nil { + return translateError(execErr) + } + + row := q.QueryRowContext(ctx, selectUserQuery, userID.String()) + var scanErr error + user, scanErr = scanUser(row) + return scanErr + }) + if err != nil { + return nil, fmt.Errorf("create user with membership: %w", err) + } + return user, nil +} + // GetUser retrieves an active user by their ID. // It returns ErrNotFound if the user does not exist or has been soft-deleted. func (d *DB) GetUser(ctx context.Context, id string) (*User, error) { diff --git a/internal/db/users_test.go b/internal/db/users_test.go index efdccb1..15c6080 100644 --- a/internal/db/users_test.go +++ b/internal/db/users_test.go @@ -9,6 +9,30 @@ import ( "golang.org/x/crypto/bcrypt" ) +// countUsers returns the number of active (non-deleted) users in the DB. +func countUsers(t *testing.T, d *DB) int { + t.Helper() + var n int + if err := d.sql.QueryRowContext(context.Background(), + "SELECT COUNT(*) FROM users WHERE deleted_at IS NULL", + ).Scan(&n); err != nil { + t.Fatalf("countUsers: %v", err) + } + return n +} + +// countMemberships returns the number of org_memberships rows for a given user. +func countMemberships(t *testing.T, d *DB, userID string) int { + t.Helper() + var n int + if err := d.sql.QueryRowContext(context.Background(), + "SELECT COUNT(*) FROM org_memberships WHERE user_id = ?", userID, + ).Scan(&n); err != nil { + t.Fatalf("countMemberships: %v", err) + } + return n +} + // mustCreateUser creates a user and fatals the test on error. func mustCreateUser(t *testing.T, d *DB, params CreateUserParams) *User { t.Helper() @@ -670,6 +694,172 @@ func TestGetUserPasswordHash(t *testing.T) { } } +// ---- CreateUserWithMembership ------------------------------------------------ + +func TestCreateUserWithMembership(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + setup func(t *testing.T, d *DB) (orgID string) + userParams CreateUserParams + role string + wantErr error + checkFunc func(t *testing.T, d *DB, got *User, orgID string) + }{ + { + name: "happy path: user and membership created atomically", + setup: func(t *testing.T, d *DB) string { + t.Helper() + org := mustCreateOrg(t, d, CreateOrgParams{Name: "Test Org", Slug: "cwm-happy"}) + return org.ID + }, + userParams: CreateUserParams{ + Email: "cwm-happy@example.com", + DisplayName: "Happy Path User", + }, + role: "member", + wantErr: nil, + checkFunc: func(t *testing.T, d *DB, got *User, orgID string) { + t.Helper() + // Returned user has the expected fields. + if got.ID == "" { + t.Error("ID is empty, want non-empty UUID") + } + if got.Email != "cwm-happy@example.com" { + t.Errorf("Email = %q, want %q", got.Email, "cwm-happy@example.com") + } + // ResolveUserRole returns the correct role and orgID. + role, resolvedOrgID, err := d.ResolveUserRole(context.Background(), got.ID) + if err != nil { + t.Fatalf("ResolveUserRole() error = %v", err) + } + if role != "member" { + t.Errorf("ResolveUserRole() role = %q, want %q", role, "member") + } + if resolvedOrgID != orgID { + t.Errorf("ResolveUserRole() orgID = %q, want %q", resolvedOrgID, orgID) + } + // Membership row exists in the DB. + if n := countMemberships(t, d, got.ID); n != 1 { + t.Errorf("org_memberships count = %d, want 1", n) + } + }, + }, + { + name: "non-existent orgID returns ErrForeignKey", + setup: func(t *testing.T, d *DB) string { + return "00000000-0000-0000-0000-000000000000" // does not exist + }, + userParams: CreateUserParams{ + Email: "cwm-fk@example.com", + DisplayName: "FK User", + }, + role: "member", + wantErr: ErrForeignKey, + checkFunc: nil, + }, + { + name: "FK failure rolls back: no orphan user record left behind", + setup: func(t *testing.T, d *DB) string { + return "00000000-0000-0000-0000-000000000000" + }, + userParams: CreateUserParams{ + Email: "cwm-rollback@example.com", + DisplayName: "Rollback User", + }, + role: "member", + wantErr: ErrForeignKey, + checkFunc: func(t *testing.T, d *DB, _ *User, _ string) { + t.Helper() + // After FK failure no user row should exist. + _, err := d.GetUserByEmail(context.Background(), "cwm-rollback@example.com") + if !errors.Is(err, ErrNotFound) { + t.Errorf("GetUserByEmail() after FK rollback = %v, want ErrNotFound (no orphan user)", err) + } + }, + }, + { + name: "role org_admin is stored correctly in membership", + setup: func(t *testing.T, d *DB) string { + t.Helper() + org := mustCreateOrg(t, d, CreateOrgParams{Name: "Admin Role Org", Slug: "cwm-admin-role"}) + return org.ID + }, + userParams: CreateUserParams{ + Email: "cwm-admin@example.com", + DisplayName: "Org Admin User", + }, + role: "org_admin", + wantErr: nil, + checkFunc: func(t *testing.T, d *DB, got *User, orgID string) { + t.Helper() + role, resolvedOrgID, err := d.ResolveUserRole(context.Background(), got.ID) + if err != nil { + t.Fatalf("ResolveUserRole() error = %v", err) + } + if role != "org_admin" { + t.Errorf("ResolveUserRole() role = %q, want %q", role, "org_admin") + } + if resolvedOrgID != orgID { + t.Errorf("ResolveUserRole() orgID = %q, want %q", resolvedOrgID, orgID) + } + }, + }, + { + name: "duplicate email returns ErrConflict", + setup: func(t *testing.T, d *DB) string { + t.Helper() + org := mustCreateOrg(t, d, CreateOrgParams{Name: "Dup Email Org", Slug: "cwm-dup-email"}) + // Pre-create a user with the same email. + mustCreateUser(t, d, CreateUserParams{ + Email: "cwm-dup@example.com", + DisplayName: "Original", + PasswordHash: testPasswordHash(t), + }) + return org.ID + }, + userParams: CreateUserParams{ + Email: "cwm-dup@example.com", + DisplayName: "Duplicate", + }, + role: "member", + wantErr: ErrConflict, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + d := openMigratedDB(t) + + if tc.userParams.PasswordHash == nil { + tc.userParams.PasswordHash = testPasswordHash(t) + } + + orgID := tc.setup(t, d) + usersBefore := countUsers(t, d) + + got, err := d.CreateUserWithMembership(context.Background(), tc.userParams, orgID, tc.role) + + if !errors.Is(err, tc.wantErr) { + t.Fatalf("CreateUserWithMembership() error = %v, wantErr %v", err, tc.wantErr) + } + + if tc.wantErr != nil { + // On any error, no new user row must have been committed. + if after := countUsers(t, d); after != usersBefore { + t.Errorf("user count = %d, want %d (no orphan rows after failure)", after, usersBefore) + } + } + + if tc.wantErr == nil && tc.checkFunc != nil { + tc.checkFunc(t, d, got, orgID) + } + }) + } +} + // ---- DeleteUser -------------------------------------------------------------- func TestDeleteUser(t *testing.T) { diff --git a/ui/src/hooks/useUsers.ts b/ui/src/hooks/useUsers.ts index 4b2a766..c35a0a6 100644 --- a/ui/src/hooks/useUsers.ts +++ b/ui/src/hooks/useUsers.ts @@ -18,6 +18,8 @@ export interface CreateUserParams { display_name: string password: string is_system_admin?: boolean + org_id: string + role?: string } export interface PaginatedUsers { diff --git a/ui/src/pages/SystemUsersPage.tsx b/ui/src/pages/SystemUsersPage.tsx index 04a5330..2988029 100644 --- a/ui/src/pages/SystemUsersPage.tsx +++ b/ui/src/pages/SystemUsersPage.tsx @@ -1,4 +1,4 @@ -import React, { useState } from 'react' +import React, { useState, useMemo } from 'react' import { Navigate } from 'react-router-dom' import { PageHeader } from '../components/ui/PageHeader' import { Table } from '../components/ui/Table' @@ -7,12 +7,14 @@ import { Dialog, ConfirmDialog } from '../components/ui/Dialog' import { Badge } from '../components/ui/Badge' import { Button } from '../components/ui/Button' import { Input } from '../components/ui/Input' +import { Select } from '../components/ui/Select' import { Toggle } from '../components/ui/Toggle' import { TimeAgo } from '../components/ui/TimeAgo' import { StatCard } from '../components/ui/StatCard' import { useMe } from '../hooks/useMe' import { useUsers, useCreateUser, useDeleteUser } from '../hooks/useUsers' import type { UserResponse, CreateUserParams } from '../hooks/useUsers' +import { useOrgs } from '../hooks/useOrgs' import { useToast } from '../hooks/useToast' // --------------------------------------------------------------------------- @@ -72,26 +74,47 @@ function CreateUserDialog({ open, onClose }: CreateUserDialogProps) { const [displayName, setDisplayName] = useState('') const [password, setPassword] = useState('') const [isSystemAdmin, setIsSystemAdmin] = useState(false) + const [orgId, setOrgId] = useState('') const [emailError, setEmailError] = useState() const [displayNameError, setDisplayNameError] = useState() const [passwordError, setPasswordError] = useState() + const [orgError, setOrgError] = useState() const createUser = useCreateUser() + const { data: orgsData } = useOrgs() const { toast } = useToast() + const orgOptions = useMemo( + () => (orgsData?.data ?? []).map((o) => ({ value: o.id, label: o.name })), + [orgsData?.data], + ) + + // Auto-select first org when list loads and nothing is selected yet + React.useEffect(() => { + if (orgOptions.length > 0 && orgId === '') { + setOrgId(orgOptions[0].value) + } + }, [orgOptions, orgId]) + function handleClose() { setEmail('') setDisplayName('') setPassword('') setIsSystemAdmin(false) + setOrgId(orgOptions.length > 0 ? orgOptions[0].value : '') setEmailError(undefined) setDisplayNameError(undefined) setPasswordError(undefined) + setOrgError(undefined) onClose() } - async function handleSubmit(e: React.FormEvent | React.MouseEvent) { + function handleSystemAdminChange(checked: boolean) { + setIsSystemAdmin(checked) + } + + function handleSubmit(e: React.FormEvent | React.MouseEvent) { e.preventDefault() let valid = true @@ -125,6 +148,13 @@ function CreateUserDialog({ open, onClose }: CreateUserDialogProps) { setPasswordError(undefined) } + if (!orgId) { + setOrgError('Organization is required') + valid = false + } else { + setOrgError(undefined) + } + if (!valid) return const params: CreateUserParams = { @@ -132,6 +162,8 @@ function CreateUserDialog({ open, onClose }: CreateUserDialogProps) { display_name: trimmedName, password, is_system_admin: isSystemAdmin, + org_id: orgId, + role: 'member', } createUser.mutate(params, { @@ -180,11 +212,23 @@ function CreateUserDialog({ open, onClose }: CreateUserDialogProps) {
+