security(files): filter connectors before secret resolution

2026-07-21 19:15:41 +02:00
parent 4722161592
commit 58af1a20a7
4 changed files with 147 additions and 28 deletions
--- 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(
--- 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(
--- 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()
--- 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"),