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

Use auth cookie set by backend instead of jwt (fixes #2193) #2208

Closed
wants to merge 1 commit into from

Conversation

Nutomic
Copy link
Member

@Nutomic Nutomic commented Oct 31, 2023

I think #2193 happens because lemmy-ui still uses the jwt cookie while the backend automatically sets auth on login. So by switching lemmy-ui to the same cookie it should prevent the login error. Unfortunately I cant reproduce the problem locally so Im not certain.

Requires LemmyNet/lemmy-js-client#208

@dessalines
Copy link
Member

You can use yalc publish --push to test lemmy-js-client changes locally, but I'll test this now.

@SleeplessOne1917
Copy link
Member

I noticed that the path for the cookie that's set on the server side is /api/v3/user. Is it supposed to be that?

image

@Nutomic
Copy link
Member Author

Nutomic commented Oct 31, 2023

Im not setting any path, seems like the library or browser is doing it automatically. Should I change it to a different one?

Btw if there is an existing jwt cookie, lemmy-ui should change that to auth (with secure and httponly) so that users dont get logged out when their instance is upgraded. But not sure how to implement that.

@dessalines
Copy link
Member

I've got this accounted for in #2210

Unfortunately we still have to use the manual set method for various reasons:

  • The cookie from the server is a short-term session one, so we'd have to manually overwrite it anyway
  • Tons of domain name problems with development if we try to read or set that cookie from the response headers, rather than the LoginResponse JSON.

@dessalines dessalines closed this Nov 2, 2023
@Nutomic
Copy link
Member Author

Nutomic commented Nov 3, 2023

We could also change how the cookie is set by the backend.

@dessalines
Copy link
Member

dessalines commented Nov 3, 2023

I think session cookies are intened to expire right after you close that tab / app. But its probably better for clients / apps to decide their own expirations anyway, and if we do have any expiration, have it built into the JWT.

If we wanted to be consistent, I'd argue that maybe setting a cookie and JWT expiration of a year or so might be a good idea. Security ppl will hate that, but less-so than now.

@Nutomic
Copy link
Member Author

Nutomic commented Nov 3, 2023

I didnt know that there are so many different options for cookies. Should we get rid of the code that sets cookies in backend then?

@dessalines
Copy link
Member

dessalines commented Nov 3, 2023

Tough call... I'd bring it up in one of the dev matrix chats. I do think it'd be much simpler tho to only support the Authorization Bearer header.

Applies to not just cookie setting, but cookie reading too.

@Nutomic
Copy link
Member Author

Nutomic commented Nov 3, 2023

I would still read the cookie for request auth besides the header, but only if it was explicitly set by the client.

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.

3 participants