Treat null and undefined scope dimensions the same in `AccessCont…

…rolService#isAllowed` (#2643)

Optional scope dimensions may sometimes be `null` or `undefined` depending on how the scope object is created. For instance, when the scope is loaded from the database, the optional dimension will be `null`, but when the scope is coming from GraphQL, the dimension can be `undefined`. Due to strict equality comparison, this led to incorrect access control checks in `AccessControlService#isAllowed`. This is now prevented by treating `null` and `undefined` dimensions as the same when checking the scope.

.changeset/quick-penguins-glow.md

@@ -0,0 +1,10 @@
+---
+"@comet/cms-api": patch
+---
+
+Treat `null` and `undefined` scope dimensions the same in `AccessControlService#isAllowed`
+
+Optional scope dimensions may sometimes be `null` or `undefined` depending on how the scope object is created.
+For instance, when the scope is loaded from the database, the optional dimension will be `null`, but when the scope is coming from GraphQL, the dimension can be `undefined`.
+Due to strict equality comparison, this led to incorrect access control checks in `AccessControlService#isAllowed`.
+This is now prevented by treating `null` and `undefined` dimensions as the same when checking the scope.

packages/api/cms-api/src/user-permissions/access-control.service.test.ts

@@ -0,0 +1,35 @@
+import { Test, TestingModule } from "@nestjs/testing";
+
+import { AbstractAccessControlService } from "./access-control.service";
+import { CurrentUser } from "./dto/current-user";
+
+describe("AbstractAccessControlService", () => {
+    class ConcreteAccessControlService extends AbstractAccessControlService {}
+
+    let service: ConcreteAccessControlService;
+
+    beforeEach(async () => {
+        const module: TestingModule = await Test.createTestingModule({
+            providers: [ConcreteAccessControlService],
+        }).compile();
+
+        service = module.get<ConcreteAccessControlService>(ConcreteAccessControlService);
+    });
+
+    describe("isAllowed", () => {
+        it("should treat null and undefined scope dimensions the same", () => {
+            const user: CurrentUser = {
+                id: "b26d86a7-32bb-4c84-ab9d-d167dddd40ff",
+                name: "User",
+                email: "user@example.com",
+                permissions: [{ permission: "pageTree", contentScopes: [{ domain: "main", language: null }] }],
+            };
+
+            expect(service.isAllowed(user, "pageTree", { domain: "main" })).toBe(true);
+
+            expect(service.isAllowed(user, "pageTree", { domain: "main", language: null })).toBe(true);
+
+            expect(service.isAllowed(user, "pageTree", { domain: "main", language: undefined })).toBe(true);
+        });
+    });
+});

packages/api/cms-api/src/user-permissions/access-control.service.ts

@@ -6,9 +6,18 @@ import { AccessControlServiceInterface } from "./user-permissions.types";
 
 @Injectable()
 export abstract class AbstractAccessControlService implements AccessControlServiceInterface {
-    private checkContentScope(userContentScopes: ContentScope[], contentScope: ContentScope): boolean {
-        return userContentScopes.some((cs) =>
-            Object.entries(contentScope).every(([scope, value]) => (cs as Record<string, unknown>)[scope] === value),
+    private checkContentScope(userContentScopes: ContentScope[], targetContentScope: ContentScope): boolean {
+        return userContentScopes.some((userContentScope) =>
+            Object.entries(targetContentScope).every(([dimension, targetContentScopeValue]) => {
+                const userContentScopeValue = (userContentScope as Record<string, unknown>)[dimension];
+
+                // Treat null and undefined the same
+                if (userContentScopeValue == null && targetContentScopeValue == null) {
+                    return true;
+                }
+
+                return userContentScopeValue === targetContentScopeValue;
+            }),
         );
     }
     isAllowed(user: CurrentUser, permission: string, contentScope?: ContentScope): boolean {