Skip to content
This repository has been archived by the owner. It is now read-only.

[WIP] : Move email verification from client to server #4767

Closed
wants to merge 1 commit into from

Conversation

@divyabiyani
Copy link
Member

@divyabiyani divyabiyani commented Feb 28, 2017

Fixes #4482.

@divyabiyani divyabiyani force-pushed the divyabiyani:emailver branch from 7683c7d to de8a237 Feb 28, 2017
uid: req.query.uid
};

let options = req.query.options;

This comment has been minimized.

@vladikoff

vladikoff Feb 28, 2017
Contributor

@divyabiyani these won't be options but req.query.service and req.query.reminder

const logger = require('mozlog')('server.get-verify-email');
const config = require('../configuration');

module.exports = function (uid, code, options) {

This comment has been minimized.

@vladikoff

vladikoff Feb 28, 2017
Contributor

uid, code, options arguments here won't be useful. All these come from req below

}
}

got.post(config.get('fxaccount_url') + '/v1/recovery_email/verify_code', {

This comment has been minimized.

@vladikoff

vladikoff Feb 28, 2017
Contributor

See if we can use a template literal here: https://developer.mozilla.org/en/docs/Web/JavaScript/Reference/Template_literals instead of +

@divyabiyani divyabiyani force-pushed the divyabiyani:emailver branch 2 times, most recently from 3f65da8 to 1de6fc7 Feb 28, 2017
path: '/verify_email/',
process: function (req, res, next) {
req.url = '/';
console.log('items', req.query);

This comment has been minimized.

@vladikoff

vladikoff Feb 28, 2017
Contributor

no need for this console log


let data = {
code: req.query.code,
uid: req.query.uid

This comment has been minimized.

@vladikoff

vladikoff Feb 28, 2017
Contributor

@divyabiyani we need to add validation to make sure these 2 items are present in the query request. If they are not then do not make the request below. You need a simpler version of this example:

use the joi module to validate the structure of uid and code. See the format of those 2 fields here: https://github.com/mozilla/fxa-auth-server/blob/7226ce0f627a0510ed93d8056663b1d6c7f42702/docs/api.md#post-v1recovery_emailverify_code

This comment has been minimized.

@vladikoff

vladikoff Feb 28, 2017
Contributor

we need tests for this validation

data.reminder = req.query.reminder;
}

const fxaAccountUrl = config.get('fxaccount_url');

This comment has been minimized.

@vladikoff

vladikoff Feb 28, 2017
Contributor

Move this section above +module.exports = function () {.

This way you are not gonna be calling config.get on every request.

got.post(`${fxaAccountUrl}/v1/recovery_email/verify_code`, {
body: data
}).then(function (res) {
// verified, all good

This comment has been minimized.

@vladikoff

vladikoff Feb 28, 2017
Contributor

need tests for success case

// verified, all good
next();
}).catch(function (err) {
// something went wrong....

This comment has been minimized.

@vladikoff

vladikoff Feb 28, 2017
Contributor

need tests for error case

@divyabiyani divyabiyani force-pushed the divyabiyani:emailver branch from 1de6fc7 to af35260 Mar 1, 2017

joi.validate(data, BODY_SCHEMA, function (err, value) {
if (err) {
logger.error(err);

This comment has been minimized.

@vladikoff

vladikoff Mar 1, 2017
Contributor

@divyabiyani in a case of an error it seems like next(); will never be called and the request will hang forever

This comment has been minimized.

@vladikoff

vladikoff Mar 1, 2017
Contributor

you can check this if you navigate to http://127.0.0.1:3030/verify_email without any query params

@divyabiyani divyabiyani force-pushed the divyabiyani:emailver branch 2 times, most recently from e7bd350 to 95dd217 Mar 1, 2017
@divyabiyani divyabiyani force-pushed the divyabiyani:emailver branch from 95dd217 to 84bdeaf Mar 6, 2017
@vladikoff
Copy link
Contributor

@vladikoff vladikoff commented Mar 8, 2017

Continued in #4794

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants