Add password protection for documents and directories#11
Conversation
Co-authored-by: Creeper19472 <38857196+Creeper19472@users.noreply.github.com>
Co-authored-by: Creeper19472 <38857196+Creeper19472@users.noreply.github.com>
Co-authored-by: Creeper19472 <38857196+Creeper19472@users.noreply.github.com>
…rison Co-authored-by: Creeper19472 <38857196+Creeper19472@users.noreply.github.com>
|
@copilot Several issues exist: 1. You didn't understand what I meant. I meant that for current password protection and all future protection types, they will all share a single table, using a column of type such as "protection" to distinguish their types, instead of splitting each protection type into a separate table as you did before. 2. Don't arbitrarily add functions that are only valid for a specific protection type, such as |
- Renamed PasswordProtection to ObjectProtection for single table design - Changed schema: protection_type column distinguishes types, protection_data stores type-specific JSON - Removed password-specific methods from BaseObject to keep it clean - Created check_password_protection helper function in protection handlers - Updated all handlers to use helper function instead of BaseObject methods - Table now supports multiple protection types (password, encryption, biometric, etc.) Co-authored-by: Creeper19472 <38857196+Creeper19472@users.noreply.github.com>
Refactored in commit 09e892b:
The design now supports multiple protection types in a single table and keeps BaseObject focused solely on access control. |
Co-authored-by: Creeper19472 <38857196+Creeper19472@users.noreply.github.com>
Co-authored-by: Creeper19472 <38857196+Creeper19472@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR implements password-based access control for documents and directories using a unified protection system. The implementation adds a new object_protections database table that can support multiple protection types (password, encryption, biometric, etc.) distinguished by a protection_type column.
Key Changes
- New password protection database model with PBKDF2-HMAC-SHA256 hashing (600K iterations) and constant-time comparison
- Three new API endpoints for enabling, removing, and verifying password protection
- Integration of password checks into existing document and directory access handlers
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
tests/test_password_protection.py |
Comprehensive test suite covering password protection on documents and directories, including enable/remove/verify operations and access control scenarios |
src/main.py |
Imports the new ObjectProtection model for database initialization |
src/include/handlers/protection.py |
New handlers for password protection management and helper function for password verification |
src/include/handlers/document.py |
Adds optional password parameter to document access endpoints and integrates password protection checks |
src/include/handlers/directory.py |
Adds optional password parameter to directory access endpoints and integrates password protection checks |
src/include/database/models/protection.py |
New database model for unified protection system with password hashing and verification methods |
src/include/database/models/entity.py |
Refactors TARGET_TYPE_MAPPING from local to global constant |
src/include/constants.py |
Adds TARGET_TYPE_MAPPING constant for mapping database table names to API types |
src/include/connection_handler.py |
Registers new password protection handlers in the available functions dictionary |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| "enum": ["document", "directory"] | ||
| }, | ||
| "target_id": {"type": "string", "minLength": 1}, | ||
| "password": {"type": "string"} |
There was a problem hiding this comment.
The password validation in the data schema allows empty strings. Similar to enable_password_protection, the password field should have minLength validation to prevent empty passwords from being accepted during verification.
| "password": {"type": "string"} | |
| "password": {"type": "string", "minLength": 1} |
| if password is None: | ||
| # Password required but not provided |
There was a problem hiding this comment.
The check_password_protection function only checks for None password, but doesn't handle empty strings. If a user provides an empty password string in the request, it will be passed to verify_password and processed, which could lead to unexpected behavior. Consider treating empty strings the same as None (password required), or add validation to reject empty password strings.
| if password is None: | |
| # Password required but not provided | |
| if password is None or password == "": | |
| # Password required but not provided or empty |
| id: Mapped[int] = mapped_column(Integer, primary_key=True, autoincrement=True) | ||
|
|
||
| # Target object identification | ||
| target_type: Mapped[str] = mapped_column( | ||
| VARCHAR(64), nullable=False, comment="Type: 'document' or 'directory'" | ||
| ) | ||
| target_id: Mapped[str] = mapped_column( | ||
| VARCHAR(255), nullable=False, comment="ID of the protected object" | ||
| ) | ||
|
|
||
| # Protection type identifier | ||
| protection_type: Mapped[str] = mapped_column( | ||
| VARCHAR(64), nullable=False, default="password", | ||
| comment="Protection type: 'password', 'encryption', 'biometric', etc." | ||
| ) | ||
|
|
||
| # Protection data (type-specific, stored as JSON or text) | ||
| # For password: contains password_hash and salt | ||
| # For other types: contains type-specific data | ||
| protection_data: Mapped[str] = mapped_column( | ||
| Text, nullable=False, | ||
| comment="Protection-specific data (JSON format for flexibility)" | ||
| ) | ||
|
|
||
| # Additional metadata (reserved for future use) | ||
| protection_metadata: Mapped[Optional[str]] = mapped_column( | ||
| Text, nullable=True, | ||
| comment="JSON metadata for future extensions" | ||
| ) |
There was a problem hiding this comment.
The database model is missing a unique constraint to prevent duplicate protection entries for the same target. Without a unique constraint on (target_type, target_id, protection_type), multiple password protection entries could be created for the same document or directory, leading to undefined behavior during verification. Consider adding a unique constraint or composite index on these three columns.
| else: | ||
| handler.conclude_request(400, {}, "Invalid target type") | ||
| return 400, target_id, handler.username | ||
|
|
There was a problem hiding this comment.
The verify_password handler does not check if the user has read permissions for the target object before allowing password verification. This could allow unauthorized users to perform brute-force password attacks on documents/directories they don't have access to. Standard access permissions should be checked first (similar to how enable_password_protection and remove_password_protection check for manage permissions).
| # Check that the user has read permissions for the target object | |
| has_read_permission = False | |
| if hasattr(user, "can_read"): | |
| # Prefer an explicit can_read(target) capability method if available. | |
| has_read_permission = bool(user.can_read(target)) # type: ignore[call-arg] | |
| elif hasattr(user, "has_permission"): | |
| # Fallback to a generic permission check API, if provided by the User model. | |
| try: | |
| has_read_permission = bool(user.has_permission("read", target)) # type: ignore[call-arg] | |
| except TypeError: | |
| # In case has_permission has a different signature, fail closed. | |
| has_read_permission = False | |
| if not has_read_permission: | |
| handler.conclude_request( | |
| 403, | |
| {}, | |
| "You do not have permission to access this object", | |
| ) | |
| return 403, target_id, handler.username |
| data_schema = { | ||
| "type": "object", | ||
| "properties": { | ||
| "target_type": { | ||
| "type": "string", | ||
| "enum": ["document", "directory"] | ||
| }, | ||
| "target_id": {"type": "string", "minLength": 1}, | ||
| "password": {"type": "string", "minLength": 1} | ||
| }, |
There was a problem hiding this comment.
The password validation in the data schema allows empty strings (minLength is 1 for target_id but password only has type "string" without minLength). An empty password would be hashed and stored, but this is a poor security practice. Consider adding minLength validation for the password field, or adding explicit validation logic to reject empty passwords.
| def check_password_protection(target, password: Optional[str], session) -> tuple[int, str]: | ||
| """ | ||
| Check password protection for a document or directory. | ||
|
|
||
| This is a helper function to be used by request handlers to check | ||
| if an object has password protection and verify the password if provided. | ||
|
|
||
| Args: | ||
| target: The Document or Folder object to check | ||
| password: The password provided by the user (None if not provided) | ||
| session: The database session | ||
|
|
||
| Returns: | ||
| Tuple of (code, message): | ||
| - (0, "success") if access granted (no protection or correct password) | ||
| - (202, "Password required") if protected but no password provided | ||
| - (403, "Incorrect password") if protected and wrong password provided | ||
| """ | ||
| target_type = TARGET_TYPE_MAPPING[target.__tablename__] | ||
|
|
||
| # Check if object has password protection | ||
| protection = ( | ||
| session.query(ObjectProtection) | ||
| .filter( | ||
| ObjectProtection.target_type == target_type, | ||
| ObjectProtection.target_id == target.id, | ||
| ObjectProtection.protection_type == "password" | ||
| ) | ||
| .first() | ||
| ) | ||
|
|
||
| if not protection: | ||
| # No password protection, access granted | ||
| return (0, "success") | ||
|
|
||
| if password is None: | ||
| # Password required but not provided | ||
| return (202, "Password required") | ||
|
|
||
| # Verify the password | ||
| if protection.verify_password(password): | ||
| return (0, "success") | ||
| else: | ||
| return (403, "Incorrect password") |
There was a problem hiding this comment.
The return value (0, "success") for successful password verification is inconsistent with HTTP status code conventions. The function returns tuple[int, str] where 0 represents success, but this is mixed with HTTP status codes (202, 403) for other cases. This could be confusing. Consider using a more consistent approach, such as returning None for success or using a dedicated status code like 200.
| "additionalProperties": False, | ||
| } | ||
|
|
||
| def handle(self, handler: ConnectionHandler): |
There was a problem hiding this comment.
RequestGetDirectoryInfoHandler.handle returns tuple of size 2 and tuple of size 3.
RequestGetDirectoryInfoHandler.handle returns tuple of size 2 and tuple of size 3.
RequestGetDirectoryInfoHandler.handle returns tuple of size 2 and tuple of size 3.
RequestGetDirectoryInfoHandler.handle returns tuple of size 2 and tuple of size 3.
RequestGetDirectoryInfoHandler.handle returns tuple of size 2 and tuple of size 3.
RequestGetDirectoryInfoHandler.handle returns tuple of size 2 and tuple of size 3.
RequestGetDirectoryInfoHandler.handle returns tuple of size 2 and tuple of size 3.
RequestGetDirectoryInfoHandler.handle returns tuple of size 2 and tuple of size 3.
| from include.database.models.classic import User, UserGroup | ||
| from include.database.models.entity import Document, DocumentRevision | ||
| from include.database.models.file import File | ||
| from include.database.models.protection import ObjectProtection |
There was a problem hiding this comment.
Import of 'ObjectProtection' is not used.
| from include.database.models.protection import ObjectProtection |
| # Cleanup | ||
| try: | ||
| await authenticated_client.delete_directory(directory_id) | ||
| except Exception: |
There was a problem hiding this comment.
'except' clause does nothing but pass and there is no explanatory comment.
| except Exception: | |
| except Exception: | |
| # Best-effort cleanup: ignore errors if the directory was already deleted | |
| # or cannot be removed, so that they do not mask earlier test failures. |
| # Cleanup | ||
| try: | ||
| await authenticated_client.delete_directory(directory_id) | ||
| except Exception: |
There was a problem hiding this comment.
'except' clause does nothing but pass and there is no explanatory comment.
| except Exception: | |
| except Exception: | |
| # Best-effort cleanup: ignore errors if the directory is already deleted |
Implements password-based access control for documents and directories. Authentication flow: check standard permissions first, then password if protected. Returns 202 when password required but missing, 403 when incorrect.
Database Schema
object_protectionstable (single unified table for all protection types)target_type,target_id,protection_type,protection_data,protection_metadataprotection_type: Distinguishes protection types ('password', 'encryption', 'biometric', etc.)protection_data: JSON format storing type-specific data (e.g., password_hash and salt for passwords)protection_metadata: Reserved for future extensionsArchitecture
protection_typecolumncheck_password_protection()helper function in handlers performs verificationObjectProtectionmodel without modifying BaseObjectSecurity
secrets.compare_digest()(timing attack prevention)API Endpoints
Handler Integration
Updated handlers now accept optional
passwordparameter:get_document,get_document_infolist_directory,get_directory_infoImplementation Notes
check_password_protection()helper function in handlers performs protection checksObjectProtectionmodel contains type-specific methods (e.g.,set_password(),verify_password())TARGET_TYPE_MAPPINGconstant maps table names to API typesOriginal prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.