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.

@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.

In D9202#12, @epriestley wrote:

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.

@epriestley, @csteipp, review?

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.

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

@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:

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...

Haha, that one was my fault.

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.

In D9202#44, @epriestley wrote:

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:

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.