Skip to content

fix(content-manager): use RBAC-aware populate in countDraftRelations#25977

Merged
innerdvations merged 5 commits intostrapi:developfrom
Akash504-ai:fix/rbac-count-draft-relations
Apr 15, 2026
Merged

fix(content-manager): use RBAC-aware populate in countDraftRelations#25977
innerdvations merged 5 commits intostrapi:developfrom
Akash504-ai:fix/rbac-count-draft-relations

Conversation

@Akash504-ai
Copy link
Copy Markdown
Contributor

What does it do?

Fixes an issue in countDraftRelations where the entity was loaded with populate: {}, preventing RBAC conditions from being evaluated correctly.

The controller now uses the RBAC-aware populate builder (populate-builder) based on permissionChecker.sanitizedQuery, aligning it with other controllers like findOne and update.

Also adds a test to ensure the endpoint does not return a 403 when RBAC conditions such as "same as creator" or "same role" are enabled.


Why is it needed?

When RBAC conditions like "same as creator" or "same role" are enabled, the controller failed to populate required relational fields (e.g., createdBy), causing permission checks to fail and return a 403 error.

This resulted in a user-facing error:

"An error occurred while fetching draft relations on this document."


How to test it?

  1. Create a role with:

    • Read / Create / Publish permissions
    • Enable conditions like "same as creator" or "same role"
  2. Create an entry as that user

  3. Open / Save / Publish the entry

Before fix:

  • countDraftRelations returns 403
  • UI shows error

After fix:

  • No 403 error
  • No UI error

Also covered by added unit test: countDraftRelations.test.ts


Related issue(s)/PR(s)

Fixes #25965

@vercel
Copy link
Copy Markdown

vercel Bot commented Apr 10, 2026

@Akash504-ai is attempting to deploy a commit to the Strapi Team on Vercel.

A member of the Team first needs to authorize it.

@dosubot dosubot Bot added pr: fix This PR is fixing a bug source: core:content-manager Source is core/content-manager package labels Apr 10, 2026
@innerdvations innerdvations self-assigned this Apr 10, 2026
@jhoward1994
Copy link
Copy Markdown
Contributor

Hi @Akash504-ai

thanks for this work! It looks like the right fix. I'd just like to improve the test coverage slightly, would you be able to apply this .diff and update the branch ?

diff --git a/packages/core/content-manager/server/src/controllers/__tests__/countDraftRelations.test.ts b/packages/core/content-manager/server/src/controllers/__tests__/countDraftRelations.test.ts
index 201d930466..65c7c6396c 100644
--- a/packages/core/content-manager/server/src/controllers/__tests__/countDraftRelations.test.ts
+++ b/packages/core/content-manager/server/src/controllers/__tests__/countDraftRelations.test.ts
@@ -3,61 +3,162 @@
 import createContext from '../../../../../../../tests/helpers/create-context';
 import controller from '../collection-types';
 
-describe('countDraftRelations', () => {
-  beforeEach(() => {
-    global.strapi = {
-      getModel: jest.fn().mockReturnValue({
-        uid: 'test-model',
-        attributes: {},
-      }),
-      plugins: {
-        'content-manager': {
-          services: {
-            'permission-checker': {
-              create: jest.fn().mockReturnValue({
-                cannot: {
-                  read: jest.fn().mockReturnValue(false),
-                },
-                requiresEntity: {
-                  read: jest.fn().mockReturnValue(true),
-                },
-                sanitizedQuery: {
-                  read: jest.fn().mockResolvedValue({}),
-                },
-              }),
-            },
-            'populate-builder': () => ({
-              populateFromQuery: jest.fn().mockReturnThis(),
-              build: jest.fn().mockResolvedValue({ some: 'populate' }),
+// Helpers to create fresh mocks per test — lets us assert call args and
+// swap return values without leaking state between tests.
+const createMocks = (overrides: Record<string, any> = {}) => {
+  const populateFromQuery = jest.fn().mockReturnThis();
+  const buildPopulate = jest.fn().mockResolvedValue({ createdBy: true });
+
+  const findOne = jest.fn().mockResolvedValue({ id: 1, createdBy: { id: 1 } });
+  const countDraftRelations = jest.fn().mockResolvedValue(3);
+  const sanitizedQueryRead = jest.fn().mockResolvedValue({});
+
+  // Permissions default: entity-level check required, no blanket deny
+  const cannotRead = jest.fn().mockReturnValue(false);
+  const requiresEntityRead = jest.fn().mockReturnValue(true);
+
+  return {
+    populateFromQuery,
+    buildPopulate,
+    findOne,
+    countDraftRelations,
+    sanitizedQueryRead,
+    cannotRead,
+    requiresEntityRead,
+    ...overrides,
+  };
+};
+
+const setupStrapi = (mocks: ReturnType<typeof createMocks>) => {
+  global.strapi = {
+    getModel: jest.fn().mockReturnValue({
+      uid: 'test-model',
+      attributes: {},
+    }),
+    plugins: {
+      'content-manager': {
+        services: {
+          'permission-checker': {
+            create: jest.fn().mockReturnValue({
+              cannot: { read: mocks.cannotRead },
+              requiresEntity: { read: mocks.requiresEntityRead },
+              sanitizedQuery: { read: mocks.sanitizedQueryRead },
             }),
-            'document-manager': {
-              findOne: jest.fn().mockResolvedValue({ id: 1, createdBy: { id: 1 } }),
-              countDraftRelations: jest.fn().mockResolvedValue(3),
-            },
+          },
+          'populate-builder': () => ({
+            populateFromQuery: mocks.populateFromQuery,
+            build: mocks.buildPopulate,
+          }),
+          'document-manager': {
+            findOne: mocks.findOne,
+            countDraftRelations: mocks.countDraftRelations,
           },
         },
       },
-    } as any;
+    },
+  } as any;
+};
+
+const createCtx = (overrides: Record<string, any> = {}) => {
+  const ctx = createContext(
+    {
+      params: { model: 'test-model', id: 'doc-1' },
+      query: {},
+      ...overrides,
+    },
+    { state: { userAbility: {} } }
+  );
+
+  // createContext is minimal — add the Koa-style helpers the controller calls
+  ctx.forbidden = jest.fn(() => {
+    ctx.status = 403;
+  });
+  ctx.notFound = jest.fn(() => {
+    ctx.status = 404;
   });
 
-  it('should return count without 403 when RBAC conditions are enabled', async () => {
-    const ctx = createContext(
-      {
-        params: {
-          model: 'test-model',
-          id: 1,
-        },
-        query: {},
-      },
-      {
-        state: {
-          userAbility: {},
-        },
-      }
+  return ctx;
+};
+
+describe('countDraftRelations', () => {
+  it('builds populate from the permission query and passes it to findOne', async () => {
+    const mocks = createMocks();
+    setupStrapi(mocks);
+
+    await controller.countDraftRelations(createCtx());
+
+    // sanitizedQuery.read was called to derive the permission-aware query
+    expect(mocks.sanitizedQueryRead).toHaveBeenCalledWith({});
+
+    // populate-builder was invoked with that query's result
+    expect(mocks.populateFromQuery).toHaveBeenCalledWith({});
+
+    // findOne received the built populate — not an empty object
+    expect(mocks.findOne).toHaveBeenCalledWith(
+      'doc-1',
+      'test-model',
+      expect.objectContaining({ populate: { createdBy: true } })
+    );
+  });
+
+  it('returns the draft relation count on success', async () => {
+    const mocks = createMocks();
+    setupStrapi(mocks);
+
+    const res = await controller.countDraftRelations(createCtx());
+
+    expect(res.data).toBe(3);
+  });
+
+  it('returns 403 when the user lacks read permission entirely', async () => {
+    const mocks = createMocks({ cannotRead: jest.fn().mockReturnValue(true) });
+    setupStrapi(mocks);
+
+    const ctx = createCtx();
+    await controller.countDraftRelations(ctx);
+
+    expect(ctx.status).toBe(403);
+    // Should not attempt to load the entity at all
+    expect(mocks.findOne).not.toHaveBeenCalled();
+  });
+
+  it('returns 404 when the entity does not exist', async () => {
+    const mocks = createMocks({ findOne: jest.fn().mockResolvedValue(null) });
+    setupStrapi(mocks);
+
+    const ctx = createCtx();
+    await controller.countDraftRelations(ctx);
+
+    expect(ctx.status).toBe(404);
+    expect(mocks.countDraftRelations).not.toHaveBeenCalled();
+  });
+
+  it('returns 403 when entity-level RBAC condition fails', async () => {
+    const cannotReadEntity = jest.fn().mockImplementation((entity) => {
+      return entity !== undefined;
+    });
+    const mocks = createMocks({ cannotRead: cannotReadEntity });
+    setupStrapi(mocks);
+
+    const ctx = createCtx();
+    await controller.countDraftRelations(ctx);
+
+    expect(ctx.status).toBe(403);
+    expect(mocks.findOne).toHaveBeenCalledWith(
+      'doc-1',
+      'test-model',
+      expect.objectContaining({ populate: { createdBy: true } })
     );
+  });
+
+  it('skips entity load when RBAC does not require it', async () => {
+    const mocks = createMocks({ requiresEntityRead: jest.fn().mockReturnValue(false) });
+    setupStrapi(mocks);
 
-    const res = await controller.countDraftRelations(ctx);
+    const res = await controller.countDraftRelations(createCtx());
 
     expect(res.data).toBe(3);
+    expect(mocks.findOne).not.toHaveBeenCalled();
+    expect(mocks.populateFromQuery).not.toHaveBeenCalled();
   });
 });

@Akash504-ai
Copy link
Copy Markdown
Contributor Author

Hi @jhoward1994, I have applied the suggested changes and updated the tests to improve RBAC coverage. All tests are passing locally. Please let me know if anything else is needed .

@jhoward1994 jhoward1994 added this to the 5.42.1 milestone Apr 13, 2026
Copy link
Copy Markdown
Contributor

@innerdvations innerdvations left a comment

Choose a reason for hiding this comment

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

LGTM!

@jhoward1994 since you've been working on admin performance, do you think this will have any significant impact?

@jhoward1994 jhoward1994 modified the milestones: 5.42.1, 5.42.2 Apr 14, 2026
@jhoward1994
Copy link
Copy Markdown
Contributor

No major performance concern from me. It's a findOne, not a list so I think it's one extra join. We also definitely need this in order to fix the 403 issue.

@innerdvations innerdvations merged commit 717c387 into strapi:develop Apr 15, 2026
94 of 95 checks passed
@innerdvations innerdvations modified the milestones: 5.42.2, 5.43.0 Apr 15, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr: fix This PR is fixing a bug source: core:content-manager Source is core/content-manager package

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Serious RBAC countDraftRelations 403 error when specific permissions are selected

3 participants