improve login error handling (#404)

ptpt · web-flow · commit fb8ac0eebf5b · 2021-06-11T12:51:33.000+02:00
diff --git a/mapillary_tools/api_v3.py b/mapillary_tools/api_v3.py
@@ -1,8 +1,6 @@
 import os
 import requests
 
-from .error import print_error
-
 API_ENDPOINT = os.getenv("API_PROXY_HOST", "https://a.mapillary.com")
 MAPILLARY_WEB_CLIENT_ID = os.getenv(
     "MAPILLARY_WEB_CLIENT_ID", "MkJKbDA0bnZuZlcxeTJHTmFqN3g1dzo1YTM0NjRkM2EyZGU5MzBh"
@@ -38,8 +36,6 @@ def get_upload_token(mail, pwd):
         params={"client_id": MAPILLARY_WEB_CLIENT_ID},
         json=payload,
     )
-    if resp.status_code == 401:
-        return None
     resp.raise_for_status()
     return resp.json().get("token")
 
@@ -52,6 +48,5 @@ def get_user_key(user_name):
     resp.raise_for_status()
     resp = resp.json()
     if not resp or "key" not in resp[0]:
-        print_error(f"Error, user name {user_name} does not exist...")
-        return None
+        raise RuntimeError(f"Error, user name {user_name} does not exist...")
     return resp[0]["key"]
diff --git a/mapillary_tools/edit_config.py b/mapillary_tools/edit_config.py
@@ -100,9 +100,5 @@ def edit_config(
     else:
         # fill in the items and save
         user_items = login.prompt_user_for_user_items(user_name)
-        if not user_items:
-            raise RuntimeError(
-                f"Authentication failed for username {user_name}, please try again."
-            )
 
     config.update_config(config_file, user_name, user_items)
diff --git a/mapillary_tools/login.py b/mapillary_tools/login.py
@@ -1,29 +1,54 @@
 import getpass
 import os
-from typing import Optional, Dict
+
+import requests
 
 from . import MAPILLARY_API_VERSION, api_v3, api_v4, config
 from .config import GLOBAL_CONFIG_FILEPATH
+from .error import print_error
+
+
+class HTTPError(Exception):
+    pass
+
 
+def wrap_http_exception(ex: requests.HTTPError):
+    resp = ex.response
+    lines = [
+        f"{ex.request.method} {resp.url}",
+        f"> HTTP Status: {ex.response.status_code}",
+        f"{ex.response.text}",
+    ]
+    return HTTPError("\n".join(lines))
 
-def prompt_user_for_user_items(user_name: str) -> Optional[dict]:
-    print(f"Enter user credentials for user {user_name}:")
-    user_email = input("Enter email: ")
-    user_password = getpass.getpass("Enter user password: ")
 
-    if MAPILLARY_API_VERSION == "v3":
-        user_key = api_v3.get_user_key(user_name)
-        if not user_key:
-            return None
-        upload_token = api_v3.get_upload_token(user_email, user_password)
-    else:
-        assert MAPILLARY_API_VERSION == "v4"
-        data = api_v4.get_upload_token(user_email, user_password)
-        upload_token = data.get("access_token")
-        user_key = data.get("user_id")
+def prompt_user_for_user_items(user_name: str) -> dict:
+    print(f"Sign in for user {user_name}")
+    user_email = input("Enter your Mapillary user email: ")
+    user_password = getpass.getpass("Enter Mapillary user password: ")
 
-    if not upload_token:
-        return None
+    try:
+        if MAPILLARY_API_VERSION == "v3":
+            user_key = api_v3.get_user_key(user_name)
+            upload_token = api_v3.get_upload_token(user_email, user_password)
+        else:
+            assert MAPILLARY_API_VERSION == "v4"
+            data = api_v4.get_upload_token(user_email, user_password)
+            upload_token = data.get("access_token")
+            user_key = data.get("user_id")
+    except requests.HTTPError as ex:
+        if 400 <= ex.response.status_code < 500:
+            resp = ex.response.json()
+            subcode = resp.get("error", {}).get("error_subcode")
+            if subcode in [1348028, 1348092, 3404005]:
+                title = resp.get("error", {}).get("error_user_title")
+                message = resp.get("error", {}).get("error_user_msg")
+                print_error(f"{title}: {message}")
+                return prompt_user_for_user_items(user_name)
+            else:
+                raise wrap_http_exception(ex)
+        else:
+            raise wrap_http_exception(ex)
 
     return {
         "MAPSettingsUsername": user_name,
@@ -32,15 +57,13 @@ def prompt_user_for_user_items(user_name: str) -> Optional[dict]:
     }
 
 
-def authenticate_user(user_name: str) -> Optional[Dict]:
+def authenticate_user(user_name: str) -> dict:
     if os.path.isfile(GLOBAL_CONFIG_FILEPATH):
         global_config_object = config.load_config(GLOBAL_CONFIG_FILEPATH)
         if user_name in global_config_object.sections():
             return config.load_user(global_config_object, user_name)
 
     user_items = prompt_user_for_user_items(user_name)
-    if not user_items:
-        return None
 
     config.create_config(GLOBAL_CONFIG_FILEPATH)
     config.update_config(GLOBAL_CONFIG_FILEPATH, user_name, user_items)
diff --git a/mapillary_tools/process_upload_params.py b/mapillary_tools/process_upload_params.py
@@ -68,21 +68,7 @@ def process_upload_params(
         sys.exit(1)
 
     if not master_upload:
-        try:
-            credentials = login.authenticate_user(user_name)
-        except:
-            print_error("Error, user authentication failed for user " + user_name)
-            processing.create_and_log_process_in_list(
-                process_file_list, "upload_params_process" "failed", verbose
-            )
-            sys.exit(1)
-        if credentials is None:
-            print_error("Error, user authentication failed for user " + user_name)
-            processing.create_and_log_process_in_list(
-                process_file_list, "upload_params_process" "failed", verbose
-            )
-            sys.exit(1)
-
+        credentials = login.authenticate_user(user_name)
         user_upload_token = credentials["user_upload_token"]
         user_key = credentials["MAPSettingsUserKey"]
 
diff --git a/mapillary_tools/processing.py b/mapillary_tools/processing.py
@@ -1050,27 +1050,21 @@ def user_properties(
     verbose: bool = False,
 ) -> Optional[Dict]:
     # basic
-    user_properties = login.authenticate_user(user_name)
-    if not user_properties:
-        print_error("Error, user authentication failed for user " + user_name)
-        print(
-            "Make sure your user credentials are correct, user authentication is required for images to be uploaded to Mapillary."
-        )
-        return None
+    user_items = login.authenticate_user(user_name)
     # organization validation
     if organization_username or organization_key:
         organization_key = process_organization(
-            user_properties, organization_username, organization_key, private
+            user_items, organization_username, organization_key, private
         )
-        user_properties.update(
+        user_items.update(
             {"MAPOrganizationKey": organization_key, "MAPPrivate": private}
         )
 
     # remove uneeded credentials
-    if "user_upload_token" in user_properties:
-        del user_properties["user_upload_token"]
+    if "user_upload_token" in user_items:
+        del user_items["user_upload_token"]
 
-    return user_properties
+    return user_items
 
 
 def process_organization(
diff --git a/mapillary_tools/uploader.py b/mapillary_tools/uploader.py
@@ -19,7 +19,7 @@
 from . import upload_api_v3
 from . import upload_api_v4
 from . import ipc
-from .login import authenticate_user
+from .login import authenticate_user, wrap_http_exception
 from .utils import force_decode
 from . import MAPILLARY_API_VERSION
 
@@ -339,6 +339,7 @@ def get_organization_key(user_key, organization_username, upload_token):
         print("Available organization user names for current user are : ")
         print(organization_usernames)
         sys.exit(1)
+
     return organization_key
 
 
@@ -539,8 +540,6 @@ def _read_captured_at(path):
         print(f"Found the compressed sequence file {sequence_zip_path}")
 
     credentials = authenticate_user(user_name)
-    if credentials is None:
-        raise RuntimeError(f"Unable to find credentials for user {user_name}")
 
     service = upload_api_v4.UploadService(credentials["user_upload_token"])
 
@@ -550,10 +549,10 @@ def _read_captured_at(path):
         upload_resp = service.upload(sequence_uuid, data_size, fp)
     try:
         upload_resp.raise_for_status()
-    except requests.RequestException as ex:
+    except requests.HTTPError as ex:
         for path in file_list:
             create_upload_log(path, "upload_failed")
-        raise RuntimeError(f"Upload server error: {ex.response.text}")
+        raise wrap_http_exception(ex)
 
     upload_resp_json = upload_resp.json()
     try:
@@ -567,10 +566,10 @@ def _read_captured_at(path):
     finish_resp = service.finish(file_handle)
     try:
         finish_resp.raise_for_status()
-    except requests.RequestException as ex:
+    except requests.HTTPError as ex:
         for path in file_list:
             create_upload_log(path, "upload_failed")
-        raise RuntimeError(f"Upload server error: {ex.response.text}")
+        raise wrap_http_exception(ex)
 
     finish_data = finish_resp.json()
     cluster_id = finish_data.get("cluster_id")
@@ -613,8 +612,6 @@ def upload_file_list_manual(
     private = first_image.get("private", False)
 
     credentials = authenticate_user(user_name)
-    if credentials is None:
-        raise RuntimeError(f"Unable to find credentials for user {user_name}")
 
     upload_options = {
         "token": credentials["user_upload_token"],
