Add Better-Auth plugin for ObjectOS authentication#132
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This pull request implements @objectos/plugin-better-auth, a standalone authentication plugin for ObjectOS based on the Better-Auth library. The plugin aims to provide email/password authentication, organization/team management, and RBAC with multi-database support, conforming to the @objectstack/spec v0.6.0 plugin lifecycle protocol.
Changes:
- Adds a new plugin package at
packages/plugins/better-auth/with authentication capabilities using the Better-Auth library - Implements full plugin lifecycle hooks (onInstall, onEnable, onDisable, onUninstall) following the @objectstack/spec protocol
- Provides multi-database support for PostgreSQL, MongoDB, and SQLite with automatic driver selection
- Includes comprehensive documentation (README, INTEGRATION guide, CHANGELOG, usage examples)
Reviewed changes
Copilot reviewed 12 out of 13 changed files in this pull request and generated 15 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/plugins/better-auth/src/auth-client.ts | Better-Auth client configuration with database connection management and user role initialization |
| packages/plugins/better-auth/src/plugin.ts | Plugin lifecycle implementation with route registration and event emission |
| packages/plugins/better-auth/src/index.ts | Public API exports for the plugin |
| packages/plugins/better-auth/package.json | Package configuration with dependencies for all supported database drivers |
| packages/plugins/better-auth/tsconfig.json | TypeScript configuration for the plugin |
| packages/plugins/better-auth/jest.config.js | Jest test configuration |
| packages/plugins/better-auth/test/plugin.test.ts | Basic structural tests validating exports and manifest |
| packages/plugins/better-auth/examples/usage.ts | Usage examples demonstrating plugin integration |
| packages/plugins/better-auth/README.md | Plugin documentation |
| packages/plugins/better-auth/INTEGRATION.md | Integration guide for ObjectOS Server |
| packages/plugins/better-auth/CHANGELOG.md | Version history |
| packages/plugins/better-auth/SUMMARY.md | Implementation summary |
| pnpm-lock.yaml | Dependency lockfile updates for new plugin dependencies |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
| app.router.all('/api/auth/*', async (req: any, res: any) => { | ||
| // Pass the request to Better-Auth handler | ||
| return handler(req, res); | ||
| }); | ||
| context.logger.info('[Better-Auth Plugin] Routes registered at /api/auth/*'); |
There was a problem hiding this comment.
Route registration uses wildcard pattern that may conflict with other plugins. Registering all routes under /api/auth/* with router.all could prevent other plugins from registering more specific auth-related routes. Consider using a more specific mount point or documenting this limitation in the plugin manifest to prevent conflicts.
| app.router.all('/api/auth/*', async (req: any, res: any) => { | |
| // Pass the request to Better-Auth handler | |
| return handler(req, res); | |
| }); | |
| context.logger.info('[Better-Auth Plugin] Routes registered at /api/auth/*'); | |
| app.router.all('/api/auth/better-auth/*', async (req: any, res: any) => { | |
| // Pass the request to Better-Auth handler | |
| return handler(req, res); | |
| }); | |
| context.logger.info('[Better-Auth Plugin] Routes registered at /api/auth/better-auth/*'); |
| if (app.eventBus && typeof app.eventBus.emit === 'function') { | ||
| app.eventBus.emit('plugin.enabled', { | ||
| pluginId: BetterAuthManifest.id, | ||
| timestamp: new Date().toISOString() | ||
| }); | ||
| } |
There was a problem hiding this comment.
Inconsistent usage of eventBus.emit. The code checks for app.eventBus.emit on lines 110 and 140, but according to the PluginContextData interface defined in the kernel, the eventBus is accessible via context.events.emit, not context.app.eventBus.emit. This code will not work as intended because it's checking for properties on the wrong object. Use context.events.emit instead of accessing eventBus through the app property.
| const sqlite3Import = await import("better-sqlite3"); | ||
| // Handle both ESM/Interop (default export) and CJS (direct export) | ||
| const Database = (sqlite3Import.default || sqlite3Import) as any; | ||
| const filename = (dbUrl?.replace('sqlite:', '')) || 'objectos.db'; |
There was a problem hiding this comment.
Inconsistent SQLite file path handling compared to existing implementation. In the plugin version (line 47), the code uses dbUrl?.replace('sqlite:', '') which would leave an empty string if dbUrl is just 'sqlite:', while the server implementation (auth.client.ts:30) uses a conditional check with fallback. The plugin should use the same logic: (dbUrl && dbUrl.replace('sqlite:', '')) ? (dbUrl && dbUrl.replace('sqlite:', '')) : 'objectos.db' to ensure consistent behavior.
| const filename = (dbUrl?.replace('sqlite:', '')) || 'objectos.db'; | |
| const filename = (dbUrl && dbUrl.replace('sqlite:', '')) | |
| ? (dbUrl && dbUrl.replace('sqlite:', '')) | |
| : 'objectos.db'; |
| console.log('[Better-Auth Plugin] Initialized successfully'); | ||
| return authInstance; | ||
| } catch (e: any) { | ||
| console.error("[Better-Auth Plugin] Initialization Error:", e); |
There was a problem hiding this comment.
Console logging prefix inconsistency within the same module. The code uses both "[Better-Auth Plugin]" (lines 48, 102, 111, 156, 180) and plain console.error without prefix (line 159). For consistency and better log filtering, all console.log and console.error calls should use the same "[Better-Auth Plugin]" prefix format.
| let authInstance: any; | ||
| let dbConnection: any; // Store database connection for cleanup |
There was a problem hiding this comment.
Module-level singleton state without proper encapsulation creates potential issues in testing and concurrent usage scenarios. The authInstance and dbConnection variables are shared across all calls to getBetterAuth, which could lead to unexpected behavior if the function is called with different configurations. Consider encapsulating this state in a class or using a Map keyed by configuration to support multiple auth instances.
| // Emit plugin disabled event | ||
| const app = context.app as ExtendedAppContext; | ||
| if (app.eventBus && typeof app.eventBus.emit === 'function') { | ||
| app.eventBus.emit('plugin.disabled', { | ||
| pluginId: BetterAuthManifest.id, | ||
| timestamp: new Date().toISOString() | ||
| }); | ||
| } | ||
|
|
There was a problem hiding this comment.
Inconsistent usage of eventBus.emit. The code checks for app.eventBus.emit on lines 140-145, but according to the PluginContextData interface defined in the kernel, the eventBus is accessible via context.events.emit, not context.app.eventBus.emit. This code will not work as intended because it's checking for properties on the wrong object. Use context.events.emit instead of accessing eventBus through the app property.
| // Emit plugin disabled event | |
| const app = context.app as ExtendedAppContext; | |
| if (app.eventBus && typeof app.eventBus.emit === 'function') { | |
| app.eventBus.emit('plugin.disabled', { | |
| pluginId: BetterAuthManifest.id, | |
| timestamp: new Date().toISOString() | |
| }); | |
| } | |
| // Emit plugin disabled event via kernel event bus | |
| if (context.events && typeof context.events.emit === 'function') { | |
| context.events.emit('plugin.disabled', { | |
| pluginId: BetterAuthManifest.id, | |
| timestamp: new Date().toISOString() | |
| }); | |
| } | |
| } catch (e) { | ||
| console.error("[Better-Auth Plugin] Error in user create hook:", e); | ||
| // Ensure a default role is set even in error cases | ||
| return { | ||
| data: { | ||
| ...user, | ||
| role: 'user' | ||
| } | ||
| }; |
There was a problem hiding this comment.
The error handler returns a user without a role assignment, which could create security issues. When the user creation hook encounters an error (line 110), it returns the user object without ensuring a default role is set. This deviates from the documented behavior and could result in users without proper access controls. The fallback should ensure the 'user' role is assigned, as shown in the comment on line 112.
| const app = context.app as ExtendedAppContext; | ||
| if (app.router && typeof app.router.all === 'function') { |
There was a problem hiding this comment.
Type casting with unsafe assertions bypasses TypeScript's type safety. The code casts context.app to ExtendedAppContext and then uses optional chaining to check for properties. This pattern is error-prone because it doesn't guarantee that the properties exist at runtime. Consider defining a type guard function to validate that the required properties exist before casting, or update the PluginContextData interface to properly type the app property.
| app.router.all('/api/auth/*', async (req: any, res: any) => { | ||
| // Pass the request to Better-Auth handler | ||
| return handler(req, res); | ||
| }); |
There was a problem hiding this comment.
The router.all method is not defined in the PluginContextData interface. Looking at the kernel implementation in plugin-context.ts, the router only has get, post, and use methods defined. The use of router.all on line 100 will fail at runtime because this method doesn't exist on the provided router interface. Consider using router.use instead, or request that the router.all method be added to the kernel's router interface.
| ### First User Setup | ||
|
|
||
| The first user to register automatically receives the `super_admin` role. Subsequent users get the `user` role by default. | ||
|
|
There was a problem hiding this comment.
The behavior described here means the databaseHooks.user.create.before hook assigns the super_admin role to the first account created solely based on there being zero existing users, with no additional bootstrap secret or restriction. On a fresh deployment where the /api/auth/sign-up endpoint is exposed to untrusted networks, an attacker can simply register the first account and gain permanent super_admin privileges (and concurrent sign-ups can result in multiple super_admin users). Consider changing the bootstrap flow so that initial administrator creation is gated by an explicit one-time secret or out-of-band setup step rather than an unauthenticated first-signup heuristic.
Implements
@objectos/plugin-better-authas a standalone authentication module based on Better-Auth library. Provides email/password auth, organization/team management, and RBAC with multi-database support (PostgreSQL, MongoDB, SQLite).Architecture
/api/auth/*duringonEnablesuper_adminroleImplementation
Usage
Plugin Manifest
Key Design Decisions
resetAuthInstance()to prevent leaksExtendedAppContextinterface for router/eventBus access instead of unsafe casts@objectos/server/authimplementationFiles
auth-client.ts,plugin.ts,index.tsOriginal prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.