Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[SqlLab] Filter out unavailable databases #3875

Merged
merged 1 commit into from
Nov 15, 2017

Conversation

dmigo
Copy link
Contributor

@dmigo dmigo commented Nov 15, 2017

Problem this PR solves:
One always can see the whole list of databases in sql lab. Even those he has no access to.

@dmigo dmigo changed the title [SqlLab]Filter out unavailable databases [SqlLab] Filter out unavailable databases Nov 15, 2017
@mistercrunch
Copy link
Member

Thanks for the PR!

@mistercrunch mistercrunch merged commit ae2205a into apache:master Nov 15, 2017
@dmigo
Copy link
Contributor Author

dmigo commented Nov 16, 2017

@mistercrunch glad to help. It's just I wasn't 100% sure about if it should be database_access or datasource_access .

@dmigo dmigo deleted the database_filte branch November 17, 2017 10:35
@john-bodley
Copy link
Member

@dmigo @mistercrunch we sense the logic in this PR is invalid as we have Gamma role users who have access to specific datasources associated with a database but not the database itself and thus these users are no-longer able to access any of their datasources within SQL Lab.

Could we either revert this PR or update the filter logic so that the whitelist set of databases were cognizant of the datasources. For the later it seems additional unit tests should be written to ensure we have coverage of various configurations.

@mistercrunch
Copy link
Member

I'm ok either way, ask me if you want me to revert.

@john-bodley
Copy link
Member

@mistercrunch would you mind reverting? @dmigo would you mind updating the filter logic w/ tests in another PR.

@dmigo
Copy link
Contributor Author

dmigo commented Nov 21, 2017

@john-bodley no, I don't mind.
My idea behind this PR was to hide the databases one doesn't have access to.
I'll create one more PR later today.

mistercrunch added a commit to mistercrunch/superset that referenced this pull request Nov 21, 2017
graceguo-supercat pushed a commit that referenced this pull request Nov 21, 2017
michellethomas pushed a commit to michellethomas/panoramix that referenced this pull request May 24, 2018
michellethomas pushed a commit to michellethomas/panoramix that referenced this pull request May 24, 2018
wenchma pushed a commit to wenchma/incubator-superset that referenced this pull request Nov 16, 2018
wenchma pushed a commit to wenchma/incubator-superset that referenced this pull request Nov 16, 2018
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.21.0 labels Feb 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.21.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants