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

feat(web): GitHub Login #151

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

aryasaatvik
Copy link
Contributor

@aryasaatvik aryasaatvik commented Jul 23, 2024

feat(web): GitHub Login

Overview

This pull request introduces GitHub login functionality to the web application, enhancing the authentication options available to users. It also refactors the existing Google login implementation to improve code organization and maintainability.

Changes

  • Key Changes:

    • Added a new GitHubLogin component for GitHub authentication.
    • Added a new GoogleLogin component for Google authentication.
    • Updated the Signin page to utilize the new login components instead of inline forms.
    • Integrated GitHub provider in the authentication server configuration.
  • New Features:

    • Users can now log in using their GitHub accounts via the newly created GitHubLogin component.
    • The GoogleLogin component allows users to log in with their Google accounts, maintaining existing functionality.
  • Refactoring:

    • The inline form for Google login in the Signin page has been replaced with the GoogleLogin component, promoting reusability and separation of concerns.
    • The signIn function calls have been encapsulated within their respective components, improving readability and maintainability of the codebase.
    • The auth.ts file has been updated to include the GitHub authentication provider, ensuring that the backend supports the new login method.

✨ Generated with love by Kaizen ❤️

Original Description None

@aryasaatvik
Copy link
Contributor Author

TODO: error handling when a user with the same email has previously logged in using a different provider

@Dhravya
Copy link
Collaborator

Dhravya commented Jul 23, 2024

Great, love it

this would certainly break parts of the app though. for eg. the ensureAuth function that is compatible with the mobile app (coming soon) login

let sessionData = await db
.select()
.from(sessions)
.innerJoin(users, eq(users.id, sessions.userId))
.where(eq(sessions.sessionToken, token!));
const isMobile =
token.split("?") && token.split("?")[1] === `source="mobile"`;
if (isMobile) {
// remove everything after ? in token
const newToken = token.split("?").slice(0, -1).join("?");
console.log(token, newToken);
const authUserFetch = await fetch(
`https://www.googleapis.com/oauth2/v1/tokeninfo?access_token=${newToken}`,
);
if (!authUserFetch.ok) {
console.error(
"Error fetching Google user,",
authUserFetch.statusText,
await authUserFetch.text(),
);
console.log("Google user not found or error.");
return undefined;
}
const authUserData = (await authUserFetch.json()) as {
email: string;
audience: string;
issued_to: string;
};
console.log(authUserData);

@aryasaatvik
Copy link
Contributor Author

Great, love it

this would certainly break parts of the app though. for eg. the ensureAuth function that is compatible with the mobile app (coming soon) login

let sessionData = await db
.select()
.from(sessions)
.innerJoin(users, eq(users.id, sessions.userId))
.where(eq(sessions.sessionToken, token!));
const isMobile =
token.split("?") && token.split("?")[1] === `source="mobile"`;
if (isMobile) {
// remove everything after ? in token
const newToken = token.split("?").slice(0, -1).join("?");
console.log(token, newToken);
const authUserFetch = await fetch(
`https://www.googleapis.com/oauth2/v1/tokeninfo?access_token=${newToken}`,
);
if (!authUserFetch.ok) {
console.error(
"Error fetching Google user,",
authUserFetch.statusText,
await authUserFetch.text(),
);
console.log("Google user not found or error.");
return undefined;
}
const authUserData = (await authUserFetch.json()) as {
email: string;
audience: string;
issued_to: string;
};
console.log(authUserData);

@Dhravya I can add the same logic for github api. But the logic for creating a user on mobile https://github.com/supermemoryai/supermemory/blob/main/apps/web/app/api/ensureAuth.ts#L80 doesn't seem correct. It is missing adding a row account table. Can I take a look at the mobile app? Is it in RN?

@Dhravya
Copy link
Collaborator

Dhravya commented Jul 24, 2024

eh, yeah that logic is probably broken right now
it works but i haven't put much thought into it

yes it is RN
code will be out soon next week

@aryasaatvik aryasaatvik marked this pull request as draft July 24, 2024 19:20
Copy link
Contributor

kaizen-bot bot commented Jul 24, 2024

Code Review

All Clear: This PR is ready to merge! 👍

✨ Generated with love by Kaizen ❤️


Useful Commands
  • Feedback: Reply with !feedback [your message]

  • Review: Reply with !review

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.

2 participants