Service Auth: Authenticate user if Bearer is present even if "Require authentication" is not checked

Patch looks good to me.
However there is a case with this patch when you do a request to a ressource that does not require auth but you have a invalid token in your request, in that case the call will fail instead of logging you as anonymous. In itself it should not be a big deal because it does not break previous workflows, so if someone else is ok with the patch i think we should mark it RTBC.

Thank you so much for this patch. This feature is exactly what I need and the patch works as expected (I am already using it on my production server).

I wished a fix like this long time ago, but I didn't have time to do it myself. Then the patch is there, for such a long time (almost a year), and I missed it. Strange!

I ran in to an issue with this patch when using the Performance configuration "Cache pages for anonymous users" turned on.

Steps to Reproduce:
1. Turn on "Cache pages for anonymous users"
2. Create a Services Resource that doesn't require authentication
3. Access the resource first as an anonymous user
4. Access the resource again as an OAuth2 authenticated user

What I notice is that I incorrectly get the anonymous users response back as an authenticated user.

This appears to be because the patch will bypass the oauth2_server_verify_access() function which specifies "drupal_page_is_cacheable(FALSE)"

This patch is a bit longer and more complicated, but I hope that it does it right. Can you check it?

The new patch #6 is working correctly for me. Thanks!

It is working for me too (but I didn't have that cache problem anyway).
Can you change the status to Reviewed and Tested By The Community?

1. +++ b/includes/oauth2_server.services_auth.inc
   @@ -9,16 +9,28 @@
   +  // Service can continue or not without authentication.
   

   I find this comment confusing, it might be better just to remove it.

2. +++ b/includes/oauth2_server.services_auth.inc
   @@ -9,16 +9,28 @@
   +  $token = oauth2_server_verify_access($auth_settings['server'], $auth_settings['scope'], $required_authentication);
   

   I think you could have used oauth2_server_check_access() here, instead of changing oauth2_server_verify_access()

@pjcdawkins, please suggest your own patch, and let us test it.

This patch shows what I mean: there is already a function for checking access without aborting the request.

Yes there was a bit of rationale. FALSE to me would imply access denied. But this is just saying that this particular access method/plugin didn't authenticate the user, it's like a temporary 401. Other plugins can act later.

And Services doesn't care about the return type.

1. It seams to me that return NULL; and return; are equivalent, and if Services doesn't care about the return type, then we should stick with the simplest form.
2. It seams to me that this is wrong and should be removed: || $result->getStatusCode() !== 401
3. By skipping the function oauth2_server_verify_access() you make it useless, a dangling function that is called nowhere else. So, this patch should also include the removal of this function.

1. OK, updated in this patch.
2. The point of this issue is that you can be authenticated if the Authentication header is provided and 'require_authentication' is off. 401 means you haven't provided the Authentication header; it doesn't indicate there should be a fatal error. Services allows multiple authentication methods to act, e.g. cookies. This allows them to co-exist with OAuth2. So... I've added a comment in the patch.
3. Yes, it's pretty much unused (apart from a test). But it's still useful to other modules. And functions are potentially part of the API, so they should not be removed in a minor version, and we do not intend to be skipping to a new major version (2.x) on D7.

Now I've removed the redundant return statement (!)

$result is instanceof \OAuth2\Response, created and returned by oauth2_server_check_access(). It should be sent to the client if authentication is required, because it indicates that something went wrong with the authentication. No further checks are needed for this.

The function oauth2_server_verify_access() is not part of oauth2_server.api.php, and I don't know how somebody could use an internal function like this in another module. Anyway, it doesn't hurt to live it around.

"It should be sent to the client if authentication is required" - yes, it is.

+    if ($required || $result->getStatusCode() !== 401) {

I think the extra checking is useless and even may be wrong. Can you show a real case (example) when it can be useful?

OK I've removed the 401 thing - yes it's a bit exotic to have other authentication methods existing alongside OAuth2.

So this is pretty much your patch above, but without return FALSE, and leveraging the ..._check_access() function.

For me it is OK.

I think there should be an error if no server is set, regardless of whether authentication is required.

Sorry, in the commit I should have credited dbouman for that very important contribution in #5 (I used Drupal's automatic attribution thing, which doesn't give enough credit to issue comments)