From b7dbdf6ea65f08cffe1d650faf001141bc8ccf60 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fran=C3=A7ois=20Voron?= Date: Thu, 9 Jul 2020 18:44:25 +0200 Subject: [PATCH] Fix #245 : handle email as case insensitive while retrieving user in DB (#250) * Add unit tests to enforce email case insensitivity * Handle email as case insentitive while retrieving user in DB * Apply isort/black * Add migration doc --- docs/migration/2x_to_3x.md | 10 ++++++++++ fastapi_users/db/mongodb.py | 4 +++- fastapi_users/db/sqlalchemy.py | 6 ++++-- fastapi_users/db/tortoise.py | 16 ++++++++-------- mkdocs.yml | 1 + tests/conftest.py | 14 ++++++++------ tests/test_db_mongodb.py | 5 +++++ tests/test_db_sqlalchemy.py | 5 +++++ tests/test_db_tortoise.py | 5 +++++ tests/test_router_auth.py | 7 +++++-- tests/test_router_register.py | 15 +++++++++++---- tests/test_router_reset.py | 7 +++++-- 12 files changed, 70 insertions(+), 25 deletions(-) create mode 100644 docs/migration/2x_to_3x.md diff --git a/docs/migration/2x_to_3x.md b/docs/migration/2x_to_3x.md new file mode 100644 index 00000000..fc1e83cf --- /dev/null +++ b/docs/migration/2x_to_3x.md @@ -0,0 +1,10 @@ +# 2.x.x ➡️ 3.x.x + +## Emails are now case-insensitive + +Before 3.x.x, the local part (before the @) of the email address was case-sensitive. Therefore, `king.arthur@camelot.bt` and `King.Arthur@camelot.bt` were considered as **two different users**. This behaviour was a bit confusing and not consistent with 99% of web services out there. + +After 3.x.x, users are fetched from the database with a case-insensitive email search. Bear in mind though that if the user registers with the email `King.Arthur@camelot.bt`, it will be stored exactly like this in the database (with casing) ; but he will be able to login as `king.arthur@camelot.bt`. + +!!! danger + It's super important then, before you upgrade to 3.x.x that you **check if there are several users with the same email with different cases** ; and that you **merge or delete those accounts**. diff --git a/fastapi_users/db/mongodb.py b/fastapi_users/db/mongodb.py index 797a407a..06723c5b 100644 --- a/fastapi_users/db/mongodb.py +++ b/fastapi_users/db/mongodb.py @@ -28,7 +28,9 @@ class MongoDBUserDatabase(BaseUserDatabase[UD]): return self.user_db_model(**user) if user else None async def get_by_email(self, email: str) -> Optional[UD]: - user = await self.collection.find_one({"email": email}) + user = await self.collection.find_one( + {"email": {"$regex": email, "$options": "i"}} + ) return self.user_db_model(**user) if user else None async def get_by_oauth_account(self, oauth: str, account_id: str) -> Optional[UD]: diff --git a/fastapi_users/db/sqlalchemy.py b/fastapi_users/db/sqlalchemy.py index b5a57b4a..81cd870a 100644 --- a/fastapi_users/db/sqlalchemy.py +++ b/fastapi_users/db/sqlalchemy.py @@ -3,7 +3,7 @@ from typing import Mapping, Optional, Type from databases import Database from pydantic import UUID4 -from sqlalchemy import Boolean, Column, ForeignKey, Integer, String, Table, select +from sqlalchemy import Boolean, Column, ForeignKey, Integer, String, Table, func, select from sqlalchemy.dialects.postgresql import UUID from sqlalchemy.ext.declarative import declared_attr from sqlalchemy.types import CHAR, TypeDecorator @@ -120,7 +120,9 @@ class SQLAlchemyUserDatabase(BaseUserDatabase[UD]): return await self._make_user(user) if user else None async def get_by_email(self, email: str) -> Optional[UD]: - query = self.users.select().where(self.users.c.email == email) + query = self.users.select().where( + func.lower(self.users.c.email) == func.lower(email) + ) user = await self.database.fetch_one(query) return await self._make_user(user) if user else None diff --git a/fastapi_users/db/tortoise.py b/fastapi_users/db/tortoise.py index 80f04f11..7902242e 100644 --- a/fastapi_users/db/tortoise.py +++ b/fastapi_users/db/tortoise.py @@ -77,19 +77,19 @@ class TortoiseUserDatabase(BaseUserDatabase[UD]): return None async def get_by_email(self, email: str) -> Optional[UD]: - try: - query = self.model.get(email=email) + query = self.model.filter(email__iexact=email).first() - if self.oauth_account_model is not None: - query = query.prefetch_related("oauth_accounts") + if self.oauth_account_model is not None: + query = query.prefetch_related("oauth_accounts") - user = await query - user_dict = await user.to_dict() + user = await query - return self.user_db_model(**user_dict) - except DoesNotExist: + if user is None: return None + user_dict = await user.to_dict() + return self.user_db_model(**user_dict) + async def get_by_oauth_account(self, oauth: str, account_id: str) -> Optional[UD]: try: query = self.model.get( diff --git a/mkdocs.yml b/mkdocs.yml index 9e483812..5fc97ad3 100644 --- a/mkdocs.yml +++ b/mkdocs.yml @@ -53,3 +53,4 @@ nav: - Migration: - migration/08_to_1x.md - migration/1x_to_2x.md + - migration/2x_to_3x.md diff --git a/tests/conftest.py b/tests/conftest.py index 91885064..ff65ae71 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -153,11 +153,12 @@ def mock_user_db(user, inactive_user, superuser) -> BaseUserDatabase: return None async def get_by_email(self, email: str) -> Optional[UserDB]: - if email == user.email: + lower_email = email.lower() + if lower_email == user.email.lower(): return user - if email == inactive_user.email: + if lower_email == inactive_user.email.lower(): return inactive_user - if email == superuser.email: + if lower_email == superuser.email.lower(): return superuser return None @@ -188,11 +189,12 @@ def mock_user_db_oauth( return None async def get_by_email(self, email: str) -> Optional[UserDBOAuth]: - if email == user_oauth.email: + lower_email = email.lower() + if lower_email == user_oauth.email.lower(): return user_oauth - if email == inactive_user_oauth.email: + if lower_email == inactive_user_oauth.email.lower(): return inactive_user_oauth - if email == superuser_oauth.email: + if lower_email == superuser_oauth.email.lower(): return superuser_oauth return None diff --git a/tests/test_db_mongodb.py b/tests/test_db_mongodb.py index a9dfc562..288f1746 100644 --- a/tests/test_db_mongodb.py +++ b/tests/test_db_mongodb.py @@ -77,6 +77,11 @@ async def test_queries(mongodb_user_db: MongoDBUserDatabase[UserDB]): assert email_user is not None assert email_user.id == user_db.id + # Get by uppercased email + email_user = await mongodb_user_db.get_by_email("Lancelot@camelot.bt") + assert email_user is not None + assert email_user.id == user_db.id + # Exception when inserting existing email with pytest.raises(pymongo.errors.DuplicateKeyError): await mongodb_user_db.create(user) diff --git a/tests/test_db_sqlalchemy.py b/tests/test_db_sqlalchemy.py index cb8cd3c9..50e9b849 100644 --- a/tests/test_db_sqlalchemy.py +++ b/tests/test_db_sqlalchemy.py @@ -95,6 +95,11 @@ async def test_queries(sqlalchemy_user_db: SQLAlchemyUserDatabase[UserDB]): assert email_user is not None assert email_user.id == user_db.id + # Get by uppercased email + email_user = await sqlalchemy_user_db.get_by_email("Lancelot@camelot.bt") + assert email_user is not None + assert email_user.id == user_db.id + # Exception when inserting existing email with pytest.raises(sqlite3.IntegrityError): await sqlalchemy_user_db.create(user) diff --git a/tests/test_db_tortoise.py b/tests/test_db_tortoise.py index 716faff9..b936d17d 100644 --- a/tests/test_db_tortoise.py +++ b/tests/test_db_tortoise.py @@ -80,6 +80,11 @@ async def test_queries(tortoise_user_db: TortoiseUserDatabase[UserDB]): assert email_user is not None assert email_user.id == user_db.id + # Get by uppercased email + email_user = await tortoise_user_db.get_by_email("Lancelot@camelot.bt") + assert email_user is not None + assert email_user.id == user_db.id + # Exception when inserting existing email with pytest.raises(IntegrityError): await tortoise_user_db.create(user) diff --git a/tests/test_router_auth.py b/tests/test_router_auth.py index b7e7b553..81ff92b9 100644 --- a/tests/test_router_auth.py +++ b/tests/test_router_auth.py @@ -63,10 +63,13 @@ class TestLogin: data = cast(Dict[str, Any], response.json()) assert data["detail"] == ErrorCode.LOGIN_BAD_CREDENTIALS + @pytest.mark.parametrize( + "email", ["king.arthur@camelot.bt", "King.Arthur@camelot.bt"] + ) async def test_valid_credentials( - self, path, test_app_client: httpx.AsyncClient, user: UserDB + self, path, email, test_app_client: httpx.AsyncClient, user: UserDB ): - data = {"username": "king.arthur@camelot.bt", "password": "guinevere"} + data = {"username": email, "password": "guinevere"} response = await test_app_client.post(path, data=data) assert response.status_code == status.HTTP_200_OK assert response.json() == {"token": str(user.id)} diff --git a/tests/test_router_register.py b/tests/test_router_register.py index e1299240..8ade5675 100644 --- a/tests/test_router_register.py +++ b/tests/test_router_register.py @@ -65,18 +65,24 @@ class TestRegister: assert response.status_code == status.HTTP_422_UNPROCESSABLE_ENTITY assert after_register.called is False + @pytest.mark.parametrize( + "email", ["king.arthur@camelot.bt", "King.Arthur@camelot.bt"] + ) async def test_existing_user( - self, test_app_client: httpx.AsyncClient, after_register + self, email, test_app_client: httpx.AsyncClient, after_register ): - json = {"email": "king.arthur@camelot.bt", "password": "guinevere"} + json = {"email": email, "password": "guinevere"} response = await test_app_client.post("/register", json=json) assert response.status_code == status.HTTP_400_BAD_REQUEST data = cast(Dict[str, Any], response.json()) assert data["detail"] == ErrorCode.REGISTER_USER_ALREADY_EXISTS assert after_register.called is False - async def test_valid_body(self, test_app_client: httpx.AsyncClient, after_register): - json = {"email": "lancelot@camelot.bt", "password": "guinevere"} + @pytest.mark.parametrize("email", ["lancelot@camelot.bt", "Lancelot@camelot.bt"]) + async def test_valid_body( + self, email, test_app_client: httpx.AsyncClient, after_register + ): + json = {"email": email, "password": "guinevere"} response = await test_app_client.post("/register", json=json) assert response.status_code == status.HTTP_201_CREATED assert after_register.called is True @@ -88,6 +94,7 @@ class TestRegister: actual_user = after_register.call_args[0][0] assert str(actual_user.id) == data["id"] + assert str(actual_user.email) == email request = after_register.call_args[0][1] assert isinstance(request, Request) diff --git a/tests/test_router_reset.py b/tests/test_router_reset.py index 32b26c2c..fde9a9b1 100644 --- a/tests/test_router_reset.py +++ b/tests/test_router_reset.py @@ -80,10 +80,13 @@ class TestForgotPassword: assert response.status_code == status.HTTP_202_ACCEPTED assert after_forgot_password.called is False + @pytest.mark.parametrize( + "email", ["king.arthur@camelot.bt", "King.Arthur@camelot.bt"] + ) async def test_existing_user( - self, test_app_client: httpx.AsyncClient, after_forgot_password, user + self, email, test_app_client: httpx.AsyncClient, after_forgot_password, user ): - json = {"email": "king.arthur@camelot.bt"} + json = {"email": email} response = await test_app_client.post("/forgot-password", json=json) assert response.status_code == status.HTTP_202_ACCEPTED assert after_forgot_password.called is True