Support for Non-401 Error Codes from the validateFunc

Oliver-Akins commented 2 years ago

Support plan

is this issue currently blocking your project? (yes/no): No
is this issue affecting a production system? (yes/no): No

Context

node version: v14.17.0
module version: 11.0.2
environment (e.g. node, browser, native): Node
used with (e.g. hapi application, another framework, standalone, ...): Hapi application (Hapi version = 20.2.1)
any other relevant information:

What problem are you trying to solve?

I have a system that involves multiple different "regions" of authorization, I want a cookie to only be valid for one of these "regions", and I have added validation checks into the validateFunc. I would love be able to respond to the client with a 403 Forbidden when the cookie provided is for a different "region" than that which it is trying to access.

Example: I have "regions" 1 and 2, and an authorization cookie is used for region 1, but the user is making a request to GET /region/2 I would like to be able to throw boom.forbidden(), and it set the response code to 403 instead of the plugin only throwing 401 to the user.

Do you have a new or modified API suggestion to solve the problem?

I think a solution following a similar vein as to how @hapi/basic does it where if the validateFunc throws an error(/Boom error) it replaces the default boom.unauthorized()

From the @hapi/basic API documentation for the validate function:

Throwing an error from this function will replace default Boom.unauthorized error

devinivy commented 2 years ago

In the code and tests, the module is fairly explicit about what it's trying to do when it sees a non-unauthorized error: https://github.com/hapijs/cookie/blob/fa728d73095278b34186797f3f42cf032eea8eca/test/index.js#L374-L414

That means that we'd need to treat this as a breaking change, most likely.

Since the original error is preserved on the error's data property, you do have an option to get this behavior using a request extension. Something like this should do the trick https://runkit.com/devinivy/627987b9369f5200089451e0:

server.ext({
    type: 'onPreResponse',
    method: (request, h) => {
        const error = request.response;
        if (Boom.isBoom(error) && error.output.statusCode === 401 && error.data instanceof Error) {
            // Preserve original error from Boom.unauthorized()
            return error.data;
        }
        return h.continue;
    }
});

Oliver-Akins commented 2 years ago

Okay, that makes sense, I can definitely see this being suited as a breaking change since it does change a substantial amount of the existing behaviour.

Thanks for the snippet! It seems like a suitable stand-in for how I'm wanting the erroring to behave.

I think this could be a nice thing to include in a future major-version release, but I am also satisfied with this resolution if this isn't something that is desired within the API.

hapijs / cookie