Make set_issuer and set_audience require aud, iss claims#520
Make set_issuer and set_audience require aud, iss claims#520mikkel3000 wants to merge 2 commits into
aud, iss claims#520Conversation
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. |
|
I'd like to try and get v11 out this month if possible, as it contains changes that others have been waiting for for quite a while |
I will have a go at it and update this PR soon 👍 |
The previous api mixed convencience methods and direct setting of attributes, which made some functionality confusing. This refactor only allows building the Validation using methods, trying to make it hard for the user to go wrong. It removes the opportunity of setting issuer or audience without requiring their existence. It also removes audience as a required claim, the user should call .with_audience method instead. This commit is still a WIP, places which needs further work or documentation are marked // TODO: This is a breaking change.
I have made a suggested API change, there is still polishing and documentation to be done, but the methods are mostly finished. |
|
From a quick glance direction seems fine, but I'm not sure about the builder style |
3 participants
Addresses #493
set_audience and set_issuer convenience functions now adds
audandisstorequired_spec_claims.