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

feat: datasource access to allow more granular access to tables on SQL Lab #18064

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 13 additions & 6 deletions superset/databases/filters.py
Original file line number Diff line number Diff line change
Expand Up @@ -25,21 +25,28 @@

class DatabaseFilter(BaseFilter):
# TODO(bogdan): consider caching.
def schema_access_databases(self) -> Set[str]: # noqa pylint: disable=no-self-use

def can_access_databases( # noqa pylint: disable=no-self-use
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: any reason why this can't be @staticmethod ?

self, view_menu_name: str,
) -> Set[str]:
return {
security_manager.unpack_schema_perm(vm)[0]
for vm in security_manager.user_view_menu_names("schema_access")
security_manager.unpack_database_and_schema(vm).database
for vm in security_manager.user_view_menu_names(view_menu_name)
}

def apply(self, query: Query, value: Any) -> Query:
if security_manager.can_access_all_databases():
return query
database_perms = security_manager.user_view_menu_names("database_access")
# TODO(bogdan): consider adding datasource access here as well.
schema_access_databases = self.schema_access_databases()
schema_access_databases = self.can_access_databases("schema_access")

datasource_access_databases = self.can_access_databases("datasource_access")

return query.filter(
or_(
self.model.perm.in_(database_perms),
self.model.database_name.in_(schema_access_databases),
self.model.database_name.in_(
[*schema_access_databases, *datasource_access_databases]
),
)
)
20 changes: 13 additions & 7 deletions superset/security/manager.py
Original file line number Diff line number Diff line change
Expand Up @@ -25,9 +25,9 @@
cast,
Dict,
List,
NamedTuple,
Optional,
Set,
Tuple,
TYPE_CHECKING,
Union,
)
Expand Down Expand Up @@ -78,6 +78,11 @@
logger = logging.getLogger(__name__)


class DatabaseAndSchema(NamedTuple):
database: str
schema: str


class SupersetSecurityListWidget(ListWidget): # pylint: disable=too-few-public-methods
"""
Redeclaring to avoid circular imports
Expand Down Expand Up @@ -237,13 +242,14 @@ def get_schema_perm( # pylint: disable=no-self-use

return None

def unpack_schema_perm( # pylint: disable=no-self-use
def unpack_database_and_schema( # pylint: disable=no-self-use
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: any reason why these can't be @staticmethod ?

self, schema_permission: str
) -> Tuple[str, str]:
# [database_name].[schema_name]
) -> DatabaseAndSchema:
# [database_name].[schema|table]

schema_name = schema_permission.split(".")[1][1:-1]
database_name = schema_permission.split(".")[0][1:-1]
return database_name, schema_name
return DatabaseAndSchema(database_name, schema_name)

def can_access(self, permission_name: str, view_name: str) -> bool:
"""
Expand Down Expand Up @@ -532,7 +538,7 @@ def get_schemas_accessible_by_user(

# schema_access
accessible_schemas = {
self.unpack_schema_perm(s)[1]
self.unpack_database_and_schema(s).schema
for s in self.user_view_menu_names("schema_access")
if s.startswith(f"[{database}].")
}
Expand Down Expand Up @@ -582,7 +588,7 @@ def get_datasources_accessible_by_user( # pylint: disable=invalid-name
)
if schema:
names = {d.table_name for d in user_datasources if d.schema == schema}
return [d for d in datasource_names if d in names]
return [d for d in datasource_names if d.table in names]

full_names = {d.full_name for d in user_datasources}
return [d for d in datasource_names if f"[{database}].[{d}]" in full_names]
Expand Down
59 changes: 59 additions & 0 deletions tests/integration_tests/core_tests.py
Original file line number Diff line number Diff line change
Expand Up @@ -161,6 +161,65 @@ def test_get_superset_tables_not_allowed(self):
rv = self.client.get(uri)
self.assertEqual(rv.status_code, 404)

@pytest.mark.usefixtures("load_energy_table_with_slice")
def test_get_superset_tables_allowed(self):
session = db.session
table_name = "energy_usage"
role_name = "dummy_role"
self.logout()
self.login(username="gamma")
gamma_user = security_manager.find_user(username="gamma")
security_manager.add_role(role_name)
dummy_role = security_manager.find_role(role_name)
gamma_user.roles.append(dummy_role)

tbl_id = self.table_ids.get(table_name)
table = db.session.query(SqlaTable).filter(SqlaTable.id == tbl_id).first()
table_perm = table.perm

security_manager.add_permission_role(
dummy_role,
security_manager.find_permission_view_menu("datasource_access", table_perm),
)

session.commit()

example_db = utils.get_example_database()
schema_name = self.default_schema_backend_map[example_db.backend]
uri = f"superset/tables/{example_db.id}/{schema_name}/{table_name}/"
rv = self.client.get(uri)
self.assertEqual(rv.status_code, 200)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you add a test where the table is not allowed also?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added 👍


# cleanup
gamma_user = security_manager.find_user(username="gamma")
gamma_user.roles.remove(security_manager.find_role(role_name))
session.commit()

@pytest.mark.usefixtures("load_energy_table_with_slice")
def test_get_superset_tables_not_allowed_with_out_permissions(self):
session = db.session
table_name = "energy_usage"
role_name = "dummy_role_no_table_access"
self.logout()
self.login(username="gamma")
gamma_user = security_manager.find_user(username="gamma")
security_manager.add_role(role_name)
dummy_role = security_manager.find_role(role_name)
gamma_user.roles.append(dummy_role)

session.commit()

example_db = utils.get_example_database()
schema_name = self.default_schema_backend_map[example_db.backend]
uri = f"superset/tables/{example_db.id}/{schema_name}/{table_name}/"
rv = self.client.get(uri)
self.assertEqual(rv.status_code, 404)

# cleanup
gamma_user = security_manager.find_user(username="gamma")
gamma_user.roles.remove(security_manager.find_role(role_name))
session.commit()

def test_get_superset_tables_substr(self):
example_db = utils.get_example_database()
if example_db.backend in {"presto", "hive"}:
Expand Down
6 changes: 4 additions & 2 deletions tests/integration_tests/datasets/api_tests.py
Original file line number Diff line number Diff line change
Expand Up @@ -223,8 +223,10 @@ def test_get_dataset_related_database_gamma(self):
rv = self.client.get(uri)
assert rv.status_code == 200
response = json.loads(rv.data.decode("utf-8"))
assert response["count"] == 0
assert response["result"] == []

assert response["count"] == 1
main_db = get_main_database()
assert filter(lambda x: x.text == main_db, response["result"]) != []

@pytest.mark.usefixtures("load_energy_table_with_slice")
def test_get_dataset_item(self):
Expand Down