joas8211 / payload-tenancy

Multi-tenancy plugin for Payload CMS
MIT License
122 stars 6 forks source link

readOnly access doesn't work #42

Open ericuldall opened 3 weeks ago

ericuldall commented 3 weeks ago

I have an upload collection with access: { read: () => true } and it seems tenancy plugin is keeping that access from working. Any quick fixes available?

Current code works by disabling tenancy plugin. Otherwise everything seems to work normally.

Payload deps:

├── payload-tenancy@2.1.2
├── payload@2.22.2
ericuldall commented 3 weeks ago

Another note, this is using viteBundler, not webapack. I'm unsure if that matters, but it is a material difference in my build that may be overlooked.

ericuldall commented 3 weeks ago

After further digging this seems to be an issue with the default user isolation strategy. The tenancy library forces a check for req.user.tenant or req.user.tenant.id, but it seems it should only do that IF the original access condition does not pass.

ericuldall commented 3 weeks ago

I vaguely understand the reasoning behind the choice but it seems a bit too overbearing as the payload developer can choose to enforce req.user.tenant in their access block, but here we're now forced to lose access to public collections. In my case users are creating images that should be publicly accessible to end users that know nothing of the payload system.

Perhaps we can do the following:

return ["path", "domain"].includes(options.isolationStrategy)
      ? // Limit requested tenant.
        limitAccess(await original(args), {
          tenant: {
            equals: (args.req as RequestWithTenant).tenant.id,
          },
        })
      : // User must be logged in and have assigned tenant.
        (Boolean(options.allowPublicCollections) && await original(args)) || Boolean(args.req.user?.tenant) &&
          // Limit access to users's tenant.
          limitAccess(await original(args), {
            tenant: {
              equals: args.req.user.tenant.id || args.req.user.tenant,
            },
          });

This should be safe for backwards compatibility and allow users that want public collections to move forward as such.

Thoughts? I'm happy to send a PR if you'd merge it.

ericuldall commented 3 weeks ago

Updated, built, linked, tested and working locally. For your consideration: https://github.com/joas8211/payload-tenancy/pull/43

joas8211 commented 1 week ago

I think this might be solved by existing feature, marking the collection as "shared". It means that the collection can be accessed by all the tenants instead of being scoped to one tenant. That skips processing the whole collection for tenant isolation.

export default buildConfig({
  plugins: [
    tenancy({
      sharedCollections: ["myCollection"],
    }),
  ],
});