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

Adding permission for can_only_access_owned_queries #7234

Merged

Conversation

michellethomas
Copy link
Contributor

@michellethomas michellethomas commented Apr 6, 2019

SUMMARY

Adding an optional restriction on query search to only allow users to access their own queries. Right now query search lets anyone access any queries. That would be the default for all roles, but if a deployment wanted to change to restrict users to only search their queries they could do that with the can_only_access_owned_queries permission.

With can_only_access_owned_queries in OBJECT_SPEC_PERMISSIONS, this permission is not added to any role by default.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

N/A

TEST PLAN

Run superset init with new can_only_access_owned_queries default off in OBJECT_SPEC_PERMISSIONS
Logged in as user1 and ran query
Logged in as user2 and confirmed that I could see all queries queries in /superset/sqllab#search and /queryview/list/
Moved perm can_only_access_owned_queries to ADMIN_ONLY_PERMISSIONS
Run superset init
Confirmed that as user2 I could see only my queries queries in /superset/sqllab#search and /queryview/list/

ADDITIONAL INFORMATION
[ ] Has associated issue:
[ ] Changes UI
[ ] Requires DB Migration. Confirm DB Migration upgrade and downgrade tested.
[ ] Introduces new feature or API
[ ] Removes existing feature or API
[ ] Fixes bug
[ ] Refactors code
[x] Adds test(s)
REVIEWERS

@mistercrunch @john-bodley

@michellethomas michellethomas force-pushed the add_permission_only_search_owned branch from 5af6617 to c036ba6 Compare April 6, 2019 00:23
@michellethomas michellethomas force-pushed the add_permission_only_search_owned branch 3 times, most recently from e359071 to 263972b Compare April 8, 2019 22:07
@michellethomas michellethomas force-pushed the add_permission_only_search_owned branch from 263972b to 94968ad Compare April 9, 2019 07:04
@codecov-io
Copy link

codecov-io commented Apr 9, 2019

Codecov Report

Merging #7234 into master will increase coverage by 0.31%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #7234      +/-   ##
==========================================
+ Coverage   64.62%   64.94%   +0.31%     
==========================================
  Files         422      424       +2     
  Lines       20593    20598       +5     
  Branches     2253     2281      +28     
==========================================
+ Hits        13309    13378      +69     
+ Misses       7161     7097      -64     
  Partials      123      123
Impacted Files Coverage Δ
superset/views/sql_lab.py 94.64% <100%> (+0.89%) ⬆️
superset/views/core.py 75.01% <100%> (+0.08%) ⬆️
superset/models/sql_lab.py 94.93% <100%> (+0.27%) ⬆️
superset/security.py 75.96% <100%> (+0.31%) ⬆️
superset/data/paris.py 33.33% <0%> (-5.8%) ⬇️
superset/data/sf_population_polygons.py 33.33% <0%> (-5.8%) ⬇️
superset/data/random_time_series.py 23.07% <0%> (-5.5%) ⬇️
superset/data/bart_lines.py 33.33% <0%> (-5.13%) ⬇️
superset/data/flights.py 20.68% <0%> (-5.12%) ⬇️
superset/data/multiformat_time_series.py 17.14% <0%> (-4.48%) ⬇️
... and 39 more

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 5192270...8dbcb2a. Read the comment docs.

@mistercrunch
Copy link
Member

Can you create the perm under the QueryView view menu? Here are the existing ones that FAB maintains:
Screen Shot 2019-04-09 at 2 47 53 PM

For reference:

SELECT B.name as view_menu, C.name as permission 
FROM ab_permission_view A 
JOIN ab_view_menu B on B.id = A.view_menu_id 
JOIN ab_permission C on C.id = A.permission_id 
WHERE B.name = 'QueryView'

@michellethomas
Copy link
Contributor Author

michellethomas commented Apr 11, 2019

@mistercrunch to confirm you mean in create_custom_perms set self.merge_perm('can_only_access_owned_queries', 'QueryView') instead of
self.merge_perm('can_only_access_owned_queries', 'can_only_access_owned_queries') right?

Whenever I do this the permissions get removed whenever the app starts I'm guessing because of something that AppBuilder is doing (the permissions exist after running sync_role_definitions but get removed later).

@michellethomas
Copy link
Contributor Author

michellethomas commented Apr 11, 2019

It looks like Flask App Builder goes through and cleans up permissions on view menus (other than can_add, can_edit,...) so I'm not sure it's possible for me to add can_only_access_owned_queries on QueryView.

@mistercrunch
Copy link
Member

Ok I didn't know it would do that. LGMT

superset/views/core.py Outdated Show resolved Hide resolved
superset/views/sql_lab.py Outdated Show resolved Hide resolved
@michellethomas michellethomas merged commit 51068f0 into apache:master Apr 17, 2019
@michellethomas michellethomas deleted the add_permission_only_search_owned branch April 17, 2019 23:11
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.34.0 labels Feb 28, 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.34.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants