Closed mikecirioli closed 1 month ago
Attention: Patch coverage is 33.33333%
with 2 lines
in your changes missing coverage. Please review.
Project coverage is 71.44%. Comparing base (
b7205f1
) to head (ad6b064
). Report is 3 commits behind head on master.
Files | Patch % | Lines |
---|---|---|
...va/org/jenkinsci/plugins/oic/OicSecurityRealm.java | 0.00% | 2 Missing :warning: |
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
@michael-doubez i realize that i did not add a label to this PR before it was merged, and i don't have permissions to trigger a release - is that something you can do?
@mikecirioli my mistake - it didn't check
to reproduce:
Although the proposed PR fixes the issue AFAICT, I am not 100% confident it is the correct fix. This issue first appears in release 4.297.vcddb_d8a_e4694, but i have not been able to identify exactly what i believe changed to cause this behavior. My theory is that because the jenkins web session is still valid, it allows the request, which is then flagged as invalid because the OIC credentials have expired, kicking the whole loop off again. Hopefully you can confirm or deny this @krezovic
I also believe that the clock skew was mistakenly being subtracted from the credentials "expires in X seconds" calculation - my understanding was the clock skew should add an additional "buffer" to the lifetime of the credentials in order to accommodate slight variances in different clocks.
Testing done
Lots of manual testing.... I've also fixed 2 tests that i believe cover the changes made in this PR:
testRefreshTokenAndTokenExpiration_withoutRefreshToken()
The test was originally usingJenkinsRule.WebClient
to check for a302
respoonse when theOicCredentials
are expired. NormallyWebClient
would follow redirects, and the test was only passing with the expectedBlocked - 302
due to the redirect loop that was occurring. Fixing that caused this test to fail, so i've replaced that bit of test code with a javaHttpClient
set to not follow redirects.the expire helper method was using the wrong number of
ms
to force the credentials to be expired. This apparently worked previously because the clock skew was actually being subtracted from the credential lifetime (instead of augmenting it). Since all we care about is ensuring the credentials appear expired I simply set the creation instant to1
(ie. the first millisecond of time ever).Submitter checklist