This is a patch that opens up for alternative authentication methods in the services module by decoupling the authentication method from the rest of the logic. These changes should not break any existing implementations that use the key authentication scheme as key authentication is the default authentication method.

CommentFileSizeAuthor
#13 pluggable_auth_4.diff46.96 KBHugo Wetterberg
#11 pluggable_auth_3.diff47.21 KBHugo Wetterberg
#8 pluggable_auth_2.diff53.61 KBHugo Wetterberg
#7 pluggable_auth.diff53.67 KBHugo Wetterberg
#2 0003-Decoupled-the-key-authentication-logic.diff29.72 KBHugo Wetterberg
#1 0002-Decoupled-the-key-authentication-logic.diff21.71 KBHugo Wetterberg
0001-Decoupled-the-key-authentication-logic.diff18.36 KBHugo Wetterberg

Comments

Hugo Wetterberg’s picture

Hugo Wetterberg commented
StatusFileSize
new 0002-Decoupled-the-key-authentication-logic.diff21.71 KB

Rebased the patch to the current DRUPAL-6--1 (#374827 by snelson: replace and, or with &&, ||)
Also fixed some stuff that didn't work properly in the last patch.

Hugo Wetterberg’s picture

Hugo Wetterberg commented
StatusFileSize
new 0003-Decoupled-the-key-authentication-logic.diff29.72 KB

I don't know if anyone keeps track of this issue or tests the patch. But here's the current status of the oauth branch as a diff.

marcingy’s picture

marcingy commented

Hugo we are going to be reviewing your patch at DC at the services code sprint http://groups.drupal.org/node/19535. Join us if you are in town.

Hugo Wetterberg’s picture

Hugo Wetterberg commented

Great! Unfortunately I won't be at the US drupalcon, but well do some long distance participation from Sweden. Looking forward to working with you all.

marcingy’s picture

marcingy commented

Hugo

Some comments about the patch as it stands:

* Don't like the fact that conditional logic is in hook_menu - all conditional menu callbacks should be in a seperate module. So we end up with a services_autheciation.module
* I would like it if we could register autheciation type and validation via a simple setting hook.

authenication_method = array(
  '#name' => 'method1',
  '#validation_method' => 'validation_method1',
)

* This would remove the need for loading via a specific name string eg services-authentication
* All args to the validation method would be passed as an array of keyed arguments.

marcingy’s picture

marcingy commented
Status: Needs review » Needs work
Hugo Wetterberg’s picture

Hugo Wetterberg commented
Status: Needs work » Needs review
StatusFileSize
new pluggable_auth.diff53.67 KB

Ok, here's the new patch. Key authentication has been moved into a separate module and the authentication module registration is done as you suggested.

Cheers
/Hugo

Hugo Wetterberg’s picture

Hugo Wetterberg commented
StatusFileSize
new pluggable_auth_2.diff53.61 KB

Removed a debugging var_dump statement

marcingy’s picture

marcingy commented

Thanks I'll review this during my time at the airport :)

marcingy’s picture

marcingy commented

The patch seems to have a number of search service items in that seemed to have been rolled into the patch by accident. I'll still review but can you reroll without these extra files?

Hugo Wetterberg’s picture

Hugo Wetterberg commented
StatusFileSize
new pluggable_auth_3.diff47.21 KB

Oops, accidentally included #328764 in my patch. Fixed now.

voxpelli’s picture

voxpelli commented

This is a nice feature - I hope it will get in so I'm subscribing to get the updates on it

Hugo Wetterberg’s picture

Hugo Wetterberg commented
StatusFileSize
new pluggable_auth_4.diff46.96 KB

Well, here's the a updated patch that applies cleanly to the current head (#359519).

I hope that someone will get time to review this patch soon.

G43USUgppHFPQ’s picture

G43USUgppHFPQ commented

Are there any devs available to review Hugo's work?
It works fine for me and the code looks good in my eyes.

I would like to see it merged so I don't have to apply it manually with each release of Drupal.

PGiro’s picture

PGiro commented

Any news on this so we can use OAuth with services ?

voxpelli’s picture

voxpelli commented

PGiro: I think that Hugo has got it all working but that the code needs a review, as all patches do, to go into the Services module. Perhaps any maintainer can answer of Services can answer the question better?

marcingy’s picture

marcingy commented

After some crazy weeks at work this is on my list to review tomorrow.

PGiro’s picture

PGiro commented

@marcingy : how far did you get down your list ;)

marcingy’s picture

marcingy commented

After discussions with heyrocker, snelson and robloach we have agreed that we get this in initially and then deal with stuff in follow up patches as required

* We need to provide a hook_service_alter which allows specific autheciation methods to add additional parameters there require - eg is the value part of the hash or can this be achieved through alter_method?
* Have made some minor style changes as per coder and code tough love.
* Moved the new module into an auth services_keyauth folder in case we decide to ship more authenication methods
* Have changed failures on browser so as no value is returned
* Have added method_name as part of the authenicate signature

Otherwise looks good and after testing we now have pluggable authenication :)

marcingy’s picture

marcingy commented
Status: Needs review » Fixed
marcingy’s picture

marcingy commented
Status: Fixed » Closed (fixed)