Refactor the Services module

Hi all. These days I've been working with services module for two sites and I had to do several in-depth research of module functionality. Somethings I did not expect, others I assume, my first touch of real implementations for this module (I've been using 6.x branch) has been a little chaotic. So I guess it is time to help. I did start my own and then I was aware about the existing D7 port. I tried to contact to marcingy yesterday, and finally I decided to help on this approach pushed in somehow by Heyrocker and RobLoach on irc today.

During these real life setups of the module(s), the first thing I missed was information about how to call the services, how to configure permissions, and what services should do. Later on, as I was reding the code I realized several 'issues' that should be addressed IMO, but as I'm not sure/aware of the functionalities in the module (and there is no point nor clue in the issue queue) I'm unable to resolve them myself.

Because of this, I just started to write some simpletest cases to be sure what is expected and what is not, and then I could move forward asking in the issue queue (that was the original plan). Now that a D7 is on the way, I guess it's time to just discuss them, and resolve if required, so I'll share them. We may have an IRC discussion on IRC: Heyrocker and RobLoach will join. I'm sure most of the issues will be addressed in the 6.x-2.x branch. This is the issue list I would like to share (I'm trying to explain as easy as possible):

- Services require authentication 1: This is not well documented (I found related prosa in the issue queue: #465546: Decouple api and user authentication). Currently, when no authentication module is enabled, services will always reply "TRUE", instead of returning an error about authentication, even if servers already have an error callback waiting to do some job.

- Authenticate using session 1: the following module list is required: services, services_keyauth, xxxxx_server, system_service, user_service, just to 'start using services'. I think this is insane, because we have to enable more modules than we require (just to use system.connect and user.login) and we are exposing services we don't want to be there, that need an extra configuration. IMHO,

- Single authentication / Multiple authorization: currently authentication is not managed in the right way I guess.. authorization and authentincation are merged in the keyauth module and access callbacks. Using both session ID and Key authentication makes no sense.. Authorization should be 'yes you are allowed' or 'not, you can't do that' and authentication is 'yes, this is you' or 'no, you are not this'.. session ID is probably the best approach for authentication (identifying the user, and doing the relationship -> user -> role?), and keys seem to be more focused in authorization. Keys could also be linked to user being able to authenticate, but who should be owner of the content created with one session ID, and the KEY of other user?.. We need to be consistent here. Splitting key and session authentications will help, but we should be sure it's done properly.

Being authenticated (user could be identified) we could provide more fined permissions per-service method (API currently supports that).

- Services require authentication 2: if services are controlled using an access callback, then why we can't just provide also anonymous services?

Ok, now move into code..

- function services_method_call(): Currently the module checks if method exist, and all the required arguments are there.. and then checks authentication.. do you see what I mean? should this be changed to first check for authentication, and then do the checks..

- access callbacks: in services_method_call(), there is call to access callbacks based on several factors ('#access arguments', '#access arguments append'). I was unable to find doc for them even if I can figure where does "#access arguments" come and what does it mean, but what about the appended part? In any case, lets see this access callback call:

...
// Just use the arguments array if no access arguments have been specified 
   $access_arguments = $args;
...

and this is one of the callbacks:

/**
 * Check if the user is allowed to get the user data.
 *
 * @param $uid
 *   Number. The user ID.
 */
function user_service_get_access($uid) {
  global $user;
  return (($user->uid == $uid && user_access('get own user data')) || ($user->uid != $uid && user_access('get any user data')));
}

$user->uid will never match an array

- In services.module: there is code relevant to resouces and also an implementation of node_load : services_node_load.. should this one be here?...

Ok, I've some more, but may be discussed later.. this is a good start :)

Just would like to focus in one thing:

- Service names: it is time to make use of the new naming convention while we convert this module to D7: #312308: Method names match Drupal function names and also #199176: Avoid using special chars in services definition (sorry, I had to mention ;) )

I'm now moving all the test cases to D7, boombatower submitted yesterday a patch to easy the tests coding, so we can use our own DrupalServicesTestCase class, focused on services testing.

So, keep this moving ;)