✨ New OpenID first login UX #4446
Conversation
actual-github-bot
bot
changed the title
✅ Deploy Preview for actualbudget ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
|
/update-vrt |
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 No assets were bigger Smaller
Unchanged No assets were unchanged |
WalkthroughThe changes span multiple components and services involved in OpenID authentication and password handling. In the desktop client, the Suggested labels
Suggested reviewers
Tip CodeRabbit's docstrings feature is now available as part of our Pro Plan! Simply use the command 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms (8)
🔇 Additional comments (1)
✨ 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: 3
🧹 Nitpick comments (3)
packages/sync-server/src/app-openid.js (1)
60-84: LGTM! The password validation and error handling look good.The changes correctly implement password validation for the OpenID configuration endpoint and follow RESTful practices by using POST instead of GET.
Consider adding a more descriptive error message to help users understand why their password was invalid:
- res.status(400).send({ status: 'error', reason: 'invalid-password' }); + res.status(400).send({ + status: 'error', + reason: 'invalid-password', + details: 'The provided password is incorrect' + });packages/desktop-client/src/components/manager/subscribe/Login.tsx (2)
141-151: Add client-side password validation.While the password is required for first-time login, there's no client-side validation of password strength. Consider adding password requirements feedback to enhance security and user experience.
<BigInput autoFocus={true} placeholder={t('Enter server password')} type="password" + pattern="^(?=.*[A-Za-z])(?=.*\d)[A-Za-z\d]{8,}$" + title={t('Password must be at least 8 characters and include both letters and numbers')} onChangeValue={newValue => { setFirstLoginPassword(newValue); }} style={{ flex: 1 }} />
183-195: Add loading state during OpenID configuration fetch.The configuration review button should show a loading state while fetching the OpenID configuration to provide better user feedback.
+const [isLoading, setIsLoading] = useState(false); onPress={() => { + setIsLoading(true); send('get-openid-config', { password: firstLoginPassword, }).then(config => { if ('error' in config) { setError(config.error); } else if ('openId' in config) { setError(null); setOpenIdConfig(config.openId); setReviewOpenIdConfiguration(true); } + setIsLoading(false); }); }} +isLoading={isLoading}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
upcoming-release-notes/4446.mdis excluded by!**/*.md
📒 Files selected for processing (9)
packages/desktop-client/src/components/manager/subscribe/ConfirmPasswordForm.tsx(2 hunks)packages/desktop-client/src/components/manager/subscribe/Login.tsx(7 hunks)packages/desktop-client/src/components/manager/subscribe/OpenIdForm.tsx(3 hunks)packages/loot-core/src/server/main.ts(1 hunks)packages/loot-core/src/types/server-handlers.d.ts(1 hunks)packages/sync-server/src/accounts/openid.js(3 hunks)packages/sync-server/src/accounts/password.js(1 hunks)packages/sync-server/src/app-account.js(1 hunks)packages/sync-server/src/app-openid.js(3 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
packages/loot-core/src/server/main.ts
[error] 1290-1291: This is an unexpected use of the debugger statement.
Unsafe fix: Remove debugger statement
(lint/suspicious/noDebugger)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build Docker image (alpine)
🔇 Additional comments (9)
packages/sync-server/src/accounts/password.js (1)
126-148: LGTM! The password validation implementation is secure and follows best practices.The function correctly:
- Validates password format
- Retrieves password hash securely
- Uses bcrypt for password comparison
packages/desktop-client/src/components/manager/subscribe/ConfirmPasswordForm.tsx (1)
98-102: LGTM! The style customization follows React best practices.The optional style parameter with null default and spread operator usage is correct.
Also applies to: 129-129
packages/sync-server/src/accounts/openid.js (1)
77-80: LGTM! The password parameter addition enhances security.The optional parameter with empty string default is appropriate for backward compatibility.
packages/loot-core/src/types/server-handlers.d.ts (1)
292-298: LGTM! The type definition correctly reflects the new password requirement.The updated method signature properly enforces the password parameter while maintaining the existing return type structure.
packages/desktop-client/src/components/manager/subscribe/OpenIdForm.tsx (2)
28-28: LGTM! The prop type correctly reflects the new data flow.The
openIdDataprop is properly typed as optional and uses theOpenIdConfigtype.
67-73: LGTM! The effect correctly handles the provided OpenID data.The implementation safely accesses properties using nullish coalescing and properly updates the form state.
packages/loot-core/src/server/main.ts (1)
1288-1309: LGTM! The handler correctly implements password validation.The implementation:
- Properly accepts and validates the password parameter
- Makes a POST request to the correct endpoint
- Includes the user token in headers
- Handles errors appropriately
🧰 Tools
🪛 Biome (1.9.4)
[error] 1290-1291: This is an unexpected use of the debugger statement.
Unsafe fix: Remove debugger statement
(lint/suspicious/noDebugger)
packages/desktop-client/src/components/manager/subscribe/Login.tsx (2)
21-21: LGTM: New imports and state variables are well-defined.The new state variables and imports are properly typed and align with the PR objectives for implementing password validation during first-time OpenID login.
Also applies to: 91-92
364-370: LGTM: Improved login methods organization.The conditional rendering of login methods is now more organized and maintainable. The separation of concerns between different login methods is well-implemented.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/desktop-client/src/components/manager/subscribe/Login.tsx (2)
142-152: Consider adding password validation requirements.While the password input is implemented correctly, consider adding validation requirements (minimum length, complexity, etc.) to ensure secure passwords for server owners.
<Input autoFocus={true} placeholder={t('Enter server password')} type="password" + minLength={8} + pattern="^(?=.*[A-Za-z])(?=.*\d)[A-Za-z\d]{8,}$" + title={t('Password must be at least 8 characters and contain both letters and numbers')} onChangeValue={newValue => { setFirstLoginPassword(newValue); }} style={{ flex: 1 }} />
184-196: Add loading state during OpenID configuration fetch.The configuration fetch operation lacks a loading indicator, which might lead to a poor user experience if the server response is slow.
+const [isLoading, setIsLoading] = useState(false); <Button variant="bare" isDisabled={firstLoginPassword === '' && warnMasterCreation} onPress={() => { + setIsLoading(true); send('get-openid-config', { password: firstLoginPassword, }).then(config => { if ('error' in config) { setError(config.error); } else if ('openId' in config) { setError(null); setOpenIdConfig(config.openId); setReviewOpenIdConfiguration(true); } + setIsLoading(false); }); }} style={{ marginTop: 5 }} > + {isLoading ? ( + <AnimatedLoading style={{ width: 16, height: 16 }} /> + ) : ( <Trans>Review OpenID configuration</Trans> + )} </Button>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/desktop-client/src/components/manager/subscribe/Login.tsx(10 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: build (macos-latest)
- GitHub Check: build (windows-latest)
- GitHub Check: Analyze
- GitHub Check: build (ubuntu-latest)
- GitHub Check: Build Docker image (alpine)
- GitHub Check: Wait for Netlify build to finish
- GitHub Check: Build Docker image (ubuntu)
🔇 Additional comments (3)
packages/desktop-client/src/components/manager/subscribe/Login.tsx (3)
7-8: LGTM! New imports and state variables are well-structured.The new imports and state variables are properly organized and align with the PR's objective of enhancing the OpenID first login UX.
Also applies to: 20-20, 23-24, 92-93
363-398: LGTM! Login method selection UI is well-implemented.The new login method selection UI using Popover and Menu components provides a better user experience while maintaining proper state management and error handling.
16-16:✅ Verification successful
Verify OpenID configuration type structure.
Ensure that the OpenID configuration type (
OpenIdConfig) properly defines all required fields for secure OpenID authentication.Also applies to: 92-92, 219-219
🏁 Script executed:
#!/bin/bash # Description: Check OpenID configuration type definition ast-grep --pattern 'type OpenIdConfig = { $$$ }'Length of output: 553
OpenID Config Structure Verified
The OpenID configuration type (
OpenIdConfig) inpackages/loot-core/src/types/models/openid.d.tsincludes all the required fields—selectedProvider,issuer,client_id,client_secret, andserver_hostname—and meets our security criteria. No further changes are needed; just ensure that all references (including at lines 92 and 219) consistently use this definition.
|
/update-vrt |
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/desktop-client/src/components/common/Input.tsx(3 hunks)packages/desktop-client/src/components/manager/subscribe/Login.tsx(12 hunks)packages/desktop-client/src/components/manager/subscribe/OpenIdForm.tsx(10 hunks)
🧰 Additional context used
🪛 GitHub Check: lint
packages/desktop-client/src/components/manager/subscribe/OpenIdForm.tsx
[warning] 19-19:
../../common/Input import should occur before import of ../../common/Link
[warning] 20-20:
../../responsive/ResponsiveProvider import should occur before import of ../../ServerContext
[failure] 221-221:
Insert ·
🪛 ESLint
packages/desktop-client/src/components/manager/subscribe/OpenIdForm.tsx
[error] 221-221: Insert ·
(prettier/prettier)
🪛 GitHub Actions: Test
packages/desktop-client/src/components/manager/subscribe/OpenIdForm.tsx
[warning] 19-19: ../../common/Input import should occur before import of ../../common/Link (import/order)
[warning] 20-20: ../../responsive/ResponsiveProvider import should occur before import of ../../ServerContext (import/order)
[error] 221-221: Insert · (prettier/prettier)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: build (windows-latest)
- GitHub Check: Build Docker image (alpine)
- GitHub Check: build (ubuntu-latest)
- GitHub Check: Wait for Netlify build to finish
- GitHub Check: Analyze
- GitHub Check: Build Docker image (ubuntu)
🔇 Additional comments (18)
packages/desktop-client/src/components/common/Input.tsx (3)
14-14: New import added to support responsive input component.The import for
useResponsiveis appropriately placed and will be used by the newResponsiveInputcomponent.
26-26: Good change: exportingInputPropstype.Exporting the
InputPropstype improves reusability and allows for better type checking in components that use these inputs.
116-120: Well-implemented responsive input component.The new
ResponsiveInputcomponent elegantly switches betweenBigInputandInputbased on screen width. This improves the user experience on different device sizes, particularly for the OpenID login form.packages/desktop-client/src/components/manager/subscribe/OpenIdForm.tsx (6)
29-29: Good API change: replacingloadDatawithopenIdData.Replacing the boolean
loadDataprop with a strongly-typedopenIdDataobject improves the component API and makes the data flow more explicit.
67-74: Improved data handling for OpenID configuration.The component now directly uses the provided configuration data rather than fetching it asynchronously. This is a better architectural approach that follows the principle of passing data through props.
137-144: Enhanced inputs with responsive behavior.Replacing
InputwithResponsiveInputimproves the user experience on different device sizes.
172-179: Enhanced inputs with responsive behavior.Replacing
InputwithResponsiveInputfor the Client ID field improves the user experience on different device sizes.
192-199: Enhanced inputs with responsive behavior.Replacing
InputwithResponsiveInputfor the Client Secret field improves the user experience on different device sizes.
431-432: Enhanced select component with responsive styling.Adding conditional styling based on screen width improves the user experience on mobile devices.
packages/desktop-client/src/components/manager/subscribe/Login.tsx (9)
66-73: Good migration to ResponsiveInput.Replacing
InputwithResponsiveInputfor the password field makes the UI more responsive across different device sizes.
77-81: Enhanced button styling with responsive properties.The button styling now adapts to screen size, ensuring a better user experience on mobile devices.
97-99: Good addition of state variables for OpenID login flow.The new state variables effectively manage the OpenID configuration and first-time login password, which are crucial for the enhanced security flow.
121-121: Security enhancement: Password included in OpenID sign-in request.Including the password in the sign-in request improves security by ensuring that only authenticated users can proceed with OpenID login, particularly for the first user.
139-157: Well-implemented password field for first-time OpenID login.The conditional rendering of the password field for first-time login effectively addresses the security requirement to validate the user before allowing OpenID setup.
161-174: Good UX: Button state and text based on context.The button is properly disabled when a password is required but not provided, and the text changes appropriately based on whether it's a first-time login.
188-201: Security enhancement: Password validation for OpenID configuration review.Including the password in the request to fetch OpenID configuration prevents unauthorized access to sensitive configuration details.
214-249: Improved OpenIdForm integration.The OpenIdForm now receives the configuration data directly as a prop rather than fetching it internally. The cancel button appropriately resets the state, including the password.
366-412: Enhanced login method selection UI.The login method selection has been improved with a popover menu that clearly separates the active method from available alternatives. This enhances user experience by providing a cleaner interface.
packages/desktop-client/src/components/manager/subscribe/OpenIdForm.tsx
Outdated
Show resolved
Hide resolved
packages/desktop-client/src/components/manager/subscribe/OpenIdForm.tsx
Outdated
Show resolved
Hide resolved
Gravacao.de.Tela.2025-02-24.123924.mp4
Changed the first login with OpenID UX: