Store emails in lowercase#888
Open
ignatiusm wants to merge 7 commits into
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Resolves #825 and resolves #542
Context
This PR has a bit of a strange origin story:
Last year, I was doing some testing and was able to create separate accounts for: joe@test.com; Joe@test.com; and joe@Test.com: #542 At some part of the app there was a search for duplicate emails, and given I was was able to create separate accounts I figured the duplicate email search should be case sensitive (backed up by some stackoverflow answers on a related question - it turns out the spec for emails is not very specific).
However, when we were getting CTRL Pen tested I encountered an issue of incompatibility with Auth0's behaviour of saving emails as lowercase: #825
On the basis of this we decided to also store emails as lowercase in CTRL. Emails are encypted at rest (using the prisma-field-encryption library. There is also an
emailHashfield to stores a hash of the email to allow for index creation and unique checks: e.g.CREATE UNIQUE INDEX "User_emailHash_key" ON "User"("emailHash");Tests and code fixes
AuthController
application/backend/src/controllers/AuthController.test.tsto check that registering an admin fails withemailHash already in usewhen an existing admin exists with an uppercase version of the email. Evidence of failing Auth test:application/backend/prisma/schema.prismato normalise the UseremailHash, but an explanatory note is also included inapplication/backend/src/controllers/AuthController.tsParticipantsController
application/backend/src/controllers/ParticipantsController.test.tsto check that when creating a batch of invites, emails with different cases do not result in two separate invites.Key fixes are in:
application/backend/src/controllers/ParticipantsController.tsto normalise email to lowercase when creating Invites for participantsapplication/backend/prisma/schema.prismato normalise the InvitesemailHashAdmin-portal invite Modal
application/admin-client/cypress/e2e/invites.cy.jstest to ensure that only lowercase emails are added to recipients list by typing and pasting.application/admin-client/src/components/InviteModal.tsxI did a bit of a refactor to simplify and clarify the logic for adding by typing vs adding by pasting and carve out some helper functions.Note: if you do not
make cleanthenyarn devandmake seedthen you may encounter errors due to prisma schema changes. You might need to runyarn prisma:generateto recreate the prisma client.I also created another issue (#890) to add invites for dev and test users. Without invites there is a chance for weird states to exist (e.g. a users should have an 'Accepted' invite) and we probably need tests for them!