Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: update internal labels endpoints to work with RBAC #5099

Merged

Conversation

joeyorlando
Copy link
Contributor

@joeyorlando joeyorlando commented Sep 30, 2024

What this PR does

Related to:

Checklist

  • Unit, integration, and e2e (if applicable) tests updated
  • Documentation added (or pr:no public docs PR label added if not required)
  • Added the relevant release notes label (see labels prefixed w/ release:). These labels dictate how your PR will
    show up in the autogenerated release notes.

@joeyorlando joeyorlando added pr:no public docs Added to a PR that does not require public documentation updates release:patch PR will be added to "Other Changes" section of release notes labels Sep 30, 2024
@joeyorlando joeyorlando requested a review from a team as a code owner September 30, 2024 14:23
@@ -545,7 +546,7 @@ def web_link(self) -> str:
@property
def declare_incident_link(self) -> str:
"""Generate a link for AlertGroup to declare Grafana Incident by click"""
incident_link = urljoin(self.channel.organization.grafana_url, "a/grafana-incident-app/incidents/declare/")
incident_link = urljoin(self.channel.organization.grafana_url, f"a/{PluginID.INCIDENT}/incidents/declare/")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll finish renaming the hardcoded plugin IDs, to use PluginID, in a subsequent PR

Comment on lines +251 to +253
LABEL_CREATE = LegacyAccessControlCompatiblePermission(
Resources.LABEL, Actions.CREATE, LegacyAccessControlRole.EDITOR, prefix=PluginID.LABELS
)
Copy link
Member

@vadimkerr vadimkerr Oct 2, 2024

Choose a reason for hiding this comment

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

I only see the Labels Reader and Labels Writer roles in the existing Grafana RBAC UI, is this PR introducing a new one for create? maybe it's a good idea to only keep read and write (so create is part of write)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

tldr; no it's not introducing any new RBAC roles. The LABEL_CREATE, LABEL_READ, and LABEL_WRITE variables are just helpers which translate to the permission strings grafana-labels-app:label:create, grafana-labels-app:label:read, and grafana-labels-app:label:write, respectively.


Those are the RBAC roles that're available from Labels. Roles grant >= 1 permission(s) . These are the permissions granted by those labels RBAC roles:

"roles": [
    {
      "role": {
        "name": "Labels Writer",
        "description": "Read/write/create/delete Labels",
        "permissions": [
          { "action": "plugins.app:access", "scope": "plugins:id:grafana-labels-app" },
          { "action": "grafana-labels-app.label:read" },
          { "action": "grafana-labels-app.label:write" },
          { "action": "grafana-labels-app.label:create" },
          { "action": "grafana-labels-app.label:delete" }
        ]
      },
      "grants": ["Admin", "Editor"]
    },
    {
      "role": {
        "name": "Labels Reader",
        "description": "Read-only access to Labels",
        "permissions": [
          { "action": "plugins.app:access", "scope": "plugins:id:grafana-labels-app" },
          { "action": "grafana-labels-app.label:read" }
        ]
      },
      "grants": ["Viewer"]
    }
  ]

so in OnCall we're basically validating that the user has the necessary permission (action), we don't necessarily care about the role names themselves.

I agree that :write and :create should be consolidated over in labels but that's the current state of things. If you're curious, see these resources for more context:

engine/apps/api/permissions.py Show resolved Hide resolved
engine/conftest.py Outdated Show resolved Hide resolved
Co-authored-by: Vadim Stepanov <vadimkerr@gmail.com>
@joeyorlando joeyorlando added this pull request to the merge queue Oct 2, 2024
Merged via the queue into dev with commit f39a755 Oct 2, 2024
25 checks passed
@joeyorlando joeyorlando deleted the jorlando/update-internal-labels-endpoints-to-work-with-rbac branch October 2, 2024 18:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr:no public docs Added to a PR that does not require public documentation updates release:patch PR will be added to "Other Changes" section of release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants