krakenjs / kraken-example-with-passport

An example integrating kraken with passport authentication
53 stars 33 forks source link

+Registration, and starting on email verification #22

Closed dennishall closed 7 years ago

dennishall commented 8 years ago

I think this will actually fully work for email verification, but you may need a real domain name, and not localhost. There should also be a way to re-send the email verification, and somewhere in the UI that lets a user know if their email is verified .. and even more fun, a page that a user cannot access if their email is not verified. I probably won't write those, as it looks like lockit is a better choice than passport for me, and I'm going to start a different project for that.

dennishall commented 8 years ago

Features of this pull request:

grawk commented 8 years ago

I wonder if maybe it would be better to sort of simulate the "welcome email" since at the moment, OOTB, attempting to register crashes the server. Requiring additional instructions beyond npm install && npm start starts to get tedious.

For example, maybe just console.log out the email content.

dennishall commented 8 years ago

Fixed. I stubbed the Email class for when there is no .env file or when it does not have the needed info. I just forgot the first two formal params in the method signature when I stubbed it. I just fixed that.

grawk commented 8 years ago

Hi @dennishall. I was running the most recent changes. Since I couldn't quickly get nodemailer configured properly, I console.log'd the verification link and tried to access it directly. That crashed the server with the message:

Mon, 22 Feb 2016 23:37:09 GMT uncaughtException undefined is not a function
TypeError: undefined is not a function
    at model.emailVerificationTokenSchema.methods.verify (/Users/medelman/src/krakex/kraken-example-with-passport/models/emailVerificationToken.js:32:19)
    at Promise.<anonymous> (/Users/medelman/src/krakex/kraken-example-with-passport/controllers/verify/index.js:21:25)
    at Promise.<anonymous> (/Users/medelman/src/krakex/kraken-example-with-passport/node_modules/mongoose/node_modules/mpromise/lib/promise.js:177:8)
    at Promise.emit (events.js:107:17)
    at Promise.emit (/Users/medelman/src/krakex/kraken-example-with-passport/node_modules/mongoose/node_modules/mpromise/lib/promise.js:84:38)
    at Promise.fulfill (/Users/medelman/src/krakex/kraken-example-with-passport/node_modules/mongoose/node_modules/mpromise/lib/promise.js:97:20)
    at /Users/medelman/src/krakex/kraken-example-with-passport/node_modules/mongoose/lib/query.js:1406:13
    at model.Document.init (/Users/medelman/src/krakex/kraken-example-with-passport/node_modules/mongoose/lib/document.js:254:11)
    at completeOne (/Users/medelman/src/krakex/kraken-example-with-passport/node_modules/mongoose/lib/query.js:1404:10)
    at Immediate.cb (/Users/medelman/src/krakex/kraken-example-with-passport/node_modules/mongoose/lib/query.js:1158:11)
    at Immediate._onImmediate (/Users/medelman/src/krakex/kraken-example-with-passport/node_modules/mongoose/node_modules/mquery/lib/utils.js:137:16)
    at processImmediate [as _immediateCallback] (timers.js:367:17)

Per the earlier comment from Aria, we don't want to have the core purpose of this example (configuring passport) to be obfuscated by a peripheral functionality (the email/verify pieces). I'm not sure what the happy medium is yet, but the current form is not yet ready for a merge.

Some of your other issues would probably more easily lead to a merge-able PR: #16, #18, #19.

grawk commented 7 years ago

closed unless original author wants to come back and discuss further.