Conversation
|
Warning Rate limit exceeded@freedit-dev has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 15 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 (1)
📝 WalkthroughWalkthroughAdds a configurable login_captcha flag: new i18n keys, a SiteConfig::login_captcha Option (default None), propagation to PageData, admin UI radio control, conditional CAPTCHA rendering on signin, and optional captcha inputs/validation in the signin handler. Changes
Sequence Diagram(s)sequenceDiagram
participant Admin as Admin UI
participant Server as Web Server
participant SiteCfg as SiteConfig
participant Page as PageData
participant Browser as User Browser
rect rgba(82,196,26,0.08)
Note over Admin,Server: Admin toggles login_captcha
Admin->>Server: POST update SiteConfig(login_captcha)
Server->>SiteCfg: persist login_captcha
end
rect rgba(20,120,200,0.06)
Note over Browser,Server: User opens signin page
Browser->>Server: GET /signin
Server->>SiteCfg: read site_config
Server->>Page: construct PageData(login_captcha = site_config.unwrap_or(false))
Server->>Browser: render signin (captcha shown only if login_captcha true)
end
rect rgba(255,180,50,0.06)
Note over Browser,Server: User submits signin form
Browser->>Server: POST /signin (captcha fields optional)
Server->>SiteCfg: read site_config (used to decide validation)
alt login_captcha enabled
Server->>Server: validate captcha (if provided)
else login_captcha disabled
Server->>Server: skip captcha validation
end
Server->>Browser: respond with auth result
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/controller/mod.rs (1)
532-533: Consider adding inline documentation.Adding a brief comment explaining the purpose of this field would improve code maintainability.
📝 Suggested documentation
+ /// When enabled, requires users to complete a CAPTCHA challenge during login #[validate(skip)] login_captcha: Option<bool>,src/controller/user.rs (1)
928-937: Consider more explicit validation of optional captcha fields.While the current implementation is functionally safe (missing fields result in empty strings that fail lookup), the error handling relies on implicit behavior. Consider explicitly checking for
Nonebefore validation for clarity and better error messages.🔎 Suggested refactor for explicit field validation
let site_config = SiteConfig::get(&DB)?; if site_config.login_captcha == Some(true) { + let captcha_id = input.captcha_id.as_deref().ok_or(AppError::CaptchaError)?; + let captcha_value = input.captcha_value.as_deref().ok_or(AppError::CaptchaError)?; + let captcha_char = DB .open_partition("captcha", Default::default())? - .take(&input.captcha_id.unwrap_or_default())? + .take(captcha_id)? .ok_or(AppError::CaptchaError)?; - if captcha_char != input.captcha_value.unwrap_or_default() { + if captcha_char != captcha_value { return Err(AppError::CaptchaError); } }This also addresses the clippy warning about unnecessary borrows.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
i18n/en.tomli18n/fr.tomli18n/ja.tomli18n/uk.tomli18n/zh_cn.tomlsrc/controller/admin.rssrc/controller/meta_handler.rssrc/controller/mod.rssrc/controller/user.rstemplates/admin.htmltemplates/signin.html
🧰 Additional context used
🪛 GitHub Check: clippy
src/controller/user.rs
[warning] 932-932:
the borrowed expression implements the required traits
[warning] 932-932:
the borrowed expression implements the required traits
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Cargo test, clippy and doc (windows-latest, x86_64-pc-windows-gnu)
- GitHub Check: Cargo test, clippy and doc (windows-latest, x86_64-pc-windows-msvc)
🔇 Additional comments (12)
src/controller/meta_handler.rs (2)
171-171: LGTM - Field addition is correct.The new
login_captchafield is properly added to thePageDatastruct with appropriate visibility.
198-198: CAPTCHA validation is properly gated in signin handler.The
signin_postfunction correctly checksif site_config.login_captcha == Some(true)before validating CAPTCHA fields (lines 929-937 insrc/controller/user.rs). TheFormSigninstruct declarescaptcha_idandcaptcha_valueas optional fields, and when CAPTCHA is disabled, the entire validation block is skipped with no errors.i18n/uk.toml (1)
75-75: LGTM - Translation key added correctly.The Ukrainian translation for "Login Captcha" has been added consistently with other locale files.
i18n/en.toml (1)
74-74: LGTM - English translation added correctly.The translation key is clear and appropriately positioned in the file.
i18n/fr.toml (1)
74-74: LGTM - French translation added correctly.The French translation "Captcha de connexion" is appropriate and consistently placed.
i18n/ja.toml (1)
74-74: LGTM - Japanese translation added correctly.The Japanese translation is appropriate and consistently positioned.
i18n/zh_cn.toml (1)
75-75: LGTM - Chinese translation added correctly.The Simplified Chinese translation "登录验证码" is appropriate and consistently placed.
src/controller/mod.rs (1)
532-533: LGTM - Field addition is correct.The new
login_captcha: Option<bool>field is properly added toSiteConfigwith appropriate validation skip. All three implementation points are confirmed:
SiteConfig::default()correctly initializes this field toNone(src/controller/admin.rs:384)- Admin interface template renders radio button controls with proper pattern matching, defaulting to "no" when unset (templates/admin.html:288-300)
- Admin handler processes this field correctly as part of the struct
Using
Option<bool>ensures backward compatibility with existing configurations.templates/signin.html (1)
31-43: LGTM - Conditional CAPTCHA rendering and form handling implemented correctly.The CAPTCHA elements are properly gated behind the
page_data.login_captchaflag in the template. The signin form handler insrc/controller/user.rscorrectly definescaptcha_idandcaptcha_valueasOption<String>fields, and the validation logic only executes whensite_config.login_captcha == Some(true), ensuring form submissions succeed without CAPTCHA data when the feature is disabled.src/controller/admin.rs (1)
384-384: LGTM!The default initialization of
login_captcha: Noneis consistent with other optional configuration fields and correctly disables the login captcha by default.src/controller/user.rs (1)
880-881: LGTM! Optional captcha fields align with conditional validation.Making
captcha_idandcaptcha_valueoptional correctly reflects that these fields are only required whenlogin_captchais enabled.templates/admin.html (1)
286-304: No duplicate field exists. The code shows a single login_captcha field with conditional rendering of the checked state. Lines 295-299 are not duplicates but rather two branches of a match expression: whenlogin_captchaisSome(true), the "yes" option is checked; otherwise, the "false" option is checked. This is the correct pattern for managing default radio button state based on config. No changes needed.
Summary by CodeRabbit
New Features
Improvements
Internationalization
✏️ Tip: You can customize this high-level summary in your review settings.