Merge pull request #740 from SprocketBot/codex/union-org-team-dual-read

fix(core): union org-team permissions during dual-read

Author: gankoji

core/src/identity/user-org-team-permission/org-team-permission-resolution.service.spec.ts

@@ -0,0 +1,76 @@
+jest.mock("../../mledb/mledb-player/mledb-player.service", () => ({
+    MledbPlayerService: jest.fn(),
+}));
+
+import {MLE_OrganizationTeam} from "../../database/mledb/enums/OrganizationTeam.enum";
+import {OrgTeamPermissionResolutionService} from "./org-team-permission-resolution.service";
+
+describe("OrgTeamPermissionResolutionService", () => {
+    const originalDualRead = process.env.ORG_TEAM_PERMISSION_DUAL_READ;
+
+    const userOrgTeamPermissionService = {
+        listOrgTeamsForUser: jest.fn(),
+    };
+    const mledbPlayerService = {
+        getMlePlayerBySprocketUser: jest.fn(),
+        getPlayerOrgs: jest.fn(),
+    };
+
+    let service: OrgTeamPermissionResolutionService;
+
+    beforeEach(() => {
+        jest.clearAllMocks();
+        service = new OrgTeamPermissionResolutionService(
+            userOrgTeamPermissionService as never,
+            mledbPlayerService as never,
+        );
+    });
+
+    afterEach(() => {
+        process.env.ORG_TEAM_PERMISSION_DUAL_READ = originalDualRead;
+    });
+
+    it("returns Sprocket permissions without consulting MLEDB when dual-read is disabled", async () => {
+        process.env.ORG_TEAM_PERMISSION_DUAL_READ = "false";
+        userOrgTeamPermissionService.listOrgTeamsForUser.mockResolvedValue([
+            MLE_OrganizationTeam.COORDINATOR,
+        ]);
+
+        const result = await service.resolveOrgTeamsForUser(42);
+
+        expect(result).toEqual([MLE_OrganizationTeam.COORDINATOR]);
+        expect(mledbPlayerService.getMlePlayerBySprocketUser).not.toHaveBeenCalled();
+        expect(mledbPlayerService.getPlayerOrgs).not.toHaveBeenCalled();
+    });
+
+    it("returns the union of Sprocket and legacy permissions while dual-read is enabled", async () => {
+        process.env.ORG_TEAM_PERMISSION_DUAL_READ = "true";
+        userOrgTeamPermissionService.listOrgTeamsForUser.mockResolvedValue([
+            MLE_OrganizationTeam.COORDINATOR,
+        ]);
+        mledbPlayerService.getMlePlayerBySprocketUser.mockResolvedValue({id: 7});
+        mledbPlayerService.getPlayerOrgs.mockResolvedValue([
+            {orgTeam: MLE_OrganizationTeam.LEAGUE_OPERATIONS},
+            {orgTeam: MLE_OrganizationTeam.COORDINATOR},
+        ]);
+
+        const result = await service.resolveOrgTeamsForUser(42);
+
+        expect(result).toEqual([
+            MLE_OrganizationTeam.COORDINATOR,
+            MLE_OrganizationTeam.LEAGUE_OPERATIONS,
+        ]);
+    });
+
+    it("keeps Sprocket permissions when the legacy lookup fails during dual-read", async () => {
+        process.env.ORG_TEAM_PERMISSION_DUAL_READ = "true";
+        userOrgTeamPermissionService.listOrgTeamsForUser.mockResolvedValue([
+            MLE_OrganizationTeam.LEAGUE_OPERATIONS,
+        ]);
+        mledbPlayerService.getMlePlayerBySprocketUser.mockRejectedValue(new Error("no linked MLE player"));
+
+        const result = await service.resolveOrgTeamsForUser(42);
+
+        expect(result).toEqual([MLE_OrganizationTeam.LEAGUE_OPERATIONS]);
+    });
+});

core/src/identity/user-org-team-permission/org-team-permission-resolution.service.ts

@@ -11,8 +11,8 @@ import {UserOrgTeamPermissionService} from "./user-org-team-permission.service";
  *
  * **Temporary dual-read:** When `ORG_TEAM_PERMISSION_DUAL_READ=true`, always loads legacy
  * `mledb.player_to_org` as well, compares the two sets, and logs on mismatch. Effective permissions
- * still prefer Sprocket when it has rows; otherwise MLEDB is used only under dual-read (unbackfilled
- * users). Remove the env var and this branch once migration is validated.
+ * are the union of Sprocket and MLEDB during the migration window so partially backfilled users do
+ * not lose legacy access. Remove the env var and this branch once migration is validated.
  */
 @Injectable()
 export class OrgTeamPermissionResolutionService {
@@ -34,6 +34,10 @@ export class OrgTeamPermissionResolutionService {
         return [...new Set(teams)].sort((x, y) => x - y).join(",");
     }
 
+    private combineOrgTeams(...teamSets: MLE_OrganizationTeam[][]): MLE_OrganizationTeam[] {
+        return [...new Set(teamSets.flat())];
+    }
+
     async resolveOrgTeamsForUser(userId: number): Promise<MLE_OrganizationTeam[]> {
         const fromSprocket = await this.userOrgTeamPermissionService.listOrgTeamsForUser(userId);
         const dualRead = process.env.ORG_TEAM_PERMISSION_DUAL_READ === "true";
@@ -67,12 +71,9 @@ export class OrgTeamPermissionResolutionService {
             }
         }
 
-        if (fromSprocket.length > 0) {
-            return fromSprocket;
-        }
-        if (dualRead && fromMledb.length > 0) {
-            return fromMledb;
+        if (dualRead) {
+            return this.combineOrgTeams(fromSprocket, fromMledb);
         }
-        return [];
+        return fromSprocket;
     }
 }

reports/issue-726-org-team-permissions.md

@@ -15,12 +15,40 @@ Admin GraphQL (MLEDB admin guard): `userOrgTeamPermissions`, `setUserOrgTeamPerm
 
 When **`ORG_TEAM_PERMISSION_DUAL_READ=true`**, every resolution loads **both** Sprocket (`user_org_team_permission`) and legacy MLEDB (`player_to_org` via the linked player) and **compares** the two sets. Mismatches are logged (`warn` when both sides have data but differ, or Sprocket has rows while MLEDB is empty; `verbose` when Sprocket is empty but MLEDB has rows—common until backfill).
 
-**Effective permissions:** if Sprocket has any rows for the user, those are returned; otherwise, under dual-read only, MLEDB’s set is returned so unbackfilled users still authorize.
+**Effective permissions:** while dual-read remains enabled, runtime authorization uses the **union** of Sprocket and MLEDB org-team rows. This keeps partially backfilled users from losing legacy permissions like `LEAGUE_OPERATIONS` during the migration window. Once dual-read is disabled, effective permissions are Sprocket-only.
+
+**Risk:** dual-read union still trusts legacy `mledb.player_to_org`. That is intentional only during the migration window; stale legacy rows can continue granting access until the flag is disabled.
 
 **Prod rollout (Pulumi):** the `platform` stack sets `ORG_TEAM_PERMISSION_DUAL_READ` on the core (and monolith) Docker service from Pulumi config key **`org-team-permission-dual-read`**. The committed `infra/platform/Pulumi.prod.yaml` sets it to **`true`** until backfill is done; flip it to **`false`** and `pulumi up` after validation.
 
 **Backfill:** run `scripts/sql/backfill-user-org-team-permission-from-mledb.sql` against prod Postgres (same DB as `mledb` + `sprocket`).
 
+**Audit before leaving dual-read enabled:**
+
+```sql
+WITH legacy AS (
+  SELECT uaa."userId", pto.org_team
+  FROM mledb.player_to_org pto
+  INNER JOIN mledb.player p ON p.id = pto.player_id
+  INNER JOIN sprocket.user_authentication_account uaa
+    ON uaa."accountId" = p.discord_id
+   AND uaa."accountType" = 'DISCORD'
+),
+sprocket_rows AS (
+  SELECT "userId", "orgTeam" AS org_team
+  FROM sprocket.user_org_team_permission
+)
+SELECT legacy."userId", legacy.org_team
+FROM legacy
+LEFT JOIN sprocket_rows sr
+  ON sr."userId" = legacy."userId"
+ AND sr.org_team = legacy.org_team
+WHERE sr."userId" IS NULL
+ORDER BY legacy."userId", legacy.org_team;
+```
+
+This query should return only expected temporary gaps immediately after backfill. Any unexpected rows are still legacy-only grants and should be reconciled before disabling dual-read.
+
 **Removal plan:** After backfilling Sprocket permissions for all users who still rely on MLEDB (or after MLEDB is fully retired for auth), set `org-team-permission-dual-read` / `ORG_TEAM_PERMISSION_DUAL_READ` to `false`, confirm no regressions, then delete the fallback branch in `OrgTeamPermissionResolutionService` and any ops docs referencing the flag.
 
 ## Migration