Breaking: Add generic _access mechanism (fixes #98)

Adds an extensible `_access` object that any API module can opt into via
`enableAccessControl()`, replacing per-module hardcoded sharing. It extends
the module schema with `_access.public` and registers the base `public`
grant on both accessCheckHook (per-item, single-document reads) and
accessQueryHook (query-level, keeps pagination accurate). Ships the
`isPublicAccess` predicate and `addAccessClause` query helper (both
re-exported) and the `access` schema extension. Other modules extend
`_access` with their own keys and tap both hooks with additive grants.

accessCheckHook observers are now additive access grants, OR-combined
across observers (previously AND-combined). Returning false now abstains
rather than denies; a restriction must veto by throwing. Existing
in-repo observers are unaffected (roles vetoes by throw, adaptframework
is a single observer), but external consumers relying on
return-false-to-deny must switch to throwing.

Author: taylortom

docs/writing-an-api.md

@@ -341,7 +341,9 @@ See the table below for a list of hooks provided by the `AbstractApiModule` clas
 | `postUpdateHook` | After updating a document | No |
 | `preDeleteHook` | Before deleting a document | No |
 | `postDeleteHook` | After deleting a document | No |
-| `accessCheckHook` | When checking access to a resource | No |
+| `accessCheckHook` | Per-item access check (single-document reads) | No |
+| `accessQueryHook` | Merges access-control clauses into a list query (skipped for super users) | No |
+| `queryHook` | Merges user-driven filter clauses into a list query (runs for all users) | Yes |
 
 ### Using hooks
 
@@ -374,17 +376,31 @@ class NotesModule extends AbstractApiModule {
 }
 ```
 
-### Access control with accessCheckHook
+### Access control
 
-Use `accessCheckHook` to implement custom access control:
+Access is an **additive grant model**: observers widen access, they don't restrict it. Both access hooks are OR-combined across all observers, so any one observer granting access is sufficient.
+
+- `accessQueryHook` — the primary gate. Merge a clause into `req.apiData.query` so the database only returns documents the user can see. Filtering here (rather than after the query) keeps pagination counts and the `Link` header accurate. Skipped for super users.
+- `accessCheckHook` — the per-item safety net for single-document reads. Return `true` to grant, a non-truthy value to abstain, or `throw` to hard-veto the item (a veto denies regardless of other grants — this is how a restriction is expressed).
+
+```javascript
+this.accessCheckHook.tap((req, doc) => doc.createdBy === req.auth.user._id.toString())
+this.accessQueryHook.tap(req => addAccessClause(req.apiData.query, { createdBy: req.auth.user._id.toString() }))
+```
+
+#### The generic `_access` mechanism
+
+Call `enableAccessControl()` in your module's `init()` to opt into a shared, extensible `_access` object. This extends the module schema with `_access.public` and registers a `public` grant on both hooks:
 
 ```javascript
-this.accessCheckHook.tap(async (req, doc) => {
-  // Return true to allow access, false or undefined to deny
-  return doc.createdBy === req.auth.user._id.toString()
-})
+async init () {
+  await super.init()
+  await this.enableAccessControl()
+}
 ```
 
+Other modules extend `_access` with their own keys (e.g. `_access.users`, `_access.groups`) and tap both hooks with additional grants, all OR-combined with the base `public` grant. Use `addAccessClause` (exported from `adapt-authoring-api`) for the query-level grant so observers compose safely with each other and with any user-driven `$or`.
+
 ## Overriding methods
 
 You can override database methods to customise behaviour:

index.js

@@ -4,5 +4,6 @@
  */
 export { default as AbstractApiModule } from './lib/AbstractApiModule.js'
 export { default } from './lib/AbstractApiModule.js'
+export { addAccessClause, isPublicAccess } from './lib/utils.js'
 /** @deprecated Use named import { stringifyValues } from 'adapt-authoring-core' instead */
 export { stringifyValues } from 'adapt-authoring-core'

lib/AbstractApiModule.js

@@ -1,6 +1,6 @@
 import _ from 'lodash'
 import { AbstractModule, DataCache, Hook, stringifyValues } from 'adapt-authoring-core'
-import { argsFromReq, generateApiMetadata, httpMethodToDBFunction } from './utils.js'
+import { addAccessClause, argsFromReq, generateApiMetadata, httpMethodToDBFunction, isPublicAccess } from './utils.js'
 import { loadRouteConfig } from 'adapt-authoring-server'
 /**
  * Abstract module for creating APIs
@@ -92,14 +92,15 @@ class AbstractApiModule extends AbstractModule {
      */
     this.postDeleteHook = new Hook()
     /**
-     * Hook invoked to check access to individual data items.
-     * Observer contract — for each item, every observer must approve:
-     *   - `return true`  — approve (or abstain; AND-combined across observers)
-     *   - `return false` — deny
-     *   - `throw`        — deny; for single-doc requests, propagates the thrown error
-     * `undefined` and any non-truthy return are treated as `false`.
-     * Single-doc requests that are denied by any observer respond `401 Unauthorised`.
-     * List requests silently filter denied items.
+     * Hook invoked to check access to individual data items. Observers are additive access grants —
+     * for each item, any one observer approving is sufficient (OR-combined):
+     *   - `return true`  — grant access
+     *   - `return false` — abstain (no grant; another observer may still grant)
+     *   - `throw`        — hard veto; denies the item regardless of other grants, and for single-doc
+     *                      requests propagates the thrown error. Restrictions must veto by throwing.
+     * `undefined` and any non-truthy return are treated as an abstention.
+     * Single-doc requests granted by no observer respond `401 Unauthorised`.
+     * List requests silently filter ungranted items.
      *
      * Runs post-query, so it is best reserved as a safety net for checks that cannot be expressed
      * as a query. For filtering, prefer `accessQueryHook` — filtering at this stage produces short
@@ -605,6 +606,25 @@ class AbstractApiModule extends AbstractModule {
     Object.assign(mongoOpts, { limit: pageSize, skip: mongoOpts.skip || (page - 1) * pageSize })
   }
 
+  /**
+   * Opts this module into the generic `_access` access-control mechanism. Extends the module schema
+   * with `_access.public` and registers the base `public` grant on both access hooks: a per-item grant
+   * on `accessCheckHook` (single-document reads) and a query-level grant on `accessQueryHook` (list /
+   * paginated reads, kept query-level so pagination stays accurate). Other modules extend `_access` with
+   * their own keys and tap both hooks with additional additive grants.
+   * @return {Promise}
+   */
+  async enableAccessControl () {
+    if (!this.schemaName) {
+      return this.log('warn', 'cannot enable access control, no schemaName defined')
+    }
+    const jsonschema = await this.app.waitForModule('jsonschema')
+    jsonschema.extendSchema(this.schemaName, 'access')
+    this.accessCheckHook.tap((req, resource) => isPublicAccess(resource))
+    this.accessQueryHook.tap(req => addAccessClause(req.apiData.query, { '_access.public': true }))
+    this.log('debug', 'access control enabled')
+  }
+
   /**
    * Invokes the access check hook to allow modules to determine whether the request user has sufficient access to the requested resource(s)
    * @param {external:ExpressRequest} req
@@ -620,7 +640,7 @@ class AbstractApiModule extends AbstractModule {
     let error
     await Promise.allSettled((isArray ? data : [data]).map(async r => {
       try {
-        if (!this.accessCheckHook.hasObservers || (await this.accessCheckHook.invoke(req, r)).every(Boolean)) {
+        if (!this.accessCheckHook.hasObservers || (await this.accessCheckHook.invoke(req, r)).some(Boolean)) {
           filtered.push(r)
         }
       } catch (e) {

lib/utils.js

@@ -1,4 +1,6 @@
+export { addAccessClause } from './utils/addAccessClause.js'
 export { argsFromReq } from './utils/argsFromReq.js'
 export { generateApiMetadata } from './utils/generateApiMetadata.js'
 export { httpMethodToAction } from './utils/httpMethodToAction.js'
 export { httpMethodToDBFunction } from './utils/httpMethodToDBFunction.js'
+export { isPublicAccess } from './utils/isPublicAccess.js'

lib/utils/addAccessClause.js

@@ -0,0 +1,29 @@
+const accessGroups = new WeakMap()
+
+/**
+ * OR-merges an access-control clause into a mongo query, mutating it in place. Repeated calls on the
+ * same query accumulate clauses into a single shared `$or` group (additive grants), AND-combined with
+ * any pre-existing query — an existing user-driven `$or` (e.g. search) is lifted into `$and` so it
+ * isn't widened by the grants.
+ * @param {Object} query The mongo query to mutate
+ * @param {Object} clause The access clause to grant (e.g. `{ '_access.public': true }`)
+ * @return {Object} The mutated query
+ * @memberof api
+ */
+export function addAccessClause (query, clause) {
+  let group = accessGroups.get(query)
+  if (!group) {
+    group = []
+    accessGroups.set(query, group)
+    if (query.$or) {
+      query.$and = [...(query.$and ?? []), { $or: query.$or }, { $or: group }]
+      delete query.$or
+    } else if (query.$and) {
+      query.$and.push({ $or: group })
+    } else {
+      query.$or = group
+    }
+  }
+  group.push(clause)
+  return query
+}

lib/utils/isPublicAccess.js

@@ -0,0 +1,9 @@
+/**
+ * Predicate determining whether a resource has been granted public access
+ * @param {Object} resource The resource to check
+ * @return {Boolean}
+ * @memberof api
+ */
+export function isPublicAccess (resource) {
+  return resource?._access?.public === true
+}

schema/access.schema.json

@@ -0,0 +1,22 @@
+{
+  "$schema": "https://json-schema.org/draft/2020-12/schema",
+  "$anchor": "access",
+  "description": "Adds a generic access-control object that modules extend with their own grant keys",
+  "$merge": {
+    "with": {
+      "properties": {
+        "_access": {
+          "type": "object",
+          "default": {},
+          "properties": {
+            "public": {
+              "description": "Whether the resource is accessible to all users",
+              "type": "boolean",
+              "default": false
+            }
+          }
+        }
+      }
+    }
+  }
+}

tests/utils-addAccessClause.spec.js

@@ -0,0 +1,66 @@
+import { describe, it } from 'node:test'
+import assert from 'node:assert/strict'
+import { addAccessClause } from '../lib/utils/addAccessClause.js'
+
+describe('addAccessClause()', () => {
+  it('should add the clause as a top-level $or on an empty query', () => {
+    const query = {}
+    addAccessClause(query, { '_access.public': true })
+    assert.deepEqual(query, { $or: [{ '_access.public': true }] })
+  })
+
+  it('should preserve non-$or top-level fields', () => {
+    const query = { _type: 'course' }
+    addAccessClause(query, { '_access.public': true })
+    assert.deepEqual(query, { _type: 'course', $or: [{ '_access.public': true }] })
+  })
+
+  it('should accumulate multiple grants into one shared $or group', () => {
+    const query = {}
+    addAccessClause(query, { '_access.public': true })
+    addAccessClause(query, { createdBy: 'abc' })
+    addAccessClause(query, { '_access.groups': { $in: ['g1'] } })
+    assert.deepEqual(query, {
+      $or: [
+        { '_access.public': true },
+        { createdBy: 'abc' },
+        { '_access.groups': { $in: ['g1'] } }
+      ]
+    })
+  })
+
+  it('should lift a pre-existing user $or into $and so grants do not widen it', () => {
+    const query = { $or: [{ title: 'a' }, { title: 'b' }] }
+    addAccessClause(query, { '_access.public': true })
+    addAccessClause(query, { createdBy: 'abc' })
+    assert.deepEqual(query, {
+      $and: [
+        { $or: [{ title: 'a' }, { title: 'b' }] },
+        { $or: [{ '_access.public': true }, { createdBy: 'abc' }] }
+      ]
+    })
+    assert.ok(!('$or' in query))
+  })
+
+  it('should append the grant group to a pre-existing $and', () => {
+    const query = { $and: [{ x: 1 }] }
+    addAccessClause(query, { '_access.public': true })
+    assert.deepEqual(query, {
+      $and: [{ x: 1 }, { $or: [{ '_access.public': true }] }]
+    })
+  })
+
+  it('should return the mutated query', () => {
+    const query = {}
+    assert.equal(addAccessClause(query, { '_access.public': true }), query)
+  })
+
+  it('should track grant groups per query object', () => {
+    const a = {}
+    const b = {}
+    addAccessClause(a, { '_access.public': true })
+    addAccessClause(b, { createdBy: 'x' })
+    assert.deepEqual(a, { $or: [{ '_access.public': true }] })
+    assert.deepEqual(b, { $or: [{ createdBy: 'x' }] })
+  })
+})

tests/utils-isPublicAccess.spec.js

@@ -0,0 +1,20 @@
+import { describe, it } from 'node:test'
+import assert from 'node:assert/strict'
+import { isPublicAccess } from '../lib/utils/isPublicAccess.js'
+
+describe('isPublicAccess()', () => {
+  const cases = [
+    { name: 'true when _access.public is true', resource: { _access: { public: true } }, expected: true },
+    { name: 'false when _access.public is false', resource: { _access: { public: false } }, expected: false },
+    { name: 'false when _access.public is missing', resource: { _access: {} }, expected: false },
+    { name: 'false when _access is missing', resource: {}, expected: false },
+    { name: 'false for a truthy non-boolean public value', resource: { _access: { public: 'true' } }, expected: false },
+    { name: 'false when resource is undefined', resource: undefined, expected: false },
+    { name: 'false when resource is null', resource: null, expected: false }
+  ]
+  cases.forEach(({ name, resource, expected }) => {
+    it(`should return ${expected}: ${name}`, () => {
+      assert.equal(isPublicAccess(resource), expected)
+    })
+  })
+})