🐛 Fix: Pre-emptively trigger login after google token expired #212
Conversation
| } catch (error) { | ||
| if (error instanceof BaseError && error.result === "No access token") { | ||
| res.promise({ valid: false, error: "Invalid Google Token" }); | ||
| } else { | ||
| res.promise({ | ||
| valid: null, // We don't know if the session is valid or not, so we return null | ||
| error, | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
I would've liked to handle this error more consistently with how other google related errors are handled but I couldn't establish a pattern in functions like isInvalidGoogleToken
There was a problem hiding this comment.
Why couldn't that pattern be established?
|
|
||
| import compassAuthService from "./compass.auth.service"; | ||
|
|
||
| const logger = Logger("app:google.auth.service"); | ||
|
|
||
| export const getGcalClient = async (userId: string) => { | ||
| export const getGAuthClientForUser = async ( |
There was a problem hiding this comment.
Extracted as its own function for reusability.
Gave the option of providing a mongodb user object or just the userId Providing the user object directly will save a db call for better performance
|
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. |
|
@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 |
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
force-pushed
the
bug_trigger_login_after_gauth_expire
branch
from
4876592 to
5c77f5e
Compare
making it a class doesn't add much value, as we're not taking advantage of any class benefits (inheritance, class methods, encapsulation)
| const dueToGAuth = !!isSessionValid && !isGAuthSessionValid; | ||
|
|
||
| if (dueToGAuth) { | ||
| navigate(`${ROOT_ROUTES.LOGIN}?reason=gauth-session-expired`); |
There was a problem hiding this comment.
Create a constant for the reason instead of just using the string here. This'll make it easier to test, access across other files, and rename it when things change
| } catch (error) { | ||
| if (error instanceof BaseError && error.result === "No access token") { | ||
| res.promise({ valid: false, error: "Invalid Google Token" }); | ||
| } else { | ||
| res.promise({ | ||
| valid: null, // We don't know if the session is valid or not, so we return null | ||
| error, | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
Why couldn't that pattern be established?
|
|
||
| if (!res.ok) return false; | ||
|
|
||
| const body = (await res.json()) as { valid: boolean }; |
There was a problem hiding this comment.
use the shared response type when casting here
| clearSession(); | ||
| }); | ||
| } | ||
| }, [searchParams]); |
There was a problem hiding this comment.
Do we not need to await clearSession?
| Status.BAD_REQUEST, | ||
| false | ||
| ); | ||
| } |
There was a problem hiding this comment.
Rather than throwing a new BaseError, create an error in error.constants for this scenario.
Then throw it using the error() function like so:
throw error(AuthError.YourNewAuthError, "Access token not retrieved");
The description property in the error provides more info about the error.
The second arg in the error() is meant to explain the result of the error not happening, which the client can use to determine next steps.
Try not to include too many implementation details in the result string, like "Probably needs a new refresh token to obtain a new access token." We don't want to send unnecessary info to the client. Instead, you can include that info in a debug log.
This'll make it easier to test, typecheck, and prevent bugs
|
|
||
| if (reason === "gauth-session-expired") { | ||
| toast.error("Google session expired, please login again"); | ||
| clearSession(); |
There was a problem hiding this comment.
-
Will this toast even render for long, if we're immediately going to clear their session and cause a redirect? Do we need to wait for it to render before running
clearSession? Is that better UX than just redirecting them immediately? -
If you want to keep the toast, don't mention "Google session expired." Users don't care what a session is or that it's the Google connection that's requiring us to make them reauthenticate. Instead, say something like "It's been a while, so we're redirecting you back to the login page. This helps us keep your account safe"
| error, | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
-
change
validtoisValidfor consistency with other booleans in the app. -
Don't return
null. KeepisValidas a boolean that always either returns true or false
Fixes issue