Create a phabricator authentication provider that uses mediawiki oauth
Closed, ResolvedPublic

Description

The Mediawiki oauth provider for Phabricator is functional and heavily used by Wikimedia developers on https://phabricator.wikimedia.org

You can download the source code from https://github.com/wikimedia/phabricator-extensions

It no longer seems necessary or particularly useful to have this live in the upstream.

20after4 created this task.May 18 2014, 6:24 AM
20after4 updated the task description. (Show Details)
20after4 raised the priority of this task from to Normal.
20after4 claimed this task.
20after4 added a project: Wikimedia.
20after4 moved this task to Important on the Wikimedia board.
20after4 changed the edit policy from "All Users" to "Wikimedia (Project)".

Cool, sounds good to me.

You can check out D9004 and D9019 for recent changes which added an OAuth provider (in that case, Wordpress).

(The construction of a separate Future class in D9004 isn't really necessary, but will make it easier for Phabricator to make API calls later, if that's something you might want to do.)

20after4 edited this Maniphest Task.May 19 2014, 7:15 PM

I've updated both D9202 and D9203 to be much more robust and configurable. This task should be nearly finished, just pending code review.

epriestley edited this Maniphest Task.Jun 28 2014, 3:27 AM
epriestley edited this Maniphest Task.Jun 28 2014, 12:00 PM
aklapper updated the task description. (Show Details)Aug 20 2014, 4:25 PM
aklapper added a subscriber: aklapper.

Oh, oops -- I updated D9203, they're both blocked on feedback in D9202 which is aggregated there to theoretically increase clarity. I basically just went through the thing and then wrote all my feedback on D9202, the two changes are pretty closely linked.

FYI: I'm still working on this. I've addressed all of the feedback in D9202 but now there is a bug somewhere.

Between the upstream changes in phabricator (which I rebased onto and adjusted as needed) combined with recent changes in mediawiki's OAuth implementation, something caused it to stop working. Now mediawiki doesn't get the consumer key with the request. I'm still debugging that issue and I'll submit updated differential revisions for D9202 and D9203 once everything is working (hopefully very soon)

qgil moved this task from Important to Potential blockers on the Wikimedia board.Sep 11 2014, 5:25 PM

@epriestley: This is now tested against mediawiki.org and both patches should be ready for review. I believe I have addressed all of your concerns and suggestions; everything should be good to go as soon as you have a chance to review.

@rush898 / @aklapper / @qgil / @20after4: all four of you have pinged me about this, it's on my radar. I'm not sure when I'll be able to get to it.

Unknown Object (User) moved this task from Potential blockers to Important on the Wikimedia board.Oct 9 2014, 8:48 AM
revi added a subscriber: revi.Oct 10 2014, 8:20 PM
qgil moved this task from Potential blockers to Important on the Wikimedia board.Nov 24 2014, 10:51 PM

What was the user in "Unknown Object (User) moved this task to Important on the Wikimedia workboard", https://secure.phabricator.com/T5096#78907?

A user who later requested that their account be deleted.

qgil added a comment.Dec 19 2014, 8:13 PM

If I recall correctly, a user that probably wanted to test a workboard and did so with ours, moving this task and a few others.

eadler added a subscriber: eadler.Apr 30 2015, 4:05 AM
20after4 added a comment.EditedMay 7 2015, 9:38 AM

@epriestley: What do you think we should do, abandon the diffs or keep'em? If you don't want them it's ok, really! (also no rush to respond, this is no longer an urgent priority for me so don't spend too much time thinking about it, I know you're busy)

tldr;

status update on this:

I've cleaned up the code a bit further and moved it out of our downstream fork, and into our Phabricator/extensions repo instead - this is much cleaner since it avoids splitting the provider and the adapter between phab and libphutil. It also avoids rebasing the patch every time we upgrade phabricator.

If upstream is still interested in this I can make the effort to revise the patches and clean things up further. It doesn't seem like there is much demand though, and 3rd parties can always grab the provider classes from our extensions repo if they want to use mediawik with phab in the same way that we are.

20after4 moved this task from Important to In Progress on the Wikimedia board.May 7 2015, 9:39 AM
20after4 closed this task as Resolved.Oct 23 2015, 9:38 AM

I guess having it exist on github is good enough.

Since we haven't seen any other interest in this in the meantime (although maybe you have?), keeping it external is probably best for us -- it's conceptually something that we're comfortable with, but it's conceivable that you're approximately the only install using it. We'd be open to bringing it upstream in the future if there's more widespread interest/use.

(We did eventually upstream phutil_hashes_are_identical() which might let you simplify the implementation very slightly.)

20after4 added a comment.EditedDec 2 2015, 3:09 AM

@epriestley: The main motivation was that we had been maintaining these as cherry-picked patches on top of phabricator core, which required constant rebasing. It was ridiculous and not even necessary. Eventually I figured out that phabricator doesn't care if the auth providers live in an external library. I sure wish I'd figured that out in the beginning, it would have made things a lot easier ;)

I'm actually embarrassed to admit that a lot of time was wasted on needless rebasing, but now I'm much happier with everything living in an extension, our phabricator "fork" is 99.9% vanilla. The only changes in our fork are the favicon.ico and robots.txt.

20after4 updated the task description. (Show Details)Dec 2 2015, 3:13 AM

Aaah! That makes sense.

Adding New Classes in the docs got updated somewhat-recently-ish and is hopefully a little more clear about things now (although maybe it could still be better, it's not super explicit on this still).