diff --git a/backend/src/api/routes/admin.py b/backend/src/api/routes/admin.py index a5cea4580..41f249607 100644 --- a/backend/src/api/routes/admin.py +++ b/backend/src/api/routes/admin.py @@ -142,6 +142,9 @@ async def update_user( # #endregion Api.Admin.UpdateUser + + + # #region Api.Admin.DeleteUser [C:3] [TYPE Function] # @ingroup Api # @BRIEF Deletes a user. @@ -251,7 +254,16 @@ async def update_role( role.name = role_in.name if role_in.description is not None: role.description = role_in.description - if role_in.is_admin is not None: + if role_in.is_admin is not None and role_in.is_admin != role.is_admin: + # Last-admin-role guard: never remove the admin flag from the final + # is_admin role, otherwise every admin user would lose access. + if role_in.is_admin is False and role.is_admin is True: + admin_role_count = db.query(Role).filter(Role.is_admin.is_(True)).count() + if admin_role_count <= 1: + raise HTTPException( + status_code=409, + detail="Cannot remove the admin flag from the last admin role — this would lock everyone out.", + ) role.is_admin = role_in.is_admin if role_in.permissions is not None: diff --git a/backend/src/api/routes/maintenance/_routes.py b/backend/src/api/routes/maintenance/_routes.py index b9c38ea70..a3bacd672 100644 --- a/backend/src/api/routes/maintenance/_routes.py +++ b/backend/src/api/routes/maintenance/_routes.py @@ -480,7 +480,7 @@ async def start_maintenance( ) # ── Broadcast maintenance event ── - task_manager.broadcast_maintenance_event({ + await task_manager.broadcast_maintenance_event({ "type": "maintenance.event_created", "maintenance_id": event.id, "tables": sorted_tables, @@ -563,7 +563,7 @@ async def end_maintenance( ) # ── Broadcast maintenance event ── - task_manager.broadcast_maintenance_event({ + await task_manager.broadcast_maintenance_event({ "type": "maintenance.event_ended", "maintenance_id": maintenance_id, }) @@ -634,7 +634,7 @@ async def end_all_maintenance( ) # ── Broadcast maintenance event ── - task_manager.broadcast_maintenance_event({ + await task_manager.broadcast_maintenance_event({ "type": "maintenance.events_ended_all", "environment_id": effective_env_id or "", }) diff --git a/backend/src/app.py b/backend/src/app.py index bad0310c1..d5b66709b 100755 --- a/backend/src/app.py +++ b/backend/src/app.py @@ -171,11 +171,15 @@ async def lifespan(app: FastAPI): ) from src.core.database import SessionLocal as _AuthDb from src.dependencies import get_plugin_loader + from src.core.auth.repository import AuthRepository _auth_db = _AuthDb() _plugin_loader = get_plugin_loader() _declared = discover_declared_permissions(plugin_loader=_plugin_loader) _inserted = sync_permission_catalog(db=_auth_db, declared_permissions=_declared) + # Self-heal: guarantee the Admin role carries is_admin=True so role-based admin + # bypass works even on DBs created before the flag existed (see Fix #3). + AuthRepository(_auth_db).ensure_admin_role() _auth_db.close() if _inserted > 0: logger.reason( diff --git a/backend/src/core/auth/__tests__/test_auth.py b/backend/src/core/auth/__tests__/test_auth.py index 97a2da9c6..0acc90c56 100644 --- a/backend/src/core/auth/__tests__/test_auth.py +++ b/backend/src/core/auth/__tests__/test_auth.py @@ -124,6 +124,42 @@ def test_role_permission_association(auth_repo): assert "admin:users:READ" in permissions assert "admin:users:WRITE" in permissions # #endregion Test.Tests.TestRolePermissionAssociation +# #region Test.Tests.TestEnsureAdminRole [TYPE Function] +# @BRIEF ensure_admin_role creates/backfills the Admin role with is_admin=True. +# @RELATION BINDS_TO -> [Core.Repository.AuthRepository] +def test_ensure_admin_role_creates_missing(auth_repo): + """Absent Admin role is created with is_admin=True.""" + assert auth_repo.get_role_by_name("Admin") is None + role = auth_repo.ensure_admin_role() + assert role.name == "Admin" + assert role.is_admin is True + # persisted & idempotent + assert auth_repo.get_role_by_name("Admin") is not None + + +def test_ensure_admin_role_backfills_flag(auth_repo): + """Existing Admin role with is_admin=False is backfilled to True.""" + legacy = Role(name="Admin", description="System administrator", is_admin=False) + auth_repo.db.add(legacy) + auth_repo.db.commit() + + role = auth_repo.ensure_admin_role() + + assert role is legacy + assert role.is_admin is True + + +def test_ensure_admin_role_keeps_existing(auth_repo): + """Existing Admin role with is_admin=True is left untouched (idempotent).""" + role = auth_repo.ensure_admin_role() # create with True + again = auth_repo.ensure_admin_role() + assert again is role + assert again.is_admin is True + count = ( + auth_repo.db.query(Role).filter(Role.name == "Admin").count() + ) + assert count == 1 +# #endregion Test.Tests.TestEnsureAdminRole # #region Test.Tests.TestUserRoleAssociation [TYPE Function] # @BRIEF Confirms user-role assignment persists and is queryable from repository reads. # @RELATION BINDS_TO -> [Test.Tests.TestAuth] diff --git a/backend/src/core/auth/repository.py b/backend/src/core/auth/repository.py index 0861ab91a..852b7b143 100644 --- a/backend/src/core/auth/repository.py +++ b/backend/src/core/auth/repository.py @@ -86,6 +86,40 @@ class AuthRepository: with belief_scope("AuthRepository.get_role_by_name"): return self.db.query(Role).filter(Role.name == name).first() # #endregion Core.Repository.GetRoleByName + # #region Core.Repository.EnsureAdminRole [C:3] [TYPE Function] [SEMANTICS auth, role, admin, rbac] + # @ingroup Auth + # @BRIEF Returns the administrative "Admin" role, creating it or backfilling its + # is_admin=True flag as needed. Idempotent — never downgrades. + # @PRE db is a bound session. + # @POST Returns the Admin role with is_admin=True; persists a commit when a change is made. + # @SIDE_EFFECT May insert the Admin role or set roles.is_admin=True and commit. + # @RELATION DEPENDS_ON -> [Models.Auth.Role] + # @RATIONALE Single source of truth for "the Admin role is administrative". The + # authorization bypasses check getattr(role, "is_admin", False); a pre-existing + # Admin role created before the flag existed would otherwise stay non-admin and + # silently deny the admin user. All callers (startup self-heal, create_admin, the + # admin toggle endpoint) reuse this instead of duplicating the backfill. + def ensure_admin_role(self, name: str = "Admin") -> Role: + with belief_scope("AuthRepository.ensure_admin_role"): + role = self.get_role_by_name(name) + if role is None: + role = Role(name=name, description="System Administrator", is_admin=True) + self.db.add(role) + self.db.commit() + self.db.refresh(role) + logger.reason( + "Created Admin role with administrative flag", + payload={"name": name, "is_admin": True}, + ) + elif not role.is_admin: + role.is_admin = True + self.db.commit() + logger.reason( + "Backfilled is_admin=True on existing Admin role", + payload={"name": name, "is_admin": True}, + ) + return role + # #endregion Core.Repository.EnsureAdminRole # #region Core.Repository.GetPermissionById [TYPE Function] # @ingroup Auth # @PURPOSE: Retrieve permission by UUID. diff --git a/backend/src/core/database.py b/backend/src/core/database.py index 018d96c9d..95e588d19 100644 --- a/backend/src/core/database.py +++ b/backend/src/core/database.py @@ -320,6 +320,9 @@ def _ensure_auth_users_columns(bind_engine): # @PRE Database connection is active. # @POST roles.is_admin column exists (BOOLEAN, default FALSE). # @SIDE_EFFECT Executes ALTER TABLE on the auth database. +# @RATIONALE The is_admin=True backfill for the Admin role is owned by +# AuthRepository.ensure_admin_role() (called at startup self-heal). This migration only +# guarantees the column exists — it no longer duplicates the flag backfill. def _ensure_roles_is_admin_column(bind_engine): with belief_scope("_ensure_roles_is_admin_column"): table_name = "roles" @@ -337,8 +340,7 @@ def _ensure_roles_is_admin_column(bind_engine): try: with bind_engine.begin() as connection: connection.execute(text(alter)) - connection.execute(text("UPDATE roles SET is_admin = true WHERE name = 'Admin'")) - logger.reason("Added roles.is_admin column, updated Admin roles", extra={"statement": alter}) + logger.reason("Added roles.is_admin column", extra={"statement": alter}) except Exception as migration_error: logger.explore( "roles.is_admin additive migration failed", diff --git a/backend/src/core/superset_client/_databases.py b/backend/src/core/superset_client/_databases.py index 718123bd5..0d391de09 100644 --- a/backend/src/core/superset_client/_databases.py +++ b/backend/src/core/superset_client/_databases.py @@ -73,7 +73,7 @@ class SupersetDatabasesMixin: # @RELATION CALLS -> [Core.Databases.SupersetClientGetDatabases] async def get_database_by_uuid(self, db_uuid: str) -> dict | None: with belief_scope("SupersetClient.get_database_by_uuid", f"uuid={db_uuid}"): - query = {"filters": [{"col": "uuid", "op": "eq", "value": db_uuid}]} + query = {"filters": [{"col": "uuid", "opr": "eq", "value": db_uuid}]} _, databases = await self.get_databases(query=query) return databases[0] if databases else None # #endregion Core.Databases.SupersetClientGetDatabaseByUuid diff --git a/backend/src/core/utils/async_network.py b/backend/src/core/utils/async_network.py index f56d45150..40079bb86 100644 --- a/backend/src/core/utils/async_network.py +++ b/backend/src/core/utils/async_network.py @@ -35,6 +35,12 @@ from .network import ( ) +# Safety cap for list-endpoint pagination. Superset refuses pagination beyond ~10000 +# page tokens; a runaway loop also hammers the environment. Cap well below that and +# fail with a clear error so callers (e.g. maintenance discovery) can report it. +MAX_PAGINATION_PAGES = 500 + + # #region Core.AsyncNetwork.AsyncAPIClient [C:4] [TYPE Class] # @defgroup Core Module group. # @BRIEF Async Superset API client backed by httpx.AsyncClient with shared auth cache and optional semaphore. @@ -437,6 +443,12 @@ class AsyncAPIClient: # Fetch remaining pages total_pages = max(0, (total_count + page_size - 1) // page_size) + if total_pages > MAX_PAGINATION_PAGES: + raise SupersetAPIError( + f"Pagination for {endpoint} would require {total_pages} pages " + f"(count={total_count}, page_size={page_size}) — exceeds the safety cap of " + f"{MAX_PAGINATION_PAGES}. The environment has too many records to scan." + ) for page in range(1, total_pages): query = {**base_query, "page": page} page_response = await self.request( diff --git a/backend/src/plugins/translate/service.py b/backend/src/plugins/translate/service.py index e150399de..0e8d05394 100644 --- a/backend/src/plugins/translate/service.py +++ b/backend/src/plugins/translate/service.py @@ -277,7 +277,23 @@ class TranslateJobService: if not env: raise ValueError(f"Environment '{env_id}' not found") client = await get_superset_client(env) - _, datasets = await client.get_datasets() + # Filter server-side by table_name so the result stays small even in huge + # environments — a full dataset scan exceeds Superset's pagination cap and + # would break translation job creation. + query = None + if search: + query = {"filters": [{"col": "table_name", "opr": "ct", "value": search}]} + try: + _, datasets = await client.get_datasets(query=query) + except Exception as e: + if query is None: + raise + # Older Superset may reject list filters — degrade to the unfiltered scan. + logger.warning( + f"[TranslateJobService] Server-side dataset filter rejected, " + f"falling back to full scan: {e}" + ) + _, datasets = await client.get_datasets() result = [] for ds in datasets: name = ds.get("table_name", "") diff --git a/backend/src/scripts/create_admin.py b/backend/src/scripts/create_admin.py index f0d038aa3..ecb46d1ff 100644 --- a/backend/src/scripts/create_admin.py +++ b/backend/src/scripts/create_admin.py @@ -19,10 +19,11 @@ import sys sys.path.append(str(Path(__file__).parent.parent.parent)) from src.core.auth.security import get_password_hash +from src.core.auth.repository import AuthRepository from ss_tools.shared.cot_logger import seed_trace_id from src.core.database import AuthSessionLocal, init_db from src.core.logger import belief_scope, logger -from src.models.auth import Role, User +from src.models.auth import User # #region Tooling.CreateAdmin [TYPE Function] @@ -40,27 +41,23 @@ def create_admin(username, password, email=None): email.strip() if isinstance(email, str) and email.strip() else None ) - # 1. Ensure Admin role exists - admin_role = db.query(Role).filter(Role.name == "Admin").first() - if not admin_role: - logger.reason("Creating Admin role") - admin_role = Role( - name="Admin", - description="System Administrator", - is_admin=True, - ) - db.add(admin_role) - db.commit() - db.refresh(admin_role) - elif not admin_role.is_admin: - logger.reason("Marking existing Admin role as administrative") - admin_role.is_admin = True - db.commit() + # 1. Ensure the Admin role exists with is_admin=True (shared helper). + repo = AuthRepository(db) + admin_role = repo.ensure_admin_role() # 2. Check if user already exists existing_user = db.query(User).filter(User.username == username).first() if existing_user: - logger.reflect("User already exists", payload={"username": username}) + # Promote an existing user to admin instead of leaving them without rights. + if admin_role not in existing_user.roles: + existing_user.roles.append(admin_role) + db.commit() + logger.reflect( + "Attached Admin role to existing user", + payload={"username": username}, + ) + else: + logger.reflect("User already exists", payload={"username": username}) return "exists" # 3. Create Admin user diff --git a/backend/src/services/maintenance/_dashboard_scanner.py b/backend/src/services/maintenance/_dashboard_scanner.py index 91d97055b..791155b8b 100644 --- a/backend/src/services/maintenance/_dashboard_scanner.py +++ b/backend/src/services/maintenance/_dashboard_scanner.py @@ -22,7 +22,11 @@ from ..sql_table_extractor import extract_tables_from_sql # Applies scope/excluded/forced filtering from MaintenanceSettings. # @PRE tables is a non-empty list of "schema.table" strings. superset_client is authenticated. # @POST Returns a deduplicated list of dashboard IDs that should receive banners. -# @SIDE_EFFECT Fetches all datasets from Superset (paginated). May be slow with many datasets. +# @SIDE_EFFECT Fetches datasets from Superset filtered server-side by target table names +# (stays small even in huge environments), plus a best-effort scan of virtual SQL datasets. +# @RATIONALE A full dataset scan hits Superset's pagination token cap ("Maximum number of +# tokens exceeded (10000)") in environments with hundreds of thousands of datasets, which +# silently failed maintenance discovery. Filtering by table_name keeps the result small. # @RELATION DEPENDS_ON -> [Core.Init.SupersetClient] # @RELATION DEPENDS_ON -> [Services.SqlTableExtractor.SqlTableExtractorModule] async def find_affected_dashboards( @@ -48,16 +52,56 @@ async def find_affected_dashboards( if not target_tables: return [] - # Fetch all datasets from Superset + # Fetch datasets from Superset, filtered server-side by target table names so the + # result stays small even in environments with hundreds of thousands of datasets + # (a full scan would exceed Superset's pagination token cap and fail discovery). + table_names = sorted({t.rsplit(".", 1)[-1] for t in target_tables}) + query = {"filters": [{"col": "table_name", "opr": "in", "value": table_names}]} try: - _, datasets = await superset_client.get_datasets() + _, datasets = await superset_client.get_datasets(query=query) except Exception as e: + # Fall back to the unfiltered scan (old behaviour) when the filter is rejected + # (e.g. an older Superset without list filters) so discovery still works there. app_logger.explore( - "Failed to fetch datasets from Superset", + "Filtered dataset fetch failed, falling back to full scan", + extra={}, + error=str(e), + ) + try: + _, datasets = await superset_client.get_datasets() + except Exception as e2: + app_logger.explore( + "Failed to fetch datasets from Superset", + extra={}, + error=str(e2), + ) + raise + + # Best-effort: virtual (SQL) datasets have a non-matching table_name, so they are + # fetched separately. This is optional — if the filter is unsupported or the set is + # too large (pagination cap), skip virtual matching and keep physical matches only. + try: + virtual_query = { + "filters": [{"col": "is_sqllab_view", "opr": "eq", "value": True}] + } + _, virtual_datasets = await superset_client.get_datasets(query=virtual_query) + # A virtual dataset may also appear in the physical result — dedupe by id. + seen_ids: set = set() + deduped: list[dict] = [] + for ds in datasets + virtual_datasets: + ds_id = ds.get("id") + if ds_id is not None: + if ds_id in seen_ids: + continue + seen_ids.add(ds_id) + deduped.append(ds) + datasets = deduped + except Exception as e: + app_logger.explore( + "Virtual dataset scan skipped, continuing with physical matches", extra={}, error=str(e), ) - raise app_logger.reason( f"Scanning {len(datasets)} datasets for table references", @@ -147,9 +191,12 @@ async def _apply_dashboard_filters( excluded_ids = set(settings.excluded_dashboard_ids or []) # Apply scope filtering - if settings.dashboard_scope != DashboardScope.ALL: + if settings.dashboard_scope != DashboardScope.ALL and dashboard_ids: try: - _, all_dashboards = await superset_client.get_dashboards() + # Fetch only the matched dashboards (by id) instead of scanning the whole + # dashboard list — the scope filter only needs published/draft state for these. + query = {"filters": [{"col": "id", "opr": "in", "value": dashboard_ids}]} + _, all_dashboards = await superset_client.get_dashboards(query=query) scope = settings.dashboard_scope filtered: list[int] = [] for did in dashboard_ids: @@ -193,7 +240,9 @@ async def _resolve_dashboard_title( superset_client: SupersetClient, ) -> str: try: - _, dashboards = await superset_client.get_dashboards() + # Fetch only the target dashboard by id instead of scanning the whole catalog. + query = {"filters": [{"col": "id", "opr": "eq", "value": dashboard_id}]} + _, dashboards = await superset_client.get_dashboards(query=query) for d in dashboards: if d.get("id") == dashboard_id: return d.get("dashboard_title") or d.get("title") or str(dashboard_id) diff --git a/backend/tests/api/test_admin.py b/backend/tests/api/test_admin.py index 8b97264ad..ebf15d76c 100644 --- a/backend/tests/api/test_admin.py +++ b/backend/tests/api/test_admin.py @@ -434,6 +434,50 @@ class TestUpdateRole: assert existing_role.name == "NewName" mock_session.commit.assert_called_once() + def test_update_role_remove_admin_last_role_blocked(self): + """Removing is_admin from the last admin role returns 409 (lockout guard).""" + mock_session = MagicMock() + mock_repo = MagicMock() + existing_role = MagicMock() + existing_role.id = "role-1" + existing_role.name = "Admin" + existing_role.description = "System Administrator" + existing_role.is_admin = True + existing_role.permissions = [] + mock_repo.get_role_by_id.return_value = existing_role + # Only one role in the system carries is_admin=True + mock_session.query.return_value.filter.return_value.count.return_value = 1 + + with patch("src.api.routes.admin.AuthRepository", return_value=mock_repo): + from src.core.database import get_auth_db + client = _make_client({get_auth_db: lambda: mock_session}) + resp = client.put("/api/admin/roles/role-1", json={"is_admin": False}) + assert resp.status_code == 409 + assert existing_role.is_admin is True # unchanged + mock_session.commit.assert_not_called() + + def test_update_role_remove_admin_allowed_with_other(self): + """Removing is_admin is allowed when another admin role remains.""" + mock_session = MagicMock() + mock_repo = MagicMock() + existing_role = MagicMock() + existing_role.id = "role-1" + existing_role.name = "Admin" + existing_role.description = "System Administrator" + existing_role.is_admin = True + existing_role.permissions = [] + mock_repo.get_role_by_id.return_value = existing_role + # Another role still carries is_admin=True + mock_session.query.return_value.filter.return_value.count.return_value = 2 + + with patch("src.api.routes.admin.AuthRepository", return_value=mock_repo): + from src.core.database import get_auth_db + client = _make_client({get_auth_db: lambda: mock_session}) + resp = client.put("/api/admin/roles/role-1", json={"is_admin": False}) + assert resp.status_code == 200 + assert existing_role.is_admin is False + mock_session.commit.assert_called_once() + def test_update_role_not_found(self): """Non-existent role returns 404.""" mock_session = MagicMock() diff --git a/backend/tests/api/test_maintenance_routes_comprehensive.py b/backend/tests/api/test_maintenance_routes_comprehensive.py index 1d82b638a..fb2e68923 100644 --- a/backend/tests/api/test_maintenance_routes_comprehensive.py +++ b/backend/tests/api/test_maintenance_routes_comprehensive.py @@ -172,6 +172,100 @@ class TestListEvents: class TestStartMaintenance: """start_maintenance — validation and creation.""" + @pytest.mark.asyncio + async def test_start_maintenance_broadcast_is_awaited(self): + """Regression: broadcast_maintenance_event coroutine must be awaited (prod RuntimeWarning).""" + from src.api.routes.maintenance._routes import start_maintenance + + mock_request = MagicMock() + mock_request.start_time = None + mock_request.end_time = None + mock_request.tables = ["table1", "Table2"] + mock_request.message = "Test" + mock_request.environment_id = "env-dev" + + mock_db = MagicMock() + mock_db.query.return_value.filter.return_value.all.return_value = [] # no existing events + # db.flush() must assign the event id like the real session + created_events = [] + mock_db.add.side_effect = created_events.append + mock_db.flush.side_effect = lambda: setattr(created_events[0], "id", "event-1") + + mock_task = MagicMock() + mock_task.id = "task-1" + mock_tm = AsyncMock() + mock_tm.create_task.return_value = mock_task + + with patch("src.api.routes.maintenance._routes.belief_scope"): + result = await start_maintenance( + request=mock_request, + db=mock_db, + _auth="jwt:admin", + task_manager=mock_tm, + ) + + assert result.status == "pending" + # The broadcast must have been awaited — otherwise the event never reaches clients. + mock_tm.broadcast_maintenance_event.assert_awaited_once() + + @pytest.mark.asyncio + async def test_end_maintenance_broadcast_is_awaited(self): + """Regression: end_maintenance must await broadcast_maintenance_event.""" + from src.api.routes.maintenance._routes import end_maintenance + from src.models.maintenance import MaintenanceEventStatus + + mock_event = MagicMock() + mock_event.id = "event-1" + mock_event.environment_id = "env-dev" + mock_event.status = MaintenanceEventStatus.PENDING + mock_event.task_id = "task-1" + + mock_db = MagicMock() + mock_db.query.return_value.filter.return_value.first.return_value = mock_event + + mock_task = MagicMock() + mock_task.id = "task-2" + mock_tm = AsyncMock() + mock_tm.create_task.return_value = mock_task + + mock_request = MagicMock() + + with patch("src.api.routes.maintenance._routes.belief_scope"): + result = await end_maintenance( + maintenance_id="event-1", + request=mock_request, + db=mock_db, + _auth="jwt:admin", + task_manager=mock_tm, + ) + + assert result.status == "pending" + mock_tm.broadcast_maintenance_event.assert_awaited_once() + + @pytest.mark.asyncio + async def test_end_all_maintenance_broadcast_is_awaited(self): + """Regression: end_all_maintenance must await broadcast_maintenance_event.""" + from src.api.routes.maintenance._routes import end_all_maintenance + + mock_request = MagicMock() + mock_request.json.side_effect = ValueError("no body") # no env in body + + mock_tm = AsyncMock() + mock_tm.create_task.return_value = MagicMock(id="task-3") + + mock_db = MagicMock() + + with patch("src.api.routes.maintenance._routes.belief_scope"): + result = await end_all_maintenance( + request=mock_request, + db=mock_db, + _auth="jwt:admin", + task_manager=mock_tm, + ) + + assert result.status == "pending" + mock_tm.broadcast_maintenance_event.assert_awaited_once() + def test_end_before_start_raises_400(self): """end_time <= start_time raises 400.""" from src.api.routes.maintenance._routes import start_maintenance diff --git a/backend/tests/api/test_maintenance_routes_edge.py b/backend/tests/api/test_maintenance_routes_edge.py index eab8e55f6..1c6930580 100644 --- a/backend/tests/api/test_maintenance_routes_edge.py +++ b/backend/tests/api/test_maintenance_routes_edge.py @@ -99,6 +99,7 @@ def mock_task_manager(): mock_task = MagicMock() mock_task.id = "test-task-id-edge" mock_tm.create_task = AsyncMock(return_value=mock_task) + mock_tm.broadcast_maintenance_event = AsyncMock() from src.app import app app.dependency_overrides[get_task_manager] = lambda: mock_tm diff --git a/backend/tests/core/superset_client/test_client_databases.py b/backend/tests/core/superset_client/test_client_databases.py index 4e7249f3a..00a640318 100644 --- a/backend/tests/core/superset_client/test_client_databases.py +++ b/backend/tests/core/superset_client/test_client_databases.py @@ -234,7 +234,7 @@ class TestGetDatabaseByUuid: call_kwargs = client.client.fetch_paginated_data.call_args base_query = call_kwargs.kwargs["pagination_options"]["base_query"] assert base_query["filters"] == [ - {"col": "uuid", "op": "eq", "value": "some-uuid-123"} + {"col": "uuid", "opr": "eq", "value": "some-uuid-123"} ] diff --git a/backend/tests/core/test_core_database.py b/backend/tests/core/test_core_database.py index d278a1df0..90a652fc4 100644 --- a/backend/tests/core/test_core_database.py +++ b/backend/tests/core/test_core_database.py @@ -275,7 +275,7 @@ def test_roles(mock_inspect, _): mock_inspect.return_value = _insp(["roles"], []) eng, conn = _eng() _ensure_roles_is_admin_column(eng) - assert conn.execute.call_count == 2 # ALTER + UPDATE + assert conn.execute.call_count == 1 # ALTER only (flag backfill lives in ensure_admin_role) # #endregion Test.Core.TestRoles # #region Test.Core.TestRolesAlreadyExists [C:2] [TYPE Function] diff --git a/backend/tests/plugins/translate/test_service.py b/backend/tests/plugins/translate/test_service.py index f9248868d..f8e7e1aeb 100644 --- a/backend/tests/plugins/translate/test_service.py +++ b/backend/tests/plugins/translate/test_service.py @@ -438,6 +438,38 @@ class TestFetchAvailableDatasources: ]) # Patch at the source module since service does lazy import + with patch('src.core.utils.client_registry.get_superset_client', + new=AsyncMock(return_value=mock_client)): + result = await svc.fetch_available_datasources("env-1", search="order") + assert len(result) == 1 + assert result[0]["table_name"] == "orders" + # Server-side filter is applied so a huge environment stays a small query. + call_kwargs = mock_client.get_datasets.call_args.kwargs + assert call_kwargs["query"]["filters"] == [ + {"col": "table_name", "opr": "ct", "value": "order"} + ] + + @pytest.mark.asyncio + async def test_fetch_search_filter_rejected_falls_back(self, db_session): + """If the server rejects the filter, the datasource picker degrades to a full scan.""" + config = MagicMock() + config.get_environment.return_value = MagicMock(id="env-1") + + svc = TranslateJobService(db_session, config) + mock_client = AsyncMock() + datasets = [ + {"id": 1, "table_name": "orders", "schema": "public", + "database": {"id": 10, "database_name": "Main DB", "backend": "postgresql"}, + "description": "Order data"}, + ] + + async def side_effect(**kwargs): + if kwargs.get("query"): + raise Exception("unsupported filter") + return (None, datasets) + + mock_client.get_datasets.side_effect = side_effect + with patch('src.core.utils.client_registry.get_superset_client', new=AsyncMock(return_value=mock_client)): result = await svc.fetch_available_datasources("env-1", search="order") diff --git a/backend/tests/scripts/test_create_admin.py b/backend/tests/scripts/test_create_admin.py index 40dfc1242..681334272 100644 --- a/backend/tests/scripts/test_create_admin.py +++ b/backend/tests/scripts/test_create_admin.py @@ -51,18 +51,20 @@ class TestCreateAdmin: @patch("src.scripts.create_admin.AuthSessionLocal") @patch("src.scripts.create_admin.get_password_hash", return_value="hashed_pwd") def test_user_already_exists(self, mock_hash, MockSession): - """Edge: existing user returns 'exists'.""" + """Edge: existing user returns 'exists' AND is promoted to the Admin role.""" from src.scripts.create_admin import create_admin mock_db = MagicMock() MockSession.return_value = mock_db - # Existing Admin role + # Existing Admin role with admin flag existing_role = MagicMock() existing_role.name = "Admin" - # Existing user + existing_role.is_admin = True + # Existing user WITHOUT admin role yet (real list so append is observable) existing_user = MagicMock() existing_user.username = "admin" + existing_user.roles = [] def query_side_effect(model): mock_query = MagicMock() @@ -77,6 +79,40 @@ class TestCreateAdmin: result = create_admin("admin", "secret123") assert result == "exists" + # The pre-existing user must be promoted to admin, not silently skipped. + assert existing_role in existing_user.roles + mock_db.close.assert_called_once() + + @patch("src.scripts.create_admin.AuthSessionLocal") + @patch("src.scripts.create_admin.get_password_hash", return_value="hashed_pwd") + def test_user_already_admin_no_duplicate(self, mock_hash, MockSession): + """Edge: existing user who is already admin stays admin (no duplicate role).""" + from src.scripts.create_admin import create_admin + + mock_db = MagicMock() + MockSession.return_value = mock_db + + existing_role = MagicMock() + existing_role.name = "Admin" + existing_role.is_admin = True + existing_user = MagicMock() + existing_user.username = "admin" + existing_user.roles = [existing_role] + + def query_side_effect(model): + mock_query = MagicMock() + if model.__name__ == "Role": + mock_query.filter.return_value.first.return_value = existing_role + elif model.__name__ == "User": + mock_query.filter.return_value.first.return_value = existing_user + return mock_query + + mock_db.query.side_effect = query_side_effect + + result = create_admin("admin", "secret123") + + assert result == "exists" + assert existing_user.roles.count(existing_role) == 1 mock_db.close.assert_called_once() @patch("src.scripts.create_admin.AuthSessionLocal") diff --git a/backend/tests/services/maintenance/test_dashboard_scanner.py b/backend/tests/services/maintenance/test_dashboard_scanner.py index 9aa47e483..7c91d51fa 100644 --- a/backend/tests/services/maintenance/test_dashboard_scanner.py +++ b/backend/tests/services/maintenance/test_dashboard_scanner.py @@ -42,6 +42,44 @@ class TestFindAffectedDashboards: with pytest.raises(Exception, match="API error"): await find_affected_dashboards(["public.table1"], mock_superset) + @pytest.mark.asyncio + async def test_get_datasets_uses_table_name_filter(self, mock_superset): + """Physical scan is filtered server-side by target table names (no full scan).""" + from src.services.maintenance._dashboard_scanner import find_affected_dashboards + mock_superset.get_datasets.return_value = (0, []) + result = await find_affected_dashboards(["public.table1", "analytics.orders"], mock_superset) + assert result == [] + + first_call = mock_superset.get_datasets.call_args_list[0] + query = first_call.kwargs.get("query") or first_call.args[0] + filters = query["filters"] + assert filters[0]["col"] == "table_name" + assert filters[0]["opr"] == "in" + assert set(filters[0]["value"]) == {"table1", "orders"} + + @pytest.mark.asyncio + async def test_virtual_scan_failure_continues(self, mock_superset): + """If the virtual-dataset scan fails, physical matches are still returned.""" + from src.services.maintenance._dashboard_scanner import find_affected_dashboards + physical = [ + {"id": 1, "schema": "public", "table_name": "table1", "sql": None, "is_sqllab_view": False} + ] + + async def side_effect(**kwargs): + q = kwargs.get("query") or {} + f = (q.get("filters") or [{}])[0] + if f.get("col") == "is_sqllab_view": + raise Exception("unsupported filter") + return (1, physical) + + mock_superset.get_datasets.side_effect = side_effect + mock_superset.get_dataset_detail.return_value = { + "linked_dashboards": [{"id": 30, "title": "Dashboard 30"}] + } + + result = await find_affected_dashboards(["public.table1"], mock_superset) + assert 30 in result + @pytest.mark.asyncio async def test_table_based_match(self, mock_superset): from src.services.maintenance._dashboard_scanner import find_affected_dashboards @@ -163,6 +201,64 @@ class TestApplyDashboardFilters: result = await _apply_dashboard_filters([1, 2], mock_superset, settings) assert result == [1] + @pytest.mark.asyncio + async def test_scope_fetch_filters_by_dashboard_ids(self, mock_superset): + """Scope filtering fetches only matched dashboards by id (no full scan).""" + from src.services.maintenance._dashboard_scanner import _apply_dashboard_filters + mock_superset.get_dashboards.return_value = (1, [ + {"id": 1, "published": True}, + ]) + settings = MaintenanceSettings( + id="default", dashboard_scope=DashboardScope.PUBLISHED_ONLY, + ) + result = await _apply_dashboard_filters([1, 2], mock_superset, settings) + assert result == [1] + + call_kwargs = mock_superset.get_dashboards.call_args.kwargs + assert call_kwargs["query"]["filters"] == [ + {"col": "id", "opr": "in", "value": [1, 2]} + ] + + @pytest.mark.asyncio + async def test_filter_rejected_falls_back_to_full_scan(self, mock_superset): + """If the server rejects the filtered query, discovery falls back to a full scan.""" + from src.services.maintenance._dashboard_scanner import find_affected_dashboards + physical = [ + {"id": 1, "schema": "public", "table_name": "table1", "sql": None, "is_sqllab_view": False} + ] + + async def side_effect(**kwargs): + q = kwargs.get("query") or {} + if q.get("filters"): + raise Exception("unsupported filter") + return (1, physical) + + mock_superset.get_datasets.side_effect = side_effect + mock_superset.get_dataset_detail.return_value = { + "linked_dashboards": [{"id": 40, "title": "Dashboard 40"}] + } + + result = await find_affected_dashboards(["public.table1"], mock_superset) + assert 40 in result + + @pytest.mark.asyncio + async def test_virtual_overlap_deduped(self, mock_superset): + """A virtual dataset also present in the physical result is scanned only once.""" + from src.services.maintenance._dashboard_scanner import find_affected_dashboards + ds = { + "id": 7, "schema": "", "table_name": "", + "sql": "SELECT * FROM public.table1", "is_sqllab_view": True, + } + mock_superset.get_datasets.return_value = (1, [ds]) # returned for BOTH calls + mock_superset.get_dataset_detail.return_value = { + "linked_dashboards": [{"id": 50, "title": "Dashboard 50"}] + } + + result = await find_affected_dashboards(["public.table1"], mock_superset) + assert 50 in result + # physical + virtual both contain ds → dedupe must keep exactly one scan + assert mock_superset.get_dataset_detail.await_count == 1 + @pytest.mark.asyncio async def test_draft_only_filter(self, mock_superset): from src.services.maintenance._dashboard_scanner import _apply_dashboard_filters diff --git a/backend/tests/test_core/test_async_network.py b/backend/tests/test_core/test_async_network.py index de75c66ef..9ad9bf297 100644 --- a/backend/tests/test_core/test_async_network.py +++ b/backend/tests/test_core/test_async_network.py @@ -794,6 +794,49 @@ class TestFetchPaginatedData: # First page has 1, second page is skipped, third page has 1 assert len(result) == 2 + @pytest.mark.asyncio + async def test_page_count_over_cap_raises_clear_error(self): + """Huge result sets must fail fast with a readable error instead of a runaway loop.""" + from src.core.utils.async_network import ( + AsyncAPIClient, + MAX_PAGINATION_PAGES, + ) + from src.core.utils.network import SupersetAPIError + + client = AsyncAPIClient({"base_url": "https://test.com", "auth": {}}) + # count=60000 with page_size=100 => 600 pages > cap + client.request = AsyncMock(return_value={"result": [{"id": 1}], "count": 60000}) + + with pytest.raises(SupersetAPIError, match="safety cap"): + await client.fetch_paginated_data( + "/dataset/", + {"base_query": {"page_size": 100}, "results_field": "result"}, + ) + # Only the first page was fetched — no runaway loop + assert client.request.call_count == 1 + + @pytest.mark.asyncio + async def test_page_count_at_cap_is_allowed(self): + """A result set exactly at the cap boundary still paginates normally.""" + from src.core.utils.async_network import ( + AsyncAPIClient, + MAX_PAGINATION_PAGES, + ) + + client = AsyncAPIClient({"base_url": "https://test.com", "auth": {}}) + client.request = AsyncMock( + side_effect=[ + {"result": [{"id": 1}], "count": MAX_PAGINATION_PAGES}, # 500 pages of 1 + *({"result": [{"id": i}]} for i in range(2, MAX_PAGINATION_PAGES + 1)), + ] + ) + + result = await client.fetch_paginated_data( + "/chart/", + {"base_query": {"page_size": 1}, "results_field": "result"}, + ) + assert len(result) == MAX_PAGINATION_PAGES + class TestUploadFile: """upload_file: multipart file upload.""" diff --git a/frontend/src/lib/i18n/locales/en/admin.json b/frontend/src/lib/i18n/locales/en/admin.json index 971e63488..02d4957c0 100644 --- a/frontend/src/lib/i18n/locales/en/admin.json +++ b/frontend/src/lib/i18n/locales/en/admin.json @@ -25,6 +25,10 @@ "create": "Create Role", "name": "Role Name", "description": "Description", + "admin": "Admin", + "is_admin": "Admin role", + "is_admin_badge": "ADMIN", + "is_admin_hint": "Users with this role bypass permission checks and gain full administrative access.", "permissions": "Permissions", "loading": "Loading roles...", "no_roles": "No roles found.", diff --git a/frontend/src/lib/i18n/locales/ru/admin.json b/frontend/src/lib/i18n/locales/ru/admin.json index 8aeca9848..a32128fd1 100644 --- a/frontend/src/lib/i18n/locales/ru/admin.json +++ b/frontend/src/lib/i18n/locales/ru/admin.json @@ -25,6 +25,10 @@ "create": "Создать роль", "name": "Имя роли", "description": "Описание", + "admin": "Админ", + "is_admin": "Административная роль", + "is_admin_badge": "АДМИН", + "is_admin_hint": "Пользователи с этой ролью обходят проверки прав и получают полный административный доступ.", "permissions": "Права доступа", "loading": "Загрузка ролей...", "no_roles": "Роли не найдены.", diff --git a/frontend/src/routes/admin/__tests__/admin-roles.test.ts b/frontend/src/routes/admin/__tests__/admin-roles.test.ts index a636090f4..8b960f560 100644 --- a/frontend/src/routes/admin/__tests__/admin-roles.test.ts +++ b/frontend/src/routes/admin/__tests__/admin-roles.test.ts @@ -6,7 +6,7 @@ // @TEST_EDGE: error state -> error message displayed on API failure import { describe, it, expect, vi, beforeEach } from 'vitest'; -import { render, screen, waitFor } from '@testing-library/svelte'; +import { render, screen, waitFor, fireEvent } from '@testing-library/svelte'; import AdminRolesPage from '../roles/+page.svelte'; // ── i18n mock ───────────────────────────────────────────────────── @@ -27,6 +27,10 @@ const mockTranslations = { name: 'Role Name', description: 'Description', permissions: 'Permissions', + admin: 'Is Admin', + is_admin: 'Admin role', + is_admin_badge: 'ADMIN', + is_admin_hint: 'Users with this role bypass permission checks.', loading: 'Loading roles...', no_roles: 'No roles found.', modal_create_title: 'Create New Role', @@ -102,12 +106,13 @@ vi.mock('$lib/auth/permissions.js', () => ({ // ── Admin service mock (hoisted) ────────────────────────────────── const mockGetRoles = vi.hoisted(() => vi.fn()); const mockGetPermissions = vi.hoisted(() => vi.fn()); +const mockUpdateRole = vi.hoisted(() => vi.fn()); vi.mock('../../../services/adminService', () => ({ adminService: { getRoles: mockGetRoles, getPermissions: mockGetPermissions, createRole: vi.fn(), - updateRole: vi.fn(), + updateRole: mockUpdateRole, deleteRole: vi.fn(), } })); @@ -120,12 +125,14 @@ describe('Admin Roles Page', () => { id: 'r1', name: 'Admin', description: 'Full system access', + is_admin: true, permissions: [{ id: 'p1', resource: 'all', action: '*' }], }, { id: 'r2', name: 'Viewer', description: 'Read-only access', + is_admin: false, permissions: [{ id: 'p2', resource: 'dashboard', action: 'read' }], }, ]; @@ -176,6 +183,49 @@ describe('Admin Roles Page', () => { expect(screen.queryByText('Loading roles...')).toBeNull(); }); // #endregion AdminRolesPageTest.Describe.TestLoadingStateTransitions + + // #region AdminRolesPageTest.Describe.TestAdminCheckboxReflectsRole [C:2] [TYPE Test] + // @BRIEF The role edit modal shows the admin checkbox reflecting role.is_admin. + it('reflects is_admin in the role edit modal', async () => { + const { container } = render(AdminRolesPage); + + await waitFor(() => { + expect(screen.getByText('Viewer')).toBeTruthy(); + }); + + // First Edit button = Admin role row (is_admin: true) + const editButtons = screen.getAllByText('Edit').map((el) => el.closest('button')); + fireEvent.click(editButtons[0]!); + let boxes = screen.getAllByRole('checkbox'); + expect((boxes[0] as HTMLInputElement).checked).toBe(true); + + // Close modal, open Viewer row (is_admin: false) + fireEvent.click(screen.getByText('Cancel').closest('button')!); + fireEvent.click(editButtons[1]!); + boxes = screen.getAllByRole('checkbox'); + expect((boxes[0] as HTMLInputElement).checked).toBe(false); + }); + // #endregion AdminRolesPageTest.Describe.TestAdminCheckboxReflectsRole + + // #region AdminRolesPageTest.Describe.TestSaveSendsIsAdmin [C:2] [TYPE Test] + // @BRIEF Saving a role with the admin checkbox set sends is_admin: true to updateRole. + it('sends is_admin when saving a role', async () => { + mockUpdateRole.mockResolvedValue({}); + render(AdminRolesPage); + await waitFor(() => { + expect(screen.getByText('Viewer')).toBeTruthy(); + }); + + fireEvent.click(screen.getAllByText('Edit')[1].closest('button')!); // Viewer row + const boxes = screen.getAllByRole('checkbox'); + fireEvent.click(boxes[0]); // enable admin + fireEvent.click(screen.getByText('Save').closest('button')!); + + await waitFor(() => { + expect(mockUpdateRole).toHaveBeenCalledWith('r2', expect.objectContaining({ is_admin: true })); + }); + }); + // #endregion AdminRolesPageTest.Describe.TestSaveSendsIsAdmin }); // #endregion AdminRolesPageTest.Describe // #endregion Tests.AdminRoles.AdminRolesPageTest diff --git a/frontend/src/routes/admin/roles/+page.svelte b/frontend/src/routes/admin/roles/+page.svelte index 96ac755e8..0b0143dbc 100644 --- a/frontend/src/routes/admin/roles/+page.svelte +++ b/frontend/src/routes/admin/roles/+page.svelte @@ -37,7 +37,8 @@ let roleForm = $state({ name: '', description: '', - permissions: [] + permissions: [], + is_admin: false }); // #region Roles.Page.LoadDataFunction [TYPE Function] @@ -76,7 +77,7 @@ log("AdminRolesPage", "REASON", "Opening create modal"); isEditing = false; currentRoleId = null; - roleForm = { name: '', description: '', permissions: [] }; + roleForm = { name: '', description: '', permissions: [], is_admin: false }; showModal = true; } // #endregion Roles.Page.OpenCreateModalFunction @@ -95,7 +96,8 @@ roleForm = { name: role.name, description: role.description || '', - permissions: role.permissions.map(p => p.id) + permissions: role.permissions.map(p => p.id), + is_admin: !!role.is_admin }; showModal = true; } @@ -188,6 +190,7 @@ {$t.admin.roles.name} {$t.admin.roles.description} + {$t.admin.roles.admin} {$t.admin.roles.permissions} {$t.common.actions} @@ -197,6 +200,13 @@ {role.name} {role.description || '-'} + + {#if role.is_admin} + {$t.admin.roles.is_admin_badge} + {:else} + - + {/if} +
{#each role.permissions as perm} @@ -235,6 +245,13 @@
+
+ +

{$t.admin.roles.is_admin_hint}

+

{$t.admin.roles.permissions}