korbit-ai[bot] commented on code in PR #32231:
URL: https://github.com/apache/superset/pull/32231#discussion_r1952016478
##########
superset-frontend/src/pages/DatabaseList/index.tsx:
##########
@@ -426,6 +431,49 @@ function DatabaseList({
handleDatabaseEditModal({ database: original, modalOpen: true });
const handleDelete = () => openDatabaseDeleteModal(original);
const handleExport = () => handleDatabaseExport(original);
+ const handleResync = () => {
+ shouldResyncPermsInAsyncMode
+ ? addInfoToast(
+ t('Validating connectivity for %s', original.database_name),
+ )
+ : addInfoToast(
+ t('Resyncing permissions for %s', original.database_name),
+ );
+ SupersetClient.post({
+ endpoint: `/api/v1/database/${original.id}/resync_permissions/`,
+ })
+ .then(({ response, json }) => {
+ // Sync request
+ if (response.status === 200) {
+ addSuccessToast(
+ t(
+ 'Permissions successfully resynced for %s',
+ original.database_name,
+ ),
+ );
+ }
+ // Async request
+ else {
+ addInfoToast(
+ t(
+ 'Syncing permissions for %s in the background',
+ original.database_name,
+ ),
+ );
+ }
+ })
+ .catch(
+ createErrorHandler(errMsg =>
+ addDangerToast(
+ t(
+ 'An error occurred while resyncing permissions for %s:
%s',
+ original.database_name,
+ errMsg,
+ ),
+ ),
+ ),
+ );
Review Comment:
### Incomplete Error Handling in Permission Resync <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The catch block only displays a toast message without preserving the error
details or rethrowing the error for global error tracking.
###### Why this matters
Loss of valuable debugging information that could help diagnose production
issues. The error details are only shown to the user but not logged or tracked.
###### Suggested change ∙ *Feature Preview*
Modify the catch block to log the error and potentially rethrow it:
```typescript
.catch(error => {
console.error('Permission resync failed:', error);
createErrorHandler(errMsg =>
addDangerToast(
t(
'An error occurred while resyncing permissions for %s: %s',
original.database_name,
errMsg,
),
),
)(error);
throw error; // If you want to propagate to error monitoring
});
```
</details>
<sub>
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/3305ef8e-32d3-4972-9c8b-32c8812da93e?suggestedFixEnabled=true)
💬 Chat with Korbit by mentioning @korbit-ai.
</sub>
<!--- korbi internal id:9bc0ad72-79e8-4b73-b0c8-1610baa4b7f2 -->
##########
superset/tasks/permissions.py:
##########
@@ -0,0 +1,56 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+from __future__ import annotations
+
+import logging
+
+from flask import current_app, g
+
+from superset import security_manager
+from superset.commands.database.resync_permissions import
ResyncPermissionsCommand
+from superset.daos.database import DatabaseDAO
+from superset.extensions import celery_app
+
+logger = logging.getLogger(__name__)
+
+
+@celery_app.task(name="resync_database_permissions", soft_time_limit=600)
+def resync_database_permissions(
+ database_id: int, username: str, original_database_name: str
+) -> None:
+ logger.info("Resyncing permissions for DB connection ID %s", database_id)
+ with current_app.test_request_context():
+ try:
+ user = security_manager.get_user_by_username(username)
+ assert user
+ g.user = user
Review Comment:
### Unsafe User Impersonation in Celery Task <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The code directly assigns a user to Flask's g.user global context based on a
username parameter without proper authentication verification in a celery task.
###### Why this matters
This allows impersonation of any user within the Celery task context, which
could bypass authentication controls if the task is invoked with an arbitrary
username.
###### Suggested change ∙ *Feature Preview*
Add authentication verification before allowing user impersonation. For
example:
```python
user = security_manager.get_user_by_username(username)
if not user or not security_manager.can_access_database(user, database_id):
raise SecurityError('Unauthorized access')
g.user = user
```
</details>
<sub>
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/97528c93-77c7-4f9f-91bf-c9c47254161a?suggestedFixEnabled=true)
💬 Chat with Korbit by mentioning @korbit-ai.
</sub>
<!--- korbi internal id:608f0e1b-37b5-4394-b5fd-87df36378400 -->
##########
superset/commands/database/resync_permissions.py:
##########
@@ -0,0 +1,276 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+from __future__ import annotations
+
+import logging
+from functools import partial
+from typing import Iterable
+
+from superset import security_manager
+from superset.commands.base import BaseCommand
+from superset.commands.database.exceptions import (
+ DatabaseConnectionFailedError,
+ DatabaseConnectionResyncPermissionsError,
+ DatabaseNotFoundError,
+)
+from superset.commands.database.utils import ping
+from superset.daos.database import DatabaseDAO
+from superset.daos.dataset import DatasetDAO
+from superset.databases.ssh_tunnel.models import SSHTunnel
+from superset.db_engine_specs.base import GenericDBException
+from superset.exceptions import OAuth2RedirectError
+from superset.models.core import Database
+from superset.utils.decorators import on_error, transaction
+
+logger = logging.getLogger(__name__)
+
+
+class ResyncPermissionsCommand(BaseCommand):
+ """
+ Command to resync database permissions.
+ """
+
+ def __init__(
+ self,
+ model_id: int,
+ old_db_connection_name: str | None = None,
+ db_connection: Database | None = None,
+ ssh_tunnel: SSHTunnel | None = None,
+ ):
+ """
+ Constructor method.
+ """
+ self.db_connection_id = model_id
+ self.old_db_connection_name: str | None = old_db_connection_name
+ self.db_connection: Database | None = db_connection
+ self.db_connection_ssh_tunnel: SSHTunnel | None = ssh_tunnel
+
+ def validate(self) -> None:
+ if not self.db_connection:
+ database = DatabaseDAO.find_by_id(self.db_connection_id)
+ if not database:
+ raise DatabaseNotFoundError()
+ self.db_connection = database
+
+ if not self.old_db_connection_name:
+ self.old_db_connection_name = self.db_connection.database_name
+
+ if not self.db_connection_ssh_tunnel:
+ self.db_connection_ssh_tunnel = DatabaseDAO.get_ssh_tunnel(
+ self.db_connection_id
+ )
+
+ with self.db_connection.get_sqla_engine() as engine:
+ try:
+ alive = ping(engine)
+ except Exception as err:
+ raise DatabaseConnectionFailedError() from err
+
+ if not alive:
+ raise DatabaseConnectionFailedError()
+
+ @transaction(
+ on_error=partial(on_error,
reraise=DatabaseConnectionResyncPermissionsError)
+ )
+ def run(self) -> None:
+ """
+ Resyncs the permissions for a DB connection.
+ """
+ self.validate()
+
+ # Make mypy happy (these are already checked in validate)
+ assert self.db_connection
+ assert self.old_db_connection_name
+
+ catalogs = (
+ self._get_catalog_names(self.db_connection)
+ if self.db_connection.db_engine_spec.supports_catalog
+ else [None]
+ )
+
+ for catalog in catalogs:
+ try:
+ schemas = self._get_schema_names(self.db_connection, catalog)
+
+ if catalog:
+ perm = security_manager.get_catalog_perm(
+ self.old_db_connection_name,
+ catalog,
+ )
+ existing_pvm = security_manager.find_permission_view_menu(
+ "catalog_access",
+ perm,
+ )
+ if not existing_pvm:
+ # new catalog
+ security_manager.add_permission_view_menu(
+ "catalog_access",
+ security_manager.get_catalog_perm(
+ self.db_connection.database_name,
+ catalog,
+ ),
+ )
+ for schema in schemas:
+ security_manager.add_permission_view_menu(
+ "schema_access",
+ security_manager.get_schema_perm(
+ self.db_connection.database_name,
+ catalog,
+ schema,
+ ),
+ )
+ continue
+ except DatabaseConnectionFailedError:
+ # more than one catalog, move to next
+ if catalog:
+ logger.warning("Error processing catalog %s", catalog)
+ continue
+ raise
+
+ # add possible new schemas in catalog
+ self._refresh_schemas(
+ self.old_db_connection_name,
+ self.db_connection.database_name,
+ catalog,
+ schemas,
+ )
+
+ if self.old_db_connection_name != self.db_connection.database_name:
+ self._rename_database_in_permissions(
+ self.old_db_connection_name,
+ self.db_connection.database_name,
+ catalog,
+ schemas,
+ )
+
+ def _get_catalog_names(self, db_connection: Database) -> set[str]:
+ """
+ Helper method to load catalogs.
+ """
+ try:
+ return db_connection.get_all_catalog_names(
+ force=True,
+ ssh_tunnel=self.db_connection_ssh_tunnel,
+ )
+ except OAuth2RedirectError:
+ # raise OAuth2 exceptions as-is
+ raise
+ except GenericDBException as ex:
+ raise DatabaseConnectionFailedError() from ex
+
+ def _get_schema_names(
+ self, db_connection: Database, catalog: str | None
+ ) -> set[str]:
+ """
+ Helper method to load schemas.
+ """
+ try:
+ return db_connection.get_all_schema_names(
+ force=True,
+ catalog=catalog,
+ ssh_tunnel=self.db_connection_ssh_tunnel,
+ )
+ except OAuth2RedirectError:
+ # raise OAuth2 exceptions as-is
+ raise
+ except GenericDBException as ex:
+ raise DatabaseConnectionFailedError() from ex
+
+ def _refresh_schemas(
+ self,
+ old_db_connection_name: str,
+ new_db_connection_name: str,
+ catalog: str | None,
+ schemas: Iterable[str],
+ ) -> None:
+ """
+ Add new schemas that don't have permissions yet.
+ """
+ for schema in schemas:
+ perm = security_manager.get_schema_perm(
+ old_db_connection_name,
+ catalog,
+ schema,
+ )
+ existing_pvm = security_manager.find_permission_view_menu(
+ "schema_access",
+ perm,
+ )
+ if not existing_pvm:
+ new_name = security_manager.get_schema_perm(
+ new_db_connection_name,
+ catalog,
+ schema,
+ )
+ security_manager.add_permission_view_menu("schema_access",
new_name)
+
+ def _rename_database_in_permissions(
+ self,
+ old_db_connection_name: str,
+ new_db_connection_name: str,
+ catalog: str | None,
+ schemas: Iterable[str],
+ ) -> None:
+ new_catalog_perm_name = security_manager.get_catalog_perm(
+ new_db_connection_name,
+ catalog,
+ )
+
+ # rename existing catalog permission
+ if catalog:
+ perm = security_manager.get_catalog_perm(
+ old_db_connection_name,
+ catalog,
+ )
+ existing_pvm = security_manager.find_permission_view_menu(
+ "catalog_access",
+ perm,
+ )
+ if existing_pvm:
+ existing_pvm.view_menu.name = new_catalog_perm_name
+
+ for schema in schemas:
+ new_schema_perm_name = security_manager.get_schema_perm(
+ new_db_connection_name,
+ catalog,
+ schema,
+ )
+
+ # rename existing schema permission
+ perm = security_manager.get_schema_perm(
+ old_db_connection_name,
+ catalog,
+ schema,
+ )
+ existing_pvm = security_manager.find_permission_view_menu(
+ "schema_access",
+ perm,
+ )
+ if existing_pvm:
+ existing_pvm.view_menu.name = new_schema_perm_name
+
+ # rename permissions on datasets and charts
+ for dataset in DatabaseDAO.get_datasets(
+ self.db_connection_id,
+ catalog=catalog,
+ schema=schema,
+ ):
+ dataset.catalog_perm = new_catalog_perm_name
+ dataset.schema_perm = new_schema_perm_name
+ for chart in
DatasetDAO.get_related_objects(dataset.id)["charts"]:
+ chart.catalog_perm = new_catalog_perm_name
+ chart.schema_perm = new_schema_perm_name
Review Comment:
### N+1 Query Pattern in Permission Updates <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
Nested loops performing database queries for each dataset and its related
charts, without batching or bulk updates.
###### Why this matters
This can lead to N+1 query performance issues when dealing with large
numbers of datasets and charts, causing excessive database round trips.
###### Suggested change ∙ *Feature Preview*
Batch the permission updates using bulk database operations. Consider using
a bulk update query or collecting all objects first and then updating them in a
single transaction:
```python
datasets = DatabaseDAO.get_datasets(self.db_connection_id, catalog=catalog,
schema=schema)
dataset_ids = [d.id for d in datasets]
# Bulk update datasets
DatasetDAO.bulk_update_permissions(dataset_ids,
catalog_perm=new_catalog_perm_name,
schema_perm=new_schema_perm_name)
# Bulk update related charts
DatasetDAO.bulk_update_chart_permissions(dataset_ids,
catalog_perm=new_catalog_perm_name,
schema_perm=new_schema_perm_name)
```
</details>
<sub>
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/c9c6bb7a-8447-44a1-806a-33d0b8837be0?suggestedFixEnabled=true)
💬 Chat with Korbit by mentioning @korbit-ai.
</sub>
<!--- korbi internal id:e9dc1c08-5e22-4957-9f56-629301c40b07 -->
##########
superset/commands/database/resync_permissions_async.py:
##########
@@ -0,0 +1,101 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+from __future__ import annotations
+
+import logging
+
+from superset import security_manager
+from superset.commands.base import BaseCommand
+from superset.commands.database.exceptions import (
+ DatabaseConnectionFailedError,
+ DatabaseNotFoundError,
+ UserNotFoundError,
+)
+from superset.commands.database.utils import ping
+from superset.daos.database import DatabaseDAO
+from superset.tasks.permissions import resync_database_permissions
+
+logger = logging.getLogger(__name__)
+
+
+class ResyncPermissionsAsyncCommand(BaseCommand):
+ """
+ Command to trigger an async task to resync database permissions.
+ """
+
+ def __init__(
+ self,
+ model_id: int,
+ username: str | None,
+ old_db_connection_name: str | None = None,
+ ):
+ """
+ Constructor method.
+ """
+ self.db_connection_id = model_id
+ self.username = username
+ self.old_db_connection_name = old_db_connection_name
+
+ def validate(self) -> None:
+ """
+ Validates the command before triggering the async task.
+
+ Confirms both the DB connection user exist. Also tests the DB
connection.
+ """
+ database = DatabaseDAO.find_by_id(self.db_connection_id)
+ if not database:
+ raise DatabaseNotFoundError()
+
+ if not self.old_db_connection_name:
+ self.old_db_connection_name = database.database_name
+
+ if not self.username or not security_manager.get_user_by_username(
+ self.username
+ ):
+ raise UserNotFoundError()
+
+ with database.get_sqla_engine() as engine:
+ # Make sure the connection works before delegating the task
+ try:
+ alive = ping(engine)
+ except Exception as err:
+ logger.error("Could not stablish a DB connection")
+ raise DatabaseConnectionFailedError() from err
+
+ if not alive:
+ logger.error("Could not stablish a DB connection")
+ raise DatabaseConnectionFailedError()
Review Comment:
### Misspelled Error Message <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The error message contains a spelling error ('stablish' instead of
'establish') which could cause confusion in logs
###### Why this matters
Incorrect error messages can make troubleshooting more difficult and appear
unprofessional in production logs
###### Suggested change ∙ *Feature Preview*
Correct the spelling in both error messages:
```python
logger.error("Could not establish a DB connection")
```
</details>
<sub>
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/c11cc760-0f80-4ca3-8083-6c414bc3bb30?suggestedFixEnabled=true)
💬 Chat with Korbit by mentioning @korbit-ai.
</sub>
<!--- korbi internal id:84c5d8be-c8fc-4be5-ae4a-0ec114ae8105 -->
##########
superset/config.py:
##########
@@ -1916,6 +1916,15 @@ class ExtraDynamicQueryFilters(TypedDict, total=False):
CATALOGS_SIMPLIFIED_MIGRATION: bool = False
+# When updating a DB connection or manually triggering a resync, the command
+# happens in sync mode. If you have a celery worker configured, it's
recommended
+# to change below config to ``True`` to run this process in async mode. A DB
+# connection might have hundreds of catalogs with thousands of schemas each,
which
+# considerably increases the time to process it. Running it in async mode
prevents
+# keeping a web API call open for this long.
+RESYNC_DB_PERMISSIONS_IN_ASYNC_MODE: bool = False
Review Comment:
### Default Async Mode Contradicts Intent <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The default value for RESYNC_DB_PERMISSIONS_IN_ASYNC_MODE is set to False,
which contradicts the developer's intent of enabling more efficient
asynchronous resynchronization of database permissions.
###### Why this matters
With the default set to False, all users will experience synchronous
permission resynchronization by default, potentially causing web API calls to
hang for long periods when dealing with large datasets.
###### Suggested change ∙ *Feature Preview*
Change the default value to True to align with the stated performance goals:
```python
RESYNC_DB_PERMISSIONS_IN_ASYNC_MODE: bool = True
```
</details>
<sub>
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/dc4ea233-468f-45b7-993a-fab8911d654a?suggestedFixEnabled=true)
💬 Chat with Korbit by mentioning @korbit-ai.
</sub>
<!--- korbi internal id:9212eb3a-692d-43fb-958b-1a006ca57928 -->
##########
superset/commands/database/utils.py:
##########
@@ -17,15 +17,33 @@
from __future__ import annotations
import logging
+import sqlite3
+from contextlib import closing
+
+from flask import current_app as app
+from sqlalchemy.engine import Engine
from superset import security_manager
from superset.databases.ssh_tunnel.models import SSHTunnel
from superset.db_engine_specs.base import GenericDBException
from superset.models.core import Database
+from superset.utils.core import timeout
logger = logging.getLogger(__name__)
+def ping(engine: Engine) -> bool:
+ try:
+ time_delta = app.config["TEST_DATABASE_CONNECTION_TIMEOUT"]
+ with timeout(int(time_delta.total_seconds())):
+ with closing(engine.raw_connection()) as conn:
+ return engine.dialect.do_ping(conn)
+ except (sqlite3.ProgrammingError, RuntimeError):
+ # SQLite can't run on a separate thread, so ``utils.timeout`` fails
+ # RuntimeError catches the equivalent error from duckdb.
+ return engine.dialect.do_ping(engine)
Review Comment:
### Silent Error Handling in Database Ping <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The error handling in the ping function silently continues execution without
logging the error details.
###### Why this matters
Without logging the error, it becomes difficult to diagnose issues when
these specific database errors occur in production.
###### Suggested change ∙ *Feature Preview*
Add error logging before continuing execution:
```python
except (sqlite3.ProgrammingError, RuntimeError) as e:
logger.warning("Database ping timeout mechanism failed, falling back to
direct ping: %s", str(e))
return engine.dialect.do_ping(engine)
```
</details>
<sub>
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/67b12db9-3c68-4777-98e0-0ed533be7e3d?suggestedFixEnabled=true)
💬 Chat with Korbit by mentioning @korbit-ai.
</sub>
<!--- korbi internal id:7e7253a4-5239-4963-aa2f-891a35fe4754 -->
##########
superset/tasks/permissions.py:
##########
@@ -0,0 +1,56 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+from __future__ import annotations
+
+import logging
+
+from flask import current_app, g
+
+from superset import security_manager
+from superset.commands.database.resync_permissions import
ResyncPermissionsCommand
+from superset.daos.database import DatabaseDAO
+from superset.extensions import celery_app
+
+logger = logging.getLogger(__name__)
+
+
+@celery_app.task(name="resync_database_permissions", soft_time_limit=600)
+def resync_database_permissions(
+ database_id: int, username: str, original_database_name: str
+) -> None:
+ logger.info("Resyncing permissions for DB connection ID %s", database_id)
+ with current_app.test_request_context():
+ try:
+ user = security_manager.get_user_by_username(username)
+ assert user
Review Comment:
### Unsafe User Existence Check <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The code uses an assert statement to check for user existence, which could
be stripped in production when running Python with -O or -OO flags.
###### Why this matters
If Python optimization is enabled in production, this check will be removed,
potentially leading to NoneType errors when trying to access user attributes.
###### Suggested change ∙ *Feature Preview*
Replace the assert with an explicit check and raise a proper exception:
```python
if not user:
raise ValueError(f"User {username} not found")
```
</details>
<sub>
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/48001443-fe11-4bef-9dc5-04701ee28f8c?suggestedFixEnabled=true)
💬 Chat with Korbit by mentioning @korbit-ai.
</sub>
<!--- korbi internal id:8f78ca04-a9e1-4073-8477-90912dbf9eea -->
##########
superset/commands/database/resync_permissions.py:
##########
@@ -0,0 +1,276 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+from __future__ import annotations
+
+import logging
+from functools import partial
+from typing import Iterable
+
+from superset import security_manager
+from superset.commands.base import BaseCommand
+from superset.commands.database.exceptions import (
+ DatabaseConnectionFailedError,
+ DatabaseConnectionResyncPermissionsError,
+ DatabaseNotFoundError,
+)
+from superset.commands.database.utils import ping
+from superset.daos.database import DatabaseDAO
+from superset.daos.dataset import DatasetDAO
+from superset.databases.ssh_tunnel.models import SSHTunnel
+from superset.db_engine_specs.base import GenericDBException
+from superset.exceptions import OAuth2RedirectError
+from superset.models.core import Database
+from superset.utils.decorators import on_error, transaction
+
+logger = logging.getLogger(__name__)
+
+
+class ResyncPermissionsCommand(BaseCommand):
+ """
+ Command to resync database permissions.
+ """
+
+ def __init__(
+ self,
+ model_id: int,
+ old_db_connection_name: str | None = None,
+ db_connection: Database | None = None,
+ ssh_tunnel: SSHTunnel | None = None,
+ ):
+ """
+ Constructor method.
+ """
+ self.db_connection_id = model_id
+ self.old_db_connection_name: str | None = old_db_connection_name
+ self.db_connection: Database | None = db_connection
+ self.db_connection_ssh_tunnel: SSHTunnel | None = ssh_tunnel
+
+ def validate(self) -> None:
+ if not self.db_connection:
+ database = DatabaseDAO.find_by_id(self.db_connection_id)
+ if not database:
+ raise DatabaseNotFoundError()
+ self.db_connection = database
+
+ if not self.old_db_connection_name:
+ self.old_db_connection_name = self.db_connection.database_name
+
+ if not self.db_connection_ssh_tunnel:
+ self.db_connection_ssh_tunnel = DatabaseDAO.get_ssh_tunnel(
+ self.db_connection_id
+ )
+
+ with self.db_connection.get_sqla_engine() as engine:
+ try:
+ alive = ping(engine)
+ except Exception as err:
+ raise DatabaseConnectionFailedError() from err
+
+ if not alive:
+ raise DatabaseConnectionFailedError()
+
+ @transaction(
+ on_error=partial(on_error,
reraise=DatabaseConnectionResyncPermissionsError)
+ )
+ def run(self) -> None:
+ """
+ Resyncs the permissions for a DB connection.
+ """
+ self.validate()
+
+ # Make mypy happy (these are already checked in validate)
+ assert self.db_connection
+ assert self.old_db_connection_name
+
+ catalogs = (
+ self._get_catalog_names(self.db_connection)
+ if self.db_connection.db_engine_spec.supports_catalog
+ else [None]
+ )
+
+ for catalog in catalogs:
+ try:
+ schemas = self._get_schema_names(self.db_connection, catalog)
+
+ if catalog:
+ perm = security_manager.get_catalog_perm(
+ self.old_db_connection_name,
+ catalog,
+ )
+ existing_pvm = security_manager.find_permission_view_menu(
+ "catalog_access",
+ perm,
+ )
+ if not existing_pvm:
+ # new catalog
+ security_manager.add_permission_view_menu(
+ "catalog_access",
+ security_manager.get_catalog_perm(
+ self.db_connection.database_name,
+ catalog,
+ ),
+ )
+ for schema in schemas:
+ security_manager.add_permission_view_menu(
+ "schema_access",
+ security_manager.get_schema_perm(
+ self.db_connection.database_name,
+ catalog,
+ schema,
+ ),
+ )
+ continue
+ except DatabaseConnectionFailedError:
+ # more than one catalog, move to next
+ if catalog:
+ logger.warning("Error processing catalog %s", catalog)
+ continue
+ raise
Review Comment:
### Inconsistent Catalog Connection Error Handling <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The error handling logic for catalog processing silently continues on
connection failures when catalog is not None, but raises the error when catalog
is None
###### Why this matters
This inconsistent error handling could mask serious connection issues for
catalogs and lead to incomplete permission synchronization without proper
notification
###### Suggested change ∙ *Feature Preview*
Implement consistent error handling for all catalog cases, preferably
logging the error and collecting all failures for a summary report:
```python
try:
schemas = self._get_schema_names(self.db_connection, catalog)
except DatabaseConnectionFailedError as e:
logger.error(f"Error processing catalog {catalog}: {str(e)}")
self.errors.append((catalog, str(e)))
if not catalog: # If this is the default catalog, we should fail
raise
continue
```
</details>
<sub>
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/31d9a5e8-aeca-482b-a5d0-59974f3fa479?suggestedFixEnabled=true)
💬 Chat with Korbit by mentioning @korbit-ai.
</sub>
<!--- korbi internal id:a747fef8-68b7-481f-b8d1-1e2360958185 -->
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
