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.

Sign up for GitHub

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

Jump to bottom

fix: PASSWORD_RECOVERY event missing #24

Closed
wants to merge 2 commits into from
Closed

fix: PASSWORD_RECOVERY event missing #24

wants to merge 2 commits into from

Conversation

j4w8n
Copy link
Contributor

@j4w8n j4w8n commented Jul 1, 2024

What kind of change does this PR introduce?

Bug fix

What is the current behavior?

When resetting a user's password with an email template which has a URL defining the type value of recovery and using verifyOtp in dev code to process the token_hash and type, the SSR server client's onAuthStateChange function does not recognize the PASSWORD_RECOVERY event that verifyOtp uses in this case. Therefore the new session does not get saved to storage and the user is not logged in, in order to reset their password.

What is the new behavior?

User is logged in.

Additional context

Fixes #21

@j4w8n
Copy link
Contributor Author

j4w8n commented Jul 3, 2024

I'm not sure what's going on with the check-conventional-commits check.

@J0
Copy link
Contributor

J0 commented Jul 3, 2024

Yeah I'm not sure - any chance you can rebase and push a commit with feat: or fix? Not sure if the merge commit I made is messing with the check

@J0
Copy link
Contributor

J0 commented Jul 3, 2024

Thanks for the PR btw :)

@j4w8n
Copy link
Contributor Author

j4w8n commented Jul 3, 2024

Yeah I'm not sure - any chance you can rebase and push a commit with feat: or fix? Not sure if the merge commit I made is messing with the check

I'm sorry to say that I'm not familiar with that process. After a search, it seems you'd typically run the rebase command when on the branch, not main: git rebase main. But it gives me a merge conflict.

image
image

@j4w8n
Copy link
Contributor Author

j4w8n commented Jul 3, 2024

Perhaps the best option at this point is for me to close this PR and start fresh?

@J0
Copy link
Contributor

J0 commented Jul 3, 2024

@j4w8n if you could do that it would be perfect. Feel free to tag me there - will approve ASAP.

@j4w8n
Copy link
Contributor Author

j4w8n commented Jul 3, 2024

Closing. Will recreate.

@j4w8n j4w8n closed this Jul 3, 2024
@j4w8n j4w8n deleted the fix-storage-change-event branch July 3, 2024 18:14
@j4w8n j4w8n mentioned this pull request Jul 3, 2024
Merged
J0 pushed a commit that referenced this pull request Jul 3, 2024
@j4w8n @j4w8n-malynium
fix: set cookies for password recovery event (#32)
7dc1837 
## What kind of change does this PR introduce?

Bug fix

## What is the current behavior?

When resetting a user's password with an email template which has a URL
defining the `type` value as `recovery`, and using the `verifyOtp`
method to process the token_hash and type, the SSR server client's
`onAuthStateChange` function does not recognize the `PASSWORD_RECOVERY`
event that verifyOtp fires. This prevents the code
[here](https://github.com/supabase/ssr/blob/main/src/createServerClient.ts#L199-L205)
from running; resulting in the new session not being saved to cookies,
and the user is not considered logged in.

Fixes #21

## What is the new behavior?

User is logged in.

## Additional context

Replaces PR #24

Co-authored-by: Jason Creviston <jason@malynium.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@J0 J0 J0 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Password Recovery Does Not Update Auth Session
3 participants
@j4w8n @J0 @j4w8n-malynium