tighten up site access, handle user additions more cleanly

src/access/adminOrSite.ts

@@ -1,50 +1,56 @@
-import type { Access, PayloadRequest } from 'payload'
-import type { Where } from 'payload'
+import type { Access, CollectionSlug, Where } from 'payload'
+import type { Post, Page, User, Site } from '@/payload-types'
 import { getSiteId } from './preferenceHelper'
 
-// Access control function signatures by method:
-// create: req, data
-// read: req, id
-// update: req, id, data
-// delete: req, id
-
-export const adminOrSiteUser: Access = async ({ req: { user, payload }, data }) => {
-  if (!user) return false
-  if (user.isAdmin) return true
-
-  const siteId = await getSiteId(payload, user)
-  if (!siteId) return false
-
-  // if the user doesn't have access to the site in the
-  // new data (create/update), deny access
-  if (data && siteId !== (data?.site?.id)) return false
+// ideally this code could be handled via a single generic but
+// certain access operations don't pass `data` which is the only
+// way to infer the shape of the documents we're operating on
+
+export function getAdminOrSiteUser(slug: CollectionSlug) {
+  const adminOrSiteUser: Access<Post | Page | User | Site > = async ({ req: { user, payload }, data }) => {
+    if (!user) return false
+    if (user.isAdmin) return true
+
+    const siteId = await getSiteId(payload, user.id)
+    if (!siteId) return false
+
+    // if collection data exists, extract the site id and match against it
+    // false for no match.
+    // note that this block is potentially unnecessary/paranoid:
+    // it prevents a user from changing data TO a site they don't have access to,
+    // the queries below already prevent them from changing existing data they don't
+    // have access to
+    if (data) {
+      if ('site' in data) {
+        const dataSiteId = typeof data.site === 'number' ? data.site : data.site.id
+        if (siteId !== dataSiteId) return false
+
+      } else if ('sites' in data) {
+        const dataSiteIds = data.sites?.map(site => typeof site.site === 'number' ? site.site : site.site.id)
+        if (!(dataSiteIds && dataSiteIds.includes(siteId))) return false
+
+      } else if ('id' in data){
+        if (data.id !== siteId) return false
+      }
+    }
 
-  // pass a query ensuring the user has access to the prior
-  // data, matching on site.id
-  const query: Where = {
-    "site.id": {
-      equals: siteId
+    let queryPath: string
+    if (slug === 'users') {
+      queryPath = "sites.site.id"
+    } else if (slug === 'sites') {
+      queryPath = "id"
+    } else {
+      queryPath = "site.id"
     }
-  }
-  return query
-}
 
-// TODO: try to handle this via generic and/or DRY this up
-// I couldn't find a suitable argument to use to help decide which field to query on
-// it's only used for reads against the user collection
-export const adminOrSiteUserUserCollection: Access = async ({ req: { user, payload } }) => {
-  if (!user) return false
-  if (user.isAdmin) return true
-
-  const siteId = await getSiteId(payload, user)
-  if (!siteId) return false
-
-  // pass a query ensuring the user has access to the prior
-  // data, matching on sites -> site.id
-  const query: Where = {
-    "sites.site.id": {
-      equals: siteId
+    // pass a query ensuring the user has access to the queried data
+    const query: Where = {
+      [queryPath]: {
+        equals: siteId
+      }
     }
+
+    return query
   }
-  return query
+  return adminOrSiteUser
 }

src/access/preferenceHelper.ts

@@ -7,7 +7,7 @@ const siteKey = 'site-key'
 // TODO: this will become easier after https://github.com/payloadcms/payload/pull/9511
 // is merged
 
-export async function getSiteId (payload: BasePayload, user: User & { collection: "users"}): Promise<number | undefined> {
+export async function getSiteId (payload: BasePayload, userId: number): Promise<number | undefined> {
     const siteIds = await payload.find({
         collection: 'payload-preferences',
         limit: 1,
@@ -20,12 +20,12 @@ export async function getSiteId (payload: BasePayload, user: User & { collection
             },
             {
               'user.relationTo': {
-                equals: user.collection,
+                equals: 'users',
               },
             },
             {
               'user.value': {
-                equals: user.id,
+                equals: userId,
               },
             },
           ],

src/collections/Pages/index.ts

@@ -1,6 +1,6 @@
 import type { CollectionConfig } from 'payload'
 
-import { adminOrSiteUser } from '../../access/adminOrSite'
+import { getAdminOrSiteUser } from '../../access/adminOrSite'
 
 import { lexicalEditor, lexicalHTML, HTMLConverterFeature } from '@payloadcms/richtext-lexical'
 
@@ -15,10 +15,10 @@ import { addSite } from '@/hooks/addSite'
 export const Pages: CollectionConfig<'pages'> = {
   slug: 'pages',
   access: {
-    create: adminOrSiteUser,
-    delete: adminOrSiteUser,
-    read: adminOrSiteUser,
-    update: adminOrSiteUser,
+    create: getAdminOrSiteUser('pages'),
+    delete: getAdminOrSiteUser('pages'),
+    read: getAdminOrSiteUser('pages'),
+    update: getAdminOrSiteUser('pages'),
   },
   // This config controls what's populated by default when a page is referenced
   // https://payloadcms.com/docs/queries/select#defaultpopulate-collection-config-property

src/collections/Posts/hooks/populateAuthors.ts

@@ -24,7 +24,6 @@ export const populateAuthors: CollectionAfterReadHook = async ({ doc, req, req:
 
     doc.populatedAuthors = authorDocs.map((authorDoc) => ({
       id: authorDoc.id,
-      name: authorDoc.name,
     }))
   }
 

src/collections/Posts/index.ts

@@ -10,16 +10,16 @@ import { slugField } from '@/fields/slug'
 
 import { customFields } from './custom'
 import { adminField } from '@/access/admin'
-import { adminOrSiteUser } from '@/access/adminOrSite'
+import { getAdminOrSiteUser } from '@/access/adminOrSite'
 import { addSite } from '@/hooks/addSite'
 
 export const Posts: CollectionConfig<'posts'> = {
   slug: 'posts',
   access: {
-    create: adminOrSiteUser,
-    delete: adminOrSiteUser,
-    read: adminOrSiteUser,
-    update: adminOrSiteUser,
+    create: getAdminOrSiteUser('posts'),
+    delete: getAdminOrSiteUser('posts'),
+    read: getAdminOrSiteUser('posts'),
+    update: getAdminOrSiteUser('posts'),
   },
   // This config controls what's populated by default when a post is referenced
   // https://payloadcms.com/docs/queries/select#defaultpopulate-collection-config-property

src/collections/Sites/index.ts

@@ -1,17 +1,17 @@
 import type { CollectionAfterChangeHook, CollectionConfig } from 'payload'
-import { admin } from '../../access/admin'
 import { Site } from '@/payload-types'
 import { v4 as uuidv4 } from 'uuid'
+import { getAdminOrSiteUser } from '@/access/adminOrSite'
 
 const createSiteBot: CollectionAfterChangeHook<Site> = async ({
-  doc, req, operation
+  doc, req, operation,
 }) => {
   const { payload } = req
   if (operation === 'create') {
     const bot = await payload.create({
       collection: 'users',
       data: {
-        email: '[email protected]',
+        email: `cloud-gov-pages-operations+${doc.name}@gsa.gov`,
         sites: [
           {
             site: doc.id,
@@ -32,16 +32,15 @@ const createSiteBot: CollectionAfterChangeHook<Site> = async ({
 export const Sites: CollectionConfig = {
   slug: 'sites',
   access: {
-    admin: admin,
-    create: admin,
-    delete: admin,
-    read: admin,
-    update: admin,
+    create: getAdminOrSiteUser('sites'),
+    delete: getAdminOrSiteUser('sites'),
+    read: getAdminOrSiteUser('sites'),
+    update: getAdminOrSiteUser('sites'),
   },
   admin: {
     defaultColumns: ['name', 'updatedAt', 'createdAt'],
     useAsTitle: 'name',
-    hidden: false,
+    hidden: ({ user }) => !(user && user.isAdmin)
   },
   fields: [
     {

src/collections/Users/index.ts

@@ -1,6 +1,8 @@
 import { admin, adminField } from '@/access/admin'
-import { adminOrSiteUser, adminOrSiteUserUserCollection } from '@/access/adminOrSite'
-import type { CollectionAfterChangeHook, CollectionBeforeChangeHook, CollectionConfig, User } from 'payload'
+import { getAdminOrSiteUser } from '@/access/adminOrSite'
+import { getSiteId } from '@/access/preferenceHelper'
+import { Site } from '@/payload-types'
+import { APIError, ValidationError, type CollectionAfterChangeHook, type CollectionBeforeChangeHook, type CollectionConfig, type FieldHook, type User } from 'payload'
 import { v4 as uuidv4 } from 'uuid'
 
 export const roles = ['manager', 'user', 'bot'] as const
@@ -20,23 +22,75 @@ const userEmail: CollectionAfterChangeHook<User> = async ({
 }
 
 const addSub: CollectionBeforeChangeHook<User> = async ({
-  data, req: { payload }, operation
+  data, operation
 }) => {
   if (operation === 'create') {
     data.sub = uuidv4()
   }
 }
 
+const testEmailUniqueness: FieldHook<User> = async({
+  data, originalDoc, req: { payload, user }, value, operation
+}) => {
+  if (operation === 'create' || operation == 'update')
+  // if value is unchanged, skip validation
+  if (originalDoc?.email === value) {
+    return value
+  }
+
+  const match = await payload.find({
+    collection: 'users',
+    depth: 2,
+    where: {
+      email: {
+        equals: value
+      }
+    }
+  })
+  if (match.docs.length && user ) {
+    const existingUser = match.docs[0]
+    if (existingUser && user.isAdmin) {
+      throw new ValidationError({
+        errors: [{
+          message: `User with email ${value} exists.`,
+          path: 'email',
+        }]
+      })
+    }
+    // if the user matches the current site, this is a true validation error
+    // if the user exists but on another site, treat this essentially like a new user
+    const siteId = await getSiteId(payload, user.id)
+    if (siteId && existingUser.sites.map(s => (s.site as Site).id).includes(siteId)) {
+      throw new ValidationError({
+        errors: [{
+          message: `User with email ${value} exists for this site`,
+          path: 'email',
+        }]
+      })
+    } else if (data) {
+      await payload.update({
+        collection: 'users',
+        id: existingUser.id,
+        data: {
+          sites: data.sites.concat(existingUser.sites)
+        }
+      })
+      return false
+    }
+  }
+  return value
+}
+
 export const Users: CollectionConfig = {
   slug: 'users',
   access: {
-    read: adminOrSiteUserUserCollection,
-    update: admin, // TODO: update to admin/sitemanager
-    delete: admin, // TODO: update to admin/sitemanager
-    create: admin, // TODO: update to admin/sitemanager
+    read: getAdminOrSiteUser('users'),
+    // update: getAdminOrSiteUser('users'), // TODO: update to admin/sitemanager
+    // delete: getAdminOrSiteUser('users'), // TODO: update to admin/sitemanager
+    // create: getAdminOrSiteUser('users'), // TODO: update to admin/sitemanager
   },
   admin: {
-    defaultColumns: ['email', 'updatedAt', 'siteRoles'],
+    defaultColumns: ['email', 'updatedAt', 'sites'],
     useAsTitle: 'email',
     hidden: false,
   },
@@ -50,14 +104,18 @@ export const Users: CollectionConfig = {
     }
   },
   fields: [
-    {
-      name: 'name',
-      type: 'text',
-    },
     {
       name: 'email',
       type: 'email',
       required: true,
+      access: {
+        // read: adminField,
+        // create: adminField, // TODO: update to admin/sitemanager
+        // update: adminField,
+      },
+      hooks: {
+        beforeValidate: [testEmailUniqueness]
+      }
     },
     {
       name: 'sub', // we have to create this manually or it isn't added to the JWT payload-token
@@ -68,18 +126,23 @@ export const Users: CollectionConfig = {
         read: adminField,
         create: adminField, // TODO: update to admin/sitemanager
         update: adminField,
+      },
+      admin: {
+        condition: (_a, _b, { user }) => Boolean(user?.isAdmin),
+        disableListColumn: true,
+        disableListFilter: true
       }
     },
     {
       name: 'sites',
       type: 'array',
       saveToJWT: true,
+      required: true,
       access: {
-        read: adminField,
-        create: adminField, // TODO: update to admin/sitemanager
-        update: adminField,
+        // read: adminField,
+        // create: adminField, // TODO: update to admin/sitemanager
+        // update: adminField,
       },
-      // TODO: custom display component
       // admin: {
       //   components: {
       //     RowLabel:
@@ -93,6 +156,11 @@ export const Users: CollectionConfig = {
           required: true,
           index: true,
           saveToJWT: true,
+          access: {
+            // read: adminField,
+            // create: adminField,
+            // update: adminField,
+          }
         },
         {
           name: 'role',

src/hooks/addSite.ts

@@ -7,7 +7,7 @@ export const addSite: CollectionBeforeChangeHook<Post | Page> = async ({
   data, req: { payload, user }, operation
 }) => {
   if (operation === 'create' && user && !user.isAdmin) {
-    data.site = await getSiteId(payload, user)
+    data.site = await getSiteId(payload, user.id)
   }
   return data
 }