fastify / fastify-bearer-auth

A Fastify plugin to require bearer Authorization headers
Other
150 stars 28 forks source link

When using typescript with @fastify/auth, verifyBearerAuth throws a "Type 'verifyBearerAuth | undefined' is not assignable to type 'FastifyAuthFunction | FastifyAuthFunction[]'" #176

Open brunoshine opened 7 months ago

brunoshine commented 7 months ago

Prerequisites

Fastify version

4.25.2

Plugin version

9.3.0

Node.js version

20.9.0

Operating system

Linux

Operating system version (i.e. 20.04, 11.3, 10)

Ubuntu 22.04.2 LTS

Description

Picking up on the sample provided on the REAME for the integration with @fastify/auth, if we create a route with just the fastify.verifyBearerAuth we get a typescript error Type 'verifyBearerAuth | undefined' is not assignable to type 'FastifyAuthFunction | FastifyAuthFunction[]'. Type 'undefined' is not assignable to type 'FastifyAuthFunction | FastifyAuthFunction[]'.ts(2322)


    fastify.route({
        method: 'GET',
        url: '/bearer',
        preHandler: fastify.auth([fastify.verifyBearerAuth]),
        handler: function (_, reply) {
            reply.send({ hello: 'world' })
        }
    })

The workaround was to add a // @ts-ignore

 fastify.route({
        method: 'GET',
        url: '/bearer',
        // @ts-ignore <<<<<<<<<<<<<<< workaround
        preHandler: fastify.auth([fastify.verifyBearerAuth]),
        handler: function (_, reply) {
            reply.send({ hello: 'world' })
        }
    })

Any thoughts? thanks

Steps to Reproduce

Follow the README sample and add a route with just the verifybearerauth call:

    fastify.route({
        method: 'GET',
        url: '/bearer',
        preHandler: fastify.auth([fastify.verifyBearerAuth]),
        handler: function (_, reply) {
            reply.send({ hello: 'world' })
        }
    })

Expected Behavior

Should work without the error.

mcollina commented 7 months ago

Thanks for reporting! Would you like to send a Pull Request to address this issue? Remember to add unit tests (we use tsd for typescript)

casantosmu commented 7 months ago

verifyBearerAuth is declared as undefined here. This reflects the reality that Fastify cannot automatically determine the presence of specific decorators in each context.

To address this, you can manually check for verifyBearerAuth before defining the route:

if (!fastify.verifyBearerAuth) {
  throw new Error("Fastify verifyBearerAuth decorator required");
}

fastify.route({
  method: "GET",
  url: "/bearer",
  preHandler: fastify.auth([fastify.verifyBearerAuth]),
  handler: function (_, reply) {
    reply.send({ hello: "world" });
  },
});

However, many plugins in the Fastify ecosystem declare these decorators as defined. I could update the module declaration to remove the possibility of verifyBearerAuth being undefined, changing this:

declare module 'fastify' {
  interface FastifyInstance {
    verifyBearerAuthFactory?: fastifyBearerAuth.verifyBearerAuthFactory
    verifyBearerAuth?: fastifyBearerAuth.verifyBearerAuth
  }
}

To this:

declare module 'fastify' {
  interface FastifyInstance {
    verifyBearerAuthFactory: fastifyBearerAuth.verifyBearerAuthFactory
    verifyBearerAuth: fastifyBearerAuth.verifyBearerAuth
  }
}

@mcollina what do you think?

mcollina commented 7 months ago

I would do that.

climba03003 commented 7 months ago

It is marked as undefined because those methods is not added by default. I believe extra checking on the existence is a good idea.

If it removes the ? part, I believe there would be a bug report asking why those methods is undefined.

casantosmu commented 7 months ago

I believe it is preferable to catch errors during the build process rather than encountering them at runtime.

climba03003 commented 7 months ago

If you compare with other plugins, they exist in non-nullable methods only because they are always decorated.

I don't think the current types is wrong, change it is giving a false image to the users it is always there.

I don't want to be chasing it back and forth.

casantosmu commented 7 months ago

It seems there might be a confusion. I agree with you.