Transformer directive throws during test due to directiveArgs always undefined

[Philzen]

We've written a transformer directive to filter unauthorized records such as this:

export const schema = gql`
  directive @ownDataOnly(list: Boolean) on FIELD_DEFINITION
`

const transform: TransformerDirectiveFunc = ({
  context,
  resolvedValue,
  directiveArgs,
}) => {
  const { list } = directiveArgs

  if (list) {
    return resolvedValue.filter(
      (record) => record.userId === context.currentUser.id
    )
  }

  return resolvedValue.userId === context.currentUser.id ? resolvedValue : null
}

export default createTransformerDirective(schema, transform)

When running a simple test on it, such as:

  beforeEach(() => {
    mockCurrentUser({ id: 101, email: 'test101@test.com' })
  })

  it("returns null if it's not the user's own record", () => {
    const mockExecution = mockRedwoodDirective(ownDataOnly, {
      mockedResolvedValue: { userId: 666 },
    })

    expect(mockExecution()).toBe(null)
  })

The following error is thrown, causing this and basically any tests involving mockRedwoodDirective to fail:

TypeError: Cannot destructure property 'list' of 'directiveArgs' as it is undefined.

Notable

• tried supplying directiveArgs: { list: true } in mockRedwoodDirective's executionMock argument, but directiveArgsis still undefined. Not sure if that isn't a separate issue on its own or if i'm simply overlooking something.
• Interestingly enough the requireAuth.ts directive that ships with RedwoodJS also uses and destructures the directiveArgs-property – which comes into the validate-callback as an empty object, hence the "requireAuth has stub implementation. Should not throw when current user"-test passes fine.
• our transform directive works absolutely fine otherwise i.e. when running in dev-environment

[dac09]

Hi @Philzen - this is probably a valid issue, but both @dthyresson and I think this isn't an appropriate use of transformer directives.

It's worth remembering that transformer directives execute after running your resolvers. This means in theory, that even if the userId requesting is not authorized, the code in your service would be running (which is probably not what you want).

It might be better to check the userId in your service, and throw an ForbiddenError if the user isn't authorized.

---

All that said, we'll still look into why the test is failing.

[dthyresson]

In addition, having the logic in your service will mean that you can use the service from another service -- and also from a web hook or other function. So you get flexibility there.

I think the key here is that you get an informative error back.

But, points for creativity :) I sort of like the concept, but I think there is a safer and more reusable approach.

Also, see: https://redwoodjs.com/docs/directives#when-should-i-use-a-redwood-directive

[Philzen]

Hi @dac09, thanks for your immediate feedback!

both @dthyresson and I think this isn't an appropriate use of transformer directives. [...] even if the userId requesting is not authorized, the code in your service would be running [...]

Actually the implementation does also check for the currentUser in context and throws if not authenticated, i left out that line (and also our own comments making clear that this runs after any query retrieval 😉 ) for the sake of brevity.

It might be better to check the userId in your service, and throw an ForbiddenError if the user isn't authorized.

We're actually kinda doing that in the service, where prisma will throw on connected records issues. A ForbiddenError would be nicer – though in our case it's really just an exercise to secure the api side against mischief as those endpoints are only intended to be used by the web workspace.

Actually, we currently have three layers of authentication security – @requireAuth, services (validateWith), @ownDataOnly – and two layers of authorization security – services (prisma related records) and @ownDataOnly, which seems like overkill, but it's basically what we've learned from Tutorial Chapter 4 ... better safe than sorry, in case some auth validation is simply forgotten or goes south during a refactoring.

I'd welcome your thoughts and feedback on this strategy. Personally, i'm a performance nerd and would go for one check is enough, but after careful discussion and consideration with my collegue we chose to go with the multiple safety-layer implementation.

[dthyresson]

FYI, we noticed that directiveArgs were not called to the mockRedwoodDirective ...

see: #5848

... and you will want to mock the context

    const mockExecution = mockRedwoodDirective(ownDataOnly, {
      mockedResolvedValue: { userId: 666 },
      context: { currentUser: { userId: 1 } },
      directiveArgs: { list: true },
    })

and note that the currentUser type should match the type returned from src/lib/auth.ts#getCurrentUser

[dac09]

I'll give you my 2p @Philzen - but please don't take this as gospel - for me, having complicated logic such as auth located in one place is always an advantage, especially if you are a growing team/building out your product.

At some point in the future you may want to change parts of how your auth system works, and you'd have to refactor three different places, which is not ideal!

Auth is also one of those things that needs a lot of "context" - so having your unit tests colocated, and in one place (even if not the same file) is a massive time saver

[dthyresson]

Personally, i'm a performance nerd and would go for one check is enough, but after careful discussion and consideration with my collegue we chose to go with the multiple safety-layer implementation.

@Philzen My concern might be that the auth rules are now in three places and if the rules change, then the risk is higher because you do not have as much confidence in what the rule applied will be. Maybe the rule is changed in 2 places but then the third check is used and people access data they cannot.

[Philzen]

    const mockExecution = mockRedwoodDirective(ownDataOnly, {
      mockedResolvedValue: { userId: 666 },
      context: { currentUser: { userId: 1 } },
      directiveArgs: { list: true },
    })

@dthyresson as referenced in issue description context is initialized via beforeEach, and i already tried supplying directiveArgs: { list: true } in mockRedwoodDirective's executionMock argument to no avail.

[dthyresson]

directiveArgs: { list: true } in mockRedwoodDirective's executionMock argument to no avail

Right. That's the bug @dac09 and I found and the PR aims to correct. For some reason we didn't give directiveArgs to the Transformer when mocking -- an oversight that we've now corrected. Thanks for finding!

[Philzen]

Thanks @dac09 & @dthyresson for the quick fix (even though i found a implementation workaround w/o requiring an argument but this will surely be useful for the next person trying to accomplish this) and even more the fruitful discussion!

having complicated logic such as auth located in one place is always an advantage, especially if you are a growing team/building out your product. At some point in the future you may want to change parts of how your auth system works, and you'd have to refactor three different places, which is not ideal

I'm so very much with you here ... keeping things simple and all in one place, saves time & confusion on refactoring and increases performance & clarity. So doesn't tutorial chapter 4 give bad advice in that regards?

In our services we're actually re-using requireAuth() from lib/auth for the authentication validation. Maybe that would be the minimum improvement for that chapter to reduce repetitive auth-logic implementation.