diff --git a/src/govoplan_files/backend/storage/files.py b/src/govoplan_files/backend/storage/files.py index 67764c6..072e1d8 100644 --- a/src/govoplan_files/backend/storage/files.py +++ b/src/govoplan_files/backend/storage/files.py @@ -596,21 +596,31 @@ def _asset_visibility_query_for_user( if owner_type == "group" and owner_id: query = query.filter(FileAsset.owner_group_id == owner_id) if campaign_id: - query = query.join(FileShare, FileShare.file_asset_id == FileAsset.id).filter( + campaign_share = exists().where( FileShare.tenant_id == tenant_id, + FileShare.file_asset_id == FileAsset.id, FileShare.target_type == "campaign", FileShare.target_id == campaign_id, effective_file_share_clause(), ) + query = query.filter(campaign_share) elif not is_admin and not owner_type: group_ids = user_group_ids(session, tenant_id=tenant_id, user_id=user_id) - query = query.outerjoin(FileShare, FileShare.file_asset_id == FileAsset.id).filter( + active_share = exists().where( + FileShare.tenant_id == tenant_id, + FileShare.file_asset_id == FileAsset.id, + effective_file_share_clause(), + or_( + (FileShare.target_type == "user") & (FileShare.target_id == user_id), + (FileShare.target_type == "group") & (FileShare.target_id.in_(group_ids)), + (FileShare.target_type == "tenant") & (FileShare.target_id == tenant_id), + ) + ) + query = query.filter( or_( (FileAsset.owner_type == "user") & (FileAsset.owner_user_id == user_id), (FileAsset.owner_type == "group") & (FileAsset.owner_group_id.in_(group_ids)), - effective_file_share_clause() & (FileShare.target_type == "user") & (FileShare.target_id == user_id), - effective_file_share_clause() & (FileShare.target_type == "group") & (FileShare.target_id.in_(group_ids)), - effective_file_share_clause() & (FileShare.target_type == "tenant") & (FileShare.target_id == tenant_id), + active_share, ) ) if path_prefix: @@ -639,7 +649,7 @@ def _asset_visibility_query_for_user( CampaignAttachmentUse.use_stage == "sent", ) query = query.filter(sent_use_exists if audit_relevant else ~sent_use_exists) - return query.distinct() + return query def count_assets_for_user( diff --git a/tests/test_share_lifecycle.py b/tests/test_share_lifecycle.py index 256d09c..55326ad 100644 --- a/tests/test_share_lifecycle.py +++ b/tests/test_share_lifecycle.py @@ -5,6 +5,7 @@ from datetime import timedelta from unittest.mock import patch from sqlalchemy import create_engine +from sqlalchemy.dialects import postgresql from sqlalchemy.orm import sessionmaker from govoplan_access.backend.db.models import Account, Group, User @@ -13,6 +14,7 @@ from govoplan_core.db.base import Base from govoplan_files.backend.db.models import FileAsset, FileShare from govoplan_files.backend.storage.common import FileStorageError, utcnow from govoplan_files.backend.storage.files import ( + _asset_visibility_query_for_user, get_asset_for_user, list_file_shares, revoke_file_share, @@ -214,6 +216,22 @@ class FileShareLifecycleTests(unittest.TestCase): expires_at=utcnow() - timedelta(seconds=1), ) + @patch( + "govoplan_files.backend.storage.files.user_group_ids", + return_value=[], + ) + def test_visibility_query_is_postgresql_json_safe(self, _groups) -> None: + query = _asset_visibility_query_for_user( + self.session, + tenant_id=TENANT_ID, + user_id=RECIPIENT_ID, + ) + + compiled = str(query.statement.compile(dialect=postgresql.dialect())) + + self.assertNotIn("SELECT DISTINCT", compiled.upper()) + self.assertIn("EXISTS", compiled.upper()) + if __name__ == "__main__": unittest.main()