🔨 OPENID: Refactor mixed stuff #4428
Conversation
actual-github-bot
bot
changed the title
✅ Deploy Preview for actualbudget ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
lelemm
changed the title
|
Warning Rate limit exceeded@lelemm has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 17 minutes and 28 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (3)
WalkthroughThe changes refactor the OpenID handling and configuration process in the sync-server package. The Possibly related PRs
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/sync-server/src/account-db.js (1)
71-71: Consider improving transaction rollback error handling.The transaction rollback in the bootstrap function could benefit from better error handling. Consider wrapping the rollback in a try-catch block to handle potential rollback failures.
Apply this diff to improve error handling:
if (!forced && (!openIdEnabled || countOfOwner > 0)) { if (!needsBootstrap()) { - accountDb.mutate('ROLLBACK'); - return { error: 'already-bootstrapped' }; + try { + accountDb.mutate('ROLLBACK'); + return { error: 'already-bootstrapped' }; + } catch (rollbackError) { + console.error('Failed to rollback transaction:', rollbackError); + return { error: 'transaction-rollback-failed' }; + } } }Also applies to: 88-93
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
upcoming-release-notes/4428.mdis excluded by!**/*.md
📒 Files selected for processing (5)
packages/sync-server/package.json(0 hunks)packages/sync-server/src/account-db.js(5 hunks)packages/sync-server/src/app.js(2 hunks)packages/sync-server/src/config-types.ts(1 hunks)packages/sync-server/src/load-config.js(0 hunks)
💤 Files with no reviewable changes (2)
- packages/sync-server/package.json
- packages/sync-server/src/load-config.js
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: matt-fidd
PR: actualbudget/actual#4363
File: packages/sync-server/src/scripts/enable-openid.js:6-6
Timestamp: 2025-02-11T23:39:06.219Z
Learning: In the Actual codebase, when `config` is used as a function parameter, the imported configuration should use the alias `finalConfig` to avoid naming conflicts. Example: `import { config as finalConfig } from '../load-config.js'`.
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: Visual regression
- GitHub Check: Functional
- GitHub Check: build (macos-latest)
- GitHub Check: build (windows-latest)
- GitHub Check: build (ubuntu-latest)
- GitHub Check: Build Docker image (alpine)
- GitHub Check: Build Docker image (ubuntu)
🔇 Additional comments (4)
packages/sync-server/src/config-types.ts (1)
7-7: LGTM! MakingloginMethodoptional aligns with the refactoring.The change to make
loginMethodoptional is appropriate as it allows for more flexible configuration and better supports the new OpenID implementation where the login method can be determined dynamically.packages/sync-server/src/app.js (1)
9-9: LGTM! Import ofbootstrapfunction.The import is correctly placed and aligns with the new OpenID configuration functionality.
packages/sync-server/src/account-db.js (2)
59-61: LGTM! Prioritizing OpenID when configured.The change correctly prioritizes OpenID when it's configured, which aligns with the PR objectives.
113-119:❓ Verification inconclusive
Verify OpenID configuration consistency.
The
forcedparameter is used to bypass OpenID checks, but we should verify that this doesn't lead to inconsistent states in the database.
🏁 Script executed:
#!/bin/bash # Description: Check for potential inconsistent auth states in the database ast-grep --pattern $'accountDb.mutate(\'SELECT * FROM auth WHERE method = \'openid\' AND active = 1 AND EXISTS (SELECT 1 FROM auth WHERE method = \'password\' AND active = 1)\')'Length of output: 191
Review Comment Update: Verify OpenID Configuration Consistency
The forced parameter bypasses the standard OpenID checks, so it's important to ensure that doing so doesn't leave the database in an inconsistent state between OpenID and password authentication entries. Although the automated AST grep search did not yield any evidence of inconsistent auth states, please manually verify—and consider running additional checks—that the integration of these two authentication methods remains consistent in all cases.
- Confirm that no conflicting records are created in the
authtable when forced OpenID login is used.- Verify on staging or through manual DB inspection that the forced parameter truly bypasses only the intended checks without leaving residual authentication issues.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/sync-server/src/app.js (1)
109-111: Improve error logging consistency.The error object is logged using
console.logwhile other errors useconsole.error. For consistency and better error tracking:- console.log(error); + console.error('Failed to configure OpenID:', error);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/sync-server/src/app.js(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: matt-fidd
PR: actualbudget/actual#4363
File: packages/sync-server/src/scripts/enable-openid.js:6-6
Timestamp: 2025-02-11T23:39:06.219Z
Learning: In the Actual codebase, when `config` is used as a function parameter, the imported configuration should use the alias `finalConfig` to avoid naming conflicts. Example: `import { config as finalConfig } from '../load-config.js'`.
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: Functional
- GitHub Check: Visual regression
- GitHub Check: build (macos-latest)
- GitHub Check: build (windows-latest)
- GitHub Check: Analyze
- GitHub Check: build (ubuntu-latest)
- GitHub Check: Build Docker image (alpine)
- GitHub Check: Build Docker image (ubuntu)
🔇 Additional comments (1)
packages/sync-server/src/app.js (1)
9-9: LGTM!The import of
bootstrapis correctly placed and necessary for the OpenID configuration functionality.
Removed variable
MULTIUSERbecause it was not being used anymoreChanged how the sync server utilizes the OpenID env vars (and config.json):
Removed the command
yarn enable-openidbecause its deprecated on this new modelMade config variable
loginMethodto be optional. So when used in the code bellow, it can reachconst activeMethod = getActiveLoginMethod();: