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

Always return new user token for JWT callback to use #1783

Merged
merged 1 commit into from
Dec 31, 2024
Merged

Conversation

ekzyis
Copy link
Member

@ekzyis ekzyis commented Dec 31, 2024

Description

As @riccardobl mentioned in #1781, #1779 is caused by the cookie pointer update in pubkeyAuth getting overridden back to the initial cookie pointer in the JWT callback.

As I mentioned in #1779 (comment), I thought I fixed this in #1618 but it appears that I didn't really test all cases, but only the case when we switched to anon and then try to add a new account. Then we automatically get switched because the JWT callback will run with the token set to the new user since we return the new user token (the variable that is called user) in pubkeyAuth, not the old user token (the variable that is called token).

However, as noticed in #1779, this does not work if you're logged in as some account and then add an account because we then return token (the old user). This is what I must have broken in #1618 and @riccardobl fixed in #1781. However, #1781 then breaks switching from anon to a new account because setMultiAuthCookies will not be called in pubkeyAuth and in the JWT callback.

Since the token that the JWT callback receives is the token we returned in pubkeyAuth, returning the token of the user we want to switch to in pubkeyAuth makes sure that the JWT callback will call setMultiAuthCookies with the token we want. So afaict, we don't actually need to set the cookie pointer in pubkeyAuth but can rely on the JWT callback to do this for us. This is what this PR does.

fix #1779 supersedes #1781

Videos

switching around to new accounts:

2024-12-31.02-53-15.mp4

switching around to existing accounts:

2024-12-31.03-03-51.mp4

unlinking + linking new nostr key:

2024-12-31.03-20-36.mp4

Checklist

Are your changes backwards compatible? Please answer below:

yes

On a scale of 1-10 how well and how have you QA'd this change and any features it might affect? Please answer below:

7. It was a little tricky to consider all cases and make sense of them but I've tested all cases afaict now as can be seen in the videos and all make sense to me now. Simply return token we want to switch to and let JWT callback set the cookies. We only need to consider account switching in pubkeyAuth to not accidentally update pubkeys when we instead wanted to switch accounts.

For frontend changes: Tested on mobile, light and dark mode? Please answer below:

n/a

Did you introduce any new environment variables? If so, call them out explicitly here:

no

@ekzyis
Copy link
Member Author

ekzyis commented Dec 31, 2024

We only need to consider account switching in pubkeyAuth to not accidentally update pubkeys when we instead wanted to switch accounts.

btw, this is the reason why the OAuth providers don't work because we don't have this control. They always think we want to update an authentication method instead of logging in as a new account.

@huumn
Copy link
Member

huumn commented Dec 31, 2024

btw, this is the reason why the OAuth providers don't work because we don't have this control. They always think we want to update an authentication method instead of logging in as a new account.

Whether an account is linked or creates a new one is affected by these lines:

if (token?.id) {
// HACK token.sub is used by nextjs v4 internally and is used like a userId
// setting it here allows us to link multiple auth method to an account
// ... in v3 this linking field was token.user.id
token.sub = Number(token.id)
}

Before this branch, accounts would not link. Maybe there's a way to conditionally enter that branch?

@huumn huumn merged commit 4623743 into master Dec 31, 2024
6 checks passed
@huumn huumn deleted the fix-1779 branch December 31, 2024 19:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Adding a new Switch Account does not switch to new account
2 participants