QA hardening: security RLS fixes, Flutter 3.47.5 upgrade, UI/validation fixes

Security — enforce write authorization server-side (was UI/RPC-only): - it_service_requests RLS: block cross-office read/edit + self-approve (QA-015) - pass_slips RLS: owner can complete but not self-approve (QA-046) - swap_requests RLS: scope select/update to participants + admin (QA-047) - storage: tighten it_service_attachments + task_attachments write/delete (QA-027) - admin_user_management edge function: allow programmers to manage users (QA-016) Fixes: - workforce generator "uncovered shifts" false alarms (QA-043/044) - network-map VLAN + New-location dialog validation, disabled-until-valid (QA-048) - de-flake time-of-day-dependent dashboard metrics test (QA-045) Toolchain: - upgrade to Flutter 3.47.5 / Dart 3.13.4; font_awesome_flutter 11.0.0, flutter_quill 11.6.0, pdfrx 2.6.5; clear resulting deprecations (QA-002) analyze clean; 139 tests pass; web build succeeds. Report + evidence in docs/qa/. Note: also carries the in-progress Brick model cleanup already present in the working tree. QA-001 (AI keys public in the build) is deferred by owner decision. Co-Authored-By: claude-flow <ruv@ruv.net>
2026-09-26 11:52:16 +08:00
parent e49b52949c
commit 3cb980b629
125 changed files with 3420 additions and 2853 deletions
@@ -91,7 +91,9 @@ serve(async (req) => {
    .eq("id", authData.user.id)
    .maybeSingle();
  const role = (profile?.role ?? "").toString().toLowerCase();
-  if (profileError || role != "admin") {
+  // QA-016: admins and programmers may manage users (the UI already exposes
+  // User Management to both roles).
+  if (profileError || (role != "admin" && role != "programmer")) {
    return jsonResponse({ error: "Forbidden" }, 403);
  }

@@ -0,0 +1,173 @@
+-- QA-015: it_service_requests and its assignments used USING (true), so any
+-- signed-in user could read every office's requests, edit any request, assign
+-- staff, or approve (status -> scheduled) via the REST API. The app only hid
+-- these actions in the UI. Mirror the app's rules server-side:
+--   * admin/programmer/dispatcher/it_staff see and manage all requests
+--   * other users see their own requests and their offices' requests, and may
+--     edit only their own while draft/pending_approval
+--   * assigned staff may update the requests they're assigned to
+--   * only admins approve (enforced by trigger; RLS can't compare OLD/NEW)
+
+-- ----- it_service_requests -----
+DROP POLICY IF EXISTS "Authenticated users can read it_service_requests" ON it_service_requests;
+DROP POLICY IF EXISTS "ISR: select" ON it_service_requests;
+CREATE POLICY "ISR: select" ON it_service_requests
+  FOR SELECT TO authenticated USING (
+    creator_id = auth.uid()
+    OR EXISTS (SELECT 1 FROM public.profiles p
+               WHERE p.id = auth.uid()
+                 AND p.role IN ('admin','programmer','dispatcher','it_staff'))
+    OR office_id IN (SELECT uo.office_id FROM public.user_offices uo
+                     WHERE uo.user_id = auth.uid())
+  );
+
+DROP POLICY IF EXISTS "Authenticated users can update it_service_requests" ON it_service_requests;
+DROP POLICY IF EXISTS "ISR: update" ON it_service_requests;
+CREATE POLICY "ISR: update" ON it_service_requests
+  FOR UPDATE TO authenticated USING (
+    EXISTS (SELECT 1 FROM public.profiles p
+            WHERE p.id = auth.uid()
+              AND p.role IN ('admin','programmer','dispatcher','it_staff'))
+    OR (creator_id = auth.uid() AND status IN ('draft','pending_approval'))
+    OR EXISTS (SELECT 1 FROM public.it_service_request_assignments a
+               WHERE a.request_id = it_service_requests.id
+                 AND a.user_id = auth.uid())
+  );
+-- WITH CHECK defaults to USING, so a creator can't move their own request
+-- out of draft/pending_approval (e.g. self-approve to 'scheduled').
+
+CREATE OR REPLACE FUNCTION public.isr_guard_approval()
+RETURNS trigger
+LANGUAGE plpgsql
+SECURITY DEFINER
+SET search_path = public
+AS $$
+BEGIN
+  -- Only end-user requests are guarded; service-role jobs have no auth.uid().
+  IF auth.uid() IS NOT NULL
+     AND (NEW.approved_by_user_id IS DISTINCT FROM OLD.approved_by_user_id
+          OR (OLD.status = 'pending_approval'
+              AND NEW.status NOT IN ('pending_approval', 'cancelled')))
+     AND NOT EXISTS (SELECT 1 FROM public.profiles
+                     WHERE id = auth.uid() AND role = 'admin') THEN
+    RAISE EXCEPTION 'Only admins can approve IT service requests'
+      USING ERRCODE = '42501';
+  END IF;
+  RETURN NEW;
+END;
+$$;
+
+DROP TRIGGER IF EXISTS isr_guard_approval ON it_service_requests;
+CREATE TRIGGER isr_guard_approval
+  BEFORE UPDATE ON it_service_requests
+  FOR EACH ROW EXECUTE FUNCTION public.isr_guard_approval();
+
+-- ----- it_service_request_assignments -----
+-- Same people who may edit the request may assign/unassign staff.
+DROP POLICY IF EXISTS "Authenticated users can insert it_service_request_assignments" ON it_service_request_assignments;
+DROP POLICY IF EXISTS "ISR assignments: insert" ON it_service_request_assignments;
+CREATE POLICY "ISR assignments: insert" ON it_service_request_assignments
+  FOR INSERT TO authenticated WITH CHECK (
+    EXISTS (SELECT 1 FROM public.profiles p
+            WHERE p.id = auth.uid()
+              AND p.role IN ('admin','programmer','dispatcher','it_staff'))
+    OR EXISTS (SELECT 1 FROM public.it_service_requests r
+               WHERE r.id = it_service_request_assignments.request_id
+                 AND r.creator_id = auth.uid()
+                 AND r.status IN ('draft','pending_approval'))
+  );
+
+DROP POLICY IF EXISTS "Authenticated users can delete it_service_request_assignments" ON it_service_request_assignments;
+DROP POLICY IF EXISTS "ISR assignments: delete" ON it_service_request_assignments;
+CREATE POLICY "ISR assignments: delete" ON it_service_request_assignments
+  FOR DELETE TO authenticated USING (
+    EXISTS (SELECT 1 FROM public.profiles p
+            WHERE p.id = auth.uid()
+              AND p.role IN ('admin','programmer','dispatcher','it_staff'))
+    OR EXISTS (SELECT 1 FROM public.it_service_requests r
+               WHERE r.id = it_service_request_assignments.request_id
+                 AND r.creator_id = auth.uid()
+                 AND r.status IN ('draft','pending_approval'))
+  );
+
+-- ----- insert_it_service_request_with_number -----
+-- Two problems surfaced after the SELECT policy above was tightened:
+--   1. The number generator reads MAX(request_number) under the CALLER's RLS.
+--      A non-privileged creator now sees only their own/office rows, so MAX
+--      came back empty and it regenerated 'ISR-YYYY-0001', colliding with the
+--      unique constraint. It must count ALL rows -> SECURITY DEFINER.
+--   2. Migration 20260604 added an 8-arg overload via CREATE OR REPLACE without
+--      dropping the original 7-arg function, leaving two overloads (PGRST203 on
+--      partial calls). Drop both signatures, then recreate one.
+DROP FUNCTION IF EXISTS insert_it_service_request_with_number(text, text[], uuid, uuid, text, uuid, text);
+DROP FUNCTION IF EXISTS insert_it_service_request_with_number(text, text[], uuid, uuid, uuid, text, uuid, text);
+
+CREATE FUNCTION insert_it_service_request_with_number(
+  p_event_name text,
+  p_services text[],
+  p_creator_id uuid,
+  p_id uuid DEFAULT NULL,
+  p_office_id uuid DEFAULT NULL,
+  p_requested_by text DEFAULT NULL,
+  p_requested_by_user_id uuid DEFAULT NULL,
+  p_status text DEFAULT 'draft'
+)
+RETURNS TABLE(id uuid, request_number text)
+LANGUAGE plpgsql
+SECURITY DEFINER
+SET search_path = public
+AS $$
+DECLARE
+  v_seq int;
+  v_id uuid;
+  v_number text;
+  v_creator uuid;
+  v_status text;
+  v_is_privileged boolean;
+BEGIN
+  -- Under SECURITY DEFINER the INSERT bypasses the WITH CHECK creator guard, so
+  -- pin the creator to the caller (a signed-in user can't forge someone else's
+  -- ownership). Service-role callers (auth.uid() null) keep the passed value.
+  v_creator := COALESCE(auth.uid(), p_creator_id);
+
+  -- Alias the table: the RETURNS TABLE(id …) OUT column shadows an unqualified
+  -- `id`, so reference profiles columns as p.id / p.role.
+  SELECT EXISTS (
+    SELECT 1 FROM public.profiles p
+    WHERE p.id = auth.uid()
+      AND p.role IN ('admin','programmer','dispatcher','it_staff')
+  ) INTO v_is_privileged;
+
+  -- Non-privileged creators can only start a request in draft/pending_approval,
+  -- so they can't create one already 'scheduled' to skip admin approval.
+  v_status := p_status;
+  IF auth.uid() IS NOT NULL AND NOT v_is_privileged
+     AND v_status NOT IN ('draft','pending_approval') THEN
+    v_status := 'pending_approval';
+  END IF;
+
+  -- ponytail: MAX+1 is racy under concurrent creates (two callers can compute
+  -- the same seq; the loser hits the unique constraint). Fine at this volume;
+  -- switch to a per-year sequence if ISR creation ever goes concurrent.
+  SELECT COALESCE(MAX(
+    CAST(NULLIF(regexp_replace(r.request_number, '^ISR-\d{4}-', ''), '') AS int)
+  ), 0) + 1
+  INTO v_seq
+  FROM it_service_requests r
+  WHERE r.request_number LIKE 'ISR-' || EXTRACT(YEAR FROM now())::text || '-%';
+
+  v_number := 'ISR-' || EXTRACT(YEAR FROM now())::text || '-' || LPAD(v_seq::text, 4, '0');
+  v_id := COALESCE(p_id, gen_random_uuid());
+
+  INSERT INTO it_service_requests (
+    id, request_number, event_name, services,
+    creator_id, office_id, requested_by, requested_by_user_id, status
+  )
+  VALUES (
+    v_id, v_number, p_event_name, p_services,
+    v_creator, p_office_id, p_requested_by, p_requested_by_user_id, v_status
+  );
+
+  RETURN QUERY SELECT v_id, v_number;
+END;
+$$;
@@ -0,0 +1,38 @@
+-- QA-046: pass_slips self-approval bypass.
+--
+-- The original "pass_slips_update" policy (20260306090200_pass_slips.sql) is:
+--   FOR UPDATE USING (user_id = auth.uid()
+--                     OR profile.role IN ('admin','dispatcher'))
+-- with NO WITH CHECK. Postgres then reuses the USING expression as the
+-- WITH CHECK, so the row OWNER can PATCH their own pass slip to any values --
+-- including `status = 'approved'`, `approved_by`, `approved_at`. The approve/
+-- reject buttons are only gated in the UI, so any signed-in user could
+-- self-approve their own pass slip through the REST API (the same class of
+-- flaw as QA-015 on it_service_requests).
+--
+-- Intended behaviour (see pass_slip_provider.dart):
+--   * admin/dispatcher may approve/reject (and otherwise manage) any slip
+--   * the owner may only mark their OWN already-approved slip 'completed'
+--     (they return from the excusal); they must never set 'approved'/'rejected'.
+
+DROP POLICY IF EXISTS "pass_slips_update" ON pass_slips;
+CREATE POLICY "pass_slips_update" ON pass_slips FOR UPDATE TO authenticated
+USING (
+  -- Approvers can act on any slip.
+  EXISTS (
+    SELECT 1 FROM profiles p
+    WHERE p.id = auth.uid() AND p.role IN ('admin', 'dispatcher')
+  )
+  -- The owner may only touch their own slip once it is approved (to complete it).
+  OR (user_id = auth.uid() AND status = 'approved')
+)
+WITH CHECK (
+  EXISTS (
+    SELECT 1 FROM profiles p
+    WHERE p.id = auth.uid() AND p.role IN ('admin', 'dispatcher')
+  )
+  -- The owner's only allowed transition is approved -> completed. This blocks
+  -- self-approval: a pending slip's owner matches neither branch here, and the
+  -- USING clause above already refuses to expose a non-approved slip to them.
+  OR (user_id = auth.uid() AND status = 'completed')
+);
@@ -0,0 +1,67 @@
+-- QA-047: swap_requests authorization bypass.
+--
+-- Live REST probes with a non-privileged it_staff token showed:
+--   * SELECT is not scoped: any authenticated user reads EVERY swap (60/60),
+--     including 48 they are neither requester nor recipient of.
+--   * UPDATE is not scoped: the requester could PATCH their OWN swap straight
+--     to status='accepted' (forging the recipient's acceptance), and a total
+--     non-participant could PATCH ANY swap's status/recipient. This bypasses the
+--     identity checks in respond_shift_swap() entirely (same class as QA-015).
+--
+-- The base swap_requests policies live in a migration that predates this repo
+-- ("already exists in many deployments"), so their names aren't known here;
+-- the DO block drops whatever SELECT/INSERT/UPDATE/DELETE policies exist and
+-- this migration then defines the complete, hardened set. Safe because:
+--   * respond_shift_swap() (accept/reject/escalate) is SECURITY DEFINER
+--     (20260322170000+), so it mutates rows regardless of these policies.
+--   * request_shift_swap() inserts with requester_id = auth.uid().
+--   * The only direct table UPDATE the app makes is reassignSwap(), an
+--     admin/dispatcher action (workforce_provider.dart:reassignSwap).
+
+ALTER TABLE public.swap_requests ENABLE ROW LEVEL SECURITY;
+
+DO $$
+DECLARE
+  pol record;
+BEGIN
+  FOR pol IN
+    SELECT policyname FROM pg_policies
+    WHERE schemaname = 'public' AND tablename = 'swap_requests'
+  LOOP
+    EXECUTE format('DROP POLICY IF EXISTS %I ON public.swap_requests', pol.policyname);
+  END LOOP;
+END $$;
+
+-- SELECT: only the two participants and admins/dispatchers. The provider still
+-- filters client-side, so tightening here only removes rows a user shouldn't
+-- have seen in the first place.
+CREATE POLICY "swap_requests: select" ON public.swap_requests
+  FOR SELECT TO authenticated USING (
+    requester_id = auth.uid()
+    OR recipient_id = auth.uid()
+    OR EXISTS (SELECT 1 FROM public.profiles p
+               WHERE p.id = auth.uid() AND p.role IN ('admin', 'dispatcher'))
+  );
+
+-- INSERT: a user may only open a swap as themselves (request_shift_swap pins
+-- requester_id to auth.uid()).
+CREATE POLICY "swap_requests: insert" ON public.swap_requests
+  FOR INSERT TO authenticated WITH CHECK (
+    requester_id = auth.uid()
+  );
+
+-- UPDATE: only admins/dispatchers may write the row directly (reassignSwap).
+-- Participants accept/reject/escalate through respond_shift_swap(), which is
+-- SECURITY DEFINER and therefore not bound by this policy. This is what closes
+-- the bypass: a requester/recipient can no longer forge status directly.
+CREATE POLICY "swap_requests: update" ON public.swap_requests
+  FOR UPDATE TO authenticated USING (
+    EXISTS (SELECT 1 FROM public.profiles p
+            WHERE p.id = auth.uid() AND p.role IN ('admin', 'dispatcher'))
+  )
+  WITH CHECK (
+    EXISTS (SELECT 1 FROM public.profiles p
+            WHERE p.id = auth.uid() AND p.role IN ('admin', 'dispatcher'))
+  );
+
+-- No DELETE policy: direct deletes stay blocked (unchanged behaviour).
@@ -0,0 +1,25 @@
+-- QA-027: it_service_attachments delete was open to any signed-in user.
+--
+-- The bucket stays public for reads (per product decision), but the DELETE
+-- policy allowed ANY authenticated user to delete ANY file in the bucket
+-- (USING (bucket_id = 'it_service_attachments')). Tighten it to the uploader
+-- (storage sets objects.owner to the uploader's auth.uid) plus the privileged
+-- IT-request roles that manage requests, so users can't delete each other's
+-- attachments. Read/upload policies are left unchanged.
+
+DROP POLICY IF EXISTS "Authenticated users can delete it_service_attachments" ON storage.objects;
+CREATE POLICY "Authenticated users can delete it_service_attachments"
+  ON storage.objects
+  FOR DELETE
+  TO authenticated
+  USING (
+    bucket_id = 'it_service_attachments'
+    AND (
+      owner = auth.uid()
+      OR EXISTS (
+        SELECT 1 FROM public.profiles p
+        WHERE p.id = auth.uid()
+          AND p.role IN ('admin', 'programmer', 'dispatcher', 'it_staff')
+      )
+    )
+  );
@@ -0,0 +1,47 @@
+-- QA-027 follow-up: task_attachments storage writes were open to the `public`
+-- role, i.e. UNAUTHENTICATED users could upload, overwrite, and delete task
+-- attachments (worse than the it_service_attachments delete hole). The bucket
+-- stays public for reads (bucket.public = true, unchanged); lock down writes:
+--   * INSERT: any authenticated user (uploads happen while signed in)
+--   * UPDATE/DELETE: the uploader (objects.owner) or a task-privileged role
+-- The read policy ("task attachments policy 6srt2u_0") is left untouched.
+
+DROP POLICY IF EXISTS "task attachments policy 6srt2u_1" ON storage.objects;  -- was INSERT/public
+CREATE POLICY "task_attachments_insert" ON storage.objects
+  FOR INSERT TO authenticated
+  WITH CHECK (bucket_id = 'task_attachments');
+
+DROP POLICY IF EXISTS "task attachments policy 6srt2u_2" ON storage.objects;  -- was UPDATE/public
+CREATE POLICY "task_attachments_update" ON storage.objects
+  FOR UPDATE TO authenticated
+  USING (
+    bucket_id = 'task_attachments'
+    AND (
+      owner = auth.uid()
+      OR EXISTS (SELECT 1 FROM public.profiles p
+                 WHERE p.id = auth.uid()
+                   AND p.role IN ('admin', 'programmer', 'dispatcher', 'it_staff'))
+    )
+  )
+  WITH CHECK (
+    bucket_id = 'task_attachments'
+    AND (
+      owner = auth.uid()
+      OR EXISTS (SELECT 1 FROM public.profiles p
+                 WHERE p.id = auth.uid()
+                   AND p.role IN ('admin', 'programmer', 'dispatcher', 'it_staff'))
+    )
+  );
+
+DROP POLICY IF EXISTS "task attachments policy 6srt2u_3" ON storage.objects;  -- was DELETE/public
+CREATE POLICY "task_attachments_delete" ON storage.objects
+  FOR DELETE TO authenticated
+  USING (
+    bucket_id = 'task_attachments'
+    AND (
+      owner = auth.uid()
+      OR EXISTS (SELECT 1 FROM public.profiles p
+                 WHERE p.id = auth.uid()
+                   AND p.role IN ('admin', 'programmer', 'dispatcher', 'it_staff'))
+    )
+  );