✨ OPENID: Added ACTUAL_OPENID_AUTO_REDIRECT variable to automatically redirect to OpenID login #4399
Conversation
✅ Deploy Preview for actualbudget ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Bundle Stats — desktop-clientHey there, this message comes from a GitHub action that helps you and reviewers to understand how these changes affect the size of this project's bundle. As this PR is updated, I'll keep you updated on how the bundle size is impacted. Total
Changeset
View detailed bundle breakdownAdded No assets were added Removed No assets were removed Bigger
Smaller No assets were smaller Unchanged
|
Bundle Stats — loot-coreHey there, this message comes from a GitHub action that helps you and reviewers to understand how these changes affect the size of this project's bundle. As this PR is updated, I'll keep you updated on how the bundle size is impacted. Total
Changeset
View detailed bundle breakdownAdded No assets were added Removed No assets were removed Bigger
Smaller No assets were smaller Unchanged No assets were unchanged |
lelemm
changed the title
WalkthroughThe pull request introduces multiple changes centered on updating the behavior of the sign-out process across the application. Many components now dispatch the Possibly related PRs
Suggested labels
Suggested reviewers
✨ Finishing Touches
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: 4
🧹 Nitpick comments (5)
packages/desktop-client/src/components/manager/subscribe/Login.tsx (2)
203-222: Improve type safety and error handling incallOpenIdRedirectURL.The function could benefit from the following improvements:
- Consistent return types
- TypeScript type annotations
- More robust error handling
Consider this refactored version:
-async function callOpenIdRedirectURL() { +async function callOpenIdRedirectURL(): Promise<string | Error> { const { error, redirect_url } = await send('subscribe-sign-in', { return_url: isElectron() ? await window.Actual.startOAuthServer() : window.location.origin, loginMethod: 'openid', }); if (!error) { if (isElectron()) { - window.Actual?.openURLInBrowser(redirect_url); + await window.Actual?.openURLInBrowser(redirect_url); } else { window.location.href = redirect_url; } - return ''; + return null; } else { - return error; + return new Error(error); } }
234-272: Consider adding a loading state during auto-login.While the auto-login logic is well-structured, users might benefit from seeing a loading indicator during the OpenID redirect process.
Consider adding a loading state:
+const [isAutoLoginLoading, setIsAutoLoginLoading] = useState(false); useEffect(() => { if ( checked && autoLogin && loginMethods.some(method => method.method === 'openid') && masterCreated ) { + setIsAutoLoginLoading(true); callOpenIdRedirectURL(); } }, [checked, autoLogin, loginMethods, masterCreated]);Then update the return statement to show a loading indicator when
isAutoLoginLoadingis true.packages/loot-core/src/client/actions/user.ts (1)
43-62:❓ Verification inconclusive
Verify URL validation for OpenID logout redirect.
The direct assignment to
window.location.hrefcould be vulnerable to XSS if theredirect_urlis not properly validated.Run this script to check URL validation in the OpenID logout handler:
Consider cleanup actions for OpenID logout.
When OpenID logout is triggered, the function returns before dispatching cleanup actions (
getUserData,loadGlobalPrefs,closeBudget). Consider if these actions should still be dispatched before the redirect.if (openidEnabled && !isElectron()) { const ret = (await send('subscribe-logout-openid', { returnUrl: window.location.origin, })) || {}; redirect_url = ret.redirect_url; + // Perform cleanup before redirect + dispatch(getUserData()); + dispatch(loadGlobalPrefs()); + dispatch(closeBudget()); + dispatch({ type: constants.SIGN_OUT }); window.location.href = redirect_url; } else {
🏁 Script executed:
#!/bin/bash # Search for URL validation in the OpenID logout handler ast-grep --pattern 'function loginOutOpenIdProvider($_) { $$$ if (!isValidRedirectUrl($_)) { $$$ } $$$ }'Length of output: 119
Attention: Verify URL Validation and Cleanup in OpenID Logout Flow
- URL Validation: There is currently no evidence that the
redirect_urlreturned from the OpenID logout (subscribe-logout-openid) is being explicitly validated (e.g., via anisValidRedirectUrlcheck). Please manually verify whether URL validation is performed elsewhere in the codebase or, if absent, add proper sanitization to mitigate XSS risks.- Cleanup Actions: In the OpenID branch, cleanup actions (such as dispatching
getUserData(),loadGlobalPrefs(),closeBudget(), and theSIGN_OUTaction) are skipped before redirecting. Confirm if these actions should be executed before the redirection or if they are intentionally omitted for a reason.Diff Suggestion (Good-to-Have Improvement):
if (openidEnabled && !isElectron()) { const ret = (await send('subscribe-logout-openid', { returnUrl: window.location.origin, })) || {}; redirect_url = ret.redirect_url; + // Validate redirect_url for safety, e.g., + // if (!isValidRedirectUrl(redirect_url)) { + // // handle error or fallback + // } + + // Optionally perform cleanup actions before redirect if needed: + // dispatch(getUserData()); + // dispatch(loadGlobalPrefs()); + // dispatch(closeBudget()); + // dispatch({ type: constants.SIGN_OUT }); window.location.href = redirect_url; } else {packages/sync-server/src/load-config.js (1)
238-265: Consider adding debug logging for OpenID configuration.The debug logging section could be enhanced to include the OpenID configuration details, particularly the new
autoLoginsetting.Add the following debug statement:
if (finalConfig.upload) { debug(`using file sync limit ${finalConfig.upload.fileSizeSyncLimitMB}mb`); debug( `using sync encrypted file limit ${finalConfig.upload.syncEncryptedFileSizeLimitMB}mb`, ); debug(`using file limit ${finalConfig.upload.fileSizeLimitMB}mb`); } + +if (finalConfig.openId) { + debug(`using OpenID auto login: ${finalConfig.openId.autoLogin}`); +}packages/sync-server/src/accounts/openid.js (1)
332-373: LGTM! The OpenID provider logout is well-implemented.The implementation correctly handles error cases and reuses existing OpenID client setup code. Consider improving the error handling when the logout URL generation fails.
Consider adding more context to the error log when the logout URL generation fails:
- console.log("Could not generate logout URL", err); + console.error('Could not generate logout URL:', err.message);🧰 Tools
🪛 GitHub Check: lint
[failure] 367-367:
Delete··
[failure] 370-370:
Replace"Could·not·generate·logout·URL"with'Could·not·generate·logout·URL'🪛 GitHub Actions: Test
[error] 367-367: Prettier formatting check failed. Delete
··.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
upcoming-release-notes/4399.mdis excluded by!**/*.md
📒 Files selected for processing (19)
packages/desktop-client/src/components/App.tsx(1 hunks)packages/desktop-client/src/components/LoggedInUser.tsx(1 hunks)packages/desktop-client/src/components/ServerContext.tsx(4 hunks)packages/desktop-client/src/components/admin/UserAccess/UserAccessRow.tsx(1 hunks)packages/desktop-client/src/components/admin/UserDirectory/UserDirectory.tsx(1 hunks)packages/desktop-client/src/components/manager/ConfigServer.tsx(3 hunks)packages/desktop-client/src/components/manager/subscribe/Login.tsx(6 hunks)packages/desktop-client/src/components/manager/subscribe/common.tsx(4 hunks)packages/desktop-client/src/components/modals/EditAccess.tsx(1 hunks)packages/desktop-client/src/components/modals/EditUser.tsx(1 hunks)packages/loot-core/src/client/actions/user.ts(2 hunks)packages/loot-core/src/client/shared-listeners.ts(1 hunks)packages/loot-core/src/server/main.ts(2 hunks)packages/loot-core/src/types/server-handlers.d.ts(2 hunks)packages/sync-server/src/accounts/openid.js(1 hunks)packages/sync-server/src/app-account.js(2 hunks)packages/sync-server/src/app-openid.js(2 hunks)packages/sync-server/src/config-types.ts(1 hunks)packages/sync-server/src/load-config.js(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
packages/sync-server/src/load-config.js (1)
Learnt from: matt-fidd
PR: actualbudget/actual#4363
File: packages/sync-server/src/scripts/enable-openid.js:6-6
Timestamp: 2025-02-11T23:39:06.219Z
Learning: In the Actual codebase, when `config` is used as a function parameter, the imported configuration should use the alias `finalConfig` to avoid naming conflicts. Example: `import { config as finalConfig } from '../load-config.js'`.
🪛 GitHub Check: lint
packages/sync-server/src/accounts/openid.js
[failure] 367-367:
Delete ··
[failure] 370-370:
Replace "Could·not·generate·logout·URL" with 'Could·not·generate·logout·URL'
🪛 GitHub Actions: Test
packages/sync-server/src/accounts/openid.js
[error] 367-367: Prettier formatting check failed. Delete ··.
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: Functional
- GitHub Check: Visual regression
- GitHub Check: build (windows-latest)
- GitHub Check: build (ubuntu-latest)
- GitHub Check: Build Docker image (alpine)
- GitHub Check: Build Docker image (ubuntu)
🔇 Additional comments (23)
packages/desktop-client/src/components/manager/subscribe/Login.tsx (1)
23-27: LGTM! Well-organized hook imports.The new hooks from ServerContext are appropriately named and their purposes are clear.
packages/sync-server/src/config-types.ts (1)
28-28: LGTM! Well-typed configuration property.The optional
autoLoginboolean property is correctly typed and appropriately placed within theopenIdconfiguration object.packages/desktop-client/src/components/manager/subscribe/common.tsx (1)
33-33: LGTM! Clean integration of auto-login state.The
useSetAutoLoginhook is well-integrated into the existing bootstrap flow, maintaining synchronization with other server states.Also applies to: 68-68, 88-88
packages/sync-server/src/app-account.js (2)
13-13: LGTM! Safe implementation of auto-login configuration.The implementation safely checks for the presence of OpenID configuration and the auto-login property, defaulting to
falsewhen either is not present.Also applies to: 41-44
41-44: LGTM! The autoLogin property is correctly implemented.The implementation safely computes the
autoLoginproperty by checking for the presence of bothopenIdandautoLoginin the config object, with a sensible default offalse.packages/desktop-client/src/components/admin/UserAccess/UserAccessRow.tsx (1)
79-79: LGTM! Consistent sign-out behavior.The change ensures proper OpenID provider logout when the session expires.
packages/desktop-client/src/components/modals/EditAccess.tsx (1)
77-77: LGTM! Consistent sign-out behavior.The change ensures proper OpenID provider logout when the session expires.
packages/desktop-client/src/components/ServerContext.tsx (3)
34-35: LGTM! Well-defined context interface.The
ServerContextValuetype and default values are properly defined for the auto-login feature.Also applies to: 48-49
90-92: LGTM! Clean hook implementations.The hooks follow React conventions and provide a clean API for accessing auto-login state.
101-101: LGTM! Proper state management.The auto-login state is correctly managed and provided through the context.
Also applies to: 171-172
packages/desktop-client/src/components/manager/ConfigServer.tsx (3)
20-24: LGTM!The import statement is well-organized and follows the pattern of importing hooks from
ServerContext.
35-35: LGTM!The
multiuserEnabledhook is correctly initialized and will be used to determine the sign-out behavior.
87-87: LGTM!The
signOutaction is now correctly parameterized withmultiuserEnabledto ensure consistent sign-out behavior across the application.packages/desktop-client/src/components/LoggedInUser.tsx (1)
121-121: LGTM!The
signOutaction is correctly parameterized withmultiuserEnabledto maintain consistent sign-out behavior.packages/desktop-client/src/components/App.tsx (2)
143-143: LGTM!The
signOutaction is correctly parameterized withtrueto ensure a complete sign-out when the token expires.
124-126: LGTM!Good catch on removing
cloudFileIdfrom the dependencies to prevent a hard crash when closing the budget in Electron.packages/sync-server/src/load-config.js (1)
179-193: LGTM!The implementation of
autoLoginproperty is well-structured:
- Correctly preserves existing OpenID configuration
- Properly validates the environment variable
- Follows the pattern used for other boolean environment variables
packages/loot-core/src/types/server-handlers.d.ts (2)
302-302: LGTM! TheautoLoginproperty is correctly typed.The addition of the
autoLoginboolean property to thesubscribe-needs-bootstrapresponse type aligns with the PR objectives for implementing auto-login functionality.
344-346: LGTM! Thesubscribe-logout-openidhandler is well-defined.The handler's type definition is consistent with other OpenID-related handlers and follows the same pattern:
- Takes a
returnUrlparameter for post-logout redirection- Returns a promise with optional error and redirect URL fields
packages/loot-core/src/server/main.ts (1)
1585-1585: LGTM! TheautoLoginproperty is safely initialized.The property is correctly extracted from the server response with a safe default value of
false, preventing undefined behavior.packages/desktop-client/src/components/admin/UserDirectory/UserDirectory.tsx (1)
175-175: LGTM! The signOut parameter is correctly updated.The change to pass
truetosignOutaligns with the PR's objective to implement OpenID provider logout functionality.packages/loot-core/src/client/shared-listeners.ts (1)
327-327: LGTM! The signOut parameter is correctly updated.The change to pass
truetosignOutaligns with the PR's objective to implement OpenID provider logout functionality.packages/desktop-client/src/components/modals/EditUser.tsx (1)
103-103: LGTM! Enhanced security for expired session handling.The update to include
truein thesignOutaction ensures proper OpenID provider logout when a session expires, maintaining security best practices.
packages/desktop-client/src/components/manager/subscribe/Login.tsx
Outdated
Show resolved
Hide resolved
enhancements md code review Apply suggestions from code review Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> linter changed deprecated reference linter removing name and default value to prevent access leak typecheck
lelemm
force-pushed
the
enhance/openid_01
branch
from
c43aab9 to
4fc32f5
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
packages/desktop-client/src/components/manager/subscribe/Login.tsx (2)
205-224: Add TypeScript type annotations to improve type safety.The
callOpenIdRedirectURLfunction could benefit from explicit type annotations for better type safety and documentation.-async function callOpenIdRedirectURL() { +async function callOpenIdRedirectURL(): Promise<string> { + type SignInResponse = { + error?: string; + redirect_url: string; + }; + - const { error, redirect_url } = await send('subscribe-sign-in', { + const { error, redirect_url } = await send<SignInResponse>('subscribe-sign-in', {
265-274: Add error handling to auto-login effect.The auto-login effect should handle potential errors from
callOpenIdRedirectURLto provide better user feedback.useEffect(() => { if ( checked && autoLogin && loginMethods.some(method => method.method === 'openid') && masterCreated ) { - callOpenIdRedirectURL(); + callOpenIdRedirectURL().catch(error => { + setError(error); + }); } }, [checked, autoLogin, loginMethods, masterCreated]);packages/loot-core/src/client/actions/user.ts (1)
43-69: Add type safety and URL validation.The OpenID logout response and redirect URL handling could be improved with proper type safety and URL validation.
Consider adding these types and URL validation:
interface OpenIDLogoutResponse { redirect_url?: string; } function isValidRedirectURL(url: string): boolean { try { const redirectURL = new URL(url); const originURL = new URL(window.location.origin); // Only allow redirects to the same origin or trusted OpenID providers return redirectURL.origin === originURL.origin || redirectURL.origin.endsWith('your-openid-provider.com'); } catch { return false; } } export function signOut(openidEnabled: boolean) { return async (dispatch: AppDispatch) => { // ... previous code ... if (openidEnabled && !isElectron()) { try { const ret = await send<OpenIDLogoutResponse>('subscribe-logout-openid', { returnUrl: window.location.origin, }); if (ret?.redirect_url && isValidRedirectURL(ret.redirect_url)) { window.location.href = ret.redirect_url; return; } } catch (error) { console.error('Failed to logout from OpenID provider:', error); } } // ... remaining code ... }; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
packages/desktop-client/src/components/App.tsx(1 hunks)packages/desktop-client/src/components/LoggedInUser.tsx(1 hunks)packages/desktop-client/src/components/ServerContext.tsx(4 hunks)packages/desktop-client/src/components/admin/UserAccess/UserAccessRow.tsx(1 hunks)packages/desktop-client/src/components/admin/UserDirectory/UserDirectory.tsx(1 hunks)packages/desktop-client/src/components/manager/ConfigServer.tsx(3 hunks)packages/desktop-client/src/components/manager/subscribe/Login.tsx(6 hunks)packages/desktop-client/src/components/manager/subscribe/common.tsx(4 hunks)packages/desktop-client/src/components/modals/EditAccess.tsx(1 hunks)packages/desktop-client/src/components/modals/EditUser.tsx(1 hunks)packages/loot-core/src/client/actions/user.ts(2 hunks)packages/loot-core/src/client/shared-listeners.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
- packages/loot-core/src/client/shared-listeners.ts
- packages/desktop-client/src/components/LoggedInUser.tsx
- packages/desktop-client/src/components/modals/EditUser.tsx
- packages/desktop-client/src/components/admin/UserAccess/UserAccessRow.tsx
- packages/desktop-client/src/components/admin/UserDirectory/UserDirectory.tsx
- packages/desktop-client/src/components/modals/EditAccess.tsx
- packages/desktop-client/src/components/manager/ConfigServer.tsx
- packages/desktop-client/src/components/App.tsx
- packages/desktop-client/src/components/manager/subscribe/common.tsx
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Visual regression
- GitHub Check: Functional
- GitHub Check: Build Docker image (alpine)
- GitHub Check: Build Docker image (ubuntu)
🔇 Additional comments (9)
packages/desktop-client/src/components/ServerContext.tsx (5)
34-35: LGTM! Type definitions are well-structured.The new type definitions for
autoLoginandsetAutoLoginfollow React conventions and are properly typed.
48-49: LGTM! Default values are properly initialized.The default values for
autoLoginandsetAutoLoginfollow React context best practices.
90-92: LGTM! Hooks are well-implemented.The new hooks
useAutoLoginanduseSetAutoLoginfollow React hook conventions and are properly implemented usinguseContext.
101-101: LGTM! State management is properly implemented.The
autoLoginstate is correctly initialized usinguseState.
171-172: LGTM! Context provider values are properly set.The
autoLoginandsetAutoLoginvalues are correctly passed to the context provider.packages/desktop-client/src/components/manager/subscribe/Login.tsx (3)
236-237: LGTM! State management is properly implemented.The
autoLoginandmasterCreatedstates are correctly initialized and follow React conventions.
239-243: LGTM! Effect is well-implemented.The effect correctly checks for OpenID method and updates master creation status with appropriate dependencies.
297-314: LGTM! Loading view is well-implemented and accessible.The loading view provides good visual feedback and follows accessibility best practices with
aria-live.packages/loot-core/src/client/actions/user.ts (1)
2-2: LGTM!The import of
isElectronis appropriate for conditionally handling OpenID logout in web environments.
lelemm
changed the title
lelemm
changed the title
|
/update-vrt |
1 similar comment
|
/update-vrt |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/sync-server/src/load-config.js (1)
181-183: Consider using a deep copy for better immutability.A shallow copy might not protect nested objects from mutation. Consider using a deep copy to ensure complete immutability.
Apply this diff to implement a deep copy:
- return { - ...config.openId, - }; + return JSON.parse(JSON.stringify(config.openId || {}));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
upcoming-release-notes/4399.mdis excluded by!**/*.md
📒 Files selected for processing (6)
packages/desktop-client/src/components/App.tsx(1 hunks)packages/loot-core/src/server/main.ts(2 hunks)packages/loot-core/src/types/server-handlers.d.ts(2 hunks)packages/sync-server/src/app-account.js(2 hunks)packages/sync-server/src/config-types.ts(1 hunks)packages/sync-server/src/load-config.js(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/sync-server/src/config-types.ts
- packages/sync-server/src/app-account.js
- packages/desktop-client/src/components/App.tsx
- packages/loot-core/src/types/server-handlers.d.ts
🧰 Additional context used
🧠 Learnings (1)
packages/sync-server/src/load-config.js (1)
Learnt from: matt-fidd
PR: actualbudget/actual#4363
File: packages/sync-server/src/scripts/enable-openid.js:6-6
Timestamp: 2025-02-11T23:39:06.219Z
Learning: In the Actual codebase, when `config` is used as a function parameter, the imported configuration should use the alias `finalConfig` to avoid naming conflicts. Example: `import { config as finalConfig } from '../load-config.js'`.
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: Functional
- GitHub Check: Visual regression
- GitHub Check: build (macos-latest)
- GitHub Check: Analyze
- GitHub Check: build (windows-latest)
- GitHub Check: build (ubuntu-latest)
- GitHub Check: Build Docker image (alpine)
- GitHub Check: Build Docker image (ubuntu)
🔇 Additional comments (4)
packages/sync-server/src/load-config.js (2)
92-92: LGTM!The
openIdAutoRedirectproperty is added with a safe default value offalse, following the codebase's naming conventions.
239-249: LGTM!The implementation follows the established pattern for boolean environment variables, with proper validation and error handling. The fallback to the config value is correctly implemented.
packages/loot-core/src/server/main.ts (2)
643-643: LGTM!The
autoLoginproperty is correctly added to the response with a safe default value offalse.
786-812: LGTM! Past review feedback has been incorporated.The implementation includes proper URL encoding and specific error handling as suggested in the previous review.
lelemm
changed the title
Based on user feedback, added this variable
ACTUAL_OPENID_AUTO_REDIRECT, so when a user tries to open actual and openid is enable, the user gets redirected to the openid login.I needed to introduce a logout flow for the openid provider. When you logout from actual, it will try to logout from the openid provider. I guess we need to test on multiple providers to ensure that its working properly.
I may add another variable enable/disable provider logout when logging out from actual