hapijs / cookie

Cookie authentication plugin
Other
229 stars 100 forks source link

Inconsistent behavior for redirectTo and mode: 'required' #191

Closed wallyfoo closed 5 years ago

wallyfoo commented 6 years ago

I truly hope this is an implementation error on my part, but I'm seeing what feels like an inconsistent application of the redirectTo behavior in my application.

Background: My happy-auth-cookie wrapper works. It sets the cookie, users log in, scope is applied. If a user has the correct scope in their session, then they can get to the correct pages.

Consider the following route:

"use strict";

module.exports = {
    method: "GET",
    path: "/admin",
     options: {
        auth: {
            mode: 'required',
            strategy: 'session',
            scope: 'admin'
        },
        plugins: {
            'hapi-auth-cookie': {
                redirectTo: '/'
            }
        },
        async handler(request, h) {

            // sensible defaults for template data
            let templateData = {
                title: "Home",
            }

            return h.view( "admin-home", templateData);
        }
    }
};

If a "guest" user were to visit -- that is, no logged in session -- and choose to try the /admin route, they're redirected to the root of the site, no problems at all.

If an "admin" user logs in, visits the /admin route, then all is available to them.

If any other logged in, scoped user visits the same route they are greeted with a Boom message, and not a redirect to the site root:

{
"statusCode": 403,
"error": "Forbidden",
"message": "Insufficient scope"
}

My configuration options for the server.auth.strategy are simple:

password: "...", // redacted cookie secret
 cookie: "remember", // Cookie name
redirectTo: "/login", // sensible default
isSecure: false,

Again, this configuration works. Cookies are set, logins are processed, scopes are applied.

But the redirect won't work if the user is logged in. If I'm missing something obvious, I'm more than happy to get educated.

sholladay commented 6 years ago

Your implementation isn't doing anything wrong, redirectTo only redirects when the user is unauthenticated, not when they are unauthorized.

Do you actually want to send your users to the login page when they are unauthorized? I feel like that's a bit strange, given that they most likely cannot upgrade their privileges there. I think a better solution would be to use optional mode and render your view with a friendly error message when request.auth.isAuthorized is false.

wallyfoo commented 6 years ago

In this case, it's an admin route. So yes, if someone stumbles on it, I just want them immediately kicked.

sholladay commented 6 years ago

Agreed they should be kicked. But to the login page? They are already logged in, so that would be confusing. My two cents: I think you should show them a message along the lines of "Hey, you're not allowed to go there. To resume what you were doing, hit the back button or click here."

Either way, you'll need optional mode to achieve your desired behavior or my recommendation. There's no way, currently, to make redirectTo trigger for authorization problems. But you can easily do so yourself in the route handler.

wallyfoo commented 6 years ago

The location of the redirect isn't relevant. : ]

I still suggest that the redirect working for one kind of auth failure (no auth) and not another kind (insufficient scope) is inconsistent. Failure is failure.

sholladay commented 6 years ago

Authentication failures are not fatal, because a user can rectify the situation by logging in. That's what the feature is designed for.

Authorization failures, on the other hand, are fatal. Hence the error response.

I wouldn't have any strong feelings about adding another option for authorization redirects. But they should definitely remain separated.

FWIW, this is all you have to do to achieve this in userland:

module.exports = {
    method: "GET",
    path: "/admin",
     options: {
        auth: {
            mode: 'optional',
            strategy: 'session',
            scope: 'admin'
        },
        plugins: {
            'hapi-auth-cookie': {
                redirectTo: false
            }
        },
        async handler(request, h) {
            if (!request.auth.isAuthenticated || !request.auth.isAuthorized) {
                return h.redirect('/');
            }
            // sensible defaults for template data
            let templateData = {
                title: "Home",
            }

            return h.view( "admin-home", templateData);
        }
    }
};
wallyfoo commented 6 years ago

👍🏼

wallyfoo commented 6 years ago

@sholladay I appreciate your input, and I'm not sure what's going on. I literally copied and pasted your code and even with optional, I still get:

{"statusCode":403,"error":"Forbidden","message":"Insufficient scope"}

I have tried every permutation auth mode I can think of. Still fails regardless of what I pass in the handler. This still doesn't strike me as the correct behavior.

sholladay commented 6 years ago

Can you see if try mode works instead of optional? That shouldn't be necessary, though. I will take a look, maybe this evening. It's definitely supposed to let the route handler execute when the mode is not required. Could be a bug, but there are tests for this. I'll post a link to a working example when I can.

wallyfoo commented 6 years ago

I've attempted both try and optional with the same result.

sholladay commented 6 years ago

Okay, confirmed that my example code doesn't work. I'm talking it over with others on Slack now to make sure it's not a bug. It appears to be the intended behavior, though not what I expected based on the documentation and how authentication works (as opposed to authorization). That is to say, the route handler in my example code will run if the user is not logged in and I thought it was supposed to do the same for authorization as of hapi 17 (when request.auth.isAuthorized was introduced).

While I disagree that hapi-auth-cookie should redirect for scope violations, hapi core should absolutely let your handler execute when mode is try or optional. And this should allow you to decide for yourself whether to redirect. That's my desired behavior (and what I believed was already possible). However, the fact that it immediately returns 403 Forbidden when there is a scope violation appears to be "working as designed". I am optimistic that they will be open to changing this.

sholladay commented 6 years ago

In the meantime, you can handle authenticated but unauthorized requests from the onPreResponse: extension point (I tested to make sure).

server.ext('onPreResponse', (request, h) => {
    if (request.auth.iaAuthenticated && !request.auth.isAuthorized) {
        return h.response('You need to become an admin somehow');
    }
    return h.continue;
});

That should be flexible enough to do anything you want. You can redirect, render views, etc. If you need different behavior depending on the route, you can use request.route to check which route was being accessed.

wallyfoo commented 6 years ago

I have done so. Thanks for your assistance.

hueniverse commented 5 years ago

As already explained, this is because authorization is handled later in the lifecycle. This module cannot access that information as it only deals with authentication.

lock[bot] commented 4 years ago

This thread has been automatically locked due to inactivity. Please open a new issue for related bugs or questions following the new issue template instructions.