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

F24/sayi/invite-user-endpoint #50

Merged
merged 10 commits into from
Nov 28, 2024
Merged

F24/sayi/invite-user-endpoint #50

merged 10 commits into from
Nov 28, 2024

Conversation

sthuray
Copy link
Contributor

@sthuray sthuray commented Nov 19, 2024

Notion ticket link

Invite User Endpoint + Email Link

Implementation description

  • Implemented generateSIgnInLink in authService
  • Implemented sendInviteEmail in authService (uses generateSignInLink)
  • Implemented /auth/invite-user endpoint in authRoutes.ts (sends invite email to newly created user with INVITED status)
  • Installed firebase app in frontend and initialized it in firebase.js (using new frontend/.env secrets)
  • Login process: checkIfSignInLink (Login.tsx) -> loginWithSignInLink (AuthAPIClient.ts) -> /auth/loginWithSignInLink (authRoutes.ts)
    • (Login.tsx) checkIfSignInLink calls authAPIClient.loginWithSignInLink if a user is not already logged in and firebase has confirmed that the url is a sign in link.
    • (AuthAPIClient.ts) loginWithSignInLink makes the call to the firebase app to signInWithEmailLink and then posts accessToken and refreshToken to /auth/loginWithSignInLink (note that this is how the regular login process works)
    • (AuthRoutes.ts) /auth/loginWithSignInLink sets refreshToken as cookie and returns data in the expected structure for login
    • (AuthAPIClient.ts) if the post to /auth/loginWithSignInLink is successful, stores data in localStorage as a json object and returns the data.
    • (Login.tsx) if the previous steps are successful, the authenticated_user is set successfully (login is completed)

Steps to test

  1. Might have to Install yarn in your frontend directory manually with "yarn add firebase" (https://classic.yarnpkg.com/en/package/firebase) if docker-compose up --build isn't working
  2. Create user by posting to /user/ endpoint
User created within firebase
  1. Test /auth/invite-user errors
    a. with an email not in firebase
Invalid email passed to :auth:invite-user b. with an email that doesn't have UserStatus.INVITED non-invited user email is passed to :auth:invite-user
  1. Send invite email to newly created user with /auth/invite-user (successful)
Valid email passed to :auth:invite-user Email sent to user
  1. Test login process by logging into an alternate account and then clicking sign-in link
    (Login with sign-in process isn't executed and original user stays logged in. Note that the sign-in link isn't invalidated, it will work if you log out and then click it).
    (Tested but no image, check that original user's authenticated_user remains in local storage).

  2. After logging out, click the sign-in link (successful)

Successful login with sign-in link Email is verified after successful login
  1. After successfully signing in with sign-in link, logout and click again (test error)
Sign-in link clicked again Sign-in link fails, no user is set
  1. Send a new invite link to the user with INVITED status but wait for 6 hours before clicking it. Link should have expired (not tested, but it should have same output as 6).

What should reviewers focus on?

  • Is the login process implemented well (are there places where it passes too much/too little information, or are the functions too tightly coupled).
  • Note: User is not updated to ACTIVE status yet (should it be part of this process)
  • Side note for future: in step 2a, it returns a 500 error code when it should return a 404 NotFound, so the getUserByEmail function in userService isn't functioning as expected

Checklist

  • My PR name is descriptive and in imperative tense
  • My commit messages are descriptive and in imperative tense. My commits are atomic and trivial commits are squashed or fixup'd into non-trivial commits
  • I have run the appropriate linter(s)
  • I have requested a review from the PL, as well as other devs who have background knowledge on this PR or who will be building on top of this PR

@trinity-y
Copy link
Contributor

trinity-y commented Nov 21, 2024

Note: User is not updated to ACTIVE status yet (should it be part of this process)

this part looks good, the user is updated to active once they have successfully reset the password (because in case they follow the link and then don't reset the password, they wouldn't be able to get another email)

Copy link
Contributor

@jerry-cheng5 jerry-cheng5 left a comment

Choose a reason for hiding this comment

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

Really great work! This ticket wasn't easy, but you managed to figure it out.

Just a few minor fixes, and we should be able to merge before end of term!

backend/typescript/rest/authRoutes.ts Outdated Show resolved Hide resolved
backend/typescript/rest/authRoutes.ts Show resolved Hide resolved
frontend/src/APIClients/AuthAPIClient.ts Show resolved Hide resolved
frontend/src/components/auth/Login.tsx Show resolved Hide resolved
Copy link
Contributor

@jerry-cheng5 jerry-cheng5 left a comment

Choose a reason for hiding this comment

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

changes look good to me

@sthuray sthuray added this pull request to the merge queue Nov 28, 2024
Merged via the queue into main with commit 9bbdc2e Nov 28, 2024
1 check passed
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.

3 participants