Skip to content

Conversation

@that-one-arab
Copy link
Contributor

@that-one-arab that-one-arab commented Jan 3, 2025

Fixes issue

This pull request includes multiple changes to the authentication system, focusing on verifying Google tokens, enhancing error handling, and improving the user authentication flow. The most important changes include adding a new method for verifying Google tokens, updating the Google authentication service, and modifying the frontend authentication checks.

Backend Changes:

  • Google Token Verification:

    • Added verifyGToken method in auth.controller.ts to check if a user's Google access token is still valid.
    • Updated auth.routes.config.ts to include a new route for verifying Google tokens. [1] [2]
    • Introduced Result_VerifyGToken interface in auth.types.ts to handle the response from Google token verification.
  • Google Authentication Service Updates:

    • Added getGAuthClientForUser method in google.auth.service.ts to obtain Google authentication clients for users. [1] [2]
    • Implemented getAccessToken method in GoogleAuthService to fetch access tokens and handle errors.
    • Introduced NoGAuthAccessToken error in error.constants.ts for handling missing Google access tokens.

Frontend Changes:

  • Authentication Flow Enhancements:

    • Refactored ProtectedRoute to use useAuthCheck for verifying both user and Google sessions.
    • Created useAuthCheck hook to manage authentication checks and session validation.
    • Updated Login.tsx to handle authentication state and navigate accordingly. [1] [2] [3] [4] [5]
  • API Updates:

    • Added validateGoogleAccessToken method in auth.api.ts to validate Google tokens via the backend. [1] [2]

These changes collectively improve the robustness of the authentication system by ensuring that Google tokens are valid and handling authentication states more effectively on the frontend.


How to Test

In lieu of of integration tests, here are the steps taken during manual QA to validate functionality.

Clear cache & cookies or do the following tests in a incognito browser

Auth Check
This is done in ProtectedRoute, which is triggered on page loads. Validate this by doing a full-page refresh, loading a new tab, or going to a different website in your current tab and then going back to Compass. You'll notice GET requests to /api/auth/google, which indicates we're checking the access token validity.

Note: ensure these checks only occurs on page refreshes. We don't want to run those excessively.

Search Params
This PR adds a URL search param when an redirecting to login after a session expired (either user or google access token), which is intended to help the UI display more information to the user about why they need to re-login.

Instead of waiting until your google access token expires, you can simulate that scenario by:

  1. Running backend and web in dev
  2. Signing in like normal
  3. Update getAccessToken to always throw (comment out the if(!token) part.
  4. Wait until the backend finishes hot reloading
  5. Reload the Compass page or open a new tab and go to Compass (this will trigger the auth check from ProtectedRoute)
  6. Notice: you'll be redirected to /login and the URL will include a param called: reason=gauth-session-expired

Login redirect
Auto-redirecting to login is a nice UX. However, it opened up a bug where a user who is already logged in and then goes to /login gets caught in an infinite loop of redirects and API calls. To address that, we're allowing the user to see the /login page regardless of auth status. If a user is already authenticated and then clicks 'Sign in with Google', however, they will simply be redirected to the root page rather than going through the auth flow again.

You can test this scenario by logging in, going to /login, and clicking that button, and then looking at the Network tab and backend logs. You'll notice that no auth API requests are being made, which is expected.

@that-one-arab that-one-arab marked this pull request as ready for review January 3, 2025 05:16
@tyler-dane
Copy link
Contributor

tyler-dane commented Jan 3, 2025

No big concerns with this at first glance. I want to QA this one more thoroughly to make sure, though.

I'm going to hold off on doing that while I wrap up #203 and #196.

In the meantime, please find an issue from the 'Ready' column of the board to work on. Alternatively, you could find an issue in the Backlog to refine. By "refine", I mean thinking it through more, adding details, and updating the issue with a high-level description of your suggested implementation. After doing that I'll read through it and give it the 👍 if it looks good to start working on.

Since you're more familiar with the codebase and have been producing good work, this could be a good time to pick up a more difficult or larger issue if you're up for it.

@that-one-arab
Copy link
Contributor Author

@tyler-dane Thank you! I think I'll opt to work on issues based on the priorities you assigned them since we have a lot of them as ready. Once we start running out of ready issues I'll work on refining existing backlog items

that-one-arab and others added 3 commits January 8, 2025 07:25
Implement a new api route to validate a user's google oauth session
also renames controller method from verifyGAuthSession to verifyGToken to more accurately represent what's happening. Compass doesn't maintain a persistent session with Google like it does with it's user's session (via Supertokens). Instead, it just passes the access token in requests
@tyler-dane tyler-dane force-pushed the bug_trigger_login_after_gauth_expire branch from 4876592 to 5c77f5e Compare January 8, 2025 14:14
making it a class doesn't add much value, as we're not taking advantage of any class benefits (inheritance, class methods, encapsulation)
- Rename `valid` to `isValid` to make it more consistent with codebase
- Always set value to either `true` or `false` for consistency
@that-one-arab that-one-arab force-pushed the bug_trigger_login_after_gauth_expire branch from f900b76 to 0f6b893 Compare January 20, 2025 05:47
that-one-arab and others added 3 commits January 20, 2025 08:48
This same type is returned as part of type inference. Explicitly setting the function return type is acceptable is it is somehow different than the inferred type. In this case, the extra code doesn't provide any value and just requires more maintenance.
This'll make it easier to test, access across other files, and rename it when things change
keeping it inside `gauth.util.ts` until I figure out where to put it
purpose of using await statements is to avoid creating traps for future devs when working on this part of the code.
Users don't care what a session is or that it's the Google connection that's requiring us to make them reauthenticate
@that-one-arab
Copy link
Contributor Author

@tyler-dane Done a round of refactors, responded to some comments that need your input and/or clarifications. Please let me know what you think

@tyler-dane
Copy link
Contributor

tyler-dane commented Jan 22, 2025

@tyler-dane Done a round of refactors, responded to some comments that need your input and/or clarifications. Please let me know what you think

Looks good, just did a few refactors. There's currently a bug where a user gets into an infinite loop if they're on the login page and then tries to go to the root route before finishing the auth flow. This might be unrelated to the changes in this PR, though.

After a little more QA I will either fix that as part of this PR or just merge this and fix it in a separate bug.

401 is now a valid response to the /api/auth/google endpoint, which is used to validate a google token
this is needed to distinguish between boolean values (when we know something happened) and the default value of null (when nothing happened). Without this, an infinite loop of routing and auth checks resulted
…ady authenticated

This is a compromise between UX and functionality that lets us provide smooth routing while also ensuring that the user doesn't re-authenticated unnecessarily. It does so by checking if the user is already authenticated as a side effect during the Login page load. If so, when the user clicks the Login button they will be redirected. If not, the user will go through the regular login flow and be redirected afterwards
@tyler-dane
Copy link
Contributor

Finished with cleanup, QA, and updating PR notes above.

I tweaked a few of your initial changes for practical reasons:

I removed the toast. This was working in isolation. But due to our duplicate renders, multiple toasts were appearing. I decided to simply remove them instead of increasing the scope of this PR so that we can focus on higher priority issues.

I used axios and our custom Auth API instead of fetch to reduce the confusion that'll come with having multiple requests implementations

@tyler-dane tyler-dane merged commit 387a09f into SwitchbackTech:main Jan 23, 2025
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.

Pre-emptively trigger login after google token expired

2 participants