v1.13.4 - #52
Merged
Merged
Conversation
Fixes #44, #45, #46. - #44: BookingDetails only ever rendered the first one_time_room_bookings / weekly_room_bookings row, dropping every additional session on a booking (confirmed against prod data -- some One-Time Room events have 2-3 rows). Now maps over every session, matching the pattern already used for Tabling and in the Administrator bookings tab. - #46: Event cards now show purpose as the primary (bold) line and the body name as secondary, swapped from before. The row-title danger flash moved with it. - #45: Each checklist item now shows a due date, computed from the event's earliest session date minus the relevant Danger Range's near edge (pa_event_mgmt_danger_end / pa_event_engage_danger_end -- settings that already existed for exactly this, per their migration comment: "stored for display and future use"). Added a new isActionDanger check to the pending-actions cascade so a specific overdue form's own label flashes red (.pa-text-danger), independent of the row-level title flash. isActionDanger is wired into app/(dashboard)/layout.tsx, the provider file as it stands at this branch's base commit. A concurrent session in this working tree is mid-refactor of that file (splitting it into a server layout + dashboard-shell.tsx); that in-progress work is intentionally left uncommitted here so it isn't disturbed.
Fixes #48. The /api/events route ordered by created_at (when the booking/tracking row was made). Events are now sorted by their own date -- the earliest session date across any of a booking's child sessions (one-time, weekly occurrence, or tabling), reusing the same sessionDatesOf/minDate helpers already used for the #45 due-date calc. A booking with no session date sorts last rather than disappearing.
fix: Events tab multi-session display, ordering, text priority, and form due dates
Load times stayed high even on a warm function because the cost was never on
the server. Every dashboard route was a client component, so nothing could
start until ~250 KB of JS had downloaded and hydrated -- and only then did a
serial, cross-origin chain begin:
getClaims() a JWKS fetch to supabase.co: 140-260ms, on a fresh TLS
connection, served with no Cache-Control so every page load
pays it again
loadIdentity users + board_memberships, read from the browser
/api/my-rooms a second browser -> Vercel -> Postgres hop, with an Upstash
rate-limit round trip awaited in front of all data work
AuthGuard held the content area behind a skeleton for the first two, so all of
it sat in front of first paint. None of it is cached on a machine the user has
not opened Chambers on before, which is exactly where it was worst.
The database was never implicated: 40 bookings, 371 occurrences, 27 users.
- (dashboard)/layout.tsx becomes a server component that resolves identity in
two in-region reads and passes it down. AuthGuard is deleted; AdminGuard and
EventsGuard read identity from context and render synchronously. As a
side effect, unauthenticated requests now 307 before any JS is sent, rather
than receiving the whole bundle and redirecting from the client.
- /my-rooms reads its bookings in-process while rendering and hands them to
the client as props. The query layer moves to lib/my-rooms-data.ts so the
API route -- still used to refresh after a mutation -- shares one
implementation.
- The rate limiter in /api/my-rooms is started rather than awaited. Its
verdict is still checked before anything is returned, so a throttled caller
is refused either way.
- The service worker precached every build artifact, 103 entries, on first
visit -- downloading the entire app while the requested page was still
fetching its own JS. Route chunks, the noModule polyfills and the
email-only logo now load at runtime instead: 29 entries.
flattenMyRooms deliberately runs in a mount effect rather than during SSR. It
drops bookings before "today", and "today" is the viewer's local date, so
computing it against the server's UTC clock would hand an EDT user loading
after 8pm a list built for tomorrow and hydrate onto rows that do not match.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Conflict was in (dashboard)/layout.tsx, and it was structural rather than textual: this branch turns that file into a server component and moves the client shell to dashboard-shell.tsx, while #47 edited the client shell in place. Git could not see the rename, so it diffed a server component against a client one. Resolved by keeping this branch's layout.tsx (the server component) and reapplying #47's issue-#45 work to dashboard-shell.tsx, which is where the client shell now lives: - the dangerActionIds memo - isActionDanger on the PendingActionsWatch value - dangerActionIds added to that memo's dependency array This is not optional after the merge: dev's PendingActionsWatch interface now declares isActionDanger, so the shell has to provide it to typecheck. Verified by diffing dev's layout.tsx against the merged dashboard-shell.tsx -- the only remaining differences are this branch's own 65 lines. Also fixes a genuine bug in the branch's first commit. The buildExcludes change altered which workbox modules next-pwa bundles, so the generated sw.js began importing ./workbox-8b396bde while the repo still carried only workbox-4754cb34.js. sw.js resolves that import by filename at registration time, so the service worker would have failed to load in production. The runtime file is now committed alongside the sw.js that references it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two problems found in testing. Role changes did not take effect until sign-out. isAdmin/isIEMS were read from the JWT's app_metadata, which is a copy stamped into the access token when it was issued. Granting or revoking a role writes the users row and the auth metadata; neither touches a token already sitting in someone's browser, so a revoked admin kept passing the check until the token expired (an hour by default). resolveShellIdentity now reads admin_role/iems_role from the users row it was already selecting, so the cost is nothing and the answer is current on the next navigation. `is_admin` in the token is exactly `admin_role != null` -- see the sync in app/api/administrator/users/route.ts -- so this is the same predicate read from the authoritative side, not a new rule. This fixes what the dashboard renders and which pages the guards allow. It does NOT close the hole: every route under /api/administrator authorizes on `user.app_metadata?.is_admin`, and the SQL is_admin() behind RLS reads the same token claim, so a revoked admin can still call those directly until it expires. That spans ~25 routes plus a database function and wants its own change. The Events tab showed an IEMS user almost nothing. The listing read `bookings` through RLS, and bookings_select_admin_or_member grants a row to admins and to members of the owning body -- IEMS is neither, it is an app-level role the policy has no concept of. An IEMS user got only the events owned by bodies they happen to sit on: 1 of 4 on current data, which reads as "there are no events". The listing now runs as service role, gated by the route's own admin-or-IEMS check. /api/events/checklist already worked this way; the listing was the outlier. This also restores the "Requested by" name, which RLS (users_select_admin_or_own) had been blanking for IEMS. The third report -- "isActionDanger is not a function" on Events as an admin -- was the pre-merge state of this branch: dev's events page calls isActionDanger while the shell did not yet provide it. Resolved in 5ffc6fc; no code change needed here. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Removes the skeleton frame on My Rooms. The rows are now flattened during the
server render and arrive as props, so the first paint is the list itself.
The reason that was not done in the first place: flattenMyRooms drops bookings
before "today", and "today" was `new Date()` + setHours(0,0,0,0) -- local
midnight in whatever timezone the *runtime* happens to be. That is UTC inside a
Vercel function and the viewer's zone in the browser, and for the last hours of
every Eastern day they name different dates. At 9pm EDT on Aug 27 the server
computes Aug 28 and drops the day's bookings; the browser computes Aug 27 and
keeps them. React would find markup that does not match what it renders while
hydrating, throw the server HTML away and redraw -- a worse flash than the
skeleton, with the day's bookings briefly missing.
So rather than work around the ambiguity, this removes it. booking_date,
occurrence_date and session_date are DATE columns: no time, no offset. They mean
a calendar day in Boston, because that is where the rooms are. todayInAppZone()
formats the current instant in America/New_York and returns 'YYYY-MM-DD', which
is the same string on both sides. The server resolves it once and passes it down
with the rows, so the client's first render uses the identical value and cannot
disagree even across a midnight boundary or clock skew.
Every date decision on the render path now takes that string:
- flattenMyRooms(data, today) past bookings, by ISO string compare
- isWithinDays(date, n, today) the "Next N Days" window
- CalendarView today={today} highlighted cell, and the month it opens on
isWithinDays now measures whole days between two date strings anchored at
Date.UTC rather than subtracting Dates built from local midnight -- across a DST
boundary two local midnights are 23 or 25 hours apart, which divides to 0.958 or
1.04 days instead of 1.
This is also a correctness fix independent of hydration. "Today" was previously
the viewer's date, so a student on co-op in California at 10pm PT saw a
different day's schedule than their peers on campus, and the calendar's "Today"
button jumped to a different cell than the one it highlighted. Both now follow
Boston.
The skeleton is kept for refetches after a mutation, which is what it was for.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Revoking someone's admin role did not take effect until their session ended. Both halves of the authorization stack answered from the JWT. The app routes read `user.app_metadata?.is_admin`, and the SQL is_admin() behind every RLS policy read `auth.jwt() -> 'app_metadata' ->> 'is_admin'`. app_metadata is a copy stamped into the access token when it was issued; granting or revoking a role writes the users row and the auth metadata, and neither touches a token already sitting in a browser. So a revoked admin kept full admin rights until that token expired -- an hour by default, and longer while the tab kept refreshing it. The signature checking was never the problem. The claim was fresh at issue time and simply went stale, and nothing re-read it. my_body_ids() and my_divisions() sit right next to is_admin() and resolve board_memberships through auth.uid() on every call, which is why membership changes always took effect immediately. is_admin() was the outlier; this brings it in line. Database is_admin() now selects from users (admin_role is not null and is_active), and a matching is_iems() replaces the iems_role expression that was only ever written inline. SECURITY DEFINER does double duty: it lets the function read users when the caller cannot, and it keeps users_select_admin_or_own -- which calls is_admin() -- from recursing, because the definer's query is not subject to RLS. That is the same mechanism my_body_ids() already relies on. The semesters and event_tracking policies inlined the raw JWT expression rather than calling is_admin(), so redefining the function alone would have left them stale. They are rewritten onto the helpers. is_active is folded into both helpers on purpose: deactivating someone should revoke their rights, not just hide the UI. Application getAuthedUserWithLiveRoles() returns the same AuthedUser shape with is_admin, admin_role and iems_role overwritten from the users row, so each of the 30 routes that make a role decision changes by one import and keeps its authorization branch exactly as written. That is deliberate: a mechanical swap is a far smaller surface for a mistake than rewriting thirty branches, and this is precisely the code where a mistake is expensive. The service-role routes are the reason this cannot wait for the migration alone. They bypass RLS entirely, so the app-level check is the only thing between a stale token and a privileged write -- the database never sees those calls. Routes that only need the caller's id keep plain getAuthedUser() and the cheap path: /api/my-rooms, /api/me/*, /api/alerts, /api/cancellation-requests, /api/cron/warm. Verified equivalent before changing anything: across every current user, (admin_role is not null and is_active) matches the token's is_admin claim exactly, so this is the same answer read from the authoritative side. What changes is when it updates -- now on the next request rather than the next sign-in. The migration is NOT applied. Review and run it before merging the app changes, since the two halves are independent and the app half is safe on its own. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The previous commit made each authorization check read the users table, so a revoked admin stops being an admin on their next request. This ends the session outright instead of leaving them signed in as a normal user. Deleting the session rows is not sufficient on its own, and the reason is the whole design here. Access tokens are ES256 and verified locally against a cached JWKS -- deliberately, since that is what keeps auth off the network on every request. A token whose session has been deleted therefore still passes signature verification until it expires. Removing auth.sessions kills the *refresh*, which puts the user out within the access token's lifetime -- an hour by default -- not at once. So revocation also stamps users.sessions_revoked_at, and the app refuses any token issued before that stamp. The JWT's `iat` is already in the verified payload, and getAuthedUserWithLiveRoles() already reads that row for the caller's roles, so the check adds no query and no latency. The result is immediate: the very next request from a revoked session is refused. `iat` is whole seconds, so the stamp is floored and the comparison is strict. A token minted in the same second as the revocation is allowed, which avoids rejecting the fresh token of someone signing straight back in; the window that leaves is one second wide, against an attacker who would have had to re-authenticate inside it and would then hold a legitimate session anyway. revoke_user_sessions() is SECURITY DEFINER because the auth schema is not reachable by the API roles, and execute is granted to service_role only -- explicitly revoked from anon and authenticated, since a function that can delete anyone's sessions is not something a logged-in user should be able to call. Sessions are ended on a grant as well as a revocation. A promoted user does not strictly need it now that roles are read live, but it means no one is ever carrying a token that disagrees with their row, and the cost is one re-login after a change that happens rarely. The revoke call is placed after the role write and its failure is surfaced rather than swallowed: an admin who believes they have cut someone off needs to be told when they have not. DEPLOYMENT ORDER: apply the migrations before deploying this. The users select now names sessions_revoked_at, and against a database without that column the query fails, profile comes back null, and every caller is denied -- fail-closed, which is the right direction for a security check but would take the admin surface down until the migration lands. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two defects found while verifying the migration against production before applying it. Both came from reading pg_policies.qual alone. The semesters policies are granted TO authenticated. A create policy with no `to` clause defaults to PUBLIC, so recreating them without it would have silently widened the grant to include anon. event_tracking_insert was missed entirely. An INSERT policy carries its rule in WITH CHECK, not qual, so it read as having no rule and never got rewritten -- it would have been the one policy left on the stale JWT claim. The semesters UPDATE policy likewise had a WITH CHECK of its own that was being dropped. Both files now recreate each policy with its original role grant and its original USING / WITH CHECK split. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A leadership member whose admin role had been revoked, and who had not signed out, could still request bookings for bodies they hold no Leadership in. The route was already converted, but the escalation happens one level down: validateScopeSelection skips the "you hold Leadership in that body" test entirely when ctx.isAdmin is true, and loadScopeContext derived that from app_metadata on whatever user it was handed. The same claim decides which bodies the request form offers, so the UI presented every body and the write accepted them. Auditing that turned up two more routes with the same hole, missed by the earlier sweep because the claim is not read in the route itself: /api/cancellation-requests and /api/revision-requests both reach requireBookingManager(), which short-circuits on user.app_metadata?.is_admin, and both were still passing a token-derived user. A revoked admin could cancel or revise anyone's booking. Both now resolve live roles. Converting call sites one at a time does not stop this recurring, though, since the helpers cannot see how their caller authenticated. So the trust is now explicit: getAuthedUserWithLiveRoles marks the user rolesVerifiedLive, and the shared helpers grant admin only through hasLiveAdmin(), which requires that mark. A caller that has not resolved live roles gets isAdmin false. That direction is deliberate. Forgetting to resolve live roles now under-privileges rather than over-privileges, which turns this class of mistake into a visible bug report instead of a silent escalation. The one caller that still passes a token-derived user is /api/my-rooms, which uses the context only for bodyIds and divisions and already forces isAdmin false, so it is unaffected and keeps its cheaper auth path. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ions perf: resolve the dashboard session on the server, not in the browser
security: resolve roles from the users table, not the access token
package.json, the lockfile's two root entries, and the sidebar label. Only lines 3 and 9 of package-lock.json are the project's own version -- the other 1.13.x strings in that file are dependency versions and are left alone. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.