Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ensure email comparisons are case-insensitive, emails stored as lowercase #2084

Merged
merged 8 commits into from
Sep 19, 2024

Conversation

ikreymer
Copy link
Member

@ikreymer ikreymer commented Sep 19, 2024

  • Add a custom EmailStr type which lowercases the full e-mail, not just the domain.
  • Ensure EmailStr is used throughout wherever e-mails are used, both for invites and user models
  • Tests: update to check for lowercase email responses, e-mails returned from APIs are always lowercase
  • Tests: remove tests where '@' was ur-lencoded, should not be possible since POSTing JSON and no url-decoding is done/expected. E-mails should have '@' present.
  • Fixes [Bug]: Invites to email with uppercase domain resulted in invalid_invite when registering #2083 where invites were rejected due to case differences
  • CI: pin pymongo dependency due to latest releases update, update python used for CI
  • bump to 1.11.7

@ikreymer ikreymer requested a review from tw4l September 19, 2024 01:00
@ikreymer
Copy link
Member Author

@tw4l Since we're using EmailStr more strictly now, we can't support url-encoded emails with @ as %40. If we want to support that, should add it to our custom EmailStr to url-decode emails, but I'm not sure if that's needed?

"""EmailStr type that lowercases the full email"""

@classmethod
def _validate(cls, value: CasedEmailStr, /) -> CasedEmailStr:
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Based on recommendation from:
pydantic/pydantic#798 (comment)

@tw4l
Copy link
Contributor

tw4l commented Sep 19, 2024

@tw4l Since we're using EmailStr more strictly now, we can't support url-encoded emails with @ as %40. If we want to support that, should add it to our custom EmailStr to url-decode emails, but I'm not sure if that's needed?

That's fine! Not sure I even meant to encode that part of the email addresses in the tests, that might have been a mistake.

@ikreymer ikreymer merged commit 7a61568 into 1.11.7-release Sep 19, 2024
5 checks passed
@ikreymer ikreymer deleted the 1.11.7-work branch September 19, 2024 17:05
ikreymer added a commit that referenced this pull request Sep 19, 2024
…case (#2084) (#2086) (fixes from 1.11.7)

- Add a custom EmailStr type which lowercases the full e-mail, not just
the domain.
- Ensure EmailStr is used throughout wherever e-mails are used, both for
invites and user models
- Tests: update to check for lowercase email responses, e-mails returned
from APIs are always lowercase
- Tests: remove tests where '@' was ur-lencoded, should not be possible
since POSTing JSON and no url-decoding is done/expected. E-mails should
have '@' present.
- Fixes #2083 where invites were rejected due to case differences
- CI: pin pymongo dependency due to latest releases update, update python used for CI
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants