feat: compose auth checks with an AND/OR relation - #137
Open
mikaelkaron wants to merge 5 commits into
Open
mikaelkaron wants to merge 5 commits into
mikaelkaron wants to merge 5 commits into
Conversation
`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>
Author
|
Pushed a follow-up commit after reviewing this branch again, fixing three defects in the composed policies. One of them granted access:
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 |
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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
applyPolicyruns 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/authprovides for a route (fastify.auth([a, b], { relation })), and the workaround has been to hand-roll it insideapplyPolicy(#93, and thedocs/auth-directive.mdmultiple-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.
New exports, all additive:
composeDirectivePolicy(options)→{ applyPolicy, sdl, authDirective }. Generates the<Name>Relationenum, the<Name>Checkinput (body is yours) and a repeatable@<name>(checks, relation)directive onOBJECT | 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
checkslist denies, any relation that is not exactly'or'combines as AND, and a policy that cannot be evaluated denies. Denial defaults tofalse, so the plugin raises its ownMER_AUTH_ERR_FAILED_POLICY_CHECKnaming the field;onDeny(context, info)overrides it. AparseCheck/validate/preparefailure 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 anErrorbecomes the newMER_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
maina default import of this package resolves to the module namespace object undernode16/nodenext, soapp.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 withexport =— plusmodule.exports.default/module.exports.mercuriusAuthfor 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.jscovers the runtime side.Fixes #132
Parity with
@fastify/authChecked against 5.0.4, and documented in a "Relation to
@fastify/auth" section:@fastify/authrelationper route,defaultRelationdefault'or'done(err); the error is reported only if the composition failsrun: 'all'runs everythingrun: 'first'short circuits; the default runs every check, concurrently'or'nor'and'composenests without a limit; a policy's own list is flat — group with a repeated directive or inevaluatefastify.auth([])throws at wiring timecheckslist denies the field: a schema can declare one, and denial is the fail-closed reading401onDenywithErrorWithPropspicks code and statusVerification
npm run lintclean,tstyche70 assertions,npm run covholds 100% statements/branches/functions/lines repo-wide.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.jswas run against a live server and behaves as documented.docs/composed-policies.md, linked from the README anddocs/apply-policy.md.