Filtering output #125

Bugs5382 · 2023-10-29T12:49:26Z

Registering a "directive" that can check a user's role could modify the "output" of string types if the directive "apply policy" returned 'false'. If the 'valueOverride' value has a string or a function that returns a string, that is what will go in it's place. Otherwise it will make the field null regardless of the graphql type. This will override any graphql errors that might say that something was missing.

app.register(mercuriusAuth, {
    authContext: hasPermissionContext,
    applyPolicy: hasFilterPolicy,
    outputPolicyErrors: {
      enabled: false,
      valueOverride: 'foo'
    },
    authDirective: 'filterData'
  })

Will Close #124

basic tests are in there are designed correctly // replacement "output" is not yet in there

initial unit tests pass // need to ensure that we are "replacing" only on String scalar types. we are going to actually return an error at this point

jonnydgreen · 2023-10-30T21:06:40Z

Great work so far! Give me a shout when it's ready for review! :)

make sure registration errors out correctly if not done correctly

how to use, but also some examples

Bugs5382 · 2023-10-31T02:43:03Z

@jonnydgreen So.. 2 issues..

I can't seem to generate unit tests for this line 59 or line 108-109. Ideas?

Also the refresh.js unit test fails all 4. Not sure why.

Also added some documentation in the mix. I will need some proof reading by a 2nd person after I re-read it tomorrow. It will form the basis of the example I place in the example folder once done.

jonnydgreen · 2023-10-31T18:44:23Z

this line 59

If you define a policy that returns an error instance that should do the trick!

line 108-109

If you define a field resolver this should also do the trick :)

Also the refresh.js unit test fails all 4. Not sure why.

Do you have any error logs?

Bugs5382 · 2023-11-01T09:40:18Z

Do you have any error logs?

TAP version 13
# Subtest: polling interval with a new schema should trigger refresh of schema policy build
    1..4
    ok 1 - expect truthy value
    ok 2 - should be equivalent
    not ok 3 - should be equivalent
      ---
      diff: |
        --- expected
        +++ actual
        @@ -3,22 +3,7 @@
             "me": Object {
               "id": "u1",
               "name": "John",
        -      "lastName": null,
        +      "lastName": "Doe",
             },
           },
        -  "errors": Array [
        -    Object {
        -      "message": "Failed auth policy check on lastName",
        -      "locations": Array [
        -        Object {
        -          "line": 5,
        -          "column": 7,
        -        },
        -      ],
        -      "path": Array [
        -        "me",
        -        "lastName",
        -      ],
        -    },
        -  ],
         }
      at:
        line: 169
        column: 7
        file: refresh.js
        type: Test
      stack: |
        Test.<anonymous> (refresh.js:169:7)
      source: |2
      
            t.same(JSON.parse(res.body), {
        ------^
              data: {
                me: {
      ...
    
    # test count(3) != plan(4)
    # failed 1 of 3 tests
not ok 1 - polling interval with a new schema should trigger refresh of schema policy build # time=-49307724.042ms

I am certain that this was passing prior to me starting...

Bugs5382 · 2023-11-01T10:32:30Z

@jonnydgreen Ok. Throwing up the 'white' flag. Not sure how to generate an error that would trigger or the resolve. Also with the refresh not sure. I think it's also causing another line not to "pass" check with 100% coverage:

Suites:   1 failed, 16 passed, 17 of 17 completed
Asserts:  4 failed, 217 passed, 2 todo, of 223
Time:   4sERROR: Coverage for lines (98.89%) does not meet global threshold (100%)
ERROR: Coverage for branches (98.59%) does not meet global threshold (100%)
ERROR: Coverage for statements (98.9%) does not meet global threshold (100%)
-----------------------|---------|----------|---------|---------|-------------------
File                   | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s 
-----------------------|---------|----------|---------|---------|-------------------
All files              |    98.9 |    98.59 |     100 |   98.89 |                   
 mercurius-js-auth     |     100 |    91.66 |     100 |     100 |                   
  index.js             |     100 |    91.66 |     100 |     100 | 25                
 mercurius-js-auth/lib |   98.82 |       99 |     100 |    98.8 |                   
  auth.js              |    96.7 |    96.66 |     100 |   96.59 | 58,107-108        
  errors.js            |     100 |      100 |     100 |     100 |                   
  filter-schema.js     |     100 |      100 |     100 |     100 |                   
  prune-schema.js      |     100 |      100 |     100 |     100 |                   
  symbols.js           |     100 |      100 |     100 |     100 |                   
  validation.js        |     100 |      100 |     100 |     100 |                   
-----------------------|---------|----------|---------|---------|-------------------

jonnydgreen · 2023-11-02T22:53:16Z

Thanks for the update @Bugs5382, fantastic work! I'm currently away but will be back on Sunday where I'll take a closer look and see what I can find/replicate :)

index.js

jonnydgreen · 2023-11-06T21:15:54Z

Hi @Bugs5382 !

line 108-109

For this one, if you define a schema of the form, this should do the trick :)

directive @filterData (disallow: String!) on FIELD_DEFINITION

type Message {
  message: String!
  notes: String
}

type Query {
  publicMessages: @filterData (disallow: "no-public-messages")
}

The reason why this is picked up the coverage is because the field publicMessages is a resolver and therefore the code that checks if the field.resolver is a function is hit.

line 59

For this one, the following policy should do the trick

async function hasFilterPolicyReturnError (authDirectiveAST, parent, args, context, info) {
  const notNeeded = authDirectiveAST.arguments.find(arg => arg.name.value === 'disallow').value.value

  const policyPassed = !context.auth.permission.includes(notNeeded)
  if (!policyPassed) {
    // This is the key bit
    return new MER_AUTH_ERR_FAILED_POLICY_CHECK(info.fieldName)
  }
}

fixes unit testing Co-authored-by: Jonny Green <[email protected]>

Bugs5382 · 2023-11-07T01:05:48Z

I'll get the other two inserted and updated soon!

Bugs5382 · 2023-11-07T12:08:41Z

Ok. So after you pointed that it is on a "resolver", I made it error out in the auth.js file:

  [kWrapFieldResolverReplace] (schemaTypeField, fieldPolicy, fieldReplace) {
    // Overwrite field resolver
    const fieldName = schemaTypeField.name
    if (typeof schemaTypeField.resolve === 'function') {
      throw new MER_AUTH_ERR_USAGE_ERROR('Replacement can not happen on a resolver. Only a field.')
    } else {
      schemaTypeField.resolve = this[kMakeProtectedResolverReplace](fieldPolicy, (parent) => parent[fieldName], fieldReplace)
    }
  }

and updated the test to check for the error output. I am not sure doing filtering on the resolver/function does any good. I didn't want to have to check for string. Unless you think it's "needed". I never envisoneded it that way @jonnydgreen .

100% coverage coming!

removed resolver filtering // fixed refresh test again // 100% code coverage!

Bugs5382 · 2023-11-07T12:11:01Z

Suites:   17 passed, 17 of 17 completed
Asserts:  225 passed, 2 todo, of 227
Time:   5s
-----------------------|---------|----------|---------|---------|-------------------
File                   | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s 
-----------------------|---------|----------|---------|---------|-------------------
All files              |     100 |      100 |     100 |     100 |                   
 mercurius-js-auth     |     100 |      100 |     100 |     100 |                   
  index.js             |     100 |      100 |     100 |     100 |                   
 mercurius-js-auth/lib |     100 |      100 |     100 |     100 |                   
  auth.js              |     100 |      100 |     100 |     100 |                   
  errors.js            |     100 |      100 |     100 |     100 |                   
  filter-schema.js     |     100 |      100 |     100 |     100 |                   
  prune-schema.js      |     100 |      100 |     100 |     100 |                   
  symbols.js           |     100 |      100 |     100 |     100 |                   
  validation.js        |     100 |      100 |     100 |     100 |                   
-----------------------|---------|----------|---------|---------|-------------------

> [email protected] typescript
> tsd

🥳 🎉

jonnydgreen · 2023-11-07T22:05:01Z

Fantastic work! I'll take a detailed look tomorrow evening!

jonnydgreen

Functionally looks great, just some comments around testing and docs! 🚀 Could we add an example as well? :)

docs/schema-replacement.md

test/result-filter-output.js

docs/schema-replacement.md

errors were still coming over for null values // unit tests updated and fixed code that does not send an error back

if outputPolicyErrors.enabled was 'true,' then no errors would show on a failure other than schema ones if they excited after a replacement

Bugs5382 · 2023-11-10T12:07:14Z

Functionally looks great, just some comments around testing and docs! 🚀 Could we add an example as well? :)

Example uploaded. Unit testing and come code fixed to solve them. 👍

jonnydgreen

LGTM!

Bugs5382 · 2023-11-13T03:36:43Z

@jonnydgreen Thank you!!! 🥇

jonnydgreen · 2023-11-13T22:55:29Z

@jonnydgreen Thank you!!! 🥇

You're welcome and thank you to you too! If it's okay, I'll get this deployed this week as I'd like to include this with another change!

@mcollina I'm thinking of including this as part of v5 (semver major to remove Node.js v14+16 support and add Node.js v20 support), wdyt?

Bugs5382 · 2023-11-15T19:25:30Z

@jonnydgreen Ya. Not a problem.

I been coding with v20, and I found no issues. Version 14 is not on my internal software plan.

add node v20

Shane Froebel added 4 commits October 26, 2023 22:07

feat: added new file for test cases

4b047d9

feat: renamed file for unit testing

8de834c

basic tests are in there are designed correctly // replacement "output" is not yet in there

feat: updated typescript // option validation

a9442e0

feat: standard unit testing working

06561f6

initial unit tests pass // need to ensure that we are "replacing" only on String scalar types. we are going to actually return an error at this point

Shane Froebel added 4 commits October 30, 2023 21:01

test: validation tests

ba4a58c

make sure registration errors out correctly if not done correctly

feat: filtered/replaced on only non-optional String Scalar

3ae83fd

feat: null can work now

cf69cfb

docs: schema-replacement

b9a2c25

how to use, but also some examples

fix: fixed unit test

07eec7b

test: add more checks for Scalar types not allowed

48aa62f

Bugs5382 marked this pull request as ready for review November 1, 2023 10:32

docs: spelling and grammar

cd2d786

jonnydgreen reviewed Nov 6, 2023

View reviewed changes

index.js Outdated Show resolved Hide resolved

fix: Update index.js

d3c8f08

fixes unit testing Co-authored-by: Jonny Green <[email protected]>

feature: fixed unit test

85cb401

removed resolver filtering // fixed refresh test again // 100% code coverage!

jonnydgreen reviewed Nov 10, 2023

View reviewed changes

docs/schema-replacement.md Show resolved Hide resolved

test/result-filter-output.js Show resolved Hide resolved

test/result-filter-output.js Outdated Show resolved Hide resolved

jonnydgreen requested a review from mcollina November 10, 2023 09:12

Bugs5382 commented Nov 10, 2023

View reviewed changes

docs/schema-replacement.md Show resolved Hide resolved

fix: error check

7bcbc64

errors were still coming over for null values // unit tests updated and fixed code that does not send an error back

Shane Froebel added 2 commits November 10, 2023 06:58

fix: another validation check

2c41d91

if outputPolicyErrors.enabled was 'true,' then no errors would show on a failure other than schema ones if they excited after a replacement

docs: added full example

38c1140

Bugs5382 requested a review from jonnydgreen November 10, 2023 12:07

jonnydgreen approved these changes Nov 12, 2023

View reviewed changes

feat: drop v14 and v16 node

35cb45f

add node v20

jonnydgreen added the semver-major label Nov 18, 2023

jonnydgreen merged commit b02fd01 into mercurius-js:main Nov 18, 2023

Bugs5382 deleted the filtering-output branch November 19, 2023 03:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Filtering output #125

Filtering output #125

Bugs5382 commented Oct 29, 2023 •

edited

Loading

jonnydgreen commented Oct 30, 2023

Bugs5382 commented Oct 31, 2023

jonnydgreen commented Oct 31, 2023

Bugs5382 commented Nov 1, 2023

Bugs5382 commented Nov 1, 2023

jonnydgreen commented Nov 2, 2023 •

edited

Loading

jonnydgreen commented Nov 6, 2023 •

edited

Loading

Bugs5382 commented Nov 7, 2023

Bugs5382 commented Nov 7, 2023

Bugs5382 commented Nov 7, 2023

jonnydgreen commented Nov 7, 2023

jonnydgreen left a comment

Bugs5382 commented Nov 10, 2023

jonnydgreen left a comment

Bugs5382 commented Nov 13, 2023

jonnydgreen commented Nov 13, 2023

Bugs5382 commented Nov 15, 2023

Filtering output #125

Filtering output #125

Conversation

Bugs5382 commented Oct 29, 2023 • edited Loading

jonnydgreen commented Oct 30, 2023

Bugs5382 commented Oct 31, 2023

jonnydgreen commented Oct 31, 2023

Bugs5382 commented Nov 1, 2023

Bugs5382 commented Nov 1, 2023

jonnydgreen commented Nov 2, 2023 • edited Loading

jonnydgreen commented Nov 6, 2023 • edited Loading

Bugs5382 commented Nov 7, 2023

Bugs5382 commented Nov 7, 2023

Bugs5382 commented Nov 7, 2023

jonnydgreen commented Nov 7, 2023

jonnydgreen left a comment

Choose a reason for hiding this comment

Bugs5382 commented Nov 10, 2023

jonnydgreen left a comment

Choose a reason for hiding this comment

Bugs5382 commented Nov 13, 2023

jonnydgreen commented Nov 13, 2023

Bugs5382 commented Nov 15, 2023

Bugs5382 commented Oct 29, 2023 •

edited

Loading

jonnydgreen commented Nov 2, 2023 •

edited

Loading

jonnydgreen commented Nov 6, 2023 •

edited

Loading