Fix bug in oauth. (#1949)

* Fix bug in oauth.

The error checking was too specific, and missed a case.
Make the error checking catch all exceptions.

  #1947

* Add logging when the cookie cannot be processed

server/auth/auth_oauth.py

@@ -218,22 +218,24 @@ class AuthTypeOAuth(AuthTypeClientBase):
 
         try:
             if self.session_cookie:
-                tokensdict = session.get(self.CXG_TOKENS)
-                if tokensdict:
-                    g.tokens = Tokens(**tokensdict)
+                value = session.get(self.CXG_TOKENS)
+                if value:
+                    g.tokens = Tokens(**value)
                 else:
                     return None
             else:
                 value = request.cookies.get(self.cookie_params["key"])
-                value = base64.b64decode(value)
-                try:
-                    tokensdict = json.loads(value)
-                    g.tokens = Tokens(**tokensdict)
-                except (TypeError, KeyError, json.decoder.JSONDecodeError):
-                    g.pop("tokens", None)
+                if value is None:
                     return None
+                value = base64.b64decode(value)
+                value = json.loads(value)
+                g.tokens = Tokens(**value)
 
-        except (TypeError, KeyError):
+        except Exception:
+            # there are many types of exceptions that can be raise in the above section.
+            # It is impractical to list all the exceptions here, since that would be brittle.
+            # If an exception occurs, then return None, meaning that no token could be retrieved.
+            current_app.logger.warning(f"auth cookie is in the wrong format: {str(value)}")
             g.pop("tokens", None)
             return None
 
@@ -331,6 +333,7 @@ class AuthTypeOAuth(AuthTypeClientBase):
 
         # if there is no id_token, return None (user is not authenticated)
         tokens = self.get_tokens()
+
         if tokens is None or tokens.id_token is None:
             return None
 

server/test/unit/auth/test_oauth.py

@@ -59,12 +59,7 @@ def logout():
 
 @mock_oauth_app.route("/.well-known/jwks.json")
 def jwks():
-    data = dict(
-        alg="RS256",
-        kty="RSA",
-        use="sig",
-        kid="fake_kid",
-    )
+    data = dict(alg="RS256", kty="RSA", use="sig", kid="fake_kid",)
     return make_response(jsonify(dict(keys=[data])))
 
 
@@ -152,18 +147,25 @@ class AuthTest(unittest.TestCase):
                 self.assertNotEqual(access_token_before, access_token_after)
                 self.assertNotEqual(id_token_before, id_token_after)
 
-                # invalid cookie fails
-                # (THIS CURRENTLY 500's ON THE SERVER, BUT SHOULD RETURN EMPTY USERINFO OR 401)
+                # invalid cookie is rejected
                 session.cookies.set(cookie_key, "TEST_" + cookie)
-                userinfo = session.get(f"{server}/d/pbmc3k.cxg/api/v0.2/userinfo").json()
-                self.assertIsNone(userinfo.get("userinfo"))
+                self.assertTrue(cookie_key in session.cookies)
+                response = session.get(f"{server}/d/pbmc3k.cxg/api/v0.2/userinfo")
+                # this is not an error, the invalid cookie is just ignored.
+                self.assertEqual(response.status_code, 200)
+                userinfo = response.json()
+                self.assertFalse(userinfo["userinfo"]["is_authenticated"])
+                self.assertIsNone(userinfo["userinfo"]["username"])
 
-                # invalid id_token fails
+                # invalid id_token is rejected
                 test_token = token
                 test_token["id_token"] = "TEST_" + id_token_after
                 encoded_cookie = base64.b64encode(json.dumps(test_token).encode()).decode()
                 session.cookies.set(cookie_key, encoded_cookie)
-                userinfo = session.get(f"{server}/d/pbmc3k.cxg/api/v0.2/userinfo").json()
+                response = session.get(f"{server}/d/pbmc3k.cxg/api/v0.2/userinfo")
+                # this is not an error, the invalid id_token is just ignored.
+                self.assertEqual(response.status_code, 200)
+                userinfo = response.json()
                 self.assertFalse(userinfo["userinfo"]["is_authenticated"])
                 self.assertIsNone(userinfo["userinfo"]["username"])
 
@@ -181,9 +183,7 @@ class AuthTest(unittest.TestCase):
         # test with session cookies
         app_config = AppConfig()
         app_config.update_server_config(app__flask_secret_key="secret")
-        app_config.update_server_config(
-            authentication__params_oauth__session_cookie=True,
-        )
+        app_config.update_server_config(authentication__params_oauth__session_cookie=True,)
         self.auth_flow(app_config)
 
     def test_auth_oauth_cookie(self):