🐛 Fix existing sessions when using the latest version with Openid #507
Conversation
Note this is only disallowed at bootstrap. If the database is edited manually, we make no attempt to detect this.
WalkthroughThe pull request introduces modifications to the migration script located in A new The session token handling logic is revised to check for a session row with a null The In Modifications in Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
migrations/1719409568000-multiuser.js (1)
46-50: Provide default user information for the admin accountThe new admin user is being created with empty
user_nameanddisplay_namefields. This might lead to confusion or issues in parts of the application that expect these fields to have meaningful values.Suggestion:
Set default values for
user_nameanddisplay_name, such as 'admin' and 'Administrator':accountDb.mutate( 'INSERT INTO users (id, user_name, display_name, enabled, owner, role) VALUES (?, ?, ?, 1, 1, ?)', - [userId, '', '', 'ADMIN'], + [userId, 'admin', 'Administrator', 'ADMIN'], );This provides clarity and ensures consistency within the user data.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
upcoming-release-notes/507.mdis excluded by!**/*.md
📒 Files selected for processing (1)
migrations/1719409568000-multiuser.js(2 hunks)
🔇 Additional comments (1)
migrations/1719409568000-multiuser.js (1)
53-55: Validate the usage of -1 for expires_at
Setting expires_at to -1 may cause unexpected behavior if the application does not handle negative expiration times correctly. Ensure that the application interprets -1 as a non-expiring session.
Run the following script to check if -1 is appropriately handled in the codebase:
This will help ensure that using -1 for expires_at won't introduce any issues.
✅ Verification successful
Using -1 for expires_at is valid and properly handled
The codebase consistently uses -1 to represent non-expiring sessions:
- It's defined as
TOKEN_EXPIRATION_NEVER = -1insrc/util/validate-user.js - The expiration check explicitly handles this case:
session.expires_at !== TOKEN_EXPIRATION_NEVER - Session cleanup query excludes -1:
DELETE FROM sessions WHERE expires_at <> -1 - Test setup also uses -1 (as
NEVER_EXPIRES) for permanent sessions - Both password and OpenID authentication flows properly utilize this convention
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for usage of 'expires_at' and check how negative values are handled.
# Find all occurrences of 'expires_at' in the codebase and display context.
rg -A 5 -B 2 'expires_at'
# Check if there's logic that handles 'expires_at' <= 0 as non-expiring.
rg -A 3 -B 2 'expires_at\s*[<]=\s*0'
Length of output: 4902
Script:
#!/bin/bash
# Let's search for TOKEN_EXPIRATION_NEVER constant to understand how -1 is handled
rg -A 2 -B 2 "TOKEN_EXPIRATION_NEVER"
# Also search for any constants related to expiration
rg -A 2 -B 2 "NEVER_EXPIRES"
# Check if there are any other places where -1 is used with sessions
rg -A 2 -B 2 "sessions.*-1|expires.*-1"
Length of output: 3232
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
… other variable to make it more clear
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
src/app-account.js (1)
36-37: Consider adding API versioningGiven this is a breaking change in the API response structure, consider implementing API versioning to maintain backward compatibility.
Example implementation:
app.get('/needs-bootstrap', (req, res) => { + const apiVersion = req.headers['api-version'] || '1.0'; res.send({ status: 'ok', data: { bootstrapped: !needsBootstrap(), + // Support both old and new formats based on version + ...(apiVersion === '1.0' ? { + loginMethods: listLoginMethods() + } : { loginMethod: getLoginMethod(), availableLoginMethods: listLoginMethods(), + }), multiuser: getActiveLoginMethod() === 'openid', }, }); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
src/app-account.js(1 hunks)
🔇 Additional comments (2)
src/app-account.js (2)
Line range hint 89-108: Review session validation during version transitions
The OpenID login flow and session validation look correct, but consider adding version-aware session validation to handle the transition between server versions smoothly. This could help prevent the reported issue where users need to log in again after server updates.
Let's verify the session handling implementation:
Consider adding version information to the session token and implementing backward-compatible validation:
// Example implementation
function validateSession(req, res) {
const session = getCurrentSession(req);
if (!session) return null;
// Handle sessions from different server versions
if (session.version !== getCurrentVersion()) {
return migrateSession(session);
}
return session;
}36-37: Breaking change in API response structure
The separation of loginMethod and availableLoginMethods improves clarity but changes the API contract. This change aligns well with fixing session persistence issues by providing more precise authentication state information.
Let's verify if any frontend code depends on the old response structure:
✅ Verification successful
API change is safe to proceed
The separation of loginMethod and availableLoginMethods is a safe change as:
- The only usage of these functions is within
src/app-account.jsitself - The implementation in
src/account-db.jsshows this is an internal API endpoint - No client-side code or documentation references the old
loginMethodsstructure
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any frontend code that might be accessing the old loginMethods property
rg -l "loginMethods" --type js --type ts --type vue --type jsx --type tsx
Length of output: 101
Script:
#!/bin/bash
# Search for loginMethods pattern in all files, showing context
rg -A 3 -B 3 "loginMethods" --type js --type ts
# Also check for any files with .vue extension
fd -e vue
# Search in vue files if they exist (using cat and grep as fallback)
fd -e vue -x cat {} | grep -A 3 -B 3 "loginMethods" || true
Length of output: 120
Script:
#!/bin/bash
# Let's check the function that's being called and its consumers
ast-grep --pattern 'function listLoginMethods' -A 5
ast-grep --pattern 'function getLoginMethod' -A 5
# Also check for any direct string references to these functions
rg "listLoginMethods|getLoginMethod" --type js --type ts
Length of output: 590
Script:
#!/bin/bash
# Let's examine the implementation and usage in account-db.js
cat src/account-db.js
# And check if there are any API clients or documentation files
fd -e md -e json -x grep -l "loginMethods" {} \;
Length of output: 5420
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (5)
src/account-db.js (2)
56-58: Add documentation for the login method resolution logic.Consider adding a comment explaining the precedence of login methods:
- Request-specified method (from req.body)
- Configured method (from config)
- Active method from database
- Default 'password' method
+ // Login method resolution order: + // 1. Request-specified method (req.body.loginMethod) + // 2. Configured method (config.loginMethod) + // 3. Active method from database + // 4. Default to 'password' const activeMethod = getActiveLoginMethod(); return config.loginMethod || activeMethod || 'password';
56-58: Consider improving session handling during auth method changes.The current implementation deletes all sessions when enabling/disabling OpenID (see
enableOpenIDanddisableOpenIDfunctions). This might be contributing to the re-login issues. Consider:
- Using transactions when modifying auth methods and sessions
- Implementing session migration instead of deletion
- Adding grace period for existing sessions during auth method changes
Would you like me to propose a more robust implementation for handling sessions during auth method changes?
src/accounts/openid.js (3)
Line range hint
307-311: Security: Hostname-only validation may be insufficient.The simplified hostname comparison removes protocol and port validation, which could lead to security vulnerabilities:
- Protocol downgrade attacks (https to http)
- Unauthorized access through different ports
Consider this more secure implementation that maintains flexibility while preventing downgrades:
- if (redirectUrl.hostname === serverUrl.hostname) { - return true; - } else { - return false; - } + // Ensure same hostname + if (redirectUrl.hostname !== serverUrl.hostname) { + return false; + } + + // Prevent protocol downgrade + if (serverUrl.protocol === 'https:' && redirectUrl.protocol !== 'https:') { + return false; + } + + // Optional: Allow specific ports if needed + if (redirectUrl.port && serverUrl.port && redirectUrl.port !== serverUrl.port) { + return false; + } + + return true;
Line range hint
307-311: Consider adding logging for security monitoring.To help detect potential security issues, consider adding logging when URL validation fails.
if (redirectUrl.hostname === serverUrl.hostname) { return true; } else { + console.warn('Redirect URL validation failed:', { + redirectHost: redirectUrl.hostname, + serverHost: serverUrl.hostname, + redirectUrl: url + }); return false; }
Line range hint
1-311: Consider investigating related session management code.While the URL validation change might help, the session persistence issue could also be affected by:
- Token expiration logic (lines 186-196)
- Session cleanup (line 203)
- Error handling during authentication
Consider a comprehensive review of the session management flow to ensure all potential causes of the re-login issue are addressed. Key areas to examine:
- Token expiration configuration
- Session cleanup timing
- Error handling during token validation
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
src/account-db.js(1 hunks)src/accounts/openid.js(1 hunks)
🔇 Additional comments (2)
src/account-db.js (1)
56-58: LGTM! Verify session handling across login method changes.
The addition of activeMethod as an intermediate fallback improves the login method resolution logic and could fix the session persistence issues. This change ensures that the active authentication method is properly considered before falling back to the default 'password' method.
Let's verify the session handling across different scenarios:
✅ Verification successful
Let me gather more information about the login method changes and session handling.
Based on the gathered information, I can now generate the final response:
Session handling is properly managed across login method changes
The code maintains session consistency when switching login methods:
- Sessions are explicitly cleared during critical auth changes (e.g., disabling OpenID)
- The
getActiveLoginMethodfunction correctly retrieves the current active method from the auth table - Session validation is consistently applied across all authenticated endpoints
- The fallback chain (config → active → password) ensures smooth transition between methods
The addition of activeMethod as an intermediate fallback is safe and maintains backward compatibility while fixing the re-login issues.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential session handling issues across login method changes
# Check if there are any direct session manipulations when changing login methods
rg -A 5 "DELETE FROM sessions"
# Look for other places where login method changes might affect sessions
ast-grep --pattern 'function $_(loginSettings) {
$$$
getAccountDb().mutate($_, ["openid"])
$$$
}'
Length of output: 1426
Script:
#!/bin/bash
# Check for login method changes and related session handling
# Look for functions that modify login methods
ast-grep --pattern 'function $_(loginSettings) {
$$$
config.loginMethod
$$$
}'
# Check for session creation/validation logic
rg -A 3 "createSession|validateSession"
# Look for places where activeMethod is used
rg -A 3 "getActiveLoginMethod"
Length of output: 8453
src/accounts/openid.js (1)
Line range hint 307-311: Verify the fix addresses the re-login issue.
Let's verify if this change resolves the session persistence issue while maintaining security.
|
@jfdoming Found a small change needed for the openid frontend, pushed it to the branch. I forgot it was linked to this PR. It does not impact the changes here |
There was a problem hiding this comment.
Tested locally and this patches it for me! Thank you @lelemm
(and apologies for taking so long again to review.. I have a baby on the way so my calendar lately is quite packed)
I feel you, gratz on the baby :) |
|
@MatissJanis there is still this: https://discord.com/channels/937901803608096828/1310791130492702740 |
* master: Fix Hype Bank sync (`HYPE_HYEEIT22`) (actualbudget#511) 🐛 Fix existing sessions when using the latest version with Openid (actualbudget#507)
This custom introduced a problem where users after updating the server will have to login again.
This also fix the header authentication that was broken