m/paliad
1
0
Fork 0
Code Issues 146 Pull Requests 8 Actions 1 Packages Projects Releases Wiki Activity

Merge: visibilityPredicate honors global_admin (t-paliad-058)

Browse Source 
Fix in-Go visibilityPredicate to mirror paliad.can_see_project — the
positional variant was comparing the bound role to 'admin' instead of
'global_admin', so global_admins without project_teams rows got 404
on every endpoint that called the positional helper (GetByID,
ListAncestors, BuildTree, GetTree, deadline counts).

Refactor to resolve global_admin via EXISTS on paliad.users keyed only
by userID — callers no longer pass role, removing the foot-gun for
future endpoints.
This commit is contained in:
m
2026-04-27 16:36:11 +02:00
parent 7e76b0e414 abd99980fc
commit 7644c2e2d8
6 changed files with 183 additions and 74 deletions
Download Patch File Download Diff File Expand all files Collapse all files

10
internal/services/appointment_service.go
View File

@@ -101,7 +101,6 @@ func (s *AppointmentService) ListVisibleForUser(ctx context.Context, userID uuid
conds := []string{visibility}
args := map[string]any{
"user_id": userID,
"role": user.GlobalRole,
}
if filter.ProjectID != nil {
@@ -448,7 +447,6 @@ func (s *AppointmentService) SummaryCounts(ctx context.Context, userID uuid.UUID
"tomorrow": tomorrow,
"endweek": endOfWeek,
"user_id": userID,
"role": user.GlobalRole,
}); err != nil {
return nil, fmt.Errorf("appointment summary: %w", err)
}
@@ -484,12 +482,8 @@ func (s *AppointmentService) AllForUser(ctx context.Context, userID uuid.UUID) (
LEFT JOIN paliad.projects p ON p.id = t.project_id
WHERE
(t.project_id IS NULL AND t.created_by = $1)
OR (t.project_id IS NOT NULL AND ($2 = 'global_admin' OR EXISTS (
SELECT 1 FROM paliad.project_teams pt
WHERE pt.user_id = $1
AND pt.project_id = ANY(string_to_array(p.path, '.')::uuid[])
)))`
if err := s.db.SelectContext(ctx, &rows, query, userID, user.GlobalRole); err != nil {
OR (t.project_id IS NOT NULL AND ` + visibilityPredicatePositional("p", 1) + `)`
if err := s.db.SelectContext(ctx, &rows, query, userID); err != nil {
return nil, fmt.Errorf("all appointments for user: %w", err)
}
return rows, nil

1
internal/services/checklist_instance_service.go
View File

@@ -75,7 +75,6 @@ func (s *ChecklistInstanceService) ListForTemplate(ctx context.Context, userID u
args := map[string]any{
"slug": slug,
"user_id": userID,
"role": user.GlobalRole,
}
return s.listWithProjekt(ctx, query, args)
}

2
internal/services/deadline_service.go
View File

@@ -86,7 +86,6 @@ func (s *DeadlineService) ListVisibleForUser(ctx context.Context, userID uuid.UU
conds := []string{visibilityPredicate("p")}
args := map[string]any{
"user_id": userID,
"role": user.GlobalRole,
}
if filter.ProjectID != nil {
conds = append(conds, `f.project_id = :project_id`)
@@ -488,7 +487,6 @@ func (s *DeadlineService) SummaryCounts(ctx context.Context, userID uuid.UUID, p
conds := []string{visibilityPredicate("p")}
args := map[string]any{
"user_id": userID,
"role": user.GlobalRole,
"today": today,
"tomorrow": tomorrow,
"endweek": endWeek,

74
internal/services/project_service.go
View File

@@ -160,7 +160,6 @@ func (s *ProjectService) List(ctx context.Context, userID uuid.UUID, f ProjectFi
conds := []string{visibilityPredicate("p")}
args := map[string]any{
"user_id": userID,
"role": user.GlobalRole,
}
if f.Type != "" {
@@ -212,8 +211,8 @@ func (s *ProjectService) GetByID(ctx context.Context, userID, id uuid.UUID) (*mo
}
var p models.Project
query := `SELECT ` + projektColumns + ` FROM paliad.projects p
WHERE p.id = $1 AND ` + visibilityPredicatePositional("p", 2, 3)
err = s.db.GetContext(ctx, &p, query, id, userID, user.GlobalRole)
WHERE p.id = $1 AND ` + visibilityPredicatePositional("p", 2)
err = s.db.GetContext(ctx, &p, query, id, userID)
if errors.Is(err, sql.ErrNoRows) {
return nil, ErrNotVisible
}
@@ -264,14 +263,14 @@ func (s *ProjectService) ListAncestors(ctx context.Context, userID, id uuid.UUID
// for safety in case path is stale.
query := `SELECT ` + projektColumns + ` FROM paliad.projects p
WHERE p.id = ANY($1::uuid[]) AND ` +
visibilityPredicatePositional("p", 2, 3)
visibilityPredicatePositional("p", 2)
// lib/pq doesn't serialise []uuid.UUID natively; render as string array.
idStrs := make([]string, len(ids))
for i, u := range ids {
idStrs[i] = u.String()
}
rows := []models.Project{}
if err := s.db.SelectContext(ctx, &rows, query, pq.StringArray(idStrs), userID, user.GlobalRole); err != nil {
if err := s.db.SelectContext(ctx, &rows, query, pq.StringArray(idStrs), userID); err != nil {
return nil, fmt.Errorf("list ancestors: %w", err)
}
// Re-order to match path order (root first).
@@ -308,10 +307,10 @@ func (s *ProjectService) BuildTree(ctx context.Context, userID uuid.UUID) ([]*Pr
}
query := `SELECT ` + projektColumns + ` FROM paliad.projects p
WHERE ` + visibilityPredicatePositional("p", 1, 2) + `
WHERE ` + visibilityPredicatePositional("p", 1) + `
ORDER BY p.path`
rows := []models.Project{}
if err := s.db.SelectContext(ctx, &rows, query, userID, user.GlobalRole); err != nil {
if err := s.db.SelectContext(ctx, &rows, query, userID); err != nil {
return nil, fmt.Errorf("build tree list: %w", err)
}
@@ -326,11 +325,11 @@ func (s *ProjectService) BuildTree(ctx context.Context, userID uuid.UUID) ([]*Pr
if err := s.db.SelectContext(ctx, &counts, `
SELECT f.project_id,
COUNT(*) FILTER (WHERE f.status = 'pending') AS open,
COUNT(*) FILTER (WHERE f.status = 'pending' AND f.due_date < $3::date) AS overdue
COUNT(*) FILTER (WHERE f.status = 'pending' AND f.due_date < $2::date) AS overdue
FROM paliad.deadlines f
JOIN paliad.projects p ON p.id = f.project_id
WHERE `+visibilityPredicatePositional("p", 1, 2)+`
GROUP BY f.project_id`, userID, user.GlobalRole, today); err != nil {
WHERE `+visibilityPredicatePositional("p", 1)+`
GROUP BY f.project_id`, userID, today); err != nil {
return nil, fmt.Errorf("build tree deadline counts: %w", err)
}
countByID := make(map[uuid.UUID]deadlineCount, len(counts))
@@ -389,21 +388,14 @@ func (s *ProjectService) GetTree(ctx context.Context, userID, id uuid.UUID) ([]m
if err != nil {
return nil, err
}
user, err := s.users.GetByID(ctx, userID)
if err != nil {
return nil, err
}
if user == nil {
return []models.Project{}, nil
}
// path LIKE root.path || '.%' OR path = root.path
prefix := root.Path + ".%"
query := `SELECT ` + projektColumns + ` FROM paliad.projects p
WHERE (p.path = $1 OR p.path LIKE $2)
AND ` + visibilityPredicatePositional("p", 3, 4) + `
AND ` + visibilityPredicatePositional("p", 3) + `
ORDER BY p.path`
rows := []models.Project{}
if err := s.db.SelectContext(ctx, &rows, query, root.Path, prefix, userID, user.GlobalRole); err != nil {
if err := s.db.SelectContext(ctx, &rows, query, root.Path, prefix, userID); err != nil {
return nil, fmt.Errorf("get tree: %w", err)
}
return rows, nil
@@ -751,50 +743,6 @@ func (s *ProjectService) ResolveClientNumber(ctx context.Context, userID, id uui
// Helpers
// ============================================================================
// visibilityPredicate returns a SQL snippet that gates rows on
// paliad.can_see_project-equivalent checks at the application layer. Uses
// named bind variables :user_id and :role.
//
// Predicate: admin OR any (direct or ancestor) team membership of user_id.
// Walks the path: project_teams.project_id = ANY(string_to_array(p.path, '.')::uuid[]).
//
// CAST(... AS uuid[]) — not the `::uuid[]` shorthand — because sqlx v1.4.0's
// named-parameter parser strips one colon from `::uuid[]` while compiling
// `:user_id`/`:role` placeholders, producing invalid SQL `:uuid[]` that
// Postgres rejects with `syntax error at or near ":"`.
func visibilityPredicate(alias string) string {
return `(:role = 'global_admin' OR EXISTS (
SELECT 1 FROM paliad.project_teams pt
WHERE pt.user_id = :user_id
AND pt.project_id = ANY(CAST(string_to_array(` + alias + `.path, '.') AS uuid[]))
))`
}
// visibilityPredicatePositional returns the same predicate with positional
// placeholders ($userArg, $roleArg). Use when the surrounding query can't
// use named parameters.
func visibilityPredicatePositional(alias string, userArg, roleArg int) string {
return fmt.Sprintf(`($%d = 'admin' OR EXISTS (
SELECT 1 FROM paliad.project_teams pt
WHERE pt.user_id = $%d
AND pt.project_id = ANY(string_to_array(%s.path, '.')::uuid[])
))`, roleArg, userArg, alias)
}
// visibilityPredicatePlaceholder returns the predicate with ? placeholders
// (for sqlx.In-compatible queries). The caller appends (userID, role) to
// args in that order.
func visibilityPredicatePlaceholder(alias string) string {
return `(? = 'global_admin' OR EXISTS (
SELECT 1 FROM paliad.project_teams pt
WHERE pt.user_id = ?
AND pt.project_id = ANY(string_to_array(` + alias + `.path, '.')::uuid[])
))`
}
// Note: visibilityPredicatePlaceholder expects (role, user_id) in that
// order. Callers must match. We document this inline where used.
// insertProjectEvent appends one audit row in the given tx.
func insertProjectEvent(ctx context.Context, tx *sqlx.Tx, projektID, userID uuid.UUID, eventType, title string, description *string) error {
now := time.Now().UTC()

55
internal/services/visibility.go Normal file
View File

@@ -0,0 +1,55 @@
package services
// Application-layer mirror of paliad.can_see_project (defined in migration
// 023). The SQL function is enforced by RLS, but services run with a
// service-role connection that has no auth.uid() — so each query that
// returns project-scoped rows re-applies the same predicate inline.
//
// Predicate shape (matches can_see_project body):
//
// global_admin in paliad.users OR any (direct or ancestor) team membership
//
// We resolve the global-admin shortcut by EXISTS-querying paliad.users on
// the supplied userID rather than asking callers to pass user.GlobalRole as
// a separate parameter. That eliminates an entire bug class (mismatched
// role string, e.g. checking 'admin' when the column stores 'global_admin')
// and means the only argument the predicate needs is the user UUID.
//
// Callers must JOIN paliad.projects under the given alias so the path-walk
// half of the predicate can read alias.path.
import "fmt"
// visibilityPredicate returns the predicate using sqlx named bind variables.
// Caller binds :user_id to the requesting user's UUID.
//
// Uses CAST(... AS uuid[]) — not the `::uuid[]` shorthand — because sqlx
// v1.4.0's named-parameter parser strips one colon from `::uuid[]` while
// compiling `:user_id` placeholders, producing invalid SQL `:uuid[]` that
// Postgres rejects with `syntax error at or near ":"`.
func visibilityPredicate(alias string) string {
return `(EXISTS (
SELECT 1 FROM paliad.users u
WHERE u.id = :user_id AND u.global_role = 'global_admin'
) OR EXISTS (
SELECT 1 FROM paliad.project_teams pt
WHERE pt.user_id = :user_id
AND pt.project_id = ANY(CAST(string_to_array(` + alias + `.path, '.') AS uuid[]))
))`
}
// visibilityPredicatePositional returns the predicate with one positional
// placeholder ($userArg) for the user UUID. Use when the surrounding query
// can't take named parameters (mixed positional args, lib/pq array binding,
// etc.). The same $userArg is referenced twice — that is a feature of
// Postgres positional binding, not a bug.
func visibilityPredicatePositional(alias string, userArg int) string {
return fmt.Sprintf(`(EXISTS (
SELECT 1 FROM paliad.users u
WHERE u.id = $%d AND u.global_role = 'global_admin'
) OR EXISTS (
SELECT 1 FROM paliad.project_teams pt
WHERE pt.user_id = $%d
AND pt.project_id = ANY(string_to_array(%s.path, '.')::uuid[])
))`, userArg, userArg, alias)
}

115
internal/services/visibility_test.go Normal file
View File

@@ -0,0 +1,115 @@
package services
import (
"context"
"errors"
"os"
"testing"
"github.com/google/uuid"
"github.com/jmoiron/sqlx"
_ "github.com/lib/pq"
"mgit.msbls.de/m/patholo/internal/db"
)
// TestVisibilityPredicate_GlobalAdminWithoutTeam is the regression for
// t-paliad-058. A global_admin without any project_teams row must still be
// able to read every Project — matching paliad.can_see_project's RLS
// behaviour. Pre-fix, visibilityPredicatePositional compared the bound role
// to the literal 'admin', so callers passing the (correct) 'global_admin'
// value silently fell through to the team-membership branch and got 404.
//
// Skipped when TEST_DATABASE_URL is unset, mirroring the other live-DB tests.
func TestVisibilityPredicate_GlobalAdminWithoutTeam(t *testing.T) {
url := os.Getenv("TEST_DATABASE_URL")
if url == "" {
t.Skip("TEST_DATABASE_URL not set — skipping live DB test")
}
if err := db.ApplyMigrations(url); err != nil {
t.Fatalf("apply migrations: %v", err)
}
pool, err := sqlx.Connect("postgres", url)
if err != nil {
t.Fatalf("connect: %v", err)
}
defer pool.Close()
ctx := context.Background()
adminID := uuid.New()
standardID := uuid.New()
projectID := uuid.New()
cleanup := func() {
pool.ExecContext(ctx, `DELETE FROM paliad.project_teams WHERE project_id = $1`, projectID)
pool.ExecContext(ctx, `DELETE FROM paliad.projects WHERE id = $1`, projectID)
pool.ExecContext(ctx, `DELETE FROM paliad.users WHERE id IN ($1, $2)`, adminID, standardID)
pool.ExecContext(ctx, `DELETE FROM auth.users WHERE id IN ($1, $2)`, adminID, standardID)
}
cleanup()
defer cleanup()
if _, err := pool.ExecContext(ctx,
`INSERT INTO auth.users (id, email) VALUES ($1, $2), ($3, $4)`,
adminID, "vis-admin@hlc.com", standardID, "vis-standard@hlc.com"); err != nil {
t.Fatalf("seed auth.users: %v", err)
}
if _, err := pool.ExecContext(ctx,
`INSERT INTO paliad.users (id, email, display_name, office, global_role, lang)
VALUES ($1, $2, 'Vis Admin', 'munich', 'global_admin', 'de'),
($3, $4, 'Vis Standard', 'munich', 'standard', 'de')`,
adminID, "vis-admin@hlc.com", standardID, "vis-standard@hlc.com"); err != nil {
t.Fatalf("seed paliad.users: %v", err)
}
// Project created by adminID. NOTE: ProjectService.Create normally
// auto-adds the creator to project_teams (so they'd see the project via
// team membership too) — for this regression we INSERT the row directly
// and leave project_teams empty. That isolates the global_admin shortcut.
if _, err := pool.ExecContext(ctx,
`INSERT INTO paliad.projects (id, type, path, title, reference, status, created_by)
VALUES ($1, 'project', $1::text, 'Visibility Test Project', '2026/9997', 'active', $2)`,
projectID, adminID); err != nil {
t.Fatalf("seed paliad.projects: %v", err)
}
users := NewUserService(pool)
projects := NewProjectService(pool, users)
// global_admin without team membership: must see the project.
if _, err := projects.GetByID(ctx, adminID, projectID); err != nil {
t.Fatalf("GetByID for global_admin without team: %v (want nil — global_admin sees all)", err)
}
tree, err := projects.BuildTree(ctx, adminID)
if err != nil {
t.Fatalf("BuildTree for global_admin: %v", err)
}
if !treeContains(tree, projectID) {
t.Errorf("BuildTree omitted project %s for global_admin", projectID)
}
// standard user without team membership: must NOT see the project.
if _, err := projects.GetByID(ctx, standardID, projectID); !errors.Is(err, ErrNotVisible) {
t.Fatalf("GetByID for standard without team: err=%v, want ErrNotVisible", err)
}
tree, err = projects.BuildTree(ctx, standardID)
if err != nil {
t.Fatalf("BuildTree for standard: %v", err)
}
if treeContains(tree, projectID) {
t.Errorf("BuildTree leaked project %s to standard user without team", projectID)
}
}
func treeContains(nodes []*ProjectTreeNode, id uuid.UUID) bool {
for _, n := range nodes {
if n.ID == id {
return true
}
if treeContains(n.Children, id) {
return true
}
}
return false
}