Invalidate access token on Logout #9218
Conversation
- Implemented DeleteAccessToken in claimsCacher to remove tokens from the cache. - Added DeleteAccessToken method to AccessTokenFactory for database token removal. Signed-off-by: IvanChalukov <ichalukov@gmail.com>
Rename LoginHandler to SkyHandler, since it is responsible on only for Login implementation. Signed-off-by: IvanChalukov <ichalukov@gmail.com>
This way it will remote tokens not only from local flyrc file, but from server cache and Database as well. Signed-off-by: IvanChalukov <ichalukov@gmail.com>
|
Hi @taylorsilva, Could you please confirm if you're okay with the changes introduced by this PR (in Skyserver, ClaimsCacher, fly logout function, etc.)? Once I have your go-ahead, I’ll move forward with updating the tests. |
Signed-off-by: IvanChalukov <ichalukov@gmail.com>
…ust tests Signed-off-by: IvanChalukov <ichalukov@gmail.com>
Signed-off-by: IvanChalukov <ichalukov@gmail.com>
…a AccessTokenFactory Signed-off-by: IvanChalukov <ichalukov@gmail.com>
… missing auth token and error cases during token deletion Signed-off-by: IvanChalukov <ichalukov@gmail.com>
|
Great, thanks so much for the feedback! |
There was a problem hiding this comment.
Code review 👨💻:
I cannot speak on the code structure and conventions themselves, as I am not that in depth familiar with the whole codebase and common practices. In any case, they look way better than what I would have done 😅, so good job! What I could do though is play around with some testing.
Tests 🧪 :
I reproduced the behaviour from master and builded an image with your code for testing. What I ended up testing:
- Logout from API 🔒 :
- invalidates the token from the cookies as before - ✅
- invalidates the token from the db and cache - ✅
flylogout (took me 2-3 minutes to figure out that I had to rebuild fly and was ready to start debugging, so I reproduced it as well)- single target (
fly logout -t some-target) - ✅
Note: Also tested that single target logout did not impact any of my other targets 🎯
- multiple targets (
fly logout -a), in my case 3 targets - ✅
- single target (
Conclusion 🏁 :
I wasn't able to think of anything else to test, but will be more than happy to if someone can think of weird scenarios, as it was quite fast and easy to test this security enhancement. Very good job on the PR and thanks for making our favourite continuous thing-doer more secure 🔐 !
Signed-off-by: Taylor Silva <dev@taydev.net>
There was a problem hiding this comment.
LGTM - I pushed a commit with some nitpick stuff, didn't change anything else you did though.
I manually tested and verified that logging out from the web UI and fly cli, results in the token being removed from the database. Thanks for improving Concourse's security! 🎉
Changes proposed by this PR
Previously, the logout endpoint of the Concourse API only removed the
skymarshal_authandskymarshal_csrfcookies from the browser, without invalidating the access tokens issued to the user. As a result, users could still use their tokens to interact with the Concourse API even after logging out.With this implementation, tokens are now explicitly removed from both the local cache and the database. This ensures that after logging out, users can no longer use their tokens to authenticate API requests.
I am opening the PR as a draft since I want to confirm this changes in SkyServer and I need to add tests cases as well.
closes #9215
Notes to reviewer
I’ve renamed loginHandler to skyHandler to better reflect its broader responsibility, as it handles more than just login logic.
Details on how to reproduce the issue can be found in the linked issue.
Release Note