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

Snapshots: Fix deleting external snapshots when using RBAC #51897

Merged
merged 1 commit into from
Jul 7, 2022

Conversation

idafurjes
Copy link
Contributor

@idafurjes idafurjes commented Jul 7, 2022

What this PR does / why we need it:
When creating an external snapshot, its dashboard content is empty. This means that the mustInt here returns a 0, which before RBAC would result in a dashboard which has no ACL. A dashboard without an ACL would fallback to the user’s org role, which for editors and admins would essentially always be allowed here. With RBAC, all permissions must be explicit, so the lack of a rule for dashboard 0 means the guardian will reject.

Which issue(s) this PR fixes:

Fixes https://github.com/grafana/support-escalations/issues/3139

Special notes for your reviewer:

@idafurjes idafurjes added this to the 9.0.3 milestone Jul 7, 2022
@idafurjes idafurjes added no-changelog Skip including change in changelog/release notes backport v9.0.x add to changelog and removed no-changelog Skip including change in changelog/release notes labels Jul 7, 2022
@idafurjes idafurjes changed the title Do not check permissions while deleting external snapshot Snapshots: Fix deleting external snapshots when using RBAC Jul 7, 2022
@idafurjes idafurjes requested a review from gamab July 7, 2022 12:35
@idafurjes idafurjes marked this pull request as ready for review July 7, 2022 12:36
@idafurjes idafurjes requested a review from a team as a code owner July 7, 2022 12:36
@idafurjes idafurjes requested review from kylebrandt, zserge and sh0rez and removed request for a team July 7, 2022 12:36
@grafanabot
Copy link
Contributor

Copy link
Contributor

@gamab gamab left a comment

Choose a reason for hiding this comment

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

Thanks for the clarification comment! LGTM.

We discussed this with @sakjur, do you think it would make sense to create these snapshots with an ID, so that we can still perform RBAC on them? Some customers might want to prevent editors from deleting external snapshots? (=> Should we create an issue to keep track of this?)

@idafurjes idafurjes merged commit ee88b44 into main Jul 7, 2022
@idafurjes idafurjes deleted the ida/esc-3139 branch July 7, 2022 13:15
@sakjur
Copy link
Contributor

sakjur commented Jul 7, 2022

We discussed this with @sakjur, do you think it would make sense to create these snapshots with an ID, so that we can still perform RBAC on them? Some customers might want to prevent editors from deleting external snapshots? (=> Should we create an issue to keep track of this?)

I think so, yes! It seems like an oversight that that was not done already

@grafanabot
Copy link
Contributor

The backport to v9.0.x failed:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new branch
git switch --create backport-51897-to-v9.0.x origin/v9.0.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x ee88b44458f525914a39aacb24a08acac513184d
# Push it to GitHub
git push --set-upstream origin backport-51897-to-v9.0.x
git switch main
# Remove the local backport branch
git branch -D backport-51897-to-v9.0.x

Then, create a pull request where the base branch is v9.0.x and the compare/head branch is backport-51897-to-v9.0.x.

@sakjur
Copy link
Contributor

sakjur commented Jul 7, 2022

@idafurjes I'll open an issue for adding the ID to external dashboard snapshots and can take care of the backport to v9.0.x 🙂

@idafurjes
Copy link
Contributor Author

@sakjur I am already taking care of the backport, but thanks for opening the ticket

idafurjes added a commit that referenced this pull request Jul 7, 2022
idafurjes added a commit that referenced this pull request Jul 7, 2022
@JayEkin
Copy link

JayEkin commented Jul 18, 2022

Hi @idafurjes ,

I am still seeing the issue on 9.0.3 Cloud for the customer in https://github.com/grafana/support-escalations/issues/3139

"runningVersion": "9.0.3-0dc0087e (commit: 0dc0087e0, branch: HEAD)",

I can see this 51897 in https://grafana.com/docs/grafana/latest/release-notes/release-notes-9-0-3/

Could you take a look ?

@sakjur
Copy link
Contributor

sakjur commented Jul 18, 2022

@JayEkin 9.0.3-0dc0087e looks like a prerelease for 9.0.3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants