Bug: Refresh tokens not deleted when user logs out #72
Description
Description
When a user logs out, the destroySession method is called which deletes the access token, but it doesn't delete the associated refresh token. This happens because the refresh token is stored in a separate table (AuthRefreshTokensTable) without a direct reference to the access token being deleted.
This issue affects all session manager implementations in the codebase:
JoseJwtSessionManager(inlibs/auth/src/session/jose-jwt.ts)JwtSessionManager(inlibs/auth/src/session/jwt.ts)BasicSessionManager(inlibs/auth/src/session/session.ts)
Currently, the destroySession method only deletes expired tokens from the refresh tokens table:
async destroySession(token: string): Promise<void> {
console.log('Destroying session for token:', token);
await this.knex(AuthAccessTokensTable).where({ token }).delete();
// Also clean up any expired tokens
await this.knex(AuthAccessTokensTable)
.where('expires_at', '<=', this.knex.fn.now())
.delete();
await this.knex(AuthRefreshTokensTable)
.where('expires_at', '<=', this.knex.fn.now())
.delete();
console.log('Session destroyed successfully.');
return;
}However, if the refresh token hasn't expired yet, it remains in the database even after logout, which could potentially be a security risk.
Expected Behavior
When a user logs out, both the access token and its associated refresh token should be deleted from the database.
Current Behavior
Only the access token is deleted, while the refresh token remains in the database until it expires.
Suggested Fix
Store a reference to the access token in the refresh token table when creating the refresh token. This would allow us to find and delete the associated refresh token when an access token is deleted.
For example, modify the schema to add an access_token column to the AuthRefreshTokensTable:
await knex.schema.createTable(AuthRefreshTokensTable, (table) => {
table.uuid('id').primary().defaultTo(knex.fn.uuid());
table.uuid('user_id').notNullable();
table.string('token').notNullable().unique();
table.string('access_token').nullable(); // Reference to the associated access token
table.timestamp('expires_at').notNullable();
table.timestamps(true, true);
// ... other columns and indexes
});Then, when creating a session, store the access token with the refresh token:
await this.knex(AuthRefreshTokensTable).insert({
token: refreshToken,
user_id: user.id,
access_token: accessToken, // Store the access token reference
expires_at: timeStringToDate(this.config.sessionSettings.refreshTokenTTL),
});Finally, update the destroySession method to delete the associated refresh token:
async destroySession(token: string): Promise<void> {
console.log('Destroying session for token:', token);
// Find and delete any refresh tokens associated with this access token
await this.knex(AuthRefreshTokensTable)
.where({ access_token: token })
.delete();
// Delete the access token
await this.knex(AuthAccessTokensTable).where({ token }).delete();
// Also clean up any expired tokens
await this.knex(AuthAccessTokensTable)
.where('expires_at', '<=', this.knex.fn.now())
.delete();
await this.knex(AuthRefreshTokensTable)
.where('expires_at', '<=', this.knex.fn.now())
.delete();
console.log('Session destroyed successfully.');
return;
}This fix would need to be implemented in all session manager classes that implement the SessionManager interface.
Impact
This issue could potentially allow an attacker to use a refresh token to obtain a new access token even after a user has logged out, if the refresh token hasn't expired yet. Fixing this would improve the security of the authentication system.