Skip to content

fix(billing): only the org owner's DOS entitlement may write the org subscription - #53

Merged
JOY (JOY) merged 2 commits into
devfrom
fix/dos-sync-owner-guard
Sep 22, 2026
Merged

JOY (JOY) merged 2 commits into
devfrom
fix/dos-sync-owner-guard

Conversation

@JOY

Copy link
Copy Markdown

What kind of change does this PR introduce?

Bug fix. Billing (DOS shared billing sync). DosSharedBillingService.syncOrg now resolves the caller's membership role in the org and only proceeds to clear/sync the subscription when the role is SUPERADMIN (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. Adds tests/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 as deleteMany({ 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

  • libraries/nestjs-libraries/src/dos-billing/dos-shared-billing.service.ts: injects OrganizationRepository (provided by the global DatabaseModule), resolves membership via the existing getOrgsByUserId (includes the caller's role), and gates the clear/sync on Role.SUPERADMIN. The member branch keeps the existing GET entitlement call so the UI still shows the member's own plan, but performs no writes.
  • tests/bootstrap-dos-sync-guard.spec.ts: pure unit suite (no DB services needed); the two repository modules are stubbed with explicit jest.mock factories because their real prisma import graph cannot load in the CJS jest context (file-type ESM).
  • Deployed-image note: prod runs an older image; this guard takes effect on the next prod promote.

Verification & Testing

  • Local: jest run of the new suite - 5 passed; backend nest build compiles.
  • Incident remediation done separately on prod data (subscription row recreated, isLifetime restored on the 3 orgs) before this PR - this PR prevents recurrence.
  • CI on this PR runs build.yml (including the bootstrap suites with the new spec) and branding-guard.

QA

  1. Open the PR Files changed - confirm only dos-shared-billing.service.ts and tests/bootstrap-dos-sync-guard.spec.ts changed
  2. Run pnpm exec jest --config tests/bootstrap.jest.cjs --ci tests/bootstrap-dos-sync-guard.spec.ts - expect 5 passed
  3. Confirm the guard logic: role SUPERADMIN proceeds to clear/sync; any other role returns the mapped view with zero subscription writes
  4. CI: build.yml and branding-guard.yml green on this PR

Checklist:

  • My code follows the project's code style and architectural conventions.
  • Local verification done: unit suite 5/5 green; backend build compiles.
  • Branding guard - CI gate on this PR.
  • Tests - added and green.
  • Documentation has been updated (if applicable) - incident context embedded in code comments.
  • No secrets or sensitive credentials are included in this PR.
  • I have filled in the QA / Verification section above with real steps to verify this change.

…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).

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +54 to 63
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);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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.

Suggested change
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.

@dos dos Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⏱️ 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:
      1. The method syncOrg is called for a user user-1 who is the SUPERADMIN (owner) of org-1.
      2. this.organizations.getOrgsByUserId(user.id) returns an array of organizations. For org-1, the users array contains the memberships of all users in that organization.
      3. Suppose org-1 has two members: user-2 (ADMIN) and user-1 (SUPERADMIN).
      4. The database or query returns the users array in an order where user-2 is at index 0 and user-1 is at index 1 (e.g., ordered by createdAt or id).
      5. The code executes membership?.users?.[0]?.role. This evaluates to Role.ADMIN (the role of user-2).
      6. The condition Role.ADMIN !== Role.SUPERADMIN is true.
      7. The code enters the if block, treating the current user (user-1) as a non-owner member.
      8. 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.
    • 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 the users array rather than assuming it is at index 0.
    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.

@JOY
JOY (JOY) merged commit 7a89125 into dev Sep 22, 2026
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant