- Mentioned In
- rPHEX50a3ecc15d33: Fix code rot in PhabricatorMediaWikiAuthProvider/OAuthAdapter
T346: configure LDAP and SUL for phab instance
T463: Enable registration for everybody at phabricator.wikimedia.org
T174: Launch Wikimedia Phabricator Day 1
- Mentioned Here
- T294: Super epic: Refactor moveToStep into a smaller function, make controllers for each step
qgil wrote on 2014-08-25 20:27:49 (UTC)
Does this task need to be a blocker of T294: Super epic: Refactor moveToStep into a smaller function, make controllers for each step? We had agreed that it was fine to upstream our changes after Day 1, if needed.
Rush wrote on 2014-08-25 20:33:40 (UTC)
It is definitely a thing we should roll into upstream officially before relying it. Even now rolling forward in git is a big pain with it, and an oauth provider is not the kind of thing you want to maintain in parallel to upstream if you don't absolutely have to. Unless we can satisfy Evan's differential concerns about the extension I think it's not production ready, and if we do get the diff's accepted it seems foolish not to wait for it to be merged since that will be pretty immediate I expect?
mmodell wrote on 2014-09-07 21:22:21 (UTC)
At this point it's actually not upstreaming that's the issue, I've addressed all of Evan's concerns in a diff that should be ready to publish. The problem is... it no longer actually works to authenticate against mediawiki.. I think that the problem is the mediawiki oauth extension has changed a lot and is no longer compatible :-/
I'm still working through debugging it, which is finally progressing now that my mediawiki vagrant environment is updated and I have everything working again.
mmodell wrote on 2014-09-08 02:39:14 (UTC)
ok I made headway, it seems the real issue is that the ssl proxy in mediawiki vagrant is somehow interfering with the oauth handshake. This wasn't the case before I updated everything to the latest verrsion. I will investigate that issue further but in the meantime I've submitted the updated differential revisions upstream ...
Evan has said and repeated that he knows that we are waiting but he doesn't know when he is going to have time to review this patch.
Proposal: if by the end of next Monday we still have no ETA form him, then we swallow the pill and we enable it here, maintaining the local patch until it is merged upstream.
Monday begins in California...
Also, it would be great to be able to showcase the Wikimedia SUL and LDAP logins in our video showcase on Wednesday. A video that will be seen by many between Wednesday, Day 1 and after.
@Qgil I agree. Especially since the changes are only minimal and they simply address epriestley's feedback from the previous review. It's entirely likely that he will accept the patch with few other changes at this point.
I don't think there will be a lot of maintenance headaches with this one either.
And finally, I would feel a lot more comfortable with getting some serious testing of this, so far it's only been tested in a very synthetic environment and not an entirely realistic one.
We agreed that this task was not a blocker for Phabricator-Production-Instance anymore, but that we should keep it in mind with high priority.
The priority stays, and I'm relating the task to bugzilla-migration as a formal way to keep present it in our minds -- beyond @mmodell, that is.
One question: imagine that now this Oauth provider is upstreamed and ships with Phabricator. If another Phabricator instance decides to use it... what happens exactly? Their users can create an account using Wikimedia SUL credentials? Or can the Phabricator admins connect their Phabricator with their MediaWiki and allow SUL between both instances, without any connection with Wikimedia?
The OAuth Provider is now published as an extension in rPHES phabricator-Security. Anyone who wants to use the extension can easily install it. Phabricator auth APIs have been stable for over a year. Upstream has expressed little interest in this code and it's fairly niche / specialized for our use case.
Conclusion: there is really no need, and no more momentum, to upstream the code.