Provide TokenAuth class to requests to ensure explicit usage #800

electrofelix · 2018-03-20T14:21:26Z

Requests will default to reading .netrc files when auth is set to
'None', therefore it's necessary to provide a callable that returns
what is given with nothing extra set as the value for auth to ensure it
will use the token given rather than reading from a .netrc file.

To reproduce, add a token for one user to ~/.netrc and attempt
to authenticate with a token for another user using this library and
calling me() on the returned object will describe the user in ~/.netrc
rather than the user of the token used with this library.

electrofelix · 2018-03-20T14:40:00Z

To test this requires performing real requests to the Github API with multiple users and it's not clear that this is feasible?

You can also reproduce this by creating two tokens with different scopes, the first added to .netrc should only have access to 'public_repo' and nothing else, and the second should have access to 'read:user'. If you insert the first token into .netrc and use the second token with the following you'll get an exception rather than a successful result.

.netrc

machine api.github.com
login <user>
password <token1>

GITHUB_TOKEN="<token2>" python -c 'import logging
import os

import github3 as github


logging.basicConfig(level=logging.DEBUG)
g_admin = github.login(token=os.environ["GITHUB_TOKEN"])
print(g_admin.me())'

Without this change the output is:

INFO:github3:Building a url from ('https://api.github.com', 'user')
INFO:github3:Missed the cache building the url
DEBUG:github3:GET https://api.github.com/user with {}
DEBUG:urllib3.connectionpool:Starting new HTTPS connection (1): api.github.com
DEBUG:urllib3.connectionpool:https://api.github.com:443 "GET /user HTTP/1.1" 200 None
INFO:github3:JSON was returned
Traceback (most recent call last):
  File "<string>", line 9, in <module>
  File "github3/decorators.py", line 30, in auth_wrapper
    return func(self, *args, **kwargs)
  File "github3/github.py", line 1019, in me
    return self._instance_or_null(users.AuthenticatedUser, json)
  File "github3/models.py", line 146, in _instance_or_null
    return instance_class(json, self)
  File "github3/models.py", line 50, in __init__
    raise exceptions.IncompleteResponse(json, kerr)
github3.exceptions.IncompleteResponse: None The library was expecting more data in the response (KeyError(u'disk_usage',)). Either GitHub modified it's response body, or your token is not properly scoped to retrieve this information.

With this change it works fine because it uses the token with the scope that can read the correct user information.

omgjlk · 2018-03-20T18:24:48Z

Nice catch on this bug. As for testing, would it simply be enough to create a .netrc file in the test with an invalid token in it, to ensure that the test using correct credentials doesn't fail?

electrofelix

@omgjlk I've not used pytest before and I'm not sure how to correctly use the tmpdir fixture to get an isolated test to use a .netrc file. Maybe you might have some suggestions on what I need to look at to get something along these lines working?

electrofelix · 2018-03-20T20:06:42Z

tests/integration/test_session.py

+            "password invalid_token_for_test_verification"
+        )
+
+        with mock.patch.dict('os.environ', {'HOME': tmpdir}):


So to test this behaviour I think I need the above tmpdir fixture, but enabling that seems to cause this class to lose it's self.gh attribute resulting in an error being thrown on self.token_login() above.

electrofelix · 2018-03-20T20:07:25Z

tests/integration/test_session.py

@@ -19,3 +21,27 @@ def _two_factor_auth():
                                        match_requests_on=match):
            r = self.session.get('https://api.github.com/users/sigmavirus24')
            assert r.status_code == 200
+
+    @pytest.fixture(autouse=True)


Is this the correct way to use this? I'm not familar with pytest but this doesn't seem to use it for only this test and has some side effects in breaking the other test in this class.

No. @pytest.fixture declares the function as a fixture. autouse=True tells it to run before every test function. It seems like you wanted a temporary directory fixture?

sigmavirus24 · 2018-03-21T00:14:18Z

github3/session.py

-        self.auth = None
+        # Requests requires a callable for 'auth' otherwise it
+        # will attempt to use any auth in .netrc
+        self.auth = lambda x: x


We could do this or we could set trust_env = False. Setting a "null auth" might affect our decorators that introspect the auth on a session.

Setting trust_env = False breaks use of http_proxy/https_proxy/no_proxy env settings, and would require then explicit handling of proxy env vars from this library down to python requests.

I wonder if it would make more sense to to treat our authentication better.

Should we have a proper auth handler for this, e.g.,

class TokenAuth(requests.auth.AuthBase): def __init__(self, token): self.token = token def __call__(self, request): request.headers['Authorization'] = 'token {}'.format(token)

That could negate this entirely.

sigmavirus24 · 2018-03-21T00:16:10Z

tests/integration/test_session.py

@@ -19,3 +21,27 @@ def _two_factor_auth():
                                        match_requests_on=match):
            r = self.session.get('https://api.github.com/users/sigmavirus24')
            assert r.status_code == 200
+
+    @pytest.fixture(autouse=True)


No. @pytest.fixture declares the function as a fixture. autouse=True tells it to run before every test function. It seems like you wanted a temporary directory fixture?

electrofelix · 2018-03-21T10:09:57Z

So I've got the tmpdir fixture working, but it seems that something in the test framework doesn't like proxies so I'm not able to record the responses correctly and test that I can inspect the auth token sent to confirm it's the one set rather than the one coming from the .netrc file.

electrofelix · 2018-03-21T10:38:35Z

Never mind, just realised I forgot I needed to set:

TOX_TESTENV_PASSENV="http_proxy https_proxy no_proxy HTTP_PROXY HTTPS_PROXY NO_PROXY" tox -e py27

in order to test with proxies ... doh!

electrofelix · 2018-03-21T11:18:13Z

And in better news, it seems this can be tested via a unit test by use of prepare_request which triggers the reading of .netrc/_netrc files when auth is set to None.

Undoing the change in github3/session.py will trigger a test failure without needing to record/replay a real request.

sigmavirus24 · 2018-03-21T11:38:21Z

tests/unit/test_github_session.py

@@ -156,6 +160,32 @@ def test_token_auth_does_not_use_falsey_values(self):
            s.token_auth(token)
            assert 'Authorization' not in s.headers

+    def test_token_auth_with_netrc_works(self):


I'm wondering why you're setting up self.tmpdir for the whole class when we only need it for this test.

Here's the documentation https://docs.pytest.org/en/latest/tmpdir.html

You don't need to import pytest, you can do

def test_token_auth_with_netrc_works(self, tmpdir): """Verify the behaviour of the library when a token is provided and netrc is present."""

For some reason that didn't work in the integration tests and I just copied across what did when moving it to a unit test, but seems to work fine for the unit tests.

sigmavirus24 · 2018-03-21T11:54:46Z

github3/session.py

-        self.auth = None
+        # Requests requires a callable for 'auth' otherwise it
+        # will attempt to use any auth in .netrc
+        self.auth = lambda x: x


I wonder if it would make more sense to to treat our authentication better.

Should we have a proper auth handler for this, e.g.,

class TokenAuth(requests.auth.AuthBase): def __init__(self, token): self.token = token def __call__(self, request): request.headers['Authorization'] = 'token {}'.format(token)

That could negate this entirely.

sigmavirus24 · 2018-03-21T16:11:51Z

tests/unit/test_github_session.py

+            assert 'Authorization' not in pr.headers
+
+    def test_token_auth_with_netrc_works(self, tmpdir):
+


Most tests in this file have a docstring, please add one here. It doesn't need to be one-line.

Sorry, will update

sigmavirus24 · 2018-03-21T16:12:22Z

tests/unit/test_github_session.py

@@ -146,15 +151,43 @@ def test_token_auth_disables_basic_auth(self):
        s = self.build_session()
        s.auth = ('foo', 'bar')
        s.token_auth('token goes here')
-        assert s.auth is None
+        assert s.auth != ('foo', 'bar')


How about another assertion, e.g.,

assert isinstance(s.auth, github3.session.TokenAuth)

Added, or did you want me to replace the existing assertion?

Provide a class based on BaseAuth that requests can all to set the required headers on any calls made to ensure that it only uses the given auth scheme. In the event that `auth` is set to None, requests will default to reading .netrc/_netrc files for any credentials set by the user. When given an auth token or password it's necessary to ensure that it will ignore any .netrc settings.

electrofelix force-pushed the token-auth-disables-netrc branch from 9ed058f to 7f6dd1c Compare March 20, 2018 14:33

omgjlk added the Bug label Mar 20, 2018

electrofelix force-pushed the token-auth-disables-netrc branch from 7f6dd1c to bc2a0ab Compare March 20, 2018 20:05

electrofelix commented Mar 20, 2018

View reviewed changes

sigmavirus24 requested changes Mar 21, 2018

View reviewed changes

electrofelix force-pushed the token-auth-disables-netrc branch from bc2a0ab to c5e102a Compare March 21, 2018 09:59

electrofelix force-pushed the token-auth-disables-netrc branch from c5e102a to a9ce2e5 Compare March 21, 2018 11:15

sigmavirus24 reviewed Mar 21, 2018

View reviewed changes

electrofelix force-pushed the token-auth-disables-netrc branch from a9ce2e5 to d829162 Compare March 21, 2018 12:40

electrofelix changed the title ~~Prevent use of .netrc files when setting token auth~~ Provide TokenAuth class to requests to ensure explicit usage Mar 21, 2018

sigmavirus24 reviewed Mar 21, 2018

View reviewed changes

electrofelix force-pushed the token-auth-disables-netrc branch from d829162 to 65eb7a2 Compare March 21, 2018 19:13

sigmavirus24 approved these changes Mar 22, 2018

View reviewed changes

sigmavirus24 merged commit aa50e04 into sigmavirus24:develop Mar 22, 2018

electrofelix deleted the token-auth-disables-netrc branch March 22, 2018 13:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Provide TokenAuth class to requests to ensure explicit usage #800

Provide TokenAuth class to requests to ensure explicit usage #800

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

		assert 'Authorization' not in pr.headers

		def test_token_auth_with_netrc_works(self, tmpdir):

Provide TokenAuth class to requests to ensure explicit usage #800

Provide TokenAuth class to requests to ensure explicit usage #800

Uh oh!

Conversation

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!