From cb3f60b5246da5fc0bd347d430759b25a883068a Mon Sep 17 00:00:00 2001 From: Maksym Mykhailenko Date: Wed, 8 May 2019 14:56:15 +0700 Subject: [PATCH 1/5] winning submission from challenge 30090281 - Topcoder Project Service - Fix duplicated email invitations, fix issue #3002 --- src/routes/projectMemberInvites/create.js | 3 +- .../projectMemberInvites/create.spec.js | 137 ++++++++++++++++-- 2 files changed, 128 insertions(+), 12 deletions(-) diff --git a/src/routes/projectMemberInvites/create.js b/src/routes/projectMemberInvites/create.js index 4839ba45..20cd9fc3 100644 --- a/src/routes/projectMemberInvites/create.js +++ b/src/routes/projectMemberInvites/create.js @@ -83,7 +83,8 @@ const buildCreateInvitePromises = (req, invite, invites, data, failed) => { }); // remove invites for users that are invited already - _.remove(nonExistentUserEmails, email => _.some(invites, i => i.email === email)); + _.remove(nonExistentUserEmails, email => + _.some(invites, i => _.toLower(i.email) === _.toLower(email))); nonExistentUserEmails.forEach((email) => { const dataNew = _.clone(data); diff --git a/src/routes/projectMemberInvites/create.spec.js b/src/routes/projectMemberInvites/create.spec.js index 6965d2c4..ad930766 100644 --- a/src/routes/projectMemberInvites/create.spec.js +++ b/src/routes/projectMemberInvites/create.spec.js @@ -73,17 +73,40 @@ describe('Project Member Invite create', () => { createdBy: 1, updatedBy: 1, }).then(() => { - models.ProjectMemberInvite.create({ - projectId: project1.id, - userId: 40051335, - email: null, - role: PROJECT_MEMBER_ROLE.MANAGER, - status: INVITE_STATUS.PENDING, - createdBy: 1, - updatedBy: 1, - createdAt: '2016-06-30 00:33:07+00', - updatedAt: '2016-06-30 00:33:07+00', - }).then(() => { + const promises = [ + models.ProjectMemberInvite.create({ + projectId: project1.id, + userId: 40051335, + email: null, + role: PROJECT_MEMBER_ROLE.MANAGER, + status: INVITE_STATUS.PENDING, + createdBy: 1, + updatedBy: 1, + createdAt: '2016-06-30 00:33:07+00', + updatedAt: '2016-06-30 00:33:07+00', + }), + models.ProjectMemberInvite.create({ + projectId: project1.id, + email: 'duplicate_lowercase@gmail.com', + role: PROJECT_MEMBER_ROLE.MANAGER, + status: INVITE_STATUS.PENDING, + createdBy: 1, + updatedBy: 1, + createdAt: '2016-06-30 00:33:07+00', + updatedAt: '2016-06-30 00:33:07+00', + }), + models.ProjectMemberInvite.create({ + projectId: project1.id, + email: 'DUPLICATE_UPPERCASE@gmail.com', + role: PROJECT_MEMBER_ROLE.MANAGER, + status: INVITE_STATUS.PENDING, + createdBy: 1, + updatedBy: 1, + createdAt: '2016-06-30 00:33:07+00', + updatedAt: '2016-06-30 00:33:07+00', + }), + ]; + Promise.all(promises).then(() => { done(); }); }); @@ -640,6 +663,98 @@ describe('Project Member Invite create', () => { }); }); + it('should return 201 and empty response when trying add already invited member by lowercase email', (done) => { + const mockHttpClient = _.merge(testUtil.mockHttpClient, { + get: () => Promise.resolve({ + status: 200, + data: { + id: 'requesterId', + version: 'v3', + result: { + success: true, + status: 200, + content: { + success: [{ + roleName: USER_ROLE.COPILOT, + }], + }, + }, + }, + }), + }); + sandbox.stub(util, 'getHttpClient', () => mockHttpClient); + request(server) + .post(`/v4/projects/${project1.id}/members/invite`) + .set({ + Authorization: `Bearer ${testUtil.jwts.copilot}`, + }) + .send({ + param: { + emails: ['DUPLICATE_LOWERCASE@gmail.com'], + role: 'customer', + }, + }) + .expect('Content-Type', /json/) + .expect(201) + .end((err, res) => { + if (err) { + done(err); + } else { + const resJson = res.body.result.content.success; + should.exist(resJson); + resJson.length.should.equal(0); + server.services.pubsub.publish.neverCalledWith('project.member.invite.created').should.be.true; + done(); + } + }); + }); + + it('should return 201 and empty response when trying add already invited member by uppercase email', (done) => { + const mockHttpClient = _.merge(testUtil.mockHttpClient, { + get: () => Promise.resolve({ + status: 200, + data: { + id: 'requesterId', + version: 'v3', + result: { + success: true, + status: 200, + content: { + success: [{ + roleName: USER_ROLE.COPILOT, + }], + }, + }, + }, + }), + }); + sandbox.stub(util, 'getHttpClient', () => mockHttpClient); + request(server) + .post(`/v4/projects/${project1.id}/members/invite`) + .set({ + Authorization: `Bearer ${testUtil.jwts.copilot}`, + }) + .send({ + param: { + emails: ['duplicate_uppercase@gmail.com'], + role: 'customer', + }, + }) + .expect('Content-Type', /json/) + .expect(201) + .end((err, res) => { + if (err) { + done(err); + } else { + const resJson = res.body.result.content.success; + should.exist(resJson); + resJson.length.should.equal(0); + server.services.pubsub.publish.neverCalledWith('project.member.invite.created').should.be.true; + done(); + } + }); + }); + describe('Bus api', () => { let createEventSpy; From 9f87f527fb172f20512740fb3c53344b09567bec Mon Sep 17 00:00:00 2001 From: Muhamad Fikri Alhawarizmi Date: Wed, 8 May 2019 18:30:33 +0700 Subject: [PATCH 2/5] implement email validation --- config/default.json | 3 +- src/routes/projectMemberInvites/create.js | 30 ++++- .../projectMemberInvites/create.spec.js | 122 +++++++++++------- 3 files changed, 108 insertions(+), 47 deletions(-) diff --git a/config/default.json b/config/default.json index 95f5666d..b8b4a2a4 100644 --- a/config/default.json +++ b/config/default.json @@ -59,5 +59,6 @@ "inviteEmailSectionTitle": "Project Invitation", "connectUrl":"https://connect.topcoder-dev.com", "accountsAppUrl": "https://accounts.topcoder-dev.com", - "MAX_REVISION_NUMBER": 100 + "MAX_REVISION_NUMBER": 100, + "UNIQUE_GMAIL_VALIDATION": true } diff --git a/src/routes/projectMemberInvites/create.js b/src/routes/projectMemberInvites/create.js index 20cd9fc3..fd4fa9bc 100644 --- a/src/routes/projectMemberInvites/create.js +++ b/src/routes/projectMemberInvites/create.js @@ -28,6 +28,31 @@ const addMemberValidations = { }, }; +/** + * Helper method to check the uniqueness of two emails + * + * @param {String} email1 first email to compare + * @param {String} email2 second email to compare + * @param {Object} options the options + * + * @returns {Boolean} true if two emails are same + */ +const compareEmail = (email1, email2, options = {}) => { + const opts = _.assign({ + UNIQUE_GMAIL_VALIDATION: config.get('UNIQUE_GMAIL_VALIDATION'), + }, options); + if (opts.UNIQUE_GMAIL_VALIDATION) { + // email is gmail + const emailSplit = /(^[\w.+\-]+)@gmail\.(.*.)$/g.exec(email1); // eslint-disable-line + if (emailSplit) { + const address = emailSplit[1]; + const regex = new RegExp(_.toLower(address).replace('.', '').split('').join('.?')); + return regex.test(_.toLower(email2)); + } + } + return _.toLower(email1) === _.toLower(email2); +}; + /** * Helper method to build promises for creating new invites in DB * @@ -68,7 +93,8 @@ const buildCreateInvitePromises = (req, invite, invites, data, failed) => { }); // non-existent users we will invite them by email only const nonExistentUserEmails = invite.emails.filter(inviteEmail => - !_.find(existentUsers, { email: inviteEmail }), + !_.find(existentUsers, existentUser => + compareEmail(existentUser.email, inviteEmail, { UNIQUE_GMAIL_VALIDATION: false })), ); // remove invites for users that are invited already @@ -84,7 +110,7 @@ const buildCreateInvitePromises = (req, invite, invites, data, failed) => { // remove invites for users that are invited already _.remove(nonExistentUserEmails, email => - _.some(invites, i => _.toLower(i.email) === _.toLower(email))); + _.some(invites, i => compareEmail(i.email, email))); nonExistentUserEmails.forEach((email) => { const dataNew = _.clone(data); diff --git a/src/routes/projectMemberInvites/create.spec.js b/src/routes/projectMemberInvites/create.spec.js index ad930766..6730fb53 100644 --- a/src/routes/projectMemberInvites/create.spec.js +++ b/src/routes/projectMemberInvites/create.spec.js @@ -87,7 +87,7 @@ describe('Project Member Invite create', () => { }), models.ProjectMemberInvite.create({ projectId: project1.id, - email: 'duplicate_lowercase@gmail.com', + email: 'duplicate_lowercase@test.com', role: PROJECT_MEMBER_ROLE.MANAGER, status: INVITE_STATUS.PENDING, createdBy: 1, @@ -97,7 +97,27 @@ describe('Project Member Invite create', () => { }), models.ProjectMemberInvite.create({ projectId: project1.id, - email: 'DUPLICATE_UPPERCASE@gmail.com', + email: 'DUPLICATE_UPPERCASE@test.com', + role: PROJECT_MEMBER_ROLE.MANAGER, + status: INVITE_STATUS.PENDING, + createdBy: 1, + updatedBy: 1, + createdAt: '2016-06-30 00:33:07+00', + updatedAt: '2016-06-30 00:33:07+00', + }), + models.ProjectMemberInvite.create({ + projectId: project1.id, + email: 'with.dot@gmail.com', + role: PROJECT_MEMBER_ROLE.MANAGER, + status: INVITE_STATUS.PENDING, + createdBy: 1, + updatedBy: 1, + createdAt: '2016-06-30 00:33:07+00', + updatedAt: '2016-06-30 00:33:07+00', + }), + models.ProjectMemberInvite.create({ + projectId: project1.id, + email: 'withoutdot@gmail.com', role: PROJECT_MEMBER_ROLE.MANAGER, status: INVITE_STATUS.PENDING, createdBy: 1, @@ -664,25 +684,6 @@ describe('Project Member Invite create', () => { }); it('should return 201 and empty response when trying add already invited member by lowercase email', (done) => { - const mockHttpClient = _.merge(testUtil.mockHttpClient, { - get: () => Promise.resolve({ - status: 200, - data: { - id: 'requesterId', - version: 'v3', - result: { - success: true, - status: 200, - content: { - success: [{ - roleName: USER_ROLE.COPILOT, - }], - }, - }, - }, - }), - }); - sandbox.stub(util, 'getHttpClient', () => mockHttpClient); request(server) .post(`/v4/projects/${project1.id}/members/invite`) .set({ @@ -690,7 +691,7 @@ describe('Project Member Invite create', () => { }) .send({ param: { - emails: ['DUPLICATE_LOWERCASE@gmail.com'], + emails: ['DUPLICATE_LOWERCASE@test.com'], role: 'customer', }, }) @@ -703,32 +704,12 @@ describe('Project Member Invite create', () => { const resJson = res.body.result.content.success; should.exist(resJson); resJson.length.should.equal(0); - server.services.pubsub.publish.neverCalledWith('project.member.invite.created').should.be.true; done(); } }); }); it('should return 201 and empty response when trying add already invited member by uppercase email', (done) => { - const mockHttpClient = _.merge(testUtil.mockHttpClient, { - get: () => Promise.resolve({ - status: 200, - data: { - id: 'requesterId', - version: 'v3', - result: { - success: true, - status: 200, - content: { - success: [{ - roleName: USER_ROLE.COPILOT, - }], - }, - }, - }, - }), - }); - sandbox.stub(util, 'getHttpClient', () => mockHttpClient); request(server) .post(`/v4/projects/${project1.id}/members/invite`) .set({ @@ -736,7 +717,7 @@ describe('Project Member Invite create', () => { }) .send({ param: { - emails: ['duplicate_uppercase@gmail.com'], + emails: ['duplicate_uppercase@test.com'], role: 'customer', }, }) @@ -749,12 +730,65 @@ describe('Project Member Invite create', () => { const resJson = res.body.result.content.success; should.exist(resJson); resJson.length.should.equal(0); - server.services.pubsub.publish.neverCalledWith('project.member.invite.created').should.be.true; done(); } }); }); + it('should return 201 and empty response when trying add already invited member by gmail email with dot', + (done) => { + request(server) + .post(`/v4/projects/${project1.id}/members/invite`) + .set({ + Authorization: `Bearer ${testUtil.jwts.copilot}`, + }) + .send({ + param: { + emails: ['WITHdot@gmail.com'], + role: 'customer', + }, + }) + .expect('Content-Type', /json/) + .expect(201) + .end((err, res) => { + if (err) { + done(err); + } else { + const resJson = res.body.result.content.success; + should.exist(resJson); + resJson.length.should.equal(0); + done(); + } + }); + }); + + it('should return 201 and empty response when trying add already invited member by gmail email without dot', + (done) => { + request(server) + .post(`/v4/projects/${project1.id}/members/invite`) + .set({ + Authorization: `Bearer ${testUtil.jwts.copilot}`, + }) + .send({ + param: { + emails: ['WITHOUT.dot@gmail.com'], + role: 'customer', + }, + }) + .expect('Content-Type', /json/) + .expect(201) + .end((err, res) => { + if (err) { + done(err); + } else { + const resJson = res.body.result.content.success; + should.exist(resJson); + resJson.length.should.equal(0); + done(); + } + }); + }); + describe('Bus api', () => { let createEventSpy; From 550b15337270ced7c61589de5a9a6be0455dbd07 Mon Sep 17 00:00:00 2001 From: Muhamad Fikri Alhawarizmi Date: Thu, 9 May 2019 09:24:32 +0700 Subject: [PATCH 3/5] update regex validation and function parameters --- src/routes/projectMemberInvites/create.js | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/src/routes/projectMemberInvites/create.js b/src/routes/projectMemberInvites/create.js index fd4fa9bc..f6ac9c31 100644 --- a/src/routes/projectMemberInvites/create.js +++ b/src/routes/projectMemberInvites/create.js @@ -37,16 +37,15 @@ const addMemberValidations = { * * @returns {Boolean} true if two emails are same */ -const compareEmail = (email1, email2, options = {}) => { - const opts = _.assign({ - UNIQUE_GMAIL_VALIDATION: config.get('UNIQUE_GMAIL_VALIDATION'), - }, options); - if (opts.UNIQUE_GMAIL_VALIDATION) { +const compareEmail = (email1, email2, options = { UNIQUE_GMAIL_VALIDATION: false }) => { + if (options.UNIQUE_GMAIL_VALIDATION) { // email is gmail - const emailSplit = /(^[\w.+\-]+)@gmail\.(.*.)$/g.exec(email1); // eslint-disable-line + const emailSplit = /(^[\w.+-]+)(@gmail\..*.)$/g.exec(email1); if (emailSplit) { const address = emailSplit[1]; - const regex = new RegExp(_.toLower(address).replace('.', '').split('').join('.?')); + const emailDomain = emailSplit[2]; + const regexAddress = _.toLower(address).replace('.', '').split('').join('\.?'); // eslint-disable-line no-useless-escape + const regex = new RegExp(`${regexAddress}${emailDomain}`); return regex.test(_.toLower(email2)); } } @@ -110,7 +109,8 @@ const buildCreateInvitePromises = (req, invite, invites, data, failed) => { // remove invites for users that are invited already _.remove(nonExistentUserEmails, email => - _.some(invites, i => compareEmail(i.email, email))); + _.some(invites, i => + compareEmail(i.email, email, { UNIQUE_GMAIL_VALIDATION: config.get('UNIQUE_GMAIL_VALIDATION') }))); nonExistentUserEmails.forEach((email) => { const dataNew = _.clone(data); From 01973ef25051a5232691c21001ca51cb9b6bf76c Mon Sep 17 00:00:00 2001 From: Muhamad Fikri Alhawarizmi Date: Thu, 9 May 2019 09:30:09 +0700 Subject: [PATCH 4/5] update email regex to lower case --- src/routes/projectMemberInvites/create.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/routes/projectMemberInvites/create.js b/src/routes/projectMemberInvites/create.js index f6ac9c31..49a3ff94 100644 --- a/src/routes/projectMemberInvites/create.js +++ b/src/routes/projectMemberInvites/create.js @@ -44,8 +44,8 @@ const compareEmail = (email1, email2, options = { UNIQUE_GMAIL_VALIDATION: false if (emailSplit) { const address = emailSplit[1]; const emailDomain = emailSplit[2]; - const regexAddress = _.toLower(address).replace('.', '').split('').join('\.?'); // eslint-disable-line no-useless-escape - const regex = new RegExp(`${regexAddress}${emailDomain}`); + const regexAddress = address.replace('.', '').split('').join('\.?'); // eslint-disable-line no-useless-escape + const regex = new RegExp(_.toLower(`${regexAddress}${emailDomain}`)); return regex.test(_.toLower(email2)); } } From 57a25811b9cea40fdd699daae53cabd980963f05 Mon Sep 17 00:00:00 2001 From: Muhamad Fikri Alhawarizmi Date: Fri, 10 May 2019 12:54:13 +0700 Subject: [PATCH 5/5] fix regex for validating email --- src/routes/projectMemberInvites/create.js | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/routes/projectMemberInvites/create.js b/src/routes/projectMemberInvites/create.js index 49a3ff94..46b70399 100644 --- a/src/routes/projectMemberInvites/create.js +++ b/src/routes/projectMemberInvites/create.js @@ -40,12 +40,12 @@ const addMemberValidations = { const compareEmail = (email1, email2, options = { UNIQUE_GMAIL_VALIDATION: false }) => { if (options.UNIQUE_GMAIL_VALIDATION) { // email is gmail - const emailSplit = /(^[\w.+-]+)(@gmail\..*.)$/g.exec(email1); + const emailSplit = /(^[\w.+-]+)(@gmail\.com|@googlemail\.com)$/g.exec(_.toLower(email1)); if (emailSplit) { - const address = emailSplit[1]; - const emailDomain = emailSplit[2]; - const regexAddress = address.replace('.', '').split('').join('\.?'); // eslint-disable-line no-useless-escape - const regex = new RegExp(_.toLower(`${regexAddress}${emailDomain}`)); + const address = emailSplit[1].replace('.', ''); + const emailDomain = emailSplit[2].replace('.', '\\.'); + const regexAddress = address.split('').join('\\.?'); + const regex = new RegExp(`${regexAddress}${emailDomain}`); return regex.test(_.toLower(email2)); } }