Update user in tables to use hashed password#1667
Conversation
…e openops-tables block on the context
…ater used in openops-tables
There was a problem hiding this comment.
Pull request overview
This PR updates the authentication flow to use hashed passwords from the database when authenticating with OpenOps Tables, rather than plain-text passwords from environment variables. This involves retrieving the user from the database to access their hashed password, updating the password management workflow, and including a migration to synchronize existing user passwords with the Tables system.
Key changes:
- Modified authentication to retrieve and use hashed passwords from the database instead of plain-text environment variables
- Added a database migration to sync existing user passwords with OpenOps Tables
- Updated the
updatePasswordfunction to return the updated user object
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/server/api/src/app/openops-tables/auth-admin-tables.ts | Modified to retrieve admin user from database and use hashed password for authentication |
| packages/server/api/src/app/user/user-service.ts | Changed updatePassword to return the updated User object |
| packages/server/api/src/app/database/seeds/seed-admin.ts | Updated admin seeding to sync hashed passwords with Tables system |
| packages/server/api/src/app/authentication/basic/authentication-service.ts | Changed to use hashed password from user object instead of request password |
| packages/server/api/src/app/authentication/new-user/create-user.ts | Updated to pass hashed password from user object to Tables |
| packages/server/api/src/app/database/migrations/1763755045436-MigrateTablesUserPassword.ts | New migration to sync existing user passwords with OpenOps Tables |
| packages/server/api/src/app/database/postgres-connection.ts | Registered the new migration |
| packages/server/api/test/unit/openops-tables/auth-admin-tables.test.ts | Updated test to mock user service and remove password environment variable checks |
| packages/server/api/test/unit/authentication/new-user/create-user.test.ts | Updated test assertion to use hashed password from user object |
| packages/server/api/src/app/openops-tables/template-tables/seed-tables-for-templates.ts | Removed unused import |
| packages/server/api/src/app/openops-tables/index.ts | Removed exported authentication function (no longer needed in public API) |
| deploy/helm/openops/values.yaml | Bumped openops-tables version to 0.2.11 |
| deploy/docker-compose/docker-compose.yml | Bumped openops-tables version to 0.2.11 |
| compose.yaml | Bumped openops-tables version to 0.2.11 |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Greptile OverviewGreptile SummaryThis PR migrates the OpenOps Tables service (Baserow) to use bcrypt-hashed passwords that match the OpenOps database, replacing the previous plaintext password flow. The Tables service version is upgraded from 0.2.10 to 0.2.11 to support this change.
Critical Issue Found:
Confidence Score: 2/5
Important Files ChangedFile Analysis
Sequence DiagramsequenceDiagram
participant User
participant OpenOpsAPI
participant OpenOpsDB
participant TablesAPI
Note over OpenOpsAPI,TablesAPI: User Sign In Flow
User->>OpenOpsAPI: POST /signin (email, plaintext password)
OpenOpsAPI->>OpenOpsDB: Fetch user by email
OpenOpsDB-->>OpenOpsAPI: User with hashed password
OpenOpsAPI->>OpenOpsAPI: Compare plaintext with hash (bcrypt)
OpenOpsAPI->>TablesAPI: POST /api/user/token-auth/ (email, hashed password)
TablesAPI-->>OpenOpsAPI: Auth tokens
OpenOpsAPI-->>User: Success with tokens
Note over OpenOpsAPI,TablesAPI: User Creation Flow
User->>OpenOpsAPI: POST /signup (email, plaintext password)
OpenOpsAPI->>OpenOpsAPI: Hash password (bcrypt)
OpenOpsAPI->>OpenOpsDB: Save user with hashed password
OpenOpsDB-->>OpenOpsAPI: User created
OpenOpsAPI->>TablesAPI: POST /api/user/ (email, hashed password)
TablesAPI-->>OpenOpsAPI: User created with refresh token
OpenOpsAPI-->>User: Success
Note over OpenOpsAPI,TablesAPI: Password Update Flow
OpenOpsAPI->>OpenOpsAPI: Hash new password
OpenOpsAPI->>OpenOpsDB: Update user password (hashed)
OpenOpsDB-->>OpenOpsAPI: Updated
OpenOpsAPI->>TablesAPI: PATCH /api/admin/users/ (email, hashed password)
TablesAPI-->>OpenOpsAPI: Password updated
Note over OpenOpsAPI,TablesAPI: Migration Flow
OpenOpsAPI->>OpenOpsDB: SELECT email, password FROM user
OpenOpsDB-->>OpenOpsAPI: All users with hashed passwords
OpenOpsAPI->>TablesAPI: Authenticate as admin
TablesAPI-->>OpenOpsAPI: Admin token
loop For each non-admin user
OpenOpsAPI->>TablesAPI: PATCH /api/admin/users/ (email, hashed password)
end
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
packages/server/api/test/unit/authentication/new-user/create-user.test.ts (1)
50-61: Test mock is missing thepasswordfield.The
createdUsermock doesn't include apasswordproperty, sores.user.passwordat line 86 will beundefined. This makes the assertion less meaningful. Compare with the test at line 132-154 which correctly includespassword: '12345678'in the mock.Add a password field to the mock to properly test the behavior:
const createdUser = { id: 'u1', email: baseParams.email, organizationId: baseParams.organizationId, firstName: baseParams.firstName, lastName: baseParams.lastName, status: UserStatus.ACTIVE, organizationRole: OrganizationRole.MEMBER, verified: baseParams.verified, trackEvents: baseParams.trackEvents, newsLetter: baseParams.newsLetter, + password: 'hashed-password', };Also applies to: 83-88
packages/server/api/test/unit/openops-tables/auth-admin-tables.test.ts (2)
64-66: Remove unused password mock.The second
mockReturnValueOnce('secret')appears to be a leftover from the old implementation whereOPENOPS_ADMIN_PASSWORDwas retrieved viagetOrThrow. Since the password now comes from the user object (Line 42), this second mock return value is no longer needed.Apply this diff:
- getOrThrowMock - .mockReturnValueOnce('admin@example.com') - .mockReturnValueOnce('secret'); + getOrThrowMock.mockReturnValueOnce('admin@example.com');
90-92: Fix password mismatch and remove unused mock.Two issues:
- The mock returns
'pwd'but the user object inbeforeEach(Line 42) haspassword: 'secret'- The second
mockReturnValueOncefor the password is no longer used since passwords now come from the user objectApply this diff:
- getOrThrowMock - .mockReturnValueOnce('admin2@example.com') - .mockReturnValueOnce('pwd'); + getOrThrowMock.mockReturnValueOnce('admin2@example.com');Additionally, verify the test expectations at lines 98-103 match the mocked user's password
'secret', not'pwd'.
🧹 Nitpick comments (1)
packages/server/api/src/app/database/migrations/1763755045436-MigrateTablesUserPassword.ts (1)
38-40: Document the irreversible nature of this migration.The lack of rollback implementation is understandable given that password resets in an external system (OpenOps Tables) cannot be easily reversed without storing previous state. However, this should be clearly documented in the PR description or migration comments to inform operators that this migration is irreversible.
Consider adding a comment explaining why rollback is not supported:
public async down(_: QueryRunner): Promise<void> { + // Rollback is not supported because this migration updates passwords in + // the external OpenOps Tables system, which cannot be reversed without + // storing the previous password state (which we don't have). throw new Error('Rollback not implemented'); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
compose.yaml(1 hunks)deploy/docker-compose/docker-compose.yml(2 hunks)deploy/helm/openops/values.yaml(1 hunks)packages/server/api/src/app/authentication/basic/authentication-service.ts(1 hunks)packages/server/api/src/app/authentication/new-user/create-user.ts(1 hunks)packages/server/api/src/app/database/migrations/1763755045436-MigrateTablesUserPassword.ts(1 hunks)packages/server/api/src/app/database/postgres-connection.ts(2 hunks)packages/server/api/src/app/database/seeds/seed-admin.ts(4 hunks)packages/server/api/src/app/openops-tables/auth-admin-tables.ts(2 hunks)packages/server/api/src/app/openops-tables/index.ts(0 hunks)packages/server/api/src/app/openops-tables/template-tables/seed-tables-for-templates.ts(0 hunks)packages/server/api/src/app/user/user-service.ts(2 hunks)packages/server/api/test/unit/authentication/new-user/create-user.test.ts(1 hunks)packages/server/api/test/unit/openops-tables/auth-admin-tables.test.ts(3 hunks)
💤 Files with no reviewable changes (2)
- packages/server/api/src/app/openops-tables/template-tables/seed-tables-for-templates.ts
- packages/server/api/src/app/openops-tables/index.ts
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/coding-guidelines.mdc)
**/*.{ts,tsx,js,jsx}: Indentation: 2 spaces for TypeScript/JavaScript
Braces required for all control blocks, even single-line
One space between keywords and parentheses:if (condition) {
Use camelCase for variables and functions
Use PascalCase for classes and types
Use UPPER_SNAKE_CASE for constants
Use lowercase with hyphens for file names (e.g.,user-profile.ts)
Preferconstoverletorvarin TypeScript/JavaScript
Prefer arrow functions for callbacks and functional components in TypeScript/JavaScript
Files:
packages/server/api/src/app/database/migrations/1763755045436-MigrateTablesUserPassword.tspackages/server/api/src/app/openops-tables/auth-admin-tables.tspackages/server/api/test/unit/authentication/new-user/create-user.test.tspackages/server/api/src/app/user/user-service.tspackages/server/api/src/app/authentication/new-user/create-user.tspackages/server/api/src/app/database/postgres-connection.tspackages/server/api/src/app/authentication/basic/authentication-service.tspackages/server/api/test/unit/openops-tables/auth-admin-tables.test.tspackages/server/api/src/app/database/seeds/seed-admin.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/coding-guidelines.mdc)
**/*.{ts,tsx}: Use types and interfaces where appropriate in TypeScript
Use explicit return types for exported functions in TypeScript
Files:
packages/server/api/src/app/database/migrations/1763755045436-MigrateTablesUserPassword.tspackages/server/api/src/app/openops-tables/auth-admin-tables.tspackages/server/api/test/unit/authentication/new-user/create-user.test.tspackages/server/api/src/app/user/user-service.tspackages/server/api/src/app/authentication/new-user/create-user.tspackages/server/api/src/app/database/postgres-connection.tspackages/server/api/src/app/authentication/basic/authentication-service.tspackages/server/api/test/unit/openops-tables/auth-admin-tables.test.tspackages/server/api/src/app/database/seeds/seed-admin.ts
**/*.{tsx,ts}
📄 CodeRabbit inference engine (.cursor/rules/coding-guidelines.mdc)
**/*.{tsx,ts}: Frontend tech stack must strictly use: React 18, Zustand, react-query v5, shadcn, and Axios withqspackage for query strings
Follow best practices for React hooks
Prefer small, composable components and extract helper functions where possible
Usecnutility to group tailwind classnames in React components
Files:
packages/server/api/src/app/database/migrations/1763755045436-MigrateTablesUserPassword.tspackages/server/api/src/app/openops-tables/auth-admin-tables.tspackages/server/api/test/unit/authentication/new-user/create-user.test.tspackages/server/api/src/app/user/user-service.tspackages/server/api/src/app/authentication/new-user/create-user.tspackages/server/api/src/app/database/postgres-connection.tspackages/server/api/src/app/authentication/basic/authentication-service.tspackages/server/api/test/unit/openops-tables/auth-admin-tables.test.tspackages/server/api/src/app/database/seeds/seed-admin.ts
🧬 Code graph analysis (5)
packages/server/api/src/app/database/migrations/1763755045436-MigrateTablesUserPassword.ts (3)
packages/server/shared/src/lib/system/system.ts (1)
system(104-200)packages/openops/src/lib/openops-tables/auth-user.ts (1)
authenticateUserInOpenOpsTables(10-30)packages/openops/src/lib/openops-tables/reset-user-password.ts (1)
resetUserPassword(4-17)
packages/server/api/src/app/openops-tables/auth-admin-tables.ts (1)
packages/openops/src/lib/openops-tables/auth-user.ts (1)
authenticateUserInOpenOpsTables(10-30)
packages/server/api/src/app/user/user-service.ts (1)
packages/shared/src/lib/user/user.ts (2)
User(28-41)User(43-43)
packages/server/api/src/app/database/postgres-connection.ts (1)
packages/server/api/src/app/database/migrations/1763755045436-MigrateTablesUserPassword.ts (1)
MigrateTablesUserPassword1763755045436(8-41)
packages/server/api/src/app/database/seeds/seed-admin.ts (3)
packages/openops/src/lib/openops-tables/auth-user.ts (1)
authenticateUserInOpenOpsTables(10-30)packages/openops/src/lib/openops-tables/reset-user-password.ts (1)
resetUserPassword(4-17)packages/server/api/src/app/user/user-service.ts (1)
userService(27-286)
🔇 Additional comments (14)
deploy/helm/openops/values.yaml (1)
93-93: Helm values version bump is consistent and correct.The tag bump from "0.2.10" to "0.2.11" aligns with configuration across other deployment files. The
MIGRATE_ON_STARTUP: 'true'setting (line 106) will handle any required database migrations automatically during container startup.deploy/docker-compose/docker-compose.yml (1)
24-24: Docker Compose version references are consistent.Both occurrences of the version bump (environment variable on line 24 and image tag on line 50) are aligned and correct. The application will automatically migrate the database on startup (line 63), ensuring schema compatibility with the new version.
Also applies to: 50-50
compose.yaml (1)
5-5: Compose file version bump is consistent across all deployment configs.All three deployment configurations (compose.yaml, docker-compose.yml, and helm values.yaml) use the same tag 0.2.11, ensuring consistency across development and production environments.
packages/server/api/src/app/database/postgres-connection.ts (1)
41-41: LGTM!The migration is correctly imported and registered at the end of the migrations list, maintaining chronological order.
Also applies to: 88-88
packages/server/api/src/app/database/migrations/1763755045436-MigrateTablesUserPassword.ts (1)
14-20: LGTM!The early return when no users exist is a good optimization. The query for passwords aligns with the PR's objective to synchronize hashed passwords to OpenOps Tables.
packages/server/api/src/app/authentication/new-user/create-user.ts (1)
121-126: LGTM - Hashed password now passed to OpenOps Tables.This change passes the hashed password (
user.password) to OpenOps Tables instead of the plain text input. This aligns with the PR objective and requires the OpenOps Tables 0.2.11 upgrade to be deployed alongside this change.Ensure the OpenOps Tables upgrade to 0.2.11 is deployed before or simultaneously with this change, as the Tables service must now accept hashed passwords during user creation.
packages/server/api/src/app/database/seeds/seed-admin.ts (2)
81-85: LGTM - Properly syncs hashed password to OpenOps Tables after admin creation.The flow correctly authenticates with the plain text password first (required for initial auth), then resets the password in OpenOps Tables to the hashed version for subsequent authentications.
172-179: LGTM - Password update correctly syncs to OpenOps Tables.The sequence is correct: update locally (which hashes the password), authenticate with the plain text password to get a token, then reset the Tables password to the hashed version.
packages/server/api/src/app/authentication/basic/authentication-service.ts (1)
39-42: LGTM - Sign-in now uses hashed password for OpenOps Tables authentication.After the local password verification succeeds (line 34-37), the hashed password from the database is used to authenticate with OpenOps Tables. This is consistent with the new password synchronization flow.
packages/server/api/src/app/openops-tables/auth-admin-tables.ts (1)
24-32: LGTM - Admin authentication now uses hashed password from database.The change correctly retrieves the admin user from the database and uses the stored hashed password for OpenOps Tables authentication. This aligns with the broader password synchronization strategy in this PR.
Consider the upgrade path: existing installations where the admin user exists in OpenOps Tables but the password wasn't yet synced (hashed) will need the seed process to run first to establish the hashed password in Tables. Verify that deployment procedures account for this ordering.
packages/server/api/test/unit/openops-tables/auth-admin-tables.test.ts (3)
22-27: LGTM! Mock setup correctly added.The mock for
getUserByEmailOrFailis properly configured to support the new authentication flow.
39-45: LGTM! User mock properly initialized.The user object with
idandpasswordfields correctly supports the new flow where admin credentials come from the user record.
10-10: LGTM! Mock updated to reflect system property changes.Replacing
OPENOPS_ADMIN_PASSWORDwithDB_TYPEcorrectly reflects the removal of the admin password from system configuration.packages/server/api/src/app/user/user-service.ts (1)
191-205: All callers properly handle the newPromise<User>return type.Verification complete: The only call site of
updatePasswordis inpackages/server/api/src/app/database/seeds/seed-admin.ts(line 173), which correctly captures the return value asupdatedUserand actively uses it on line 177 to accessupdatedUser.password. The signature change has been properly propagated and all call sites are compatible with the new return type.
|
Fixes OPS-3123.
Summary by CodeRabbit
Chores
Refactor
Tests
✏️ Tip: You can customize this high-level summary in your review settings.