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

🐛 Fix: Pre-emptively trigger login after google token expired #212

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 16 additions & 7 deletions packages/backend/src/auth/auth.routes.config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,26 +5,35 @@ import { CommonRoutesConfig } from "@backend/common/common.routes.config";
import authController from "./controllers/auth.controller";
import authMiddleware from "./middleware/auth.middleware";

/**
* Routes with the verifyIsDev middleware are
* only available when running the app in dev,
* as they are not called by production code.
*/
export class AuthRoutes extends CommonRoutesConfig {
constructor(app: express.Application) {
super(app, "AuthRoutes");
}

configureRoutes(): express.Application {
/**
* Convenience routes for debugging (eg via Postman)
*
* Production code shouldn't call these
* directly, which is why they're limited to devs only
* Checks whether user's google access token is still valid
*/
this.app.route(`/api/auth/google`).get([
verifySession(),
//@ts-expect-error res.promise is not returning response types correctly
authController.verifyGToken,
]);

this.app
.route(`/api/auth/session`)
.all(authMiddleware.verifyIsDev)
//@ts-ignore
//@ts-expect-error res.promise is not returning response types correctly
// eslint-disable-next-line @typescript-eslint/no-misused-promises
.post(authController.createSession)
.get([
verifySession(),
//@ts-ignore
//@ts-expect-error res.promise is not returning response types correctly
authController.getUserIdFromSession,
]);

Expand All @@ -38,7 +47,7 @@ export class AuthRoutes extends CommonRoutesConfig {
*/
this.app.route(`/api/oauth/google`).post([
authMiddleware.verifyGoogleOauthCode,
//@ts-ignore
//@ts-expect-error res.promise is not returning response types correctly
authController.loginOrSignup,
]);

Expand Down
32 changes: 31 additions & 1 deletion packages/backend/src/auth/controllers/auth.controller.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,11 +20,14 @@ import {
findCompassUserBy,
updateGoogleRefreshToken,
} from "@backend/user/queries/user.queries";
import GoogleAuthService from "@backend/auth/services/google.auth.service";
import GoogleAuthService, {
getGAuthClientForUser,
} from "@backend/auth/services/google.auth.service";
import userService from "@backend/user/services/user.service";
import compassAuthService from "@backend/auth/services/compass.auth.service";
import syncService from "@backend/sync/services/sync.service";
import { isInvalidGoogleToken } from "@backend/common/services/gcal/gcal.utils";
import { BaseError } from "@core/errors/errors.base";

import { initGoogleClient } from "../services/auth.utils";

Expand Down Expand Up @@ -63,6 +66,33 @@ class AuthController {
res.promise({ userId });
};

verifyGToken = async (req: SessionRequest, res: Res_Promise) => {
try {
const userId = req.session?.getUserId();

if (!userId) {
res.promise({ valid: false, error: "No session found" });
return;
}

const gAuthClient = await getGAuthClientForUser({ _id: userId });

// Upon receiving an access token, we know the session is valid
const accessToken = await gAuthClient.getAccessToken();

res.promise({ valid: true });
} 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,
});
}
}
Comment on lines +84 to +93
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Why couldn't that pattern be established?

Copy link
Contributor

Choose a reason for hiding this comment

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

  1. change valid to isValid for consistency with other booleans in the app.

  2. Don't return null. Keep isValid as a boolean that always either returns true or false

};

loginOrSignup = async (req: SReqBody<{ code: string }>, res: Res_Promise) => {
try {
const { code } = req.body;
Expand Down
64 changes: 57 additions & 7 deletions packages/backend/src/auth/services/google.auth.service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,17 +3,56 @@ import { OAuth2Client, TokenPayload } from "google-auth-library";
import { Logger } from "@core/logger/winston.logger";
import { Status } from "@core/errors/status.codes";
import { BaseError } from "@core/errors/errors.base";
import { gCalendar } from "@core/types/gcal";
import { UserInfo_Google } from "@core/types/auth.types";
import { ENV } from "@backend/common/constants/env.constants";
import { findCompassUserBy } from "@backend/user/queries/user.queries";
import { UserError } from "@backend/common/constants/error.constants";
import { error } from "@backend/common/errors/handlers/error.handler";
import { Schema_User } from "@core/types/user.types";
import { WithId } from "mongodb";

import compassAuthService from "./compass.auth.service";

const logger = Logger("app:google.auth.service");

export const getGcalClient = async (userId: string) => {
export const getGAuthClientForUser = async (
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

user: WithId<Schema_User> | { _id: string }
) => {
const gAuthClient = new GoogleAuthService();

let gRefreshToken: string | undefined;

if ("google" in user && user.google) {
gRefreshToken = user.google.gRefreshToken;
}

if (!gRefreshToken) {
const userId = "_id" in user ? (user._id as string) : undefined;

if (!userId) {
logger.error(`Expected to either get a user or a userId.`);
throw error(UserError.InvalidValue, "User not found");
}

const _user = await findCompassUserBy("_id", userId);

if (!_user) {
logger.error(`Couldn't find user with this id: ${userId}`);
throw error(UserError.UserNotFound, "User not found");
}

gRefreshToken = _user.google.gRefreshToken;
}

gAuthClient.oauthClient.setCredentials({
refresh_token: gRefreshToken,
});

return gAuthClient;
};

export const getGcalClient = async (userId: string): Promise<gCalendar> => {
const user = await findCompassUserBy("_id", userId);
if (!user) {
logger.error(`Couldn't find user with this id: ${userId}`);
Expand All @@ -24,11 +63,7 @@ export const getGcalClient = async (userId: string) => {
);
}

const gAuthClient = new GoogleAuthService();

gAuthClient.oauthClient.setCredentials({
refresh_token: user.google.gRefreshToken,
});
const gAuthClient = await getGAuthClientForUser(user);

const calendar = google.calendar({
version: "v3",
Expand All @@ -49,7 +84,7 @@ class GoogleAuthService {
);
}

getGcalClient() {
getGcalClient(): gCalendar {
const gcal = google.calendar({
version: "v3",
auth: this.oauthClient,
Expand Down Expand Up @@ -82,6 +117,21 @@ class GoogleAuthService {
const payload = ticket.getPayload() as TokenPayload;
return payload;
}

async getAccessToken() {
const { token } = await this.oauthClient.getAccessToken();

if (!token) {
throw new BaseError(
"No access token",
"oauth client is missing access token. Probably needs a new refresh token to obtain a new access token",
Status.BAD_REQUEST,
false
);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

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


return token;
}
}

export default GoogleAuthService;
19 changes: 15 additions & 4 deletions packages/web/src/auth/ProtectedRoute.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -3,21 +3,32 @@ import Session from "supertokens-auth-react/recipe/session";
import { useNavigate } from "react-router-dom";
import { ROOT_ROUTES } from "@web/common/constants/routes";
import { AbsoluteOverflowLoader } from "@web/components/AbsoluteOverflowLoader";
import { validateGoogleAccessToken } from "@web/auth/gauth.util";

export const ProtectedRoute = ({ children }: { children: ReactNode }) => {
const navigate = useNavigate();

const [isAuthenticated, setIsAuthenticated] = useState<boolean | null>(null);

useLayoutEffect(() => {
async function fetchSession() {
const isAuthenticated = await Session.doesSessionExist();
async function ensureAuthentication() {
const isSessionValid = await Session.doesSessionExist();
const isGAccessTokenValid = await validateGoogleAccessToken();

const isAuthenticated = isSessionValid && isGAccessTokenValid;
setIsAuthenticated(isAuthenticated);
if (!isAuthenticated) {
navigate(ROOT_ROUTES.LOGIN);
const dueToGAuth = !!isSessionValid && !isGAccessTokenValid;

if (dueToGAuth) {
navigate(`${ROOT_ROUTES.LOGIN}?reason=gauth-session-expired`);
Copy link
Contributor

Choose a reason for hiding this comment

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

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

} else {
navigate(ROOT_ROUTES.LOGIN);
}
}
}

void fetchSession();
void ensureAuthentication();
}, [navigate]);

if (isAuthenticated === null) {
Expand Down
19 changes: 19 additions & 0 deletions packages/web/src/auth/gauth.util.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
import { ENV_WEB } from "@web/common/constants/env.constants";

export const validateGoogleAccessToken = async () => {
try {
const res = await fetch(`${ENV_WEB.API_BASEURL}/auth/google`, {
method: "GET",
credentials: "include",
});

if (!res.ok) return false;

const body = (await res.json()) as { valid: boolean };
Copy link
Contributor

Choose a reason for hiding this comment

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

use the shared response type when casting here


return !!body.valid;
} catch (error) {
console.error(error);
return false;
}
};
39 changes: 31 additions & 8 deletions packages/web/src/views/Login/Login.tsx
Original file line number Diff line number Diff line change
@@ -1,13 +1,16 @@
import { v4 as uuidv4 } from "uuid";
import React, { useEffect, useRef, useState } from "react";
import { Navigate } from "react-router-dom";
import { Navigate, useSearchParams } from "react-router-dom";
import GoogleButton from "react-google-button";
import Session from "supertokens-auth-react/recipe/session";
import Session, { signOut } from "supertokens-auth-react/recipe/session";
import { validateGoogleAccessToken } from "@web/auth/gauth.util";
import { useGoogleLogin } from "@react-oauth/google";
import { AlignItems, FlexDirections } from "@web/components/Flex/styled";
import { AuthApi } from "@web/common/apis/auth.api";
import { ROOT_ROUTES } from "@web/common/constants/routes";
import { AbsoluteOverflowLoader } from "@web/components/AbsoluteOverflowLoader";
import { toast } from "react-toastify";
import { SyncApi } from "@web/common/apis/sync.api";

import {
SignInButtonWrapper,
Expand All @@ -20,7 +23,18 @@ import {
StyledLoginContainer,
} from "./styled";

const clearSession = async () => {
try {
await SyncApi.stopWatches();
await signOut();
} catch (error) {
console.error("Failed to clear session", error);
}
};
tyler-dane marked this conversation as resolved.
Show resolved Hide resolved

export const LoginView = () => {
const [searchParams] = useSearchParams();

const [isAuthenticated, setIsAuthenticated] = useState(false);
const [isAuthenticating, setIsAuthenticating] = useState(false);

Expand All @@ -29,14 +43,23 @@ export const LoginView = () => {
useEffect(() => {
const checkSession = async () => {
const isAlreadyAuthed = await Session.doesSessionExist();
setIsAuthenticated(isAlreadyAuthed);
const isGAuthSessionValid = await validateGoogleAccessToken();
setIsAuthenticated(isAlreadyAuthed && isGAuthSessionValid);
};

checkSession().catch((e) => {
alert(e);
console.log(e);
});
}, []);
const reason = searchParams.get("reason");

if (reason === "gauth-session-expired") {
toast.error("Google session expired, please login again");
clearSession();
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. 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?

  2. 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"

} else {
checkSession().catch((e) => {
alert(e);
console.log(e);
clearSession();
});
}
}, [searchParams]);
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we not need to await clearSession?


const SCOPES_REQUIRED = [
"email",
Expand Down