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

Support JWT #9

Open
joshbuker opened this issue Apr 5, 2021 · 7 comments
Open

Support JWT #9

joshbuker opened this issue Apr 5, 2021 · 7 comments
Labels
enhancement New feature or request
Milestone

Comments

@joshbuker
Copy link
Member

Opening this issue to track the conversation on adding JWT to Sorcery.

@hayfever
Copy link

@athix Is there anything missing from the referenced PRs or sorcery-jwt that you would like to see implemented here? From what I can tell we were all doing roughly the same thing, however, I'm curious if there's more you'd like to see IE refresh tokens, OAuth compatibility, revocation strategies

@joshbuker
Copy link
Member Author

@hayfever That's an excellent question. I'm not really sure what the right balance between a simple API interface and feature completeness would be for JWT.

Ideally, I would want our solution to be designed such that we can add any common JWT features we decided to omit later, without introducing breaking changes to the API. Alternatively, follow in the spirit of Sorcery and allow the downstream developer to easily extend the existing feature-set themselves to suit their needs. (or both)

I think the main goal with the JWT plugin would be to add complete API-only support to a Sorcery application. Whatever the current standards are for that, we should support. I have yet to use Sorcery for that purpose, but my current project will be an API-only Rails app that acts as the back-end, with Sorcery v1 as the authentication.

This is a good place to collect the features that would be nice-to-haves and must-haves.

Something else to consider is cross-plugin support. E.g. supporting the MFA plugin when using JWT. Although that's more of a library design decision than something JWT specific.

@joshbuker
Copy link
Member Author

To be explicit on my intentions, I do want to support OAuth compatibility, refresh tokens, and revocation. I also plan on including multi-session (or enforcing single session per user) support, MFA, and combining said features as desired.

I'm currently trying to think through how to get everything to interoperate properly. Documentation on how to do this properly seems to be all over the place, to be honest. Incorporating the MFA challenge response is the trickiest part in my mind currently, as it breaks from the current flow the most.

Not a small undertaking, that's for certain.

@joshbuker
Copy link
Member Author

@hayfever quick sanity check, would it be reasonable to offload the responsibility of token revocation to the database?

It would be relatively simple to tie the JWT to a session record instead of the user, and allow Sorcery to check that session record for if it has been revoked. This won't have a performance impact in most scenarios, due to sorcery already doing a database request on every query to load the current_user. (New code would join session and user to provide both with a single query). This would also tie into the plans for allowing multi-session/single-session support neatly.

This would avoid the significant complexity of alternative techniques: https://auth0.com/blog/denylist-json-web-token-api-keys/

One other problem I'm trying to figure out is how to have a sessions table without the ID column quickly getting insane. active-record_session-store should be doing something similar to what I'm looking at, so maybe I can get some inspiration from their solution. I suspect I'm doing something incorrect design-wise though, considering how much effort is put into avoiding database reads for auth.

@hayfever
Copy link

hayfever commented May 8, 2021

Yeah I think it's definitely reasonable since you're already querying for the user like you said. This is what I've referenced for revocation in the past and seems to have some decent ideas: https://waiting-for-dev.github.io/blog/2017/01/24/jwt_revocation_strategies

As far as the table not growing to an unwieldy size, the only thing that immediately comes to mind is having a periodic job to delete the revoked tokens that are past their expiry - maybe this could be done using ActiveJob (instead of making it sidekiq/resque specific)?

@joshbuker
Copy link
Member Author

@hayfever It has some known sharp edges, but the JWT implementation is passing the test suite if you want to take a look.

The query will still do two DB hits, as I'm still thinking through how to best expose that find method. As it is (contained in the sorcery_orm_adapter), I imagine it would be frustrating trying to modify the query downstream in an app. One thing that I'm curious about is if we can remove the ORM adapter entirely, or if other ORMs (like mongodb) don't overload the same activerecord methods. The cleanup job isn't implemented yet for similar reasons (how to best expose it to apps).

The current flow screams security vulnerability waiting to happen to me, due to allowing the login/logout flow to raise an exception in the middle of the process. I'll spend more time thinking about that flow and how to make it safer; anyone with ideas on how to approach the problem are welcome to participate.

What is the sanest option when logout fails due to ActiveRecord failing to destroy the record (meaning the session was not properly revoked)? Obviously it should enter some sort of error state and let the application make a decision on what to do, but what about the logout callbacks and such? Do we expose a new callback specifically for the error state? Lots of questions to mull through.

@Nerian
Copy link

Nerian commented Jul 5, 2022

Being able to use Redis as the backend for sessions/tokens would be neat and it will auto expire keys without overhead by simply setting ttl

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants