added filter on databaseView that filters databases based on access in roles

[pbr0ck3r]

No description provided.

[xrmx]

Looks like there's a lot of unwanted changes in this PR

[pbr0ck3r]

If you could explain to me the unwanted changes or the way you would prefer I implement the filter that would be helpful. I previously just followed @mistercrunch suggestion in #5207 (comment).

[pbr0ck3r]

@xrmx I believe I understand what you mean by unwanted changes. Will remove unwanted file changes.

[codecov-io]

Codecov Report

Merging #6356 into master will increase coverage by <.01%.
The diff coverage is 77.77%.

@@            Coverage Diff             @@
##           master    #6356      +/-   ##
==========================================
+ Coverage   73.37%   73.37%   +<.01%     
==========================================
  Files          67       67              
  Lines        9587     9596       +9     
==========================================
+ Hits         7034     7041       +7     
- Misses       2553     2555       +2

Impacted Files	Coverage Δ
superset/security.py	75.77% <100%> (+0.21%)	⬆️
superset/views/core.py	74.94% <71.42%> (-0.02%)	⬇️

---

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 7eb4684...8a60ed3. Read the comment docs.

[mistercrunch]

Minor comment, otherwise LGTM

[mistercrunch]

Oh one more thing, we should make sure that role Alpha sees all databases as well for backwards compatibility purposes.
You'll need to add something here:
https://github.com/apache/incubator-superset/blob/master/superset/security.py#L87

And remove this line:
https://github.com/apache/incubator-superset/blob/master/superset/security.py#L71

Would be great to have a unit test validating that:

1. Alpha sees all dbs when calling /databaseview/api/read
2. Gamma sees no dbs when calling /databaseview/api/read

[pbr0ck3r]

@mistercrunch I went ahead and made the suggestion changes and also updated the tests accordingly. Did not have the time to dig into adding the extra unit tests you suggested.

[tomaszj]

Hi folks, is there anything that can be contributed to integrate the changes from this PR?

[mistercrunch]

Please rebase / fix conflicts

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. For admin, please label this issue .pinned to prevent stale bot from closing the issue.

[pbr0ck3r]

Not stale

[mistercrunch]

A similar PR has been merged already, closing https://github.com/apache/incubator-superset/blob/8c17c1927937362a22c07d1362f00992e56621a1/superset/views/core.py#L203-L208

[pbr0ck3r]

Kind of salty that this PR has been open for 8 months and changes that were asked were fixed months ago as well. But I guess if someone else beat me to the punch of getting the same changes in... glad the security issue is fixed.

[mistercrunch]

@pbr0ck3r it's hard for maintainers to keep up with the pace of this repository and the volume of information that gets through here. I wish all PRs and issues would get the attention they deserve.

But I don't think I get notifications when new commits are added to a PR, so I'd recommend commenting/tagging on the PR once comments are addressed. I process most of my notifications, though I'll miss a certain percentage, but if there's no notifications there's no chance maintainers will catch things.