denoland / deno_kv_oauth

High-level OAuth 2.0 powered by Deno KV.
https://jsr.io/@deno/kv-oauth
MIT License
246 stars 25 forks source link

Expose methods to allow running cronjobs against session's expiry time #148

Closed adoublef closed 1 year ago

adoublef commented 1 year ago

I have enjoyed using this library for handling sessions. However I have noticed that at some point there will be sessions that don't get deleted from the the store and so overtime could grow and will need a way to delete them.

As I understand the Kv store API currently doesn't have and true TTL capabilities natively but I think it should be possible to still expose a way to search/filter sessions by expiry time.

The official docs state this:

- A range selector selects all keys that are lexicographically between the given start and end keys (including the start, and excluding the end). For example, the selector ["users", "a"], ["users", "n"] will select all keys that start with the prefix ["users"] and have a second key part that is lexicographically between a and n, such as ["users", "alice"], ["users", "bob"], and ["users", "mike"], but not ["users", "noa"] or ["users", "zoe"].

Based on this I think if a key included a timestamp (could be [PREFIX, timestamp]) then could expose a function like getExpiredSessions that may look like this:

const db = await Deno.openKv();
const entries = db.list({ start: [KV_PREFIX, 0], end: [KV_PREFIX, unix_timestamp] })
for await (const entry of entries) {
  entry.key; // [KV_PREFIX, expiry_as_unix]
  entry.value; // { sessionId, expiryAsUnix, ...rest }
  entry.versionstamp; // "00000000000000010000"
}

I'm unsure on the deleteExpiredSessions method but I think it would need to use transactions for consistency.

Be interested to open the dialogue to see other opinions on the matter.

I saw this bit of code and felt it may be a good starting point

iuioiua commented 1 year ago

Yeah, I've also been thinking about this issue. Taking advantage of kv.list(), I think, is a great approach! Let me do some tinkering.

adoublef commented 1 year ago

Yh amazing, I will keep a look out for that. another take I have is maybe hold an array of sessionIds that were issued in a day  as the value. then I think you could open up to using both list or get 

const db = await Deno.openKv();
const entries = db.list({ start: [KV_PREFIX, "01-01-2020"], end: [KV_PREFIX, "04-01-2022"] }) // array of arrays
for await (const entry of entries) {
  entry.key; // [KV_PREFIX, expiry_as_unix]
  entry.value; // [sessionId1, sessionId2, sessionIdN]
  entry.versionstamp; // "00000000000000010000"
}

const entry = db.get([KV_PREFIX, "01-01-2020"]) 
entry.key; // [KV_PREFIX, expiry_as_unix]
entry.value; // [sessionId1, sessionId2, sessionIdN]
entry.versionstamp; // "00000000000000010000"
iuioiua commented 1 year ago

So it seems that to strike the best balance between KV-space efficiency and user convenience, the most we can do is set OAuth session entries to expire in KV automatically and not site sessions. Keeping site sessions indefinitely allows us to use the refresh token to refresh the access token without requiring user interaction.

Check out #170. I'll merge that PR once KV entry expiry is supported on Deno Deploy.

adoublef commented 1 year ago

Ah I see, that looks good to me. Also reading through the PR, was nice to learn the recommended expiry time stated by RFC6749.

And I found the PR referencing the support for expiry so will keep a look for that too. Thank you

iuioiua commented 1 year ago

FYI, take a look at #224. It removes all database storage and retrieval of tokens. That means this module will automatically keep the database completely clean once implemented alongside OAuth session expiry.

adoublef commented 1 year ago

Oh I have just updated my deno project using this module so good timing. I will lookout for the breaking change. ty