[wip] add jwt action
#1
Conversation
There was a problem hiding this comment.
The strategy I have used to check if the string is a valid jwt is:
- check if the characters used are valid and the string is divided in three parts (using a regex)
- since the header is an object with
typand optionalalgorithmproperties, check the header too (using JavaScript)
Something similar was done here but I think the first check with the regex is still needed), do you think it's a good idea?
There was a problem hiding this comment.
I will review this and the 'none' algorithm question after we solved the other questions.
| } from '../../types/index.ts'; | ||
| import { _addIssue } from '../../utils/index.ts'; | ||
|
|
||
| const POSSIBLE_JWT_REGEX = /^(?:[\w-]+\.){2}[\w-]+$/u; |
There was a problem hiding this comment.
Usually the regexes are stored inside /library/src/regex.ts. But I think the file is used to only store complete public regexes. By complete I mean if the string passes the regex's test, the string is valid. Like I stated (the comment at the top of this file), this regex is just the first part of the whole check, so its better it is a private member than in /library/src/regex.ts. Also, by naming it POSSIBLE_JWT_REGEX I meant if the string passes the test, there is a possiblity of the string being a valid jwt. If you have a better name for this regex ref, let me know.
| | 'PS256' | ||
| | 'PS384' | ||
| | 'PS512' | ||
| | 'none'; |
There was a problem hiding this comment.
library/src/actions/jwt/jwt.ts
Outdated
| | 'PS512' | ||
| | 'none'; | ||
|
|
||
| function isValidJwt( |
There was a problem hiding this comment.
Would you recommend inlining this function in the requirement function of the implementation? Or is this okay? I think this is better for readability.
There was a problem hiding this comment.
I think I would implement it similar to how creditCard is implemented.
library/src/actions/jwt/jwt.ts
Outdated
| /** | ||
| * The algorithm used to sign the jwt. | ||
| */ | ||
| readonly algorithm: TJwtAlgorithm; |
There was a problem hiding this comment.
For some actions, there is a requirement property. Initially I concluded, the algorithm can be considered as a requirement but after thinking about it, I decided to add a new key because I think a requirement is something complete. Example: the length action with requirement set to 5 would mean testing the input with the requirement, nothing else. But in this case there are two steps. So, should I use the requirement key? If yes, what value should I set it to? What was the purpose of the requirement key in the issue and action objects?
There was a problem hiding this comment.
I have answered this at the other comment.
| /** | ||
| * The algorithm used to sign the jwt. | ||
| */ | ||
| readonly algorithm: TJwtAlgorithm; |
There was a problem hiding this comment.
Same question mentioned here
There was a problem hiding this comment.
Adding the algorithm parameter as a property to JwtAction looks good to me, but I would not add it to JwtIssue as issues so far only contain common properties. In simple words, we never added additional properties to issues in the past, but we added them to the action itself to make them accessible and allow them to be serialized and dezerialized sometime in the future.
Improvements:
- remove the `algorithm` property from the `JwtIssue` interface
so that only common properties are present in the interface
- add the `requirement` property to `JwtAction` & `JwtIssue`. The
property is mapped to the jwt validation function
|
Thank you! I think everything is clear from this point, except the strategy used. My plan:
|
No description provided.