-
-
Notifications
You must be signed in to change notification settings - Fork 414
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: supabase auth #4112
base: master
Are you sure you want to change the base?
feat: supabase auth #4112
Conversation
fix: build;
docs: add cypress docs;
fix: question slug unique constraint must consider tenant_id;
updated routes to use return url;
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.
Niiiiice. Will run it all locally now.
<Link | ||
to={ | ||
session?.id | ||
? '/questions/create' | ||
: '/sign-in?returnUrl=' + encodeURIComponent(location.pathname) | ||
} | ||
> |
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.
This change isn't needed, the user only gets this component if they're logged in.
onClick={() => | ||
props.isLoggedIn ? handleUsefulClick() : navigate('/sign-in') | ||
props.isLoggedIn | ||
? handleUsefulClick() | ||
: navigate( | ||
'/sign-in?returnUrl=' + encodeURIComponent(location.pathname), | ||
) |
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.
Realising this should use the UserAction. I can do that quickly if you like?
<a | ||
href="/sign-in" | ||
href={'/sign-in?returnUrl=' + encodeURIComponent(location.pathname)} | ||
style={{ | ||
textDecoration: 'underline', | ||
color: 'inherit', |
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.
Didn't realise there was some raw anchors hanging around still! Shock and horror!
I'm thinking a little util component wrapper around Link
would be helpful. Wouldn't then need to add + encodeURIComponent(location.pathname)
everywhere then.
I can make it if you like?
if (!result.ok) { | ||
const data = await result.json() | ||
|
||
if (data.error) { | ||
setSubmitResults({ type: 'error', message: data.error }) | ||
} else { | ||
setSubmitResults({ | ||
type: 'error', | ||
message: 'Oops, something went wrong!', | ||
}) | ||
} | ||
|
||
return | ||
} |
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.
Shouldn't this flow get caught by the try/catch block and if not, shouldn't the route logic be edited so it does?
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've tried that, it doesn't, even though the request returns 400.
<Form | ||
onSubmit={() => {}} | ||
validate={async (values: any) => { | ||
try { | ||
await validationSchema.validate(values, { abortEarly: false }) | ||
} catch (err) { | ||
return err.inner.reduce( | ||
(acc: any, error) => ({ |
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.
Planned component move?
const signInResult = await client.auth.signInWithPassword({ | ||
email: user!.email as string, | ||
password, | ||
}) | ||
|
||
if (signInResult.error) { | ||
return Response.json( | ||
{ error: 'Invalid password' }, | ||
{ headers, status: 400 }, | ||
) | ||
} | ||
|
||
const result = await client.auth.updateUser({ email: newEmail }) | ||
|
||
if (result.error) { |
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.
Try/catch for all this?
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.
supabase api doesn't throw exceptions
const signInResult = await client.auth.signInWithPassword({ | ||
email: user!.email as string, | ||
password: oldPassword, | ||
}) | ||
|
||
if (signInResult.error) { | ||
return Response.json( | ||
{ error: 'Invalid password' }, |
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.
Try/catch for all this?
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.
supabase api doesn't throw exceptions
src/utils/domain.server.ts
Outdated
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.
Flagging the empty file. :)
PR Checklist
PR Type
What kind of change does this PR introduce?
What is the new behavior?
Authentication is done via supabase.
We now have a SessionContext which has the authenticated user info. Or null if unauthenticated.
The userStore is kept in sync based on the SessionContext throught
UserStoreWrapper
.Profiles are still stored in firebase.
A user profile is created both on firebase and supabase on user sign up.
Removed firebase functions "onCreate" and "onDelete" that synced profiles with supabase. Still kept "onUpdate".
BEFORE MERGE: User Migration.
Additional checks for firebase login.
Does this PR introduce a DB Schema Change or Migration?
Git Issues
Closes #4060