Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
OIDC private key JWT / client assertion #155
OIDC private key JWT / client assertion #155
Changes from 11 commits
f8b3e76
d640633
ca292a6
8fca05f
2fe4cae
b40fd48
4dfbc63
d8b89d1
a71134d
53388a4
25a0e5c
8f9aa91
1d1813c
cf24de8
9f0b393
95b5c03
c06d559
70ea140
c871e25
e6d416e
62aaf97
9981bce
d6932e0
cd236a5
97b3588
e8dfe19
a94da54
8051410
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Do we still need this function? I think if we can get this public API down to
NewJWT
and.Serialize()
, that would be ideal. Can we move the checks that make sense from this function intoNewJWT
instead? If we're trying to help users using&JWT{}
instead ofNewJWT()
, I don't think we can assume that they will callValidate
if they already ignored the constructor, so I'm OK with that case just panicking or erroring in a bad way.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.
Seems reasonable enough to me -- Okay if I keep it as a separate function, just privatize it? I like small single-purpose functions.
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.
Sure, but I think some of this error checking doesn't really belong here anymore. E.g. the checking if j.key and j.secret are both set aren't necessary if you move the logic into the option as I suggested. Calling
Serialize
inNew
also feel really wrong IMO, since it's the primary thing the user wants to do. If that's where we're going with this - why do we need a whole struct (or even package) encapsulating this? We could just haveNewSerializedJWT
as an exported function (maybe we should?). I thinkNew()
andSerialize()
can be separate, but if this logic was moved intoNew
, it would be clear from the context of that function what should and shouldn't be necessary to validate. Ideally nothing other than the inputs to the function, which should be all the required parameters to create a JWT, should need to be validated. On that note, since either Key or Secret must be set, should it be a parameter rather than an option?I'm personally not a huge fan of more smaller functions, because the context in which they are used is not clear and indirection always leads to overhead for the reader. I almost left a comment saying that the structure in
Serialize
was unnecessarily abstracted into smaller functions, but it's such a minor comment that I didn't in the end feel that it was important enough to bring up, but since we're talking about it now, there you go 😁.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.
Thanks for the thoughtful reply!
I'll say up front that I'm down to remove the
Serialize()
call fromNew
- it felt weird when I put it there, so easy to drop it. Also good to move validation intoNew
.I like cross-field validation to happen in its own spot, but it amounts to the same result either way, so I'd be happy to put the checks in their respective options. except:
The current not-actually-optional-
Option
thing has been bugging me. Here's some (pardon the pun) other options:""
/nil
jose
does, and accept anany
but assert supported types. Personally this kind of annoys me aboutjose
That would leave a lot less validation to do, but both could set up and return a
JWT
, which is mostly shared logic internally.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 like option 3 a lot!
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 believe the algorithm will need to be a parameter too, and you could even make it RSAlgorithm and HSAlgorithm respectively :)