From 3b3eff70cae37c5f47a78ece64c8aa80742e2904 Mon Sep 17 00:00:00 2001 From: tamarrow Date: Thu, 3 Nov 2016 16:08:24 -0700 Subject: [PATCH] http: don't retry requests if response is 401 (#824) We were retrying requests with 401 so that we can use the response headers to determine type of authentication. Instead, always send request with auth if we already have credentials (from core.dcos_acs_token). This also fixes multi-part requests that currently break if we have to retry the request (since the stream gets opened by the initial request). --- cli/dcoscli/auth/main.py | 5 ++- cli/tests/unit/test_http_auth.py | 15 ++++---- dcos/errors.py | 2 +- dcos/http.py | 65 ++++++++++++++++---------------- 4 files changed, 45 insertions(+), 42 deletions(-) diff --git a/cli/dcoscli/auth/main.py b/cli/dcoscli/auth/main.py index 32e31e2..517c251 100644 --- a/cli/dcoscli/auth/main.py +++ b/cli/dcoscli/auth/main.py @@ -84,8 +84,9 @@ def _login(): # hit protected endpoint which will prompt for auth if cluster has auth try: - url = urllib.parse.urljoin(dcos_url, 'exhibitor/') - http.get(url) + endpoint = '/pkgpanda/active.buildinfo.full.json' + url = urllib.parse.urljoin(dcos_url, endpoint) + http.request_with_auth('HEAD', url) # if the user is authenticated, they have effectively "logged in" even if # they are not authorized for this endpoint except DCOSAuthorizationException: diff --git a/cli/tests/unit/test_http_auth.py b/cli/tests/unit/test_http_auth.py index 50b1d0f..26c0013 100644 --- a/cli/tests/unit/test_http_auth.py +++ b/cli/tests/unit/test_http_auth.py @@ -103,8 +103,9 @@ def test_request_with_bad_auth_basic(mock, req_mock, auth_mock): req_mock.return_value = mock with pytest.raises(DCOSException) as e: - http._request_with_auth(mock, "method", mock.url) - assert e.exconly().split(':')[1].strip() == "Authentication failed" + http.request_with_auth("method", mock.url) + msg = "Authentication failed. Please run `dcos auth login`" + assert e.exconly().split(':')[1].strip() == msg @patch('requests.Response') @@ -120,7 +121,7 @@ def test_request_with_bad_auth_acl(mock, req_mock, auth_mock): req_mock.return_value = mock with pytest.raises(DCOSException) as e: - http._request_with_auth(mock, "method", mock.url) + http.request_with_auth("method", mock.url) msg = "Your core.dcos_acs_token is invalid. Please run: `dcos auth login`" assert e.exconly().split(':', 1)[1].strip() == msg @@ -138,7 +139,7 @@ def test_request_with_bad_oauth(mock, req_mock, auth_mock): req_mock.return_value = mock with pytest.raises(DCOSException) as e: - http._request_with_auth(mock, "method", mock.url) + http.request_with_auth("method", mock.url) msg = "Your core.dcos_acs_token is invalid. Please run: `dcos auth login`" assert e.exconly().split(':', 1)[1].strip() == msg @@ -158,7 +159,7 @@ def test_request_with_auth_basic(mock, req_mock, auth_mock): mock2.status_code = 200 req_mock.return_value = mock2 - response = http._request_with_auth(mock, "method", mock.url) + response = http.request_with_auth("method", mock.url) assert response.status_code == 200 @@ -177,7 +178,7 @@ def test_request_with_auth_acl(mock, req_mock, auth_mock): mock2.status_code = 200 req_mock.return_value = mock2 - response = http._request_with_auth(mock, "method", mock.url) + response = http.request_with_auth("method", mock.url) assert response.status_code == 200 @@ -196,5 +197,5 @@ def test_request_with_auth_oauth(mock, req_mock, auth_mock): mock2.status_code = 200 req_mock.return_value = mock2 - response = http._request_with_auth(mock, "method", mock.url) + response = http.request_with_auth("method", mock.url) assert response.status_code == 200 diff --git a/dcos/errors.py b/dcos/errors.py index 077d809..577b08d 100644 --- a/dcos/errors.py +++ b/dcos/errors.py @@ -34,7 +34,7 @@ class DCOSAuthenticationException(DCOSHTTPException): self.response = response def __str__(self): - return "Authentication failed" + return "Authentication failed. Please run `dcos auth login`" class DCOSAuthorizationException(DCOSHTTPException): diff --git a/dcos/http.py b/dcos/http.py index b241142..52d9d80 100644 --- a/dcos/http.py +++ b/dcos/http.py @@ -17,7 +17,7 @@ lock = threading.Lock() DEFAULT_TIMEOUT = 5 -# only accessed from _request_with_auth +# only accessed from request_with_auth AUTH_CREDS = {} # (hostname, auth_scheme, realm) -> AuthBase() @@ -80,6 +80,15 @@ def _request(method, :rtype: Response """ + if 'headers' not in kwargs: + kwargs['headers'] = {'Accept': 'application/json'} + + verify = _verify_ssl(verify) + + # Silence 'Unverified HTTPS request' and 'SecurityWarning' for bad certs + if verify is not None: + silence_requests_warnings() + logger.info( 'Sending HTTP [%r] to [%r]: %r', method, @@ -119,17 +128,14 @@ def _request(method, return response -def _request_with_auth(response, - method, - url, - is_success=_default_is_success, - timeout=None, - verify=None, - **kwargs): +def request_with_auth(method, + url, + is_success=_default_is_success, + timeout=None, + verify=None, + **kwargs): """Try request (3 times) with credentials if 401 returned from server - :param response: requests.response - :type response: requests.Response :param method: method for the new Request object :type method: str :param url: URL for the new Request object @@ -146,6 +152,8 @@ def _request_with_auth(response, :rtype: requests.Response """ + response = _request(method, url, is_success, timeout, verify, **kwargs) + i = 0 while i < 3 and response.status_code == 401: parsed_url = urlparse(url) @@ -209,24 +217,24 @@ def request(method, :rtype: Response """ - if 'headers' not in kwargs: - kwargs['headers'] = {'Accept': 'application/json'} - - verify = _verify_ssl(verify) - - # Silence 'Unverified HTTPS request' and 'SecurityWarning' for bad certs - if verify is not None: - silence_requests_warnings() - + auth_token = config.get_config_val( + "core.dcos_acs_token", config.get_config()) + if auth_token is None: + auth = None + else: + auth = DCOSAcsAuth(auth_token) response = _request(method, url, is_success, timeout, - verify=verify, **kwargs) - - if response.status_code == 401: - response = _request_with_auth(response, method, url, is_success, - timeout, verify, **kwargs) + auth=auth, verify=verify, **kwargs) if is_success(response.status_code): return response + elif response.status_code == 401: + if auth_token is not None: + msg = ("Your core.dcos_acs_token is invalid. " + "Please run: `dcos auth login`") + raise DCOSException(msg) + else: + raise DCOSAuthenticationException(response) elif response.status_code == 403: raise DCOSAuthorizationException(response) elif response.status_code == 400: @@ -482,15 +490,8 @@ def _get_dcos_auth(auth_scheme, username, password, hostname): else: creds = _get_dcos_oauth_creds(dcos_url) - verify = _verify_ssl() - # Silence 'Unverified HTTPS request' and 'SecurityWarning' for bad cert - if verify is not None: - silence_requests_warnings() - url = urllib.parse.urljoin(dcos_url, 'acs/api/v1/auth/login') - # using private method here, so we don't retry on this request - # error here will be bubbled up to _request_with_auth - response = _request('post', url, json=creds, verify=verify) + response = _request('post', url, json=creds) if response.status_code == 200: token = response.json()['token']