nextstrain / nextstrain.org

The Nextstrain website
https://nextstrain.org
GNU Affero General Public License v3.0
87 stars 49 forks source link

authn: Stop request processing on failed Bearer authn attempts #756

Closed tsibley closed 7 months ago

tsibley commented 7 months ago

Fixes regression where after a failed Bearer authn attempt the request processing continued on (still unauthenticated). Failed attempts now once again produce an error response (typically 400 or 401) with the appropriate challenge (if any) in the WWW-Authenticate header. In particular, this restores responses like:

HTTP/1.1 401 Unauthorized
WWW-Authenticate: error="invalid_token" …

when a stale token is sent in the request. Nextstrain CLI relies on these responses to form a more helpful error message to the user.

The regression was introduced by "authn: Switch to custom authenticator callbacks" (aca4d07f) because the Passport "strategy" we use here stops performing the default behaviour we were relying on once a callback is provided. Instead, it expects the callback to provide all the behaviour.

Note that this regression was merely a usability issue, not a security issue, as the rest of the request processing remained unauthenticated.

Good tests of the interaction of Bearer authn with our "stale before" functionality would have caught this regression, but we continue to not have suitable testing scaffolding for tests involving authentication and so adding them (at the time or now) is not trivial.

Checklist

tsibley commented 7 months ago

Repushed to fix the commit message since I realized I omitted part of the WWW-Authenticate header in the example.

image