Feature/ser 280 require email confirmation

[B3rry]

If this PR fixes a bug, you must add test cases representative of the bug.

What's this PR do?

• Adds a model for migrating the database (20190408115030-add-validation-columns-users.js)
• Adds methods for verifying a user's email address
• Rejects requests to the login and reset password endpoints for users when the env vars for requiring account verification are true.

Related JIRA tickets:

• ORANGE-1211

How should this be manually tested?

The best way to test this flow to spin up a copy of Orange-Web PR #85.

• Test the verification process (You can start this process at https://_webPortalURL_/verify-email)
• Check to ensure login and password reset are disabled for unverified accounts (when the env is configured to require it)

🚨 NEW ENV VARIABLES:

• AUTH_SERVICE_REQUIRE_ACCOUNT_VERIFICATION, required, boolean
• AUTH_SERVICE_REQUIRE_SECURE_ACCOUNT_VERIFICATION, required, boolean

Any background context you want to provide?

Screenshots (if appropriate):

[rmharrison]

Static review looks fine.

Rebase/Squash

Before merging to develop, I'd recommend squashing the 1st attempt ... 4th attempt linting fixes, and possibly some of the other commits.

Testing

It looks like only the original case is tested

• AUTH_SERVICE_REQUIRE_ACCOUNT_VERIFICATION=false
• AUTH_SERVICE_REQUIRE_SECURE_ACCOUNT_VERIFICATION=false

Not the entire set
AUTH_SERVICE_REQUIRE_ACCOUNT_VERIFICATION: [true, false]
X (cross)
AUTH_SERVICE_REQUIRE_SECURE_ACCOUNT_VERIFICATION: [true, false]

Indeed, testing the other 3x cases would require programmatically updating these variables set in .env.test, probably by modifying the config object in the test suite.

It's probably sufficient to add test(s) for just the case

• AUTH_SERVICE_REQUIRE_ACCOUNT_VERIFICATION=true
• AUTH_SERVICE_REQUIRE_SECURE_ACCOUNT_VERIFICATION=true

Indeed, when I update .env.test with these, I get "test failures" (a good thing in this case), e.g.

  1) Auth API: Seed with environmental password should seed with the config admin with a environmentally derived password:
     Error: expected 200 "OK", got 404 "Not Found"

[mountHouli]

I was able to do a pretty thorough job reviewing this. I read all or nearly all of the code. However, I didn't get a chance to test, except for the one bug found/recorded in my review of the orange-web PR.

1. The forward migration needs to make every existing account already verified. (This is critical).

2. All of the 'messaging protocol' terminology is not clear. Can you think of a more straight-forward way of naming it?

[mountHouli]

Please add thorough testing steps. (esp. see item 2.C. below)

1. Auto-verification

We need some way for users to be auto-verified. Two different examples:
A. The AUTH_SERVICE_SEED_ADMIN_* should be automatically verified when it is auto-generated the first time the auth service starts.
B. The Push Notifications Service User gets created as a part of the deployment process. It should be able to be auto-verified. Perhaps the logic in the create-user endpoint should be if (JWT belongs to a super admin && req.body.auto_verify_this_new_user_being_created) { /* Auto-complete the verification process */ }

(Note: I might remove the PNSU from the deployment process and instead just use the Seed Admin as the PNSU. However, I still think we should have the above (B) functionality).

2. Verification Process Questions/Changes

Regarding User.verifyAccountToken and User.prototype.updateVerifyAccountToken:

A. Together they accomplish one, discrete bit of functionality, so I think it should all be in one function, not split into two functions.

B. updateVerifyAccountToken implies that a token can be passed into this function. Since this function always auto-generates the token (you can only pass in an email and an expiration time), it should be called createVerifyAccountToken or generateVerifyAccountToken (probably the former rather than the latter). Also, verifyAccountToken misleads the developer/maintainer because it sounds like this function is involved in the process of verifying an existing token, not the process of generating a new token to be verified later. It should be renamed to createVerifyAccountToken or getAccountVerificationToken or createContactMethodVerificationToken or something like this.

C(ish / Related) (the following is my thought without thinking through it thoroughly)
/dispatch-verification-request should not exist. The way it should work is, any time you create a new user, as a part of that same API call to the auth service, it should populate the account verification fields in the Users table according to the account verification env vars, then dispatch the account verification email. In this same vein, why does orange-web VerifyEmailPrompt.jsx have a button to call /dispatch-verification-request? The token should already be created, and the web page should only be the landing page for when a user clicks on the verification email link. Along the same lines, in orange-web, what is the /verify-email page for?

[mountHouli]

Now that #90 is merged, this PR must be rebased to include those changes.

Note: This will include having to change the signature of any calls to new APIError(). Very easy. I would be happy to help if you have any questions.

[B3rry]

Please add thorough testing steps. (esp. see item 2.C. below)

1. Auto-verification

We need some way for users to be auto-verified. Two different examples:
A. The AUTH_SERVICE_SEED_ADMIN_* should be automatically verified when it is auto-generated the first time the auth service starts.

@rmharrison may have some guidance on that.

B. The Push Notifications Service User gets created as a part of the deployment process. It should be able to be auto-verified. Perhaps the logic in the create-user endpoint should be if (JWT belongs to a super admin && req.body.auto_verify_this_new_user_being_created) { /* Auto-complete the verification process */ }

(Note: I might remove the PNSU from the deployment process and instead just use the Seed Admin as the PNSU. However, I still think we should have the above (B) functionality).

I think we should be able to do that, should we consider adding a new scope for system-level users?

2. Verification Process Questions/Changes

Regarding User.verifyAccountToken and User.prototype.updateVerifyAccountToken:

A. Together they accomplish one, discrete bit of functionality, so I think it should all be in one function, not split into two functions.

I left them split, because I see a class method vs instance may be needed here, especially if we go the route of dispatching a token and email when a user registers

B. updateVerifyAccountToken implies that a token can be passed into this function. Since this function always auto-generates the token (you can only pass in an email and an expiration time), it should be called createVerifyAccountToken or generateVerifyAccountToken (probably the former rather than the latter). Also, verifyAccountToken misleads the developer/maintainer because it sounds like this function is involved in the process of verifying an existing token, not the process of generating a new token to be verified later. It should be renamed to createVerifyAccountToken or getAccountVerificationToken or createContactMethodVerificationToken or something like this.

changed to generate

C(ish / Related) (the following is my thought without thinking through it thoroughly)
/dispatch-verification-request should not exist. The way it should work is, any time you create a new user, as a part of that same API call to the auth service, it should populate the account verification fields in the Users table according to the account verification env vars, then dispatch the account verification email. In this same vein, why does orange-web VerifyEmailPrompt.jsx have a button to call /dispatch-verification-request? The token should already be created, and the web page should only be the landing page for when a user clicks on the verification email link. Along the same lines, in orange-web, what is the /verify-email page for?

We talked on the phone about this, it's inelegant for the first iteration but gets it out the door.

[mountHouli]

Regarding 1. Auto-Verification (A. and B.) we decided this is too much work for our budget/timeline right now. We will manually verify any accounts that need it (such as service accounts). Related, but different problem: We will, however, be writing into the DB migration the auto-verification of all users that already exist in the DB.

[mountHouli]

Regarding 2. Verification Process Questions/Changes

A. (Resolved)

Jon and I discussed. Resolution: No action, because…

• The class method takes an email arg and looks up the one user is belongs to; and if the user exists, it calls the instance method
• The instance method generates a verification token for this user
  They are broken apart because we might want to call the token generation instance method on an array of users.

B. (Resolved)

This is now fixed (changed to User.prototype.generateVerifyAccountToken)

C. (Resolved for now)

Jon and I agreed we wanted to make this change; however, we ran out of budget/time so the change is not getting done in this PR.

[mountHouli]

The migrations have one bug that needs to be fixed. See inline feedback regarding the WHERE clause of the UPDATE "Users" SET "verifiedContactMethods" = ... statement.

[mountHouli]

Ton of great work! Many thanks!

[orndorffgrant]

Good (and big!) PR!
Did a fairly thorough code review. Some small things to consider before approval.

[orndorffgrant]

Future/not applicable to this PR: If we get time to invest in this service, we should move to using a real templating engine for creating emails

[rmharrison]

Outside of scope of this PR.
Keeping open (unresolved) FFR.

[B3rry]

Seeing as Grant's comments have been addressed by Houli, and the package is running, merging.

[rmharrison]

Cherrypicked (minor) changes onto branch:release/2.8.0 because this branch had already been merged.

Was unable to integration test with orange-web

After creating user via POSTMAN, and sending sending email at orange-web/verify-url

2019-05-17T14:27:18.045Z error connect ECONNREFUSED 127.0.0.1:587 
Error: connect ECONNREFUSED 127.0.0.1:587
    at TCPConnectWrap.afterConnect [as oncomplete] (net.js:1054:14) 
{
  "errno": "ECONNREFUSED",
  "code": "ECONNECTION",
  "syscall": "connect",
  "address": "127.0.0.1",
  "port": 587,
  "command": "CONN"
}
2019-05-17T14:27:18.050Z info HTTP POST /api/v2/auth/dispatch-verification-request 500 77ms  
{
  "res": {
    "statusCode": 500,
    "responseTime": 77
  },
  "req": {
    "url": "/api/v2/auth/dispatch-verification-request",
    "method": "POST",
    "httpVersion": "1.1",
    "originalUrl": "/api/v2/auth/dispatch-verification-request",
    "query": {}
  },
  "responseTime": 77
}

I'm assuming I missed some configuration.