search

zfcampus / zf-mvc-auth

BSD 3-Clause "New" or "Revised" License
42 stars 46 forks source link

Incorrect reponses when using the OAuth authentication adapter #108

Open nuxwin opened 8 years ago

nuxwin commented 8 years ago

Due to the changes from 101feec Oauth authentication adapter behavior is inconsistent and wrong.

Current Behavior

I. Clients are never challenged with proper 401 response

This is due to changes from 101feec in which a 401 response is assumed only if the error parameter from the OAuth2 response is not null. While this assertion is true for an invalid, a malformed or an expired token, it is not the case for "initial challenge" (When no credential were provided at all).

Indeed, when an authentication request is checked, and if something goes wrong, we have only few response possibilities:

No credential were provided at all by the client

In such case, a 401 response with the appropriate WWW-Authenticate header is created. This doesn't involve any error parameters in the WWW-Authenticate header.

Invalid, malformed or expired token

Same as above, but here, some error related parameters are added to the WWW-Authenticate header.

Insufficient scope

In such case, a 403 response is returned with a WWW-Authenticate header. Some error related parameters are also added to the WWW-Authenticate header.

II. Problem responses are not ApiProblemResponse

Apigility should be consistent when returning a response denoting a problem, whatever the authentication adapter in use. Currently, in the OAUth authentication adapter case, the problem responses are not marshalled to ApiProblemResponse, resulting to an empty body. Even if valid, we should be consistent here.

Expected behavior

As per 101feec @weierophinney assumes that if no credential were provided at all, a GuestIdentity must be assumed (case of a public API). This assertion would be valid if all resources were public, but in Apigility, permissions are based on HTTP methods and not on API endpoints. Thus, assuming a GuestIdentity (therefore, a public API) when no credential are provided is wrong because clients are never challenged correctly in case of private resource. This is exactly the same assertion I've made by mistake in #92, which resulted to the #106 issue.

This demonstrates all the problem with current Apigility permissions system. Authorizations are always evaluated after authentication. The problem which we must solve is how to let the authentication layer know when a route require authentication or not. We could solve this problem by adding Guards at authentication level (see below).

Authentication level guards

Those guards would informs the MvcAuth event (e.g, with a route guard resolver listener) when authentication is required for the matched route. If so, client would be challenged or not. This would solve our problems and more, this would allows performance boost because if no required, authentication process would be aborted early (no authentication adapter involved).

Authorization level

We have either a Guest identity or Authenticated identity. Whatever... In case of Apigility, authentication guards already tell us if user can access the route or not. Here, a 403 response would result of auhorization failure only. This would be the case if the developer implement authorization layer (e.g. conditional ACL with assertions).

Related issues

Directly related: #99 Indirectly related: #97 and #106

nuxwin commented 8 years ago

@jackdpeterson

Your opinion on the proposal above?

jackdpeterson commented 8 years ago

I agree with the above proposal. The problems with the current Apigility approach are well described above and are counter-intuitive for anyone who is developing a client-side application that relies on an appropriate HTTP status code. 403 is for insufficient privileges WHEN an identity is asserted. 401 is when one needs to assert an identity but hasn't or the credentials asserted are invalid.

I like the idea of differentiating between public and private API 'services' so long as it can be done at a per-method level (e.g., differentiate between GET and POST). I also like the idea of the early exit should no authentication step be required for performance reasons.

This is definitely a Backwards compatibility break; however, in my opinion, it's a forward-compatibility enhancement and will promote wider-adoption for those that get to the point of developing an API that must be interacted with using numerous clients AND leveraging oauth or other complex authentication/authorization schemes.

Per RFC-2616:

10.4.2 401 Unauthorized

The request requires user authentication. The response MUST include a WWW-Authenticate header field (section 14.47) containing a challenge applicable to the requested resource. The client MAY repeat the request with a suitable Authorization header field (section 14.8). If the request already included Authorization credentials, then the 401 response indicates that authorization has been refused for those credentials. 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 SHOULD be presented the entity that was given in the response, since that entity might include relevant diagnostic information. HTTP access authentication is explained in "HTTP Authentication: Basic and Digest Access Authentication" [43].

AND

10.4.4 403 Forbidden

The server understood the request, but is refusing to fulfill it. Authorization will not help and the request SHOULD NOT be repeated. If the request method was not HEAD and the server wishes to make public why the request has not been fulfilled, it SHOULD describe the reason for the refusal in the entity. If the server does not wish to make this information available to the client, the status code 404 (Not Found) can be used instead.

I think the article here, http://robertlathanh.com/2012/06/http-status-codes-401-unauthorized-and-403-forbidden-for-authentication-and-authorization-and-oauth/ covers this topic very well and is consistent with my view on how 401 vs 403 should be treated (with the exception on the very last part regarding oauth 1.0).

weierophinney commented 4 years ago

This repository has been closed and moved to laminas-api-tools/api-tools-mvc-auth; a new issue has been opened at https://github.com/laminas-api-tools/api-tools-mvc-auth/issues/9.