Confusing message when authenticator result type is invalid

[niallmccullagh]

Version Info:

• node: 10.8.0
• exegesis-express: 1.0.5
• exegesis-express: 1.0.0
• express: 4.16.3

Problem

I built a simple API to gain an understanding of your library. There is a single endpoint that is secured with the following scheme, which uses the sample authenticator that is detailed in OAS3 Security.md

  securitySchemes:
    sessionKey:
      type: apiKey
      in: header
      name: session

When the session header is missing from the request the expected message is returned with a 401 status.

curl -i -X GET 'http://localhost:3000/v1/greet?name=ted'
HTTP/1.1 401 Unauthorized
content-type: application/json
Date: Fri, 31 Aug 2018 09:47:16 GMT
Connection: keep-alive
Transfer-Encoding: chunked

{"message":"Must authenticate using one of the following schemes: sessionKey."}

However, if the session header is specified but it is invalid, the same message is returned.

curl -i -X GET 'http://localhost:3000/v1/greet?name=ted' -H 'session: secret1'
HTTP/1.1 401 Unauthorized
content-type: application/json
Date: Fri, 31 Aug 2018 09:48:42 GMT
Connection: keep-alive
Transfer-Encoding: chunked

{"message":"Must authenticate using one of the following schemes: sessionKey."}

My initial conclusion was that the library was not detecting the session header, so I went digging into the code. I got down to the authenticate method on in lib/oas3/Operation.js and realised that it does detect the header, but noticed that it doesn't return the result if the security type is invalid.

I would've expected that the message and status code that I defined and returned in the authenticator to be set in the response.

For multiple authenticators, I would expect the behaviour to the same as a successful authentication. That is, it would return the result of the first invalid authenticator encountered.

[jwalton]

This is confusing.

So, let's say you have an operation that has a security requirement like:

{
    "sessionKey": [],
    "basicAuth": []
}

Now, if someone authenticates with a good sessionKey, we're never going to even try to authenticate against basicAuth. If someone authenticates with a bad sessionKey, though, should we continue on to try basicAuth, or should we reject immediately? What if they have good basicAuth credentials and a bad sessionKey?

It's tempting to say we should reject immediately, but then, if you have a good basicAuth and a bad sessionKey and you get this security requirement:

{
    "basicAuth": [],
    "sessionKey": []
}

where basicAuth and sessionKey are reversed, then we'd accept this (since we'd pass the basicAuth, and never bother to check the sessionKey). It's kind of weird that we pass in the one case, and fail in the other, just because the order of security requirements is different.

Maybe what we should really be doing here is not short-circuiting on success - always run all the authenticators, and if any of them returns "invalid" we fail. The only real downside to that is that exegesis-passport usually doesn't know the difference between a missing result and an invalid result, but so long as it errors on the side of "missing" it's not any worse than what we have now.

[niallmccullagh]

I think the invalid credentials scenario is going to be more common with the scenario of multiple "credentials" being an edge case. The multiple credentials being passed would be also easier to spot from the request.

It's tempting to say we should reject immediately, but then, if you have a good basicAuth and a bad sessionKey ... It's kind of weird that we pass in the one case, and fail in the other, just because the order of security requirements is different

That a very good point. Could this flow be a possible solution?

For each authenticator

1. check the security requirement
2. collect in array if invalid
3. return on the first success
4. If there was no successful result but one or more invalid results then return an error listing the schemes(s) that were invalid {message: "Authentication was invalid for the the following schemes: sessionKey." errors: [{scheme: result[x].failedSchemeName, message: result[x].message, ... }] with the detailed messages of the invalid results }
5. If all are missing then return { "message":"Must authenticate using one of the following schemes: sessionKey." }
   Obviously setting the headers appropriately.

I think the consumer of the API would have a clear understanding what occurred. An update to the developer documentation would be needed to call out that the order that the authenticators are run in and that the process is short-circuited on a success.

Maybe what we should really be doing here is not short-circuiting on success - always run all the authenticators, and if any of them returns "invalid" we fail. The only real downside to that is that exegesis-passport usually doesn't know the difference between a missing result and an invalid result, but so long as it errors on the side of "missing" it's not any worse than what we have now.

Agreed. The passport flow would be no worse than the current flow, but things would be improved for other integrations.

[jwalton]

Yes, although I'd suggest if there's at least one set of invalid credentials, we should still fail, even if there's also some valid passing credentials. I'm working on something - hopefully I'll have a fix for this after the weekend.

[niallmccullagh]

Hi Jason,

Some interesting documentation in the specification under Using Multiple Authentication Types ...

Some REST APIs support several authentication types. The security section lets you combine the security requirements using logical OR and AND to achieve the desired result.

OR case

    security:    # A OR B
      - A
      - B

• Successfully authenticate if either A or B returns a success.
• A 'invalid' or 'missing' result in only one will be ignored.

AND case

    security:    # A AND B
      - A
        B

• Successfully authenticate only if both A or B returns a success.
• A 'invalid' or 'missing' result in either will cause authentication to fail.

It makes sense to follow the spec but I can see that it wouldn't be a small change. It would also have an impact on the authenticator results, as they would need merged together when there is multiple results.

[jwalton]

🎉 This issue has been resolved in version 1.0.7 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀