-
Notifications
You must be signed in to change notification settings - Fork 89
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
feat!: use redis as backend for koa-session #2693
Changes from all commits
e1a0ef7
15e4fa6
ed94f55
1970190
a5908b0
7e5783d
926fe1a
05fcb02
dda0079
e2c92c1
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 | ||||
---|---|---|---|---|---|---|
|
@@ -50,6 +50,7 @@ import { AccessService } from './access/service' | |||||
import { AccessTokenService } from './accessToken/service' | ||||||
import { InteractionRoutes } from './interaction/routes' | ||||||
import { ApolloArmor } from '@escape.tech/graphql-armor' | ||||||
import { Redis } from 'ioredis' | ||||||
import { LoggingPlugin } from './graphql/plugin' | ||||||
|
||||||
export interface AppContextData extends DefaultContext { | ||||||
|
@@ -98,6 +99,7 @@ export interface AppServices { | |||||
accessTokenService: Promise<AccessTokenService> | ||||||
grantRoutes: Promise<GrantRoutes> | ||||||
interactionRoutes: Promise<InteractionRoutes> | ||||||
redis: Promise<Redis> | ||||||
} | ||||||
|
||||||
export type AppContainer = IocContract<AppServices> | ||||||
|
@@ -346,12 +348,31 @@ export class App { | |||||
|
||||||
koa.use(cors()) | ||||||
koa.keys = [this.config.cookieKey] | ||||||
|
||||||
const redis = await this.container.use('redis') | ||||||
const maxAgeMs = 60 * 1000 | ||||||
koa.use( | ||||||
session( | ||||||
{ | ||||||
key: 'sessionId', | ||||||
maxAge: 60 * 1000, | ||||||
signed: true | ||||||
maxAge: maxAgeMs, | ||||||
signed: true, | ||||||
store: { | ||||||
async get(key) { | ||||||
return await redis.hgetall(key) | ||||||
}, | ||||||
async set(key, session) { | ||||||
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. I see there is a third parameter 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. I assumed I would use that then purposefully did not after seeing it could be Does that make sense? Open to feedback here if anyone sees a better way. |
||||||
// Add a delay to cookie age to ensure redis record expires after cookie | ||||||
const expireInMs = maxAgeMs + 10 * 1000 | ||||||
Comment on lines
+365
to
+366
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. If I understand correctly, this is to possibly allow the "final" request with the to-be-expired cookie parameter to complete before deleting it in redis? 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. Yeah basically. could potentially be shorter I guess - just needs to be long enough to cover the time between finding the non-expired koa-session in the server and retrieving the redis record. no harm in having a little extra buffer though I figure. |
||||||
const op = redis.multi() | ||||||
op.hset(key, session) | ||||||
op.expire(key, expireInMs) | ||||||
await op.exec() | ||||||
}, | ||||||
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.
Suggested change
I think 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.
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. BTW, no need to do stringify on session object. Instead, you can just use hmset(key, session) 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. Updated to hmset as suggested except I used |
||||||
async destroy(key) { | ||||||
await redis.hdel(key) | ||||||
} | ||||||
} | ||||||
}, | ||||||
koa | ||||||
) | ||||||
|
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.
How does this correspond to what is stored inside redis? The key I see in redis is a UUID -> where does "sessionId" come into play?
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.
This is the original configuration and from what I can tell the key name does not correspond to anything in redis, it's just for the http cookie returned to the client:
The value there would be the UUID key you see in redis.