[QUESTION] Override passport-local's "Bad Request"

[ghost]

Hi, is it possible to overwrite passport-local's default response when the authentication parameters are not present? Because, as of now, it only reponds with "Bad Request" which isn't enough for the client, I would rather have something like "Missing credentials" and add 403 Missing credentials (or smth) for routes that are protected.

In you example there is an missmatch with the described unit test, it expects 403 but clearly you knew it wasn't going to return 403 and wrote 400 instead:

https://github.com/TypedProject/tsed-example-passportjs/blob/d9214788c1dc43a651a5e609302a6c0babe6a4f4/test/integration/controllers/auth/auth.spec.ts#L18-L25

Anyway, some people pointed out that it was due to the hardcoded message in the passport-local itself and/or missing body-parser dependency (which my project doesn't)

[Romakita]

Ha yes when a written the unit test for this example, I expected a 403 instead of 400.
Maybe the solution could be writting a middleware to validate the payload and use it before the Auth.

  @Post("/login")
  @ValidateCredential()
  @Authenticate("login", {failWithError: false})
  @Returns(User)
  @Responses(400, {description: "Validation error"})
  login(@Req() req: Req, @BodyParams() credentials: Credentials) {
    // FACADE
    return req.user;
  }

Decorator and middleware:

function ValidateCredential() {
  return applyDecorators(
     UseBefore(ValidateCredentialMiddleware),
    Responses(403, {description: "Missing credential"})
  )
}

@Middleware()
class ValidateCredentalMiddleware() {
   use(@BodyParams() credentials: any ) {
      if (!credentials.email && !credentials.password){
         throw new Forbidden("missing credentials")
      } 
   }
} 

Seconds solution is to implement create a class inherited from Passport local statregy:

const {lookup} = require('passport-local/lib/utils')
export class CustomLocalStrategy extends Strategy {
   authenticate(req: any, options: any) {
     options = options || {};
     const username = lookup(req.body, this._usernameField) || lookup(req.query, this._usernameField);
     const password = lookup(req.body, this._passwordField) || lookup(req.query, this._passwordField);
  
     if (!username || !password) {
       return this.fail({ message: options.badRequestMessage || 'Missing credentials' }, 403);
     }
     return super.authenticate(req, options)
  }
}

Then, add this custom strategy to the LoginLocalProtocol configuration :)

I hope theses solutions will help you :)

Romain

[ghost]

Hello @Romakita , thanks for the response, I ended up with this:

import { applyDecorators } from "@tsed/core";
import { UseBefore, Middleware, Req} from "@tsed/common";
import { Responses } from "@tsed/swagger";
import { Forbidden } from "ts-httpexceptions";

export function ValidateCredential() {
    return applyDecorators(
        UseBefore(ValidateCredentialMiddleware),
        Responses(403, { description: "Missing credentials." })
    )
}

@Middleware()
export class ValidateCredentialMiddleware {
    // `req.user` is set and unset by /login and /logout respectively
    use(@Req() req: Req) {
        if (!req.user) {
            throw new Forbidden("Missing credentials.");
        }
    }
} 



// contollers/UserControlller.ts
  @Get("/me")
  @ValidateCredential()
  // @Authorize()
  async me(): Promise<User> {
   //...
  }

For routes that need authentication. For /login I left the same with @Authenticate("login"), for me, it makes more sense to have 400 there, as the route itself expects these credentials in the body.

About the protected routes: it is a shame, I thought that @Authorize() decorator was supposed to take care of this (probably does, but doesn't return 403), are there any plans to extend on these settings in @tsed/passport? I'm using this custom decorator instead of @Authorize()

[Romakita]

For me the Authorize decorator should protect your route.

Here is the implementation of the middleware:

@Middleware()
export class PassportMiddleware {
  @Inject(ProtocolsService)
  protocolsService: ProtocolsService;

  use(@Req() request: Req, @EndpointInfo() endpoint: EndpointInfo) {
    if (request.user && request.isAuthenticated()) {
      return;
    }

    const {options, protocol, method} = endpoint.store.get(PassportMiddleware);
    const protocols = getProtocolsFromRequest(request, protocol, this.protocolsService.getProtocolsNames());

    if (protocols.length === 0) {
      throw new Unauthorized("Not authorized");
    }

    // @ts-ignore
    return Passport[method](protocols.length === 1 ? protocols[0] : protocols, options);
  }
}

The middleware is automatically added by @Authorize passport. This is the passportjs responsability to throw an error about missing credentials (I hope). But maybe is missed something?

[ghost]

Yeah, using @Authorize() throws "400 Bad Request":

It is a simple setup like these examples:
https://tsed.io/tutorials/passport.html#create-the-passport-controller
https://tsed.io/tutorials/passport.html#protect-a-route

From my understanding:

    if (protocols.length === 0) {
      throw new Unauthorized("Not authorized");
    }

should throw "401 Unauthorized", for I have configured the passport protocol, instead of 400. It's either a bug or has something to do with this:

https://github.com/jaredhanson/passport-local/blob/96a4727243588a6f835efefd9e2511c0da1060e6/lib/strategy.js#L74-L76

Side note: the correct response is 401 Unauthorized not 403 Forbidden 😅

[Romakita]

Hello @RickStanley
I'd just come back from my holiday and I lost the thread. Should I do something?

[ghost]

@Romakita

tl;td
Would you mind checking if @tsed/passport, along with passport-local are being setup correctly? I don't think that it should throw 400 Bad Request if credentials are missing, but rather 400 Missing Credentials (found in the passport's strategy core code).

If it's too much of a hassle, you can close this issue, as the custom solution fits my need.

---

My initial suggestion was that maybe either body-parser or passport-local weren't beign handled properly, thus causing the 400 Bad Request (unhelpful message), whereas 400|*401*|403 Missing Credentials would be a better fit¹.

But after some reading through passportjs' issues, I found that many people are trying to overwrite the "Bad Request", as the message is not helpful at all as is, which of course you've alreay provided a solution for this, and I've tweaked to fit my need.

Finally, I was going to close this, but since you asked if there was something else to do, I went to look for some similar issues. I found this, which leads to my initial thought that maybe @tsed/passport or using body-parser causes some missconfigurations, because as of now, using @Authorize() or @Authenticate() (specifying the protocol's name or not) throws 400 Bad Request, even with passport strategy setup to throw 401 Missing Credentias (I made the change in my app, I'm just linking the example for reference), in other words, bypassing my protocol setting.

But then again, this current solution is enough, though I really would like to use @tsed/passport & passport-local to keep thing consistent.

In any case, I think this was a good issue, I've seen this kind question in a lot of places like NestJS and FoalTS. I hope someone find this helpful.

---

Refs:
How to overwrite the "Bad Request" error message

¹: https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/401