jwt: do not use json_request? to infer response type

[HoneyryderChuck]

As discussed in the google group, and having this example, a lot of actions in jwt mode aren't consistent because one is using the Content-Type header of the request to define if the response is going to be a json response.

As by the links presented in the google group thread, the Accept header should be used to infer if the response is going to be json (unless rodauth is marked as json only, by which all answers will be json).

This is the reason why, json-only mode:

• sending a login request will try to access flash in case of error;
• Content-Type of the response will not be json in 405 case;
• Any Type of request using the JWT tries again to access flash without the Content-Type header

My proposal:

• Drop the json_request? method, and replace it with json_response?. This method should check if one of these conditions is set:
  • rodauth is json-only
  • the response has Content-Type set to application/json
  • the request has Accept header with application/json

[jeremyevans]

When the jwt feature is used, Rodauth has two modes: the JSON API (using JWT for authentication data) and regular mode (using Rack sessions for authentication data). If the :only=>:json plugin option is used, only the JSON API mode is allowed.

The regular mode does not support JSON requests, and the JSON API does not support non-JSON requests. The regular mode always uses text/html responses or redirects, the JSON API always returns JSON responses. So if the request type is JSON and the request is to a Rodauth endpoint, the response is always going to be JSON. Likewise, if you send a non-JSON request to a Rodauth endpoint, the response will never be JSON. If you are sending a JSON request to a Rodauth endpoint and expecting a non-JSON response, you are out of luck, Rodauth does not and will not support that.

The Accept request header should only be considered if the endpoint can return multiple possible response types. If the endpoint can only return a single response type, it doesn't really matter what the Accept header is. You could argue that we should return a 406 error in cases where the only response type supported is not included in the Accept header. I don't really like that idea, but I'm open to supporting it as an optional feature.

[HoneyryderChuck]

... if the :only=>:json plugin option is used, only the JSON API mode is allowed. ...

That matches the first condition I suggested.

... So if the request type is JSON and the request is to a Rodauth endpoint, the response is always going to be JSON ...

Nothing in the HTTP spec forbids sending a POST request with JSON payload body and getting html in return. Same thing with encoded form data in request and getting JSON in return. If the framework assumes that, it is not being http-compliant. From my user perspective, I obviously wouldn't want that, so I'm not suggesting building support for it.

... You could argue that we should return a 406 error ...

Yes. But this goes partially beyond the scope of my suggestion. My main concern here is correctness from the jwt feature. Semantically speaking, and regarding the link from above, you should only call super if the response is going to be json (not request). So, at least the method name should change (it's private API, so no discussion about back-compat). Beyond that, the conditions could be updated.

• Obviously, json-only mode will have precedence.
• The second condition is, I think, beyond discussion.
• The 3rd is the tricky one. To be fully correct, one would have to, as you suggested, get all Accept media types, and then go one by one right-to-left until we find one that is supported and return it, or 406 it. This might be overkill, specially if you think that most client side frameworks set the Accept to application/json or application/vnd.api+json, or optionally don't set any media type, and this is usually the job of frontend servers. So maybe it's the implementation cost we are talking about. Rails supports this, so one could investigate if it'd be feasible, but this would have to be probably implemented in Roda. So, for the purpose of this specific rodauth feature, checking wether json is there would probably be sufficient: here's a draft:

def json_response?
  return @json_response if defined?(@json_response)
  @json_response = scope.class.opts[:json] == :only || 
                                 response.content_type =~ /application\/(vnd\.api+)?json/i ||
                                 request.accept =~ /application\/(vnd\.api+)?json/i
end

[jeremyevans]

While the HTTP spec does not forbid POST requests with JSON content-type and non-JSON response type, that does not mean that all endpoints that accept JSON formatted requests must be able to return non-JSON responses. Like I said, Rodauth only supports JSON responses for JSON requests, and does not support JSON responses for non-JSON requests. I think you are correct that strictly speaking Rodauth is violating the HTTP spec by returning a result instead of an error.

I guess I'm OK with having an option that returns a 406 error if the Accept header doesn't include application/json. I checked and GitHub's API returns a 415 error if you use an Accept header that doesn't include application/json, but I think 406 is the more correct error. I think this option would have to be false by default for backwards compatibility, but could change to true by default at the next major version. I'll try to implement that soon.

[jeremyevans]

I think these issues are fixed by 2101cfc1b50ba60956feb77b2512ee1010585419. If you still think there are problems, please let me know so we can discuss and possibly reopen.

[HoneyryderChuck]

I left a comment here, I think that request Content-Typeis still too strictly handled, specially in the GET requests case. Most of the issues have been handled though, thx for that!

[jeremyevans]

I've added 8877a4ab2d49b325de80d544507f5aae8d674040 to hopefully address your concerns. Please let me know if you think further changes should be made. I'm planning to release 1.5.0 this week, but I would like your feedback first.

[HoneyryderChuck]

So, here is are the latest updates:

• GET request to a non-rodauth endpoint without an Authorization header is now asking me to login and returns Content-Type: application/json, but gives me a 400 Bad Request. Shouldn't this be a 401? I think that rodauth could unilaterally set this status to 401 and ignore the settable error status code, because redirect with error is called when user can't access the resource (correct me if I'm wrong).
• Handling of the Accept header might be too strict again. Check this trace from curl:

*   Trying ::1...
* Connected to localhost (::1) port 9292 (#0)
> POST /api/login HTTP/1.1
> Host: localhost:9292
> User-Agent: curl/7.43.0
> Accept: */*
> Content-Type: application/json
> 
< HTTP/1.1 406 Not Acceptable
< Content-Type: application/json
< Content-Length: 71
< 
* Connection #0 to host localhost left intact
{"error":"Unsupported Accept header. Must accept \"application/json\""}

Bottom line, */* should also pass. Also, interestingly enough, the accept header is not filtered by endpoints outside of rodauth's scope. Is this by design?

• Sending a request to the login endpoint with not parameters returns 400. I'd say that the most correct would be 422, as 400 means "client formed an unvalid request/body" (ex: content-type x-form-encoded, json body). But this is probably that option. I'd also argue, as in the case of 401, that the error types can vary and shouldn't be a user option, rather a rodauth decision.
• This check here might be too brittle. Consider that the key could be set by the server. If you look at how github handles it, they use token: hash.keyin the Authorization header. I think that Bearer should/could be the default from a settable variable, as users might want to change.
• This might not be enough. Consider that the JSON API spec defines a more verbose content-type. Also consider that this header is used to fetch the API version to use (I don't know if roda has a plugin which handles that, but I know that grape can handle this). I quick way to handle this could be (and also referring to my report above) replacing the regex with /\*\/\*|application\/.*json/i.

From my latest comment on the commit: I think this here could be simplified to: ruby def use_jwt? ; not jwt_token.nil? ; end, so one does not mix the authentication strategy with the content types. This might break more things internally as jwt strategy is tightly coupled to json content types, but could also be considered a different issue altogether, and if you prefer not to handled this now or at all, that's fine.

[jeremyevans]

I don't think Rodauth should use a 401 header, unless you can think of a suitable response WWW-Authenticate response header that it could use. Also, using 401 instead of the current default of 400 would break backwards compatibility, and thus the default could not change until the next major version.

I agree that the Accept handling is not a perfect match. The regexp you suggest is also not perfectly valid, as application/* wouldn't be handled correctly (false negative), and there would be many false positives. The regexp is already user configurable, so users can configure it if they need to change it, but I agree that we could modify the default. maybe: /\b(?:(?:\*|application)\/\*|application/(?:vnd\.api\+)?json)\b/?

400 does not necessarily mean "client formed an invalid request/body", it is a generic "Bad Request". The error code is already user-configurable, and I disagree that rodauth should make it not-user configurable. If you want to use seperate error types for different types of errors, you can look at json_response[json_response_error_key] and choose which error code to use based on that.

I've already stated that making the Bearer removal from the Authorization header user configurable is fine with me, so I'll go ahead and make that change.

I'm fine splitting the Accept regexp from Content-Type regexp, and using /\bapplication/(?:vnd\.api\+)?json\b/ for the Content-Type regexp.

As mentioned in the commit comments, there has to be some way to specify using an empty session instead of the rack session if a JWT is not provided in the authorization header and :json=>:only is not used, otherwise I think the login API would break. Basing it on the request content-type is not perfect, but I think it is a reasonable default, and users can override it if they need to.

[jeremyevans]

I've added eb8448b1217c5ad6acb886d4fbd67ff37696ade5 to change the default accept/content-type regexps, as well as make the Authorization header handling user configurable. Please review and test if you have time, and let me know if you think additional changes should be made before the release of 1.5.0.

[HoneyryderChuck]

I don't think Rodauth should use a 401 header, unless you can think of a suitable response WWW-Authenticate response header that it could use.

I have to admit, I am at a conundrum here. I took 3 API examples (github, slack and heroku). All except slack respond with 401 Unauthorized with no WWW-Authenticate (slack always responds 200 OK, so let's ignore that). According to the rfc none is HTTP/1.1-compliant. One option could be to do it minimal. First step would be to set a default authentication scheme (ex. "Bearer"; see below), and then:

WWW-Authenticate: %{scheme}

According to the rfc, although the header in mandatory for 401, the scheme is the only mandatory parameter, all others ("realm","scope", etc...) are optional. Rodauth would only need to make this a configurable value. This might be a big change, it will not be backwards-compatible, but I don't also know a good way with the current API to set 401 as error code AND add a response header on authentication failed (maybe there is a callback for this?)

400 does not necessarily mean "client formed an invalid request/body", it is a generic "Bad Request".

I still think that 400 means something else semantically and 401 is the correct one for when access to resource is authentication-protected, but I understand the backwards-compatibility argument and lack of a sane WWW-Authenticate header. I will check more about this though. I don't feel so strongly about the 422 code for when the authentication login fails (github and heroku both seem to keep 401 for this case).

I've already stated that making the Bearer removal from the Authorization header user configurable is fine with me, so I'll go ahead and make that change.

I've tested, seems to work fine. The response after login responds with Authorization: %{hash}, as currently there is no way to set a custom authentication scheme (Authorization: %{scheme} %{hash}). This could have a direct influence in the WWW-Authenticate comment above. Now, what happens is that, if I pass an undecodable hash in the Authorization header, the app blows up with 500, and backtrace has JWT::DecodeError: Invalid segment encoding. Shouldn't rodauth rescue decoding exceptions and respond just as if it is unauthorized?

As mentioned in the commit comments, ...

I thought that this could be set like: if jwt auth possible (AUTHORIZATION header and non Basic/Digest) stick with it, otherwise if session available and rodauth session available, proceed. I haven't checked the implementation difficulty, but I agree that this is customizable and probably out of scope for this particular issue, so let's re-discuss this again if one comes up with the need.

[jeremyevans]

I agree with you that semantically, 401 makes more sense than 400 if the request is not authorized. I suppose I would consider an option that tries to parse some common error messages and sets 401 or 422, as long as the option was off by default. I'm also open to an option to prefix the response Authorization header, again, as long as it was off by default.

I'll add a commit that rescues JWT::DecodeError and returns an error response in that case.

Thank you again for your detailed feedback.

[jeremyevans]

I've added 953268770d236bed282b4395f268352ef0fe0e3c to rescue the decode errors. Can you please review/test and let me know if you think further changes should be made before the 1.5.0 release?

[HoneyryderChuck]

The decoding error response works as expected. Thx for the patch!

Just to expand on the topic of error codes for different scenarios, and taking the last patch as an example: I think that an invalid undecodable token should be interpreted as a 400 bad request; 401 should be issued when accessing resources protected by authentication; 422 (or 401, a la github) should be issued when the /login endpoint receives wrong/invalid/insufficient credentials.

To expand on the topic "do not mix authentication strategy with request/response content type", I'll share a link of a recent change that gitlab has done in the API source code to address this problem and with reasoning on why this could be important to the client.

Both issues are breaking changes, if taken into consideration, as so can only be discussed in a rodauth 2.0 scope. I don't know where to place requests for those (I assume you don't want github issues for those), so if you prefer to move this discussion somewhere else, I'll be open to that.

Concerning this issue, I'd consider this closed. Looking forward to 1.5.0!

[jeremyevans]

Thank you for the feedback. I'll be releasing 1.5.0 with the current master code today or tomorrow, but we can still work on improving the other areas in future versions.

For separate status codes per error, one approach is to have all existing features that want to indicate non-400 error codes set something, and have the jwt feature check for it and use it to determine which error code to set (assuming the option is enabled, it would be false by default). This removes the need to parse strings to determine error types. The user would still have full control as they do now if they want to override the error codes used. I think that change can make 1.6.0.

For not mixing authentication strategy with request/response content types, you will probably have to provide a pull request that shows the behavior you want. I understand what the issue is, but as I've discussed previously, there has to be some way to specify that the rack session should not be used even if a JWT is not provided, otherwise login and similar features will not work correctly in JSON API mode. Falling back to the request content type is the best default I can think of currently to do that, though it does have issues. So far, I haven't seen a proposal that will work, but I'm open to supporting such a thing via an option.

[janko]

@jeremyevans @TiagoCardoso1983 Thank you for all your work on making the jwt feature even better! ❤️

[jeremyevans]

I just pushed 0986225044d4c6180a2dd0fe2d9a8ab24f009686 which enables custom 4xx response status codes. Sorry this didn't make 1.6.0 or 1.7.0. @TiagoCardoso1983 if you could please test and provide feedback, I would appreciate it. I'd like to release 1.8.0 next week with these changes.

[HoneyryderChuck]

@jeremyevans it looks better, at least on a superficial overview. Unfortunately I'll only have time to see it end of next week, I'll come back to you then with feedback.have a great start to 2017!

[HoneyryderChuck]

@jeremyevans I've just had a look at the custom codes, and this is a short summary of my analysis:

• For login, I've seen you have taken the approach of 401'ing when POSTing account exists and password is wrong. I'd argue that this usually happens in a POST action to retrieve a valid token/session, so not exactly accessing an authorized area, so the status code should/could be 422 (invalid parameters). In fact, I'd argue that failing to provide a proper user/password pair should "always" (most of cases) be 422, as this won't leak account information (I've just seen that the github oauth endpoint gives 404 when account doesn't exist, I wouldn't recommend this approach either for no_matching_login_error_status). These are my suggestions for defaults:
  • invalid_password_error_status: 422
  • invalid_key_error_status: 422 (key not from account, invalid parameter)
  • no_matching_login_error_status: 404 or 422 (depending of whether you agree that 404 leaks info)
  • login_error_status: 422 (used in POST, not accessing authorized area)
• unopen_account_error_status, lockout_error_status, seem ok as 403, although the account info leak argument stands.
• I can't comment too much on sms_codes and two_factor_base, as I don't use them (yet).
• I don't have a strong opinion on the browser-related changes, as I mostly use rodauth for API authentication, so I tend to agree with you.
• Another argument against 401 would be that the WWW-Authenticate header isn't defined anywhere (a source of discussion when I PR'ed the http_basic_auth). But then again, this comment is only valid when requiring login in an "authorized area" (which is what plain json_response_error_status does already).

I'm only referring to what should be sane defaults, I'm aware this is all customizable if we don't agree. Also, I've read some of my previous comments, and the one stating that I don't feel strongly about 401/422 for invalid parameters hasn't been reflected in my points, but just because I think that the defaults require that you feel strongly about something.

[jeremyevans]

First, thanks for taking the time to look over the code an offer suggestions.

If by "leak account information" you mean leak that a given login has an account, Rodauth does not consider that secure information. For example, it uses different error messages for wrong login and wrong password. This is a specific design choice that is much friendlier to users with almost no negative security impact.

In almost all cases, it's possible to figure out whether an account has a been setup for a login (try creating a new account with the login, or changing your login to the given login), so it's pointless to try to prevent an attacker from determining that information. Even if you somehow are able to make the output the same in all cases, you could probably run a timing attack to determine whether or not an account exists for a login.

In terms of 401 vs 422, RFC2616 states "If the request already included Authorization credentials, then the 401 response indicates that authorization has been refused for those credentials." Therefore, I think that 401 is the most appropriate error code for login/key failures. Yes, it's true that WWW-Authenticate header is not used unless http_basic_auth is used, but I think using 401 best keeps with the spirit of RFC2616.

[HoneyryderChuck]

We might need to clear out this interpretation detail, but I think that by "Authorization credentials", the RFC means entity/token pair. As the http spec doesn't define authentication beyond basic/digest, and these are independent from POST body/form or invalid form parameters, I think one is relatively free to implement as he wishes. 422 seems to me clearer in the sense that "there was a POST form with parameters and one or more are invalid". But as stated before, I don't feel strongly about it, there is no reference implementation (github uses 401 as well), and one can change the status if he disagrees.

Thx for the updates, these will certainly be more helpful in the json authentication case.