From 54b273a6e90061588953842ceead28132da2d9bc Mon Sep 17 00:00:00 2001 From: Urjeet Patel Date: Fri, 3 Jun 2022 14:22:11 +0000 Subject: [PATCH 01/12] chore: :adhesive_bandage: Add No Cover to app startup since it is not being tested --- project_name/app.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/project_name/app.py b/project_name/app.py index c3bdfe0..691f176 100644 --- a/project_name/app.py +++ b/project_name/app.py @@ -56,4 +56,4 @@ def read(*paths, **kwargs): @app.on_event("startup") def on_startup(): - create_db_and_tables(engine) + create_db_and_tables(engine) # pragma: no cover From 6d475907eaefd309923383c4055ff01d315cfb77 Mon Sep 17 00:00:00 2001 From: Urjeet Patel Date: Fri, 3 Jun 2022 14:44:08 +0000 Subject: [PATCH 02/12] fix: :bug: 404 not returned for incorrect slug --- project_name/routes/content.py | 5 +++-- tests/test_content_api.py | 12 ++++++++++++ 2 files changed, 15 insertions(+), 2 deletions(-) diff --git a/project_name/routes/content.py b/project_name/routes/content.py index 7ee33ec..55e3fad 100644 --- a/project_name/routes/content.py +++ b/project_name/routes/content.py @@ -21,15 +21,16 @@ async def list_contents(*, session: Session = ActiveSession): async def query_content( *, id_or_slug: Union[str, int], session: Session = ActiveSession ): - content = session.query(Content).where( + query = select(Content).where( or_( Content.id == id_or_slug, Content.slug == id_or_slug, ) ) + content = session.exec(query).one_or_none() if not content: raise HTTPException(status_code=404, detail="Content not found") - return content.first() + return content @router.post( diff --git a/tests/test_content_api.py b/tests/test_content_api.py index 5ff8533..28b7c82 100644 --- a/tests/test_content_api.py +++ b/tests/test_content_api.py @@ -18,3 +18,15 @@ def test_content_list(api_client_authenticated): assert response.status_code == 200 result = response.json() assert result[0]["slug"] == "hello-test" + + +def test_content_get_individual(api_client_authenticated): + response = api_client_authenticated.get("/content/hello-test") + assert response.status_code == 200 + result = response.json() + assert result["slug"] == "hello-test" + + +def test_content_get_individual_404(api_client_authenticated): + response = api_client_authenticated.get("/content/does-not-exist/") + assert response.status_code == 404 From 49763280fd54697aab2e7b2d73f0f91173162551 Mon Sep 17 00:00:00 2001 From: Urjeet Patel Date: Fri, 3 Jun 2022 18:17:02 +0000 Subject: [PATCH 03/12] test: :white_check_mark: Add Tests to completly cover update content --- tests/conftest.py | 18 +++++++++++++++ tests/test_content_api.py | 47 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 65 insertions(+) diff --git a/tests/conftest.py b/tests/conftest.py index 05abd61..f30d74e 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -62,6 +62,24 @@ def api_client_authenticated(): return client +@pytest.fixture(scope="function") +def api_client_not_superuser(): + + try: + create_user("regular", "regular", superuser=False) + except IntegrityError: + pass + + client = TestClient(app) + token = client.post( + "/token", + data={"username": "regular", "password": "regular"}, + headers={"Content-Type": "application/x-www-form-urlencoded"}, + ).json()["access_token"] + client.headers["Authorization"] = f"Bearer {token}" + return client + + @pytest.fixture(scope="function") def cli_client(): return CliRunner() diff --git a/tests/test_content_api.py b/tests/test_content_api.py index 28b7c82..35463f2 100644 --- a/tests/test_content_api.py +++ b/tests/test_content_api.py @@ -30,3 +30,50 @@ def test_content_get_individual(api_client_authenticated): def test_content_get_individual_404(api_client_authenticated): response = api_client_authenticated.get("/content/does-not-exist/") assert response.status_code == 404 + + +def test_content_update(api_client_authenticated): + response = api_client_authenticated.post( + "/content/", + json={ + "title": "Test Post 2 for Patch", + "text": "this is just a test", + "published": True, + "tags": ["test", "hello"], + }, + ) + assert response.status_code == 200 + result = response.json() + assert result["slug"] == "test-post-2-for-patch" + response2 = api_client_authenticated.patch( + f"/content/{result['id']}/", + json={ + "published": "false", + }, + ) + assert response2.status_code == 200 + result2 = response2.json() + assert result["slug"] == result2["slug"] + assert result["text"] == result2["text"] + assert result["tags"] == result2["tags"] + assert result["published"] != result2["published"] + + +def test_content_update_404(api_client_authenticated): + response = api_client_authenticated.patch( + "/content/999/", + json={ + "published": "false", + }, + ) + assert response.status_code == 404 + + +def test_content_update_unauthorized(api_client_not_superuser): + response = api_client_not_superuser.patch( + "/content/1/", + json={ + "published": "false", + }, + ) + assert response.status_code == 403 From 494ee5c8ab9ca2a8d52671dc95a029ea9aeebf3e Mon Sep 17 00:00:00 2001 From: Urjeet Patel Date: Fri, 3 Jun 2022 18:22:15 +0000 Subject: [PATCH 04/12] test: :white_check_mark: Completly test content delete --- tests/test_content_api.py | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/tests/test_content_api.py b/tests/test_content_api.py index 35463f2..e7354e9 100644 --- a/tests/test_content_api.py +++ b/tests/test_content_api.py @@ -77,3 +77,18 @@ def test_content_update_unauthorized(api_client_not_superuser): }, ) assert response.status_code == 403 + + +def test_content_delete_404(api_client_authenticated): + response = api_client_authenticated.delete("/content/999/") + assert response.status_code == 404 + + +def test_content_delete_unauthorized(api_client_not_superuser): + response = api_client_not_superuser.delete("/content/2/") + assert response.status_code == 403 + + +def test_content_delete(api_client_authenticated): + response = api_client_authenticated.delete("/content/2/") + assert response.status_code == 200 From 49d984abb08a306133e0fd7a9d640ba12ada548a Mon Sep 17 00:00:00 2001 From: Urjeet Patel Date: Fri, 3 Jun 2022 18:46:23 +0000 Subject: [PATCH 05/12] test: :white_check_mark: Tests for querying users --- tests/test_user_api.py | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/tests/test_user_api.py b/tests/test_user_api.py index c7d547e..be6aff2 100644 --- a/tests/test_user_api.py +++ b/tests/test_user_api.py @@ -18,3 +18,27 @@ def test_user_create(api_client_authenticated): assert response.status_code == 200 result = response.json() assert result["username"] == "foo" + + +def test_get_my_profile(api_client_not_superuser): + response = api_client_not_superuser.get("/user/me") + assert response.status_code == 200 + result = response.json() + assert result["username"] == "regular" + assert result["id"] == 3 + + +def test_get_other_profile_by_id(api_client_not_superuser): + response = api_client_not_superuser.get("/user/1") + assert response.status_code == 200 + result = response.json() + assert result["username"] == "admin2" + assert result["id"] == 1 + + +def test_get_other_profile_by_name(api_client_not_superuser): + response = api_client_not_superuser.get("/user/admin") + assert response.status_code == 200 + result = response.json() + assert result["username"] == "admin" + assert result["id"] == 2 From 1f37e8564306160c7e75eb6f6502edcfef438aac Mon Sep 17 00:00:00 2001 From: Urjeet Patel Date: Fri, 3 Jun 2022 18:46:55 +0000 Subject: [PATCH 06/12] refactor: :bug: Change the order of functions for proper wildcard matching --- project_name/routes/user.py | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/project_name/routes/user.py b/project_name/routes/user.py index 6f98600..7f97666 100644 --- a/project_name/routes/user.py +++ b/project_name/routes/user.py @@ -69,6 +69,13 @@ async def update_user_password( session.refresh(user) return user +# Order of these functions matters here +#The /me/ path needs to be higher than the wildcard path below or else +# this function will never be called. +@router.get("/me/", response_model=UserResponse) +async def my_profile(current_user: User = AuthenticatedUser): + return current_user + @router.get( "/{user_id_or_username}/", @@ -90,11 +97,6 @@ async def query_user( return user.first() -@router.get("/me/", response_model=UserResponse) -async def my_profile(current_user: User = AuthenticatedUser): - return current_user - - @router.delete("/{user_id}/", dependencies=[AdminUser]) def delete_user( *, session: Session = ActiveSession, request: Request, user_id: int From f2edea63b9f7f429ed5825b3c6763eb6d2450ea5 Mon Sep 17 00:00:00 2001 From: Urjeet Patel Date: Fri, 3 Jun 2022 18:50:27 +0000 Subject: [PATCH 07/12] fix: :bug: Query User not returning 404 --- project_name/routes/user.py | 9 +++++---- tests/test_user_api.py | 6 ++++++ 2 files changed, 11 insertions(+), 4 deletions(-) diff --git a/project_name/routes/user.py b/project_name/routes/user.py index 7f97666..cd414d2 100644 --- a/project_name/routes/user.py +++ b/project_name/routes/user.py @@ -69,8 +69,9 @@ async def update_user_password( session.refresh(user) return user + # Order of these functions matters here -#The /me/ path needs to be higher than the wildcard path below or else +# The /me/ path needs to be higher than the wildcard path below or else # this function will never be called. @router.get("/me/", response_model=UserResponse) async def my_profile(current_user: User = AuthenticatedUser): @@ -85,16 +86,16 @@ async def my_profile(current_user: User = AuthenticatedUser): async def query_user( *, session: Session = ActiveSession, user_id_or_username: Union[str, int] ): - user = session.query(User).where( + query = select(User).where( or_( User.id == user_id_or_username, User.username == user_id_or_username, ) ) - + user = session.exec(query).one_or_none() if not user: raise HTTPException(status_code=404, detail="User not found") - return user.first() + return user @router.delete("/{user_id}/", dependencies=[AdminUser]) diff --git a/tests/test_user_api.py b/tests/test_user_api.py index be6aff2..29999b2 100644 --- a/tests/test_user_api.py +++ b/tests/test_user_api.py @@ -42,3 +42,9 @@ def test_get_other_profile_by_name(api_client_not_superuser): result = response.json() assert result["username"] == "admin" assert result["id"] == 2 + + +def test_get_other_profile_404(api_client_not_superuser): + response = api_client_not_superuser.get("/user/99999") + result = response.json() + assert response.status_code == 404 From 14d96a647f3a26a38616f572c91dc94110eeb96f Mon Sep 17 00:00:00 2001 From: Urjeet Patel Date: Fri, 3 Jun 2022 19:13:49 +0000 Subject: [PATCH 08/12] test: :white_check_mark: Completly cover user update endpoints with tests --- tests/test_user_api.py | 40 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 40 insertions(+) diff --git a/tests/test_user_api.py b/tests/test_user_api.py index 29999b2..7869712 100644 --- a/tests/test_user_api.py +++ b/tests/test_user_api.py @@ -48,3 +48,43 @@ def test_get_other_profile_404(api_client_not_superuser): response = api_client_not_superuser.get("/user/99999") result = response.json() assert response.status_code == 404 + + +def test_change_password_404(api_client_not_superuser): + response = api_client_not_superuser.patch( + "/user/99999/password/", + json={"password": "string", "password_confirm": "string"}, + ) + result = response.text + assert response.status_code == 404 + + +def test_change_password_unauthorised(api_client_not_superuser): + response = api_client_not_superuser.patch( + "/user/1/password/", + json={"password": "string", "password_confirm": "string"}, + ) + result = response.text + assert response.status_code == 403 + + +def test_change_password_no_match(api_client_not_superuser): + my_user = api_client_not_superuser.get("/user/me/").json() + response = api_client_not_superuser.patch( + f"/user/{my_user['id']}/password/", + json={"password": "string", "password_confirm": "string1"}, + ) + assert response.status_code == 400 + result = response.json() + assert result["detail"] == "Passwords don't match" + + +def test_change_password(api_client_not_superuser): + my_user = api_client_not_superuser.get("/user/me/").json() + response = api_client_not_superuser.patch( + f"/user/{my_user['id']}/password/", + json={"password": "string", "password_confirm": "string"}, + ) + assert response.status_code == 200 + result = response.json() + assert result == my_user From 2b8496e28526898c0b5a1a6c901495747b58a2b9 Mon Sep 17 00:00:00 2001 From: Urjeet Patel Date: Fri, 3 Jun 2022 19:27:18 +0000 Subject: [PATCH 09/12] test: :white_check_mark: Complete tests for user deletion --- tests/test_user_api.py | 36 ++++++++++++++++++++++++++++++++++++ 1 file changed, 36 insertions(+) diff --git a/tests/test_user_api.py b/tests/test_user_api.py index 7869712..7b064d7 100644 --- a/tests/test_user_api.py +++ b/tests/test_user_api.py @@ -88,3 +88,39 @@ def test_change_password(api_client_not_superuser): assert response.status_code == 200 result = response.json() assert result == my_user + + +def test_change_password_by_admin(api_client_authenticated): + regular_user = api_client_authenticated.get("/user/regular/").json() + response = api_client_authenticated.patch( + f"/user/{regular_user['id']}/password/", + json={"password": "string", "password_confirm": "string"}, + ) + assert response.status_code == 200 + result = response.json() + assert result == regular_user + + +def test_delete_user_404(api_client_authenticated): + response = api_client_authenticated.delete( + "/user/99999/", + ) + assert response.status_code == 404 + + +def test_delete_user_self_not_allowed(api_client_authenticated): + my_user = api_client_authenticated.get("/user/me/").json() + response = api_client_authenticated.delete( + f"/user/{my_user['id']}/", + ) + assert response.status_code == 403 + + +def test_delete_user(api_client_authenticated): + user = api_client_authenticated.get("/user/foo/").json() + response = api_client_authenticated.delete( + f"/user/{user['id']}/", + ) + assert response.status_code == 200 + result = response.json() + assert result["ok"] == True From 0cd961d274b86ea50e3cd672cdff03b37b46181d Mon Sep 17 00:00:00 2001 From: Urjeet Patel Date: Fri, 3 Jun 2022 19:30:47 +0000 Subject: [PATCH 10/12] chore: :adhesive_bandage: Add no coverage tag for type_checking --- project_name/models/content.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/project_name/models/content.py b/project_name/models/content.py index 3497122..d6b18b9 100644 --- a/project_name/models/content.py +++ b/project_name/models/content.py @@ -4,7 +4,7 @@ from pydantic import BaseModel, Extra from sqlmodel import Field, Relationship, SQLModel -if TYPE_CHECKING: +if TYPE_CHECKING: # pragma: no coverage from project_name.security import User From b78df8ae5202366c018f23031b9cb5141a0f18a6 Mon Sep 17 00:00:00 2001 From: Urjeet Patel Date: Fri, 3 Jun 2022 19:40:50 +0000 Subject: [PATCH 11/12] test: :white_check_mark: Test user login api for incorrect username and password --- tests/test_security_api.py | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) create mode 100644 tests/test_security_api.py diff --git a/tests/test_security_api.py b/tests/test_security_api.py new file mode 100644 index 0000000..aaf1a52 --- /dev/null +++ b/tests/test_security_api.py @@ -0,0 +1,22 @@ +from fastapi.testclient import TestClient +from project_name import app + + +def test_user_login_no_username_match(): + client = TestClient(app) + response = client.post( + "/token", + data={"username": "doesNotExist", "password": "doesNotExist"}, + headers={"Content-Type": "application/x-www-form-urlencoded"}, + ) + assert response.status_code == 401 + + +def test_user_login_no_password_match(): + client = TestClient(app) + response = client.post( + "/token", + data={"username": "admin", "password": "incorrect password"}, + headers={"Content-Type": "application/x-www-form-urlencoded"}, + ) + assert response.status_code == 401 From 66d9e286101678c7939cdadc2b2a3014f64ef63b Mon Sep 17 00:00:00 2001 From: Urjeet Patel Date: Fri, 3 Jun 2022 20:15:08 +0000 Subject: [PATCH 12/12] test: :white_check_mark: add tests for security module --- project_name/security.py | 10 +++++----- tests/test_security.py | 17 +++++++++++++++++ tests/test_security_api.py | 13 +++++++++++++ 3 files changed, 35 insertions(+), 5 deletions(-) create mode 100644 tests/test_security.py diff --git a/project_name/security.py b/project_name/security.py index c2aaefb..b418df8 100644 --- a/project_name/security.py +++ b/project_name/security.py @@ -50,7 +50,7 @@ def __get_validators__(cls): @classmethod def validate(cls, v): """Accepts a plain text password and returns a hashed password.""" - if not isinstance(v, str): + if not isinstance(v, str): # pragma: no coverage raise TypeError("string required") hashed_password = get_password_hash(v) @@ -114,7 +114,7 @@ def create_access_token( to_encode = data.copy() if expires_delta: expire = datetime.utcnow() + expires_delta - else: + else: # pragma: no cover expire = datetime.utcnow() + timedelta(minutes=15) to_encode.update({"exp": expire}) encoded_jwt = jwt.encode(to_encode, SECRET_KEY, algorithm=ALGORITHM) @@ -150,7 +150,7 @@ def get_current_user( if authorization := request.headers.get("authorization"): try: token = authorization.split(" ")[1] - except IndexError: + except IndexError: # pragma: no cover raise credentials_exception try: @@ -170,7 +170,7 @@ def get_current_user( async def get_current_active_user( current_user: User = Depends(get_current_user), ) -> User: - if current_user.disabled: + if current_user.disabled: # pragma: no cover raise HTTPException(status_code=400, detail="Inactive user") return current_user @@ -181,7 +181,7 @@ async def get_current_active_user( async def get_current_admin_user( current_user: User = Depends(get_current_user), ) -> User: - if not current_user.superuser: + if not current_user.superuser: # pragma: no cover raise HTTPException( status_code=status.HTTP_403_FORBIDDEN, detail="Not an admin user" ) diff --git a/tests/test_security.py b/tests/test_security.py new file mode 100644 index 0000000..6cb8da8 --- /dev/null +++ b/tests/test_security.py @@ -0,0 +1,17 @@ +from project_name import security +from fastapi.exceptions import HTTPException +import pytest + + +def test_get_current_user(): + malformed_token_no_username = security.create_access_token({}) + with pytest.raises(HTTPException): + user = security.get_current_user(token=malformed_token_no_username) + + malformed_token_invalid_username = security.create_access_token( + {"sub": "InvalidUserName"} + ) + with pytest.raises(HTTPException): + user = security.get_current_user( + token=malformed_token_invalid_username + ) diff --git a/tests/test_security_api.py b/tests/test_security_api.py index aaf1a52..1cf96d4 100644 --- a/tests/test_security_api.py +++ b/tests/test_security_api.py @@ -20,3 +20,16 @@ def test_user_login_no_password_match(): headers={"Content-Type": "application/x-www-form-urlencoded"}, ) assert response.status_code == 401 + + +def test_secure_api_malformed_headers(): + client = TestClient(app) + token = client.post( + "/token", + data={"username": "admin", "password": "admin"}, + headers={"Content-Type": "application/x-www-form-urlencoded"}, + ).json()["access_token"] + malformed_token = token[:5] + "a" + token[5:] + client.headers["Authorization"] = f"Bearer {malformed_token}" + response = client.get("/user/me") + assert response.status_code == 401