Decouple api and user authentication

Great job with this one, the split into two separate modules makes sense. The session authentication module is actually the only one I think that it makes sense to include with services out of the box.

Will review this afternoon.

I'll review it latter just wanted to comment on This bug caused authentication to fail when no auth module was selected.

This feature was added after discussion with Heyrocker simply to prevent people opening their sites up by having no modules installed or authenication set up, which appeared to be a security hole. Maybe the answer is have a 3rd auth method - open access which does this but ensures that people have actively selected this approach - I don't think the module needs to do anything except put a warning on the config screen.

Hmm, I think I disagree on this point. People who activate the services module and service-implementation modules have made a very active choice to open up their site. No security was bypassed, as a session with the required privileges still is required to access services, so I wouldn't call it a security hole. No authentication equals cookie based session authentication.

And then that's another problem with this patch: that it, combined with the decision to disable services until a authentication method is selected, closes the door on those depending on the previously valid option of using "browser-based cookies".

Ah so basically we remove the option to use keys from keyauth(as it is now) and add back an optional session handling feature. Even still though it doesn't make sense that the access check is always being carried out given that an access check might not be required - ie I have a key and am not using sessions. One option is for key_auth or what ever auth method is being used to check and see if sessions are being used if they aren't unset ['#access'] on a method - key_auth now has controls on methods at a key level.

Good pont - updating tickets while having my mind on work at the same time is never a good idea.

So all we need to do is allow for sessions on the key auth ;)

New version of the patch

* Sessions can now be enabled on keyauth
* Keys are none selectable
* Failure if no authenication method is selected have been removed and replaced with a prompt to take action
* Introduction a no authenication method that sits in the core service module - if it is selected prompt above goes.
* Tweak to ensure the browser doesn't error in certain cases with key auth (system.connect)

I finally got a chance to do some review on this

1) When applying the patch to the current -dev I got the following:

I told it no and did not apply anyways.

2) When key authentication is enabled, the settings page still has a checkbox for "Use Sessid". This should be removed.

3) It also appears that the variable 'services_use_sessid' is still being used. If I enable key auth and check the 'use sessid' box, it still requires both key api and sessid, and gives the missing required parameter error if both are not present. Do we even need the 'services_use_sessid' and 'services_use_key' variables anymore?

4) It would be really nice if, when upgrading from a new version, we could have an update_n() hook that automatically checks which auth method they were using (sessions or keys) and enables the appropriate module. This would make the upgrade path a lot easier and avoid the kind of issues we've seen in the queue recently where people are taken by surprise at having to enable a new module. Is this possible? I'm not completely sure.

That's what I've got from giving it a spin, haven't looked at the actual code yet.

This version does all the above plus it makes domain a required field under keys - this is the identifier to the server so it needs to be required.

I'd like to see this modified such that if you have no authentication method chosen, then all requests to Services return Access Denied, regardless of the menu system. Intuitively it makes the most sense - if I have not chosen an authentication method, then things should in fact not get authenticated. It is also the safest course of action, and will prevent people from accidentally opening their sites up. For instance, say a bug crops up in the uprgade code which doesn't properly transfer a person's choice of key authentication to their new installation. Instead no authentication module is chosen, but their permission settings are still the same. They are now totally open. We should be handling this defensively rather than just assuming nothing will go wrong. It is the safest route and 100% guaranteed to keep the security team happy.

In the situation where someone has not chosen an authentication method we shouldn't have a drupal_set_message() on every page either. Instead I'd like to see this rolled into hook_requirements so you only get the warning on install and on the status page.

Also, if we're going to continue to allow sessions in key authentication, then why have a separate session authentication at all? If unchecking both is going to open up the server as before, why have a no authentication option at all? This whole situation as the patch is now, where there are multiple paths to the same result, many of which are not clearly labeled as such, is really confusing.

More thoughts anyone?

After discussions with heyrocker postponed

The issue is that given that keyauth as it stand supports session and key combos then I'm thinking that we simply say if you want key based auth you need to move to oauth otherwise we end up with key auth that ends up effectively handling session auth and keyauth to support existing configurations. In otherwords we kill keyauth.