From 8c13234af98603a37e2830b668faed0e36ee1157 Mon Sep 17 00:00:00 2001 From: roschaefer Date: Tue, 24 Sep 2019 16:20:27 +0200 Subject: [PATCH] Handle edge case It might be that people try to register email addresses that they don't own. Then if the actual owner tries to add this email address, she should not get a unique constraint violation. Instead the email will be re-used. Is this a security issue? Because we re-use the nonce? :thinking: --- backend/src/schema/resolvers/emails.js | 7 +++- backend/src/schema/resolvers/emails.spec.js | 26 ++++++++++++- .../resolvers/helpers/existingEmailAddress.js | 26 +++++++++++++ .../resolvers}/helpers/generateNonce.js | 0 backend/src/schema/resolvers/registration.js | 39 ++++--------------- .../src/schema/resolvers/registration.spec.js | 7 +++- webapp/components/Registration/Signup.spec.js | 2 +- webapp/components/Registration/Signup.vue | 2 +- 8 files changed, 69 insertions(+), 40 deletions(-) create mode 100644 backend/src/schema/resolvers/helpers/existingEmailAddress.js rename backend/src/{ => schema/resolvers}/helpers/generateNonce.js (100%) diff --git a/backend/src/schema/resolvers/emails.js b/backend/src/schema/resolvers/emails.js index 281c7d8ce..664df2803 100644 --- a/backend/src/schema/resolvers/emails.js +++ b/backend/src/schema/resolvers/emails.js @@ -1,10 +1,13 @@ -import generateNonce from '../../helpers/generateNonce' +import generateNonce from './helpers/generateNonce' import Resolver from './helpers/Resolver' +import existingEmailAddress from './helpers/existingEmailAddress' export default { Mutation: { AddEmailAddress: async (_parent, args, context, _resolveInfo) => { - let response + let response = await existingEmailAddress(_parent, args, context) + if (response) return response + const nonce = generateNonce() const { user: { id: userId }, diff --git a/backend/src/schema/resolvers/emails.spec.js b/backend/src/schema/resolvers/emails.spec.js index ec9e831e3..5029622ae 100644 --- a/backend/src/schema/resolvers/emails.spec.js +++ b/backend/src/schema/resolvers/emails.spec.js @@ -95,11 +95,33 @@ describe('AddEmailAddress', () => { }) describe('if a lone `EmailAddress` node already exists with that email', () => { - it.todo('returns this `EmailAddress` node') + it('returns this `EmailAddress` node', async () => { + await factory.create('EmailAddress', { + verifiedAt: null, + createdAt: '2019-09-24T14:00:01.565Z', + email: 'new-email@example.org', + }) + await expect(mutate({ mutation, variables })).resolves.toMatchObject({ + data: { + AddEmailAddress: { + email: 'new-email@example.org', + verifiedAt: null, + createdAt: '2019-09-24T14:00:01.565Z', // this is to make sure it's the one above + }, + }, + errors: undefined, + }) + }) }) describe('but if another user owns an `EmailAddress` already with that email', () => { - it.todo('throws UserInputError because of unique constraints') + it('throws UserInputError because of unique constraints', async () => { + await factory.create('User', { email: 'new-email@example.org' }) + await expect(mutate({ mutation, variables })).resolves.toMatchObject({ + data: { AddEmailAddress: null }, + errors: [{ message: 'A user account with this email already exists.' }], + }) + }) }) }) }) diff --git a/backend/src/schema/resolvers/helpers/existingEmailAddress.js b/backend/src/schema/resolvers/helpers/existingEmailAddress.js new file mode 100644 index 000000000..d05122df1 --- /dev/null +++ b/backend/src/schema/resolvers/helpers/existingEmailAddress.js @@ -0,0 +1,26 @@ +import { UserInputError } from 'apollo-server' +export default async function alreadyExistingMail(_parent, args, context) { + let { email } = args + email = email.toLowerCase() + const cypher = ` + MATCH (email:EmailAddress {email: $email}) + OPTIONAL MATCH (email)-[:PRIMARY_EMAIL]-(user) + RETURN email, user + ` + let transactionRes + const session = context.driver.session() + try { + transactionRes = await session.run(cypher, { email }) + } finally { + session.close() + } + const [result] = transactionRes.records.map(record => { + return { + alreadyExistingEmail: record.get('email').properties, + user: record.get('user') && record.get('user').properties, + } + }) + const { alreadyExistingEmail, user } = result || {} + if (user) throw new UserInputError('A user account with this email already exists.') + return alreadyExistingEmail +} diff --git a/backend/src/helpers/generateNonce.js b/backend/src/schema/resolvers/helpers/generateNonce.js similarity index 100% rename from backend/src/helpers/generateNonce.js rename to backend/src/schema/resolvers/helpers/generateNonce.js diff --git a/backend/src/schema/resolvers/registration.js b/backend/src/schema/resolvers/registration.js index 72e499038..bd62b32c3 100644 --- a/backend/src/schema/resolvers/registration.js +++ b/backend/src/schema/resolvers/registration.js @@ -1,41 +1,16 @@ import { UserInputError } from 'apollo-server' -import uuid from 'uuid/v4' import { neode } from '../../bootstrap/neo4j' import fileUpload from './fileUpload' import encryptPassword from '../../helpers/encryptPassword' +import generateNonce from './helpers/generateNonce' +import existingEmailAddress from './helpers/existingEmailAddress' const instance = neode() -const alreadyExistingMail = async (_parent, args, context) => { - let { email } = args - email = email.toLowerCase() - const cypher = ` - MATCH (email:EmailAddress {email: $email}) - OPTIONAL MATCH (email)-[:PRIMARY_EMAIL]-(user) - RETURN email, user - ` - let transactionRes - const session = context.driver.session() - try { - transactionRes = await session.run(cypher, { email }) - } finally { - session.close() - } - const [result] = transactionRes.records.map(record => { - return { - alreadyExistingEmail: record.get('email').properties, - user: record.get('user') && record.get('user').properties, - } - }) - const { alreadyExistingEmail, user } = result || {} - if (user) throw new UserInputError('User account with this email already exists.') - return alreadyExistingEmail -} - export default { Mutation: { CreateInvitationCode: async (_parent, args, context, _resolveInfo) => { - args.token = uuid().substring(0, 6) + args.token = generateNonce() const { user: { id: userId }, } = context @@ -54,9 +29,9 @@ export default { return response }, Signup: async (_parent, args, context) => { - const nonce = uuid().substring(0, 6) + const nonce = generateNonce() args.nonce = nonce - let emailAddress = await alreadyExistingMail(_parent, args, context) + let emailAddress = await existingEmailAddress(_parent, args, context) if (emailAddress) return emailAddress try { emailAddress = await instance.create('EmailAddress', args) @@ -67,9 +42,9 @@ export default { }, SignupByInvitation: async (_parent, args, context) => { const { token } = args - const nonce = uuid().substring(0, 6) + const nonce = generateNonce() args.nonce = nonce - let emailAddress = await alreadyExistingMail(_parent, args, context) + let emailAddress = await existingEmailAddress(_parent, args, context) if (emailAddress) return emailAddress try { const result = await instance.cypher( diff --git a/backend/src/schema/resolvers/registration.spec.js b/backend/src/schema/resolvers/registration.spec.js index 81326004d..0f3af5a8d 100644 --- a/backend/src/schema/resolvers/registration.spec.js +++ b/backend/src/schema/resolvers/registration.spec.js @@ -257,7 +257,7 @@ describe('SignupByInvitation', () => { it('throws unique violation error', async () => { await expect(mutate({ mutation, variables })).resolves.toMatchObject({ - errors: [{ message: 'User account with this email already exists.' }], + errors: [{ message: 'A user account with this email already exists.' }], }) }) }) @@ -307,6 +307,7 @@ describe('Signup', () => { it('is allowed to signup users by email', async () => { await expect(mutate({ mutation, variables })).resolves.toMatchObject({ data: { Signup: { email: 'someuser@example.org' } }, + errors: undefined, }) }) @@ -342,7 +343,7 @@ describe('Signup', () => { it('throws UserInputError error because of unique constraint violation', async () => { await expect(mutate({ mutation, variables })).resolves.toMatchObject({ - errors: [{ message: 'User account with this email already exists.' }], + errors: [{ message: 'A user account with this email already exists.' }], }) }) }) @@ -351,6 +352,7 @@ describe('Signup', () => { it('resolves with the already existing email', async () => { await expect(mutate({ mutation, variables })).resolves.toMatchObject({ data: { Signup: { email: 'someuser@example.org' } }, + errors: undefined, }) }) @@ -359,6 +361,7 @@ describe('Signup', () => { await expect(neode.all('EmailAddress')).resolves.toHaveLength(2) await expect(mutate({ mutation, variables })).resolves.toMatchObject({ data: { Signup: { email: 'someuser@example.org' } }, + errors: undefined, }) await expect(neode.all('EmailAddress')).resolves.toHaveLength(2) }) diff --git a/webapp/components/Registration/Signup.spec.js b/webapp/components/Registration/Signup.spec.js index afcf94c37..ed78d47a2 100644 --- a/webapp/components/Registration/Signup.spec.js +++ b/webapp/components/Registration/Signup.spec.js @@ -115,7 +115,7 @@ describe('Signup', () => { mocks.$apollo.mutate = jest .fn() .mockRejectedValue( - new Error('UserInputError: User account with this email already exists.'), + new Error('UserInputError: A user account with this email already exists.'), ) }) diff --git a/webapp/components/Registration/Signup.vue b/webapp/components/Registration/Signup.vue index dcf4f0e88..36a43f180 100644 --- a/webapp/components/Registration/Signup.vue +++ b/webapp/components/Registration/Signup.vue @@ -128,7 +128,7 @@ export default { } catch (err) { const { message } = err const mapping = { - 'User account with this email already exists': 'email-exists', + 'A user account with this email already exists': 'email-exists', 'Invitation code already used or does not exist': 'invalid-invitation-token', } for (const [pattern, key] of Object.entries(mapping)) {