Add README with security considerations and add TODOs

auth_external/README.md

@@ -0,0 +1,22 @@
+# Security considerations
+
+## Absence of method for token revocation
+
+See https://stackoverflow.com/a/40385939 
+TODO: should we support token revocation?
+
+Attack:
+The victim logs in through a shared machine to the translation platform.
+They leave the machine and forget to log out.
+The attacker extracts the JWT from the localStorage.
+The attacker now has the same capabilities as the victim, and this token cannot be revoked.
+
+## Indefinite validity of refresh_token
+IMPL1: In the current implementation, if an attacker obtains a valid refresh token, they can keep getting fresh access_tokens indefinitely. This is because to obtain a pair (fresh_access_token, fresh_refresh_token), it is only necessary to provide a valid refresh_token. An attacker can thus continuously refresh their refresh_token to keep indefinite access. This is not good because there is no revocation mechanism.
+IMPL2: alternate implementation: only refresh the access_token, not the refresh_token, so that the attacker loses access to the account after expiration of the refresh_token. To get a fresh refresh_token, it is necessary to login with (login, password, totp?)
+
+Attack possible in IMPL1 and not IMPL2:
+The victim logs in and their browser stores the refresh_token in the browser's storage.
+The victim's computer is infected by malware and their (still valid) refresh_token is exfiltrated to the attacker's machine.
+The attacker monitors all the victim's actions on odoo using the stolen access_token, refreshing the stolen refresh_token when necessary.
+This can continue indefinitely in IMPL1 but not in IMPL2, as the refresh_token eventually expires.

auth_external/models/res_users.py

@@ -3,6 +3,7 @@
 import re
 import secrets
 from datetime import datetime, timedelta
+from typing import Any, override
 
 from jwt import JWT, AbstractJWKBase, supported_key_types
 from jwt.exceptions import JWTDecodeError
@@ -34,6 +35,7 @@
 access_token_signing_key = supported_key_types()["oct"](global_secret_access)
 refresh_token_signing_key = supported_key_types()["oct"](global_secret_refresh)
 
+JWT_ALG = "HS256"
 
 class InvalidTotp(AccessDenied):
     pass
@@ -66,7 +68,7 @@ def _generate_jwt(
             "jti": "uuid",  # TODO: Generate token UID
         }
 
-        token = JWT().encode(payload, key, alg="HS256")
+        token = JWT().encode(payload, key, alg=JWT_ALG)
 
         return payload, token
 
@@ -114,6 +116,7 @@ def generate_external_auth_token(self, refresh_token=None):
         # Verification succeeded, we generate tokens.
 
         access_token_exp = datetime.now() + timedelta(0, access_token_duration)
+        # TODO: this is not good, it essentially makes refresh tokens have an infinite lifetime
         refresh_token_exp = datetime.now() + timedelta(0, refresh_token_duration)
 
         payload, new_token = self._generate_jwt(
@@ -124,6 +127,7 @@ def generate_external_auth_token(self, refresh_token=None):
             access_token_signing_key,
         )
 
+        # TODO : if the refresh_token is provided, DO NOT generate a new refresh_token to allow it to expire
         refresh_payload, new_refresh_token = self._generate_jwt(
             issuer_id,
             self.env.user.id,
@@ -188,7 +192,7 @@ def _check_access_token(self, token: str):
 
     @classmethod
     def _parse_jwt_token(
-        cls, token, sub: any, iss: str, aud: str, key: AbstractJWKBase
+        cls, token, sub: Any, iss: str, aud: str, key: AbstractJWKBase
     ) -> dict:
         """Verifies the validity of a JWT.
         :param token: The token to check for validity.
@@ -199,7 +203,8 @@ def _parse_jwt_token(
         :raises AccessDenied: if the token is invalid.
         """
         try:
-            payload = JWT().decode(token, key, algorithms={"HS256"})
+            # Check the signature and expiration time of token
+            payload = JWT().decode(token, key, algorithms={JWT_ALG}, do_verify=True, do_time_check=True)
         except JWTDecodeError as exc:
             _logger.info(
                 "JWT check failed: %s", exc.__cause__ if exc.__cause__ else exc
@@ -221,6 +226,7 @@ def _parse_jwt_token(
 
         return payload
 
+    @override
     def _check_credentials(self, password, user_agent_env):
         # Check for Bearer *before* parent to prevent costly password check
         # when we are trying to authenticate using Bearer.

auth_external/tests/test_auth_controller.py

@@ -268,10 +268,6 @@ def test_access_denied_without_correct_user_id(self):
             ),
         )
 
-
-
-
-
     """
     We assume the attacker knows the username of the victim
     TO TEST: