Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: extend composite auth #217

Merged
merged 1 commit into from
Feb 2, 2024

Conversation

yakovenkodenis
Copy link
Contributor

@yakovenkodenis yakovenkodenis commented Jan 19, 2024

This PR addresses Issue #216: Extending composite authentication to allow nesting auth arrays with both and and or relations.

If the relation (defaultRelation) parameter is and, then the relation inside sub-arrays will be or.
If the relation (defaultRelation) parameter is or, then the relation inside sub-arrays will be and.

auth code resulting logical expression
fastify.auth([f1, f2, [f3, f4]], { relation: 'or' }) f1 OR f2 OR (f3 AND f4)
fastify.auth([f1, f2, [f3, f4]], { relation: 'and' }) f1 AND f2 AND (f3 OR f4)

Checklist

@yakovenkodenis yakovenkodenis force-pushed the feat/composite-auth branch 2 times, most recently from 5938ff8 to a8d16a4 Compare January 20, 2024 12:58
@yakovenkodenis yakovenkodenis marked this pull request as ready for review January 20, 2024 14:55
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@mcollina mcollina requested a review from simoneb January 20, 2024 19:40
@mcollina mcollina merged commit 3c895a2 into fastify:master Feb 2, 2024
19 checks passed
@yakovenkodenis yakovenkodenis deleted the feat/composite-auth branch February 2, 2024 20:36
@isnifer
Copy link

isnifer commented Feb 7, 2024

So, this PR breaks my preHandler:

// I expect [[f1 AND f2 AND Fn] OR [F3 AND F4 AND Fn]], { relation: 'or' }
fastify.auth(
  [
    [fastify.checkAuth('admins'), ...someOtherAdminChecks],
    [fastify.checkAuth('clients'), ...someOtherClientChecks],
  ],
  { relation: 'or' }
)

It runs both paths even first path is passing. I'll try to find the issue in the changes.

method: 'POST',
url: '/check-two-sub-arrays-or',
payload: {
n: 11
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I updated 11 to 110 and now verifyBigAsync is passing BUT verifyOddAsync still called and doesn't wait until verifyBigAsync finished

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mcollina my thoughts on this — due to broken default behavior it should be 5.0.0, not 4.5.0

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@isnifer Could you provide a test case for this?

I've just tried the following case locally:

  • a route:
fastify.route({
  method: 'POST',
  url: '/check-two-sub-arrays-or-2',
  preHandler: fastify.auth([[fastify.verifyBigAsync], [fastify.verifyOddAsync]], { relation: 'or' }),
  handler: (req, reply) => {
    reply.send({
      big: req.big,
      odd: req.odd ?? 'hello',
    })
  }
})
  • a test:
test('Two sub-arrays Or Relation success', t => {
  t.plan(2)

  fastify.inject({
    method: 'POST',
    url: '/check-two-sub-arrays-or-2',
    payload: {
      n: 110
    }
  }, (err, res) => {
    t.error(err)
    const payload = JSON.parse(res.payload)

    t.same(payload, {
      big: true,
      odd: 'hello',
    })
  })
})

And the test is passing meaning the fastify.verifyOddAsync was not called.

Copy link

@isnifer isnifer Feb 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yakovenkodenis yes, sure, I forked this repo https://github.com/isnifer/fastify-auth.

  1. I hide all routes except this one — https://github.com/isnifer/fastify-auth/blob/master/test/example-composited.js#L148-L159
  2. Added important console.log()s here — https://github.com/isnifer/fastify-auth/blob/master/test/example-composited.js#L52-L73
  3. And hide all tests except this one — https://github.com/isnifer/fastify-auth/blob/master/test/example-composited.test.js#L473-L490

And my test logs are:

npm t

> @fastify/auth@4.5.0 test
> npm run test:unit && npm run test:typescript


> @fastify/auth@4.5.0 test:unit
> tap

 PASS ​ ./test/auth.test.js 29 OK 38.833ms
 PASS ​ ./test/example-async.test.js 18 OK 48.952ms
./test/example-composited.test.js 1> If you see it — IT IS OK
./test/example-composited.test.js 1> THIS IS A SECOND AUTH PATH SHOULD NOT BE CALLED
 PASS ​ ./test/example-composited.test.js 2 OK 17.618ms
 PASS ​ ./test/example.test.js 20 OK 50.974ms

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yakovenkodenis also important note — I've changed line https://github.com/isnifer/fastify-auth/blob/master/test/example-composited.test.js#L481 to make verifyBigAsync pass since it's a first auth path

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could @isnifer open a dedicated issue with your findings?

It is hard to discuss a bug in a merged PR

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Eomm no problem, I'll do it

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Eomm I've created #223

isnifer added a commit to isnifer/fastify-auth that referenced this pull request Feb 8, 2024
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.

4 participants