41526 Storage [Backend] Create a file service#53
Conversation
…ion API - Add FileAttachment.access_type field with account/restricted options - Add FileAttachment.file_id field for unique file identification - Create FileAttachmentPermission model for user-specific file access - Implement AttachmentService.check_user_permission method - Add AttachmentsViewSet with check-permission endpoint - Add Redis caching for public template authentication - Add response_forbidden method to BaseResponseMixin - Include comprehensive test coverage for permission checking This enables fine-grained file access control with two access types: - account: accessible by all users in the same account - restricted: accessible only by users with explicit permissions
…integration - Add FastAPI-based file upload and download endpoints with streaming support - Implement Clean Architecture with domain entities, use cases, and repositories - Add authentication middleware with JWT token validation and Redis caching - Integrate Google Cloud Storage S3-compatible API for file storage - Add comprehensive error handling with custom exceptions and HTTP status codes - Implement file access permissions validation through external HTTP service - Add database models and Alembic migrations for file metadata storage - Include Docker containerization with docker-compose for local development - Add comprehensive test suite with unit, integration, and e2e tests - Configure pre-commit hooks with ruff, mypy, and pytest for code quality
…e_access_rights' into 41526__сreate_file_service # Conflicts: # backend/src/processes/enums.py # backend/src/processes/models/__init__.py # backend/src/processes/models/workflows/attachment.py # backend/src/processes/services/attachments.py # backend/src/processes/tests/test_services/test_attachments.py
… and implement caching for template data
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
….toml to dedicated config files - Move mypy configuration from pyproject.toml to mypy.ini for better separation of concerns - Simplify ruff.toml configuration by removing extensive rule selections and using "ALL" selector - Update ruff target version from py37 to py311 to match project Python version - Remove redundant ruff configuration from pyproject.toml to avoid duplication - Apply code formatting fixes across entire codebase - Standardize import statements and code style according to new linting rules - Update test files to comply with new formatting standards
…ore rule in ruff configuration - Update docstrings across various modules to ensure consistency and clarity. - Remove unused "D" rule from ruff.toml configuration. - Enhance readability and maintainability of the codebase.
…ling for consistency - Adjust import paths in test files to ensure they reference the correct locations. - Replace instances of FileNotFoundError with DomainFileNotFoundError for better clarity in exception handling. - Streamline fixture definitions and improve code readability across various test modules.
… configuration - Update docstrings across various test files for consistency and clarity. - Add new linting rules in ruff.toml for improved code quality. - Enhance readability and maintainability of the codebase by refining fixture definitions and mock implementations.
…line permission handling - Refactor the AuthenticationMiddleware to enhance error handling and response formatting. - Update permission classes to use direct Request type hints instead of string annotations. - Consolidate permission checks into FastAPI dependency wrappers for better clarity and usability. - Remove unused exception classes and error messages to clean up the codebase. - Adjust test cases to reflect changes in authentication and permission handling.
…exception tests - Remove unused infrastructure error codes from error_codes.py to streamline the codebase. - Update the AuthenticationMiddleware constructor to use direct FastAPI type hints for clarity. - Add new tests for validation exceptions, including file size and storage errors, to improve coverage and ensure accurate error handling.
…achment model and create FileAttachmentPermission model
…h SeaweedFS integration in Docker configuration
…02:8002 to 8002:8000 in Docker configurations
…ame and clean up GCS S3 environment variable assignments
…l-storage' in Docker Compose files
… to Docker Compose files
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
… redundant FileServiceConnectionException imports
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 3 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
…ser photo upload logic
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
…essary calls and add tests for logo update scenarios
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 4 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
# Conflicts: # backend/docker-compose.yml # backend/src/accounts/services/user.py # backend/src/accounts/tests/views/test_user/test_update.py # backend/src/accounts/views/user.py # backend/src/processes/enums.py # backend/src/processes/models/workflows/attachment.py # backend/src/processes/tests/fixtures.py # backend/src/processes/tests/test_services/test_attachments.py # backend/src/processes/tests/test_services/test_workflows/test_workflow_version_service.py # docker-compose.src.yml # docker-compose.yml # frontend/docker-compose.yml
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
…dedicated Attachment model and migration commands for existing attachments.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
…ices, alongside new tests and a Docker Compose configuration.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 3 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
…h API integration and shared error codes.
| AuthUser( | ||
| auth_type=UserType.AUTHENTICATED, | ||
| user_id=1, | ||
| account_id=2, | ||
| ) | ||
|
|
There was a problem hiding this comment.
unit/test_auth_middleware.py:73
The AuthUser object created on lines 73-77 is never assigned to a variable or used — it is constructed and immediately discarded. This appears to be leftover code that serves no purpose in the test.
- AuthUser(
- auth_type=UserType.AUTHENTICATED,
- user_id=1,
- account_id=2,
- )
-
# Mock PneumaticToken.data🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file storage/tests/unit/test_auth_middleware.py around lines 73-78:
The `AuthUser` object created on lines 73-77 is never assigned to a variable or used — it is constructed and immediately discarded. This appears to be leftover code that serves no purpose in the test.
Evidence trail:
storage/tests/unit/test_auth_middleware.py lines 60-95 (viewed at REVIEWED_COMMIT): Lines 73-77 show `AuthUser(...)` call with no left-hand side assignment. The test at lines 67-92 shows the `AuthUser` object is not referenced anywhere else in the test - not in the mock setup, not in assertions, and not returned by any mock.
| def test_assign_task_permissions__performer__ok(self): | ||
| # arrange | ||
| user = create_test_admin() | ||
| workflow = create_test_workflow(user=user, tasks_count=1) | ||
| task = workflow.tasks.first() | ||
| service = AttachmentService(user=user) |
There was a problem hiding this comment.
🟢 Low test_services/test_attachments_permissions.py:26
test_assign_task_permissions__performer__ok does not explicitly add the user as a task performer, so it actually tests owner/member permissions instead of performer permissions. The test passes even when performer permission assignment is broken because create_test_workflow(user=user) makes the user an implicit workflow owner/member. Consider creating a separate user and explicitly adding them via task.taskperformer_set.create(user=performer) to properly isolate the performer permission path.
- def test_assign_task_permissions__performer__ok(self):
- # arrange
- user = create_test_admin()
- workflow = create_test_workflow(user=user, tasks_count=1)
- task = workflow.tasks.first()
- service = AttachmentService(user=user)
+ def test_assign_task_permissions__performer__ok(self):
+ # arrange
+ owner = create_test_admin()
+ performer = create_test_user(account=owner.account)
+ workflow = create_test_workflow(user=owner, tasks_count=1)
+ task = workflow.tasks.first()
+ task.taskperformer_set.create(user=performer)
+ service = AttachmentService(user=owner)🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file backend/src/storage/tests/test_services/test_attachments_permissions.py around lines 26-31:
`test_assign_task_permissions__performer__ok` does not explicitly add the user as a task performer, so it actually tests owner/member permissions instead of performer permissions. The test passes even when performer permission assignment is broken because `create_test_workflow(user=user)` makes the user an implicit workflow owner/member. Consider creating a separate user and explicitly adding them via `task.taskperformer_set.create(user=performer)` to properly isolate the performer permission path.
Evidence trail:
backend/src/storage/tests/test_services/test_attachments_permissions.py lines 22-47 (test method), backend/src/processes/tests/fixtures.py lines 393-455 (create_test_workflow adds user to owners/members), backend/src/processes/tests/fixtures.py line 350 (add_raw_performer adds user as performer), backend/src/storage/services/attachments.py lines 82-148 (_assign_task_permissions grants permissions from multiple sources: performers, owners, members), lines 232-254 show properly isolated test (test_assign_task_permissions__guest_performer__ok creates separate performer)
| total = attachments.count() | ||
| self.stdout.write(f'Found {total} records ready to be synced.') | ||
|
|
||
| try: |
There was a problem hiding this comment.
🟢 Low commands/sync_files_to_file_service.py:96
If a non-psycopg2.Error exception is raised during the sync loop (e.g., from attr.account.get_owner()), the database connection opened on line 98 is never closed, causing a connection leak. Consider using a context manager or try/finally to ensure conn.close() is always called.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file backend/src/processes/management/commands/sync_files_to_file_service.py around line 96:
If a non-`psycopg2.Error` exception is raised during the sync loop (e.g., from `attr.account.get_owner()`), the database connection opened on line 98 is never closed, causing a connection leak. Consider using a context manager or `try/finally` to ensure `conn.close()` is always called.
Evidence trail:
backend/src/processes/management/commands/sync_files_to_file_service.py - full file viewed at REVIEWED_COMMIT. Connection opened at line 98 (`conn = self.get_file_db_connection()`), inner try/except only catches psycopg2.Error (lines 139-165), potential exception source at line 135 (`attr.account.get_owner()`), cleanup at lines 172-175 (`conn.close()`) is not protected by try/finally.
| command = UploadFileCommand( | ||
| file_content=b'test file content', | ||
| filename='test_file.txt', | ||
| content_type='text/plain', | ||
| size=18, | ||
| user_id=1, | ||
| account_id=1, | ||
| ) |
There was a problem hiding this comment.
🟢 Low integration/test_use_cases_integration.py:37
UploadFileCommand is constructed with size=18 but the actual file_content b'test file content' is only 17 bytes, so the test exercises the system with mismatched metadata. If production code validates that size matches the content length, this test would incorrectly fail; if it doesn't, the test may mask bugs in size-dependent logic. Consider setting size=17 to match the actual content length.
command = UploadFileCommand(
file_content=b'test file content',
filename='test_file.txt',
content_type='text/plain',
- size=18,
+ size=17,
user_id=1,
account_id=1,
)🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file storage/tests/integration/test_use_cases_integration.py around lines 37-44:
`UploadFileCommand` is constructed with `size=18` but the actual `file_content` `b'test file content'` is only 17 bytes, so the test exercises the system with mismatched metadata. If production code validates that `size` matches the content length, this test would incorrectly fail; if it doesn't, the test may mask bugs in size-dependent logic. Consider setting `size=17` to match the actual content length.
Evidence trail:
storage/tests/integration/test_use_cases_integration.py lines 37-46 (REVIEWED_COMMIT): Shows `UploadFileCommand` with `file_content=b'test file content'` and `size=18`.
Byte count verification: `b'test file content'` = 17 bytes (t,e,s,t, ,f,i,l,e, ,c,o,n,t,e,n,t)
storage/src/presentation/api/files.py lines 66-67 (REVIEWED_COMMIT): Production code calculates `file_size = len(file_content)` ensuring consistency, which the test bypasses.
storage/src/application/use_cases/file_upload.py lines 48-56 (REVIEWED_COMMIT): Shows `command.size` is directly used to create `FileRecord` without validation against content length.
| except (ValueError, TypeError, IntegrityError): | ||
| return [], bool(existent_file_ids) |
There was a problem hiding this comment.
🟡 Medium storage/utils.py:278
When the transaction in refresh_attachments_for_event rolls back, the function returns has_attachments = bool(existent_file_ids). Since existent_file_ids only contains IDs that appear in both the new file_ids and existing attachments, it's empty when the text references entirely new files. The rollback restores the original attachments, so returning False is wrong — the event still has attachments. This causes _refresh_workflow_event_attachments to incorrectly set event.with_attachments = False.
Consider querying the actual attachment count after rollback, or restructuring to avoid the stale has_attachments value.
- except (ValueError, TypeError, IntegrityError):
- return [], bool(existent_file_ids)🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file backend/src/storage/utils.py around lines 278-279:
When the transaction in `refresh_attachments_for_event` rolls back, the function returns `has_attachments = bool(existent_file_ids)`. Since `existent_file_ids` only contains IDs that appear in both the new `file_ids` and existing attachments, it's empty when the text references entirely new files. The rollback restores the original attachments, so returning `False` is wrong — the event still has attachments. This causes `_refresh_workflow_event_attachments` to incorrectly set `event.with_attachments = False`.
Consider querying the actual attachment count after rollback, or restructuring to avoid the stale `has_attachments` value.
Evidence trail:
backend/src/storage/utils.py lines 247-253 (existent_file_ids computation), lines 257-278 (transaction and exception handler returning bool(existent_file_ids)), lines 493-508 (_refresh_workflow_event_attachments using has_attachments to update event.with_attachments). Test at backend/src/storage/tests/test_workflow_event_attachments.py lines 148-181 confirms different scenario (event with no pre-existing attachments).
| source_type = SourceType.ACCOUNT | ||
|
|
||
| task_id = None | ||
| if attr.event_id and attr.event.task_id: | ||
| task_id = attr.event.task_id | ||
| elif attr.output_id and attr.output.task_id: | ||
| task_id = attr.output.task_id | ||
|
|
||
| template_id = None | ||
| if attr.workflow_id: | ||
| template_id = attr.workflow.template_id | ||
| elif ( | ||
| attr.event_id and | ||
| attr.event.task_id and | ||
| attr.event.task.workflow_id | ||
| ): | ||
| template_id = attr.event.task.workflow.template_id | ||
| elif attr.output_id and attr.output.workflow_id: | ||
| template_id = attr.output.workflow.template_id | ||
|
|
There was a problem hiding this comment.
🟠 High commands/migrate_file_attachment_to_attachment.py:72
source_type is hardcoded to SourceType.ACCOUNT regardless of which relationships are populated. When workflow_id, task_id, or template_id are set, the migrated attachment still reports SourceType.ACCOUNT, causing any logic that branches on source_type to misroute. Consider deriving source_type from the actual relationship: use SourceType.TASK if task_id is set, SourceType.WORKFLOW if workflow_id is set, SourceType.TEMPLATE if template_id is set, and fall back to SourceType.ACCOUNT only when none apply.
- source_type = SourceType.ACCOUNT🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file backend/src/processes/management/commands/migrate_file_attachment_to_attachment.py around lines 72-91:
`source_type` is hardcoded to `SourceType.ACCOUNT` regardless of which relationships are populated. When `workflow_id`, `task_id`, or `template_id` are set, the migrated attachment still reports `SourceType.ACCOUNT`, causing any logic that branches on `source_type` to misroute. Consider deriving `source_type` from the actual relationship: use `SourceType.TASK` if `task_id` is set, `SourceType.WORKFLOW` if `workflow_id` is set, `SourceType.TEMPLATE` if `template_id` is set, and fall back to `SourceType.ACCOUNT` only when none apply.
Evidence trail:
1. backend/src/processes/management/commands/migrate_file_attachment_to_attachment.py line 72: `source_type = SourceType.ACCOUNT` hardcoded
2. Same file lines 74-89: `task_id` and `template_id` are computed from relationships but never used to set `source_type`
3. backend/src/storage/enums.py lines 4-18: `SourceType` enum with ACCOUNT, WORKFLOW, TASK, TEMPLATE values and docstring "Defines which entity the file is attached to"
4. backend/src/storage/services/attachments.py lines 75-79: branching logic `if self.instance.source_type == SourceType.TASK: ... elif ... TEMPLATE ... elif ... WORKFLOW`
5. backend/src/storage/models.py line 22: index on `[source_type, account]` confirming it's used for queries
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 4 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
| def save(self, *args, **kwargs): | ||
| super().save(*args, **kwargs) | ||
| from src.authentication.services.public_auth import PublicAuthService | ||
| PublicAuthService.invalidate_template_public_tokens(self) |
There was a problem hiding this comment.
Template save cache invalidation breaks on transaction rollback
Medium Severity
The Template.save() override calls invalidate_template_public_tokens after super().save(), performing Redis cache deletions that are non-transactional. If save() is called within a transaction.atomic() block that later rolls back, the database reverts but the cache entry remains deleted. This temporarily breaks file service access for public/embed tokens, since the file service relies on this cache for authentication. The comment in invalidate_template_public_tokens even states "Storage service relies on this cache for public token auth."
Additional Locations (1)
| 'error': str(ex), | ||
| }, | ||
| level=SentryLogLevel.ERROR, | ||
| ) |
There was a problem hiding this comment.
Unhandled AttributeError when form user is None
Low Severity
In IntegrationCreateForm.save(), self.user defaults to None (via kwargs.pop('user', None)), but self.user.account is accessed inside the try block without a None check. If self.user is None, this raises an AttributeError, which is not a subclass of FileServiceException and won't be caught by the except handler, causing an unhandled crash. While the admin's get_form wrapper currently always injects request.user, the __init__ explicitly accepts None as a default, making this a latent bug.
| except psycopg2.Error as e: | ||
| logger.error("Error syncing %s: %s", file_id, e) | ||
| conn.rollback() | ||
| errors += 1 |
There was a problem hiding this comment.
Batch rollback silently loses successfully synced records
Low Severity
In the sync loop, conn.rollback() on a single insert error rolls back ALL uncommitted records since the last batch commit, not just the failed one. Records that were successfully inserted but not yet committed (up to batch_size - 1 records) are silently lost. The synced counter still includes these rolled-back records, so the final report overestimates the number of actually synced records.
| user=self.user, | ||
| old_values=[old_photo], | ||
| new_values=[self.instance.photo], | ||
| ) |
There was a problem hiding this comment.
sync_account_file_fields always called even when photo unchanged
Low Severity
sync_account_file_fields is called unconditionally on every partial_update in UserGroupService, even when the photo hasn't changed. In contrast, AccountService.partial_update guards the equivalent call with a change check (if (old_logo_lg, old_logo_sm) != ...). This inconsistency means unnecessary work extracting file IDs and querying attachments on every group update regardless of whether photo-related fields were modified.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
There are 102 total unresolved issues (including 100 from previous reviews).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 3dfa00f. Configure here.
| account=user.account, | ||
| user=user, | ||
| old_values=old_values, | ||
| new_values=[], |
There was a problem hiding this comment.
Microsoft photo sync omits new URL
Medium Severity
After updating user.photo, apply_photo_to_user calls sync_account_file_fields with an empty new_values list. That helper only creates account-scoped Attachment rows from new_values, so a photo URL applied without a matching upload in the same flow may never get an Attachment record while old IDs can still be removed.
Reviewed by Cursor Bugbot for commit 3dfa00f. Configure here.
| ] = fs_domain | ||
| url_mapping[ | ||
| f"https://storage.cloud.google.com/{account.bucket_name}" | ||
| ] = fs_domain |
There was a problem hiding this comment.
Bucket prefix breaks migrated links
High Severity
The migration maps whole GCS bucket base URLs to the file-service base URL. Substring replacement then rewrites full object URLs into {file-service}/{object-path} instead of {file-service}/{file_id}, so many migrated links won’t match the file service’s download routes.
Reviewed by Cursor Bugbot for commit 3dfa00f. Configure here.
3 participants
Note
Add a dedicated file storage microservice to replace Google Cloud Storage uploads
storage/that handles file uploads and downloads using an S3-compatible backend (SeaweedFS or GCS), with its own Postgres database, Alembic migrations, JWT/Redis-based authentication, and object-level permission checks.Attachmentmodel inbackend/src/storage/withdjango-guardianobject permissions, replacingFileAttachment-based attachment tracking across tasks, workflows, templates, and comments.file_idrather than numeric attachment IDs.$FILE_DOMAINto the new service, and extendsdocker-composefiles to includefile-service,file-postgres, and SeaweedFS stack services.file_idon existingFileAttachmentrows from GCS URLs, migrating them to the newAttachmentmodel, syncing records to the file DB, and replacing legacy storage links with file service URLs.google-cloud-storagedependency andStoragePermissionclass; addsdjango-guardian.WorkflowEventserializer no longer includes anattachmentsfield;/workflows/attachmentsand/workflows/public/attachmentsendpoints are removed; attachment IDs are now strings throughout the frontend.Macroscope summarized 3dfa00f.
Note
High Risk
Large cross-cutting change to file storage, auth, API contracts (comments/attachments), and data migration paths affecting security-sensitive uploads and existing clients.
Overview
This PR replaces in-app Google Cloud Storage uploads with a dedicated file microservice wired into local Docker (backend API,
file-service, file Postgres, SeaweedFS) and removes legacySTORAGE/ GCS env from Celery workers.Uploads and avatars/logos now go through
FileServiceClient(upload_file_with_attachment) in admin, contacts, integrations, Microsoft Graph photos, and related paths;sync_account_file_fieldskeepsstorage.Attachmentrows aligned when account logos or user/group photos change. django-guardian and a customGroupObjectPermissionapp support object-level access on the new attachment model.Workflow attachments shift from integer
FileAttachmentIDs to markdown links to the file service; comments require text only (noattachmentslist). Event/task serializers drop embeddedattachments;refresh_attachmentsand Guardian cleanup run on comments and task updates. Public/embed template auth caches template data in Redis and invalidates on template save.Migration tooling adds management commands (
fill_file_attachment_file_id, migrate toAttachment, sync file DB, replace GCS URLs,run_file_migration) plus analytics/tests updated forstorage.Attachment. Breaking: API consumers using commentattachmentsor file fields as ID lists, or expectingattachmentson workflow events, must move to file-service URLs in markdown.Reviewed by Cursor Bugbot for commit 3dfa00f. Bugbot is set up for automated code reviews on this repo. Configure here.