Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Returning 401 status without WWW-Authenticate header #851

Open
alxndrsn opened this issue Apr 27, 2023 · 5 comments
Open

Returning 401 status without WWW-Authenticate header #851

alxndrsn opened this issue Apr 27, 2023 · 5 comments

Comments

@alxndrsn
Copy link
Contributor

Backend is returning HTTP status 401 in a few places, and I can't see any inclusion of a WWW-Authenticate header.

The server generating a 401 response MUST send a WWW-Authenticate header field
-- https://www.rfc-editor.org/rfc/rfc9110#status.401

@alxndrsn
Copy link
Contributor Author

Resolution

Simple

A simple option would be to include WWW-Authenticate: None with these responses.

More complex

The 3 different Problem might warrant different changes:

// no detail information for security reasons.
authenticationFailed: problem(401.2, () => 'Could not authenticate with the provided credentials.'),
httpsOnly: problem(401.3, () => 'This authentication method is only available over HTTPS'),
openRosaAuthenticationRequired: problem(401.4, () => 'This resource requires authentication.'),

  • Problem.authenticationFailed: used for both authentication and authorisation failures in various places. It could be renamed authFailure, and return 403.
  • Problem.httpsOnly: returned when basic auth is used on a non-https connection. Returning 400, 403, or 302 might be appropriate.
  • Problem.openRosaAuthenticationRequired: the code looks like it should check if the authenticated user has the app_user role, and if not return an authorisation failure:
    .then(rejectIf((() => auth.actor.isEmpty()), noargs(Problem.user.openRosaAuthenticationRequired)))

@matthew-white
Copy link
Member

matthew-white commented Apr 28, 2023

Problem.authenticationFailed: used for both authentication and authorisation failures in various places.

Where do you see it used for authorization? I see one instance here:

const forceAuthFailed = Problem.translate(Problem.user.insufficientRights, Problem.user.authenticationFailed);

I don't quite remember what's going on here. I think Collect (or maybe Enketo?) displays an informative error message for a 401 but not a 403. @lognaturel would know more! It looks like this was introduced in 7b13eb7.

Problem.httpsOnly: returned when basic auth is used on a non-https connection. Returning 400, 403, or 302 might be appropriate.

Just my two cents, I vote for a 400!

Problem.openRosaAuthenticationRequired: the code looks like it should check if the authenticated user has the app_user role, and if not return an authorisation failure:

I don't think so, because web users and public links can also access the OpenRosa formList. I'm not 100% sure, but I think it's just checking that there was any authentication. It looks like this was introduced in 5a748e7. This endpoint is unusual in that it doesn't call auth.canOrReject(). Instead, it returns the list of forms that the actor can form.read using Forms.getByAuthForOpenRosa(). It's similar to the project list endpoint in that way (except that the project list endpoint doesn't require authentication at all).

Side note: I'm a little confused about why the endpoint is checking auth.actor.isEmpty() rather than auth.isAuthenticated. I think it's possibly just historical: maybe isAuthenticated didn't exist until #331? (UPDATE: Nope, looks like isAuthenticated existed as early as 471782b. Maybe it's because Forms.getByAuthForOpenRosa() uses auth.actor.)

@lognaturel
Copy link
Member

I think Collect (or maybe Enketo?) displays an informative error message for a 401 but not a 403.

Both only prompt for auth if they get a 401. That's from the OpenRosa spec.

@alxndrsn
Copy link
Contributor Author

Both only prompt for auth if they get a 401. That's from the OpenRosa spec.

Interesting! Per that link:

The server responds with the 401 response code, either requesting Digest Authentication and providing the authentication realm and a randomly-generated, single-use value called a nonce, or requesting Basic Authentication...

So maybe WWW-Authenticate should be sent with the appropriate auth scheme?

...(in which case the server should also redirect and negotiate TLS channel security (https) if the client is not already communicating over https).

This sounds like it refers to the case that @matthew-white quotes above:

const forceAuthFailed = Problem.translate(Problem.user.insufficientRights, Problem.user.authenticationFailed);

Should this read that the server either returns 401 or "redirect[s] and negotiate[s] TLS channel security (https)"? I'm not sure it can do both in one response.

@matthew-white
Copy link
Member

matthew-white commented Apr 28, 2023

I just noticed that the API docs about Basic auth say something about WWW-Authenticate:

Unlike the standard, we do not require the client to first send an unauthenticated request and retry the request only after receiving a WWW-Authenticate response, and in fact we will never send the WWW-Authenticate header. This is mostly because, as noted above, we generally discourage the use of this authentication method, and would rather not advertise its use openly. As a result, if you wish to use Basic Authentication, directly supply the header on any request that needs it.

Specifying WWW-Authenticate: None or not listing Basic in WWW-Authenticate seems like it'd be consistent with the idea of not advertising Central's support for Basic auth. As long as that works with clients like Power BI that do use Basic auth.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants