OpenID #498
OpenID #498
Conversation
Note this is only disallowed at bootstrap. If the database is edited manually, we make no attempt to detect this.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (15)
src/account-db.js (2)
97-107: Consider optimizing permission checksThe permission checks could be optimized to reduce database queries. Currently,
isAdminmight perform two database queries when checking both owner status and ADMIN permission.export function isAdmin(userId) { - const user = - getAccountDb().first('SELECT owner FROM users WHERE id = ?', [userId]) || - {}; - if (user?.owner === 1) return true; - return hasPermission(userId, 'ADMIN'); + const user = getAccountDb().first( + 'SELECT owner, role FROM users WHERE id = ?', + [userId] + ) || {}; + return user?.owner === 1 || user?.role === 'ADMIN'; }
185-194: Consider making session expiration configurableThe session expiration threshold is currently hardcoded to 1 hour (3600 seconds). Consider making this configurable to allow for different session lifetime requirements.
+const SESSION_EXPIRY_SECONDS = config.sessionExpirySeconds || 3600; + export function clearExpiredSessions() { - const clearThreshold = Math.floor(Date.now() / 1000) - 3600; + const clearThreshold = Math.floor(Date.now() / 1000) - SESSION_EXPIRY_SECONDS; const deletedSessions = getAccountDb().mutate( 'DELETE FROM sessions WHERE expires_at <> -1 and expires_at < ?', [clearThreshold], ).changes; console.log(`Deleted ${deletedSessions} old sessions`); }src/app-sync/services/files-service.js (2)
Line range hint
106-117: Fix property name inconsistency in set methodThe SQL column is named
ownerbut the value is passed asfile.userId.Apply this fix:
- file.userId, + file.owner,
122-144: Consider optimizing the find query performanceWhile the implementation is functionally correct, consider these optimizations:
- Use
UNION ALLinstead ofUNIONsince duplicate results are not possible due to the WHERE conditions- Add indexes on
files.owneranduser_access.file_id, user_access.user_idif not already present- UNION + UNION ALLsrc/services/user-service.js (5)
3-31: Consider consistent return types across getter functionsThe getter functions have inconsistent return types:
getUserByUsernameandgetUserByIdreturn only the IDgetFileByIdusesSELECT *but returns only the IDConsider standardizing the return types:
- const { id } = - getAccountDb().first('SELECT * FROM users WHERE id = ?', [userId]) || {}; - return id || null; + const user = + getAccountDb().first('SELECT id, user_name, display_name, enabled, role FROM users WHERE id = ?', [userId]) || null; + return user;Consider adding database indexes on frequently queried columns:
CREATE INDEX idx_users_username ON users(user_name); CREATE INDEX idx_users_id ON users(id); CREATE INDEX idx_files_id ON files(id);
33-36: Define roles as constantsThe role validation uses a hardcoded array which could lead to inconsistencies if roles are referenced elsewhere in the codebase.
+const ROLES = { + BASIC: 'BASIC', + ADMIN: 'ADMIN' +} as const; + export function validateRole(roleId) { - const possibleRoles = ['BASIC', 'ADMIN']; - return possibleRoles.some((a) => a === roleId); + return Object.values(ROLES).includes(roleId); }
38-43: Optimize owner count queryThe current query uses string comparison which may not utilize indexes effectively.
export function getOwnerCount() { const { ownerCount } = getAccountDb().first( - `SELECT count(*) as ownerCount FROM users WHERE users.user_name <> '' and users.owner = 1`, + `SELECT count(*) as ownerCount FROM users WHERE users.owner = 1 AND length(users.user_name) > 0`, ) || { ownerCount: 0 }; return ownerCount; }
133-142: Optimize getUserAccess query performanceThe current query could be optimized by:
- Using EXISTS clause instead of JOIN when checking ownership
- Adding appropriate indexes
export function getUserAccess(fileId, userId, isAdmin) { return getAccountDb().all( - `SELECT users.id as userId, user_name as userName, files.owner, display_name as displayName - FROM users - JOIN user_access ON user_access.user_id = users.id - JOIN files ON files.id = user_access.file_id - WHERE files.id = ? and (files.owner = ? OR 1 = ?)`, + `SELECT DISTINCT u.id as userId, u.user_name as userName, + CASE WHEN f.owner = u.id THEN 1 ELSE 0 END as owner, + u.display_name as displayName + FROM users u + WHERE EXISTS ( + SELECT 1 FROM files f + LEFT JOIN user_access ua ON ua.file_id = f.id AND ua.user_id = u.id + WHERE f.id = ? AND (f.owner = ? OR ? = 1) + )`, [fileId, userId, isAdmin ? 1 : 0], ); }Add the following indexes to improve query performance:
CREATE INDEX idx_user_access_file_user ON user_access(file_id, user_id); CREATE INDEX idx_files_owner ON files(owner);
217-222: Add type safety to OpenID configurationThe function could benefit from type safety to ensure consistent return types.
+interface OpenIDConfig { + method: string; + client_id: string; + client_secret: string; + discovery_url: string; +} + export function getOpenIDConfig() { return ( - getAccountDb().first(`SELECT * FROM auth WHERE method = ?`, ['openid']) || + getAccountDb().first<OpenIDConfig>(`SELECT method, client_id, client_secret, discovery_url FROM auth WHERE method = ?`, ['openid']) || null ); }src/load-config.js (1)
153-198: Consider extracting OpenID configuration logic.While the implementation is robust, the complexity of the IIFE could be reduced by extracting the configuration logic into a separate function.
Consider this structure:
- openId: (() => { + openId: buildOpenIdConfig(config.openId), +}; + +function buildOpenIdConfig(defaultConfig) { if ( !process.env.ACTUAL_OPENID_DISCOVERY_URL && !process.env.ACTUAL_OPENID_AUTHORIZATION_ENDPOINT ) { - return config.openId; + return defaultConfig; } // ... rest of the implementation - })(),src/accounts/openid.js (3)
32-45: Consider adding rollback handling for database transaction errorsWhile the transaction error is caught and logged, it might be helpful to explicitly rollback any partial changes in case of failure. Also, consider adding more specific error types to help with debugging.
try { accountDb.transaction(() => { accountDb.mutate('DELETE FROM auth WHERE method = ?', ['openid']); accountDb.mutate('UPDATE auth SET active = 0'); accountDb.mutate( "INSERT INTO auth (method, display_name, extra_data, active) VALUES ('openid', 'OpenID', ?, 1)", [JSON.stringify(config)], ); }); } catch (err) { - console.error('Error updating auth table:', err); + console.error('Error updating auth table:', { + error: err, + operation: 'openid_bootstrap', + timestamp: new Date().toISOString() + }); + try { + accountDb.exec('ROLLBACK'); + } catch (rollbackErr) { + console.error('Rollback failed:', rollbackErr); + } return { error: 'database-error' }; }
74-127: Consider adding rate limiting for login attemptsTo prevent potential abuse, consider implementing rate limiting for OpenID login attempts per IP address.
Consider using a rate limiting middleware or implementing a simple counter in the database:
function checkRateLimit(ip) { const window = 15 * 60 * 1000; // 15 minutes const limit = 5; // attempts const attempts = accountDb.first( 'SELECT COUNT(*) as count FROM pending_openid_requests WHERE ip = ? AND expiry_time > ?', [ip, Date.now() - window] ); return attempts.count < limit; }
266-273: Use optional chaining for safer property accessThe code can be simplified using optional chaining.
- if (auth && auth.extra_data) { + if (auth?.extra_data) { try { const openIdConfig = JSON.parse(auth.extra_data); return openIdConfig.server_hostname; } catch (error) { console.error('Error parsing OpenID configuration:', error); } }🧰 Tools
🪛 Biome
[error] 266-266: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
src/app-sync.js (2)
251-255: Simplify owner field validationWhile the validation is correct, the IIFE pattern makes it unnecessarily complex.
Consider simplifying to:
- owner: - res.locals.user_id || - (() => { - throw new Error('User ID is required for file creation'); - })(), + owner: res.locals.user_id ?? (() => { + throw new Error('User ID is required for file creation'); + })(),Or even simpler:
- owner: - res.locals.user_id || - (() => { - throw new Error('User ID is required for file creation'); - })(), + owner: res.locals.user_id,Since
validateSessionMiddlewareis already in place,res.locals.user_idshould always be defined.
Line range hint
336-344: Consider implementing the TODO for fileId validationThe TODO comment suggests implementing proper validation for the fileId parameter. This would improve error handling consistency across endpoints.
Would you like me to help implement the fileId validation and create a GitHub issue to track this enhancement?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
src/account-db.js(3 hunks)src/accounts/openid.js(1 hunks)src/app-secrets.js(1 hunks)src/app-sync.js(7 hunks)src/app-sync/services/files-service.js(8 hunks)src/load-config.js(3 hunks)src/services/user-service.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/app-secrets.js
🧰 Additional context used
📓 Learnings (3)
src/app-sync/services/files-service.js (1)
Learnt from: lelemm
PR: actualbudget/actual-server#498
File: src/app-sync/services/files-service.js:146-163
Timestamp: 2024-11-11T16:51:00.464Z
Learning: In `src/app-sync/services/files-service.js`, the `findUsersWithAccess` method is always called after a file find, so it does not need to independently check for invalid `fileId` or handle cases where the file doesn't exist.
src/load-config.js (1)
Learnt from: lelemm
PR: actualbudget/actual-server#498
File: src/load-config.js:79-81
Timestamp: 2024-11-11T16:36:02.137Z
Learning: In `src/load-config.js`, the default `token_expiration` value of `'never'` is intentional and should be maintained, as the application supports non-expiring tokens.
src/services/user-service.js (1)
Learnt from: lelemm
PR: actualbudget/actual-server#498
File: src/services/user-service.js:155-163
Timestamp: 2024-11-11T12:57:09.322Z
Learning: In `src/services/user-service.js`, the `checkFilePermission` function is intended to check ownership, and admin access is verified separately after calling this function.
🪛 Biome
src/accounts/openid.js
[error] 266-266: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (13)
src/account-db.js (3)
5-6: LGTM! Clean modularization and well-structured API
The separation of password and OpenID authentication into separate modules improves code organization. The listLoginMethods function provides a clean interface with proper field mapping.
Also applies to: 25-33
169-172: Consider limiting exposed user fields
The current implementation returns all user fields. Consider explicitly selecting only the required fields to prevent accidentally exposing sensitive information.
[security]
export function getUserInfo(userId) {
let accountDb = getAccountDb();
- return accountDb.first('SELECT * FROM users WHERE id = ?', [userId]);
+ return accountDb.first(
+ 'SELECT id, user_name, role, owner FROM users WHERE id = ?',
+ [userId]
+ );
}174-183: LGTM! Well-handled role retrieval
The function properly handles null cases and provides a clean interface for role checks.
src/app-sync/services/files-service.js (4)
1-1: LGTM: Clean implementation of owner property
The addition of the isAdmin import and owner property to the FileBase class is well-structured and properly initialized.
Also applies to: 14-14, 24-24
146-163: LGTM: Well-implemented user access query
The findUsersWithAccess implementation is efficient:
- Uses
UNION ALLfor better performance - Properly joins with users table to get display information
- Correctly handles both direct owners and users with access
234-234: LGTM: Consistent owner property handling
The validate method correctly maps the owner field from the database to the File object.
39-39:
Add owner property to FileUpdate class
The owner property is missing from the FileUpdate class while it's present in both FileBase and File classes.
Apply this fix:
class FileUpdate extends FileBase {
constructor({
name = undefined,
groupId = undefined,
encryptSalt = undefined,
encryptTest = undefined,
encryptKeyId = undefined,
encryptMeta = undefined,
syncVersion = undefined,
deleted = undefined,
+ owner = undefined,
}) {
super(
name,
groupId,
encryptSalt,
encryptTest,
encryptKeyId,
encryptMeta,
syncVersion,
deleted,
+ owner,
);
}
}Also applies to: 50-50, 61-78
src/load-config.js (3)
79-81: LGTM! Default configuration values are well-chosen.
The new default values provide sensible defaults while maintaining backward compatibility:
multiuser: false- Safe default for existing single-user deploymentstoken_expiration: 'never'- Maintains support for non-expiring tokensloginMethod: 'password'- Sensible default authentication method
110-118: LGTM! Robust validation for multiuser configuration.
The implementation includes proper validation ensuring the ACTUAL_MULTIUSER environment variable is explicitly set to either "true" or "false", with clear error messaging.
199-201: LGTM! Token expiration and login method handling.
The implementation correctly:
- Allows configuration of token expiration via environment variable
- Provides clear debug logging with a sensible fallback
Also applies to: 209-209
src/app-sync.js (3)
8-8: LGTM: Improved security with session validation
The switch to validateSessionMiddleware enhances security by validating sessions at the application level, ensuring consistent authentication across all endpoints.
Also applies to: 28-28
116-117: Skip review: Already addressed
The redundant check was correctly removed as noted in the previous review comments, since validateSessionMiddleware ensures res.locals is always defined.
315-315: Verify access control implementation
The changes correctly implement user-scoped file access and ownership information. Let's verify the implementation of findUsersWithAccess.
Also applies to: 324-330
✅ Verification successful
Let me gather more information about the access control implementation.
Based on the gathered information, I can now provide the final response:
Access control implementation is properly secured
The implementation correctly enforces user-scoped access through multiple layers:
- File listing is properly filtered by user ID from the authenticated session
findUsersWithAccessuses secure SQL joins with proper user access tables- Additional security through
checkFilePermissionandcountUserAccessmethods that verify both ownership and explicit access grants - The
findmethod infiles-service.jshas proper user ID validation with admin check
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the implementation of findUsersWithAccess method
# Check for the method implementation
ast-grep --pattern 'findUsersWithAccess($id) {
$$$
}'
# Check for potential access control bypass
rg -g '*.js' -A 5 'findUsersWithAccess'
Length of output: 1099
Script:
#!/bin/bash
# Check the SQL schema and access control related tables
rg -g '*.sql' 'CREATE TABLE (files|user_access)'
# Check for other methods that might modify user access
ast-grep --pattern 'function $_(fileId, userId) {
$$$
}'
# Check for any direct file access without user validation
rg -g '*.js' 'find\(' -A 3
Length of output: 7886
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (9)
src/account-db.js (2)
110-119: Consider enhancing permission system for better flexibility.The current permission system is quite basic. Consider implementing a more flexible permission system that supports:
- Multiple permissions per user
- Permission hierarchies
- Role-based access control (RBAC)
Example enhancement:
export function hasPermission(userId, permission) { - return getUserPermission(userId) === permission; + const userPermissions = getUserPermissions(userId); + return userPermissions.some(p => + p === permission || + p === 'ADMIN' || + isPermissionInHierarchy(p, permission) + ); }
198-207: Consider making session expiration threshold configurable.The current implementation uses a hardcoded 1-hour (3600 seconds) expiration threshold. Consider making this configurable through the application's configuration system.
+const SESSION_EXPIRY_SECONDS = config.sessionExpirySeconds || 3600; export function clearExpiredSessions() { - const clearThreshold = Math.floor(Date.now() / 1000) - 3600; + const clearThreshold = Math.floor(Date.now() / 1000) - SESSION_EXPIRY_SECONDS;src/app-sync/services/files-service.js (1)
Line range hint
108-119: Fix column name casing inconsistencyThe column names in the SQL query use inconsistent casing:
encrypt_testandencrypt_keyidin the values array don't match the property namesencryptTestandencryptKeyIdApply this fix:
'INSERT INTO files (id, group_id, sync_version, name, encrypt_meta, encrypt_salt, encrypt_test, encrypt_keyid, deleted, owner) VALUES (?, ?, ?, ?, ?, ?, ?, ? ,?, ?)', [ file.id, file.groupId, file.syncVersion.toString(), file.name, file.encryptMeta, file.encryptSalt, - file.encrypt_test, - file.encrypt_keyid, + file.encryptTest, + file.encryptKeyId, deletedInt, file.owner, ],src/services/user-service.js (3)
8-10: Maintain consistency in column selection across queriesSome queries use
SELECT *while others specify columns explicitly. For better performance and maintainability, consider explicitly selecting only the required columns.- getAccountDb().first('SELECT * FROM users WHERE id = ?', [userId]) || {}; + getAccountDb().first('SELECT id FROM users WHERE id = ?', [userId]) || {}; - getAccountDb().first('SELECT * FROM files WHERE files.id = ?', [fileId]) || + getAccountDb().first('SELECT id FROM files WHERE files.id = ?', [fileId]) ||Also applies to: 19-19, 28-29
33-36: Consider moving role constants to a configuration fileHardcoding role types in the function makes it harder to maintain and reuse. Consider moving these to a separate configuration file.
+// roles.js +export const ROLES = { + BASIC: 'BASIC', + ADMIN: 'ADMIN' +}; +// user-service.js +import { ROLES } from './roles.js'; export function validateRole(roleId) { - const possibleRoles = ['BASIC', 'ADMIN']; + return Object.values(ROLES).includes(roleId); }
255-260: Consider caching OpenID configurationThe OpenID configuration is likely to be accessed frequently but changes rarely. Consider implementing caching to improve performance.
+let openIDConfigCache = null; +const CACHE_TTL = 5 * 60 * 1000; // 5 minutes +let lastCacheTime = 0; export function getOpenIDConfig() { + const now = Date.now(); + if (openIDConfigCache && (now - lastCacheTime) < CACHE_TTL) { + return openIDConfigCache; + } + + openIDConfigCache = getAccountDb().first( `SELECT * FROM auth WHERE method = ?`, ['openid'] - ) || null; + ) || null; + lastCacheTime = now; + return openIDConfigCache; }src/accounts/openid.js (3)
11-48: Add type validation for config parametersWhile the function checks for the presence of required fields, it doesn't validate their types or formats. Consider adding type validation to ensure the configuration is properly formatted.
export async function bootstrapOpenId(config) { + if (typeof config !== 'object' || config === null) { + return { error: 'invalid-config-format' }; + } if (!('issuer' in config)) { return { error: 'missing-issuer' }; } + if (typeof config.client_id !== 'string' || !config.client_id.trim()) { + return { error: 'invalid-client-id' }; + } + if (typeof config.client_secret !== 'string' || !config.client_secret.trim()) { + return { error: 'invalid-client-secret' }; + } + if (typeof config.server_hostname !== 'string' || !config.server_hostname.trim()) { + return { error: 'invalid-server-hostname' }; + }
50-72: Add validation for issuer object propertiesWhen the issuer is provided as an object, its required properties should be validated to prevent runtime errors.
? await Issuer.discover(config.issuer) : new Issuer({ + // Validate required properties + ...(!config.issuer.name || typeof config.issuer.name !== 'string' + ? { error: 'invalid-issuer-name' } + : {}), + ...(!config.issuer.authorization_endpoint || typeof config.issuer.authorization_endpoint !== 'string' + ? { error: 'invalid-authorization-endpoint' } + : {}), issuer: config.issuer.name, authorization_endpoint: config.issuer.authorization_endpoint, token_endpoint: config.issuer.token_endpoint, userinfo_endpoint: config.issuer.userinfo_endpoint, });
264-271: Optimize using optional chainingThe code can be simplified using optional chaining while maintaining the same functionality.
- if (auth && auth.extra_data) { - try { - const openIdConfig = JSON.parse(auth.extra_data); - return openIdConfig.server_hostname; - } catch (error) { - console.error('Error parsing OpenID configuration:', error); - } - } + try { + return JSON.parse(auth?.extra_data)?.server_hostname ?? null; + } catch (error) { + console.error('Error parsing OpenID configuration:', error); + return null; + }🧰 Tools
🪛 Biome
[error] 264-264: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
src/account-db.js(3 hunks)src/accounts/openid.js(1 hunks)src/app-sync/services/files-service.js(10 hunks)src/services/user-service.js(1 hunks)src/util/validate-user.js(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/util/validate-user.js
🧰 Additional context used
📓 Learnings (3)
src/accounts/openid.js (1)
Learnt from: lelemm
PR: actualbudget/actual-server#498
File: src/accounts/openid.js:64-67
Timestamp: 2024-11-11T17:46:05.564Z
Learning: In `src/accounts/openid.js`, the `server_hostname` can accept both `http` and `https` URLs, so validation enforcing `https` is not required.
src/app-sync/services/files-service.js (1)
Learnt from: lelemm
PR: actualbudget/actual-server#498
File: src/app-sync/services/files-service.js:146-163
Timestamp: 2024-11-11T16:51:00.464Z
Learning: In `src/app-sync/services/files-service.js`, the `findUsersWithAccess` method is always called after a file find, so it does not need to independently check for invalid `fileId` or handle cases where the file doesn't exist.
src/services/user-service.js (1)
Learnt from: lelemm
PR: actualbudget/actual-server#498
File: src/services/user-service.js:155-163
Timestamp: 2024-11-11T12:57:09.322Z
Learning: In `src/services/user-service.js`, the `checkFilePermission` function is intended to check ownership, and admin access is verified separately after calling this function.
🪛 Biome
src/accounts/openid.js
[error] 264-264: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (14)
src/account-db.js (6)
5-6: LGTM! Clean implementation of authentication method listing.
The imports are well-organized, and the listLoginMethods function provides a clean API for retrieving available authentication methods.
Also applies to: 25-33
48-48: LGTM! Improved login method fallback logic.
The fallback to config.loginMethod provides better flexibility compared to the previous hardcoded approach.
52-108: LGTM! Robust implementation with proper transaction handling.
The bootstrap function effectively manages different authentication methods with proper validation and transaction safety.
122-132: LGTM! Proper validation for OpenID enablement.
The function correctly validates input and handles errors.
182-196: LGTM! Clean implementation of user info retrieval.
The functions are well-implemented with proper SQL injection prevention through parameterized queries.
135-174:
Add transaction safety for multiple database operations.
The function performs multiple database operations that should be wrapped in a transaction.
The previous review comment about transaction safety is still applicable here. Please wrap the database operations in a transaction as suggested.
src/app-sync/services/files-service.js (6)
1-1: LGTM: Clean integration of owner property
The addition of the isAdmin import and owner property to the FileBase class is well-structured and maintains proper encapsulation.
Also applies to: 14-14, 24-24
39-39: LGTM: Consistent owner property inheritance
The File class correctly extends the owner property implementation from FileBase with appropriate default value.
Also applies to: 50-50
71-71: LGTM: Proper owner field handling in updates
The FileUpdate class correctly implements the owner field with undefined default, maintaining consistency with the partial update pattern.
Also applies to: 82-82
124-146: LGTM: Well-structured file access control
The find method effectively implements file access control:
- Handles admin access appropriately
- Correctly combines owned and shared files using UNION
- Properly parameterizes the SQL query
148-165: LGTM: Comprehensive user access retrieval
The findUsersWithAccess method effectively:
- Retrieves both direct access users and file owners
- Uses appropriate JOIN operations
- Returns consistent user information fields
236-236: LGTM: Proper owner field validation
The validate method correctly includes the owner field when creating a new File instance.
src/services/user-service.js (1)
81-86:
Add input validation and error handling to insertUser
The function should validate inputs and handle potential database errors consistently with other functions in the file.
export function insertUser(userId, userName, displayName, enabled, role) {
+ if (!userId || !userName || !validateRole(role)) {
+ throw new Error('Invalid user parameters');
+ }
+ try {
getAccountDb().mutate(
'INSERT INTO users (id, user_name, display_name, enabled, owner, role) VALUES (?, ?, ?, ?, 0, ?)',
[userId, userName, displayName, enabled, role],
);
+ } catch (error) {
+ throw new Error(`Failed to insert user: ${error.message}`);
+ }
}Likely invalid or redundant comment.
src/accounts/openid.js (1)
275-295: LGTM! Well-implemented URL validation
The function correctly validates redirect URLs by comparing origins, which includes protocol, hostname, and port. The implementation is secure and handles errors appropriately.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (7)
src/services/user-service.js (4)
19-19: Optimize SELECT statements to fetch only required columnsThe queries are selecting all columns (
SELECT *) but only using theidcolumn. This can impact performance, especially if the tables have many columns.- getAccountDb().first('SELECT * FROM users WHERE id = ?', [userId]) || {}; + getAccountDb().first('SELECT id FROM users WHERE id = ?', [userId]) || {};- getAccountDb().first('SELECT * FROM files WHERE files.id = ?', [fileId]) || + getAccountDb().first('SELECT id FROM files WHERE files.id = ?', [fileId]) ||Also applies to: 28-28
33-36: Enhance role validation robustnessThe current implementation could be improved by:
- Using constants for roles to prevent typos
- Making the comparison case-insensitive
+const ROLES = { + BASIC: 'BASIC', + ADMIN: 'ADMIN', +} as const; + export function validateRole(roleId) { - const possibleRoles = ['BASIC', 'ADMIN']; - return possibleRoles.some((a) => a === roleId); + return Object.values(ROLES).some( + (role) => role.toLowerCase() === roleId?.toLowerCase() + ); }
169-178: Optimize the SQL query in getUserAccessThe current query can be simplified and optimized by:
- Using EXISTS clause instead of JOIN
- Moving the owner check to a UNION query
export function getUserAccess(fileId, userId, isAdmin) { return getAccountDb().all( - `SELECT users.id as userId, user_name as userName, files.owner, display_name as displayName - FROM users - JOIN user_access ON user_access.user_id = users.id - JOIN files ON files.id = user_access.file_id - WHERE files.id = ? and (files.owner = ? OR 1 = ?)`, + `SELECT DISTINCT u.id as userId, u.user_name as userName, + CASE WHEN f.owner = u.id THEN 1 ELSE 0 END as owner, + u.display_name as displayName + FROM users u + WHERE EXISTS ( + SELECT 1 FROM files f + WHERE f.id = ? AND ( + (f.owner = ? OR ? = 1) OR + EXISTS (SELECT 1 FROM user_access ua WHERE ua.user_id = u.id AND ua.file_id = f.id) + ) + )`, [fileId, userId, isAdmin ? 1 : 0], ); }
268-273: Consider caching OpenID configurationThe OpenID configuration is likely to change infrequently. Consider caching the result to improve performance and reduce database load.
+let openIDConfigCache = null; +const CACHE_TTL = 5 * 60 * 1000; // 5 minutes +let lastCacheTime = 0; + export function getOpenIDConfig() { + const now = Date.now(); + if (openIDConfigCache && now - lastCacheTime < CACHE_TTL) { + return openIDConfigCache; + } + + openIDConfigCache = getAccountDb().first(`SELECT * FROM auth WHERE method = ?`, ['openid']) || null + ); + lastCacheTime = now; + return openIDConfigCache; }src/accounts/openid.js (3)
109-111: Consider making the request expiry time configurableThe 5-minute expiry time for OpenID requests is hardcoded. Consider making this configurable through the application's configuration system.
-const expiry_time = now_time + 300 * 1000; +const REQUEST_EXPIRY_SECONDS = finalConfig.openid_request_expiry || 300; +const expiry_time = now_time + REQUEST_EXPIRY_SECONDS * 1000;
179-185: Consider a more unique default usernameThe current default username 'default-username' might cause conflicts if multiple users lack identification. Consider using a more unique value, perhaps incorporating a timestamp or random string.
- userInfo.name ?? - 'default-username'; + userInfo.name ?? + `user-${Date.now()}-${uuid.v4().slice(0, 8)}`;
288-295: Improve code readability with optional chainingThe nested if condition and property access can be simplified using optional chaining.
- if (auth && auth.extra_data) { - try { - const openIdConfig = JSON.parse(auth.extra_data); - return openIdConfig.server_hostname; - } catch (error) { - console.error('Error parsing OpenID configuration:', error); - } - } + try { + const openIdConfig = JSON.parse(auth?.extra_data); + return openIdConfig?.server_hostname; + } catch (error) { + console.error('Error parsing OpenID configuration:', error); + }🧰 Tools
🪛 Biome
[error] 288-288: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
src/accounts/openid.js(1 hunks)src/services/user-service.js(1 hunks)
🧰 Additional context used
📓 Learnings (2)
src/accounts/openid.js (1)
Learnt from: lelemm
PR: actualbudget/actual-server#498
File: src/accounts/openid.js:64-67
Timestamp: 2024-11-11T17:46:05.564Z
Learning: In `src/accounts/openid.js`, the `server_hostname` can accept both `http` and `https` URLs, so validation enforcing `https` is not required.
src/services/user-service.js (1)
Learnt from: lelemm
PR: actualbudget/actual-server#498
File: src/services/user-service.js:155-163
Timestamp: 2024-11-11T12:57:09.322Z
Learning: In `src/services/user-service.js`, the `checkFilePermission` function is intended to check ownership, and admin access is verified separately after calling this function.
🪛 Biome
src/accounts/openid.js
[error] 288-288: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (4)
src/accounts/openid.js (4)
1-10: LGTM: Imports are well-organized and appropriate
The imports cover all necessary dependencies for OpenID authentication implementation.
11-48: LGTM: Robust configuration validation and error handling
The function properly validates OpenID configuration, handles errors, and manages database transactions.
50-72: LGTM: Flexible issuer configuration and secure client setup
The function supports both auto-discovery and manual issuer configuration, with proper URL handling for the redirect URI.
299-319: LGTM: Secure URL validation implementation
The function properly validates redirect URLs using the URL API and origin comparison, with appropriate error handling for invalid URLs.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
src/app-account.js (1)
128-143: Consider strengthening error handling in the responseWhile the user information exposure is intentional (as confirmed in previous discussions), consider removing optional chaining to make error cases more explicit:
- res.send({ - status: 'ok', - data: { - validated: true, - userName: user?.user_name, - permission: user?.role, - userId: session?.user_id, - displayName: user?.display_name, - loginMethod: session?.auth_method, - }, - }); + res.send({ + status: 'ok', + data: { + validated: true, + userName: user.user_name, + permission: user.role, + userId: session.user_id, + displayName: user.display_name, + loginMethod: session.auth_method, + }, + });This change would cause explicit errors if any required fields are missing, making debugging easier.
src/account-db.js (2)
110-116: Consider caching frequent permission checksThe
isAdminfunction might be called frequently and makes two potential database queries. Consider caching the results for performance optimization.+const permissionCache = new Map(); +const CACHE_TTL = 60000; // 1 minute + export function isAdmin(userId) { + const cached = permissionCache.get(`admin:${userId}`); + if (cached && cached.timestamp > Date.now() - CACHE_TTL) { + return cached.value; + } + const user = getAccountDb().first('SELECT owner FROM users WHERE id = ?', [userId]) || {}; - if (user?.owner === 1) return true; - return hasPermission(userId, 'ADMIN'); + const isAdminUser = user?.owner === 1 || hasPermission(userId, 'ADMIN'); + + permissionCache.set(`admin:${userId}`, { + value: isAdminUser, + timestamp: Date.now() + }); + + return isAdminUser; }
198-207: Consider making session expiration configurableThe session cleanup threshold is hardcoded to 1 hour (3600 seconds). Consider making this configurable through the application config.
export function clearExpiredSessions() { - const clearThreshold = Math.floor(Date.now() / 1000) - 3600; + const sessionTTL = config.sessionTTL || 3600; // Default to 1 hour + const clearThreshold = Math.floor(Date.now() / 1000) - sessionTTL; const deletedSessions = getAccountDb().mutate( 'DELETE FROM sessions WHERE expires_at <> -1 and expires_at < ?', [clearThreshold], ).changes; console.log(`Deleted ${deletedSessions} old sessions`); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
src/account-db.js(3 hunks)src/app-account.js(4 hunks)
🔇 Additional comments (6)
src/app-account.js (3)
6-6: LGTM! Well-structured authentication module separation
The changes properly separate different authentication methods into dedicated modules and follow Express.js best practices for middleware setup.
Also applies to: 14-15, 19-20
34-38: LGTM! Robust error handling and consistent response structure
The endpoints properly handle errors and maintain a consistent response format. The bootstrap validation is handled correctly as confirmed in previous discussions.
Also applies to: 42-50, 52-54
80-95: Verify OpenID implementation security measures
While the redirect URL validation is in place, please ensure:
- The
isValidRedirectUrlfunction implements strict validation - Error messages don't leak sensitive information about the authentication process
Let's verify the implementation:
✅ Verification successful
Let me gather more information about the error handling in the OpenID implementation.
OpenID implementation security measures are properly implemented
The verification reveals robust security measures:
- The
isValidRedirectUrlfunction implements strict validation by:- Comparing full origin (protocol + hostname + port)
- Only allowing same-origin redirects
- Properly handling malformed URLs
- Error messages are security-conscious:
- Using generic codes like 'openid-setup-failed' instead of exposing internal errors
- Logging detailed errors server-side while returning safe messages to clients
- Additional security features found:
- PKCE implementation (code_verifier/code_challenge)
- State parameter for CSRF protection
- Request expiry handling
- Proper scope restrictions
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check the redirect URL validation implementation
echo "Checking redirect URL validation..."
ast-grep --pattern 'function isValidRedirectUrl($_) {
$$$
}'
# Check for potential information leakage in error messages
echo "Checking error handling..."
rg -A 2 "status: 'error'" src/accounts/openid.js
Length of output: 1265
Script:
#!/bin/bash
# Check the loginWithOpenIdSetup implementation for error handling
echo "Checking OpenID setup error handling..."
ast-grep --pattern 'async function loginWithOpenIdSetup($_) {
$$$
}'
# Check for error messages in the OpenID module
echo "Checking error messages..."
rg "error.*=|throw.*Error" src/accounts/openid.js
# Check getServerHostname implementation
echo "Checking server hostname retrieval..."
ast-grep --pattern 'function getServerHostname() {
$$$
}'
Length of output: 4489
src/account-db.js (3)
25-33: LGTM! Clean implementation of login methods listing
The function provides a well-structured interface for retrieving authentication methods, with proper field mapping and clear return types.
103-103:
Verify the conditional return value handling
The return value differs based on the authentication method used. For password auth, it returns the login result, but for OpenID, it returns an empty object. This asymmetry might cause issues in the calling code.
#!/bin/bash
# Check how the bootstrap function's return value is used
rg -A 5 'bootstrap\(' .Consider standardizing the return value:
- return passEnabled ? loginWithPassword(loginSettings.password) : {};
+ const result = passEnabled ? await loginWithPassword(loginSettings.password) : {};
+ return { ...result, authMethod: passEnabled ? 'password' : 'openid' };48-48: Consider adding validation for loginMethod value
While the login method fallback logic is good, it might be worth validating that the provided loginMethod is one of the supported values.
if (
typeof req !== 'undefined' &&
(req.body || { loginMethod: null }).loginMethod
) {
- return req.body.loginMethod;
+ const method = req.body.loginMethod;
+ if (!['password', 'openid'].includes(method)) {
+ return config.loginMethod || 'password';
+ }
+ return method;
}
| usersWithAccess: fileService | ||
| .findUsersWithAccess(row.id) | ||
| .map((access) => ({ | ||
| ...access, | ||
| owner: access.userId === row.owner, | ||
| })), |
There was a problem hiding this comment.
It may not be for this PR, but this seems a little inefficient to me. I think it would be better to have findUsersWithAccess(rows.map(({id}) => id)) so we don't have to look at disk for each separate file. What do you think?
(same comment below for /get-user-file-info)
There was a problem hiding this comment.
yes, this is inefficient. This code was totally different, but I had to merge my code with the fileservice (it didn't exist before I started doing the openid). I tried to not change too much the fileservice
| }); | ||
|
|
||
| app.get('/config', async (req, res) => { | ||
| const { cnt: ownerCount } = UserService.getOwnerCount() || {}; |
There was a problem hiding this comment.
Correct me if I am wrong: it seems like the intent is that if OpenID is enabled, then the first user to connect to it will become the admin. IIRC, we also disable server-wide password auth if OpenID is enabled. In that case, isn't there a race to see who can create the first account? I.e., theoretically, a bad actor can gain admin control over any new server this way? If this is indeed possible, it does seems like an edge case we don't need to resolve in this PR, but we should definitely make sure to address it before the next release. Happy to be told which of my assumptions is wrong here, though!
There was a problem hiding this comment.
Yes, you are correct. There is a race condition here. A bad actor could gain access being the first to login. We could maybe mitigate this by forcing the first user to input the server password too. Can we improve this on next release?
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (8)
jest.global-setup.js (1)
64-64: Move NEVER_EXPIRES constant to the top.For consistency, consider moving the NEVER_EXPIRES constant to the top with other constants.
src/account-db.js (4)
25-33: Consider adding JSDoc for better type documentationThe function provides a clean interface for fetching login methods. Consider adding JSDoc to document the return type structure.
+/** + * Lists all available login methods + * @returns {Array<{method: string, active: boolean, displayName: string}>} + */ export function listLoginMethods() {
110-117: Consider a more flexible permission modelThe current implementation suggests a single-role system where each user has one role. Consider if a more flexible permission model (e.g., multiple roles or granular permissions) might be needed in the future.
178-192: Enhance error handling for user queriesThe functions silently handle missing users by returning undefined or an empty role. Consider:
- Throwing errors for missing users
- Documenting the behavior for missing users
- Using a constant for the default role instead of an empty string
+const DEFAULT_ROLE = ''; + export function getUserPermission(userId) { let accountDb = getAccountDb(); const { role } = accountDb.first( `SELECT role FROM users WHERE users.id = ?`, [userId], - ) || { role: '' }; + ) || { role: DEFAULT_ROLE }; return role; }
194-203: Improve session cleanup configuration and error handlingConsider these enhancements:
- Make the expiration threshold configurable
- Add error handling for database operations
- Use a more precise logging level (e.g., debug for routine cleanup, warn for large deletions)
+const DEFAULT_SESSION_EXPIRY_HOURS = 1; + export function clearExpiredSessions() { - const clearThreshold = Math.floor(Date.now() / 1000) - 3600; + const expiryHours = config.sessionExpiryHours || DEFAULT_SESSION_EXPIRY_HOURS; + const clearThreshold = Math.floor(Date.now() / 1000) - (expiryHours * 3600); + + try { const deletedSessions = getAccountDb().mutate( 'DELETE FROM sessions WHERE expires_at <> -1 and expires_at < ?', [clearThreshold], ).changes; - console.log(`Deleted ${deletedSessions} old sessions`); + if (deletedSessions > 100) { + console.warn(`Deleted ${deletedSessions} old sessions`); + } else { + console.debug(`Deleted ${deletedSessions} old sessions`); + } + } catch (error) { + console.error('Failed to clear expired sessions:', error); + throw error; + } }src/app-sync/services/files-service.js (1)
148-165: LGTM with a minor readability suggestion.The implementation correctly retrieves both owners and users with access. Consider improving query readability:
- `SELECT UA.user_id as userId, users.display_name displayName, users.user_name userName + `SELECT + UA.user_id as userId, + users.display_name as displayName, + users.user_name as userNamesrc/load-config.js (2)
153-198: Consider extracting OpenID configuration logic to a separate function.The IIFE makes the code harder to test and maintain. Consider extracting this to a named function for better readability and testability.
- openId: (() => { + openId: buildOpenIdConfig(process.env, config.openId), +}; + +function buildOpenIdConfig(env, defaultOpenId) { if ( - !process.env.ACTUAL_OPENID_DISCOVERY_URL && - !process.env.ACTUAL_OPENID_AUTHORIZATION_ENDPOINT + !env.ACTUAL_OPENID_DISCOVERY_URL && + !env.ACTUAL_OPENID_AUTHORIZATION_ENDPOINT ) { - return config.openId; + return defaultOpenId; } // ... rest of the function ... - })(),
199-201: Add validation and documentation for token expiration values.Consider adding validation for the token expiration format and documenting accepted values:
- Time-based values (e.g., '24h', '7d')
- Special value 'never'
+const VALID_TIME_UNITS = ['h', 'd', 'w', 'm', 'y']; + +function validateTokenExpiration(value) { + if (value === 'never') return value; + const match = value.match(/^(\d+)([hdwmy])$/); + if (!match || !VALID_TIME_UNITS.includes(match[2])) { + throw new Error( + 'Invalid token_expiration format. Use "never" or "<number><unit>" where unit is one of: ' + + VALID_TIME_UNITS.join(', ') + ); + } + return value; +} token_expiration: process.env.ACTUAL_TOKEN_EXPIRATION - ? process.env.ACTUAL_TOKEN_EXPIRATION + ? validateTokenExpiration(process.env.ACTUAL_TOKEN_EXPIRATION) : config.token_expiration,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
jest.global-setup.js(1 hunks)src/account-db.js(3 hunks)src/app-sync/services/files-service.js(10 hunks)src/app-sync/tests/services/files-service.test.js(4 hunks)src/load-config.js(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/app-sync/tests/services/files-service.test.js
🔇 Additional comments (13)
jest.global-setup.js (3)
4-7: LGTM! Well-defined constants.
The constants follow a consistent naming convention and are self-documenting.
9-37: LGTM! Robust implementation with proper validation.
The function implementation addresses all previous review comments with thorough parameter validation and error handling.
72-94: Clarify the session expiration strategy.
The comment "or consider using a far future timestamp" suggests uncertainty about the expiration strategy. Consider:
- Using a specific far future timestamp (e.g., 10 years from now) for test sessions
- Adding a comment explaining why -1 was chosen as NEVER_EXPIRES
- Documenting how this aligns with the production expiration strategy
This will help other developers understand the rationale behind the expiration handling.
src/account-db.js (2)
5-6: LGTM! Good separation of concerns
The authentication logic has been properly modularized into separate files for password and OpenID handling, following good software engineering practices.
103-103: Clarify the asymmetric return value
The function returns different values based on the authentication method:
- For password auth: Returns result from
loginWithPassword - For OpenID auth: Returns an empty object
Consider documenting why this difference exists or standardizing the return value.
✅ Verification successful
Let me check the loginWithPassword implementation to understand its return value structure.
Return value asymmetry is intentional and correct
The return value difference is justified based on the authentication flow:
- Password auth returns
{ token }on success or{ error }on failure, which is needed for session management - OpenID auth returns
{}because token management is handled separately by the OpenID provider
The code in app-account.js correctly handles both cases by only checking for errors (boot?.error). No changes needed.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check how the return values are handled in the calling code
rg -A 5 "bootstrap\(.*\)" --type js
Length of output: 1065
Script:
#!/bin/bash
# Check loginWithPassword implementation and usage
ast-grep --pattern 'function loginWithPassword($_) {
$$$
}'
# Also check for any other usages
rg "loginWithPassword" --type js
Length of output: 4701
src/app-sync/services/files-service.js (5)
1-1: LGTM: Clean implementation of owner property.
The owner property is consistently added to the FileBase class with proper imports.
Also applies to: 14-14, 24-24
39-39: LGTM: Consistent constructor implementations.
The owner property is appropriately added with suitable defaults:
nullfor new File instancesundefinedfor FileUpdate to support partial updates
Also applies to: 50-50, 71-71, 82-82
108-108: LGTM: SQL query correctly updated with owner field.
The property name inconsistency (userId vs owner) has been fixed as per previous review comments.
Also applies to: 119-119
236-236: LGTM: Consistent validation implementation.
The owner property is correctly included in the File instance creation.
124-146: Consider adding an index for performance optimization.
The UNION query joins multiple tables and filters by user access. Consider adding an index on user_access(file_id, user_id) to improve query performance.
src/load-config.js (3)
80-81: LGTM: Default configuration additions are appropriate.
The default values are well-chosen:
multiuser: falseprovides a secure defaulttoken_expiration: 'never'maintains backward compatibility with existing non-expiring token support
110-118: Implementation matches previous review suggestion.
209-209: LGTM: Improved debug logging reliability.
Good defensive programming practice by providing a fallback value for undefined loginMethod.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
@jfdoming check my comments when you can please |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
jest.global-setup.js (1)
64-64: Consider using a more explicit expiration strategy.Using
-1asNEVER_EXPIRESmight not be the most maintainable approach. Consider using a far future timestamp or defining this at a higher level in the application's configuration.- const NEVER_EXPIRES = -1; // or consider using a far future timestamp + const NEVER_EXPIRES = new Date('2099-12-31').getTime();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
jest.global-setup.js(1 hunks)
🔇 Additional comments (4)
jest.global-setup.js (4)
4-7: LGTM! Well-defined constants.
The constants are clearly named and appropriately defined for test user and role identifiers.
9-37: Previous review suggestions implemented successfully.
The function now includes comprehensive parameter validation and error handling as suggested in the previous reviews.
39-61: Previous review suggestions implemented successfully.
The function now includes proper error handling, session validation, and database connection reuse as suggested in the previous reviews.
72-94: Verify session cleanup across test runs.
The transaction handling for session setup is good, but we should verify that sessions are properly cleaned up between test runs to prevent test interference.
✅ Verification successful
Let me gather more information about session cleanup in test files.
Sessions are properly managed through global test setup and teardown
The session cleanup is well-handled through Jest's global setup and teardown mechanism:
- Sessions are cleared and re-initialized in
jest.global-setup.jsat the start of test runs - Database is completely torn down via migrations in
jest.global-teardown.js - Additional safeguards exist through transaction rollback in case of setup failures
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any lingering sessions from previous test runs
# Search for session cleanup in test files
rg -l 'DELETE FROM sessions' --type js
# Check for session cleanup in afterAll/afterEach blocks
ast-grep --pattern 'afterAll($$$) {
$$$
DELETE FROM sessions
$$$
}'
Length of output: 193
Script:
#!/bin/bash
# Check for session cleanup in test teardown
rg "DELETE FROM sessions" -B 5 -A 5
# Look for global test setup/teardown
fd "jest.global" --type f --exec cat {}
# Check for any beforeAll/afterAll blocks that might handle sessions
ast-grep --pattern 'beforeAll($$$) {
$$$
}'
ast-grep --pattern 'afterAll($$$) {
$$$
}'
Length of output: 5920
Cleaning up the PR #381