Allow users to add multiple email addresses to their account

[kailan]

👋 Hello crates.io team,

This pull request updates the emails table to support multiple email addresses per user, as well as adding API endpoints for managing emails (/api/v1/users/{user}/emails), and the relevant UI changes to go with it, closing #11597.

Please see the below video for a demonstration:

cratesio.video.mov

Example user response

{
  "id": 1,
  "login": "arbitrary_username",
  "emails": [
    {
      "id": 1,
      "email": "foo@example.com",
      "verified": true,
      "verification_email_sent": true,
      "send_notifications": true
    },
    {
      "id": 2,
      "email": "bar@example.com",
      "verified": false,
      "verification_email_sent": true,
      "send_notifications": false
    }
  ],
  "name": null,
  "email_verified": true,
  "email_verification_sent": true,
  "email": "foo@example.com",
  "avatar": null,
  "url": "https://github.com/arbitrary_username",
  "is_admin": false,
  "publish_notifications": true
}

Points to consider

API Stability

I have implemented this in a way that all existing API endpoints still work and previously-used properties are still available – specifically:

• PUT /api/v1/users/{user} will still respect the email parameter, adding the given email to the account. This endpoint is still needed for toggling publish notifications at the user level.
• GET /api/v1/me still returns email, email_verified, email_verification_sent – these correspond to the matching properties on the email configured as the notification email. Internally these are now prefixed with notification_ and marked as deprecated.

Is backwards compatibility expected here, or would it be okay to consider this a breaking change, removing the legacy behaviours and properties? The UI has been updated to not use them.

Duplicate Emails

This implementation allows an email address to exist on more than one user account. Is this wanted or should there be a constraint to restrict this?

GitHub Email Sync

Upon login, users are now prompted to also provide the user:email scope. This allows us to fetch all emails from the user's GitHub account and add them to their crates.io account. This will also happen on subsequent logins in the case that a user has added more emails to their GitHub account.

Is this behaviour wanted? Will users be concerned about providing this extra permission?

NOTE: The session controller will still work as it did before if fetching the extra emails from GitHub fails, in the event that the user:email scope is not granted, but I cannot see a way for a user to opt out of this while still going through the login process, despite GitHub documentation suggesting that this is possible: https://docs.github.com/en/apps/oauth-apps/building-oauth-apps/scopes-for-oauth-apps#requested-scopes-and-granted-scopes

Email Verification

With the extra user:email scope, the app can now determine if an email has been verified by GitHub. As long as the crates.io team are comfortable trusting that GitHub has verified the email correctly, this means that crates.io would no longer have to send a verification email, which reduces the friction to sign up and would also reduce the usage of the email provider.

If you'd prefer to verify them manually anyway, that change is straightforward to revert.

---

It was a pleasure to work on this and I look forward to collaborating with you to get this shipped. I have not previously worked with Ember so it is likely I have not followed best practices while implementing the UI elements. Please feel free to reach out with any questions or concerns!

[kailan]

Going to work on resolving the failing tests.

[Turbo87]

The PR is rather large, which generally makes it a bit harder to review, so I would recommend splitting it up, which should also make the deployment a bit more resilient to any issues.

My recommendation, I think, would be three (or four) steps:

1. database changes, without affecting the API output
2. API changes, without affecting the frontend
3. frontend changes
4. (maybe the GitHub API changes should be a dedicated step?)

Then we can review and deploy these steps separately and address any potential issues before continuing with the next one.

I've focused the initial round of review on the database changes, since that would be step one of the deployment.

[Turbo87]

why do we allow deletion if it's the only email address?

[kailan]

You can't delete a user row if there is an email row referencing it, so I did this to allow for users to be deleted.

Whether that's beneficial in development or production is a question to be answered I suppose.

[Turbo87]

hmm, I'm not that familiar with that constraint but that seems like a job for "on cascade delete" unless I'm missing something 🤔

[Turbo87]

since this migration introduces a lot of new database constraints it might be worth it to write dedicated tests for them. in other words, set up a test database, trigger these constraints with regular diesel calls and then use snapshot assertions to check that the errors are returned as expected.

[kailan]

Thanks very much for the feedback @Turbo87 – I'll address each point in this branch so I have everything working together, then I'll split it out into separate PRs for review.

now go enjoy your holiday!!