Repository navigation
MM-69161 Adding permission check model to allow/deny requests - #678
Conversation
- Applied permission model to webhook slash commands
|
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 (3)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe system-admin authorization gate is removed from ChangesNamespace-Based Webhook Authorization with Audit Logging
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
|
@edgarbellot This PR here adds the general flow we had previously discussed, but now that I see it in action I can't help but wonder if we should just remove the admin check from the webhook commands instead as Gitlab's own API would be rejecting a call made with the oauth token of a user that doesn't have the needed permissions anyway :D Unless we see any value in preventing an API call from being made to Gitlab until we're sure the user has due access beforehand, but for now I kept the changes and flow until we reach a decision, it would be quite quick and easy to revert the previous commit and add a new one that doesn't enforce sys admin checks for these two commands |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
server/gitlab/permissions_test.go (1)
55-63: ⚡ Quick winAdd explicit 403 coverage for both access-level lookups.
These tests currently pin 404 and 500, but not the 403 mapping path that influences downstream authorization fallback behavior. Adding a focused 403 case for each method will lock this contract down.
Suggested test additions
func TestGetProjectAccessLevel(t *testing.T) { @@ t.Run("returns error on server failure", func(t *testing.T) { @@ }) + + t.Run("returns ErrForbidden on forbidden response", func(t *testing.T) { + client, token := newTestClient(t, func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusForbidden) + _, _ = w.Write([]byte(`{"message":"403 Forbidden"}`)) + }) + + level, err := client.GetProjectAccessLevel(context.Background(), user, token, "group", "project") + assert.ErrorIs(t, err, ErrForbidden) + assert.Equal(t, internGitlab.NoPermissions, level) + }) } @@ func TestGetGroupAccessLevel(t *testing.T) { @@ t.Run("returns error on server failure", func(t *testing.T) { @@ }) + + t.Run("returns ErrForbidden on forbidden response", func(t *testing.T) { + client, token := newTestClient(t, func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusForbidden) + _, _ = w.Write([]byte(`{"message":"403 Forbidden"}`)) + }) + + level, err := client.GetGroupAccessLevel(context.Background(), user, token, "group") + assert.ErrorIs(t, err, ErrForbidden) + assert.Equal(t, internGitlab.NoPermissions, level) + }) }Also applies to: 92-100
🤖 Prompt for 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. In `@server/gitlab/permissions_test.go` around lines 55 - 63, Add explicit 403 test cases for both access-level lookup methods: when the test HTTP handler returns http.StatusForbidden, call client.GetProjectAccessLevel(...) and client.GetGroupAccessLevel(...) and assert the returned error is the forbidden sentinel (use require.ErrorIs/ assert.ErrorIs with ErrForbidden or the appropriate forbidden error constant). Place one test alongside the existing "returns error on server failure" block for the project path and a matching one alongside the analogous group path (the block around lines 92-100) so the 403->forbidden mapping is covered for both GetProjectAccessLevel and GetGroupAccessLevel.
🤖 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.
Nitpick comments:
In `@server/gitlab/permissions_test.go`:
- Around line 55-63: Add explicit 403 test cases for both access-level lookup
methods: when the test HTTP handler returns http.StatusForbidden, call
client.GetProjectAccessLevel(...) and client.GetGroupAccessLevel(...) and assert
the returned error is the forbidden sentinel (use require.ErrorIs/
assert.ErrorIs with ErrForbidden or the appropriate forbidden error constant).
Place one test alongside the existing "returns error on server failure" block
for the project path and a matching one alongside the analogous group path (the
block around lines 92-100) so the 403->forbidden mapping is covered for both
GetProjectAccessLevel and GetGroupAccessLevel.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 78bfbbbe-1c0b-4e13-9926-a5d8a4ee7aa5
📒 Files selected for processing (9)
server/command.goserver/command_test.goserver/gitlab/gitlab.goserver/gitlab/mocks/mock_gitlab.goserver/gitlab/permissions.goserver/gitlab/permissions_test.goserver/mocks/mock_gitlab.goserver/permissions.goserver/permissions_test.go
| } | ||
| if err != nil { | ||
| return internGitlab.NoPermissions, err | ||
| } |
There was a problem hiding this comment.
nit: If the request errored out at the transport layer resp could be nil here depending on checkResponse's contract. I would swap the order so you check err != nil first when resp == nil. Same pattern repeats in GetGroupAccessLevel.
| return internGitlab.NoPermissions, err | ||
| } | ||
|
|
||
| endpoint := fmt.Sprintf("groups/%s/members/all/%d", internGitlab.PathEscape(group), user.GitlabUserID) |
There was a problem hiding this comment.
Nit: here we use internGitlab.PathEscape for the group identifier but GetProjectAccessLevel above does fmt.Sprintf("%s/%s", namespace, project) with no escaping. Can we align both to use PathEscape for symmetry?
|
|
||
| required, ok := minimumAccessLevels[action][scope] | ||
| if !ok { | ||
| return false, errors.Errorf("no minimum access level configured for action %q scope %q", action, scope) |
There was a problem hiding this comment.
Consider making this a build time guarantee like a func init() that verifies every permissionAction constant has a complete Project/Group entry in minimumAccessLevels. That way its caught at plugin boot isntead when we run the command in production.
| } | ||
| if !allowed { | ||
| return insufficientPermissionsMessage | ||
| } |
There was a problem hiding this comment.
The err is logged but then discarded when allowed's zero value is false so a misconfig silently surfaces to the user as insufficientPermissionsMessage which might be confusing UX
| gitlab.AddCommand(settings) | ||
|
|
||
| webhook := model.NewAutocompleteData("webhook", "[command]", "Available Commands: list, add") | ||
| webhook.RoleID = model.SystemAdminRoleId |
There was a problem hiding this comment.
Would this mean that if a non-Maintainer will see and try the command they will hit the denial message?
| @@ -0,0 +1,110 @@ | |||
| // Copyright (c) 2019-present Mattermost, Inc. All Rights Reserved. | |||
There was a problem hiding this comment.
Nice coverage on the matrix. I would add a couple cases for full safety but living up to your consideration!
- Action with no min-access-level configured which exercises the errors.Errorf("no minimum access level configured for action %q scope %q", ...) branch.
- requireWebhookPermission directly as currently only hasGitlabPermission is unit-tested.
nang2049
left a comment
nang2049
left a comment
There was a problem hiding this comment.
Amazing change thanks @avasconcelos114 a few minor comments!
I agree, let's remove the check and rely on GitLab. Since these calls use the user's own OAuth token, GitLab is already the source of truth and rejects anything they can't do. That holds for sysadmins too. |
edgarbellot
left a comment
edgarbellot
left a comment
There was a problem hiding this comment.
Looking good! just one nit. Thanks Andre!
| return namespaceErr.Error() | ||
| } | ||
|
|
||
| if msg := p.requireWebhookPermission(ctx, info, group, project); msg != "" { |
There was a problem hiding this comment.
nit: webhook add creates a state-changing resource but doesn't emit an audit record, unlike other state-changing ops in the repo (createIssue). Since you're touching the auth flow here it could be a good spot to add a MakeAuditRecord / LogAuditRec capturing the actor and target. Non-blocking :)
There was a problem hiding this comment.
Ah yep sounds good! so for this PR I'll now:
- Revert the previous PR with the explicit permission checks
- Remove the sysadmin checks for webhook commands in order to allow Gitlab's own permission checks to kick in (with some graceful error handling so it's clear why was the call rejected)
- Add audit logging for the webhook command handlers
There was a problem hiding this comment.
with some graceful error handling so it's clear why was the call rejected
good call!
Sounds good - happy to review the changes once they are ready.
This reverts commit 24a987e.
- Removed active permission checks and allowing gitlab to dictate outcome of request
There was a problem hiding this comment.
🧹 Nitpick comments (1)
server/command.go (1)
1162-1162: 💤 Low valueAutocomplete visibility restricts to sysadmin but command execution does not.
The
RoleID = model.SystemAdminRoleIdhides the webhook commands from non-admin users in autocomplete suggestions, but the actual command handler no longer enforces admin authorization (relying on GitLab API permissions instead). This creates a UX gap where eligible users (GitLab maintainers who aren't Mattermost admins) won't discover the command through autocomplete.Consider removing the
RoleIDrestriction to align autocomplete visibility with the new permission model, or document that users can manually type/gitlab webhookeven if it doesn't appear in suggestions.🤖 Prompt for 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. In `@server/command.go` at line 1162, The webhook command's RoleID is set to model.SystemAdminRoleId, which restricts autocomplete visibility to system admins only, but the command handler no longer enforces admin authorization and instead relies on GitLab API permissions. Remove the RoleID restriction (or set it to an empty/default value) at the line where webhook.RoleID is assigned to align autocomplete visibility with the actual permission model that checks GitLab maintainer status rather than Mattermost admin status.
🤖 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.
Nitpick comments:
In `@server/command.go`:
- Line 1162: The webhook command's RoleID is set to model.SystemAdminRoleId,
which restricts autocomplete visibility to system admins only, but the command
handler no longer enforces admin authorization and instead relies on GitLab API
permissions. Remove the RoleID restriction (or set it to an empty/default value)
at the line where webhook.RoleID is assigned to align autocomplete visibility
with the actual permission model that checks GitLab maintainer status rather
than Mattermost admin status.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: eba732ac-ca9d-47d2-8632-7fc4ed0a324a
📒 Files selected for processing (4)
server/audit.goserver/command.goserver/command_test.goserver/utils.go
|
Made the changes necessary to simplify this quite a bit by simply allowing Gitlab to reject the request made with the user's oauth token and removed the sysadmin checks previously in place. I've also gone ahead and enforced the same checks we had in other commands for namespace limiting via the |
| } | ||
| model.AddEventParameterAuditableToAuditRec(auditRec, "add_webhook", auditParams) | ||
|
|
||
| if err := p.isNamespaceAllowed(resolvedNamespace); err != nil { |
…680) * MM-69161 Adding permission check model to allow/deny requests - Applied permission model to webhook slash commands * Revert "MM-69161 Adding permission check model to allow/deny requests" This reverts commit 24a987e. * Adding audit logging to webhook command - Removed active permission checks and allowing gitlab to dictate outcome of request * Allowing webhook autocomplete to be visible to standard users * Added more human-readable error responses to webhook cmd
Summary
This PR adds a layer that checks gitlab's own permission model to prevent any state-changing requests from being made, and failing that it falls back to the current flow for only allowing sysadmins from accessing a particular command. This has been for now only integrated with the
/gitlab webhookcommandsTicket Link
Fixes https://mattermost.atlassian.net/browse/MM-69161
Change Impact: 🟠 Medium
Reasoning: The PR changes authorization behavior for
/gitlab webhookby removing a local sysadmin gate and relying on GitLab permission enforcement plus namespace allow/deny checks, and it extends audit logging for webhook creation. While this is security-relevant, the blast radius is primarily confined to webhook command flows and is supported by newly added namespace-denial tests.Regression Risk: Medium—command access semantics change (admin-only → namespace-scoped/remote-permission-scoped), and new early-denial paths can alter behavior for existing configurations; however, the affected logic is localized and key denial/success scenarios are covered by tests.
QA Recommendation: Moderate manual QA. Validate for both admin and non-admin users:
/gitlab webhook listfor allowed vs disallowed namespaces (including denial messaging and no hook retrieval),/gitlab webhook addfor allowed vs disallowed namespaces (including denial messaging),Generated by CodeRabbitAI