#54 email verification

[Jordan141]

Reference to related ticket: resolves: #54 Description of changes: Adds email verification for users.

[Jordan141]

TODO: Add check to login to make sure the user's active flag is true, otherwise return a message to notify them that they still need to verify their account.

[Jordan141]

TODO: Better way of ascertaining server's host URL to create the verification email link.

[Jordan141]

TODO: Add email login details to config/env vars check script

[Jordan141]

Final To-Do list:

• [x] Fix broken register tests due to behavior change of the register controller.
• [ ] Extend config and ENV checks to include email details.
• [x] Extend login functionality to check if User has been verified.
• [ ] Test config extension
• [x] Test login extension
• [x] Complete review

[Jordan141]

Some documentation based on a few of the changes I pushed during my offline period:

registerController Testing Environment Bug

• Request mocked variables such as username never reach the function, as they are never parsed to body, only params.

This does not affect the register tests because it's never tested that those variables are actually there. We depend on the User model to throw an error if anything is empty, however, in the testing environment, the User model is mocked and does not contain this verification.

This is detrimental to further tests which contain functions that do test if the inputs are there, as they will begin throwing errors. As such, I will be implementing a fix in this PR, as it is relevant to email verification. (My code runs through the registerController).

Example of this conflicting with existing tests when you bring in the emailController:

Current implemented fix:

• Changed the params keyname of the requestMock to body. Details are now propagating through the code as expected.
• Cons to this fix: None found so far.

[Jordan141]

Irritating code

The sendVerificationMail requires a mocked transporter, after already extending the registerController to contain the verifyModel, this amount of dependency injection is beginning to feel dirty.

However, the transporter is required to send an e-mail in the real code, so I must keep it.

I'll be adding it to the parameters of the DI for now, further research would be preferred down the line. Assistance in figuring out a cleaner solution to this problem would be appreciated.

This code has way too much moving parts.

[Jordan141]

Final final TO-DO list

• [x] Update config generation and tests
• [x] Final review changes
• [ ] Final changes and fixes after manual tests

[Jordan141]

There is more write ups regarding my offline code spree, however they're mostly just me writing down my thoughts as I'm coding to keep it structured.

Also, I'd like someone to attempt to replicate a bug I came across, the full test suite is hanging for me, it seems the real transporter is somehow managing to slip into tests, which, I'm unable to find. It is a possibility that it's simply a bug with my development environment or some conflicting nuisance somewhere.

Steps to replicate: -> Fresh install of this branch somewhere following the installation instructions on master readme. -> npm test or npm run test-coverage

*Note that running mocha individually on any file.spec.js does not result in hanging.

[ghost]

DeepCode analyzed this pull request. There are 7 new info reports.

Click to see more details.

[michaelKurowski]

In email.js don't create new connection to SMTP server every time when you perform operations that require mailing. Instead establish the connection during application startup. Also it should throw a real error that will cause the application to crash, if LokIM won't be able to connect to SMTP during startup.

Lack of connection to SMTP server should prevent LokIM from booting in the first place, so server admin won't know that smtp credentials are false, until it will encounter error during runtime.

[michaelKurowski]

E2E detected that LokIM is unable to send any mail. That's due to the fact that you're calling transporter.sendMail, but at this point transporter is a promsie object that resolves to nodemail transporter instead of a transporter itself. https://github.com/michaelKurowski/lokim2/blob/907c42e3f35d61f663c633522a09c0173c6519e9/src/server/routes/controllers/email.js#L48-L59

Artifacts: LokIM Logs Config generated by LokIM during tests Video from E2E tests

Pipeline that failed

[michaelKurowski]

SMTP mock implemented to the CircleCI. Env variables set. You're free to continue with your PR

[ghost]

DeepCode analyzed this pull request. There are 6 new info reports.

Click to see more details.

[michaelKurowski]

Rebased

[ghost]

DeepCode analyzed this pull request. There are 6 new info reports.

Click to see more details.

[ghost]

DeepCode analyzed this pull request. There are 6 new info reports.

Click to see more details.

[codecov[bot]]

Codecov Report

Merging #114 into master will increase coverage by 1.01%. The diff coverage is 96.62%.

@@            Coverage Diff             @@
##           master     #114      +/-   ##
==========================================
+ Coverage    83.6%   84.62%   +1.01%     
==========================================
  Files          64       67       +3     
  Lines        1049     1138      +89     
==========================================
+ Hits          877      963      +86     
- Misses        172      175       +3

Impacted Files	Coverage Δ
src/server/models/user.js	100% <ø> (ø)	:arrow_up:
src/server/init.js	100% <100%> (ø)	:arrow_up:
src/server/connectToSMTP.js	100% <100%> (ø)
src/server/routes/router.js	94.44% <100%> (+0.69%)	:arrow_up:
src/server/configFileService.js	100% <100%> (ø)	:arrow_up:
src/server/routes/controllers/register.js	100% <100%> (ø)	:arrow_up:
src/server/models/verification.js	100% <100%> (ø)
src/server/passport/strategies.js	95.65% <75%> (-4.35%)	:arrow_down:
src/server/routes/controllers/email.js	95.65% <95.65%> (ø)
... and 2 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update be01451...63553cc. Read the comment docs.

[michaelKurowski]

TODO Setting up mail server with custom api to receive emails

[ghost]

DeepCode analyzed this pull request. There are 6 new info reports.

Click to see more details.

[ghost]

DeepCode analyzed this pull request. There are 6 new info reports.

Click to see more details.

[ghost]

DeepCode analyzed this pull request. There are 6 new info reports.

Click to see more details.

[ghost]

DeepCode analyzed this pull request. There are 6 new info reports.

Click to see more details.

[ghost]

DeepCode analyzed this pull request. There are 7 new info reports.

Click to see more details.

[ghost]

DeepCode analyzed this pull request. There are 7 new info reports.

Click to see more details.

[codecov[bot]]

Codecov Report

Merging #114 into master will decrease coverage by 25.71%. The diff coverage is 41.96%.

@@             Coverage Diff             @@
##           master     #114       +/-   ##
===========================================
- Coverage   83.74%   58.03%   -25.72%     
===========================================
  Files          62       66        +4     
  Lines        1046     1158      +112     
===========================================
- Hits          876      672      -204     
- Misses        170      486      +316

Impacted Files	Coverage Δ
src/server/models/user.js	0% <ø> (-100%)	:arrow_down:
src/server/routes/router.js	0% <0%> (-93.75%)	:arrow_down:
src/server/models/verification.js	0% <0%> (ø)
src/server/init.js	0% <0%> (-100%)	:arrow_down:
src/server/configFileService.js	12.06% <0%> (-87.94%)	:arrow_down:
.../frontEnd/src/.tests/theme/scenes/register.spec.js	100% <100%> (ø)
src/server/frontEnd/src/theme/App.js	100% <100%> (ø)	:arrow_up:
src/server/passport/strategies.js	17.39% <25%> (-82.61%)	:arrow_down:
src/server/routes/controllers/register.js	33.33% <28.57%> (-66.67%)	:arrow_down:
...ontEnd/src/theme/scenes/verifyEmail/verifyEmail.js	40% <40%> (ø)
... and 25 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 35d9c9f...3a9dc72. Read the comment docs.

[michaelKurowski]

E2E still needs to be fixed

[michaelKurowski]

@Jordan141 fixed email verification

[ghost]

DeepCode analyzed this pull request. There are 7 new info reports.

Click to see more details.

[ghost]

DeepCode analyzed this pull request. There are 7 new info reports.

Click to see more details.

[michaelKurowski]

TODO: Make a real email confirmation page

[michaelKurowski]

@Jordan141 E2E are testing the new functionality, and are passing. CI green.

[michaelKurowski]

TODO: make cypress read config

[michaelKurowski]

Also, we need an information for others to use some mail server in order to have lokIM up and running now. I use schickling/mailcatcher docker image. It opens SMTP on 1025, and is easily accessible through http on 1080

[michaelKurowski]

TODO: Make it work on Heroku

[michaelKurowski]

@Jordan141 do you use any publicly availible SMTP server?

[Jordan141]

@Jordan141 do you use any publicly availible SMTP server?

Nope, I made a mail.com account for testing, I'll provide you with the details via FB

[ghost]

DeepCode analyzed this pull request. There are 8 new info reports.

Click to see more details.

[ghost]

DeepCode analyzed this pull request. There are 8 new info reports.

Click to see more details.

[ghost]

DeepCode analyzed this pull request. There are 8 new info reports.

Click to see more details.

[michaelKurowski]

@Jordan141 @MarcinGrzeszczak Please do a code review

[michaelKurowski]

@Jordan141 Issues with the CI have been fixed. Please continue with the review findings.

[michaelKurowski]

@MarcinGrzeszczak please revide findings

[michaelKurowski]

@Jordan141 could you manually recheck whether it's 100% working, then we'll merge.