Change authentication for self-service migrations to send API key instead of JWT#24462
Change authentication for self-service migrations to send API key instead of JWT#24462PaulAdamDavis merged 3 commits intomainfrom
Conversation
WalkthroughThe changes involve renaming and simplifying the method responsible for retrieving an API credential in the Estimated code review effort2 (~20 minutes) 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/migrate.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'
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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). (1)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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
![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 (1)
ghost/admin/app/services/migrate.js (1)
3-3: Remove unused import.The
SignJWTimport from 'jose' is no longer used since JWT generation was removed from theapiKeymethod.-import {SignJWT} from 'jose';
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between caa1802620761c22fb61812052dc162ca02c3ccb and f99b47f3f21296e7e706af66081b661073c2ffeb.
📒 Files selected for processing (2)
ghost/admin/app/services/migrate.js(1 hunks)ghost/admin/tests/unit/services/migrate-test.js(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
ghost/admin/tests/unit/services/migrate-test.js (3)
Learnt from: ErisDS
PR: #23588
File: ghost/core/test/e2e-api/admin/backup.test.js:136-148
Timestamp: 2025-05-29T10:37:26.369Z
Learning: In the Ghost test framework, when testing CSV exports via the admin API, the response body field is empty while the actual CSV data is provided through the text field. Tests should use expectEmptyBody() and then validate the CSV content via .expect(({text}) => ...) - this is not contradictory behavior.
Learnt from: mike182uk
PR: #22471
File: apps/admin-x-activitypub/src/utils/pending-activity.ts:13-71
Timestamp: 2025-03-13T09:00:20.205Z
Learning: The pending activity utilities in Ghost's ActivityPub module are thoroughly tested in apps/admin-x-activitypub/test/unit/utils/pending-activity.ts, which covers generatePendingActivityId, isPendingActivity, and generatePendingActivity functions.
Learnt from: mike182uk
PR: #22471
File: apps/admin-x-activitypub/src/utils/pending-activity.ts:13-71
Timestamp: 2025-03-13T09:00:20.205Z
Learning: The pending activity utilities in the Ghost ActivityPub module are covered by tests in the file apps/admin-x-activitypub/test/unit/utils/pending-activity.ts.
🔇 Additional comments (5)
ghost/admin/app/services/migrate.js (2)
31-42: LGTM! Simplified API key retrieval aligns with PR objectives.The method correctly retrieves and returns the raw API key instead of generating a JWT. This addresses the 5-minute JWT expiration limitation mentioned in the PR objectives and allows migrations more time to process.
45-50: LGTM! Payload correctly updated to use raw API key.The changes properly update the method call from
apiToken()toapiKey()and the payload property fromapiTokentoapiKey, maintaining consistency with the authentication method change.ghost/admin/tests/unit/services/migrate-test.js (3)
29-29: LGTM! Test stub correctly updated to match service method rename.The stub properly reflects the change from
apiTokentoapiKeymethod in the service.
41-41: LGTM! Payload property assertion updated correctly.The test assertion properly checks for the new
apiKeyproperty instead ofapiToken, maintaining alignment with the service payload structure.
44-44: LGTM! Value assertion updated to match new property name.The assertion correctly validates
payload.apiKeyinstead ofpayload.apiToken, ensuring test coverage remains intact after the authentication method change.
ErisDS
force-pushed
the
6.x
branch
2 times, most recently
from
7b135d2 to
336f1e7
Compare
…tead of JWT ref https://linear.app/ghost/issue/CON-372/change-ghost-self-service-migrations-app-auth - The self-service migrations app is currently authenticated with a JWT, but the expire time for these is capped at 5 minutes in the (Admin API key auth service)[https://github.com/TryGhost/Ghost/blob/main/ghost/core/core/server/services/auth/api-key/admin.js] - I wasn't aware of this in #24119, making that change ineffective - The API key used to make the JWT is already available to privileged users of Ghost Admin - Changing the migrate service to send the API key rather than a JWT (and handling that in the self-service app) yields the same result; allowing migraitons more time to process, improving the end result
PaulAdamDavis
force-pushed
the
migrate-auth
branch
from
b6a22bc to
4f1b2ac
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #24462 +/- ##
=======================================
Coverage 71.55% 71.55%
=======================================
Files 1529 1529
Lines 114778 114775 -3
Branches 13833 13833
=======================================
+ Hits 82127 82130 +3
- Misses 31631 31643 +12
+ Partials 1020 1002 -18
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Calling postMessage with '*' as the origin makes me feel a little uncomfortable, especially when sending an API key
I see we're passing '*' when updating the route info, but I can't see where this is happening when sending the key
We should restrict the postMessage call to the origin of the migrate service, this ensures that data can only ever be sent there, and not to a different site
There's a small note about this in the MDN docs here https://developer.mozilla.org/en-US/docs/Web/API/Window/postMessage#targetorigin
|
@allouis Good call. I have updated the The payload that contains the API key is sent to the iframe here https://github.com/TryGhost/Ghost/blob/main/ghost/admin/app/components/gh-migrate-iframe.js#L65 and is untouched in this PR. The origin is These are the only 2 places where data is sent. On the receiving end, we check the origin here https://github.com/TryGhost/Ghost/blob/main/ghost/admin/app/components/gh-migrate-iframe.js#L30-L31 |
3 participants
ref https://linear.app/ghost/issue/CON-372/change-ghost-self-service-migrations-app-auth
josedependency