Support Forums: use the topic ID passed to user_can_resolve() - #856
Support Forums: use the topic ID passed to user_can_resolve()#856obenland wants to merge 3 commits into
user_can_resolve()#856Conversation
The `$topic_id` argument was overwritten with `bbp_get_topic_id()`, so the check always evaluated the topic being displayed rather than the one it was asked about. Resolve the topic from the passed ID, falling back to the displayed one when none is given, and use `bbp_get_topic()` so a non-topic ID can't satisfy the author comparison. The directory compat filter has the same mismatch: it grants authors, contributors, and support reps permission based on the plugin or theme loaded for the current request, without regard for which topic is being checked. Limit it to topics that belong to that plugin or theme. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the Core Committers: Use this line as a base for the props when committing in SVN: To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
There was a problem hiding this comment.
Pull request overview
Fixes topic-resolution authorization to consistently evaluate permissions against the topic ID provided by callers (falling back to the displayed topic only when no usable ID is provided), preventing mismatches when actions are triggered from request data.
Changes:
- Pass the
$topic_idargument through tobbp_get_topic_id( $topic_id )and fetch the topic viabbp_get_topic()to ensure authorization is evaluated against a real topic. - In Support Forums directory compat filtering, ensure the compat-derived author/contributor/support-rep lists only affect resolution permissions when the topic actually belongs to the compat object.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
wordpress.org/public_html/wp-content/plugins/wporg-bbp-topic-resolution/inc/class-plugin.php |
Ensures user_can_resolve() evaluates permission against the requested topic ID (with safe fallback) and only matches against topic posts. |
wordpress.org/public_html/wp-content/plugins/support-forums/inc/class-directory-compat.php |
Guards the compat permission override so it only applies when the topic is tagged for the currently loaded compat object. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| // The compat object is loaded from the request, not from $topic_id. | ||
| $terms = get_the_terms( $topic_id, $this->taxonomy() ); | ||
| if ( empty( $terms ) || is_wp_error( $terms ) || ! in_array( (string) $this->slug(), wp_list_pluck( $terms, 'slug' ), true ) ) { | ||
| return $retval; | ||
| } |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 8 included reviews per hour; 6 remain after this review. 📝 WalkthroughWalkthroughTopic resolution now validates the submitted topic’s forum, targets the requested topic for authorization, and verifies compatibility taxonomy ownership with strict forum ID handling. ChangesTopic resolution authorization
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to This is a localized correction to use the requested topic ID when determining resolution permissions; no actionable merge-blocking risk remains after normal checks and review. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
`Support_Compat::is_enabled_on_forum()` read the passed forum ID and then overwrote its answer from the current request, so on a single topic or view the argument had no effect at all. Return early when one is supplied, which also makes the existing call in `handle_extra_reply_actions()` mean what it says. `topic_resolution_handler()` had the same mismatch: it asked whether resolution was enabled before it read `$_POST['topic_id']`, and so answered for the forum being viewed. Move the check below the topic lookup and pass that topic's forum. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`$forum_id` can arrive as a numeric string, so cast it rather than relying on a loose comparison against the integer constant. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Topic_Resolution\Plugin::user_can_resolve()takes a$topic_idargument and then discards it on the next line, replacing it withbbp_get_topic_id(). It has done that since the method was introduced in [3797]. The result is that the method reports on whichever topic the current request is rendering, not the one the caller asked about.That is fine for the two callers that read the topic from the page anyway, but
Hooks::handle_extra_reply_actions()andPlugin::topic_resolution_handler()both pass an ID that came in with the request, and the answer they get back doesn't correspond to it.This passes the argument through to
bbp_get_topic_id(), which returns it when it's a usable number and otherwise falls back to the displayed topic, so callers that legitimately pass nothing behave as before. It also swapsget_post()forbbp_get_topic(), so an ID that points at some other post type can no longer match onpost_author.Directory_Compat::user_can_resolve(), which filters the result, has the same mismatch in a different form: it ignores$topic_identirely and answers from$this->authors,$this->contributors, and$this->support_reps, all populated from the plugin or theme loaded for the current request. So the answer for a topic belonging to one plugin could be decided by another plugin's author list. The filter now returns early unless the topic carries the term for the loaded object, which is the same condition under whichcheck_topic_for_compat()loads that object in the first place.Testing instructions
🤖 Generated with Claude Code
Summary by CodeRabbit