-
Notifications
You must be signed in to change notification settings - Fork 4
feat(org): org-scoped Messages and Requests, and enforce the app/ layering boundary #1033
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
2051fcd
feat(org): give org members their own Messages and Requests surfaces
teetangh 37eea26
refactor(layering): app/ is the routing layer — enforce it
teetangh a698712
chore(git): ignore agent worktrees and generated screen artefacts
teetangh 90695f0
feat(org): let members act on their own org sessions, not just join them
teetangh e12b902
fix(org): show sponsors the group events they funded, not only the on…
teetangh 10aefb5
fix(stream): align DM channel-id precedence, and stop the length guar…
teetangh File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
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
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
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
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
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
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
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,113 @@ | ||
| /** | ||
| * @jest-environment node | ||
| */ | ||
|
|
||
| /** | ||
| * Four call sites compute a DM channel id, and every one of them has to agree | ||
| * with the creators in `actions/stream/chat/channel.action.ts` — they are | ||
| * recomputing an id those creators already used, so any divergence points at a | ||
| * channel that does not exist. | ||
| * | ||
| * The creators resolve context as: | ||
| * | ||
| * plan.organizationId ?? appointment.organizationId ?? null | ||
| * | ||
| * Two genuinely distinct cases sit behind that `??`: a plan can be org-HOSTED | ||
| * while the booking is self-funded, and a personal plan can be booked through an | ||
| * org-funded membership. Review found three consumers reading only the | ||
| * appointment, which treated every org-hosted-plan booking as personal. The | ||
| * reconcile set then looked for `dm-<a>-<b>` while the real channel was `dmo-…` | ||
| * — never re-joined at best, and at worst treated as stale so the user was | ||
| * removed from their own conversation. | ||
| * | ||
| * These assertions are source-level because the point is which expression each | ||
| * site uses, not what a mocked Prisma row would return. | ||
| */ | ||
|
|
||
| import { readFileSync } from "fs"; | ||
| import { join } from "path"; | ||
|
|
||
| import { getDmChannelId, STREAM_CHANNEL_ID_MAX } from "@/lib/stream-utils"; | ||
|
|
||
| const read = (rel: string) => readFileSync(join(process.cwd(), rel), "utf8"); | ||
|
|
||
| describe("every consumer matches the creators' precedence", () => { | ||
| it("the creators put plan org first", () => { | ||
| const src = read("actions/stream/chat/channel.action.ts"); | ||
| expect(src).toContain( | ||
| "consultation.consultationPlan.organizationId ??\n consultation.appointment?.organizationId ??", | ||
| ); | ||
| }); | ||
|
|
||
| it.each([ | ||
| ["reconcile", "actions/stream/chat/event-channel.action.ts", "consultationPlan?.organizationId"], | ||
| ["search", "app/api/stream/channels/search-appointments/route.ts", "consultationPlan.organizationId ??"], | ||
| ["backfill", "scripts/stream/backfill-channel-org.ts", "consultationPlan?.organizationId ??"], | ||
| ])("%s reads the plan org before the appointment's", (_label, rel, needle) => { | ||
| expect(read(rel)).toContain(needle); | ||
| }); | ||
|
|
||
| it.each([ | ||
| ["reconcile", "actions/stream/chat/event-channel.action.ts"], | ||
| ["search", "app/api/stream/channels/search-appointments/route.ts"], | ||
| ["backfill", "scripts/stream/backfill-channel-org.ts"], | ||
| ])("%s loads the plan org it now depends on", (_label, rel) => { | ||
| // A precedence that reads a field the query never selected is silently | ||
| // `undefined`, which falls through to the appointment and reintroduces the | ||
| // bug without failing anything. | ||
| expect(read(rel)).toContain("organizationId: true"); | ||
| }); | ||
| }); | ||
|
|
||
| describe("channel ids stay inside Stream's cap without throwing", () => { | ||
| const CUID_A = "cmqb1757m005stxyoe218odf1"; | ||
| const CUID_B = "cmqb190qa00hotxyor367yjz1"; | ||
| const UUID_A = "aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa"; | ||
| const UUID_B = "bbbbbbbb-bbbb-bbbb-bbbb-bbbbbbbbbbbb"; | ||
|
|
||
| it("leaves ordinary personal ids byte-identical, so no channel moves", () => { | ||
| expect(getDmChannelId(CUID_A, CUID_B)).toBe(`dm-${CUID_A}-${CUID_B}`); | ||
| }); | ||
|
|
||
| it("is order-independent", () => { | ||
| expect(getDmChannelId(CUID_B, CUID_A)).toBe(getDmChannelId(CUID_A, CUID_B)); | ||
| }); | ||
|
|
||
| it("degrades legacy uuid pairs instead of throwing", () => { | ||
| // `dm-<36>-<36>` is 76 chars. Throwing here rejected the whole | ||
| // reconciliation — `getDmPairsForUser` and the `expectedChannelIds` map are | ||
| // not wrapped per-item — so one legacy account broke channel sync for | ||
| // everyone paired with it. | ||
| expect(`dm-${UUID_A}-${UUID_B}`.length).toBeGreaterThan( | ||
| STREAM_CHANNEL_ID_MAX, | ||
| ); | ||
|
|
||
| const id = getDmChannelId(UUID_A, UUID_B); | ||
| expect(id.startsWith("dmh-")).toBe(true); | ||
| expect(id.length).toBeLessThanOrEqual(STREAM_CHANNEL_ID_MAX); | ||
| // Deterministic, or the fallback would strand the conversation. | ||
| expect(getDmChannelId(UUID_B, UUID_A)).toBe(id); | ||
| }); | ||
|
|
||
| it("keeps the three namespaces distinct", () => { | ||
| const personal = getDmChannelId(CUID_A, CUID_B); | ||
| const org = getDmChannelId(CUID_A, CUID_B, "org-1"); | ||
| const legacy = getDmChannelId(UUID_A, UUID_B); | ||
|
|
||
| expect(new Set([personal, org, legacy]).size).toBe(3); | ||
| expect(org.startsWith("dmo-")).toBe(true); | ||
| }); | ||
|
|
||
| it("separates two orgs for the same pair, with room to spare", () => { | ||
| const a = getDmChannelId(CUID_A, CUID_B, "org-1"); | ||
| const b = getDmChannelId(CUID_A, CUID_B, "org-2"); | ||
| expect(a).not.toBe(b); | ||
|
|
||
| // The org segment is the ONLY differentiator between two orgs' otherwise | ||
| // identical pair digest, so a collision would merge two organizations' DM | ||
| // threads. 8 hex chars was 32 bits; this is 64. | ||
| const orgSegment = a.split("-")[1]; | ||
| expect(orgSegment).toHaveLength(16); | ||
| expect(a.length).toBeLessThanOrEqual(STREAM_CHANNEL_ID_MAX); | ||
| }); | ||
| }); | ||
70 changes: 70 additions & 0 deletions
70
__tests__/security/org-appointment-detail-ownership.test.ts
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,70 @@ | ||
| /** | ||
| * The org appointment detail page takes BOTH ids from the URL, and neither | ||
| * constrains the other: `/dashboard/organization/<orgId>/appointments/<apptId>` | ||
| * would happily pair a member's own org with somebody else's appointment. | ||
| * | ||
| * Membership alone is not enough to close that. `requireOrgAccess` answers "is | ||
| * the caller in this org", which says nothing about whether the appointment | ||
| * belongs to the org or whether the caller is on it. Both have to be asked | ||
| * separately, and this file pins that they are — it is the same shape as the | ||
| * SSR ownership hole closed in #1029, where a server page trusted a route param | ||
| * because a client layout appeared to have checked it. | ||
| * | ||
| * Participation rather than `operations.read`: the page renders documents and | ||
| * offers reschedule and cancel, which are participant actions. An operator's | ||
| * view of org sessions stays the metadata-only list (ADR 20), so an OWNER who | ||
| * is not on the session gets a 404 here, not a read. | ||
| */ | ||
|
|
||
| import { readFileSync } from "fs"; | ||
| import { join } from "path"; | ||
|
|
||
| const PAGE = | ||
| "app/dashboard/organization/[orgId]/appointments/[appointmentId]/page.tsx"; | ||
|
|
||
| const src = readFileSync(join(process.cwd(), PAGE), "utf8"); | ||
|
|
||
| describe("org appointment detail binds both ids", () => { | ||
| it("requires org membership first", () => { | ||
| expect(src).toContain("await requireOrgAccess(orgId)"); | ||
| }); | ||
|
|
||
| it("checks the appointment belongs to THIS org, not merely to some org", () => { | ||
| // Without this, any member of any org could read any org-funded | ||
| // appointment by pairing their own orgId with a foreign appointmentId. | ||
| expect(src).toContain("appointment.organizationId !== orgId"); | ||
| }); | ||
|
|
||
| it("checks the caller is a party to the appointment", () => { | ||
| // Requester, trial consultee, or attached to a slot — the same test the | ||
| // consultee detail page applies. | ||
| expect(src).toContain("requestedBy?.id === profile.id"); | ||
| expect(src).toContain("trialSession?.consulteeProfile?.id === profile.id"); | ||
| expect(src).toContain("slotsOfAppointment.some"); | ||
| }); | ||
|
|
||
| it("fails closed on every branch", () => { | ||
| // notFound() rather than a redirect: a redirect would confirm the | ||
| // appointment exists to someone who should not know that. | ||
| const checks = [ | ||
| "if (access.error)", | ||
| "if (!detail || !profile) notFound()", | ||
| "if (appointment.organizationId !== orgId) notFound()", | ||
| "if (!owns) notFound()", | ||
| ]; | ||
| for (const c of checks) expect(src).toContain(c); | ||
| }); | ||
|
|
||
| it("orders the org check before the participation check", () => { | ||
| // Cheap scalar comparison before the ownership walk; also means a | ||
| // cross-org id never reaches the participation logic at all. | ||
| const orgCheck = src.indexOf("appointment.organizationId !== orgId"); | ||
| const ownsCheck = src.indexOf("const owns ="); | ||
| expect(orgCheck).toBeGreaterThan(-1); | ||
| expect(ownsCheck).toBeGreaterThan(orgCheck); | ||
| }); | ||
|
|
||
| it("is a server component, so the checks run before anything streams", () => { | ||
| expect(src.slice(0, 200)).not.toContain('"use client"'); | ||
| }); | ||
| }); |
Oops, something went wrong.
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Exact-string precedence assertion is brittle against reformatting.
Matching a literal multi-line substring (with exact indentation/line breaks) means an unrelated Prettier/formatting pass on
channel.action.tscould fail this test with no functional regression. Consider normalizing whitespace before comparing, or matching on a formatting-agnostic pattern (e.g. a regex ignoring whitespace/newlines), while keeping the source-level intent the docstring describes.♻️ Example: whitespace-agnostic match
📝 Committable suggestion
🤖 Prompt for AI Agents