-
Notifications
You must be signed in to change notification settings - Fork 0
feat(auth): password change and reset #226
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,10 @@ | ||
| import { ChangePasswordForm } from "@/features/password-change"; | ||
|
|
||
| export default function ChangePasswordPage() { | ||
| return ( | ||
| <div className="container mx-auto flex h-full flex-col items-center justify-center p-4 sm:p-8"> | ||
| <h1 className="mb-4 text-2xl font-semibold">Tutaj zmienisz hasło</h1> | ||
| <ChangePasswordForm /> | ||
| </div> | ||
| ); | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -30,7 +30,13 @@ export async function handleResponse<T>( | |
| }; | ||
| const code = errorReport?.error.code ?? String(response.status); | ||
| const errorMessage = `Request failed with code ${code}: ${message}`; | ||
| logger.error( | ||
|
|
||
| const isValidationError = | ||
| code === "E_VALIDATION_ERROR" || | ||
| Array.isArray(errorReport?.error.validationIssues); | ||
| const logFunction = isValidationError ? logger.warn : logger.error; | ||
|
|
||
| logFunction( | ||
|
Comment on lines
+34
to
+39
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. błędy walidacji powinny dalej być jako błędy. jeśli chcesz zrobić wyjątek dla niepoprawnego hasła to możesz to złapać osobno, wtedy nie dawaj warna tylko po prostu nic, ale musi to być bardziej precyzyjne żeby łapać tylko ten konkretny case |
||
| { | ||
| url: request.url, | ||
| method: request.method, | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,22 @@ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import { fetchMutation } from "@/features/backend"; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import type { MessageResponse } from "@/features/backend/types"; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * Calls POST /api/v1/auth/change_password | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * Body: { oldPassword, newPassword, newPasswordConfirm } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. niekonwencjonalny sposób opisania parametrów. albo to usuń albo użyj JSDoc |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * Requires authentication; fetchMutation should attach tokens automatically | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * Requires authentication; fetchMutation should attach tokens automatically | |
| * | |
| * Authentication: | |
| * - Requires an authenticated user access token. | |
| * - The token is attached automatically by `fetchMutation` using the standard | |
| * project auth mechanism (for example, an Authorization header or HTTP-only | |
| * auth cookies). | |
| * - If the request is unauthenticated or the token is invalid/expired, the | |
| * backend is expected to return an authentication error (such as HTTP 401/403), | |
| * which `fetchMutation` will surface to the caller. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
zbędna informacja która może ulec rozbieżności. usunąć
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tutaj wskazane byłoby ręczne skonstruowanie tego obiektu, aby nie zostały wysłane nadmiarowe argumenty. daj za przykład takie wywołanie:
changePassword({ oldPassword: "...", newPassword: "...", newPasswordConfirm: "...", otherSensitiveField: "..." });W obecnej implementacji przez sieć również przejdzie pole i wartość otherSensitiveField. osobiście uważam to za lekki błąd, bo może doprowadzić do podatności.
sugerowane polepszenie:
| export async function changePassword(body: { | |
| oldPassword: string; | |
| newPassword: string; | |
| newPasswordConfirm: string; | |
| }) { | |
| const response = await fetchMutation<MessageResponse>( | |
| "auth/change_password", | |
| { | |
| method: "POST", | |
| body, | |
| }, | |
| ); | |
| return response; | |
| } | |
| export async function changePassword({ | |
| oldPassword, | |
| newPassword, | |
| newPasswordConfirm, | |
| }: { | |
| oldPassword: string; | |
| newPassword: string; | |
| newPasswordConfirm: string; | |
| }) { | |
| const response = await fetchMutation<MessageResponse>( | |
| "auth/change_password", | |
| { | |
| method: "POST", | |
| body: { | |
| oldPassword, | |
| newPassword, | |
| newPasswordConfirm, | |
| }, | |
| }, | |
| ); | |
| return response; | |
| } |
poza tym ogólnie utworzenie tej funkcji dla deduplikacji wywołań do api jest jak najbardziej na plus
| Original file line number | Diff line number | Diff line change | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,120 @@ | ||||||||||||||||
| "use client"; | ||||||||||||||||
|
|
||||||||||||||||
| import { zodResolver } from "@hookform/resolvers/zod"; | ||||||||||||||||
| import { useMutation } from "@tanstack/react-query"; | ||||||||||||||||
| import { useForm } from "react-hook-form"; | ||||||||||||||||
| import { toast } from "sonner"; | ||||||||||||||||
|
|
||||||||||||||||
| import { PasswordInput } from "@/components/inputs/password-input"; | ||||||||||||||||
| import { Button } from "@/components/ui/button"; | ||||||||||||||||
| import { Form, FormField } from "@/components/ui/form"; | ||||||||||||||||
| import { FetchError } from "@/features/backend"; | ||||||||||||||||
|
|
||||||||||||||||
| import { changePassword } from "../api/change-password"; | ||||||||||||||||
| import { ChangePasswordSchema } from "../schemas/change-password-schema"; | ||||||||||||||||
| import type { ChangePasswordFormValues } from "../schemas/change-password-schema"; | ||||||||||||||||
|
|
||||||||||||||||
| export function ChangePasswordForm() { | ||||||||||||||||
| const form = useForm<ChangePasswordFormValues>({ | ||||||||||||||||
| resolver: zodResolver(ChangePasswordSchema), | ||||||||||||||||
| defaultValues: { | ||||||||||||||||
| oldPassword: "", | ||||||||||||||||
| newPassword: "", | ||||||||||||||||
| newPasswordConfirm: "", | ||||||||||||||||
| }, | ||||||||||||||||
| }); | ||||||||||||||||
|
|
||||||||||||||||
| const { mutate, isPending } = useMutation({ | ||||||||||||||||
| mutationFn: async (data: ChangePasswordFormValues) => | ||||||||||||||||
| changePassword({ | ||||||||||||||||
| oldPassword: data.oldPassword, | ||||||||||||||||
| newPassword: data.newPassword, | ||||||||||||||||
| newPasswordConfirm: data.newPasswordConfirm, | ||||||||||||||||
| }), | ||||||||||||||||
|
Comment on lines
+28
to
+33
|
||||||||||||||||
| mutationFn: async (data: ChangePasswordFormValues) => | |
| changePassword({ | |
| oldPassword: data.oldPassword, | |
| newPassword: data.newPassword, | |
| newPasswordConfirm: data.newPasswordConfirm, | |
| }), | |
| mutationFn: changePassword, |
Copilot
AI
Jan 12, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error handling logic has complex nested conditions and type assertions that could be simplified. The repeated use of 'as Record<string, unknown>' and optional chaining makes the code harder to maintain. Consider:
- Defining a proper TypeScript interface for validation issues
- Creating a helper function to extract field-specific errors
- This would improve type safety and make the error handling logic more maintainable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tu jest zdecydowanie za dużo logiki, zwłaszcza że aplikacja w innych miejscach ma już error handling. koncept jest fajny ale raczej zrezygnujemy z tego ponieważ od backendu otrzymujemy błędy po angielsku, a interfejs mamy po polsku. wystarczy wywalić modala z odpowiednim tekstem, który powinien zostać zdefiniowany w get-toast-messages.ts. powinien być odrębny przypadek dla błędnego hasła i generyczny dla innych błędów, żeby użytkownik wiedział czy coś się popsuło czy źle wpisał.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
czemu to tu jest? jeśli nie ma konkretnego powodu to nie zalecam ustawiania tego, bo wtedy byle błąd sieciowy uniemożliwi zmianę hasła
Copilot
AI
Jan 12, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error handling approach is inconsistent with the existing pattern used in LoginForm. The LoginForm uses toast.promise with getToastMessages for cleaner error handling, while this implementation manually handles errors in onSuccess/onError callbacks. Consider refactoring to use the toast.promise pattern for consistency:
- Add a changePassword entry to getToastMessages in lib/get-toast-messages.ts
- Replace the mutate/onSuccess/onError pattern with toast.promise(mutateAsync(data), getToastMessages.changePassword)
This would simplify the error handling code and maintain consistency across the codebase.
Copilot
AI
Mar 25, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The password fields don’t set appropriate autocomplete hints (e.g. current-password for old password and new-password for the new/confirm fields). Without this, browser/password-manager autofill can behave poorly and users may accidentally overwrite stored credentials. Consider extending PasswordInput to accept input props (or an autoComplete prop) and pass the correct values from this form.
Copilot
AI
Jan 12, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The ChangePasswordForm component lacks test coverage. The similar LoginForm component in the authentication feature has comprehensive tests (login-page.test.tsx). Consider adding tests to verify:
- Form rendering with all three password fields
- Client-side validation (password length, password matching, old vs new password difference)
- Successful password change flow with form reset
- Server-side validation error handling (especially oldPassword errors)
- Generic error handling paths
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| export * from "./components/change-password-form"; | ||
| export * from "./api/change-password"; | ||
| export * from "./schemas/change-password-schema"; | ||
|
Comment on lines
+2
to
+3
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. dlaczego to jest eksportowane poza feature? jedynym kodem, który jest konsumowany poza tym featurem jest formularz do zmiany
Comment on lines
+1
to
+3
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,22 @@ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import { z } from "zod"; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import { RequiredStringSchema } from "@/schemas"; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| export const ChangePasswordSchema = z | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| .object({ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| oldPassword: RequiredStringSchema, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| newPassword: RequiredStringSchema.min(8, { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| message: "Hasło musi mieć co najmniej 8 znaków", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }), | |
| }) | |
| .regex(/[A-Z]/, { | |
| message: "Hasło musi zawierać przynajmniej jedną wielką literę", | |
| }) | |
| .regex(/[a-z]/, { | |
| message: "Hasło musi zawierać przynajmniej jedną małą literę", | |
| }) | |
| .regex(/[0-9]/, { | |
| message: "Hasło musi zawierać przynajmniej jedną cyfrę", | |
| }) | |
| .regex(/[^A-Za-z0-9]/, { | |
| message: "Hasło musi zawierać przynajmniej jeden znak specjalny", | |
| }), |
Copilot
AI
Jan 12, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The hardcoded error messages in Polish should be moved to a centralized location for consistency. The authentication feature uses FORM_ERROR_MESSAGES from data/form-error-messages.ts for validation messages. Consider:
- Adding password-specific messages to FORM_ERROR_MESSAGES or creating a similar constants file
- This makes translation management easier and ensures consistency across the application
| export const ChangePasswordSchema = z | |
| .object({ | |
| oldPassword: RequiredStringSchema, | |
| newPassword: RequiredStringSchema.min(8, { | |
| message: "Hasło musi mieć co najmniej 8 znaków", | |
| }), | |
| newPasswordConfirm: RequiredStringSchema, | |
| }) | |
| .refine((data) => data.newPassword === data.newPasswordConfirm, { | |
| message: "Hasła muszą być identyczne", | |
| path: ["newPasswordConfirm"], | |
| }) | |
| .refine((data) => data.oldPassword !== data.newPassword, { | |
| message: "Nowe hasło musi się różnić od starego", | |
| export const CHANGE_PASSWORD_ERROR_MESSAGES = { | |
| minLength: "Hasło musi mieć co najmniej 8 znaków", | |
| passwordsMustMatch: "Hasła muszą być identyczne", | |
| passwordMustDiffer: "Nowe hasło musi się różnić od starego", | |
| } as const; | |
| export const ChangePasswordSchema = z | |
| .object({ | |
| oldPassword: RequiredStringSchema, | |
| newPassword: RequiredStringSchema.min(8, { | |
| message: CHANGE_PASSWORD_ERROR_MESSAGES.minLength, | |
| }), | |
| newPasswordConfirm: RequiredStringSchema, | |
| }) | |
| .refine((data) => data.newPassword === data.newPasswordConfirm, { | |
| message: CHANGE_PASSWORD_ERROR_MESSAGES.passwordsMustMatch, | |
| path: ["newPasswordConfirm"], | |
| }) | |
| .refine((data) => data.oldPassword !== data.newPassword, { | |
| message: CHANGE_PASSWORD_ERROR_MESSAGES.passwordMustDiffer, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-> "Zmiana hasła"