fix(billing): only the org owner's DOS entitlement may write the org subscription - #53
Conversation
…subscription
Incident 2026-09-22: a free-plan member login (test1@dos.me, ADMIN in the
org) triggered the DOS shared-billing sync on the users endpoint and
clearDosSyncedSubscription - deleteMany({ organizationId }) - wiped the org's
ULTIMATE stripe subscription. Any free-plan member of any paid org could do
this on every page load.
- syncOrg now resolves the caller's membership role and only proceeds to
clear/sync when the role is SUPERADMIN (the owner role this codebase
assigns to org creators). Members get a read-only mapped view of their own
DOS plan instead; the org subscription is untouched.
- tests/bootstrap-dos-sync-guard.spec.ts: 5 pure unit cases (owner free
clears, owner plus syncs, member free/plus read-only, non-DOS user no
write). Repository modules are stubbed with explicit jest.mock factories -
their real prisma import graph cannot load in the CJS jest context.
Prod data was restored separately (subscription recreated, isLifetime
flipped back on the 3 orgs).
There was a problem hiding this comment.
Code Review
This pull request introduces a guard in DosSharedBillingService to ensure only organization owners (with the SUPERADMIN role) can modify or clear organization subscriptions, while regular members receive a read-only view of their plan. A new unit test suite is also added to verify this behavior. The review feedback suggests refactoring the logic to eliminate duplicate calls to getEntitlement and mapDosPlanToCrove by extracting them before the role check.
| const membership = await this.organizations | ||
| .getOrgsByUserId(user.id) | ||
| .then((orgs) => orgs.find((o) => o.id === organizationId)); | ||
| if (membership?.users?.[0]?.role !== Role.SUPERADMIN) { | ||
| const entitlement = await this.client.getEntitlement(dosUserId); | ||
| return mapDosPlanToCrove(entitlement.plan); | ||
| } | ||
|
|
||
| const entitlement = await this.client.getEntitlement(dosUserId); | ||
| const mapped = mapDosPlanToCrove(entitlement.plan); |
There was a problem hiding this comment.
The getEntitlement API call and the plan mapping logic are duplicated in both the conditional branch (for non-owners) and the main execution path (for owners). We can fetch the entitlement and map it once at the beginning of the method, and then simply return the mapped plan if the user is not a SUPERADMIN. This improves efficiency by avoiding redundant code and keeping the logic clean and maintainable.
| const membership = await this.organizations | |
| .getOrgsByUserId(user.id) | |
| .then((orgs) => orgs.find((o) => o.id === organizationId)); | |
| if (membership?.users?.[0]?.role !== Role.SUPERADMIN) { | |
| const entitlement = await this.client.getEntitlement(dosUserId); | |
| return mapDosPlanToCrove(entitlement.plan); | |
| } | |
| const entitlement = await this.client.getEntitlement(dosUserId); | |
| const mapped = mapDosPlanToCrove(entitlement.plan); | |
| const entitlement = await this.client.getEntitlement(dosUserId); | |
| const mapped = mapDosPlanToCrove(entitlement.plan); | |
| const membership = await this.organizations | |
| .getOrgsByUserId(user.id) | |
| .then((orgs) => orgs.find((o) => o.id === organizationId)); | |
| if (membership?.users?.[0]?.role !== Role.SUPERADMIN) { | |
| return mapped; | |
| } |
The reviewer verified the real repository modules do in fact load in this CJS jest context (the file-type ESM claim was wrong); the stubs exist for isolation so interaction assertions stay on the injected instances. State that accurately.
There was a problem hiding this comment.
⏱️ Code Review completed (2 files · 7,806 chars · 1 PR unit(s))
⏱️ Adversarial Review completed (Model: qwen3.8-27b)
🔍 Verified Adversarial Review Findings
🟡 IMPORTANT
libraries/nestjs-libraries/src/dos-billing/dos-shared-billing.service.ts:54-58: Incorrect Role Check via Arbitrary Array Index- Failure Trace:
- The method
syncOrgis called for a useruser-1who is theSUPERADMIN(owner) oforg-1. this.organizations.getOrgsByUserId(user.id)returns an array of organizations. Fororg-1, theusersarray contains the memberships of all users in that organization.- Suppose
org-1has two members:user-2(ADMIN) anduser-1(SUPERADMIN). - The database or query returns the
usersarray in an order whereuser-2is at index0anduser-1is at index1(e.g., ordered bycreatedAtorid). - The code executes
membership?.users?.[0]?.role. This evaluates toRole.ADMIN(the role ofuser-2). - The condition
Role.ADMIN !== Role.SUPERADMINistrue. - The code enters the
ifblock, treating the current user (user-1) as a non-owner member. - The owner's DOS entitlement is fetched, but the subscription sync/clear logic is skipped. The owner cannot manage the org subscription, violating the business logic that the owner drives the subscription.
- The method
- Actionable Fix:
The code must verify that the membership being checked actually belongs to the current user. It should find the specific user's membership within theusersarray rather than assuming it is at index0.
- Failure Trace:
const membership = await this.organizations
.getOrgsByUserId(user.id)
.then((orgs) => orgs.find((o) => o.id === organizationId));
const userMembership = membership?.users?.find(
(u) => u.userId === user.id || u.id === user.id
);
if (userMembership?.role !== Role.SUPERADMIN) {
const entitlement = await this.client.getEntitlement(dosUserId);
return mapDosPlanToCrove(entitlement.plan);
}
(Note: The exact field name for the user ID in the users array depends on the Prisma schema. If users is a relation to a User model, it might be u.id. If it's a join table, it might be u.userId. The fix above assumes a standard relation where the user ID is accessible. If the users array contains full User objects, u.id is correct. If it contains membership objects, u.userId is likely correct.)
🛡️ Dismissed Claims
- None: The candidate claim is valid and retained as an Important issue.
What kind of change does this PR introduce?
Bug fix. Billing (DOS shared billing sync).
DosSharedBillingService.syncOrgnow resolves the caller's membership role in the org and only proceeds to clear/sync the subscription when the role isSUPERADMIN(the owner role this codebase assigns to org creators). Non-owner members (USER / ADMIN / SUPERADMIN members of orgs they do not own) get a read-only mapped view of their own DOS plan; the org subscription is never written. Addstests/bootstrap-dos-sync-guard.spec.ts: 5 pure unit cases covering owner free (clears), owner plus (syncs), member free/plus (read-only), and non-DOS users (no write). No schema changes; no checkout/portal/cancel behavior changes.Why was this change needed?
Incident 2026-09-22: a free-plan member login (test1@dos.me, ADMIN in the org JOY on prod) loaded the app, which triggered the DOS shared-billing sync on the users endpoint, and
clearDosSyncedSubscription- implemented asdeleteMany({ organizationId })with no provider or lifetime filter - wiped the org's ULTIMATE stripe subscription. Any free-plan member of any paid org could repeat this on every page load: log in, subscription gone. The guard removes the write path for non-owners while keeping the owner-driven sync (the intended "one checkout covers the member orgs" model) intact.Technical Details & Scope
OrganizationRepository(provided by the global DatabaseModule), resolves membership via the existinggetOrgsByUserId(includes the caller's role), and gates the clear/sync onRole.SUPERADMIN. The member branch keeps the existing GET entitlement call so the UI still shows the member's own plan, but performs no writes.Verification & Testing
QA
Checklist: