-
-
Notifications
You must be signed in to change notification settings - Fork 115
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: authentication with Google and GitHub #49
Conversation
Signed-off-by: Abhishek kushwaha <abhishekkushwaha1479@gmail.com>
Signed-off-by: Abhishek kushwaha <abhishekkushwaha1479@gmail.com>
Signed-off-by: Abhishek kushwaha <abhishekkushwaha1479@gmail.com>
Signed-off-by: Abhishek kushwaha <abhishekkushwaha1479@gmail.com>
Signed-off-by: Abhishek kushwaha <abhishekkushwaha1479@gmail.com>
PR Description updated to latest commit (a15e58d) |
PR Analysis
PR Feedback💡 General suggestions: The PR is well-structured and introduces significant enhancements to the application. However, it would be beneficial to include tests to verify the new functionality, especially the authentication and webhook handling. Additionally, it would be helpful to handle potential errors in the webhook handler to ensure the application's robustness. 🤖 Code feedback:
✨ Usage tips:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey man, thats some great work. Although, I would like to flag off some stuff. See, the user management and stuff like those are already built into the backend under apps/api
. Naturally, we would like all of these computations to happen in the backend. This is so because in the longer run, we might want to give the users the opportunity to lon in via the cli
sure i will try to migrate it to apps/api . However i never code with nestjs . need sometime to figure out stuffs |
We are always there for assisting. Also, most of the code regarding authentication and issuing JWTs are already done. All you need to do is integrate github and google and we are good to go |
Clerk issues Jwt token of their own , we can tweak body of it to some extend. |
@Abbhiishek that's not gonna work then :/ We need to move to manual authentication. I am linking some articles for your reference.
|
@rajdip-b i am not getting the point why clerk is a bad choice here. Open to feedback
|
Hmm, okie. Well see the bottomline is this:
If these can be done, I am totally fine. I just dont want to overcomplicate the flow thats all |
We can do this. There is a one concern, lets say we suddenly get a high rush of users (10k+) and clerk starts charging us. there would be a chance we would not like to pay that as its costly, right? Now if we are planning to Oauth only for github and google, i feel it would be better that we handle our authentication manually in nestJs itself. Now why this
|
@Abbhiishek I'm sort of in agreement with @HarshPatel5940 in this matter. I mean, paying extra just to make something that takes 10 20 LOC is not ideal. You might want to give a look at passportjs. Alternatively, you can also collaborate with Harsh to build this feature! |
One of the potential alternatives can be using lucia, https://lucia-auth.com/ which can be easily integrated with nestJs api |
@Abbhiishek @HarshPatel5940 hey folks! Made any progress in this one? |
@Abbhiishek @HarshPatel5940 closing this one since @HarshPatel5940 will be creating a new PR. |
Type
enhancement
Description
This PR introduces several enhancements to the application:
PR changes walkthrough
7 files
page.tsx
apps/web/app/(auth)/login/[[...sign-in]]/page.tsx
Added a new SignIn component from Clerk to handle user
authentication.
page.tsx
apps/web/app/(auth)/signup/[[...sign-up]]/page.tsx
Added a new SignUp component from Clerk to handle user
registration.
Providers.tsx
apps/web/app/Providers.tsx
Introduced a new ClerkProvider component to manage user
authentication state.
route.ts
apps/web/app/api/webhook/user/route.ts
Implemented a new webhook handler to manage user creation
and updates.
layout.tsx
apps/web/app/layout.tsx
Integrated the Providers component into the main application
layout.
prismadb.ts
apps/web/lib/prismadb.ts
Added Prisma client for database operations.
schema.prisma
apps/web/prisma/schema.prisma
Introduced a new Prisma schema for structured data
management.
2 files
.env.example
.env.example
Added new environment variables for Clerk and webhook
authentication.
tsconfig.json
apps/web/tsconfig.json
Added new paths configuration for TypeScript.
1 files
environment-variables.md
docs/contributing-to-keyshade/environment-variables.md
Updated the documentation to include new environment
variables.
1 files
package.json
package.json
Added new dependencies for Clerk, themes, and svix.
User description
Description
This PR introduces several new features and updates to enhance our application's authentication and data management capabilities.
Features
Authentication Support for Google and GitHub: Users can now authenticate using their Google or GitHub accounts. This provides a seamless and secure way for users to access our application.
Webhook for User Account Updates: A new webhook has been implemented that triggers whenever a user creates or updates their account. This allows us to keep our user data up-to-date and consistent across our application.
Prisma Schema Addition to Web App Module: The Prisma schema has been added to the web app module. This will facilitate efficient and structured data management in our application.
Fixes #3 #4
ENV Updates
New Environment Variables: Three new secrets have been introduced - CLERK_SECRET_KEY, NEXT_PUBLIC_CLERK_PUBLISHABLE_KEY, and CLERK_WEBHOOK_SIGNING_KEY. These will be used for secure communication and data transfer.
Dependencies
svix
: For managing and handling webhooks (verify webhook payload)@clerk/nextjs
: For integrating Clerk authentication in Next.js applications.@clerk/themes
: For theming support in Clerk.Future Improvements
Mentions
@rajdip-b
Screenshots of relevant screens
Login Page
Sign Up page
Developer's checklist
If changes are made in the code:
Documentation Update