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

Disable user access request #4405

Merged
merged 4 commits into from
Feb 14, 2018

Conversation

timifasubaa
Copy link
Contributor

@timifasubaa timifasubaa commented Feb 12, 2018

The request access feature isn't fully implemented and it can potentially be misleading to users who click the access request button and expect to gain access eventually. This PR makes the access request button optional via a feature flag that allows it by default.

If you set the flag to false, you'll no longer see the image below whenever you don't have access to any of the slices in a dashboard.
no_access_

Instead, you'll see all the slices and an error message for any one you don't have access to. Like in the image below.
screen shot 2018-02-11 at 11 13 09 pm

@john-bodley @michellethomas @mistercrunch

@timifasubaa timifasubaa force-pushed the disable_user_access_request branch 2 times, most recently from 123335b to 117db38 Compare February 12, 2018 08:23
@timifasubaa timifasubaa force-pushed the disable_user_access_request branch from 117db38 to 5f3862b Compare February 12, 2018 18:42
@mistercrunch
Copy link
Member

You may want to disable the ModelView as well

@timifasubaa
Copy link
Contributor Author

timifasubaa commented Feb 12, 2018

@mistercrunch Done. I left the access requests on by default. Is it better off by default?

@timifasubaa timifasubaa force-pushed the disable_user_access_request branch from d94539c to 63a7cdf Compare February 12, 2018 23:03
@mistercrunch
Copy link
Member

Let's flip it off by default.

@timifasubaa timifasubaa force-pushed the disable_user_access_request branch from 63a7cdf to 410e764 Compare February 13, 2018 17:41
@timifasubaa
Copy link
Contributor Author

Done

@timifasubaa timifasubaa force-pushed the disable_user_access_request branch 4 times, most recently from 4ff61ea to fb24089 Compare February 13, 2018 19:25
@mistercrunch
Copy link
Member

Wait you're deleting all those tests :(

Why not just have them look at the same config/feature flag?

@timifasubaa
Copy link
Contributor Author

Ah, true. I'll do it that way instead.

@timifasubaa timifasubaa force-pushed the disable_user_access_request branch 3 times, most recently from ccc1b05 to 5550e58 Compare February 13, 2018 20:48
@timifasubaa timifasubaa force-pushed the disable_user_access_request branch from 5550e58 to c4c21a2 Compare February 13, 2018 20:54
@maver1ck
Copy link
Contributor

Do you have idea how does it join with #4352 ?

@timifasubaa
Copy link
Contributor Author

timifasubaa commented Feb 14, 2018

@maver1ck This PR only gives different deployments the the option to remove the ability to request access from users. It doesn't change whether gamma has access to dashboards it created.
Like you suggested in the issue, you could create a new rule in the permissions check that allows the creator to see their dashboards even if it is a gamma user.

@mistercrunch mistercrunch merged commit fa0aa33 into apache:master Feb 14, 2018
michellethomas pushed a commit to michellethomas/panoramix that referenced this pull request May 24, 2018
* add feature flag to config

* wrap check around a feature flag

* add flag to the model view

* remove access request from seurity tests
wenchma pushed a commit to wenchma/incubator-superset that referenced this pull request Nov 16, 2018
* add feature flag to config

* wrap check around a feature flag

* add flag to the model view

* remove access request from seurity tests
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.23.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.23.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants