Make set_issuer and set_audience require aud, iss claims#520
Conversation
mikkel3000
force-pushed
the
feature/493-set_spec_claims
branch
from
e73b62c to
a5a4aab
Compare
arckoor
left a comment
arckoor
left a comment
There was a problem hiding this comment.
Should be fine, without some more doc on set_required_spec_claims opens up the door for another footgun because that method unconditionally overwrites everything, it's not additive.
The entire interface is imo weirdly split between using methods on Validation, and setting some attributes directly. Thoughts @Keats?
| validation.set_required_spec_claims(&["exp", "iss"]); | ||
| validation.leeway = 5; | ||
| validation.set_issuer(&["iss no check"]); | ||
| validation.set_audience(&["iss no check"]); |
There was a problem hiding this comment.
Could be augmented by not adding "iss" manually
Though then you can just remove the entire set_required_spec_claims
The API of the Validation struct is a bit confusing IMO (and has been adding things without too much thought put into it) and was always something I wanted to redo but never spent time on. It would be good to see what something designed from scratch for all the current features would look like. |
Addresses Keats#493 set_audience and set_issuer convenience functions now adds `aud` and `iss` to `required_spec_claims`.
mikkel3000
force-pushed
the
feature/493-set_spec_claims
branch
from
a5a4aab to
6f4a9d5
Compare
Does that mean you do not want to use this change, and wait for a larger refactor of the validation API? |
There is a new major version coming so it would be fine. |
3 participants
Addresses #493
set_audience and set_issuer convenience functions now adds
audandisstorequired_spec_claims.