Updated 2FA to clarify different modes #22817
Conversation
WalkthroughThe changes update the error handling for two-factor authentication in both the admin AJAX service and the core session middleware. In the AJAX service, the function formerly checking against a single error code now uses an array of two-factor authentication codes and calls a helper to correctly parse JSON payloads. In the session middleware, the error code is chosen dynamically based on whether verification is required, and additional context is provided with the error message. Corresponding unit, end-to-end, and integration tests have been updated to use async/await syntax, verify the new error codes, and simulate the necessary service behavior. These adjustments ensure the system distinguishes between a generic two-factor token requirement and a new device detection scenario. Possibly related PRs
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/ajax.jsOops! Something went wrong! :( ESLint: 8.57.1 Error: Failed to load parser '@babel/eslint-parser' declared in 'ghost/admin/.eslintrc.js': Cannot find module '@babel/eslint-parser'
ghost/admin/tests/integration/services/ajax-test.jsOops! Something went wrong! :( ESLint: 8.57.1 Error: Failed to load parser '@babel/eslint-parser' declared in 'ghost/admin/.eslintrc.js': Cannot find module '@babel/eslint-parser'
Tip ⚡💬 Agentic Chat (Pro Plan, General Availability)
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (5)
⏰ Context from checks skipped due to timeout of 90000ms (12)
🪧 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 (
|
ErisDS
force-pushed
the
2fa-admin-errors
branch
2 times, most recently
from
6804d87 to
a22fadd
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
ghost/admin/app/controllers/signin.js (1)
60-66: Enhanced 2FA error handling approachThis change improves the system's ability to handle different types of 2FA errors by capturing the specific error code from the payload and storing it in the session before transitioning. This will allow more specific handling of different 2FA scenarios downstream.
Consider adding a fallback in case the error structure doesn't match expectations:
-let errorCode = error.payload?.errors[0]?.code; +let errorCode = error.payload?.errors?.[0]?.code || '2FA_TOKEN_REQUIRED';
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
ghost/core/test/e2e-api/admin/__snapshots__/session.test.js.snapis excluded by!**/*.snap
📒 Files selected for processing (5)
ghost/admin/app/controllers/signin-verify.js(1 hunks)ghost/admin/app/controllers/signin.js(1 hunks)ghost/admin/app/templates/signin-verify.hbs(2 hunks)ghost/admin/tests/unit/components/editor/modals/re-authenticate-test.js(6 hunks)ghost/core/test/e2e-api/admin/session.test.js(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- ghost/admin/tests/unit/components/editor/modals/re-authenticate-test.js
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Setup
🔇 Additional comments (3)
ghost/core/test/e2e-api/admin/session.test.js (1)
107-107: Error code change from2FA_TOKEN_REQUIREDto2FA_NEW_DEVICE_DETECTEDlooks appropriate.The updated error code provides more specific context about why 2FA verification is required (new device detection) rather than just indicating that a token is needed. This change aligns with the PR objective of enhancing error handling for different 2FA scenarios.
Also applies to: 136-136
ghost/admin/app/templates/signin-verify.hbs (2)
7-7: Effective conditional message based on authentication contextThe conditional header text properly differentiates between standard verification and 2FA confirmation contexts.
27-27: Security improvement for input placeholderChanging the placeholder from a numeric example to asterisks is a security improvement that avoids revealing the expected input format.
ErisDS
force-pushed
the
2fa-admin-errors
branch
2 times, most recently
from
fba46eb to
4c4a90b
Compare
ErisDS
force-pushed
the
2fa-admin-errors
branch
4 times, most recently
from
1e0a4fe to
91ad6d8
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
ghost/admin/tests/unit/components/editor/modals/re-authenticate-test.js (1)
1-91: Consider adding tests for the new device detection error case.While the first test verifies that
2FA_NEW_DEVICE_DETECTEDis correctly identified as a 2FA error, the second test only tests the modal behavior with2FA_TOKEN_REQUIRED. Consider adding another test or extending the current one to verify that the re-verify modal is also opened when encountering a2FA_NEW_DEVICE_DETECTEDerror.it('calls re-verify modal for new device detection 2FA error', async function () { const modals = this.owner.lookup('service:modals'); const openSpy = sinon.spy(modals, 'open'); const handleError = async (error) => { if (isTwoFactorTokenRequiredError(error)) { await modals.open('editor/modals/re-verify'); return true; } return false; }; const newDeviceError = { payload: { errors: [{ code: '2FA_NEW_DEVICE_DETECTED' }] } }; await handleError(newDeviceError); expect(openSpy.calledOnce, 're-verify modal called once for new device error').to.be.true; expect(openSpy.firstCall.args[0]).to.equal('editor/modals/re-verify'); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
ghost/core/test/e2e-api/admin/__snapshots__/session.test.js.snapis excluded by!**/*.snap
📒 Files selected for processing (7)
ghost/admin/app/services/ajax.js(1 hunks)ghost/admin/tests/integration/services/ajax-test.js(1 hunks)ghost/admin/tests/unit/components/editor/modals/re-authenticate-test.js(1 hunks)ghost/core/core/server/services/auth/session/middleware.js(1 hunks)ghost/core/core/server/services/auth/session/session-service.js(2 hunks)ghost/core/test/e2e-api/admin/session.test.js(2 hunks)ghost/core/test/unit/server/services/auth/session/middleware.test.js(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- ghost/core/core/server/services/auth/session/middleware.js
- ghost/core/test/e2e-api/admin/session.test.js
- ghost/core/core/server/services/auth/session/session-service.js
- ghost/admin/app/services/ajax.js
- ghost/core/test/unit/server/services/auth/session/middleware.test.js
- ghost/admin/tests/integration/services/ajax-test.js
🧰 Additional context used
🧬 Code Graph Analysis (1)
ghost/admin/tests/unit/components/editor/modals/re-authenticate-test.js (1)
ghost/admin/app/services/ajax.js (1)
isTwoFactorTokenRequiredError(202-211)
🔇 Additional comments (3)
ghost/admin/tests/unit/components/editor/modals/re-authenticate-test.js (3)
1-14: Good setup with proper testing dependencies and service stub.The file correctly imports necessary testing utilities and sets up a clean ModalsStub implementation that returns a Promise, mimicking the actual service behavior.
15-21: Well-structured test suite with proper service registration.The test suite is properly configured with
setupTest()and correctly registers the stub service before each test, following best practices for Ember testing.
66-90: Good test for modal invocation with proper spy usage.This test effectively verifies that the error handler opens the re-verify modal exactly once for 2FA errors. The spy is properly set up to monitor the modal's open method, and the assertions correctly verify both that it was called once and with the expected modal name.
ghost/admin/tests/unit/components/editor/modals/re-authenticate-test.js
Outdated
Show resolved
Hide resolved
ErisDS
force-pushed
the
2fa-admin-errors
branch
from
91ad6d8 to
e34e369
Compare
- We're going to be diversifying the error codes returned from the auth middleware, depending on different cases - This makes sure that ghost admin will still handle the cases correctly
ErisDS
force-pushed
the
2fa-admin-errors
branch
from
e34e369 to
fda1594
Compare
ErisDS
changed the title
ref https://linear.app/ghost/issue/ENG-2074/adjust-verification-copy-if-2fa-is-enabled-for-all-logins