Conversation
📝 WalkthroughWalkthroughThis PR introduces a conditional login CAPTCHA feature across the application. It adds translation keys in five languages, a new boolean config flag to enable/disable CAPTCHA, passes the setting through the page rendering pipeline, makes captcha form fields optional, implements conditional captcha validation in the signin handler, and updates the admin panel and signin template to support the feature. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant SigninForm as Signin Form
participant SigninHandler as Signin Handler
participant Config as SiteConfig
participant CaptchaService as Captcha Service
participant AuthService as Auth Service
User->>SigninForm: Submit credentials + optional captcha
SigninForm->>SigninHandler: POST /signin
SigninHandler->>Config: Fetch login_captcha setting
alt login_captcha enabled
SigninHandler->>SigninHandler: Validate captcha_id and captcha_value present
SigninHandler->>CaptchaService: Verify captcha token
CaptchaService-->>SigninHandler: Captcha valid/invalid
alt Captcha invalid
SigninHandler-->>SigninForm: Return error
else Captcha valid
SigninHandler->>AuthService: Validate username & password
end
else login_captcha disabled
SigninHandler->>AuthService: Validate username & password
end
alt Auth successful
AuthService-->>SigninHandler: Auth token
SigninHandler-->>User: Redirect to dashboard
else Auth failed
AuthService-->>SigninHandler: Auth error
SigninHandler-->>SigninForm: Return error
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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/user.rs (2)
928-939: LGTM! Conditional CAPTCHA validation is correctly implemented.The logic properly checks
site_config.login_captchaand only validates the CAPTCHA when enabled. The early unwrapping ofOptionfields withok_or(AppError::CaptchaError)provides clear error handling.Security note: When
login_captchais disabled, the signin endpoint has no CAPTCHA protection against brute force attacks. However, the existing 1-second sleep on failed attempts (line 963) provides some mitigation. Consider documenting this trade-off for administrators in the UI or docs.
896-924: Minor inefficiency: CAPTCHA always generated regardless of setting.The
signinGET handler always creates a CAPTCHA session (line 915) even whenlogin_captchamight be disabled. This is slightly wasteful but likely necessary for template compatibility, as the template expectscaptcha_idandcaptcha_imagefields to exist.If this causes performance concerns, consider conditionally generating the CAPTCHA based on
site_config.login_captchaand updating the template to handle missing fields gracefully.
📜 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
🧬 Code graph analysis (1)
src/controller/user.rs (3)
src/controller/mod.rs (1)
get(539-549)src/controller/admin.rs (1)
default(363-386)src/config.rs (1)
default(53-66)
🔇 Additional comments (13)
i18n/fr.toml (1)
74-74: LGTM!The French translation for the login_captcha key is correct and consistent with the other locale additions across the PR.
i18n/ja.toml (1)
74-74: LGTM!The Japanese translation for the login_captcha key is appropriate and aligns with the feature implementation.
i18n/zh_cn.toml (1)
75-75: LGTM!The Chinese translation for the login_captcha key is accurate and consistent with the multilingual support for this feature.
i18n/en.toml (1)
74-74: LGTM!The English translation key is correctly added and serves as the base for the other locale translations.
i18n/uk.toml (1)
75-75: LGTM!The Ukrainian translation for the login_captcha key is correct and completes the multilingual support for this feature.
src/controller/meta_handler.rs (2)
171-171: LGTM!The
login_captchafield addition toPageData<'a>withpub(super)visibility is appropriate for exposing the configuration to templates within the controller module.
198-198: LGTM!The initialization of
login_captchafromsite_config.login_captchacorrectly propagates the configuration setting to the page data for template rendering.templates/admin.html (1)
286-303: LGTM!The admin UI implementation for the
login_captchaconfiguration correctly uses conditional rendering to ensure the appropriate radio button is checked based on the currentsite_config.login_captchavalue. The form will submit string values ("true" or "false"), which the backend should parse appropriately.templates/signin.html (1)
31-43: Backend correctly handles optional CAPTCHA fields.The FormSignin struct properly defines
captcha_idandcaptcha_valueasOption<String>fields, allowing them to be absent when CAPTCHA is disabled. The signin_post handler conditionally validates these fields only whensite_config.login_captchais enabled, using.ok_or()to enforce their presence in that case. The template's conditional rendering andrequiredattribute are correctly aligned with this backend behavior.src/controller/admin.rs (1)
362-386: LGTM! Default initialization is correct.The
login_captchafield is properly initialized tofalsein theDefaultimplementation, making the login CAPTCHA feature opt-in. This ensures backward compatibility for existing deployments.src/controller/mod.rs (1)
532-534: LGTM! Configuration field is well-defined.The
login_captchafield is properly documented and follows the established pattern forSiteConfigfields. The#[validate(skip)]attribute is appropriate for a boolean configuration flag.src/controller/user.rs (2)
880-881: LGTM! Optional fields enable conditional CAPTCHA flow.Changing
captcha_idandcaptcha_valuetoOption<String>correctly supports the conditional CAPTCHA feature. Whenlogin_captchais disabled, these fields won't be present in the form submission.
941-944: Excellent security improvement: prevents username enumeration.Changing the error from
AppError::NotFoundtoAppError::WrongPasswordwhen username lookup fails prevents attackers from determining which usernames exist in the system. This is a security best practice for authentication flows.
Summary by CodeRabbit
New Features
Localization
✏️ Tip: You can customize this high-level summary in your review settings.