From b9f22298246df1b5d0bdd31fb36696414bf95945 Mon Sep 17 00:00:00 2001 From: Wang Haoyang Date: Fri, 1 Nov 2024 01:35:28 +0800 Subject: [PATCH 01/10] Fix password reset backend -Move /api/password-reset backend endpoint from authenticated router to unauthenticated router -This fixes password reset at last -Previously, trying to reset password throws HTTP Unauthorized error, but we can't expect user to be logged in (authenticated) when they forgot their password) --- backend/src/auth/router.py | 47 ++++++++++++++++++++------------------ 1 file changed, 25 insertions(+), 22 deletions(-) diff --git a/backend/src/auth/router.py b/backend/src/auth/router.py index 2994137d..47683e23 100644 --- a/backend/src/auth/router.py +++ b/backend/src/auth/router.py @@ -151,27 +151,10 @@ def auth_google( ) -@routerWithAuth.get("/session") -def get_user( - current_user: Annotated[User, Depends(get_current_user)], - session=Depends(get_session), -) -> UserPublic: - user = session.get(User, current_user.id) - if user: - user.last_accessed = datetime.now() - session.add(user) - session.commit() - - return current_user - - -@routerWithAuth.get("/logout") -def logout(response: Response): - response.delete_cookie(key="session") - return "" - - -@routerWithAuth.post("/password-reset") +####################### +# Reset password # +####################### +@router.post("/password-reset") def request_password_reset( data: PasswordResetRequestData, background_task: BackgroundTasks, @@ -193,7 +176,7 @@ def request_password_reset( background_task.add_task(send_reset_password_email, email, code) -@routerWithAuth.put("/password-reset") +@router.put("/password-reset") def complete_password_reset( code: str, data: PasswordResetCompleteData, @@ -214,6 +197,26 @@ def complete_password_reset( session.commit() +@routerWithAuth.get("/session") +def get_user( + current_user: Annotated[User, Depends(get_current_user)], + session=Depends(get_session), +) -> UserPublic: + user = session.get(User, current_user.id) + if user: + user.last_accessed = datetime.now() + session.add(user) + session.commit() + + return current_user + + +@routerWithAuth.get("/logout") +def logout(response: Response): + response.delete_cookie(key="session") + return "" + + @routerWithAuth.put("/change-password") def change_password( user: Annotated[User, Depends(get_current_user)], From a30c88f50ab50081725ee74833f5691b1f5628f8 Mon Sep 17 00:00:00 2001 From: Wang Haoyang Date: Fri, 1 Nov 2024 01:39:50 +0800 Subject: [PATCH 02/10] Improve password reset backend -Mark all existing password reset codes for the given user as used -This prevents user from using old codes to reset password(potential security issue maybe?) --- backend/src/auth/router.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/backend/src/auth/router.py b/backend/src/auth/router.py index 47683e23..ac0efbc1 100644 --- a/backend/src/auth/router.py +++ b/backend/src/auth/router.py @@ -7,7 +7,7 @@ from fastapi import BackgroundTasks, Depends, APIRouter, HTTPException, Response from fastapi.security import OAuth2PasswordRequestForm import httpx -from sqlalchemy import select +from sqlalchemy import select, update from sqlalchemy.orm import selectinload from src.auth.utils import create_token, send_reset_password_email from src.common.constants import ( @@ -169,6 +169,10 @@ def request_password_reset( if not user: return + # Mark all existing password reset codes for the given user as used to prevent usage of old codes + session.execute(update(PasswordReset).where(PasswordReset.user_id == user.id).values(used=True)) + session.commit() + code = str(uuid4()) password_reset = PasswordReset(user_id=user.id, code=code, used=False) session.add(password_reset) From 259a2382dfbd26c3b916cc8ebbe8d836d1fca5a9 Mon Sep 17 00:00:00 2001 From: Wang Haoyang Date: Fri, 1 Nov 2024 01:47:23 +0800 Subject: [PATCH 03/10] Add more backend validation checks -Return a distinct HTTP response code to frontend when given password reset code has already been used -This prevents a potential security issue when someone tries to reuse an existing password reset code --- backend/src/auth/router.py | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/backend/src/auth/router.py b/backend/src/auth/router.py index ac0efbc1..196563c7 100644 --- a/backend/src/auth/router.py +++ b/backend/src/auth/router.py @@ -190,8 +190,13 @@ def complete_password_reset( password_reset = session.scalars( select(PasswordReset).where(PasswordReset.code == code) ).first() - if not password_reset or password_reset.used: + if not password_reset: + print("ERROR: Attempt to use password reset code that wasn't previously generated by Jippy") raise HTTPException(HTTPStatus.NOT_FOUND) + + if password_reset.used: + print("ERROR: Attempt to use password reset code that has already been used") + raise(HTTPException(HTTPStatus.CONFLICT)) user = session.get(User, password_reset.user_id) user.hashed_password = get_password_hash(data.password) From e29ebdaa87025b8d7a8d999e77fb10435a6aaddf Mon Sep 17 00:00:00 2001 From: Wang Haoyang Date: Fri, 1 Nov 2024 01:49:22 +0800 Subject: [PATCH 04/10] Improve password reset frontend page -Distinguish between different HTTP response codes returned from backend after sending password reset request -Display descriptive error messages that match each known response code that backend would return -Also provide a generic but user-friendly error message when an unknown HTTP response code for an error is returned from backend --- .../_components/reset-password-complete-form.tsx | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/frontend/app/(unauthenticated)/reset-password/_components/reset-password-complete-form.tsx b/frontend/app/(unauthenticated)/reset-password/_components/reset-password-complete-form.tsx index 9c91e822..c385c068 100644 --- a/frontend/app/(unauthenticated)/reset-password/_components/reset-password-complete-form.tsx +++ b/frontend/app/(unauthenticated)/reset-password/_components/reset-password-complete-form.tsx @@ -26,6 +26,7 @@ import { FormMessage, } from "@/components/ui/form"; import { Input } from "@/components/ui/input"; +import { HttpStatusCode } from "axios"; const resetPasswordCompleteFormSchema = z .object({ @@ -54,6 +55,7 @@ export default function ResetPasswordCompleteForm({ onComplete: () => void; }) { const [isError, setIsError] = useState(false); + const [errorMsg, setErrorMsg] = useState("Your password needs to match."); const form = useForm({ resolver: zodResolver(resetPasswordCompleteFormSchema), @@ -74,6 +76,13 @@ export default function ResetPasswordCompleteForm({ if (response.error) { setIsError(true); + if (response.status === HttpStatusCode.Conflict) { + setErrorMsg("Password reset link has expired. Please check your email for the latest link"); + } else if (response.status === HttpStatusCode.NotFound) { + setErrorMsg("Invalid password reset link. Please only click on links sent by us"); + } else { + setErrorMsg("Error while resetting your password. Please try again"); + } } else { setIsError(false); onComplete(); @@ -94,7 +103,7 @@ export default function ResetPasswordCompleteForm({ - Your password needs to match. + {errorMsg} )} From 22620eb8b75c75f8fac0f0788e0d7d4fc378e07b Mon Sep 17 00:00:00 2001 From: Wang Haoyang Date: Fri, 1 Nov 2024 01:57:01 +0800 Subject: [PATCH 05/10] Fix alignment on password reset page -Align the CircleAlert icon vertically in the Alert component on the password reset page when error message is to be displayed --- .../_components/reset-password-complete-form.tsx | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/frontend/app/(unauthenticated)/reset-password/_components/reset-password-complete-form.tsx b/frontend/app/(unauthenticated)/reset-password/_components/reset-password-complete-form.tsx index c385c068..1b7c1eb4 100644 --- a/frontend/app/(unauthenticated)/reset-password/_components/reset-password-complete-form.tsx +++ b/frontend/app/(unauthenticated)/reset-password/_components/reset-password-complete-form.tsx @@ -100,9 +100,11 @@ export default function ResetPasswordCompleteForm({ {isError && ( - - - + +
+ +
+ {errorMsg} From 2fdfca1a69f088df51210bff56f4520f35621406 Mon Sep 17 00:00:00 2001 From: Wang Haoyang Date: Fri, 1 Nov 2024 02:04:17 +0800 Subject: [PATCH 06/10] Fix alignment on register page -Vertically align the CircleAlert icon in the center of the Alert component --- frontend/app/(unauthenticated)/register/page.tsx | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/frontend/app/(unauthenticated)/register/page.tsx b/frontend/app/(unauthenticated)/register/page.tsx index f9581b22..ac253ea7 100644 --- a/frontend/app/(unauthenticated)/register/page.tsx +++ b/frontend/app/(unauthenticated)/register/page.tsx @@ -86,8 +86,10 @@ function RegisterPage() { {/* Body */} {isError && ( - - + +
+ +
This email is already registered.{" "} From c01fbf8a90472b58a86b32a2a0a0ecbfccf6151b Mon Sep 17 00:00:00 2001 From: Wang Haoyang Date: Fri, 1 Nov 2024 02:05:31 +0800 Subject: [PATCH 07/10] Fix password reset on frontend -Fix the link that 'Reset password' clickable components on the frontend redirect to -Now user is finally directed to the reset password page instead of a 404 Not Found page --- frontend/app/(unauthenticated)/login/page.tsx | 2 +- frontend/components/form/inputs/password-input.tsx | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/frontend/app/(unauthenticated)/login/page.tsx b/frontend/app/(unauthenticated)/login/page.tsx index b33bed32..3929dc14 100644 --- a/frontend/app/(unauthenticated)/login/page.tsx +++ b/frontend/app/(unauthenticated)/login/page.tsx @@ -95,7 +95,7 @@ function LoginPage() { Your email or password is incorrect. Please try again, or{" "} - + reset your password . diff --git a/frontend/components/form/inputs/password-input.tsx b/frontend/components/form/inputs/password-input.tsx index 2a156f80..f2b3a4f0 100644 --- a/frontend/components/form/inputs/password-input.tsx +++ b/frontend/components/form/inputs/password-input.tsx @@ -47,7 +47,7 @@ const PasswordInput = forwardRef( {showForgetPassword && ( - + Forgot password? )} From 55dea368a844951c4a9a304b4f5a03fa80616665 Mon Sep 17 00:00:00 2001 From: Wang Haoyang Date: Fri, 1 Nov 2024 02:58:30 +0800 Subject: [PATCH 08/10] Improve login backend endpoint -Make login errors slightly more distinguishable by returning distinct error codes in the HTTP response body depending on what issue ocurred while trying to verify the given username and password --- backend/src/auth/dependencies.py | 11 +++++++++-- backend/src/auth/router.py | 10 ++++------ 2 files changed, 13 insertions(+), 8 deletions(-) diff --git a/backend/src/auth/dependencies.py b/backend/src/auth/dependencies.py index 90ad8e91..0455f7ad 100644 --- a/backend/src/auth/dependencies.py +++ b/backend/src/auth/dependencies.py @@ -1,3 +1,4 @@ +from http import HTTPStatus from typing import Annotated, Optional from fastapi.security import OAuth2PasswordBearer from passlib.context import CryptContext @@ -56,9 +57,15 @@ def authenticate_user(email: str, password: str): ) ).first() if not user: - return False + raise HTTPException( + status_code=HTTPStatus.UNAUTHORIZED, + detail={"error": "Invalid username.", "error_id": "1"}, + ) if not verify_password(password, user.hashed_password): - return False + raise HTTPException( + status_code=HTTPStatus.UNAUTHORIZED, + detail={"error": "Incorrect password.", "error_id": "2"}, + ) return user diff --git a/backend/src/auth/router.py b/backend/src/auth/router.py index 196563c7..6481582b 100644 --- a/backend/src/auth/router.py +++ b/backend/src/auth/router.py @@ -77,12 +77,10 @@ def sign_up( def log_in( form_data: Annotated[OAuth2PasswordRequestForm, Depends()], response: Response ) -> Token: - user = authenticate_user(form_data.username, form_data.password) - if not user: - raise HTTPException( - status_code=HTTPStatus.UNAUTHORIZED, - detail="Incorrect username or password.", - ) + try: + user = authenticate_user(form_data.username, form_data.password) + except HTTPException as exc: + raise exc return create_token(user, response) From 5122d376df8bf33cf37efacd13ad92bebe342b4a Mon Sep 17 00:00:00 2001 From: Wang Haoyang Date: Fri, 1 Nov 2024 02:58:55 +0800 Subject: [PATCH 09/10] Format backend with ruff --- backend/src/auth/router.py | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/backend/src/auth/router.py b/backend/src/auth/router.py index 6481582b..77dcb984 100644 --- a/backend/src/auth/router.py +++ b/backend/src/auth/router.py @@ -165,10 +165,15 @@ def request_password_reset( .where(User.account_type == AccountType.NORMAL) ).first() if not user: - return + print( + f"""ERROR: Attempt to reset password for email {email} that doesn't match any existing user""" + ) + raise HTTPException(HTTPStatus.NOT_FOUND) # Mark all existing password reset codes for the given user as used to prevent usage of old codes - session.execute(update(PasswordReset).where(PasswordReset.user_id == user.id).values(used=True)) + session.execute( + update(PasswordReset).where(PasswordReset.user_id == user.id).values(used=True) + ) session.commit() code = str(uuid4()) @@ -189,12 +194,14 @@ def complete_password_reset( select(PasswordReset).where(PasswordReset.code == code) ).first() if not password_reset: - print("ERROR: Attempt to use password reset code that wasn't previously generated by Jippy") + print( + "ERROR: Attempt to use password reset code that wasn't previously generated by Jippy" + ) raise HTTPException(HTTPStatus.NOT_FOUND) - + if password_reset.used: print("ERROR: Attempt to use password reset code that has already been used") - raise(HTTPException(HTTPStatus.CONFLICT)) + raise (HTTPException(HTTPStatus.CONFLICT)) user = session.get(User, password_reset.user_id) user.hashed_password = get_password_hash(data.password) From ff64d2016f4056a5b2eb7c564666cf77e1ecbcf5 Mon Sep 17 00:00:00 2001 From: Wang Haoyang Date: Fri, 1 Nov 2024 02:59:20 +0800 Subject: [PATCH 10/10] Format frontend with eslint --- .../app/(unauthenticated)/register/page.tsx | 5 +++- .../reset-password-complete-form.tsx | 23 ++++++++++++------- 2 files changed, 19 insertions(+), 9 deletions(-) diff --git a/frontend/app/(unauthenticated)/register/page.tsx b/frontend/app/(unauthenticated)/register/page.tsx index ac253ea7..5baf041f 100644 --- a/frontend/app/(unauthenticated)/register/page.tsx +++ b/frontend/app/(unauthenticated)/register/page.tsx @@ -86,7 +86,10 @@ function RegisterPage() { {/* Body */} {isError && ( - +
diff --git a/frontend/app/(unauthenticated)/reset-password/_components/reset-password-complete-form.tsx b/frontend/app/(unauthenticated)/reset-password/_components/reset-password-complete-form.tsx index 1b7c1eb4..6a7e6e3c 100644 --- a/frontend/app/(unauthenticated)/reset-password/_components/reset-password-complete-form.tsx +++ b/frontend/app/(unauthenticated)/reset-password/_components/reset-password-complete-form.tsx @@ -3,6 +3,7 @@ import { useState } from "react"; import { SubmitHandler, useForm } from "react-hook-form"; import { zodResolver } from "@hookform/resolvers/zod"; +import { HttpStatusCode } from "axios"; import { CircleAlert } from "lucide-react"; import { z } from "zod"; @@ -26,7 +27,6 @@ import { FormMessage, } from "@/components/ui/form"; import { Input } from "@/components/ui/input"; -import { HttpStatusCode } from "axios"; const resetPasswordCompleteFormSchema = z .object({ @@ -55,7 +55,9 @@ export default function ResetPasswordCompleteForm({ onComplete: () => void; }) { const [isError, setIsError] = useState(false); - const [errorMsg, setErrorMsg] = useState("Your password needs to match."); + const [errorMsg, setErrorMsg] = useState( + "Your password needs to match.", + ); const form = useForm({ resolver: zodResolver(resetPasswordCompleteFormSchema), @@ -77,9 +79,13 @@ export default function ResetPasswordCompleteForm({ if (response.error) { setIsError(true); if (response.status === HttpStatusCode.Conflict) { - setErrorMsg("Password reset link has expired. Please check your email for the latest link"); + setErrorMsg( + "Password reset link has expired. Please check your email for the latest link", + ); } else if (response.status === HttpStatusCode.NotFound) { - setErrorMsg("Invalid password reset link. Please only click on links sent by us"); + setErrorMsg( + "Invalid password reset link. Please only click on links sent by us", + ); } else { setErrorMsg("Error while resetting your password. Please try again"); } @@ -100,13 +106,14 @@ export default function ResetPasswordCompleteForm({ {isError && ( - +
- - {errorMsg} - + {errorMsg} )}