Restricted notification creation to administrators - #29754
Conversation
ref #29746 Editor roles only need to view and dismiss system notifications. Limiting creation to administrators closes an unnecessary permission gap.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
WalkthroughThe migration removes Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
|
It looks like this PR contains a migration 👀 General requirements
Schema changes
Data changes
|
|
| Command | Status | Duration | Result |
|---|---|---|---|
nx run ghost:test:ci:integration |
✅ Succeeded | 3m 13s | View ↗ |
nx run ghost:test:integration |
✅ Succeeded | 3m 26s | View ↗ |
nx run ghost:test:legacy |
✅ Succeeded | 2m 59s | View ↗ |
nx run ghost:test:e2e |
✅ Succeeded | 2m 49s | View ↗ |
nx run ghost-monorepo:lint:boundaries |
✅ Succeeded | 22s | View ↗ |
nx run-many -t test:unit -p ghost |
✅ Succeeded | 33s | View ↗ |
nx run-many --target=build --projects=tag:publi... |
✅ Succeeded | 1s | View ↗ |
nx run-many -t lint -p ghost,ghost-monorepo |
✅ Succeeded | 22s | View ↗ |
nx run @tryghost/admin:build |
✅ Succeeded | 4s | View ↗ |
💡 Verify your cache is correct by running tasks in a sandbox. Read docs ↗
☁️ Nx Cloud last updated this comment at 2026-08-04 16:06:25 UTC
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #29754 +/- ##
==========================================
- Coverage 75.53% 75.52% -0.02%
==========================================
Files 1612 1612
Lines 142066 142181 +115
Branches 17590 17582 -8
==========================================
+ Hits 107315 107377 +62
- Misses 33679 33759 +80
+ Partials 1072 1045 -27
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
EvanHahn
left a comment
There was a problem hiding this comment.
Troy and I both looked at this, and LGTU (looks good to us).
| }, | ||
| "Super Editor": { | ||
| "notification": "all", | ||
| "notification": ["browse", "destroy"], |
There was a problem hiding this comment.
question: do they still need read? (Same comment applies for Editor below.)
There was a problem hiding this comment.
There isn't a separate read action for notifications: GET /notifications is authorized by browse. Editors still need browse to render notifications and destroy to dismiss them per-user, so those two permissions are intentional.
no ref Staff tokens keep permission coverage focused and avoid coupling unrelated endpoint tests to session login rate limits.

What changed
notification:addfrom the Editor and Super Editor rolesnotification:browseandnotification:destroyso those roles can still view and dismiss notificationsWhy
Notifications are a system-wide administrative channel. Editor-tier roles need to view and dismiss them, but do not need to create them.
This PR is intentionally limited to permission changes. Notification HTML sanitization can be evaluated separately.
Thanks to @pptx704 for reporting this permission gap in #29746.
Validation
pnpm nx run ghost:test:unit— 7,710 tests passedpnpm nx run ghost:test:integration -- test/integration/migrations/migration.test.js— 4 tests passedpnpm nx run ghost:test:e2e -- test/e2e-api/admin/notifications.test.js— 9 tests passed