Validate subscription ownership before editing

[nevyangelova]

Summary

Load the existing subscription from the store and verify the user has permission to manage it before applying any changes.

Ticket Link

https://mattermost.atlassian.net/browse/MM-68272

[coderabbitai]

📝 Walkthrough

Walkthrough

The PR updates subscription edit authorization to verify permissions using the existing subscription's channel rather than the request's channel ID, with multi-step validation for channel transfers and a new test for non-existent subscriptions.

Changes

Cohort / File(s)	Summary
Authorization Logic Update server/subscribe.go	Modified httpChannelEditSubscription to look up existing subscription first, use its channel for initial permission/membership checks, conditionally validate target channel if different, and move channel-type validation to final step.
Test Case Updates server/http_test.go	Updated TestEditSubscription test cases to mock KV state retrieval and permission outcomes matching new authorization flow; added test case for non-existent subscription rejection.

Sequence Diagram

sequenceDiagram
    participant Client
    participant Handler as httpChannelEditSubscription
    participant KV as KV Store
    participant PermSvc as Permission Service
    participant ChannelSvc as Channel Service

    Client->>Handler: Edit subscription request
    Handler->>KV: getChannelSubscription(existing)
    KV-->>Handler: existingSub (or nil)
    alt Subscription not found
        Handler-->>Client: 400 BadRequest
    else Subscription exists
        Handler->>PermSvc: HasPermissionTo(existingSub.ChannelID)
        PermSvc-->>Handler: permission result
        Handler->>ChannelSvc: GetChannel(existingSub.ChannelID)
        ChannelSvc-->>Handler: channel info
        
        alt Transferring to different channel
            Handler->>PermSvc: HasPermissionTo(newChannelID)
            PermSvc-->>Handler: permission result
            Handler->>ChannelSvc: GetChannel(newChannelID)
            ChannelSvc-->>Handler: channel info
        end
        
        Handler->>ChannelSvc: Validate channel type (reject Direct/Group)
        ChannelSvc-->>Handler: validation result
        Handler-->>Client: 200 OK / error
    end

Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

• PR #1286 — Both PRs modify httpChannelEditSubscription and its tests, changing authorization and channel-type validation logic for subscription edit requests.

Suggested labels

2: Dev Review

Suggested reviewers

• avasconcelos114
• ogi-m

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'Validate subscription ownership before editing' directly describes the main security change: verifying user permission on the existing subscription before allowing edits, which is the core focus of both modified files.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch MM-68272-idor-subscription-edit

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[nevyangelova]

@edgarbellot is this the fix you had in mind?

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

server/http_test.go (1)

610-658: ⚠️ Potential issue | 🟡 Minor

Add test coverage for cross-channel subscription transfers.

The authorization logic includes a distinct code path at lines 1254-1266 in subscribe.go when a subscription is moved between channels (subscription.ChannelID != existingSub.ChannelID). This validates permissions and channel membership separately for the target channel, but the current tests don't exercise this path—all "Editing subscription" test cases keep the subscription in the same channel.

Add test cases for:

1. Successfully moving a subscription from Channel A to Channel B (user has permissions on both)
2. Rejecting a move when user has permission on the source channel but not the target channel

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@server/http_test.go` around lines 610 - 658, Add two TestEditSubscription
cases that cover cross-channel moves: create one case "Move to another channel -
allowed" where the existing subscription returned by the KVGet mock has a
different ChannelID than the incoming payload and mock plugintest.API so
HasPermissionTo returns true for both source and target and the
channel-membership check for the target channel (the API method used in
subscribe.go to verify membership) returns a valid member; assert success (200).
Add another case "Move to another channel - forbidden on target" with the same
KVGet existing subscription but mock HasPermissionTo/target-membership so the
user lacks permission or membership on the target channel and assert
http.StatusForbidden. Reuse the TestEditSubscription structure, update the
apiCalls closures to return the existing subscription with a different ChannelID
and to set HasPermissionTo and the channel-membership API mock responses
appropriately to exercise the subscribe.go branch that handles
subscription.ChannelID != existingSub.ChannelID.

🧹 Nitpick comments (1)

server/subscribe.go (1)

1300-1308: Consider notifying the source channel when a subscription is moved.

When a subscription is transferred from one channel to another, only the target channel receives a notification. Users in the original channel won't know the subscription was moved away.

This is optional UX polish - the current behavior may be intentional if the team prefers minimal notifications.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@server/subscribe.go` around lines 1300 - 1308, The current code only creates
a notification post in the new channel via p.client.Post.CreatePost; update it
to also notify the source channel when a subscription is moved by comparing the
old channel ID (capture the previous channel ID before updating, e.g.,
oldChannelID) with subscription.ChannelID and if they differ call
p.client.Post.CreatePost with UserId p.getConfig().botUserID and ChannelId =
oldChannelID and a message like "Jira subscription, \"%v\", was moved to %v by
%v" (include subscription.Name, the target channel name or ID, and
connection.DisplayName); handle and return errors the same way (using respondErr
and errors.WithMessage) and avoid sending duplicate notifications when channels
are equal.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Outside diff comments:
In `@server/http_test.go`:
- Around line 610-658: Add two TestEditSubscription cases that cover
cross-channel moves: create one case "Move to another channel - allowed" where
the existing subscription returned by the KVGet mock has a different ChannelID
than the incoming payload and mock plugintest.API so HasPermissionTo returns
true for both source and target and the channel-membership check for the target
channel (the API method used in subscribe.go to verify membership) returns a
valid member; assert success (200). Add another case "Move to another channel -
forbidden on target" with the same KVGet existing subscription but mock
HasPermissionTo/target-membership so the user lacks permission or membership on
the target channel and assert http.StatusForbidden. Reuse the
TestEditSubscription structure, update the apiCalls closures to return the
existing subscription with a different ChannelID and to set HasPermissionTo and
the channel-membership API mock responses appropriately to exercise the
subscribe.go branch that handles subscription.ChannelID !=
existingSub.ChannelID.

---

Nitpick comments:
In `@server/subscribe.go`:
- Around line 1300-1308: The current code only creates a notification post in
the new channel via p.client.Post.CreatePost; update it to also notify the
source channel when a subscription is moved by comparing the old channel ID
(capture the previous channel ID before updating, e.g., oldChannelID) with
subscription.ChannelID and if they differ call p.client.Post.CreatePost with
UserId p.getConfig().botUserID and ChannelId = oldChannelID and a message like
"Jira subscription, \"%v\", was moved to %v by %v" (include subscription.Name,
the target channel name or ID, and connection.DisplayName); handle and return
errors the same way (using respondErr and errors.WithMessage) and avoid sending
duplicate notifications when channels are equal.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository UI (base), Organization UI (inherited)

Review profile: CHILL

Plan: Pro

Run ID: dc62e413-1238-4305-a975-ac7956ee0a04

📥 Commits

Reviewing files that changed from the base of the PR and between 737f3cb and bdb08fc.

📒 Files selected for processing (2)

• server/http_test.go
• server/subscribe.go

[avasconcelos114]

The logic LGTM! It might be good to also add a test case for when the user lacks permissions for the target channel

[ogi-m]

LGTM!

• User B without access to the channel gets blocked with 403 because the server checks permissions against the original channel (User A's), not the channel User B supplied in the request body
• Fabricated subscription ID correctly rejected with 400
• User A can still edit their own subscription normally, no regression

[edgarbellot]

@nevyangelova yep, this correctly fixes the vuln. Thank you!