Skip to content
This repository has been archived by the owner on Dec 12, 2023. It is now read-only.

Refactor session handling #48

Open
wants to merge 26 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 20 commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
5138f30
Add rolling option
interpretor Nov 24, 2022
a34d5d8
Add saveUninitialized option
interpretor Nov 25, 2022
4dc1d99
Update configuration example
interpretor Nov 28, 2022
1a261b7
Avoid modifying input arguments
interpretor Nov 28, 2022
99ac27f
Update configuration example
interpretor Nov 28, 2022
efc7dc2
Refactor saveUninitialized
interpretor Nov 28, 2022
3629aa8
Add resave option
interpretor Nov 28, 2022
8fbed4a
Update configuration example
interpretor Nov 28, 2022
1cb0215
Use types for session options
interpretor Nov 28, 2022
dbd6bf4
Use types for session options
interpretor Nov 28, 2022
46d9d1f
Use types for session options
interpretor Nov 28, 2022
59b3b09
Merge branch 'add-saveuninitialized-option' into dev
interpretor Nov 28, 2022
79eaf84
Merge branch 'add-resave-option' into dev
interpretor Nov 28, 2022
9d39b4d
Refactor handling of modified sessions
interpretor Nov 29, 2022
293ad80
Remove unnecessary interface declaration
interpretor Nov 29, 2022
50cd690
Merge branch 'main' into refactor-session-handling
interpretor Nov 30, 2022
0621975
Merge branch 'main' into refactor-session-handling
interpretor Dec 1, 2022
477769f
Merge branch 'main' into refactor-session-handling
interpretor Dec 5, 2022
b98f67e
Merge branch 'main' into refactor-session-handling
interpretor Dec 5, 2022
8238d43
Rename variable
interpretor Dec 5, 2022
7b55c78
Add better explanation to saveUninitialized
interpretor Dec 10, 2022
90d68bc
Provide better naming
interpretor Dec 12, 2022
65a5f34
Merge branch 'refactor-session-handling' of github.com:interpretor/nu…
interpretor Dec 12, 2022
0b5b25a
Provide better explanation for resave
interpretor Dec 12, 2022
0418846
Fix indentation and comma
interpretor Dec 12, 2022
dd4b7b3
Merge branch 'main' into refactor-session-handling
interpretor Feb 4, 2023
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -226,7 +226,11 @@ Here's what the full _default_ module configuration looks like:
// Sessions aren't pinned to the user's IP address
ipPinning: false,
// Expiration of the sessions are not reset to the original expiryInSeconds on every request
rolling: false
rolling: false,
// Uninitialized, resp. unmodified sessions are saved to the store, so session cookies are set at the first response
interpretor marked this conversation as resolved.
Show resolved Hide resolved
saveUninitialized: true,
// Sessions are saved to the store, even if they were never modified during the request
resave: true

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Same here, you need to provide better explanation
  • Also expressjs/session recommends false by default

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

expressjs/session recommends false, but the current behavior of @sidebase/nuxt-session is true, so I've chosen to not change the default behavior. It is also recommended to set resave to false.

We can change it if you want.

},
api: {
// The API is enabled
Expand Down
7 changes: 3 additions & 4 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@
"argon2": "^0.30.2",
"dayjs": "^1.11.6",
"defu": "^6.1.0",
"fast-deep-equal": "^3.1.3",
"h3": "^1.0.1",
"unstorage": "^1.0.1"
},
Expand Down
4 changes: 3 additions & 1 deletion src/module.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,9 @@ const defaults: FilledModuleOptions = {
},
domain: false,
ipPinning: false as boolean|SessionIpPinningOptions,
rolling: false
rolling: false,
saveUninitialized: true,
resave: true
},
api: {
isEnabled: true,
Expand Down
52 changes: 31 additions & 21 deletions src/runtime/server/middleware/session/index.ts
Original file line number Diff line number Diff line change
@@ -1,10 +1,12 @@
import { deleteCookie, eventHandler, H3Event, parseCookies, setCookie } from 'h3'
import { nanoid } from 'nanoid'
import dayjs from 'dayjs'
import equal from 'fast-deep-equal'
import { SameSiteOptions, Session, SessionOptions } from '../../../../types'
import { dropStorageSession, getStorageSession, setStorageSession } from './storage'
import { processSessionIp, getHashedIpAddress } from './ipPinning'
import { SessionExpired } from './exceptions'
import { resEndProxy } from './resEndProxy'
import { useRuntimeConfig } from '#imports'

const SESSION_COOKIE_NAME = 'sessionId'
Expand Down Expand Up @@ -66,25 +68,24 @@ export const deleteSession = async (event: H3Event) => {
}

const newSession = async (event: H3Event) => {
const runtimeConfig = useRuntimeConfig()
const sessionOptions = runtimeConfig.session.session as SessionOptions
const sessionOptions = useRuntimeConfig().session.session as SessionOptions
const now = new Date()

// (Re-)Set cookie
const sessionId = nanoid(sessionOptions.idLength)
safeSetCookie(event, SESSION_COOKIE_NAME, sessionId, now)

// Store session data in storage
const session: Session = {
id: sessionId,
id: nanoid(sessionOptions.idLength),
createdAt: now,
ip: sessionOptions.ipPinning ? await getHashedIpAddress(event) : undefined
}
await setStorageSession(sessionId, session)

return session
}

const setSession = async (session: Session, event: H3Event) => {
safeSetCookie(event, SESSION_COOKIE_NAME, session.id, session.createdAt)
await setStorageSession(session.id, session)
return session
}

const getSession = async (event: H3Event): Promise<null | Session> => {
// 1. Does the sessionId cookie exist on the request?
const existingSessionId = getCurrentSessionId(event)
Expand All @@ -98,8 +99,7 @@ const getSession = async (event: H3Event): Promise<null | Session> => {
return null
}

const runtimeConfig = useRuntimeConfig()
const sessionOptions = runtimeConfig.session.session as SessionOptions
const sessionOptions = useRuntimeConfig().session.session as SessionOptions
const sessionExpiryInSeconds = sessionOptions.expiryInSeconds

try {
Expand Down Expand Up @@ -143,21 +143,31 @@ const ensureSession = async (event: H3Event) => {

event.context.sessionId = session.id
event.context.session = session

return session
}

export default eventHandler(async (event: H3Event) => {
const sessionOptions = useRuntimeConfig().session.session as SessionOptions

// 1. Ensure that a session is present by either loading or creating one
await ensureSession(event)

// 2. Setup a hook that saves any changed made to the session by the subsequent endpoints & middlewares
event.res.on('finish', async () => {
// Session id may not exist if session was deleted
const session = await getSession(event)
if (!session) {
return
const session = await ensureSession(event)
// 2. Save current state of the session
const oldSession = { ...session }

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest to move copying logic this into the getSession so ensureSession either creates a new one or returns a copy of the old

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moving the copying logic to getSession would not work, as the oldSession object has to be different from the one saved to the event context. We could return a copy of the session after saving a reference to the event context in ensureSession.


// 3. Setup a hook that saves any changed made to the session by the subsequent endpoints & middlewares
resEndProxy(event.node.res, async () => {
const newSession = event.context.session as Session
const storedSession = await getSession(event)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's get confusing here, as you got too many session names: session, oldSession, newSession and storedSession (where oldSession is in actuality can be newly created or existing one)

Please propose a better naming for those


if (!storedSession) {
// Save a new session if saveUninitialized is true, or if the session has been modified
if (sessionOptions.saveUninitialized || !equal(newSession, oldSession)) {
await setSession(newSession, event)
}
// Update the session in the storage if resave is true, or if the stored session has been modified
} else if (sessionOptions.resave || !equal(newSession, storedSession)) {
await setStorageSession(storedSession.id, newSession)
}

await setStorageSession(session.id, event.context.session)
})
})
14 changes: 14 additions & 0 deletions src/runtime/server/middleware/session/resEndProxy.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
import type { ServerResponse } from 'node:http'

type MiddleWare = () => Promise<void>

// Proxy res.end() to get a callback at the end of all event handlers
export const resEndProxy = (res: ServerResponse, middleWare: MiddleWare) => {
const end = res.end

// @ts-ignore Replacing res.end() will lead to type checking error
res.end = async (chunk: any, encoding: BufferEncoding) => {
Comment on lines +9 to +10
Copy link

@valiafetisov valiafetisov Dec 6, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overwriting standard node js methods is obviously not recommended. Is there a better way doing that?

  • What is the exact type error that is thrown here?
  • Also, I'm not sure if res.end can be async, since it is expected to return this according to the docs I don't think it's being awaited. Was it at least tested with long timeouts?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems that there is no alternative, as already mentioned in #41. There is no hook at the end of all event handlers, and no event that fires just before sending the response.

  • res.end returns always a ServerResponse, and here it returns a Promise<ServerResponse>. Also there are multiple overloads, and res.end also accepts a callback. In express session this leads to problems, so they decided to ditch the callback, as it is very rarely used.
  • The types don't allow res.end being async, but it works perfectly. I tested with long timeouts with various conditions, so it must be awaited, but I didn't check it in the node source code.

await middleWare()
return end.call(res, chunk, encoding) as ServerResponse

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you need to typecast it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

res.end.call() returns any, not ServerResponse

}
}
17 changes: 16 additions & 1 deletion src/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,22 @@ export interface SessionOptions {
* @example true
* @type boolean
*/
rolling: boolean
rolling: boolean,
/**
* Forces a session that is "uninitialized" to be saved to the store. A session is uninitialized when it is new but not modified.
* Choosing false is useful for implementing login sessions, reducing server storage usage, or complying with laws that require permission before setting a cookie.
* @default true
* @example false
* @type boolean
*/
saveUninitialized: boolean,
/**
* Forces the session to be saved back to the session store, even if the session was never modified during the request.
* @default true
* @example false
* @type boolean
*/
resave: boolean
}

export interface ApiOptions {
Expand Down