Skip to content

feat: compose auth checks with an AND/OR relation - #137

Open
mikaelkaron wants to merge 5 commits into
mercurius-js:mainfrom
mikaelkaron:feat/compose-policy
Open

mikaelkaron wants to merge 5 commits into
mercurius-js:mainfrom
mikaelkaron:feat/compose-policy

Conversation

@mikaelkaron

Copy link
Copy Markdown

applyPolicy runs once per protected field and its single result allows or denies that field, so a field guarded by several conditions — "an admin or the owner of the record" — has to combine them by hand in every application. That is the composition @fastify/auth provides for a route (fastify.auth([a, b], { relation })), and the workaround has been to hand-roll it inside applyPolicy (#93, and the docs/auth-directive.md multiple-roles section).

This adds that composition at the seam that already exists, so nothing else about the plugin changes. The plugin still ships no checks: what a check contains and what makes it pass stay with the application.

const { applyPolicy, sdl, authDirective } = mercuriusAuth.composeDirectivePolicy({
  checkInput: 'scope: String!',
  parseCheck: node => ({ scope: node.fields.find(f => f.name.value === 'scope').value.value }),
  prepare: context => context.auth.scopes,
  evaluate: (check, scopes) => scopes.includes(check.scope)
})

// type Query {
//   add(x: Int, y: Int): Int @auth(checks: [{ scope: "admin" }, { scope: "owner" }], relation: OR)
// }
await app.graphql.extendSchema(sdl)
app.register(mercuriusAuth, { applyPolicy, authDirective })

New exports, all additive:

  • composeDirectivePolicy(options){ applyPolicy, sdl, authDirective }. Generates the <Name>Relation enum, the <Name>Check input (body is yours) and a repeatable @<name>(checks, relation) directive on OBJECT | FIELD_DEFINITION, so repeating it ANDs the composed policies and type policies work.
  • composeExternalPolicy(options){ applyPolicy } reading { checks, relation } policy values.
  • compose(checks, relation, run) — the combinator underneath both, for checks that are themselves compositions.
  • composeDirectiveSdl(authDirective, checkInput) — for schemas assembled before the policy.

Fail-closed throughout: an empty checks list denies, any relation that is not exactly 'or' combines as AND, and a policy that cannot be evaluated denies. Denial defaults to false, so the plugin raises its own MER_AUTH_ERR_FAILED_POLICY_CHECK naming the field; onDeny(context, info) overrides it. A parseCheck/validate/prepare failure returns that error instead, which is what separates "you may not do this" from "authorization could not be established". A failure signalled with something that is not an Error becomes the new MER_AUTH_ERR_INVALID_POLICY — returning it unchanged would hand the resolver wrapper a truthy non-Error, which reads as granted. The checks of a field are parsed (or validated) once, not per request.

Also fixes the ESM types (#132)

The composed policies are meant to be used from TypeScript, and on main a default import of this package resolves to the module namespace object under node16/nodenext, so app.register(mercuriusAuth, opts) fails with TS2769 and needs a cast at every call site. The package is CommonJS, so the declarations now follow the convention the fastify plugins use — types in a namespace merged with the callable declaration, exported with export = — plus module.exports.default / module.exports.mercuriusAuth for bundlers and ESM consumers. Named type imports are unchanged.

@arethetypeswrong/cli --pack . reported "Incorrect default export" on all four resolution modes before, and is clean after. test/esm.js covers the runtime side.

Fixes #132

Parity with @fastify/auth

Checked against 5.0.4, and documented in a "Relation to @fastify/auth" section:

@fastify/auth here
relation per route, defaultRelation default 'or' same, per field
Verifier fails with done(err); the error is reported only if the composition fails a check that throws is a check that did not pass, so it cannot deny a field another check grants under OR
Short circuits by default, run: 'all' runs everything run: 'first' short circuits; the default runs every check, concurrently
Rejects a relation that is neither 'or' nor 'and' rejects a relation the mode does not define, per field
One level of sub-arrays compose nests without a limit; a policy's own list is flat — group with a repeated directive or in evaluate
fastify.auth([]) throws at wiring time an empty checks list denies the field: a schema can declare one, and denial is the fail-closed reading
Replies 401 a GraphQL field error; onDeny with ErrorWithProps picks code and status

Verification

  • npm run lint clean, tstyche 70 assertions, npm run cov holds 100% statements/branches/functions/lines repo-wide.
  • 173 tests. One pre-existing failure on main, registration - should error if mercurius is not loaded, is unrelated to this branch and fixed separately in test: assert the fastify 5 plugin dependency error #136.
  • examples/composed-policy.js was run against a live server and behaves as documented.
  • Docs: docs/composed-policies.md, linked from the README and docs/apply-policy.md.

`applyPolicy` runs once per protected field and its single result allows or
denies that field, so a field guarded by several conditions — "an admin *or*
the owner of the record" — has to combine them by hand in every application.
`@fastify/auth` composes verifiers for a route with a relation; this does the
same for the policies of a field, at the seam that already exists.

Three exports are added, built on top of `applyPolicy` so nothing else about
the plugin changes:

- `composeDirectivePolicy(options)` returns the `applyPolicy`, the `sdl`
  defining a `@auth(checks: [...], relation: AND | OR)` directive, and the
  `authDirective` name to register with.
- `composeExternalPolicy(options)` returns the `applyPolicy` reading
  `{ checks, relation }` policy values in external mode.
- `compose(checks, relation)` is the combinator underneath both, for checks
  that are themselves compositions.

The plugin still ships no checks: `parseCheck`/`validate` and `evaluate` are
supplied by the application, which keeps the check shape entirely its own.

Composition is fail-closed throughout: an empty check list denies, any
relation that is not exactly `'or'` combines as AND, and a `parseCheck`,
`validate` or `prepare` failure denies with that error — a new
`MER_AUTH_ERR_INVALID_POLICY` when it was not signalled with an `Error`.
Denial defaults to `false` so the plugin raises its own
`MER_AUTH_ERR_FAILED_POLICY_CHECK` naming the field, with `onDeny` to
override it.

Signed-off-by: Mikael Karon <mikael@karon.se>
The package is CommonJS — `module.exports = plugin`, no `"type"` or `"exports"`
in package.json — but the types declared the plugin with an ESM-style
`export default`. Under `module`/`moduleResolution` `node16`/`nodenext`, a
default import of a CommonJS module resolves to the module namespace object, so
`mercuriusAuth` was typed as `typeof import('mercurius-auth')` instead of
`FastifyPluginAsync<MercuriusAuthOptions>` and `app.register(mercuriusAuth,
opts)` failed with TS2769, forcing a cast at every call site.

The declarations now follow the convention the fastify plugins use for a
CommonJS package: the types live in a namespace merged with the callable
declaration, and the module is exported with `export =`. A default import, a
namespace import and `require` all resolve to the plugin, and the named type
imports are unchanged. `module.exports.default` and
`module.exports.mercuriusAuth` are added alongside, so bundlers and ESM
consumers that reach for either resolve the plugin rather than the namespace.

`@arethetypeswrong/cli` reported "Incorrect default export" on all four
resolution modes before this change and is clean after it.

Fixes mercurius-js#132

Signed-off-by: Mikael Karon <mikael@karon.se>
…licies

Brings the composed policies level with `@fastify/auth`, which is the reference
for what composing auth checks should do:

- `run: 'all' | 'first'`. `'all'` stays the default — the checks of a field run
  concurrently, and a check with a side effect behaves the same under either
  relation — while `'first'` evaluates them in order and stops at the check that
  decides the outcome, which is the short circuit `@fastify/auth` does by
  default.
- A check that throws is a check that did not pass, the way a verifier that
  calls `done(err)` is. Its error is reported only when the composition denies,
  so a failing check no longer denies a field that another check grants under
  OR, and the error explains the denial when it does deny.
- A relation the mode does not define — `'AND'` in an external policy value, an
  enum value the generated directive does not declare — denies the field with
  `MER_AUTH_ERR_INVALID_POLICY` instead of silently falling back to
  `defaultRelation`. `@fastify/auth` rejects such a relation at wiring time; a
  policy is per field, so it is rejected when the field is resolved.
- `compose` rejects a `checks` argument that is not an array, as
  `fastify.auth` does.

The remaining differences are the ones the GraphQL surface dictates, and the
docs now state them: an empty `checks` list denies the field rather than failing
at wiring time, and a denial is a GraphQL field error rather than a `401`.

Signed-off-by: Mikael Karon <mikael@karon.se>
Three defects found reviewing the composed policies, one of which grants access:

- `onDeny` is application code and its return value was passed straight back to
  the resolver wrapper, which reads `result instanceof Error` or a falsy value as
  a denial and *anything else as a grant*. An `onDeny` returning a plain object
  or a string therefore granted the field it was written to deny. What it returns
  is now checked, and a value that is not an `Error` denies with
  `MER_AUTH_ERR_INVALID_POLICY` rather than becoming a grant.
- GraphQL coerces a single value where a list is expected, so
  `@auth(checks: { scope: "admin" })` is valid SDL for a list of one check. It
  was read as no checks at all and the field was denied. A single value is now a
  one check list, while an absent or null argument stays no checks, which denies.
- `parseCheck` and `validate` were invoked through `map`/`forEach`, so they
  received `(node, index, array)` where their documented signature takes one
  argument — the `map(parseInt)` trap for any check parser that takes a second
  parameter of its own. Both are now called with the check alone.

Signed-off-by: Mikael Karon <mikael@karon.se>
@mikaelkaron

Copy link
Copy Markdown
Author

Pushed a follow-up commit after reviewing this branch again, fixing three defects in the composed policies. One of them granted access:

  • onDeny returning a non-Error granted the field. The hook's return value went straight back to the resolver wrapper, which denies on result instanceof Error or a falsy value and reads anything else as a grant — so onDeny: () => ({ message: 'nope' }) granted the field it was written to deny. The return value is now checked, and a non-Error denies with MER_AUTH_ERR_INVALID_POLICY instead of becoming a grant.
  • A single check was read as no checks. GraphQL coerces a single value where a list is expected, so @auth(checks: { scope: "admin" }) is valid SDL for a one check list; it was denied instead. A single value is now a one element list, while an absent or null argument stays no checks, which denies.
  • parseCheck and validate received (node, index, array) through map/forEach, against their one argument signature — the map(parseInt) trap for any parser taking a second parameter of its own. Both are now called with the check alone.

Each has a regression test; coverage stays at 100%.

Worth knowing for this feature: a composed directive always declares an input type for its checks, and filterSchema: true currently fails for any schema whose auth directive takes a named input type (Schema must contain uniquely named types but contains multiple types named "AuthCheck"). That is a pre-existing bug in prune-schema, reproducible on main with a hand-written directive, and I have opened #138 for it. Without #138, composeDirectivePolicy and filterSchema: true cannot be used together.

Denying with `false` rather than an error is what lets the plugin replace the
value of a protected field instead of reporting a field error, so the default
denial and `outputPolicyErrors` compose. An `onDeny` returning an error opts out
of replacement, which the docs now say.

Signed-off-by: Mikael Karon <mikael@karon.se>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Types issue with ESM setup

1 participant