Ghost: Error when accepting invitation with existing email

[daniellockyer]

Issue Summary

Accepting a staff invitation email, and submitting an email of an already existing user will cause the DB to throw an error because the email would no longer be unique.

The fix is likely to be to check if the email already exists.

To Reproduce

1. Invite a new staff user
2. Open the invitation link
3. Use an email of an already existing staff member
4. :boom: Error shown below form

    HTTP/1.1 500 Internal Server Error
    --
    InternalServerError: insert into `users` (`accessibility`, `bio`, `cover_image`, `created_at`, `created_by`, `email`, `facebook`, `id`, `last_seen`, `locale`, `location`, `meta_description`, `meta_title`, `name`, `password`, `profile_image`, `slug`, `status`, `tour`, `twitter`, `updated_at`, `updated_by`, `visibility`, `website`) values (NULL, NULL, NULL, '2020-07-20 03:24:52', '1', 'secret', NULL, 'id', NULL, NULL, NULL, NULL, NULL, 'my name', 'secret password', NULL, 'secret', 'active', NULL, NULL, '2020-07-20 03:24:52', '1', 'public', NULL) - ER_DUP_ENTRY: Duplicate entry '[email protected]' for key 'users_email_unique'
        at new GhostError (/home/ghost/node_modules/@tryghost/errors/lib/errors.js:10:26)
        at _private.prepareError (/home/ghost/core/server/web/shared/middlewares/error-handler.js:53:19)
        at Layer.handle_error (/home/ghost/node_modules/express/lib/router/layer.js:71:5)
        at trim_prefix (/home/ghost/node_modules/express/lib/router/index.js:315:13)
        at /home/ghost/node_modules/express/lib/router/index.js:284:7
        at Function.process_params (/home/ghost/node_modules/express/lib/router/index.js:335:12)
........

    Error: ER_DUP_ENTRY: Duplicate entry '[email protected]' for key 'users_email_unique'
        at Query.Sequence._packetToError (/home/ghost/node_modules/mysql/lib/protocol/sequences/Sequence.js:47:14)
        at Query.ErrorPacket (/home/ghost/node_modules/mysql/lib/protocol/sequences/Query.js:79:18)
        at Protocol._parsePacket (/home/ghost/node_modules/mysql/lib/protocol/Protocol.js:291:23)
        at Parser._parsePacket (/home/ghost/node_modules/mysql/lib/protocol/Parser.js:433:10)
 ........

Originating from Sentry: https://sentry.io/organizations/ghost-foundation/issues/1634959159/events/ea2ce7b6681a4c119104533d78c9520e/

Technical details:

• Ghost Version: 3.25.0
• Node Version: 12.18.0
• Browser/OS: Firefox/Linux
• Database: SQLite

[imox2]

@matthanley @ErisDS @naz can I look into it?

I think the error is @ routes.js => authentication.js => accept.js .

accept.js only checks the invitation model and check two things:

• if invitation exists

• if invitation has not expired

Then it calls the user model to add
await models.User.add({ email: data.email, name: data.name, password: data.password, roles: [invite.toJSON().role_id] }, options);

It should first check if the email already exists on user model before doing the above addition

[naz]

Hey @imox2! You got the root of the problem nailed. As you've pointed out, adding an additional check should prevent having this error :+1:

In this situation we'd need to throw more specific ValidationError error, so the user accepting the invite knows how to act on the problem. Think @peterzimon would be able to provide a good copy for such error. What I'm thinking of something along these lines (inspiration from existing errors and given the context user would see this error):

message: "Could not create an account."
context: "Attempting to create an account with existing email address."
help: "Use different email address to register your account."

@imox2 fyi, the copy above will have to be place in the i18n file around here, but need confirmation from Zimo first. Also, when you are working on this problem would be nice to have a regression test similar to this one that replicates this error.

[imox2]

@naz Thanks for the valuable info. I will wait for the confirmation from @peterzimon before starting. I will provide the regression test with the solution 👍

[naz]

@imox2 I don't think the confirmation of the copy is blocking from starting on the issue :wink: It's just something to have in mind before everything goes into master

[imox2]

@naz Okay 🙂 . As i saw that signup.hbs is not using anything other than message to show the error to user.

As signup.js sets only message to flowErrors

this.set('flowErrors', error.payload.errors[0].message);

Should i have to make changes in signup.hbs to show context , something like it is done in modal-import-members.hbs

{{#if error.context}}
<p class="gh-members-import-errorcontext">{{{error.context}}}</p>
{{/if}}

[naz]

Great you've noticed. No need to make changes to how errors are displayed for this issue, that's different scope :) The context and help available in response error will mostly be useful for general API consumers.

[imox2]

@naz yes, I noticed help and context were visible on the console.

Shouldn't we show user a different message than "Could not create an account." . Something like the context you mentioned above "Attempting to create an account with existing email address." User won't know why his account is not getting created if we show "Could not create an account."

[imox2]

@naz Also, even if we make sure that user is not able to sign up using an already existing email.

I have a few related questions:

• why are we allowing user to select any email of his choice

• I looked into invite model and can see that it has an email field. Can we not allow signup only if token and email matches on the model instead of allowing correct token + any email?

Thanks

[imox2]

@naz

I think we wouldn't have to test anything with user. Rather just adding an extra condition to the invite ( that email and token both should match) will handle everything.

Changing:

let invite = await models.Invite.findOne({token: inviteToken, status: 'sent'}, options);

To

let invite = await models.Invite.findOne({token: inviteToken, status: 'sent', email: data.email}, options);

Scenarios:

• If i get invitation on email x, if i try to signup with email y, I will get an error _Invite not found_

• If i get invitation on email x, if i try to signup with email x, sign up will succeed, and invitation deleted from model

[naz]

@imox2 cool to see you diving deeper into the problem :+1: The ability to signup with a different email is by design and has been functioning like this since the very introduction of invitations. We are not looking to change this logic right now as that needs a lot more consideration :smiley: Would focus on fixing the 500 itself. Changing the design of signup flow would be something we could review once there's a need to.

[imox2]

@naz I have sent a pull request fixing the 500 and also added a test