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

Transition to JWT Authentication #68

Closed
wants to merge 5 commits into from

Conversation

ajmeese7
Copy link
Member

This PR is part of a series intended to improve security for the authentication flow for OS.js. Storing a cookie with the cleartext username/password combination on the client side is not a safe method of authentication, and this PR is intended to help resolve that issue.

@andersevenrud
Copy link
Member

Storing a cookie with the cleartext username/password combination on the client side is not a safe method of authentication, and this PR is intended to help resolve that issue.

I don't understand this part. The default authorization with cookie only stores a "session id" that is validated on the server each request. No credentials are exposed.

@ajmeese7
Copy link
Member Author

OS.js stores the stringified username/password in a client-side cookie (reference) for use in authentication. This is insecure and should be avoided, as is explained in this thread. Additional information can be linked if necessary.

This is the server-side portion of the three PRs that will be made, it is still a draft until the client-side and main OS.js PRs are opened.

@andersevenrud
Copy link
Member

OS.js stores the stringified username/password in a client-side cookie

Ah, you're referring to the auto-login.

os-js/OS.js#761 (comment)

@ajmeese7
Copy link
Member Author

ajmeese7 commented Aug 22, 2022

Correct, I am replacing the username/password based autologin with the ability to use a JWT refresh token to create another access token. This should replace the insecure cookie login and be the more secure default alternative, ideally.

Sorry if that was unclear before :)

@andersevenrud
Copy link
Member

andersevenrud commented Aug 22, 2022

I understand now. And this is exactly what I wanted in the issue I linked (and is for example how the Auth0 adapter I made works) :)

@ajmeese7
Copy link
Member Author

@andersevenrud as we discussed, the code has been pushed to make it easier to review. I am going to request your review since I know that changes will be required before this is merged, I just have not yet had the time to get to them.

Copy link
Member

@andersevenrud andersevenrud left a comment

Choose a reason for hiding this comment

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

Here's my preliminary feedback. Let's tackle this and then I'll do another pass :)

src/auth.js Outdated Show resolved Hide resolved
src/auth.js Outdated Show resolved Hide resolved
src/auth.js Outdated Show resolved Hide resolved
src/utils/core.js Outdated Show resolved Hide resolved
@ajmeese7
Copy link
Member Author

@andersevenrud I believe all of the requested changes have been made

src/auth.js Show resolved Hide resolved
src/auth.js Show resolved Hide resolved
src/auth.js Show resolved Hide resolved
src/auth.js Show resolved Hide resolved
@andersevenrud
Copy link
Member

Note to self: Check out how I integrated Auth0 Auth adapters because some of the features added here potentially needs a way to be overridden so that there's not two token mechanisms on top of each other.

@andersevenrud
Copy link
Member

andersevenrud commented Aug 30, 2022

Note to self:

JWT however is immutable. You put the session data into the token on login, and that's it.

Does this have an impact on any current users ? The manual and examples contains stuff that demonstrates how to mutate the session for doing arbitrary stuff on the back-end with service providers and applications.

I.e.: is there a need to issue new tokens if that's the case ? This scenario would require new mechanics in the client.

Also, related to arbitrary stuff; what about secrets that's technically secure with the cookie mechanism, but now would be clear text in the token ?!

@ajmeese7 ajmeese7 closed this Sep 1, 2022
ajmeese7 added a commit to meese-os/meeseOS that referenced this pull request Sep 3, 2022
The previous method of authentication involved storing the username/password as a cleartext cookie on the client, whereas now a refresh token is stored that can be revoked from the server-side database at any time. This is far from perfect, but it will be a step in the right direction as we explore alternative options moving forwards.

Related to the now-defunct os-js/osjs-server#68.
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.

2 participants