Account Switching

[ekzyis]

Closes #489

This PR implements account switching:

Implementation details

If a user wants to add an account, they are redirected to /login with multiAuth param set:

components/switch-account.js:

const AddAccount = () => {
  const router = useRouter()
  return (
    <div className='d-flex flex-column me-2 my-1 text-center'>
      <Image
        width='135' height='135' src='https://imgs.search.brave.com/t8qv-83e1m_kaajLJoJ0GNID5ch0WvBGmy7Pkyr4kQY/rs:fit:860:0:0/g:ce/aHR0cHM6Ly91cGxv/YWQud2lraW1lZGlh/Lm9yZy93aWtpcGVk/aWEvY29tbW9ucy84/Lzg5L1BvcnRyYWl0/X1BsYWNlaG9sZGVy/LnBuZw' style={{ cursor: 'pointer' }} onClick={() => {
          router.push({
            pathname: '/login',
            query: { callbackUrl: window.location.origin + router.asPath, multiAuth: true }
          })
        }}
      />
      <div className='fst-italic'>+ add account</div>
    </div>
  )
}

With this param, a login flow within a session can be initiated. This param is checked in the backend to only set cookies without actually switching the user (return token instead of return user):

if (multiAuth) {
  // we want to add a new account to 'switch accounts'
  const secret = process.env.NEXTAUTH_SECRET
  // default expiration for next-auth JWTs is in 1 month
  const expiresAt = datePivot(new Date(), { months: 1 })
  const cookieOptions = {
    path: '/',
    httpOnly: true,
    secure: true,
    sameSite: 'lax',
    expires: expiresAt
  }
  const userJWT = await encodeJWT({
    token: {
      id: user.id,
      name: user.name,
      email: user.email
    },
    secret
  })
  const me = await prisma.user.findUnique({ where: { id: token.id } })
  const tokenJWT = await encodeJWT({ token, secret })
  // NOTE: why can't I put this in a function with a for loop?!
  res.appendHeader('Set-Cookie', cookie.serialize(`multi_auth.${user.id}`, userJWT, cookieOptions))
  res.appendHeader('Set-Cookie', cookie.serialize(`multi_auth.${me.id}`, tokenJWT, cookieOptions))
  res.appendHeader('Set-Cookie',
    cookie.serialize('multi_auth',
      JSON.stringify([
        { id: user.id, name: user.name, photoId: user.photoId },
        { id: me.id, name: me.name, photoId: me.photoId }
      ]),
      { ...cookieOptions, httpOnly: false }))
  // don't switch accounts, we only want to add. switching is done in client via "pointer cookie"
  return token
}
return null

With these multi_auth.* cookies set, we can now show accounts to the user to which they can switch. The multi_auth cookie is not HTTP only since we want to access it from JS. This shouldn't be a problem since this is only for reading which accounts can be switched to and render this view:

On click, another cookie multi_auth.user-id is created via JS. This cookie is read inside a middleware in the backend to replace the session cookie that will be used to determine the user in the backend:

middleware.js:

const multiAuthMiddleware = (request) => {
  // switch next-auth session cookie with multi_auth cookie if cookie pointer present
  const userId = request.cookies?.get('multi_auth.user-id')?.value
  const sessionCookieName = '__Secure-next-auth.session-token'
  const hasSession = request.cookies?.has(sessionCookieName)
  if (userId && hasSession) {
    const userJWT = request.cookies.get(`multi_auth.${userId}`)?.value
    if (userJWT) request.cookies.set(sessionCookieName, userJWT)
  }
  const response = NextResponse.next({ request })
  return response
}

Showcase

https://github.com/stackernews/stacker.news/assets/27162016/49f9ff4f-dc4a-4e46-bcd2-966be20fe66b

TODO

• [x] switching to anon
• [x] UI to go from anon back to session
• add multiAuth param to all login methods
  • [x] lightning auth
  • [x] nostr auth
  • [ ] ~~email auth~~ (decided to remove this auth method, see https://github.com/stackernews/stacker.news/pull/644#issuecomment-1822106605)
  • [ ] ~~github auth~~ (decided to remove this auth method, see https://github.com/stackernews/stacker.news/pull/644#issuecomment-1822106605)
  • [ ] ~~twitter auth~~ (decided to remove this auth method, see https://github.com/stackernews/stacker.news/pull/644#issuecomment-1822106605)
• [x] testing
• [x] replace hardcoded link for add account image
• [x] handle case where account is added which doesn't exist yet (this will simply create a new account now but current account will never be updated if multiAuth is used)
• [ ] ~~also refresh JWTs in multi_auth.*~~ (?) (how?)
• [x] think more about if using this "cookie pointer" approach is secure (found an article which also mentions "cookie pointers" - and I thought I invented that term, lol)
• [x] middleware is now applied to all routes ... performance impact? (nah, i think it's okay)
• [x] fix bug when looking at item and switching between anon and user ("fixed" in 0c8893c)
• [x] on logout, only delete session token for current logged in account and move user to next available account

[ekzyis]

There is a bug where you can't switch between anon and a user if you're currently looking at an item:

https://files.ekzyis.com/public/sn/account_switching_item_bug.mp4

I'll probably need to rewrite the corresponding code which calls hooks conditionally (depending on me)

edit: "fixed" in 0c8893c

[ekzyis]

Current state: https://files.ekzyis.com/public/sn/account_switching_002.mp4

Missing (see PR description):

• JWT refresh: Figuring out if I can hook into whatever NextAuth calls to refresh the session token.
• Multi auth support for Github, Email, Twitter: Going to be hard since I can't really test (maybe github) and their flows are very different. But let's see
• some more testing
• replace hardcoded link for add account image

[ekzyis]

Mhh, okay, can't find how to access a response to set the required multi_auth.* cookies on successful authentication for the Github, Twitter and Email provider. Looking at the source also didn't help. I decided to just remove these providers from the available auth methods if multiAuth is set in 77887ae.

Same problem with refreshing JWTs. For some reason, the jwt callback only has access to request and response on login/signup.

Putting this out of draft since everything else works, so I think we can ship as is.

---

Update: Unfortunately, this doesn't improve anon UX a lot since you can't pay from your account balance yet. And the cookies are shared between tabs so if you switch in one tab, you also switch in the other tab (obviously). Switching to anon in one tab also currently breaks the other tab since the state is not properly updated in the other tab (educated guess). The other tab then thinks you're completely signed out.

Keeping this as ready for review since I think this is still ready for review (lol)

One upside is that with this feature, I can easily keep track of replies to @hn.

[ekzyis]

Reminder for myself: check if there is still an error (there probably is) if you're on /settings and then you switch to anon

[ekzyis]

I'll fix the conflicts, give me a sec

[huumn]

No rush. This won't until I ship territories first

[ekzyis]

This is going to be great for my new puppet account @oracle :eyes:

[huumn]

For some reason this doesn't work for me:

1. can't switch to anon
2. when I go to add account only some providers are listed

This might need migration logic to be added for stackers that are already logged in if that wasn't tested.

https://github.com/stackernews/stacker.news/assets/34140557/3da75c20-c66b-485f-9414-42c86352b412

[ekzyis]

For some reason this doesn't work for me:

1. can't switch to anon

Mhh, I had this error before. I think I fixed it by clearing all my session cookies since it's related to a (new) cookie being HTTP only when it should not have been set as HTTP only for some reason iirc. I think I also noticed that sometimes, cookies are not overridden but duplicated. This should only happen if the cookie path is different - it defaults to the current path if no path is specified (which should no longer be the case). I don't remember 100% though.

Can you show me your cookies when this bug happens to you?

2. when I go to add account only some providers are listed

I wasn't able to hook into the OAuth login process and Email auth (and was hard to test locally). So I thought - at least for the initial version - we can provide only Login with Lightning and Nostr if people want to use account switching.

This might need migration logic to be added for stackers that are already logged in if that wasn't tested.

Mhh, I didn't think too much about this but I think this should be 100% backwards compatible. But I will test (again).

[ekzyis]

Did a full test (afaict) again.

https://files.ekzyis.com/public/sn/account_switching_full_test.mp4

1. 00:00 - 00:15 -- Login sets multi_auth cookie which is a base64 encoded list of all available accounts and is accessible from JS (HTTP only set to false) and multi_auth.<userid> cookie with HTTP only set to true containing the JWT for account switching to this user.

2. 00:27 - 00:32 -- Switching to anon set the pointer cookie multi_auth.user-id to anonymous. This tells the backend to not use any JWT.

3. 00:33 - 00:56 -- adding a new account updates multi_auth cookie and adds a new multi_auth.<userid> cookie for that user.

4. 00:59 - 01:06 -- switching between accounts (anon or not anon)

5. 01:07 - 01:13 -- logging out switches to next available account and deletes multi_auth.<userid> cookie of completely logs you out (same as before)

6. 01:26 - 02:28 -- testing backwards compatibility by simulating a user that logged in without account switching deployed and then uses account switching

7. 02:28 -- ok there is indeed a bug lol. new account does not show up in the list even though cookies seem to be set correctly at first glance.

[ekzyis]

Putting in draft until bug is fixed

Edit: mhh, can't find how to put in draft in the Github app

[ekzyis]

Sessions that existed before we deployed account switching should now also be able to use account switching immediately. We now sync the cookie from their session if there is no multi_auth cookie. See https://github.com/stackernews/stacker.news/pull/644/commits/6839066466c67c33486119c384929445a32abaaf

https://github.com/stackernews/stacker.news/assets/27162016/6dd4d767-b7b7-49dc-8002-7e8939db23df

[huumn]

So this is out of draft now?

[ekzyis]

So this is out of draft now?

Going to fix conflicts now, just had a call with someone from Alby

btw, regarding this:

can't switch to anon

I noticed that sometimes, it just takes some time (max 5 seconds) for the switch to anon to complete. Not sure if it's because of dev mode or network latency or something else (bug). But yes, in your video, it didn't look like anything is going to happen, no matter how long you wait. Would be interesting to see if multi_auth.user-id is set to anonymous in that case.

update: testing currently again after fixing conflicts

[huumn]

This is still not working for me. I even logged out and deleted all cookies.

1. when I click on anon it gets selected but nothing changes a. even after waiting a very long time b. my user also stays selected
2. when I attempt to add another account, no account is added and instead I get a ?error=CredentialsSignin
3. here's my cookie state
4. I'm using brave.
5. When I logout, then it does switch me to anon

[ekzyis]

Mhh, interesting, why are your NextAuth cookies named differently than mine - even using the same browser? Maybe OS has an effect?

My code assumes that NextAuth uses __Secure-next-auth.session-token for the session cookie name. See here and here.

This could cause your problems but I am not 100% sure if that's all there is to it. Can you test ~~b156b0b~~ ~~049bd752~~ 1eb6665?

Tested it myself with the new session cookie, still works for me:

video

https://github.com/stackernews/stacker.news/assets/27162016/e75aea5b-ab08-46c8-8443-a0166700a7c4

edit: btw, why have your NextAuth cookies not Secure set?

On my browser, every cookie has Secure set and this is how it should be. That's probably related to why your session cookie does not start with __Secure:

Cookie prefixes make it possible to flag your cookies to have different behavior, in a backward compatible way. It uses a dirty trick to put a flag in the name of the cookie. When a cookie name starts with this flag, it triggers additional browser policy on the cookie in supporting browsers.

The __Secure- prefix makes a cookie accessible from HTTPS sites only. A HTTP site can not read or update a cookie if the name starts with __Secure-. This protects against the attack we earlier described, where an attacker uses a forged insecure site to overwrite a secure cookie.

update: ok, I also have this switching bug now:

video

https://github.com/stackernews/stacker.news/assets/27162016/4f2f206b-215f-48c8-a807-d3b0c1a80a25

~~So seems to be related to the name of the session cookie. But it worked before, I only rebased this branch on master again (from b156b0b to bfc4955) :thinking: Will test b156b0b again.~~

~~update: b156b0b no longer works, too. I think I am relying on some inconsistent behavior. At least I can reproduce the bug consistently now.~~

update: okay, i think it's related to the middleware not running for some reason. I literally only added some console.log's in the middleware and then it started to work again on b156b0b. Also works on bfc4955 now. 049bd75 might fix this problem since now the middleware is explicitly told to match routes.

However, I noticed that we might have a serviceworker caching issue again. /next/_data routes don't seem to hit the middleware since they are already handled by the service worker. But I am pretty sure that there are cases where the output of SSR / client side routing (since that's where /next/_data is used) depends on the session. Maybe that's also why on 00:48 the reply input suddenly gets filled? However, probably not since that's something that is handled by local storage only afaik.

video

https://files.ekzyis.com/public/sn/account_switching_middleware_match_next_data.mp4

Going to test this further.

update: this is (related to) what I am talking about - fixed in e471954 (see apollo docs)

video

https://github.com/stackernews/stacker.news/assets/27162016/77794fec-0d7e-4a07-91a9-5dda8904a3b3

update: regarding

I noticed that we might have a serviceworker caching issue again

We already use NetworkOnly in the serviceworker and there doesn't seem to be an issue but I am still confused why the middleware doesn't seem to get hit on client-side routing. It's client-side routing but shouldn't it still load data from the server?

[ekzyis]

Okay, found out why this doesn't work on localhost (big duh moment in hindsight, lol)

With 1845db2, I actually completely broke login on non-HTTPS sites since I am forcing the session cookie to have the Secure attribute set. Additionally, I am setting the__Secure- cookie prefix so the browser can't even read the cookie:

The __Secure- prefix makes a cookie accessible from HTTPS sites only. A HTTP site can not read or update a cookie if the name starts with __Secure-. This protects against the attack we earlier described, where an attacker uses a forged insecure site to overwrite a secure cookie.

-- https://www.sjoerdlangkemper.nl/2017/02/09/cookie-prefixes/

I am also setting Secure on all cookies required for multi authentication. So this explains a lot.

I am going to see how this could work on localhost / HTTP without introducing vulnerabilities. Going to check how Next-Auth does it.

update: Seems like all they do is to check if the protocol is http://. Then they don't use secure cookies:

This option defaults to false on URLs that start with http:// (e.g. http://localhost:3000) for developer convenience.

-- https://next-auth.js.org/configuration/options#usesecurecookies

[ekzyis]

Also works on HTTP now:

https://github.com/stackernews/stacker.news/assets/27162016/9c39ad41-2cf2-4896-9e3c-8de7c5358055

[ekzyis]

Account switching could also be useful for local development

Need to rebase the code and see if I can remove the code in the middleware and replace it with code similar to #915

[socket-security[bot]]

New and removed dependencies detected. Learn more about Socket for GitHub ↗︎

Package	New capabilities	Transitives	Size	Publisher
npm/[email protected]	None	0	23.7 kB	dougwilson
npm/[email protected]	environment, filesystem Transitive: network, shell, unsafe	+86	17.8 MB	google-wombot

🚮 Removed packages: npm/[email protected], npm/[email protected]

View full report↗︎

[ekzyis]

This PR is rebased now and switching between user and anon works. I was able to move the cookie logic from middleware.js to pages/api/graphql.js. However, adding a new account does not work yet since multiAuth isn't set in the backend for some reason.

TODO:

• [x] fix multiAuth not set in backend
• [x] fix alignment of selected
• [x] use something prettier instead of + add account
• [x] close sidebar if switch account was selected
• [x] ~fix stacking of modal to switch accounts if you press "switch account" multiple times~ fixed by closing the sidebar since you're not supposed to be able to click "switch accounts" multiple times
• [x] fix all cases where switching account would lead to a crash instead of 404. example: create item with one account but don't pay so it's a pending item. click on pending item, then switch account.
• [x] use width: fit-content for signup button in header as anon

[ekzyis]

Update: Unfortunately, this doesn't improve anon UX a lot since you can't pay from your account balance yet.

-- https://github.com/stackernews/stacker.news/pull/644#issuecomment-1822106605

If we allow to share attached wallets between switched accounts (including if you switched to anon but not if you're entirely logged out), that would fix this.

[ekzyis]

Current state:

https://github.com/user-attachments/assets/d189bc9d-72b6-42e9-8f9f-2b6a8b861710

Just noticed the flash from "Login" to "Signup" when switching to anon. Investigating.

(non-blocking) TODOs:

• [x] fix flash from "Login" to "Signup"
• [ ] look into support for other auth methods
• [x] fix inconsistent button padding in offcanvas vs dropdown when anon

[ekzyis]

Mhh, I should probably refetch specifically the queries that I want to refetch instead of blocking the ones that I don't (whitelist vs blacklist approach). Found a bug where an invoice from a different user was fetched.

TODO:

• [x] really fix all cases where switching account leads to bugs, for example because of query refetches
• [ ] text-underline disappears on hover because of a:hover { text-underline: none } which means on mobile it doesn't show up after click
• [x] make sure this works for existing sessions which don't have any multi_auth.* cookies

[ekzyis]

The only real thing left here is to add the OAuth providers but I'm having a hard time to get it working.

If an OAuth provider is not linked to any account, it links the auth method to the existing account but I would prefer it to create a new separate account with that provider. If the OAuth provider is already linked to an account, it throws OAuthAccountNotLinked. Both make sense since that's the normal OAuth flow but it's not what we want for account switching. If I hit "add account", I don't want to link a new auth method. I want to be able to sign in as the user (that might already exist or not) while I am already signed in as another user.

Essentially, I don't know how to use the OAuth options to tell NextAuth to handle the case where multiAuth is set differently.

Other than that, this is ready for review.

[huumn]

The only way to do something like that with oauth would be to hack around nextjs in some way. To get account linking to work at all, I had to read all the relevant nextjs source. It was designed to link on email address matches between providers iirc.

I haven’t read the code, but perhaps we can “logout” the user (make cookies appear logged out to nextjs) when they go to create a new account so there’s nothing to link to.

[ekzyis]

I haven’t read the code, but perhaps we can “logout” the user (make cookies appear logged out to nextjs) when they go to create a new account so there’s nothing to link to.

Mhh, this could work using something similar to credentials: "omit" for the fetch API. But the response must still be able to set cookies.

Mhhh ...

[ekzyis]

I noticed yesterday that Github actually also supports account switching with a nice icon we could use, too:

TODO:

• [x] show OAuth providers on multi auth screen but as disabled until fixed. On hover, show "not available yet" or something like that
• [ ] I forgot about email authentication. Maybe that is possible. Look into it

[huumn]

show OAuth providers on multi auth screen but as disabled until fixed. On hover, show "not available yet" or something like that

Great idea!

[ekzyis]

Okay, I didn't really try with email, only looked at the options but I suspect it's going to be a mess like OAuth so let's skip this for now (more important things to do right now).

This can be looked at again when we want to send codes instead of magic links (https://github.com/stackernews/stacker.news/issues/727)