-
-
Notifications
You must be signed in to change notification settings - Fork 605
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Openid #381
base: master
Are you sure you want to change the base?
Openid #381
Conversation
Note this is only disallowed at bootstrap. If the database is edited manually, we make no attempt to detect this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 30
🧹 Outside diff range and nitpick comments (10)
src/app-secrets.js (1)
32-32
: Remove unnecessary return statement.The
return null;
statement after sending the error response is unnecessary. The function execution will naturally end after sending the response.Apply this diff to remove the unnecessary return:
- return null;
src/util/middlewares.js (2)
22-24
: Approve changes with a minor suggestion.The renaming from
validateUserMiddleware
tovalidateSessionMiddleware
and the corresponding logic updates are consistent and improve the clarity of the session-based validation approach.Consider adding a type annotation for the
session
variable to improve code readability:-let session = await validateSession(req, res); +let session: SessionType = await validateSession(req, res);Replace
SessionType
with the appropriate type definition for your session object.Also applies to: 27-29
28-28
: Consider future improvements for session management.Acknowledging the previous discussion about express-session, the current implementation is a reasonable solution for the immediate needs. However, as the application grows, consider the following improvements:
- Implement a custom session store to persist sessions across server restarts.
- Add session expiration and renewal logic.
- Implement CSRF protection for session security.
These enhancements can be made incrementally without immediately adopting express-session.
src/db.js (1)
65-65
: Add a comment explaining the CHUNK_SIZE constant.The
CHUNK_SIZE
of 999 is used to avoid SQL query limits, but this might not be immediately clear to other developers. Consider adding a comment to explain the reasoning behind this specific value.Here's a suggested improvement:
- const CHUNK_SIZE = 999; + // Use 999 as CHUNK_SIZE to stay below the common SQL limit of 1000 placeholders per query + const CHUNK_SIZE = 999;src/app.js (1)
Line range hint
1-97
: Overall assessment: Enhancements for authentication and user management.The changes in this file successfully introduce new functionality for admin and OpenID routes, along with improved authentication checks. The additions are consistent with the existing code structure and follow good practices. The
toggleAuthentication
check adds an important safeguard to ensure proper authentication configuration before the server starts.Consider addressing the minor suggestions from previous reviews, such as using
process.exit(-1)
and removing the unnecessaryexit
import.src/app-gocardless/app-gocardless.js (1)
Line range hint
1-262
: Coordinate with related PR and update documentation.Given that this PR is meant to work in conjunction with PR #2945, it's important to ensure that the changes in both PRs are compatible and complementary.
Consider the following recommendations:
Cross-reference the changes in PR #2945 to ensure they align with the middleware update in this file.
Update any relevant documentation, including:
- API documentation to reflect the new session-based authentication
- Developer guides or READMEs that explain the authentication flow
- Any configuration files that might need adjustments for the new middleware
If not already done, consider adding comments in the code to explain the rationale behind the middleware change and any specific behaviors developers should be aware of.
Evaluate if this change necessitates updates to client-side code or SDKs that interact with these routes.
These steps will help maintain consistency across the project and ensure that all team members understand the implications of this authentication change.
src/app-admin.js (1)
1-322
: Summary of review for src/app-admin.jsOverall, this file provides a comprehensive set of endpoints for user and file management with good attention to security through session validation and permission checks. However, there are several areas for improvement:
- Asynchronous operations: Ensure proper handling of asynchronous functions, especially for
isAdmin
checks.- Error handling: Implement more robust error handling, particularly for database operations.
- Performance: Consider optimizing bulk operations, such as user deletion.
- Security: Review the exposure of configuration data and implement stricter input validation.
- Code structure: Consider breaking down larger route handlers into smaller, more manageable functions.
- Consistency: Ensure consistent use of async/await throughout the file.
Addressing these points will significantly improve the robustness, security, and maintainability of the application.
src/app-sync.js (1)
Line range hint
1-424
: Overall improvements in security and access control.The changes in this file significantly enhance security, access control, and user management. Key improvements include:
- Better session validation
- Admin-specific functionality
- Improved file ownership tracking
- More granular access control in file listing
These modifications effectively address the previous concerns about permission checks.
Consider adding more robust error handling for the new logic, especially in the
/list-user-files
endpoint. For example:app.get('/list-user-files', (req, res) => { try { const canSeeAll = isAdmin(req.userSession.user_id); // ... existing logic ... } catch (error) { console.error('Error in list-user-files:', error); res.status(500).send({ status: 'error', message: 'Internal server error' }); } });This will ensure that any unexpected errors are caught and handled gracefully.
src/app-openid.js (1)
60-60
: Typo in error message reason: 'already-bootstraped'There's a typo in the error message on line 60: 'already-bootstraped' should be corrected to 'already-bootstrapped'.
src/account-db.js (1)
216-219
: Simplify variable naming for clarityThe variable
enabledMethodsButOpenId
could be renamed for better readability. Consider using a name likenonOpenIdMethodsEnabled
orenabledNonOpenIdMethodsCount
to more clearly convey its purpose.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (20)
- jest.global-setup.js (1 hunks)
- migrations/1718889148000-openid.js (1 hunks)
- migrations/1719409568000-multiuser.js (1 hunks)
- src/account-db.js (3 hunks)
- src/accounts/openid.js (1 hunks)
- src/accounts/password.js (1 hunks)
- src/app-account.js (4 hunks)
- src/app-admin.js (1 hunks)
- src/app-admin.test.js (1 hunks)
- src/app-gocardless/app-gocardless.js (2 hunks)
- src/app-openid.js (1 hunks)
- src/app-secrets.js (1 hunks)
- src/app-sync.js (5 hunks)
- src/app.js (3 hunks)
- src/config-types.ts (2 hunks)
- src/db.js (1 hunks)
- src/load-config.js (4 hunks)
- src/services/user-service.js (1 hunks)
- src/util/middlewares.js (3 hunks)
- src/util/validate-user.js (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- migrations/1718889148000-openid.js
- migrations/1719409568000-multiuser.js
- src/accounts/openid.js
- src/app-admin.test.js
- src/config-types.ts
🧰 Additional context used
🪛 Biome
src/app-account.js
[error] 86-86: Useless case clause.
because the default clause is present:
Unsafe fix: Remove the useless case.
(lint/complexity/noUselessSwitchCase)
src/services/user-service.js
[error] 2-190: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
🔇 Additional comments (26)
src/app-secrets.js (4)
26-30
: Approve error response structure.The error response structure looks good and follows the suggested format from a previous review. It provides clear information about the reason for the error, which is helpful for debugging and user feedback.
17-18
:⚠️ Potential issueFix asynchronous database query.
The database query is not properly awaited, which could lead to unexpected behavior. This issue was previously identified in a past review.
Apply this diff to fix the issue:
- const { method } = - getAccountDb().first('SELECT method FROM auth WHERE active = 1') || {}; + const { method } = + (await getAccountDb().first('SELECT method FROM auth WHERE active = 1')) || {};
22-34
:⚠️ Potential issueFix asynchronous admin check and approve error response structure.
The admin check is not properly awaited, which could lead to unexpected behavior if
isAdmin
is an asynchronous function. This issue was previously identified in a past review. The error response structure looks good and follows the suggested format.Apply this diff to fix the asynchronous issue:
- let canSaveSecrets = isAdmin(req.userSession.user_id); + let canSaveSecrets = await isAdmin(req.userSession.user_id);
3-3
: Approve changes to imports and middleware.The new imports and middleware change appear to be in line with the PR objectives. However, it's important to ensure that these changes are consistent across the codebase.
Let's verify the usage of these new imports and the middleware change:
Also applies to: 6-6
src/util/middlewares.js (1)
46-46
: Export statement correctly updated.The export statement has been properly updated to reflect the renaming of
validateUserMiddleware
tovalidateSessionMiddleware
.jest.global-setup.js (2)
15-20
: Implement suggested improvements from previous review.The
setSessionUser
function could benefit from the improvements suggested in the previous review:
- Make the token a parameter for increased flexibility.
- Add error handling and return the result for better feedback.
Here's the suggested implementation:
const setSessionUser = (userId, token = 'valid-token') => { return getAccountDb().mutate('UPDATE sessions SET user_id = ? WHERE token = ?', [ userId, token, ]).then(result => { if (result.changes === 0) { throw new Error('No session updated. Token might be invalid.'); } return result; }); };These changes will make the function more flexible, provide better error handling, and give feedback on the operation's success.
34-44
: Ensure consistent session setup.The session management logic looks good overall. Deleting all sessions and creating new ones ensures a clean state for tests. However, there's still an inconsistency in the session setup:
- Two session records are created, one for 'valid-token' and another for 'valid-token-admin'.
setSessionUser
is called only for 'valid-token', but not for 'valid-token-admin'.To address this, consider one of the following options:
If
setSessionUser
is necessary, call it for both tokens:setSessionUser('genericAdmin', 'valid-token'); setSessionUser('genericAdmin', 'valid-token-admin');If
setSessionUser
is redundant (since the sessions are already created with user IDs), you might want to remove thesetSessionUser
call entirely.Choose the option that best fits your testing requirements and ensures consistency across all session setups.
src/util/validate-user.js (4)
4-4
: LGTM: Import statement reordering.The repositioning of the
getSession
import statement is acceptable and doesn't affect the functionality.
6-6
: LGTM: New constant for token expiration.The introduction of
TOKEN_EXPIRATION_NEVER
is a good practice. It provides a clear, semantic constant for representing tokens that never expire, improving code readability and maintainability.
31-42
: LGTM: Token expiration check enhances security.The new token expiration check is a valuable addition that improves the application's security. It correctly uses the
TOKEN_EXPIRATION_NEVER
constant and provides an appropriate error response.The use of a 401 status code for expired tokens aligns with HTTP standards and addresses the previous review comments. Well implemented!
12-12
: LGTM: Function rename improves clarity.The renaming of
validateUser
tovalidateSession
better reflects the function's purpose. This change enhances code readability and maintainability.Please ensure that all references to this function have been updated across the codebase. Run the following script to verify:
src/db.js (1)
53-77
: LGTM! Well-implemented deleteByIds method.The new
deleteByIds
method is a solid implementation for bulk deletion of records. It handles large arrays of IDs efficiently by using a chunking approach, and it properly accumulates and returns the total number of deleted rows.src/app.js (4)
14-15
: LGTM: New imports for admin and OpenID functionality.The addition of
adminApp
andopenidApp
imports aligns with the PR objectives and follows the existing import pattern in the file.
16-16
: LGTM: Import of toggleAuthentication function.The import of
toggleAuthentication
from './account-db.js' is correct and aligns with its usage later in the file.
55-56
: LGTM: New route handlers for admin and OpenID.The addition of middleware for '/admin' and '/openid' routes is consistent with the existing pattern and aligns with the PR objectives for enhancing authentication and user management.
94-94
: LGTM: Usage of toggleAuthentication for critical error handling.The addition of the
toggleAuthentication
check in therun
function is a good practice for ensuring the application only starts if authentication is properly configured.As a reminder, consider using
process.exit(-1)
instead ofexit(-1)
as suggested in a previous review comment.src/app-gocardless/app-gocardless.js (2)
Line range hint
1-262
: Ensure comprehensive testing of all routes with the new middleware.While the middleware change is the only modification in this file, it's crucial to verify that all existing routes continue to function correctly with the new
validateSessionMiddleware
. The shift from user-based to session-based validation could potentially affect the behavior of some routes.To ensure the integrity of the application, please conduct thorough testing:
Test all existing routes (
/link
,/status
,/create-web-token
,/get-accounts
,/get-banks
,/remove-account
,/transactions
) with various scenarios, including:
- Valid sessions
- Invalid or expired sessions
- Different user roles or permissions (if applicable)
Pay special attention to error handling, especially in the
/transactions
route, to ensure that the new middleware doesn't interfere with the existing error categorization and responses.Verify that the
goCardlessService
methods are still receiving the necessary user information from the new session-based middleware.Consider adding or updating integration tests to cover these scenarios systematically.
17-17
: Middleware update looks good, but verify compatibility.The change from
validateUserMiddleware
tovalidateSessionMiddleware
aligns with the PR objectives related to OpenID integration. This update suggests a shift towards session-based authentication, which is a common approach in OpenID implementations.To ensure this change doesn't introduce any unintended side effects, please run the following verification:
This script will help identify any remaining references to the old middleware, confirm the correct usage of the new middleware, and highlight any potential conflicts in route handlers that might be affected by this change.
Also applies to: 29-29
✅ Verification successful
Middleware replacement verified successfully.
All references to
validateUserMiddleware
have been removed, andvalidateSessionMiddleware
is correctly implemented across relevant modules. No conflicts detected in route handlers.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the impact of the new middleware on existing routes # Test 1: Check for any remaining references to validateUserMiddleware echo "Checking for remaining references to validateUserMiddleware:" rg "validateUserMiddleware" --type js # Test 2: Verify that validateSessionMiddleware is imported and used correctly echo "Verifying validateSessionMiddleware usage:" rg "validateSessionMiddleware" --type js -C 2 # Test 3: Check for any potential conflicts or issues in route handlers echo "Checking route handlers for potential conflicts:" rg "app\.(get|post|put|delete|patch)" --type js -C 5Length of output: 25851
src/app-sync.js (3)
338-354
: Improved access control in file listing endpoint.The new logic effectively differentiates between admin and non-admin users, addressing the permission check concern raised in the past review. Admins can see all files, while non-admin users only see their own and shared files. The union query efficiently handles this distinction.
This change addresses the past review comment about adding permission checks to endpoints.
To ensure the SQL query's performance, especially for large datasets, please run the following verification:
#!/bin/bash # Description: Verify the performance of the list-user-files SQL query echo "Checking list-user-files SQL query:" rg -A 20 "app.get\('/list-user-files'" src/app-sync.js echo "Analyzing query execution plan (if possible):" echo "Please run an EXPLAIN QUERY PLAN for both admin and non-admin cases on a test database with a significant amount of data to ensure query efficiency."
364-364
: Added owner information to file listing response.The inclusion of the 'owner' field in the response provides valuable information about file ownership to the client. This aligns well with the earlier change of adding the 'owner' field during file upload.
To ensure proper handling of this new field on the client-side, please run the following verification:
#!/bin/bash # Description: Verify client-side handling of the new 'owner' field echo "Checking client-side code for handling 'owner' field:" rg "data\s*=>\s*\{[^}]*owner" src/components/FileList.js src/components/FileDetails.js echo "Ensure that the client-side code properly handles and displays the 'owner' information where appropriate."
8-8
: Improved session validation and admin checks.The changes from
validateUserMiddleware
tovalidateSessionMiddleware
and the addition ofisAdmin
import suggest enhanced session management and admin-specific functionality. These improvements align well with better security and access control.To ensure the new middleware is correctly implemented, please run the following verification:
Also applies to: 15-15, 24-24
✅ Verification successful
Middleware and isAdmin function implementations verified.
The implementations of
validateSessionMiddleware
insrc/util/middlewares.js
andisAdmin
insrc/account-db.js
have been confirmed.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the implementation of validateSessionMiddleware and isAdmin function # Check validateSessionMiddleware implementation echo "Checking validateSessionMiddleware implementation:" rg -A 10 "export const validateSessionMiddleware" src/util/middlewares.js # Check isAdmin function implementation echo "Checking isAdmin function implementation:" rg -A 5 "export const isAdmin" src/account-db.jsLength of output: 330
Script:
#!/bin/bash # Description: Re-verify the implementation of validateSessionMiddleware and isAdmin function with adjusted patterns # Check validateSessionMiddleware implementation echo "Re-checking validateSessionMiddleware implementation:" rg "validateSessionMiddleware" src/util/middlewares.js # Check isAdmin function implementation echo "Re-checking isAdmin function implementation:" rg "isAdmin" src/account-db.jsLength of output: 477
src/app-openid.js (1)
57-57
:⚠️ Potential issueVerify if 'UserService.getOwnerCount()' and 'UserService.getOpenIDConfig()' are Asynchronous
The methods
UserService.getOwnerCount()
(line 57) andUserService.getOpenIDConfig()
(line 64) are being called withoutawait
. Please verify whether these methods are asynchronous. If they return promises,await
should be used to handle the asynchronous calls properly.Run the following script to check the function definitions:
Also applies to: 64-64
✅ Verification successful
UserService.getOwnerCount()
andUserService.getOpenIDConfig()
are synchronous functions. There is no need to useawait
when calling these methods.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if 'getOwnerCount' and 'getOpenIDConfig' are asynchronous functions. # Search for the function definitions in 'UserService'. rg --type javascript -A 5 'getOwnerCount' rg --type javascript -A 5 'getOpenIDConfig'Length of output: 155
Script:
#!/bin/bash # Description: Check if 'getOwnerCount' and 'getOpenIDConfig' are asynchronous functions. # Search for the function definitions in 'UserService'. rg --type js -A 5 'getOwnerCount' rg --type js -A 5 'getOpenIDConfig'Length of output: 1899
Script:
#!/bin/bash # Description: Verify if 'getOwnerCount' and 'getOpenIDConfig' are asynchronous functions in UserService. # Check if 'getOwnerCount' is declared as an async function. ast-grep --lang javascript --pattern 'class UserService { $$$ async getOwnerCount($_) { $$$ } $$$ }' # Check if 'getOpenIDConfig' is declared as an async function. ast-grep --lang javascript --pattern 'class UserService { $$$ async getOpenIDConfig($_) { $$$ } $$$ }'Length of output: 207
src/accounts/password.js (1)
31-108
: Review User and Session Management for Multi-User SupportThe current implementation appears to primarily handle a single user with an empty
user_name
. In a multi-user environment, this could lead to issues with user identification and session management.Consider modifying the authentication flow to support multiple users:
- Accept a
username
parameter in addition to thepassword
.- Update database queries to use the provided
username
.- Ensure that sessions are correctly associated with the authenticated user.
To verify if multi-user support is needed, you can run the following script:
This script helps identify if multi-user functionality is present or needs to be implemented.
✅ Verification successful
Multi-User Support is Properly Implemented
After reviewing the codebase, it is clear that multi-user functionality is supported. The use of an empty
user_name
inpassword.js
serves as a bootstrap mechanism for the initial admin user. Subsequent users are managed with properuser_name
fields, ensuring correct user identification and session handling.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for code related to multi-user support. # Test: Search for functions that handle multiple users. # Expect: Functions that accept or manage different usernames. rg --type js -A 5 "function .*User" src/ # Search for routes or handlers that involve usernames. rg --type js -A 5 "user_name" src/Length of output: 8376
src/load-config.js (3)
74-75
: Appropriate default settings for 'multiuser' and 'token_expiration'The addition of
multiuser
andtoken_expiration
todefaultConfig
with suitable default values enhances configurability.
178-180
: Configuration of 'token_expiration' via environment variableThe inclusion allows
token_expiration
to be set throughACTUAL_TOKEN_EXPIRATION
or defaults to the value inconfig
, enhancing flexibility.
74-75
: Verify usage of new configuration propertiesEnsure that the new
multiuser
andtoken_expiration
configurations are properly handled throughout the application.You can run the following script to verify where these properties are used:
✅ Verification successful
Configuration properties
multiuser
andtoken_expiration
are properly handled throughout the application.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify usage of 'config.multiuser' and 'config.token_expiration' in the codebase # Test: Search for occurrences of 'config.multiuser' and 'config.token_expiration' rg --type javascript 'config\.multiuser|config\.token_expiration' -A 5Length of output: 107
Script:
#!/bin/bash # Description: Verify usage of 'config.multiuser' and 'config.token_expiration' in the codebase # Test: Search for occurrences of 'config.multiuser' and 'config.token_expiration' rg --type js 'config\.multiuser|config\.token_expiration' -A 5Length of output: 915
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (2)
src/app-sync.js (1)
Line range hint
1-448
: Summary of changes and recommendations for further improvementsThe changes in this file have significantly enhanced security, access control, and file ownership tracking. Key improvements include:
- Transition to session-based validation
- Implementation of role-based access control for file listing
- Enhanced file ownership tracking in the upload process
While these changes are positive, there are areas for further improvement:
- Implement comprehensive permission checks across all endpoints, preferably using middleware for consistency.
- Optimize database queries, particularly in the
/list-user-files
endpoint, by adding appropriate indexes.- Ensure proper error handling and input validation across all endpoints.
Consider the following architectural improvements:
- Separate route handlers into individual files or modules for better organization and maintainability.
- Implement a centralized error handling mechanism.
- Use dependency injection for database access to improve testability.
To facilitate these improvements, it would be beneficial to conduct a comprehensive security audit and performance profiling of the application.
src/services/user-service.js (1)
183-185
: Avoid throwing generic errors without contextIn the
deleteUserAccessByFileId
method, the error thrown whenuserIds
is invalid provides limited information. Including more context in the error message can aid in debugging and improve developer experience.Consider updating the error message to include the invalid
userIds
value:if (!Array.isArray(userIds) || userIds.length === 0) { - throw new Error('The provided userIds must be a non-empty array.'); + throw new Error(\`Invalid userIds provided: \${JSON.stringify(userIds)}. Expected a non-empty array.\`); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- src/app-admin.js (1 hunks)
- src/app-sync.js (5 hunks)
- src/services/user-service.js (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/app-admin.js
🧰 Additional context used
🪛 Biome
src/services/user-service.js
[error] 2-244: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
🔇 Additional comments (4)
src/app-sync.js (3)
263-271
: Enhanced file ownership tracking in upload endpoint.The addition of the 'owner' field in the INSERT query, set to
req.userSession.user_id
, improves file ownership tracking. This change enhances the data model and strengthens the user-file relationship.To ensure proper user session handling, please run the following verification:
#!/bin/bash # Description: Verify user session handling in the upload-user-file endpoint echo "Checking user session handling in upload-user-file endpoint:" rg -A 10 "app.post\('/upload-user-file'" src/app-sync.js echo "Checking userSession object structure:" rg "req.userSession" src/app-sync.js
15-15
:⚠️ Potential issuePartial implementation of permission checks, further improvements needed.
While some endpoints (like
/list-user-files
) now implement role-based access control, the concern raised in the previous comment about applying permission checks to most endpoints remains partially unaddressed.Consider implementing a middleware solution for bulk application of permission checks to relevant endpoints. This would enhance security and ensure consistent access control across the application.
Example middleware implementation:
function permissionCheckMiddleware(req, res, next) { // Check if the user has permission to access the requested resource // This could involve checking the user's role, the resource they're trying to access, etc. if (userHasPermission(req.userSession, req.path)) { next(); } else { res.status(403).send({ status: 'error', reason: 'permission-denied' }); } } // Apply the middleware to all routes that need permission checks app.use('/protected-routes', permissionCheckMiddleware);To assess the current state of permission checks, run the following:
#!/bin/bash # Description: Analyze current permission check implementation echo "Checking for existing permission checks:" rg "isAdmin|userHasPermission|req\.userSession" src/app-sync.js echo "Listing all route handlers:" rg "app\.(get|post|put|delete|patch)" src/app-sync.jsCompare the output to identify routes that might need additional permission checks.
Line range hint
338-384
: Improved access control and file listing functionality.The changes in the
/list-user-files
endpoint significantly enhance access control and file listing functionality:
- Implemented role-based access control, allowing admins to see all files while regular users see only their own and shared files.
- Added a query to fetch comprehensive user access information.
- Enhanced the response to include owner and access information.
These improvements provide better security and more detailed file access information to the client.
To ensure the queries are performant and secure, please run the following verification:
#!/bin/bash # Description: Verify the performance and security of the list-user-files queries echo "Checking list-user-files query structure:" rg -A 20 "app.get\('/list-user-files'" src/app-sync.js echo "Verifying index existence for performance:" sqlite3 path/to/your/database.sqlite "SELECT name FROM sqlite_master WHERE type='index' AND (tbl_name='files' OR tbl_name='user_access' OR tbl_name='users');" echo "Checking for potential SQL injection vulnerabilities:" rg "SELECT.*FROM.*WHERE" src/app-sync.jsAlso, consider adding appropriate indexes to the
files
,user_access
, andusers
tables to optimize query performance, especially for larger datasets.src/services/user-service.js (1)
195-198
: Ensure proper parameter ordering in SQL queriesWhen constructing the
DELETE
statement indeleteUserAccessByFileId
, it's important to ensure that the parameters correspond correctly to the placeholders, especially when dealing with batched queries.Please verify that the parameters are correctly aligned with the placeholders in the SQL query.
✅ Verification successful
Placeholders and parameters are correctly aligned in the SQL query.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check parameter alignment in the DELETE query within deleteUserAccessByFileId. # Expected: The number of placeholders should match the number of parameters passed. # Count the number of placeholders PLACEHOLDER_COUNT=$(echo "DELETE FROM user_access WHERE user_id IN ($(printf '?%.0s,' {1..5} | sed 's/,$//')) AND file_id = ?" | grep -o '?' | wc -l) # Count the number of parameters PARAM_COUNT=$((5 + 1)) # For example, 5 userIds and 1 fileId echo "Placeholders: $PLACEHOLDER_COUNT" echo "Parameters: $PARAM_COUNT" if [ "$PLACEHOLDER_COUNT" -ne "$PARAM_COUNT" ]; then echo "Mismatch in placeholders and parameters." else echo "Placeholders and parameters are aligned." fiLength of output: 374
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 8
🧹 Outside diff range and nitpick comments (1)
src/app-admin.js (1)
19-19
: Consider using a more descriptive export name.The current export
export { app as handlers };
might be confusing. A more explicit name likeadminHandlers
oruserManagementHandlers
would better reflect the purpose of this module.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- src/app-admin.js (1 hunks)
- src/services/user-service.js (1 hunks)
🧰 Additional context used
🪛 Biome
src/services/user-service.js
[error] 2-244: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
🔇 Additional comments (1)
src/services/user-service.js (1)
1-246
: Well-structured implementation ofUserService
with comprehensive methodsThe
UserService
class provides a wide range of static methods for user and access management. The use of parameterized queries ensures protection against SQL injection attacks. The code is clean, and the methods are well-organized and self-explanatory.🧰 Tools
🪛 Biome
[error] 2-244: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 12
🧹 Outside diff range and nitpick comments (6)
jest.global-setup.js (2)
7-25
: LGTM! Consider returning the created user ID.The
createUser
function is well-structured with good error handling and input validation. However, it could be improved by returning the created user ID or a success indicator. This would allow callers to confirm the operation's success programmatically.Consider modifying the function to return the user ID:
const createUser = (userId, userName, role, owner = 0, enabled = 1) => { // ... existing code ... try { // ... existing code ... return userId; // Return the created user ID } catch (error) { console.error('Error creating user:', error); throw error; } };This change would make the function more versatile without breaking existing usage.
37-49
: LGTM! Consider using constants for token values.The modifications to the setup function look good. Using
createUser
for admin creation and ensuring a clean session state are excellent practices. Creating two sessions with different tokens provides good flexibility for testing.Consider defining constants for the token values to improve maintainability:
const VALID_TOKEN = 'valid-token'; const VALID_TOKEN_ADMIN = 'valid-token-admin'; // Then use these constants in the INSERT statementsThis change would centralize these values, making them easier to update if needed in the future.
src/app.js (1)
93-93
: LGTM! Authentication check added. Consider improving error handling.The authentication check using
toggleAuthentication
is correctly implemented. The use ofprocess.exit(-1)
addresses a past review comment.Consider adding an error log before exiting to provide more context:
- if (!(await toggleAuthentication())) process.exit(-1); + if (!(await toggleAuthentication())) { + console.error('Authentication toggle failed. Exiting...'); + process.exit(-1); + }src/app-account.js (1)
Line range hint
53-87
: Improved login endpoint, but fix async handlingThe login endpoint has been significantly enhanced with OpenID support and asynchronous handling. However, there's an issue in the password login case.
In the default case (password login),
loginWithPassword
is called withoutawait
. This could lead to unexpected behavior. Please update the code as follows:- tokenRes = loginWithPassword(req.body.password); + tokenRes = await loginWithPassword(req.body.password);Also, make sure to apply the same fix to the 'header' case:
- tokenRes = loginWithPassword(headerVal); + tokenRes = await loginWithPassword(headerVal);src/load-config.js (2)
104-106
: LGTM. Consider improving code formatting.The implementation of the
multiuser
property looks good. It correctly handles various truthy values for theACTUAL_MULTIUSER
environment variable, which aligns with best practices for boolean environment variables.Consider improving the code formatting for better readability:
multiuser: process.env.ACTUAL_MULTIUSER - ? ['true', '1', 'yes', 'on'].includes(process.env.ACTUAL_MULTIUSER.toLowerCase()) - : config.multiuser, + ? ['true', '1', 'yes', 'on'].includes( + process.env.ACTUAL_MULTIUSER.toLowerCase() + ) + : config.multiuser,This change addresses the linting error mentioned in the static analysis hints and improves code readability.
🧰 Tools
🪛 GitHub Check: lint
[failure] 105-105:
Replaceprocess.env.ACTUAL_MULTIUSER.toLowerCase()
with⏎········process.env.ACTUAL_MULTIUSER.toLowerCase(),⏎······
178-180
: LGTM. Consider adding validation forACTUAL_TOKEN_EXPIRATION
.The implementation of the
token_expiration
property looks good. It correctly uses the environment variableACTUAL_TOKEN_EXPIRATION
with a fallback to the config value.Consider adding validation for the
ACTUAL_TOKEN_EXPIRATION
value to ensure it's always set to a valid option. Here's a suggested implementation:function validateTokenExpiration(value) { const validOptions = ['never', '1h', '1d', '7d', '30d']; // Add all valid options return validOptions.includes(value) ? value : 'never'; } // In finalConfig token_expiration: validateTokenExpiration( process.env.ACTUAL_TOKEN_EXPIRATION || config.token_expiration ),This change ensures that
token_expiration
always has a valid value, preventing potential issues in the application due to invalid configuration.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (9)
- jest.global-setup.js (1 hunks)
- src/accounts/password.js (1 hunks)
- src/app-account.js (4 hunks)
- src/app-admin.js (1 hunks)
- src/app-admin.test.js (1 hunks)
- src/app-openid.js (1 hunks)
- src/app.js (3 hunks)
- src/load-config.js (4 hunks)
- src/services/user-service.js (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/app-admin.js
- src/app-admin.test.js
- src/app-openid.js
🧰 Additional context used
🪛 GitHub Check: lint
src/load-config.js
[failure] 105-105:
Replaceprocess.env.ACTUAL_MULTIUSER.toLowerCase()
with⏎········process.env.ACTUAL_MULTIUSER.toLowerCase(),⏎······
src/services/user-service.js
[failure] 19-19:
Replace⏎······fileId,⏎····])·||
withfileId])·||⏎···
[failure] 27-27:
Replace⏎······roleId,⏎····])·||
withroleId])·||⏎···
[failure] 101-101:
Replace⏎····'UPDATE·user_roles·SET·role_id·=·?·WHERE·user_id·=·?',
with'UPDATE·user_roles·SET·role_id·=·?·WHERE·user_id·=·?',·[
[failure] 103-103:
Replace[roleId,·userId]
withroleId,⏎····userId
[failure] 104-104:
Insert]
[failure] 107-107:
ReplaceuserId,·userName,·displayName,·enabled,·roleId
with⏎··userId,⏎··userName,⏎··displayName,⏎··enabled,⏎··roleId,⏎
[failure] 121-121:
Replace⏎····'DELETE·FROM·users·WHERE·id·=·?·and·owner·=·0',
with'DELETE·FROM·users·WHERE·id·=·?·and·owner·=·0',·[
[failure] 123-123:
Replace[userId]
withuserId
[failure] 124-124:
Insert]
🔇 Additional comments (31)
jest.global-setup.js (2)
4-5
: LGTM! Good use of constants for improved maintainability.Using constants for the admin ID and role ID is a good practice. It centralizes these values, making them easier to update if needed and improving code readability.
51-52
: LGTM! Consistent session setup.The calls to
setSessionUser
ensure that both created sessions are properly associated with the admin user. This is consistent with the two sessions created earlier and provides a good setup for testing different scenarios.src/app.js (2)
14-16
: LGTM! New imports added and typo fixed.The new imports for admin and OpenID functionality align with the PR objectives. The
toggleAuthentication
import correctly addresses the typo issue mentioned in past review comments.
54-55
: LGTM! New route handlers added.The new route handlers for admin and OpenID functionality are correctly implemented and mounted on their respective paths.
src/app-account.js (5)
6-6
: LGTM: Improved imports and middleware usageThe new imports for session validation, user info, and login methods align well with the enhanced authentication and user management features. The use of Express built-in middleware (
express.json()
andexpress.urlencoded()
) is a good practice, replacing the need for the external body-parser package.Also applies to: 11-13, 15-16, 19-20
48-51
: LGTM: New login-methods endpointThe addition of the
/login-methods
endpoint is a good improvement. It provides clients with information about available authentication options, following RESTful practices. The implementation is clean and straightforward.
101-102
: LGTM: Improved change-password endpointThe change-password endpoint now correctly uses session validation, aligning with the overall authentication strategy. The error handling has been improved with appropriate status codes, providing more informative responses to clients.
Also applies to: 107-108
Line range hint
1-130
: Overall assessment: Significant improvements with minor issuesThis pull request introduces valuable enhancements to the authentication and session management system. The changes include:
- Improved middleware usage
- New endpoints for login methods
- Enhanced login functionality with OpenID support
- More comprehensive user validation
These improvements will greatly benefit the application's security and user management capabilities. However, there are a few minor issues with asynchronous function handling that need to be addressed:
- Missing
await
in the login endpoint for password authentication- Missing
await
in the validate endpoint for user info and permissionsOnce these issues are resolved, the code will be in excellent shape. Great work on the overall improvements!
38-46
: Improved bootstrap endpoint, but verify token removalThe bootstrap endpoint has been enhanced with asynchronous handling and improved error responses. However, the removal of the token from the response is a significant change.
Can you confirm if removing the token from the bootstrap response is intentional? This change might affect clients expecting a token in the response.
✅ Verification successful
Bootstrap Endpoint Verified Without Token Dependency
The bootstrap endpoint's asynchronous handling and error responses are improved. Automated searches confirm that no client code expects a token in the bootstrap response.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if any client code is expecting a token from the bootstrap endpoint rg --type js 'bootstrap.*token' -C 3Length of output: 38
Script:
#!/bin/bash # Search for any references to 'token' within the bootstrap endpoint in JavaScript files rg --type js "app\.post\('/bootstrap'.*\btoken\b" -C 3Length of output: 62
src/accounts/password.js (1)
1-132
: 🛠️ Refactor suggestionAddress overarching concerns in password management implementation
While the individual functions have been reviewed, there are some file-wide concerns that should be addressed:
Consistent error handling:
Consider implementing a consistent error handling mechanism across all functions. This could involve creating custom error types or using a standardized error object structure.
SQL injection prevention:
Ensure that all user inputs are properly sanitized before being used in SQL queries. While the current implementation uses parameterized queries, which is good, it's crucial to verify that this practice is consistently applied throughout the file and in any future modifications.
Security logging:
Implement logging for security-sensitive operations such as password changes, failed login attempts, and user creation. This will aid in auditing and debugging. Consider using a logging library that supports different log levels.
Password policy enforcement:
The current
isValidPassword
function only checks if the password is not null or empty. Consider implementing a more robust password policy (e.g., minimum length, required character types) to enhance security.Rate limiting:
Consider implementing rate limiting for sensitive operations like login attempts to prevent brute-force attacks. This might involve tracking failed attempts and implementing temporary lockouts.
Addressing these concerns will significantly enhance the overall security and robustness of the password management system.
✅ Verification successful
SQL query parameter usage verified
All SQL queries in
src/accounts/password.js
utilize parameterized queries consistently, effectively mitigating SQL injection risks.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify SQL query parameter usage rg --type js 'accountDb\.(mutate|first)' src/accounts/password.jsLength of output: 658
src/load-config.js (2)
74-75
: LGTM. Please clarify thetoken_expiration
property.The addition of
multiuser
andtoken_expiration
properties to thedefaultConfig
object looks good. However, could you provide more information about thetoken_expiration
property? Specifically:
- What types of values are accepted for this property?
- How is the 'never' value interpreted in the application?
- Are there any security implications of setting this to 'never' by default?
189-189
: LGTM. Good addition to debug logging.The new debug log for the login method is a helpful addition. It provides valuable information during configuration debugging and uses the nullish coalescing operator appropriately to fall back to 'password' if
loginMethod
is not set.src/services/user-service.js (19)
3-9
: Function 'getUserByUsername' implemented correctlyThe function correctly retrieves the user ID by username from the database.
11-15
: Function 'getUserById' implemented correctlyThe function correctly retrieves the user ID based on the provided user ID.
17-23
: Function 'getFileById' implemented correctlyThe function correctly retrieves the file ID based on the provided file ID.
🧰 Tools
🪛 GitHub Check: lint
[failure] 19-19:
Replace⏎······fileId,⏎····])·||
withfileId])·||⏎···
25-31
: Function 'validateRole' implemented correctlyThe function accurately validates the existence of a role based on the role ID.
🧰 Tools
🪛 GitHub Check: lint
[failure] 27-27:
Replace⏎······roleId,⏎····])·||
withroleId])·||⏎···
33-39
: Function 'getOwnerCount' implemented with default valueThe function retrieves the count of owners, defaulting to
0
if none are found, which ensures consistency.
41-47
: Function 'getOwnerId' implemented correctlyThe function correctly retrieves the ID of the owner user.
49-55
: Function 'getFileOwnerId' implemented correctlyThe function retrieves the owner ID of a specified file as expected.
57-67
: Function 'getFileOwnerById' implemented correctlyThe function retrieves detailed information about the file's owner, including user ID, username, and display name.
69-77
: Function 'getAllUsers' implemented correctlyThe function retrieves a list of all users with their details, including roles and ownership status.
79-84
: Function 'insertUser' implemented correctlyThe function correctly inserts a new user into the database with the specified details.
86-91
: Function 'insertUserRole' implemented correctlyThe function associates a role with a user as expected.
93-98
: Function 'updateUser' implemented correctlyThe function updates user information in the database accurately.
100-105
: Function 'updateUserRole' implemented correctlyThe function updates the role associated with a user as intended.
🧰 Tools
🪛 GitHub Check: lint
[failure] 101-101:
Replace⏎····'UPDATE·user_roles·SET·role_id·=·?·WHERE·user_id·=·?',
with'UPDATE·user_roles·SET·role_id·=·?·WHERE·user_id·=·?',·[
[failure] 103-103:
Replace[roleId,·userId]
withroleId,⏎····userId
[failure] 104-104:
Insert]
107-118
: Function 'updateUserWithRole' implemented correctly with transactionThe function atomically updates user and role information within a transaction, ensuring data integrity.
🧰 Tools
🪛 GitHub Check: lint
[failure] 107-107:
ReplaceuserId,·userName,·displayName,·enabled,·roleId
with⏎··userId,⏎··userName,⏎··displayName,⏎··enabled,⏎··roleId,⏎
120-125
: Function 'deleteUser' implemented correctlyThe function deletes a user if they are not an owner, as per the business logic.
🧰 Tools
🪛 GitHub Check: lint
[failure] 121-121:
Replace⏎····'DELETE·FROM·users·WHERE·id·=·?·and·owner·=·0',
with'DELETE·FROM·users·WHERE·id·=·?·and·owner·=·0',·[
[failure] 123-123:
Replace[userId]
withuserId
[failure] 124-124:
Insert]
127-129
: Function 'deleteUserRoles' implemented correctlyThe function removes all roles associated with a user.
131-135
: Function 'deleteUserAccess' implemented correctlyThe function deletes all access records associated with a user.
137-142
: Function 'transferAllFilesFromUser' implemented correctlyThe function transfers all files from one user to another, which is useful for user deactivation scenarios.
144-149
: Function 'updateFileOwner' implemented correctlyThe function updates the owner of a specific file as expected.
function hashPassword(password) { | ||
return bcrypt.hashSync(password, 12); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Consider using asynchronous bcrypt functions
The hashPassword
function currently uses bcrypt.hashSync
, which is a synchronous operation. For better performance and to avoid blocking the event loop, especially in high-load scenarios, consider using the asynchronous bcrypt.hash
function instead.
Here's a suggested refactor:
-function hashPassword(password) {
- return bcrypt.hashSync(password, 12);
+async function hashPassword(password) {
+ return await bcrypt.hash(password, 12);
}
Note that this change will require updating the calling functions to handle the returned promise.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
function hashPassword(password) { | |
return bcrypt.hashSync(password, 12); | |
} | |
async function hashPassword(password) { | |
return await bcrypt.hash(password, 12); | |
} |
export function bootstrapPassword(password) { | ||
if (!isValidPassword(password)) { | ||
return { error: 'invalid-password' }; | ||
} | ||
|
||
let hashed = hashPassword(password); | ||
let accountDb = getAccountDb(); | ||
accountDb.transaction(() => { | ||
accountDb.mutate('DELETE FROM auth WHERE method = ?', ['password']); | ||
accountDb.mutate('UPDATE auth SET active = 0'); | ||
accountDb.mutate( | ||
"INSERT INTO auth (method, display_name, extra_data, active) VALUES ('password', 'Password', ?, 1)", | ||
[hashed], | ||
); | ||
}); | ||
|
||
return {}; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Enhance return value for successful operation
The bootstrapPassword
function returns an empty object {}
on success, which might not be as clear as explicitly indicating a successful operation.
Consider modifying the return value to clearly indicate success:
- return {};
+ return { success: true };
This change makes the successful outcome more explicit and consistent with the error case.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
export function bootstrapPassword(password) { | |
if (!isValidPassword(password)) { | |
return { error: 'invalid-password' }; | |
} | |
let hashed = hashPassword(password); | |
let accountDb = getAccountDb(); | |
accountDb.transaction(() => { | |
accountDb.mutate('DELETE FROM auth WHERE method = ?', ['password']); | |
accountDb.mutate('UPDATE auth SET active = 0'); | |
accountDb.mutate( | |
"INSERT INTO auth (method, display_name, extra_data, active) VALUES ('password', 'Password', ?, 1)", | |
[hashed], | |
); | |
}); | |
return {}; | |
} | |
export function bootstrapPassword(password) { | |
if (!isValidPassword(password)) { | |
return { error: 'invalid-password' }; | |
} | |
let hashed = hashPassword(password); | |
let accountDb = getAccountDb(); | |
accountDb.transaction(() => { | |
accountDb.mutate('DELETE FROM auth WHERE method = ?', ['password']); | |
accountDb.mutate('UPDATE auth SET active = 0'); | |
accountDb.mutate( | |
"INSERT INTO auth (method, display_name, extra_data, active) VALUES ('password', 'Password', ?, 1)", | |
[hashed], | |
); | |
}); | |
return { success: true }; | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 18
🧹 Outside diff range and nitpick comments (2)
src/accounts/openid.js (1)
1-256
: Overall assessment: Functional but needs improvementsThe OpenID implementation is functional and covers the necessary steps for authentication. However, there are several areas that could be improved:
- Error handling should be more consistent and informative throughout the file.
- Some functions, particularly
loginWithOpenIdFinalize
, are complex and could be refactored into smaller, more manageable functions.- Security measures, such as token validation and protection against potential abuse, should be enhanced.
- Code documentation could be improved, especially for complex processes like PKCE.
- Consider making some hardcoded values (like expiration times) configurable.
These improvements would enhance the maintainability, security, and robustness of the OpenID implementation.
Consider implementing the following architectural improvements:
- Add a configuration management system to handle OpenID settings without manual database edits.
- Implement a more robust error handling and logging system.
- Add rate limiting and additional security measures to prevent potential abuse of the authentication process.
- Create a separate module for user management operations to improve separation of concerns.
src/app-sync.js (1)
Line range hint
1-438
: Summary: Improved multi-user support with suggestions for further enhancementsThe changes in this file significantly improve multi-user support and access control:
- Introduction of
validateSessionMiddleware
for base authentication.- Enhanced file ownership tracking in the upload endpoint.
- Improved access control in the list-user-files endpoint, differentiating between admin and regular users.
- More detailed file information including owner and access details in responses.
These changes align well with the PR objectives. However, there are opportunities for further improvements:
- Implement granular permission checks for specific endpoints (e.g.,
/sync
,/upload-user-file
) to ensure users can only access resources they have permissions for.- Review and update error handling to provide more specific error messages related to permissions and access control.
- Consider implementing audit logging for sensitive operations to track user actions.
For future iterations, consider implementing a comprehensive role-based access control (RBAC) system to manage permissions more flexibly and securely.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
- src/accounts/openid.js (1 hunks)
- src/accounts/password.js (1 hunks)
- src/app-admin.js (1 hunks)
- src/app-sync.js (5 hunks)
- src/load-config.js (4 hunks)
- src/services/user-service.js (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/app-admin.js
🧰 Additional context used
🔇 Additional comments (13)
src/accounts/password.js (6)
120-132
: 🛠️ Refactor suggestionImprove
changePassword
function for better performance and clarityThe
changePassword
function can be improved in terms of performance and clarity of its return value.Consider the following changes:
- Use asynchronous password hashing for better performance.
- Provide a clear success indicator in the return value.
Here's a suggested refactor:
-export function changePassword(newPassword) { +export async function changePassword(newPassword) { let accountDb = getAccountDb(); if (!isValidPassword(newPassword)) { return { error: 'invalid-password' }; } - let hashed = hashPassword(newPassword); + let hashed = await hashPassword(newPassword); accountDb.mutate("UPDATE auth SET extra_data = ? WHERE method = 'password'", [ hashed, ]); - return {}; + return { success: true }; }These changes improve performance by using asynchronous hashing and provide a clear success indicator in the return value.
To check for other functions that might benefit from similar improvements, run:
#!/bin/bash rg --type javascript 'function.*changePassword|function.*updatePassword'
1-132
: Overall assessment: Solid password management implementation with room for improvementThe
password.js
file provides a comprehensive set of functions for password management, including validation, hashing, login, and password changes. The overall structure and logic are sound, but there are several areas where improvements can be made:
- Asynchronous operations: Convert synchronous bcrypt operations to asynchronous for better performance and to avoid blocking the event loop.
- Function refactoring: Break down larger functions (especially
loginWithPassword
) into smaller, more focused functions for improved maintainability and testability.- Session management: Enhance session queries to be user-specific, preventing potential conflicts in multi-user environments.
- Error handling and return values: Improve consistency in error handling and provide clear success indicators in return values.
- Code readability: Refactor complex logic (e.g., token expiration) for improved readability and maintainability.
Implementing these suggestions will significantly enhance the overall quality, performance, and security of the password management system.
To get an overview of the file's structure and identify any missed opportunities for improvement, run:
#!/bin/bash # Display an overview of the file structure rg --type javascript -n '^(import|export|function)' src/accounts/password.js # Check for any TODO comments that might need addressing rg --type javascript 'TODO|FIXME' src/accounts/password.js
103-113
: 🛠️ Refactor suggestion
⚠️ Potential issueImprove session update logic and add affected rows check
The current session management logic updates the
user_id
for existing sessions, which might not be the intended behavior. Additionally, there's no check to ensure the update actually affected any rows.Consider modifying the update logic as follows:
if (!sessionRow) { accountDb.mutate( 'INSERT INTO sessions (token, expires_at, user_id, auth_method) VALUES (?, ?, ?, ?)', [token, expiration, userId, 'password'], ); } else { const result = accountDb.mutate( 'UPDATE sessions SET expires_at = ? WHERE token = ? AND user_id = ?', [expiration, token, userId], ); if (result.changes === 0) { // Handle case where update didn't affect any rows return { error: 'session-mismatch' }; } }This change ensures that we're only updating the expiration for the correct user's session and provides error handling for mismatched sessions.
To verify the impact of this change, run the following script:
#!/bin/bash # Check for other places where session updates occur rg --type javascript 'UPDATE sessions'
15-32
: 🛠️ Refactor suggestionEnhance return value for successful operation
The
bootstrapPassword
function returns an empty object{}
on success, which might not be as clear as explicitly indicating a successful operation.Consider modifying the return value to clearly indicate success:
- return {}; + return { success: true };This change makes the successful outcome more explicit and consistent with the error case.
Likely invalid or redundant comment.
11-13
: 🛠️ Refactor suggestionConsider using asynchronous bcrypt hashing
The
hashPassword
function currently usesbcrypt.hashSync
, which is a synchronous operation. For better performance and to avoid blocking the event loop, especially in high-load scenarios, consider using the asynchronousbcrypt.hash
function instead.Here's a suggested refactor:
-function hashPassword(password) { - return bcrypt.hashSync(password, 12); +async function hashPassword(password) { + return await bcrypt.hash(password, 12); }Note that this change will require updating the calling functions to handle the returned promise.
Likely invalid or redundant comment.
34-118
: 🛠️ Refactor suggestionRefactor
loginWithPassword
for improved maintainability and securityThe
loginWithPassword
function is handling multiple responsibilities and has some potential issues:
- The function is long and complex, making it hard to maintain and test.
- Session management doesn't filter by user, which could lead to conflicts in a multi-user environment.
- Synchronous bcrypt comparison is used, which could block the event loop.
Consider the following improvements:
- Break the function into smaller, focused functions (e.g.,
validatePassword
,createUserIfNotExists
,manageSession
,generateToken
).- Update session queries to include user ID:
let sessionRow = accountDb.first( 'SELECT * FROM sessions WHERE user_id = ? AND auth_method = ?', [userId, 'password'], );
- Use asynchronous bcrypt comparison:
-let confirmed = bcrypt.compareSync(password, passwordHash); +let confirmed = await bcrypt.compare(password, passwordHash);Remember to make the
loginWithPassword
function async and handle the promise appropriately.To verify the impact of these changes, run the following script:
✅ Verification successful
Verified:
loginWithPassword
uses synchronous bcrypt methodsThe
loginWithPassword
function employsbcrypt.compareSync
, which blocks the event loop and may impact performance. It's recommended to replace it with the asynchronousbcrypt.compare
method and refactor the function for better maintainability.
- Location:
src/accounts/password.js
, lines 34-118🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for other uses of synchronous bcrypt methods rg --type javascript 'bcrypt\.compareSync|bcrypt\.hashSync' # Check for other functions that might benefit from similar refactoring rg --type javascript -A 20 'function.*\(' | rg -C 20 'accountDb\.first|accountDb\.mutate'Length of output: 241
Script:
#!/bin/bash # Check for other uses of synchronous bcrypt methods rg --type js 'bcrypt\.compareSync|bcrypt\.hashSync' # Check for other functions that might benefit from similar refactoring rg --type js -A 20 'function\s+\w+\s*\(' | rg -C 20 'accountDb\.first|accountDb\.mutate'Length of output: 10230
src/accounts/openid.js (2)
1-9
: Import statements look goodThe necessary modules and functions are imported correctly for OpenID implementation.
51-70
: setupOpenIdClient function looks goodThe function correctly handles different issuer configurations and sets up the OpenID client with appropriate parameters.
src/app-sync.js (3)
263-271
: Approve: Enhanced file ownership trackingThe addition of the 'owner' field in the INSERT query, set to
req.userSession.user_id
, improves file ownership tracking. This change enhances the data model and strengthens the user-file relationship.To ensure proper user session handling, please run the following verification:
#!/bin/bash # Description: Verify user session handling in the upload-user-file endpoint echo "Checking user session handling in upload-user-file endpoint:" rg -A 10 "app.post\('/upload-user-file'" src/app-sync.js echo "Checking userSession object structure:" rg "req.userSession" src/app-sync.js
8-8
: Verify the implementation ofvalidateSessionMiddleware
The change from
validateUserMiddleware
tovalidateSessionMiddleware
suggests an improvement in session validation. However, ensure that this middleware is correctly implemented insrc/util/middlewares.js
.Run the following script to verify the implementation:
Also applies to: 24-24
✅ Verification successful
validateSessionMiddleware
is correctly implemented and exported insrc/util/middlewares.js
.
- The middleware is properly applied in
src/app-sync.js
at lines 8 and 24.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the implementation of validateSessionMiddleware echo "Checking validateSessionMiddleware implementation:" rg -A 10 "export function validateSessionMiddleware" src/util/middlewares.jsLength of output: 189
Script:
#!/bin/bash # Description: Search for all instances of validateSessionMiddleware in src/util/middlewares.js echo "Searching for validateSessionMiddleware implementations:" rg "validateSessionMiddleware" src/util/middlewares.js -A 10 -B 10Length of output: 1067
Line range hint
338-385
: Approve: Improved access control and detailed file listingThe changes in the
/list-user-files
endpoint significantly enhance access control and provide more detailed file information:
- Admin users can now see all files, while regular users only see their own files and shared files.
- The response now includes owner information and a list of users with access to each file.
These improvements align well with the multi-user support objectives.
To ensure the new logic works as expected, please run the following verification:
src/load-config.js (1)
74-75
: Addition of 'multiuser' and 'token_expiration' default configurationsThe addition of
multiuser
set tofalse
andtoken_expiration
set to'never'
indefaultConfig
is appropriate and clearly defines default behaviors.src/services/user-service.js (1)
221-232
: Consider edge cases for users without access or ownershipIn the
getAllUserAccess
function, users who neither own the file nor have access will havehaveAccess
andowner
set to0
. Ensure that the front-end or calling code properly handles these cases to prevent unauthorized access.Run the following script to check if there are users without access or ownership:
Replace
<FILE_ID>
with an actual file ID to perform the check.
let expiration = TOKEN_EXPIRATION_NEVER; | ||
if ( | ||
finalConfig.token_expiration != 'never' && | ||
finalConfig.token_expiration != 'openid-provider' && | ||
typeof finalConfig.token_expiration === 'number' | ||
) { | ||
expiration = | ||
Math.floor(Date.now() / 1000) + finalConfig.token_expiration * 60; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Refactor token expiration logic for improved readability
The current implementation of token expiration logic is correct but could be made more readable.
Consider refactoring this section as follows:
let expiration = TOKEN_EXPIRATION_NEVER;
const isValidNumberExpiration =
typeof finalConfig.token_expiration === 'number' &&
finalConfig.token_expiration !== 'never' &&
finalConfig.token_expiration !== 'openid-provider';
if (isValidNumberExpiration) {
const currentTimestamp = Math.floor(Date.now() / 1000);
expiration = currentTimestamp + finalConfig.token_expiration * 60;
}
This refactoring:
- Combines the conditions for a valid number expiration.
- Extracts the timestamp calculation for clarity.
- Makes the logic more straightforward and easier to understand at a glance.
export async function loginWithOpenIdSetup(body) { | ||
if (!body.return_url) { | ||
return { error: 'return-url-missing' }; | ||
} | ||
|
||
let accountDb = getAccountDb(); | ||
let config = accountDb.first('SELECT extra_data FROM auth WHERE method = ?', [ | ||
'openid', | ||
]); | ||
if (!config) { | ||
return { error: 'openid-not-configured' }; | ||
} | ||
|
||
try { | ||
config = JSON.parse(config['extra_data']); | ||
} catch (err) { | ||
return { error: 'openid-setup-failed: ' + err }; | ||
} | ||
|
||
let client; | ||
try { | ||
client = await setupOpenIdClient(config); | ||
} catch (err) { | ||
return { error: 'openid-setup-failed: ' + err }; | ||
} | ||
|
||
const state = generators.state(); | ||
const code_verifier = generators.codeVerifier(); | ||
const code_challenge = generators.codeChallenge(code_verifier); | ||
|
||
const now_time = Date.now(); | ||
const expiry_time = now_time + 300 * 1000; | ||
|
||
accountDb.mutate( | ||
'DELETE FROM pending_openid_requests WHERE expiry_time < ?', | ||
[now_time], | ||
); | ||
accountDb.mutate( | ||
'INSERT INTO pending_openid_requests (state, code_verifier, return_url, expiry_time) VALUES (?, ?, ?, ?)', | ||
[state, code_verifier, body.return_url, expiry_time], | ||
); | ||
|
||
const url = client.authorizationUrl({ | ||
response_type: 'code', | ||
scope: 'openid email profile', | ||
state, | ||
code_challenge, | ||
code_challenge_method: 'S256', | ||
}); | ||
|
||
return { url }; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Improve error handling and add documentation
The function is well-implemented, but consider the following improvements:
- Make error handling more consistent across the function.
- Add comments explaining the PKCE (Proof Key for Code Exchange) process.
- Consider making the expiry time for pending requests configurable.
Here's a suggested improvement for error handling:
try {
config = JSON.parse(config['extra_data']);
} catch (err) {
- return { error: 'openid-setup-failed: ' + err };
+ console.error('Failed to parse OpenID configuration:', err);
+ return { error: 'openid-setup-failed', details: 'Invalid configuration' };
}
Add a comment explaining the PKCE process:
// Generate PKCE parameters
// PKCE (Proof Key for Code Exchange) is used to prevent authorization code interception attacks
const state = generators.state();
const code_verifier = generators.codeVerifier();
const code_challenge = generators.codeChallenge(code_verifier);
Consider making the expiry time for pending requests configurable:
- const expiry_time = now_time + 300 * 1000;
+ const PENDING_REQUEST_EXPIRY = 300 * 1000; // 5 minutes, make this configurable
+ const expiry_time = now_time + PENDING_REQUEST_EXPIRY;
if (!pendingRequest) { | ||
return { error: 'invalid-or-expired-state' }; | ||
} | ||
|
||
let { code_verifier, return_url } = pendingRequest; | ||
|
||
try { | ||
let grant = await client.grant({ | ||
grant_type: 'authorization_code', | ||
code: body.code, | ||
code_verifier, | ||
redirect_uri: client.redirect_uris[0], | ||
}); | ||
const userInfo = await client.userinfo(grant); | ||
const identity = | ||
userInfo.preferred_username ?? | ||
userInfo.login ?? | ||
userInfo.email ?? | ||
userInfo.id ?? | ||
userInfo.name; | ||
if (identity == null) { | ||
return { error: 'openid-grant-failed: no identification was found' }; | ||
} | ||
|
||
let { countUsersWithUserName } = accountDb.first( | ||
'SELECT count(*) as countUsersWithUserName FROM users WHERE user_name <> ?', | ||
[''], | ||
); | ||
let userId = null; | ||
if (countUsersWithUserName === 0) { | ||
userId = uuid.v4(); | ||
accountDb.mutate( | ||
'INSERT INTO users (id, user_name, display_name, enabled, owner) VALUES (?, ?, ?, 1, 1)', | ||
[userId, identity, userInfo.name ?? userInfo.email ?? identity], | ||
); | ||
|
||
const { id: adminRoleId } = | ||
accountDb.first('SELECT id FROM roles WHERE name = ?', ['Admin']) || {}; | ||
|
||
if (!adminRoleId) { | ||
return { error: 'administrator-role-not-found' }; | ||
} | ||
|
||
accountDb.mutate( | ||
'INSERT INTO user_roles (user_id, role_id) VALUES (?, ?)', | ||
[userId, adminRoleId], | ||
); | ||
|
||
const userFromPasswordMethod = getUserByUsername(''); | ||
if (userFromPasswordMethod) { | ||
transferAllFilesFromUser(userId, userFromPasswordMethod.user_id); | ||
} | ||
} else { | ||
let { id: userIdFromDb, display_name: displayName } = | ||
accountDb.first( | ||
'SELECT id, display_name FROM users WHERE user_name = ? and enabled = 1', | ||
[identity], | ||
) || {}; | ||
|
||
if (userIdFromDb == null) { | ||
return { error: 'openid-grant-failed' }; | ||
} | ||
|
||
if (!displayName && userInfo.name) { | ||
accountDb.mutate('UPDATE users set display_name = ? WHERE id = ?', [ | ||
userInfo.name, | ||
userIdFromDb, | ||
]); | ||
} | ||
|
||
userId = userIdFromDb; | ||
} | ||
|
||
const token = uuid.v4(); | ||
|
||
let expiration; | ||
if (finalConfig.token_expiration === 'openid-provider') { | ||
expiration = grant.expires_at ?? TOKEN_EXPIRATION_NEVER; | ||
} else if (finalConfig.token_expiration === 'never') { | ||
expiration = TOKEN_EXPIRATION_NEVER; | ||
} else if (typeof finalConfig.token_expiration === 'number') { | ||
expiration = | ||
Math.floor(Date.now() / 1000) + finalConfig.token_expiration * 60; | ||
} else { | ||
expiration = TOKEN_EXPIRATION_NEVER; | ||
} | ||
|
||
accountDb.mutate( | ||
'INSERT INTO sessions (token, expires_at, user_id, auth_method) VALUES (?, ?, ?, ?)', | ||
[token, expiration, userId, 'openid'], | ||
); | ||
|
||
clearExpiredSessions(); | ||
|
||
return { url: `${return_url}/openid-cb?token=${token}` }; | ||
} catch (err) { | ||
console.error('OpenID grant failed:', err); | ||
return { error: 'openid-grant-failed: ' + err }; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Refactor for improved readability, security, and error handling
This function is complex and handles multiple scenarios. Consider the following improvements:
- Refactor the function into smaller, more manageable functions.
- Improve error handling and logging.
- Enhance security by validating the OpenID token and not exposing error details.
- Improve the logic for determining the user's identity.
Break down the function into smaller functions, for example:
async function loginWithOpenIdFinalize(body) {
const { code, state } = validateInput(body);
const config = getOpenIdConfig();
const client = await setupOpenIdClient(config);
const pendingRequest = getPendingRequest(state);
const grant = await exchangeCodeForGrant(client, code, pendingRequest);
const userInfo = await getUserInfo(client, grant);
const userId = await getOrCreateUser(userInfo);
const token = createSessionToken(userId, grant);
return generateRedirectUrl(pendingRequest.return_url, token);
}
Improve error handling and logging:
- console.error('OpenID grant failed:', err);
- return { error: 'openid-grant-failed: ' + err };
+ console.error('OpenID grant failed:', err);
+ return { error: 'openid-grant-failed', details: 'An error occurred during authentication' };
Enhance security by validating the OpenID token:
const tokenSet = await client.callback(client.redirect_uris[0], { code, state }, { code_verifier });
// Validate the token
if (!tokenSet.valid()) {
throw new Error('Invalid token set');
}
const userInfo = await client.userinfo(tokenSet.access_token);
Improve the logic for determining the user's identity:
const identity = userInfo.sub || userInfo.preferred_username || userInfo.email;
if (!identity) {
throw new Error('Unable to determine user identity from OpenID response');
}
Consider implementing rate limiting and additional security measures to prevent potential abuse of the OpenID authentication process.
export async function bootstrapOpenId(config) { | ||
if (!('issuer' in config)) { | ||
return { error: 'missing-issuer' }; | ||
} | ||
if (!('client_id' in config)) { | ||
return { error: 'missing-client-id' }; | ||
} | ||
if (!('client_secret' in config)) { | ||
return { error: 'missing-client-secret' }; | ||
} | ||
if (!('server_hostname' in config)) { | ||
return { error: 'missing-server-hostname' }; | ||
} | ||
|
||
try { | ||
await setupOpenIdClient(config); | ||
} catch (err) { | ||
return { error: 'configuration-error' }; | ||
} | ||
|
||
getAccountDb().mutate('DELETE FROM auth WHERE method = ?', ['openid']); | ||
|
||
// Beyond verifying that the configuration exists, we do not attempt | ||
// to check if the configuration is actually correct. | ||
// If the user improperly configures this during bootstrap, there is | ||
// no way to recover without manually editing the database. However, | ||
// this might not be a real issue since an analogous situation happens | ||
// if they forget their password. | ||
let accountDb = getAccountDb(); | ||
accountDb.transaction(() => { | ||
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)], | ||
); | ||
}); | ||
|
||
return {}; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Enhance error handling and improve configuration validation
While the overall structure is good, consider the following improvements:
- Make error messages more specific by including the missing property name.
- Provide more detailed error information in the catch block.
- Consider implementing a mechanism to update OpenID configuration without manual database editing.
Here's a suggested improvement for error messages:
- if (!('issuer' in config)) {
- return { error: 'missing-issuer' };
- }
+ if (!('issuer' in config)) {
+ return { error: 'missing-property', property: 'issuer' };
+ }
Apply similar changes for other property checks.
For the catch block:
try {
await setupOpenIdClient(config);
} catch (err) {
- return { error: 'configuration-error' };
+ return { error: 'configuration-error', details: err.message };
}
Consider implementing an admin interface or API endpoint to update OpenID configuration without requiring manual database edits. This would improve maintainability and reduce the risk of database corruption.
Committable suggestion was skipped due to low confidence.
import { getPathForUserFile, getPathForGroupFile } from './util/paths.js'; | ||
|
||
import * as simpleSync from './sync-simple.js'; | ||
|
||
import { SyncProtoBuf } from '@actual-app/crdt'; | ||
import getAccountDb, { isAdmin } from './account-db.js'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Consider implementing granular permission checks
While the addition of validateSessionMiddleware
provides a base level of authentication for all routes, the previous suggestion to implement granular permission checks for specific endpoints (e.g., /sync
) is still valid. This would further enhance security by ensuring users can only access resources they have permissions for.
Consider implementing a middleware for granular permission checks. For example:
function checkPermission(requiredPermission) {
return (req, res, next) => {
if (hasPermission(req.userSession.user_id, requiredPermission)) {
next();
} else {
res.status(403).send('Forbidden');
}
};
}
// Usage
app.post('/sync', checkPermission('sync'), async (req, res) => {
// ...
});
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 = ?)`, | ||
[fileId, userId, isAdmin ? 1 : 0], | ||
); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Refactor SQL logic for clarity in getUserAccess
The current SQL query uses (files.owner = ? OR 1 = ?)
to handle admin access, which can be confusing. Consider restructuring the query to conditionally include the owner check based on the isAdmin
flag.
Apply this diff:
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 = ?)`,
- [fileId, userId, isAdmin ? 1 : 0],
+ WHERE files.id = ?${isAdmin ? '' : ' AND files.owner = ?'}`,
+ isAdmin ? [fileId] : [fileId, userId],
);
}
This refactoring improves readability and removes the unconventional use of 1 = ?
in the WHERE clause.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
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 = ?)`, | |
[fileId, userId, isAdmin ? 1 : 0], | |
); | |
} | |
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 = ?${isAdmin ? '' : ' AND files.owner = ?'}`, | |
isAdmin ? [fileId] : [fileId, userId], | |
); | |
} |
export function getUserById(userId) { | ||
const { id } = | ||
getAccountDb().first('SELECT id FROM users WHERE id = ?', [userId]) || {}; | ||
return id; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Ensure consistent return values when no user is found
Similarly, in the getUserById
function, returning undefined
when no user is found can cause issues. Returning null
provides a clear indication of the absence of a user.
Apply this diff:
export function getUserById(userId) {
const { id } =
getAccountDb().first('SELECT id FROM users WHERE id = ?', [userId]) || {};
- return id;
+ return id || null;
}
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
export function getUserById(userId) { | |
const { id } = | |
getAccountDb().first('SELECT id FROM users WHERE id = ?', [userId]) || {}; | |
return id; | |
} | |
export function getUserById(userId) { | |
const { id } = | |
getAccountDb().first('SELECT id FROM users WHERE id = ?', [userId]) || {}; | |
return id || null; | |
} |
export function getFileOwnerId(fileId) { | ||
const { owner } = | ||
getAccountDb().first(`SELECT files.owner FROM files WHERE files.id = ?`, [ | ||
fileId, | ||
]) || {}; | ||
return owner; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Ensure consistent return values when no owner is found
In the getFileOwnerId
function, returning null
when no owner is found enhances clarity.
Apply this diff:
export function getFileOwnerId(fileId) {
const { owner } =
getAccountDb().first(`SELECT files.owner FROM files WHERE files.id = ?`, [
fileId,
]) || {};
- return owner;
+ return owner || null;
}
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
export function getFileOwnerId(fileId) { | |
const { owner } = | |
getAccountDb().first(`SELECT files.owner FROM files WHERE files.id = ?`, [ | |
fileId, | |
]) || {}; | |
return owner; | |
} | |
export function getFileOwnerId(fileId) { | |
const { owner } = | |
getAccountDb().first(`SELECT files.owner FROM files WHERE files.id = ?`, [ | |
fileId, | |
]) || {}; | |
return owner || null; | |
} |
export function getOwnerId() { | ||
const { id } = | ||
getAccountDb().first( | ||
`SELECT users.id FROM users WHERE users.user_name <> '' and users.owner = 1`, | ||
) || {}; | ||
return id; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Ensure consistent return values when no owner is found
In the getOwnerId
function, if no owner is found, id
will be undefined
. Consider returning null
to indicate that there is no owner.
Apply this diff:
export function getOwnerId() {
const { id } =
getAccountDb().first(
`SELECT users.id FROM users WHERE users.user_name <> '' and users.owner = 1`,
) || {};
- return id;
+ return id || null;
}
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
export function getOwnerId() { | |
const { id } = | |
getAccountDb().first( | |
`SELECT users.id FROM users WHERE users.user_name <> '' and users.owner = 1`, | |
) || {}; | |
return id; | |
} | |
export function getOwnerId() { | |
const { id } = | |
getAccountDb().first( | |
`SELECT users.id FROM users WHERE users.user_name <> '' and users.owner = 1`, | |
) || {}; | |
return id || null; | |
} |
export function deleteUserAccessByFileId(userIds, fileId) { | ||
if (!Array.isArray(userIds) || userIds.length === 0) { | ||
throw new Error('The provided userIds must be a non-empty array.'); | ||
} | ||
|
||
const CHUNK_SIZE = 999; | ||
let totalChanges = 0; | ||
|
||
for (let i = 0; i < userIds.length; i += CHUNK_SIZE) { | ||
const chunk = userIds.slice(i, i + CHUNK_SIZE); | ||
const placeholders = chunk.map(() => '?').join(','); | ||
|
||
const sql = `DELETE FROM user_access WHERE user_id IN (${placeholders}) AND file_id = ?`; | ||
|
||
const result = getAccountDb().mutate(sql, [...chunk, fileId]); | ||
totalChanges += result.changes; | ||
} | ||
|
||
return totalChanges; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Validate fileId
in deleteUserAccessByFileId
Currently, there's a check for userIds
but no validation for fileId
. Adding a check to ensure that fileId
is valid and not undefined
enhances robustness.
Apply this diff:
export function deleteUserAccessByFileId(userIds, fileId) {
if (!Array.isArray(userIds) || userIds.length === 0) {
throw new Error('The provided userIds must be a non-empty array.');
}
+ if (!fileId) {
+ throw new Error('A valid fileId must be provided.');
+ }
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
export function deleteUserAccessByFileId(userIds, fileId) { | |
if (!Array.isArray(userIds) || userIds.length === 0) { | |
throw new Error('The provided userIds must be a non-empty array.'); | |
} | |
const CHUNK_SIZE = 999; | |
let totalChanges = 0; | |
for (let i = 0; i < userIds.length; i += CHUNK_SIZE) { | |
const chunk = userIds.slice(i, i + CHUNK_SIZE); | |
const placeholders = chunk.map(() => '?').join(','); | |
const sql = `DELETE FROM user_access WHERE user_id IN (${placeholders}) AND file_id = ?`; | |
const result = getAccountDb().mutate(sql, [...chunk, fileId]); | |
totalChanges += result.changes; | |
} | |
return totalChanges; | |
} | |
export function deleteUserAccessByFileId(userIds, fileId) { | |
if (!Array.isArray(userIds) || userIds.length === 0) { | |
throw new Error('The provided userIds must be a non-empty array.'); | |
} | |
if (!fileId) { | |
throw new Error('A valid fileId must be provided.'); | |
} | |
const CHUNK_SIZE = 999; | |
let totalChanges = 0; | |
for (let i = 0; i < userIds.length; i += CHUNK_SIZE) { | |
const chunk = userIds.slice(i, i + CHUNK_SIZE); | |
const placeholders = chunk.map(() => '?').join(','); | |
const sql = `DELETE FROM user_access WHERE user_id IN (${placeholders}) AND file_id = ?`; | |
const result = getAccountDb().mutate(sql, [...chunk, fileId]); | |
totalChanges += result.changes; | |
} | |
return totalChanges; | |
} |
To work in conjunction with #2945
Following the old PR #219