zitadel / oidc

Easy to use OpenID Connect client and server library written for Go and certified by the OpenID Foundation
https://zitadel.com
Apache License 2.0
1.39k stars 147 forks source link

fix(deps): update go-jose to new updated repo due to migration #630

Closed andrewpmartinez closed 2 months ago

andrewpmartinez commented 3 months ago

Definition of Ready

fforootd commented 3 months ago

Relates to https://discord.com/channels/927474939156643850/927866013545025566/1269022628191010929

eliobischof commented 2 months ago

@muhlemmer I see you changed square/go-jose to go-jose/go-jose in v3. Do you see any red flags if we change it also for v2?

Is it possible that the tests fails because uf the change to go-jose?

muhlemmer commented 2 months ago

@eliobischof I initially thought the same. But checking the error logs:

Error Trace:    /home/runner/work/oidc/oidc/pkg/op/op_test.go:391
2024-08-02T20:50:10.0979418Z            Error:          "{\"access_token\":\"7YRjDyaWUAY4Iiqnlp3HhDPJTax840IQ1MKcHHvCLjNhNKBEdTBKt2bJf4sMZWAHsGwNy0UAtoU\",\"issued_token_type\":\"urn:ietf:params:oauth:token-type:refresh_token\",\"token_type\":\"Bearer\",\"expires_in\":299,\"scope\":\"openid offline_access\",\"refresh_token\":\"9b6a1d27-e969-4ead-b4b8-c579b9db2c73\"}\n" does not contain "\",\"issued_token_type\":\"urn:ietf:params:oauth:token-type:refresh_token\",\"token_type\":\"Bearer\",\"expires_in\":300,\"scope\":\"openid offline_access\",\"refresh_token\":\""`

We do a very naive Contains check here on the JSON string. It seems the failed because in the op_test.go file @andrewpmartinez changed the expected expires_in from 299 to 300. IMO this change needs to be reverted.

As we round expires_in to seconds, it might be that rounding flips the second to either position, depending on how fast the tests run. It might be that on the local system of @andrewpmartinezc 300 is the correct value, but for me locally and in the pipeline 299 always worked. This might be improved, but as the test is a regression test on all routes, there is not one specific type we can unmarshal into and do more advanced assertions.

muhlemmer commented 2 months ago

@andrewpmartinez did you notice my review? Do you wish to proceed with this PR?

andrewpmartinez commented 2 months ago

@andrewpmartinez did you notice my review? Do you wish to proceed with this PR?

Yes, I did. I have been working on other tasks, but this is still relevant to me. I will try to address it this week.

andrewpmartinez commented 2 months ago

Done. Also, it was rebased to the top of 2.12.x for merge conflicts.

Running the tests locally do infact fail for me w/ rounding errors of 299 vs 300. I can't tell if the tests will pass in the CI pipeline till they run remotely. Fingers crossed.

github-actions[bot] commented 2 months ago

:tada: This PR is included in version 2.12.2 :tada:

The release is available on GitHub release

Your semantic-release bot :package::rocket: