fix(group-subscriptions): tighten authorization on the REST routes - #1023
fix(group-subscriptions): tighten authorization on the REST routes#1023jason10lee wants to merge 2 commits into
Conversation
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🟢 Approval recommended
The authorization tightening is coherent across layers and is backed by targeted unit tests, with only minor doc wording noted.
Pull request overview
This PR hardens the newspack-plugin group-subscription REST API authorization by ensuring an unresolved/anonymous identity is never treated as authorized, and by restricting the “search users” endpoint to store staff only (since it answers about site user records rather than a specific subscription).
Changes:
- Fail-closed authorization for group-subscription REST routes by explicitly rejecting unauthenticated requests and by preventing user ID
0from ever matching an ownerless subscription. - Restrict
/search-userspermissions tomanage_woocommerceusers (store staff) rather than group managers. - Update and extend unit coverage for ownerless subscriptions and for the narrowed search-users permissions.
File summaries
| File | Description |
|---|---|
| plugins/newspack-plugin/tests/unit-tests/plugins/woocommerce-subscriptions/group-subscription/class-group-subscription-api.php | Adds REST dispatch tests for anonymous/ownerless and admin-only search behavior; ensures feature flag is set for route registration. |
| plugins/newspack-plugin/tests/unit-tests/content-gate/group-subscriptions.php | Updates expectations for manager resolution and adds coverage for ownerless subscriptions and unauthenticated callers. |
| plugins/newspack-plugin/includes/plugins/woocommerce-subscriptions/group-subscription/class-group-subscription.php | Ensures manager resolution doesn’t seed a phantom manager (0) and guards user_is_manager against unauthenticated IDs. |
| plugins/newspack-plugin/includes/plugins/woocommerce-subscriptions/group-subscription/class-group-subscription-api.php | Adds an anonymous guard in the shared permission callback and switches /search-users to an admin-only permission callback. |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
dkoo
left a comment
There was a problem hiding this comment.
@jason10lee thanks for the tightening here! I tested and encountered no issues or regressions, and the explicit handling for unresolved users and the additional admin permission checks are well-documented and make sense to me.
All Submissions:
Changes proposed in this Pull Request:
The group-subscription REST routes share one permission callback, and it could resolve an unauthenticated caller as authorized under a state the data layer can reach on its own. This makes an unresolved caller fail closed, and it narrows the one route in the namespace that answers about the site's readers instead of about a subscription.
Three of the four changes establish the same invariant from different places: an unresolved identity is never an authorized one. Any one of them closes the path, and together a regression in one is still not a grant. The guard in the shared callback turns away nothing that works today, which is what makes it cheap to keep.
The fourth is the narrowing. The member search answers about the site's user records, not about the subscription named in the request, so managing a group is the wrong authorization for it. Only the subscription admin screen calls that route; the reader-facing My Account bundle uses the rename and invite-link routes. For store staff, nothing changes.
We weighed two alternatives. The first was narrowing the search instead of closing it, by restricting non-staff callers to an exact-address lookup. We drafted that and rejected it: it leaves the route reachable by a population with no UI for it, and an exact lookup still answers whether a given address has an account here. The second was gating the two other admin-only routes the same way. We left that out, because neither answers about anything beyond the subscription the caller was already authorized for. That is hardening, not a fix, and it would remove a capability a manager may be intended to have.
Deliberately out of scope: what a group subscription should become when its owner no longer exists. This makes that state safe without deciding it, and it does not repair records already in it. Both are filed separately.
One test asserted the behavior the first change removes, so that assertion is updated here instead of worked around.
Detail is in https://linear.app/a8c/issue/NPPM-3203 and https://linear.app/a8c/issue/NPPM-3200.
How to test the changes in this Pull Request:
The refusal paths are covered by unit tests added here.
Other information:
Full newspack-plugin suite green (3902 tests, 11026 assertions), phpcs clean, and the three manual steps above were driven on an isolated env.