Implement server-side session invalidation using token_invalid_before#1593
Implement server-side session invalidation using token_invalid_before#1593Oykunle wants to merge 19 commits into
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
Great question. JWT stores the iat value in seconds, while the database timestamp (token_invalid_before) is stored with millisecond precision. Because of this difference in precision, a direct comparison at second-level granularity can lead to edge cases when issuance and invalidation occur within the same second. To address this, I converted the JWT iat value to milliseconds and performed the comparison at millisecond precision. This avoids edge cases without requiring additional libraries and keeps the comparison consistent with the database format. |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as outdated.
This comment was marked as outdated.
There was a problem hiding this comment.
@Oykunle First, I want to say I'm sorry this took so long. I communicated with you earlier about why and I appreciate your patience.
This is looking good. I merged in development to get rid of the DB startup issue from a recently merged PR as this sat for a while due to me. Testing after that did not see issues. I also went through all the previous comments and hid all the resolved ones so it would be easier to see what is left. I think all the old ones are done (thanks) and there are three new ones to consider. I don't think they are complex but let me know if you have questions/thoughts.
I know this is past the time you intended to work on OED so let me know if you want someone else to address the final comments.
I will want to do a careful read of the code once it looks done to be sure this security patch is all good.
| const meters = require('./routes/meters'); | ||
| const preferences = require('./routes/preferences'); | ||
| const login = require('./routes/login'); | ||
| const login = require('./routes/loginLogout'); |
There was a problem hiding this comment.
While I think I see the appeal of leaving the name as login so all the current routes work, I think making the actual server route file differ from the route name is confusing and prone to mistakes. The only other case of doing this is for readings where the actual name is used in the app.use(). I think this was an attempt to decouple the two names but it was not done in any consistent way. Something that can be fixed. For now, I think it should become loginLogout with all uses updated including app.use below.
| .send({ token }); | ||
|
|
||
| expect(beforeVerify).to.have.status(HTTP_CODES.OK); | ||
| expect(beforeVerify.body).to.have.property('success', true); |
There was a problem hiding this comment.
Above has a check for no message property. Should it be here (and I think there is another usage below)?
|
|
||
| expect(secondLogoutRes).to.have.status(HTTP_CODES.OK); | ||
| expect(secondLogoutRes.body).to.have.property('success', true); | ||
| expect(secondLogoutRes.body).to.have.property('message', 'Logout successful.'); |
There was a problem hiding this comment.
Here you check the message but not right above for the first attempt. Is there a reason?
Description
This PR implements server-side session invalidation for JWT authentication using a token_invalid_before timestamp stored on the users table.
Previously, issued JWTs remained valid until expiration, even after logout. This meant that a token could still be reused after the user had logged out.
This change adds a server-side invalidation mechanism by comparing the token’s iat (issued-at timestamp) against the user’s token_invalid_before value. Any token issued before that timestamp is rejected.
Type of change
Changes Made
Database
token_invalid_beforecolumn to theuserstable1.0.0 → 2.0.0as requestedAuthentication / Security
Logout
/api/login/logouttoken_invalid_beforeto invalidate all existing tokensCode Improvements
Tests
Added automated tests to validate session invalidation behavior:
All tests pass successfully in the test environment.
Security Impact
Limitations