feat: implement google oauth backend [TASK 4]#26
feat: implement google oauth backend [TASK 4]#26burgerphilic18 wants to merge 6 commits intop-society:mainfrom
Conversation
Signed-off-by: Spandan Hota <spandanhota2005@outlook.com>
Signed-off-by: Spandan Hota <spandanhota2005@outlook.com>
There was a problem hiding this comment.
Summary of Changes
Hello @burgerphilic18, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request significantly enhances the server's authentication capabilities by integrating a custom Google OAuth 2.0 backend. It provides a robust mechanism for user login and registration via Google accounts, issuing JWTs for session management, and directing users to appropriate post-login pages based on their existing status.
Highlights
- Google OAuth Backend Implementation: This PR introduces the core backend logic for Google OAuth 2.0, allowing users to log in with their Google accounts without relying on Google's SDK. It handles the full OAuth flow from initiation to callback.
- New Authentication Endpoints: Two new API endpoints have been added under
/auth:/auth/googleto initiate the OAuth redirect to Google, and/auth/google/callbackto process Google's response, exchange the authorization code for tokens, and retrieve user information. - User Management and JWT Issuance: The authentication flow now checks if a user already exists in the database based on their Google email. For existing users, a JSON Web Token (JWT) is issued, and they are redirected to
/home?token. For new users, a partial user entry is created in the database, a JWT is issued, and they are redirected to/user-details?tokento complete their profile. - Dependency and Configuration Updates: The
jsonwebtokenpackage has been added to enable JWT creation and signing. The server now requires new environment variables (GOOGLE_CLIENT_ID,GOOGLE_CLIENT_SECRET,GOOGLE_REDIRECT_URI,JWT_SECRET) for proper OAuth and JWT functionality.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request introduces a Google OAuth 2.0 backend, which is a significant feature. The implementation is straightforward and follows the OAuth 2.0 flow correctly. However, there are a few critical issues regarding security and correctness that need to be addressed before this can be merged.
Specifically, the handling of environment variables in the callback route is unsafe and could lead to server crashes. There's also a logical flaw in how new user usernames are generated, which will cause unique constraint violations in the database. I've provided detailed comments and suggestions to fix these issues and improve the overall maintainability of the code.
Once these points are addressed, the code will be much more robust and secure.
apps/server/src/routers/auth.ts
Outdated
| const clientId = process.env.GOOGLE_CLIENT_ID as string; | ||
| const clientSecret = process.env.GOOGLE_CLIENT_SECRET as string; | ||
| const redirectURL = process.env.GOOGLE_REDIRECT_URI as string; |
There was a problem hiding this comment.
These type assertions as string for environment variables are unsafe. The /google/callback route does not validate the presence of GOOGLE_CLIENT_ID, GOOGLE_CLIENT_SECRET, and GOOGLE_REDIRECT_URI. If these environment variables are missing, the application will throw an error at runtime when they are used in the fetch call, potentially crashing the server process.
You should validate these environment variables at the beginning of this route handler, similar to how it's done in the /google route, to ensure the server is configured correctly and fails gracefully. A 500 status code would be more appropriate for a server misconfiguration.
const clientId = process.env.GOOGLE_CLIENT_ID;
const clientSecret = process.env.GOOGLE_CLIENT_SECRET;
const redirectURL = process.env.GOOGLE_REDIRECT_URI;
if (!clientId || !clientSecret || !redirectURL) {
fastify.log.error("❌ Missing Google OAuth environment variables");
return reply.status(500).send({
error: "Server misconfiguration: missing Google OAuth keys",
});
}There was a problem hiding this comment.
added the "as string" to pass the lint checks. will fix it in next commit
apps/server/src/routers/auth.ts
Outdated
| if (user) { | ||
| const token = jwt.sign({ userId: user.id }, jwtSecret, { | ||
| expiresIn: "1h", | ||
| }); | ||
| fastify.log.info("🔑 Existing user JWT:", token); | ||
| return reply.redirect(`/home?token=${token}`); | ||
| } | ||
|
|
||
| const newUser = await DrizzleClient.insert(users) | ||
| .values({ | ||
| id: crypto.randomUUID(), | ||
| username: name, | ||
| email, | ||
| firstName: "", | ||
| lastName: "", | ||
| pronouns: "", | ||
| bio: "", | ||
| branch: "", | ||
| passingOutYear: "", | ||
| totalPosts: 0, | ||
| }) | ||
| .returning(); | ||
|
|
||
| const token = jwt.sign({ userId: newUser[0].id }, jwtSecret, { | ||
| expiresIn: "1h", | ||
| }); | ||
| fastify.log.info("🆕 New user created, JWT:", token); | ||
|
|
||
| return reply.redirect(`/user-details?token=${token}`); |
There was a problem hiding this comment.
This block has duplicated logic and two potential bugs:
- Unsafe array access:
newUser[0].idcould throw an error if the insert operation doesn't return a user. - Non-unique username: The
usernamecolumn has a unique constraint, but a user's Googlenameis not guaranteed to be unique. This will cause inserts to fail for users with common names.
You can refactor this to be more DRY (Don't Repeat Yourself) and robust by:
- Determining the
userIdandredirectPathfirst, then signing the token and redirecting once. - Setting the initial
usernametonulland letting the user choose a unique one on the/user-detailspage. - Safely handling the new user creation result.
- Using
.returning({ id: users.id })for efficiency.
let userId: string;
let redirectPath: string;
if (user) {
userId = user.id;
redirectPath = "/home";
fastify.log.info("✅ Existing user found");
} else {
const newUserResult = await DrizzleClient.insert(users)
.values({
id: crypto.randomUUID(),
username: null, // Let user set this on the details page
email,
firstName: "",
lastName: "",
pronouns: "",
bio: "",
branch: "",
passingOutYear: "",
totalPosts: 0,
})
.returning({ id: users.id });
if (!newUserResult?.[0]?.id) {
fastify.log.error("❌ Failed to create new user or retrieve new user's ID", { newUser: newUserResult });
return reply.status(500).send({ error: "Failed to create user" });
}
userId = newUserResult[0].id;
redirectPath = "/user-details";
fastify.log.info("🆕 New user created");
}
const token = jwt.sign({ userId }, jwtSecret, {
expiresIn: "1h",
});
fastify.log.info(`🔑 JWT created. Redirecting to ${redirectPath}`);
return reply.redirect(`${redirectPath}?token=${token}`);
apps/server/src/routers/auth.ts
Outdated
| const oauthURL = | ||
| `https://accounts.google.com/o/oauth2/v2/auth` + | ||
| `?client_id=${clientId}` + | ||
| `&redirect_uri=${encodeURIComponent(redirectURL)}` + | ||
| `&response_type=code` + | ||
| `&scope=${encodeURIComponent(scope)}` + | ||
| `&access_type=offline` + | ||
| `&prompt=consent`; |
There was a problem hiding this comment.
Manually constructing URLs with string concatenation can be error-prone and less readable. Using the URL and URLSearchParams APIs is a safer and more idiomatic way to build query strings, as it handles URL encoding automatically.
| const oauthURL = | |
| `https://accounts.google.com/o/oauth2/v2/auth` + | |
| `?client_id=${clientId}` + | |
| `&redirect_uri=${encodeURIComponent(redirectURL)}` + | |
| `&response_type=code` + | |
| `&scope=${encodeURIComponent(scope)}` + | |
| `&access_type=offline` + | |
| `&prompt=consent`; | |
| const oauthURL = new URL("https://accounts.google.com/o/oauth2/v2/auth"); | |
| oauthURL.searchParams.append("client_id", clientId); | |
| oauthURL.searchParams.append("redirect_uri", redirectURL); | |
| oauthURL.searchParams.append("response_type", "code"); | |
| oauthURL.searchParams.append("scope", scope); | |
| oauthURL.searchParams.append("access_type", "offline"); | |
| oauthURL.searchParams.append("prompt", "consent"); |
|
@burgerphilic18 which llm do you use ? |
only used github copilot to pass the lint checks |
|
why did you removed the .env.example ? |
renamed that file to .env |
dayumnnnnnnnnn bro |
should have created another separate .env file ig? 😅 |
its okayy it okayy lemme review i'll get back to you |
Signed-off-by: Spandan Hota <spandanhota2005@outlook.com>
Signed-off-by: Spandan Hota <spandanhota2005@outlook.com>
|
@burgerphilic18, |
Signed-off-by: Spandan Hota <spandanhota2005@outlook.com>
neoandmatrix
left a comment
There was a problem hiding this comment.
This is from initial review. Need to review more.
| export const DrizzleClient = drizzle(process.env.DATABASE_URL || "", { | ||
| schema: { users, topics, threads, posts }, | ||
| }); |
There was a problem hiding this comment.
Why this? I don't think it needed.
There was a problem hiding this comment.
i imported all schema.ts files at beginning then didn't remove it later
There was a problem hiding this comment.
Sooooo, what do you think should it not be reverted back or fixed??
| fastify.log.info(`🔑 JWT created. Redirecting to ${redirectPath}`); | ||
| return reply.redirect(`${redirectPath}?token=${token}`); | ||
| } catch (err) { | ||
| fastify.log.error("🔥 Error during Google OAuth:", err); |
There was a problem hiding this comment.
i put random emojis in console.log msgs to identify easily then changed it to fastify.log and didn't remove emoji 😅
neoandmatrix
left a comment
There was a problem hiding this comment.
@ujsquared @iamanishx PTAL.
| reply.redirect(oauthURL.toString()); | ||
| }); | ||
|
|
||
| fastify.get("/google/callback", async (request, reply) => { |
There was a problem hiding this comment.
Is this correct? Some query parameters are missing i believe.
| const clientSecret = process.env.GOOGLE_CLIENT_SECRET; | ||
| const redirectURL = process.env.GOOGLE_REDIRECT_URI; | ||
|
|
||
| if (!clientId || !clientSecret || !redirectURL) { |
There was a problem hiding this comment.
Why check again? Check above no?
| return reply.send({ error: "Failed to fetch user info from Google" }); | ||
| } | ||
|
|
||
| fastify.log.info("👤 Google user info:", userInfo); |
There was a problem hiding this comment.
Why log user info? Not needed i believe.
| } else { | ||
| const newUserResult = await DrizzleClient.insert(users) | ||
| .values({ | ||
| id: crypto.randomUUID(), |
There was a problem hiding this comment.
No need, db already has been configured for this.
There was a problem hiding this comment.
No need, db already has been configured for this.
And even if being sent we can get name fields from Google account only.
| firstName: "", | ||
| lastName: "", | ||
| pronouns: "", | ||
| bio: "", | ||
| branch: "", | ||
| passingOutYear: "", |
There was a problem hiding this comment.
I don't believe we need to send empty strings. Db will handle this.
| fastify.log.info("🆕 New user created"); | ||
| } | ||
| const token = jwt.sign({ userId }, jwtSecret, { | ||
| expiresIn: "1h", |
There was a problem hiding this comment.
We aren't using any refresh token strategy for now. So 1hr is too less
|
Also we need to limit the domain to |
@fastify/rate-limit |
|
@burgerphilic18 use google-auth-library for verification |
|
@burgerphilic18 any updates? |
|
closing due to inactivity. |
Description
This PR adds backend for Google OAuth 2.0 login without using google's sdk. Users will login with their Google accounts. If a user already exists in the database, a JWT is issued, and they are redirected to
/home?token. If the user is new, a half filled user entry is created in the database, and they are redirected to/user-details?tokento complete their profile.Changes Made
GET /auth/googleendpoint to initiate OAuth flow.GET /auth/google/callbackendpoint to handle Google’s redirect and fetch user info.jsonwebtokenfor JWT creation and signing.GOOGLE_CLIENT_IDGOOGLE_CLIENT_SECRETGOOGLE_REDIRECT_URIJWT_SECRETIssue
Solves #25
Additional Note
haven't implemented the optional enhancements yet.