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

refactor(t-paliad-076): consolidate visibility predicate — 6 dashboard/agenda sites use helper

Browse Source 
F-2 from t-paliad-074 audit. The inlined visibility predicate had drifted
back into 6 hot-path SQL sites despite the central helper extracted in
t-paliad-058. Consolidating now so future visibility changes (e.g.
Chinese-wall in design v2 §8) only need one edit.

**Sites converted (6):**
- dashboard_service.go:158, 214, 244, 274
- agenda_service.go:138, 204

All six replace `$N = 'global_admin' OR EXISTS (path-walk)` with the
existing `visibilityPredicatePositional("p", 1)` helper. The helper
resolves global_admin via EXISTS on paliad.users — the role string no
longer flows through positional args, removing one foot-gun (typo'd
literal mismatched against bound role) entirely. Equivalence verified
on the live youpc DB:

    tester@hlc.de (global_admin, 1 team membership):
      old predicate count = 11   new predicate count = 11
    standard user (no team):
      old predicate count =  0   new predicate count =  0

**No new helper variant added.** The audit suggested
`visibilityPredicateLateral`, but the existing positional helper drops
into the dashboard/agenda WHERE clauses unchanged — adding a redundant
variant would be technical debt. dashboard/agenda do not use LATERAL
JOIN; they use plain WHERE EXISTS in (sub-)SELECT context, which is
already what visibilityPredicatePositional emits.

**Other 4 sites flagged by audit — left intentionally:**

- reminder_service.go:312, 325 are role-restricted (`pt.role = 'lead'`)
  membership checks, NOT visibility predicates. Adding a global_admin
  shortcut to the lead branch would over-include rows: every global
  admin would receive every project's lead-targeted reminder, even with
  the `own.escalation_contact_id` override that exists precisely to
  avoid that. global_admin already has its own dedicated branch in the
  query (`$3 = TRUE AND own.escalation_contact_id IS NULL` at line 328).

- deadline_service.go:422 (`assertCanAdminProject`) is role-restricted
  (`pt.role IN ('admin', 'lead')`) and already short-circuits global_admin
  at the Go level before the SQL runs (line 413). Both halves correct;
  no change needed.

- team_service.go:162 (`IsEffectiveMember`) was dead code with no callers
  in the entire repo. "Is this user a structural team member?" and
  "can this user see this project?" are different questions; adding a
  global_admin shortcut would have conflated them. Deleted instead.

**Test:** new TestVisibilityPredicate_DashboardAgendaForGlobalAdmin in
visibility_test.go seeds a project + deadline + appointment + activity
event with project_teams empty, then asserts a global_admin sees all
four on /dashboard and /agenda while a standard user sees none. Skips
when TEST_DATABASE_URL is unset (matching the existing live-DB tests).

**Pre-existing finding (separate concern):** the live-DB test gate is
currently blocked locally by a stale `public.paliad_schema_migrations`
(version=2, dirty=t) left over from before the schema-pinned tracker
landed. Authoritative `paliad.paliad_schema_migrations` is at version
27, dirty=f. Out of scope for this task; should be filed as cleanup.
This commit is contained in:
m
2026-04-30 03:48:49 +02:00
parent 17aa840977
commit 31db66e3b7
4 changed files with 244 additions and 73 deletions
Download Patch File Download Diff File Expand all files Collapse all files

35
internal/services/agenda_service.go
View File

@@ -70,6 +70,9 @@ func (s *AgendaService) List(ctx context.Context, userID uuid.UUID, f AgendaFilt
return []AgendaItem{}, nil
}
// Existence gate — visibility predicate inside loadDeadlines/Appointments
// reads global_role directly from paliad.users via the SQL helper, so we
// no longer need the row itself, just confirmation that the caller exists.
user, err := s.users.GetByID(ctx, userID)
if err != nil {
return nil, err
@@ -81,14 +84,14 @@ func (s *AgendaService) List(ctx context.Context, userID uuid.UUID, f AgendaFilt
items := make([]AgendaItem, 0, 64)
if f.IncludeDeadlines {
rows, err := s.loadDeadlines(ctx, userID, user.GlobalRole, f.From, f.To)
rows, err := s.loadDeadlines(ctx, userID, f.From, f.To)
if err != nil {
return nil, err
}
items = append(items, rows...)
}
if f.IncludeAppointments {
rows, err := s.loadAppointments(ctx, userID, user.GlobalRole, f.From, f.To)
rows, err := s.loadAppointments(ctx, userID, f.From, f.To)
if err != nil {
return nil, err
}
@@ -113,7 +116,7 @@ func (s *AgendaService) List(ctx context.Context, userID uuid.UUID, f AgendaFilt
// loadDeadlines pulls pending deadlines whose due_date falls in [from, to).
// Completed deadlines are hidden — agenda is forward-looking.
func (s *AgendaService) loadDeadlines(ctx context.Context, userID uuid.UUID, role string, from, to time.Time) ([]AgendaItem, error) {
func (s *AgendaService) loadDeadlines(ctx context.Context, userID uuid.UUID, from, to time.Time) ([]AgendaItem, error) {
// due_date is a DATE; compare against the date portion of the window.
fromDate := from.Format("2006-01-02")
toDate := to.Format("2006-01-02")
@@ -130,13 +133,9 @@ SELECT f.id,
FROM paliad.deadlines f
JOIN paliad.projects p ON p.id = f.project_id
WHERE f.status = 'pending'
AND f.due_date >= $3::date
AND f.due_date < $4::date
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[])
))
AND f.due_date >= $2::date
AND f.due_date < $3::date
AND ` + visibilityPredicatePositional("p", 1) + `
ORDER BY f.due_date ASC, f.created_at ASC`
type row struct {
@@ -150,7 +149,7 @@ SELECT f.id,
ProjectReference *string `db:"project_reference"`
}
var rows []row
if err := s.db.SelectContext(ctx, &rows, query, userID, role, fromDate, toDate); err != nil {
if err := s.db.SelectContext(ctx, &rows, query, userID, fromDate, toDate); err != nil {
return nil, fmt.Errorf("agenda deadlines: %w", err)
}
@@ -180,7 +179,7 @@ SELECT f.id,
// loadAppointments pulls appointments whose start_at falls in [from, to).
// Includes personal appointments (project_id IS NULL, creator-only) and
// project-attached appointments subject to the team predicate.
func (s *AgendaService) loadAppointments(ctx context.Context, userID uuid.UUID, role string, from, to time.Time) ([]AgendaItem, error) {
func (s *AgendaService) loadAppointments(ctx context.Context, userID uuid.UUID, from, to time.Time) ([]AgendaItem, error) {
query := `
SELECT t.id,
t.title,
@@ -194,15 +193,11 @@ SELECT t.id,
p.reference AS project_reference
FROM paliad.appointments t
LEFT JOIN paliad.projects p ON p.id = t.project_id
WHERE t.start_at >= $3
AND t.start_at < $4
WHERE t.start_at >= $2
AND t.start_at < $3
AND (
(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[])
)))
OR (t.project_id IS NOT NULL AND ` + visibilityPredicatePositional("p", 1) + `)
)
ORDER BY t.start_at ASC, t.created_at ASC`
@@ -219,7 +214,7 @@ SELECT t.id,
ProjectReference *string `db:"project_reference"`
}
var rows []row
if err := s.db.SelectContext(ctx, &rows, query, userID, role, from, to); err != nil {
if err := s.db.SelectContext(ctx, &rows, query, userID, from, to); err != nil {
return nil, fmt.Errorf("agenda appointments: %w", err)
}

56
internal/services/dashboard_service.go
View File

@@ -146,26 +146,22 @@ func (s *DashboardService) Get(ctx context.Context, userID uuid.UUID) (*Dashboar
// loadSummary fills DeadlineSummary + MatterSummary.
//
// Visibility predicate: admin OR any ancestor-or-direct team membership.
// Applied once via a CTE; downstream queries reuse the same pattern.
// Visibility predicate: see internal/services/visibility.go — global_admin
// shortcut OR any ancestor-or-direct team membership. Applied once via a CTE;
// downstream queries reuse the same helper.
func (s *DashboardService) loadSummary(ctx context.Context, data *DashboardData, user *models.User, today, endOfWeek string, sevenDaysAgo time.Time) error {
query := `
WITH visible_projekte AS (
SELECT p.id, p.status
FROM paliad.projects p
WHERE $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[])
)
WHERE ` + visibilityPredicatePositional("p", 1) + `
),
deadline_stats AS (
SELECT
COUNT(*) FILTER (WHERE f.due_date < $3::date AND f.status = 'pending') AS overdue,
COUNT(*) FILTER (WHERE f.due_date >= $3::date AND f.due_date <= $4::date AND f.status = 'pending') AS this_week,
COUNT(*) FILTER (WHERE f.due_date > $4::date AND f.status = 'pending') AS upcoming,
COUNT(*) FILTER (WHERE f.status = 'completed' AND f.completed_at >= $5) AS completed_this_week
COUNT(*) FILTER (WHERE f.due_date < $2::date AND f.status = 'pending') AS overdue,
COUNT(*) FILTER (WHERE f.due_date >= $2::date AND f.due_date <= $3::date AND f.status = 'pending') AS this_week,
COUNT(*) FILTER (WHERE f.due_date > $3::date AND f.status = 'pending') AS upcoming,
COUNT(*) FILTER (WHERE f.status = 'completed' AND f.completed_at >= $4) AS completed_this_week
FROM paliad.deadlines f
JOIN visible_projekte v ON v.id = f.project_id
),
@@ -185,7 +181,7 @@ SELECT ds.overdue, ds.this_week, ds.upcoming, ds.completed_this_week,
MatterSummary
}
err := s.db.GetContext(ctx, &row, query,
user.ID, user.GlobalRole, today, endOfWeek, sevenDaysAgo)
user.ID, today, endOfWeek, sevenDaysAgo)
if errors.Is(err, sql.ErrNoRows) {
return nil
}
@@ -208,17 +204,13 @@ SELECT f.id,
FROM paliad.deadlines f
JOIN paliad.projects p ON p.id = f.project_id
WHERE f.status = 'pending'
AND f.due_date >= $3::date
AND f.due_date <= $4::date
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[])
))
AND f.due_date >= $2::date
AND f.due_date <= $3::date
AND ` + visibilityPredicatePositional("p", 1) + `
ORDER BY f.due_date ASC
LIMIT 10`
if err := s.db.SelectContext(ctx, &data.UpcomingDeadlines, query,
user.ID, user.GlobalRole, today, endOfWeek); err != nil {
user.ID, today, endOfWeek); err != nil {
return fmt.Errorf("dashboard upcoming deadlines: %w", err)
}
return nil
@@ -236,20 +228,16 @@ SELECT t.id,
COALESCE(p.reference, NULL) AS project_reference
FROM paliad.appointments t
LEFT JOIN paliad.projects p ON p.id = t.project_id
WHERE t.start_at >= $3
AND t.start_at < ($3 + interval '7 days')
WHERE t.start_at >= $2
AND t.start_at < ($2 + interval '7 days')
AND (
(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[])
)))
OR (t.project_id IS NOT NULL AND ` + visibilityPredicatePositional("p", 1) + `)
)
ORDER BY t.start_at ASC
LIMIT 10`
if err := s.db.SelectContext(ctx, &data.UpcomingAppointments, query,
user.ID, user.GlobalRole, now); err != nil {
user.ID, now); err != nil {
return fmt.Errorf("dashboard upcoming appointments: %w", err)
}
return nil
@@ -269,16 +257,10 @@ SELECT COALESCE(e.event_date, e.created_at) AS timestamp,
FROM paliad.project_events e
JOIN paliad.projects p ON p.id = e.project_id
LEFT JOIN paliad.users u ON u.id = e.created_by
WHERE $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[])
)
WHERE ` + visibilityPredicatePositional("p", 1) + `
ORDER BY COALESCE(e.event_date, e.created_at) DESC
LIMIT 10`
if err := s.db.SelectContext(ctx, &data.RecentActivity, query,
user.ID, user.GlobalRole); err != nil {
if err := s.db.SelectContext(ctx, &data.RecentActivity, query, user.ID); err != nil {
return fmt.Errorf("dashboard recent activity: %w", err)
}
return nil

16
internal/services/team_service.go
View File

@@ -152,22 +152,6 @@ func (s *TeamService) ListEffectiveMembers(ctx context.Context, callerID, projek
return rows, nil
}
// IsEffectiveMember reports whether userID is a direct or inherited member of projektID.
func (s *TeamService) IsEffectiveMember(ctx context.Context, projektID, userID uuid.UUID) (bool, error) {
var ok bool
err := s.db.GetContext(ctx, &ok,
`SELECT EXISTS (
SELECT 1 FROM paliad.projects p
JOIN paliad.project_teams pt
ON pt.project_id = ANY(string_to_array(p.path, '.')::uuid[])
WHERE p.id = $1 AND pt.user_id = $2
)`, projektID, userID)
if err != nil {
return false, fmt.Errorf("check effective membership: %w", err)
}
return ok, nil
}
// ---------------------------------------------------------------------------
func isValidRole(r string) bool {

210
internal/services/visibility_test.go
View File

@@ -5,6 +5,7 @@ import (
"errors"
"os"
"testing"
"time"
"github.com/google/uuid"
"github.com/jmoiron/sqlx"
@@ -113,3 +114,212 @@ func treeContains(nodes []*ProjectTreeNode, id uuid.UUID) bool {
}
return false
}
// TestVisibilityPredicate_DashboardAgendaForGlobalAdmin is the regression for
// t-paliad-076. After consolidating the inlined predicates in dashboard and
// agenda services, a global_admin without any project_teams row must still
// see Project-scoped rows on:
// - DashboardService.Get (summary counts, upcoming deadlines, upcoming
// appointments, recent activity)
// - AgendaService.List (deadlines + appointments)
//
// The standard user with no membership must see none of those rows.
//
// Skipped when TEST_DATABASE_URL is unset, mirroring the other live-DB tests.
func TestVisibilityPredicate_DashboardAgendaForGlobalAdmin(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()
deadlineID := uuid.New()
appointmentID := uuid.New()
eventID := uuid.New()
cleanup := func() {
pool.ExecContext(ctx, `DELETE FROM paliad.project_events WHERE project_id = $1`, projectID)
pool.ExecContext(ctx, `DELETE FROM paliad.appointments WHERE id = $1`, appointmentID)
pool.ExecContext(ctx, `DELETE FROM paliad.deadlines WHERE id = $1`, deadlineID)
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-da-admin@hlc.com", standardID, "vis-da-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 DA Admin', 'munich', 'global_admin', 'de'),
($3, $4, 'Vis DA Standard', 'munich', 'standard', 'de')`,
adminID, "vis-da-admin@hlc.com", standardID, "vis-da-standard@hlc.com"); err != nil {
t.Fatalf("seed paliad.users: %v", err)
}
// Project owned by adminID. project_teams intentionally left empty so the
// only path to visibility is the global_admin branch of the predicate.
if _, err := pool.ExecContext(ctx,
`INSERT INTO paliad.projects (id, type, path, title, reference, status, created_by)
VALUES ($1, 'project', $1::text, 'Visibility DA Project', '2026/9996', 'active', $2)`,
projectID, adminID); err != nil {
t.Fatalf("seed paliad.projects: %v", err)
}
// Seed one pending deadline within the dashboard's 7-day window so it
// shows up in both summary counts AND the upcoming list.
dueDate := time.Now().UTC().AddDate(0, 0, 2).Format("2006-01-02")
if _, err := pool.ExecContext(ctx,
`INSERT INTO paliad.deadlines (id, project_id, title, due_date, source, status, created_by)
VALUES ($1, $2, 'Vis DA Deadline', $3::date, 'manual', 'pending', $4)`,
deadlineID, projectID, dueDate, adminID); err != nil {
t.Fatalf("seed paliad.deadlines: %v", err)
}
// Seed one appointment within the dashboard's 7-day window.
startAt := time.Now().UTC().Add(48 * time.Hour)
if _, err := pool.ExecContext(ctx,
`INSERT INTO paliad.appointments
(id, project_id, title, start_at, appointment_type, created_by)
VALUES ($1, $2, 'Vis DA Appointment', $3, 'meeting', $4)`,
appointmentID, projectID, startAt, adminID); err != nil {
t.Fatalf("seed paliad.appointments: %v", err)
}
// Seed one project event so recent activity has a row to surface.
if _, err := pool.ExecContext(ctx,
`INSERT INTO paliad.project_events
(id, project_id, event_type, title, description, event_date,
created_by, metadata, created_at, updated_at)
VALUES ($1, $2, 'project_created', 'Created', NULL, $3, $4,
'{}'::jsonb, $3, $3)`,
eventID, projectID, time.Now().UTC(), adminID); err != nil {
t.Fatalf("seed paliad.project_events: %v", err)
}
users := NewUserService(pool)
dashboard := NewDashboardService(pool, users)
agenda := NewAgendaService(pool, users)
t.Run("global_admin sees dashboard rows without team membership", func(t *testing.T) {
data, err := dashboard.Get(ctx, adminID)
if err != nil {
t.Fatalf("dashboard.Get: %v", err)
}
if data.MatterSummary.Total < 1 {
t.Errorf("MatterSummary.Total = %d, want >= 1 (global_admin must see the seeded project)", data.MatterSummary.Total)
}
if data.DeadlineSummary.ThisWeek+data.DeadlineSummary.Upcoming+data.DeadlineSummary.Overdue < 1 {
t.Errorf("DeadlineSummary has 0 pending; want >= 1 (global_admin must see the seeded deadline)")
}
if !containsDeadline(data.UpcomingDeadlines, deadlineID) {
t.Errorf("UpcomingDeadlines missing %s for global_admin", deadlineID)
}
if !containsAppointment(data.UpcomingAppointments, appointmentID) {
t.Errorf("UpcomingAppointments missing %s for global_admin", appointmentID)
}
if !containsActivityProject(data.RecentActivity, projectID) {
t.Errorf("RecentActivity missing project %s for global_admin", projectID)
}
})
t.Run("standard user sees nothing without team membership", func(t *testing.T) {
data, err := dashboard.Get(ctx, standardID)
if err != nil {
t.Fatalf("dashboard.Get standard: %v", err)
}
if containsDeadline(data.UpcomingDeadlines, deadlineID) {
t.Errorf("UpcomingDeadlines leaked %s to standard user", deadlineID)
}
if containsAppointment(data.UpcomingAppointments, appointmentID) {
t.Errorf("UpcomingAppointments leaked %s to standard user", appointmentID)
}
if containsActivityProject(data.RecentActivity, projectID) {
t.Errorf("RecentActivity leaked project %s to standard user", projectID)
}
})
t.Run("global_admin sees agenda rows without team membership", func(t *testing.T) {
from := time.Now().UTC().AddDate(0, 0, -1)
to := time.Now().UTC().AddDate(0, 0, 14)
items, err := agenda.List(ctx, adminID, AgendaFilter{
From: from, To: to, IncludeDeadlines: true, IncludeAppointments: true,
})
if err != nil {
t.Fatalf("agenda.List: %v", err)
}
if !containsAgendaID(items, deadlineID) {
t.Errorf("agenda missing deadline %s for global_admin", deadlineID)
}
if !containsAgendaID(items, appointmentID) {
t.Errorf("agenda missing appointment %s for global_admin", appointmentID)
}
})
t.Run("standard user sees no agenda rows without team membership", func(t *testing.T) {
from := time.Now().UTC().AddDate(0, 0, -1)
to := time.Now().UTC().AddDate(0, 0, 14)
items, err := agenda.List(ctx, standardID, AgendaFilter{
From: from, To: to, IncludeDeadlines: true, IncludeAppointments: true,
})
if err != nil {
t.Fatalf("agenda.List standard: %v", err)
}
if containsAgendaID(items, deadlineID) {
t.Errorf("agenda leaked deadline %s to standard user", deadlineID)
}
if containsAgendaID(items, appointmentID) {
t.Errorf("agenda leaked appointment %s to standard user", appointmentID)
}
})
}
func containsDeadline(rows []UpcomingDeadline, id uuid.UUID) bool {
for _, r := range rows {
if r.ID == id {
return true
}
}
return false
}
func containsAppointment(rows []UpcomingAppointment, id uuid.UUID) bool {
for _, r := range rows {
if r.ID == id {
return true
}
}
return false
}
func containsActivityProject(rows []ActivityEntry, projectID uuid.UUID) bool {
for _, r := range rows {
if r.ProjectID == projectID {
return true
}
}
return false
}
func containsAgendaID(items []AgendaItem, id uuid.UUID) bool {
for _, it := range items {
if it.ID == id {
return true
}
}
return false
}