-
Notifications
You must be signed in to change notification settings - Fork 3
Add account creation to Vortex #912
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
base: staging
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for vortexfi ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for vortex-sandbox ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
… AUTH_SUCCESS handling
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.
Pull request overview
Copilot reviewed 67 out of 68 changed files in this pull request and generated 11 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
apps/frontend/src/components/widget-steps/AuthEmailStep/index.tsx
Outdated
Show resolved
Hide resolved
apps/api/src/database/migrations/022-add-user-id-to-entities.ts
Outdated
Show resolved
Hide resolved
|
@copilot open a new pull request to apply changes based on the comments in this thread |
… limits, auth handling Co-authored-by: ebma <6690623+ebma@users.noreply.github.com>
…ent infinite loop Co-authored-by: ebma <6690623+ebma@users.noreply.github.com>
…ility and user experience
Address code review comments: security, performance, and bug fixes
| }); | ||
| } | ||
|
|
||
| await SupabaseAuthService.sendOTP(email); |
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.
I believe this operation is stateless on the backend, right? Maybe we could add an entry to the users table, to mark the last OTP timestamp or any other metadata.
In case we loose the state in the UI or we want to keep track.
| // Sync user to local database (upsert) | ||
| await User.upsert({ | ||
| email: email, | ||
| id: result.user_id |
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.
Is the upsert only for handling new users? I assume the userId should never change but I may be wrong.
gianfra-t
left a comment
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.
Very nice and complete feature, thanks @ebma ! The only thing I would like to discuss is what you think about changing the token into the cookies and avoid storing it in the local storage.
I understand there is a tradeoff in terms of security between both methods.
| }), | ||
| ({ event, context }) => { | ||
| // Store tokens in localStorage for session persistence | ||
| AuthService.storeTokens({ |
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.
I also wanted to discuss the pros/cons of using localStorage vs cookies (httpOnly) for the tokens.
I believe our application already handles httpOnly cookiens for the SIWE login, so we have an example, which seems to be the preferred way for browser apps.
Do you see this solution better in terms of security, for our case?
| if (!taxIdRecord) { | ||
| // Validate that user is authenticated since userId is required in the schema | ||
| if (!req.userId) { | ||
| res.status(httpStatus.UNAUTHORIZED).json({ |
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.
Can we use the required validator here? instead of the optional, and then throwing.
| apiClient.interceptors.request.use( | ||
| config => { | ||
| // Add any common headers here | ||
| // Add Authorization header if user is authenticated |
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.
Do you know, if the header refreshes after the user gets authenticated?
What I mean is, if the apiClient initially starts with an authenticated state, do the headers get added after the Auth state changes?
| }, | ||
| EnterOTP: { | ||
| on: { | ||
| CHANGE_EMAIL: { |
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.
What do we want to allow changing the email at this stage? I assume Supabase already created the user.
|
|
||
| if (error) { | ||
| // If error is "PGRST116" (no rows returned), user doesn't exist | ||
| if (error.code === "PGRST116") { |
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.
I don't know if it's possible, but this code seems confusing for an empty query return. Is this what the sdk recommends?
| @@ -0,0 +1,110 @@ | |||
| import {DataTypes, Model, Optional} from "sequelize"; | |||
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.
We are we adding this new table?
| type: DataTypes.UUID | ||
| }); | ||
|
|
||
| await queryInterface.sequelize.query(` |
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.
Can we keep the userId relation for this one (and any that allows for null) empty instead of using the dummy? Or do you think there is a benefit of using the dummy user here?
Closes #882.
This pull request introduces Supabase-based authentication to the API, including OTP (one-time password) flows, token verification, and user session management. It adds new authentication endpoints, middleware for extracting and verifying user identity from requests, and propagates the authenticated
userIdthroughout the business logic and data models. Several routes now optionally or mandatorily require authentication, and the system is prepared for future endpoint protection. Additionally, the codebase is updated to store and utilize theuserIdin key models and service layers.Supabase Authentication Integration
.env.exampleand included@supabase/supabase-jsas a dependency inpackage.json. [1] [2]SupabaseAuthServicewith methods for checking user existence, sending and verifying OTPs, verifying and refreshing tokens, and retrieving user profiles. [1] [2]AuthControllerwith endpoints for checking email registration, requesting OTP, verifying OTP, refreshing tokens, and verifying tokens. [1] [2]/v1/authroutes for authentication endpoints and registered them in the main API router. [1] [2] [3]Authentication Middleware and Route Protection
requireAuthandoptionalAuthmiddleware to extract and verify user identity from Bearer tokens, attachinguserIdto the request.optionalAuthmiddleware, allowing endpoints to accessuserIdif provided. [1] [2] [3] [4] [5] [6] [7]User Identity Propagation
brla.controller.ts,quote.controller.ts,ramp.controller.ts) to storeuserIdfrom the request in database records and service calls. [1] [2] [3] [4] [5] [6]userIdas part of the request context and data models. [1] [2]These changes collectively enable secure, user-specific flows across the API, laying the groundwork for robust authentication and authorization using Supabase.