Page MenuHomePhabricator

Investigate using python-requests for OAuth
Closed, ResolvedPublic

Description

To add OAuth support to pywikibot, we need to add OAuth to httplib2 or allow and alternative http client such as python-requests.

This task is to evaluate the second alternative in more detail.

Event Timeline

jayvdb created this task.May 9 2015, 12:52 PM
jayvdb assigned this task to VcamX.
jayvdb raised the priority of this task from to Needs Triage.
jayvdb updated the task description. (Show Details)
jayvdb added subscribers: jayvdb, Aklapper.
Restricted Application added a subscriber: Unknown Object (MLST). · View Herald TranscriptMay 9 2015, 12:52 PM
VcamX added a comment.EditedMay 21 2015, 3:41 PM

@jayvdb This is the travis ci test report. I reviewed the patch and also tried to follow its idea but found it break some internal things. So I made the patch myself. I checkouted a new branch based on the commit from master branch, which showed itself pass the test on travis ci.

Test 7 and 8 failed because requests introduced some warning to stderr, which cause some assertIsNone(stderr_other) failed. So I think we could ignore this assertion error.

Test 12 failed because AssertionError: LockedPage not raised by save. I think this test is for non-sysop account. But pywikibot doesn't figure out my account is sysop, so not to skip this test, which cause saving a locked page succeed. This also showed on travis ci report for the base commit using my account.

I almost replaced httplib2 with requests. And I removed some tests for httplib2, also modified some test case to fit with requests lib. You could check those modification. Sorry about those small and messy commits. I could squash them If you want :)

XZise added a subscriber: XZise.May 21 2015, 4:27 PM

Hi @VcamX github allows you to compare your branch with the master branch relatively easy (at least if you rebase your changes only): https://github.com/wikimedia/pywikibot-core/compare/master...VcamX:requests

Regarding the error that LockedPage: The username and not sysop username is defined in the .travis.yml file so afaik all tests on Travis assume the test is not done by a sysop account. If you configure a sysop account in pywikibot as a normal user account it assumes that this account is not a sysop account. There is practically no reliable way to determine if your account is a sysop account which is why @jayvdb is working on a patch (or actually has submitted it already) which tries to avoid this difference (see also T71283). I guess noone is using a sysop account to be run on Travis which so we didn't noticed that yet. In theory you could change in your .travis.yml that it also defines sysopusernames although better would be to change that test to check if the configured user has the permission to edit that page.

Test 7 and 8 failed because requests introduced some warning to stderr, which cause some assertIsNone(stderr_other) failed. So I think we could ignore this assertion error.

I agree this assertion is very likely to not be a significant problem, however we need the tests to pass. ;-)

It looks like this is a problem only with the Python 2.6 builds, so any solution should use 'if sys.version_info < (2, 7):' so the 'solution' doesnt affect more modern python versions which dont need fixing.

There are three possible options that I can see quickly, which may or may not work:

  1. call urllib3.disable_warnings() somewhere to prevent this warning
  2. try to fix the security problem described by this warning, e.g. by installing root certificates - https://urllib3.readthedocs.org/en/latest/security.html#certifi-with-urllib3
  3. silence this specific warning only. I believe it is being captured by the logging layer, and is being routed through pywikibot/userinterfaces/terminal_interface_base.py TerminalHandler.emit , so you may be able to detect and suppress this warning in there.
XZise added a comment.May 21 2015, 5:24 PM

Well according to https://urllib3.readthedocs.org/en/latest/security.html#insecureplatformwarning it only affects versions below 2.7.9 and Travis is using 2.7.9 so only the 2.6 tests have that problem.

In your setup.py, you require 'requests >=2, <3' , but pwb.py reports 'Python module requests >= 2.7.0 is required.'

Does requests pre 2.7 not include __version__?

VcamX added a comment.May 22 2015, 5:36 AM

@XZise I tried to run that test locally. It's under tests/edit_failure_tests.py. I'm using the same user-config.py as travis ci test. It showed that it skipped all 3 tests. That means pywikibot figure out I'm using a sysop account. It's confusing.

@jayvdb The latest version passed the test. With the help of @XZise, I also sent a patch to improve test_protected of TestSaveFailure in tests/edit_failure_tests.py

The test results are looking good. Could you please put a patch up for review in Gerrit. There are a large group of the devs together at a Hackathon, who can review the patch.

The one area which isnt covered by tests is async_request . Either we need to create tests for that, or we need to manually verify that works correctly after this patch.

Change 213977 had a related patch set uploaded (by VcamX):
Switch from httplib2 to requests

https://gerrit.wikimedia.org/r/213977

VcamX added a comment.May 27 2015, 4:18 AM

@jayvdb The patch is on Gerrit now.

The async_request seems a little hard to test..

The patch needs an update to pass jenkins rules.

Change 213977 merged by jenkins-bot:
Switch from httplib2 to requests

https://gerrit.wikimedia.org/r/213977

jayvdb closed this task as Resolved.Jun 2 2015, 5:00 PM
jayvdb moved this task from Backlog to requests on the Pywikibot-OAuth board.Jun 5 2015, 8:16 AM