-
Notifications
You must be signed in to change notification settings - Fork 14.1k
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
Changes from 3 commits
1fa370c
c5ddcfc
b2b230c
24455bf
ed7c36f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -25,9 +25,9 @@ | |
cast, | ||
Dict, | ||
List, | ||
NamedTuple, | ||
Optional, | ||
Set, | ||
Tuple, | ||
TYPE_CHECKING, | ||
Union, | ||
) | ||
|
@@ -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 | ||
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: any reason why these can't be |
||
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: | ||
""" | ||
|
@@ -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}].") | ||
} | ||
|
@@ -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] | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can you add a test where the table is not allowed also? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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"}: | ||
|
There was a problem hiding this comment.
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
?