From af41d4bdd9119ba8195dfe8ea169bdbc3b677441 Mon Sep 17 00:00:00 2001 From: alex-dembele Date: Tue, 29 Sep 2026 18:27:43 +0100 Subject: [PATCH 1/5] fix(users): remove the /users/:id writes that act on the global account (#807) PATCH /users/:id/status, PATCH /users/:id/role and DELETE /users/:id wrote the users row itself. An admin of one organization could deactivate a person in every organization they belong to, or delete the account everywhere, and the role route reported success for a write no session ever reads. No live consumer exists: Settings uses /organization/members/:memberId, and the useUsers() hook that called these routes is imported nowhere. So the routes are removed rather than reimplemented. POST /users also dropped its in-handler admin check, which read the global users.role_id. The RequireRole("admin") route guard already authorizes it from the session's role in the active organization. Signed-off-by: alex-dembele --- backend/cmd/server/main.go | 8 +- backend/internal/handler/user_handler.go | 205 +----------------- .../internal/handler/user_isolation_test.go | 12 +- 3 files changed, 12 insertions(+), 213 deletions(-) diff --git a/backend/cmd/server/main.go b/backend/cmd/server/main.go index fd47317e..58a7d46b 100644 --- a/backend/cmd/server/main.go +++ b/backend/cmd/server/main.go @@ -2049,9 +2049,11 @@ func main() { adminRole := middleware.RequireRole("admin") protected.Get("/users", adminRole, handlers.GetUsers) protected.Post("/users", adminRole, handlers.CreateUser) - protected.Patch("/users/:id/status", adminRole, handlers.UpdateUserStatus) - protected.Patch("/users/:id/role", adminRole, handlers.UpdateUserRole) - protected.Delete("/users/:id", adminRole, handlers.DeleteUser) + // PATCH /users/:id/status, PATCH /users/:id/role and DELETE /users/:id were + // removed (#807). They wrote the global users row, so an admin of one + // organization could lock a person out of every other one, or delete them + // everywhere. A member's role and status are per organization and live on + // /organization/members/:memberId/{role,status}. // Self-service profile, preferences and avatar (#719, replaces the // misleading PATCH /users/:id of #574). Every verb acts on the session's // own user, so the session is the authorization. The avatar read is gated diff --git a/backend/internal/handler/user_handler.go b/backend/internal/handler/user_handler.go index 18e23f5b..52f2a872 100644 --- a/backend/internal/handler/user_handler.go +++ b/backend/internal/handler/user_handler.go @@ -18,14 +18,6 @@ import ( "github.com/opendefender/openrisk/internal/service" ) -type UpdateUserStatusInput struct { - IsActive bool `json:"is_active"` -} - -type UpdateUserRoleInput struct { - Role string `json:"role" validate:"required"` -} - type CreateUserInput struct { Email string `json:"email" validate:"required,email"` Username string `json:"username" validate:"required,min=3"` @@ -62,22 +54,6 @@ func auditTenant(c *fiber.Ctx) *uuid.UUID { return nil } -// userInTenant reports whether the target user is a member of the caller's -// organization. domain.User is many-to-many with organizations via -// OrganizationMember, so the legacy /users management endpoints must scope every -// action to the caller's tenant — an admin may only see/modify/delete users who -// share their tenant (RULE #2), never every user in the deployment. -func userInTenant(userID, tenantID uuid.UUID) bool { - if tenantID == uuid.Nil { - return false - } - var count int64 - database.DB.Model(&domain.OrganizationMember{}). - Where("organization_id = ? AND user_id = ?", tenantID, userID). - Count(&count) - return count > 0 -} - // GetUsers retrieves the users in the caller's tenant (admin only) func GetUsers(c *fiber.Ctx) error { claims := middleware.GetUserClaims(c) @@ -128,174 +104,6 @@ func GetUsers(c *fiber.Ctx) error { return c.Status(fiber.StatusOK).JSON(response) } -// UpdateUserStatus enables or disables a user (admin only) -func UpdateUserStatus(c *fiber.Ctx) error { - ipAddress := c.IP() - userAgent := c.Get("User-Agent") - - claims := middleware.GetUserClaims(c) - if claims == nil { - return c.Status(fiber.StatusUnauthorized).JSON(fiber.Map{"error": "Unauthorized"}) - } - - // Check if user is admin - var currentUser domain.User - if err := database.DB.Preload("Role").First(¤tUser, "id = ?", claims.Sub).Error; err != nil { - return c.Status(fiber.StatusNotFound).JSON(fiber.Map{"error": "User not found"}) - } - - if currentUser.Role.Name != "admin" { - return c.Status(fiber.StatusForbidden).JSON(fiber.Map{"error": "Only admins can update user status"}) - } - - userID := c.Params("id") - input := new(UpdateUserStatusInput) - if err := c.BodyParser(input); err != nil { - return c.Status(fiber.StatusBadRequest).JSON(fiber.Map{"error": "Invalid input"}) - } - - var targetUser domain.User - if err := database.DB.First(&targetUser, "id = ?", userID).Error; err != nil { - return c.Status(fiber.StatusNotFound).JSON(fiber.Map{"error": "User not found"}) - } - - // The target must belong to the caller's tenant. - if !userInTenant(targetUser.ID, safeGetUUID(c, "tenant_id")) { - return c.Status(fiber.StatusNotFound).JSON(fiber.Map{"error": "User not found"}) - } - - targetUser.IsActive = input.IsActive - if err := database.DB.Save(&targetUser).Error; err != nil { - return c.Status(fiber.StatusInternalServerError).JSON(fiber.Map{"error": "Failed to update user"}) - } - - // Log the action - var action domain.AuditLogAction - if input.IsActive { - action = domain.ActionUserActivate - } else { - action = domain.ActionUserDeactivate - } - - _ = auditService.LogAction(&domain.AuditLog{ - TenantID: auditTenant(c), - UserID: &claims.Sub, - Action: action, - Resource: domain.ResourceUser, - ResourceID: &targetUser.ID, - Result: domain.ResultSuccess, - IPAddress: parseIPAddressHelper(ipAddress), - UserAgent: userAgent, - }) - - return c.Status(fiber.StatusOK).JSON(fiber.Map{"message": "User status updated"}) -} - -// UpdateUserRole changes a user's role (admin only) -func UpdateUserRole(c *fiber.Ctx) error { - ipAddress := c.IP() - userAgent := c.Get("User-Agent") - - claims := middleware.GetUserClaims(c) - if claims == nil { - return c.Status(fiber.StatusUnauthorized).JSON(fiber.Map{"error": "Unauthorized"}) - } - - // Check if user is admin - var currentUser domain.User - if err := database.DB.Preload("Role").First(¤tUser, "id = ?", claims.Sub).Error; err != nil { - return c.Status(fiber.StatusNotFound).JSON(fiber.Map{"error": "User not found"}) - } - - if currentUser.Role.Name != "admin" { - return c.Status(fiber.StatusForbidden).JSON(fiber.Map{"error": "Only admins can update user roles"}) - } - - userID := c.Params("id") - input := new(UpdateUserRoleInput) - if err := c.BodyParser(input); err != nil { - return c.Status(fiber.StatusBadRequest).JSON(fiber.Map{"error": "Invalid input"}) - } - - // Get the role - var role domain.Role - if err := database.DB.Where("name = ?", input.Role).First(&role).Error; err != nil { - return c.Status(fiber.StatusNotFound).JSON(fiber.Map{"error": "Role not found"}) - } - - var targetUser domain.User - if err := database.DB.Preload("Role").First(&targetUser, "id = ?", userID).Error; err != nil { - return c.Status(fiber.StatusNotFound).JSON(fiber.Map{"error": "User not found"}) - } - - // The target must belong to the caller's tenant. - if !userInTenant(targetUser.ID, safeGetUUID(c, "tenant_id")) { - return c.Status(fiber.StatusNotFound).JSON(fiber.Map{"error": "User not found"}) - } - - oldRole := "" - if targetUser.Role != nil { - oldRole = targetUser.Role.Name - } - targetUser.RoleID = role.ID - if err := database.DB.Save(&targetUser).Error; err != nil { - return c.Status(fiber.StatusInternalServerError).JSON(fiber.Map{"error": "Failed to update user role"}) - } - - // Log the role change - _ = auditService.LogRoleChange(auditTenant(c), claims.Sub, targetUser.ID, oldRole, input.Role, ipAddress, userAgent) - - return c.Status(fiber.StatusOK).JSON(fiber.Map{"message": "User role updated"}) -} - -// DeleteUser deletes a user (admin only) -func DeleteUser(c *fiber.Ctx) error { - ipAddress := c.IP() - userAgent := c.Get("User-Agent") - - claims := middleware.GetUserClaims(c) - if claims == nil { - return c.Status(fiber.StatusUnauthorized).JSON(fiber.Map{"error": "Unauthorized"}) - } - - // Check if user is admin - var currentUser domain.User - if err := database.DB.Preload("Role").First(¤tUser, "id = ?", claims.Sub).Error; err != nil { - return c.Status(fiber.StatusNotFound).JSON(fiber.Map{"error": "User not found"}) - } - - if currentUser.Role.Name != "admin" { - return c.Status(fiber.StatusForbidden).JSON(fiber.Map{"error": "Only admins can delete users"}) - } - - userID := c.Params("id") - - // Prevent admin from deleting their own account - if userID == claims.Sub.String() { - return c.Status(fiber.StatusBadRequest).JSON(fiber.Map{"error": "Cannot delete your own account"}) - } - - // Get the target user to pass to audit log - var targetUser domain.User - if err := database.DB.First(&targetUser, "id = ?", userID).Error; err != nil { - return c.Status(fiber.StatusNotFound).JSON(fiber.Map{"error": "User not found"}) - } - - // The target must belong to the caller's tenant. - if !userInTenant(targetUser.ID, safeGetUUID(c, "tenant_id")) { - return c.Status(fiber.StatusNotFound).JSON(fiber.Map{"error": "User not found"}) - } - - if err := database.DB.Delete(&domain.User{}, "id = ?", userID).Error; err != nil { - return c.Status(fiber.StatusInternalServerError).JSON(fiber.Map{"error": "Failed to delete user"}) - } - - // Log the user deletion - _ = auditService.LogUserDelete(auditTenant(c), claims.Sub, targetUser.ID, ipAddress, userAgent) - - return c.Status(fiber.StatusNoContent).Send([]byte{}) -} - // Helper function to parse IP address func parseIPAddressHelper(ipStr string) *net.IP { if ipStr == "" { @@ -312,15 +120,10 @@ func CreateUser(c *fiber.Ctx) error { return c.Status(fiber.StatusUnauthorized).JSON(fiber.Map{"error": "Unauthorized"}) } - // Check if user is admin - var currentUser domain.User - if err := database.DB.Preload("Role").First(¤tUser, "id = ?", claims.Sub).Error; err != nil { - return c.Status(fiber.StatusNotFound).JSON(fiber.Map{"error": "User not found"}) - } - - if currentUser.Role.Name != "admin" { - return c.Status(fiber.StatusForbidden).JSON(fiber.Map{"error": "Only admins can create users"}) - } + // Authorization is the RequireRole("admin") route guard, which reads the + // caller's role in the ACTIVE organization from the signed session. The + // in-handler check this replaced read users.role_id, a global column no + // session derives its role from (#807). input := new(CreateUserInput) if err := c.BodyParser(input); err != nil { diff --git a/backend/internal/handler/user_isolation_test.go b/backend/internal/handler/user_isolation_test.go index 8ff8d189..ac2d8dc1 100644 --- a/backend/internal/handler/user_isolation_test.go +++ b/backend/internal/handler/user_isolation_test.go @@ -40,9 +40,9 @@ func addUser(t *testing.T, db *gorm.DB, org, user uuid.UUID) { uuid.NewString(), org.String(), user.String(), "user").Error) } -// TestUser_TenantScoping proves the legacy /users management surface is scoped to -// the caller's organization. It previously operated on domain.User globally, so -// an admin of tenant A could list, modify or delete tenant B's users. +// TestUser_TenantScoping proves the legacy /users list is scoped to the caller's +// organization. It previously returned every user in the deployment. The write +// routes that used to sit beside it were removed in #807. func TestUser_TenantScoping(t *testing.T) { db := withUserDB(t) tenantA := uuid.New() @@ -52,12 +52,6 @@ func TestUser_TenantScoping(t *testing.T) { addUser(t, db, tenantA, userA) addUser(t, db, tenantB, userB) - // userInTenant gates every write path (status/role/delete). - require.True(t, userInTenant(userA, tenantA)) - require.False(t, userInTenant(userA, tenantB), "A's user is not in tenant B") - require.False(t, userInTenant(userB, tenantA), "B's user is not in tenant A") - require.False(t, userInTenant(userA, uuid.Nil), "nil tenant is denied (fail closed)") - // GetUsers list predicate: the pluck-then-IN scoping returns only the caller's // members, never every user in the deployment. var idsForA []uuid.UUID From 0a6e2619cf7426b10d0f370b6605d544cf157976 Mon Sep 17 00:00:00 2001 From: alex-dembele Date: Tue, 29 Sep 2026 18:31:55 +0100 Subject: [PATCH 2/5] fix(auth): sign in to another organization when the default one withdrew access (#807) Login only tried the user's default organization. If that organization deactivated or revoked the membership, the sign-in was refused outright, even when another organization still granted access. So an admin of A could lock a person out of B just by A being their default. When the default membership no longer grants access, login now picks the earliest-joined active membership in another active organization. With no such membership the refusal is unchanged. Signed-off-by: alex-dembele --- backend/internal/application/auth/login.go | 44 +++++++++ .../application/auth/login_fallback_test.go | 96 +++++++++++++++++++ 2 files changed, 140 insertions(+) create mode 100644 backend/internal/application/auth/login_fallback_test.go diff --git a/backend/internal/application/auth/login.go b/backend/internal/application/auth/login.go index caf4d6b4..725545ae 100644 --- a/backend/internal/application/auth/login.go +++ b/backend/internal/application/auth/login.go @@ -211,6 +211,17 @@ func (uc *LoginUseCase) Execute(ctx context.Context, input LoginInput) (*LoginOu if err != nil { return nil, fmt.Errorf("failed to get organization membership: %w", err) } + if member != nil && !member.IsActive { + // The default organization withdrew this person's access. That is its + // decision about itself only: if another organization still grants + // access, sign them in there. Otherwise one organization could lock a + // person out of every other one just by being their default (#807). + if alt, altErr := uc.fallbackMembership(ctx, user.ID, org.ID); altErr != nil { + return nil, altErr + } else if alt != nil { + org, member = alt.Organization, alt + } + } if member != nil { // A revoked (deactivated) membership must not yield a session, even though // the user account itself is active: being removed from an organization is @@ -378,6 +389,39 @@ func (uc *LoginUseCase) decideMFA(ctx context.Context, member *domain.Organizati return domain.DecideMFA(in) } +// activeMembershipLister is the optional read login uses to find another +// organization to sign a person into when their default one withdrew access. +// GormUserRepository implements it; a repository without it keeps the old +// behavior of refusing the sign-in. +type activeMembershipLister interface { + ListActiveMemberships(ctx context.Context, userID uuid.UUID) ([]*domain.OrganizationMember, error) +} + +// fallbackMembership returns the user's earliest-joined active membership in +// an active organization other than exclude, or nil when there is none. +func (uc *LoginUseCase) fallbackMembership(ctx context.Context, userID, exclude uuid.UUID) (*domain.OrganizationMember, error) { + lister, ok := uc.userRepo.(activeMembershipLister) + if !ok { + return nil, nil + } + memberships, err := lister.ListActiveMemberships(ctx, userID) + if err != nil { + return nil, fmt.Errorf("failed to list organization memberships: %w", err) + } + var best *domain.OrganizationMember + for _, m := range memberships { + if m == nil || !m.IsActive || m.OrganizationID == exclude || + m.Organization == nil || !m.Organization.IsActive { + continue + } + if best == nil || m.JoinedAt.Before(best.JoinedAt) || + (m.JoinedAt.Equal(best.JoinedAt) && m.OrganizationID.String() < best.OrganizationID.String()) { + best = m + } + } + return best, nil +} + // UserRepository interface for user operations type UserRepository interface { GetByEmail(ctx context.Context, email string) (*domain.User, error) diff --git a/backend/internal/application/auth/login_fallback_test.go b/backend/internal/application/auth/login_fallback_test.go new file mode 100644 index 00000000..7c802129 --- /dev/null +++ b/backend/internal/application/auth/login_fallback_test.go @@ -0,0 +1,96 @@ +// Copyright (c) 2026 OpenDefender Contributors +// SPDX-License-Identifier: AGPL-3.0-only +// This program is free software: you can redistribute it and/or modify it under +// the terms of the GNU Affero General Public License v3.0 (see LICENSE). + +package auth + +import ( + "context" + "errors" + "testing" + "time" + + "github.com/google/uuid" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/opendefender/openrisk/internal/domain" +) + +// #807 — the organization login falls back to when the default one withdrew +// access. The end-to-end proof (a real membership write in A, a real sign-in +// landing in B) is handler.TestCrossTenant_AdminOfAWithdrawingAccessLeavesBUntouched. + +// listingUsers is loginUsers plus the optional membership listing. +type listingUsers struct { + loginUsers + active []*domain.OrganizationMember + err error +} + +func (l *listingUsers) ListActiveMemberships(context.Context, uuid.UUID) ([]*domain.OrganizationMember, error) { + return l.active, l.err +} + +func activeIn(org *domain.Organization, joined time.Time) *domain.OrganizationMember { + return &domain.OrganizationMember{ + ID: uuid.New(), OrganizationID: org.ID, Organization: org, + Role: domain.RoleUser, IsActive: true, JoinedAt: joined, + } +} + +func TestLoginFallback_Success_PicksEarliestActiveOtherOrganization(t *testing.T) { + def := &domain.Organization{ID: uuid.New(), IsActive: true} + older := &domain.Organization{ID: uuid.New(), IsActive: true} + newer := &domain.Organization{ID: uuid.New(), IsActive: true} + closed := &domain.Organization{ID: uuid.New(), IsActive: false} + + repo := &listingUsers{active: []*domain.OrganizationMember{ + activeIn(def, loginNow.Add(-72*time.Hour)), // the default itself is never chosen + activeIn(newer, loginNow.Add(-1*time.Hour)), + activeIn(closed, loginNow.Add(-96*time.Hour)), // an inactive organization is skipped + activeIn(older, loginNow.Add(-48*time.Hour)), + }} + uc := &LoginUseCase{userRepo: repo} + + got, err := uc.fallbackMembership(context.Background(), uuid.New(), def.ID) + require.NoError(t, err) + require.NotNil(t, got) + assert.Equal(t, older.ID, got.OrganizationID) +} + +func TestLoginFallback_NotFound_NoOtherActiveMembership(t *testing.T) { + def := &domain.Organization{ID: uuid.New(), IsActive: true} + uc := &LoginUseCase{userRepo: &listingUsers{active: []*domain.OrganizationMember{activeIn(def, loginNow)}}} + + got, err := uc.fallbackMembership(context.Background(), uuid.New(), def.ID) + require.NoError(t, err) + assert.Nil(t, got) + + // A repository that cannot list memberships keeps the old refusal. + uc = &LoginUseCase{userRepo: &loginUsers{}} + got, err = uc.fallbackMembership(context.Background(), uuid.New(), def.ID) + require.NoError(t, err) + assert.Nil(t, got) +} + +func TestLoginFallback_Unauthorized_WithdrawnDefaultWithNowhereElseIsRefused(t *testing.T) { + uc, users, _, _ := newLoginHarness(t, domain.RoleUser) + users.member.IsActive = false + users.member.Status = domain.MembershipRevoked + + out, err := uc.Execute(context.Background(), LoginInput{ + Email: "admin@opendefender.io", Password: "Ancre-Vitrail7-Cobalt", + }) + require.Error(t, err) + assert.Nil(t, out) + assert.Contains(t, err.Error(), "revoked") +} + +func TestLoginFallback_ListingErrorIsNotASession(t *testing.T) { + uc := &LoginUseCase{userRepo: &listingUsers{err: errors.New("db down")}} + got, err := uc.fallbackMembership(context.Background(), uuid.New(), uuid.New()) + require.Error(t, err) + assert.Nil(t, got) +} From 034ea49eeb996e28445a01904fce5cd671e8bdf4 Mon Sep 17 00:00:00 2001 From: alex-dembele Date: Tue, 29 Sep 2026 18:31:55 +0100 Subject: [PATCH 3/5] test(users): prove a membership change in one tenant leaves the others alone (#807) Drives the real membership route in tenant A, then signs the person in through the real login use case: they land in B with B's role, and the users row and the B membership are unchanged. Also proves users.role_id = admin grants nothing, and pins the three removed /users/:id routes out of the router. Signed-off-by: alex-dembele --- .../handler/authz_route_coverage_test.go | 23 ++ .../handler/user_cross_tenant_test.go | 265 ++++++++++++++++++ 2 files changed, 288 insertions(+) create mode 100644 backend/internal/handler/user_cross_tenant_test.go diff --git a/backend/internal/handler/authz_route_coverage_test.go b/backend/internal/handler/authz_route_coverage_test.go index 6d77a74e..f93b90ca 100644 --- a/backend/internal/handler/authz_route_coverage_test.go +++ b/backend/internal/handler/authz_route_coverage_test.go @@ -337,3 +337,26 @@ func TestProtectedRoutes_MostRoutesCarryAPermissionGuard(t *testing.T) { assert.Greater(t, guarded*100/len(routes), 60, "fewer than 60%% of protected routes carry a permission guard — something was dropped wholesale") } + +// #807 — these routes wrote the global users row, so an administrator of one +// organization could deactivate or delete a person in every organization they +// belong to. They were removed, not guarded; per-organization role and status +// live on /organization/members/:memberId. Mounting any of them again, under +// any guard, fails here. +var removedGlobalAccountRoutes = []string{ + "PATCH /users/:id/status", + "PATCH /users/:id/role", + "DELETE /users/:id", +} + +func TestProtectedRoutes_GlobalAccountWritesStayRemoved(t *testing.T) { + mounted := map[string]bool{} + for _, r := range parseProtectedRoutes(t) { + mounted[routeKey(r)] = true + } + for _, k := range removedGlobalAccountRoutes { + assert.False(t, mounted[k], "%s acts on the global account, not the caller's membership (#807)", k) + } + // Guard against a parser that silently matches nothing. + assert.True(t, mounted["GET /users"], "the parser no longer sees the /users block") +} diff --git a/backend/internal/handler/user_cross_tenant_test.go b/backend/internal/handler/user_cross_tenant_test.go new file mode 100644 index 00000000..e895660f --- /dev/null +++ b/backend/internal/handler/user_cross_tenant_test.go @@ -0,0 +1,265 @@ +// Copyright (c) 2026 OpenDefender Contributors +// SPDX-License-Identifier: AGPL-3.0-only +// This program is free software: you can redistribute it and/or modify it under +// the terms of the GNU Affero General Public License v3.0 (see LICENSE). + +package handler_test + +import ( + "context" + "crypto/rand" + "crypto/rsa" + "net/http" + "net/http/httptest" + "strings" + "testing" + + "github.com/gofiber/fiber/v2" + "github.com/google/uuid" + + authapp "github.com/opendefender/openrisk/internal/application/auth" + coreauth "github.com/opendefender/openrisk/internal/auth" + "github.com/opendefender/openrisk/internal/domain" + "github.com/opendefender/openrisk/internal/infrastructure/repository" + "github.com/opendefender/openrisk/internal/middleware" + "github.com/opendefender/openrisk/internal/testsupport/sqliteschema" + authpkg "github.com/opendefender/openrisk/pkg/auth" +) + +// --------------------------------------------------------------------------- +// #807 — nothing done in the context of organization A may change whether a +// person can sign in to organization B, or what they can do there. +// +// The routes that broke this (PATCH /users/:id/status, PATCH /users/:id/role, +// DELETE /users/:id) are gone; authz_route_coverage_test.go keeps them gone. +// What remains for A's administrator is the membership route, and these tests +// drive it for real, then sign the person in through the real login use case +// against the same database. +// --------------------------------------------------------------------------- + +// verbatimHasher treats the stored hash as "hashed:", like the rest +// of the suite's fakes. +type verbatimHasher struct{} + +func (verbatimHasher) Hash(p string) (string, error) { return "hashed:" + p, nil } +func (verbatimHasher) Verify(h, p string) bool { return h == "hashed:"+p } + +const dualPassword = "Ardoise-Lanterne-42" + +type signInRig struct { + login *authapp.LoginUseCase + keys *authpkg.RSAKeys +} + +// newSignInRig adds what login needs on top of the membership fixture: the +// organizations the memberships point at, and a refresh-token store. +func newSignInRig(t *testing.T, f *orgFixture) *signInRig { + t.Helper() + for _, ddl := range []string{ + `CREATE TABLE organizations (id TEXT PRIMARY KEY)`, + `CREATE TABLE refresh_tokens ( + id TEXT PRIMARY KEY, user_id TEXT NOT NULL, tenant_id TEXT NOT NULL, + family_id TEXT NOT NULL, token_hash TEXT NOT NULL UNIQUE, + device_fingerprint TEXT, ip_address TEXT, user_agent TEXT, + expires_at DATETIME NOT NULL, rotated_at DATETIME, last_used_at DATETIME, + created_at DATETIME, updated_at DATETIME)`, + } { + if err := f.db.Exec(ddl).Error; err != nil { + t.Fatalf("create table: %v", err) + } + } + if err := sqliteschema.Reconcile(f.db, "organizations", &domain.Organization{}); err != nil { + t.Fatalf("reconcile organizations: %v", err) + } + for _, o := range []domain.Organization{ + {ID: f.tenantA, Name: "Banque Atlantique", Slug: "banque-atlantique", IsActive: true}, + {ID: f.tenantB, Name: "Other Corp", Slug: "other-corp", IsActive: true}, + } { + o := o + if err := f.db.Create(&o).Error; err != nil { + t.Fatalf("seed organization: %v", err) + } + } + + priv, err := rsa.GenerateKey(rand.Reader, 2048) + if err != nil { + t.Fatalf("rsa: %v", err) + } + keys := &authpkg.RSAKeys{PrivateKey: priv, PublicKey: &priv.PublicKey} + login := authapp.NewLoginUseCase(repository.NewGormUserRepository(f.db), coreauth.NewTokenManager(f.db, keys), verbatimHasher{}) + return &signInRig{login: login, keys: keys} +} + +// session signs the person in and returns the organization and the claims the +// minted access token actually carries. A refused sign-in fails the test. +func (r *signInRig) session(t *testing.T, email string) (*domain.Organization, *authpkg.Claims) { + t.Helper() + out, err := r.login.Execute(context.Background(), authapp.LoginInput{Email: email, Password: dualPassword}) + if err != nil { + t.Fatalf("sign-in of %s refused: %v", email, err) + } + if out.TokenPair == nil { + t.Fatalf("sign-in of %s issued no session", email) + } + claims, err := authpkg.ValidateAccessToken(r.keys, out.TokenPair.AccessToken, nil) + if err != nil { + t.Fatalf("minted token does not validate: %v", err) + } + return out.Organization, claims +} + +// seedDualMember creates one person with an active membership in A (their +// default organization, the worst case) and in B, with a different role in each. +func seedDualMember(t *testing.T, f *orgFixture) (user *domain.User, inA, inB *domain.OrganizationMember) { + t.Helper() + user = &domain.User{ + ID: uuid.New(), Email: "dual@both.io", Username: "dual", FullName: "Dual Member", + IsActive: true, Password: "hashed:" + dualPassword, DefaultOrgID: &f.tenantA, + } + if err := f.db.Create(user).Error; err != nil { + t.Fatalf("seed user: %v", err) + } + mk := func(org uuid.UUID, role domain.MemberRole, business domain.BusinessRoleKey) *domain.OrganizationMember { + m := &domain.OrganizationMember{ + ID: uuid.New(), OrganizationID: org, UserID: user.ID, Role: role, BusinessRole: business, + Status: domain.MembershipActive, IsActive: true, + JoinedAt: f.now, CreatedAt: f.now, UpdatedAt: f.now, + } + if err := f.db.Create(m).Error; err != nil { + t.Fatalf("seed member: %v", err) + } + return m + } + inA = mk(f.tenantA, domain.RoleUser, "") + inB = mk(f.tenantB, domain.RoleAdmin, "") + return user, inA, inB +} + +func TestCrossTenant_AdminOfAWithdrawingAccessLeavesBUntouched(t *testing.T) { + for _, status := range []domain.MembershipStatus{domain.MembershipDeactivated, domain.MembershipRevoked} { + t.Run(string(status), func(t *testing.T) { + f := newOrgFixture(t) + rig := newSignInRig(t, f) + user, inA, inB := seedDualMember(t, f) + + // Before: the default organization is A. + org, claims := rig.session(t, user.Email) + if org.ID != f.tenantA || claims.TenantID != f.tenantA { + t.Fatalf("precondition: expected a session in A, got %s", org.ID) + } + + // A's administrator withdraws access in A. + f.mustCall(t, "admin@a.io", http.MethodPut, + "/api/v1/organization/members/"+inA.ID.String()+"/status", + map[string]any{"status": string(status)}, http.StatusOK) + + // After: the person still signs in, and lands in B with B's role. + org, claims = rig.session(t, user.Email) + if org.ID != f.tenantB || claims.TenantID != f.tenantB { + t.Fatalf("expected a session in B after A withdrew access, got %s", claims.TenantID) + } + if got := claims.OrgRoles[f.tenantB]; got != string(domain.RoleAdmin) { + t.Fatalf("role in B = %q, want %q", got, domain.RoleAdmin) + } + if _, leaked := claims.OrgRoles[f.tenantA]; leaked { + t.Fatal("the session in B must carry no role in A") + } + t.Logf("after A set %s: sign-in lands in B (%s) with org_roles=%v", status, claims.TenantID, claims.OrgRoles) + + // The account and B's membership are byte-for-byte what they were. + var storedUser domain.User + f.db.First(&storedUser, "id = ?", user.ID) + if !storedUser.IsActive { + t.Fatal("users.is_active was changed by a membership action in A") + } + var storedB domain.OrganizationMember + f.db.First(&storedB, "id = ?", inB.ID) + if storedB.Role != inB.Role || storedB.Status != domain.MembershipActive || !storedB.IsActive { + t.Fatalf("B membership changed: role=%s status=%s active=%v", storedB.Role, storedB.Status, storedB.IsActive) + } + var storedA domain.OrganizationMember + f.db.First(&storedA, "id = ?", inA.ID) + if storedA.Status != status || storedA.IsActive { + t.Fatalf("A membership should be %s, got %s active=%v", status, storedA.Status, storedA.IsActive) + } + }) + } +} + +// The fallback opens nothing that was closed: a person whose only membership +// was withdrawn is still refused. +func TestCrossTenant_SoleMembershipWithdrawnStillRefusesSignIn(t *testing.T) { + f := newOrgFixture(t) + rig := newSignInRig(t, f) + user, inA, inB := seedDualMember(t, f) + for _, m := range []struct { + actor string + id uuid.UUID + }{{"admin@a.io", inA.ID}, {"admin@b.io", inB.ID}} { + f.mustCall(t, m.actor, http.MethodPut, "/api/v1/organization/members/"+m.id.String()+"/status", + map[string]any{"status": string(domain.MembershipRevoked)}, http.StatusOK) + } + out, err := rig.login.Execute(context.Background(), authapp.LoginInput{Email: user.Email, Password: dualPassword}) + if err == nil || out != nil { + t.Fatalf("a person with no active membership must not get a session, got %+v", out) + } + if !strings.Contains(err.Error(), "revoked") { + t.Fatalf("unexpected refusal: %v", err) + } +} + +// Criterion 3: users.role_id is not an input to any authorization decision. +// A person whose global role row says admin, but whose membership says user, +// gets a user session and is refused by the admin gate on /users. +func TestCrossTenant_GlobalRoleIDGrantsNothing(t *testing.T) { + f := newOrgFixture(t) + rig := newSignInRig(t, f) + user, _, _ := seedDualMember(t, f) + + if err := f.db.Exec(`CREATE TABLE roles (id TEXT PRIMARY KEY)`).Error; err != nil { + t.Fatalf("create roles: %v", err) + } + if err := sqliteschema.Reconcile(f.db, "roles", &domain.Role{}); err != nil { + t.Fatalf("reconcile roles: %v", err) + } + adminRole := domain.Role{ID: uuid.New(), Name: "admin"} + if err := f.db.Create(&adminRole).Error; err != nil { + t.Fatalf("seed role: %v", err) + } + if err := f.db.Model(&domain.User{}).Where("id = ?", user.ID).Update("role_id", adminRole.ID).Error; err != nil { + t.Fatalf("set role_id: %v", err) + } + + _, claims := rig.session(t, user.Email) + if got := claims.OrgRoles[f.tenantA]; got != string(domain.RoleUser) { + t.Fatalf("session role = %q, want the membership's %q", got, domain.RoleUser) + } + if claims.HasPermission("*") { + t.Fatal("users.role_id = admin must not put a wildcard in the session") + } + + // The /users admin gate, fed exactly what the auth middleware stamps from + // that token. The handler is a sentinel: reaching it is the failure. + app := fiber.New() + stamp := func(c *fiber.Ctx) error { + c.Locals("user", claims) + c.Locals("org_roles", claims.OrgRoles) + c.Locals("permissions", claims.Permissions) + c.Locals("tenant_id", claims.TenantID) + return c.Next() + } + reached := func(c *fiber.Ctx) error { return c.SendStatus(http.StatusTeapot) } + gate := middleware.RequireRole("admin") + app.Get("/users", stamp, gate, reached) + app.Post("/users", stamp, gate, reached) + + for _, method := range []string{http.MethodGet, http.MethodPost} { + res, err := app.Test(httptest.NewRequest(method, "/users", nil)) + if err != nil { + t.Fatalf("%s /users: %v", method, err) + } + if res.StatusCode != http.StatusForbidden { + t.Fatalf("%s /users with users.role_id=admin and membership role user: got %d, want 403", method, res.StatusCode) + } + } +} From 504ee0dae742c30fb9ff6245099056ad19db5659 Mon Sep 17 00:00:00 2001 From: alex-dembele Date: Tue, 29 Sep 2026 18:33:22 +0100 Subject: [PATCH 4/5] chore(settings): remove the dead useUsers hook and AdminUser type (#807) useUsers() was imported nowhere and was the only client of the removed /users/:id status, role and delete routes. Signed-off-by: alex-dembele --- frontend/src/features/settings/adminData.ts | 52 ++------------------- 1 file changed, 5 insertions(+), 47 deletions(-) diff --git a/frontend/src/features/settings/adminData.ts b/frontend/src/features/settings/adminData.ts index 1d4daa24..1c06f3d5 100644 --- a/frontend/src/features/settings/adminData.ts +++ b/frontend/src/features/settings/adminData.ts @@ -1,57 +1,15 @@ // Copyright (c) 2026 OpenDefender Contributors // SPDX-License-Identifier: AGPL-3.0-only // -// Data hooks for the admin features consolidated into Settings. Members, API -// Tokens and Custom Fields are live; Roles / Organizations / Audit-log endpoints -// currently 500 (their tables aren't migrated in this schema) so their hooks -// surface an error the UI degrades on gracefully. +// Data hooks for the admin features consolidated into Settings. API Tokens and +// Custom Fields are live; Members use /organization/members (#807). Roles / +// Organizations / Audit-log endpoints currently 500 (their tables aren't +// migrated in this schema) so their hooks surface an error the UI degrades on +// gracefully. import { useMutation, useQuery, useQueryClient } from '@tanstack/react-query'; import { api } from '../../lib/api'; -/* ---------------- Members (/users) ---------------- */ -export interface AdminUser { - id: string; - email: string; - username: string; - full_name: string; - role: string; - is_active: boolean; - created_at: string; - last_login?: string; -} - -export function useUsers() { - const qc = useQueryClient(); - const query = useQuery({ - queryKey: ['admin', 'users'], - queryFn: async () => (await api.get('/users')).data ?? [], - }); - const invalidate = () => qc.invalidateQueries({ queryKey: ['admin', 'users'] }); - const setStatus = useMutation({ - mutationFn: ({ id, is_active }: { id: string; is_active: boolean }) => - api.patch(`/users/${id}/status`, { is_active }), - onSuccess: invalidate, - }); - const setRole = useMutation({ - mutationFn: ({ id, role }: { id: string; role: string }) => - api.patch(`/users/${id}/role`, { role }), - onSuccess: invalidate, - }); - const remove = useMutation({ - mutationFn: (id: string) => api.delete(`/users/${id}`), - onSuccess: invalidate, - }); - return { - users: query.data ?? [], - isLoading: query.isLoading, - isError: query.isError, - setStatus, - setRole, - remove, - }; -} - /* ---------------- API Tokens (/tokens) ---------------- */ export interface ApiToken { id: string; From 5f5e107146afda47f5df711af3cf5d4431f0ad74 Mon Sep 17 00:00:00 2001 From: alex-dembele Date: Tue, 29 Sep 2026 18:54:49 +0100 Subject: [PATCH 5/5] test(security): drop the isolation decision for the removed /users/{id} routes (#807) TestNoStaleDecisions failed: /api/v1/users/{id} no longer matches a live route. The /api/v1/users/* entry now covers only the avatar read, so it cites the avatar's cross-tenant test. Signed-off-by: alex-dembele --- backend/internal/security/isolation/registry.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/backend/internal/security/isolation/registry.go b/backend/internal/security/isolation/registry.go index 82edc370..866e5f68 100644 --- a/backend/internal/security/isolation/registry.go +++ b/backend/internal/security/isolation/registry.go @@ -131,10 +131,10 @@ var decisions = []Decision{ "handler/team_isolation_test TestTeam_TenantIsolationPredicate"}, {"/api/v1/teams/*", Covered, "handler/team_isolation_test covers member add/remove paths"}, - {"/api/v1/users/{id}", Covered, - "handler/user_isolation_test TestUser_TenantScoping"}, + // /api/v1/users/{id} (status, role, delete) was removed in #807; the only + // sub-route left is the avatar read. {"/api/v1/users/*", Covered, - "handler/user_isolation_test covers status/role/delete by id"}, + "application/profile TestGetAvatar_CrossTenantIsIndistinguishableFromMissing: the avatar read is gated by the target's membership in the caller's tenant"}, {"/api/v1/notifications/{id}", Covered, "repository/notification_repository_test scopes by user and tenant"}, {"/api/v1/notifications/{id}/read", Covered,