fix(message): apply group revoke permission to community topics (#222)

[an9xyz]

Problem

Closes #222.

CommunityTopic (Thread) messages had no admin revoke logic. Group owners and managers could only revoke their own messages and messages from bots they created, because hasRevokePermission (modules/message/api.go) only handled ChannelTypeGroup and fell through to deny for community topics.

Change

• Add a ChannelTypeCommunityTopic branch to hasRevokePermission that resolves the parent group via thread.ParseChannelID and reuses the group revoke role matrix.
• Extract the shared role matrix into groupRoleRevokeAllowed(groupNo, fromUID, loginUID), used by both the Group and CommunityTopic branches. The existing Group behavior is unchanged (pure refactor).
• Parse failures fail closed (deny).
• The existing bot-owner short-circuit applies to topics as well.

Resulting permission matrix (based on parent-group role)

Operator	Can revoke
Owner	anyone
Manager	normal members only (not other managers / owner)
Normal member	only their own messages

Per product decision on the issue: topics reuse the channel (group) permission model, topic-creator gets no special privilege, and managers cannot revoke each other. This aligns with how authorizeMutualDelete already handles community topics.

Tests

• revoke_topic_test.go — unit tests (fake group service) covering the parent-role matrix, channel-ID parse failure, and the bot-owner branch.
• revoke_topic_integration_test.go — integration test (integration build tag, isolated DB) exercising the real group.Service.GetMember lookup via the resolved parent group. Tagged to stay out of the default CI test-DB run to avoid the sql-migrate conflict tracked in OCTO migration test debt: ~20+ broken tests masked by stale main CI #17.

Test plan

• go test ./modules/message/ (unit, clean DB) — pass
• go test -race ./modules/message/ (no tag, mirrors CI) — pass
• go test -tags=integration -run TestHasRevokePermission_CommunityTopic_Integration ./modules/message/ — pass
• gofmt / go vet (both tag sets) — clean

[lml2468]

Summary

Clean fix: community topic (子区) revoke permissions now inherit the parent group's role matrix. Correct, secure, well-tested. No blocking issues.

What this PR does

1. Refactored groupRoleRevokeAllowed — Extracts the group role-based revoke permission logic into a standalone method. The existing matrix (creator > manager > normal) is preserved exactly, zero logic change for existing group channels.

2. CommunityTopic case — Parses parent group from topic channelID via thread.ParseChannelID, then delegates to the same groupRoleRevokeAllowed. Consistent with the authorizeMutualDelete pattern already in the codebase (L617, L1783).

3. Fail-closed on parse error — If ParseChannelID fails, log + deny. Correct security posture.

4. No topic-creator privilege — Intentionally ignores topic creator concept; permissions are purely parent-group-role based. This is the right call — topic creation doesn't imply admin authority over the parent group's messages.

Tests

Excellent coverage:

• Unit tests (revoke_topic_test.go): 8 cases covering full parent-group role matrix, outsider, sender-left-group, invalid channelID (fail-closed), bot-owner short-circuit for topics.

• Integration test (revoke_topic_integration_test.go): Real MySQL with //go:build integration tag. Critical: the unit test's fakeGroupService ignores groupNo (looks up by uid only), so it cannot catch the regression where hasRevokePermission accidentally passes the raw topic channelID instead of the parsed parent group number. The integration test with real group.Service.GetMember catches exactly this class of bug.

Findings

None. Clean diff.

Verdict

APPROVED.

[Jerry-Xin]

✅ APPROVE

Clean refactor that closes #222: community topics now inherit group revoke permissions from the parent group.

What's good:

• groupRoleRevokeAllowed extracted as a reusable method — group and topic share the same permission matrix without duplication
• switch on ChannelType is cleaner than the old if chain and naturally extensible
• thread.ParseChannelID fail-closed: parse error → deny (consistent with delete2)
• Test coverage is excellent: unit tests cover the full parent-role matrix (8 cases), invalid channelID fail-closed, and bot-owner short-circuit in topics; integration test verifies real DB path catches groupNo vs channelID misuse
• Comments clearly explain design decisions (no topic-creator privilege, alignment with authorizeMutualDelete)

No blocking issues found.

🟡 Minor observation: fakeGroupService.GetMember ignores groupNo and keys on uid only — this is fine for unit tests (documented in comments), but the integration test is what actually validates correct groupNo propagation. Good layering.

[lml2468]

Re-review (8bffe05)

Pure rebase onto main (includes merged #223 + #224). The #226 commit itself is unchanged — same 3 files, same logic. No new issues.

APPROVED — same assessment as previous review on 06033e6.

[yujiawei]

Code Review — PR #226 (octo-server)

Verdict: APPROVED — clean, well-scoped fix with no blocking issues.

Summary

This PR fixes #222 by adding a ChannelTypeCommunityTopic branch to hasRevokePermission so that group owners/managers can revoke messages in community topics (Threads), which previously fell through to a deny. The shared role matrix is extracted into groupRoleRevokeAllowed, reused by both the Group and CommunityTopic branches.

Verification

I reviewed the diff against modules/message/api.go and the surrounding code at head 8bffe05.

• Pure refactor of the Group branch — confirmed. The extracted groupRoleRevokeAllowed(groupNo, fromUID, loginUID) is logically identical to the previous inline block (sender-left-group handling, creator/normal short-circuit, manager-revokes-normal rule, default deny). For ChannelTypeGroup, ChannelID is the group number, so passing messageM.ChannelID preserves the original behavior exactly. No regression.
• Parent-group resolution is consistent with existing code. The new topic branch uses thread.ParseChannelID to resolve the parent group, matching the established pattern in authorizeMutualDelete (api.go:1783) and enrichPayloadWithSpaceID (api.go:617). ParseChannelID returns an error on a missing/empty ____ separator, and the branch fails closed on parse error — correct and matches the delete2 path.
• Bot-owner short-circuit now applies to topics as well (it sits above the switch). This is consistent with the group behavior and is the intended outcome per the PR description.
• Membership enforcement. A non-member loginUID resolves to nil from GetMember → denied. Self-revoke is short-circuited at the top of hasRevokePermission before the matrix.
• Role constants used in the tests (group.MemberRoleCreator=1, MemberRoleManager=2, MemberRoleCommon=0) match common.GroupMemberRole*.

Tests

• revoke_topic_test.go covers the full parent-role matrix, channel-ID parse failure (fail-closed), the bot-owner branch, and the sender-left-group cases.
• revoke_topic_integration_test.go (integration tag) exercises the real group.Service.GetMember lookup, which guards against the realistic regression of querying with the raw topic channelID instead of the resolved parent group number.
• Locally verified: go build, go vet, go test, and go test -race on ./modules/message/ all pass.

Findings

No P0/P1 issues.

Suggestions (non-blocking, nits)

• The fail-closed log on parse failure ("解析子区频道ID失败，拒绝撤回") uses Warn. Given this path should be unreachable for well-formed topic messages reaching the revoke endpoint, that severity is reasonable; no change needed.
• The integration test creates a minimal user table to satisfy the LEFT JOIN in GetMember. This is self-contained and acceptable, though slightly brittle if the join shape changes — fine to leave as-is.

Overall this is a focused, correct fix with good test coverage. Approving.

[Jerry-Xin]

The PR is in scope and correctly extends message revoke authorization for community topics by reusing the parent group role model.

💬 Non-blocking

🔵 Suggestion — modules/message/revoke_topic_test.go:32 / modules/message/revoke_topic_test.go:39: the unit fake ignores groupNo, so the default unit test would not catch a future regression that passes the topic channel ID instead of the parsed parent group. Since modules/message/revoke_topic_integration_test.go:1 is build-tagged and may not run in normal CI, consider making the fake group service assert or key by groupNo.

✅ Highlights

• modules/message/api.go:2117 correctly adds ChannelTypeCommunityTopic handling and fails closed on invalid topic channel IDs.
• modules/message/api.go:2140 keeps the existing group permission matrix centralized without changing group behavior.
• The tests cover owner, manager, normal member, outsider, departed sender, invalid channel ID, and bot-owner paths.

Verification: targeted revoke tests passed with go test -run 'TestHasRevokePermission_(CommunityTopic|BotOwner|NotBotOwner|EmptyCreatorUID|RobotServiceError)' ./modules/message/. Full go test ./modules/message/ could not complete in this environment because Redis at 127.0.0.1:6379 is unavailable.