Add "Require 2FA" in Ghost Admin settings #22386
Conversation
WalkthroughThis pull request introduces a new configuration setting, Suggested reviewers
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 ESLint
ghost/admin/app/services/settings.jsOops! Something went wrong! :( ESLint: 8.44.0 Error: Failed to load parser '@babel/eslint-parser' declared in 'ghost/admin/.eslintrc.js': Cannot find module '@babel/eslint-parser'
apps/admin-x-framework/src/api/settings.tsOops! Something went wrong! :( ESLint: 8.44.0 ESLint couldn't find the plugin "eslint-plugin-react-hooks". (The package "eslint-plugin-react-hooks" was not found when loaded as a Node module from the directory "/apps/admin-x-framework".) It's likely that the plugin isn't installed correctly. Try reinstalling by running the following: The plugin "eslint-plugin-react-hooks" was referenced from the config file in "apps/admin-x-framework/.eslintrc.cjs". If you still can't figure out the problem, please stop by https://eslint.org/chat/help to chat with the team. apps/admin-x-settings/src/components/settings/general/Users.tsxOops! Something went wrong! :( ESLint: 8.44.0 ESLint couldn't find the plugin "eslint-plugin-react-hooks". (The package "eslint-plugin-react-hooks" was not found when loaded as a Node module from the directory "/apps/admin-x-settings".) It's likely that the plugin isn't installed correctly. Try reinstalling by running the following: The plugin "eslint-plugin-react-hooks" was referenced from the config file in "apps/admin-x-settings/.eslintrc.cjs". If you still can't figure out the problem, please stop by https://eslint.org/chat/help to chat with the team. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (13)
🚧 Files skipped from review as they are similar to previous changes (10)
⏰ Context from checks skipped due to timeout of 90000ms (15)
🔇 Additional comments (10)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
github-actions
bot
added
the
migration
|
It looks like this PR contains a migration 👀 General requirements
Schema changes
Data changes
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
apps/admin-x-settings/src/components/settings/general/Users.tsx (2)
301-323: Consider adding a loading state during settings updateThe toggle implementation for requiring email 2FA is functional, but lacks visual feedback during the settings update process. This could lead to confusion if there's server latency.
Consider adding a loading state to provide better user feedback:
{labs.staff2fa && ( <> <Separator /> <Toggle checked={require2fa} + disabled={isUpdating} direction='rtl' gap='gap-0' label='Require email two-factor authentication on every login' labelClasses='w-full' onChange={async () => { const newValue = !require2fa; + let isUpdating = true; try { await editSettings([{ key: 'require_email_mfa', value: newValue }]); } catch (error) { handleError(error); + } finally { + isUpdating = false; } }} /> + {isUpdating && <span className="ml-2 text-sm text-grey">Updating...</span>} </> )}
310-320: Consider adding a confirmation dialog for this security setting changeToggling this setting affects security for all users and forces them to verify on every login. Without a confirmation, an admin might toggle this accidentally without understanding the consequences.
Consider adding a confirmation dialog before applying the change:
onChange={async () => { const newValue = !require2fa; + // For enabling MFA, show a confirmation + if (newValue && !window.confirm('This will require all staff to verify their login with email 2FA every time they sign in. Are you sure you want to proceed?')) { + return; + } try { await editSettings([{ key: 'require_email_mfa', value: newValue }]); } catch (error) { handleError(error); } }}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
apps/admin-x-framework/src/test/responses/settings.json(1 hunks)apps/admin-x-settings/src/components/settings/general/Users.tsx(3 hunks)ghost/core/core/server/api/endpoints/utils/serializers/input/settings.js(1 hunks)ghost/core/core/server/api/endpoints/utils/serializers/input/utils/settings-key-group-mapper.js(1 hunks)ghost/core/core/server/api/endpoints/utils/serializers/input/utils/settings-key-type-mapper.js(1 hunks)ghost/core/core/server/data/migrations/versions/5.111/2025-03-03-15-05-38-add-require-mfa-setting.js(1 hunks)ghost/core/core/server/data/schema/default-settings/default-settings.json(1 hunks)ghost/core/core/shared/settings-cache/public.js(1 hunks)ghost/session-service/lib/session-service.js(1 hunks)ghost/session-service/test/SessionService.test.js(5 hunks)
🔇 Additional comments (18)
ghost/core/core/shared/settings-cache/public.js (1)
51-51: Clean Addition of therequire_email_mfaSettingThe new setting is correctly added to the list of public settings, making it accessible to the frontend components that will need to display or modify this configuration.
ghost/session-service/lib/session-service.js (1)
311-316: Well-Implemented MFA Session State ManagementThe code correctly checks the
require_email_mfasetting and removes the verified status when a user signs out, ensuring that 2FA will be required on the next login when this setting is enabled. This implementation aligns perfectly with the PR objective of requiring 2FA for every login attempt rather than just once per device.ghost/core/core/server/data/migrations/versions/5.111/2025-03-03-15-05-38-add-require-mfa-setting.js (1)
1-8: Clean and Proper Migration ImplementationThe migration file correctly uses the utility function
addSettingto add the newrequire_email_mfasetting with appropriate metadata:
- The default value is set to 'false', making it an opt-in feature
- The type is properly set as 'boolean'
- The setting is correctly categorized under the 'security' group
This follows the established patterns for adding new settings in the Ghost platform.
apps/admin-x-framework/src/test/responses/settings.json (1)
338-342: Test Settings Properly UpdatedThe new setting has been correctly added to the test responses, ensuring that tests will have access to the
require_email_mfaconfiguration with its default value offalse. This keeps the test environment in sync with the production settings.ghost/core/core/server/api/endpoints/utils/serializers/input/utils/settings-key-type-mapper.js (1)
57-58: Mapping added correctly for the new 2FA setting.The
require_email_mfasetting is correctly mapped to the 'boolean' type, which aligns with its implementation in the default settings schema.ghost/core/core/server/api/endpoints/utils/serializers/input/utils/settings-key-group-mapper.js (1)
50-51: Appropriate security categorization for the 2FA setting.The
require_email_mfasetting is appropriately mapped to the 'security' group, which correctly categorizes this authentication-related configuration.ghost/core/core/server/data/schema/default-settings/default-settings.json (1)
599-603: New security section with 2FA setting properly defined.The new
securitysection with therequire_email_mfasetting is properly defined with the appropriate default value (false) and type (boolean). This implementation aligns with the PR objective to make 2FA configurable at the admin level.One thing to note is that this setting will default to not requiring 2FA on every login (which is a reasonable default for backward compatibility), but admins can enable it through the settings UI.
ghost/core/core/server/api/endpoints/utils/serializers/input/settings.js (2)
78-79: Added setting to editable settings list.The
require_email_mfasetting is correctly added to theEDITABLE_SETTINGSarray, making it configurable through the API.This change connects the new setting to the rest of the system by making it editable via the Ghost Admin interface, fulfilling the PR objective.
1-130:✅ Verification successful
Verify session management and sign-out behavior with the new 2FA setting.
Based on the PR description, this setting should ensure that the
verifiedflag is removed upon signing out when the setting is enabled. While the setting itself is correctly implemented here, we should verify that the session service has been updated to check this setting during logout operations.
🏁 Script executed:
#!/bin/bash # Description: Check if the session service handles the require_email_mfa setting during sign-out # Look for code that handles the require_email_mfa setting in the session service echo "Checking for session service handling of require_email_mfa..." rg -A 5 -B 5 "require_email_mfa" --glob "ghost/session-service/**" # Look for sign-out or logout related functionality that might be affected echo "Checking for modified sign-out behavior..." rg -A 5 -B 5 "signOut|logout|sign-out|log-out" --glob "ghost/session-service/**"Length of output: 4569
Session Management 2FA Behavior Confirmed
After verifying the session service code:
- In
ghost/session-service/lib/session-service.js, the logout function checks therequire_email_mfasetting usinggetSettingsCache('require_email_mfa')and correctly resets thesession.verifiedflag when the setting is enabled.- The test files in
ghost/session-service/test/SessionService.test.jscover both cases (when MFA is enabled and disabled), ensuring the expected behavior during sign-out.No further changes are required.
apps/admin-x-settings/src/components/settings/general/Users.tsx (4)
6-6: Good addition of necessary UI componentsAdding
SeparatorandTogglecomponents from the design system is appropriate for creating the new 2FA setting UI.
10-10: Well-structured import of settings API methodsImporting
getSettingValueanduseEditSettingsfrom the framework API provides a clean approach to interact with the settings functionality.
278-283: Good implementation of settings access and state managementThe code correctly retrieves the current state of the
require_email_mfasetting and parses the labs configuration to check for feature availability. This follows the recommended pattern for settings management in Ghost.
308-308:❓ Verification inconclusive
Verify the label text with stakeholders
The label "Require email two-factor authentication on every login" is clear, but should be verified with stakeholders to ensure it matches the intended user experience and product terminology.
🌐 Web query:
Ghost email-based MFA terminologyError during web search
Action Required: Verify MFA Terminology Label with Stakeholders
The label
label='Require email two-factor authentication on every login'appears clear from a wording perspective. However, since our attempt to automatically verify "Ghost email-based MFA terminology" returned an error, please ensure that this label aligns with the intended product terminology by directly confirming with stakeholders.
ghost/session-service/test/SessionService.test.js (5)
17-21: Well-structured mock for the settings cacheThe
getSettingsCachespy correctly mocks the behavior of returning the require_email_mfa setting value.
166-170: Consistent approach to mocking settings valuesThe implementation maintains consistency with the first test in how the settings cache mock is structured.
582-643: Good test case for the new behavior with require_email_mfa enabledThis test case properly validates the new functionality: when the
require_email_mfasetting is enabled, theverifiedflag should be removed upon signing out. This ensures that 2FA is required for every login attempt, not just the first one per device.The test structure follows the same pattern as existing tests, making it easy to understand and maintain.
592-598: Consistent mock implementation for getSettingsCacheGood implementation of the settings cache mock that returns
truefor the 'require_email_mfa' key and 'site-title' for other keys. This matches how other tests in the file handle similar scenarios.
636-638: Critical verification for the core functionalityThese assertions are the key test for the new functionality: when
require_email_mfais enabled, removing a user from a session should also clear theverifiedflag. This differs from the existing behavior where theverifiedflag was maintained.The test correctly verifies that both
user_idandverifiedare undefined afterremoveUserForSessionis called.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
apps/admin-x-settings/src/components/settings/general/Users.tsx (3)
278-282: Consider making JSON parsing more robust.The code correctly retrieves and manages global settings, but there's a potential edge case when parsing the labs JSON string.
- const labs = JSON.parse(getSettingValue<string>(settings, 'labs') || '{}'); + const labsValue = getSettingValue<string>(settings, 'labs') || '{}'; + const labs = JSON.parse(labsValue);This change separates the retrieval and parsing steps, making the code more readable and avoiding potential issues if
getSettingValuereturnednullinstead of an empty string.
301-327: Consider adding loading state and success feedback to the toggle.The implementation of the 2FA security settings section is clean and well-structured, with proper conditional rendering based on the feature flag. However, there are a few user experience improvements that could be made.
The toggle doesn't show a loading state while the setting is being updated, which could lead to users clicking multiple times if there's any delay. Additionally, there's no success feedback when the setting is updated successfully.
Here's a suggested improvement:
+ const [isUpdating, setIsUpdating] = useState(false); <Toggle checked={require2fa} direction='rtl' gap='gap-0' + disabled={isUpdating} onChange={async () => { const newValue = !require2fa; try { + setIsUpdating(true); await editSettings([{ key: 'require_email_mfa', value: newValue }]); + showToast({ + title: newValue ? 'Email 2FA requirement enabled' : 'Email 2FA requirement disabled', + type: 'success' + }); } catch (error) { handleError(error); } finally { + setIsUpdating(false); } }} />This change adds:
- A loading state to prevent multiple clicks
- A success toast to provide feedback when the setting is updated
305-308: Enhance accessibility with detailed explanations.The UI text is clear but could benefit from additional context about what email 2FA means for administrators.
Consider expanding the description to provide more context:
<div className='flex flex-col'> <span className='text-[1.5rem] font-semibold tracking-tight'>Security settings</span> - <span>Enable email 2FA for all staff logins</span> + <span>Enable email 2FA for all staff logins. When enabled, staff members will need to verify their login via email every time they sign in, even on previously verified devices.</span> </div>This helps users understand the implications of enabling this setting.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/admin-x-settings/src/components/settings/general/Users.tsx(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Setup
🔇 Additional comments (1)
apps/admin-x-settings/src/components/settings/general/Users.tsx (1)
6-6: Good addition of necessary imports.The imports for UI components (
Separator,Toggle) and settings management hooks (getSettingValue,useEditSettings) are properly added to support the new 2FA functionality.Also applies to: 10-10
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/admin-x-settings/src/components/settings/general/Users.tsx (1)
301-327: Well-implemented conditional 2FA toggle UIThe 2FA toggle UI is properly implemented with:
- Conditional rendering based on the labs.staff2fa flag
- Clear labeling and description for the feature
- Proper toggle state management and error handling
- Asynchronous update of the setting when the toggle is changed
Consider using a more consistent naming convention - the setting is called 'require_email_mfa' but the variable is 'require2fa'. While functionally correct, consistent naming would improve maintainability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
apps/admin-x-framework/src/api/settings.ts(1 hunks)apps/admin-x-framework/src/test/responses/settings.json(1 hunks)apps/admin-x-settings/src/components/settings/general/Users.tsx(3 hunks)ghost/admin/app/services/settings.js(1 hunks)ghost/core/core/server/api/endpoints/utils/serializers/input/settings.js(1 hunks)ghost/core/core/server/api/endpoints/utils/serializers/input/utils/settings-key-group-mapper.js(1 hunks)ghost/core/core/server/api/endpoints/utils/serializers/input/utils/settings-key-type-mapper.js(1 hunks)ghost/core/core/server/data/migrations/versions/5.111/2025-03-03-15-05-38-add-require-mfa-setting.js(1 hunks)ghost/core/core/server/data/schema/default-settings/default-settings.json(1 hunks)ghost/session-service/lib/session-service.js(1 hunks)ghost/session-service/test/SessionService.test.js(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- apps/admin-x-framework/src/test/responses/settings.json
- ghost/core/core/server/api/endpoints/utils/serializers/input/utils/settings-key-type-mapper.js
- ghost/core/core/server/api/endpoints/utils/serializers/input/settings.js
- ghost/core/core/server/data/schema/default-settings/default-settings.json
- ghost/session-service/lib/session-service.js
- ghost/core/core/server/api/endpoints/utils/serializers/input/utils/settings-key-group-mapper.js
- ghost/core/core/server/data/migrations/versions/5.111/2025-03-03-15-05-38-add-require-mfa-setting.js
🔇 Additional comments (7)
apps/admin-x-framework/src/api/settings.ts (1)
34-34: Good addition of 'security' group to settings queryAdding the 'security' group to the defaultSearchParams ensures that security-related settings, particularly the new 'require_email_mfa' setting, are loaded when browsing settings.
ghost/admin/app/services/settings.js (1)
58-58: Good addition of 'security' group to queryRecordIncluding the 'security' group in the settings query ensures the Ember admin application can load and modify security-related settings, particularly the new require_email_mfa setting.
apps/admin-x-settings/src/components/settings/general/Users.tsx (2)
6-6: Appropriate UI component imports for 2FA toggleThe addition of
SeparatorandTogglecomponents from the design system, along with the necessary settings API functions, provides the UI elements needed for the 2FA requirement feature.Also applies to: 10-10
278-282: Properly retrieves and prepares settings for 2FA toggleGood implementation that retrieves the global settings, extracts the 'require_email_mfa' setting with a default value of false, and sets up the editSettings mutation for updating the setting.
ghost/session-service/test/SessionService.test.js (3)
17-21: Good test setup for settings cacheThe implementation of the getSettingsCache spy function properly mocks the settings retrieval for testing purposes, returning false for the 'require_email_mfa' setting.
Also applies to: 32-32
166-170: Consistent settings spy setup across testsGood consistency in implementing the getSettingsCache spy function across multiple test cases, ensuring proper test isolation.
Also applies to: 181-181
582-643: Comprehensive test for verified session removal with 2FA enabledThis test case properly verifies that when the 'require_email_mfa' setting is enabled:
- A session can be created for a user
- The session can be verified, setting the 'verified' flag to true
- When the user is removed from the session, both the user_id and verified properties are cleared
- When the session is recreated, the verified property remains undefined
This ensures that 2FA is required for every login attempt when the setting is enabled, not just once per device.
|
Deployed to staging with ID: |
1 similar comment
|
Deployed to staging with ID: |
|
Deployed to staging with ID: |
ref ENG-2072
This adds the setting to make signing out also remove the
verifiedflag. This effectively ensures that 2FA is required on every login, not just once per device.