fix(plugin-multi-tenant): simplified getTenantOptions function and added access control for regular users
What?
Simplified getTenantOptions utility to respect access control when populating tenant selector options.
Why?
Previously, the tenant selector would display all tenants that a user had in their tenants array, regardless of whether they actually had read access to those tenants through the collection's access control configuration. This created a security/UX issue where users could see tenant options they couldn't actually access.
The old implementation manually iterated through user[tenantsArrayFieldName] and populated tenant IDs, bypassing Payload's access control system. This meant that even if a tenant collection had restrictive read access rules, those rules weren't applied when generating selector options.
How?
Removed the branching logic that differentiated between users with "access to all tenants" vs. limited access. Now, all users go through the same code path:
- Single
payload.find()call withoverrideAccess: falseand theusercontext - Payload's built-in access control automatically filters results based on the tenant collection's
readaccess configuration - Returns only tenants the user has actual read access to
Before:
- If
userHasAccessToAllTenants(user)→ fetch all tenants from DB - Otherwise → manually extract tenant IDs from
user[tenantsArrayFieldName], then query those specific IDs - Access control was only partially enforced
After:
- Always use
payload.find()withoverrideAccess: false - Access control is fully enforced by Payload's existing access control system
- Simpler code, fewer edge cases
Question: Should I remove the now-unused parameters (tenantsArrayFieldName, tenantsArrayTenantFieldName, userHasAccessToAllTenants) from getTenantOptions, or keep them?
Fixes https://github.com/payloadcms/payload/discussions/14051
@AdrianMaj Im not opposed to this fix, I like it. Can we see why the test is failing? Specifically this one "should only show users assigned tenants".
The other test is flakey and will pass on re-run, but I would like to figure out if the one that failed is real.
Hey @JarrodMFlesch I'll check this test today and update this PR with fix, thanks!
@JarrodMFlesch The multi-tenant tests are now passing, but some unrelated flaky tests are failing. While the fix should work correctly now, I think we should add an e2e test that verifies if the selector respects access control when populating options. The simplification also made some props unused - should I remove them or keep them for backward compatibility?
@JarrodMFlesch The multi-tenant tests are now passing, but some unrelated flaky tests are failing. While the fix should work correctly now, I think we should add an e2e test that verifies if the selector respects access control when populating options. The simplification also made some props unused - should I remove them or keep them for backward compatibility?
I think we should add a test! Can do that in another PR since I already merged this one.
I don't see any unused args, maybe you can clarify here.
@JarrodMFlesch The multi-tenant tests are now passing, but some unrelated flaky tests are failing. While the fix should work correctly now, I think we should add an e2e test that verifies if the selector respects access control when populating options. The simplification also made some props unused - should I remove them or keep them for backward compatibility?
I think we should add a test! Can do that in another PR since I already merged this one.
I don't see any unused args, maybe you can clarify here.
Works for me! I'll create another PR for the test and reference it here.
Regarding the unused args - sorry for the confusion! They're all being used now. The original implementation had unused args, but I needed them when fixing the access issue.
🚀 This is included in version v3.68.0