mirror of
https://github.com/suitenumerique/meet.git
synced 2026-09-07 07:55:50 +00:00
🐛(backend) allow all printable ASCII in the user sub field
The user `sub` field was rejecting some ASCII characters that are actually valid according to the OIDC spec. Loosen the validation to accept the full ASCII range except control characters, so the field is compliant with the RFC and works with any spec-compliant identity provider. Based on the Stack Overflow discussion in question 279832. Closes #1609.
This commit is contained in:
committed by
aleb_the_flash
parent
eb6b3ba1df
commit
ac4be27445
@@ -3,7 +3,11 @@
|
||||
import contextlib
|
||||
|
||||
from django.conf import settings
|
||||
from django.core.exceptions import ImproperlyConfigured, SuspiciousOperation
|
||||
from django.core.exceptions import (
|
||||
ImproperlyConfigured,
|
||||
SuspiciousOperation,
|
||||
ValidationError,
|
||||
)
|
||||
from django.utils.translation import gettext_lazy as _
|
||||
|
||||
from lasuite.oidc_login.backends import (
|
||||
@@ -17,6 +21,7 @@ from core.services.marketing import (
|
||||
ContactData,
|
||||
get_marketing_service,
|
||||
)
|
||||
from core.validators import sub_validator
|
||||
|
||||
|
||||
class OIDCAuthenticationBackend(LaSuiteOIDCAuthenticationBackend):
|
||||
@@ -84,6 +89,19 @@ class OIDCAuthenticationBackend(LaSuiteOIDCAuthenticationBackend):
|
||||
|
||||
def get_existing_user(self, sub, email):
|
||||
"""Fetch existing user by sub or email."""
|
||||
|
||||
sub = str(sub)
|
||||
|
||||
try:
|
||||
sub_validator(sub)
|
||||
except ValidationError as err:
|
||||
raise SuspiciousOperation(
|
||||
"User info contained an invalid sub claim"
|
||||
) from err
|
||||
|
||||
if len(sub) > 255:
|
||||
raise SuspiciousOperation("User info contained an invalid sub claim")
|
||||
|
||||
try:
|
||||
return User.objects.get(sub=sub)
|
||||
except User.DoesNotExist:
|
||||
|
||||
@@ -8,6 +8,8 @@ import uuid
|
||||
from django.conf import settings
|
||||
from django.db import migrations, models
|
||||
|
||||
import core.validators
|
||||
|
||||
|
||||
class Migration(migrations.Migration):
|
||||
|
||||
@@ -41,7 +43,7 @@ class Migration(migrations.Migration):
|
||||
('id', models.UUIDField(default=uuid.uuid4, editable=False, help_text='primary key for the record as UUID', primary_key=True, serialize=False, verbose_name='id')),
|
||||
('created_at', models.DateTimeField(auto_now_add=True, help_text='date and time at which a record was created', verbose_name='created on')),
|
||||
('updated_at', models.DateTimeField(auto_now=True, help_text='date and time at which a record was last updated', verbose_name='updated on')),
|
||||
('sub', models.CharField(blank=True, help_text='Optional for pending users; required upon account activation. 255 characters or fewer. Letters, numbers, and @/./+/-/_ characters only.', max_length=255, null=True, unique=True, validators=[django.core.validators.RegexValidator(message='Enter a valid sub. This value may contain only letters, numbers, and @/./+/-/_ characters.', regex='^[\\w.@+-]+\\Z')], verbose_name='sub')),
|
||||
('sub', models.CharField(blank=True, help_text='Optional for pending users; required upon account activation. 255 characters or fewer. Printable ASCII characters only.', max_length=255, null=True, unique=True, validators=[core.validators.sub_validator], verbose_name='sub')),
|
||||
('email', models.EmailField(blank=True, max_length=254, null=True, verbose_name='identity email address')),
|
||||
('admin_email', models.EmailField(blank=True, max_length=254, null=True, unique=True, verbose_name='admin email address')),
|
||||
('language', models.CharField(choices=settings.LANGUAGES, default=settings.LANGUAGE_CODE, help_text='The language in which the user wants to see the interface.', max_length=10, verbose_name='language')),
|
||||
|
||||
@@ -27,6 +27,7 @@ from timezone_field import TimeZoneField
|
||||
|
||||
from . import fields, utils
|
||||
from .recording.enums import FileExtension
|
||||
from .validators import sub_validator
|
||||
|
||||
logger = getLogger(__name__)
|
||||
|
||||
@@ -145,19 +146,11 @@ class BaseModel(models.Model):
|
||||
class User(AbstractBaseUser, BaseModel, auth_models.PermissionsMixin):
|
||||
"""User model to work with OIDC only authentication."""
|
||||
|
||||
sub_validator = validators.RegexValidator(
|
||||
regex=r"^[\w.@+-]+\Z",
|
||||
message=_(
|
||||
"Enter a valid sub. This value may contain only letters, "
|
||||
"numbers, and @/./+/-/_ characters."
|
||||
),
|
||||
)
|
||||
|
||||
sub = models.CharField(
|
||||
_("sub"),
|
||||
help_text=_(
|
||||
"Optional for pending users; required upon account activation. "
|
||||
"255 characters or fewer. Letters, numbers, and @/./+/-/_ characters only."
|
||||
"255 characters or fewer. Printable ASCII characters only."
|
||||
),
|
||||
max_length=255,
|
||||
unique=True,
|
||||
|
||||
@@ -40,6 +40,111 @@ def test_authentication_getter_existing_user(monkeypatch):
|
||||
assert user == db_user
|
||||
|
||||
|
||||
@pytest.mark.parametrize(
|
||||
"sub",
|
||||
[
|
||||
# NUL (U+0000) passes str.isascii() but PostgreSQL text fields
|
||||
# cannot store or compare it (DataError)
|
||||
"auth0|abc\x00def",
|
||||
# lone surrogates cannot be encoded to UTF-8 for the DB lookup
|
||||
# (UnicodeEncodeError), which runs before any model validation
|
||||
"bad\ud800sub",
|
||||
# plainly invalid subs would otherwise escape as ValidationError
|
||||
# on user creation, which mozilla-django-oidc does not catch
|
||||
"\u00e9milie",
|
||||
"a" * 256,
|
||||
# ASCII control characters are rejected by policy
|
||||
"tab\tsub",
|
||||
"del\x7fsub",
|
||||
],
|
||||
)
|
||||
def test_authentication_getter_invalid_sub_rejected_cleanly(monkeypatch, sub):
|
||||
"""
|
||||
Subs that can never be persisted should be rejected with
|
||||
SuspiciousOperation (turned into a clean authentication failure by
|
||||
mozilla-django-oidc) instead of leaking DataError, UnicodeEncodeError
|
||||
or ValidationError as a server error.
|
||||
"""
|
||||
klass = OIDCAuthenticationBackend()
|
||||
|
||||
def get_userinfo_mocked(*args):
|
||||
return {"sub": sub, "email": "john@example.com"}
|
||||
|
||||
monkeypatch.setattr(OIDCAuthenticationBackend, "get_userinfo", get_userinfo_mocked)
|
||||
|
||||
with pytest.raises(
|
||||
SuspiciousOperation,
|
||||
match="User info contained an invalid sub claim",
|
||||
):
|
||||
klass.get_or_create_user(access_token="test-token", id_token=None, payload=None)
|
||||
|
||||
assert models.User.objects.exists() is False
|
||||
|
||||
|
||||
def test_authentication_getter_numeric_sub(monkeypatch):
|
||||
"""
|
||||
Some providers serialize the sub as a JSON number. It should keep working
|
||||
(CharField coerces it to a string on save) and must not crash the early
|
||||
sub checks in get_existing_user.
|
||||
"""
|
||||
klass = OIDCAuthenticationBackend()
|
||||
|
||||
def get_userinfo_mocked(*args):
|
||||
return {"sub": 12345, "email": "john@example.com"}
|
||||
|
||||
monkeypatch.setattr(OIDCAuthenticationBackend, "get_userinfo", get_userinfo_mocked)
|
||||
|
||||
user = klass.get_or_create_user(
|
||||
access_token="test-token", id_token=None, payload=None
|
||||
)
|
||||
|
||||
assert user.sub == "12345"
|
||||
assert models.User.objects.count() == 1
|
||||
|
||||
|
||||
def test_authentication_getter_new_user_auth0_pipe_sub(monkeypatch):
|
||||
"""
|
||||
A first login with an Auth0-style sub containing a pipe ("provider|user-id")
|
||||
should create the user instead of raising a ValidationError.
|
||||
Regression test for https://github.com/suitenumerique/meet/issues/[XXX].
|
||||
"""
|
||||
klass = OIDCAuthenticationBackend()
|
||||
|
||||
def get_userinfo_mocked(*args):
|
||||
return {"sub": "auth0|644c0bc8f1874ef6d339fb34", "email": "john@example.com"}
|
||||
|
||||
monkeypatch.setattr(OIDCAuthenticationBackend, "get_userinfo", get_userinfo_mocked)
|
||||
|
||||
user = klass.get_or_create_user(
|
||||
access_token="test-token", id_token=None, payload=None
|
||||
)
|
||||
|
||||
assert user.sub == "auth0|644c0bc8f1874ef6d339fb34"
|
||||
assert user.email == "john@example.com"
|
||||
assert models.User.objects.count() == 1
|
||||
|
||||
|
||||
def test_authentication_getter_existing_user_auth0_pipe_sub(monkeypatch):
|
||||
"""
|
||||
A returning user with an Auth0-style pipe sub should be matched by sub,
|
||||
not duplicated or rejected.
|
||||
"""
|
||||
klass = OIDCAuthenticationBackend()
|
||||
db_user = UserFactory(sub="auth0|644c0bc8f1874ef6d339fb34")
|
||||
|
||||
def get_userinfo_mocked(*args):
|
||||
return {"sub": db_user.sub}
|
||||
|
||||
monkeypatch.setattr(OIDCAuthenticationBackend, "get_userinfo", get_userinfo_mocked)
|
||||
|
||||
user = klass.get_or_create_user(
|
||||
access_token="test-token", id_token=None, payload=None
|
||||
)
|
||||
|
||||
assert user == db_user
|
||||
assert models.User.objects.count() == 1
|
||||
|
||||
|
||||
def test_authentication_getter_new_user_no_email(monkeypatch):
|
||||
"""
|
||||
If no user matches, a user should be created.
|
||||
|
||||
@@ -26,6 +26,64 @@ def test_models_users_id_unique():
|
||||
factories.UserFactory(id=user.id)
|
||||
|
||||
|
||||
@pytest.mark.parametrize(
|
||||
"sub,is_valid",
|
||||
[
|
||||
# cases from suitenumerique/docs PR #1295 (same validator)
|
||||
("valid_sub.@+-:=/", True),
|
||||
("invalid süb", False),
|
||||
(12345, True),
|
||||
# Auth0 emits "provider|user-id" subject identifiers
|
||||
("auth0|644c0bc8f1874ef6d339fb34", True),
|
||||
("google-oauth2|103547991597142817347", True),
|
||||
# Keycloak-style UUID
|
||||
("f:550e8400-e29b-41d4-a716-446655440000:jdoe", True),
|
||||
# base64/URN-style identifiers
|
||||
("dGVzdC1zdWItdmFsdWU=", True),
|
||||
("urn:example:user/42", True),
|
||||
# legacy format still accepted
|
||||
("user@example.com", True),
|
||||
# space (U+0020) is printable ASCII and remains allowed
|
||||
("sub with space", True),
|
||||
# non-ASCII values are rejected
|
||||
("émilie", False),
|
||||
# ASCII control characters (U+0000-U+001F, U+007F) are rejected:
|
||||
# NUL passes isascii() but cannot be stored in PostgreSQL text
|
||||
# fields, and the others invite log injection and interop issues
|
||||
("nul\x00sub", False),
|
||||
("\x00", False),
|
||||
("tab\tsub", False),
|
||||
("newline\nsub", False),
|
||||
("del\x7fsub", False),
|
||||
],
|
||||
)
|
||||
def test_models_users_sub_validator(sub, is_valid):
|
||||
"""
|
||||
The "sub" field should accept any ASCII string as required by
|
||||
OpenID Connect Core 1.0 §2 and RFC 7519 §4.1.2, and reject non-ASCII values.
|
||||
"""
|
||||
user = factories.UserFactory()
|
||||
user.sub = sub
|
||||
if is_valid:
|
||||
user.full_clean()
|
||||
else:
|
||||
with pytest.raises(
|
||||
ValidationError,
|
||||
match="Enter a valid sub. This value should be printable ASCII only.",
|
||||
):
|
||||
user.full_clean()
|
||||
|
||||
|
||||
def test_models_users_sub_max_length():
|
||||
"""The "sub" field should enforce the 255 ASCII characters limit of OIDC Core 1.0 §2."""
|
||||
user = factories.UserFactory(sub="a" * 255)
|
||||
assert user.sub == "a" * 255
|
||||
|
||||
user.sub = "a" * 256
|
||||
with pytest.raises(ValidationError, match="at most 255 characters"):
|
||||
user.full_clean()
|
||||
|
||||
|
||||
def test_models_users_send_mail_main_existing():
|
||||
"""The "email_user' method should send mail to the user's email address."""
|
||||
user = factories.UserFactory()
|
||||
|
||||
@@ -0,0 +1,23 @@
|
||||
"""Custom validators for the core app."""
|
||||
|
||||
from django.core.exceptions import ValidationError
|
||||
from django.utils.translation import gettext_lazy as _
|
||||
|
||||
|
||||
def sub_validator(value):
|
||||
"""Validate that the sub is printable ASCII only.
|
||||
|
||||
OpenID Connect Core 1.0 (section 2) allows any ASCII (RFC 20) string of
|
||||
at most 255 characters, so no character whitelist is applied: providers
|
||||
legitimately emit "|" (Auth0), ":" (Keycloak), "=", "/", etc. As a
|
||||
deliberate hardening beyond the spec, ASCII control characters
|
||||
(U+0000-U+001F and U+007F) are rejected: no known provider emits them,
|
||||
NUL cannot be stored in PostgreSQL text fields, and the others invite
|
||||
log-injection and interoperability issues. For str values,
|
||||
``isprintable()`` is false exactly for those control characters, while
|
||||
space (U+0020) remains allowed.
|
||||
"""
|
||||
if not value.isascii() or not value.isprintable():
|
||||
raise ValidationError(
|
||||
_("Enter a valid sub. This value should be printable ASCII only.")
|
||||
)
|
||||
Reference in New Issue
Block a user