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

Login hooks. #1815

Merged
merged 7 commits into from Mar 13, 2014

Conversation

Projects
None yet
5 participants
@awwx
Contributor

awwx commented Feb 5, 2014

Adds Accounts.validateLoginAttempt, Accounts.onLogin, and
Accounts.onLoginFailure.

The process for logging in a user is consolidated in accounts-base,
which is now the only package which directly accesses login tokens in
the database.

All login methods now go through Accounts._loginMethod, which
ensures that exceptions are captured and login hooks are called in all
cases.

The callback hook implementation code from livedata is extracted into
an internal callback-hook package, where it can be used by accounts.

Login hooks.
Adds `Accounts.validateLoginAttempt`, `Accounts.onLogin`, and
`Accounts.onLoginFailure`.

The process for logging in a user is consolidated in accounts-base,
which is now the only package which directly accesses login tokens in
the database.

All login methods now go through `Accounts._loginMethod`, which
ensures that exceptions are captured and login hooks are called in all
cases.

The callback hook implementation code from livedata is extracted into
an internal `callback-hook` package, where it can be used by accounts.
@jagill

This comment has been minimized.

Contributor

jagill commented Feb 5, 2014

@awwx, you make me happy.

/// LOGIN HOOKS
///
var validateLoginHook = new Hook();

This comment has been minimized.

@n1mmy

n1mmy Feb 11, 2014

Member

Maybe comment on why no string here? (exceptions handled specially)

This comment has been minimized.

@awwx

awwx Feb 11, 2014

Contributor

Or maybe the Hook constructor should take an options argument which explicitly describes which exception handling method to use?

validateLoginHook.each(function (callback) {
var ret;
try {
ret = callback(attempt);

This comment has been minimized.

@n1mmy

n1mmy Feb 11, 2014

Member

Would it be silly to add in a safety belt in case a validate login callback mistakenly sets attempt.allowed to true? (e.g. immediately after each callback runs, we could make sure that attempt.allowed is equal to what it was before the callback ran)

I'm torn between "it's ridiculous to try to protect against all possible developer mistakes" and "extra safety belt can't hurt, and if somehow someone decides to set attempt.allowed to true for some silly reason, that definitely shouldn't cause the login attempt to succeed if a different callback returned falsey previously".

In fact, maybe we should just clone attempt before passing it into each callback?

This comment has been minimized.

@awwx

awwx Feb 11, 2014

Contributor

I agree, cloning attempt is a good idea.

This comment has been minimized.

@n1mmy

n1mmy Feb 13, 2014

Member

+1 cloning

//
// options:
// optional object merged into the result returned by the login
// method.

This comment has been minimized.

@n1mmy

n1mmy Feb 11, 2014

Member

Maybe comment on what this is used for (HAMK from SRP)?

// Login a user on a connection.
//
// We use the method invocation to set the user id on the connection

This comment has been minimized.

@n1mmy

n1mmy Feb 11, 2014

Member

Hrm? I don't understand this.

This comment has been minimized.

@awwx

awwx Feb 11, 2014

Contributor

We're really setting the user id on the connection, so the connection could have a setUserId method available, and we could be passing the connection around instead of the method invocation. But that would bypass the unblock check.

This comment has been minimized.

@n1mmy

n1mmy Feb 13, 2014

Member

Oooh, I gotcha. How about something like:

We use the method invocation to set the user id on the connection, not the connection object directly. setUserId is tied to methods to enforce clear ordering of method application (using wait methods on the client, and a no setUserId after unblock restriction on the server)

///
/// LOGIN HANDLERS
///
// The main entry point for auth packages to hook in to login.
//
// A login handler is a login method which can return `undefined` to

This comment has been minimized.

@n1mmy

n1mmy Feb 11, 2014

Member

It looks like this was changed from undefined to false-y.

I think we used to use undefined so a handler could return null to indicate that it handled and blocked the login. It looks like we're doing that with explicit object returns now.

These comments should be updated.

This comment has been minimized.

@awwx

awwx Feb 11, 2014

Contributor

Or require the handler to either return undefined or a result object?

var result = tryLoginMethod(
handler.name,
function (knownUserId) {

This comment has been minimized.

@n1mmy

n1mmy Feb 11, 2014

Member

Hrm... threading the knownUserId callback around like this feels a little awkward.

Could we use an EnvironmentVariable (ala DDP._CurrentInvocation) to get this info around?

This comment has been minimized.

@awwx

awwx Feb 11, 2014

Contributor

There's an argument in favor of explicitness: we have an unusual control flow, and it's easier to follow when you can see how it's threaded.

Using an EnvironmentVariable would mean that in theory any function a login handler called could set the known user id, so there would be more places to look to see where it is being set.

This comment has been minimized.

@awwx

awwx Feb 11, 2014

Contributor

We could have a login handler invocation object passed through this.

This comment has been minimized.

@n1mmy

n1mmy Feb 13, 2014

Member

Hrm... I think this needs some reworking. All 3 reviewers were kinda confused by knownUserId =)

A couple ideas:

  1. just get rid of it. Callsites can already return an object with userId set and not allowed. I think all the current callsites that do this (know a userid but can reject) can simply change a throw into a return. Much simpler.

  2. define a new exception class LoginError and have login handlers throw this instead when the want to report "clean" failure (eg, an expected case, not a programmer error)

This comment has been minimized.

@awwx

awwx Feb 13, 2014

Contributor

As long as you don't mind unexpected errors to result in the user object not being included in attempt info.

This comment has been minimized.

@n1mmy

n1mmy Feb 13, 2014

Member

Yeah, I think thats an OK tradeoff for code simplicity.

};
// Used by tests.

This comment has been minimized.

@n1mmy

n1mmy Feb 11, 2014

Member

nitpicky but can we change this to "Exported for tests"? I understood "Used by tests" to mean "ONLY used by tests"

@n1mmy

This comment has been minimized.

Member

n1mmy commented Feb 11, 2014

This is looking really good, @awwx. Nice work!

A couple comments inline from a first pass. Still need to dive a little deeper, expect more comments to be forthcoming.

}
if (! ret) {
attempt.allowed = false;
attempt.error = new Meteor.Error(403, "Login Disallowed");

This comment has been minimized.

@n1mmy

n1mmy Feb 13, 2014

Member

-> "Login forbidden". Clearer and less technical sounding

};
// Login a user on a connection.

This comment has been minimized.

@n1mmy

n1mmy Feb 13, 2014

Member

s/Login/Log in/. "Login" makes a good noun or adjective but not a super good verb.

// which aren't successful (such as an invalid password, etc).
//
// If the login is allowed and isn't aborted by a validate login hook
// callback, login the user.

This comment has been minimized.

@n1mmy

n1mmy Feb 13, 2014

Member

nitpick s/login the user/log the user in/

query._id = userId;
Meteor.users.update(
query,
{$addToSet: {

This comment has been minimized.

@n1mmy

n1mmy Feb 13, 2014

Member

style: { $addToSet: (space inside brace)

"Please login again.");
}
if (! user)
throw new Meteor.Error(403, "You've been logged out by the server. Please login again.");

This comment has been minimized.

@n1mmy

n1mmy Feb 13, 2014

Member

s/login again/log in again/

(and also the other pre-existing site right below)

// - Forgetting about the reset token that was just used
// - Verifying their email, since they got the password reset via email.
var affectedRecords = Meteor.users.update(
{_id: user._id, 'emails.address': email},

This comment has been minimized.

@n1mmy

n1mmy Feb 13, 2014

Member

Unrelated to this change, but we should probably include { 'services.password.reset.token': token } in the selector here. (To be super paranoid about password reset tokens being one-time use.)

@@ -0,0 +1,100 @@
// Encapsulates the pattern of registering callbacks on a hook.

This comment has been minimized.

@n1mmy

n1mmy Feb 13, 2014

Member

This pattern is cool! It spawned a long discussion last night about how to handle hooks in general, and a plan for a better public API for hooks (eg replacing Meteor.onConnection and these login hooks with a different syntax).

I think it's probably better to go ahead with this syntax for now, get it merged and shipped, then do the new API as a separate project. More API churn for users, but I think tactically easier to build.

For now, can we just put a big XXX comment at the top here warning people off using this in more places in the mean time. Something like:

XXX this pattern is under development. Do not add more callsites using this package for now.

@n1mmy

This comment has been minimized.

Member

n1mmy commented Feb 13, 2014

Hey Andrew, comments from another round of review are above. I think the main functional thing is knownUserId, plus a bunch of misc small things.

@n1mmy

This comment has been minimized.

Member

n1mmy commented Feb 13, 2014

Oh, also, Emily just merged sso to devel which will probably conflict a bit with this. Can you rebase or merge on top of devel?

awwx added some commits Feb 16, 2014

Clone the attempt object passed to login hooks, so the original can't
be modified by hook callbacks.

Give the Hook constructor a named option to debug print and ignore
exceptions, as just passing in a string argument to do that wasn't
very clear.
Drop `knownUserId`.
This comes at the expense of the password login methods which now need
to explicitly return the user id with errors, but makes for a less
confusing flow of control.
@mizzao

This comment has been minimized.

Contributor

mizzao commented Mar 4, 2014

Do these hooks allow for interception of resume logins as well? If so, they could be really help for the wonky issue I'm dealing with in #1835.

@awwx

This comment has been minimized.

Contributor

awwx commented Mar 4, 2014

@mizzao Yes, the resume login handler is a login handler, and so is treated the same as the others.

@timhaines

This comment has been minimized.

Contributor

timhaines commented Mar 11, 2014

This is going to be helpful.

@n1mmy n1mmy merged commit 645e23c into meteor:devel Mar 13, 2014

1 check passed

default The author has signed the Meteor Contributor Agreement.
Details
@n1mmy

This comment has been minimized.

Member

n1mmy commented Mar 13, 2014

Merged! Thanks, @awwx. Nice work.

@mizzao

This comment has been minimized.

Contributor

mizzao commented Mar 13, 2014

Sweet! Thanks all! This should give me something to chew on for a while.

@mizzao

This comment has been minimized.

Contributor

mizzao commented Mar 13, 2014

@awwx: any chance these login hooks make available the IP address grabbed from the code that you pushed earlier?

@n1mmy

This comment has been minimized.

Member

n1mmy commented Mar 13, 2014

@mizzao Yup! attempt.connection.clientAddress

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