Pass options into server-side log-in lifecycle hooks

[nazrhyn]

Details

Currently, any options provided by the log-in handler that services a log-in attempt are not passed to the server-side Accounts.onLogin and .onLoginFailure lifecycle hooks as part of the attempt. We use these lifecycle hooks for various integrations and application features, and it would be nice to be able to transport custom information between those two locations. Currently, we're doing something very unpleasant to bridge that gap.

This change modifies Accounts._attemptLogin, following the pattern for error and user, attaching options to the attempt object being built there. This is really the only material change, but I've also done some more work.

Related to this change is the following PR where a co-worker of mine made a similar change for the client-side userCallback of Accounts.callLoginMethod. My change brings parity on the server-side, making sure options are passed everywhere they might be useful.

• pass full result to userCallback #11913

As a gesture of good will 😇, I have also written a small suite of tests that covers:

1. Server-side log-in in several permutations
   • Checks the Accounts.validateLoginAttempt hook
   • Checks the Accounts.onLogin hook
   • Checks the Accounts.onLoginFailure hook
2. Client-side successful log-in
   (These tests essentially cover my co-worker's PR changes from before.)
   • Checks the validateResult callback
   • Checks the userCallback callback

Potentially Contentious Things

• I created new modules to house these tests, as I knew I would want some helpers and I didn't necessarily want to just jam all of this stuff into the existing modules. Please let me know if you'd like changes to how this is organized.
• I see you have a Prettier config, but when I ran it, it produced an output that seems to indicate that it is not often being run, based on comparisons with existing code. Please let me know if you want me to run it, even if it produces inconsistent output.
• You didn't have any concept of a mock function, as far as I could see, and I wanted to use something like that to easily record calls without having to write that functionality over and over. I created a MockFunction class and added it to test-helpers. I considered it to be an expedient in writing these tests, but if it's too new or out of line with what you want in your test suite, please let me know.
• I think I understood what you were doing with accounts_tests_setup.js, but please let me know if I did that wrong.
• As far as I could see, you didn't have any existing tests that directly test the server-side Accounts.registerLoginHandler function with a test-only handler. I wanted to write tests that were focused in scope, and so that's how I went about it, rather than relying on existing handlers that might've been added in other packages.

[CLAassistant]

All committers have signed the CLA.

[nazrhyn]

I completely understand if you want me to revert this and just add the single thing to make this diff better. I just ... really wanted to make this cleaner for the future.

[StorytellerCZ]

I'm fine with this.

[nazrhyn]

Aside: running the test suite locally to verify it's all passing, it finished like this:

"All tests pass!" but "Passed 1598 of 1599" 🤔

EDIT: Oh hmm, maybe this is significant. This is what happened in CI:
https://app.travis-ci.com/github/meteor/meteor/builds/269881541#L5522

Tests complete with 1 failures
Tests complete with 1598 passes
S: tinytest - livedata server - onMessage hook failed: {"groupPath":["tinytest","livedata server"],"test":"onMessage hook","events":[{"sequence":2829,"type":"fail","details":{"type":"string_equal","expected":"livedata_server_test_inner","actual":"tinytest/getCurrentRunningTestName"}}],"server":true}

I'm not seeing what that 1 failure is, though... hopefully not related to my change.

[denihs]

Hey @nazrhyn, do you need this for Meteor 2? It might take a while before we have another version 2 release. But we could include this in the next 3.0-rc

[nazrhyn]

@denihs Thanks for the follow up!

We have a local fork that includes this change in our current version, so there's no urgency on our side. We just wanted to contribute this back in case the Meteor team liked it.

[StorytellerCZ]

LGTM, the only thing is that I would probably target this on the 3.x branch.

[nazrhyn]

Would you like me to rebase it or is that something y'all will do? I don't mind, just need the exact branch name.