Error response should allow generating other HTTP responses than 302 for errors related to redirect URI

[Krzysztof Trojan]

The spec requires the authorization server NOT TO redirect the client to an invalid URI, if such error is detected, in particular it is forbidden to redirect to an URI that is not on the list of registered URIs, if those are associated with the given client_id.

However, the current code, even for an error response that relates to invalid URI, generates 302 redirection blindly.

    @Override
    public HTTPResponse toHTTPResponse() {

        if (ResponseMode.FORM_POST.equals(rm)) {
            throw new SerializeException("The response mode must not be form_post");
        }

        HTTPResponse response= new HTTPResponse(HTTPResponse.SC_FOUND);
        response.setLocation(toURI());
        return response;
    }

See:

RFC 6749 - 4.1.2.1. Error Response

If the request fails due to a missing, invalid, or mismatching redirection URI, or if the client identifier is missing or invalid, the authorization server SHOULD inform the resource owner of the error and MUST NOT automatically redirect the user-agent to the invalid redirection URI.

[Krzysztof Trojan]

I have temporarily worked around the issue, by extending the class, and using a marker URL:

public class ImprovedAuthorizationErrorResponse extends com.nimbusds.oauth2.sdk.AuthorizationErrorResponse {

    public final static URI ERROR_MARKER_URI = URIInitHelper.createMarkerURI();

    /**
     * Creates a new authorisation error response.
     *
     * @param redirectURI The base redirection URI. May be null.
     * @param error       The error. Should match one of the
     *                    {@link #getStandardErrors standard errors} for an
     *                    authorisation error response. Must not be
     *                    {@code null}.
     * @param state       The state, {@code null} if not requested.
     * @param rm          The implied response mode, {@code null} if
     */
    public ImprovedAuthorizationErrorResponse(URI redirectURI, ErrorObject error, State state, ResponseMode rm) {
        super(redirectURI!=null ? redirectURI : ERROR_MARKER_URI, error, state, rm);
    }

    @Override
    public HTTPResponse toHTTPResponse() {
        if (ERROR_MARKER_URI.equals(getRedirectionURI())) {
            HTTPResponse response= new HTTPResponse(HTTPResponse.SC_BAD_REQUEST);
            try {
                response.setContentType("application/json");
            } catch (ParseException e) {
                // ignored on purpose, we know what we are setting!
            }
            response.setContent(JSONObject.toJSONString(toParameters()));
            return response;
        }

        return super.toHTTPResponse();
    }

    // somewhat dirty way to initialize a constant with an URI, ignoring the exceptions (not possible to happen)
    private static class URIInitHelper {
        static URI createMarkerURI() {
            try {
                return new URI("error://uri");
            } catch (Exception ex){
                // is not to happen!
            }
            return null;
        }
    }
}

[Vladimir Dzhuvinov]

Hi,

Thank you for writing.

I think there was some confusion here. The AuthorizationErrorResponse class is intended for situations when an error was encountered and it is allowed to be communicated back to the client (hence the Response in the class name).

If the error is such that redirection cannot or must not happen (e.g. invalid client_id), the error condition should just be displayed back to the user (at the Authorisation Server or OpenID provider).

In the Connect2id server, which builds on top of this SDK, we have defined a NonRedirectableError class to handle that, which outputs the following message:

https://connect2id.com/products/server/docs/integration/authz-session#authz-error

There are no standard codes for such errors (the error codes listed in the RFC apply only for the errors which may be redirected).

A class to represent non-redirectable errors was deliberately not included in the spec, as this falls outside the spec, and we don't think we have a universal recipe how to go about this.

I wonder what can be done to prevent incorrect usage of the AuthorizationErrorResponse class. For sure, the JavaDocs can be improved. Programmatically, I'm no sure. Do you have any suggestions?

[Vladimir Dzhuvinov]

Commit 136612c: Updates JavaDocs to state that AuthorizationErrorResponse is intended for redirectable responses only

[Vladimir Dzhuvinov]

Also for the OpenID authentication error response: 5bc302d

[Krzysztof Trojan]

I suggest to add the missing kind of response, possibly with a helper class to produce the right one from an exception.

I tend to disagree such "non-redirectable" response would be outside the spec, as is clearly shown in the specification fragment I have quoted. The behavior is the REQUIRED behavior of the authorization server (see "MUST NOT" definition in the spec preamble).

Also, please consider the use case: the non-redirectable error response will most likely be generated as a result of the parse exception from the constructor of AuthorisationRequest (or similar exception thrown when validating the URI against the list of URIs for given client). I would assume the exception should contain enough information to choose from two modes of responding (400 BAD REQUEST vs 302 with error params) - and actually, ParseException does contain enough information, just not explicitly marks it as the input to such decision).

I would suggest one of: * allowing error response to use other codes than 302, and change the logic of the constructor in a way similar to shown * adding a method to exceptions similar to "toBeCommunicatedInRedirectErrorResponse()" & "toBeReturnedAsBadRequest()", and extra class for the "400 BAD REQUEST"error message * embody the logic of the methods above into some helper, that would produce a proper kind of error response from an exception (GenericException, I guess).

[Connect2id OSS]

Pushed updated Javadoc as v5.36.1 to Maven Central.

[Vladimir Dzhuvinov]

Ok, I'll think about adding a NonRedirectingError class to the SDK, perhaps based on what we already have in the Connect2id server.

I deliberately want to avoid having "Response" in the name, because the AS / OP doesn't respond back to the client in such cases. The common UX on such error is to show a clear message to alert the end-user, so the user can report the problem to the client publisher / developer via some other mean (or if the user happens to be the client developer herself).

For that same reason, setting a particular HTTP status code in the SDK, such as 400, could be misleading, as no error is actually being relayed back to the client app, and the 400 will not really be understood by the user. The above error message can be displayed with a HTTP 200 page. We also have customers who use a single page pattern for the UI, so there that will not work either.

I will double check what happens in the parser, in regard to differentiating between redirectable error (and setting the redirect_uri) responses and such that are not.

[Krzysztof Trojan]

Returning 200 OK for an error is SEMANTICALLY incorrect.

Why not return 400 WITH some presentable content? HTTP 400 responses are allowed to have a response body, AFAIK, and it should be displayed according to its content type?

[Yavor Vasilev]

Internal OP / AS web APIs shouldn't really be covered by the OAuth SDK.