Fix: Make exp claim required for back-channel logout#1441
Conversation
This reverts commit 7fcb03d. Signed-off-by: Spitap <dev@asdrip.fr>
Spitfireap
changed the title
| } | ||
|
|
||
| if (isset($logoutTokenPayload->exp) && $logoutTokenPayload->exp < $this->timeFactory->getTime()) { | ||
| if ($logoutTokenPayload->exp < $this->timeFactory->getTime()) { |
There was a problem hiding this comment.
| if ($logoutTokenPayload->exp < $this->timeFactory->getTime()) { | |
| if (($logoutTokenPayload->exp ?? 0) < $this->timeFactory->getTime()) { |
Let's handle the case where it is not defined to avoid unpredictable behaviours.
There was a problem hiding this comment.
Let's handle the case where it is not defined to avoid unpredictable behaviours.
Ok, wouldn't it be better to catch null values here ?
Or is it a failsafe for clarity and robustness against later development ?
There was a problem hiding this comment.
As you wish. My goal was to keep the code short.
There was a problem hiding this comment.
Must be me, but I fail to see any scenario where this might change the result of the comparison. I'd be glad if you had time to explain it :).
Anyway, done
There was a problem hiding this comment.
If the exp attribute is not defined then we use 0 so we know the comparison with getTime will fail so we actually require this attribute. Am I missing something?
Your initial version might have had the exact same behaviour but is less explicit.
There was a problem hiding this comment.
If the exp attribute is not defined then we use 0 so we know the comparison with getTime will fail so we actually require this attribute. Am I missing something?
Your initial version might have had the exact same behaviour but is less explicit.
For me it cannot be undefined because it is checked at line 892 ($requiredClaims) and it will return (l.901) before if missing (undefined).
Worse case scenario is that it is null or empty string but the int casting will make it 0.
Signed-off-by: Spitap <dev@asdrip.fr>
|
Hello there, We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process. Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6 Thank you for contributing to Nextcloud and we hope to hear from you soon! (If you believe you should not receive this message, you can add yourself to the blocklist.) |
2 participants
fixes #1436
This reverts commit 7fcb03d.
The exp claim in the logout token during back-channel logout was not checked due to LemonLDAP (third party IdP) not including it. LemonLDAP's users would have been unable to logout with back-channel.
Now that the fix is merged, the commit can be reverted.