From d65ac8c84522322a71671e163ccf91d86bc4f099 Mon Sep 17 00:00:00 2001 From: briquet Date: Wed, 21 Oct 2026 21:14:47 +0200 Subject: [PATCH] =?UTF-8?q?=F0=9F=90=9B(backend)=20return=20RFC=206749=20e?= =?UTF-8?q?rrors=20from=20application=20token=20endpoint?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The application token endpoint implements the client credentials grant (RFC 6749 section 4.4), but its errors did not follow section 5.2: serializer errors were keyed by field, and the other errors used either `detail` or a free-text `error`. Standard OAuth 2.0 client libraries could not parse them. Every error is now rendered as `{"error": , "error_description": }` by a view-specific exception handler: - missing or blank client credentials and wrong credentials or inactive applications return `invalid_client` - an unsupported grant type returns `unsupported_grant_type` - an invalid email, a denied domain or an unknown user return `invalid_scope`; - the provisional user creation conflict returns `server_error`, since the RFC defines no code for server-side failures here - any other error returns `invalid_request`. Serializer errors are flattened into one description listing every invalid field. Descriptions are restricted to the characters allowed by the RFC, as DRF messages may be translated or contain quotes. --- CHANGELOG.md | 4 + docs/openapi.yaml | 106 ++++++++++--- docs/resource_server.yaml | 21 ++- src/backend/core/audit/drf.py | 8 +- src/backend/core/external_api/exceptions.py | 98 ++++++++++++ src/backend/core/external_api/viewsets.py | 44 +++--- src/backend/core/tests/audit/test_drf.py | 19 ++- .../core/tests/test_external_api_token.py | 139 ++++++++++++++++-- 8 files changed, 381 insertions(+), 58 deletions(-) create mode 100644 src/backend/core/external_api/exceptions.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 09699a419..822ce038f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,10 @@ and this project adheres to ## [Unreleased] +### Fixed + +- ๐Ÿ›(backend) return RFC 6749 errors from the application token endpoint + ## [1.36.1] - 2026-10-21 ### Added diff --git a/docs/openapi.yaml b/docs/openapi.yaml index 108f2e13f..31cea804d 100644 --- a/docs/openapi.yaml +++ b/docs/openapi.yaml @@ -94,8 +94,38 @@ paths: token_type: "Bearer" expires_in: 3600 scope: "rooms:list rooms:retrieve rooms:create" + '400': + description: | + Invalid request. When several parameters are invalid, `error` reports + the most relevant one (client credentials first, then grant type) and + `error_description` lists every invalid parameter. + content: + application/json: + schema: + $ref: '#/components/schemas/OAuthError' + examples: + missingClientCredentials: + summary: Missing client credentials + value: + error: "invalid_client" + error_description: "client_id : This field is required.; client_secret : This field is required." + unsupportedGrantType: + summary: Unsupported grant type + value: + error: "unsupported_grant_type" + error_description: "grant_type : 'authorization_code' is not a valid choice." + missingParameter: + summary: Missing required parameter + value: + error: "invalid_request" + error_description: "grant_type : This field is required." + invalidScope: + summary: Scope is not a valid email address + value: + error: "invalid_scope" + error_description: "Scope should be a valid email address." '401': - description: Authentication failed + description: Client authentication failed content: application/json: schema: @@ -104,22 +134,13 @@ paths: invalidCredentials: summary: Invalid credentials value: - error: "Invalid credentials" + error: "invalid_client" + error_description: "Invalid credentials" inactiveApplication: summary: Application is inactive value: - error: "Application is inactive" - '400': - description: Invalid request - content: - application/json: - schema: - $ref: '#/components/schemas/OAuthError' - examples: - userNotFound: - summary: User not found - value: - error: "User not found." + error: "invalid_client" + error_description: "Application is inactive" '403': description: Access denied - cannot delegate user content: @@ -130,7 +151,36 @@ paths: delegationDenied: summary: Domain not authorized value: - error: "This application is not authorized for this email domain." + error: "invalid_scope" + error_description: "This application is not authorized for this email domain." + '404': + description: | + User not found. Only returned when the application is not allowed + to create users. + content: + application/json: + schema: + $ref: '#/components/schemas/OAuthError' + examples: + userNotFound: + summary: User not found + value: + error: "invalid_scope" + error_description: "User not found." + '409': + description: | + The user could not be created or retrieved because of a concurrent + request. Retrying the request should succeed. + content: + application/json: + schema: + $ref: '#/components/schemas/OAuthError' + examples: + provisionalUserConflict: + summary: Concurrent user creation + value: + error: "server_error" + error_description: "Failed to create or retrieve provisional user." '415': description: | Unsupported media type. The request body must be sent as @@ -138,12 +188,13 @@ paths: content: application/json: schema: - $ref: '#/components/schemas/Error' + $ref: '#/components/schemas/OAuthError' examples: unsupportedMediaType: summary: Unsupported request content type value: - detail: 'Unsupported media type "text/plain" in request.' + error: "invalid_request" + error_description: "Unsupported media type 'text/plain' in request." /rooms: get: @@ -552,11 +603,28 @@ components: OAuthError: type: object - description: OAuth2-compliant error response + description: OAuth 2.0 error response (RFC 6749, section 5.2) + required: + - error + - error_description properties: error: type: string - description: Human-readable error description + description: | + Error code. `server_error` is not defined by RFC 6749 for the token + endpoint and is only returned on an unexpected server-side failure. + enum: + - invalid_request + - invalid_client + - unsupported_grant_type + - invalid_scope + - server_error + example: "invalid_client" + error_description: + type: string + description: | + Human-readable description of the error. Contains printable ASCII + characters only, without double quotes or backslashes. example: "Invalid credentials" Error: diff --git a/docs/resource_server.yaml b/docs/resource_server.yaml index c6b310d3a..8bf97e3cd 100644 --- a/docs/resource_server.yaml +++ b/docs/resource_server.yaml @@ -393,11 +393,28 @@ components: OAuthError: type: object - description: OAuth2-compliant error response + description: OAuth 2.0 error response (RFC 6749, section 5.2) + required: + - error + - error_description properties: error: type: string - description: Human-readable error description + description: | + Error code. `server_error` is not defined by RFC 6749 for the token + endpoint and is only returned on an unexpected server-side failure. + enum: + - invalid_request + - invalid_client + - unsupported_grant_type + - invalid_scope + - server_error + example: "invalid_client" + error_description: + type: string + description: | + Human-readable description of the error. Contains printable ASCII + characters only, without double quotes or backslashes. example: "Invalid credentials" Error: diff --git a/src/backend/core/audit/drf.py b/src/backend/core/audit/drf.py index 02537d304..16f27c143 100644 --- a/src/backend/core/audit/drf.py +++ b/src/backend/core/audit/drf.py @@ -52,10 +52,14 @@ _logger = logging.getLogger(__name__) def error_message(response) -> Any: - """Return the message of an error response, as DRF or the view wrote it.""" + """Return the message of an error response, as DRF or the view wrote it. + + An OAuth 2.0 error (RFC 6749 ยง5.2) carries its message in + ``error_description``, its ``error`` being only a code. + """ data = getattr(response, "data", None) if isinstance(data, Mapping): - return data.get("detail") or data.get("error") + return data.get("detail") or data.get("error_description") or data.get("error") return None diff --git a/src/backend/core/external_api/exceptions.py b/src/backend/core/external_api/exceptions.py new file mode 100644 index 000000000..4faaf919b --- /dev/null +++ b/src/backend/core/external_api/exceptions.py @@ -0,0 +1,98 @@ +"""OAuth 2.0 error responses for the external API token endpoint (RFC 6749 ยง5.2).""" + +import unicodedata + +from django.http import Http404 + +from rest_framework import exceptions as drf_exceptions +from rest_framework import status as drf_status + +from core.api import exception_handler as default_exception_handler + +OAUTH2_ERROR_INVALID_REQUEST = "invalid_request" +OAUTH2_ERROR_INVALID_CLIENT = "invalid_client" +OAUTH2_ERROR_UNSUPPORTED_GRANT_TYPE = "unsupported_grant_type" +OAUTH2_ERROR_INVALID_SCOPE = "invalid_scope" +OAUTH2_ERROR_SERVER_ERROR = "server_error" + +OAUTH2_CLIENT_CREDENTIALS_FIELDS = {"client_id", "client_secret"} + + +class OAuth2Error(drf_exceptions.APIException): + """Token endpoint error carrying an RFC 6749 error code as its DRF code.""" + + def __init__(self, error, description, status_code=drf_status.HTTP_400_BAD_REQUEST): + super().__init__(detail=description, code=error) + self.status_code = status_code + + +def _get_validation_error_code(codes): + """Map serializer error codes to the most relevant OAuth 2.0 error code.""" + if OAUTH2_CLIENT_CREDENTIALS_FIELDS & codes.keys(): + return OAUTH2_ERROR_INVALID_CLIENT + if "invalid_choice" in codes.get("grant_type", []): + return OAUTH2_ERROR_UNSUPPORTED_GRANT_TYPE + return OAUTH2_ERROR_INVALID_REQUEST + + +def _get_error_code(exc): + """Return the OAuth 2.0 error code matching an exception.""" + if isinstance(exc, OAuth2Error): + return exc.get_codes() + if isinstance(exc, drf_exceptions.ValidationError) and isinstance(exc.detail, dict): + return _get_validation_error_code(exc.get_codes()) + if isinstance( + exc, (drf_exceptions.AuthenticationFailed, drf_exceptions.NotAuthenticated) + ): + return OAUTH2_ERROR_INVALID_CLIENT + return OAUTH2_ERROR_INVALID_REQUEST + + +def _format_error_description(data): + """Flatten DRF error data into "field : message; field : message".""" + if isinstance(data, dict): + if data.keys() == {"detail"}: + return _format_error_description(data["detail"]) + return "; ".join( + f"{field} : {_format_error_description(messages)}" + for field, messages in data.items() + ) + if isinstance(data, list): + return " ".join(_format_error_description(message) for message in data) + return str(data) + + +def _sanitize_error_description(description): + """Restrict a description to the characters allowed by RFC 6749 ยง5.2. + + Only printable ASCII is allowed, except for the double quote and the backslash. + Translated messages are transliterated so they stay readable ("รฉ" becomes "e"). + """ + ascii_description = ( + unicodedata.normalize("NFKD", description) + .encode("ascii", "ignore") + .decode("ascii") + .replace('"', "'") + .replace("\\", "/") + ) + return "".join(char for char in ascii_description if " " <= char <= "~") + + +def oauth2_exception_handler(exc, context): + """Render errors as RFC 6749 ยง5.2 responses: {"error", "error_description"}. + + Status codes and headers set by the default handler are kept. + """ + response = default_exception_handler(exc, context) + + # A disabled feature flag raises Http404: keep it looking like a missing route. + if response is None or isinstance(exc, Http404): + return response + + response.data = { + "error": _get_error_code(exc), + "error_description": _sanitize_error_description( + _format_error_description(response.data) + ), + } + return response diff --git a/src/backend/core/external_api/viewsets.py b/src/backend/core/external_api/viewsets.py index a08c08244..4efc64f3f 100644 --- a/src/backend/core/external_api/viewsets.py +++ b/src/backend/core/external_api/viewsets.py @@ -35,7 +35,7 @@ from ..services.provisional_user_service import ( ProvisionalUserIntegrityError, ProvisionalUserService, ) -from . import authentication, permissions, serializers +from . import authentication, exceptions, permissions, serializers class ApplicationViewSet(audit.AuditViewMixin, viewsets.ViewSet): @@ -44,6 +44,10 @@ class ApplicationViewSet(audit.AuditViewMixin, viewsets.ViewSet): audit_actor = None audit_claimed_client_id = None + def get_exception_handler(self): + """Render errors as OAuth 2.0 error responses (RFC 6749 section 5.2).""" + return exceptions.oauth2_exception_handler + @decorators.action( detail=False, methods=["post"], @@ -92,31 +96,33 @@ class ApplicationViewSet(audit.AuditViewMixin, viewsets.ViewSet): try: validate_email(email) - except ValidationError: - return drf_response.Response( - { - "error": "Scope should be a valid email address.", - }, - status=drf_status.HTTP_400_BAD_REQUEST, - ) + except ValidationError as e: + raise exceptions.OAuth2Error( + exceptions.OAUTH2_ERROR_INVALID_SCOPE, + "Scope should be a valid email address.", + ) from e if not application.can_delegate_email(email): - return drf_response.Response( - { - "error": "This application is not authorized for this email domain.", - }, - status=drf_status.HTTP_403_FORBIDDEN, + raise exceptions.OAuth2Error( + exceptions.OAUTH2_ERROR_INVALID_SCOPE, + "This application is not authorized for this email domain.", + status_code=drf_status.HTTP_403_FORBIDDEN, ) try: user, created = ProvisionalUserService().get_or_create(email, client_id) except ProvisionalUserCreationDisabledError as not_found_error: - raise drf_exceptions.NotFound("User not found.") from not_found_error - except ProvisionalUserIntegrityError: - return drf_response.Response( - {"error": "Failed to create or retrieve provisional user."}, - status=drf_status.HTTP_409_CONFLICT, - ) + raise exceptions.OAuth2Error( + exceptions.OAUTH2_ERROR_INVALID_SCOPE, + "User not found.", + status_code=drf_status.HTTP_404_NOT_FOUND, + ) from not_found_error + except ProvisionalUserIntegrityError as integrity_error: + raise exceptions.OAuth2Error( + exceptions.OAUTH2_ERROR_SERVER_ERROR, + "Failed to create or retrieve provisional user.", + status_code=drf_status.HTTP_409_CONFLICT, + ) from integrity_error scope = " ".join(application.scopes or []) diff --git a/src/backend/core/tests/audit/test_drf.py b/src/backend/core/tests/audit/test_drf.py index f4230f00d..dcd0f4204 100644 --- a/src/backend/core/tests/audit/test_drf.py +++ b/src/backend/core/tests/audit/test_drf.py @@ -33,6 +33,7 @@ class ThingViewSet(audit.AuditViewMixin, viewsets.ViewSet): permission_classes = [] audit_actions = {"list": "thing.list", "create": "thing.create"} error = None + error_body = {"error": "Already exists."} def list(self, request): if self.error is not None: @@ -41,7 +42,7 @@ class ThingViewSet(audit.AuditViewMixin, viewsets.ViewSet): return Response([]) def create(self, request): - return Response({"error": "Already exists."}, status=409) + return Response(self.error_body, status=409) def destroy(self, request, pk=None): return Response(status=204) @@ -198,6 +199,22 @@ def test_error_message_of_a_view_response(audit_events): assert event["error"] == {"message": "Already exists."} +def test_error_message_of_an_oauth2_error_response(audit_events): + """An OAuth 2.0 error reports its description rather than its code.""" + view = ThingViewSet.as_view( + {"post": "create"}, + error_body={"error": "server_error", "error_description": "Already exists."}, + ) + + response = view(APIRequestFactory().post("/things/")) + + assert response.status_code == 409 + + [event] = find_events(audit_events, "thing.create") + + assert event["error"] == {"message": "Already exists."} + + def test_actions_missing_from_the_map_are_not_audited(audit_events): """Only the actions listed in ``audit_actions`` emit events.""" view = ThingViewSet.as_view({"delete": "destroy"}) diff --git a/src/backend/core/tests/test_external_api_token.py b/src/backend/core/tests/test_external_api_token.py index d01f6f8ae..936e886a7 100644 --- a/src/backend/core/tests/test_external_api_token.py +++ b/src/backend/core/tests/test_external_api_token.py @@ -27,6 +27,11 @@ from core.services import provisional_user_service pytestmark = pytest.mark.django_db +INVALID_CREDENTIALS_ERROR = { + "error": "invalid_client", + "error_description": "Invalid credentials", +} + def test_api_applications_generate_token_application_disabled(settings): """When APPLICATION_ENABLED is False, the endpoint should return 404.""" @@ -53,6 +58,7 @@ def test_api_applications_generate_token_application_disabled(settings): ) assert response.status_code == 404 + assert response.json() == {"detail": "Not found."} def test_api_applications_generate_token_success(settings): @@ -140,7 +146,7 @@ def test_api_applications_generate_token_invalid_credentials_query_count( ) assert response.status_code == 401 - assert "Invalid credentials" in str(response.data) + assert response.json() == INVALID_CREDENTIALS_ERROR def test_api_applications_generate_token_form_urlencoded(settings): @@ -201,7 +207,7 @@ def test_api_applications_generate_token_form_urlencoded_invalid_credentials(): ) assert response.status_code == 401 - assert "Invalid credentials" in str(response.data) + assert response.json() == INVALID_CREDENTIALS_ERROR def test_api_applications_generate_token_form_urlencoded_missing_fields(): @@ -215,8 +221,14 @@ def test_api_applications_generate_token_form_urlencoded_missing_fields(): ) assert response.status_code == 400 - for field in ("client_id", "client_secret", "scope"): - assert field in response.data + assert response.json() == { + "error": "invalid_client", + "error_description": ( + "client_id : This field is required.; " + "client_secret : This field is required.; " + "scope : This field is required." + ), + } def test_api_applications_generate_token_form_urlencoded_invalid_grant_type(): @@ -240,7 +252,12 @@ def test_api_applications_generate_token_form_urlencoded_invalid_grant_type(): ) assert response.status_code == 400 - assert "grant_type" in response.data + assert response.json() == { + "error": "unsupported_grant_type", + "error_description": ( + "grant_type : 'authorization_code' is not a valid choice." + ), + } def test_api_applications_generate_token_form_urlencoded_special_characters(): @@ -283,6 +300,75 @@ def test_api_applications_generate_token_unsupported_media_type(): ) assert response.status_code == 415 + assert response.json() == { + "error": "invalid_request", + "error_description": "Unsupported media type 'text/plain' in request.", + } + + +def test_api_applications_generate_token_missing_fields(): + """Missing fields should return an RFC 6749 error listing every invalid + field, reported as "invalid_client" when client credentials are missing.""" + client = APIClient() + response = client.post( + "/external-api/v1.0/application/token/", + {}, + format="json", + ) + + assert response.status_code == 400 + assert response.json() == { + "error": "invalid_client", + "error_description": ( + "client_id : This field is required.; " + "client_secret : This field is required.; " + "grant_type : This field is required.; " + "scope : This field is required." + ), + } + + +def test_api_applications_generate_token_missing_grant_type(): + """A request with client credentials but no grant_type should return + an "invalid_request" error.""" + user = UserFactory(email="user@example.com") + plain_secret = "test-secret-123" + application = ApplicationFactory(client_secret=plain_secret, is_active=True) + + client = APIClient() + response = client.post( + "/external-api/v1.0/application/token/", + { + "client_id": application.client_id, + "client_secret": plain_secret, + "scope": user.email, + }, + format="json", + ) + + assert response.status_code == 400 + assert response.json() == { + "error": "invalid_request", + "error_description": "grant_type : This field is required.", + } + + +def test_api_applications_generate_token_error_description_charset(): + """Translated error descriptions should only contain the characters + allowed by RFC 6749 section 5.2: printable ASCII, except double quotes + and backslashes.""" + user = UserFactory(email="user@example.com") + application = ApplicationFactory(is_active=True) + + response = _post_token( + application.client_id, "", user.email, HTTP_ACCEPT_LANGUAGE="fr-fr" + ) + + assert response.status_code == 400 + assert response.json() == { + "error": "invalid_client", + "error_description": "client_secret : Ce champ ne peut etre vide.", + } def test_api_applications_generate_token_invalid_client_id(): @@ -302,7 +388,7 @@ def test_api_applications_generate_token_invalid_client_id(): ) assert response.status_code == 401 - assert "Invalid credentials" in str(response.data) + assert response.json() == INVALID_CREDENTIALS_ERROR def test_api_applications_generate_token_invalid_client_secret(): @@ -323,7 +409,7 @@ def test_api_applications_generate_token_invalid_client_secret(): ) assert response.status_code == 401 - assert "Invalid credentials" in str(response.data) + assert response.json() == INVALID_CREDENTIALS_ERROR def test_token_unknown_client_id_with_valid_secret(): @@ -344,7 +430,7 @@ def test_token_unknown_client_id_with_valid_secret(): ) assert response.status_code == 401 - assert "Invalid credentials" in str(response.data) + assert response.json() == INVALID_CREDENTIALS_ERROR def test_token_rejects_secret_owned_by_another_application(): @@ -366,7 +452,7 @@ def test_token_rejects_secret_owned_by_another_application(): ) assert response.status_code == 401 - assert "Invalid credentials" in str(response.data) + assert response.json() == INVALID_CREDENTIALS_ERROR def test_api_applications_generate_token_inactive_application(): @@ -388,7 +474,10 @@ def test_api_applications_generate_token_inactive_application(): ) assert response.status_code == 401 - assert "Application is inactive" in str(response.data) + assert response.json() == { + "error": "invalid_client", + "error_description": "Application is inactive", + } def test_api_applications_generate_token_inactive_application_wrong_secret(): @@ -409,7 +498,7 @@ def test_api_applications_generate_token_inactive_application_wrong_secret(): ) assert response.status_code == 401 - assert "Invalid credentials" in str(response.data) + assert response.json() == INVALID_CREDENTIALS_ERROR assert "inactive" not in str(response.data).lower() @@ -431,7 +520,10 @@ def test_api_applications_generate_token_invalid_email_format(): ) assert response.status_code == 400 - assert "scope should be a valid email address." in str(response.data).lower() + assert response.json() == { + "error": "invalid_scope", + "error_description": "Scope should be a valid email address.", + } def test_api_applications_generate_token_domain_not_authorized(): @@ -454,7 +546,12 @@ def test_api_applications_generate_token_domain_not_authorized(): ) assert response.status_code == 403 - assert "not authorized for this email domain" in str(response.data) + assert response.json() == { + "error": "invalid_scope", + "error_description": ( + "This application is not authorized for this email domain." + ), + } def test_api_applications_generate_token_domain_authorized(): @@ -502,7 +599,10 @@ def test_api_applications_generate_token_user_not_found(): ) assert response.status_code == 404 - assert "User not found" in str(response.data) + assert response.json() == { + "error": "invalid_scope", + "error_description": "User not found.", + } @freeze_time("2023-01-15 12:00:00") @@ -715,7 +815,7 @@ def test_api_applications_token_new_user_race_condition(mock_get_by_email, setti def test_api_applications_token_new_user_race_condition_unrecoverable( mock_get_or_create, settings ): - """Should return 500 when ProvisionalUserIntegrityError is raised.""" + """Should return 409 when ProvisionalUserIntegrityError is raised.""" settings.APPLICATION_ALLOW_USER_CREATION = True settings.OIDC_FALLBACK_TO_EMAIL_FOR_IDENTIFICATION = True @@ -739,6 +839,10 @@ def test_api_applications_token_new_user_race_condition_unrecoverable( ) assert response.status_code == 409 + assert response.json() == { + "error": "server_error", + "error_description": "Failed to create or retrieve provisional user.", + } assert mock_get_or_create.call_count == 1 @@ -1069,6 +1173,9 @@ def test_api_applications_generate_token_domain_denied_is_audited(audit_events): assert event["entity"]["target"]["id"] == str(application.pk) assert event["lasuite"]["details"] == {"requested_domain": "random.com"} assert event["http"]["response"] == {"status_code": 403} + assert event["error"] == { + "message": "This application is not authorized for this email domain." + } def test_api_applications_generate_token_invalid_email_is_audited(audit_events): @@ -1084,6 +1191,7 @@ def test_api_applications_generate_token_invalid_email_is_audited(audit_events): assert event["event"]["reason"] == "validation_error" assert event["lasuite"]["actor"] == {"type": "application"} assert event["http"]["response"] == {"status_code": 400} + assert event["error"] == {"message": "Scope should be a valid email address."} assert "details" not in event["lasuite"] @@ -1102,6 +1210,7 @@ def test_api_applications_generate_token_unknown_user_is_audited(audit_events): assert event["event"]["reason"] == "not_found" assert event["lasuite"]["details"] == {"requested_domain": "example.com"} assert event["http"]["response"] == {"status_code": 404} + assert event["error"] == {"message": "User not found."} def test_api_applications_generate_token_provisioning_is_audited(