Importing admin owned chart as gamma user changes ownership

[joezeon]

Bug description

Essentially when re-importing chart (owned by Superset Admin) as gamma role user, gamma user can take complete ownership of the admin owned chart. Seems like a bug because it doesnt allow gamma user to change ownership in Edit properties for the admin chart. Explained the steps to reproduce below.

How to reproduce the bug

1. Following charts show when a Gamma role user is logged in. Some are owned by Superset Admin identified by ADMIN.CHART and some owned by the Gamma user.
2. Logged in as a Gamma user (Gamma role assigned). ADMIN.CHART2 is owned by Superset Admin. If I try to change the ownership and add Gamma User to it, it throws error.
3. Take export of ADMIN.CHART2. Import it back in. Overwrite to Confirm. Notice ownership now shows both Superset Admin and Gamma User.
4. Furthermore, I can now remove Superset Admin as the owner and also add other Gamma users as owners. Shows undefined at first and when refreshed show ownership has changed.

Screenshots/recordings

No response

Superset version

4.0.2

Python version

3.9

Node version

16

Browser

Chrome

Additional context

No response

Checklist

• [X] I have searched Superset docs and Slack and didn't find a solution to my problem.
• [X] I have searched the GitHub issue tracker and didn't find a similar bug report.
• [ ] I have checked Superset's logs for errors and if I found a relevant Python stacktrace, I included it here as text in the "additional context" section.

[dosubot[bot]]

To resolve the issue where a Gamma role user can take ownership of a chart originally owned by a Superset Admin by re-importing the chart in Apache Superset version 4.0.2, you need to ensure that the ownership validation logic is correctly implemented. The UpdateChartCommand class in superset/commands/chart/update.py provides a good example of how to handle ownership validation.

Here is an example of how you can modify the import_chart function to include proper ownership checks:

def import_chart(
    config: dict[str, Any],
    overwrite: bool = False,
    ignore_permissions: bool = False,
) -> Slice:
    can_write = ignore_permissions or security_manager.can_access("can_write", "Chart")
    existing = db.session.query(Slice).filter_by(uuid=config["uuid"]).first()
    if existing:
        if overwrite and can_write and get_user():
            if not security_manager.can_access_chart(existing):
                raise ImportFailedError(
                    "A chart already exists and user doesn't "
                    "have permissions to overwrite it"
                )
        if not overwrite or not can_write:
            return existing
        config["id"] = existing.id
    elif not can_write:
        raise ImportFailedError(
            "Chart doesn't exist and user doesn't have permission to create charts"
        )

    filter_chart_annotations(config)

    # TODO (betodealmeida): move this logic to import_from_dict
    config["params"] = json.dumps(config["params"])

    # migrate old viz types to new ones
    config = migrate_chart(config)

    chart = Slice.import_from_dict(config, recursive=False, allow_reparenting=True)
    if chart.id is None:
        db.session.flush()

    if (user := get_user()) and user not in chart.owners:
        try:
            security_manager.raise_for_ownership(chart)
            if security_manager.can_access("can_write", "Chart"):
                chart.owners.append(user)
        except SupersetSecurityException:
            raise ImportFailedError(
                "User doesn't have permission to take ownership of the chart"
            )

    return chart

In this modification, the security_manager.raise_for_ownership(chart) method is used to ensure that the user has the necessary permissions to take ownership of the chart. If the user does not have the required permissions, an ImportFailedError is raised, preventing the Gamma role user from taking ownership of the chart. This approach aligns with the ownership validation logic found in the UpdateChartCommand class ^[1].

Additionally, you can refer to the integration tests in tests/integration_tests/import_export_tests.py to ensure that your changes are properly tested. For example, the test_import_new_dashboard_slice_reset_ownership test case verifies that the ownership is correctly reset when a dashboard is imported by a different user:

def test_import_new_dashboard_slice_reset_ownership(self):
    admin_user = security_manager.find_user(username="admin")
    self.assertTrue(admin_user)
    gamma_user = security_manager.find_user(username="gamma")
    self.assertTrue(gamma_user)
    g.user = gamma_user

    dash_with_1_slice = self._create_dashboard_for_import(id_=10200)
    # set another user as an owner of importing dashboard
    dash_with_1_slice.created_by = admin_user
    dash_with_1_slice.changed_by = admin_user
    dash_with_1_slice.owners = [admin_user]

    imported_dash_id = import_dashboard(dash_with_1_slice)
    imported_dash = self.get_dash(imported_dash_id)
    self.assertEqual(imported_dash.created_by, gamma_user)
    self.assertEqual(imported_dash.changed_by, gamma_user)
    self.assertEqual(imported_dash.owners, [gamma_user])

    imported_slc = imported_dash.slices[0]
    self.assertEqual(imported_slc.created_by, gamma_user)
    self.assertEqual(imported_slc.changed_by, gamma_user)
    self.assertEqual(imported_slc.owners, [gamma_user])

This test ensures that the ownership is correctly assigned to the importing user, preventing unauthorized ownership changes ^[2].

To continue talking to Dosu, mention @dosu.

[joezeon]

@dosu Can you point me to documentation how ownership works. Im reading some old discussion threads and it seems "Owners are the only people who can alter their objects, or expand the ownership of objects." And clearly importing seems to change ownership to the user doing the import from the UI.

Alternately, is there a way to disable imports from UI for Gamma role. Since, I do need to grant write on charts and dashboards to gamma users so they can create their own charts/dashboards.

[dosubot[bot]]

Ownership in Apache Superset is managed through roles and permissions. The Gamma role has limited access, primarily for consuming data and creating slices and dashboards, but not for altering or adding data sources.

To disable imports from the UI for users with the Gamma role while still granting them write permissions on charts and dashboards, you can customize the permissions for the Gamma role. Specifically, you can remove the permissions related to importing from the Gamma role and ensure they have the necessary permissions to edit charts and dashboards.

Here is a step-by-step approach:

1. Create a Custom Role: Create a new role that includes the permissions for editing charts and dashboards but excludes the import permissions.
2. Assign Permissions: Assign the necessary permissions to this custom role.
3. Assign Roles to Users: Assign both the Gamma role and the new custom role to the users.

Example Configuration

1. Create a Custom Role:

   • Go to Menu -> Security -> List Roles.
   • Click the + sign to create a new role.
   • Name the role (e.g., CustomGamma).

2. Assign Permissions:

   • In the permissions dropdown, select the permissions for editing charts and dashboards (e.g., can_edit on Dashboard, can_edit on Slice).
   • Ensure that permissions related to importing (e.g., can_import_dashboards, can_import_slices) are not included.

3. Assign Roles to Users:

   • Assign both the Gamma role and the new CustomGamma role to the users.

Additionally, you can leverage utility functions such as canUserEditDashboard and userHasPermission to programmatically check and enforce these permissions. For example, canUserEditDashboard ensures that only users with the appropriate roles and permissions can edit dashboards, while userHasPermission can be used to verify specific permissions for a user.

In the superset/security/manager.py file, you can see that the SupersetSecurityManager class defines various role-based access controls. For instance, the GAMMA_READ_ONLY_MODEL_VIEWS set includes views that are read-only for Gamma users. You can customize this to ensure that import-related views are not accessible to Gamma users while still allowing them to edit charts and dashboards ^[1][2][3].

To continue talking to Dosu, mention @dosu.