Merge pull request #525 from bheesham/python-logging-use-log-exception

Use logging.exception when we care about traceback

dashboard/api/idp.py

@@ -93,23 +93,23 @@ def decorated(*args, **kwargs):
                         audience=self.audience,
                         issuer="https://" + self.auth0_domain + "/",
                     )
-                except jwt.ExpiredSignatureError as e:
-                    logger.error(e)
+                except jwt.ExpiredSignatureError:
+                    logger.exception("Token expired")
                     raise AuthError(
                         {"code": "token_expired", "description": "token is expired"},
                         401,
                     )
-                except jwt.JWTClaimsError as e:
-                    logger.error(e)
+                except jwt.JWTClaimsError:
+                    logger.exception("Claims error")
                     raise AuthError(
                         {
                             "code": "invalid_claims",
                             "description": "incorrect claims," "please check the audience and issuer",
                         },
                         401,
                     )
-                except Exception as e:
-                    logger.error(e)
+                except Exception:
+                    logger.exception("Some unknown error")
                     raise AuthError(
                         {
                             "code": "invalid_header",

dashboard/models/tile.py

@@ -50,12 +50,16 @@ def _etag(self):
         filename = os.path.join(this_dir, "../data/{name}").format(name="apps.yml-etag")
         try:
             with open(filename, "r") as f:
+                # What happens if we read nothing?
                 etag = f.read()
             return etag
-        except Exception as e:
-            """If the etag file is not found return a default etag."""
-            logger.warning("Error fetching etag: {e}".format(e=e))
-            # Return a fake ETag if etag file doesn't exist
+        # This can fail with
+        # * FileNotFoundError
+        # * PermissionError
+        # TODO(bhee): Do we want to be specific?
+        except Exception:
+            """If the etag file is not found return a fake etag."""
+            logger.exception("Error fetching etag")
             return "12345678"
 
     def _download_config(self):
@@ -67,8 +71,8 @@ def _download_config(self):
             response = http.request("GET", self.url)
             if response.status != 200:
                 raise HTTPError(f"HTTP request failed with status {response.status}")
-        except HTTPError as e:
-            logger.error("Request for apps.yml failed: %s", str(e))
+        except HTTPError as exc:
+            exc.add_note("Request for apps.yml failed")
             raise
 
         this_dir = os.path.dirname(__file__)
@@ -85,9 +89,8 @@ def _download_config(self):
                 # It is very important that the ETag file is written before we close
                 # apps.yml file. Otherwise, this may cause a reload loop
                 self._update_etag(response.headers["ETag"])
-        except Exception as e:
-            # Handle potential errors
-            logger.error("An error occurred while attempting to write apps.yml: %s", str(e))
+        except Exception as exc:
+            exc.add_note("An error occurred while attempting to write apps.yml")
             raise
 
     def _load_apps_yml(self):
@@ -105,16 +108,16 @@ def sync_config(self):
             if self.is_updated():
                 logger.info("Config file is updated fetching new config.")
                 self._download_config()
-        except Exception as e:
-            logger.error("Problem fetching config file {error}".format(error=e))
+        except Exception:
+            logger.exception("Problem fetching config file")
 
         # Load the apps.yml file into self.apps_list
         # if it isn't already loaded
         try:
             if not self.apps_yml:
                 self._load_apps_yml()
-        except Exception as e:
-            logger.error("Problem loading the config file {error}".format(error=e))
+        except Exception:
+            logger.exception("Problem loading the config file")
 
 
 class Tile(object):

dashboard/models/user.py

@@ -18,8 +18,8 @@ def __init__(self, session, app_config):
     def email(self):
         try:
             email = self.userinfo.get("email")
-        except Exception as e:
-            logger.error("The email attribute does no exists falling back to OIDC Conformant: {}.".format(e))
+        except Exception:
+            logger.exception("The email attribute does no exists falling back to OIDC Conformant.")
             email = self.userinfo.get("https://sso.mozilla.com/claim/emails")[0]["emails"]
         return email
 
@@ -92,7 +92,7 @@ def _is_valid_yaml(self, app):
             app["application"]["authorized_groups"]
             app["application"]["authorized_users"]
             return True
-        except Exception:
+        except KeyError:
             return False
 
 

dashboard/oidc_auth.py

@@ -97,17 +97,17 @@ def _verified(self):
                 self.jws_data["connection_name"] = self._get_connection_name(self.jws_data["connection"])
                 return True
         except UnicodeDecodeError:
-            logger.warning("UnicodeDecodeError: The jws {jws}".format(jws=self.jws))
+            logger.warning(f"UnicodeDecodeError: The jws {self.jws}")
             return False
         except JWSErrors.DeserializationError:
-            logger.warning("DeserializationError jws {jws}".format(jws=self.jws))
+            logger.warning(f"DeserializationError jws {self.jws}")
             return False
         except Exception:  # pylint: disable=broad-exception-caught
             # This is a broad except to catch every error.  It's not great but since we're
             # in _validate, our job is to pass/fail everything, and letting code raise out
             # of here blows up the website in front of customers.  Let's do something better
             # as a last-choice, maybe we need more exceptions caught above
-            logger.warning("Unknown error occurred " + traceback.format_exc())
+            logger.exception("Unknown error occurred")
             return False
 
     def error_message(self):

dashboard/op/yaml_loader.py

@@ -17,9 +17,9 @@ def __init__(self, app_dict):
     def _load_data(self):
         try:
             stream = yaml.safe_load(self.app_dict)
-        except yaml.YAMLError as e:
+        except yaml.YAMLError:
+            logger.exception("Could not load YAML")
             stream = None
-            logger.info(e)
         return stream
 
     def _render_data(self):

dashboard/vanity.py

@@ -1,9 +1,12 @@
+import logging
 from flask import make_response
 from flask import redirect
 from flask import request
 
 from dashboard.op import yaml_loader
 
+logger = logging.getLogger()
+
 
 class Router(object):
     def __init__(self, app, app_list):
@@ -16,8 +19,8 @@ def setup(self):
                 try:
                     self.app.add_url_rule(vanity_url, vanity_url, self.redirect_url)
                     self.app.add_url_rule(vanity_url + "/", vanity_url + "/", self.redirect_url)
-                except Exception as e:
-                    print(e)
+                except Exception:
+                    logger.exception(f"Could not add {vanity_url}")
 
     def redirect_url(self):
         vanity_url = "/" + request.url.split("/")[3]