MM-69953 Fixing inconsistent behavior for issue webhook events - #689
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughConfidential issue subscriptions now check group access, apply project and group permission filtering, and propagate confidentiality through issue, note, and other webhook lookups. Tests cover authorization, access evaluation, filtering, channel routing, and flag propagation. ChangesConfidential issue access and delivery
Estimated code review effort: 4 (Complex) | ~45 minutes Mergeability Score: ⚪ Minimal · up to The pull request updates issue webhook behavior and related tests; no actionable merge-blocking risk remains based on the supplied evidence. Sequence Diagram(s)sequenceDiagram
participant GitLabWebhook
participant WebhookHandler
participant gitlabRetreiver
participant Plugin
GitLabWebhook->>WebhookHandler: issue or note event with confidentiality
WebhookHandler->>gitlabRetreiver: GetSubscribedChannelsForProject with isConfidential
gitlabRetreiver->>Plugin: GetSubscribedChannelsForProject
Plugin-->>gitlabRetreiver: filtered subscriptions
gitlabRetreiver-->>WebhookHandler: subscribed channels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@server/webhook/issue_test.go`:
- Around line 208-212: Extend the assertions in the result-comparison loop to
compare each expected test.res[index].ToChannels value with
res[index].ToChannels, alongside the existing Message, ToUsers, and From checks,
so confidential-delivery results validate channel recipients.
In `@server/webhook/note.go`:
- Around line 67-71: Update the subscription delivery loop in the note webhook
handler to skip confidential issue comments unless each subscription’s
ConfidentialIssues() opt-in is enabled, matching the existing handleChannelIssue
behavior; add a regression test in note_test.go covering a non-opted-in
subscription.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 2489b9ed-5e5c-4b32-b9dd-b2747831188d
📒 Files selected for processing (19)
server/command.goserver/command_test.goserver/subscriptions.goserver/subscriptions_test.goserver/webhook.goserver/webhook/deployment.goserver/webhook/issue.goserver/webhook/issue_fixture_test.goserver/webhook/issue_test.goserver/webhook/jobs.goserver/webhook/merge_request.goserver/webhook/note.goserver/webhook/note_test.goserver/webhook/pipeline.goserver/webhook/push.goserver/webhook/release.goserver/webhook/tag.goserver/webhook/webhook.goserver/webhook/webhook_test.go
|
@CodeRabbit approve if the last commit addressed the previously raised concerns |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="mattermost/mattermost-plugin-gitlab"
pr="689"
echo "== PR metadata and commits =="
gh pr view "$pr" --repo "$repo" --json url,headRefOid,commits,reviews \
--jq '{url, headRefOid, commits: [.commits[] | {oid: .oid, messageHeadline: .messageHeadline, committedDate: .committedDate}], reviews: [.reviews[] | {author: .author.login, state: .state, submittedAt: .submittedAt, body: .body}]}'
echo
echo "== CodeRabbit PR review comments =="
gh api --paginate "repos/$repo/pulls/$pr/comments?per_page=100" \
--jq '.[] | select(.user.login == "coderabbitai") | {id, path, line, original_line, commit_id, created_at, body, html_url}'
echo
echo "== Review threads and resolution status =="
gh api graphql -F owner="mattermost" -F name="mattermost-plugin-gitlab" -F number="$pr" -f query='
query($owner: String!, $name: String!, $number: Int!) {
repository(owner: $owner, name: $name) {
pullRequest(number: $number) {
reviewThreads(first: 100) {
nodes {
isResolved
path
line
comments(first: 20) {
nodes {
author { login }
body
url
commit { oid }
}
}
}
}
}
}
}' --jq '.data.repository.pullRequest.reviewThreads.nodes[]
| select(any(.comments.nodes[]; .author.login == "coderabbitai"))
| {isResolved, path, line, comments: [.comments.nodes[] | {author: .author.login, body, url, commit: .commit.oid}]}'
echo
echo "== Latest commit patch =="
latest=$(gh pr view "$pr" --repo "$repo" --json headRefOid --jq .headRefOid)
git show --format=fuller --stat "$latest"
git show --format= --unified=80 "$latest" -- server/command.go server/subscriptions.go server/webhook/issue.go server/webhook/note.goLength of output: 17515 🏁 Script executed: #!/bin/bash
set -euo pipefail
latest="80025be463b3cdcfe852b4f5e15ef74ae14149f4"
echo "== Latest test-only diff =="
git show --format= --unified=100 "$latest" -- \
server/webhook/issue_test.go \
server/webhook/note_test.go
echo
echo "== Relevant test declarations =="
ast-grep outline server/webhook/note_test.go --items all --type function --match 'Confidential|IssueComment'
ast-grep outline server/webhook/issue_test.go --items all --type function --match 'ConfidentialIssueWebhook'Length of output: 13526
[approve] You are interacting with an AI system. |
| // Check if the user has guest permission or less for both project and group level | ||
| if (userPermission.ProjectAccess != nil && userPermission.ProjectAccess.AccessLevel <= gitlabLib.GuestPermissions) || (userPermission.GroupAccess != nil && userPermission.GroupAccess.AccessLevel <= gitlabLib.GuestPermissions) { | ||
| return false | ||
| } |
There was a problem hiding this comment.
The core issue seems to still be here, if both ProjectAccess and GroupAccess is nil, you skip the checks. The recommendation is to add a helper to handle this:
func effectiveAccess(perms *gitlabLib.Permissions) gitlabLib.AccessLevelValue {
if perms == nil {
return 0
}
var level gitlabLib.AccessLevelValue
if perms.ProjectAccess != nil && perms.ProjectAccess.AccessLevel > level {
level = perms.ProjectAccess.AccessLevel
}
if perms.GroupAccess != nil && perms.GroupAccess.AccessLevel > level {
level = perms.GroupAccess.AccessLevel
}
return level
}
// For confidential content, require > Guest
// return effectiveAccess(result.Permissions) > gitlabLib.GuestPermissions
I definitely think this is more readable and more correct
|
Note: Testing the cherry-pick automation - I'll be closing any PRs prematurely created as part of the tests |
| subs := w.gitlabRetreiver.GetSubscribedChannelsForProject( | ||
| ctx, namespace, project, | ||
| repo.Visibility == gitlab.PublicVisibility, | ||
| event.Issue.Confidential, |
There was a problem hiding this comment.
I think CodeRabbit didn't quite go far enough here - sorry I missed it. We should use the same metric for the comments as for handleChannelIssue and add
isConfidential := event.Issue.Confidential || event.EventType == string(gitlab.EventConfidentialNote)
above and use isConfidential in place of the bare event.Issue.Confidential (So we catch it either way.)
We would also want to add a regression test where event_type is confidential_note and issue.confidential is absent/false.
jgheithcock
left a comment
jgheithcock
left a comment
There was a problem hiding this comment.
I'm marking this approved so you don't need a re-review, but I do think we should be consistent here (as well as catching this edge case).
hmohammed-prodsec
left a comment
hmohammed-prodsec
left a comment
There was a problem hiding this comment.
Thanks for the changes, LGTM 👍
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
* MM-69953 Fixing inconsistent behavior for issue webhook events (#689) * MM-69953 Fixing inconsistent behavior for issue webhook events * Applying PR feedback * Added function to improve readability and type safety * Making confidential checks more consistent * Applying PR feedback * Apply lint fixes Co-authored-by: Andre Vasconcelos <andre.onogoro@gmail.com> --------- Co-authored-by: Andre Vasconcelos <a-andre.vasconcelos@mattermost.com> Co-authored-by: Cursor Agent <cursoragent@cursor.com> Co-authored-by: Andre Vasconcelos <andre.onogoro@gmail.com>
* MM-69953 Fixing inconsistent behavior for issue webhook events (#689) * MM-69953 Fixing inconsistent behavior for issue webhook events * Applying PR feedback * Added function to improve readability and type safety * Making confidential checks more consistent * Applying PR feedback * Apply lint fixes Co-authored-by: Andre Vasconcelos <andre.onogoro@gmail.com> --------- Co-authored-by: Andre Vasconcelos <a-andre.vasconcelos@mattermost.com> Co-authored-by: Cursor Agent <cursoragent@cursor.com> Co-authored-by: Andre Vasconcelos <andre.onogoro@gmail.com>
3 participants
Summary
This PR closes a gap in behaviors that happen across different types of issues and how webhooks handle them
Ticket Link
Fixes https://mattermost.atlassian.net/browse/MM-69953
Change Impact: 🟠 Medium
Reasoning: The change updates a shared webhook subscription API across multiple handlers and adds authorization checks for confidential issues. Tests cover confidential and non-confidential notification paths, which reduces regression risk.
Regression Risk: Medium—incorrect confidentiality propagation or permission handling could affect notification delivery across several webhook event types.
QA Recommendation: Perform targeted manual QA for confidential issues, issue comments, and group/project subscriptions. Verify that non-confidential webhook events remain unchanged. Skipping manual QA carries moderate risk.
Generated by CodeRabbitAI