Add check_ownership call in save_or_overwrite_slice

[jaylindquist]

Make sure these boxes are checked before submitting your issue - thank you!

• I have checked the superset logs for python stacktraces and included it here as text if any
• I have reproduced the issue with at least the latest released version of superset
• I have checked the issue tracker for the same issue and I haven't found one similar

Superset version

• 0.17.0
• 0.19.1

Expected results

Only users with proper permissions can create or update dashboards or change slices

Actual results

A user with the following two permissions can add a slice to a new dashboard, add a slice to an existing dashboard, overwrite the definition of an existing slice.

• can explore on Superset
• all datasource access on all_datasource_access

Steps to reproduce

1. Install superset, configure admin, load example data
2. Create a new Role ReadOnly with the following permissions

• can explore on Superset
• all datasource access on all_datasource_access

1. Create a new User test and add the user to the ReadOnly role
2. Login to SuperSet as test
3. Go to the following URL (generated from saving a slice and adding to a new dashboard as an admin):

http://<server>/superset/explore/table/3/?form_data={%22datasource%22:%223__table%22,%22viz_type%22:%22table%22,%22slice_id%22:15,%22granularity_sqla%22:%22ds%22,%22time_grain_sqla%22:null,%22since%22:%22100+years+ago%22,%22until%22:%22now%22,%22groupby%22:[%22name%22],%22metrics%22:[%22sum__num%22],%22include_time%22:false,%22all_columns%22:[],%22order_by_cols%22:[],%22table_timestamp_format%22:%22%Y-%m-%d+%H:%M:%S%22,%22row_limit%22:50,%22page_length%22:0,%22include_search%22:false,%22table_filter%22:false,%22where%22:%22%22,%22having%22:%22%22,%22filters%22:[{%22op%22:%22in%22,%22val%22:[%22girl%22],%22col%22:%22gender%22}]}&action=overwrite&slice_id=15&slice_name=Girls&add_to_dash=new&new_dashboard_name=new-dash&goto_dash=true

This will create a new dashboard called 'new-dash' owned by the test user.

Simplifying the URL can cause a lot of other issues. For instance if the test user goes to

http://<server>/superset/explore/table/3/?form_data={%22slice_id%22:22,%22viz_type%22:%22table%22}&action=overwrite&slice_id=22&add_to_dash=new&new_dashboard_name=break

They can overwrite slice ID 22 while creating the new dashboard

Changing the parameters to add_to_dash=existing&save_to_dashboard_id=1 will allow them to add slices to existing dashboards as well.

[xrmx]

Have you read the security documentation section?

[jaylindquist]

@xmrx Yes, as explained here: https://superset.incubator.apache.org/security.html#permissions

The roles given to the test user:

• can explore on Superset - gives the user access to see the explore view (basically, see details on a slice)
• all datasource access on all_datasource_access - give the user access to see slices for any data source

Neither of these permissions allow a user to create or edit slices and dashboards.

The problem comes from the Save button when exploring.

• The code executed starts at def explore(self, datasource_type, datasource_id) in core.py https://github.com/apache/incubator-superset/blob/c988080990691eceb7a78e6448400a519ba4bb0b/superset/views/core.py#L1033
• The only permission check is that the user has access to the explore view (can explore on Superset) and can access the datasource (all datasource access on all_datasource_access)
• If the query includes an action parameter set to saveas or overwrite then the save_or_overwrite_slice function is called https://github.com/apache/incubator-superset/blob/c988080990691eceb7a78e6448400a519ba4bb0b/superset/views/core.py#L1135
• In this function, the user can create a dashboard or add a slice to a dashboard without needing any additional permissions https://github.com/apache/incubator-superset/blob/c988080990691eceb7a78e6448400a519ba4bb0b/superset/views/core.py#L1160-L1186

[xrmx]

My understanding is that all datasource access on all_datasource_access means they can do everything not that they can see everything.

[jaylindquist]

@xmrx The documentation implies that all datasource access of all_datasource_access allows users to see slices using all datasources, not create slices or create dashboards

From the security documentation:

• Data source: For each data source, a permission is created. If the user does not have the all_datasource_access permission granted, the user will only be able to see Slices or explore the data sources that are granted to them

I'm reading that as "Granting datasource access on <specific data source> lets the user see Slices or explore data on that specific data source. Granting all_datasource_access lets the user see Slices or explore data on all data sources"

Meanwhile, Model permissions are used to let a user add / edit slices and dashboards. If I don't give the user can_add on Dashboards, then they are not allowed to create dashboards.

• Model & action: models are entities like Dashboard, Slice, or User. Each model has a fixed set of permissions, like can_edit, can_show, can_delete, can_list, can_add, and so on. By adding can_delete on Dashboard to a role, and granting that role to a user, this user will be able to delete dashboards.

Am I reading that wrong? If so, is there no way to let users see existing dashboards while preventing them from creating dashboards?

[mistercrunch]

That's right, it looks like there's a check_ownership check missing in save_or_overwrite_slice

[graceguo-supercat]

@jaylindquist check these lines
slice_add_perm = self.can_access('can_add', 'SliceModelView')
slice_overwrite_perm = is_owner(slc, g.user)
slice_download_perm = self.can_access('can_download', 'SliceModelView')
...
if action == 'overwrite' and not slice_overwrite_perm:
return json_error_response("You don't have the rights to alter this slice", status=400)

we checked all permissions and passing down to self.save_or_overwrite_slice. is it a good permission check?

[jaylindquist]

Hi @graceguo-supercat Unfortunately I can't get SuperSet to build locally right now so I can't test, but...

The dialog presented to the user gives them the option to save / overwrite slices as well as save / overwrite dashboards.

So, the above check that gives the user an error when saving / overwriting the slice will need to be two-fold:

if action == 'overwrite' and not slice_overwrite_perm:
  return json_error_response("You don't have the rights to alter this slice", status=400)

if action == 'saveas' and not slice_add_perm:
  return json_error_response("You don't have the rights to create a slice", status=400)

If the user has those permissions, a dashboard permission check would be needed before the section saving / creating dashboards:

dash = ... # find dashboard from request
check_ownership(dash, raise_if_false=True)
... # update dashboard

dash_add_perm = self.can_access('can_add', 'DashboardModelView')
if request.args.get('add_to_dash') == 'new' and not dash_add_perm:
  return json_error_response("You don't have the rights to create a dashboard", status=400)

# ... save dashboard

That is mostly psuedo-code since I can't actually test it right now and I'm not sure the best design for error handling here.

Also, from a UX viewpoint, it would be nice to have the "Save" button in the explore view be disabled if the user doesn't have the ability to save / alter slices, but I have no idea what those changes would look like.

[aabdinur]

Doesn't this problem also exist on a dashboard level? Users with read only access are able to delete slices from the dashboard and save the dashboard.

[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.