Page MenuHomePhabricator

MediaWiki oauth1 adaptor for phabricator
AbandonedPublic

Authored by 20after4 on May 19 2014, 6:59 PM.
Referenced Files
Unknown Object (File)
Fri, Nov 29, 11:23 AM
Unknown Object (File)
Tue, Nov 26, 11:27 PM
Unknown Object (File)
Mon, Nov 25, 6:40 PM
Unknown Object (File)
Mon, Nov 25, 6:17 AM
Unknown Object (File)
Sat, Nov 23, 10:27 AM
Unknown Object (File)
Thu, Nov 21, 1:25 PM
Unknown Object (File)
Sun, Nov 17, 1:14 AM
Unknown Object (File)
Tue, Nov 12, 11:50 AM

Details

Summary

This is now a more robust implementation borrowing a bit from the JIRA oauth adaptor, this now configurable to work with any mediawiki install.

Test Plan

added and configured the mediawiki provider to work with my local test wiki. Associated my account with a wiki account. Logged in via mediawiki oauth. bingo!

Diff Detail

Repository
rPHU libphutil
Branch
arcpatch-D9202
Lint
Lint Passed
Unit
Tests Passed
Build Status
Buildable 2507
Build 2511: [Placeholder Plan] Wait for 30 Seconds

Event Timeline

20after4 retitled this revision from to Initial proof of concept implementation of a mediawiki oauth adaptor.
20after4 updated this object.
20after4 edited the test plan for this revision. (Show Details)
20after4 added reviewers: qgil, epriestley.

General notes:

  • en.wikipedia.beta.wmflabs.org should probably end up coming out of getAdapterDomain() and getting configured.
  • The mwoauth cookie is really sketchy / side-effectey / bleh, but there's probably nothing we can do about that.
src/auth/PhutilAuthAdapterOAuthMediaWiki.php
143–150 ↗(On Diff #21848)

It's very hard for me to believe this attack is plausible, but let's move this to something like phutil_constant_time_compare() for clarity, we "should" "probably" "use" it "elsewhere" "too".

qgil removed a reviewer: qgil.

@epriestley I really don't like the cookie either, is there any way I can jack into phabricator's session handling to stash that value somewhere other than a cookie?

After some thought, I realize we should actually already be handling this. Specifically, we set a client ID cookie (phcid) and then verify it by appending it to the callback URI (in PhabricatorAuthProviderOAuth1->processLoginRequest()). So it should be safe to just delete the cookie.

After some thought, I realize we should actually already be handling this. Specifically, we set a client ID cookie (phcid) and then verify it by appending it to the callback URI (in PhabricatorAuthProviderOAuth1->processLoginRequest()). So it should be safe to just delete the cookie.

That's what the Twitter authentication does, but then they (break OAuth spec and) don't sign the request for the Access token with the secret from the Request token.

MediaWiki needs the secret from the first token to sign the request for the second token. We could try to encrypt it and pass it through as state in the authorization call, but it would be much easier (on me) if the client could keep track of it.

In D9202#13, @csteipp wrote:

That's what the Twitter authentication does, but then they (break OAuth spec and) don't sign the request for the Access token with the secret from the Request token.

MediaWiki needs the secret from the first token to sign the request for the second token. We could try to encrypt it and pass it through as state in the authorization call, but it would be much easier (on me) if the client could keep track of it.

Or... if phabricator could keep it somewhere so that the client doesn't have to. I'd prefer that over encrypting it and sending it to the client as a cookie. The cookie seems really likely to fail, but maybe I'm just jaded from having many bad experiences with setcookie() in the past.

In D9202#14, @20after4 wrote:
In D9202#13, @csteipp wrote:

That's what the Twitter authentication does, but then they (break OAuth spec and) don't sign the request for the Access token with the secret from the Request token.

MediaWiki needs the secret from the first token to sign the request for the second token. We could try to encrypt it and pass it through as state in the authorization call, but it would be much easier (on me) if the client could keep track of it.

Or... if phabricator could keep it somewhere so that the client doesn't have to. I'd prefer that over encrypting it and sending it to the client as a cookie. The cookie seems really likely to fail, but maybe I'm just jaded from having many bad experiences with setcookie() in the past.

Definitely. The request token's secret isn't usually supposed to be known by the end user, so keeping it on the server and retrieving it only for the user who initiated the process would be best.

20after4 edited edge metadata.

Updated so that provider's url is configurable.

Use "clean" urls for mediawiki. This resolves a bug with mobile browsers caused
by an automatic redirect that happens inadvertently during the oauth process.

20after4 retitled this revision from Initial proof of concept implementation of a mediawiki oauth adaptor to MediaWiki oauth1 adaptor for phabricator.Jun 26 2014, 6:20 PM
20after4 updated this object.
20after4 edited the test plan for this revision. (Show Details)
20after4 added a project: Wikimedia.

This should be nearly complete, it's passed wikimedia code review
at https://gerrit.wikimedia.org/r/#/c/139438/3

Let me pull up the spec and figure out the signing issue. If this is a spec thing and Twitter is an anomaly, support should be at the generic OAuth1 level, not here.

src/auth/PhutilAuthAdapterOAuthMediaWiki.php
10–11 ↗(On Diff #23434)

Should these be empty? It seems unlikely that any install other than WMF will want to authenticate against this domain?

165–170 ↗(On Diff #23434)

Throw a comment here like "// This is a constant-time compare."? It took me a couple of minutes to figure out what was going on here the first time, since it looked like signature verification at first glance (e.g., "compare hash to signature").

D9760 should provide generic support and get rid of the need to set cookies, assuming I got the change right.

src/auth/PhutilAuthAdapterOAuthMediaWiki.php
10–11 ↗(On Diff #23434)

Fair enough.

@epriestley I've addressed your feedback from here as well as D9760 and the code seems to be working fine in my test install.

Couple of things I've hit so far:

  • This hard-codes /w/ as a URI prefix, but that looks like it's unique to the WMF install.
  • When the OAuth plugin responds with an error, the response doesn't have an HTTP error code (e.g., HTTP 200 + body of "Error: An error occurred in the OAuth protocol: Invalid signature"), so we don't handle it very usefully. I don't immediately have a clean fix for this, but maybe adding a callback hook which allows adapters to inspect HTTP/200 response bodies and throw if they're actually errors.
  • I'm getting the signature error above, looking into what's up with that...
  • Signature issue was extra slashes in the URL, because I'd entered http://wiki.local.com/. We should normalize this.
  • It wasn't obvious to me at first that I needed to have the OAuth consumer approved, and there are two different "Manage" interfaces (one for users, one for administrators) which threw me. Maybe worth a mention in the instructions.
  • I had to remove all the /wiki/ stuff too, I replaced that with index.php?title=, which seems to work.
  • Now hitting this, looking into that:

Screen_Shot_2014-06-30_at_7.05.42_AM.png (1×1 px, 184 KB)

From digging through the code, it looks like that's because:

OAuth relies on memcached for temporary tokens and sessions

If Memcache isn't configured, the $cache on MWOAuthDataStore is EmptyBagOStuff and all OAuth fails with an indirect error. Did I miss a setup warning somewhere? This seems like an odd failure mode.

Failure mode for Memcache config (maybe? Not sure what I got wrong here?) seems to be HTTP 200 with a message like this:

string(21) "MemcachedPhpBagOStuff" Error: Sorry, something went wrong connecting this application. Go back and try to connect your account again, or contact the application author. <span class="plainlinks mw-mwoautherror-details">OAuth token not found, [https://www.mediawiki.org/wiki/Help:O

I can make it through the first Memcache issue, so I'm not immediately sure what the problem here is. Digging into it...

epriestley edited edge metadata.

Okay, this works for me. Stuff we should fix:

  • Remove /w/ and /wiki/ hardcodes, these do not work in the general case.
  • Provide a getWikiURI() method or similar which normalizes slashes, so the user can enter http://wiki.domain.com or http://wiki.domain.com/ (with a trailing slash) when configuring an adapter.
  • We should deal with the 200 + error responses. I think it's reasonable to provide a (default no-op) callback hook like willProcessTokenRequestResponse($raw_body). The parent can just return;, and the MW adapter can check if the response contains an error message. One way to trigger this is to intentionally break the signature by adding extra slashes to the URL.
  • The setup process should more fully commit to being two-phase, like JIRA. In particular, the getProviderConfigurationHelp() method currently gives bad advice:

Screen_Shot_2014-06-30_at_6.45.43_AM.png (225×996 px, 33 KB)

Two things are wrong here:

  • The link points to the main MW install, but should point locally.
  • The URL is incorrect, because the user hasn't added chosen a name yet.

Instead, this shuld be fully two-phase:

  • In the setup phase, ask for a name and URI. Provide Help text like "step 1 of 2, continue...".
  • In the edit phase, ask for the keys. Provide help text with correct URIs.

This may be a bit messy, but the JIRA adapter is a fully two-phase OAuth1 adapter, so it should be possible.

Then:

  • It would be helpful to mention that applications must be approved in the help text.
  • It would be helpful to mention that MW must be running memcache, and the process will fail in a somewhat confusing/nonobvious way if it isn't, if this isn't already improved at HEAD in the OAuth extension.
This revision now requires changes to proceed.Jun 30 2014, 2:42 PM

The /w/ and /wiki/ urls are the default and recommended mediawiki configuration:

https://www.mediawiki.org/wiki/Manual:Short_URL

One issue is that the plugin only works with mediawiki.org when it uses /w/ and /wiki/ in appropriate places. I'm not sure how to make that part configurable without making it sorta confusing...

@csteipp can you offer any suggestions here?

In D9202#47, @20after4 wrote:

The /w/ and /wiki/ urls are the default and recommended mediawiki configuration:

https://www.mediawiki.org/wiki/Manual:Short_URL

One issue is that the plugin only works with mediawiki.org when it uses /w/ and /wiki/ in appropriate places. I'm not sure how to make that part configurable without making it sorta confusing...

@csteipp can you offer any suggestions here?

I think it's pretty easy actually. Don't use /wiki/ and assume the wiki never has short URLs (which they very well might not). We don't have to use them. Then include /w/ in the base part of the URI you configure and you're set.

In D9202#48, @WikiChad wrote:
In D9202#47, @20after4 wrote:

The /w/ and /wiki/ urls are the default and recommended mediawiki configuration:

https://www.mediawiki.org/wiki/Manual:Short_URL

One issue is that the plugin only works with mediawiki.org when it uses /w/ and /wiki/ in appropriate places. I'm not sure how to make that part configurable without making it sorta confusing...

@csteipp can you offer any suggestions here?

I think it's pretty easy actually. Don't use /wiki/ and assume the wiki never has short URLs (which they very well might not). We don't have to use them. Then include /w/ in the base part of the URI you configure and you're set.

We can just use whatever url the user inputs (maybe suggest they use the script path, /w/, etc), but then mobile redirects don't work.

We could add an optional parameter in the setup, which asks for the optional url to redirect the user's browser to, and suggest a /wiki/Special:OAuth url. If the user fills it out, we use that for the /authorize url. Otherwise we just use the one url they gave us?

In D9202#50, @csteipp wrote:

We can just use whatever url the user inputs (maybe suggest they use the script path, /w/, etc), but then mobile redirects don't work.

We could add an optional parameter in the setup, which asks for the optional url to redirect the user's browser to, and suggest a /wiki/Special:OAuth url. If the user fills it out, we use that for the /authorize url. Otherwise we just use the one url they gave us?

I really think we should skip short urls as I think they'll cause more problem than they're worth.

@csteipp, @WikiChad: I thought the getAuthorizeTokenURI had to usee /wiki/ instead of /w/index.php?

In D9202#53, @20after4 wrote:

@csteipp, @WikiChad: I thought the getAuthorizeTokenURI had to usee /wiki/ instead of /w/index.php?

Only if we want it to work with mobile users.

If this were mediawiki, I'd suggest we do the initial implementation with a single url, and then we can add a separate patch later that adds another text input to the configuration screen that says something like, "Optionally specify a url that the user will be redirected to for authorization, e.g., the wiki's mobile-friendly, short url for Special:OAuth/authorize." and use that for authorization.

The other option is to refactor the phutil OAuth class to allow passing in extra, non-url parameters to be signed. That would simplify the Jira plugin, since they also have to work around that.

In D9202#54, @csteipp wrote:

The other option is to refactor the phutil OAuth class to allow passing in extra, non-url parameters to be signed. That would simplify the Jira plugin, since they also have to work around that.

^ I don't have time to do this, so I'm not volunteering... it's not too difficult, I just have too many other things going on.

20after4 edited edge metadata.

Rebased on a much more modern upstream (current trunk)

  • PhutilAuthAdapterOAuthMediaWiki becomes PhutilMediaWikiAuthAdapter due to new naming conventions that went into effect since this patch was created.

    Improved url handling:
  • normalize slashes in the wiki URLs
  • don't assume /w/ or /wiki/ - just append /index.php?title=$whatever to the base url
  • so now the user should include the /w part in the url field when configuring the provider.
  • this still doesn't handle mobile browsers, unfortunately, they will break because of some stupidity related to handling of pretty-urls in mediawiki.
20after4 edited edge metadata.

Normalize the expected uri in decodeAndVerifyJWT

  • this is needed because base url is now likely to contain a path that needs to be stripped before comparing the jwt returned by mediawiki.

Okay, this works for me. Stuff we should fix:

  • Remove /w/ and /wiki/ hardcodes, these do not work in the general case.

Done

  • Provide a getWikiURI() method or similar which normalizes slashes, so the user can enter http://wiki.domain.com or http://wiki.domain.com/ (with a trailing slash) when configuring an adapter.

Done, though user is now required to include to /w path part in the base url.

  • We should deal with the 200 + error responses. I think it's reasonable to provide a (default no-op) callback hook like willProcessTokenRequestResponse($raw_body). The parent can just return;, and the MW adapter can check if the response contains an error message. One way to trigger this is to intentionally break the signature by adding extra slashes to the URL.

I missed this one in the patch above. I'll add this shortly.

  • The setup process should more fully commit to being two-phase, like JIRA. In particular, the getProviderConfigurationHelp() method currently gives bad advice:

Screen_Shot_2014-06-30_at_6.45.43_AM.png (225×996 px, 33 KB)

Two things are wrong here:

  • The link points to the main MW install, but should point locally.
  • The URL is incorrect, because the user hasn't added chosen a name yet.

Instead, this shuld be fully two-phase:

  • In the setup phase, ask for a name and URI. Provide Help text like "step 1 of 2, continue...".
  • In the edit phase, ask for the keys. Provide help text with correct URIs.

This may be a bit messy, but the JIRA adapter is a fully two-phase OAuth1 adapter, so it should be possible.

Done

Then:

  • It would be helpful to mention that applications must be approved in the help text.

Done

  • It would be helpful to mention that MW must be running memcache, and the process will fail in a somewhat confusing/nonobvious way if it isn't, if this isn't already improved at HEAD in the OAuth extension.

It appears that the MediaWiki OAuth extension now properly returns an error message that mentions memcache in the relevant situations.

  • Added a hook to handle error messages in responses with non-error http status codes
epriestley edited edge metadata.

Per T5096, let's keep this out of the upstream until we see more interest from other installs. We're hesitant to bring features upstream if they'll be used by only one install.

This revision now requires changes to proceed.Nov 2 2015, 4:49 PM

This is now published as part of our wikimedia/phabricator-extensions repository. There isn't much demand (or need) for it to be in the upstream.