fix(PR): helpful review comments

Author: frickjack

gen3/auth.py

@@ -13,13 +13,6 @@
 class Gen3AuthError(Exception):
     pass
 
-class Gen3CurlError(Exception):
-    """ Custom exception for capturing backoff and retry"""
-    pass
-
-class Gen3CurlStatusError(Gen3CurlError):
-    """ Custom exception for status > 399"""
-    pass
 
 def decode_token(token_str):
     """
@@ -35,7 +28,7 @@ def decode_token(token_str):
 
 def endpoint_from_token(token_str):
     """
-    Extract the endpoint from a JWT issuer - ex:
+    Extract the endpoint from a JWT issue ("iss" property)
     """
     info = decode_token(token_str)
     urlparts = urlparse(info["iss"])
@@ -75,8 +68,7 @@ def get_wts_endpoint(namespace=os.getenv("NAMESPACE", "default")):
 
 def get_wts_idps(namespace=os.getenv("NAMESPACE", "default")):
     resp = requests.get(get_wts_endpoint(namespace) + "/external_oidc")
-    if resp.status_code != 200:
-        raise Exception("non-200 status {} - {}".format(resp.status_code, resp.text))
+    resp.raise_for_status()
     return resp.json()
 
 def get_access_token_from_wts(namespace=os.getenv("NAMESPACE", "default"), idp="local"):
@@ -91,11 +83,6 @@ def get_access_token_from_wts(namespace=os.getenv("NAMESPACE", "default"), idp="
     resp = requests.get(auth_url)
     return _handle_access_token_response(resp, "token")
 
-def check_curl_status(res):
-    """Raise Gen3CurlError if status is not < 400"""
-    if res.status_code > 399:
-        raise Gen3CurlStatusError("Got status: " + str(res.status_code))
-
 
 def token_cache_file(key):
     """Compute the path to the access-token cache file"""
@@ -198,6 +185,7 @@ def __init__(self, endpoint=None, refresh_file=None, refresh_token=None, idp=Non
                     with open(refresh_file) as f:
                         file_data = f.read()
                     self._refresh_token = json.loads(file_data)
+                    assert "api_key" in self._refresh_token
                 except Exception as e:
                     raise ValueError(
                         "Couldn't load your refresh token file: {}\n{}".format(
@@ -301,27 +289,31 @@ def _get_auth_value(self):
 
 
     def curl(self, path, request=None, data=None):
-        """Curl the given endpoint - ex: gen3 curl /user/user"""
-        try:
-            if not request:
-                request = "GET"
-                if data:
-                    request = "POST"
-            json_data = data
-            output = None
-            if data and data[0] == "@":
-                with open(data[1:]) as f:
-                    json_data = f.read()
-            if request == "GET":
-                output = requests.get(self.endpoint + "/" + path, auth=self)
-            elif request == "POST":
-                output = requests.post(self.endpoint + "/" + path, json=json_data, auth=self)
-            elif request == "PUT":
-                output = requests.put(self.endpoint + "/" + path, json=json_data, auth=self)
-            elif request == "DELETE":
-                output = requests.delete(self.endpoint + "/" + path, auth=self)
-            else:
-                raise Exception("Invalid request type: " + request)
-            return output
-        except Exception as ex:
-            raise Gen3CurlError from ex
+        """
+        Curl the given endpoint - ex: gen3 curl /user/user.  Return requests.Response
+
+        Args:
+            path (str): path under the commons to curl (/user/user, /index/index, /authz/mapping, ...)
+            request (str in GET|POST|PUT|DELETE): default to GET if data is not set, else default to POST
+            data (str): json string or "@filename" of a json file
+        """
+        if not request:
+            request = "GET"
+            if data:
+                request = "POST"
+        json_data = data
+        output = None
+        if data and data[0] == "@":
+            with open(data[1:]) as f:
+                json_data = f.read()
+        if request == "GET":
+            output = requests.get(self.endpoint + "/" + path, auth=self)
+        elif request == "POST":
+            output = requests.post(self.endpoint + "/" + path, json=json_data, auth=self)
+        elif request == "PUT":
+            output = requests.put(self.endpoint + "/" + path, json=json_data, auth=self)
+        elif request == "DELETE":
+            output = requests.delete(self.endpoint + "/" + path, auth=self)
+        else:
+            raise Exception("Invalid request type: " + request)
+        return output

gen3/cli/__main__.py

@@ -20,7 +20,13 @@ def get(self):
 
 
 @click.group()
-@click.option("--auth", 'auth_config', default=os.getenv("GEN3_API_KEY", None), help="path to api key, or shorthand for key in ~/.gen3/, or idp: identifier if in workspace")
+@click.option("--auth", 'auth_config', default=os.getenv("GEN3_API_KEY", None), 
+    help="""authentication source: 
+    "idp://wts/<idp>" is an identity provider in a Gen3 workspace,
+    "accesstoken:///<token>" is an access token,
+    otherwise a path to an api key or basename of key under ~/.gen3/;
+    default value is "credentials" if ~/.gen3/credentials.json exists, otherwise "idp://wts/local"
+    """)
 @click.option("--endpoint", 'endpoint', default=os.getenv("GEN3_ENDPOINT", "default"), help="commons hostname - optional if API Key given")
 @click.pass_context
 def main(ctx=None, auth_config=None, endpoint=None):

gen3/cli/auth.py

@@ -56,12 +56,6 @@ def wts_list():
     """list the idp's available from the wts in a Gen3 workspace environment """
     print(json.dumps(auth_tool.get_wts_idps(), indent=2))
 
-@click.command()
-@click.pass_context
-def info(ctx):
-    """Info about the current authentication mechanism"""
-    print(ctx.obj["auth_factory"].get().get_access_token())
-
 @click.group()
 def auth():
     """Gen3 sdk auth commands"""

gen3/index.py

@@ -16,18 +16,23 @@ class Gen3Index:
     A class for interacting with the Gen3 Index services.
 
     Args:
+        endpoint (str): public endpoint for reading/querying indexd - only necessary if auth_provider not provided
         auth_provider (Gen3Auth): A Gen3Auth class instance or indexd basic creds tuple
 
     Examples:
         This generates the Gen3Index class pointed at the sandbox commons while
         using the credentials.json downloaded from the commons profile page.
 
         >>> auth = Gen3Auth(refresh_file="credentials.json")
-        ... sub = Gen3Submission(auth.endpoint, auth)
+        ... sub = Gen3Submission(auth)
 
     """
 
     def __init__(self, endpoint=None, auth_provider=None, service_location="index"):
+        # legacy interface required endpoint as 1st arg
+        if endpoint and isinstance(endpoint,Gen3Auth):
+            auth_provider = endpoint
+            endpoint = None
         if auth_provider and isinstance(auth_provider,Gen3Auth):
             endpoint = auth_provider.endpoint
         endpoint = endpoint.strip("/")

gen3/jobs.py

@@ -43,6 +43,7 @@ def __init__(self, endpoint=None, auth_provider=None, service_location="job"):
             service_location (str, optional): deployment location relative to the
                 endpoint provided
         """
+        # auth_provider legacy interface required endpoint as 1st arg
         auth_provider = auth_provider or endpoint
         endpoint = auth_provider.endpoint.strip("/")
         # if running locally, mds is deployed by itself without a location relative

gen3/metadata.py

@@ -9,6 +9,7 @@
 import sys
 
 from gen3.utils import append_query_params, DEFAULT_BACKOFF_SETTINGS
+from gen3.auth import Gen3Auth
 
 
 class Gen3Metadata:
@@ -19,17 +20,17 @@ class Gen3Metadata:
         This generates the Gen3Metadata class pointed at the sandbox commons while
         using the credentials.json downloaded from the commons profile page.
 
-        >>> auth = Gen3Auth(endpoint, refresh_file="credentials.json")
-        ... sub = Gen3Metadata(auth.endpoint, auth)
+        >>> auth = Gen3Auth(refresh_file="credentials.json")
+        ... sub = Gen3Metadata(auth)
 
     Attributes:
-        admin_endpoint (str): endpoint for admin functionality (Create/Update/Delete)
-        endpoint (str): public endpoint for reading/querying metadata
+        endpoint (str): public endpoint for reading/querying metadata - only necessary if auth_provider not provided
+        auth_provider (Gen3Auth): auth manager
     """
 
     def __init__(
         self,
-        endpoint,
+        endpoint=None,
         auth_provider=None,
         service_location="mds",
         admin_endpoint_suffix="-admin",
@@ -44,6 +45,12 @@ def __init__(
             service_location (str, optional): deployment location relative to the
                 endpoint provided
         """
+        # legacy interface required endpoint as 1st arg
+        if endpoint and isinstance(endpoint,Gen3Auth):
+            auth_provider = endpoint
+            endpoint = None
+        if auth_provider and isinstance(auth_provider,Gen3Auth):
+            endpoint = auth_provider.endpoint
         endpoint = endpoint.strip("/")
         # if running locally, mds is deployed by itself without a location relative
         # to the commons

gen3/submission.py

@@ -37,6 +37,7 @@ class Gen3Submission:
     """
 
     def __init__(self, endpoint=None, auth_provider=None):
+        # auth_provider legacy interface required endpoint as 1st arg
         self._auth_provider = auth_provider or endpoint
         self._endpoint = self._auth_provider.endpoint
 

gen3/wss.py

@@ -6,7 +6,6 @@
 from urllib.parse import urlparse
 
 from gen3.utils import append_query_params, DEFAULT_BACKOFF_SETTINGS
-from gen3.auth import Gen3CurlError, check_curl_status
 
 
 def wsurl_to_tokens(ws_urlstr):
@@ -20,23 +19,23 @@ def wsurl_to_tokens(ws_urlstr):
         raise Exception("invalid path {}".format(ws_urlstr))
     return (pathparts[0], "/".join(pathparts[1:]))
 
-@backoff.on_exception(backoff.expo, Gen3CurlError, **DEFAULT_BACKOFF_SETTINGS)
+@backoff.on_exception(backoff.expo, requests.HTTPError, **DEFAULT_BACKOFF_SETTINGS)
 def get_url(urlstr, dest_path):
     """Simple url fetch to dest_path with backoff"""
     res = requests.get(urlstr)
-    check_curl_status(res)
+    res.raise_for_status()
     if dest_path == "-":
         sys.stdout.write(res.text)
     else:
         with open(dest_path, 'wb') as f:
             f.write(res.content)
 
-@backoff.on_exception(backoff.expo, Gen3CurlError, **DEFAULT_BACKOFF_SETTINGS)
+@backoff.on_exception(backoff.expo, requests.HTTPError, **DEFAULT_BACKOFF_SETTINGS)
 def put_url(urlstr, src_path):
     """Simple put src_path to url with backoff"""
     with open(src_path, 'rb') as f:
         res = requests.put(urlstr, data=f)
-    check_curl_status(res)
+    res.raise_for_status()
 
 
 class Gen3WsStorage:
@@ -63,7 +62,7 @@ def __init__(
         """
         self._auth_provider = auth_provider
 
-    @backoff.on_exception(backoff.expo, Gen3CurlError, **DEFAULT_BACKOFF_SETTINGS)
+    @backoff.on_exception(backoff.expo, requests.HTTPError, **DEFAULT_BACKOFF_SETTINGS)
     def upload_url(self, ws, wskey):
         """
         Get a upload url for the given workspace key
@@ -74,19 +73,19 @@ def upload_url(self, ws, wskey):
         """
         wskey = wskey.lstrip("/")
         res = self._auth_provider.curl("/ws-storage/upload/{}/{}".format(ws, wskey))
-        check_curl_status(res)
+        res.raise_for_status()
         return res.json()
 
 
-    @backoff.on_exception(backoff.expo, Gen3CurlError, **DEFAULT_BACKOFF_SETTINGS)
+    @backoff.on_exception(backoff.expo, requests.HTTPError, **DEFAULT_BACKOFF_SETTINGS)
     def upload(self, src_path, dest_ws, dest_wskey):
         """
         Upload a local file to the specified workspace path
         """
         url = self.upload_url(dest_ws, dest_wskey)["Data"]
         put_url(url, src_path)
 
-    @backoff.on_exception(backoff.expo, Gen3CurlError, **DEFAULT_BACKOFF_SETTINGS)
+    @backoff.on_exception(backoff.expo, requests.HTTPError, **DEFAULT_BACKOFF_SETTINGS)
     def download_url(self, ws, wskey):
         """
         Get a download url for the given workspace key
@@ -97,7 +96,7 @@ def download_url(self, ws, wskey):
         """
         wskey = wskey.lstrip("/")
         res = self._auth_provider.curl("/ws-storage/download/{}/{}".format(ws, wskey))
-        check_curl_status(res)
+        res.raise_for_status()
         return res.json()
 
 
@@ -128,7 +127,7 @@ def copy(self, src_urlstr, dest_urlstr):
             return self.upload(src_urlstr, pathparts[0], pathparts[1])
         raise Exception("source and destination may not both be local")
 
-    @backoff.on_exception(backoff.expo, Gen3CurlError, **DEFAULT_BACKOFF_SETTINGS)
+    @backoff.on_exception(backoff.expo, requests.HTTPError, **DEFAULT_BACKOFF_SETTINGS)
     def ls(self, ws, wskey):
         """
         List the contents under the given workspace path
@@ -139,7 +138,7 @@ def ls(self, ws, wskey):
         """
         wskey = wskey.lstrip("/")
         res = self._auth_provider.curl("/ws-storage/list/{}/{}".format(ws, wskey))
-        check_curl_status(res)
+        res.raise_for_status()
         return res.json()
 
 
@@ -155,7 +154,7 @@ def ls_path(self, ws_urlstr):
         pathparts = wsurl_to_tokens(ws_urlstr)
         return self.ls(pathparts[0], pathparts[1])
 
-    @backoff.on_exception(backoff.expo, Gen3CurlError, **DEFAULT_BACKOFF_SETTINGS)
+    @backoff.on_exception(backoff.expo, requests.HTTPError, **DEFAULT_BACKOFF_SETTINGS)
     def rm(self, ws, wskey):
         """
         Remove the given workspace key
@@ -166,7 +165,7 @@ def rm(self, ws, wskey):
         """
         wskey = wskey.lstrip("/")
         res = self._auth_provider.curl("/ws-storage/list/{}/{}".format(ws, wskey), request="DELETE")
-        check_curl_status(res)
+        res.raise_for_status()
         return res.json()
 
     def rm_path(self, ws_urlstr):

tests/test_auth.py

@@ -59,7 +59,6 @@ def _mock_request(url, **kwargs):
         mocked_response.json.return_value = {
             "token": access_token
         }
-        #mocked_response.raise_for_status.side_effect = lambda *args: None
         return mocked_response
 
     with patch("gen3.auth.requests") as mock_request: