From 910ffddd1b375014d14f75e96e26629f6de70237 Mon Sep 17 00:00:00 2001 From: Cedric Brandily Date: Fri, 27 Feb 2015 16:17:35 +0100 Subject: [PATCH] Support authentication in run_http_exc Authentication could be required when performing REST API authenticated queries over http(s) (done by run_http_exc). Typically, it appends with gerrit behind apache2. This change queries git credential for gerrit password when http request returns a 401. Change-Id: Iad60eea938c42210ba8c5df4a1b76f8d2f4dd76d --- doc/source/installation.rst | 5 ++++ git-review.1 | 7 ++++++ git_review/cmd.py | 32 ++++++++++++++++++------ git_review/tests/test_unit.py | 46 +++++++++++++++++++++++++++++++++++ 4 files changed, 83 insertions(+), 7 deletions(-) diff --git a/doc/source/installation.rst b/doc/source/installation.rst index 0247ce5..b290046 100644 --- a/doc/source/installation.rst +++ b/doc/source/installation.rst @@ -60,6 +60,11 @@ defaultremote (default: gerrit). * You can specify different values to be used as defaults in ~/.config/git-review/git-review.conf or /etc/git-review/git-review.conf. +* Git-review will query git credential system for gerrit user/password when + authentication failed over http(s). Unlike git, git-review does not persist + gerrit user/password in git credential system for security purposes and git + credential system configuration stays under user responsibility. + Hooks ===== diff --git a/git-review.1 b/git-review.1 index 195577f..237212b 100644 --- a/git-review.1 +++ b/git-review.1 @@ -235,6 +235,13 @@ If you want all output to use color If you wish not to use color for any output. (default with Git older than 1.8.4) .El .El +.Pp +.Nm +will query git credential system for gerrit user/password when +authentication failed over http(s). Unlike git, +.Nm +does not persist gerrit user/password in git credential system for security +purposes and git credential system configuration stays under user responsibility. .Sh FILES To use .Nm diff --git a/git_review/cmd.py b/git_review/cmd.py index 8a17b4c..1d082a1 100755 --- a/git_review/cmd.py +++ b/git_review/cmd.py @@ -120,7 +120,7 @@ def build_review_number(review, patchset): return review -def run_command_status(*argv, **env): +def run_command_status(*argv, **kwargs): if VERBOSE: print(datetime.datetime.now(), "Running:", " ".join(argv)) if len(argv) == 1: @@ -129,17 +129,21 @@ def run_command_status(*argv, **env): argv = shlex.split(argv[0].encode('utf-8')) else: argv = shlex.split(str(argv[0])) + stdin = kwargs.pop('stdin', None) newenv = os.environ.copy() - newenv.update(env) - p = subprocess.Popen(argv, stdout=subprocess.PIPE, - stderr=subprocess.STDOUT, env=newenv) - (out, nothing) = p.communicate() + newenv.update(kwargs) + p = subprocess.Popen(argv, + stdin=subprocess.PIPE if stdin else None, + stdout=subprocess.PIPE, + stderr=subprocess.STDOUT, + env=newenv) + (out, nothing) = p.communicate(stdin) out = out.decode('utf-8', 'replace') return (p.returncode, out.strip()) -def run_command(*argv, **env): - (rc, output) = run_command_status(*argv, **env) +def run_command(*argv, **kwargs): + (rc, output) = run_command_status(*argv, **kwargs) return output @@ -154,6 +158,15 @@ def run_command_exc(klazz, *argv, **env): return output +def git_credentials(klazz, url): + """Get credentials using git credential.""" + cmd = 'git', 'credential', 'fill' + stdin = 'url=%s' % url + out = run_command_exc(klazz, *cmd, stdin=stdin) + data = dict(l.split('=', 1) for l in out.splitlines()) + return data['username'], data['password'] + + def http_code_2_return_code(code): """Tranform http status code to system return code.""" return (code - 301) % 255 + 1 @@ -173,6 +186,11 @@ def run_http_exc(klazz, url, **env): try: res = requests.get(url, **env) + if res.status_code == 401: + env['auth'] = git_credentials(klazz, url) + res = requests.get(url, **env) + except klazz: + raise except Exception as err: raise klazz(255, str(err), ('GET', url), env) if not 200 <= res.status_code < 300: diff --git a/git_review/tests/test_unit.py b/git_review/tests/test_unit.py index 6e41aa1..30c4fc4 100644 --- a/git_review/tests/test_unit.py +++ b/git_review/tests/test_unit.py @@ -168,6 +168,14 @@ class FakeException(Exception): self.code = code +FAKE_GIT_CREDENTIAL_FILL = """\ +protocol=http +host=gerrit.example.com +username=user +password=pass +""" + + class GitReviewUnitTest(testtools.TestCase): """Class for misc unit tests.""" @@ -190,3 +198,41 @@ class GitReviewUnitTest(testtools.TestCase): except FakeException as err: self.assertEqual(255, err.code) mock_get.assert_called_once_with(url) + + @mock.patch('git_review.cmd.run_command_exc') + @mock.patch('requests.get', return_value=FakeResponse(200)) + def test_run_http_exc_without_auth(self, mock_get, mock_run): + url = 'http://user@gerrit.example.com' + + cmd.run_http_exc(FakeException, url) + self.assertFalse(mock_run.called) + mock_get.assert_called_once_with(url) + + @mock.patch('git_review.cmd.run_command_exc', + return_value=FAKE_GIT_CREDENTIAL_FILL) + @mock.patch('requests.get', + side_effect=[FakeResponse(401), FakeResponse(200)]) + def test_run_http_exc_with_auth(self, mock_get, mock_run): + url = 'http://user@gerrit.example.com' + + cmd.run_http_exc(FakeException, url) + mock_run.assert_called_once_with(mock.ANY, 'git', 'credential', 'fill', + stdin='url=%s' % url) + calls = [mock.call(url), mock.call(url, auth=('user', 'pass'))] + mock_get.assert_has_calls(calls) + + @mock.patch('git_review.cmd.run_command_exc', + return_value=FAKE_GIT_CREDENTIAL_FILL) + @mock.patch('requests.get', return_value=FakeResponse(401)) + def test_run_http_exc_with_failing_auth(self, mock_get, mock_run): + url = 'http://user@gerrit.example.com' + + try: + cmd.run_http_exc(FakeException, url) + self.fails('Exception expected') + except FakeException as err: + self.assertEqual(cmd.http_code_2_return_code(401), err.code) + mock_run.assert_called_once_with(mock.ANY, 'git', 'credential', 'fill', + stdin='url=%s' % url) + calls = [mock.call(url), mock.call(url, auth=('user', 'pass'))] + mock_get.assert_has_calls(calls)