Skip to content

Query History: Prevent viewers from accessing#88735

Merged
gelicia merged 11 commits into
mainfrom
kristina/query-history-viewer-lock
Jul 19, 2024
Merged

Query History: Prevent viewers from accessing#88735
gelicia merged 11 commits into
mainfrom
kristina/query-history-viewer-lock

Conversation

@gelicia

@gelicia gelicia commented Jun 5, 2024

Copy link
Copy Markdown
Contributor

What is this feature?

This adds a permissions check to ensure a user attempting to access query history has access to do so.

Why do we need this feature?

There is an edge case where a user had, then lost access, but still can access their previously ran queries. Viewers should not have access to queries. In Query History, users can only access their own queries, so this is a low priority security issue.

Which issue(s) does this PR fix?:

Fixes #88722

Special notes for your reviewer:
Tested this with an editor, viewer, and viewer with

[users]
viewers_can_edit = true

Please check that:

  • It works as expected from a user's perspective.
  • If this is a pre-GA feature, it is behind a feature toggle.
  • The docs are updated, and if this is a notable improvement, it's added to our What's New doc.

@ghost ghost added this to the 11.1.x milestone Jun 5, 2024
Comment thread pkg/services/queryhistory/api.go Outdated
@gelicia gelicia added no-backport Skip backport of PR no-changelog Skip including change in changelog/release notes labels Jun 5, 2024
@gelicia gelicia marked this pull request as ready for review June 5, 2024 00:54
@gelicia gelicia requested a review from a team as a code owner June 5, 2024 00:54
@gelicia gelicia requested a review from a team June 5, 2024 00:54
@ifrost

ifrost commented Jun 5, 2024

Copy link
Copy Markdown
Contributor

I think we should also apply it to all query history endpoints 🤔

@diegommm diegommm modified the milestones: 11.1.x, 11.2.x Jun 14, 2024
@diegommm

Copy link
Copy Markdown
Contributor

Hi! Greetings from the Release Guild. Just letting you know I'm updating your PR's milestone to 11.2.x. If you need to see your changes in a previous version, please consider adding a backport label.

Thank you!

@github-actions

Copy link
Copy Markdown
Contributor

This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 2 weeks if no further activity occurs. Please feel free to give a status update or ping for review. Thank you for your contributions!

@github-actions github-actions Bot added the stale Issue with no recent activity label Jul 15, 2024
gelicia added 2 commits July 15, 2024 13:48
…na/query-history-viewer-lock

# Conflicts:
#	pkg/services/queryhistory/api.go
@gelicia gelicia removed the stale Issue with no recent activity label Jul 15, 2024
@gelicia

gelicia commented Jul 15, 2024

Copy link
Copy Markdown
Contributor Author

@ifrost I'm really sorry, I spaced on this one. I was asking the access team if they had anything for an exception for viewers_can_access and the outcome was a no. I thought about making a global auth middleware setting for this, but in the end I think it's a fairly unique scenario where trying to bake it into something core is overkill. I just added the same logic to every handler.

Why I think this is unique: Viewers can edit rides this line where people can run queries, but not save anything. Explore access is fully allowed, dashboard panels can be edited, but not saved. Query history a.) is an outcome of running queries (good!) b.) saves something to the database (bad!) so its kind of in-between. I do think we just want to save stuff to the database for viewers here, and if it ever becomes an issue we can change the frontend logic to only save query history data to local storage for viewers.

gelicia added 3 commits July 15, 2024 17:03
…na/query-history-viewer-lock

# Conflicts:
#	pkg/services/queryhistory/queryhistory_search_test.go
@ifrost

ifrost commented Jul 17, 2024

Copy link
Copy Markdown
Contributor

@ifrost I'm really sorry, I spaced on this one. I was asking the access team if they had anything for an exception for viewers_can_access and the outcome was a no.

I feel like this should be implemented the same way as the access to Explore, i.e. the access being elevated when viewers_can_edit = true

We can have a new permission for QueryHistory specifically that is available only to Editors (or Viewers when viewers_can_edit is enabled). Otherwise we end up in two ways of handling viewers_can_edit which will be hard to maintain.

But anyway - Dashboards to their own magic when handling viewer_can_edit anyway, so I'm happy to merge it as it as as well and create a follow-up ticket to refactor it 👍

@gelicia gelicia merged commit a0268a9 into main Jul 19, 2024
@gelicia gelicia deleted the kristina/query-history-viewer-lock branch July 19, 2024 19:45
giorgioatmerqury pushed a commit to cybermerqury/grafana that referenced this pull request Aug 21, 2024
* Add permissions check for viewer without viewers_can_edit

* Add test

* fix lint

* Add role checks on other handlers

* Linter and fix Go issue

* Fix conflict

* Remove invalid way of testing for error
@aangelisc aangelisc modified the milestones: 11.2.x, 11.2.0 Aug 21, 2024
PoorlyDefinedBehaviour pushed a commit that referenced this pull request May 6, 2026
* Add permissions check for viewer without viewers_can_edit

* Add test

* fix lint

* Add role checks on other handlers

* Linter and fix Go issue

* Fix conflict

* Remove invalid way of testing for error
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/backend no-backport Skip backport of PR no-changelog Skip including change in changelog/release notes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Query History: Do not return results from API if user is in the viewer role

5 participants