✨ 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
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 |
WalkthroughThe changes encompass modifications to both UI components and server-side authentication processes. In the desktop client, the Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧬 Code Definitions (1)packages/loot-core/src/server/main.ts (3)
⏰ Context from checks skipped due to timeout of 90000ms (7)
🔇 Additional comments (8)
✨ Finishing Touches
🪧 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
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/desktop-client/src/components/manager/subscribe/Login.tsx (1)
218-218: Remove extra space in style propertyThere's an extra space after the 'bold' value which should be removed.
- fontWeight: 'bold ', + fontWeight: 'bold',
📜 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(12 hunks)
🔇 Additional comments (8)
packages/desktop-client/src/components/manager/subscribe/Login.tsx (8)
96-97: Added new state variables for OpenID first login flowThese new state variables enhance the OpenID login experience by storing the OpenID configuration and password for first-time login scenarios. This matches the PR objective of implementing password validation during first login.
120-120: Password now included in OpenID sign-in requestGood addition of the password parameter to the sign-in request, which supports the PR objective of implementing password validation for first-time OpenID login.
146-156: Password input conditionally rendered for first-time loginThe conditional rendering of the password input field when
warnMasterCreationis true is well-implemented. This properly supports the PR objective of requiring password validation during first login with OpenID.
167-167: Button disabled when password is emptyGood UX enhancement to disable the login button when password is required but not provided, preventing submission of invalid forms.
169-173: Dynamic button text based on login scenarioThe conditional text for the button improves user experience by clearly differentiating between first-time setup and regular sign-in.
187-200: Password validation for OpenID configuration reviewThis implementation correctly addresses the PR objective by requiring password verification before allowing users to review OpenID configuration during first login. The error handling is comprehensive.
238-238: Password state reset when cancellingGood practice to reset the password state when cancelling the operation, preventing sensitive data from persisting in memory longer than necessary.
293-414: Improved login method selection UIThe login method selection has been enhanced with a dropdown menu using Popover and Menu components, which provides a more intuitive user experience compared to the previous implementation.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/component-library/src/Input.tsx (1)
11-11: Fix import ordering.The import of
useResponsiveshould appear before the import of./stylesaccording to project conventions.-import { styles, type CSSProperties } from './styles'; -import { theme } from './theme'; -import { useResponsive } from './hooks/useResponsive'; +import { useResponsive } from './hooks/useResponsive'; +import { styles, type CSSProperties } from './styles'; +import { theme } from './theme';🧰 Tools
🪛 GitHub Check: lint
[warning] 11-11:
./hooks/useResponsiveimport should occur before import of./styles🪛 GitHub Actions: Test
[warning] 11-11:
./hooks/useResponsiveimport should occur before import of./stylesimport/order
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/component-library/src/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)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/desktop-client/src/components/manager/subscribe/Login.tsx
🧰 Additional context used
🪛 GitHub Check: lint
packages/component-library/src/Input.tsx
[warning] 11-11:
./hooks/useResponsive import should occur before import of ./styles
🪛 GitHub Actions: Test
packages/component-library/src/Input.tsx
[warning] 11-11: ./hooks/useResponsive import should occur before import of ./styles import/order
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: build (macos-latest)
- GitHub Check: build (windows-latest)
- GitHub Check: build (ubuntu-latest)
- GitHub Check: Wait for Netlify build to finish
- GitHub Check: Analyze
- GitHub Check: Build Docker image (alpine)
- GitHub Check: Build Docker image (ubuntu)
🔇 Additional comments (12)
packages/component-library/src/Input.tsx (2)
23-30: LGTM! Enhanced type accessibility.Making the
InputPropstype exported allows it to be reused in other components, improving modularity.
106-110: Good addition of responsive input component.The
ResponsiveInputcomponent elegantly handles responsive rendering based on screen width, which will improve user experience across different devices.packages/desktop-client/src/components/manager/subscribe/OpenIdForm.tsx (10)
6-7: LGTM! Added responsive functionality imports.The addition of these imports enables responsive behavior in the form.
29-29: Improved data flow architecture.Changed from a boolean flag that triggers data loading to directly passing the OpenID configuration data. This is a better pattern as it makes the component more predictable and testable.
49-52: LGTM! Enhanced component with responsive capabilities.Adding the
useResponsivehook enables the component to adapt to different screen sizes.
67-74: Simplified data handling.The effect now directly uses the passed
openIdDatainstead of making an API call. This streamlines the data flow and reduces unnecessary network requests.
137-144: Enhanced input with responsive behavior.Replacing
InputwithResponsiveInputimproves the user experience across different screen sizes.
172-180: LGTM! Consistent use of responsive inputs.Using
ResponsiveInputconsistently throughout the form ensures a cohesive user experience.
192-199: LGTM! Consistent use of responsive inputs.Using
ResponsiveInputconsistently throughout the form ensures a cohesive user experience.
221-221: LGTM! Added responsive styling.The conditional styling based on screen width enhances the user experience on smaller devices.
410-410: LGTM! Added responsive functionality to the provider selector.Adding responsive capabilities to the provider selector component ensures consistent behavior across the form.
431-431: LGTM! Consistent responsive styling.The conditional styling based on screen width is consistently applied across components.
Changed the first login with OpenID UX:
Steps to test this.
yarnrm -rf packages/sync-server/server-filesto delete all server filesrm packages/sync-server/.migrateto delete old migrationsyarn start:server-devpasswordless.idprovider)