diff --git a/auth_external/controllers/auth.py b/auth_external/controllers/auth.py index 59ef3c2d..5aff9488 100644 --- a/auth_external/controllers/auth.py +++ b/auth_external/controllers/auth.py @@ -18,7 +18,8 @@ def _validate_fields_as_expected(self, fields: List[str], data: dict) -> None: for f in fields: if f not in data: _logger.info( - f"Request failed because field '{f}' was missing from the request data" + f"""Request failed because field '{f}' was missing from the + request data""" ) raise AccessDenied if len(fields) != len(data): @@ -64,7 +65,8 @@ def _validate_refresh_token(self, request) -> Tuple[str, dict]: AccessDenied: if the refresh_token is None Returns: - dict: Payload of the refresh token, if the token was authentic and non-expired + dict: Payload of the refresh token, if the token was authentic and + non-expired """ self._validate_fields_as_expected(["refresh_token"], request.jsonrequest) @@ -132,9 +134,10 @@ def logout(self): if rt_model.is_revoked: _logger.warning( f"""[RTRD] Refresh Token Reuse Detection triggered - on logout ({jti=}, {user_id=}). Anyway, we were going to revoke - the token family, so no harm done (but still - worrying: is there an XSS being exploited?) """ + on logout ({jti=}, {user_id=}). Anyway, we were + going to revoke the token family, so no harm done + (but still worrying: is there an XSS being + exploited?) """ ) rt_model.sudo().revoke_family() diff --git a/auth_external/models/refresh_tokens.py b/auth_external/models/refresh_tokens.py index 443f2753..22bab433 100644 --- a/auth_external/models/refresh_tokens.py +++ b/auth_external/models/refresh_tokens.py @@ -2,15 +2,16 @@ from datetime import datetime, timezone from typing import Callable, List, Optional -from odoo import api, fields, models +from odoo import _, api, fields, models _logger = logging.getLogger(__name__) class RefreshTokens(models.Model): """ - This model allows to store refresh tokens in the database for as long as they are not expired. - This allows immediate revocation of refresh tokens as well as automatic reuse detection. + This model allows to store refresh tokens in the database for as long as + they are not expired. This allows immediate revocation of refresh tokens as + well as automatic reuse detection. """ _name = "auth_external.refresh_tokens" @@ -71,7 +72,7 @@ class RefreshTokens(models.Model): def _check_hierarchy(self): if not self._check_recursion(): raise models.ValidationError( - "Error! You cannot create recursive refresh_token families." + _("Error! You cannot create recursive refresh_token families.") ) @api.model @@ -183,5 +184,6 @@ def remove_expired_tokens(self): removed_rts += 1 remaining_rts = self.sudo().search_count([]) _logger.info( - f"RefreshTokens: removed {removed_rts} expired tokens, remains {remaining_rts} in the db." + f"""RefreshTokens: removed {removed_rts} expired tokens, remains + {remaining_rts} in the db.""" ) diff --git a/auth_external/models/res_users.py b/auth_external/models/res_users.py index 76e1d4c7..0d036303 100644 --- a/auth_external/models/res_users.py +++ b/auth_external/models/res_users.py @@ -25,16 +25,15 @@ def gen_signing_key() -> AbstractJWKBase: - """Generates a cryptographically secure random signing/verification key, which needs to - be used in HMAC. - - Regarding the secret size: - "A key of the same size as the hash output (for instance, 256 bits for - "HS256") or larger MUST be used with this algorithm. (This - requirement is based on Section 5.3.4 (Security Effect of the HMAC - Key) of NIST SP 800-117 [NIST.800-107], which states that the - effective security strength is the minimum of the security strength - of the key and two times the size of the internal hash value.)" + """Generates a cryptographically secure random signing/verification key, + which needs to be used in HMAC. + + Regarding the secret size: "A key of the same size as the hash output (for + instance, 256 bits for "HS256") or larger MUST be used with this algorithm. + (This requirement is based on Section 5.3.4 (Security Effect of the HMAC + Key) of NIST SP 800-117 [NIST.800-107], which states that the effective + security strength is the minimum of the security strength of the key and two + times the size of the internal hash value.)" https://www.rfc-editor.org/rfc/rfc7518#section-3.2 Returns: @@ -46,17 +45,15 @@ def gen_signing_key() -> AbstractJWKBase: return supported_key_types()["oct"](secret) -# We use symmetric signing/verification keys as only the server needs to sign and verify tokens. -# The keys are stored in program memory to avoid the difficult problem of storing secrets. -# *** This means that on server restart, all clients will have to login again *** +# We use symmetric signing/verification keys as only the server needs to sign +# and verify tokens. The keys are stored in program memory to avoid the +# difficult problem of storing secrets. *** This means that on server restart, +# all clients will have to authenticate again *** access_token_signing_key = gen_signing_key() -""" -Secret key used to sign/verify access_tokens -""" +"Secret key used to sign/verify access_tokens" + refresh_token_signing_key = gen_signing_key() -""" -Secret key used to sign/verify refresh_tokens -""" +"Secret key used to sign/verify refresh_tokens" JWT_ALG = "HS256" @@ -102,7 +99,8 @@ def _generate_jwt( def generate_external_auth_token(self, rt_old=None): """Generates a new access token and refresh token for the user. - :param rt_old: the token allowing refreshing auth tokens. This token will get revoked. + :param rt_old: the token allowing refreshing auth tokens. This token + will get revoked. :returns: the freshly generated tokens (access + refresh tokens). :raises AccessDenied: if the user can't generate tokens. """ @@ -155,8 +153,9 @@ def generate_external_auth_token(self, rt_old=None): # the whole token family and emit a warning in the server logs rt_old_model.sudo().revoke_family() - # Absolutely necessary otherwise the changes of revoke_family() - # are not committed due to the following exception + # Absolutely necessary to commit otherwise the changes of + # revoke_family() are not committed due to the AccessDenied + # which follows self.env.cr.commit() user_id = rt_old_payload["sub"] _logger.warning( @@ -315,16 +314,19 @@ def _parse_jwt_token( return payload def _check_credentials(self, password: str, user_agent_env) -> None: - """Overrides the default method in res.users to accept authorization via a valid JWT access_token. + """Overrides the default method in res.users to accept authorization via + a valid JWT access_token. If no access_token is found, defaults to password authentication. Args: - password (str): Password used as fallback if no Authorization header with a valid JWT access_token is present in the request headers. - user_agent_env (dict): additional credential data. This is used to provide the totp_token in 2FA. + password (str): Password used as fallback if no Authorization header + with a valid JWT access_token is present in the request headers. + user_agent_env (dict): additional credential data. This is used to + provide the totp_token in 2FA. Raises: - InvalidTotp: If the provided totp code is invalid. - AccessDenied: If the credentials are incorrect. + InvalidTotp: If the provided totp code is invalid. AccessDenied: If + the credentials are incorrect. """ # Check for Bearer *before* parent to prevent costly password check # when we are trying to authenticate using Bearer. diff --git a/auth_external/security/res_groups.xml b/auth_external/security/res_groups.xml index 041a0d11..b476259d 100644 --- a/auth_external/security/res_groups.xml +++ b/auth_external/security/res_groups.xml @@ -1,6 +1,6 @@ - + Admin group for auth_external module diff --git a/auth_external/tests/test_auth_controller.py b/auth_external/tests/test_auth_controller.py index a4c99ebb..482e96df 100644 --- a/auth_external/tests/test_auth_controller.py +++ b/auth_external/tests/test_auth_controller.py @@ -29,9 +29,7 @@ ACCESS_DENIED_XMLRPC = "Access Denied" LoginRespData = Tuple[str, str, str, datetime] -""" -user_id, access_token, refresh_token, at_exp_datetime -""" +"user_id, access_token, refresh_token, at_exp_datetime" class TestAuthController(HttpCase): @@ -177,12 +175,14 @@ def assert_cannot_write_user_data( target_user_id=None, expected_fault_substring=None, ) -> None: - write_fn = lambda: self.write_user_sig( - user_id, - access_token, - user_id if target_user_id is None else target_user_id, - "Malicious signature", - ) + def write_fn(): + self.write_user_sig( + user_id, + access_token, + user_id if target_user_id is None else target_user_id, + "Malicious signature", + ) + if expected_fault_substring is None: self.assert_xmlrpc_access_denied(write_fn) else: @@ -269,8 +269,9 @@ def xmlrpc_execute_kw( model_name: str, method: str, pos_args: list, - kw_args={}, + kw_args=None, ) -> dict: + kw_args = {} if kw_args is None else kw_args models = ServerProxy( f"{self.xmlrpc_url}object", headers=[("Authorization", f"Bearer {access_token}")], @@ -351,7 +352,8 @@ def test_access_denied_with_additional_fields(self): def test_access_denied_incorrect_password(self): """ - An attacker is denied access to a normal user account when providing an incorrect password + An attacker is denied access to a normal user account when providing an + incorrect password """ data_incorrect_password = self.user_normal_login_data() data_incorrect_password["password"] = "incorrect_password" @@ -367,11 +369,14 @@ def test_access_denied_2fa_correct_password_absent_totp(self): def test_access_denied_2fa_correct_password_incorrect_totp(self): """ - An attacker is denied access to a 2fa user account when providing a correct password but incorrect totp + An attacker is denied access to a 2fa user account when providing a + correct password but incorrect totp """ data_incorrect_totp = self.user_2fa_login_data() data_incorrect_totp["totp"] = ( - "123456" # 1/1'000'000 chance that this is the correct totp and that the test fails + "123456" + # 1/1'000'000 chance that this is the correct totp and that the test + # fails ) self.login_should_produce_invalid_totp(data_incorrect_totp) @@ -402,7 +407,8 @@ def test_fresh_access_token_is_accepted(self): def test_access_denied_without_correct_user_id(self): """ - An attacker cannot use their access_token to modify data from another account + An attacker cannot use their access_token to modify data from another + account """ user_id_normal, access_token_normal, _, _ = self.user_normal_login() user_id_2fa, _, _, _ = self.user_2fa_login() @@ -495,13 +501,15 @@ def test_refresh_token_reuse_detection_mechanism_works(self): _, last_rt, _ = self.refresh(rts[-1]) - # Oh no, an attacker intercepted a random refresh token (but no the last one which is still valid) + # Oh no, an attacker intercepted a random refresh token (but no the last + # one which is still valid) rt_intercepted = random.choice(rts) # They try to use it to get fresh tokens. # Haha, it doesn't work! Automatic reuse detection was triggered! self.assert_refresh_access_denied(rt_intercepted) - # Automatic reuse detection also revoked the last rt (which was still valid before) + # Automatic reuse detection also revoked the last rt (which was still + # valid before) self.assert_refresh_access_denied(last_rt) def test_refresh_token_revoked_after_logout(self): diff --git a/auth_external/tests/test_refresh_tokens.py b/auth_external/tests/test_refresh_tokens.py index 96bc378d..8279e93e 100644 --- a/auth_external/tests/test_refresh_tokens.py +++ b/auth_external/tests/test_refresh_tokens.py @@ -1,4 +1,5 @@ import random +from typing import Any import uuid from datetime import datetime, timedelta @@ -29,7 +30,7 @@ def create_refresh_token( parent.link_child(rt) return rt - def create_user(self) -> "res.users": + def create_user(self) -> Any: login = f"testuser_{random.randint(0, 10000)}" return self.env["res.users"].create({"name": f"Name {login}", "login": login})