-
Notifications
You must be signed in to change notification settings - Fork 229
Claims JWT Support #716
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
base: main
Are you sure you want to change the base?
Claims JWT Support #716
Conversation
Some (foreign and unrelated to this PR) tests in |
The test was fixed in #728. Could you please rebase? |
Merged! Now all the tests should pass! ✅ |
Looked over this a few times over the past weeks but conflicted about the somewhat large surface area it covers (sort of feels like it should be its own package). Me and @wallyqs will discuss this offline. |
Any progress on this @caspervonb ? My company is waiting for this change to implement NATS Auth Callout |
Yeah so since this doesn't actually touch anything in the nats package, I've been thinking to turn this into its own namespace package as I have multiple pending pull requests that take the same approach, e.g How do you feel about that @arnavdas88? I'd probably open a derivative PR doing it. |
Yes @caspervonb , That sounds great. I have made some updates as well that I have yet to commit. I will directly push the updated code in the new namespace then. |
@carlosjgp In the meanwhile, you can try using the arnavdas88:jwt-claims for testing as well as to develop on top of it, locally. Once the new namespace is ready, you will just have some minimal changes to make it work. That will also give us time to add additional functionalities, or change the structure if necessary as well as help in making the jwt-claims PR, more mature for developers to use. I believe, you can install the pip install git+https://github.com/arnavdas88/nats.py.git@jwt-claims |
Started to clean this up over in #740 but I just have to ask, is this AI generated @arnavdas88? Some problematic patterns that became immediately apparent when starting to clean up:
|
1.) The Flattening Model is indeed over complicated (I have already written that in the PR). Also, it cannot load a dict / json right now. It can only convert the dataclass to a dict. I have an alternate implementation that makes it a little bit simpler, while adding both serialization and serialization. But it will still add some complications. 2.) There are Redundant Constructors because I took a lot of reference code from the golang package, as well as some code (for the constants and the jwt encoding and decoding) from the javascript/typescript package. For the Redundant Code as well as the Flattening Model, I am trying to keep the code format and classes as consistent as possible with the golang code. 3.) About the inconsistent dataclass approach, dataclasses does make it easier for us to develop, but when inheriting multiple dataclasses, the classes will not have the nested structure (that is possible with golang's struct), and instead become a flat class (all the attributes are flat. I wanted to avoid that. Currently the classes that do not need to inherit any other model (source reference from go package) is replaced by dataclasses, whereas any model that inherits other classes use the 4.) utils.py is indeed a complete copy paste of the 5.) Empty classes for claims - They are mostly used as placeholder right now, as most of the behaviour for the claims is a bit confusing to me! Specially the OperatorClaims. The placeholder can be modified by someone more experienced in the claims part. 6.) Manual JWTs were constructed because that part of the code was referenced from the typescript/javascript code where the jwt were manually constructed. It was necessary as the jwt the algorithm used in the jwt for claims encoding doesn't usually fall under the standard algorithms, and may or may not raise issues. Hence I kept those functions custom. 7.) Constants doesn't only not pass lint, most of the constants are not even used. But they exist and may help to fetch specific data from NATS server. So I kept all of them! The constants exist in js/ts package but not in the python package. I wonder why! Would love to know your opinion on these! |
Did anyone interested in this evaluate https://pypi.org/project/nats-jwt for this purpose, and if so what where did you find it fall short? |
I didn't know this existed. Looks like nats-py-jwt is the source GitHub. It looks like they can use some help with the tests. But overall the structure is pretty pythonic. It should work for most of the use cases. I'll give it a try ⭐! |
Maybe the route to go with this is to lend a hand over there, looks like they've gotten pretty far. |
Added Pythonic Interface for creating
Account Claims
andUser claims
as JWT.Issue: Issue 717
Replicated the example at NATS by Example - Programmatic NKeys and JWTs in the test file tests/contrib/test_jwt.py.
Example Code Snippet
Some issues with the PR
contrib/
.encode_user_claim()
andencode_account_claim()
is not yet mature and only exists in the test file.