Hint from authorizer which error response to return to the user

[fantapop]

Our authorizer supports 3 different types of auth. In the case of all auth failing, the promise to resolve last is the one that is returned to the user. For the express route that happens here:

tsoa/src/routeGeneration/templates/express.hbs

Line 112 in 0df645d

if (responded == security.length && !success) {

This is usually the correct error response. I think the one that is actually being attempted will likely get further in the authentication check than auth methods that can be immediately pruned out, However I've seen the wrong error being shown to our users on a few occasions and its very confusing.

Sorting

• I'm submitting a ...

  • bug report
  • feature request
  • support request

• I confirm that I

  • used the search to make sure that a similar issue hasn't already been submit

Expected Behavior

The error related to the intended authentication method will be returned to the user.

Current Behavior

The auth promise that takes the longest to resolve is returned to the user.

Possible Solution

My idea is to allow the authorizer to reject the auth method with some additional information which indicates that it was not attempted by the user. For example if an api supports api key as well as bearer token and a request is made in which the x-api-key header was not passed the authorizer could reject with:

{
    error: new Error("X-Api-Key Header was missing"),
    priority: 2,
}

and

{
    error: new Error("Bearer Token was expired"),
    priority: 1,
}

Context (Environment)

Version of the library: 3.0.4
Version of NodeJS: 12

• Confirm you were using yarn not npm: [x]

Detailed Description

Breaking change?

This could be made in a way that supports the old way and the new way

[WoH]

Can you provide some additional info, so we can better understand your use case.
Are you using one Security Decorator here or multiple?
Maybe it's a good idea to have a combined security decorator (if you are using multiple) so you have full control what to return from that one method?

[fantapop]

We have 3 security decorators currently. 2 types of oauth implementations and 1 api key decorator. Our web app has access to a different but overlapping set of methods as our app. I’ve also written an endpoint to create a custom swagger spec which is filtered by security type so that we can generate clients with the subset of methods which are exposed to the particular client. Currently that mechanism is relying on the security type. I suppose it would be possible to twist scopes into doing what we want here. I do think that the current mechanism is a little flawed in how it works with multiple failures and multiple successes.

…

On Sat, Apr 18, 2020 at 1:25 PM Wolfgang Hobmaier ***@***.***> wrote: Can you provide some additional info, so we can better understand your use case. Are you using one Security Decorator here or multiple? Maybe it's a good idea to have a combined security decorator (if you are using multiple) so you have full control what to return from that one method? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#659 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABSZXKGDJW75JDFANLZZNDTRNHPBLANCNFSM4MGHFMPQ> .

[github-actions]

This issue is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days

[fantapop]

This one is still biting us every once and a while. I could come up with a backwards compatible POC if you think this would have a possibility of getting merged @WoH

[10ko]

I was looking around to check if someone has solved this problem and stumbled upon this issue.
@fantapop How did you solve it at the end?

[fantapop]

I never did solve for this. We occasionally send the wrong error response back to our users. Do you think the solution I suggested here would work for you? I think it would be fairly easy to implement.

…

On Thu, Sep 17, 2020 at 3:55 AM Emanuele Libralato ***@***.***> wrote: I was looking around to check if someone has solved this problem and stumbled upon this issue. @fantapop <https://github.com/fantapop> How did you solve it at the end? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#659 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABSZXKB47ILWSYOXPGD7BUTSGHTJNANCNFSM4MGHFMPQ> .

[fantapop]

assuming #995 gets merged in, I think something here could be pretty straightforward. One idea is to check for a field on an error object which dictates the priority of whether it is the correct error. Tsoa users would then opt-in by having errors formatted with this additional field. Something like this could work:

const findHighestPriorityError = (failedAttempts) => {
    let highestPriError;
    for (const current of failedAttempts) {
         if (current.priority && (!highestPriError || highestPriError.priority < current.priority)) {
             highestPriError = current;
          }
     }
      return highestPriError;
 };

 try {
    request['user'] = await promiseAny(secMethodOrPromises);
    next();
} 
catch(err) {
    const error = findHighestPriorityError(failedAttempts) || failedAttempts.pop();
    error.status = error.status || 401;
    next(error);
}