[PATCH 1 of 5] auth: do not redirect to login page on invalid API key

[Mads Kiilerich]

On 03/26/2015 08:57 PM, Thomas De Schampheleire wrote:
> Hi Mads,
>
> On Wed, Mar 25, 2015 at 7:54 PM, Mads Kiilerich <mads at kiilerich.com> wrote:
>> On 03/25/2015 12:01 PM, Thomas De Schampheleire wrote:
>>> # HG changeset patch
>>> # User Thomas De Schampheleire <thomas.de.schampheleire at gmail.com>
>>> # Date 1427269791 -3600
>>> #      Wed Mar 25 08:49:51 2015 +0100
>>> # Node ID c5828585502f1a061f162abe8cbd181c17039843
>>> # Parent  6017996e4dcfda0f5623498a45c51bb184eb67bb
>>> auth: do not redirect to login page on invalid API key
>>>
>>> When accessing Kallithea through an API call, providing an API key, it
>>> doesn't make sense to redirect to a login page on failed authentication.
>>> Instead, raise a 401 Unauthorized exception.
>>>
>>> The WWW-authenticate header is a mandatory element for 401 Unauthorized,
>>> as
>>> specified by RFC 7235. The exact contents do not seem to be important, so
>>> define a custom auth scheme 'APIKEY' with a realm of 'Kallithea'.
>>>
>>> diff --git a/kallithea/lib/auth.py b/kallithea/lib/auth.py
>>> --- a/kallithea/lib/auth.py
>>> +++ b/kallithea/lib/auth.py
>>> @@ -58,6 +58,7 @@
>>>        get_user_group_slug, conditional_cache
>>>    from kallithea.lib.caching_query import FromCache
>>>    +from webob.exc import HTTPUnauthorized
>>>      log = logging.getLogger(__name__)
>>>    @@ -763,6 +764,8 @@
>>>                        log.debug("API KEY *NOT* present in request")
>>>                    else:
>>>                        log.warning("API KEY ****%s *NOT* valid" %
>>> _api_key[-4:])
>>> +                    headers = [('WWW-Authenticate', 'APIKEY
>>> realm="Kallithea"')]
>>
>> The API does not use HTTP authentication. I thus think it is misleading to
>> return 401 and WWW-Authenticate.
>>
>> I think it would be better to just fail with 400 Bad Request.
>>
>> When we don't add headers, we can just use "return abort(400)" as done
>> elsewhere in the same file.
> The API does not use the classic 'Basic' nor 'Digest' authentication
> methods of HTTP, but IMO passing the API key is a form of
> authentication in the spirit of the HTTP RFC 7235:
>
> ------------
> 3.1.  401 Unauthorized
>
>     The 401 (Unauthorized) status code indicates that the request has not
>     been applied because it lacks valid authentication credentials for
>     the target resource.  The server generating a 401 response MUST send
>     a WWW-Authenticate header field (Section 4.1) containing at least one
>     challenge applicable to the target resource.
>
>     If the request included authentication credentials, then the 401
>     response indicates that authorization has been refused for those
>     credentials.  The user agent MAY repeat the request with a new or
>     replaced Authorization header field (Section 4.2).  If the 401
>     response contains the same challenge as the prior response, and the
>     user agent has already attempted authentication at least once, then
>     the user agent SHOULD present the enclosed representation to the
>     user, since it usually contains relevant diagnostic information.
>
> --------------
>
> except for the fact that we are not using the Authorization header but
> a GET value 'api_key'.

Hard question ... no easy answer ... and I changed my mind several times ...

Some random related but (IMO) non-conclusive links:
http://brockallen.com/2013/10/27/using-cookie-authentication-middleware-with-web-api-and-401-response-codes/
http://www.w3.org/html/wg/tracker/issues/13
https://github.com/tbroyer/http-cookie-auth/blob/master/spec/draft-broyer-http-cookie-auth.rst
http://webmasters.stackexchange.com/questions/24443/should-i-return-a-http-401-status-code-on-an-html-based-login-form

I think I will maintain that even though it might work and not is 
explicitly wrong, HTTP authorization mechanisms are currently only 
intended and supported for HTTP level authentication. It would be 
unfortunate to respond with non-standard mechanism in Authorization 
header - that will make it impossible to use the API with "clever" 
standards compliant client software / middleware.

Also, a random link for some inspiration: 
https://groups.google.com/forum/#!topic/json-rpc/PN462g49yL8 where 
Kallithea pretty much is option b (and I don't want to mix it up with c).

> Failing with 400 Bad Request is indeed also an option, but it would
> not allow clear differentiation between a bad API key, or invalid
> parameters.
>
> And what about API requests to methods that are not exposed through
> API? In the patch I had used 403 Forbidden. Do you suggest to keep
> this or move to 400 Bad Request too?

I think it is the nature of (our?) json-rpc to be transport independant. 
All response have an error field that is used for reporting errors. Our 
documentation says: "All responses from API will be ``HTTP/1.0 200 OK``. 
If there is an error, the reponse will have a failure description in 
*error* and *result* will be null."

Some examples of error reporting:

{"error": "Incorrect JSON query missing 'api_key'", "id": null, 
"result": null}
{"error": "Invalid API KEY", "id": 1, "result": null}
{"error": "No such method: foo", "id": 1, "result": null}
{"error": "Missing non optional `repoid` arg in JSON DATA", "id": 1, 
"result": null}
{"error": "repository `foo` does not exist", "id": 1, "result": null}

I think missing / bad authentication or requests to a disabled method 
fits perfectly into that scheme.

Do you see problems with that?

I guess one problem is that it would be nice to have some stable 
identifier of the kind of error instead of just having the human 
readable error message. We could perhaps redefine the error field or add 
another field, depending on what is most 'standard json-rpc'. That is a 
general existing problem that I guess would solve this problem too.

/Mads