Handling verification options about joken HOT 15 CLOSED

I think this is only a question of supporting an optional parameter in all verify function variants. Something like:

def verify(other_parameters, options // []) do
  // check if options has skip_claims or skip_verifying
end

Though I really think that skipping verification is a somewhat dangerous option and I think we should ask if the new API supports the mentioned use case better.

@bryanjos @keichan34 what do you think?

from joken.

Good question. I think for skip_verifying, the none algorithm could be used. It's specifically used for unsecure JWSs if I'm reading the spec correctly. For skip_claims, we could add an option as stated or maybe another function to remove validations from the Token struct?

from joken.

I'd say both 👍

from joken.

Actually I may have to rethink what I said about the none algorithm for this. You would still need the algorithm to decode it. For skip_claims, which one do you think works better, adding it to the options or a remove_validations function?

from joken.

I think we should have both an option parameter and a remove_validation function. They would work a bit different. The skip_claims in the option list would skip for that single verification and the remove_validation would remove it from then on.

from joken.

@keichan34 do you still have the use case of skipping verification? Does the new API help with your use case? If that is not a problem I'll propose to close this issue.

Thanks!

from joken.

I do have this use case, yes. The "none" algorithm would work, but I think for now it may be out of the scope of this project? I can parse the token for its claims manually, so I might end up just doing that. (I'm validating the token based on what is in the "iss" claim -- this is probably an edge case?)

The other changes look great!

from joken.

I think that is somewhat an edge case yes. The none algorithm would probably not help you much here because you would run verify 2 times (one with a token configuration with a none signer and one with your real signer) and that is unnecessary. I guess your best bet is to take the second part of the token (the one between dots) and decode it on your own to take the iss claim.

Another approach (and probably safer) is to use the token header. It might contain some specific data like kid (key id) which could help in cases where you have different keys for verifying. Right now we have no means to pre-process the header of a token (where you could look for the kid). That seems like a candidate for a future version. @bryanjos we should think about it!

Other than that I've been thinking about and talking with @bryanjos about adding some context to validating functions but that happens after after signature verification and so wouldn't help you either.

from joken.

I'll go ahead with decoding the token on my own, then. Thanks for your help! Looking forward to the release. 😃

from joken.

Awesome! I guess this is resolved for now so I'll close this issue.

from joken.

Reopening as it seems the use case for getting claims before verification is a common one and one we should implement. Erlang-Jose is going to implement something similar and we can build off of that.

from joken.

@bryanjos I have a question about this use case: should we implement it inside the signing or verifying steps or as a standalone function outside of these steps?

I mean, the first case can be used to notify or log before signing and verifying. The second case can be used to change which keys to use. I think this is the one asked the first time, right?

If that is so, we don't need to wait for erlang-jose. We can simply export a function that will use return the claim map.

What do you think?

from joken.

@cs-victor-nascimento it looks like there is a JOSE.JWS.peek/1 function now in erlang-jose which is the one @potatosalad said he was going to implement. We could make it it's own function I guess if it makes sense to. We could name it "peek" as well. I have some time now to look into it

from joken.

@bryanjos peek is already merged! As mentioned, that fixes this use case and we are good to close this one, right?

from joken.

Yes, peek has been merged and should resolve this issue. Closing now

from joken.