Skip to content

Commit

Permalink
Fix pre-commit
Browse files Browse the repository at this point in the history
  • Loading branch information
nlachat-compassion committed Oct 29, 2024
1 parent 46fcbb0 commit b6462af
Show file tree
Hide file tree
Showing 6 changed files with 71 additions and 55 deletions.
13 changes: 8 additions & 5 deletions auth_external/controllers/auth.py
Original file line number Diff line number Diff line change
Expand Up @@ -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):
Expand Down Expand Up @@ -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)

Expand Down Expand Up @@ -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()
Expand Down
12 changes: 7 additions & 5 deletions auth_external/models/refresh_tokens.py
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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."""
)
56 changes: 29 additions & 27 deletions auth_external/models/res_users.py
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand All @@ -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"

Expand Down Expand Up @@ -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.
"""
Expand Down Expand Up @@ -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(
Expand Down Expand Up @@ -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.
Expand Down
2 changes: 1 addition & 1 deletion auth_external/security/res_groups.xml
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
<?xml version="1.0" encoding="utf-8" ?>
<odoo>
<record id="auth_external.group_admin" model="res.groups">
<record id="group_admin" model="res.groups">
<field name="name">Admin group for auth_external module</field>
<field name="users" eval="[(4, ref('base.user_admin'))]" />
</record>
Expand Down
40 changes: 24 additions & 16 deletions auth_external/tests/test_auth_controller.py
Original file line number Diff line number Diff line change
Expand Up @@ -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):
Expand Down Expand Up @@ -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:
Expand Down Expand Up @@ -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}")],
Expand Down Expand Up @@ -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"
Expand All @@ -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)

Expand Down Expand Up @@ -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()
Expand Down Expand Up @@ -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):
Expand Down
3 changes: 2 additions & 1 deletion auth_external/tests/test_refresh_tokens.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import random
from typing import Any
import uuid
from datetime import datetime, timedelta

Expand Down Expand Up @@ -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})

Expand Down

0 comments on commit b6462af

Please sign in to comment.