Skip to content

AuthorizationMiddleware returns a legacy error envelope instead of problem details #544

Description

@arhimede

Every error response in the application is an RFC 9457 problem details document, produced by ProblemDetailsMiddleware from an exception implementing ProblemDetailsExceptionInterface — except the one clients hit most often.

Api\App\Middleware\AuthorizationMiddleware never throws ForbiddenException. It builds the response itself:

protected function unauthorizedResponse(string $message): ResponseInterface
{
    return new JsonResponse([
        "error" => [
            "messages" => [
                $message,
            ],
        ],
    ], StatusCodeInterface::STATUS_FORBIDDEN);
}

Because no throwable reaches ProblemDetailsMiddleware, the body is the pre-problem-details envelope:

{
    "error": {
        "messages": [
            "Resource not allowed."
        ]
    }
}

rather than the title / type / status / detail document every other error returns. Clients therefore have to parse two unrelated error shapes, and the shape they need for authorization failures is the one no longer documented as the API format.

Second problem: the status is 403 for all six cases

unauthorizedResponse() is called from six places, and hardcodes 403 for all of them:

Condition Current Arguably correct
RBAC check not granted (Message::RESOURCE_NOT_ALLOWED) 403 403
Message::ADMIN_NOT_FOUND 403 401
Message::ADMIN_INACTIVE 403 401
Message::USER_NOT_FOUND 403 401
Message::USER_NOT_ACTIVATED 403 401
Message::INVALID_CLIENT_ID 403 401

Only the first is genuinely a role failure. The other five are failures to establish an identity at all, which is what 401 is for — and the method name says unauthorized while the status says 403.

Suggested fix

Throw ForbiddenException::create() for the RBAC failure and UnauthorizedException::create() for the identity failures, and let ProblemDetailsMiddleware render both. Both exceptions already exist with the right statuses. That unifies the error format and corrects the statuses in one change.

This is a breaking change for any client parsing the current envelope, so it likely belongs in a major release.

Found while auditing the v7 documentation against 7.0 at commit 45ad282.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Labels

No labels
No labels

Type

No type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions