Passwordless or Email only login (2nd)

[henrjk]

This is to address issue #124 and is a replacement for pull request #253 with a cleaner but rewritten commit history. Otherwise the content is currently the same except that it is freshly rebased and coverage is no longer in .gitignore.

The configuration and UI is described at https://github.com/henrjk/connect-docs/blob/pwless/providers/Passwordless.md

The overall contribution is fairly sizeable. However 2/3 are tests. According to git diff --numstat upstream/master.. -- '*.js' in the (non-test) javascript code 720 lines were added and 104 removed. However things counted as removed code were typically moved or refactored, for example from signin.js to render/renderSignin.js.

I did not completely squash the history to more easily identify pieces which can be reviewed individually.

However the main contribution is in file routes/passwordless.js (449 lines with comments) and its integration with routes/signin.js. These are presumably best reviewed by looking at their final state.

Summary of implementation

Changes:

routes/signin.js: main integration changes are here.
server.js: trivial change
providers/index.js: enable overriding tokenTTL.
models/OneTimeToken: one new code path when id is undefined.
oidc/determineProvider: Support {options: requireProvider}
shared oidc verifyEmailValid verifyMailerConfigured

Additions js code:

routes/passwordless.js -- the main part of this PR.
protocols/Passwordless.js -- just a stub to allow other code to not change
providers/passwordless.js -- similar to providers/password.js
errors/PasswordlessDisabledError.js
render/renderSignin: New module to allow res.render('signin') call with proper context from passwordless code. Refactored from signin.js

Noteworthy observations

1. The code is not using passport.
2. It does not plug-in to the provider architecture. It should be reviewed whether this is somehow wrong.
3. Most code handling passwordless is in file routes/passwordless.js. Most passwordless middleware seems not to be a candidate for sharing. This is my motivation to keep the passwordless specific middleware outside of the oidc folder.
4. The main integration is done with routes/signin.js. In addition the passwordless middleware may render the 'signin' view to display errors when the email is invalid for example. To allow showing the signin page with the configured visible providers this code was moved to render/renderSignin.js.

New shareable middleware is oidc/verifyEmailValid.js
and oidc/veriftMailerConfigured.js. Is usage outside of passwordless code has not yet been coded, but should be done in case this PR will ultimately be merged.

Testing

There are new tests related to passwordless in test/integration/routes and in test/unit/oidc/passwordless*.coffee The coverage with these is quite high. My shippable build reports 92.19% branch coverage and 99.26% sequence coverage.

I made some changes for unit testing in particular so that the settings middleware does not read and lock-in cached settings during setup but instead when requests come in. This simplified having tests with specific settings in place. The coffee class TestSetting helps setting up and restoring these settings.

proxyquire a new dev dependency was used for a test related to settings. This could probably be avoided if the settings initialization would be more injectible whatever that shall mean. Alternatively this test could potentially be removed from the PR prior to shippment.

Known Issues

AMR for passwordless?

Currently 'email' is used as AMR for passwordless. However this is not in the standard doc at https://tools.ietf.org/html/draft-jones-oauth-amr-values-01#section-2 .

Needs to be clarified how to handle this?

interrupted flow in popup with angular client

In password authentication the authorization happens in response to a redirect when the password is submitted.
This allows the popup when the callback is displayed to

1. send a message to the opener window
2. close itself

However this does not work when an outside email link is activated which results in a poor user experience. At the moment it is unclear to me whether there is a good way to address this.

[henrjk]

This should clarify a bit what changes were made to allow writing integration tests in a simpler way.

Changes for integration tests.

What is meant by integration tests?

There are plenty of unit tests for the existing middleware in oidc.
These allow to:

• Fairly easily stub the req, res, and next.
• Cover all code paths.

However the middleware functions typically make assumptions on what has been done previously by other middleware. A good way to test these is to have a test cover an entire route.

In the test suite there were already similar tests which cover routes of the rest API. These tests test the server on an http level with the help of the supertest library.

The intention for similar integration tests of routes related to passwordless is that their assumptions are tested at least for the green thread.

A key issue I ran into is that when the server is setup some modules would read settings at that time and set them in stone. For example the mailer is used as follows in oidc/requireVerifiedEmail.js the mailer access is done as follows:

 var mailer = require('../boot/mailer').getMailer()
 ... later in the code ... the mailer is referenced
  return function requireVerifiedEmail (req, res, next) {
        ...
           from: options.locals.from || mailer.from,

The mailer variable above does not reference the module but a function inside the module. This does not allow an integration test to easily substitute their own configuration of the mailer as the above happens early on during server setup.

This also makes a test potentially behave differently depending on the configuration which would be present for example in config/development.json.

Once can use proxyquire to replace the mailer module for an integration test. However while this ultimately worked, it took me several hours to come up with a single test that worked both when run individually and in the entire integration suite. This process was overly complex and the result would be fragile and make this test more of debt than an advantage.

My conclusion were:

• Using proxyquire should be a last resort as it requires to encode exact details about the loading of modules.
• proxyquire seems particularly ill suited for integration type tests as the test surfaces which modules load a particular stubbed module.
• Simplifying the way to stub modules such as the mailer would be a good idea.

A simpler way to replace modules during a test

Node caches module loading. Require of a particular module will return the same reference.

A test can replace a modules properties for the duration of the test by replacing and restoring these properties.

This works if code accessing the module reference it through the cached module. In particular the code must not have made their own copies of a module properties during setup.

The mailer code shown above violates this demand.

It could be changed to:

 var mailer = require('../boot/mailer')
  ... later in the code ... the mailer is referenced
  return function requireVerifiedEmail (req, res, next) {
        ...
           from: options.locals.from || mailer.getMailer().from,

However it seemed better to me to remove the getMailer() method entirely so that this early look- in of the mailer function could not happen.

Assuming a module m's properties are always used by referencing the module one can stub it freely as follows:

1. Within the get a reference to module 'm'
2. Before test all properties of m are replaced with the stubs as needed by the test.
3. After the test these properties are reset.

In coffee code:

describe `a test with a replacement module`, ->
  m = require '../../m/'
  priorM = {}
  testM = {stub: 'foo'}

  before ->
    _.assign(priorM, m)
    for own key of m
      delete m[key]
    _.assign(m, testM)

  after ->
    for own key of m
      delete m[key]
    _.assign(m, priorM)

  it 'something', ->
    an assertion here

By putting the mechanics of the above into its own module test/lib/testSettings the code above becomes:

TestSettings = require '../../lib/testSettings'

describe `a test with a replacement module`, ->
  m = require '../../m/'
  priorM = new TestSettings(m, {stub: 'foo'}).

  after ->
    priorM.restore()

  it 'something', ->
    an assertion here

Another case

$ git show eaeabf13
commit eaeabf13
... stuff left out here ...
diff --git a/oidc/enforceReferrer.js b/oidc/enforceReferrer.js
index 53324855..11e7de9f 100644
--- a/oidc/enforceReferrer.js
+++ b/oidc/enforceReferrer.js
@@ -18,9 +18,11 @@ module.exports = function (pathname) {
     pathname = [ pathname ]
   }

-  var host = url.parse(settings.issuer).host
-
   return function enforceReferrer (req, res, next) {
+    // allows changing settings for test by not reading them
+    // during module initialization.
+
+    var host = url.parse(settings.issuer).host
     var referrer = req.get('referrer')

Here oidc/enforceReferrer creates a module variable with content derived form the settings.

This gets in the way of the simple testing approach above.

The change causes a small performance penalty, which seemed ok to me in this case.

Summary

The approach to replace a modules properties allowed to write integration tests in a straight forward way for the passwordless routes.

This was much simpler than fiddling with module loading using proxyquire.

However it required changing code so that it references the (cached) module instead of capturing module state during setup.

Sometimes this very caching could be essential perhaps for performance reasons. However for the passwordless integration tests this was not the case.

The module mailer invited keeping the reference of its inside mailer so that changing its API seemed the best approach to me.

Are there good alternatives to accomplish writing these integration tests?

[henrjk]

There are some minor changes and I would like to update the pull request, rebase and rewrite it. I think we can update this pull request this time.
I would expect that GitHub handles this gracefully, but if not I have made a backup of the comments above.

[j-a-m-l]

Could I do something for advancing with this PR?

[henrjk]

I'd be happy to refresh this with the minor changes mentioned above, unless there are concerns from @christiansmith or @vsimonian.

I wanted to highlight that this would rewrite git history. I am assuming that as long as the pr is not actually pulled in, it is not set in stone yet. Unless somebody stops me I might do those updates at the end of today (PST) or tomorrow morning.

[christiansmith]

I want to be able to merge this feature, but there are several problems with the implementation in this PR as we've discussed previously at length. If we were to merge this now, it would severely complicate some of the design changes and refactoring in the works.

[henrjk]

I updated this PR with small changes. This addresses some concerns I had after previous discussions of the changes. It may not address all concerns and most certainly not that incorporating this relatively large change now would make the intended design changes and refactoring more difficult.

I understand the plan is to retrofit this PR (or open a new PR) to target the new design once it becomes available. The updated state of this PR would be a good starting point for this.

Compared to the earlier state pushed initially this changes the following:

• removes dependency on proxyquire and associated test
• removes a change in OneTimeToken which ultimately is not necessary.
• uses the renderSignin function in routes/connect.js. The rendering of signin was added since the original push of this PR.
• In addition there was a problem in two tests which would corrupt the global settings state so that other test failed. This has been fixed with commit 27dcabe.

Here is a diff of the new changes without the just mentioned commit.

diff.txt

[christiansmith]

Thanks @henrjk. I'll do my best to carve out some time.

[j-a-m-l]

Thank you both for resuming this PR.