Skip to content

Fix public dashboard visibility bug

Kirstie Cook requested to merge 216505-public-dashboard-visibility into master

What does this MR do?

Currently, anonymous users are allowed in ProjectPolicy to read_metrics_user_starred_dashboard, but this causes an error because a user-starred dashboard requires a user. This fix prevents an error from occurring when trying to load starred dashboards as an anonymous user that happens automatically when viewing the public dashboards.

In addition, anonymous users are currently prevented from viewing public dashboards if builds or repository is disabled for the project. This fix allows users to read_environment in that context with metrics_dashboard_allowed is enabled. This is kind of a hacky way around the tight coupling that currently exists between metrics and environments, so we're introducing some technical debt that would be able to be addressed once the work begins for decouple metrics and environments (discussed in #213833 (closed)).

Debug output without fix:

[2] pry(main)> policy = ProjectPolicy.new(nil, project)
=> #<ProjectPolicy (<anonymous> : Project/19)>
[3] pry(main)> policy.debug(:read_environment)
- [0] enable when auditor ((<anonymous> : Project/19))
- [0] prevent when all?(anonymous, ~public_project) ((<anonymous> : Project/19))
  ProjectFeature Load (0.8ms)  SELECT "project_features".* FROM "project_features" WHERE "project_features"."project_id" = 19 LIMIT 1
+ [28] prevent when builds_disabled ((<anonymous> : Project/19))
  [28] prevent when repository_disabled ((<anonymous> : Project/19))
  [116] enable when can?(:reporter_access) ((<anonymous> : Project/19))
  [176] enable when can?(:metrics_dashboard) ((<anonymous> : Project/19))
=> #<DeclarativePolicy::Runner::State:0x00007fd0d54a2380 @enabled=false, @prevented=true>

Debug output with fix:

[2] pry(main)> policy = ProjectPolicy.new(nil, project)
=> #<ProjectPolicy (<anonymous> : Project/19)>
[3] pry(main)> policy.debug(:read_environment)
- [0] enable when auditor ((<anonymous> : Project/19))
- [0] prevent when all?(anonymous, ~public_project) ((<anonymous> : Project/19))
  ProjectFeature Load (0.8ms)  SELECT "project_features".* FROM "project_features" WHERE "project_features"."project_id" = 19 LIMIT 1
- [63] prevent when all?(any?(builds_disabled, repository_disabled), ~metrics_dashboard_allowed) ((<anonymous> : Project/19))
+ [0] enable when all?(any?(builds_disabled, repository_disabled), metrics_dashboard_allowed) ((<anonymous> : Project/19))
=> #<DeclarativePolicy::Runner::State:0x00007f8c49dc9560 @enabled=true, @prevented=false>

Screenshots

This recording is a demonstration of the existing bug: enabling "Everyone with Access" for Metrics Dashboard results in an error. (Left side is project admin, right sight is incognito/anonymous user.) PublicDashboardBugDemo

The next series of recordings are demonstrations of the fix.

First, all features are set to "Everyone With Access";

PublicDashboardFix1

second, disabling pipelines verifying that the dashboard is still visible: (Due to existing coupling between environments and metrics, loading the public dashboard is still attempting to pull deployment information which causes the "There was an error getting deployment information" error when pipelines are disabled. However, this error message does not affect actually viewing the dashboard.) PublicDashboardFixDemo2

and third, disabling repository verifying that the dashboard is still visible: PublicDashboardFixDemo3

Demonstration that disabling public dashboard still works appropriately: PublicDashboardFixDemo4

Does this MR meet the acceptance criteria?

Conformity

Availability and Testing

Security

If this MR contains changes to processing or storing of credentials or tokens, authorization and authentication methods and other items described in the security review guidelines:

  • [-] Label as security and @ mention @gitlab-com/gl-security/appsec
  • [-] The MR includes necessary changes to maintain consistency between UI, API, email, or other methods
  • [-] Security reports checked/validated by a reviewer from the AppSec team

Closes #216505 (closed)

Edited by 🤖 GitLab Bot 🤖

Merge request reports