Skip to content
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

[FEAT] move authentication to the server #6

Merged
merged 35 commits into from
May 31, 2024
Merged

[FEAT] move authentication to the server #6

merged 35 commits into from
May 31, 2024

Conversation

zhaojzn
Copy link
Collaborator

@zhaojzn zhaojzn commented Apr 11, 2024

frontend auth branch

@jlui17 jlui17 self-requested a review April 13, 2024 19:58
@@ -6,6 +6,46 @@ interface TimesheetWidgetProps {
timesheetData: TimesheetData;
}

//i think u guys have an hours work data but this works too
function calculateHoursWorked(startTime: string, endTime: string, breakDuration: string): string {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's more reliable to get the data from the google sheet. We have some special rules for calculating this, and I don't want there to be any issues with differences between frontend and db. Let's take this out for now and we will work on bringing this later along with a few other cool changes.

}

_, err = cognitoClient.ConfirmSignUp(confirmInput)
if err != nil {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

err can be many different errors, and we want to decipher whether its an actual system error (like the lambda failed) or if it's a user error like a code mismatch. If it's a user error, we want to return 400 status with a useful error message so it can be displayed on the frontend alert.

Please look through the SDK docs and figure out ones we should look for.
See this as an example.

Password: aws.String(resetReq.Password),
}

_, err = svc.ConfirmForgotPassword(ctx, input)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

err can be many different errors, and we want to decipher whether its an actual system error (like the lambda failed) or if it's a user error like a code mismatch. If it's a user error, we want to return 400 status with a useful error message so it can be displayed on the frontend alert.

Please look through the SDK docs and figure out ones we should look for.
See this as an example.

_, err = svc.ForgotPassword(ctx, input)
if err != nil {
fmt.Println("error calling ForgotPassword,", err)
return events.APIGatewayProxyResponse{
Copy link
Owner

@jlui17 jlui17 May 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

err can be many different errors, and we want to decipher whether its an actual system error (like the lambda failed) or if it's a user error like a code mismatch. If it's a user error, we want to return 400 status with a useful error message so it can be displayed on the frontend alert.

In this case, InvalidParameterException is returned when the email is unverified/doesn't exist. Let's return a 400 with a useful message in that case. Do the same for other user error cases.

Please look through the SDK docs and figure out ones we should look for.
See this as an example.

'Content-Type': 'application/json',
},
});
switch (response.status) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After updating the backend to include 400 responses for user errors, please handle the 400 responses to display a useful error message to the user so they know what to do next.

See this as an example:

switch (response.status) {
case 200:
const data = await response.json();
return Promise.resolve(data);
case 401:
const errorText = await response.text();
console.log("ERROR: ", errorText);
if (errorText.includes(ERROR_MESSAGES.EMPLOYEE_NOT_CONFIRMED)) {
return Promise.reject(ERROR_MESSAGES.EMPLOYEE_NOT_CONFIRMED);
} else if (errorText.includes("UserNotFound")) {
return Promise.reject(new Error("You have not registered yet."));
} else if (errorText.includes("NotAuthorized")) {
return Promise.reject(new Error("Incorrect email or password."));
}
return Promise.reject("Unknown Error");
case 500:
return Promise.reject(new Error("Internal server error"));
default:
return Promise.reject(new Error("Unknown error occurred"));
}
};

},
body: JSON.stringify({ email, code: verificationCode, password: newPassword }),
});
switch (response.status) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After updating the backend to include 400 responses for user errors, please handle the 400 responses to display a useful error message to the user so they know what to do next.

See this as an example:

switch (response.status) {
case 200:
const data = await response.json();
return Promise.resolve(data);
case 401:
const errorText = await response.text();
console.log("ERROR: ", errorText);
if (errorText.includes(ERROR_MESSAGES.EMPLOYEE_NOT_CONFIRMED)) {
return Promise.reject(ERROR_MESSAGES.EMPLOYEE_NOT_CONFIRMED);
} else if (errorText.includes("UserNotFound")) {
return Promise.reject(new Error("You have not registered yet."));
} else if (errorText.includes("NotAuthorized")) {
return Promise.reject(new Error("Incorrect email or password."));
}
return Promise.reject("Unknown Error");
case 500:
return Promise.reject(new Error("Internal server error"));
default:
return Promise.reject(new Error("Unknown error occurred"));
}
};

Copy link
Owner

@jlui17 jlui17 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NIT: the error message that's displayed to the client is written both on the backend and frontend. Since the error handling is kinda jank throughout our code, I won't block approval because of it. However, in the future, maybe we should consolidate all the error message to the server and just have the client display the message.

However... LGTM. GOOD WORK!!!

Let's squash merge with a message like [FEAT] move authentication to the server.

@jlui17 jlui17 changed the title Frontend auth [FEAT] move authentication to the server May 28, 2024
Copy link
Owner

@jlui17 jlui17 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jlui17 jlui17 merged commit fb64728 into main May 31, 2024
1 check passed
@jlui17 jlui17 deleted the frontend_auth branch June 12, 2024 05:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants