jitsi / jibri

Jitsi BRoadcasting Infrastructure
Apache License 2.0
607 stars 313 forks source link

fix: webhooks JWT recreation after expiry #495

Closed cristianbotiza closed 1 year ago

cristianbotiza commented 1 year ago

Previous fix has slightly refactored the handling of the JWT header, thereby introducing a regression. https://community.jitsi.org/t/jibri-is-not-sending-webhook-authorization-header/117184/

When passing the defaultRequest closure we must ensure we supply a block that forces re-evaluation of the jwt property, subsequently executing the expiry check in RefreshingProperty. As such, $jwt is valid, while $it is not, because it is resolved only once, when the Client is setup. Whatever value is computed then, is seen as a constant when passed to defaultRequest. $jwt, on the other hand, forces re-evaluation every time the default request block is executed, i.e. for every webhook request.

jitsi-jenkins commented 1 year ago

Hi, thanks for your contribution! If you haven't already done so, could you please make sure you sign our CLA (https://jitsi.org/icla for individuals and https://jitsi.org/ccla for corporations)? We would unfortunately be unable to merge your patch unless we have that piece :(.

codecov[bot] commented 1 year ago

Codecov Report

Merging #495 (eeec10c) into master (7ab9aa2) will not change coverage. The diff coverage is 0.00%.

Additional details and impacted files [![Impacted file tree graph](https://codecov.io/gh/jitsi/jibri/pull/495/graphs/tree.svg?width=650&height=150&src=pr&token=P6jrfpYsWM&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=jitsi)](https://codecov.io/gh/jitsi/jibri/pull/495?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=jitsi) ```diff @@ Coverage Diff @@ ## master #495 +/- ## ========================================= Coverage 48.09% 48.09% Complexity 174 174 ========================================= Files 73 73 Lines 2104 2104 Branches 199 199 ========================================= Hits 1012 1012 Misses 1022 1022 Partials 70 70 ``` | [Impacted Files](https://codecov.io/gh/jitsi/jibri/pull/495?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=jitsi) | Coverage Δ | | |---|---|---| | [...otlin/org/jitsi/jibri/webhooks/v1/WebhookClient.kt](https://codecov.io/gh/jitsi/jibri/pull/495/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=jitsi#diff-c3JjL21haW4va290bGluL29yZy9qaXRzaS9qaWJyaS93ZWJob29rcy92MS9XZWJob29rQ2xpZW50Lmt0) | `40.47% <0.00%> (ø)` | | ------ [Continue to review full report at Codecov](https://codecov.io/gh/jitsi/jibri/pull/495?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=jitsi). > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=jitsi) > `Δ = absolute (impact)`, `ø = not affected`, `? = missing data` > Powered by [Codecov](https://codecov.io/gh/jitsi/jibri/pull/495?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=jitsi). Last update [7ab9aa2...eeec10c](https://codecov.io/gh/jitsi/jibri/pull/495?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=jitsi). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=jitsi).
bbaldino commented 1 year ago

Nice catch, that's a sneaky one.