From 58af1a20a72fc722a174a831ec7e87de175d5fc3 Mon Sep 17 00:00:00 2001 From: Albrecht Degering Date: Tue, 21 Jul 2026 19:15:41 +0200 Subject: [PATCH] security(files): filter connectors before secret resolution --- .../storage/connector_profile_store.py | 29 ++++++- .../backend/storage/connector_visibility.py | 76 ++++++++++++++----- tests/test_connector_profile_store.py | 33 +++++++- tests/test_connector_visibility.py | 37 +++++++-- 4 files changed, 147 insertions(+), 28 deletions(-) diff --git a/src/govoplan_files/backend/storage/connector_profile_store.py b/src/govoplan_files/backend/storage/connector_profile_store.py index a9f9f9a..ca2d730 100644 --- a/src/govoplan_files/backend/storage/connector_profile_store.py +++ b/src/govoplan_files/backend/storage/connector_profile_store.py @@ -1,6 +1,6 @@ from __future__ import annotations -from collections.abc import Mapping +from collections.abc import Callable, Mapping from typing import Any from sqlalchemy.orm import Session @@ -20,7 +20,26 @@ def list_database_connector_profiles( *, tenant_id: str, include_disabled: bool = False, + row_visible: Callable[[FileConnectorProfile], bool] | None = None, ) -> list[ConnectorProfile]: + profiles, _profile_ids = select_database_connector_profiles( + session, + tenant_id=tenant_id, + include_disabled=include_disabled, + row_visible=row_visible, + ) + return profiles + + +def select_database_connector_profiles( + session: Session, + *, + tenant_id: str, + include_disabled: bool = False, + row_visible: Callable[[FileConnectorProfile], bool] | None = None, +) -> tuple[list[ConnectorProfile], set[str]]: + """Return visible profiles and every database id that shadows settings.""" + query = session.query(FileConnectorProfile).filter( (FileConnectorProfile.scope_type == "system") | (FileConnectorProfile.tenant_id == tenant_id) @@ -28,9 +47,15 @@ def list_database_connector_profiles( if not include_disabled: query = query.filter(FileConnectorProfile.enabled.is_(True)) rows = query.order_by(FileConnectorProfile.scope_type.asc(), FileConnectorProfile.label.asc()).all() + profile_ids = {row.id for row in rows} + if row_visible is not None: + rows = [row for row in rows if row_visible(row)] credential_ids = {_clean(row.credential_profile_id) for row in rows if _clean(row.credential_profile_id)} credentials = credential_rows_by_id(session, tenant_id=tenant_id, credential_ids={item for item in credential_ids if item}, include_disabled=include_disabled) - return [connector_profile_from_row(row, credential_row=credentials.get(row.credential_profile_id or "")) for row in rows] + return ( + [connector_profile_from_row(row, credential_row=credentials.get(row.credential_profile_id or "")) for row in rows], + profile_ids, + ) def list_connector_profile_rows( diff --git a/src/govoplan_files/backend/storage/connector_visibility.py b/src/govoplan_files/backend/storage/connector_visibility.py index 78a534f..d28960b 100644 --- a/src/govoplan_files/backend/storage/connector_visibility.py +++ b/src/govoplan_files/backend/storage/connector_visibility.py @@ -9,7 +9,7 @@ from govoplan_files.backend.storage.connector_deployment import ( connector_effective_endpoint_url, ) from govoplan_files.backend.storage.connector_profile_store import ( - list_database_connector_profiles, + select_database_connector_profiles, ) from govoplan_files.backend.storage.connector_profiles import ( ConnectorProfile, @@ -48,16 +48,31 @@ def visible_connector_profiles_for_actor( provider_norm = provider.strip().casefold() if provider else None actor_group_ids = {str(group_id) for group_id in group_ids if str(group_id)} - database_profiles = list_database_connector_profiles( + database_profiles, database_profile_ids = select_database_connector_profiles( session, tenant_id=tenant_id, include_disabled=include_disabled, + row_visible=lambda row: ( + include_admin_scopes + and row.scope_type in {"user", "group", "campaign"} + ) or _scope_visible_to_actor( + scope_type=row.scope_type, + scope_id=row.scope_id, + tenant_id=tenant_id, + user_id=user_id, + group_ids=actor_group_ids, + campaign_id=campaign_id, + campaign_visible=campaign_visible, + ), ) configured_profiles = connector_profiles_from_settings(settings) profiles_by_id: dict[str, ConnectorProfile] = {} - for profile in [*database_profiles, *configured_profiles]: + for profile in database_profiles: if profile.id not in profiles_by_id: profiles_by_id[profile.id] = profile + for profile in configured_profiles: + if profile.id not in database_profile_ids and profile.id not in profiles_by_id: + profiles_by_id[profile.id] = profile visible: list[ConnectorProfile] = [] for profile in profiles_by_id.values(): @@ -92,10 +107,16 @@ def visible_connector_profiles_for_actor( def connector_profile_usable_for_import(profile: ConnectorProfile) -> bool: """Whether a visible profile can safely offer the current live browse/import task.""" + effective_endpoint_url = connector_effective_endpoint_url( + provider=profile.provider, + endpoint_url=profile.endpoint_url, + metadata=profile.metadata, + ) if ( not profile.enabled - or not profile.endpoint_url + or not effective_endpoint_url or not profile.credentials_configured + or bool(profile.secret_ref) ): return False descriptor = next( @@ -128,11 +149,7 @@ def connector_profile_usable_for_import(profile: ConnectorProfile) -> bool: credential_id=profile.credential_profile_id, provider=profile.provider, external_path="", - external_url=connector_effective_endpoint_url( - provider=profile.provider, - endpoint_url=profile.endpoint_url, - metadata=profile.metadata, - ), + external_url=effective_endpoint_url, operation="import", ), profile.policy_sources, @@ -148,19 +165,40 @@ def _profile_visible_to_actor( campaign_id: str | None, campaign_visible: CampaignVisibility | None, ) -> bool: - if profile.scope_type == "system": + return _scope_visible_to_actor( + scope_type=profile.scope_type, + scope_id=profile.scope_id, + tenant_id=tenant_id, + user_id=user_id, + group_ids=group_ids, + campaign_id=campaign_id, + campaign_visible=campaign_visible, + ) + + +def _scope_visible_to_actor( + *, + scope_type: str, + scope_id: str | None, + tenant_id: str, + user_id: str, + group_ids: set[str], + campaign_id: str | None, + campaign_visible: CampaignVisibility | None, +) -> bool: + if scope_type == "system": return True - if profile.scope_type == "tenant": - return profile.scope_id == tenant_id - if profile.scope_type == "user": - return profile.scope_id == user_id - if profile.scope_type == "group": - return bool(profile.scope_id and profile.scope_id in group_ids) - if profile.scope_type != "campaign" or not profile.scope_id: + if scope_type == "tenant": + return scope_id == tenant_id + if scope_type == "user": + return scope_id == user_id + if scope_type == "group": + return bool(scope_id and scope_id in group_ids) + if scope_type != "campaign" or not scope_id: return False - if campaign_id and profile.scope_id != campaign_id: + if campaign_id and scope_id != campaign_id: return False - return bool(campaign_visible and campaign_visible(profile.scope_id)) + return bool(campaign_visible and campaign_visible(scope_id)) def _with_effective_connector_policy( diff --git a/tests/test_connector_profile_store.py b/tests/test_connector_profile_store.py index ecb352c..f6c76b2 100644 --- a/tests/test_connector_profile_store.py +++ b/tests/test_connector_profile_store.py @@ -11,7 +11,10 @@ from govoplan_core.db.base import Base from govoplan_core.security.secrets import decrypt_secret, encrypt_secret from govoplan_files.backend.db.models import FileConnectorProfile from govoplan_files.backend.storage.common import FileStorageError -from govoplan_files.backend.storage.connector_profile_store import update_connector_profile_row +from govoplan_files.backend.storage.connector_profile_store import ( + list_database_connector_profiles, + update_connector_profile_row, +) class ConnectorProfileStoreUpdateTests(unittest.TestCase): @@ -140,6 +143,34 @@ class ConnectorProfileStoreUpdateTests(unittest.TestCase): self.assertEqual("Original profile", persisted.label) self.assertEqual("original-password", decrypt_secret(persisted.password_encrypted)) + def test_visibility_filter_runs_before_connector_secrets_are_decrypted(self) -> None: + invisible = FileConnectorProfile( + id="invisible-profile", + tenant_id="tenant-1", + scope_type="user", + scope_id="another-user", + label="Invisible profile", + provider="webdav", + endpoint_url="https://invisible.example.test", + enabled=True, + credential_mode="basic", + password_encrypted="not-a-valid-encrypted-secret", + capabilities=["browse"], + policy={}, + metadata_={}, + ) + self.session.add(invisible) + self.session.commit() + + profiles = list_database_connector_profiles( + self.session, + tenant_id="tenant-1", + row_visible=lambda row: row.id == self.row.id, + ) + + self.assertEqual([self.row.id], [profile.id for profile in profiles]) + self.assertEqual("original-password", profiles[0].password_value) + if __name__ == "__main__": unittest.main() diff --git a/tests/test_connector_visibility.py b/tests/test_connector_visibility.py index e044ad6..bd78133 100644 --- a/tests/test_connector_visibility.py +++ b/tests/test_connector_visibility.py @@ -41,7 +41,7 @@ class ConnectorVisibilityTests(unittest.TestCase): "govoplan_files.backend.storage.connector_visibility.connector_profiles_from_settings" ) @patch( - "govoplan_files.backend.storage.connector_visibility.list_database_connector_profiles" + "govoplan_files.backend.storage.connector_visibility.select_database_connector_profiles" ) def test_profiles_are_filtered_to_actor_scopes_and_database_definition_wins( self, @@ -49,7 +49,7 @@ class ConnectorVisibilityTests(unittest.TestCase): configured_profiles, _policy_sources, ) -> None: - database_profiles.return_value = [ + profiles = [ _profile("system"), _profile("tenant", scope_type="tenant", scope_id="tenant-1"), _profile("other-tenant", scope_type="tenant", scope_id="tenant-2"), @@ -60,6 +60,7 @@ class ConnectorVisibilityTests(unittest.TestCase): _profile("disabled", enabled=False), _profile("duplicate", provider="webdav"), ] + database_profiles.return_value = (profiles, {profile.id for profile in profiles}) configured_profiles.return_value = [_profile("duplicate", provider="nextcloud")] visible = visible_connector_profiles_for_actor( @@ -87,7 +88,7 @@ class ConnectorVisibilityTests(unittest.TestCase): return_value=[], ) @patch( - "govoplan_files.backend.storage.connector_visibility.list_database_connector_profiles" + "govoplan_files.backend.storage.connector_visibility.select_database_connector_profiles" ) def test_campaign_and_admin_visibility_remain_explicit( self, @@ -95,11 +96,12 @@ class ConnectorVisibilityTests(unittest.TestCase): _configured_profiles, _policy_sources, ) -> None: - database_profiles.return_value = [ + profiles = [ _profile("campaign-a", scope_type="campaign", scope_id="campaign-a"), _profile("campaign-b", scope_type="campaign", scope_id="campaign-b"), _profile("other-user", scope_type="user", scope_id="user-2"), ] + database_profiles.return_value = (profiles, {profile.id for profile in profiles}) campaign_only = visible_connector_profiles_for_actor( object(), # type: ignore[arg-type] @@ -131,7 +133,7 @@ class ConnectorVisibilityTests(unittest.TestCase): return_value=[], ) @patch( - "govoplan_files.backend.storage.connector_visibility.list_database_connector_profiles" + "govoplan_files.backend.storage.connector_visibility.select_database_connector_profiles" ) @patch( "govoplan_files.backend.storage.connector_visibility.effective_connector_policy_sources" @@ -148,7 +150,7 @@ class ConnectorVisibilityTests(unittest.TestCase): label="System", policy={"allow": {"providers": ["webdav"]}}, ) - database_profiles.return_value = [profile] + database_profiles.return_value = ([profile], {profile.id}) policy_sources.return_value = [source] visible = visible_connector_profiles_for_actor( @@ -198,6 +200,29 @@ class ConnectorVisibilityTests(unittest.TestCase): ) ) ) + self.assertTrue( + connector_profile_usable_for_import( + ConnectorProfile( + id="metadata-endpoint", + label="Metadata endpoint", + provider="webdav", + credential_mode="anonymous", + metadata={"webdav_endpoint_url": "https://files.example.invalid"}, + ) + ) + ) + self.assertFalse( + connector_profile_usable_for_import( + ConnectorProfile( + id="unresolved-secret-reference", + label="Unresolved secret reference", + provider="webdav", + endpoint_url="https://files.example.invalid", + credential_mode="basic", + secret_ref="vault://files/webdav", + ) + ) + ) denied = replace( _profile("denied"),