-
Notifications
You must be signed in to change notification settings - Fork 9
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Optimize session deletion, recreation, and persistence #10
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -100,18 +100,52 @@ export function sessionMiddleware(options: SessionOptions) { | |
|
||
await next() | ||
|
||
if (c.get('session_key_rotation') === true && !(store instanceof CookieStore)) { | ||
await store.deleteSession(sid) | ||
sid = await nanoid(21) | ||
await store.createSession(sid, session.getCache()) | ||
|
||
setCookie(c, sessionCookieName, encryptionKey ? await encrypt(encryptionKey, sid) : sid, cookieOptions) | ||
const shouldDelete = session.getCache()._delete; | ||
const shouldRotateSessionKey = c.get("session_key_rotation") === true; | ||
const storeIsCookieStore = store instanceof CookieStore; | ||
Comment on lines
+103
to
+105
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The original conditionals were logical but a bit challenging for me to grok. I'm a big fan of abstracting conditionals into well named variables. I personally lean towards verbosity over succinctness to make the intention really clear. That being said, happy to make changes if there is different stylistic preference on how to write these conditionals. |
||
|
||
if (shouldDelete) { | ||
store instanceof CookieStore | ||
? await store.deleteSession(c) | ||
: await store.deleteSession(sid); | ||
} | ||
|
||
store instanceof CookieStore ? await store.persistSessionData(c, session.getCache()) : await store.persistSessionData(sid, session.getCache()) | ||
/* | ||
* Only update session data if we didn't just delete it. | ||
* If session key rotation is enabled and the store is not a CookieStore, | ||
* we need to roate the session key by deleting the old session and creating a new one. | ||
*/ | ||
const shouldRecreateSessionForNonCookieStore = | ||
!shouldDelete && | ||
!storeIsCookieStore && | ||
shouldRotateSessionKey; | ||
|
||
if (shouldRecreateSessionForNonCookieStore) { | ||
await store.deleteSession(sid); | ||
sid = await nanoid(21); | ||
await store.createSession(sid, session.getCache()); | ||
|
||
setCookie( | ||
c, | ||
sessionCookieName, | ||
encryptionKey ? await encrypt(encryptionKey, sid) : sid, | ||
cookieOptions | ||
); | ||
} | ||
|
||
if (session.getCache()._delete) { | ||
store instanceof CookieStore ? await store.deleteSession(c) : await store.deleteSession(sid) | ||
/* | ||
* We skip session data persistence if it was just deleted. | ||
* Only persist if we didn't just rotate the session key, | ||
* or the store is a CookieStore (which does not have its session key rotated) | ||
*/ | ||
const shouldPersistSession = | ||
!shouldDelete && | ||
(!shouldRotateSessionKey || storeIsCookieStore); | ||
|
||
if (shouldPersistSession) { | ||
store instanceof CookieStore | ||
? await store.persistSessionData(c, session.getCache()) | ||
: await store.persistSessionData(sid, session.getCache()); | ||
} | ||
} | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
None of these changes are intended to alter the functionality, just re-organize the code so we aren't unnecessarily updating the session store if we don't need to.