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

Add resetPasswordRedirectTo prop to Auth component #223

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

ozanmakes
Copy link

What kind of change does this PR introduce?

feature

What is the current behavior?

Previously, it would behave as if the users just logged in.

In my app PASSWORD_RECOVERY event doesn't fire after following the password reset link, at least locally. I want to make sure users end up in the right page to set their new password.

You can find a relevant discussion in this issue: #77 (comment)

What is the new behavior?

This prop allows you to redirect users to a page where they can set their new password.

This prop allows you to redirect users to a page where they can set
their new password. Previously, it would behave as if the users just
logged in.
@silentworks
Copy link
Contributor

This isn't addressing any issue as you've just added a new prop that does exactly the same as the existing redirectTo prop, it's even making use of the redirectTo prop internally. I have outlined in the issue you mentioned about how these components work #77 (comment).

@ozanmakes
Copy link
Author

ozanmakes commented Sep 25, 2023

Hi @silentworks, thanks for responding to my PR. I do use the view prop but I'm having trouble implementing a good flow for reset password. The problem I have is, <Auth /> component only takes one redirectTo param and doesn't have a callback to update this when the view changes.

  1. The user goes to my /register or /login page, and I render either <Auth view="sign_in" redirectTo="www.example.com/dashboard" /> or <Auth view="sign_up" redirectTo="www.example.com/dashboard" />. redirectTo is set to www.example.com/dashboard to redirect the user to the dashboard after logging in.
  2. On this page, user can follow the internal navigation links of the Auth component and press Forgot your password link.
  3. On the forgot password page, there they enter their email. This will send an email for them to reset their password and use the redirectTo="www.example.com/dashboard" link.
  4. After they follow the link in their email, they will be sent to www.example.com/dashboard instead of "set new password" page which is very confusing.

If I understand this correctly, some Supabase documentation suggest listening to PASSWORD_RECOVERY event so that the /dashboard page the user gets redirected to then can send the user to the "set new password" page. But in my app this event doesn't seem to be getting fired.

And besides that, I would like the users to end up in the "set new password" page right away after following the link in the email they receive.

This PR adds a new prop that only is used for ForgottenPassword view. You mentioned that this new prop "does exactly the same as the existing redirectTo prop". Could you please describe how to use redirectTo prop the same way?

Please let me know if my understanding or implementation of this flow is wrong, and how to implement this correctly. If there's a public repo implementing a traditional reset password flow please let me know.

@levity
Copy link

levity commented Sep 26, 2023

This isn't addressing any issue as you've just added a new prop that does exactly the same as the existing redirectTo prop, it's even making use of the redirectTo prop internally. I have outlined in the issue you mentioned about how these components work #77 (comment).

I'm not sure why you're saying this isn't addressing any issue. Before this change, those Auth components can't be configured so that password recovery works correctly; after this change, they can.

Even if those components are intended to be reference implementations, and people should not be using them directly as you state in #77, it would be nice if they were fully functional. Or perhaps they should be removed entirely? 🤷

@silentworks
Copy link
Contributor

@osener if you take a look at the repo in the issue I linked it has the password reset flow implemented in it. Also take a look at your code again, all you did was pass an alias to the redirectTo. https://github.com/supabase/auth-ui/pull/223/files#diff-112c28ac025a8ffe2a93903f96d912b0f922cbf226dc9637dc32323939cec8d4R177 As I've stated you should be using these as separate view's rather than using the mega <Auth /> component.

@silentworks
Copy link
Contributor

@levity the components are fully functional, documentation might be what's missing the most. I linked to a repo in my comment on issue #77 which shows how you should be handling the password reset flow.

@ozanmakes
Copy link
Author

ozanmakes commented Sep 27, 2023

I think I understand why your example works and my code doesn't. It looks like the key is the custom email template with the query param &next=/account/update-password:

<a href="{{ .SiteURL }}/auth/confirm?token_hash={{ .TokenHash }}&type=recovery&next=/account/update-password">Reset Password</a>

What I've been trying to do is to pass a different parameter, resetPasswordRedirectTo, so that when the user uses the internal links of the Auth component to go to Forgotten Password view, they end up with the right redirectTo parameter.

What you've shown is that to implement this flow correctly you have to do two things:

  • Disregard the Auth component, use underlying components with showLinks={false} and render all the links yourself.
  • Use custom email templates to redirect user to Update Password page instead of redirectTo parameter.

Neither the uselessness of the Auth component nor the requirement to supply custom email templates is at all obvious reading Supabase documentation, so I hope you understand the confusion. And shipping a broken Auth component that has all this internal links with no possibility to implement a working password reset flow is quite misleading as it does allow user to reset their password.

@levity
Copy link

levity commented Sep 27, 2023

@osener if you take a look at the repo in the issue I linked it has the password reset flow implemented in it. Also take a look at your code again, all you did was pass an alias to the redirectTo. https://github.com/supabase/auth-ui/pull/223/files#diff-112c28ac025a8ffe2a93903f96d912b0f922cbf226dc9637dc32323939cec8d4R177 As I've stated you should be using these as separate view's rather than using the mega <Auth /> component.

The point we're trying to make is that the "mega Auth component" isn't fully functional. In the absence of other documentation, it is the documentation. And it's confusing to people (see #77 again for evidence of this) that it appears to Just Work -- it has a functional link that switches to the Forgot Password view -- but you can't actually configure that view correctly through the mega Auth component.

So it just makes sense that the "mega Auth component" should either be removed or improved. This PR seems like a good step in that direction to me. After all, since you're saying we should be using separate views, then this "mega Auth component" is just example code. What's the downside of adding this change? Since it's just example code, there's no need to preserve "API cleanliness" or anything like that.

The only other alternative I see is to add documentation that says, effectively, "Don't be misled by this mega Auth component! It doesn't fully work and you shouldn't use it." That seems... less straightforward... than just making it work.

@levity
Copy link

levity commented Sep 27, 2023

Anyway, thanks for all your patience and consideration while reading this feedback @silentworks 🙏

@@ -173,7 +174,7 @@ function Auth({
appearance={appearance}
supabaseClient={supabaseClient}
setAuthView={setAuthView}
redirectTo={redirectTo}
redirectTo={resetPasswordRedirectTo ?? redirectTo}
Copy link

@alukach alukach Sep 28, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be designed to specify the redirect URL based on the view state of the Auth component?

redirectTo={
  view === 'forgotten_password' 
    ? resetPasswordRedirectTo ?? redirectTo 
    : redirectTo
}

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I now see that this is already under the case VIEWS.FORGOTTEN_PASSWORD: condition, thus there's no need to check view... Disregard.

@alukach
Copy link

alukach commented Sep 28, 2023

If I understand this correctly, some Supabase documentation suggest listening to PASSWORD_RECOVERY event so that the /dashboard page the user gets redirected to then can send the user to the "set new password" page. But in my app this event doesn't seem to be getting fired.

This fits in with my experience as well. The default email has a link sending me to https://{REDACTED}.supabase.co/auth/v1/verify?token=pkce_{REDACTED}&type=recovery&redirect_to=http://localhost:3000/auth/callback. When my browser navigates to that URL, I receive a 303 response with a Location header of http://localhost:3000/auth/callback?code={REDACTED}. At this point, it seems that just the SIGNED_IN and INITIAL_SESSION events fire.

I'm fine with the reset password flow being pretty much akin to a magic link flow, but where I send the user to a reset form as soon as they're signed-in. However, I don't seem to have any contextual information available when handling the /auth/callback route. Allowing a user to provide a separate resetPasswordRedirectTo is a fine idea.

Alternatively, I could imagine a world where supabase.auth.resetPasswordForEmail() accepted a queryParams to specify custom flags to ease their ability to parse callback requests without setting up separate routes. Of course, doing this would also mean that the Auth component in this codebase would need to provide a standard queryparam for password resets and then inform users that they need to prepare to handle that case in their callback route. This sounds like a bit more work on Supabase's side.

@hf
Copy link

hf commented Jan 31, 2024

Reopening so that we can review this when available.

@hf hf reopened this Jan 31, 2024
@logicminds
Copy link

I am also experiencing this issue. Can we just pass the type parameter that is sent to supabase back to the client so we can parse this out in /auth/callback and decide what to do?

@J0
Copy link
Contributor

J0 commented Feb 7, 2024

Hey team,

An update on this thread - could y'all try updating the Supabase version like in this PR and see you can listen on the event? There was a fix while back which helps emit the PASSWORD_RECOVERY event so perhaps that might help as well. We will try on our end as well when a slot frees up.

As an additional aside we'll likely have someone from the frontend team look into Next.js 13+ forgot password flow.

Thanks everyone for your patience with us.

@ozanmakes
Copy link
Author

Hi @J0, thanks for revisiting this issue.

I made sure I am on latest versions, gotrue-js@2.62.2 and supabase-js@2.39.3, but I am still not seeing PASSWORD_RECOVERY event get fired when I follow the password reset link from the email in Inbucket.

image

I tried to follow the code in the linked PR and added some logs to GoTrueClient. In my case the changed function, verifyOtp, doesn't seem to be called. I instead get the SIGNED_IN event from this line with redirectType === null.

In case you also don't have the reset password flow working, I hope the info above will help you debug the issue. If you do get it working and the problem is on my side somehow, I'd appreciate some example that shows how it is supposed to work.

Thanks!

@J0
Copy link
Contributor

J0 commented Feb 13, 2024

@ozanmakes Thanks for trying it out, we'll get back to you shortly

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.

None yet

7 participants