-
Notifications
You must be signed in to change notification settings - Fork 111
Parse unverified JWT #756
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: ladvoc/uniffi-trial
Are you sure you want to change the base?
Parse unverified JWT #756
Conversation
|
@1egoman I'm thinking of removing JWTKit dependency in Swift, what else do you need for JS - something like: pub fn with_unverified_options(
token: &str,
validate_exp: bool,
validate_nbf: bool,
leeway: Option<Duration>,
) -> Result<Self, AccessTokenError> |
|
@pblazej I think the As an unrelated FYI, I actually implemented some quite similar logic here but I like the way you did it here better, so I'll just use this interface instead once this one is merged. |
| #[cfg(test)] | ||
| { | ||
| validation.validate_exp = false; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Out of curiosity, could it be possible to add make the test token have an expiry time way off in the future (like greater than the year 2100) and get rid of disabling this conditionally?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1, can we use https://www.jwt.io/ to generate signed tokens that with long expired date for testing purpose ?
|
|
||
| #[test] | ||
| fn test_unverified_token() { | ||
| let claims = Claims::with_unverified(TEST_TOKEN).expect("Failed to parse token"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO, it would be good to have a 2 other tests:
- Try parse a token that was valid in the past but no longer is valid, and make sure it fails
- Try to parse a token which will become valid in the future and make sure it fails
Yeah, I realized after a while it must be duplicated 😃
Yes, I struggled a bit with custom types there, but may be worth doing
Is that I'll update this PR in spare time 👍 If the token source goes merged first, feel free to close it/use it somehow. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM ✅
| } | ||
|
|
||
| impl Claims { | ||
| pub fn with_unverified(token: &str) -> Result<Self, AccessTokenError> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick: I think from_unverified is a more descriptive name (it decodes Claims from an unverified token)
| /// WARNING: Do not use this for authentication - the signature is not verified! | ||
| /// | ||
| #[uniffi::export] | ||
| pub fn unverified_token(token: &str) -> Result<Claims, AccessTokenError> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question: Not specific to this PR, but in Swift, symbols will be imported in the global namespace, right? If so, we might consider using a standardized naming scheme for exported symbols from each module. For example, in this module we would have token_claims_from_unverified (this method), token_verify, and token_generate—everything is prefixed with "token" so it is clear which module it comes from.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep I wanted to ask that in Notion actually, it's not possible to create something like Claims.myNewMethod at all?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
^ I've only played around with the python bindings, but at least with those I was able to export a class as an "object" with static methods (I think I had to mark them technically as "alternate constructors") and that seemed to work. But maybe the swift bindings are different.
Enables "caching token source" on the client without 3rd party dependencies.