add emergency contact #18
Conversation
WalkthroughThe changes introduce a complete emergency contacts feature to the application. This includes database schema updates, new API endpoints, service and repository logic, and model relationships. Supporting improvements were made to authentication, authorization, and error handling, as well as Docker configuration adjustments to load environment variables from a Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant API (Flask)
participant EmergencyContactsService
participant EmergencyContactRepository
participant Database
User->>API (Flask): Authenticated request (GET /emergency-contacts)
API (Flask)->>EmergencyContactsService: get_emergency_contacts(user_id)
EmergencyContactsService->>EmergencyContactRepository: get_by_user_id(user_id)
EmergencyContactRepository->>Database: Query emergency_contacts by user_id
Database-->>EmergencyContactRepository: List of contacts
EmergencyContactRepository-->>EmergencyContactsService: List of contacts
EmergencyContactsService-->>API (Flask): List of contacts
API (Flask)-->>User: JSON response
User->>API (Flask): Authenticated request (POST /emergency-contacts)
API (Flask)->>EmergencyContactsService: create_emergency_contacts(user_id, contact_data)
EmergencyContactsService->>EmergencyContactRepository: create(new_contact)
EmergencyContactRepository->>Database: Insert new contact
Database-->>EmergencyContactRepository: Created contact
EmergencyContactRepository-->>EmergencyContactsService: Created contact
EmergencyContactsService-->>API (Flask): Created contact
API (Flask)-->>User: JSON response (201 Created)
User->>API (Flask): Authenticated request (PUT /emergency-contacts/<id>)
API (Flask)->>EmergencyContactsService: update_emergency_contacts(contact_id, contact_data)
EmergencyContactsService->>EmergencyContactRepository: update(contact_id, contact_data)
EmergencyContactRepository->>Database: Update contact
Database-->>EmergencyContactRepository: Updated contact
EmergencyContactRepository-->>EmergencyContactsService: Updated contact
EmergencyContactsService-->>API (Flask): Updated contact
API (Flask)-->>User: JSON response
User->>API (Flask): Authenticated request (DELETE /emergency-contacts/<id>)
API (Flask)->>EmergencyContactsService: delete_emergency_contacts(contact_id)
EmergencyContactsService->>EmergencyContactRepository: delete(contact_id)
EmergencyContactRepository->>Database: Delete contact
Database-->>EmergencyContactRepository: Success
EmergencyContactRepository-->>EmergencyContactsService: None
EmergencyContactsService-->>API (Flask): None
API (Flask)-->>User: 204 No Content
Poem
Note ⚡️ AI Code Reviews for VS Code, Cursor, WindsurfCodeRabbit now has a plugin for VS Code, Cursor and Windsurf. This brings AI code reviews directly in the code editor. Each commit is reviewed immediately, finding bugs before the PR is raised. Seamless context handoff to your AI code agent ensures that you can easily incorporate review feedback. Note ⚡️ Faster reviews with cachingCodeRabbit now supports caching for code and dependencies, helping speed up reviews. This means quicker feedback, reduced wait times, and a smoother review experience overall. Cached data is encrypted and stored securely. This feature will be automatically enabled for all accounts on May 16th. To opt out, configure ✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
♻️ Duplicate comments (2)
docker-compose.yml (2)
36-37: Ensure consistency ofenv_fileusage for the prod service.
Same guidance applies here formind-matter-flask-prod: confirm the existence of.env, its git-ignoring, and consider using a dedicated.env.productionfile.
52-53: Ensure consistency ofenv_fileusage for the manage service.
Same guidance applies here formind-matter-manage: verify.envpresence and ignore settings (or use a context-specific env file).
🧹 Nitpick comments (10)
migrations/versions/596e8c1b8988_.py (1)
21-24: Consider named constraints for better maintainabilityThe foreign key constraint is created without a specific name (
None), which might make future schema maintenance more difficult. Consider using explicit constraint names for easier tracking and maintenance.- batch_op.create_foreign_key(None, 'users', ['user_id'], ['user_id']) + batch_op.create_foreign_key('fk_emergency_contact_user', 'users', ['user_id'], ['user_id'])mind_matter_api/utils/decorators.py (2)
2-2: Remove unusedrequestimport
requestisn’t referenced anywhere in the module; keeping it around will failruff(F401) and slightly increase import time.-from flask import request, abort, current_app as app +from flask import abort, current_app as app🧰 Tools
🪛 Ruff (0.8.2)
2-2:
flask.requestimported but unusedRemove unused import:
flask.request(F401)
6-6: Avoid configuring global logging inside a library/helper moduleCalling
logging.basicConfig()at import time overrides the log format/level for the whole process, which is the application’s responsibility (often done in the entry-point or a dedicated logging config file).
Consider removing this line and letting the Flask app (or a CLI) decide the log configuration.-logging.basicConfig(level=logging.DEBUG) +# Prefer: configure logging in the main application initialisation codemind_matter_api/models/emergency_contacts.py (1)
8-8: Consider addingondelete="CASCADE"and an index onuser_idMost queries will fetch contacts by
user_id; an index greatly improves performance.
ondelete="CASCADE"(paired with SQLAlchemy’spassive_deletes=Trueif needed) keeps orphaned contacts from accumulating when a user is removed.- user_id = db.Column(db.Integer, db.ForeignKey('users.user_id'), nullable=False) + user_id = db.Column( + db.Integer, + db.ForeignKey('users.user_id', ondelete="CASCADE"), + nullable=False, + index=True, + )mind_matter_api/repositories/emergency_contacts.py (2)
3-4: Remove unuseddbimport
dbisn’t referenced after the refactor; keeping it violates lint rules (F401).-from mind_matter_api.models import db🧰 Tools
🪛 Ruff (0.8.2)
4-4:
mind_matter_api.models.dbimported but unusedRemove unused import:
mind_matter_api.models.db(F401)
10-12: Type-safe filtering inget_by_user_id
user_idis annotated asstrbut the column isInteger; relying on implicit coercion can hurt performance and break on some DBs (e.g., PostgreSQL with strict casting).- def get_by_user_id(self, user_id: str) -> List[EmergencyContact]: - return self.session.query(EmergencyContact).filter(EmergencyContact.user_id == user_id).all() + def get_by_user_id(self, user_id: int) -> List[EmergencyContact]: + return ( + self.session.query(EmergencyContact) + .filter(EmergencyContact.user_id == int(user_id)) + .all() + )Optionally, add pagination to keep large result sets manageable.
mind_matter_api/utils/auth.py (1)
31-35: Repository/Service instantiation on every callCreating
UserRepository()andUserService()per admin check adds connection overhead on hot paths (every request hitting an admin-guarded endpoint).Consider:
- Re-using a single repository/service per request (store in
gor a lightweight cache), or- Moving the admin role check into a DB query that only fetches the
rolecolumn.This keeps latency low under load.
mind_matter_api/api/emergency_contacts.py (2)
9-10: Avoid callinglogging.basicConfigin library / route modulesInvoking
basicConfigin a non-entrypoint file can:
- Override logging configuration established by the application’s main entrypoint or tests.
- Add multiple handlers when this module is imported repeatedly (e.g., in unit tests with
flask.testing.FlaskClient).Instead, rely on the application-level configuration done in
app.py/wsgi.py, or conditionally add a module-specific logger.
37-38: Avoid double (de)serialization round-trip
validated_data = schema.load(); schema.dump(validated_data)immediately serialises the object back into a dict, doing redundant work.-validated_data = EmergencyContactSchema().load(request_data) -contact_data = EmergencyContactSchema().dump(validated_data) +contact_data = EmergencyContactSchema().load(request_data)mind_matter_api/services/emergency_contacts.py (1)
7-8: Move logger configuration out of the service moduleAs with the API file, calling
logging.basicConfiginside a library class can clash with the main application’s configuration and add duplicate handlers.Remove these lines and rely on application-level setup.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
⛔ Files ignored due to path filters (1)
mind_matter_api/.DS_Storeis excluded by!**/.DS_Store
📒 Files selected for processing (11)
docker-compose.yml(3 hunks)migrations/versions/596e8c1b8988_.py(1 hunks)mind_matter_api/api/__init__.py(2 hunks)mind_matter_api/api/emergency_contacts.py(1 hunks)mind_matter_api/models/emergency_contacts.py(1 hunks)mind_matter_api/models/surveys.py(1 hunks)mind_matter_api/models/users.py(2 hunks)mind_matter_api/repositories/emergency_contacts.py(1 hunks)mind_matter_api/services/emergency_contacts.py(1 hunks)mind_matter_api/utils/auth.py(2 hunks)mind_matter_api/utils/decorators.py(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
mind_matter_api/utils/decorators.py (1)
mind_matter_api/utils/auth.py (2)
is_user_admin(29-38)is_user_owner(40-64)
mind_matter_api/api/emergency_contacts.py (5)
mind_matter_api/schemas.py (1)
EmergencyContactSchema(177-181)mind_matter_api/services/emergency_contacts.py (5)
EmergencyContactsService(9-34)get_emergency_contacts(13-15)create_emergency_contacts(17-20)update_emergency_contacts(22-31)delete_emergency_contacts(33-34)mind_matter_api/utils/decorators.py (1)
require_auth(8-13)mind_matter_api/utils/auth.py (2)
is_user_owner(40-64)is_user_admin(29-38)mind_matter_api/repositories/emergency_contacts.py (1)
get_by_id(18-19)
🪛 Ruff (0.8.2)
mind_matter_api/repositories/emergency_contacts.py
4-4: mind_matter_api.models.db imported but unused
Remove unused import: mind_matter_api.models.db
(F401)
mind_matter_api/utils/decorators.py
2-2: flask.request imported but unused
Remove unused import: flask.request
(F401)
mind_matter_api/api/emergency_contacts.py
1-1: flask.current_app imported but unused
Remove unused import: flask.current_app
(F401)
5-5: mind_matter_api.utils.decorators.require_owner imported but unused
Remove unused import: mind_matter_api.utils.decorators.require_owner
(F401)
🔇 Additional comments (8)
mind_matter_api/models/surveys.py (1)
15-15: Excellent addition of the bidirectional relationship to User modelThis relationship addition creates a proper bidirectional link with the User model, which aligns with SQLAlchemy best practices and complements the existing foreign key. This enables cleaner ORM-level navigation between Survey and User instances.
mind_matter_api/api/__init__.py (2)
18-21: Well-structured imports for the emergency contacts featureThe imports follow the established pattern in the codebase with clear organization and descriptive comments.
37-39: Proper integration of emergency contacts service and routesThe emergency contacts feature initialization follows the same architectural pattern used for other services in the application. Good job maintaining consistency in the application structure.
mind_matter_api/models/users.py (2)
36-37: Good implementation of bidirectional relationshipsThese relationships follow SQLAlchemy best practices and properly complement the relationships defined in the Survey and EmergencyContact models, enabling clean ORM-level navigation between related entities.
71-71: Improved consistency in secret key accessThis change aligns with how the secret key is accessed in the
encode_auth_tokenmethod (line 54), creating better consistency in the authentication implementation.migrations/versions/596e8c1b8988_.py (1)
21-24:Details
❓ Verification inconclusive
Verify handling of existing emergency contact records
The migration adds a non-nullable
user_idcolumn without a default value. This could cause the migration to fail if there are existing emergency contact records in the database.
🏁 Script executed:
#!/bin/bash # Check if there are existing records in the emergency_contacts table # that would need user_id assignments during migration # Use SQLite or PostgreSQL command based on your database echo "Checking for existing emergency contacts..." sqlite3 instance/test.db "SELECT COUNT(*) FROM emergency_contacts;" 2>/dev/null || \ psql -U postgres -d mind_matter -c "SELECT COUNT(*) FROM emergency_contacts;" 2>/dev/null # Also check the schema to confirm understanding echo "Current schema of emergency_contacts table:" sqlite3 instance/test.db ".schema emergency_contacts" 2>/dev/null || \ psql -U postgres -d mind_matter -c "\d emergency_contacts" 2>/dev/nullLength of output: 460
Verify existing emergency_contacts before adding NOT NULL user_id
This migration in migrations/versions/596e8c1b8988_.py (lines 21–24) adds a non-nullable
user_idcolumn without a default. If anyemergency_contactsrows already exist, the ALTER will fail.Points to address:
- Confirm that
emergency_contactsis empty prior to running this migration, OR- Change the new column to nullable, backfill
user_idvalues, then alter it to NOT NULL, OR- Add a
server_defaultforuser_id(and remove it after backfill)Please verify your existing data handling or adjust the migration accordingly.
mind_matter_api/models/emergency_contacts.py (1)
14-15: Verify reciprocal relationship onUsermodel
back_populates="emergency_contacts"assumesUser.emergency_contactsis defined.
Please double-check that side exists; otherwise SQLAlchemy will raise at startup.
If missing, add:emergency_contacts = db.relationship( "EmergencyContact", back_populates="user", cascade="all, delete-orphan", )mind_matter_api/utils/auth.py (1)
51-60: Edge cases in integer conversion
int(user_id)will raise ifuser_idis something unexpected (e.g., UUID).
If the application plans to migrate to UUIDs later, guard the conversion or compare as strings.- user_id_int = int(user_id) if isinstance(user_id, str) else user_id + try: + user_id_int = int(user_id) + except (TypeError, ValueError): + user_id_int = user_id # fall back to original value
| env_file: | ||
| - .env |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Add env_file to load environment variables for the dev service.
This directive ensures that variables declared in .env are injected into the mind-matter-flask-dev container. Please verify that:
- A
.envfile exists at the repository root. .envis listed in.gitignore(or equivalent) to avoid committing secrets.
Additionally, consider splitting environment files by context (e.g., .env.development, .env.production) and referencing them here to clearly separate dev vs. prod variables.
🏁 Script executed:
#!/bin/bash
# Verify .env file presence and git-ignoring
if [ ! -f .env ]; then
echo "ERROR: .env file is missing in the repo root"
exit 1
fi
if ! grep -q '^\.env$' .gitignore; then
echo "ERROR: .env is not listed in .gitignore"
exit 1
fi
echo "OK: .env exists and is git-ignored"Length of output: 129
Add a .env file for the env_file directive
The env_file: setting in docker-compose.yml (lines 19–20) will fail because no .env file exists at the repo root. Please:
- Create a root-level
.envfile with your development variables. - Add
.envto.gitignoreto prevent committing secrets. - (Optional) Split configs—e.g.
.env.development/.env.production—and updateenv_file:accordingly to keep environments separate.
🤖 Prompt for AI Agents
In docker-compose.yml at lines 19-20, the env_file directive references a .env
file that does not exist at the repository root. To fix this, create a .env file
at the root containing your development environment variables, ensure that .env
is added to .gitignore to prevent committing sensitive data, and optionally
consider splitting environment files by context (e.g., .env.development and
.env.production) and update the env_file path accordingly to separate
configurations for different environments.
| try: | ||
| resource = resource_getter(*args, **kwargs) | ||
|
|
||
| if not is_user_owner(user_id, resource) and not is_user_admin(user_id): | ||
| app.logger.debug(f"[require_owner] Permission denied for user {user_id}") | ||
| abort(403, description="Permission denied") | ||
|
|
||
| return f(user_id, resource, *args, **kwargs) | ||
| except Exception as e: | ||
| app.logger.error(f"[require_owner] Error checking ownership: {str(e)}") | ||
| abort(403, description="Error checking permissions") |
There was a problem hiding this comment.
Over-broad except hides real errors and returns misleading 403
except Exception swallows programming/DB errors (e.g., Sequelize session issues) and replies with “Permission denied” (403).
This makes debugging very hard and leaks no useful information to clients.
Recommended approach:
- Narrow the
try/exceptto only wrap the ownership checks. - Log and re-raise unexpected errors as a 500 instead of 403.
- try:
- resource = resource_getter(*args, **kwargs)
-
- if not is_user_owner(user_id, resource) and not is_user_admin(user_id):
- app.logger.debug(f"[require_owner] Permission denied for user {user_id}")
- abort(403, description="Permission denied")
-
- return f(user_id, resource, *args, **kwargs)
- except Exception as e:
- app.logger.error(f"[require_owner] Error checking ownership: {str(e)}")
- abort(403, description="Error checking permissions")
+ resource = None
+ try:
+ resource = resource_getter(*args, **kwargs)
+ except Exception as e:
+ app.logger.exception("[require_owner] Failed to load resource")
+ abort(500, description="Internal server error")
+
+ if not is_user_owner(user_id, resource) and not is_user_admin(user_id):
+ app.logger.debug(f"[require_owner] Permission denied for user {user_id}")
+ abort(403, description="Permission denied")
+
+ return f(user_id, resource, *args, **kwargs)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| try: | |
| resource = resource_getter(*args, **kwargs) | |
| if not is_user_owner(user_id, resource) and not is_user_admin(user_id): | |
| app.logger.debug(f"[require_owner] Permission denied for user {user_id}") | |
| abort(403, description="Permission denied") | |
| return f(user_id, resource, *args, **kwargs) | |
| except Exception as e: | |
| app.logger.error(f"[require_owner] Error checking ownership: {str(e)}") | |
| abort(403, description="Error checking permissions") | |
| resource = None | |
| try: | |
| resource = resource_getter(*args, **kwargs) | |
| except Exception as e: | |
| app.logger.exception("[require_owner] Failed to load resource") | |
| abort(500, description="Internal server error") | |
| if not is_user_owner(user_id, resource) and not is_user_admin(user_id): | |
| app.logger.debug(f"[require_owner] Permission denied for user {user_id}") | |
| abort(403, description="Permission denied") | |
| return f(user_id, resource, *args, **kwargs) |
🤖 Prompt for AI Agents
In mind_matter_api/utils/decorators.py around lines 28 to 38, the current
try/except block is too broad, catching all exceptions and returning a 403
error, which hides real errors and misleads clients. Refactor the code to wrap
only the ownership check calls (is_user_owner and is_user_admin) in the try
block. If an expected permission error occurs, abort with 403; for unexpected
exceptions, log the error and abort with a 500 status to indicate a server
error, allowing better debugging and clearer client feedback.
| from marshmallow import ValidationError | ||
| from mind_matter_api.schemas import EmergencyContactSchema | ||
| from mind_matter_api.services.emergency_contacts import EmergencyContactsService | ||
| from mind_matter_api.utils.decorators import require_auth, require_owner |
There was a problem hiding this comment.
Drop unused require_owner import
require_owner is imported but not used anywhere in the file. Eliminating it keeps the import list tidy and resolves the Ruff F401 warning.
-from mind_matter_api.utils.decorators import require_auth, require_owner
+from mind_matter_api.utils.decorators import require_auth📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| from mind_matter_api.utils.decorators import require_auth, require_owner | |
| -from mind_matter_api.utils.decorators import require_auth, require_owner | |
| +from mind_matter_api.utils.decorators import require_auth |
🧰 Tools
🪛 Ruff (0.8.2)
5-5: mind_matter_api.utils.decorators.require_owner imported but unused
Remove unused import: mind_matter_api.utils.decorators.require_owner
(F401)
🤖 Prompt for AI Agents
In mind_matter_api/api/emergency_contacts.py at line 5, the import statement
includes require_owner which is not used anywhere in the file. Remove
require_owner from the import statement to clean up unused imports and resolve
the Ruff F401 warning.
| @@ -0,0 +1,98 @@ | |||
| from flask import jsonify, request, current_app | |||
There was a problem hiding this comment.
Remove unused current_app import to satisfy linter and avoid dead code
current_app is never referenced in this module. Ruff already flags this (F401), so the PR will fail the lint stage until it is removed.
-from flask import jsonify, request, current_app
+from flask import jsonify, request📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| from flask import jsonify, request, current_app | |
| -from flask import jsonify, request, current_app | |
| +from flask import jsonify, request |
🧰 Tools
🪛 Ruff (0.8.2)
1-1: flask.current_app imported but unused
Remove unused import: flask.current_app
(F401)
🤖 Prompt for AI Agents
In mind_matter_api/api/emergency_contacts.py at line 1, the import statement
includes `current_app` which is not used anywhere in the file. Remove
`current_app` from the import list to satisfy the linter and avoid dead code,
ensuring the PR passes the lint stage.
| app.logger.error(f"Error retrieving emergency contacts: {str(e)}") | ||
| return jsonify({'error': str(e)}), 500 |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Return generic error message to avoid leaking internals
str(e) may expose stack-trace fragments or database information to clients. Return a generic message and log the full details server side.
- return jsonify({'error': str(e)}), 500
+ app.logger.exception("Error retrieving emergency contacts") # logs stack-trace
+ return jsonify({'error': 'Internal server error'}), 500🤖 Prompt for AI Agents
In mind_matter_api/api/emergency_contacts.py around lines 22 to 23, the current
code returns the full error message string to the client, which may expose
sensitive internal details. Modify the return statement to send a generic error
message like "Internal server error" to the client, while keeping the detailed
error logged on the server side using app.logger.error.
| # Get and validate the request data | ||
| request_data = request.get_json() | ||
|
|
||
| # Add user_id to request data before validation | ||
| request_data['user_id'] = user_id | ||
|
|
||
| # Validate the complete data and convert to dict | ||
| validated_data = EmergencyContactSchema().load(request_data) | ||
| contact_data = EmergencyContactSchema().dump(validated_data) |
There was a problem hiding this comment.
Handle missing / invalid JSON body early
request.get_json() can return None when the Content-Type header is wrong or the body is empty, causing a TypeError at request_data['user_id'].
-request_data = request.get_json()
+request_data = request.get_json(silent=True)
+if not isinstance(request_data, dict):
+ return jsonify({'error': 'JSON body required'}), 400This small guard improves robustness and user feedback.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Get and validate the request data | |
| request_data = request.get_json() | |
| # Add user_id to request data before validation | |
| request_data['user_id'] = user_id | |
| # Validate the complete data and convert to dict | |
| validated_data = EmergencyContactSchema().load(request_data) | |
| contact_data = EmergencyContactSchema().dump(validated_data) | |
| # Get and validate the request data | |
| request_data = request.get_json(silent=True) | |
| if not isinstance(request_data, dict): | |
| return jsonify({'error': 'JSON body required'}), 400 | |
| # Add user_id to request data before validation | |
| request_data['user_id'] = user_id | |
| # Validate the complete data and convert to dict | |
| validated_data = EmergencyContactSchema().load(request_data) | |
| contact_data = EmergencyContactSchema().dump(validated_data) |
🤖 Prompt for AI Agents
In mind_matter_api/api/emergency_contacts.py around lines 30 to 38, the code
assumes request.get_json() always returns a dictionary, but it can return None
if the JSON body is missing or invalid, causing a TypeError when accessing
request_data['user_id']. Add a check immediately after calling
request.get_json() to verify the result is not None; if it is, return an
appropriate error response indicating the JSON body is missing or invalid before
proceeding with adding user_id and validation.
| # Validate the data | ||
| data = EmergencyContactSchema().load(request_data, partial=True) | ||
| # Update the contact | ||
| updated = svc.update_emergency_contacts(contact_id, data) |
There was a problem hiding this comment.
Prevent privilege escalation by disallowing user_id updates
A caller could include "user_id": some_other_user in the JSON payload and transfer ownership. Strip the field before updating (or explicitly forbid it).
-data = EmergencyContactSchema().load(request_data, partial=True)
-if 'user_id' in data:
- del data['user_id']
+data = EmergencyContactSchema(exclude=('user_id',)).load(request_data, partial=True)Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In mind_matter_api/api/emergency_contacts.py around lines 68 to 71, the update
logic allows the user_id field to be updated from the request data, which can
lead to privilege escalation by transferring ownership. To fix this, explicitly
remove or ignore the user_id field from the validated data before calling the
update_emergency_contacts method, ensuring that user_id cannot be changed
through the update payload.
| if isinstance(contact_data, EmergencyContact): | ||
| contact_data = { | ||
| 'contact_name': contact_data.contact_name, | ||
| 'phone_number': contact_data.phone_number, | ||
| 'description': contact_data.description | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Guard against unintended field updates in update_emergency_contacts
The current implementation forwards any keys in contact_data directly to the repository.
Add an allow-list or strip dangerous fields (id, user_id) to prevent ownership or PK tampering.
🤖 Prompt for AI Agents
In mind_matter_api/services/emergency_contacts.py around lines 24 to 30, the
code converts an EmergencyContact object to a dictionary but then forwards all
keys in contact_data directly to the repository, risking unintended updates to
protected fields like id or user_id. To fix this, add an allow-list of safe
fields (e.g., contact_name, phone_number, description) or explicitly remove id
and user_id from contact_data before passing it on, ensuring these critical
fields cannot be tampered with during updates.
| def create_emergency_contacts(self, user_id: str, contact_data: Dict[str, Any]) -> EmergencyContact: | ||
| new_contact = EmergencyContact(**contact_data) | ||
| created_contact = self.emergency_contacts_repository.create(new_contact) | ||
| return created_contact | ||
|
|
There was a problem hiding this comment.
user_id parameter is ignored – possible data integrity bug
create_emergency_contacts accepts user_id but never uses it, trusting the caller-supplied contact_data.
If a malicious client injects a different user_id, they can create contacts under another user’s account.
-def create_emergency_contacts(self, user_id: str, contact_data: Dict[str, Any]) -> EmergencyContact:
- new_contact = EmergencyContact(**contact_data)
+def create_emergency_contacts(self, user_id: str, contact_data: Dict[str, Any]) -> EmergencyContact:
+ # Enforce correct ownership
+ contact_data['user_id'] = user_id
+ new_contact = EmergencyContact(**contact_data)
created_contact = self.emergency_contacts_repository.create(new_contact)
return created_contact🤖 Prompt for AI Agents
In mind_matter_api/services/emergency_contacts.py around lines 17 to 21, the
user_id parameter is accepted but not used, which risks data integrity by
allowing contact_data to specify a different user_id. To fix this, explicitly
set the user_id field of the new EmergencyContact instance to the provided
user_id argument, overriding any user_id in contact_data before creating the
contact. This ensures the contact is always associated with the correct user.
Description
Brief summary of the changes and any relevant context (design doc, screenshots of bugs...).
Fixes # (issue)
Changes
Testing
Describe tests run to verify changes. If manual testing has been done, provide instructions to reproduce.
Checklist:
Summary by CodeRabbit
New Features
Bug Fixes
Chores
.envfile.