Allow users to view dashboards they own

[jeffreythewang]

This change allows users to see the dashboards they own listed in the dashboard list. Previously, users could only see dashboards in the list if the dashboard contained at least 1 chart that they have access to. This means that a user cannot find their dashboard in the list if they create an empty dashboard (either by mistake, or as a blank template to start adding charts).

For the case where they are the owner of a dashboard, but all of the charts in their dashboard are not accessible (which can only happen if an additional owner adds an inaccessible chart to the dashboard), they will see a dashboard with a bunch of charts with permission errors. I think this is okay, as it is the same behavior as when a user has access to some but not all charts in a dashboard. Feel free to comment otherwise, and I can add an additional check for this to be applied to empty dashboards only.

[timifasubaa]

LGTM

[jeffreythewang]

It seems like users have a similar issue beyond a Dashboard context. I did not realize it was possible to create a datasource/table without having access to the underlying database/schema of that datasource/table. Even with this change, they may still have that issue, as this change only solves the case of not being able to see an empty / inaccessible dashboard in the dashboard list. I do not think this fixes the issues mentioned above, unless other parts of the code have changed since then.

[timifasubaa]

Ah, I see. I misread Dashboard as Datasource.
Some parts of the code have changed. Be default, the request access form no longer shows up. permissions will now be decided on a per slice level. (#4405). But your change is still relevant.

[jeffreythewang]

Any updates on this?

[timifasubaa]

I don't have merge privileges but this LGTM.

[john-bodley]

@jeffreythewang isn't the current logic that a user is able to see the dashboard if the have access to at least one of the underlying slices (as opposed to all), per this logic:

query = query.filter(
    Dash.id.in_(
        db.session.query(Dash.id)
        .distinct()
        .join(Dash.slices)
        .filter(Slice.id.in_(slice_ids_qry)),
     ),
)

My understanding is this query returns the distinct dashboard IDs which are represented by any (per the `IN condition) of the sanctioned slices. I wonder whether this is being confused with #4737.

[jeffreythewang]

@john-bodley Yes that is the current logic. This change mainly addresses the case where a Dashboard is empty, as the user will not be able to see it since they do not have access to any slices in the Dashboard (because there are no slices in the Dashboard).

With this change, if the Dashboard contains zero slices, but you are the owner of that Dashboard, you will still be able to view it in the Dashboard list. This happens when a user wants to start by creating an empty dashboard, and then adding slices to it. It's not a common use case, but it happens sometimes.

[john-bodley]

@jeffreythewang I agree with your logic regarding empty dashboards which this PR resolves, but I think your original comment

For the case where they are the owner of a dashboard, but their dashboard has charts they do not have access to (which can only happen if an additional owner adds an inaccessible chart to the dashboard)

is not correct according to the current logic. Maybe it would be good to updated the PR title and description to better reflect the change. Also I wonder whether a unit test would be valuable here.

[jeffreythewang]

@john-bodley I think what I meant by that was, if there is a dashboard where all of the charts on that dashboard are inaccessible to the user, but the user is the owner of the dashboard, then in the current implementation, the user will not see that dashboard in the dashboard list view.

Also, I'll add a unit test.

[john-bodley]

Thanks @jeffreythewang

[john-bodley]

Note this filter is also used when you try to add a slice to an existing dashboard and thus if you create an empty dashboard you'll now be able to add a slice to it.

[codecov-io]

Codecov Report

Merging #4520 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff            @@
##           master   #4520      +/-   ##
=========================================
+ Coverage    77.5%   77.5%   +<.01%     
=========================================
  Files          44      44              
  Lines        8720    8722       +2     
=========================================
+ Hits         6758    6760       +2     
  Misses       1962    1962

Impacted Files	Coverage Δ
superset/views/core.py	74.7% <100%> (+0.03%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0511d1f...56c3cc6. Read the comment docs.

[jeffreythewang]

@john-bodley just an update - I've added a unit test