From e93bc1e7364f9724b73e6458dbd0db321e0b9c32 Mon Sep 17 00:00:00 2001 From: Wang Haoyang Date: Fri, 1 Nov 2024 01:26:15 +0800 Subject: [PATCH 01/11] Add backend error response code -Throw 404 HTTP status code when backend receives request for password reset but given email doesn't correspond to any existing user --- backend/src/auth/router.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/backend/src/auth/router.py b/backend/src/auth/router.py index 2994137d..0b4a0eb0 100644 --- a/backend/src/auth/router.py +++ b/backend/src/auth/router.py @@ -184,7 +184,8 @@ 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) code = str(uuid4()) password_reset = PasswordReset(user_id=user.id, code=code, used=False) From 3ec6a488d731ae31897762028fd76bc30db37a49 Mon Sep 17 00:00:00 2001 From: Wang Haoyang Date: Fri, 1 Nov 2024 01:35:28 +0800 Subject: [PATCH 02/11] 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 0b4a0eb0..57ee5ed6 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, @@ -194,7 +177,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, @@ -215,6 +198,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 000b5877d76ade64bf416ed5baace3af8114c48c Mon Sep 17 00:00:00 2001 From: Wang Haoyang Date: Fri, 1 Nov 2024 01:39:50 +0800 Subject: [PATCH 03/11] 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 57ee5ed6..075e1d15 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 ( @@ -170,6 +170,10 @@ def request_password_reset( 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.commit() + code = str(uuid4()) password_reset = PasswordReset(user_id=user.id, code=code, used=False) session.add(password_reset) From 43be5f287c42f26b39ea6eecdf8c83fd911d5446 Mon Sep 17 00:00:00 2001 From: Wang Haoyang Date: Fri, 1 Nov 2024 01:47:23 +0800 Subject: [PATCH 04/11] 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 075e1d15..f8e22e95 100644 --- a/backend/src/auth/router.py +++ b/backend/src/auth/router.py @@ -191,8 +191,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 47b34ba7c4139a52681d5d9cf349e40c152bdeef Mon Sep 17 00:00:00 2001 From: Wang Haoyang Date: Fri, 1 Nov 2024 01:49:22 +0800 Subject: [PATCH 05/11] 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 f6f04f8cdc50aae06c3ba6c4ea4330ed7241a31f Mon Sep 17 00:00:00 2001 From: Wang Haoyang Date: Fri, 1 Nov 2024 01:57:01 +0800 Subject: [PATCH 06/11] 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 e87fa0fbf5b6487a72556da77d6897087040e1ea Mon Sep 17 00:00:00 2001 From: Wang Haoyang Date: Fri, 1 Nov 2024 02:04:17 +0800 Subject: [PATCH 07/11] 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 01a92f1f..52885ed3 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 77f13d527c560fae43d0d81a8f3fd37321ee94c2 Mon Sep 17 00:00:00 2001 From: Wang Haoyang Date: Fri, 1 Nov 2024 02:05:31 +0800 Subject: [PATCH 08/11] 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 cd6c191f9b42191aa78746d91093aa9df2ecae6f Mon Sep 17 00:00:00 2001 From: Wang Haoyang Date: Fri, 1 Nov 2024 02:58:30 +0800 Subject: [PATCH 09/11] 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 f8e22e95..3f4756d3 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 0b641bd24140c61b523918e4bbb1b14eace07dc4 Mon Sep 17 00:00:00 2001 From: Wang Haoyang Date: Fri, 1 Nov 2024 02:58:55 +0800 Subject: [PATCH 10/11] Format backend with ruff --- backend/src/auth/router.py | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/backend/src/auth/router.py b/backend/src/auth/router.py index 3f4756d3..77dcb984 100644 --- a/backend/src/auth/router.py +++ b/backend/src/auth/router.py @@ -165,11 +165,15 @@ def request_password_reset( .where(User.account_type == AccountType.NORMAL) ).first() if not user: - print(f"""ERROR: Attempt to reset password for email {email} that doesn't match any existing user""") + 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()) @@ -190,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 2cf748db9d92d91cb599c5dcd58aa3bc3325cfd5 Mon Sep 17 00:00:00 2001 From: Wang Haoyang Date: Fri, 1 Nov 2024 02:59:20 +0800 Subject: [PATCH 11/11] 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 52885ed3..45ef1acc 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} )}