Add oauth2 authentication support for the http_client.

The attached patch does this. However it assumes that this patch has been applied first:
#1285310-14: Authentication settings in the UI
The attached patch makes a small correction to the code of the previous patch and also
adds support for oauth2 authentication.

This patch is important (required) by the patch on this issue: #2138627: Create plugin for OAuth2 authentication

CommentFileSizeAuthor
#7 wsclient-2138617-oauth2_support-2-combined.patch3.4 KBdman
#1 wsclient-2138617-oauth2_support.patch1.28 KBdashohoxha

Comments

dashohoxha’s picture

dashohoxha commented
Issue summary: View changes
StatusFileSize
new wsclient-2138617-oauth2_support.patch1.28 KB
dashohoxha’s picture

dashohoxha commented
Assigned: dashohoxha » Unassigned
Issue summary: View changes
Related issues: +#2138627: Create plugin for OAuth2 authentication

The patch needs to be reviewed and applied.

dashohoxha’s picture

dashohoxha commented
Status: Patch (to be ported) » Reviewed & tested by the community

This patch is so simple that it can't be wrong. Also I have tested it (I am using it on my project). The required patch on http_client has also been applied. So, why not apply and commit this patch?

dman’s picture

dman commented

Good point. I agree this can't be wrong.
I'm active in some wsclient stuff today, and would love to put this in... but would also like to test...

Are you able to give me an example to run against? I'm hoping to get the Google APIs (which have switched to OAuth) up and running again under WSClient this week, so if I could add a working example to the docs that would be the sweetest thing ever!

dashohoxha’s picture

dashohoxha commented

I don't have an example using it against Google or something. I use it like this:

  $service_name = 'btr';
  $server_url = variable_get('oauth2_login_oauth2_server', '');
  $client_id = variable_get('oauth2_login_client_id', '');
  $client_secret = variable_get('oauth2_login_client_secret', '');
  // 
  $token_endpoint = $server_url . '/oauth2/token';
  $authorization_endpoint = $server_url . '/oauth2/authorize';
  $redirect_uri = url('oauth2/authorized', array('absolute' => TRUE));
  //
  $service = new WSClientServiceDescription();
  $service->name = $service_name;
  $service->label = 'Authenticated B-Translator Services';
  $service->type = 'rest';
  $service->url = $server_url . '/btr/';
  //
  $service->settings['authentication']['oauth2'] = array(
    'token_endpoint' => $token_endpoint,
    // auth_flow can be: server-side | client-credentials | user-password
    'auth_flow' => 'server-side',
    'client_id' => $client_id,
    'client_secret' => $client_secret,
    'redirect_uri' => $redirect_uri,
    'authorization_endpoint' => $authorization_endpoint,
  );
  //
  $service->operations['get_translations'] = array(
    'label' => 'get_translations',
    'url' => 'translations/@sguid',
    'parameter' => array(
      'sguid' => array('type' => 'text'),
      'lng' => array('type' => 'text'),
    ),
  );

And then something like this:

  $btr = wsclient_service_load('btr');
  $strings = $btr->get_translations($sguid, $lng);

I am not sure whether it can be adopted easily for Google.

dman’s picture

dman commented

Thanks heaps, that is exactly the sort of "How to use this in the real world" doc I was looking for.
Great!
So I went to see if I could apply this immediately..

HOWEVER, so it depends on #1285310: Authentication settings in the UI first ... well that's OK if they re so related.

I've attached the combined patch...
BUT it looks like that other issue was stalled waiting for a UI to be exposed. :-(
and yeah, variable_get('wsclient_httpauth_username') isn't sustainable going forward. Damn.

And for now, the new oauth method added here doesn't do a UI EITHER. Blast.
That's sorta pretty important, given Klausi's decision "This issue is about making HTTP Basic Auth configurable in the UI"

Hm.

Options:
A: stall until we get a UI up? Hopefully at least combine the two threads here because it's a pain running both that are interdependant
B: Just push these two api-level improvements in and then layer on the UI in a second round? Even so, the variable_get method sucks and would have to be removed again.

Neither makes me happy.

C: Just build on the UI now today and get one largish patch. It won't be terrible, as it's not touching things that need too much revision.
Just doing that is the only way I can break this deadlock without lots of to-and-fro.

D: Put this feature into a small -dev branch on the repo to share until it's settled?

dman’s picture

dman commented
Status: Reviewed & tested by the community » Needs work
StatusFileSize
new wsclient-2138617-oauth2_support-2-combined.patch3.4 KB

Needs work as it's blocked by [#7657727-15] - requires that one to land first AND needs a UI exposed really.

dashohoxha’s picture

dashohoxha commented

I would say go on with B. I have applied these two patches and it was not the end of the world. Let's not make things more complicated then they are.

dman’s picture

dman commented

The variable_get there sucks as-is, ... and is it even used?
Also, it looks like it introduces an undocumented, un-checked and break-able dependency on HttpClientBasicAuth

[#7657727] is just not clean enough yet.

The patch you give here is fine, and that's what I looked at first. But as it requires that other broken stuff to be pulled in as well first, it's not cool.

dashohoxha’s picture

dashohoxha commented

Which 'variable_get' are you talking about? Is it not on the removed part?
And then, if nobody tries to use basic auth, it brings no harm, even if HttpClientBasicAuth is undefined.
Finally, you can comment out the lines about basic auth and leave only those related to oauth2 auth, and it will be fine.

dman’s picture

dman commented

Ah sorry, I see the variable_get stuff was indeed removed - the patch-diff I was looking at was showing it to me as a reverse with +, yep. My confusion.

dman’s picture

dman commented

OK, I've done some testing, and can't see how this would introduce any regression. All my old tests work with it, so that's a start.
It's not got the UI there yet, but it lays a better foundation that the UI can actually expose now, so that's a safe step forward. I'm gonna go for that.

dashohoxha’s picture

dashohoxha commented

I agree. It can be used from the code, without needing a UI, and the lack of the UI does not break anything.

dman’s picture

dman commented

Pushed http://drupalcode.org/project/wsclient.git/commit/85696e5cbe42729110c447... as a copmbined patch, as the second one repaired some issues with the first.

STILL NEEDS UI added to put this to bed.

jonhattan’s picture

jonhattan
Primary language Spanish
Location Plasencia
commented
Issue summary: View changes

Updated issue summary to fix links to the mentioned issues.