search

rilldata / rill

Rill is a tool for effortlessly transforming data sets into powerful, opinionated dashboards using SQL. BI-as-code.
https://www.rilldata.com
Apache License 2.0
1.7k stars 114 forks source link

Security policy without measures should not fire certain API requests #5224

Closed ericpgreen2 closed 1 month ago

ericpgreen2 commented 3 months ago

If you create a security policy that only includes dimension fields, but not any measure fields, it:

  1. Makes a /timeseries request, which fails because there are no measure names
  2. Makes /aggregation requests with an empty sort key
  3. Makes a request for /resource?name.kind=rill.runtime.v1.ProjectParser&name.name=parser, which fails because restricted users don’t have access to parse errors
security:
  access: true
  include:
    - if: true
      names: [company, plan_name]

Screenshot 2024-07-08 at 18 45 24

lovincyrus commented 2 months ago

Could you record a repro for me to get to the screenshot state? @ericpgreen2

After adding the codeblock to rill.yaml

security:
  access: true
  include:
    - if: true
      names: [company, plan_name]

I can't seem to view the error requests as mentioned in the screenshot.. Here's what I have

https://github.com/user-attachments/assets/2b6af5bb-5972-4b0c-897c-357c693e24dd

ericpgreen2 commented 2 months ago

Yeah, the key is adding the codeblock to the metrics view, not rill.yaml. Here's one way to reproduce:

  1. Clone our example partner-filtered dashboard project

  2. Replace the security policy of the first dashboard (a_admin_access.yaml) with the above code snippet. Here's the edited file in full:

    # a_admin_access.yaml
# Visit https://docs.rilldata.com/reference/project-files to learn more about Rill project files.

title: Admin-level access
type: metrics_view
model: margins_model
timeseries: __time
measures:
  - name: total_cost
    label: "Total Cost"
    expression: "SUM(cost)"
    description: "The sum of cost"
    format_preset: currency_usd
  - name: total_revenue
    label: Total Revenue
    expression: SUM(revenue)
    description: The sum of revenue
    format_preset: currency_usd
  - name: net_revenue
    label: "Net Revenue"
    expression: "SUM(revenue) - SUM(cost)"
    description: "The sum of revenue minus the sum of cost"
    format_preset: currency_usd
  - name: margin
    label: "Margin"
    expression: "(SUM(revenue) - SUM(cost))/SUM(revenue)"
    description: "Net revenue divided by sum of revenue"
    format_preset: percentage
  - name: unique_customers
    label: "Unique Customers"
    expression: "COUNT(DISTINCT company)"
    description: "The count of unique companies"
    format_preset: humanize
dimensions:
  - name: company
    expression: company
    label: Company
    description: "The name of the company"
  - name: plan_name
    expression: plan_name
    label: Plan Name
    description: "The name of the billing plan"
  - name: location
    expression: "location"
    label: "Cost by Region"
    description: "The region incurring costs"
  - name: component
    expression: component
    label: Cost by Component
    description: "The component generating costs"
  - name: app_name
    expression: "app_name"
    label: "Cost by App Name"
    description: "The app generating costs"
  - name: sku_description
    expression: "sku_description"
    label: "Cost by SKU"
    description: "The sku description for costs"
  - name: pipeline
    expression: "pipeline"
    label: "Cost by Data Pipeline"
    description: "The pipeline incurring costs"
  - name: environment
    expression: "environment"
    label: "Cost by Environment"
    description: "The environment incurring costs"
available_time_zones:
  - America/Los_Angeles
  - America/Chicago
  - America/New_York
security:
  access: true
  include:
    - if: true
      names: [company, plan_name]
  3. Preview the dashboard, and "View as" admin@rilldata.com
  4. Observe the broken state image
lovincyrus commented 2 months ago

Makes a request for /resource?name.kind=rill.runtime.v1.ProjectParser&name.name=parser, which fails because restricted users don’t have access to parse errors

What should be the expected action item? Do we want to allow restricted users to access and parse the errors? @ericpgreen2

ericpgreen2 commented 2 months ago

Makes a request for /resource?name.kind=rill.runtime.v1.ProjectParser&name.name=parser, which fails because restricted users don’t have access to parse errors

This is a pretty tricky case. For context, we currently support a couple things:

  1. In Rill Developer, when editing project files in an external IDE, we support "hot reloading". External edits are instantly reflected in the dashboard. When there’s a parse error, we say so.
  2. In Rill Cloud, when a dashboard becomes invalid, rather than show an error (e.g. a 404), we show the “last valid” dashboard. Consequently, for both Rill Developer and Rill Cloud, even if there's an error in the dashboard file, the dashboard resource is retained.

Do we want to allow restricted users to access and parse the errors?

I don't think so. For "real" restricted users, that'd be a security hole. For "mock" restricted users, I think it'd be conceptually confusing to grant them artificial privileges.

What should be the expected action item?

Here's one idea:

  1. Rather than showing a full-screen "Error parsing dashboard" message, we: a. At the top of the page, show a red error banner that says "Error parsing dashboard – you are viewing your last valid dashboard specification" b. Display the "last valid dashboard", like we do in Cloud. This would actually be an accurate representation of what would happen if the user committed their changes.
  2. Only for project admins do we show the error banner (and request the project parser). I.e.: a. When there's no mock user b. When the mock user is a project admin
lovincyrus commented 2 months ago

At the top of the page, show a red error banner that says "Error parsing dashboard – you are viewing your last valid dashboard specification"

Hey @jkhwu, do we currently use any error banner atm?

If not, I was thinking of placing the error banner here:

Image

Update: can refer to https://www.figma.com/design/Qt6EyotCBS3V6O31jVhMQ7/RILL?node-id=144-815&t=AYrXdJOOdMJ4KUmx-0

lovincyrus commented 1 month ago

Rather than showing a full-screen "Error parsing dashboard" message, we: a. At the top of the page, show a red error banner that says "Error parsing dashboard – you are viewing your last valid dashboard specification" b. Display the "last valid dashboard", like we do in Cloud. This would actually be an accurate representation of what would happen if the user committed their changes.

Are you referring to the banner triggered from the event bus or the RepresentingUserBanner?

ericpgreen2 commented 1 month ago

Are you referring to the banner triggered from the event bus or the RepresentingUserBanner?

The one controlled by the event bus + BannerCenter. I'd have preferred the RepresentingUserBanner to also have been controlled via event bus + BannerCenter.

lovincyrus commented 1 month ago

Only for project admins do we show the error banner (and request the project parser).

To confirm, are you referring to a user who can manage projects or a mock user who is an admin?

ericpgreen2 commented 1 month ago

To confirm, are you referring to a user who can manage projects or a mock user who is an admin?

Both of them :)