OpenId Implementation #3878
OpenId Implementation #3878
Conversation
actual-github-bot
bot
changed the title
|
@coderabbitai ignore |
✅ Deploy Preview for actualbudget ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
|
Note Reviews pausedUse the following commands to manage reviews:
WalkthroughThe recent changes encompass a range of enhancements and new features primarily focused on user authentication and access management within a React application. Key modifications include the introduction of an Additionally, several new modal components have been created to facilitate user management tasks, such as editing user access and transferring ownership. The application now supports OpenID authentication, with new configurations and handlers added to manage this feature. Error handling has been improved across various components, ensuring that users receive appropriate feedback during authentication processes. The overall structure of the application has been refined to enhance readability and maintainability, with a focus on integrating user access control throughout the application. Suggested labels
Suggested reviewers
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 (
|
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 |
lelemm
force-pushed
the
openid_squashed
branch
2 times, most recently
from
25fb62c to
53cc7c7
Compare
|
/update-vrt |
lelemm
force-pushed
the
openid_squashed
branch
3 times, most recently
from
1b55a16 to
a94aa4c
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
🛑 Comments failed to post (76)
packages/loot-core/src/types/models/openid.d.ts (1)
1-7: 🛠️ Refactor suggestion
Enhance type safety and documentation
The type definition could benefit from the following improvements:
- Add JSDoc documentation to explain the purpose and usage of each field
- Use more specific types for better type safety
- Consider making fields readonly to prevent accidental modifications
- Mark optional fields appropriately
Consider applying these improvements:
+/** + * Configuration for OpenID Connect authentication. + * @see https://openid.net/specs/openid-connect-core-1_0.html + */ export type OpenIdConfig = { + /** The selected OpenID provider (e.g., 'passwordless.id') */ - selectedProvider: string; + readonly selectedProvider: 'passwordless.id' | string; + + /** The OpenID Connect issuer URL */ - issuer: string; + readonly issuer: string; + + /** The client ID obtained from the OpenID provider */ - client_id: string; + readonly client_id: string; + + /** The client secret obtained from the OpenID provider */ - client_secret: string; + readonly client_secret: string; + + /** The hostname where the server is running */ - server_hostname: string; + readonly server_hostname: string; + + /** Additional optional configuration parameters */ + readonly additional_params?: Record<string, string>; };📝 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./** * Configuration for OpenID Connect authentication. * @see https://openid.net/specs/openid-connect-core-1_0.html */ export type OpenIdConfig = { /** The selected OpenID provider (e.g., 'passwordless.id') */ readonly selectedProvider: 'passwordless.id' | string; /** The OpenID Connect issuer URL */ readonly issuer: string; /** The client ID obtained from the OpenID provider */ readonly client_id: string; /** The client secret obtained from the OpenID provider */ readonly client_secret: string; /** The hostname where the server is running */ readonly server_hostname: string; /** Additional optional configuration parameters */ readonly additional_params?: Record<string, string>; };packages/desktop-client/src/components/manager/subscribe/OpenIdCallback.ts (2)
9-14:
⚠️ Potential issueAdd dependency array to prevent infinite loop
The useEffect hook is missing its dependency array, which will cause it to run on every render. This could lead to multiple unnecessary API calls.
Apply this fix:
- useEffect(() => { + useEffect(() => { const token = new URLSearchParams(window.location.search).get('token'); send('subscribe-set-token', { token: token as string }).then(() => { dispatch(loggedIn()); }); - }); + }, []); // Run only once on mount📝 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.useEffect(() => { const token = new URLSearchParams(window.location.search).get('token'); send('subscribe-set-token', { token: token as string }).then(() => { dispatch(loggedIn()); }); }, []); // Run only once on mount
10-13:
⚠️ Potential issueAdd error handling and token validation
The current implementation lacks error handling for the API call and proper token validation. This could lead to silent failures and security issues.
Consider implementing this improved version:
- const token = new URLSearchParams(window.location.search).get('token'); - send('subscribe-set-token', { token: token as string }).then(() => { - dispatch(loggedIn()); - }); + const token = new URLSearchParams(window.location.search).get('token'); + if (!token) { + console.error('No token provided in callback URL'); + // Consider redirecting to error page or login + return; + } + + send('subscribe-set-token', { token }) + .then(() => { + dispatch(loggedIn()); + }) + .catch(error => { + console.error('Failed to set token:', error); + // Handle error appropriately + });Committable suggestion skipped: line range outside the PR's diff.
packages/loot-core/src/types/models/user.ts (3)
1-6: 🛠️ Refactor suggestion
Strengthen type safety for the role field
The
rolefield should be constrained to valid role values using a union type of possible roles instead of a generic string.+type Role = typeof PossibleRoles[keyof typeof PossibleRoles]; export interface NewUserEntity { userName: string; displayName: string; - role: string; + role: Role; enabled: boolean; }Committable suggestion skipped: line range outside the PR's diff.
8-11:
⚠️ Potential issueEnsure consistency in owner property type
There's an inconsistency between
UserEntity.owner(boolean) andUserAvailable.owner(number). Consider standardizing the type across interfaces to prevent confusion and potential type conversion issues.Choose one of these approaches:
export interface UserEntity extends NewUserEntity { id: string; - owner: boolean; + owner: number; // If number is preferred }OR update UserAvailable to use boolean if that's more appropriate.
Committable suggestion skipped: line range outside the PR's diff.
19-25: 🛠️ Refactor suggestion
Improve type clarity for access control fields
The
haveAccessandownerfields using number type is not self-documenting:
- Consider using boolean if these are flags
- If numbers represent specific access levels, consider using an enum
- Add JSDoc comments explaining the meaning of these numeric values
+/** Represents a user's availability and access status */ export interface UserAvailable { userId: string; displayName?: string; userName: string; + /** Access level of the user. @example 0 = no access, 1 = read, 2 = write */ haveAccess?: number; + /** Owner level. @example 0 = not owner, 1 = owner */ owner?: number; }Committable suggestion skipped: line range outside the PR's diff.
packages/desktop-client/src/icons/v1/Role.tsx (1)
13-18: 🛠️ Refactor suggestion
Convert SVG attributes to React camelCase format.
React expects camelCase for all attributes, including SVG properties.
Apply this change:
- <g fill="none" stroke="currentColor" stroke-width="2" stroke-linecap="round" stroke-linejoin="round"> + <g fill="none" stroke="currentColor" strokeWidth={2} strokeLinecap="round" strokeLinejoin="round"> <circle cx="12" cy="8" r="4"/> <path d="M5.5 21a8.5 8.5 0 0 1 13 0"/> <path d="M12 1v2M12 21v2M4.22 4.22l1.42 1.42M18.36 18.36l1.42 1.42M1 12h2M21 12h2M4.22 19.78l1.42-1.42M18.36 5.64l1.42-1.42"/> <circle cx="12" cy="12" r="3"/> </g>Committable suggestion skipped: line range outside the PR's diff.
packages/desktop-client/src/components/admin/UserDirectory/UserDirectoryHeader.tsx (1)
9-12: 🛠️ Refactor suggestion
Add TypeScript interface and proper component typing.
The component lacks proper TypeScript typing which could lead to type-safety issues.
+interface UserDirectoryHeaderProps { + // Add any props if needed in the future +} -export function UserDirectoryHeader() { +export const UserDirectoryHeader: FC<UserDirectoryHeaderProps> = () => {Committable suggestion skipped: line range outside the PR's diff.
packages/desktop-client/src/auth/AuthProvider.tsx (1)
18-37: 🛠️ Refactor suggestion
Consider performance and type safety improvements
The implementation has several areas that could be enhanced:
- The
hasPermissionfunction is recreated on every render- The permission comparison could be more type-safe
- The user data access could benefit from type narrowing
export const AuthProvider = ({ children }: AuthProviderProps) => { const userData = useSelector((state: State) => state.user.data); + const hasPermission = React.useCallback((permission?: Permissions) => { - const hasPermission = (permission?: Permissions) => { if (!permission) { return true; } + const userPermission = userData?.permission; + if (!userPermission && !userData?.offline) { + return false; + } return ( (userData?.offline ?? false) || - userData?.permission?.toUpperCase() === permission?.toUpperCase() + userPermission?.toUpperCase() === permission.toUpperCase() ); - }; + }, [userData]); return ( <AuthContext.Provider value={{ hasPermission }}> {children} </AuthContext.Provider> ); };📝 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 const AuthProvider = ({ children }: AuthProviderProps) => { const userData = useSelector((state: State) => state.user.data); const hasPermission = React.useCallback((permission?: Permissions) => { if (!permission) { return true; } const userPermission = userData?.permission; if (!userPermission && !userData?.offline) { return false; } return ( (userData?.offline ?? false) || userPermission?.toUpperCase() === permission.toUpperCase() ); }, [userData]); return ( <AuthContext.Provider value={{ hasPermission }}> {children} </AuthContext.Provider> ); };packages/loot-core/src/server/admin/types/handlers.ts (1)
26-41: 🛠️ Refactor suggestion
Standardize error handling across access management methods
The access management methods have inconsistent error handling:
access-addincludes error handlingaccess-getdoesn't include error handlingaccess-delete-allhas the same type mismatch asuser-delete-all(string[] vs number[])Consider standardizing the return types:
'access-get': (fileId: string) => Promise<ApiResponse<UserAccessEntity[]>>; 'access-delete-all': ({ fileId, ids, }: { fileId: string; ids: string[]; }) => Promise<ApiResponse<{ someDeletionsFailed: boolean; ids?: string[] }>>;packages/desktop-client/src/components/modals/SimpleFinInitialiseModal.tsx (1)
43-54:
⚠️ Potential issueFix modal closure behavior on error
The modal closes even when an error occurs, which prevents users from seeing the error message and retrying.
if (error) { setIsValid(false); setError(getSecretsError(error, reason)); + setIsLoading(false); + return; } else { onSuccess(); + setIsLoading(false); + close(); } -setIsLoading(false); -close();Committable suggestion skipped: line range outside the PR's diff.
packages/desktop-client/src/components/modals/OpenIDEnableModal.tsx (1)
30-40: 🛠️ Refactor suggestion
Enhance error handling for async operations
The async operations could benefit from proper error handling to prevent silent failures and improve user experience.
Consider implementing this improved version:
async function onSave(config: OpenIdConfig) { + try { const { error } = (await send('enable-openid', { openId: config })) || {}; if (!error) { originalOnSave?.(); - await refreshLoginMethods(); - await asyncStorage.removeItem('user-token'); - await closeBudget(); + try { + await Promise.all([ + refreshLoginMethods(), + asyncStorage.removeItem('user-token'), + closeBudget() + ]); + } catch (cleanupError) { + console.error('Failed to cleanup after OpenID enable:', cleanupError); + setError('OpenID was enabled but cleanup failed. Please refresh the application.'); + } } else { setError(getOpenIdErrors(error)); } + } catch (e) { + setError('Failed to enable OpenID. Please try again.'); + console.error('OpenID enable failed:', e); + } }📝 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.async function onSave(config: OpenIdConfig) { try { const { error } = (await send('enable-openid', { openId: config })) || {}; if (!error) { originalOnSave?.(); try { await Promise.all([ refreshLoginMethods(), asyncStorage.removeItem('user-token'), closeBudget() ]); } catch (cleanupError) { console.error('Failed to cleanup after OpenID enable:', cleanupError); setError('OpenID was enabled but cleanup failed. Please refresh the application.'); } } else { setError(getOpenIdErrors(error)); } } catch (e) { setError('Failed to enable OpenID. Please try again.'); console.error('OpenID enable failed:', e); } }packages/desktop-client/src/components/settings/AuthSettings.tsx (3)
34-57:
⚠️ Potential issueImplement the onSave callback for OpenID enablement
The empty callback in the OpenID enable modal could lead to unexpected behavior.
- onSave: async () => {}, + onSave: async () => { + // TODO: Implement OpenID enablement logic + throw new Error('Not implemented'); + },Committable suggestion skipped: line range outside the PR's diff.
22-33: 🛠️ Refactor suggestion
Simplify feature flag handling
The current implementation has redundant feature flag checks and unnecessary style properties.
- <View style={{ display: openidAuthFeatureFlag ? 'visible' : 'none' }}> - {openidAuthFeatureFlag && ( + {openidAuthFeatureFlag && ( + <View> <Setting primaryAction={ // ... } > // ... </Setting> + </View> - )} - </View> + )}📝 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.return ( {openidAuthFeatureFlag && ( <View> <Setting primaryAction={ <> <label> OpenID is{' '} <label style={{ fontWeight: 'bold' }}> {loginMethod === 'openid' ? 'enabled' : 'disabled'} </label> </label>
58-81: 🛠️ Refactor suggestion
Add loading state to prevent multiple submissions
The buttons lack loading states, which could lead to multiple submissions.
+ const [isLoading, setIsLoading] = React.useState(false); // ... <Button style={{ marginTop: '10px', }} variant="normal" + disabled={isLoading} onPress={async () => { + setIsLoading(true); try { await dispatch( pushModal('enable-password-auth', { onSave: async () => { // Implementation here }, }), ); + } finally { + setIsLoading(false); + } }} > - Disable OpenID + {isLoading ? 'Disabling...' : 'Disable OpenID'} </Button>Committable suggestion skipped: line range outside the PR's diff.
packages/desktop-client/src/components/manager/subscribe/Bootstrap.tsx (1)
38-43: 🛠️ Refactor suggestion
Use translation system for OpenID error messages
The new OpenID error messages are hardcoded in English, while other messages use the translation system. For consistency and internationalization support, consider using the translation system.
- return 'OpenID server cannot be empty'; + return t('OpenID server cannot be empty'); - return 'Client ID cannot be empty'; + return t('Client ID cannot be empty'); - return 'Client secret cannot be empty'; + return t('Client secret cannot be empty');Committable suggestion skipped: line range outside the PR's diff.
packages/desktop-client/src/components/manager/subscribe/ConfirmPasswordForm.tsx (2)
87-87: 🛠️ Refactor suggestion
Consider handling errors consistently.
Unlike
ConfirmPasswordForm, this component doesn't handle errors through theonErrorprop. Consider adding error handling for consistency:-export function ConfirmOldPasswordForm({ buttons, onSetPassword }) { +export function ConfirmOldPasswordForm({ buttons, onSetPassword, onError }) { // ... async function onSubmit() { if (loading) { return; } setLoading(true); - await onSetPassword(password); + try { + await onSetPassword(password); + } catch (error) { + onError('password-error'); + } setLoading(false); }Also applies to: 92-100
117-118: 🛠️ Refactor suggestion
Use translation for the password placeholder.
The "Password" placeholder is hardcoded. Use the translation utility like in the original component:
- placeholder="Password" + placeholder={t('Password')}Committable suggestion skipped: line range outside the PR's diff.
packages/desktop-client/src/components/admin/UserDirectory/UserDirectoryRow.tsx (1)
130-136:
⚠️ Potential issueAdd null check for onEditUser callback
The Edit button's onPress handler doesn't check if onEditUser is defined, which could lead to runtime errors.
<Button style={{ margin: 4, fontSize: 14, color: theme.pageTextLink }} variant="bare" - onPress={() => onEditUser(user)} + onPress={() => onEditUser?.(user)} > Edit </Button>📝 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.<Button style={{ margin: 4, fontSize: 14, color: theme.pageTextLink }} variant="bare" onPress={() => onEditUser?.(user)} > Edit </Button>packages/desktop-client/src/components/modals/GoCardlessInitialiseModal.tsx (1)
48-71: 🛠️ Refactor suggestion
Consider optimizing the secret submission process.
The current implementation has several areas for improvement:
- Sequential API calls could be parallelized
- Potential race conditions if multiple submissions occur
- Missing cleanup for pending requests on component unmount
Consider this refactoring:
- let { error, reason } = - (await send('secret-set', { - name: 'gocardless_secretId', - value: secretId, - })) || {}; - - if (error) { - setIsLoading(false); - setIsValid(false); - setError(getSecretsError(error, reason)); - return; - } else { - ({ error, reason } = - (await send('secret-set', { - name: 'gocardless_secretKey', - value: secretKey, - })) || {}); - if (error) { - setIsLoading(false); - setIsValid(false); - setError(getSecretsError(error, reason)); - return; - } - } + try { + const results = await Promise.all([ + send('secret-set', { + name: 'gocardless_secretId', + value: secretId, + }), + send('secret-set', { + name: 'gocardless_secretKey', + value: secretKey, + }) + ]); + + const errorResult = results.find(result => result?.error); + if (errorResult) { + setIsValid(false); + setError(getSecretsError(errorResult.error, errorResult.reason)); + return; + } + + setIsValid(true); + onSuccess(); + close(); + } catch (e) { + setIsValid(false); + setError(t('errors.gocardless.unexpected_error')); + } finally { + setIsLoading(false); + }Also, add cleanup handling:
useEffect(() => { let mounted = true; // Update state only if component is mounted const safeSetState = (setter: Function) => { if (mounted) setter(); }; // Your existing state setters should use safeSetState return () => { mounted = false; }; }, []);packages/desktop-client/src/components/admin/UserAccess/UserAccessRow.tsx (2)
1-1:
⚠️ Potential issueRemove ts-strict-ignore comment.
The
// @ts-strict-ignorecomment bypasses TypeScript's strict type checking. This could hide potential type-related issues.Consider fixing the underlying TypeScript issues instead of ignoring them. Would you like assistance in identifying and resolving these issues?
30-57: 🛠️ Refactor suggestion
Improve type safety in handleAccessToggle.
The function has potential type safety issues:
- The
cloudFileIdtype assertion could fail at runtime- The error type from
sendfunction is not properly typedConsider these improvements:
const handleAccessToggle = async () => { const newValue = !marked; + if (!cloudFileId) { + actions.addNotification({ + type: 'error', + title: 'Missing File ID', + message: 'Cannot modify access without a valid file ID.', + sticky: true, + }); + return; + } if (newValue) { const { error } = await send('access-add', { - fileId: cloudFileId as string, + fileId: cloudFileId, userId: access.userId, }); if (error) { handleError(error); + return; } } else { const { someDeletionsFailed } = await send('access-delete-all', { - fileId: cloudFileId as string, + fileId: cloudFileId, ids: [access.userId], });Committable suggestion skipped: line range outside the PR's diff.
packages/desktop-client/src/components/modals/PasswordEnableModal.tsx (2)
65-133: 🛠️ Refactor suggestion
Improve code reuse and accessibility.
The UI implementation has several areas for improvement:
- Extract common button code to reduce duplication
- Add proper ARIA labels for accessibility
- Make warning messages more prominent for better UX
+ const CancelButton = () => ( + <Button + variant="bare" + style={{ fontSize: 15, marginRight: 10 }} + onPress={() => popModal()} + aria-label="Cancel password setup" + > + Cancel + </Button> + ); return ( <Modal - name="enable-password-auth" + name="enable-password-auth" + aria-labelledby="password-modal-title" > {({ state: { close } }) => ( <> <ModalHeader + id="password-modal-title" title="Revert to server password" rightContent={<ModalCloseButton onPress={close} />} /> // ... rest of the code - buttons={<Button variant="bare" style={{fontSize: 15, marginRight: 10}} onPress={() => popModal()}>Cancel</Button>} + buttons={<CancelButton />} // ... rest of the code </> )} </Modal> );Committable suggestion skipped: line range outside the PR's diff.
52-63: 🛠️ Refactor suggestion
Add loading state and improve error handling.
The password setting logic could benefit from several improvements:
- Add loading state to prevent multiple submissions
- Implement proper error handling for async operations
- Add user feedback during the process
+ const [isLoading, setIsLoading] = useState(false); async function onSetPassword(password: string) { + if (isLoading) return; setError(null); + setIsLoading(true); try { const { error } = (await send('enable-password', { password })) || {}; if (!error) { originalOnSave?.(); await refreshLoginMethods(); await asyncStorage.removeItem('user-token'); await closeBudget(); } else { setError(getErrorMessage(error)); } } catch (e) { + setError('An unexpected error occurred. Please try again.'); + } finally { + setIsLoading(false); } }Committable suggestion skipped: line range outside the PR's diff.
packages/loot-core/src/client/reducers/budgets.ts (2)
51-51:
⚠️ Potential issueInitialize
usersWithAccessfor consistencyWhen handling the unknown state,
usersWithAccessshould also be initialized to maintain consistency with other states and prevent potential type errors.Apply this change:
hasKey: true, owner: '', + usersWithAccess: [], };📝 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.hasKey: true, owner: '', usersWithAccess: [],
95-95:
⚠️ Potential issueInitialize
usersWithAccessfor broken stateWhen handling the broken state (unauthorized access),
usersWithAccessshould be initialized to an empty array for consistency and to explicitly indicate no access.Apply this change:
hasKey: true, owner: '', + usersWithAccess: [], };Committable suggestion skipped: line range outside the PR's diff.
packages/loot-core/src/server/post.ts (2)
131-177: 🛠️ Refactor suggestion
Same code duplication issue as the
delfunction.The
patchfunction also duplicates the common HTTP request logic. It should be refactored using the same approach suggested for thedelfunction.After refactoring both functions, consider these architectural improvements:
- Create a proper HTTP client class that encapsulates all HTTP methods
- Add request/response interceptors for common operations
- Consider using a well-tested HTTP client library like
axiosorkyinstead of maintaining custom implementationsExample of a class-based approach:
class HttpClient { private async request(method: string, url: string, data?: unknown, options: RequestOptions = {}) { const { headers = {}, timeout = null } = options; return makeRequest(method, url, data, headers, timeout); } async get(url: string, options?: RequestOptions) { return this.request('GET', url, undefined, options); } async post(url: string, data: unknown, options?: RequestOptions) { return this.request('POST', url, data, options); } async patch(url: string, data: unknown, options?: RequestOptions) { return this.request('PATCH', url, data, options); } async delete(url: string, data?: unknown, options?: RequestOptions) { return this.request('DELETE', url, data, options); } }
83-129: 🛠️ Refactor suggestion
Reduce code duplication by extracting common HTTP request logic.
The
delfunction duplicates most of its implementation from thepostfunction. Consider refactoring to extract the shared logic into a reusable utility function.Here's a suggested refactoring approach:
+ async function makeRequest(method: string, url: string, data: unknown, headers = {}, timeout: number | null = null) { + let text; + let res; + try { + const controller = new AbortController(); + const timeoutId = setTimeout(() => controller.abort(), timeout); + const signal = timeout ? controller.signal : null; + res = await fetch(url, { + method, + body: JSON.stringify(data), + signal, + headers: { + ...headers, + 'Content-Type': 'application/json', + }, + }); + clearTimeout(timeoutId); + text = await res.text(); + } catch (err) { + throw new PostError('network-failure'); + } + + throwIfNot200(res, text); + + try { + res = JSON.parse(text); + } catch (err) { + throw new PostError('parse-json', { meta: text }); + } + + if (res.status !== 'ok') { + console.log( + `API call failed: ${url}\nData: ${JSON.stringify(data, null, 2)}\nResponse: ${JSON.stringify(res, null, 2)}` + ); + throw new PostError(res.description || res.reason || 'unknown'); + } + + return res.data; + } - export async function del(url, data, headers = {}, timeout = null) { - // ... current implementation - } + export async function del(url: string, data: unknown, headers = {}, timeout: number | null = null) { + return makeRequest('DELETE', url, data, headers, timeout); + }This refactoring would:
- Eliminate code duplication
- Make the code more maintainable
- Add TypeScript types for better type safety
- Make it easier to add new HTTP methods in the future
Committable suggestion skipped: line range outside the PR's diff.
packages/loot-core/src/shared/errors.ts (3)
157-168: 🛠️ Refactor suggestion
Add i18next translations and improve error messages.
The function needs internationalization and could provide more helpful error messages:
Apply this diff to fix the issues:
export function getOpenIdErrors(reason: string) { switch (reason) { case 'unauthorized': - return 'You are not logged in.'; + return t('You are not logged in.'); case 'configuration-error': - return 'This configuration is not valid. Please check it again.'; + return t('The OpenID configuration is not valid. Please verify all required fields.'); case 'unable-to-change-file-config-enabled': - return 'Unable to enable OpenID. Please update the config.json file in this case.'; + return t('Unable to enable OpenID. Please ensure you have write permissions and the config.json file exists.'); default: - return `An internal error occurred, sorry! Visit https://actualbudget.org/contact/ for support. (ref: ${reason})`; + return t('An internal error occurred, sorry! Visit https://actualbudget.org/contact/ for support. (ref: {{reason}})', { reason }); } }Committable suggestion skipped: line range outside the PR's diff.
146-155: 🛠️ Refactor suggestion
Add i18next translations and improve error handling.
The function has several issues:
- Missing i18next translations
- The default case might expose sensitive information
- The error parameter is only used in the default case
Apply this diff to fix the issues:
-export function getSecretsError(error: string, reason: string) { +export function getSecretsError(reason: string) { switch (reason) { case 'unauthorized': - return 'You are not logged in.'; + return t('You are not logged in.'); case 'not-admin': - return 'You have to be admin to set secrets'; + return t('You must be an administrator to set secrets.'); default: - return error; + return t('An internal error occurred, sorry! Visit https://actualbudget.org/contact/ for support. (ref: {{reason}})', { reason }); } }📝 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 getSecretsError(reason: string) { switch (reason) { case 'unauthorized': return t('You are not logged in.'); case 'not-admin': return t('You must be an administrator to set secrets.'); default: return t('An internal error occurred, sorry! Visit https://actualbudget.org/contact/ for support. (ref: {{reason}})', { reason }); } }
127-144: 🛠️ Refactor suggestion
Ensure consistency with i18next translations and string formatting.
The function has several inconsistencies with the rest of the codebase:
- Missing i18next translations unlike other error functions
- Inconsistent string formatting (mixing template literals with plain strings)
- HTML entity in string that should be a plain apostrophe
- Grammar issue in error message
Apply this diff to fix the issues:
export function getUserAccessErrors(reason: string) { switch (reason) { case 'unauthorized': - return 'You are not logged in.'; + return t('You are not logged in.'); case 'token-expired': - return 'Login expired, please login again.'; + return t('Login expired, please login again.'); case 'user-cant-be-empty': - return 'Please select a user.'; + return t('Please select a user.'); case 'invalid-file-id': - return 'This file is invalid.'; + return t('This file is invalid.'); case 'file-denied': - return `You don't have permissions over this file.`; + return t('You don\'t have permissions over this file.'); case 'user-already-have-access': - return `User already have access.`; + return t('User already has access.'); default: - return `An internal error occurred, sorry! Visit https://actualbudget.org/contact/ for support. (ref: ${reason})`; + return t('An internal error occurred, sorry! Visit https://actualbudget.org/contact/ for support. (ref: {{reason}})', { reason }); } }Committable suggestion skipped: line range outside the PR's diff.
packages/desktop-client/src/components/modals/TransferOwnership.tsx (3)
150-160: 🛠️ Refactor suggestion
Add confirmation dialog and loading state to transfer button
The transfer button should show loading state and require confirmation for this irreversible action.
<Button isDisabled={availableUsers.length === 0} + isLoading={isLoading} onPress={async () => { + if (!window.confirm('Are you sure you want to transfer ownership? This action cannot be undone.')) { + return; + } await onSave(); close(); await dispatch(closeAndLoadBudget((currentFile as Budget).id)); }} > - Transfer ownership + {isLoading ? 'Transferring...' : 'Transfer ownership'} </Button>Committable suggestion skipped: line range outside the PR's diff.
43-56: 🛠️ Refactor suggestion
Add loading state and error handling for user fetching
The user fetching logic lacks loading state and error handling, which could lead to poor user experience if the API call fails.
Consider adding loading state and error handling:
+const [isLoading, setIsLoading] = useState(false); useEffect(() => { + setIsLoading(true); send('users-get').then((users: UserEntity[]) => setAvailableUsers( users .filter(f => currentFile?.owner !== f.id) .map(user => [ user.id, user.displayName ? `${user.displayName} (${user.userName})` : user.userName, ]), ), - ); + ) + .catch(err => { + setError('Failed to fetch users. Please try again.'); + console.error('Failed to fetch users:', err); + }) + .finally(() => setIsLoading(false)); }, [userData?.userId, currentFile?.owner]);Committable suggestion skipped: line range outside the PR's diff.
58-73: 🛠️ Refactor suggestion
Enhance save functionality with validation and loading state
The save functionality could benefit from proper validation and loading state management.
Consider these improvements:
async function onSave() { + if (!userId) { + setError('Please select a user to transfer ownership to.'); + return; + } + + setIsLoading(true); + setError(null); + if (cloudFileId) { const response = await send('transfer-ownership', { fileId: cloudFileId as string, newUserId: userId, - }); + }).finally(() => setIsLoading(false)); const { error } = response || {}; if (!error) { originalOnSave?.(); } else { - setSetError(getUserAccessErrors(error)); + setError(getUserAccessErrors(error)); } } else { - setSetError('Cloud file ID is missing.'); + setError('Cloud file ID is missing.'); + setIsLoading(false); } }Committable suggestion skipped: line range outside the PR's diff.
packages/desktop-client/src/components/manager/ManagementApp.tsx (1)
139-154:
⚠️ Potential issueFix inconsistent route paths for user directory.
There are two different paths used for the user directory:
/user-directoryfor authenticated users/userdirectoryfor non-authenticated usersThis inconsistency could lead to routing issues.
Apply this change to standardize the paths:
- path="/userdirectory" + path="/user-directory"Also applies to: 189-199
packages/desktop-client/src/components/common/Button2.tsx (1)
187-187: 🛠️ Refactor suggestion
Clean up props handling and improve type safety
The permission prop is unnecessarily passed to ReactAriaButton through restProps.
Consider this refactor to improve type safety and props handling:
-isDisabled={restProps.isDisabled || !hasPermission(permission)} -{...restProps} +isDisabled={isPermissionDisabled || restProps.isDisabled} +aria-label={isPermissionDisabled ? "Insufficient permissions" : restProps['aria-label']} +{...Object.fromEntries(Object.entries(restProps).filter(([key]) => key !== 'permission'))}Committable suggestion skipped: line range outside the PR's diff.
packages/desktop-client/src/components/App.tsx (1)
130-164:
⚠️ Potential issueFix duplicate effects and improve notification handling.
There are several issues with the current implementation:
- Two identical useEffect hooks are performing the same task
- The dependency array contains redundant items
- Multiple notifications could be shown for the same expiration
- Missing cleanup for notifications on unmount
Apply this fix to consolidate the hooks and improve the implementation:
-useEffect(() => { - if (userData?.tokenExpired) { - addNotification({ - type: 'error', - id: 'login-expired', - title: 'Login expired', - sticky: true, - message: 'Login expired, please login again.', - button: { - title: 'Go to login', - action: () => { - signOut(); - }, - }, - }); - } -}, [userData, userData?.tokenExpired]); - -useEffect(() => { +useEffect(() => { if (userData?.tokenExpired) { addNotification({ type: 'error', id: 'login-expired', title: 'Login expired', sticky: true, message: 'Login expired, please login again.', button: { title: 'Go to login', action: () => { signOut(); }, }, }); + + // Cleanup notification when component unmounts + return () => { + // Assuming there's a removeNotification action + removeNotification('login-expired'); + }; } -}, [userData, userData?.tokenExpired]); +}, [userData?.tokenExpired]);Committable suggestion skipped: line range outside the PR's diff.
packages/loot-core/src/client/state-types/modals.d.ts (2)
290-293:
⚠️ Potential issueAdd missing import for NewUserAccessEntity.
The
NewUserAccessEntitytype is used but not imported.Add the following import at the top of the file:
-import { type UserAccessEntity } from '../../types/models/userAccess'; +import { type NewUserAccessEntity, type UserAccessEntity } from '../../types/models/userAccess';📝 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.import { type NewUserAccessEntity, type UserAccessEntity } from '../../types/models/userAccess'; 'edit-access': { access: UserAccessEntity | NewUserAccessEntity; onSave: (rule: UserEntity) => void; };
286-293:
⚠️ Potential issueFix parameter naming in callback types.
The
onSavecallbacks useruleas the parameter name, but they handle user entities. This appears to be a copy-paste error.Apply this diff to fix the parameter names:
'edit-user': { user: UserEntity | NewUserEntity; - onSave: (rule: UserEntity) => void; + onSave: (user: UserEntity) => void; }; 'edit-access': { access: UserAccessEntity | NewUserAccessEntity; - onSave: (rule: UserEntity) => void; + onSave: (user: UserEntity) => void; };📝 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.'edit-user': { user: UserEntity | NewUserEntity; onSave: (user: UserEntity) => void; }; 'edit-access': { access: UserAccessEntity | NewUserAccessEntity; onSave: (user: UserEntity) => void; };packages/loot-core/src/client/shared-listeners.ts (2)
279-290: 🛠️ Refactor suggestion
Add i18n translations for notification messages
The notification messages should be internationalized using the
t()function, consistent with other notifications in this file.Apply this diff:
case 'token-expired': notif = { - title: 'Login expired', - message: 'Please login again.', + title: t('Login expired'), + message: t('Please login again.'), sticky: true, id: 'login-expired', button: { - title: 'Go to login', + title: t('Go to login'), action: () => actions.signOut(), }, }; break;Committable suggestion skipped: line range outside the PR's diff.
286-288: 💡 Codebase verification
Based on the findings, I can now provide the final response:
OpenID session cleanup is missing from signOut implementation
The current signOut implementation only handles basic cleanup:
- Removes user token and encryption keys
- Clears local storage items
- Dispatches Redux actions for state cleanup
However, it lacks specific cleanup for OpenID/OIDC sessions. This could lead to lingering sessions when users sign out.
packages/loot-core/src/server/main.ts: Add OpenID session cleanup in thesubscribe-sign-outhandlerpackages/loot-core/src/client/actions/user.ts: Consider adding OpenID cleanup in the signOut action🔗 Analysis chain
Verify signOut action implementation
Let's verify that the
signOutaction is properly implemented and handles the OpenID session cleanup.
Let me gather more information about the signOut implementation.
Let me try a different search pattern to find the signOut implementation.
Let me check the implementation of the
subscribe-sign-outhandler since it's called in the signOut action.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check the implementation of signOut action # Expected: Find the signOut implementation and verify it handles OpenID session cleanup # Look for signOut action implementation echo "Searching for signOut action implementation..." rg -A 10 "signOut.*=.*\(" # Look for OpenID related cleanup in the codebase echo "Searching for OpenID cleanup..." rg -l "openid|oidc"Length of output: 4155
Script:
#!/bin/bash # Search for the actual signOut implementation and any related auth cleanup echo "Searching for signOut implementation in actions..." ast-grep --pattern 'signOut = async $$$' echo "Searching for auth cleanup related code..." rg -A 5 "clearAuth|removeAuth|resetAuth" echo "Searching for OpenID related cleanup functions..." rg -A 5 "(clear|reset|remove).*[Ss]ession"Length of output: 3432
Script:
#!/bin/bash # Search for signOut action implementation with different patterns echo "Searching for signOut action definition..." rg -A 10 "export const signOut|const signOut|function signOut" echo "Searching for sign out related handlers..." rg -A 5 "handleSignOut|onSignOut" echo "Searching for auth related actions..." rg -A 5 "actions\.signOut|dispatch\(signOut"Length of output: 3702
Script:
#!/bin/bash # Search for subscribe-sign-out handler implementation echo "Searching for subscribe-sign-out handler..." rg -A 10 "subscribe-sign-out" # Search for session cleanup in server code echo "Searching for session cleanup in server..." rg -A 10 "clearSession|resetSession|destroySession"Length of output: 2330
packages/loot-core/src/server/cloud-storage.ts (1)
47-52:
⚠️ Potential issueFix token expiration handling logic and add error handling.
There are several issues with the token expiration handling:
- The
else if (res.status === 403)will never execute because it's after the initial status check- Unsafe JSON parsing without try-catch
- Browser-specific code in a server module
Apply this fix:
async function checkHTTPStatus(res) { if (res.status === 403) { const text = await res.text(); try { const data = JSON.parse(text)?.data; if (data?.reason === 'token-expired') { await asyncStorage.removeItem('user-token'); // Consider using a platform-agnostic way to handle redirection if (typeof window !== 'undefined') { window.location.href = '/'; } throw new HTTPError(res.status, 'Token expired'); } } catch (e) { throw new HTTPError(res.status, text); } } if (res.status !== 200) { return res.text().then(str => { throw new HTTPError(res.status, str); }); } return res; }Committable suggestion skipped: line range outside the PR's diff.
packages/desktop-client/src/components/transactions/TransactionsTable.test.jsx (1)
152-178: 🛠️ Refactor suggestion
Add test cases for authentication-related behavior.
While the AuthProvider has been correctly integrated into the component hierarchy, there are no test cases that verify:
- Permission-based access control for transaction operations
- Authentication state handling
- User role restrictions
Consider adding test cases that cover these scenarios to ensure the authentication integration works as expected.
Example test cases to add:
test('restricts transaction operations based on user permissions', async () => { // Mock different user permission levels const mockNoPermissions = { hasPermission: () => false }; const mockFullPermissions = { hasPermission: () => true }; // Test with no permissions const { container: noPermContainer } = renderTransactions({ authContext: mockNoPermissions }); // Verify restricted operations expect(screen.queryByTestId('add-button')).toBeNull(); expect(screen.queryByTestId('edit-button')).toBeNull(); // Test with full permissions const { container: fullPermContainer } = renderTransactions({ authContext: mockFullPermissions }); // Verify allowed operations expect(screen.getByTestId('add-button')).toBeInTheDocument(); expect(screen.getByTestId('edit-button')).toBeInTheDocument(); });packages/desktop-client/src/auth/ProtectedRoute.tsx (2)
26-31:
⚠️ Potential issueEnsure safe access to nested Redux state properties
Accessing
state.budgets.allFilesandstate.user.datawithout verifying the existence ofstate.budgetsandstate.usermight result in runtime errors if these properties areundefined.Implement checks or provide default values to safeguard against undefined properties:
- Adjust the selectors to include default fallback objects:
const allFiles = useSelector(state => state.budgets?.allFiles || []); const userData = useSelector(state => state.user?.data || null);- Alternatively, ensure that
state.budgetsandstate.userare always initialized in your Redux store.
27-30: 🛠️ Refactor suggestion
Use type predicates instead of type assertion with 'as'
Using type assertions with
ascan bypass TypeScript's type checking, potentially introducing bugs if the data doesn't match the expected types.Refactor the filter to use a type predicate, ensuring type safety:
const remoteFiles = allFiles.filter( (f): f is SyncedLocalFile | RemoteFile => f.state === 'remote' || f.state === 'synced' || f.state === 'detached', );This approach leverages TypeScript's type narrowing, providing compile-time type safety without the need for assertions.
packages/desktop-client/src/components/ServerContext.tsx (2)
106-117:
⚠️ Potential issueHandle errors in the
useEffectforsubscribe-needs-bootstrapThe
useEffecthook that callssubscribe-needs-bootstrapdoes not handle potential errors from thesendfunction. This omission could lead to unhandled promise rejections.Apply this diff to add error handling:
useEffect(() => { if (serverURL) { - send('subscribe-needs-bootstrap').then( + send('subscribe-needs-bootstrap') + .then( (data: Awaited<ReturnType<Handlers['subscribe-needs-bootstrap']>>) => { if ('hasServer' in data && data.hasServer) { setAvailableLoginMethods(data.loginMethods); setMultiuserEnabled(data.multiuser); } }, - ); + ) + .catch(error => { + // Handle error appropriately, e.g., set an error state or log the error + console.error('Failed to fetch bootstrap data:', error); + }); } }, [serverURL]);Proper error handling prevents application crashes and improves stability.
📝 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.useEffect(() => { if (serverURL) { send('subscribe-needs-bootstrap') .then( (data: Awaited<ReturnType<Handlers['subscribe-needs-bootstrap']>>) => { if ('hasServer' in data && data.hasServer) { setAvailableLoginMethods(data.loginMethods); setMultiuserEnabled(data.multiuser); } }, ) .catch(error => { // Handle error appropriately, e.g., set an error state or log the error console.error('Failed to fetch bootstrap data:', error); }); } }, [serverURL]);
99-104:
⚠️ Potential issueAdd error handling to
refreshLoginMethodsThe
refreshLoginMethodsfunction lacks error handling for the asynchronoussendoperation. If the request fails, it could lead to unhandled promise rejections and inconsistent state within the application.Apply this diff to include error handling:
const refreshLoginMethods = useCallback(async () => { if (serverURL) { - const data = await send('subscribe-get-login-methods'); - setAvailableLoginMethods(data.methods); + try { + const data = await send('subscribe-get-login-methods'); + setAvailableLoginMethods(data.methods); + } catch (error) { + // Handle error appropriately, e.g., set an error state or log the error + console.error('Failed to refresh login methods:', error); + } } }, [serverURL]);Implementing error handling ensures robustness and enhances user experience by managing failures gracefully.
📝 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.const refreshLoginMethods = useCallback(async () => { if (serverURL) { try { const data = await send('subscribe-get-login-methods'); setAvailableLoginMethods(data.methods); } catch (error) { // Handle error appropriately, e.g., set an error state or log the error console.error('Failed to refresh login methods:', error); } } }, [serverURL]);packages/loot-core/src/server/admin/app.ts (7)
1-1: 🛠️ Refactor suggestion
Avoid suppressing TypeScript strictness with
// @ts-strict-ignoreThe use of
// @ts-strict-ignoreat the top of the file disables TypeScript's strict type checking for the entire file. This can hide potential type errors and reduce code reliability. It's recommended to address any underlying type issues to ensure type safety.
233-241: 🛠️ Refactor suggestion
Handle possible errors in
owner-createdmethodThe method assumes the request will succeed and only checks if
resexists. Consider adding atry-catchblock to handle potential errors during thegetrequest.Apply this diff to add error handling:
-app.method('owner-created', async function () { - const res = await get(getServer().BASE_SERVER + '/admin/owner-created/'); +app.method('owner-created', async function () { + try { + const res = await get(getServer().BASE_SERVER + '/admin/owner-created/');And add a
catchblock to handle errors.Committable suggestion skipped: line range outside the PR's diff.
171-190:
⚠️ Potential issueEnsure proper encoding of URL parameters in
access-get-available-usersSimilar to previous methods, ensure that
fileIdis properly URL-encoded to prevent errors with special characters.Apply this diff to fix the issue:
-\`${getServer().BASE_SERVER + '/admin/access/users'}?fileId=\${fileId}\`, +\`${getServer().BASE_SERVER + '/admin/access/users'}?fileId=\${encodeURIComponent(fileId)}\`,📝 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.app.method('access-get-available-users', async function (fileId) { const userToken = await asyncStorage.getItem('user-token'); if (userToken) { const res = await get( `${getServer().BASE_SERVER + '/admin/access/users'}?fileId=${encodeURIComponent(fileId)}`, { headers: { 'X-ACTUAL-TOKEN': userToken, }, }, ); if (res) { return JSON.parse(res) as UserAvailable[]; } } return []; });
216-227:
⚠️ Potential issueEnsure proper parsing of JSON response
Verify that the response
resis a JSON string before parsing withJSON.parse. Ifresis already an object, parsing it again will cause an error.Apply this diff if necessary:
-if (res) { - return JSON.parse(res) as UserEntity; +if (res && typeof res === 'string') { + return JSON.parse(res) as UserEntity;📝 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.const res = await get( `${getServer().BASE_SERVER + '/admin/file/owner'}?fileId=${fileId}`, { headers: { 'X-ACTUAL-TOKEN': userToken, }, }, ); if (res && typeof res === 'string') { return JSON.parse(res) as UserEntity; }
109-128:
⚠️ Potential issuePotential issue with URL parameter encoding in
access-getIn the
access-getmethod,fileIdis appended directly to the URL without encoding. IffileIdcontains special characters, this may lead to an invalid request. UseencodeURIComponent(fileId)to safely encode the parameter.Apply this diff to fix the issue:
-`${getServer().BASE_SERVER + '/admin/access/'}?fileId=${fileId}`, +`${getServer().BASE_SERVER + '/admin/access/'}?fileId=${encodeURIComponent(fileId)}`,📝 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.app.method('access-get', async function (fileId) { const userToken = await asyncStorage.getItem('user-token'); if (userToken) { const res = await get( `${getServer().BASE_SERVER + '/admin/access/'}?fileId=${encodeURIComponent(fileId)}`, { headers: { 'X-ACTUAL-TOKEN': userToken, }, }, ); if (res) { return JSON.parse(res) as UserAccessEntity[]; } } return []; });
45-46:
⚠️ Potential issueAvoid parsing JSON if response is already an object
If
resis already parsed as an object, there's no need to parse it again withJSON.parse(res). Ensure that the response fromgetis a string before parsing.Apply this diff if
resis already an object:-if (res) { - const list = JSON.parse(res) as UserEntity[]; +if (res && typeof res === 'string') { + const list = JSON.parse(res) as UserEntity[];Committable suggestion skipped: line range outside the PR's diff.
148-169: 💡 Codebase verification
⚠️ Potential issueBased on the findings from the shell scripts, particularly the implementation of the
delfunction inserver/post.ts, I can now provide the final response:Both
fileIdencoding andidshandling need fixes inaccess-delete-allThe
delfunction implementation shows that request data should be sent in the request body as JSON, not as request options. The current implementation has two issues:
fileIdneeds URL encodingidsis correctly sent in the request body (thedelfunction stringifies the data object)Apply these changes:
- URL encode the
fileIdparameter:-getServer().BASE_SERVER + `/admin/access?fileId=${fileId}`, +getServer().BASE_SERVER + `/admin/access?fileId=${encodeURIComponent(fileId)}`,
- No changes needed for
idshandling as it's correctly sent in the request body, which matches thedelfunction implementation that usesJSON.stringify(data)and sends it in the request body.🔗 Analysis chain
Inconsistent query parameter handling in
access-delete-allThe
delrequest inaccess-delete-allconstructs the URL with a query parameterfileId=${fileId}. Ensure thatfileIdis properly encoded usingencodeURIComponent. Additionally, verify that the server expectsidsin the body or as query parameters; currently,idsare being sent in the request options which may not be correct.Apply this diff to properly encode
fileId:-getServer().BASE_SERVER + \`/admin/access?fileId=\${fileId}\`, +getServer().BASE_SERVER + \`/admin/access?fileId=\${encodeURIComponent(fileId)}\`,And verify the handling of
idsin the request.Run the following script to check if
idsshould be sent in the request body:
Let's examine how the server handles DELETE requests and access-related endpoints to verify the correct way to send the
idsparameter.
Let's examine how the HTTP client is implemented to understand the correct way to pass parameters in DELETE requests.
Let's examine the HTTP client implementation to understand how request parameters are handled.
Let's examine how DELETE requests are implemented and how parameters are handled in the HTTP client.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check how 'ids' should be sent in DELETE requests in the server API. # Expected: 'ids' should be in the request body or query parameters. # Since actual request can't be made, check the API documentation or type definitions. rg -A 5 'deleteAllAccess' packages/loot-core/src/server/admin/types/handlers.tsLength of output: 79
Script:
#!/bin/bash # Let's check the server-side implementation to understand how 'ids' are handled rg -A 10 "app.(delete|del)" packages/loot-core/src/server/ # Also check for any route handlers or API endpoints related to access deletion rg -A 10 "router\.(delete|del).*access" packages/loot-core/src/server/ # Look for any type definitions or interfaces related to access deletion rg -A 5 "interface.*Access" packages/loot-core/src/server/Length of output: 592
Script:
#!/bin/bash # Let's examine the implementation of the 'del' function to understand how it handles parameters rg -A 10 "export (function|const) del" packages/loot-core/src/ # Also check for any HTTP client configuration or utility functions rg -A 10 "function (delete|del).*Request" packages/loot-core/src/Length of output: 5678
Script:
#!/bin/bash # Let's check for HTTP client implementation or network request utilities rg -A 10 "function (request|fetch)" packages/loot-core/src/ # Also look for any DELETE endpoint implementations in the server ast-grep --pattern 'app.method("access-delete",$$$)'Length of output: 2857
Script:
#!/bin/bash # Let's check for the implementation of the 'del' function used in the HTTP client rg -A 10 "export (async )?function del\(" packages/loot-core/src/ rg -A 10 "const del = " packages/loot-core/src/ # Also check for any HTTP utility functions that handle request parameters rg -A 10 "(makeRequest|makeURL|buildURL)" packages/loot-core/src/Length of output: 1176
packages/desktop-client/src/components/LoggedInUser.tsx (1)
61-71:
⚠️ Potential issueAdd missing dependencies to the useEffect hook
The
useEffecthook at lines 61-71 depends oncurrentFileanduserData, but only includes[cloudFileId]in the dependency array. To ensure the effect runs whenever any of its dependencies change, please addcurrentFileanduserDatato the dependency array.Apply this diff to fix the issue:
useEffect(() => { if (cloudFileId && currentFile) { setIsOwner( currentFile.usersWithAccess.some( u => u.userId === userData?.userId && u.owner, ), ); } else { setIsOwner(false); } - }, [cloudFileId]); + }, [cloudFileId, currentFile, userData]);📝 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.useEffect(() => { if (cloudFileId && currentFile) { setIsOwner( currentFile.usersWithAccess.some( u => u.userId === userData?.userId && u.owner, ), ); } else { setIsOwner(false); } }, [cloudFileId, currentFile, userData]);packages/desktop-client/src/components/manager/subscribe/Login.tsx (3)
126-130:
⚠️ Potential issueAvoid Using
<label>Without Associated Form ControlThe
<label>element is typically used with form controls and should have a correspondinghtmlForattribute. Using it as a general text container can lead to semantic and accessibility issues.Consider replacing
<label>with a<Text>component for proper semantics:-<label style={{ color: theme.warningText, marginTop: 10 }}> +<Text style={{ color: theme.warningText, marginTop: 10 }}> The first user to login with OpenID will be the{' '} <Text style={{ fontWeight: 'bold' }}>server owner</Text>. This can't be changed using UI. -</label> +</Text>📝 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.<Text style={{ color: theme.warningText, marginTop: 10 }}> The first user to login with OpenID will be the{' '} <Text style={{ fontWeight: 'bold' }}>server owner</Text>. This can't be changed using UI. </Text>
94-97:
⚠️ Potential issueValidate
redirect_urlBefore RedirectingUsing
window.location.href = redirect_urlwithout validating the URL could lead to open redirect vulnerabilities.[security_issue]
Ensure that
redirect_urlis a trusted URL before redirecting:if (error) { setError(error); } else { - window.location.href = redirect_url; + if (redirect_url.startsWith(window.location.origin)) { + window.location.href = redirect_url; + } else { + setError('Invalid redirect URL'); + } }Committable suggestion skipped: line range outside the PR's diff.
90-91:
⚠️ Potential issueAdd Error Handling for Asynchronous Calls
The
send('owner-created')call does not handle potential rejections, which could result in unhandled promise rejections and unexpected behavior.Include a
.catchblock to handle any errors:useEffect(() => { - send('owner-created').then(created => setWarnMasterCreation(!created)); + send('owner-created') + .then(created => setWarnMasterCreation(!created)) + .catch(error => { + setError('Failed to check owner creation status'); + }); }, []);Committable suggestion skipped: line range outside the PR's diff.
packages/desktop-client/src/components/admin/UserAccess/UserAccess.tsx (3)
105-113:
⚠️ Potential issueHandle potential null or undefined
usersWithAccessIn the
loadOwnerfunction, whenfile?.usersWithAccessisundefinedornull, callingfilteron it could result in a runtime error.Apply this diff to safely access
usersWithAccess:- const owner = file?.usersWithAccess.filter(user => user.owner); - if (owner.length > 0) { + const owner = file?.usersWithAccess?.filter(user => user.owner) || []; + if (owner?.length > 0) {This ensures that
owneris always an array and prevents errors whenusersWithAccessis not available.📝 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.const file: Awaited<ReturnType<Handlers['get-user-file-info']>> = (await send('get-user-file-info', cloudFileId as string)) ?? {}; const owner = file?.usersWithAccess?.filter(user => user.owner) || []; if (owner?.length > 0) { return owner[0]; } return null;
78-101:
⚠️ Potential issueEnsure
setLoadingis defined before callingThe
setLoadingfunction is used withinloadAccessandloadData, but it's defined as an optional prop inUserAccessContent. IfsetLoadingis not provided, calling it may result in an error.Apply this diff to provide a default no-operation function for
setLoading:function UserAccessContent({ isModal, - setLoading, + setLoading = () => {}, }: ManageUserAccessContentProps) {This ensures that
setLoadingis always defined and can be safely called without checking forundefined.Committable suggestion skipped: line range outside the PR's diff.
104-104:
⚠️ Potential issueRemove unintended
debuggerstatementThe
debugger;statement at line 104 seems to have been left in unintentionally. It can cause the application to pause execution when running in a development environment.Apply this diff to remove the
debuggerstatement:- debugger;📝 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.🧰 Tools
🪛 Biome (1.9.4)
[error] 104-104: This is an unexpected use of the debugger statement.
Unsafe fix: Remove debugger statement
(lint/suspicious/noDebugger)
packages/loot-core/src/types/server-handlers.d.ts (3)
273-288: 🛠️ Refactor suggestion
Consider using discriminated unions for 'subscribe-needs-bootstrap' return type
The method
'subscribe-needs-bootstrap'returns a Promise that can resolve to different types with varying structures. Using discriminated unions with a common discriminant property can improve type safety and simplify error handling.You can define discriminated types like:
type NeedsBootstrapError = { error: string }; type NeedsBootstrapNoServer = { bootstrapped: boolean; hasServer: false; }; type NeedsBootstrapWithServer = { bootstrapped: boolean; hasServer: true; loginMethods: LoginMethod[]; multiuser: boolean; }; type NeedsBootstrapResponse = | NeedsBootstrapError | NeedsBootstrapNoServer | NeedsBootstrapWithServer;Then update the method signature:
- 'subscribe-needs-bootstrap': (args: { url }) => Promise< - | { error: string } - | { bootstrapped: boolean; hasServer: false; } - | { bootstrapped: boolean; hasServer: true; loginMethods: { method: string; displayName: string; active: boolean; }[]; multiuser: boolean; } - >; + 'subscribe-needs-bootstrap': (args: { url: string }) => Promise<NeedsBootstrapResponse>;This approach enhances type safety and makes error handling more consistent.
401-403:
⚠️ Potential issueEnsure 'openId' parameter is mandatory if OpenID is enabled
In the
'enable-openid'method, theopenIdparameter is optional (openId?: OpenIdConfig;). If enabling OpenID authentication, theopenIdconfiguration should be required to avoid misconfiguration.Apply this diff to make the
openIdparameter required:'enable-openid': (arg: { - openId?: OpenIdConfig; + openId: OpenIdConfig; }) => Promise<{ error?: string }>;This ensures that when OpenID is enabled, the necessary configuration is 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.'enable-openid': (arg: { openId: OpenIdConfig; }) => Promise<{ error?: string }>;
315-325:
⚠️ Potential issueAdd missing type annotations for parameters in 'subscribe-sign-in'
The method
'subscribe-sign-in'has parameters without explicit type annotations. This can lead to type safety issues and reduce code clarity.Apply the following diff to specify the parameter types:
'subscribe-sign-in': ( arg: | { - password; + password: string; loginMethod?: string; } | { - return_url; + return_url: string; - loginMethod?: 'openid'; + loginMethod: 'openid'; }, ) => Promise<{ error?: string }>;By adding explicit types, you enhance type safety and prevent potential runtime errors.
📝 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.'subscribe-sign-in': ( arg: | { password: string; loginMethod?: string; } | { return_url: string; loginMethod: 'openid'; }, ) => Promise<{ error?: string }>;packages/desktop-client/src/components/admin/UserDirectory/UserDirectory.tsx (1)
276-283:
⚠️ Potential issueFix style spreading in
EmptyMessagecomponent.Include the
styleprop correctly by spreading it inside the style object to ensure styles are applied properly.Apply this diff to fix the issue:
style={{ textAlign: 'center', color: theme.pageTextSubdued, fontStyle: 'italic', fontSize: 13, marginTop: 5, - style, + ...style, }}📝 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.style={{ textAlign: 'center', color: theme.pageTextSubdued, fontStyle: 'italic', fontSize: 13, marginTop: 5, ...style, }}packages/desktop-client/src/components/modals/EditUser.tsx (1)
70-85: 🛠️ Refactor suggestion
Handle unexpected errors in the
saveUserfunction.Currently, the
saveUserfunction handles known error reasons, but unexpected errors might not provide meaningful feedback. Consider adding a default error handler.Apply this diff:
} + } else { + setError('An unexpected error occurred. Please try again.'); + return false; }Committable suggestion skipped: line range outside the PR's diff.
packages/desktop-client/src/components/manager/subscribe/OpenIdForm.tsx (3)
91-96: 🛠️ Refactor suggestion
Inconsistent Variable Naming: Rename 'newclientSecret' to 'newClientSecret'
The variable
newclientSecretdoes not follow the camelCase naming convention used elsewhere in the code (e.g.,newIssuer,newClientId). For consistency and readability, please rename it tonewClientSecret.Proposed change:
const newClientSecret = typeof provider.clientSecret === 'function' ? provider.clientSecret(location, serverUrl ?? '') : provider.clientSecret; setClientSecret(newClientSecret ?? '');📝 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.const newClientSecret = typeof provider.clientSecret === 'function' ? provider.clientSecret(location, serverUrl ?? '') : provider.clientSecret; setClientSecret(newClientSecret ?? '');
63-73:
⚠️ Potential issueHandle Errors When Fetching OpenID Configuration
The
send('get-openid-config')call may fail, and currently, there's no error handling for rejected promises. Add error handling to manage potential exceptions and improve the resilience of the component.Proposed change:
useEffect(() => { if (loadData) { send('get-openid-config') .then((config: { openId?: OpenIdConfig }) => { setProviderName(config?.openId?.selectedProvider ?? 'other'); setIssuer(config?.openId?.issuer ?? ''); setClientId(config?.openId?.client_id ?? ''); setClientSecret(config?.openId?.client_secret ?? ''); }) + .catch((error) => { + // Handle error, e.g., log it or display an error message + console.error('Failed to fetch OpenID configuration:', error); + // Optionally, set default values or inform the user + }); } }, [loadData]);Committable suggestion skipped: line range outside the PR's diff.
109-123:
⚠️ Potential issueAdd Error Handling in 'onSubmit' Function
Currently, any errors thrown during
onSetOpenIdare not handled, which may result in unhandled promise rejections and a poor user experience if the submission fails silently. Wrap theonSetOpenIdcall in a try-catch block to handle errors appropriately and provide feedback to the user.Proposed change:
async function onSubmit() { if (loading) { return; } setLoading(true); + try { await onSetOpenId({ selectedProvider: providerName, issuer: issuer ?? '', client_id: clientId ?? '', client_secret: clientSecret ?? '', server_hostname: serverUrl ?? '', }); + } catch (error) { + // Handle error, e.g., display an error message to the user + console.error('Failed to set OpenID configuration:', error); + // Optionally, set an error state and display feedback to the user + } finally { setLoading(false); } }Committable suggestion skipped: line range outside the PR's diff.
packages/desktop-client/src/components/modals/CreateAccountModal.tsx (1)
246-297:
⚠️ Potential issueAvoid reusing 'triggerRef' for multiple popovers
The
triggerRefis being used for both the GoCardless and SimpleFIN popovers. This can lead to unexpected behavior because both popovers share the same trigger reference. Each popover should have its owntriggerRefto function correctly.Apply this diff to define separate refs for each popover:
// Add new refs for each popover + const goCardlessTriggerRef = useRef(null); + const simpleFinTriggerRef = useRef(null); // Remove the original triggerRef if not used elsewhere - const triggerRef = useRef(null); // Update GoCardless button and popover <Button - ref={triggerRef} + ref={goCardlessTriggerRef} variant="bare" onPress={() => setGoCardlessMenuOpen(true)} aria-label="GoCardless menu" > <Popover - triggerRef={triggerRef} + triggerRef={goCardlessTriggerRef} isOpen={menuGoCardlessOpen} onOpenChange={() => setGoCardlessMenuOpen(false)} > // Update SimpleFIN button and popover <Button - ref={triggerRef} + ref={simpleFinTriggerRef} variant="bare" onPress={() => setSimplefinMenuOpen(true)} aria-label="SimpleFIN menu" > <Popover - triggerRef={triggerRef} + triggerRef={simpleFinTriggerRef} isOpen={menuSimplefinOpen} onOpenChange={() => setSimplefinMenuOpen(false)} >Also applies to: 339-373
packages/desktop-client/src/components/manager/BudgetList.tsx (1)
481-487: 🛠️ Refactor suggestion
Redundant try-catch block in
fetchUsersfunctionThe
try...catchblock in thefetchUsersfunction may be unnecessary sincesetCurrentUserIdis unlikely to throw an error. Consider removing it to simplify the code.Apply this diff to remove the redundant try-catch block:
- try { setCurrentUserId(userData?.userId ?? ''); - } catch (error) { - console.error('Failed to fetch users:', error); - }📝 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.const fetchUsers = useCallback(async () => { setCurrentUserId(userData?.userId ?? ''); }, [userData?.userId]);packages/loot-core/src/server/main.ts (5)
1802-1805:
⚠️ Potential issueAwait asynchronous operation in
get-user-file-infoIn
handlers['get-user-file-info'], thecloudStorage.getRemoteFile(fileId)function may return a Promise. Ensure that the function awaits this Promise to handle asynchronous execution correctly.handlers['get-user-file-info'] = async function (fileId: string) { - return cloudStorage.getRemoteFile(fileId); + return await cloudStorage.getRemoteFile(fileId); };📝 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.handlers['get-user-file-info'] = async function (fileId: string) { return await cloudStorage.getRemoteFile(fileId); };
2080-2087: 🛠️ Refactor suggestion
Add error handling when parsing OpenID configuration
In
handlers['get-openid-config'], callingJSON.parse(res)without error handling may lead to unhandled exceptions if the response is not valid JSON. Wrap the parsing logic in a try-catch block:const res = await get(getServer().BASE_SERVER + '/openid/config'); - if (res) { - return JSON.parse(res) as OpenIdConfig; - } - return {}; + if (res) { + try { + return JSON.parse(res) as OpenIdConfig; + } catch (err) { + console.error('Error parsing OpenID configuration:', err); + return { error: 'parse-failure' }; + } + } else { + return {}; + }📝 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.const res = await get(getServer().BASE_SERVER + '/openid/config'); if (res) { try { return JSON.parse(res) as OpenIdConfig; } catch (err) { console.error('Error parsing OpenID configuration:', err); return { error: 'parse-failure' }; } } else { return {}; } };
1611-1621: 🛠️ Refactor suggestion
Handle potential exceptions when parsing JSON responses
In
handlers['subscribe-get-user'], the response from the server is parsed usingJSON.parse(res)without a try-catch block. If the response is not valid JSON, this will result in an unhandled exception, causing the function to fail unexpectedly.Wrap the JSON parsing in a try-catch block to handle any parsing errors gracefully:
- const { - status, - reason, - data: { - userName = null, - permission = '', - userId = null, - displayName = null, - loginMethod = null, - } = {}, - } = JSON.parse(res) || {}; + let parsedRes; + try { + parsedRes = JSON.parse(res) || {}; + } catch (e) { + console.error('Error parsing response:', e); + return { offline: true }; + } + const { + status, + reason, + data: { + userName = null, + permission = '', + userId = null, + displayName = null, + loginMethod = null, + } = {}, + } = parsedRes;📝 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.let parsedRes; try { parsedRes = JSON.parse(res) || {}; } catch (e) { console.error('Error parsing response:', e); return { offline: true }; } const { status, reason, data: { userName = null, permission = '', userId = null, displayName = null, loginMethod = null, } = {}, } = parsedRes;
1577-1579: 🛠️ Refactor suggestion
Ensure proper error handling for fetch requests
In
handlers['subscribe-get-login-methods'], thefetchcall does not check for HTTP errors before parsing the response withres.json(). This may lead to unhandled exceptions or unexpected behavior if the server returns an error status code.Consider checking the response status before parsing. Here's a suggested refactor:
- res = await fetch(getServer().SIGNUP_SERVER + '/login-methods').then(res => - res.json(), - ); + const response = await fetch(getServer().SIGNUP_SERVER + '/login-methods'); + if (!response.ok) { + return { error: response.statusText || 'network-failure' }; + } + res = await response.json();📝 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.const response = await fetch(getServer().SIGNUP_SERVER + '/login-methods'); if (!response.ok) { return { error: response.statusText || 'network-failure' }; } res = await response.json();
1704-1706: 🛠️ Refactor suggestion
Validate
tokenbefore setting it in storageIn
handlers['subscribe-set-token'], there is no validation on thetokenparameter. To prevent potential issues, add a check to ensuretokenis a non-empty string before storing it.handlers['subscribe-set-token'] = async function ({ token }) { + if (typeof token !== 'string' || token.trim() === '') { + return { error: 'invalid-token' }; + } await asyncStorage.setItem('user-token', token); };📝 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.handlers['subscribe-set-token'] = async function ({ token }) { if (typeof token !== 'string' || token.trim() === '') { return { error: 'invalid-token' }; } await asyncStorage.setItem('user-token', token); };
lelemm
force-pushed
the
openid_squashed
branch
from
a94aa4c to
5a65c68
Compare
|
@coderabbitai resume |
There was a problem hiding this comment.
Actionable comments posted: 57
🧹 Outside diff range and nitpick comments (76)
packages/desktop-client/src/auth/AuthProvider.tsx (2)
39-45: Consider enhancing error handling in useAuth hook.The error message could be more descriptive to help developers understand where to add the AuthProvider in the component tree.
Consider this improvement:
if (context === undefined) { - throw new Error('useAuth must be used within an AuthProvider'); + throw new Error( + 'useAuth hook must be used within an AuthProvider component. ' + + 'Ensure AuthProvider exists in the component tree above the current component.' + ); }
1-45: Consider adding tests and documentation for the auth system.As this is a critical authentication component, consider:
- Adding comprehensive unit tests covering different permission scenarios
- Including JSDoc documentation explaining the permission system
- Creating a README.md file documenting the authentication architecture
Would you like me to help generate:
- A test suite for the AuthProvider and useAuth hook?
- JSDoc documentation for the components and types?
- A README.md template for the auth system?
packages/desktop-client/src/auth/ProtectedRoute.tsx (4)
12-16: Add JSDoc documentation for the props type.Consider adding JSDoc documentation to improve code maintainability and developer experience.
+/** + * Props for the ProtectedRoute component + * @property {Permissions} permission - Required permission to access the route + * @property {ReactElement} element - Component to render if permission is granted + * @property {boolean} [validateOwner] - Whether to validate file ownership + */ type ProtectedRouteProps = { permission: Permissions; element: ReactElement; validateOwner?: boolean; };
27-29: Consider extracting file filtering logic to a custom hook.The file filtering logic could be reused across components. Consider creating a custom hook like
useRemoteFiles().function useRemoteFiles() { const allFiles = useSelector(state => state.budgets.allFiles || []); return allFiles.filter( f => f.state === 'remote' || f.state === 'synced' || f.state === 'detached' ) as (SyncedLocalFile | RemoteFile)[]; }
33-51: Optimize permission checking logic.The current implementation could be simplified and optimized:
- Flatten the nested conditions
- Memoize the permission check result
+ const hasRequiredPermission = useMemo( + () => hasPermission(permission), + [hasPermission, permission] + ); + + const hasOwnerAccess = useMemo( + () => validateOwner && currentFile?.usersWithAccess.some(u => u.userId === userData?.userId), + [validateOwner, currentFile, userData?.userId] + ); + useEffect(() => { - const hasRequiredPermission = hasPermission(permission); - setPermissionGranted(hasRequiredPermission); - - if (!hasRequiredPermission && validateOwner) { - if (currentFile) { - setPermissionGranted( - currentFile.usersWithAccess.some(u => u.userId === userData?.userId), - ); - } - } + setPermissionGranted(hasRequiredPermission || hasOwnerAccess); }, [ - cloudFileId, - permission, - validateOwner, - hasPermission, - currentFile, - userData, + hasRequiredPermission, + hasOwnerAccess ]);
53-64: Enhance the error message display.Consider the following improvements:
- Use a design token for the margin
- Move styles to a styled component
- Provide more informative error message
+ const ErrorContainer = styled(View)` + margin: ${tokens.spacing.xxlarge}; + text-align: center; + `; + return permissionGranted ? ( element ) : ( - <View - style={{ - margin: '50px', - }} - > - <h3>You don't have permission to view this page</h3> - </View> + <ErrorContainer> + <h3>Access Denied</h3> + <p>You don't have the required permissions to access this page.</p> + <p>Required permission: {permission}</p> + </ErrorContainer> );packages/desktop-client/src/components/modals/OpenIDEnableModal.tsx (4)
10-10: Consider renaming the Error component importThe imported
Errorcomponent shadows the globalErrorobject. While this works, it could lead to confusion.Consider renaming the import:
-import { Error } from '../alerts'; +import { Error as AlertError } from '../alerts';🧰 Tools
🪛 Biome (1.9.4)
[error] 10-10: Do not shadow the global "Error" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
18-24: Add JSDoc documentation for better maintainabilityThe component and its props would benefit from detailed documentation explaining their purpose and usage.
Consider adding documentation:
+/** + * Modal component for enabling OpenID authentication. + * Displays a configuration form and handles the enabling process. + */ type OpenIDEnableModalProps = { + /** Callback function called after successful OpenID configuration */ onSave?: () => void; };
65-86: Improve accessibility and style managementThe warning labels could be more accessible, and styles could be better organized.
- Add aria-label attributes to improve accessibility
- Extract common styles to reduce duplication:
+const warningLabelStyles = { + ...styles.verySmallText, + color: theme.pageTextLight, + paddingTop: 5, +}; +const errorLabelStyles = { + ...styles.verySmallText, + color: theme.warningText, +}; <Label - style={{ - ...styles.verySmallText, - color: theme.pageTextLight, - paddingTop: 5, - }} + style={warningLabelStyles} + aria-label="Session closure warning" title="After enabling openid all sessions will be closed" />
54-63: Standardize modal closing mechanismThe cancel button uses
actions.popModalwhile the close button uses the modal'sclosefunction. Consider standardizing this for consistency.otherButtons={[ <Button key="cancel" variant="bare" style={{ marginRight: 10 }} - onPress={actions.popModal} + onPress={close} > Cancel </Button>, ]}packages/desktop-client/src/components/settings/AuthSettings.tsx (4)
38-40: Extract inline styles to theme or styled componentsInline styles should be moved to theme constants or styled components for better maintainability and consistency.
Consider creating styled components:
const StyledButton = styled(Button)` margin-top: 10px; `;Then use it consistently:
-<Button - style={{ - marginTop: '10px', - }} +<StyledButtonAlso applies to: 61-63
28-33: Simplify nested labelsThe nested label structure is unnecessary and can be simplified.
-<label> - OpenID is{' '} - <label style={{ fontWeight: 'bold' }}> - {loginMethod === 'openid' ? 'enabled' : 'disabled'} - </label> -</label> +<Text> + OpenID is{' '} + <strong> + {loginMethod === 'openid' ? 'enabled' : 'disabled'} + </strong> +</Text>
37-37: Add consistent button IDs for testingOnly the "Start using OpenID" button has an ID. For consistency and testing purposes, add IDs to all buttons.
<Button + id="disable-openid" style={{ marginTop: '10px', }} variant="normal" onPress={() =>Also applies to: 60-74
16-93: Consider architectural improvements for better maintainabilityA few architectural suggestions:
- Consider splitting the enable/disable functionality into separate components for better maintainability
- Add ARIA labels and roles for better accessibility
- Consider implementing a proper state machine for auth method transitions
- Add error boundaries to handle potential failures in the authentication flow
packages/desktop-client/src/components/manager/subscribe/common.tsx (3)
Line range hint
1-1: Consider removing ts-strict-ignore.The
@ts-strict-ignoredirective bypasses TypeScript's strict type checking, which could hide potential type-related issues. Consider addressing the underlying type issues instead of ignoring them.
82-85: Improve type safety for server response handling.The
hasServercheck uses type narrowing throughinoperator, but the type system could be more explicit about the shape of the response.-if ('hasServer' in result && result.hasServer) { +interface ServerResponse { + hasServer: boolean; + multiuser: boolean; + loginMethods: string[]; +} + +if (isServerResponse(result) && result.hasServer) { setMultiuserEnabled(result.multiuser); setLoginMethods(result.loginMethods); } + +function isServerResponse(result: unknown): result is ServerResponse { + return ( + typeof result === 'object' && + result !== null && + 'hasServer' in result && + 'multiuser' in result && + 'loginMethods' in result + ); +}
Line range hint
96-114: Consider using styled-components for better style management.The Title component uses inline styles which could be better managed using styled-components, especially since you're already using a theme system.
+import styled from 'styled-components'; + +const StyledTitle = styled.h1` + font-size: 40px; + font-weight: 700; + color: ${props => props.theme.pageTextPositive}; + margin-bottom: 20px; +`; + export function Title({ text }: TitleProps) { - return ( - <h1 - style={{ - fontSize: 40, - fontWeight: 700, - color: theme.pageTextPositive, - marginBottom: 20, - }} - > - {text} - </h1> - ); + return <StyledTitle>{text}</StyledTitle>; }packages/desktop-client/src/components/manager/subscribe/Bootstrap.tsx (1)
38-43: Consider using translation system for new error messagesWhile the new OpenID error cases are good additions, their messages should use the translation system (t function) for consistency with other error messages in the component.
- return 'OpenID server cannot be empty'; + return t('OpenID server cannot be empty'); - return 'Client ID cannot be empty'; + return t('Client ID cannot be empty'); - return 'Client secret cannot be empty'; + return t('Client secret cannot be empty');packages/desktop-client/src/components/manager/subscribe/ConfirmPasswordForm.tsx (2)
1-2: Consider removing ts-strict-ignore.The addition of proper TypeScript types like
ChangeEventis a step in the right direction. Consider removing the// @ts-strict-ignoredirective and fixing any remaining type issues to improve type safety across the component.
106-149: Consider extracting common functionality into shared components.Both password forms share similar UI structure and behavior. Consider refactoring to reduce code duplication:
Extract common styled components:
- Password input container
- Controls container
- Show password toggle
Create a reusable password input component with consistent props:
interface PasswordInputProps { value: string; onChange: (value: string) => void; placeholder: string; showPassword: boolean; onEnter: () => void; autoFocus?: boolean; } function PasswordInput({ value, onChange, placeholder, showPassword, onEnter, autoFocus }: PasswordInputProps) { return ( <BigInput autoFocus={autoFocus} placeholder={placeholder} type={showPassword ? 'text' : 'password'} value={value} onChange={(e: ChangeEvent<HTMLInputElement>) => onChange(e.target.value)} onEnter={onEnter} /> ); }
- Create a shared password form layout component:
interface PasswordFormLayoutProps { children: React.ReactNode; showPasswordToggle: boolean; onShowPasswordChange: (show: boolean) => void; buttons: React.ReactNode; onSubmit: () => void; isLoading: boolean; }packages/desktop-client/src/components/admin/UserDirectory/UserDirectoryRow.tsx (2)
1-1: Consider enabling TypeScript strict modeThe
@ts-strict-ignoredirective disables strict type checking. Consider addressing the underlying type issues to enable strict mode for better type safety.
41-43: Simplify callback checks with optional chainingThe callback checks can be simplified using optional chaining for better readability.
- onMouseEnter={() => onHover && onHover(user.id)} - onMouseLeave={() => onHover && onHover(null)} + onMouseEnter={() => onHover?.(user.id)} + onMouseLeave={() => onHover?.(null)}🧰 Tools
🪛 Biome (1.9.4)
[error] 41-41: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 42-42: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
packages/desktop-client/src/components/modals/GoCardlessInitialiseModal.tsx (3)
42-42: Consider using generic error messages for security.While the current error message is user-friendly, it might reveal too much about the required credentials. Consider using a more generic message like "Invalid credentials" to avoid potential security implications.
- setError('It is required to provide both the secret id and secret key.'); + setError('Please ensure all required credentials are provided.');
130-130: Enhance error message accessibility.Consider adding aria attributes to improve accessibility for screen readers.
- {!isValid && <Error>{error}</Error>} + {!isValid && <Error role="alert" aria-live="polite">{error}</Error>}
Line range hint
1-146: Consider additional security measures for financial data handling.As this component handles sensitive financial credentials, consider implementing:
- Rate limiting for failed attempts
- Ensure all API calls are made over HTTPS
- Add logging for security audit trails (without logging sensitive data)
- Consider implementing session timeout for added security
Would you like assistance in implementing any of these security measures?
packages/desktop-client/src/components/ServerContext.tsx (3)
13-17: Consider using string literal types for the method fieldThe
LoginMethodstype could be more specific about allowed method values.Consider updating the type definition:
type LoginMethods = { - method: string; + method: 'password' | 'openid' | string; displayName: string; active: boolean; };
54-62: Optimize active login method lookupThe current implementation filters the entire array to find the first active method. Consider using
findinstead offilterfor better performance.export const useLoginMethod = () => { const availableLoginMethods = useContext(ServerContext).availableLoginMethods; if (!availableLoginMethods || availableLoginMethods.length === 0) { return 'password'; } - return availableLoginMethods.filter(m => m.active)[0]?.method ?? 'password'; + return availableLoginMethods.find(m => m.active)?.method ?? 'password'; };
48-52: Consider decoupling multiuser support from OpenIDThe current implementation tightly couples multiuser support with the OpenID login method. This might limit future extensibility if other authentication methods need to support multiple users.
Consider making multiuser support independent of the authentication method or documenting the architectural decision if this coupling is intentional.
packages/desktop-client/src/components/admin/UserAccess/UserAccessRow.tsx (4)
24-26: Consider extracting state initialization logic.The state initialization combines two conditions which could be made more explicit for better readability.
Consider this refactoring:
- const [marked, setMarked] = useState( - access.owner === 1 || access.haveAccess === 1, - ); + const hasAccess = access.owner === 1 || access.haveAccess === 1; + const [marked, setMarked] = useState(hasAccess);
30-57: Replace magic numbers with constants.The comparison with
1is used multiple times throughout the code. Consider using named constants for better maintainability and clarity.Consider this refactoring:
+const ACCESS_GRANTED = 1; +const ACCESS_DENIED = 0; - access.owner === 1 || access.haveAccess === 1, + access.owner === ACCESS_GRANTED || access.haveAccess === ACCESS_GRANTED,
94-95: Use optional chaining for event handlers.The onHover callback checks could be simplified using optional chaining.
Apply this improvement:
- onMouseEnter={() => onHover && onHover(access.userId)} - onMouseLeave={() => onHover && onHover(null)} + onMouseEnter={() => onHover?.(access.userId)} + onMouseLeave={() => onHover?.(null)}🧰 Tools
🪛 Biome (1.9.4)
[error] 94-94: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 95-95: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
143-143: Consider a more descriptive component display name.The display name 'UserRow' could be more specific to match the component's actual name for better debugging.
Consider this change:
-UserAccessRow.displayName = 'UserRow'; +UserAccessRow.displayName = 'UserAccessRow';packages/loot-core/src/server/post.ts (1)
83-177: Add integration tests for OpenID-related HTTP operationsGiven that these new HTTP methods (
delandpatch) are part of the OpenID implementation, it's crucial to ensure they work correctly with your OpenID provider.Consider adding integration tests that:
- Mock OpenID provider responses
- Test error scenarios (network failures, timeouts)
- Verify correct handling of OpenID-specific headers and responses
- Include examples in the documentation of how these methods should be used with OpenID
Would you like me to help create a test suite for these HTTP methods with OpenID-specific test cases?
packages/loot-core/src/shared/errors.ts (1)
Line range hint
1-1: Improve type safety.Consider the following TypeScript improvements:
- Remove the
@ts-strict-ignoredirective and fix any type issues.- Add return type annotations to the new functions.
Apply this diff to improve type safety:
-// @ts-strict-ignore import { t } from 'i18next'; -export function getUserAccessErrors(reason: string) { +export function getUserAccessErrors(reason: string): string { -export function getSecretsError(error: string, reason: string) { +export function getSecretsError(error: string, reason: string): string { -export function getOpenIdErrors(reason: string) { +export function getOpenIdErrors(reason: string): string {Also applies to: 127-168
packages/desktop-client/src/components/modals/TransferOwnership.tsx (3)
33-33: Fix typo in error state setter name.The setter name has a duplicate "Set" which should be corrected.
-const [error, setSetError] = useState<string | null>(null); +const [error, setError] = useState<string | null>(null);
37-39: Consider using type predicate for better type safety.The current type assertion could be replaced with a type predicate function for better type safety.
+function isRemoteOrSyncedFile(f: any): f is (SyncedLocalFile | RemoteFile) { + return f.state === 'remote' || f.state === 'synced' || f.state === 'detached'; +} -const remoteFiles = allFiles.filter( - f => f.state === 'remote' || f.state === 'synced' || f.state === 'detached', -) as (SyncedLocalFile | RemoteFile)[]; +const remoteFiles = allFiles.filter(isRemoteOrSyncedFile);
58-73: Improve type safety in ownership transfer logic.The function could benefit from stronger type checking and more specific error handling.
async function onSave() { if (cloudFileId) { const response = await send('transfer-ownership', { - fileId: cloudFileId as string, + fileId: cloudFileId, newUserId: userId, }); - const { error } = response || {}; + const { error } = response ?? {}; if (!error) { originalOnSave?.(); } else { - setSetError(getUserAccessErrors(error)); + setError(getUserAccessErrors(error)); } } else { - setSetError('Cloud file ID is missing.'); + setError('Cloud file ID is missing. Please ensure the budget is synced.'); } }packages/desktop-client/src/components/common/Button2.tsx (1)
144-152: Consider enhancing permission-based disable feedbackThe permission check implementation is functional but could benefit from these improvements:
- Consider providing visual feedback (e.g., tooltip) when a button is disabled due to permissions
- The permission check could be memoized if the permission value is stable
Here's a suggested implementation:
const Button = forwardRef<HTMLButtonElement, ButtonProps>( (props, ref) => { const { permission, children, variant = 'normal', bounce = true, + title, ...restProps } = props; const { hasPermission } = useAuth(); + const hasRequiredPermission = useMemo( + () => hasPermission(permission), + [hasPermission, permission] + ); + + const disabledReason = !hasRequiredPermission + ? "You don't have permission to perform this action" + : title; return ( <ReactAriaButton ref={ref} - isDisabled={restProps.isDisabled || !hasPermission(permission)} + isDisabled={restProps.isDisabled || !hasRequiredPermission} + title={disabledReason} {...restProps}Also applies to: 187-187
packages/desktop-client/src/components/LoggedInUser.tsx (3)
47-53: Consider performance optimizations for state managementThe file lookup and owner status calculations could benefit from memoization to prevent unnecessary re-renders.
Consider using useMemo:
- const currentFile = remoteFiles.find(f => f.cloudFileId === cloudFileId); + const currentFile = useMemo( + () => remoteFiles.find(f => f.cloudFileId === cloudFileId), + [remoteFiles, cloudFileId] + );
154-205: Simplify menu items generation logicThe getMenuItems function has complex nested conditions that could be simplified using array methods.
Consider restructuring like this:
const getMenuItems = (): (MenuItem | typeof Menu.line)[] => { const isAdmin = hasPermission(Permissions.ADMINISTRATOR); - const baseMenu: (MenuItem | typeof Menu.line)[] = []; - if (serverUrl && !userData?.offline && userData?.loginMethod === 'password') { - baseMenu.push({ name: 'change-password', text: t('Change password') }); - } + const baseMenu = [ + ...(serverUrl && !userData?.offline && userData?.loginMethod === 'password' + ? [{ name: 'change-password', text: t('Change password') }] + : []), + ...(serverUrl ? [{ name: 'sign-out', text: t('Sign out') }] : []), + { + name: 'config-server', + text: serverUrl ? t('Change server URL') : t('Start using a server'), + }, + ]; // Similar changes for adminMenu
218-222: Enhance accessibility for user displayThe user display section could benefit from better accessibility markup.
Consider these improvements:
- <small> - (logged as: <span>{userData?.displayName}</span>) - </small> + <small aria-label={`Logged in as ${userData?.displayName}`}> + (logged as: <span className="user-name">{userData?.displayName}</span>) + </small>packages/desktop-client/src/components/App.tsx (2)
54-55: Consider adding type safety improvements.While the implementation is correct, consider adding explicit typing for the destructured actions:
- const { signOut, addNotification } = useActions(); + const { signOut, addNotification }: { + signOut: () => void; + addNotification: (params: NotificationParams) => void; + } = useActions();
130-164: Consider architectural improvements for session management.To enhance maintainability and reusability, consider:
- Moving the session expiration logic to a custom hook (e.g.,
useSessionExpiration)- Internationalizing the notification messages using the existing
useTranslationhookExample implementation:
// hooks/useSessionExpiration.ts export function useSessionExpiration() { const { t } = useTranslation(); const { signOut, addNotification } = useActions(); const userData = useSelector((state: State) => state.user.data); useEffect(() => { if (userData?.tokenExpired) { addNotification({ type: 'error', id: 'login-expired', title: t('Login expired'), sticky: true, message: t('Login expired, please login again.'), button: { title: t('Go to login'), action: signOut, }, }); } }, [userData?.tokenExpired, addNotification, signOut, t]); }Then in App.tsx:
function AppInner() { useSessionExpiration(); // ... rest of the component }packages/desktop-client/src/components/manager/subscribe/Login.tsx (5)
Line range hint
1-1: Consider removing @ts-strict-ignore.The presence of @ts-strict-ignore suggests potential type-safety issues. Consider addressing the TypeScript errors properly instead of bypassing them to ensure better type safety and maintainability.
124-139: Improve server owner warning visibility.The warning about server ownership is crucial but might be easily missed. Consider:
- Making it more prominent visually
- Adding a confirmation dialog
- Persisting this information
164-190: Add TypeScript interface for HeaderLogin props.The component lacks proper TypeScript typing for its props. Consider adding an interface:
interface HeaderLoginProps { error: string | null; } function HeaderLogin({ error }: HeaderLoginProps) { // ... existing implementation }
Line range hint
223-237: Enhance error message handling.Consider:
- Adding more specific error messages
- Implementing error codes for better error tracking
- Adding logging for internal errors
function getErrorMessage(error) { switch (error) { // ... existing cases case 'internal-error': - return 'Internal error'; + const errorId = Date.now(); + console.error(`Internal error (ID: ${errorId}):`, error); + return `Internal error occurred. Reference ID: ${errorId}`; default: return `An unknown error occurred: ${error}`; } }
196-201: Consider memoizing login method state.The login method state could benefit from memoization to prevent unnecessary rerenders:
- const defaultLoginMethod = useLoginMethod(); - const [method, setMethod] = useState(defaultLoginMethod); + const defaultLoginMethod = useLoginMethod(); + const [method, setMethod] = useState(defaultLoginMethod); + const loginMethods = useAvailableLoginMethods(); + + const memoizedLoginMethods = useMemo(() => + loginMethods.map(m => [m.method, m.displayName]), + [loginMethods] + );packages/desktop-client/src/components/FinancesApp.tsx (1)
Line range hint
1-343: Overall implementation is solid with room for improvement.The OpenID implementation is well-integrated into the existing routing structure. The code maintains a clear separation of concerns and properly handles permissions. However, consider implementing:
- Comprehensive loading states
- Error boundaries for protected routes
- Documentation for new props and features
- Tests for the new routes and permission checks
Would you like help implementing any of these suggestions or creating test cases for the new functionality?
packages/desktop-client/src/components/admin/UserAccess/UserAccess.tsx (2)
124-126: Enhance error handling with user feedback.Errors are currently only logged to the console. Consider adding user-facing error messages to improve the user experience.
} catch (error) { console.error('Error loading user access data:', error); + dispatch( + pushModal('error', { + title: 'Error Loading Users', + message: 'Failed to load user access data. Please try again.', + }) + ); } finally {
293-318: Consider memoizing UserAccessList component.Since this component renders a list that could potentially be large, consider using React.memo to prevent unnecessary re-renders.
-function UserAccessList({ +const UserAccessList = React.memo(function UserAccessList({ accesses, hoveredAccess, onHover, -}: UsersAccessListProps) { +}: UsersAccessListProps) { if (accesses.length === 0) { return null; } return ( <View> {accesses.map(access => { const hovered = hoveredAccess === access.userId; return ( <UserAccessRow key={access.userId} access={access} hovered={hovered} onHover={onHover} /> ); })} </View> ); -} +});packages/loot-core/src/types/server-handlers.d.ts (5)
273-294: Consider adding JSDoc comments for better documentationThe type definitions are well-structured, but adding JSDoc comments would improve developer experience by documenting:
- The purpose of each login method field
- The meaning of the
activeflag- Possible error scenarios
Example documentation:
/** * Checks if the server needs bootstrap configuration * @param {Object} args - The arguments object * @param {string} args.url - The server URL to check * @returns {Promise<Object>} Server bootstrap status and available login methods */ 'subscribe-needs-bootstrap': (args: { url }) => Promise<...>; /** * Retrieves available login methods from the server * @returns {Promise<Object>} List of configured login methods and their status */ 'subscribe-get-login-methods': () => Promise<...>;
296-309: Consider using an enum for the permission fieldWhile the types are well-structured, the
permissionfield using a string type might be too permissive. Consider using an enum to restrict possible values.Example implementation:
enum UserPermission { ADMIN = 'admin', USER = 'user', READONLY = 'readonly' // Add other permission levels as needed } // Then update the type: permission?: UserPermission;
315-325: Consider improving type safety with literal typesThe type definition is good, but could be enhanced with literal types for better type safety.
Example implementation:
type LoginMethod = 'password' | 'openid'; 'subscribe-sign-in': ( arg: | { password: string; loginMethod?: Extract<LoginMethod, 'password'>; } | { return_url: string; loginMethod: Extract<LoginMethod, 'openid'>; }, ) => Promise<{ error?: string }>;
329-329: Consider adding token format validationThe method looks good, but consider adding a type to validate the token format.
Example implementation:
type AuthToken = string & { readonly __brand: unique symbol }; 'subscribe-set-token': (arg: { token: AuthToken }) => Promise<void>;
401-409: Consider grouping related authentication methodsThe OpenID configuration methods are well-defined but could benefit from being grouped together with other authentication-related methods for better organization.
Consider moving these methods next to other authentication-related methods (around line 315) and adding a comment to denote the authentication section:
// Authentication methods 'subscribe-sign-in': ... 'subscribe-sign-out': ... 'enable-openid': ... 'enable-password': ... 'get-openid-config': ...packages/desktop-client/src/components/admin/UserDirectory/UserDirectory.tsx (3)
93-101: Optimize user data loadingThe
loadUsersfunction is called in multiple places and could benefit from caching or debouncing to prevent unnecessary API calls.Consider using React Query or a similar library to handle data fetching and caching:
import { useQuery, useMutation } from 'react-query'; const { data: loadedUsers, isLoading } = useQuery('users', async () => { const users = await send('users-get'); return users ?? []; });
120-150: Improve error handling in delete operationThe error handling in
onDeleteSelectedcould be simplified and made more consistent:
- The error handling logic is duplicated
- The notification structure is repeated
Consider extracting the notification logic:
const showErrorNotification = (error: string, options = {}) => { const isExpiredToken = error === 'token-expired'; actions.addNotification({ type: 'error', title: isExpiredToken ? 'Login expired' : 'Operation failed', message: getUserDirectoryErrors(error), sticky: true, ...(isExpiredToken && { button: { title: 'Go to login', action: () => actions.signOut(), }, }), ...options, }); };
302-308: Fix type definition in UsersListPropsThere's a typo in the
onEditUserprop type.- onEditUser?: (rule: UserEntity) => void; + onEditUser?: (user: UserEntity) => void;packages/loot-core/src/server/cloud-storage.ts (3)
25-30: LGTM! Consider adding JSDoc comments.The new interfaces for user access management are well-structured. The separation between owner and users with access is clear and appropriate for multi-user scenarios.
Consider adding JSDoc comments to document the purpose of these interfaces and their properties:
+/** + * Represents a user who has access to a file + */ export interface UsersWithAccess { + /** Unique identifier of the user */ userId: string; + /** Username for authentication */ userName: string; + /** Display name shown in the UI */ displayName: string; + /** Whether this user is an owner */ owner: boolean; }Also applies to: 38-39
392-422: Consider enhancing error handling and resilienceThe implementation is functional but could benefit from improved error handling and resilience.
Consider these improvements:
export async function getRemoteFile( fileId: string, ): Promise<RemoteFile | null> { const userToken = await asyncStorage.getItem('user-token'); if (!userToken) { return null; } let res; try { res = await fetchJSON(getServer().SYNC_SERVER + '/get-user-file-info', { headers: { 'X-ACTUAL-TOKEN': userToken, 'X-ACTUAL-FILE-ID': fileId, }, + // Add retry logic for transient failures + retry: { + retries: 3, + factor: 2, + minTimeout: 1000, + } }); } catch (e) { - console.log('Unexpected error fetching file from server', e); + // Log more specific error information + console.error('Failed to fetch file info', { + fileId, + error: e instanceof Error ? e.message : String(e), + }); return null; } if (res.status === 'error') { - console.log('Error fetching file from server', res); + console.error('Server returned error for file info', { + fileId, + error: res.error, + }); return null; } + // Validate required fields + if (!res.data?.fileId || !res.data?.name) { + console.error('Invalid file info received', { fileId }); + return null; + } return { ...res.data, hasKey: encryption.hasKey(res.data.encryptKeyId), }; }
Line range hint
1-423: Consider implementing request cancellation and sensitive data handlingThe file handles sensitive operations but could benefit from additional security measures.
- Implement request cancellation using AbortController to prevent memory leaks and handle component unmounting:
export interface RequestOptions { signal?: AbortSignal; } export async function getRemoteFile( fileId: string, options?: RequestOptions ): Promise<RemoteFile | null> { // ... existing code ... try { res = await fetchJSON(getServer().SYNC_SERVER + '/get-user-file-info', { headers: { /* ... */ }, signal: options?.signal, }); } // ... rest of the code ... }
- Consider implementing a cleanup mechanism for sensitive data:
- Clear encryption keys from memory after use
- Implement automatic token rotation
- Add rate limiting for API requests
packages/desktop-client/src/components/manager/subscribe/OpenIdForm.tsx (1)
387-416: Enhance accessibility and type safetyConsider the following improvements:
- Add aria-label for better screen reader support
- Add explicit typing for the Select component options
<Select + aria-label="Select OpenID Provider" options={openIdProviders.map(provider => provider === Menu.line ? Menu.line : [provider.value, provider.label], )} defaultLabel="Select Provider" value={defaultValue} onChange={handleProviderChange} />Also, consider creating a dedicated type for the Select options to improve type safety:
type SelectOption = [string, string] | typeof Menu.line;packages/desktop-client/src/components/modals/CreateAccountModal.tsx (2)
238-385: Consider refactoring duplicated bank connection UI code.The GoCardless and SimpleFIN sections share very similar structure and logic. Consider extracting this into a reusable component to improve maintainability.
Example refactor:
type BankConnectionProps = { type: 'gocardless' | 'simplefin'; isSetupComplete: boolean; canSetSecrets: boolean; onConnect: () => void; onReset: () => void; isLoading?: boolean; region: string; // 'European' | 'North American' }; function BankConnectionSection({ type, isSetupComplete, canSetSecrets, onConnect, onReset, isLoading, region }: BankConnectionProps) { const [menuOpen, setMenuOpen] = useState(false); const menuRef = useRef(null); // ... implement shared UI logic }
386-398: Enhance the permission warning message.The warning message could be more specific about the required permissions and provide clearer guidance.
Consider this improvement:
- You don't have the required permissions to set up secrets. Please contact an Admin to configure + You need administrator permissions to configure secrets. Please contact your system administrator to set up {[ isGoCardlessSetupComplete ? '' : 'GoCardless', isSimpleFinSetupComplete ? '' : 'SimpleFin', ] .filter(Boolean) .join(' or ')} - . + for bank synchronization.packages/desktop-client/src/components/manager/BudgetList.tsx (5)
481-487: Simplify fetchUsers and improve error handlingThe
fetchUserscallback can be simplified, and the error handling should be more robust.Consider this improvement:
- const fetchUsers = useCallback(async () => { - try { - setCurrentUserId(userData?.userId ?? ''); - } catch (error) { - console.error('Failed to fetch users:', error); - } - }, [userData?.userId]); + const fetchUsers = useCallback(() => { + if (!userData) { + console.warn('User data is not available'); + return; + } + setCurrentUserId(userData.userId ?? ''); + }, [userData]);The function doesn't need to be async since it's not performing any asynchronous operations.
641-642: Simplify user access filteringThe filtering logic can be simplified and made more explicit.
- let usersAccess = currentFile?.usersWithAccess ?? []; - usersAccess = usersAccess?.filter(user => user.userName !== '') ?? []; + const usersAccess = currentFile?.usersWithAccess?.filter(user => user.userName) ?? [];
644-650: Extract sorting logic to a separate functionThe sorting logic should be extracted for better maintainability and reusability.
const getUserDisplayName = (user: UserAccess, currentUserId: string): string => user.userId === currentUserId ? 'You' : (user.displayName ?? user.userName); const sortUsersByDisplayName = (users: UserAccess[], currentUserId: string): UserAccess[] => [...users].sort((a, b) => getUserDisplayName(a, currentUserId).localeCompare(getUserDisplayName(b, currentUserId)) );Then use it like:
- const sortedUsersAccess = [...usersAccess].sort((a, b) => { - const textA = - a.userId === currentUserId ? 'You' : (a.displayName ?? a.userName); - const textB = - b.userId === currentUserId ? 'You' : (b.displayName ?? b.userName); - return textA.localeCompare(textB); - }); + const sortedUsersAccess = sortUsersByDisplayName(usersAccess, currentUserId);
665-711: Extract tooltip styles to a separate constantThe nested styles in the Tooltip content make the component harder to read and maintain.
Consider extracting the styles:
const tooltipStyles = { container: { margin: 5, }, header: { ...styles.altMenuHeaderText, ...styles.verySmallText, color: theme.pageTextLight, }, userList: { padding: 0, }, userItem: { flexDirection: 'row' as const, }, userIcon: { width: 10, height: 10, opacity: 0.7, marginTop: 3, marginRight: 5, }, userName: { ...styles.verySmallText, color: theme.pageTextLight, margin: 0, listStylePosition: 'inside', }, };This will make the component more readable and easier to maintain.
Line range hint
1-727: Consider splitting presentational and container componentsThe file mixes presentational and container components, which could make it harder to maintain and test. Consider splitting the components into:
- Presentational components (FileItem, FileState)
- Container components (BudgetList, UserAccessForFile)
This separation would:
- Improve testability
- Make the components more reusable
- Make the code easier to maintain
packages/desktop-client/src/components/Modals.tsx (2)
612-620: Consider documenting the distinction between 'load-backup' and 'manager-load-backup'.There are two cases using
LoadBackupModalwith different prop configurations. Consider adding a comment to explain the different use cases and when each should be used.+ // Used in the file manager context where backups are disabled and updates don't need to be watched case 'manager-load-backup': return ( <LoadBackupModal
Line range hint
89-653: Consider refactoring the modal mapping logic for better maintainability.The switch statement is growing quite large. Consider breaking it down into smaller, more manageable pieces. Some options:
- Group related modal cases into separate functions
- Use a modal registry pattern
- Create a modal configuration map
This would improve maintainability and make the code easier to test.
Example approach using a modal registry:
// modalRegistry.ts const modalRegistry = { 'edit-access': (options) => ( <EditUserAccess key="edit-access" defaultUserAccess={options.access} onSave={options.onSave} /> ), // ... other modal mappings }; // Modals.tsx return modalStack.map(({ name, options }) => { const createModal = modalRegistry[name]; if (!createModal) { throw new Error('Unknown modal'); } return createModal(options); });packages/loot-core/src/server/main.ts (1)
1610-1641: Enhanced user validation response with additional fields.Good addition of new fields for user validation:
- tokenExpired flag
- userName
- permission
- userId
- displayName
- loginMethod
Consider adding JSDoc comments to document the purpose of these fields.
+/** + * Response from user validation containing: + * @property {boolean} tokenExpired - Indicates if the user token has expired + * @property {string} userName - Username of the authenticated user + * @property {string} permission - User's permission level + * @property {string} userId - Unique identifier for the user + * @property {string} displayName - User's display name + * @property {string} loginMethod - Method used for authentication + */ return { offline: false, userName, permission, userId, displayName, loginMethod, tokenExpired, };packages/loot-core/src/server/admin/app.ts (2)
91-107: Standardize error handling across methodsIn the
user-updatemethod, errors are returned as{ error: err.reason }, whereas in other methods, different patterns are used (e.g., returningnullor{ someDeletionsFailed: true }). For consistency and better error management, consider standardizing the error response format across all methods.
192-210: Ensure consistent method responsesIn the
transfer-ownershipmethod, if an error occurs, it returns{ error: err.reason }, but on success, it doesn't explicitly return any value. For better consistency and clarity, consider returning an empty object{}upon successful completion, as done in other methods likeaccess-add.Apply this diff:
app.method('transfer-ownership', async function ({ fileId, newUserId }) { const userToken = await asyncStorage.getItem('user-token'); if (userToken) { try { await post( getServer().BASE_SERVER + '/admin/access/transfer-ownership/', { fileId, newUserId }, { 'X-ACTUAL-TOKEN': userToken, }, ); + return {}; } catch (err) { return { error: err.reason }; } } + return null; });packages/desktop-client/src/components/modals/EditUser.tsx (1)
180-191: Use appropriate elements for informational textThe label element is being used for informational text, which is semantically incorrect. Consider using a
Textcomponent or aptag instead to convey the message about changing the username of the server owner.Apply this diff to use the
Textcomponent:{defaultUser.owner && ( - <label + <Text style={{ ...styles.verySmallText, color: theme.warningTextLight, marginTop: 5, }} > Change this username with caution; it is the server owner. - </label> + </Text> )}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (4)
packages/desktop-client/e2e/settings.test.js-snapshots/Settings-checks-the-page-visuals-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/settings.test.js-snapshots/Settings-checks-the-page-visuals-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/src/icons/v1/role.svgis excluded by!**/*.svgupcoming-release-notes/3878.mdis excluded by!**/*.md
📒 Files selected for processing (67)
packages/api/methods.ts(1 hunks)packages/desktop-client/src/auth/AuthProvider.tsx(1 hunks)packages/desktop-client/src/auth/ProtectedRoute.tsx(1 hunks)packages/desktop-client/src/auth/types.ts(1 hunks)packages/desktop-client/src/components/App.tsx(4 hunks)packages/desktop-client/src/components/FinancesApp.tsx(4 hunks)packages/desktop-client/src/components/LoggedInUser.tsx(4 hunks)packages/desktop-client/src/components/ManageRules.tsx(1 hunks)packages/desktop-client/src/components/Modals.tsx(4 hunks)packages/desktop-client/src/components/ServerContext.tsx(4 hunks)packages/desktop-client/src/components/admin/UserAccess/UserAccess.tsx(1 hunks)packages/desktop-client/src/components/admin/UserAccess/UserAccessHeader.tsx(1 hunks)packages/desktop-client/src/components/admin/UserAccess/UserAccessPage.tsx(1 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/admin/UserDirectory/UserDirectoryHeader.tsx(1 hunks)packages/desktop-client/src/components/admin/UserDirectory/UserDirectoryPage.tsx(1 hunks)packages/desktop-client/src/components/admin/UserDirectory/UserDirectoryRow.tsx(1 hunks)packages/desktop-client/src/components/autocomplete/PayeeAutocomplete.test.tsx(2 hunks)packages/desktop-client/src/components/common/Button.tsx(4 hunks)packages/desktop-client/src/components/common/Button2.tsx(3 hunks)packages/desktop-client/src/components/common/SimpleTable.tsx(1 hunks)packages/desktop-client/src/components/manager/BudgetList.tsx(13 hunks)packages/desktop-client/src/components/manager/ManagementApp.tsx(5 hunks)packages/desktop-client/src/components/manager/subscribe/Bootstrap.tsx(6 hunks)packages/desktop-client/src/components/manager/subscribe/ConfirmPasswordForm.tsx(2 hunks)packages/desktop-client/src/components/manager/subscribe/Login.tsx(4 hunks)packages/desktop-client/src/components/manager/subscribe/OpenIdCallback.ts(1 hunks)packages/desktop-client/src/components/manager/subscribe/OpenIdForm.tsx(1 hunks)packages/desktop-client/src/components/manager/subscribe/common.tsx(4 hunks)packages/desktop-client/src/components/modals/CreateAccountModal.tsx(5 hunks)packages/desktop-client/src/components/modals/EditAccess.tsx(1 hunks)packages/desktop-client/src/components/modals/EditUser.tsx(1 hunks)packages/desktop-client/src/components/modals/GoCardlessInitialiseModal.tsx(3 hunks)packages/desktop-client/src/components/modals/OpenIDEnableModal.tsx(1 hunks)packages/desktop-client/src/components/modals/PasswordEnableModal.tsx(1 hunks)packages/desktop-client/src/components/modals/SimpleFinInitialiseModal.tsx(4 hunks)packages/desktop-client/src/components/modals/TransferOwnership.tsx(1 hunks)packages/desktop-client/src/components/responsive/wide.ts(1 hunks)packages/desktop-client/src/components/settings/AuthSettings.tsx(1 hunks)packages/desktop-client/src/components/settings/Experimental.tsx(1 hunks)packages/desktop-client/src/components/settings/index.tsx(2 hunks)packages/desktop-client/src/components/transactions/TransactionsTable.test.jsx(2 hunks)packages/desktop-client/src/hooks/useFeatureFlag.ts(1 hunks)packages/desktop-client/src/hooks/useSyncServerStatus.ts(1 hunks)packages/desktop-client/src/icons/v1/Role.tsx(1 hunks)packages/desktop-client/src/index.tsx(2 hunks)packages/desktop-client/vite.config.mts(1 hunks)packages/loot-core/src/client/reducers/budgets.ts(5 hunks)packages/loot-core/src/client/shared-listeners.ts(1 hunks)packages/loot-core/src/client/state-types/modals.d.ts(2 hunks)packages/loot-core/src/server/admin/app.ts(1 hunks)packages/loot-core/src/server/admin/types/handlers.ts(1 hunks)packages/loot-core/src/server/api-models.ts(1 hunks)packages/loot-core/src/server/cloud-storage.ts(2 hunks)packages/loot-core/src/server/main.ts(11 hunks)packages/loot-core/src/server/post.ts(1 hunks)packages/loot-core/src/shared/errors.ts(1 hunks)packages/loot-core/src/types/budget.d.ts(1 hunks)packages/loot-core/src/types/file.d.ts(4 hunks)packages/loot-core/src/types/handlers.d.ts(2 hunks)packages/loot-core/src/types/models/index.d.ts(1 hunks)packages/loot-core/src/types/models/openid.d.ts(1 hunks)packages/loot-core/src/types/models/user.ts(1 hunks)packages/loot-core/src/types/models/userAccess.ts(1 hunks)packages/loot-core/src/types/prefs.d.ts(2 hunks)packages/loot-core/src/types/server-handlers.d.ts(4 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/loot-core/src/types/models/openid.d.ts
🚧 Files skipped from review as they are similar to previous changes (34)
- packages/api/methods.ts
- packages/desktop-client/src/auth/types.ts
- packages/desktop-client/src/components/ManageRules.tsx
- packages/desktop-client/src/components/admin/UserAccess/UserAccessHeader.tsx
- packages/desktop-client/src/components/admin/UserAccess/UserAccessPage.tsx
- packages/desktop-client/src/components/admin/UserDirectory/UserDirectoryHeader.tsx
- packages/desktop-client/src/components/admin/UserDirectory/UserDirectoryPage.tsx
- packages/desktop-client/src/components/autocomplete/PayeeAutocomplete.test.tsx
- packages/desktop-client/src/components/common/Button.tsx
- packages/desktop-client/src/components/common/SimpleTable.tsx
- packages/desktop-client/src/components/manager/ManagementApp.tsx
- packages/desktop-client/src/components/manager/subscribe/OpenIdCallback.ts
- packages/desktop-client/src/components/modals/EditAccess.tsx
- packages/desktop-client/src/components/modals/PasswordEnableModal.tsx
- packages/desktop-client/src/components/modals/SimpleFinInitialiseModal.tsx
- packages/desktop-client/src/components/responsive/wide.ts
- packages/desktop-client/src/components/settings/Experimental.tsx
- packages/desktop-client/src/components/settings/index.tsx
- packages/desktop-client/src/hooks/useFeatureFlag.ts
- packages/desktop-client/src/hooks/useSyncServerStatus.ts
- packages/desktop-client/src/icons/v1/Role.tsx
- packages/desktop-client/src/index.tsx
- packages/desktop-client/vite.config.mts
- packages/loot-core/src/client/reducers/budgets.ts
- packages/loot-core/src/client/shared-listeners.ts
- packages/loot-core/src/server/admin/types/handlers.ts
- packages/loot-core/src/server/api-models.ts
- packages/loot-core/src/types/budget.d.ts
- packages/loot-core/src/types/file.d.ts
- packages/loot-core/src/types/handlers.d.ts
- packages/loot-core/src/types/models/index.d.ts
- packages/loot-core/src/types/models/user.ts
- packages/loot-core/src/types/models/userAccess.ts
- packages/loot-core/src/types/prefs.d.ts
🧰 Additional context used
🪛 Biome (1.9.4)
packages/desktop-client/src/components/admin/UserAccess/UserAccess.tsx
[error] 104-104: This is an unexpected use of the debugger statement.
Unsafe fix: Remove debugger statement
(lint/suspicious/noDebugger)
packages/desktop-client/src/components/admin/UserAccess/UserAccessRow.tsx
[error] 94-94: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 95-95: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
packages/desktop-client/src/components/admin/UserDirectory/UserDirectoryRow.tsx
[error] 41-41: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 42-42: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
packages/desktop-client/src/components/modals/OpenIDEnableModal.tsx
[error] 10-10: Do not shadow the global "Error" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
🔇 Additional comments (45)
packages/desktop-client/src/auth/AuthProvider.tsx (1)
1-16: LGTM! Clean type definitions and well-organized imports.
The type definitions are well-structured and the imports are logically organized.
packages/desktop-client/src/auth/ProtectedRoute.tsx (1)
1-10: LGTM! Well-organized imports.
The imports are logically grouped and all appear to be necessary for the component's functionality.
packages/desktop-client/src/components/modals/OpenIDEnableModal.tsx (1)
30-32: Verify secure OpenID configuration handling
Since this handles authentication configuration, ensure that:
- The OpenID configuration is properly validated server-side
- The enable-openid endpoint requires proper authentication
✅ Verification successful
OpenID configuration is properly secured
The implementation shows proper security measures:
- The
enable-openidhandler verifies user authentication by checking for a valid user token - The configuration is sent to a protected endpoint (
/openid/enable) with proper authentication headers - The server maintains session state through
X-ACTUAL-TOKENheader
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for server-side validation of OpenID config
rg -A 5 "enable-openid" --type ts
# Check for authentication middleware usage
rg -A 5 "app\.(post|put)\(['\"]\/api\/enable-openid" --type ts
Length of output: 3934
Script:
#!/bin/bash
# Let's check the implementation of the enable-openid handler and related OpenID configuration validation
ast-grep --pattern 'handlers["enable-openid"] = async function $_($_) {
$$$
}'
# Check for OpenID configuration type definition
rg -A 10 "type OpenIdConfig" --type ts
# Check for any OpenID validation utilities
rg -A 5 "validateOpenId|validateOpenID|openid.*validate" --type ts
Length of output: 4486
Script:
#!/bin/bash
# Let's check the full implementation of the enable-openid handler
rg -A 15 "handlers\['enable-openid'\]" --type ts
# Check for any OpenID-related security checks or validations in the server code
rg -A 10 "openid|OpenID" packages/loot-core/src/server --type ts
# Check for any authentication middleware or security checks
rg -A 5 "userToken|authenticate|requireAuth" packages/loot-core/src/server --type ts
Length of output: 36285
packages/desktop-client/src/components/settings/AuthSettings.tsx (1)
1-15: LGTM! Imports are well-organized and complete.
The imports are logically grouped and include all necessary dependencies for the component's functionality.
packages/desktop-client/src/components/manager/subscribe/common.tsx (2)
50-54:
Improve error handling for server URL validation.
The server URL construction using window.location.origin could be vulnerable to DNS rebinding attacks. Consider adding URL validation and sanitization.
-const serverURL = window.location.origin;
+const serverURL = validateAndSanitizeURL(window.location.origin);
+function validateAndSanitizeURL(url: string): string {
+ const parsed = new URL(url);
+ if (!['http:', 'https:'].includes(parsed.protocol)) {
+ throw new Error('Invalid protocol');
+ }
+ return parsed.origin;
+}Likely invalid or redundant comment.
Line range hint 24-94: Verify OpenID integration points.
The useBootstrapped hook handles login method configuration, but let's verify the integration points with the OpenID implementation.
✅ Verification successful
OpenID integration is properly implemented and verified
The code review shows that the useBootstrapped hook correctly integrates with the OpenID authentication flow:
- Login methods are properly managed through the
ServerContextwith appropriate types and handlers - OpenID is fully integrated with dedicated components (
OpenIdForm,OpenIdCallback) - The authentication flow supports multiple login methods including OpenID
- Feature flag control is implemented for OpenID authentication
- Proper error handling and redirection logic is in place
The changes are consistent with the codebase and follow the established authentication patterns.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for OpenID-related configurations and usage
echo "Checking OpenID-related imports and configurations..."
rg -l "openid|oidc" --type ts --type tsx
echo "Checking login method handlers..."
ast-grep --pattern 'loginMethods.includes($_)'
echo "Checking authentication flow components..."
fd -e tsx -e ts | xargs rg -l "useBootstrapped|AuthProvider|ProtectedRoute"
Length of output: 1271
Script:
#!/bin/bash
# Let's examine the authentication flow and login methods handling more closely
# Check the login methods handling in Login component
echo "=== Login Component Implementation ==="
rg -A 10 "loginMethods" packages/desktop-client/src/components/manager/subscribe/Login.tsx
# Check AuthProvider implementation
echo -e "\n=== AuthProvider Implementation ==="
cat packages/desktop-client/src/auth/AuthProvider.tsx
# Check ProtectedRoute implementation
echo -e "\n=== ProtectedRoute Implementation ==="
cat packages/desktop-client/src/auth/ProtectedRoute.tsx
# Look for login method type definitions
echo -e "\n=== Login Method Types ==="
rg "type.*LoginMethod" --type ts --type tsx
Length of output: 5147
Script:
#!/bin/bash
# Let's check the type definitions and handlers for login methods
# Search for login method types in all possible extensions
echo "=== Login Method Type Definitions ==="
rg "type.*LoginMethod|interface.*LoginMethod" .
# Check the subscribe-needs-bootstrap handler implementation
echo -e "\n=== Bootstrap Handler Implementation ==="
rg -A 10 "subscribe-needs-bootstrap" .
# Check how login methods are used in the authentication flow
echo -e "\n=== Login Methods Usage ==="
rg -A 5 "useAvailableLoginMethods|setLoginMethods" .
# Look for any OpenID/OAuth related configurations
echo -e "\n=== Auth Configuration ==="
rg "openid|oauth|oidc" .
Length of output: 17535
packages/desktop-client/src/components/manager/subscribe/Bootstrap.tsx (5)
9-9: LGTM: New imports support OpenID implementation
The addition of useNavigate and useRefreshLoginMethods hooks aligns with the PR's OpenID implementation objectives.
Also applies to: 16-16
25-25: LGTM: Proper hook initialization
The hooks are correctly initialized at the component level and follow React best practices.
Also applies to: 28-28
70-70: LGTM: Style adjustments maintain consistency
The style modifications properly utilize the theme system and improve visual consistency.
Also applies to: 107-111
56-57: Verify error handling in the new authentication flow
The new flow correctly refreshes login methods and navigates to the login page. However, let's ensure error states are properly handled.
✅ Verification successful
Error handling is properly implemented in the new authentication flow
The error handling in the new authentication flow is well-implemented with comprehensive coverage:
- The
onSetPasswordfunction in Bootstrap.tsx properly handles errors:- Clears previous errors with
setError(null) - Captures and sets errors from the bootstrap request
- Only proceeds with navigation on success
- Clears previous errors with
- Error states include specific handling for:
- Invalid/empty passwords
- Network failures
- Password mismatches
- Server communication issues
- Error messages are user-friendly and localized
- The UI appropriately displays error states with proper styling
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for error handling patterns in authentication flows
rg -A 5 "catch|error|fail" packages/desktop-client/src/components/manager/subscribe/
Length of output: 23069
Line range hint 22-58: Consider implementing a centralized authentication state management
Given that this component is part of the authentication flow, consider implementing a centralized authentication state management system (if not already present) to handle various authentication methods consistently.
packages/desktop-client/src/components/manager/subscribe/ConfirmPasswordForm.tsx (1)
87-150: Verify the integration with OpenID authentication flow.
Since this component is part of a larger OpenID implementation, let's verify its usage in the authentication flow.
✅ Verification successful
Let me try again with the correct command syntax:
Let me check the PasswordEnableModal to understand the complete flow:
Based on the gathered information, I can now provide the final response:
Component correctly integrates with password management flow
The ConfirmOldPasswordForm component is properly integrated within the authentication system:
- It's used in
PasswordEnableModalfor password confirmation - The component handles password validation through the
enable-passwordserver endpoint - After successful password confirmation, it properly manages the authentication state by:
- Refreshing login methods
- Clearing user token
- Closing the budget
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usage of ConfirmOldPasswordForm in the authentication flow
echo "Searching for ConfirmOldPasswordForm usage..."
rg -l "ConfirmOldPasswordForm" --type typescript
echo "\nChecking authentication-related files that might use this component..."
rg -l "OpenID|Authentication|Login" --type typescript
echo "\nChecking for potential password confirmation flows..."
ast-grep --pattern 'import { $_ } from "$_/ConfirmPasswordForm"'
Length of output: 618
Script:
#!/bin/bash
# Search for usage of ConfirmOldPasswordForm
echo "Searching for ConfirmOldPasswordForm usage..."
rg "ConfirmOldPasswordForm"
echo -e "\nChecking authentication-related files..."
rg -l "OpenID|Authentication|Login"
echo -e "\nChecking the component's direct usage context..."
rg -A 5 "ConfirmOldPasswordForm"
Length of output: 3919
Script:
#!/bin/bash
# Check the password enable modal implementation
echo "Checking PasswordEnableModal implementation..."
rg -A 20 "function PasswordEnableModal" packages/desktop-client/src/components/modals/PasswordEnableModal.tsx
echo -e "\nChecking how password confirmation is handled..."
rg -B 5 -A 10 "onSetPassword" packages/desktop-client/src/components/modals/PasswordEnableModal.tsx
Length of output: 2802
packages/desktop-client/src/components/admin/UserDirectory/UserDirectoryRow.tsx (1)
21-26: LGTM! Well-structured component implementation
Good use of memo for performance optimization and clean implementation of state-dependent styling.
packages/desktop-client/src/components/modals/GoCardlessInitialiseModal.tsx (1)
6-6: LGTM: Error handling setup looks good!
The centralized error handling with a dedicated state variable and standardized error messages through getSecretsError is a good practice.
Also applies to: 35-37
packages/desktop-client/src/components/ServerContext.tsx (1)
Line range hint 48-81: Verify usage of new hooks across the codebase
Let's ensure that all new hooks are being used correctly throughout the application.
✅ Verification successful
All new hooks are properly implemented and used consistently
The verification shows that the new hooks are being used appropriately throughout the application:
useMultiuserEnabled: Used in multiple components for access control (FinancesApp, ManagementApp, BudgetList, etc.)useLoginMethod: Properly used in auth-related components (Login.tsx, AuthSettings.tsx)useAvailableLoginMethods: Correctly used in login and password managementuseRefreshLoginMethods: Implemented in bootstrap and modal componentsuseSetMultiuserEnabledanduseSetLoginMethods: Used in the common subscription components
All direct context access is properly encapsulated within the hook implementations in ServerContext.tsx, following React best practices.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usage of the new hooks to verify proper implementation
echo "Checking usage of new hooks..."
rg "use(MultiuserEnabled|LoginMethod|AvailableLoginMethods|RefreshLoginMethods|SetMultiuserEnabled|SetLoginMethods)" -A 2
# Check for potential direct context access that should use hooks instead
echo "Checking for direct context access..."
ast-grep --pattern 'useContext(ServerContext)'
Length of output: 11842
packages/desktop-client/src/components/admin/UserAccess/UserAccessRow.tsx (1)
33-36: Verify server-side access control.
Ensure that the server-side implementation of 'access-add' properly validates the user's permissions before granting access. Client-side checks alone are not sufficient for security.
packages/loot-core/src/server/post.ts (1)
131-177: Improve error logging and error handling documentation
While the implementation is functionally correct, there are opportunities for improvement in error handling and logging.
- Replace string concatenation with template literals for better readability and performance:
- console.log(
- 'API call failed: ' +
- url +
- '\nData: ' +
- JSON.stringify(data, null, 2) +
- '\nResponse: ' +
- JSON.stringify(res, null, 2),
- );
+ console.log(
+ `API call failed: ${url}
+Data: ${JSON.stringify(data, null, 2)}
+Response: ${JSON.stringify(res, null, 2)}`
+ );- Consider using a proper logging library that supports log levels and structured logging.
- Document the TODO about error handling more explicitly or create a ticket to track it.
Let's check if this logging pattern is used consistently across the codebase:
✅ Verification successful
Refactor logging pattern across HTTP client functions
The search results confirm that this logging pattern appears three times in post.ts, likely in the post, patch, and del functions. Since these functions share identical logging implementation, the suggested improvements should be applied consistently across all three functions.
packages/loot-core/src/server/post.ts: Update all three HTTP client functions (post,patch,del) to use template literals for API failure logging
- Replace string concatenation with template literals in all three functions:
console.log(
`API call failed: ${url}
Data: ${JSON.stringify(data, null, 2)}
Response: ${JSON.stringify(res, null, 2)}`
);- Consider extracting this logging logic into a shared helper function to maintain consistency and make future improvements easier.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for similar logging patterns that could benefit from the same improvement
rg "API call failed.*JSON\.stringify.*Response:.*JSON\.stringify" --type ts
Length of output: 77
Script:
#!/bin/bash
# Let's try a broader search for API call logging patterns
rg "API call failed" --type ts -A 5
Length of output: 1225
Script:
#!/bin/bash
# Search for console.log statements with JSON.stringify
rg "console\.log.*JSON\.stringify" --type ts -A 2
Length of output: 557
Script:
#!/bin/bash
# Search for similar error handling patterns
ast-grep --pattern 'if ($_.status !== "ok") { console.log($$$) }'
Length of output: 67
packages/desktop-client/src/components/modals/TransferOwnership.tsx (2)
1-25: LGTM! Clean imports and type definitions.
The imports are well-organized and the TransferOwnershipProps type is appropriately defined.
1-166: Verify component integration with OpenID implementation.
Let's verify how this component is integrated with the OpenID authentication system.
✅ Verification successful
Based on the verification results, I can now provide a final response:
Component is properly integrated with authentication system
The TransferOwnership component is correctly integrated with the authentication system:
- It verifies user authentication through the
user-tokencheck in the backend - It properly handles authorization errors through
getUserAccessErrors - The ownership transfer endpoint
/admin/access/transfer-ownership/requires valid authentication token - The component correctly handles session expiration and unauthorized states
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for TransferOwnership component usage and related auth checks
echo "Checking for TransferOwnership usage and auth integration..."
# Look for modal registration
rg -l "transfer-ownership.*Modal" --type typescript
# Check for auth-related guards around ownership transfer
ast-grep --pattern 'send("transfer-ownership"' -l
# Look for related permission checks
rg -l "Permissions.*transfer.*ownership" --type typescript
Length of output: 473
Script:
#!/bin/bash
# Check for TransferOwnership component usage and auth integration
echo "Checking for TransferOwnership usage and auth integration..."
# Look for modal registration and usage
rg -l "TransferOwnership"
# Check for auth-related guards around ownership transfer
rg -C 3 "transfer-ownership"
# Look for related permission checks in the codebase
rg -C 3 "Permissions.*ownership"
Length of output: 4914
Script:
#!/bin/bash
# Check for authentication and authorization implementation
echo "Checking for auth implementation..."
# Look for user token and auth checks
rg -C 3 "user-token"
# Check the server endpoint implementation
rg -C 5 "/admin/access/transfer-ownership"
# Look for related user access checks
rg -C 3 "getUserAccessErrors"
Length of output: 29618
packages/desktop-client/src/components/common/Button2.tsx (2)
12-13: LGTM: Clean auth-related imports
The new imports are properly organized and necessary for the permission handling functionality.
137-137: Verify permission enum usage across the codebase
The optional permission prop is well-typed and properly integrated. Let's verify its consistent usage across the codebase.
✅ Verification successful
Permission prop usage is consistent and well-integrated
The verification shows that:
- The
Permissionsenum is properly defined withADMINISTRATORpermission - The permission prop is consistently used in protected routes and components
- All files importing the
Permissionstype are using it from the correct path (auth/types) - The Button2 component's optional permission prop aligns with the existing permission-based access control pattern in the codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent usage of Permissions enum with Button components
# Look for any potential mismatches or inconsistencies in permission usage
# Find all Button usages with permission prop
rg -t typescript -t tsx "Button.*permission=" -A 2
# Find the Permissions enum definition to verify available values
ast-grep --pattern 'enum Permissions {
$$$
}'
Length of output: 324
Script:
#!/bin/bash
# Let's try a more comprehensive search
# Search for Button2 component usage
rg -l "Button2"
# Search for permission prop usage with proper file extensions
rg "permission=" --type-add 'ts:*.{ts,tsx}' -t ts -C 2
# Look for imports of Permissions enum
rg "import.*Permissions.*from" --type-add 'ts:*.{ts,tsx}' -t ts
Length of output: 12296
packages/desktop-client/src/components/LoggedInUser.tsx (1)
149-152: LGTM: Well-structured type definition
The MenuItem type is properly defined and provides good type safety for the menu items.
packages/desktop-client/src/components/App.tsx (1)
12-12: LGTM: Imports are appropriate for the new functionality.
The new imports properly support the Redux state management and actions required for session handling.
Also applies to: 23-23, 30-30
packages/loot-core/src/client/state-types/modals.d.ts (1)
11-12: LGTM! Clean type-only imports
The imports follow TypeScript best practices by using the type keyword for type-only imports in declaration files.
packages/desktop-client/src/components/FinancesApp.tsx (3)
17-18: LGTM! Imports are well-organized.
The new imports are logically grouped and properly support the OpenID implementation.
Also applies to: 26-26, 40-40, 42-42
101-102: Consider handling loading state for multiuser flag.
While the implementation is correct, consider handling the loading state of multiuserEnabled to prevent potential flash of incorrect content. This is especially important for protected routes that depend on this flag.
Let's check if the useMultiuserEnabled hook handles loading states:
Consider updating the code to handle loading state:
- const multiuserEnabled = useMultiuserEnabled();
+ const { enabled: multiuserEnabled, loading } = useMultiuserEnabled();
+ if (loading) {
+ return <LoadingIndicator />;
+ }291-313: Architectural suggestions for protected routes.
The protected routes implementation looks good, but consider these improvements:
- Add loading states during route transitions
- Implement error boundaries for protected routes
- Document the purpose of
validateOwnerprop
Consider updating the code:
<ProtectedRoute
permission={Permissions.ADMINISTRATOR}
+ fallback={<LoadingIndicator />}
+ errorBoundary={<ErrorBoundary />}
element={<UserDirectoryPage />}
/>Let's verify the implementation of ProtectedRoute and related components:
packages/desktop-client/src/components/admin/UserAccess/UserAccess.tsx (1)
80-83: Verify API endpoint access controls.
Since this component handles sensitive user access management, ensure that the API endpoints ('access-get-available-users' and 'transfer-ownership') have proper access control checks on the server side.
Also applies to: 234-241
✅ Verification successful
Let me gather more information about the server-side access controls.
Let me search for more security-related code, particularly around permissions and authentication.
Based on the gathered information, I can now provide a final response:
API endpoints have proper access controls in place
The codebase shows a robust security implementation:
- Client-side permission checks through
hasPermissionandPermissions.ADMINISTRATOR - Protected routes with
ProtectedRoutecomponent requiring specific permissions - Server-side authentication via
X-ACTUAL-TOKENheader for all API requests - User token validation before accessing sensitive endpoints
- Role-based access control differentiating between admin and regular users
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for API endpoint definitions and their access controls
rg -A 10 "access-get-available-users|transfer-ownership" --type ts
Length of output: 10388
Script:
#!/bin/bash
# Search for server-side endpoint implementations and access controls
rg -A 10 "/admin/access/(users|transfer-ownership)" --type ts
# Also search for any middleware or authentication checks
rg -A 10 "requireAdmin|isAdmin|checkAccess" --type ts
Length of output: 4375
Script:
#!/bin/bash
# Search for permission checks and authentication middleware
rg -A 10 "hasPermission|Permissions\.|authenticate|checkAuth" --type ts
# Look for user token validation
rg -A 10 "X-ACTUAL-TOKEN|validateToken|verifyToken" --type ts
Length of output: 38879
packages/loot-core/src/types/server-handlers.d.ts (2)
20-20: LGTM: Import is properly organized
The OpenIdConfig import is correctly placed with other model imports and is consistently used throughout the interface.
349-349: LGTM: File info retrieval is properly typed
The method signature is well-defined with appropriate return types handling both success and null cases.
packages/desktop-client/src/components/manager/subscribe/OpenIdForm.tsx (3)
19-41: LGTM! Well-structured type definitions
The type definitions are comprehensive and provide good type safety for the OpenID implementation. The OpenIdProviderOption type is particularly well-designed, allowing for both static and dynamic configuration of provider details.
226-385: LGTM! Well-documented provider configurations
The OpenID provider configurations are comprehensive and well-documented. Each provider includes:
- Clear configuration parameters
- Helpful documentation links
- Appropriate warnings for providers with specific requirements
- Good use of sorting for better UX
1-416: Verify security considerations
Please ensure the following security aspects are properly handled:
- Client secrets should not be logged or stored in browser storage
- CSRF protection for the API endpoints
- Proper sanitization of provider URLs before use
✅ Verification successful
Based on the shell script results, let me analyze the security aspects:
- Client secrets handling:
- No direct exposure of client secrets through console.log in the OpenIdForm component
- No storage of secrets in localStorage
- The code properly handles sensitive data by only passing it through the API call
- URL handling:
- Proper URL validation and sanitization using the native URL API
- Server URLs are properly validated before use
- Client-side URL handling uses safe methods
- Data storage:
- Only non-sensitive data stored in localStorage (language preference, UI flags)
- No storage of authentication tokens or secrets in browser storage
Security implementation verified
The implementation follows security best practices:
- Proper URL validation and sanitization
- No exposure of sensitive data in logs
- No client-side storage of secrets
- Secure handling of authentication flow
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for potential security issues in the codebase
# Look for any console.log statements that might expose sensitive data
echo "Checking for potential sensitive data exposure..."
rg -i "console\.(log|debug|info)" -A 2 -B 2
# Check for localStorage usage that might store sensitive data
echo "Checking for localStorage usage..."
rg -i "localStorage\." -A 2 -B 2
# Look for URL sanitization
echo "Checking URL handling..."
rg -i "url\.parse|new URL|window\.location" -A 2 -B 2
Length of output: 59002
packages/desktop-client/src/components/modals/CreateAccountModal.tsx (2)
9-10: LGTM! Clean permission check implementation.
The permission check logic is well-implemented, considering both multiuser mode and administrator permissions in a clear, maintainable way.
Also applies to: 190-192
Line range hint 1-445: Verify test coverage for permission-based rendering.
Please ensure there are tests covering:
- Permission-based visibility of bank connection options
- Warning message display conditions
- Menu interaction behaviors
Let's check for existing test coverage:
packages/desktop-client/src/components/Modals.tsx (2)
624-640: LGTM! User management modal cases are well-implemented.
The implementation follows the established patterns and includes proper prop passing.
642-650: Verify the authentication flow implementation.
The modal implementations look good, but since these handle critical authentication features:
- Ensure that the OpenID implementation follows the OpenID Connect specifications
- Verify that password authentication follows security best practices
- Confirm that ownership transfer has proper authorization checks
✅ Verification successful
Security implementation verified with appropriate safeguards in place
The modal implementations follow security best practices:
-
OpenID implementation:
- Validates configuration before enabling
- Properly handles errors from OpenID validation
- Closes all sessions and removes user tokens for security
- Clearly communicates ownership implications
-
Password authentication:
- Includes password confirmation
- Handles both new password setup and existing password verification
- Implements proper error handling
- Forces session closure on authentication changes
-
Ownership transfer:
- Requires explicit user selection
- Implements proper authorization checks
- Includes clear warning messages about irreversibility
- Properly handles errors and session management
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for OpenID-related security measures
echo "Checking OpenID implementation..."
rg -l 'openid|oidc' --type ts --type tsx
# Look for password-related security measures
echo "Checking password authentication implementation..."
rg -l 'password.*hash|bcrypt|argon2' --type ts --type tsx
# Check for ownership transfer authorization
echo "Checking ownership transfer authorization..."
rg -l 'isAdmin|hasPermission|authorize' --type ts --type tsx
Length of output: 536
Script:
#!/bin/bash
# Search for OpenID-related security measures with correct file type flags
echo "Checking OpenID implementation..."
rg -l 'openid|oidc' -t typescript -t javascriptreact -t typescriptreact
# Look for the actual modal implementations
echo "Checking modal implementations..."
ast-grep --pattern 'const OpenIDEnableModal = $_'
ast-grep --pattern 'const PasswordEnableModal = $_'
ast-grep --pattern 'const TransferOwnership = $_'
# Look for authentication and security related code
echo "Checking security implementations..."
rg -l 'password|authentication|authorization|security' -t typescript -t javascriptreact -t typescriptreact
# Check for ownership and permission checks
echo "Checking permission checks..."
rg -l 'isAdmin|hasPermission|role|owner' -t typescript -t javascriptreact -t typescriptreact
Length of output: 9807
Script:
#!/bin/bash
# Try with simpler file extensions
echo "Checking OpenID implementation..."
rg -l 'openid|oidc' --type ts
echo "Checking modal components..."
fd -e tsx -e ts | xargs rg -l 'OpenIDEnableModal|PasswordEnableModal|TransferOwnership'
echo "Checking security implementations..."
rg -l 'password|authentication|authorization|security' --type ts
echo "Checking permission checks..."
rg -l 'isAdmin|hasPermission|role|owner' --type ts
# Look for the specific modal file content
echo "Checking modal implementations..."
rg -A 5 'OpenIDEnableModal|PasswordEnableModal|TransferOwnership' packages/desktop-client/src/components/Modals.tsx
Length of output: 7529
Script:
#!/bin/bash
# Check the implementation details of the security-related modals
echo "Checking OpenID modal implementation..."
cat packages/desktop-client/src/components/modals/OpenIDEnableModal.tsx
echo "Checking Password modal implementation..."
cat packages/desktop-client/src/components/modals/PasswordEnableModal.tsx
echo "Checking Transfer Ownership modal implementation..."
cat packages/desktop-client/src/components/modals/TransferOwnership.tsx
# Check for authentication handlers
echo "Checking authentication handlers..."
rg -A 5 'handleOpenIDAuth|handlePasswordAuth|handleTransferOwnership' --type ts
Length of output: 13456
packages/desktop-client/src/components/transactions/TransactionsTable.test.jsx (3)
25-25: LGTM: Import statement is correctly placed and follows conventions.
The AuthProvider import is appropriately grouped with other component imports and uses the correct relative path.
152-178: LGTM: AuthProvider is correctly integrated into the component hierarchy.
The AuthProvider is appropriately placed as the outermost provider, ensuring authentication context is available to all nested components while maintaining the existing provider hierarchy.
Line range hint 1-1: Consider adding authentication-related test cases.
With the introduction of AuthProvider, consider adding test cases to verify:
- Authentication state's impact on transaction operations
- Permission-based access control for transaction management
- Behavior when authentication context changes
Would you like me to help create test cases for authentication scenarios?
packages/loot-core/src/server/main.ts (5)
22-22: LGTM: OpenID type import added.
The import of OpenIdConfig type is appropriately placed with other type imports.
881-881: Enhance error handling with reason.
Good improvement to include the error reason in the response, providing more context for debugging and error handling.
1704-1706: LGTM: Token management handler added.
The token management handler is simple and focused.
2277-2277: LGTM: Admin app integration.
The admin app is properly integrated with the main application.
1557-1572: Verify the bootstrap handler's error handling.
The bootstrap handler has been updated to accept a more flexible loginConfig parameter, but we should ensure comprehensive error handling.
✅ Verification successful
Error handling in the bootstrap handler is appropriate and consistent
The error handling in the bootstrap handler follows the established pattern used throughout the codebase for network-related operations:
- It properly catches errors and returns a standardized error format
{ error: err.reason || 'network-failure' } - The error handling is consistent with other network operations in the same file
- The client-side code in
Bootstrap.tsxandLogin.tsxappropriately handles these error responses
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for error handling patterns in bootstrap-related code
# Look for error handling patterns in bootstrap-related code
rg -A 5 "bootstrap.*error"
Length of output: 28
Script:
#!/bin/bash
# Let's search for bootstrap-related code and error handling patterns more broadly
rg -A 10 "subscribe-bootstrap|/bootstrap"
# Also search for any error handling patterns in the same file
rg -A 5 "catch.*err" packages/loot-core/src/server/main.ts
Length of output: 11682
packages/desktop-client/src/components/modals/OpenIDEnableModal.tsx
Outdated
Show resolved
Hide resolved
packages/desktop-client/src/components/settings/AuthSettings.tsx
Outdated
Show resolved
Hide resolved
|
You might want to pause it again if you make any changes until you're happy for it to do another review, otherwise it can get a bit busy doing it on every commit |
|
@coderabbitai ignore |
yeah, there is a bunch of suggestion already. I'm accepting some to fix what code rabbit suggested (some won't compile) |
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
This PR is a nightmare omg lol. went through more than a half of the code rabbit suggestions. IF anyone want help me with this, checking for string that need translation would be super (and send me a PR for my branch) |
|
/update-vrt |
|
@coderabbitai ignore |
...-client/e2e/settings.test.js-snapshots/Settings-checks-the-page-visuals-1-chromium-linux.png
Outdated
Show resolved
Hide resolved
|
/update-vrt |
Following the PR #1164
Instruction on how to test this.
Clone actual server PR and this PR locally, at the server, in the packages.json, point the @actual/web to link to this PR clone (this is important because the pages will come from this package)
This is how I do:
It's important to remember to BUILD the desktop-client package before starting the server.
I suggest to start testing this using a empty server instead of an server already configured.
Once you have actual server running, you can open the URL and start configuring it using this preview doc https://deploy-preview-447.www.actualbudget.org/docs/advanced/oauth-auth
I suggest start with configuring from bootstrap. Use passwordless.id for testing, probably the easiest way to test.
You can test the openid provider login now. But to test multiple users, you have to configure multiuser support