Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Authentication throws are treated as valid reply() #1581

Closed
shaunlimjin opened this issue Apr 18, 2014 · 15 comments
Closed

Authentication throws are treated as valid reply() #1581

shaunlimjin opened this issue Apr 18, 2014 · 15 comments
Assignees
Labels
Milestone

Comments

@shaunlimjin
Copy link

@shaunlimjin shaunlimjin commented Apr 18, 2014

I upgraded to hapi v4.0 from v1.2 and had to make some changes to my authentication strategy due to breaking changes introduced in 2.x.

I believe that because my authenticate() function contains a promise, it finishes executing before the promise is resolved and results in the following error:

Debug: hapi, internal, implementation, error 
    Error: Authentication response missing both error and credentials

This is my authenticate() function:

Within a Authentication object:

/**
 * Authenticate user via token
 * @param request
 * @param reply
 */
exports.authenticate = function (request, reply) {
    var token = getToken(request);
    if (token == null) {
        return reply(Hapi.boom.unauthorized('Header has to include Authorization key with value Bearer token.'));
    } else {
        //attempt to get user json object from session store
        this.Session.get(token)
            .then(function (user) {
                //successful authentication, renew key, return user json
                return reply(null, {credentials: user});
            }, function (err) {
                return reply(Hapi.boom.unauthorized('Session does not exist for token: ' + token));
            });
    }
};

And wiring it up:

server.auth.scheme('tokenScheme', function (server, options) {
     return Authentication;
});

server.auth.strategy('token', 'tokenScheme');

Everything was working fine in v1.2. Can someone please tell me what I'm doing wrong?

Thanks,
Shaun

@bendrucker
Copy link
Contributor

@bendrucker bendrucker commented Apr 19, 2014

Everything looks okay, although it's worth noting that passing two callbacks to then is an anti-pattern. Always use .then(success).catch(failure). Can you confirm that this.Session.get(token) resolves with the expected value? You can just drop a console.log before your reply.

@hueniverse
Copy link
Contributor

@hueniverse hueniverse commented Apr 19, 2014

Can you tell which reply() is being called?

@shaunlimjin
Copy link
Author

@shaunlimjin shaunlimjin commented Apr 20, 2014

Thanks for the replies! As far as I can tell, none of the reply() statements are being called.

It does not even look like the method that's supposed to resolve a promise is being called.

I modified my code for simplicity:

var Authentication = {
authenticate: function (request, reply) {
            this.testPromise().then(function () {
                console.log('this does not print either.');
                return reply(null, {credentials: {
                    email: 'test@test.com',
                    name: 'test'
                }});
            }).catch(function (err) {
                console.log(err);
            });
    },
testPromise: function () {
        console.log('this does not print.');
        var deferred = Q.defer();
        setTimeout(function () {
            deferred.resolve();
        }, 2000);
        return deferred.promise;
    }
};

module.exports = Authentication;

and

//Register our custom authentication strategy

server.auth.scheme('tokenScheme', function (server, options) {
     return Authentication;
});

server.auth.strategy('token', 'tokenScheme');

@hueniverse
Copy link
Contributor

@hueniverse hueniverse commented Apr 21, 2014

Did you put a console.log before the testPromise code? Also, the error in your initial issue means that some reply is being called, or the route would just hang. You need to figure out which one is returning. Also, can you post the route config for one route you are trying to authenticate with this?

@shaunlimjin
Copy link
Author

@shaunlimjin shaunlimjin commented Apr 21, 2014

A console.log before the testPromise code prints.

authenticate: function (request, reply) {
            console.log('before calling testPromise(). this prints.');
            this.testPromise().then(function () {
                console.log('this does not print either.');
                return reply(null, {credentials: {
                    email: 'test@test.com',
                    name: 'test'
                }});
            }).catch(function (err) {
                console.log(err);
            });
    }

This is an example of a route config:

    {
        path: '/clients',
        method: 'GET',
        config: {
            auth: 'token',
            handler: Admin.getClients
        }
    }

and the handler code:

/**
 * Get list of clients.
 * @param request
 * @param reply
 */
exports.getClients = function (request, reply) {
    console.log('getClients() requested by: ' + request.auth.credentials.email);
    ZServices.getClients(function (error, responseCode, body) {
        if (!error) {
            reply(body).code(responseCode);
        } else {
            reply(error).code(responseCode);
        }
    });
};

You're right, the route just hangs if I don't make any function calls in the authenticate function:

 authenticate: function (request, reply) {
        console.log('this just hangs, which is expected');
    }

If we call just any random function, we immediately get the error:

 authenticate: function (request, reply) {
        just_a_function();
    }

@bendrucker
Copy link
Contributor

@bendrucker bendrucker commented Apr 21, 2014

So your first example says reply was called with bad params? Don't see how that's possible if that message is never logged.

@shaunlimjin
Copy link
Author

@shaunlimjin shaunlimjin commented Apr 21, 2014

@bendrucker Exactly, I must be doing something really wrong.

For example, reply() is never called in this snippet, but the same error is thrown:

 authenticate: function (request, reply) {
        just_a_function();
    }

Full stack trace:

Debug: hapi, internal, implementation, error 
    Error: Authentication response missing both error and credentials
    at Object.exports.create (/Users/shaun/workspace/orchestration/node_modules/hapi/node_modules/boom/lib/index.js:21:17)
    at Object.exports.internal (/Users/shaun/workspace/orchestration/node_modules/hapi/node_modules/boom/lib/index.js:235:92)
    at Object.exports.badImplementation (/Users/shaun/workspace/orchestration/node_modules/hapi/node_modules/boom/lib/index.js:271:23)
    at internals.Auth._authenticate.validate (/Users/shaun/workspace/orchestration/node_modules/hapi/lib/auth.js:206:30)
    at internals.Protect.run.finish (/Users/shaun/workspace/orchestration/node_modules/hapi/lib/protect.js:48:21)
    at exports.once.wrapped (/Users/shaun/workspace/orchestration/node_modules/hapi/node_modules/hoek/lib/index.js:484:20)
    at internals.Protect.run._error (/Users/shaun/workspace/orchestration/node_modules/hapi/lib/protect.js:55:16)
    at Domain.module.exports.internals.Protect (/Users/shaun/workspace/orchestration/node_modules/hapi/lib/protect.js:29:20)
    at Domain.EventEmitter.emit (events.js:95:17)
    at process._fatalException (node.js:249:27)

@bendrucker
Copy link
Contributor

@bendrucker bendrucker commented Apr 21, 2014

Are you sure you have your bind option set properly? Seems like that's the problem and that this.Session is undefined.

@shaunlimjin
Copy link
Author

@shaunlimjin shaunlimjin commented Apr 21, 2014

@bendrucker Can you please elaborate on what you mean?

Anyway, if you look at my later code snippets, I stopped using this.Session and started using dummy functions.

@bendrucker
Copy link
Contributor

@bendrucker bendrucker commented Apr 21, 2014

Actually there's no bind property globally available for the server. So never mind that.

I'm assuming your dummy code is as-written and you're calling non-existent functions and triggering ReferenceErrors. Yes?

@shaunlimjin
Copy link
Author

@shaunlimjin shaunlimjin commented Apr 21, 2014

Nope, I fleshed out dummy functions as well.

{
authenticate: function (request, reply) {
            console.log('before calling testPromise(). this prints.');
            this.testPromise().then(function () {
                console.log('this does not print either.');
                return reply(null, {credentials: {
                    email: 'test@test.com',
                    name: 'test'
                }});
            }).catch(function (err) {
                console.log(err);
            });
    },
 testPromise: function () {
        console.log('this does not print.');
        var deferred = Q.defer();
        setTimeout(function () {
            deferred.resolve();
        }, 2000);
        return deferred.promise;
    },
    justafunction: function () {
        console.log('just a function called');
    }
}
}

@bendrucker
Copy link
Contributor

@bendrucker bendrucker commented Apr 21, 2014

Your testPromise method is clearly not being called and it's because this.testPromise is undefined. You shouldn't rely on the this value when using a framework because you don't know how your method will be called. For example, I can break your code with:

var authenticate = Authentication.authenticate;
authenticate(); // => TypeError undefined has no method 'testPromise'

This may seem contrived but with a framework it's hard to know how your methods will be called. You should assume that the context will be lost unless the docs specify otherwise.

Take a look at where your strategy will actually be called. authenticate is called on null.

@bendrucker
Copy link
Contributor

@bendrucker bendrucker commented Apr 22, 2014

Although this does unearth a separate issue, which is that authentication functions swallow exceptions

@shaunlimjin
Copy link
Author

@shaunlimjin shaunlimjin commented Apr 22, 2014

@bendrucker Thanks for your help!

I fixed my code by using closures to retain the reference to 'this':

//Register our custom authentication strategy
server.auth.scheme('token-auth-scheme', function(server, options) {
   return Authentication.tokenAuthentication();
});

server.auth.strategy('token', 'token-auth-scheme');
    tokenAuthentication: function () {
        var self = this;
        return {
            authenticate: function (request, reply) {
                var token = self.getToken(request);
                if (token == null) {
                    callback(Hapi.boom.unauthorized('Header has to include Authorization key with value Bearer token.'));
                } else {
                    //attempt to get user json object from redis
                    self.Session.get(token)
                        .then(function (user) {
                            //successful authentication, renew key, return user json
                            return reply(null, { credentials: user });
                        }).catch(function (err) {
                            return reply(Hapi.boom.unauthorized('Session does not exist for token: ' + token));
                        });
                }
            }
        }
    }

@hueniverse
Copy link
Contributor

@hueniverse hueniverse commented Apr 22, 2014

@bendrucker nice catch. The issue is that when the function throws, it is treated as a valid reply call.

@hueniverse hueniverse added the bug label Apr 22, 2014
@hueniverse hueniverse changed the title [Authentication] Promise in authenticate function Authentication throws are treated as valid reply() Apr 22, 2014
@hueniverse hueniverse added this to the 5.0.1 milestone May 20, 2014
@hueniverse hueniverse self-assigned this May 20, 2014
@lock lock bot locked as resolved and limited conversation to collaborators Jan 12, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants