feat(auth): add pluggable auth state store#22
Conversation
|
Hi, I have raised this PR for issue #15. I’ve added a pluggable auth state store with memory and database-backed adapters, migrated the existing in-memory auth state usage, and added tests for expiry, revocation, cleanup, and multi-instance behavior. Checks passed locally with: npm run verify
npm testTest result: Test Suites: 1 skipped, 44 passed, 44 of 45 total
Tests: 9 skipped, 278 passed, 287 totalPlease review it when you get a chance. Thank you! |
|
Hi @Kawshikk-Shrii, thanks for the PR. The overall approach is good and focused for issue #15. I checked the implementation and also ran the tests locally: npm run verify
npm test -- --runInBand
npm run typecheckAll of these passed. However, I found one important integration issue before this can be approved/merged.
Current code: // core/auth.js
this.store = options.store || createAuthStore();
// core/enterpriseAuth.js
this.store = config.store || createAuthStore();Because of that, this code still selects the memory store: new AuthManager({ knex })
new EnterpriseAuth({ knex })Expected behavior: passing Please update this to pass the options/config into the factory, for example: // core/auth.js
this.store = options.store || createAuthStore(options);
// core/enterpriseAuth.js
this.store = config.store || createAuthStore(config);Also please add/adjust tests to prove that:
After this fix, I can review again. If everything still passes, we can approve, add the GSSoC labels, and merge. |
|
Hi @Kawshikk-Shrii, please confirm whether you are still interested in continuing work on this PR/issue. The remaining requested fix is the If you are not interested or not available to continue, please let me know so I can assign the issue to another contributor. |
|
Yeah. I am interested and want to contribute and continue to work in this PR. |
|
Hi @Kawshikk-Shrii, your PR now has merge conflicts in:
This happened because another security PR was merged into main and it changed the same auth files. Please sync your branch with the latest main and resolve the conflicts. While resolving, make sure you keep both changes:
After resolving conflicts, push to the same PR branch. Once CI passes, I will re-review. |
Summary
This PR introduces a production-ready, pluggable auth state store for handling refresh tokens, password reset tokens, email verification tokens, enterprise sessions, MFA data, OAuth state, and login attempt tracking.
Previously, auth-related state was stored directly in in-memory maps inside
core/auth.jsandcore/enterpriseAuth.js. That approach works for local development but is not reliable in production because state is lost on server restart and cannot be shared across multiple running instances.This change moves auth state management behind a store interface while keeping an in-memory adapter for development and test environments.
Closes #15
Changes Made
core/authStore/module.MemoryAuthStorefor development and test usage.DatabaseAuthStorefor persistent production usage.unref()to avoid Jest open-handle warnings.Testing
The following checks were run successfully:
npm run verify npm testTest result:
Notes