scitokens / scitokens-cpp

A C++ implementation of the SciTokens library with a C library interface
Apache License 2.0
5 stars 22 forks source link

scitokens_internal: catch matching exception type after jwt-cpp update #125

Closed olifre closed 1 year ago

olifre commented 1 year ago

After updating the vendored jwt-cpp version in: a8c597775f44ad2fa0f7bde8d186aca9466757ab the exception type if a claim is not found has changed, breaking the "no kid claim" use case.

Adapt the code to catch the new exception type.

This essentially broke: https://github.com/scitokens/scitokens-cpp/pull/55

@djw8605 Can you think of a good test we could add to ensure this issue does not reappear?

djw8605 commented 1 year ago

I suppose you can just add a kid test to start with? For example, create a token without a kid, then test getting the kid.

djw8605 commented 1 year ago

did you accidently merge something else into this pull request? It's adding more than just the exception fix and test.

olifre commented 1 year ago

did you accidently merge something else into this pull request? It's adding more than just the exception fix and test.

Indeed, sorry, it should be fixed now — I have also taken the chance to rebase onto current master.

The changes now are:

djw8605 commented 1 year ago

Quick question, are we using the exception stuff for any other parts?

olifre commented 1 year ago

I think all other places either catch std::exception directly, i.e. are generic enough, and then just pass on the error message, or don't cross the boundary between jwt-cpp and scitokens-cpp. So while there are other uses of exceptions, these appear to be safe / more safe.

olifre commented 1 year ago

@djw8605 Many thanks for merging! In case it's not too much of a hassle, it would be nice to get a release with this fix — we've realized this hits us with tokens issued by the Unity IAM (e.g. used by B2Access, Helmholtz AAI etc.). Thanks in advance!

olifre commented 1 year ago

@djw8605 Sorry for bumping this, but since this issue is biting us (and is a regression), would it be possible to get a new release within the next weeks= This would help us to use scitokens-cpp with the Unity IAM.

Many thanks in advance!

djw8605 commented 1 year ago

Hi, yes I expect to see a release in epel in the next week. I’ll comment here when it’s in epel to let you know.

olifre commented 1 year ago

This is great to hear, many thanks in advance, and have a nice weekend! :smile:

djw8605 commented 1 year ago

Hi, I had some time tonight, so updates pushed out the door. They are in the bodhi update system, I expect them to be in epel-testing in ~24 hours, and in epel-release repos in ~7 days.

Please test the 1.0.2 version! You can easily grab it from epel-testing when that hits. EL7: https://bodhi.fedoraproject.org/updates/FEDORA-EPEL-2023-52740761bf EL8: https://bodhi.fedoraproject.org/updates/FEDORA-EPEL-2023-5586be8a7b EL9: https://bodhi.fedoraproject.org/updates/FEDORA-EPEL-2023-9948cdabde

olifre commented 1 year ago

@djw8605 Many thanks, I tested the new packages on el7 and el8 (RockyLinux) together with XRootD and everything works as expected, so I provided some karma on the packages :+1: . Thanks again!