Session invalidation#1672
Conversation
…on invalidation changes
|
@Better-Code-Saul-E Thank you for this contribution. The PR description states the CLA has been signed by all contributors. However, OED records do not have one for you. Thus, I would like you to sign the CLA via the link in the description or let me know that you think OED records are off. Once this is done, this PR can be formally reviewed. Please let me know if you have any questions/thoughts. |
|
I have signed the CLA. |
huss
left a comment
There was a problem hiding this comment.
@Better-Code-Saul-E Thank you for this contribution to OED. Your changes mostly addressed my previous comments. I've made one comment about a remaining item for you to consider. I have not carefully retested but will once all the changes are done. I see nothing you changed that should cause an issue but want to be careful. Please let me know if anything is not clear or you have thoughts/questions.
|
@huss I made the change. The manual login and automated tests worked! Out of curiosity, besides consistency, is there a reason 'loginLogout' was preferred over something like 'auth' for the endpoint? |
Mostly it is historical since it was login so loginLogout seemed natural. auth isn't bad but I recall it may be used elsewhere as a term in OED to authenticate a user on the server when a request is made. |
There was a problem hiding this comment.
@Better-Code-Saul-E Thank you for addressing my previous comment. I have made two new ones to consider. Also, I forgot to mention last time that this PR is out-of-date with development (probably because it continues a previous, older PR). Now it has a merge conflict. Could you please fix this up so I can more fully test and merge the PR. Please let me know if you have any thoughts/questions.
|
@huss I resolved the merge conflict, removed devcontainer-lock.json, and fixed the duplicate LOGIN_ENDPOINT declaration in authenticatorParamsTest.js. |
huss
left a comment
There was a problem hiding this comment.
@Better-Code-Saul-E Thank you for the update. Review and testing found it is fine. However, the CI tests failed on GitHub. Unfortunately, the error message does not make the problem clear. I reran on my local machine and found it passed all tests. I reran them on GitHub and they failed again. I'm investigating as I have time and hope this isn't directly related to this PR. If anyone has thoughts/insights then they are more than welcome since I'm not an expert in this.
|
@huss I fixed the issue with the CI test failing. I added a missing license header in one of the sql files. The test passed. Could you verify on your machine when you get a chance? |
Thank you for figuring that out. I should have tried the check and not just the test command. I was doing too many things and it slipped my mind. They pass on my machine and GitHub so all is good. |
huss
left a comment
There was a problem hiding this comment.
@Better-Code-Saul-E Review and testing found all is ready. Congratulations on your first accepted contribution to OED.
Description
Continue the work started in #1593 by @Oykunle to implement server-side session invalidation for JWT authentication. This PR addresses the three outstanding review coments from #1593 that were left unresolved when the original author became unavailable to continue the work. See #1652.
Authors: @Oykunle (original implementation), @Better-Code-Saul-E (review comment fixes)
Fixes #1652
Type of change
Checklist
Limitations