Skip to content

feat: declare required RBAC permissions on each tool - #1306

Merged
sd2k merged 8 commits into
mainfrom
feat/tool-rbac-permissions
Oct 8, 2026
Merged

sd2k merged 8 commits into
mainfrom
feat/tool-rbac-permissions

Conversation

@sd2k

@sd2k sd2k commented Oct 7, 2026 •

Copy link
Copy Markdown
Collaborator

Every tool now declares the Grafana RBAC actions its calls need, on its MustTool call:

var QueryFoo = mcpgrafana.MustTool("query_foo", "...", queryFoo,
	mcpgrafana.WithReadOnlyHintAnnotation(true),
	mcpgrafana.RequiresPermissions(datasourceQuery...),
)

A caller missing any of them gets a 403 from that tool, so this gives one place to see what a tool requires.

  • RequiresPermissions (permissions.go) is a ToolOption. It writes the actions to the tool's _meta under grafana.com/requiredPermissions. Repeated calls add to the list, so a shared group can be combined with extra actions. ToolRequiredPermissions reads them back, including from a tools/list response, so anything embedding or listing the server can get at them.
  • tools/rbac.go holds the shared groups (datasourceQuery, dashboardRead, the incident and agento11y groups). Most new tools only need datasourceQuery.
  • TestEveryToolDeclaresPermissions lists every tool in read-write and read-only mode and fails if one has no declaration. The failure message says exactly what to add. Tools that need nothing declare RequiresPermissions().
  • The README's "Required RBAC Permissions" column is now generated from the declarations. TestReadmeToolPermissions fails on drift, or when a registered tool has no row, and -update-readme regenerates it. This adds the 20 tools that were missing from the table and fixes several stale entries (e.g. the snapshot tools listed dashboards:read). The Scopes column stays hand-written, since scopes depend on how a tool is used.

The tests check that every tool has made a declaration, not that the declared actions are correct.

Notes for review:

  • _meta isn't counted by the token check (it counts name, description and input schema), and clients don't pass it to models.
  • Tools that call /apis endpoints (dashboard reads and writes, provisioning) declare both the user's legacy action (dashboards:read) and the on-behalf-of token's group/resource:verb permission (dashboard.grafana.app/dashboards:get). Grafana checks both on those paths. The README notes that service accounts only need the legacy names.
  • General (not rule-scoped) silences are gated on alert.instances:*, not alert.silences:*, so alerting_silences_write lists both.
  • Read-write variants declare their write actions; the read-only agento11y_manage_* variants declare only reads.
  • grafana_api_request declares nothing. It runs with whatever the caller already holds.

🤖 Generated with Claude Code

@github-actions github-actions Bot added the cla: yes Contributor License Agreement is signed label Oct 7, 2026
@sd2k
sd2k marked this pull request as ready for review October 7, 2026 19:06
@sd2k
sd2k requested a review from a team as a code owner October 7, 2026 19:06
@sd2k sd2k changed the title feat(tools): declare required RBAC actions per tool feat: declare required RBAC permissions on each tool Oct 7, 2026
sd2k and others added 6 commits October 7, 2026 23:57
Hosted deployments (Grafana Cloud MCP) mint a delegated token from a
hand-curated permission list, so a tool whose action is missing there
403s even for an Admin. Running every tool against the hosted server
found five such gaps (silences, contact points, snapshots, annotation
delete).

Add tools.RequiredPermissions, mapping every tool to the RBAC actions it
needs, and a test that fails when a registered tool has no entry or an
entry names a tool that no longer exists. The hosted server can build
and check its delegated scopes from this map instead of a list kept in
sync by hand.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
On-behalf-of tokens are checked against group/resource:verb permissions for
/apis calls, in addition to the user's mapped legacy action.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Replace the tools.RequiredPermissions map with a RequiresPermissions
ToolOption that writes the actions to the tool's _meta under
grafana.com/requiredPermissions. The declaration now sits on the
MustTool call, so a copied tool brings it along, and read-only variants
declare only the read actions. Consumers read it back with
ToolRequiredPermissions, including from a tools/list response.

tools/rbac.go keeps the shared action groups. The coverage test now
checks every listed tool in read-write and read-only mode.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
TestReadmeToolPermissions rewrites the tool table's Required RBAC
Permissions column from RequiresPermissions (with -update-readme) and
fails when it drifts or a registered tool has no row. Adds the 20 tools
that were missing from the table and corrects several stale entries.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@sd2k
sd2k force-pushed the feat/tool-rbac-permissions branch from 9fea964 to 7b00840 Compare October 7, 2026 22:58

@cursor cursor Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Want higher recall? High effort reviews run extra passes and find more bugs. A team admin can switch effort levels in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 7b00840. Configure here.

Comment thread tools/oncall.go
mcpgrafana.WithReadOnlyHintAnnotation(true),
mcpgrafana.WithDestructiveHintAnnotation(false),
mcpgrafana.WithOpenWorldHintAnnotation(false),
mcpgrafana.RequiresPermissions("plugins.app:access", "grafana-irm-app.schedules:read"),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OnCall tools drop oncall-app RBAC

Medium Severity

OnCall tools now declare only grafana-irm-app.* actions. Grafana still authorizes OSS and non-IRM OnCall with grafana-oncall-app.*, and only switches to the IRM prefix when IRM is enabled. Callers following these declarations will grant actions that do not match many OnCall instances, so those tools 403. Other dual-checked APIs in this PR list both names.

Additional Locations (2)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 7b00840. Configure here.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not changing this: the OnCall tools only call grafana-irm-app routes (/api/plugins/grafana-irm-app/resources in tools/oncall_proxy.go, and the settings lookup in tools/oncall.go). On an instance running only grafana-oncall-app, they fail regardless of RBAC, so declaring grafana-oncall-app.* actions would list permissions no tool call is ever checked against. If the tools grow an oncall-app fallback, that change should add the matching actions.

- get_panel_image: declare datasources:read, datasources:explore and
  provisioning.repositories:read for the explore and preview modes.
- alerting_manage_routing, alerting_rules_read: declare the datasource
  actions their datasource_uid paths need.
- Incident tools: IRM has no incident RBAC actions, so declare only
  plugins.app:access.
- get_plugin: declare plugins.app:access for app plugin settings.
- ToolRequiredPermissions returns a copy, not the shared slice.
- README: list write-only actions apart for read-only variants, fix
  snapshot scopes, and parse the table safely (escaped pipes, row
  length).
- Check every write/query flag combination for declarations.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

@radiohead radiohead left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Found three snapshot permission declaration issues, detailed inline. Verified against the linked public Grafana source; no live permission-restricted or OBO endpoint testing was performed. The full local unit suite passed.

Comment thread tools/snapshot.go Outdated
Comment thread tools/snapshot.go Outdated
Comment thread tools/snapshot.go Outdated
- get_snapshot: GET /api/snapshots/:key has no RBAC check, so declare
  no actions.
- create_snapshot: regular mode also checks dashboards:read on the
  payload's dashboard.
- delete_snapshot: a non-creator needs dashboards:write on the
  snapshot's dashboard.

Restore the dashboard UID scope in the README for create and delete.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

@radiohead radiohead left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rechecked commit 000aa4f: all three snapshot permission findings are addressed, and the README permissions and dashboard UID scopes match the corrected declarations. The full local unit suite passed, including the permission declaration and README checks. No new findings in the follow-up diff.

@sd2k
sd2k merged commit 5caea2d into main Oct 8, 2026
27 of 28 checks passed
@sd2k
sd2k deleted the feat/tool-rbac-permissions branch October 8, 2026 13:48
sd2k added a commit that referenced this pull request Oct 9, 2026
#1306 made the README tool table complete and test-enforced, but the
copy in docs/sources/reference/mcp-tools-table.md is still maintained
by hand and was missing 13 tools, including grafana_api_request and the
Graphite, plugin and datasource management tools. Add them, using the
README rows (whose permissions are generated from each tool's
declaration), and add a test that the docs table lists exactly the
registered tools.

Also mark Admin and InfluxDB as off by default in the README, matching
the docs table and the --enabled-tools default.

Fixes #1318

Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla: yes Contributor License Agreement is signed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants