From ee9a07d96414b637ffc3a99c1be772886b9819b3 Mon Sep 17 00:00:00 2001 From: alex-dembele Date: Tue, 29 Sep 2026 18:49:17 +0100 Subject: [PATCH 1/3] fix(teams): authorize /teams from the session, not the global users.role_id (#830) Every /teams handler re-checked the global users.role_id and looked the caller up by the token id (claims.ID) instead of the user id, so each call answered 404 before the check ran. The RequireRole("admin") route guard already authorizes these routes from the caller's role in the active organization; the in-handler checks are removed. AddTeamMember now also requires the target's membership to be active and answers a malformed id with the same 404 as an unknown one. DeleteTeam removes the members and the team in one transaction. Team and member ids are set explicitly rather than left to a Postgres-only column default. Signed-off-by: alex-dembele --- backend/internal/handler/team_handler.go | 113 ++++++----------------- 1 file changed, 30 insertions(+), 83 deletions(-) diff --git a/backend/internal/handler/team_handler.go b/backend/internal/handler/team_handler.go index 94e52c66..4a855d50 100644 --- a/backend/internal/handler/team_handler.go +++ b/backend/internal/handler/team_handler.go @@ -10,11 +10,19 @@ import ( "github.com/gofiber/fiber/v2" "github.com/google/uuid" + "gorm.io/gorm" + "github.com/opendefender/openrisk/internal/domain" "github.com/opendefender/openrisk/internal/infrastructure/database" "github.com/opendefender/openrisk/internal/middleware" ) +// Authorization for every /teams route is the RequireRole("admin") guard in +// the composition root, which reads the caller's role in the ACTIVE +// organization from the signed session. The handlers used to re-check the +// global users.role_id, and looked the caller up by the token id rather than +// the user id, so every call answered 404 (#830). + type CreateTeamInput struct { Name string `json:"name" validate:"required"` Description string `json:"description"` @@ -57,16 +65,6 @@ func CreateTeam(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.ID).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 teams"}) - } - tenantID := safeGetUUID(c, "tenant_id") if tenantID == uuid.Nil { return c.Status(fiber.StatusUnauthorized).JSON(fiber.Map{"error": "invalid tenant"}) @@ -78,6 +76,7 @@ func CreateTeam(c *fiber.Ctx) error { } team := domain.Team{ + ID: uuid.New(), TenantID: tenantID, Name: input.Name, Description: input.Description, @@ -105,16 +104,6 @@ func GetTeams(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.ID).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 view teams"}) - } - tenantID := safeGetUUID(c, "tenant_id") var teams []domain.Team @@ -147,16 +136,6 @@ func GetTeam(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.ID).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 view teams"}) - } - tenantID := safeGetUUID(c, "tenant_id") teamID := c.Params("id") var team domain.Team @@ -200,16 +179,6 @@ func UpdateTeam(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.ID).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 teams"}) - } - tenantID := safeGetUUID(c, "tenant_id") teamID := c.Params("id") var team domain.Team @@ -255,16 +224,6 @@ func DeleteTeam(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.ID).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 teams"}) - } - tenantID := safeGetUUID(c, "tenant_id") teamID := c.Params("id") var team domain.Team @@ -272,13 +231,13 @@ func DeleteTeam(c *fiber.Ctx) error { return c.Status(fiber.StatusNotFound).JSON(fiber.Map{"error": "Team not found"}) } - // Delete team members first - if err := database.DB.Where("team_id = ?", team.ID).Delete(&domain.TeamMember{}).Error; err != nil { - return c.Status(fiber.StatusInternalServerError).JSON(fiber.Map{"error": "Failed to delete team members"}) - } - - // Delete team - if err := database.DB.Delete(&team).Error; err != nil { + // Members and team go together or not at all (RULE #7). + if err := database.DB.Transaction(func(tx *gorm.DB) error { + if err := tx.Where("team_id = ?", team.ID).Delete(&domain.TeamMember{}).Error; err != nil { + return err + } + return tx.Delete(&team).Error + }); err != nil { return c.Status(fiber.StatusInternalServerError).JSON(fiber.Map{"error": "Failed to delete team"}) } @@ -292,20 +251,16 @@ func AddTeamMember(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.ID).Error; err != nil { - return c.Status(fiber.StatusNotFound).JSON(fiber.Map{"error": "User not found"}) + tenantID := safeGetUUID(c, "tenant_id") + teamID, errTeam := uuid.Parse(c.Params("id")) + userID, errUser := uuid.Parse(c.Params("userId")) + if errTeam != nil { + return c.Status(fiber.StatusNotFound).JSON(fiber.Map{"error": "Team not found"}) } - - if currentUser.Role.Name != "admin" { - return c.Status(fiber.StatusForbidden).JSON(fiber.Map{"error": "Only admins can add team members"}) + if errUser != nil { + return c.Status(fiber.StatusNotFound).JSON(fiber.Map{"error": "User not found"}) } - tenantID := safeGetUUID(c, "tenant_id") - teamID := c.Params("id") - userID := c.Params("userId") - // Verify team exists in this tenant var team domain.Team if err := database.DB.First(&team, "id = ? AND tenant_id = ?", teamID, tenantID).Error; err != nil { @@ -318,11 +273,12 @@ func AddTeamMember(c *fiber.Ctx) error { return c.Status(fiber.StatusNotFound).JSON(fiber.Map{"error": "User not found"}) } - // The target user must belong to this tenant — never let an admin pull a - // member from another organization into their team. + // The target user must be an ACTIVE member of this tenant — never let an + // admin pull a member from another organization, or one whose access was + // withdrawn, into their team. Both answer exactly like an unknown id. var orgMemberCount int64 if err := database.DB.Model(&domain.OrganizationMember{}). - Where("organization_id = ? AND user_id = ?", tenantID, userID). + Where("organization_id = ? AND user_id = ? AND is_active = ?", tenantID, userID, true). Count(&orgMemberCount).Error; err != nil { return c.Status(fiber.StatusInternalServerError).JSON(fiber.Map{"error": "Failed to verify membership"}) } @@ -338,8 +294,9 @@ func AddTeamMember(c *fiber.Ctx) error { // Add member member := domain.TeamMember{ - TeamID: uuid.MustParse(teamID), - UserID: uuid.MustParse(userID), + ID: uuid.New(), + TeamID: teamID, + UserID: userID, Role: "member", JoinedAt: time.Now(), } @@ -358,16 +315,6 @@ func RemoveTeamMember(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.ID).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 remove team members"}) - } - tenantID := safeGetUUID(c, "tenant_id") teamID := c.Params("id") userID := c.Params("userId") From 4cbf2a4d1ab70a685f8101a7b7ce5ce853795954 Mon Sep 17 00:00:00 2001 From: alex-dembele Date: Tue, 29 Sep 2026 18:49:17 +0100 Subject: [PATCH 2/3] test(teams): cover /teams success, cross-tenant 404 and non-admin 403 (#830) Signed-off-by: alex-dembele --- backend/internal/handler/team_handler_test.go | 222 ++++++++++++++++++ .../internal/security/isolation/registry.go | 2 +- 2 files changed, 223 insertions(+), 1 deletion(-) create mode 100644 backend/internal/handler/team_handler_test.go diff --git a/backend/internal/handler/team_handler_test.go b/backend/internal/handler/team_handler_test.go new file mode 100644 index 00000000..e3fd6e21 --- /dev/null +++ b/backend/internal/handler/team_handler_test.go @@ -0,0 +1,222 @@ +// 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 + +import ( + "encoding/json" + "io" + "net/http" + "net/http/httptest" + "strings" + "testing" + "time" + + "github.com/gofiber/fiber/v2" + "github.com/google/uuid" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "gorm.io/gorm" + + "github.com/opendefender/openrisk/internal/domain" + "github.com/opendefender/openrisk/internal/infrastructure/database" + "github.com/opendefender/openrisk/internal/middleware" + "github.com/opendefender/openrisk/internal/testsupport/sqliteschema" + authpkg "github.com/opendefender/openrisk/pkg/auth" +) + +// #830 — /teams through the real guard and the real handlers. The caller is +// stamped exactly as the auth middleware stamps it; users.role_id is set to a +// role named "admin" for everybody, to prove it decides nothing. + +type teamRig struct { + app *fiber.App + db *gorm.DB + tenantA, tenantB uuid.UUID + callers map[string]*authpkg.Claims +} + +func newTeamRig(t *testing.T) *teamRig { + t.Helper() + db := setupTeamDB(t) + for _, ddl := range []string{ + `CREATE TABLE users (id TEXT PRIMARY KEY)`, + `CREATE TABLE organization_members (id TEXT PRIMARY KEY)`, + } { + require.NoError(t, db.Exec(ddl).Error) + } + require.NoError(t, sqliteschema.Reconcile(db, "users", &domain.User{})) + require.NoError(t, sqliteschema.Reconcile(db, "organization_members", &domain.OrganizationMember{})) + + orig := database.DB + database.DB = db + t.Cleanup(func() { database.DB = orig }) + + r := &teamRig{db: db, tenantA: uuid.New(), tenantB: uuid.New(), callers: map[string]*authpkg.Claims{}} + + app := fiber.New() + stamp := func(c *fiber.Ctx) error { + claims, ok := r.callers[c.Get("X-Test-Actor")] + if !ok { + return c.SendStatus(http.StatusUnauthorized) + } + c.Locals("user", claims) + c.Locals("org_roles", claims.OrgRoles) + c.Locals("permissions", claims.Permissions) + c.Locals("tenant_id", claims.TenantID) + return c.Next() + } + adminRole := middleware.RequireRole("admin") + app.Post("/teams", stamp, adminRole, CreateTeam) + app.Get("/teams", stamp, adminRole, GetTeams) + app.Get("/teams/:id", stamp, adminRole, GetTeam) + app.Patch("/teams/:id", stamp, adminRole, UpdateTeam) + app.Delete("/teams/:id", stamp, adminRole, DeleteTeam) + app.Post("/teams/:id/members/:userId", stamp, adminRole, AddTeamMember) + app.Delete("/teams/:id/members/:userId", stamp, adminRole, RemoveTeamMember) + r.app = app + return r +} + +// person creates a user with users.role_id pointing at a legacy "admin" role +// and a membership in org with the given membership role and activity. +func (r *teamRig) person(t *testing.T, name string, org uuid.UUID, role domain.MemberRole, active bool) uuid.UUID { + t.Helper() + u := &domain.User{ID: uuid.New(), Email: name + "@x.io", Username: name, IsActive: true, RoleID: uuid.New()} + require.NoError(t, r.db.Create(u).Error) + m := &domain.OrganizationMember{ + ID: uuid.New(), OrganizationID: org, UserID: u.ID, Role: role, + Status: domain.MembershipActive, IsActive: true, JoinedAt: time.Now(), + } + require.NoError(t, r.db.Create(m).Error) + if !active { + require.NoError(t, r.db.Model(m).Updates(map[string]any{"is_active": false, "status": string(domain.MembershipDeactivated)}).Error) + } + r.callers[name] = &authpkg.Claims{ + Sub: u.ID, TenantID: org, OrgRoles: map[uuid.UUID]string{org: string(role)}, + } + return u.ID +} + +func (r *teamRig) call(t *testing.T, actor, method, path string, body any) (int, string) { + t.Helper() + var rdr io.Reader + if body != nil { + raw, err := json.Marshal(body) + require.NoError(t, err) + rdr = strings.NewReader(string(raw)) + } + req := httptest.NewRequest(method, path, rdr) + req.Header.Set("Content-Type", "application/json") + req.Header.Set("X-Test-Actor", actor) + res, err := r.app.Test(req, 5000) + require.NoError(t, err) + defer func() { _ = res.Body.Close() }() + raw, _ := io.ReadAll(res.Body) + return res.StatusCode, string(raw) +} + +func (r *teamRig) createTeam(t *testing.T, actor, name string) string { + t.Helper() + status, body := r.call(t, actor, http.MethodPost, "/teams", map[string]any{"name": name}) + require.Equal(t, http.StatusCreated, status, body) + var dto TeamResponseDTO + require.NoError(t, json.Unmarshal([]byte(body), &dto)) + return dto.ID +} + +func TestTeams_Success(t *testing.T) { + r := newTeamRig(t) + r.person(t, "adminA", r.tenantA, domain.RoleAdmin, true) + colleague := r.person(t, "colleague", r.tenantA, domain.RoleUser, true) + + id := r.createTeam(t, "adminA", "Blue team") + + status, body := r.call(t, "adminA", http.MethodPost, "/teams/"+id+"/members/"+colleague.String(), nil) + require.Equal(t, http.StatusOK, status, body) + + status, body = r.call(t, "adminA", http.MethodGet, "/teams/"+id, nil) + require.Equal(t, http.StatusOK, status, body) + var detail TeamDetailDTO + require.NoError(t, json.Unmarshal([]byte(body), &detail)) + assert.Equal(t, 1, detail.MemberCount) + + status, _ = r.call(t, "adminA", http.MethodPatch, "/teams/"+id, map[string]any{"name": "Red team"}) + assert.Equal(t, http.StatusOK, status) + status, _ = r.call(t, "adminA", http.MethodDelete, "/teams/"+id+"/members/"+colleague.String(), nil) + assert.Equal(t, http.StatusNoContent, status) + + // Deleting the team removes its memberships with it. + _, _ = r.call(t, "adminA", http.MethodPost, "/teams/"+id+"/members/"+colleague.String(), nil) + status, _ = r.call(t, "adminA", http.MethodDelete, "/teams/"+id, nil) + assert.Equal(t, http.StatusNoContent, status) + var left int64 + r.db.Model(&domain.TeamMember{}).Where("team_id = ?", id).Count(&left) + assert.Zero(t, left) +} + +func TestTeams_NotFound(t *testing.T) { + r := newTeamRig(t) + r.person(t, "adminA", r.tenantA, domain.RoleAdmin, true) + r.person(t, "adminB", r.tenantB, domain.RoleAdmin, true) + foreign := r.person(t, "foreign", r.tenantB, domain.RoleUser, true) + withdrawn := r.person(t, "withdrawn", r.tenantA, domain.RoleUser, false) + + teamA := r.createTeam(t, "adminA", "A team") + teamB := r.createTeam(t, "adminB", "B team") + + // A foreign user, a withdrawn member, an unknown id and a malformed id all + // get the same answer: nothing tells A's admin which of them exist. + _, want := r.call(t, "adminA", http.MethodPost, "/teams/"+teamA+"/members/"+uuid.NewString(), nil) + for name, target := range map[string]string{ + "foreign user": foreign.String(), + "withdrawn member": withdrawn.String(), + "malformed id": "not-a-uuid", + } { + status, body := r.call(t, "adminA", http.MethodPost, "/teams/"+teamA+"/members/"+target, nil) + assert.Equal(t, http.StatusNotFound, status, name) + assert.Equal(t, want, body, "%s must answer exactly like an unknown id", name) + } + + // B's team is invisible and untouchable from A. + for _, tc := range []struct{ method, path string }{ + {http.MethodGet, "/teams/" + teamB}, + {http.MethodPatch, "/teams/" + teamB}, + {http.MethodDelete, "/teams/" + teamB}, + {http.MethodPost, "/teams/" + teamB + "/members/" + foreign.String()}, + {http.MethodDelete, "/teams/" + teamB + "/members/" + foreign.String()}, + } { + status, _ := r.call(t, "adminA", tc.method, tc.path, map[string]any{"name": "pwned"}) + assert.Equal(t, http.StatusNotFound, status, "%s %s", tc.method, tc.path) + } + var stored domain.Team + require.NoError(t, r.db.First(&stored, "id = ?", teamB).Error) + assert.Equal(t, "B team", stored.Name) + + var members int64 + r.db.Model(&domain.TeamMember{}).Where("team_id = ?", teamA).Count(&members) + assert.Zero(t, members, "no refused add may leave a row behind") +} + +func TestTeams_Unauthorized(t *testing.T) { + r := newTeamRig(t) + r.person(t, "adminA", r.tenantA, domain.RoleAdmin, true) + // users.role_id is set on this person too; only the membership role counts. + r.person(t, "member", r.tenantA, domain.RoleUser, true) + teamA := r.createTeam(t, "adminA", "A team") + + for _, tc := range []struct{ method, path string }{ + {http.MethodPost, "/teams"}, + {http.MethodGet, "/teams"}, + {http.MethodGet, "/teams/" + teamA}, + {http.MethodPatch, "/teams/" + teamA}, + {http.MethodDelete, "/teams/" + teamA}, + {http.MethodPost, "/teams/" + teamA + "/members/" + uuid.NewString()}, + {http.MethodDelete, "/teams/" + teamA + "/members/" + uuid.NewString()}, + } { + status, _ := r.call(t, "member", tc.method, tc.path, map[string]any{"name": "x"}) + assert.Equal(t, http.StatusForbidden, status, "%s %s as a plain member", tc.method, tc.path) + } +} diff --git a/backend/internal/security/isolation/registry.go b/backend/internal/security/isolation/registry.go index 82edc370..d9750510 100644 --- a/backend/internal/security/isolation/registry.go +++ b/backend/internal/security/isolation/registry.go @@ -130,7 +130,7 @@ var decisions = []Decision{ {"/api/v1/teams/{id}", Covered, "handler/team_isolation_test TestTeam_TenantIsolationPredicate"}, {"/api/v1/teams/*", Covered, - "handler/team_isolation_test covers member add/remove paths"}, + "handler/team_handler_test TestTeams_NotFound: a foreign, withdrawn, unknown or malformed member id answers the same 404, and B's team is unreachable from A (#830)"}, {"/api/v1/users/{id}", Covered, "handler/user_isolation_test TestUser_TenantScoping"}, {"/api/v1/users/*", Covered, From 57b3aa14a5990e40db149d08f5cb1829adeb71e1 Mon Sep 17 00:00:00 2001 From: alex-dembele Date: Tue, 29 Sep 2026 18:59:49 +0100 Subject: [PATCH 3/3] test(security): cite the /teams HTTP tests on the non-adjacent registry entry (#830) Keeps this branch from conflicting with #807's edit of the /users entries directly below. Signed-off-by: alex-dembele --- backend/internal/security/isolation/registry.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/backend/internal/security/isolation/registry.go b/backend/internal/security/isolation/registry.go index d9750510..a5a3d403 100644 --- a/backend/internal/security/isolation/registry.go +++ b/backend/internal/security/isolation/registry.go @@ -128,9 +128,9 @@ var decisions = []Decision{ {"/api/v1/custom-fields/*", Covered, "service/custom_field_service_test TestCustomField_TenantIsolation"}, {"/api/v1/teams/{id}", Covered, - "handler/team_isolation_test TestTeam_TenantIsolationPredicate"}, + "handler/team_isolation_test TestTeam_TenantIsolationPredicate; handler/team_handler_test TestTeams_NotFound drives every /teams route and member path cross-tenant (#830)"}, {"/api/v1/teams/*", Covered, - "handler/team_handler_test TestTeams_NotFound: a foreign, withdrawn, unknown or malformed member id answers the same 404, and B's team is unreachable from A (#830)"}, + "handler/team_isolation_test covers member add/remove paths"}, {"/api/v1/users/{id}", Covered, "handler/user_isolation_test TestUser_TenantScoping"}, {"/api/v1/users/*", Covered,