✨ Give editors the ability to manage members and moderate comments #22310
Conversation
|
Warning Rate limit exceeded@cathysarisky has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 7 minutes and 3 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughThis pull request refines the access control and role-based permissions related to member management features. The navigation component’s condition is updated to use a new helper that checks if a user can manage members, replacing the previous admin-based check. Several routes have been modified to extend a base class that accommodates authenticated users, with added pre-navigation logic that redirects users who lack either editor or admin roles. The permission fixtures and migration scripts are updated to grant the Editor role a broader set of member-related permissions. Additionally, related tests—including unit, integration, and acceptance tests—are enhanced to verify these new permission scenarios. Minor updates to UI components provide more detailed role descriptions. Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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 (4)
ghost/admin/app/helpers/gh-user-can-manage-members.js (1)
3-7: Comment clarity and typo issuesThe helper comments contain some unclear phrasing and a typo that should be fixed.
-// Handlebars Helper {{gh-user-can-manage-members}} group users by admin and owner using if, or group them author using unless -// Usage: call helper as with aparameter of session.user +// Handlebars Helper {{gh-user-can-manage-members}} determines if a user can manage members +// Usage: call helper with a parameter of session.userghost/admin/app/routes/members.js (1)
22-26: Consider centralizing role checks for maintainabilityThe new conditional check restricts route access to admins or editors, which is correct per your requirements. However, if multiple places need the same check (e.g., a
gh-user-can-manage-membershelper), referencing a shared helper or service can reduce duplication and ease future updates (e.g., if another role gains member access).ghost/admin/app/routes/member.js (1)
7-7: Consistent class naming but watch for mismatchRenaming this to
MemberRouteor ensuring the route name matches the file context might help clarity, but it's purely a style preference.ghost/core/test/integration/migrations/migration.test.js (1)
199-203: Fix lint error for consistent quote styleESLint warns about the inconsistent use of double quotes at line 202. To maintain single quotes throughout JavaScript code, apply this fix:
- permissions.should.havePermission('Add Members', ['Administrator', 'Admin Integration', 'Self-Serve Migration Integration', "Editor"]); + permissions.should.havePermission('Add Members', ['Administrator', 'Admin Integration', 'Self-Serve Migration Integration', 'Editor']);🧰 Tools
🪛 ESLint
[error] 202-202: Strings must use singlequote.
(quotes)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
ghost/admin/app/components/gh-nav-menu/main.hbs(1 hunks)ghost/admin/app/helpers/gh-user-can-manage-members.js(1 hunks)ghost/admin/app/routes/member.js(2 hunks)ghost/admin/app/routes/members.js(2 hunks)ghost/core/core/server/data/migrations/versions/5.111/2025-03-02-16-30-00-add-member-permissions-to-editors.js(1 hunks)ghost/core/core/server/data/schema/fixtures/fixtures.json(1 hunks)ghost/core/test/integration/migrations/migration.test.js(1 hunks)
🧰 Additional context used
🪛 ESLint
ghost/core/test/integration/migrations/migration.test.js
[error] 202-202: Strings must use singlequote.
(quotes)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: Regression tests (Node 20.11.1, mysql8)
- GitHub Check: Database tests (Node 20.11.1, sqlite3)
- GitHub Check: Database tests (Node 22.13.1, mysql8)
- GitHub Check: Ghost-CLI tests
- GitHub Check: Database tests (Node 20.11.1, mysql8)
- GitHub Check: Database tests (Node 18.12.1, mysql8)
- GitHub Check: Admin tests - Chrome
🔇 Additional comments (9)
ghost/core/core/server/data/migrations/versions/5.111/2025-03-02-16-30-00-add-member-permissions-to-editors.js (1)
1-23: Well-structured migration to add member permissions to EditorsThis migration correctly adds all the necessary member-related permissions to the Editor role. Using
combineTransactionalMigrationsensures that all permissions are added as a single atomic transaction, which is a good practice for data consistency.ghost/admin/app/components/gh-nav-menu/main.hbs (1)
135-135: Permission check correctly updated for the Members navigation itemThe change from
gh-user-can-admintogh-user-can-manage-membersproperly implements the new permission model, allowing both admins and editors to access the Members section.ghost/core/core/server/data/schema/fixtures/fixtures.json (1)
937-940: Appropriate permission additions for Editor roleThe new permissions added to the Editor role in the fixtures file align with the migration script and properly grant editors the ability to manage members and read signin URLs. This ensures that new installations will have the correct permissions from the start.
ghost/admin/app/helpers/gh-user-can-manage-members.js (2)
8-10: Well-implemented permission check logicThe function correctly checks if the user is either an admin or editor, which aligns with the permission model changes in the migration and fixtures files.
12-14: Properly exported Ember helperThe helper is correctly exported following Ember's conventions, making it usable in Handlebars templates throughout the application.
ghost/admin/app/routes/members.js (2)
1-2: Transitioning from AdminRoute to AuthenticatedRouteRemoving the
AdminRouteimport and usingAuthenticatedRouteis consistent with allowing broader but still authenticated access. This aligns with the new strategy of letting editors as well as admins manage members.
6-6: Class now extends AuthenticatedRouteExtending
AuthenticatedRouteprovides a foundation for more granular role checks while maintaining an authenticated context. This is a clean approach given the new permissions model.ghost/admin/app/routes/member.js (2)
2-2: Refined import for improved access controlUsing
AuthenticatedRouteinvites more flexible, role-based gating thanAdminRoute. Keep an eye on calls that may still assume admin privileges explicitly.
25-33: Role-based gating logic is on targetThe
beforeModelhook prevents unauthorized users (non-admin, non-editor) from accessing the route, which correctly aligns with the new permissions approach.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
ghost/admin/app/routes/members-activity.js (1)
4-11: Role-based access control implemented correctly.The
beforeModelhook properly implements the permission check to allow both editors and admins to access this route while redirecting other users. This aligns with the PR objective of giving editors member management capabilities.However, consider adding error handling for potential edge cases where
session.usermight be undefined or have an unexpected structure.beforeModel() { super.beforeModel(...arguments); // give editors the ability to reach this route also. - if (!this.session.user.isEditor && !this.session.user.isAdmin) { + if (!this.session?.user?.isEditor && !this.session?.user?.isAdmin) { return this.transitionTo('home'); } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
ghost/admin/app/routes/member.js(2 hunks)ghost/admin/app/routes/members-activity.js(1 hunks)
🔇 Additional comments (4)
ghost/admin/app/routes/members-activity.js (1)
1-3: Inheritance changed from AdminRoute to AuthenticatedRoute.This is a good change to support the broader permission model enhancement that gives editors access to member management. The route now inherits from a more permissive base class while implementing custom access control.
ghost/admin/app/routes/member.js (3)
2-2: Import changed from AdminRoute to AuthenticatedRoute.This change aligns with the goal of providing editors access to member management functionality by using a less restrictive base route class.
7-7: Class renamed and inheritance changed.The class has been renamed from
MembersRoutetoMemberRoute(plural to singular) and now extendsAuthenticatedRoute. This better reflects that this route handles a single member and follows the same permission pattern implemented in other routes.
82-107:✅ Verification successful
Verify willTransition behavior with the route inheritance change.
Since the route inheritance has changed from AdminRoute to AuthenticatedRoute, confirm that the transition handling in this method still behaves as expected, particularly regarding any behaviors that might have been inherited from AdminRoute.
🏁 Script executed:
#!/bin/bash # Look for similar transition handling in other routes and check for differences between AdminRoute and AuthenticatedRoute echo "Checking AdminRoute implementation for transition handling..." rg "willTransition" "ghost/admin/app/routes/admin.js" -A 10 echo "Checking AuthenticatedRoute implementation for transition handling..." rg "willTransition" "ghost/admin/app/routes/authenticated.js" -A 10 echo "Checking for similar patterns in other routes that extend AuthenticatedRoute..." fd -t f "\.js$" --full-path "ghost/admin/app/routes" --exec grep -l "extends AuthenticatedRoute" {} \; | xargs rg "willTransition" -lLength of output: 842
Will Transition Behavior Consistency Confirmed
I verified that the transition logic in
ghost/admin/app/routes/member.jsaligns with the similar implementations in other routes (e.g.tag.js,pro.js, andexplore/index.js) extendingAuthenticatedRoute. Comparisons with theAdminRouteimplementation did not reveal any discrepancies that would affect thewillTransitionbehavior. It appears the inheritance change fromAdminRoutetoAuthenticatedRoutedoes not impact the expected functionality of this method.
- Verified similar handling in routes extending
AuthenticatedRoute.- Confirmed consistent transition logic across implementations.
- No further modifications are required regarding this behavior.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
ghost/core/core/server/data/migrations/versions/5.111/2025-03-02-16-30-00-add-member-permissions-to-editors.js (2)
1-2: Consider adding a brief documentation header for the migration.While the filename is descriptive, adding a simple JSDoc or comment describing the purpose and scope of this migration would improve maintainability.
const {addPermissionToRole, combineTransactionalMigrations} = require('../../utils'); +/** + * Migration to grant member management capabilities to Editors. + * This expands Editors' permissions beyond content management to include + * member operations and related features like offers and signin URLs. + */ module.exports = combineTransactionalMigrations(
1-1:❓ Verification inconclusive
Future date in filename may affect migration sequencing.
The filename contains the date 2025-03-02, which is in the future. Check if this is intentional, as it could affect when this migration is executed in the sequence.
🏁 Script executed:
#!/bin/bash # Description: Check migration naming conventions in the codebase # List recent migration files to compare naming patterns ls -la ghost/core/core/server/data/migrations/versions/5.111/ echo "------" # Check other migration directories to see the date patterns used ls -la ghost/core/core/server/data/migrations/versions/ | grep -v "5.111"Length of output: 6419
Action Required: Verify Future Timestamp in Migration Filename
- The migration file
ghost/core/core/server/data/migrations/versions/5.111/2025-03-02-16-30-00-add-member-permissions-to-editors.jsis using a date (2025-03-02) that could be interpreted as a future timestamp relative to some environments.- Please confirm that this date is intentional and that it won’t disrupt the sequential execution of migrations, especially in production where ordering may rely on filename dates.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
ghost/core/core/server/data/migrations/versions/5.111/2025-03-02-16-30-00-add-member-permissions-to-editors.js(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (14)
- GitHub Check: Regression tests (Node 20.11.1, sqlite3)
- GitHub Check: Regression tests (Node 20.11.1, mysql8)
- GitHub Check: Ghost-CLI tests
- GitHub Check: Unit tests (Node 22.13.1)
- GitHub Check: Admin-X Settings tests
- GitHub Check: Unit tests (Node 20.11.1)
- GitHub Check: Database tests (Node 20.11.1, sqlite3)
- GitHub Check: Unit tests (Node 18.12.1)
- GitHub Check: Database tests (Node 22.13.1, mysql8)
- GitHub Check: Database tests (Node 20.11.1, mysql8)
- GitHub Check: Database tests (Node 18.12.1, mysql8)
- GitHub Check: Lint
- GitHub Check: i18n
- GitHub Check: Admin tests - Chrome
🔇 Additional comments (3)
ghost/core/core/server/data/migrations/versions/5.111/2025-03-02-16-30-00-add-member-permissions-to-editors.js (3)
31-34:✅ Verification successful
Ensure consistency with other signin URL-related permissions.
The 'Read member signin urls' permission is included, but check if there are other related permissions like 'Browse member signin urls' that might also be needed for complete functionality.
🏁 Script executed:
#!/bin/bash # Description: Check for other signin URL-related permissions that might need to be included # Search for all signin URL-related permissions rg -i "permission.*signin" --type js rg -i "signin.*permission" --type js # Check which roles have these permissions in fixtures fd -e json fixtures | xargs grep -i "signin" | grep permissionLength of output: 747
Consistent signin URL permissions detected across the codebase.
Our search confirms that both migration files and tests consistently use the
"Read member signin urls"permission. No additional related permissions (like"Browse member signin urls") were found. Please verify that relying solely on the"Read member signin urls"permission meets the intended functionality.
23-30:✅ Verification successful
Verify if Editors need additional offer permissions.
The migration grants 'Read offers' and 'Browse offers' permissions, but not 'Add offers', 'Edit offers', or 'Delete offers'. Confirm if this limited set of permissions is intentional or if Editors should have complete offer management capabilities.
🏁 Script executed:
#!/bin/bash # Description: Check all existing offer-related permissions to determine the complete set # Find all offer-related permissions in the codebase rg -i "permission.*offer" --type js # Check Admin role permissions for comparison fd -e json fixtures | xargs grep -i "offer" | grep -i "admin" | grep permissionLength of output: 1603
Confirming Intentional Permission Granularity
After verifying the codebase, it appears that the Editors migration (in ghost/core/core/server/data/migrations/versions/5.111/2025-03-02-16-30-00-add-member-permissions-to-editors.js) intentionally grants only the "Read offers" and "Browse offers" permissions. In contrast, the earlier migration for Admin Integration (from version 4.40) assigns additional permissions ("Add offers" and "Edit offers"). Moreover, the integration tests confirm that Editors are only expected to have read and browse access, while administrative roles receive the enhanced offer management capabilities.No changes are required based on these findings.
1-35:✅ Verification successful
Comprehensive permission set for Editor role management of members.
This migration script effectively adds all the necessary permissions to enable Editors to manage members, which aligns perfectly with the PR objective. The use of
combineTransactionalMigrationsis excellent as it ensures atomicity - either all permissions are added or none are.
🏁 Script executed:
#!/bin/bash # Description: Check for any other member-related permissions that might be missing from this migration # Search for member-related permissions in the codebase rg -i "permission.*member" --type js # Check existing permissions in fixtures to ensure we didn't miss any fd -e json fixtures | xargs grep -i "member" | grep permissionLength of output: 9384
Migration script validated: Comprehensive permissions successfully applied to the Editor role.
The migration script in
ghost/core/core/server/data/migrations/versions/5.111/2025-03-02-16-30-00-add-member-permissions-to-editors.js
correctly adds a full set of permissions—including member management (Browse Members,Read Members,Edit Members,Add Members,Delete Members,Read member signin urls) along with the additional offers permissions (Read offers,Browse offers)—to the Editor role. Verification via our codebase search confirms that all expected member-related permissions are present and no additional ones appear to be missing. The use ofcombineTransactionalMigrationsensures that the updates are applied atomically.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
ghost/core/test/regression/api/admin/members-signin-url.test.js (2)
81-83: Appropriate test role adjustment to reflect new permissions model.This change correctly updates the test to use an Author role instead of an Editor role for the "non-Owner and non-Admin" test case. Since the PR is granting Editors the ability to manage members and access signin URLs, using an Author role (roles[2]) for testing the negative permission case is necessary to maintain the test's intent of verifying that users without proper permissions cannot access these URLs.
The test structure will properly verify the access control boundaries after this change, ensuring that:
- Owners and Admins have access (as tested in previous sections)
- Authors do not have access (this test case)
- Editors will now have access (which should be covered in a separate test)
Consider adding a new test section "As Editor" to explicitly verify that Editors now have the expected permissions to access member signin URLs.
72-91: Test description should be updated to better reflect the roles being tested.While the test implementation is correct, the description "As non-Owner and non-Admin" is now less specific than it could be. With the new permission model, it's important to be explicit about which roles are being tested.
Consider updating the test description to be more specific:
-describe('As non-Owner and non-Admin', function () { +describe('As Author (without member management permissions)', function () {This would make it clearer which role is being tested and why we expect the permission to be denied.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
ghost/core/test/integration/migrations/migration.test.js(2 hunks)ghost/core/test/regression/api/admin/members-signin-url.test.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- ghost/core/test/integration/migrations/migration.test.js
🔇 Additional comments (1)
ghost/core/test/regression/api/admin/members-signin-url.test.js (1)
93-100:✅ Verification successful
Verify the test assertions match the new permission model.
The test correctly attempts to access member signin URLs and expects a 403 status code for an Author role. This is consistent with the updated permission model where Authors shouldn't have access to member signin URLs.
🏁 Script executed:
#!/bin/bash # Verify that the permission model in fixtures aligns with this test's assertions # Check the permissions defined for Editor and Author roles in fixtures echo "Checking role permissions in fixtures..." cat $(fd --type f "fixtures.*\.json$" | grep -v "node_modules") | grep -A 20 "roles" | grep -A 10 -E "name.*(Editor|Author)" # Check for any migrations that modify these permissions echo "Checking permission migrations..." fd --type f ".*migration.*\.js$" | xargs grep -l "member.*permission" | xargs catLength of output: 478
Ensure the test is correctly validating the new permission model for member signin URLs.
The verification confirms that the fixtures correctly define the Author role as lacking permission to access member signin URLs and that no conflicting migrations alter this behavior. The test, which expects a 403 response for an Author role, is aligned with the updated permission model.
No further changes are required.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
ghost/admin/app/templates/member.hbs (1)
41-41: Improved access control with helper functionThis change replaces a direct admin check with a more flexible permission helper function, aligning with the PR's objective to extend member management capabilities to editors.
The use of a dedicated helper function for permission checking is a good architectural decision as it:
- Centralizes permission logic, making it easier to maintain
- Makes the code more declarative and easier to understand
- Supports future extensions to other roles without template changes
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
ghost/admin/app/helpers/members-count-fetcher.js(1 hunks)ghost/admin/app/routes/members/import.js(1 hunks)ghost/admin/app/templates/member.hbs(1 hunks)ghost/admin/mirage/config/members.js(1 hunks)ghost/admin/tests/acceptance/members-test.js(4 hunks)ghost/core/core/server/data/schema/fixtures/fixtures.json(1 hunks)ghost/core/test/utils/fixtures/fixtures.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- ghost/core/core/server/data/schema/fixtures/fixtures.json
🔇 Additional comments (8)
ghost/core/test/utils/fixtures/fixtures.json (1)
1106-1109: Permission structure updated to allow Editors to manage membersThe permissions added to the "Editor" role are aligned with the PR objective of giving editors the ability to manage members. The changes grant editors read, add, browse, edit, and destroy capabilities for members, as well as read access to member sign-in URLs and browse/read permissions for offers.
This balanced approach gives editors comprehensive member management capabilities while still preserving some privilege separation by limiting offer management to read-only access. This maintains appropriate access control boundaries while enabling editors to perform needed member management tasks.
ghost/admin/tests/acceptance/members-test.js (4)
21-23: Test updated to reflect new role-based permissions modelThe test now correctly checks that Authors (rather than Editors) are redirected away from the members page, which aligns with the new permission structure where Editors can manage members.
32-41: Test suite renamed and properly configured for testing Editor capabilitiesThe renamed test suite now specifically tests Editor capabilities, with appropriate setup including invalidating the session before creating an Editor user. This ensures tests start with a clean authentication state.
353-407: New test suite added for Owner roleA dedicated test suite for the Owner role has been added to ensure that owners maintain the ability to manage members alongside the newly empowered editors.
However, there's significant duplication between the editor and owner test suites. Consider extracting common testing logic to reduce redundancy while still verifying both roles have the appropriate permissions.
-describe('as editor', function () { +describe('as user with member management permissions', function () { + // Function to set up different roles that should have member access + function setupUserWithRole(roleName) { + beforeEach(async function () { + await invalidateSession(); + this.server.loadFixtures('configs'); + let role = this.server.create('role', {name: roleName}); + this.server.create('user', {roles: [role]}); + await authenticateSession(); + }); + } + + describe('as editor', function () { + setupUserWithRole('Editor'); it('it renders, can be navigated, can edit member', async function () { // Test implementation remains the same }); // ... other tests ... + }); + + describe('as owner', function () { + setupUserWithRole('Owner'); it('it renders, can be navigated, can edit member', async function () { // Test implementation remains the same }); + });This is a suggested refactoring that would reduce duplication while maintaining test coverage for both roles.
10-10:✅ Verification successful
Test suite renamed to indicate a potential split in test coverage
The test suite has been renamed to "Acceptance: Members 1", which suggests this might be part of a series of member-related test files.
🏁 Script executed:
#!/bin/bash # Search for other numbered member test files to confirm a pattern echo "Checking for other member test files..." fd -t f "members-.*test.js" ghost/admin/tests/acceptance/Length of output: 247
Test Suite Naming Pattern Confirmed
The renaming to "Acceptance: Members 1" aligns with an anticipated expansion of member-related tests. The search output shows another file—ghost/admin/tests/acceptance/members-activity-test.js—which confirms that splitting the test coverage is already in progress. Please ensure that any future tests follow a consistent naming and numbering scheme to maintain clarity.ghost/admin/app/helpers/members-count-fetcher.js (1)
37-42:✅ Verification successful
Access control updated to include editor role
The comment and conditional check have been updated to allow both Admins/Owners and Editors to access member counts, consistent with the PR objectives.
🏁 Script executed:
#!/bin/bash # Verify that the gh-user-can-manage-members helper has consistent logic with this conditional # Find the gh-user-can-manage-members helper implementation echo "Checking for gh-user-can-manage-members helper implementation:" fd -t f -e js "gh-user-can-manage-members" "ghost/admin/app/helpers/" --exec cat {}Length of output: 773
Access control updated correctly. The update in
ghost/admin/app/helpers/members-count-fetcher.jsnow properly allows both Admins and Editors to access the member counts, and the correspondinggh-user-can-manage-membershelper implementation confirms this logic is applied consistently.
- In
ghost/admin/app/helpers/members-count-fetcher.js(Lines 37-42), the check now correctly returnsnullwhen the user is neither an admin nor an editor.- The helper in
ghost/admin/app/helpers/gh-user-can-manage-members.jsreturnstruefor users with either theisAdminorisEditorflag, which aligns with the updated access control.ghost/admin/mirage/config/members.js (1)
12-16:✅ Verification successful
Added Editor role to allowed roles
The Editor role has been added to the ALLOWED_ROLES array, enabling editors to access all member management endpoints in the mirage mock server.
🏁 Script executed:
#!/bin/bash # Check if editor permissions are granted consistently across the application # Check fixtures and migrations to ensure editor permissions for members are defined echo "Checking for editor permissions in fixtures and migrations:" fd -t f "fixtures" "ghost/core" | grep -i "json" | xargs grep -l "Editor" | xargs grep -A 10 -B 10 "members"Length of output: 39436
Editor Role Addition Verified
The mirage configuration in
ghost/admin/mirage/config/members.jsnow correctly includes theEditorrole alongsideOwnerandAdministrator, ensuring that editors have access to all member management endpoints. The consistency check across fixtures and migrations confirms that the permissions setup remains aligned with our design.
- File:
ghost/admin/mirage/config/members.js(lines 12-16)- The shell script verification shows no discrepancies in permissions related to member endpoints.
ghost/admin/app/routes/members/import.js (1)
1-3:✅ Verification successful
Import route now extends MemberRoute for consistent access control
Changed from AdminRoute to MemberRoute for the MembersImportRoute, ensuring that access control is consistently applied across all member-related routes.
🏁 Script executed:
#!/bin/bash # Check for consistent route inheritance across member routes # Find all member routes to ensure they extend MemberRoute echo "Checking route inheritance for member routes:" fd -t f -p "routes/members/.+\.js" "ghost/admin/app" # Check the base MemberRoute implementation echo "Checking MemberRoute implementation:" cat ghost/admin/app/routes/member.jsLength of output: 4141
Verified: Consistent Access Control Implementation
The updated
MembersImportRoutenow correctly extendsMemberRoute, ensuring that access control is consistently applied across all member-related routes. Verification confirmed that:
- The only member route file (
ghost/admin/app/routes/members/import.js) now imports and extendsMemberRoute.- The
MemberRouteimplementation inghost/admin/app/routes/member.jsprovides the necessary access control logic.No further changes are required.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
ghost/admin/app/routes/members/import.js (3)
1-2: Remove the commented import if no longer needed
Keeping a commented-out import can clutter the code and cause confusion. If it's not intended for future use, please remove it.-//import MembersRoute from 'ghost-admin/routes/members'; import AuthenticatedRoute from 'ghost-admin/routes/authenticated';
6-8: Handle or remove the TODO around disabled members
A placeholder comment suggests upcoming work to handle cases where members are disabled. Consider implementing a redirect or removing the comment if it's not needed.Would you like me to help implement a check that redirects or denies access if members are disabled?
10-14: Use a dedicated permission check for manage-members access
Hardcoding role checks (isEditororisAdmin) may introduce duplication and potential drift from later permission updates. Consider using or extracting a centralized helper (such asgh-user-can-manage-members) to handle all manage-member permission checks consistently.if (!this.session.user.isEditor && !this.session.user.isAdmin) { return this.transitionTo('home'); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
ghost/admin/app/routes/member.js(2 hunks)ghost/admin/app/routes/members/import.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- ghost/admin/app/routes/member.js
⏰ Context from checks skipped due to timeout of 90000ms (13)
- GitHub Check: Ghost-CLI tests
- GitHub Check: Unit tests (Node 22.13.1)
- GitHub Check: Database tests (Node 20.11.1, sqlite3)
- GitHub Check: Unit tests (Node 20.11.1)
- GitHub Check: Regression tests (Node 20.11.1, sqlite3)
- GitHub Check: Database tests (Node 22.13.1, mysql8)
- GitHub Check: Unit tests (Node 18.12.1)
- GitHub Check: Regression tests (Node 20.11.1, mysql8)
- GitHub Check: Database tests (Node 20.11.1, mysql8)
- GitHub Check: Database tests (Node 18.12.1, mysql8)
- GitHub Check: Admin-X Settings tests
- GitHub Check: Admin tests - Chrome
- GitHub Check: Lint
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
ghost/admin/app/routes/members/import.js(1 hunks)
🔇 Additional comments (3)
ghost/admin/app/routes/members/import.js (3)
1-3: Appropriate base class change to support editor access.Changing from
AdminRoutetoAuthenticatedRouteis a good approach for implementing more granular permission controls. This change aligns with the PR objective of giving editors the ability to manage members.
4-12: Role-based authorization implemented correctly.The
beforeModelmethod properly implements the authorization logic by:
- Calling the parent method first (preserving any setup it performs)
- Adding a role check that allows both editors and admins
- Redirecting unauthorized users to the home route
This implementation correctly supports the goal of allowing editors to manage members while maintaining security.
9-9:✅ Verification successful
Verify session service availability.
The code uses
this.session.userbut there's no explicit import for a session service. Verify that the session service is properly made available through theAuthenticatedRouteparent class or add the appropriate import if needed.
🏁 Script executed:
#!/bin/bash # Check how session is imported or made available in authenticated routes echo "Checking AuthenticatedRoute implementation:" cat $(fd "authenticated.js" --type f) echo "Checking how other routes access the session:" rg "session\.user" --glob "routes/*.js" -B 5Length of output: 1595
Session Service Injection Verified
After reviewing the base routes, we can confirm that the session service is injected via the
@service sessiondecorator inAuthenticatedRoute(seeghost/admin/app/routes/authenticated.js). As long as theghost/admin/app/routes/members/import.jsroute extends this base class (or a similar one that provides the session), an explicit import for the session service isn’t necessary.
- Verify that
import.jsextendsAuthenticatedRoute(or another appropriate base) to ensurethis.sessionis available.
| // give editors the ability to reach this route also. | ||
| if (!this.session.user.isEditor && !this.session.user.isAdmin) { | ||
| return this.transitionTo('home'); | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Consider adding a test for this authorization logic.
Since this is a security-related change that affects access control, it would be beneficial to add a test that verifies:
- Admins can access this route
- Editors can access this route
- Authors or contributors are redirected
This ensures the permission changes are working as expected and prevents future regressions.
🏁 Script executed:
#!/bin/bash
# Check if there are any tests for this route's authorization
echo "Checking for existing tests for members/import route:"
rg -A 3 "MembersImportRoute|members/import" --glob "*test.js"
echo "Checking for similar authorization tests in other routes:"
rg -A 5 "isEditor|isAdmin" --glob "*test.js"Length of output: 7126
Enhance Authorization Test Coverage
The /members/import route currently enforces that only Admins and Editors have access, while other roles are redirected. Although there is an acceptance test (ghost/admin/tests/acceptance/members/import-test.js), it doesn’t explicitly verify the behavior for different user roles. Please add or extend tests to explicitly cover the following cases:
- Admins: Verify that an Admin user can access
/members/importwithout redirection. - Editors: Verify that an Editor user can access
/members/importwithout redirection. - Authors/Contributors: Verify that users without Editor or Admin privileges are redirected (e.g., to
/home).
This additional coverage will help ensure the security-related access control logic is robust and guard against regressions.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
ghost/core/core/server/data/migrations/versions/5.111/2025-03-02-11-00-00-update-comments-permissions.js(1 hunks)ghost/core/core/server/data/schema/fixtures/fixtures.json(1 hunks)ghost/core/test/unit/server/data/schema/integrity.test.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- ghost/core/test/unit/server/data/schema/integrity.test.js
🧰 Additional context used
🪛 ESLint
ghost/core/core/server/data/migrations/versions/5.111/2025-03-02-11-00-00-update-comments-permissions.js
[error] 1-1: 'addPermission' is assigned a value but never used.
(no-unused-vars)
[error] 38-38: Unexpected trailing comma.
(comma-dangle)
⏰ Context from checks skipped due to timeout of 90000ms (13)
- GitHub Check: Unit tests (Node 22.13.1)
- GitHub Check: Unit tests (Node 20.11.1)
- GitHub Check: Unit tests (Node 18.12.1)
- GitHub Check: Regression tests (Node 20.11.1, sqlite3)
- GitHub Check: Regression tests (Node 20.11.1, mysql8)
- GitHub Check: Database tests (Node 20.11.1, sqlite3)
- GitHub Check: Database tests (Node 22.13.1, mysql8)
- GitHub Check: Lint
- GitHub Check: Database tests (Node 20.11.1, mysql8)
- GitHub Check: Ghost-CLI tests
- GitHub Check: Database tests (Node 18.12.1, mysql8)
- GitHub Check: Admin tests - Chrome
- GitHub Check: Admin-X Settings tests
🔇 Additional comments (2)
ghost/core/core/server/data/schema/fixtures/fixtures.json (1)
937-942: LGTM! Appropriate permissions added to Editor roleThe changes appropriately extend the Editor role with member management permissions, which aligns with the PR objective to "give editors the ability to manage members". The permissions include full CRUD operations for members, access to member signin URLs, read/browse permissions for offers, and comprehensive comment management.
ghost/core/core/server/data/migrations/versions/5.111/2025-03-02-11-00-00-update-comments-permissions.js (1)
1-39:❓ Verification inconclusive
Verify migration file naming convention
The file follows Ghost's migration naming convention with timestamp format. However, verify that the date used (2025-03-02) is intentional, as it's set in the future relative to current development dates (PR submitted in March 2025).
🏁 Script executed:
#!/bin/bash # Check if other migration files follow the same date pattern in this PR echo "Looking for other migration files with future dates..." fd "^2025-03-.*\.js$" ghost/core/core/server/data/migrations echo "Looking for migration naming pattern documentation..." grep -r "migration naming" --include="*.md" .Length of output: 562
Action Required: Confirm Migration File Timestamp
- The migration file follows the established naming pattern (e.g., see the similar file on 2025-03-01).
- Please verify that using the timestamp "2025-03-02" is intentional, given that it's set in the near future relative to the current development period.
🧰 Tools
🪛 ESLint
[error] 1-1: 'addPermission' is assigned a value but never used.
(no-unused-vars)
[error] 38-38: Unexpected trailing comma.
(comma-dangle)
| @@ -0,0 +1,39 @@ | |||
| const {addPermissionToRole, combineTransactionalMigrations, addPermission} = require('../../utils'); | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove unused import
The addPermission function is imported but never used in this migration file.
-const {addPermissionToRole, combineTransactionalMigrations, addPermission} = require('../../utils');
+const {addPermissionToRole, combineTransactionalMigrations} = require('../../utils');📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const {addPermissionToRole, combineTransactionalMigrations, addPermission} = require('../../utils'); | |
| const {addPermissionToRole, combineTransactionalMigrations} = require('../../utils'); |
🧰 Tools
🪛 ESLint
[error] 1-1: 'addPermission' is assigned a value but never used.
(no-unused-vars)
| module.exports = combineTransactionalMigrations( | ||
| addPermissionToRole({ | ||
| permission: 'Moderate comments', | ||
| role: 'Editor' | ||
| }), | ||
| addPermissionToRole({ | ||
| permission: 'Like comments', | ||
| role: 'Editor' | ||
| }), | ||
| addPermissionToRole({ | ||
| permission: 'Unlike comments', | ||
| role: 'Editor' | ||
| }), | ||
| addPermissionToRole({ | ||
| permission: 'Add comments', | ||
| role: 'Editor' | ||
| }), | ||
| addPermissionToRole({ | ||
| permission: 'Edit comments', | ||
| role: 'Editor' | ||
| }), | ||
| addPermissionToRole({ | ||
| permission: 'Delete comments', | ||
| role: 'Editor' | ||
| }), | ||
| addPermissionToRole({ | ||
| permission: 'Read comments', | ||
| role: 'Editor' | ||
| }), | ||
| addPermissionToRole({ | ||
| permission: 'Browse comments', | ||
| role: 'Editor' | ||
| }), | ||
| addPermissionToRole({ | ||
| permission: 'Report comments', | ||
| role: 'Editor' | ||
| }), | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix trailing comma and ensure comprehensive comment permissions
This migration properly adds all necessary comment-related permissions to the Editor role, which aligns with the "comment": "all" permission in the fixtures file. However, there's a linting issue with the trailing comma at the end.
module.exports = combineTransactionalMigrations(
addPermissionToRole({
permission: 'Moderate comments',
role: 'Editor'
}),
addPermissionToRole({
permission: 'Like comments',
role: 'Editor'
}),
addPermissionToRole({
permission: 'Unlike comments',
role: 'Editor'
}),
addPermissionToRole({
permission: 'Add comments',
role: 'Editor'
}),
addPermissionToRole({
permission: 'Edit comments',
role: 'Editor'
}),
addPermissionToRole({
permission: 'Delete comments',
role: 'Editor'
}),
addPermissionToRole({
permission: 'Read comments',
role: 'Editor'
}),
addPermissionToRole({
permission: 'Browse comments',
role: 'Editor'
}),
addPermissionToRole({
permission: 'Report comments',
role: 'Editor'
- }),
+ })
);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| module.exports = combineTransactionalMigrations( | |
| addPermissionToRole({ | |
| permission: 'Moderate comments', | |
| role: 'Editor' | |
| }), | |
| addPermissionToRole({ | |
| permission: 'Like comments', | |
| role: 'Editor' | |
| }), | |
| addPermissionToRole({ | |
| permission: 'Unlike comments', | |
| role: 'Editor' | |
| }), | |
| addPermissionToRole({ | |
| permission: 'Add comments', | |
| role: 'Editor' | |
| }), | |
| addPermissionToRole({ | |
| permission: 'Edit comments', | |
| role: 'Editor' | |
| }), | |
| addPermissionToRole({ | |
| permission: 'Delete comments', | |
| role: 'Editor' | |
| }), | |
| addPermissionToRole({ | |
| permission: 'Read comments', | |
| role: 'Editor' | |
| }), | |
| addPermissionToRole({ | |
| permission: 'Browse comments', | |
| role: 'Editor' | |
| }), | |
| addPermissionToRole({ | |
| permission: 'Report comments', | |
| role: 'Editor' | |
| }), | |
| ); | |
| module.exports = combineTransactionalMigrations( | |
| addPermissionToRole({ | |
| permission: 'Moderate comments', | |
| role: 'Editor' | |
| }), | |
| addPermissionToRole({ | |
| permission: 'Like comments', | |
| role: 'Editor' | |
| }), | |
| addPermissionToRole({ | |
| permission: 'Unlike comments', | |
| role: 'Editor' | |
| }), | |
| addPermissionToRole({ | |
| permission: 'Add comments', | |
| role: 'Editor' | |
| }), | |
| addPermissionToRole({ | |
| permission: 'Edit comments', | |
| role: 'Editor' | |
| }), | |
| addPermissionToRole({ | |
| permission: 'Delete comments', | |
| role: 'Editor' | |
| }), | |
| addPermissionToRole({ | |
| permission: 'Read comments', | |
| role: 'Editor' | |
| }), | |
| addPermissionToRole({ | |
| permission: 'Browse comments', | |
| role: 'Editor' | |
| }), | |
| addPermissionToRole({ | |
| permission: 'Report comments', | |
| role: 'Editor' | |
| }) | |
| ); |
🧰 Tools
🪛 ESLint
[error] 38-38: Unexpected trailing comma.
(comma-dangle)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
ghost/core/core/frontend/src/admin-auth/message-handler.js (1)
66-67: Enhanced user role support for member management capabilities.The fetch request now includes roles when retrieving the current user, which is essential for the new permission model that grants editors access to member management functionality.
Consider adding error handling for the case where the roles information might not be available in the response:
const res = await fetch( adminUrl + '/users/me/?include=roles' ); const json = await res.json(); +// Ensure the roles property exists to prevent potential errors elsewhere +if (json && json.users && json.users[0] && !json.users[0].roles) { + json.users[0].roles = []; +} respond(null, json);apps/comments-ui/src/components/content/Comment.tsx (3)
42-42: Remove debug console log before production.This console log statement was likely added for debugging purposes during development and should be removed before merging to production.
-console.log('admin at commentcomponent is', admin, comment.id);
67-67: Remove debug console log before production.This console log statement was likely added for debugging purposes during development and should be removed before merging to production.
-console.log('useCommentVisibility', comment.status, admin);
87-87: Remove debug console log before production.This console log statement was likely added for debugging purposes during development and should be removed before merging to production.
-console.log('published comment says admin is', admin, comment.id);apps/comments-ui/src/App.tsx (1)
116-120: Debug log should be removed and consider adding documentation.The implementation correctly checks for allowed moderator roles, but includes a debug console log that should be removed before merging to production. Also, consider adding a brief comment explaining why this check exists.
-// unset admin for non-moderator roles. -console.log(admin.roles.some(role => ALLOWED_MODERATORS.includes(role.name))) +// Only users with specific moderator roles (Owner, Administrator, Editor) can manage comments if (!admin || !(admin.roles.some(role => ALLOWED_MODERATORS.includes(role.name)))) { admin = null; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/comments-ui/src/App.tsx(2 hunks)apps/comments-ui/src/components/content/Comment.tsx(3 hunks)ghost/core/core/frontend/src/admin-auth/message-handler.js(1 hunks)
🔇 Additional comments (2)
apps/comments-ui/src/App.tsx (2)
20-20: Good addition of role-based access control for moderators.Adding the constant to define allowed moderator roles makes the code more maintainable and aligns with the PR objective of giving editors the ability to manage members.
115-121:❓ Verification inconclusive
Verify all components correctly handle the new permissions model.
This change extends moderation capabilities to editors, which is a significant change in permission model. Make sure all components and views that rely on admin status correctly handle this new model.
🏁 Script executed:
#!/bin/bash # Description: Look for components that might rely on admin status checks but don't account for editors # Check for UI components that might be using admin flags directly echo "Searching for components using admin checks that might need updating:" rg -A 2 -B 2 "\.isAdmin|admin:" --type-add "js:*.{js,ts,hbs,jsx,tsx}" --type js # Look for Ember templates using isAdmin checks echo "Searching for Ember templates using isAdmin checks:" rg -A 2 -B 2 "isAdmin" --type "hbs" # Look for permission checks that might be related to member management echo "Searching for permission checks related to member management:" rg -A 2 -B 2 "can.*member|member.*permission" --type-add "js:*.{js,ts,jsx,tsx}" --type jsLength of output: 84746
Attention: Confirm Extended Moderation Rights in All Components
The update in
apps/comments-ui/src/App.tsxnow unsets the admin flag for users whose roles don’t match the allowed moderators (which now includes editors). However, our repository search shows multiple components, templates, and tests still rely on direct checks (e.g.isAdminor staticadmin: true) without accounting for the new editor permissions. In particular:
- UI Components & Hooks: Files such as
apps/comments-ui/src/components/content/Comment.tsxuse the admin flag directly (e.g. in functions likeuseCommentVisibility), which may not correctly reflect the new permissions for editors.- Templates: Ember templates like those in
ghost/admin/app/components/gh-members-segment-count.hbsandghost/admin/app/components/editor/publish-options/email-recipients.hbsstill check forisAdminrather than using a more inclusive permissions check.- Tests: Several test files (both acceptance and unit tests) explicitly pass
admin: true, which assumes a strict admin-only condition, potentially masking issues with the extended moderation logic.Please review these areas and update any logic or test cases that assume an exclusive admin role so they correctly grant moderation capabilities to editors as well.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/comments-ui/src/App.tsx (1)
116-119: Well-implemented role permission check for comment moderation.The code correctly filters admin users based on their role, only allowing Owner, Administrator, and Editor roles to access moderation capabilities. The implementation uses an appropriate array method (
some()) to check if any of the user's roles match the allowed moderator roles.A small suggestion would be to add a defensive check for
admin.rolesas well, in case the roles property is undefined:- if (!admin || !(admin.roles.some(role => ALLOWED_MODERATORS.includes(role.name)))) { + if (!admin || !admin.roles || !(admin.roles.some(role => ALLOWED_MODERATORS.includes(role.name)))) {However, this is a minor concern if you're confident the API always returns a roles array.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/comments-ui/src/App.tsx(2 hunks)apps/comments-ui/src/components/content/Comment.tsx(0 hunks)
💤 Files with no reviewable changes (1)
- apps/comments-ui/src/components/content/Comment.tsx
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Setup
🔇 Additional comments (2)
apps/comments-ui/src/App.tsx (2)
20-20: Good addition of the ALLOWED_MODERATORS constant to define valid moderator roles.The constant clearly defines which roles have moderation capabilities, including Editors, which aligns with the PR objective of giving editors member management capabilities.
103-144: Member management permission expansion improves role-based access control.This change correctly extends the existing admin authentication flow to accommodate Editors as moderators. The function maintains the same behavior for existing moderator roles while adding Editors to the permitted roles list. This implementation ensures backward compatibility while enabling the new feature defined in the PR objectives.
The approach taken here - resetting
adminto null when the user doesn't have permission - elegantly handles permission denial without requiring special downstream logic.
cathysarisky
changed the title
The current Ghost permissions system gives Editors the ability to publish posts and newsletters and to invite new authors and contributors, but does not give them any abilities in terms of member management, such as creating new members, providing complimentary access, viewing subscription status, or cancelling member subscriptions. Currently, that functionality is reserved for the Administrator level, but this access level includes substantial additional permissions, that the site owner may not want to grant to the person in their organization doing "member support".
The situation is similar for comments. Only administrators can moderate comments, even though a busy site might have a lot of staffing need in this area to keep things under control.
This PR gives the editor role the ability to do member management, including all member actions and comments moderation.
While I was there, I closed #22328
Tests updated.