{"action":"created","issue":{"url":"https://api.github.com/repos/anvilresearch/connect/issues/260","labels_url":"https://api.github.com/repos/anvilresearch/connect/issues/260/labels{/name}","comments_url":"https://api.github.com/repos/anvilresearch/connect/issues/260/comments","events_url":"https://api.github.com/repos/anvilresearch/connect/issues/260/events","html_url":"https://github.com/anvilresearch/connect/pull/260","id":111241540,"number":260,"title":"Passwordless or Email only login (2nd)","user":{"login":"henrjk","id":7439282,"avatar_url":"https://avatars.githubusercontent.com/u/7439282?v=3","gravatar_id":"","url":"https://api.github.com/users/henrjk","html_url":"https://github.com/henrjk","followers_url":"https://api.github.com/users/henrjk/followers","following_url":"https://api.github.com/users/henrjk/following{/other_user}","gists_url":"https://api.github.com/users/henrjk/gists{/gist_id}","starred_url":"https://api.github.com/users/henrjk/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/henrjk/subscriptions","organizations_url":"https://api.github.com/users/henrjk/orgs","repos_url":"https://api.github.com/users/henrjk/repos","events_url":"https://api.github.com/users/henrjk/events{/privacy}","received_events_url":"https://api.github.com/users/henrjk/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"milestone":null,"comments":5,"created_at":"2015-10-13T18:35:23Z","updated_at":"2015-11-26T15:14:12Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/anvilresearch/connect/pulls/260","html_url":"https://github.com/anvilresearch/connect/pull/260","diff_url":"https://github.com/anvilresearch/connect/pull/260.diff","patch_url":"https://github.com/anvilresearch/connect/pull/260.patch"},"body":"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`.  \r\n\r\nThe configuration and UI is described at https://github.com/henrjk/connect-docs/blob/pwless/providers/Passwordless.md\r\n\r\nThe 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.\r\n\r\nI did not completely squash the history to more easily identify pieces which can be reviewed individually. \r\n\r\nHowever 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.   \r\n\r\n## Summary of implementation\r\n\r\n**Changes:**\r\n\r\nroutes/signin.js: main integration changes are here.\r\nserver.js: trivial change\r\nproviders/index.js: enable overriding tokenTTL.\r\nmodels/OneTimeToken: one new code path when id is undefined.\r\noidc/determineProvider: Support {options: requireProvider}\r\nshared oidc verifyEmailValid verifyMailerConfigured \r\n\r\n**Additions js code:**\r\n\r\nroutes/passwordless.js -- the main part of this PR.\r\nprotocols/Passwordless.js -- just a stub to allow other code to not change\r\nproviders/passwordless.js -- similar to providers/password.js\r\nerrors/PasswordlessDisabledError.js\r\nrender/renderSignin: New module to allow res.render('signin') call with proper context from passwordless code. Refactored from signin.js\r\n\r\n## Noteworthy observations\r\n\r\n1. The code is not using passport.\r\n\r\n2. It does not plug-in to the provider architecture. It should be reviewed whether this is somehow wrong.\r\n\r\n3. 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.\r\n\r\n4. 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.\r\n\r\nNew shareable middleware is oidc/verifyEmailValid.js\r\nand 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.\r\n\r\n## Testing\r\n\r\nThere 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.\r\n\r\nI 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.\r\n\r\nproxyquire 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.\r\n\r\n## Known Issues\r\n\r\n### AMR for passwordless?\r\n\r\nCurrently '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 .\r\n\r\nNeeds to be clarified how to handle this?\r\n\r\n### interrupted flow in popup with angular client\r\n\r\nIn password authentication the authorization happens in response to a redirect when the password is submitted.\r\nThis allows the popup when the callback is displayed to\r\n1. send a message to the opener window\r\n2. close itself\r\n\r\nHowever 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.\r\n\r\n\r\n\r\n\r\n"},"comment":{"url":"https://api.github.com/repos/anvilresearch/connect/issues/comments/159935771","html_url":"https://github.com/anvilresearch/connect/pull/260#issuecomment-159935771","issue_url":"https://api.github.com/repos/anvilresearch/connect/issues/260","id":159935771,"user":{"login":"christiansmith","id":332377,"avatar_url":"https://avatars.githubusercontent.com/u/332377?v=3","gravatar_id":"","url":"https://api.github.com/users/christiansmith","html_url":"https://github.com/christiansmith","followers_url":"https://api.github.com/users/christiansmith/followers","following_url":"https://api.github.com/users/christiansmith/following{/other_user}","gists_url":"https://api.github.com/users/christiansmith/gists{/gist_id}","starred_url":"https://api.github.com/users/christiansmith/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/christiansmith/subscriptions","organizations_url":"https://api.github.com/users/christiansmith/orgs","repos_url":"https://api.github.com/users/christiansmith/repos","events_url":"https://api.github.com/users/christiansmith/events{/privacy}","received_events_url":"https://api.github.com/users/christiansmith/received_events","type":"User","site_admin":false},"created_at":"2015-11-26T15:14:12Z","updated_at":"2015-11-26T15:14:12Z","body":"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."}}