Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(examples): fix access control model for Users. #10102

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
+290 −213
Open

fix(examples): fix access control model for Users. #10102

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2ea4ec3
fix(examples): fix access control model for Users. Only super-admins …
rikrose Dec 20, 2024
d3fa5bb
chore(formatting): fix formatting of multi-tenant example access cont…
rikrose Dec 20, 2024
d0b8252
fix(examples): separate update and delete. Only super-admins can dele…
rikrose Dec 24, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 22 additions & 4 deletions examples/multi-tenant/src/collections/Users/access/create.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import { isSuperAdmin } from '../../../access/isSuperAdmin'
import { getTenantAdminTenantAccessIDs } from '../../../utilities/getTenantAccessIDs'

export const createAccess: Access<User> = (args) => {
const { req } = args
const { req, data }: { req: any; data: User } = args
if (!req.user) {
return false
}
Expand All @@ -17,9 +17,27 @@ export const createAccess: Access<User> = (args) => {

const adminTenantAccessIDs = getTenantAdminTenantAccessIDs(req.user)

if (adminTenantAccessIDs.length > 0) {
return true
if (!data) {
// Triggered by visit to the User collection on Admin console
// If we return true, then this enables the "Create New" button
// so, we do that if we're an admin of at least 1 tenant
return Boolean(adminTenantAccessIDs.length)
}

if (data.roles?.includes('super-admin')) {
// No escalation of privs.
return false
}

if (data.tenants && data.tenants.length === 0) {
// Gotta have at least 1 tenant
return false
}

return false
// allow only if the tenants are where this user is an admin.
return data.tenants?.every((tenant) =>
adminTenantAccessIDs.includes(
typeof tenant.tenant === 'string' ? tenant.tenant : tenant.tenant.id,
),
)
}
25 changes: 20 additions & 5 deletions examples/multi-tenant/src/collections/Users/access/updateAndDelete.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,11 @@ import type { Access } from 'payload'

import { isSuperAdmin } from '../../../access/isSuperAdmin'
import { getTenantAdminTenantAccessIDs } from '../../../utilities/getTenantAccessIDs'
import { User } from '@/payload-types'

export const updateAndDeleteAccess: Access = (args) => {
const { req } = args
const { req, data }: { req: any; data: User } = args
rikrose marked this conversation as resolved.
Show resolved Hide resolved

if (!req.user) {
return false
}
Expand All @@ -15,9 +17,22 @@ export const updateAndDeleteAccess: Access = (args) => {

const adminTenantAccessIDs = getTenantAdminTenantAccessIDs(req.user)

return {
'tenants.tenant': {
in: adminTenantAccessIDs,
},
if (!data) {
// Triggered by visit to the User collection on Admin console
// If we return true, then this enables the "Create New" button
// so, we do that if we're an admin of at least 1 tenant
return Boolean(adminTenantAccessIDs.length)
}

// If you're only a tenant-admin, then you can't delete super-admins
if (data.roles?.includes('super-admin')) {
return false
}

// Can delete a user, only if you're an admin in *all* of their tenants
return data.tenants?.every((tenant) =>
Copy link
Contributor

Choose a reason for hiding this comment

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

What if I'm a tenant admin and I need to remove admin access for another user who is also an admin of another?

Copy link
Author

Choose a reason for hiding this comment

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

This is a use-case not covered, yet, and talking about it has made me remember that this change would allow a tentant-admin to delete a user provided that they aren't also a super-user and also not a tentant-admin of any shared tenants. This does not cover the case of being an admin of another tenant.

It's possible that some query magic might be able to cover the case, but I haven't managed that yet.

I will work on this some more.

Copy link
Author

Choose a reason for hiding this comment

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

I have separated the delete and update access check functions. The delete function checks for super-admin, and otherwise returns false. The update check function checks that the only affected tenants are ones that the requesting user is a tenant-admin of.

adminTenantAccessIDs.includes(
typeof tenant.tenant === 'string' ? tenant.tenant : tenant.tenant.id,
),
)
}