Conversation
…-owned-by-tenant-superclass
…ding from database
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 17
🔭 Outside diff range comments (3)
fleet_management_api/database/db_models.py (1)
423-437: Add transaction handling and improve error handling in _use_ref_tenant_id.The function should handle potential race conditions and provide more detailed error messages.
def _use_ref_tenant_id(mapper: Base, connection, target, ref_id: str, ref_base: type[Base]): session = SessionWithTenants.object_session(target) if session: - ref = session.query(ref_base).filter_by(id=getattr(target, ref_id)).one() + try: + ref = session.query(ref_base).filter_by(id=getattr(target, ref_id)).with_for_update().one() + except NoResultFound: + raise ValueError(f"{ref_base.__name__} with ID {getattr(target, ref_id)} not found") + except MultipleResultsFound: + raise ValueError(f"Multiple {ref_base.__name__} found with ID {getattr(target, ref_id)}") assert hasattr(ref, "tenant_id"), f"Reference {ref_base} does not have tenant_id." tenant_id = ref.tenant_id - tenant = session.query(TenantDB).filter_by(id=tenant_id).one() + try: + tenant = session.query(TenantDB).filter_by(id=tenant_id).with_for_update().one() + except NoResultFound: + raise ValueError(f"Tenant with ID {tenant_id} not found") if tenant: if session.tenants.is_accessible(tenant.name): target.tenant_id = ref.tenant_id else: raise _TenantNotAccessible(f"Tenant '{tenant.name}' is not accessible.") - else: - raise ValueError(f"Tenant with ID {tenant_id} not found.")fleet_management_api/api_impl/controllers/order_state.py (1)
255-261: Fix potential tenant data leak in delete operation.The
delete_ncall doesn't use the tenant parameter, which could lead to unauthorized deletion of order states from other tenants.response = _db_access.delete_n( + tenants, _db_models.OrderStateDB, n=delta, column_name="timestamp", start_from="minimum", criteria={"order_id": lambda x: x == order_id}, )tests/controllers/order/test_get_order_state.py (1)
291-321: Add tenant isolation tests for order states.Verify that order states cannot be accessed across tenant boundaries.
def test_order_states_are_isolated_between_tenants(self): with self.app.app.test_client() as client: # Create order in tenant_1 client.set_cookie("", "tenant", "tenant_1") response = client.post("/v2/management/order", json=[Order(car_id=1, target_stop_id=1, stop_route_id=1)]) order_id = response.json[0]["id"] # Post state in tenant_1 state = OrderState(order_id=order_id, status="in_progress") client.post("/v2/management/orderstate", json=[state]) # Attempt to access from tenant_2 client.set_cookie("", "tenant", "tenant_2") response = client.get(f"/v2/management/orderstate/{order_id}") self.assertEqual(response.status_code, 404)
♻️ Duplicate comments (1)
fleet_management_api/controllers/security_controller.py (1)
26-28:⚠️ Potential issueImprove exception handling in JWT validation.
The broad
except Exceptionclause catches all exceptions without distinguishing between different JWT-related errors.Replace with specific exception handling:
try: decoded_token = jwt.decode( token, get_public_key(), algorithms=["RS256"], audience="account" ) - except Exception as e: - log_warning(f"Failed to decode JWT token: {str(e)}") + except jwt.InvalidTokenError as e: + log_warning(f"Invalid token: {str(e)}") + return None + except jwt.InvalidKeyError as e: + log_warning(f"Invalid key configuration: {str(e)}") + return None + except Exception as e: + log_warning(f"Unexpected error during JWT validation: {str(e)}") return None
🧹 Nitpick comments (48)
fleet_management_api/api_impl/obj_to_db.py (1)
33-36: Keep the if-else blocks for better readability and maintainability.While static analysis suggests using ternary operators, the current if-else blocks are:
- More readable for complex conditions
- Consistent across the codebase
- Easier to extend with additional logic
If you still want to use ternary operators, here's how the changes would look:
- if car_state.position is None: - car_position = None - else: - car_position = car_state.position.to_dict() + car_position = None if car_state.position is None else car_state.position.to_dict()Also applies to: 52-55, 94-97, 213-216
🧰 Tools
🪛 Ruff (0.8.2)
33-36: Use ternary operator
car_position = None if car_state.position is None else car_state.position.to_dict()instead ofif-else-blockReplace
if-else-block withcar_position = None if car_state.position is None else car_state.position.to_dict()(SIM108)
openapi/tenant.yaml (5)
5-27: Review: HEAD Tenant Cookie Endpoint
The HEAD operation for/tenant/{tenantId}is well-structured with a clearoperationIdand controller mapping. One suggestion is to explicitly document theSet-Cookieheader in the "200" response (for example, by including a header definition), which can help API consumers understand the expected response structure.
14-14: Typo and Clarity in Summary
There is a typo ("tenand" should be "tenant"). Consider rephrasing the summary for clarity, for example:
"Send a response with a Set-Cookie header that sets the cookie value to the tenant name corresponding to the provided tenantId."
29-51: Review: GET Tenants Endpoint
The GET endpoint for/tenantis clearly defined and returns a comprehensive JSON response. For improved consistency and clarity, consider pluralizing the description (e.g., "Find and return all existing Tenants"). Additionally, if the number of tenants can become large, you might consider pagination or filtering options.
52-77: Review: DELETE Tenant Endpoint
The DELETE endpoint is correctly defined with appropriate parameters and response codes. For consistency with other endpoints, consider returning a JSON response (even if it’s a simple confirmation message) instead of plain text.
79-94: Review: Tenant Schema Definition
TheTenantschema is well-defined and clearly documents the requirednameproperty along with an optionalid. To further enhance data integrity, consider adding validation constraints (such as maximum length or a regex pattern) for thenameproperty. Also, double-check whetheridshould be mandated based on your data model design.fleet_management_api/database/db_access.py (4)
2-2: Remove unused Protocol import.The
Protocoltype is imported but never used in the code.Apply this diff to remove the unused import:
-from typing import Any, Optional, Literal, Callable, Iterable, ParamSpec, TypeVar, Protocol +from typing import Any, Optional, Literal, Callable, Iterable, ParamSpec, TypeVar🧰 Tools
🪛 Ruff (0.8.2)
2-2:
typing.Protocolimported but unusedRemove unused import:
typing.Protocol(F401)
587-589: Consider using getattr instead of dict access.Direct access to
__dict__can be unsafe as it bypasses property descriptors and may not work with all object types. Consider usinggetattrfor safer attribute access.Apply this diff to use
getattr:- return hasattr(item, item_attr_name) and func(item.__dict__[item_attr_name]) + return hasattr(item, item_attr_name) and func(getattr(item, item_attr_name))
431-431: Optimize dictionary key iteration.Using
.keys()is unnecessary when checking for key existence.Apply this diff to optimize the code:
- return [criteria[name](getattr(base.__table__.columns, name)) for name in criteria.keys()] + return [criteria[name](getattr(base.__table__.columns, name)) for name in criteria]🧰 Tools
🪛 Ruff (0.8.2)
431-431: Use
key in dictinstead ofkey in dict.keys()Remove
.keys()(SIM118)
489-489: Remove unused exception variable.The exception variable
eis assigned but never used.Apply this diff to remove the unused variable:
- except _sqaexc.NoResultFound as e: + except _sqaexc.NoResultFound:🧰 Tools
🪛 Ruff (0.8.2)
489-489: Local variable
eis assigned to but never usedRemove assignment to unused variable
e(F841)
fleet_management_api/database/db_models.py (4)
53-66: Add validation for tenant names in the Tenants protocol.The protocol should specify constraints on valid tenant names (e.g., allowed characters, length limits) to ensure consistent validation across implementations.
class Tenants(Protocol): """Protocol for a class that provides the current tenant and all accessible tenants.""" @property - def current(self) -> str: ... + def current(self) -> str: + """Returns the current tenant name. + + Returns: + str: A non-empty string containing only alphanumeric characters and hyphens, + with a maximum length of 63 characters. + """ + ...
38-51: Consider adding tenant validation in SessionWithTenants.The session should validate the provided
tenantsinstance to ensure it implements theTenantsprotocol correctly.def __init__(self, *args, tenants: Tenants, **kwargs): + if not isinstance(tenants, Tenants): + raise TypeError("tenants must implement the Tenants protocol") super().__init__(*args, **kwargs) self._tenants = tenants
90-118: Add indexes to improve query performance in TenantDB.Consider adding indexes for frequently queried columns to improve performance.
class TenantDB(Base): """ORM-mapped class representing a tenant in the database.""" model_name = "Tenant" __tablename__ = "tenants" + __table_args__ = ( + Index('ix_tenants_name', 'name'), # Improve lookup by name + ) name: Mapped[str] = mapped_column(String, unique=True)
246-270: Optimize relationships in OrderDB for common queries.The relationships could be configured more efficiently based on common access patterns.
- states: Mapped[list[OrderStateDB]] = relationship( - "OrderStateDB", cascade=CASCADE, back_populates="order" - ) + states: Mapped[list[OrderStateDB]] = relationship( + "OrderStateDB", + cascade=CASCADE, + back_populates="order", + order_by="desc(OrderStateDB.timestamp)", # Latest states first + lazy="select" + )fleet_management_api/api_impl/controllers/order_state.py (2)
68-82: Consider optimizing tenant access verification.The current implementation makes two separate database queries to check order accessibility. While the logic is correct, it could be optimized.
Consider consolidating the queries:
- unrestricted = _AccessibleTenants.from_dict({"current": "", "all": []}) - all_orders = _existing_orders(unrestricted, *order_ids) - accessible_orders = _existing_orders(tenants, *order_ids) - - all_order_ids = {order.id for order in all_orders.values() if order is not None} - accessible_order_ids = {order.id for order in accessible_orders.values() if order is not None} - - inaccessible_order_ids = all_order_ids - accessible_order_ids + orders = _existing_orders(tenants, *order_ids, include_inaccessible=True) + inaccessible_order_ids = { + id_ for id_, (order, accessible) in orders.items() + if order is not None and not accessible + }
88-90: Remove redundant assertions.The assertions after the null check are redundant since we've already confirmed the order exists.
- assert order is not None, "Order is None" - assert order.id is not None, "Order ID is None." - order_id: int = order.id + order_id: int = order.id # order is guaranteed to be non-None heretests/_utils/setup_utils.py (2)
21-33: Extract common test configuration values.The hardcoded "localhost" domain is used across multiple functions. Consider extracting it to a constant at the module level.
+TEST_DOMAIN = "localhost" + def create_platform_hws( app: TestApp, count: int = 1, tenant: str = TEST_TENANT_NAME, api_key: str = "" ) -> None: with app.app.test_client(tenant) as c: - c.set_cookie("localhost", "tenant", tenant) + c.set_cookie(TEST_DOMAIN, "tenant", tenant)Also, enhance the error message to include more context:
- assert response.status_code == 200, f"Failed to create platform HW: {response.json}" + assert response.status_code == 200, f"Failed to create platform HW (tenant: {tenant}): {response.json}"
46-49: Enhance error handling for stop creation.The response validation could be more robust by checking the response structure before accessing indices.
- assert response.status_code == 200, f"Failed to create stop: {response.json}" - assert response.json is not None - ids.append(response.json[0]["id"]) + assert response.status_code == 200, f"Failed to create stop (tenant: {tenant}): {response.json}" + assert response.json is not None, "Empty response received" + assert len(response.json) > 0, "No stops created in response" + assert "id" in response.json[0], "Stop ID missing in response" + ids.append(response.json[0]["id"])tests/client/test_client_authorization_and_tenant.py (3)
26-28: Remove unused helper function.The
stop_appfunction appears to be unused in the test suite and can be safely removed.-def stop_app(): - raise RuntimeError("Stopping the app") -
32-33: Consider using exponential backoff for retry intervals.Using a fixed retry interval might not be optimal. Consider implementing exponential backoff to handle different server startup scenarios more efficiently.
- retry_interval = 1 - start = time.time() + retry_interval = 0.1 # Start with a shorter interval + max_interval = 2 # Cap the maximum interval + start = time.time()Then update the retry logic:
except Exception as e: print(f"Server not ready: {e}") time.sleep(retry_interval) + retry_interval = min(retry_interval * 2, max_interval) # Exponential backoff
65-80: LGTM! Comprehensive test coverage for authorization scenarios.The test methods effectively validate:
- API key + tenant cookie authentication
- JWT token + tenant cookie authentication
- Unauthorized access handling
Consider adding these test cases to improve coverage:
- Invalid tenant cookie
- Missing tenant cookie
- Expired JWT token
- Invalid API key
Example test case:
def test_using_invalid_tenant_cookie_raises_exception(self): client = ApiClient( configuration=Configuration( host="http://localhost:8080/v2/management", api_key={"APIKeyAuth": "APIKey"}, ), cookie="tenant=invalid-tenant", # Invalid tenant ) platform_hw_api = PlatformHWApi(client) with self.assertRaises(UnauthorizedException): platform_hw_api.create_hws(platform_hw=[PlatformHWFor(name="test_hw")])Also applies to: 81-94, 95-104
fleet_management_api/api_impl/controllers/platform_hw.py (3)
53-64: Update docstring to reflect tenant requirements.Similar to
create_hws(), the docstring should document tenant-related behavior.def get_hws() -> _Response: - """Get all existing platform HWs.""" + """Get all existing platform HWs for the accessible tenant. + + Returns only the platform HWs that belong to the tenant specified in the request + and that the user has access to. + """
67-85: Update docstring to reflect tenant requirements.The docstring should be updated to clarify tenant-specific behavior.
def get_hw(platform_hw_id: int) -> _Response: - """Get an existing platform HW identified by 'platformhw_id'.""" + """Get an existing platform HW identified by 'platformhw_id'. + + Returns the platform HW only if it belongs to the tenant specified in the request + and the user has access to that tenant. + """
1-114: Consider implementing tenant validation as middleware.The current implementation extracts and validates tenant information in each endpoint. Consider implementing this as middleware to:
- Reduce code duplication
- Ensure consistent tenant validation across all endpoints
- Separate concerns between request handling and tenant validation
- Make it easier to modify tenant validation logic in one place
This would also align better with the PR's objective of implementing comprehensive tenant management.
fleet_management_api/api_impl/controllers/route.py (1)
193-209: Optimize stop existence checking.The current implementation makes multiple database calls for each stop ID. Consider fetching all stops once and checking existence in memory.
def _find_nonexistent_stops(tenants: _AccessibleTenants, *routes: _Route) -> _Response: + # Fetch all stops once + existing_ids = set(stop.id for stop in _db_access.get(tenants, _StopDB)) + for route in routes: checked_id_set: set[int] = set(route.stop_ids) - for id_ in checked_id_set: - foo = lambda x, id_: x == id_ - _db_access.exists(tenants, _StopDB, criteria={"id": partial(foo, id_=id_)}) - existing_ids = set([stop_id.id for stop_id in _db_access.get(tenants, _StopDB)]) nonexistent_stop_ids = checked_id_set.difference(existing_ids) if nonexistent_stop_ids: return _error(🧰 Tools
🪛 Ruff (0.8.2)
197-197: Do not assign a
lambdaexpression, use adefRewrite
fooas adef(E731)
fleet_management_api/openapi/openapi.yaml (1)
3656-3674: New Tenant Schema Added
A newTenantschema has been added to define the data structure for tenant objects. This is essential for supporting multi-tenancy. Consider whether additional tenant attributes (e.g., contact info or status) might be needed in future revisions.tests/database/test_db_operations.py (3)
2-4: Consider using absolute imports.The use of
sys.path.append(".")to modify the Python path at runtime is not a recommended practice. It can lead to import conflicts and make the code less maintainable.Consider using either:
- Python's
-mflag to run tests- Setting PYTHONPATH environment variable
- Using
setuptoolsfor development installation-import sys - -sys.path.append(".")
298-298: Consider using a test runner configuration file.Setting verbosity in code makes it harder to configure test execution. Consider using a pytest.ini or setup.cfg file instead.
Create a
pytest.inifile:[pytest] verbosity = 2Then update the code:
- unittest.main(verbosity=2) # pragma: no cover + unittest.main() # pragma: no cover
36-44: Add test case for concurrent tenant operations.While basic tenant operations are tested, there's no coverage for concurrent operations that could potentially lead to race conditions.
Add a test case for concurrent operations:
def test_concurrent_tenant_operations(self): test_obj_1 = models.TestItem(test_str="test_string", test_int=5, id=0) test_obj_2 = models.TestItem(test_str="test_string", test_int=8, id=1) # Simulate concurrent operations from different tenants tenant2 = TenantFromTokenMock("another_tenant") _db_access.add_without_tenant(TenantDB(name="another_tenant")) # Add same records to different tenants _db_access.add(self.tenant, test_obj_1) _db_access.add(tenant2, test_obj_2) # Verify tenant isolation tenant1_records = _db_access.get(tenants=self.tenant, base=models.TestItem) tenant2_records = _db_access.get(tenants=tenant2, base=models.TestItem) self.assertEqual(len(tenant1_records), 1) self.assertEqual(len(tenant2_records), 1) self.assertEqual(tenant1_records[0].test_int, test_obj_1.test_int) self.assertEqual(tenant2_records[0].test_int, test_obj_2.test_int)fleet_management_api/api_impl/controllers/stop.py (2)
36-54: Consider enhancing error logging with tenant context.The tenant-aware implementation looks good, but error messages could be more helpful for debugging in a multi-tenant environment.
Consider adding tenant context to the error message:
- f"Stops (name='{[s.name for s in stop_db_models]})' could not be created. {response.body['detail']}", + f"Stops (name='{[s.name for s in stop_db_models]})', tenant='{tenants.current}' could not be created. {response.body['detail']}",
1-158: Consider implementing a centralized tenant-aware error handling mechanism.While the tenant integration is well done, consider creating a centralized error handling mechanism that automatically sanitizes error messages in a multi-tenant environment. This would:
- Ensure consistent error handling across all endpoints
- Prevent accidental exposure of cross-tenant information
- Reduce code duplication in error handling logic
Consider creating a new module
tenant_aware_error_handler.pythat provides tenant-aware versions of_error,_log_error_and_respond, etc. These functions would automatically sanitize error messages based on the tenant context.fleet_management_api/api_impl/load_request.py (1)
19-37: Consider broadening exception handling inloadmethod.
Currently, onlyRuntimeErroris caught for extracting theAuthorizationheader. If other unexpected issues arise when accessing request attributes, they may go unhandled. Consider adding broader exception handling or verifying if the known exceptions are properly captured.fleet_management_api/controllers/tenant_controller.py (1)
1-8: Unused imports flagged by static analysis.
As this file is auto-generated, manual edits may be overwritten. If possible, update the OpenAPI generator settings to avoid generating unused imports.🧰 Tools
🪛 Ruff (0.8.2)
1-1:
connexionimported but unusedRemove unused import:
connexion(F401)
2-2:
typing.Dictimported but unusedRemove unused import:
typing.Dict(F401)
3-3:
typing.Tupleimported but unusedRemove unused import:
typing.Tuple(F401)
4-4:
typing.Unionimported but unusedRemove unused import:
typing.Union(F401)
6-6:
fleet_management_api.models.error.Errorimported but unusedRemove unused import:
fleet_management_api.models.error.Error(F401)
7-7:
fleet_management_api.models.tenant.Tenantimported but unusedRemove unused import:
fleet_management_api.models.tenant.Tenant(F401)
8-8:
fleet_management_api.utilimported but unusedRemove unused import:
fleet_management_api.util(F401)
fleet_management_api/api_impl/controllers/car_state.py (3)
32-37: Improve error handling in response construction.The error handling could be more robust by providing specific error messages for different failure scenarios.
Apply this diff to enhance error handling:
request = _RequestJSON.load() if not request: return _log_invalid_request_body_format() tenants = _AccessibleTenants(request) -car_states = [_CarState.from_dict(s) for s in request.data] # noqa: E501 -return create_car_states_from_argument_and_post(tenants, car_states) +try: + car_states = [_CarState.from_dict(s) for s in request.data] # noqa: E501 + return create_car_states_from_argument_and_post(tenants, car_states) +except ValueError as e: + return _log_error_and_respond( + f"Invalid car state data format: {str(e)}", + 400, + "Invalid Request" + )
63-71: Improve cleanup response error handling.The cleanup error message is being appended to the success message without proper formatting, which could lead to confusing error messages.
Apply this diff to improve the error message formatting:
cleanup_response = _remove_old_states(tenants, model.car_id) if cleanup_response.status_code != 200: code, cleanup_error_msg = ( cleanup_response.status_code, cleanup_response.body, ) _log_error(cleanup_error_msg) - msg = msg + "\n" + cleanup_error_msg + msg = f"{msg}\nCleanup failed: {cleanup_error_msg}" title = "Could not delete object"
84-86: Add input validation for query parameters.The function accepts query parameters without validation, which could lead to unexpected behavior.
Apply this diff to add input validation:
def get_all_car_states( since: int = 0, wait: bool = False, last_n: int = 0, tenant: str = "" ) -> _Response: + if since < 0: + return _log_error_and_respond( + "'since' parameter must be non-negative", + 400, + "Invalid Parameter" + ) + if last_n < 0: + return _log_error_and_respond( + "'last_n' parameter must be non-negative", + 400, + "Invalid Parameter" + )fleet_management_api/api_impl/tenants.py (2)
16-43: Enhance exception messages with more context.The exception classes could provide more descriptive error messages to help with debugging.
Apply this diff to improve exception messages:
class UsingEmptyTenant(Exception): """Raise when trying use an empty tenant.""" - pass + def __init__(self, operation: str = ""): + super().__init__(f"Cannot {operation} with empty tenant") class TenantNotAccessible(Exception): """Raise when the tenant is not accessible.""" - pass + def __init__(self, tenant: str): + super().__init__(f"Tenant '{tenant}' is not accessible") class NoAccessibleTenants(Exception): """Raise when no accessible tenants are found in a JWT token.""" - pass + def __init__(self, details: str = ""): + super().__init__(f"No accessible tenants found. {details}") class NoHeaderWithJWTToken(Exception): """Raise when no header with a JWT token is found in a request.""" - pass + def __init__(self): + super().__init__("Authorization header with JWT token is missing") class MissingRSAKey(Exception): """Raise when a RSA key (either public or private) is not set.""" - pass + def __init__(self, key_type: str = ""): + super().__init__(f"Missing {key_type} RSA key")
155-158: Simplify code using ternary operator.The if-else block can be simplified using a ternary operator for better readability.
Apply this diff to simplify the code:
- if ignore_cookie: - current_tenant = "" - else: - current_tenant = _get_current_tenant(request) + current_tenant = "" if ignore_cookie else _get_current_tenant(request)🧰 Tools
🪛 Ruff (0.8.2)
155-158: Use ternary operator
current_tenant = "" if ignore_cookie else _get_current_tenant(request)instead ofif-else-blockReplace
if-else-block withcurrent_tenant = "" if ignore_cookie else _get_current_tenant(request)(SIM108)
fleet_management_api/api_impl/controllers/car.py (1)
78-79: Remove useless attribute access.The result of
_post_default_car_action_stateis accessed but not used.Apply this diff to fix the issue:
car_states = _post_default_car_state(tenants, ids).body -_post_default_car_action_state(tenants, ids).body +_post_default_car_action_state(tenants, ids)🧰 Tools
🪛 Ruff (0.8.2)
79-79: Found useless attribute access. Either assign it to a variable or remove it.
(B018)
fleet_management_api/api_impl/controllers/car_action.py (1)
33-35: Add input validation for query parameters.The function accepts query parameters without validation, which could lead to unexpected behavior.
Apply this diff to add input validation:
def get_car_action_states( car_id: int, since: int = 0, wait: bool = False, last_n: int = 0, tenant: str = "" ) -> _Response: + if car_id <= 0: + return _log_error_and_respond( + "'car_id' must be positive", + 400, + "Invalid Parameter" + ) + if since < 0: + return _log_error_and_respond( + "'since' parameter must be non-negative", + 400, + "Invalid Parameter" + ) + if last_n < 0: + return _log_error_and_respond( + "'last_n' parameter must be non-negative", + 400, + "Invalid Parameter" + )tests/security/test_tenants.py (3)
34-39: Enhance error handling intenants_from_token.The error handling could be more informative by including the audience in the error message and logging the error before re-raising.
def tenants_from_token(request: ConnexionRequest, key: str, audience: str) -> AccessibleTenants: try: return AccessibleTenants(request, key, audience) except jwt.exceptions.InvalidKeyError: - print(f"Could not parse key:\n{key}") + import logging + logging.error(f"Could not parse key for audience '{audience}':\n{key}") raise
42-89: Consider adding edge cases to RSA key tests.The RSA key tests could be enhanced with additional scenarios:
- Test with malformed RSA keys
- Test with expired keys
- Test with keys using different algorithms
Example test case:
def test_malformed_rsa_key_raises_error(self): generate_test_keys() malformed_key = "malformed-key-data" set_auth_params(malformed_key, "test_client") with self.assertRaises(jwt.exceptions.InvalidKeyError): AccessibleTenants( ConnexionRequest( TEST_URL, method="GET", headers={"Authorization": testing_auth_header("tenant_x")}, ), malformed_key, "account", )
91-165: Enhance tenant cookie test assertions.The tenant cookie tests could benefit from additional assertions to verify the error messages and ensure proper tenant isolation.
Example enhancement:
def test_tenant_in_cookies_not_matching_tenants_from_jwt_raises_exception(self): request = ConnexionRequest( TEST_URL, method="GET", cookies={"tenant": "inaccessible_tenant"}, headers={ "Authorization": testing_auth_header("test_tenant"), }, ) with self.assertRaises(NoAccessibleTenants): AccessibleTenants(request, get_test_public_key(), "account") + with self.assertRaisesRegex(NoAccessibleTenants, "Tenant 'inaccessible_tenant' is not accessible"): + AccessibleTenants(request, get_test_public_key(), "account")tests/controllers/car/test_car_controller.py (1)
27-34: Optimize tenant test setup.The test setup could be more efficient by creating a base test class that handles common tenant setup operations.
class TenantTestCase(api_test.TestCase): def setUp(self, *args) -> None: super().setUp() self.app = _app.get_test_app(use_previous=True) self.app.app.def_accessible_tenants(TEST_TENANT_NAME) self._setup_platform_hw() def _setup_platform_hw(self): create_platform_hws(self.app, 2)tests/controllers/order/test_get_order_state.py (1)
68-74: Simplify nested context managers.Use a single
withstatement with multiple contexts instead of nestedwithstatements.- with self.app.app.test_client(TEST_TENANT_NAME) as c: - with ThreadPoolExecutor(max_workers=4) as executor: + with self.app.app.test_client(TEST_TENANT_NAME) as c, ThreadPoolExecutor(max_workers=4) as executor:🧰 Tools
🪛 Ruff (0.8.2)
68-69: Use a single
withstatement with multiple contexts instead of nestedwithstatements(SIM117)
tests/controllers/car/test_car_state_controller.py (1)
499-574: Enhance tenant cookie test validations.The tenant cookie tests could benefit from additional assertions and error message validation.
def test_posting_car_state_is_not_allowed_if_car_is_owned_by_inaccessible_tenant(self): state = CarState(status="idle", car_id=5) with self.app.app.test_client() as c: c.set_cookie("", "tenant", "") response = c.post( "/v2/management/carstate", headers={"Authorization": f"Bearer {_app.get_token('tenant_A', 'tenant_B')}"}, json=[state], ) self.assertEqual(response.status_code, 401) + self.assertIn("not accessible", response.json["message"]) + + # Verify state was not created + response = c.get("/v2/management/carstate/5") + self.assertEqual(response.status_code, 404)tests/controllers/car/test_car_action_controller.py (1)
495-561: Improve test method names for clarity.The test methods thoroughly verify tenant access control, but their names could be more descriptive:
Apply this diff to improve method names:
- def test_pausing_car_is_allowed_without_cookie_set_if_car_owned_by_accessible_tenant( + def test_pause_car_allowed_for_accessible_tenant_without_cookie( self, ) -> None: - def test_posting_car_state_to_tenant_is_not_allowed_if_car_is_owned_by_inaccessible_tenant( + def test_pause_car_denied_for_inaccessible_tenant_without_cookie( self, ) -> None:tests/controllers/order/test_order_state_controller.py (1)
730-730: Remove debug print statement.The print statement appears to be left over from debugging.
Apply this diff to remove the debug print:
- print(c.get("/v2/management/order?test_").json)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (51)
config/config.json(1 hunks)fleet_management_api/api_impl/api_keys.py(3 hunks)fleet_management_api/api_impl/api_logging.py(1 hunks)fleet_management_api/api_impl/controllers/car.py(6 hunks)fleet_management_api/api_impl/controllers/car_action.py(10 hunks)fleet_management_api/api_impl/controllers/car_state.py(8 hunks)fleet_management_api/api_impl/controllers/order.py(11 hunks)fleet_management_api/api_impl/controllers/order_state.py(8 hunks)fleet_management_api/api_impl/controllers/platform_hw.py(5 hunks)fleet_management_api/api_impl/controllers/route.py(8 hunks)fleet_management_api/api_impl/controllers/route_visualization.py(3 hunks)fleet_management_api/api_impl/controllers/stop.py(5 hunks)fleet_management_api/api_impl/controllers/tenant.py(1 hunks)fleet_management_api/api_impl/load_request.py(1 hunks)fleet_management_api/api_impl/obj_to_db.py(15 hunks)fleet_management_api/api_impl/tenants.py(1 hunks)fleet_management_api/app.py(1 hunks)fleet_management_api/controllers/car_action_controller.py(3 hunks)fleet_management_api/controllers/car_controller.py(2 hunks)fleet_management_api/controllers/car_state_controller.py(1 hunks)fleet_management_api/controllers/order_controller.py(3 hunks)fleet_management_api/controllers/order_state_controller.py(1 hunks)fleet_management_api/controllers/platform_hw_controller.py(2 hunks)fleet_management_api/controllers/security_controller.py(3 hunks)fleet_management_api/controllers/stop_controller.py(2 hunks)fleet_management_api/controllers/tenant_controller.py(1 hunks)fleet_management_api/database/db_access.py(16 hunks)fleet_management_api/database/db_models.py(5 hunks)fleet_management_api/openapi/openapi.yaml(22 hunks)openapi/car.yaml(11 hunks)openapi/check_api.yaml(1 hunks)openapi/common_models.yaml(3 hunks)openapi/order.yaml(8 hunks)openapi/platform_hw.yaml(3 hunks)openapi/route.yaml(7 hunks)openapi/stop.yaml(4 hunks)openapi/tenant.yaml(1 hunks)scripts/add_api_key.py(2 hunks)tests/_utils/setup_utils.py(1 hunks)tests/client/test_client_authorization_and_tenant.py(1 hunks)tests/controllers/car/test_car_action_controller.py(16 hunks)tests/controllers/car/test_car_controller.py(16 hunks)tests/controllers/car/test_car_state_controller.py(21 hunks)tests/controllers/order/test_get_order_state.py(18 hunks)tests/controllers/order/test_order_state_controller.py(29 hunks)tests/controllers/tenant/test_tenant.py(1 hunks)tests/controllers/test_api_checking.py(1 hunks)tests/controllers/test_db_models.py(5 hunks)tests/database/test_db_operations.py(2 hunks)tests/database/test_wait_for_content.py(4 hunks)tests/security/test_tenants.py(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- config/config.json
- openapi/check_api.yaml
🚧 Files skipped from review as they are similar to previous changes (18)
- fleet_management_api/controllers/car_controller.py
- fleet_management_api/controllers/car_action_controller.py
- fleet_management_api/controllers/platform_hw_controller.py
- scripts/add_api_key.py
- fleet_management_api/controllers/stop_controller.py
- fleet_management_api/controllers/order_state_controller.py
- fleet_management_api/controllers/car_state_controller.py
- tests/controllers/test_api_checking.py
- openapi/route.yaml
- fleet_management_api/controllers/order_controller.py
- fleet_management_api/api_impl/controllers/route_visualization.py
- openapi/common_models.yaml
- openapi/stop.yaml
- openapi/car.yaml
- openapi/order.yaml
- openapi/platform_hw.yaml
- tests/controllers/test_db_models.py
- fleet_management_api/api_impl/controllers/order.py
🧰 Additional context used
📓 Learnings (1)
tests/controllers/car/test_car_state_controller.py (1)
Learnt from: jiristrouhal
PR: bringauto/fleet-management-http-api#27
File: tests/controllers/car/test_get_last_car_state.py:25-25
Timestamp: 2025-01-31T13:35:03.741Z
Learning: The fleet-management-http-api codebase has comprehensive tenant isolation testing across multiple test files:
1. Security tests in tests/security/test_tenants.py
2. Database tests in tests/database/test_using_tenants.py
3. Car-specific tenant isolation in tests/controllers/car/test_car_controller.py
4. API endpoint tests in tests/controllers/tenant/test_tenant.py
🪛 Ruff (0.8.2)
tests/client/test_client_authorization_and_tenant.py
44-44: Do not use bare except
(E722)
tests/controllers/order/test_get_order_state.py
68-69: Use a single with statement with multiple contexts instead of nested with statements
(SIM117)
117-118: Use a single with statement with multiple contexts instead of nested with statements
(SIM117)
362-363: Use a single with statement with multiple contexts instead of nested with statements
(SIM117)
fleet_management_api/controllers/tenant_controller.py
1-1: connexion imported but unused
Remove unused import: connexion
(F401)
2-2: typing.Dict imported but unused
Remove unused import: typing.Dict
(F401)
3-3: typing.Tuple imported but unused
Remove unused import: typing.Tuple
(F401)
4-4: typing.Union imported but unused
Remove unused import: typing.Union
(F401)
6-6: fleet_management_api.models.error.Error imported but unused
Remove unused import: fleet_management_api.models.error.Error
(F401)
7-7: fleet_management_api.models.tenant.Tenant imported but unused
Remove unused import: fleet_management_api.models.tenant.Tenant
(F401)
8-8: fleet_management_api.util imported but unused
Remove unused import: fleet_management_api.util
(F401)
fleet_management_api/app.py
166-166: Do not use bare except
(E722)
fleet_management_api/api_impl/tenants.py
155-158: Use ternary operator current_tenant = "" if ignore_cookie else _get_current_tenant(request) instead of if-else-block
Replace if-else-block with current_tenant = "" if ignore_cookie else _get_current_tenant(request)
(SIM108)
201-203: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
fleet_management_api/api_impl/controllers/route.py
197-197: Do not assign a lambda expression, use a def
Rewrite foo as a def
(E731)
fleet_management_api/api_impl/controllers/car.py
79-79: Found useless attribute access. Either assign it to a variable or remove it.
(B018)
tests/controllers/car/test_car_action_controller.py
294-295: Use a single with statement with multiple contexts instead of nested with statements
(SIM117)
fleet_management_api/api_impl/obj_to_db.py
33-36: Use ternary operator car_position = None if car_state.position is None else car_state.position.to_dict() instead of if-else-block
Replace if-else-block with car_position = None if car_state.position is None else car_state.position.to_dict()
(SIM108)
fleet_management_api/database/db_access.py
2-2: typing.Protocol imported but unused
Remove unused import: typing.Protocol
(F401)
431-431: Use key in dict instead of key in dict.keys()
Remove .keys()
(SIM118)
489-489: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
🔇 Additional comments (56)
fleet_management_api/api_impl/obj_to_db.py (2)
6-223: LGTM! Consistent return type updates across conversion functions.The changes to return types from *DBModel to *DB are applied consistently across all conversion functions while maintaining the existing logic and patterns for handling optional fields.
🧰 Tools
🪛 Ruff (0.8.2)
33-36: Use ternary operator
car_position = None if car_state.position is None else car_state.position.to_dict()instead ofif-else-blockReplace
if-else-block withcar_position = None if car_state.position is None else car_state.position.to_dict()(SIM108)
226-235: LGTM! Clean tenant model conversion implementation.The tenant conversion functions follow the established pattern consistently.
The parameter name issue in
tenant_from_db_modelwas previously addressed.openapi/tenant.yaml (1)
1-3: Review: Security Definitions
The security block correctly lists bothAPIKeyAuthandoAuth2AuthCodefor tenant endpoints. However, ensure that detailed scopes, descriptions, and any rate-limiting considerations are either defined here or referenced from centralized security scheme documentation.fleet_management_api/controllers/security_controller.py (1)
6-7: LGTM! Good modularization of auth parameters.The new imports from auth_controller improve encapsulation by moving auth parameters to a dedicated module.
tests/database/test_wait_for_content.py (3)
63-65: LGTM! Tenant initialization in setUp.The tenant initialization in setUp is correctly implemented using
TenantFromTokenMockwithTEST_TENANT_NAME.
115-117: LGTM! Tenant initialization in setUp.The tenant initialization in setUp is correctly implemented using
TenantFromTokenMockwithTEST_TENANT_NAME.
172-189: LGTM! Tenant integration in connection pool test.The test correctly integrates tenant functionality while maintaining the original connection pool testing logic.
fleet_management_api/database/db_access.py (1)
64-73: LGTM! Well-documented tenant exception classes.The exception classes are well-defined with clear docstrings explaining their purpose.
fleet_management_api/api_impl/controllers/order_state.py (2)
24-28: LGTM! Clean implementation of tenant-related imports.The new imports properly separate request handling and tenant access control concerns, aligning well with the PR's multi-tenancy objectives.
34-54: Verify consistent tenant parameter usage across endpoints.Let's ensure all order state-related endpoints consistently handle tenant parameters.
Also applies to: 155-183, 185-209
✅ Verification successful
Consistent Tenant Parameter Usage Verified
The shell script outputs confirm that:
- Tenant parameters are consistently instantiated (e.g.,
_AccessibleTenants(request)) infleet_management_api/api_impl/controllers/order_state.pyat various points.- Route definitions in
fleet_management_api/api_impl/controllers/order.pyalso use tenant parameters.- All database operations in the order state endpoints correctly pass the tenant parameter.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify consistent tenant parameter usage in order state endpoints # Expected: All endpoints should handle tenant parameters consistently # Check for tenant parameter in route definitions echo "Checking route definitions for tenant parameters..." rg -A 5 "def.*order_state.*tenant.*:" fleet_management_api/ # Check for tenant validation in endpoint implementations echo "Checking tenant validation in implementations..." ast-grep --pattern 'tenants = _AccessibleTenants($$$)' fleet_management_api/api_impl/controllers/order_state.py # Check for consistent tenant parameter passing to database operations echo "Checking database operations for tenant usage..." rg -A 2 "_db_access\.(get|add|delete)" fleet_management_api/api_impl/controllers/order_state.pyLength of output: 2807
tests/client/test_client_authorization_and_tenant.py (1)
53-64: LGTM! Well-structured test setup and cleanup.The test class properly handles:
- Test environment initialization with key generation and auth params
- Server startup with health checks
- Resource cleanup in teardown
Also applies to: 106-111
fleet_management_api/api_impl/controllers/platform_hw.py (2)
2-2: LGTM! Import changes align with tenant implementation.The new imports for request handling and tenant management are well-organized and follow the existing naming conventions.
Also applies to: 15-19
88-113: Update docstring to reflect tenant requirements and verify tenant validation.The docstring should document tenant-related constraints. Additionally, verify that tenant validation is performed before checking car assignments.
def delete_hw(platform_hw_id: int) -> _Response: """Delete an existing platform HW identified by 'platformhw_id'. The platform HW cannot be deleted if: - - assigned to a Car. + - it is assigned to a Car + - it belongs to a different tenant + - the user doesn't have access to the tenant """✅ Verification successful
Tenant validation is correctly integrated into the delete operation.
The controller first extracts tenant information through
_AccessibleTenants(request), and this tenant context is passed into both the car assignment check (_db_access.exists) and the deletion call (_db_access.delete). The tenant validation occurs implicitly via these calls—specifically, the deletion function infleet_management_api/database/db_access.pyvalidates tenant ownership before proceeding. The only update required is to enhance the docstring to document these tenant-related constraints.
- Verify that
_AccessibleTenantsextracts and validates tenant access early on.- Confirm that both
_db_access.existsand_db_access.deleteuse the provided tenant context for their operations.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify tenant validation order in delete operations # Expected: Tenant validation should occur before checking car assignments # Search for tenant validation patterns in database access rg -A 5 "def delete.*tenant" fleet_management_api/database/Length of output: 1165
fleet_management_api/api_impl/controllers/route.py (1)
37-74: LGTM! Well-implemented tenant support.The tenant implementation is consistent across all public methods:
- Request loading with proper tenant context
- Database operations with tenant awareness
- Error handling with tenant context
- Proper logging of operations
Also applies to: 77-100, 103-117, 120-131, 134-168
fleet_management_api/openapi/openapi.yaml (18)
12-12: API Version Update
The API version has been updated to4.0.0-alpha. Ensure that all client integrations and downstream consumers are aware of this change and that the documentation reflects any breaking updates.
39-40: Tenant Tag Declaration
A new tag for tenant-related functions has been introduced. This is an important step for supporting multi-tenancy. Please verify that the new tenant endpoints include appropriate security controls (e.g. role-based access control and rate limiting) as suggested in previous reviews.
46-51: Standardize Car ID Parameter
The/action/car/{carId}endpoint now uses a standardized parameter for the car ID that references#/components/schemas/Id. This improves consistency across different endpoints.
142-147: Consistent Parameter in Car Pause Endpoint
The/action/car/{carId}/pauseendpoint now employs the standardizedIdschema for the car ID parameter. This update aligns it with other endpoints and simplifies client-side handling.
210-215: Consistent Parameter in Car Unpause Endpoint
The/action/car/{carId}/unpauseendpoint parameters have been updated to use the sharedIdschema for the car ID. This consistency helps with validation and documentation across the API.
304-304: Health Check Endpoint Security
TheHEAD /apialiveendpoint now explicitly setssecurity: [], indicating that no authentication is required. This is typical for public health checks, but please confirm that this behavior aligns with your overall security policy.
515-520: Standardize Car ID in Delete Endpoint
In the/car/{carId}DELETE endpoint, the car ID parameter has been updated to reference theIdschema. This standardization ensures a uniform approach to identifier handling throughout the API.
600-605: Standardize Car ID in Get Endpoint
The/car/{carId}GET endpoint now uses the standardizedIdschema for the car ID parameter. This promotes consistency and simplifies maintenance of the API.
848-853: Car State Endpoint Parameter Consistency
The/carstate/{carId}GET endpoint’s car ID parameter has been updated to use the standardizedIdschema. This change aligns the endpoint with the rest of the API.
1155-1166: Standardized IDs in Order Deletion Endpoint
Both the car ID and order ID parameters in the/order/{carId}/{orderId}DELETE endpoint have been standardized to reference theIdschema. This enhances validation and consistency across order-related endpoints.
1075-1080: Consistent Car ID in Order Retrieval
The GET endpoint under/order/{carId}now employs the standardizedIdschema for the car ID parameter. This revision aids in consistency with other identifier usages across the API.
1498-1503: Standardize Order ID in Order State Endpoint
The/orderstate/{orderId}GET endpoint has been updated so that the order ID parameter uses theIdschema. This change merits consistency and more straightforward client-side integration.
1716-1721: Consistent Platform HW ID in Delete Endpoint
The/platformhw/{platformHwId}DELETE endpoint now references the standardizedIdschema for the platform hardware ID parameter. This update improves consistency and validation across hardware-related endpoints.
1800-1805: Consistent Platform HW ID in Get Endpoint
Similarly, the/platformhw/{platformHwId}GET endpoint has been updated to use the standardizedIdschema. This ensures that all endpoints handling platform hardware IDs apply a uniform format.
2762-2820: Tenant Deletion Endpoint
A new tenant deletion endpoint has been added at/tenant(DELETE). While the implementation appears functionally correct, please verify that robust authorization checks (for example, role-based access control) are enforced to prevent unauthorized deletions.
2847-2897: Tenant Retrieval Endpoint
The/tenantGET endpoint, which returns all existing tenants, has been introduced to support multi-tenancy. Ensure that the response structure and any applicable filtering or sorting rules are well documented.
2898-2971: Tenant Cookie Setup via HEAD Endpoint
The/tenant/{tenantId}HEAD endpoint is designed to set a cookie containing tenant information. Please check that the cookie’s configuration (such as HttpOnly and Secure flags) adheres to security best practices.
3344-3347: Standardized ID Schema Definition
The centralIdschema has been defined/updated to standardize the format of identifiers across all endpoints. This is a valuable improvement that simplifies client integration and API validation.tests/database/test_db_operations.py (1)
210-219: Fix inconsistent test method name.The test method name suggests deleting records with least attribute values, but the implementation deletes records with maximum values.
- def test_deleting_n_records_with_highest_attribute_other_than_id(self): + def test_deleting_n_records_with_least_attribute_other_than_id(self):fleet_management_api/api_impl/controllers/stop.py (3)
21-25: LGTM! Clean integration of tenant-aware request handling.The new imports follow the existing naming conventions and provide the necessary components for tenant-aware request handling.
62-74: LGTM! Proper tenant-aware deletion with reference checks.The implementation correctly integrates tenant context while maintaining the important security check for referenced stops.
104-108: LGTM! Clean tenant-aware implementation.The implementation correctly integrates tenant context for retrieving stops.
fleet_management_api/api_impl/load_request.py (5)
10-18: Good use of a dataclass for a base request model.
It provides a clear declaration of request attributes and leverages Python's standard library. No issues found here.
38-41: Propertyapi_keyis clear and concise.
Extracting the API key from query parameters is straightforward and consistent with typical usage patterns.
53-64:RequestJSONfunctionality appears correct.
The class checks for JSON content and gracefully handles decoding errors. This is an appropriate approach for JSON payloads.
66-75:RequestNoDatais straightforward.
This class correctly returnsNonefor data and always validates. Good for endpoints that do not accept body content.
77-91:RequestEmptyclass is well-defined for fallback scenarios.
It sets default values for all fields, returningNoneand validating trivially, which is suitable for requests that genuinely do not contain data.fleet_management_api/controllers/tenant_controller.py (1)
11-46: Placeholder methods remain unimplemented.
These stubs currently return'do some magic!', and a real implementation is necessary to fulfill tenant management operations. However, the past comments clarify this file is auto-generated code not used directly.fleet_management_api/api_impl/api_logging.py (1)
25-28:log_warningfunction aligns with existing logging structure.
Adding a dedicated function for warnings improves the granularity of logs. No issues found.fleet_management_api/api_impl/api_keys.py (1)
8-8: LGTM! Model transition looks good.The changes consistently update the API key management to use the new
ApiKeyDBmodel and tenant-aware database access methods.Also applies to: 29-31, 37-39, 56-58
fleet_management_api/api_impl/controllers/tenant.py (3)
19-36: LGTM! Cookie management looks good.The function properly validates tenant ID, checks accessibility, and sets the cookie with appropriate error handling.
39-49: LGTM! Tenant retrieval is well implemented.The function properly handles request validation, tenant access, and logging.
52-76: LGTM! Tenant deletion is well implemented.The function properly checks for car associations and handles errors appropriately.
tests/controllers/tenant/test_tenant.py (2)
19-65: LGTM! Tenant retrieval tests are comprehensive.The test class thoroughly covers:
- API key access to all tenants
- JWT token access to specific tenants
- Tenant cookie data restrictions
67-113: LGTM! Cookie management tests are thorough.The test class properly validates:
- Nonexistent tenant handling
- Existing tenant cookie setting
- Inaccessible tenant restrictions
fleet_management_api/app.py (2)
31-46: LGTM! JWT token generation looks good.The function properly handles RSA key and JWT encoding errors with appropriate logging.
61-148: LGTM! Test client implementation looks good.The implementation properly handles:
- API key insertion
- Tenant cookie management
- JWT token authorization
fleet_management_api/api_impl/tenants.py (1)
196-205: Careful decoding in_accessible_tenants_from_jwtParsing the "Payload" field from decoded JWT is critical. Consider handling potential key mismatches or missing fields (e.g., "Payload" not present). Adding explicit error handling for KeyError or JSON parsing failure would improve robustness.
🧰 Tools
🪛 Ruff (0.8.2)
201-203: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
tests/controllers/car/test_car_action_controller.py (2)
14-26: LGTM! Auth imports and constants added.The new imports and constants support the tenant-related test cases.
Also applies to: 34-34
37-494: LGTM! Tenant context added consistently.Test client instantiations have been updated to include tenant context while preserving the original test logic.
🧰 Tools
🪛 Ruff (0.8.2)
294-295: Use a single
withstatement with multiple contexts instead of nestedwithstatements(SIM117)
296-296: Found useless attribute access. Either assign it to a variable or remove it.
(B018)
tests/controllers/order/test_order_state_controller.py (2)
16-26: LGTM! Auth imports added consistently.The new imports support tenant-related test cases.
27-645: LGTM! Tenant context added consistently.Test client instantiations have been updated to include tenant context while preserving the original test logic.
🧰 Tools
🪛 Ruff (0.8.2)
244-244: Loop control variable
inot used within loop bodyRename unused
ito_i(B007)
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (1)
fleet_management_api/api_impl/controllers/stop.py (1)
78-82: Consider enhancing error messages for tenant security.The error message includes the stop ID which could leak information about stops from other tenants.
- f"Could not delete stop with ID={stop_id}{note}. {response.body['detail']}", + f"Could not delete the requested stop{note}. {response.body['detail']}",
♻️ Duplicate comments (1)
fleet_management_api/api_impl/load_request.py (1)
19-36:⚠️ Potential issueValidate tenant before setting cookie.
The current implementation sets the tenant cookie without validation. Consider adding tenant validation to prevent unauthorized tenant access.
🧹 Nitpick comments (23)
tests/_utils/docker-compose.yaml (1)
14-16: Network Definition and YAML Best PracticesThe network
api-networkis defined correctly. For improved clarity and to avoid any ambiguity in YAML parsing, consider explicitly defining the network as an empty mapping (e.g.,api-network: {}). Additionally, if desired for broader compatibility, you might include aversionfield at the top of this Docker Compose file (for example,version: "3.8").tests/client/test_client_authorization_and_tenant.py (2)
26-28: Remove unused helper function.The
stop_appfunction appears to be unused in the test suite and its purpose is unclear. Consider removing it if not needed.-def stop_app(): - raise RuntimeError("Stopping the app") -
67-107: Add test cases for additional tenant scenarios.While the current tests cover basic authorization flows, consider adding test cases for:
- Attempting access without a tenant cookie
- Using an invalid tenant cookie
- Attempting to access data from a different tenant's scope
These scenarios would strengthen the test coverage for tenant isolation.
Example test case:
def test_accessing_data_without_tenant_cookie_raises_exception(self): client = ApiClient( configuration=Configuration( host="http://localhost:8080/v2/management", api_key={"APIKeyAuth": "APIKey"}, ), # No tenant cookie provided ) platform_hw_api = PlatformHWApi(client) with self.assertRaises(UnauthorizedException): platform_hw_api.get_hws()fleet_management_api/api_impl/controllers/route.py (2)
143-168: Consider improving error handling for tenant-specific cases.While the implementation correctly handles tenant access, the error message could be more specific about tenant-related failures.
Consider updating the error message to include tenant context:
- f"Routes {[r.name for r in routes]} could not be updated {note}. {response.body['detail']}", + f"Routes {[r.name for r in routes]} could not be updated for tenant {tenants.current} {note}. {response.body['detail']}",
200-200: Consider optimizing database query.The current implementation makes multiple database calls: first to check existence and then to get all stops. This could be optimized to a single query.
Consider refactoring to use a single query:
- _db_access.exists(tenants, _StopDB, criteria={"id": partial(check_id, id_=id_)}) - existing_ids = set([stop_id.id for stop_id in _db_access.get(tenants, _StopDB)]) + existing_ids = set([stop_id.id for stop_id in _db_access.get(tenants, _StopDB, criteria={"id": lambda x: x in checked_id_set})])fleet_management_api/database/db_access.py (3)
2-2: Remove unused import.Remove
Protocolfrom the imports as it's not used in the code.-from typing import Any, Optional, Literal, Callable, Iterable, ParamSpec, TypeVar, Protocol +from typing import Any, Optional, Literal, Callable, Iterable, ParamSpec, TypeVar🧰 Tools
🪛 Ruff (0.8.2)
2-2:
typing.Protocolimported but unusedRemove unused import:
typing.Protocol(F401)
495-501: Improve error handling in the update method.The caught exception
eis unused in the except block. Consider using it in the error message for better debugging.- except _sqaexc.NoResultFound as e: + except _sqaexc.NoResultFound: msg = f"{item.model_name} (ID={item.id}) was not found. Nothing to update." response = _error(404, msg, title="Cannot update nonexistent object")🧰 Tools
🪛 Ruff (0.8.2)
495-495: Local variable
eis assigned to but never usedRemove assignment to unused variable
e(F841)
434-438: Optimize dictionary key iteration.Use direct key iteration instead of calling
.keys()for better performance.- return [criteria[name](getattr(base.__table__.columns, name)) for name in criteria.keys()] + return [criteria[name](getattr(base.__table__.columns, name)) for name in criteria]🧰 Tools
🪛 Ruff (0.8.2)
437-437: Use
key in dictinstead ofkey in dict.keys()Remove
.keys()(SIM118)
tests/controllers/car/test_car_controller.py (2)
41-47: Optimize app instance usage.The test method recreates the app instance unnecessarily when it's already available as
self.appfrom setUp.Apply this optimization:
def test_creating_car_without_existing_platform_hw_yields_404_error_code(self): car = Car( name="Test Car", platform_hw_id=216465168, under_test=False, car_admin_phone=PHONE, ) - app = _app.get_test_app(use_previous=True) - with app.app.test_client(TEST_TENANT_NAME) as c: + with self.app.app.test_client(TEST_TENANT_NAME) as c:Also applies to: 48-49, 50-55
311-312: Consider fixing type issues instead of using type ignore.The type ignore comments suggest potential type issues with the response JSON. Consider adding proper type hints or validation instead of suppressing the type checker.
Example improvement:
- self.assertEqual(len(response.json), 1) # type: ignore - self.assertEqual(response.json[0]["name"], "Updated Test Car") # type: ignore + response_data = response.json + if not isinstance(response_data, list): + self.fail("Expected response.json to be a list") + self.assertEqual(len(response_data), 1) + self.assertEqual(response_data[0]["name"], "Updated Test Car")fleet_management_api/api_impl/load_request.py (3)
1-18: LGTM! Consider adding type stubs for connexion.The imports and base class definition are well-structured with proper type hints. The dataclass implementation with default factories is a clean approach.
Consider adding type stubs for connexion instead of using
# type: ignoreto improve type safety.
66-75: Add docstring to clarify class purpose.Consider adding a docstring to explain when this class should be used, as its purpose isn't immediately clear from the implementation.
Example docstring:
""" Request handler for endpoints that don't expect request data. Used for GET, DELETE, or other methods where the request body is empty. """
77-91: Clarify the distinction between RequestEmpty and RequestNoData.The codebase has two similar classes for handling empty requests:
RequestEmptyandRequestNoData. This could lead to confusion about which class to use in different scenarios.Consider:
- Adding docstrings to clarify the use case for each class
- Merging them if they serve the same purpose
- Renaming them to better reflect their specific uses if they serve different purposes
tests/database/test_database_cleanup.py (2)
23-27: Consider using logging instead of print for connection status.The connection logic looks good, but consider using Python's logging module instead of print for better control over debug output.
- print(f"Connection to the test database '{DB_NAME}' has been created.") + import logging + logging.debug(f"Connection to the test database '{DB_NAME}' has been created.")
62-104: Consider adding tenant isolation test.While the current tests cover tenant-specific operations well, consider adding a test to verify that data is properly isolated between tenants.
Example test:
def test_tenant_data_isolation(self): # Create data for first tenant self._set_up_test_data() # Create second tenant and verify it can't see first tenant's data second_tenant = TenantFromTokenMock("second_tenant") _db_access.add_without_tenant(_db_models.TenantDB(name="second_tenant")) cars = _db_access.get(second_tenant, _db_models.CarDB) self.assertFalse(cars, "Second tenant should not see first tenant's data")tests/security/test_tenants.py (5)
29-32: Add docstring to testing_auth_header function.The utility function would benefit from a docstring explaining its purpose and parameters.
def testing_auth_header(*tenants: str) -> str: + """Generate an authorization header with JWT token for given tenants. + + Args: + *tenants: Variable number of tenant identifiers. + + Returns: + str: Authorization header string in Bearer format. + """ token = get_token(*tenants) return f"Bearer {token}"
34-39: Enhance error handling in tenants_from_token function.The function could benefit from more specific error handling and logging.
def tenants_from_token(request: ConnexionRequest, key: str, audience: str) -> AccessibleTenants: + """Extract accessible tenants from JWT token in request. + + Args: + request: The connection request containing the JWT token. + key: The key used to verify the token. + audience: The intended audience for the token. + + Returns: + AccessibleTenants: Object containing accessible tenant information. + + Raises: + jwt.exceptions.InvalidKeyError: If the key is invalid. + """ try: return AccessibleTenants(request, key, audience) except jwt.exceptions.InvalidKeyError: - print(f"Could not parse key:\n{key}") + import logging + logging.error(f"Failed to parse RSA key for JWT token validation:\n{key}") raise
42-89: Add class-level docstring to Test_RSA_Key_Accessibility.The test class would benefit from a docstring explaining its purpose and test coverage.
class Test_RSA_Key_Accessibility(unittest.TestCase): + """Test suite for validating RSA key accessibility and error handling. + + This class tests various scenarios related to RSA key generation, + accessibility, and authentication parameter settings. + """
172-229: Enhance test assertions in Test_Setting_Tenant_Cookie.The test methods could benefit from more descriptive assertion messages.
def test_no_api_key_yields_access_only_to_accessible_tenants(self) -> None: with self.app.app.test_client() as client: response = client.get( "/v2/management/platformhw", headers={"Authorization": f"Bearer {get_token(TEST_TENANT_1, TEST_TENANT_2)}"}, ) assert isinstance(response.json, list) - self.assertEqual(len(response.json), 2) - self.assertEqual(response.json[0]["name"], self.hw_1.name) - self.assertEqual(response.json[1]["name"], self.hw_2.name) + self.assertEqual(len(response.json), 2, "Expected exactly two accessible tenants") + self.assertEqual(response.json[0]["name"], self.hw_1.name, "First tenant data mismatch") + self.assertEqual(response.json[1]["name"], self.hw_2.name, "Second tenant data mismatch")
1-282: Consider adding integration tests for tenant isolation.While the unit tests provide good coverage of individual tenant functionality, consider adding integration tests to verify tenant data isolation in real-world scenarios. This could include:
- Concurrent access from multiple tenants
- Cross-tenant data access attempts
- Tenant deletion with cascading effects
This would help ensure the robustness of the multi-tenant architecture.
fleet_management_api/app.py (3)
49-59: Consider adding cleanup mechanism for cached app.The global caching mechanism could lead to state leakage between tests. Consider adding a cleanup method to reset the global state.
+def clear_test_app() -> None: + """Clears the cached test app instance.""" + global _test_app + _test_app = None def get_app(use_previous: bool = False) -> _FlaskApp: global _test_app if use_previous and _test_app is not None: return _test_app else: app = _FlaskApp(__name__, specification_dir="./openapi/") app.app.json_encoder = JSONEncoder app.add_api("openapi.yaml", pythonic_params=True) _test_app = app return app
88-92: Add validation for tenant parameter.The
test_clientmethod accepts any string as a tenant name without validation. Consider adding validation to ensure tenant names follow expected format.+def _is_valid_tenant_name(tenant: str) -> bool: + """Validates tenant name format.""" + return bool(tenant and tenant.strip() and tenant.isalnum() or "-" in tenant) def test_client(self, tenant: str = TEST_TENANT_NAME) -> _FlaskClient: + if not _is_valid_tenant_name(tenant): + raise ValueError(f"Invalid tenant name: {tenant}") if self._api_key == "": return _TestClient(self, self._api_key, tenant=tenant) else: return self._app.test_client(TEST_TENANT_NAME)
153-182: Add cleanup for accessible tenants.While the function clears orders between tests, it should also reset accessible tenants to prevent state leakage.
def get_test_app( predef_api_key: str = "", accessible_tenants: Optional[list[str]] = None, use_previous: bool = False, ) -> TestApp: try: _db_access.add_without_tenant( _ApiKeyDB( key=predef_api_key, name=TEST_API_KEY_NAME, creation_timestamp=_timestamp_ms() ), ) except _db_access.DuplicateError: print(f"API key under name {TEST_API_KEY_NAME} already exists.") except Exception as e: print(f"Error when adding API key: {e}.") if accessible_tenants is None: accessible_tenants = [] clear_active_orders() clear_inactive_orders() + # Reset any previously set accessible tenants + app = TestApp(predef_api_key, use_previous=use_previous) + app.app.def_accessible_tenants() # Reset to empty list + if accessible_tenants: + app.app.def_accessible_tenants(*accessible_tenants) - app = TestApp(predef_api_key, use_previous=use_previous) - app.app.def_accessible_tenants(*accessible_tenants) return app
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
docker-compose.yaml(1 hunks)fleet_management_api/api_impl/controllers/route.py(8 hunks)fleet_management_api/api_impl/controllers/stop.py(4 hunks)fleet_management_api/api_impl/load_request.py(1 hunks)fleet_management_api/app.py(1 hunks)fleet_management_api/controllers/security_controller.py(2 hunks)fleet_management_api/database/db_access.py(16 hunks)tests/_utils/docker-compose.yaml(1 hunks)tests/_utils/setup_utils.py(1 hunks)tests/_utils/test_config.json(1 hunks)tests/client/test_client_authorization_and_tenant.py(1 hunks)tests/controllers/car/test_car_controller.py(16 hunks)tests/database/test_database_cleanup.py(3 hunks)tests/database/test_db_operations.py(2 hunks)tests/security/test_tenants.py(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- docker-compose.yaml
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/_utils/test_config.json
- fleet_management_api/controllers/security_controller.py
- tests/_utils/setup_utils.py
🧰 Additional context used
📓 Learnings (2)
tests/database/test_database_cleanup.py (1)
Learnt from: jiristrouhal
PR: bringauto/fleet-management-http-api#27
File: tests/database/test_database_cleanup.py:53-54
Timestamp: 2025-01-31T13:43:48.677Z
Learning: In fleet-management-http-api tests, the database is completely recreated for each test using docker compose, making explicit cleanup of individual records unnecessary.
tests/controllers/car/test_car_controller.py (1)
Learnt from: jiristrouhal
PR: bringauto/fleet-management-http-api#27
File: tests/controllers/car/test_get_last_car_state.py:25-25
Timestamp: 2025-01-31T13:35:03.741Z
Learning: The fleet-management-http-api codebase has comprehensive tenant isolation testing across multiple test files:
1. Security tests in tests/security/test_tenants.py
2. Database tests in tests/database/test_using_tenants.py
3. Car-specific tenant isolation in tests/controllers/car/test_car_controller.py
4. API endpoint tests in tests/controllers/tenant/test_tenant.py
🪛 Ruff (0.8.2)
fleet_management_api/database/db_access.py
2-2: typing.Protocol imported but unused
Remove unused import: typing.Protocol
(F401)
437-437: Use key in dict instead of key in dict.keys()
Remove .keys()
(SIM118)
495-495: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
🔇 Additional comments (32)
tests/_utils/docker-compose.yaml (1)
1-13: Service Definition and Environment VariablesThe PostgreSQL service configuration is clearly defined for a test environment. The use of specific environment variables (e.g.,
POSTGRES_USER,POSTGRES_PASSWORD, andPOSTGRES_DB) is appropriate for facilitating tenant-specific testing. Note that since these credentials are in plain text, ensure that this configuration is used only in non-production environments.tests/client/test_client_authorization_and_tenant.py (2)
30-51: LGTM! Robust server initialization implementation.The
wait_for_processfunction implements a reliable server readiness check with proper timeout and error handling. This is a good practice to prevent flaky tests.
55-66: LGTM! Well-structured test setup and teardown.The test class follows best practices with proper initialization and cleanup:
- Complete setup with test keys and auth params
- Uses robust server initialization
- Proper resource cleanup in teardown
Also applies to: 108-113
fleet_management_api/api_impl/controllers/route.py (5)
30-34: LGTM! Clean tenant-related imports.The new imports for tenant-aware request handling and access control are well-organized and follow the established naming conventions.
46-74: LGTM! Tenant-aware route creation.The implementation correctly:
- Uses tenant-aware request handling
- Validates tenant access before processing routes
- Includes tenant context in database operations
- Maintains proper error handling with tenant context
79-83: LGTM! Tenant-aware route deletion.The implementation properly validates tenant access before attempting deletion operations.
105-110: LGTM! Tenant-aware route retrieval.The implementation correctly scopes the database query to the accessible tenants.
197-199: LGTM! Lambda refactored to named function.The lambda has been properly refactored to a named function as previously suggested.
fleet_management_api/database/db_access.py (3)
49-49: LGTM! Good type alias definition.The
Criteriatype alias improves code readability and maintainability by providing a clear type hint for filtering conditions.
52-79: LGTM! Well-structured exception hierarchy.The new exception classes are properly documented and follow Python's exception hierarchy conventions. They provide clear error messages for tenant-related issues.
583-596: LGTM! Well-structured validation helpers.The validation helper functions are well-implemented with clear separation of concerns and proper type hints.
tests/controllers/car/test_car_controller.py (2)
5-22: LGTM! Import changes and constant definitions look good.The changes properly organize imports for tenant support and improve code reusability by introducing the PHONE constant.
170-171: Fix typo in test method names.The test method names contain typos: "naems" should be "names".
Also applies to: 182-183
fleet_management_api/api_impl/load_request.py (1)
53-64: LGTM! Clean JSON request handling.The implementation properly handles JSON data extraction with appropriate error handling.
tests/database/test_database_cleanup.py (5)
10-16: LGTM! Good job extracting constants and organizing imports.The separation of tenant-specific imports and the extraction of database configuration into constants improves maintainability.
37-38: LGTM! Good use of -v flag for complete cleanup.The addition of the -v flag ensures volumes are removed, providing a clean slate for each test.
52-54: LGTM! Good tenant initialization in setUp.The setup properly initializes the test environment with tenant configuration, which is essential for multi-tenancy testing.
57-60: LGTM! Proper tenant context in test data setup.The test data setup correctly incorporates tenant context in database operations.
108-111: LGTM! Proper cleanup using docker compose.The teardown properly cleans up the test environment using docker compose.
tests/security/test_tenants.py (2)
231-270: Improve test cleanup in Test_Adding_Tenants_To_Database.The test class already has proper cleanup in tearDown as suggested in the previous review, which is good practice.
280-282: LGTM!The main entry point is correctly implemented with proper test execution and pragma directive.
tests/database/test_db_operations.py (4)
20-23: Consolidate duplicated tenant setup code.The tenant setup code is duplicated across all test classes. This violates the DRY principle and makes maintenance harder.
Also applies to: 53-56, 102-105, 152-155, 175-178, 197-200
208-208: Add tenant parameter to delete_n calls.The
delete_ncalls are missing the tenant parameter, which could lead to data deletion across tenants.Also applies to: 219-219, 229-229, 238-238, 249-249, 254-254, 275-275
214-214: Test case for deleting records by non-id attribute is inconsistent.The test method name suggests deleting records with highest attribute values, which is consistent with the implementation.
302-302: LGTM! Improved test output verbosity.The addition of verbosity=2 provides more detailed test output, which is helpful for debugging. The pragma comment correctly excludes this line from coverage reports.
fleet_management_api/api_impl/controllers/stop.py (4)
21-25: LGTM! Appropriate imports for tenant support.The new imports properly support tenant-aware request handling and access control.
87-96: LGTM! Secure tenant-aware implementation.The implementation correctly handles tenant context and uses tenant-agnostic error messages.
104-108: LGTM! Proper tenant-aware implementation.The function correctly filters stops based on tenant context.
146-157: LGTM! Secure tenant-aware implementation.The function correctly handles tenant context and uses tenant-agnostic error messages.
fleet_management_api/app.py (3)
1-26: LGTM! Well-organized imports and constants.The imports are properly organized and typed, with clear separation between standard library, third-party, and local imports. Constants follow naming conventions.
135-147: Improve error handling in header generation.The error handling in
_get_headersfollows good practices by catching specific exceptions and logging errors.
31-46: 🛠️ Refactor suggestionImprove error handling in JWT token generation.
While the error handling is better than using bare except clauses, returning an empty string on JWT encoding errors could silently mask authentication issues.
Consider this improvement:
try: encoded = jwt.encode(payload, private, algorithm="RS256") except jwt.PyJWTError as e: _log_error(f"Failed to encode JWT token: {str(e)}") - return "" + raise return encodedAlso, consider adding debug logging for successful token generation to aid in troubleshooting:
try: encoded = jwt.encode(payload, private, algorithm="RS256") + _log_debug(f"Successfully generated JWT token for tenants: {tenants}") return encodedLikely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
tests/database/test_db_operations.py (2)
205-205:⚠️ Potential issueAdd tenant parameter to delete_n calls.
The
delete_ncalls are missing the tenant parameter, which could lead to data deletion across tenants.Apply this pattern to all delete_n calls:
-_db_access.delete_n(base=models.TestItem, n=2, column_name="id", start_from="minimum") +_db_access.delete_n(self.tenant, base=models.TestItem, n=2, column_name="id", start_from="minimum")Also applies to: 225-225, 234-234, 245-245, 250-250, 272-272
211-220:⚠️ Potential issueTest method name is inconsistent with implementation.
The test method name suggests deleting records with least attribute values, but the implementation deletes records with maximum values.
- def test_deleting_n_records_with_highest_attribute_other_than_id(self): + def test_deleting_n_records_with_least_attribute_other_than_id(self):
🧹 Nitpick comments (2)
fleet_management_api/api_impl/controllers/stop.py (2)
62-82: Consider enhancing error message for tenant security.While the tenant-aware implementation is correct, consider making the error message more tenant-agnostic.
Apply this diff to enhance security:
- f"Could not delete stop with ID={stop_id}{note}. {response.body['detail']}", + f"Could not delete the requested stop{note}. {response.body['detail']}",
104-113: Consider enhancing log message for tenant security.While the tenant-aware implementation is correct, consider making the log message more tenant-agnostic to avoid exposing the total count of stops across all tenants.
Apply this diff to enhance security:
- _log_info(f"Found {len(stops)} stops.") + _log_info("Retrieved stops successfully.")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
fleet_management_api/api_impl/controllers/stop.py(4 hunks)tests/database/test_db_operations.py(2 hunks)
🔇 Additional comments (7)
fleet_management_api/api_impl/controllers/stop.py (5)
21-25: LGTM! Appropriate imports for tenant support.The new imports correctly introduce the necessary components for tenant-aware request handling.
36-54: LGTM! Well-implemented tenant-aware create operation.The changes correctly implement tenant context handling and follow security best practices by using tenant-agnostic error messages.
87-96: LGTM! Well-implemented tenant-aware get operation.The changes correctly implement tenant context handling and follow security best practices with tenant-agnostic error messages.
125-143: LGTM! Well-implemented tenant-aware update operation.The changes correctly implement tenant context handling and follow security best practices with tenant-agnostic messages.
146-157: LGTM! Well-implemented tenant-aware route reference check.The changes correctly implement tenant context handling and follow security best practices with tenant-agnostic error messages.
tests/database/test_db_operations.py (2)
11-12: Consider consolidating tenant setup into a base test class.The
_set_up_test_data()function is called in every test class's setUp method. This violates the DRY principle.Create a base test class that handles the tenant setup:
class TenantTestCase(api_test.TestCase): def setUp(self, test_db_path: str = "") -> None: super().setUp(test_db_path) _db_access.add_without_tenant(TenantDB(name=TEST_TENANT_NAME)) self.tenant = TenantFromTokenMock(TEST_TENANT_NAME)
298-299: LGTM! Improved test output verbosity.The increased verbosity level will provide more detailed test output, making it easier to diagnose test failures.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
docs/development.md (2)
6-7: Clarify Update for Example Models.
The change from “Car and Car State,” to “Car and Car Action State,” correctly reflects the new addition. Please ensure that all related documentation and examples across the codebase are updated to maintain consistency.
35-41: Review and Refine the New Tenant Section.
The newly introduced Tenant section is well structured and clearly explains tenant-related requirements. However, note the following suggestions to improve clarity and correctness:
Subject-Verb Agreement (Line ~37):
The sentence currently reads:"Each entity except for API keys and Tenants have a Tenant ID attribute representing the Tenant to which the entity belongs."
Consider revising it to ensure correct subject-verb agreement:-Each entity except for API keys and Tenants have a Tenant ID attribute representing the Tenant to which the entity belongs. +Each entity except for API keys and Tenants has a Tenant ID attribute representing the Tenant to which the entity belongs.Sentence Flow (Lines ~38-39):
The instruction about non-state entities is split across two lines:"For non-state Entities, the tenant has to be set in a cookie or in the header of the request to inform the server to which tenant"
"the entity belongs."
Consider combining these lines to improve readability—for example, "For non-state entities, the tenant must be specified in a cookie or request header, indicating the tenant to which the entity belongs."Adjusting these minor issues will enhance the clarity and professionalism of the documentation.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~37-~37: This verb does not appear to agree with the subject. Consider using a different form.
Context: ... entity except for API keys and Tenants have a Tenant ID attribute representing the ...(AI_EN_LECTOR_REPLACEMENT_VERB_AGREEMENT)
docs/entity_manipulations.md (1)
326-340: Define Tenant Deletion Constraints
The newly added Tenant entity section correctly specifies deletion prerequisites. However, the two bullet points could be merged for clarity. For example, consider using the diff below to reduce redundancy:- - There must be no item on the server owned by the Tenant - - The Tenant must exist. + - The Tenant must exist and have no associated items on the server.This change would make the requirement more concise without losing its meaning.
🧰 Tools
🪛 LanguageTool
[grammar] ~334-~334: This phrase is duplicated. You should probably use “the Tenant” only once.
Context: ... must be no item on the server owned by the Tenant - The Tenant must exist. Result - The Tenant is de...(PHRASE_REPETITION)
docs/endpoints.md (1)
649-655: Tenant GET Endpoint Description & Punctuation
The GET endpoint for tenants is clearly defined and specifies that it returns a JSON array of Tenant objects. A minor nitpick: the description “Return all accessible tenants depending on the client's authorization” could benefit from a slight punctuation tweak (e.g., inserting a comma after “authorization”) to enhance readability.🧰 Tools
🪛 LanguageTool
[uncategorized] ~655-~655: Possible missing comma found.
Context: .../tenant ### GET Return all accessible tenants depending on the client's authorization...(AI_HYDRA_LEO_MISSING_COMMA)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
README.md(5 hunks)docs/development.md(2 hunks)docs/endpoints.md(2 hunks)docs/entity_manipulations.md(9 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- README.md
🧰 Additional context used
📓 Learnings (1)
docs/entity_manipulations.md (1)
Learnt from: jiristrouhal
PR: bringauto/fleet-management-http-api#27
File: docs/entity_manipulations.md:326-338
Timestamp: 2025-01-30T11:13:29.777Z
Learning: The DELETE /tenant endpoint in fleet-management-http-api returns specific error codes: 400 for tenants with associated items, 404 if tenant doesn't exist, and 405 for incorrect HTTP method.
🪛 LanguageTool
docs/development.md
[uncategorized] ~37-~37: This verb does not appear to agree with the subject. Consider using a different form.
Context: ... entity except for API keys and Tenants have a Tenant ID attribute representing the ...
(AI_EN_LECTOR_REPLACEMENT_VERB_AGREEMENT)
docs/endpoints.md
[uncategorized] ~655-~655: Possible missing comma found.
Context: .../tenant ### GET Return all accessible tenants depending on the client's authorization...
(AI_HYDRA_LEO_MISSING_COMMA)
docs/entity_manipulations.md
[grammar] ~334-~334: This phrase is duplicated. You should probably use “the Tenant” only once.
Context: ... must be no item on the server owned by the Tenant - The Tenant must exist. Result - The Tenant is de...
(PHRASE_REPETITION)
🔇 Additional comments (8)
docs/entity_manipulations.md (6)
12-12: Include Tenant in Index
The addition of the Tenant entity link in the index is clear. Please verify that the anchor reference (#tenant) correctly matches the Tenant section header later in the document.
28-28: Enforce Unique Car Name Per Tenant (POST)
Adding the requirement “The name must be unique under a given tenant” for Car creation is a good reinforcement of multi-tenancy. Ensure that this rule is also validated in the backend logic.
47-47: Enforce Unique Car Name Per Tenant (PUT)
The tenant-specific uniqueness constraint for Car updates is consistently applied. Double-check that both POST and PUT operations follow the same validation rules in your API implementation.
221-221: Tenant-Specific Uniqueness for Route Names
Specifying that a Route name must be unique under a given tenant enhances data isolation. Please ensure that this constraint is also enforced in the data model and business logic.
292-292: Enforce Unique Stop Name (POST)
The new requirement that Stop names be unique per tenant is a valuable addition. Confirm that the corresponding input validations in the API and database schema reflect this rule.
307-308: Clarify Stop Update Requirements for Tenant Context
By requiring that “The name must be unique under a given tenant” and “The Stop must exist under the given tenant,” the documentation clearly outlines the data integrity checks needed during updates. Please ensure that the endpoint’s implementation adheres to these constraints.docs/endpoints.md (2)
656-663: Tenant DELETE Endpoint Definition
The DELETE endpoint for a specific tenant is well-documented with appropriate response codes (200, 400, 404, and 500). Please ensure that the backend logic enforces these constraints, particularly that deletion is only allowed if the tenant exists and has no associated items.
664-669: HEAD Endpoint Response Format Consideration
The HEAD endpoint for tenants is documented as returning a response containing aSet-Cookieheader along with a “JSON array of Tenant objects.” Note that according to HTTP standards, HEAD responses do not include a message body. It would be helpful to verify that the intended behavior is achieved (e.g., that only header information is transmitted) and, if necessary, update the documentation to clarify that the response body is omitted.
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
README.md (3)
3-4: [Nitpick] Improve Introductory Sentence Structure
The introductory sentence on line 3 appears to end abruptly ("It enables a user to") before the bullet list. Consider rephrasing or joining the sentence with the subsequent list (e.g., ending the sentence with a colon) for a smoother introduction.
26-30: [Nitpick] Minor Whitespace Correction in Auto-generated Code Section
The note regarding the exception forfleet_management_api/controllers/security_controller.py(line 30) is clear; however, consider adding a space between "manually maintained" and`fleet_management_api/api_impl`for better readability.
210-210: [Nitpick] Grammar Improvement for Clarity
In line 210, consider revising "the server will then send response including the headerSet-Cookiewith the tenant ID" to "the server will then send a response including the headerSet-Cookiewith the tenant ID" to improve grammatical correctness.🧰 Tools
🪛 LanguageTool
[uncategorized] ~210-~210: Possible missing article found.
Context: ...{tenantId}- the server will then send response including the headerSet-Cookie` with ...(AI_HYDRA_LEO_MISSING_A)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
README.md(5 hunks)openapi/tenant.yaml(1 hunks)
🧰 Additional context used
🪛 LanguageTool
README.md
[uncategorized] ~210-~210: Possible missing article found.
Context: ...{tenantId}- the server will then send response including the headerSet-Cookie` with ...
(AI_HYDRA_LEO_MISSING_A)
🔇 Additional comments (12)
README.md (6)
9-10: [Duplicate Comment] Data Flow Description is Clear
The revised description of data flow—from processing incoming API data into commands for cars and vice versa—is clear and aligns with previous feedback.
12-16: [Approved] Clear Implementation Details Section
The "Implementation details" section now clearly outlines that the server is written in Python using Flask and utilizes a PostgreSQL database. This provides a concise overview of the technology stack.
22-25: [Approved] Requirements Section is Concise
The "### Requirements" section specifying "Python 3.10.12+" is straightforward. This meets the need for clarity in prerequisites.
113-123: [Approved] Enhanced Testing Setup Instructions
The added section "## Preparing the environment and dependencies" (lines 113–123) provides clear instructions for setting up the virtual environment and installing both regular and test dependencies. This change effectively addresses previous recommendations regarding test setup documentation.
139-140: [Approved] Testing Example Section is Clear
The "### Example" header under the testing section is a useful addition that clearly labels a sample command for running tests.
200-216: [Approved] Comprehensive Multi-Tenant Support Documentation
The newly added "## Multi-tenant support" section effectively describes multi-tenancy: it details how tenant IDs are associated with entities, how to set them via cookies or headers, and even explains the semi-automatic setup process. This clear documentation will greatly aid API clients in managing tenant-specific data access.🧰 Tools
🪛 LanguageTool
[uncategorized] ~210-~210: Possible missing article found.
Context: ...{tenantId}- the server will then send response including the headerSet-Cookie` with ...(AI_HYDRA_LEO_MISSING_A)
openapi/tenant.yaml (6)
1-3: Security Schemes Definition.
The file introduces two security schemes (APIKeyAuthandoAuth2AuthCode). Ensure that these schemes align with your overall security requirements including any necessary scope definitions, token validation, and rate-limiting considerations.
16-27: Response Codes for the HEAD Endpoint.
The response codes (200,400,401,403,404, and a default error) cover a broad range of scenarios. Ensure that theSet-Cookieheader behavior is clearly documented in the API docs or examples, so clients know what to expect.
29-51: GET Endpoint for Retrieving Tenants.
The GET endpoint for/tenantis well-defined with a clear summary and appropriate response schema. The provided example helps clarify the expected JSON response.
52-77: DELETE Endpoint for Tenant Deletion.
The DELETE endpoint is clearly specified with a proper parameter reference and comprehensive response codes. Ensure that your backend logic accurately handles deletions and edge cases (for example, attempting to delete a non-existent tenant).
79-94: Tenant Schema Definition.
The Tenant schema is defined with the required propertynameand a reference forid. This is concise and clear. Consider whether additional tenant-related properties (such as creation timestamp or status) might be needed in the future, and document the versioning strategy if the schema evolves.
7-15: HEAD Endpoint 'setTenantCookie' – Minor Text Correction and Verb Choice.
The HEAD endpoint is properly defined to set a tenant cookie. However, the summary text contains a minor typo: "tenand" should be corrected to "tenant". Additionally, consider whether the use of the HEAD verb is optimal for setting cookies (since HEAD responses do not include a body); if a body is eventually needed or for consistency with other endpoints, you might evaluate using a GET or POST method instead.
Apply the following diff if needed:- summary: Make the server send back a response with set-cookie header to set cookie equal to the name of the tenand with the tenantId. + summary: Make the server send back a response with set-cookie header to set cookie equal to the name of the tenant with the tenantId.✅ Verification successful
Verification Complete: Minor Text Correction and Endpoint Method Consideration
- The snippet in openapi/tenant.yaml indeed contains the typo
"tenand"which should be corrected to"tenant".- The review comment’s suggestion to consider if a HEAD endpoint is optimal for setting cookies (given that HEAD responses have no body) is appropriate. In the current design, if no response body is needed, using HEAD is acceptable; however, if a body is eventually required, switching to GET or POST should be evaluated.
Based on the provided snippet and context, the diff correction is valid and the additional endpoint method note is a good design consideration.
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts or web queries found
…pi into BAIP-244/implement-tenants
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (7)
fleet_management_api/api_impl/controllers/order.py (2)
252-256: Inconsistent variable naming - tenant vs tenantsThere's an inconsistency in variable naming: line 255 uses singular
tenantwhile other functions use pluraltenants. This could cause confusion.- tenant = _AccessibleTenants(request) - response = _db_access.delete(tenant, _db_models.OrderDB, order_id) + tenants = _AccessibleTenants(request) + response = _db_access.delete(tenants, _db_models.OrderDB, order_id)
137-138: Missing docstring update for remove_orderWhile the implementation seems correct, the docstring doesn't fully describe the function's behavior. This is a minor documentation issue.
- """Remove an order from both active and inactive orders.""" + """Remove an order from both active and inactive orders. + + This function removes the order from the cached lists without affecting the database. + """fleet_management_api/api_impl/controllers/route.py (3)
160-161: Fix typo in success messagesThere's a typo in the success messages - "succesfully" should be "successfully".
- _log_info(f"Route (ID={r.id} has been succesfully updated.") - return _text_response("Routes were succesfully updated.") + _log_info(f"Route (ID={r.id} has been successfully updated.") + return _text_response("Routes were successfully updated.")
205-206: Fix typo in error messageThere's a typo in the error message - "Nonexstent" should be "Nonexistent".
- f"Nonexstent stop ids: {nonexistent_stop_ids}", + f"Nonexistent stop ids: {nonexistent_stop_ids}",
171-175: Consider updating the function docstringThe function signature has been updated to include the
tenantsparameter, but the docstring hasn't been updated. Consider adding documentation for this parameter.def _check_route_model(tenants: _AccessibleTenants, route: _Route) -> _Response: + """Check if a route model is valid. + + Args: + tenants: Tenant context for data access control + route: The route model to check + + Returns: + Response object with the result of the check + """ check_stops_response = _find_nonexistent_stops(tenants, route)fleet_management_api/api_impl/controllers/car.py (1)
78-80: Fix unused return value in car action state creationLine 79 has an unused return value from the
_post_default_car_action_statefunction.- _post_default_car_action_state(tenants, ids).body + _post_default_car_action_state(tenants, ids)🧰 Tools
🪛 Ruff (0.8.2)
79-79: Found useless attribute access. Either assign it to a variable or remove it.
(B018)
fleet_management_api/api_impl/controllers/order_state.py (1)
68-82: Potential security improvement in tenant access controlWhile the code correctly checks for inaccessible orders, it first fetches all orders with unrestricted tenant access before filtering for accessibility. This approach could be optimized from a security perspective.
Consider revising the approach to directly fetch only accessible orders using the tenant context:
- unrestricted = _AccessibleTenants.from_dict({"current": "", "all": []}) - all_orders = _existing_orders(unrestricted, *order_ids) - accessible_orders = _existing_orders(tenants, *order_ids) - all_order_ids = {order.id for order in all_orders.values() if order is not None} - accessible_order_ids = {order.id for order in accessible_orders.values() if order is not None} - inaccessible_order_ids = all_order_ids - accessible_order_ids - if inaccessible_order_ids: - return _log_error_and_respond( - f"Order with IDs={inaccessible_order_ids} are not accessible.", - 401, - title="Unauthorized", - ) + accessible_orders = _existing_orders(tenants, *order_ids) + missing_order_ids = {id_ for id_, order in accessible_orders.items() if order is None} + if missing_order_ids: + # Check if these orders exist but are inaccessible + unrestricted = _AccessibleTenants.from_dict({"current": "", "all": []}) + inaccessible_orders = _existing_orders(unrestricted, *missing_order_ids) + inaccessible_order_ids = {id_ for id_, order in inaccessible_orders.items() if order is not None} + if inaccessible_order_ids: + return _log_error_and_respond( + f"Orders with IDs={inaccessible_order_ids} are not accessible.", + 401, + title="Unauthorized", + )Also note the grammar correction in the error message from "Order with IDs" to "Orders with IDs".
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
fleet_management_api/api_impl/controllers/car.py(6 hunks)fleet_management_api/api_impl/controllers/car_action.py(10 hunks)fleet_management_api/api_impl/controllers/car_state.py(8 hunks)fleet_management_api/api_impl/controllers/order.py(11 hunks)fleet_management_api/api_impl/controllers/order_state.py(8 hunks)fleet_management_api/api_impl/controllers/platform_hw.py(5 hunks)fleet_management_api/api_impl/controllers/route.py(8 hunks)fleet_management_api/api_impl/controllers/route_visualization.py(3 hunks)fleet_management_api/api_impl/controllers/stop.py(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- fleet_management_api/api_impl/controllers/route_visualization.py
- fleet_management_api/api_impl/controllers/car_action.py
🧰 Additional context used
🧬 Code Definitions (7)
fleet_management_api/api_impl/controllers/platform_hw.py (3)
fleet_management_api/api_impl/load_request.py (1)
load(20-36)fleet_management_api/api_impl/tenants.py (1)
AccessibleTenants(51-122)fleet_management_api/database/db_access.py (3)
add(188-251)get(370-415)delete(255-275)
fleet_management_api/api_impl/controllers/car_state.py (6)
fleet_management_api/models/car_state.py (3)
CarState(13-227)id(77-84)id(87-95)fleet_management_api/api_impl/load_request.py (3)
RequestJSON(53-63)RequestEmpty(78-90)load(20-36)fleet_management_api/database/db_models.py (3)
tenants(45-46)CarDB(142-173)CarStateDB(176-211)fleet_management_api/api_impl/tenants.py (1)
AccessibleTenants(51-122)fleet_management_api/database/db_access.py (2)
get(370-415)get_by_id(328-344)fleet_management_api/api_impl/obj_to_db.py (1)
car_state_from_db_model(49-64)
fleet_management_api/api_impl/controllers/stop.py (3)
fleet_management_api/api_impl/load_request.py (3)
RequestJSON(53-63)RequestEmpty(78-90)load(20-36)fleet_management_api/api_impl/tenants.py (1)
AccessibleTenants(51-122)fleet_management_api/database/db_access.py (4)
add(188-251)delete(255-275)get(370-415)update(473-501)
fleet_management_api/api_impl/controllers/car.py (4)
fleet_management_api/api_impl/load_request.py (2)
RequestJSON(53-63)load(20-36)fleet_management_api/database/db_models.py (5)
tenants(45-46)PlatformHWDB(124-139)RouteDB(332-353)CarDB(142-173)CarStateDB(176-211)fleet_management_api/api_impl/tenants.py (1)
AccessibleTenants(51-122)fleet_management_api/database/db_access.py (3)
add(188-251)delete(255-275)get(370-415)
fleet_management_api/api_impl/controllers/route.py (4)
fleet_management_api/database/db_models.py (3)
RouteDB(332-353)RouteVisualizationDB(356-371)tenants(45-46)fleet_management_api/api_impl/load_request.py (2)
RequestJSON(53-63)load(20-36)fleet_management_api/api_impl/tenants.py (1)
AccessibleTenants(51-122)fleet_management_api/database/db_access.py (4)
add(188-251)delete(255-275)get(370-415)update(473-501)
fleet_management_api/api_impl/controllers/order_state.py (5)
fleet_management_api/api_impl/load_request.py (2)
RequestJSON(53-63)load(20-36)fleet_management_api/api_impl/tenants.py (3)
AccessibleTenants(51-122)from_dict(112-114)unrestricted(107-109)fleet_management_api/models/order_state.py (6)
OrderState(11-147)from_dict(49-57)id(60-67)id(70-78)order_id(104-111)order_id(114-124)fleet_management_api/models/order.py (3)
from_dict(83-91)id(94-101)id(104-112)fleet_management_api/database/db_access.py (2)
get(370-415)add(188-251)
fleet_management_api/api_impl/controllers/order.py (5)
fleet_management_api/api_impl/load_request.py (3)
RequestJSON(53-63)RequestNoData(66-74)load(20-36)fleet_management_api/api_impl/tenants.py (1)
AccessibleTenants(51-122)fleet_management_api/database/db_access.py (4)
CheckBeforeAdd(103-132)add(188-251)delete(255-275)get(370-415)fleet_management_api/models/order.py (9)
Order(15-313)car_id(163-170)car_id(173-183)target_stop_id(207-214)target_stop_id(217-227)id(94-101)id(104-112)last_state(274-281)last_state(284-292)fleet_management_api/api_impl/controllers/order_state.py (1)
create_order_states_from_argument_and_post(56-152)
🪛 Ruff (0.8.2)
fleet_management_api/api_impl/controllers/car.py
79-79: Found useless attribute access. Either assign it to a variable or remove it.
(B018)
🔇 Additional comments (71)
fleet_management_api/api_impl/controllers/car_state.py (11)
14-22: Well-organized imports for tenant supportThe imports have been reorganized to include the necessary modules for tenant support, with
RequestJSON,RequestEmpty, andAccessibleTenantsnow available for request processing and tenant management.
33-38: Good request handling improvement with tenant supportThe function now uses the
_RequestJSONclass for request validation instead of direct JSON checking, following a more structured approach. The addition of tenant extraction through_AccessibleTenantsproperly implements the tenant-based access control requirement.
41-43: Appropriate function renaming and signature updateThe function has been renamed from "create_car_states_from_argument_and_save_to_db" to "create_car_states_from_argument_and_post" which better reflects its purpose. The signature now properly includes the tenant parameter, ensuring tenant-based access control.
54-58: Correct tenant parameter passing and model reference updateThe function now passes the tenant parameter to the database access layer and uses the updated
_db_models.CarDBreference. This ensures that car state creation is properly restricted based on tenant permissions.
64-64: Updated cleanup function call with tenant parameterThe
_remove_old_statesfunction call now correctly includes the tenant parameter, ensuring that cleanup operations respect tenant boundaries.
85-87: Added tenant parameter to function signatureThe
get_all_car_statesfunction now includes atenantparameter with a default empty string. This parameter is used in conjunction with_AccessibleTenantsto enforce tenant-based data access.
98-104: Request validation and tenant filtering for get_all_car_statesThe function now properly validates the request using
_RequestEmptyand applies tenant filtering through_AccessibleTenantswhen querying the database. The model reference has been updated to_db_models.CarStateDB.
118-120: Added tenant parameter to get_car_states signatureThe
get_car_statesfunction now includes atenantparameter with a default empty string, consistent with the approach inget_all_car_states.
131-138: Updated model reference and added request validation with tenant supportThe function now uses the correct
_db_models.CarDBreference and implements request validation with_RequestEmpty. It properly applies tenant filtering through_AccessibleTenantswhen querying for car states.
160-163: Updated _remove_old_states to support tenant-based filteringThe
_remove_old_statesfunction signature now includes thetenantsparameter and correctly passes it to the database access layer, ensuring that cleanup operations respect tenant boundaries.
165-169: Updated model reference in cleanup logicThe function now uses the correct method
max_n_of_stored_states()from_db_models.CarStateDBand the updated model reference in thedelete_ncall, ensuring consistency with the rest of the changes.fleet_management_api/api_impl/controllers/stop.py (11)
21-25: Well-structured import additions for tenant support!The imports of
RequestJSON,RequestEmpty, andAccessibleTenantsclasses provide the necessary building blocks for implementing tenant-aware request handling throughout the controller.
36-43: Good implementation of tenant-aware request handling in create_stops!The function now properly validates the request using
_RequestJSON.load()and initializes tenant context using_AccessibleTenants. The database operation correctly includes tenant information to ensure proper data isolation.
50-54: Good security improvement in error messages!The generic error message no longer exposes stop names, which aligns with multi-tenant security best practices by preventing information leakage about entities from other tenants.
62-66: Proper tenant context handling in delete_stop!The function correctly validates the request and initializes tenant context before proceeding with the operation.
73-73: Tenant-aware database deletion implemented correctly!The database deletion operation now includes tenant context to ensure proper data isolation.
87-93: Good implementation of tenant-aware get_stop function!The function now properly validates the request and includes tenant context in the database query.
96-96: 🛠️ Refactor suggestionConsider enhancing error message for tenant security.
While most error messages have been updated to be tenant-agnostic, this one still exposes a specific stop ID which could leak information in a multi-tenant environment.
- return _log_info_and_respond(f"Stop (ID={stop_id}) not found.", 404, title=_OBJ_NOT_FOUND) + return _log_info_and_respond("Stop not found.", 404, title=_OBJ_NOT_FOUND)
104-108: Well-implemented tenant-aware get_stops function!The function correctly validates the request and includes tenant context in the database query to ensure proper data isolation.
125-143: Great implementation of tenant-aware update_stops function!The function properly validates the request and includes tenant context in the database update operation. The error and success messages have been appropriately updated to be generic and not expose specific stop information.
146-149: Well-implemented tenant context in _get_routes_referencing_stop!The internal function now properly receives and uses tenant context when checking for routes that reference a stop.
152-152: Good security improvement in error message!The error message no longer exposes details about the routes, only showing the count. This prevents information leakage in a multi-tenant environment.
fleet_management_api/api_impl/controllers/order.py (12)
23-27: New imports support tenant implementationThese imports introduce request validation classes (
RequestJSON,RequestNoData) and tenant management functionality (AccessibleTenants), which align with the PR's objective of implementing tenant-based data access controls.
175-179: Structured request validation with tenant awarenessGood implementation of structured request validation replacing direct JSON access. The tenant-aware approach ensures proper access control from the beginning of the request lifecycle.
199-199: Tenant-aware car existence checkCar existence check now properly enforces tenant boundaries.
221-221: Database access with tenant contextThe database add operation now correctly includes tenant context to enforce data access boundaries.
232-232: Tenant-aware default order state creationOrder state creation now properly passes tenant context, ensuring states are created within appropriate access boundaries.
276-282: Structured request handling for get_orderGood implementation of request validation and tenant-aware database access.
325-331: Tenant-aware implementation for get_ordersGood implementation of tenant-aware request validation and data access.
341-342: Updated helper functions with tenant contextThe helper function has been correctly updated to use tenant information for access control checks.
345-350: Tenant-aware order state retrievalThe function now properly passes tenant context to ensure state data is accessed within appropriate boundaries.
363-368: Tenant-aware last order state retrievalThe function now correctly uses tenant context for data access control.
383-387: Tenant-aware default order state creationThe helper function now properly uses tenant context when creating default order states.
298-313:Details
❌ Incorrect review comment
Missing tenant parameter in get_children call
The
get_childrendatabase call doesn't include thetenantsparameter, unlike other database access functions. This could potentially bypass tenant verification.db_orders: list[_db_models.OrderDB] = _db_access.get_children( # type: ignore parent_base=_db_models.CarDB, parent_id=car_id, children_col_name="orders", criteria={"timestamp": lambda z: z >= since}, + tenants=tenants, )
🏁 Script executed:
#!/bin/bash # Find the definition of get_children to confirm parameter requirements rg -A 5 "def get_children" --glob "fleet_management_api/database/db_access.py"Length of output: 516
Tenant parameter inclusion is not applicable
The verification shows that theget_childrenfunction (defined infleet_management_api/database/db_access.py) does not accept atenantsparameter. The function’s signature only includesparent_base,parent_id,children_col_name,connection_source, andcriteria. Since tenant validation is not part of its contract, the proposed change would be incompatible with the current implementation.Likely an incorrect or invalid review comment.
fleet_management_api/api_impl/controllers/route.py (5)
30-34: Excellent tenant implementationThe new imports for request handling and tenant management are well-organized and properly introduce the tenant-aware architecture required for this PR.
46-49: Good request handling refactoringThe switch from direct request handling to using the structured
_RequestJSONand tenant context extraction is a good approach that centralizes request processing and adds tenant awareness.
60-67: LGTM - Tenant-aware database operationsThe database operations are now properly tenant-aware, ensuring data isolation between different tenants. The change maintains the core functionality while adding multi-tenancy support.
103-109: Proper implementation of the previous review suggestionYou've correctly removed the unused
tenantparameter and now properly derive tenant context from the request, which is a cleaner approach.
197-199: Good implementation of the previous review suggestionYou've properly replaced the lambda with a named function as suggested in the previous review. This improves code readability and maintainability.
fleet_management_api/api_impl/controllers/platform_hw.py (6)
2-2: Appropriate imports for tenant implementationThe new imports for request handling and tenant management are correctly added, providing the necessary components for implementing multi-tenancy support.
Also applies to: 15-19
30-36: Good refactoring for tenant-based request handlingThe implementation correctly uses the new
_RequestJSON.load()method to handle the request and adds tenant validation through_AccessibleTenants. This ensures proper tenant isolation when creating hardware.
37-50: Improved response handling with tenant contextThe response handling has been updated to include tenant context and provides more descriptive error messages. The code correctly processes the DB response and returns appropriate JSON.
55-59: Secure implementation of tenant validation for get_hwsThe implementation correctly validates the tenant context before retrieving hardware information, ensuring that users can only access data from tenants they're authorized to view.
69-75: Proper tenant validation for single hardware retrievalThe implementation properly validates the tenant context before retrieving individual hardware information, with appropriate criteria filtering.
93-104: Secure implementation of tenant validation for deletionThe implementation correctly validates tenant context before performing deletion operations, ensuring hardware can only be deleted within the proper tenant context.
fleet_management_api/api_impl/controllers/car.py (14)
12-12: Appropriate imports for tenant implementationThe imports correctly include the request handling classes and tenant management class needed for implementing multi-tenancy in the car controller.
Also applies to: 29-33
47-56: Good implementation of tenant-aware car creationThe implementation correctly validates the request and establishes the tenant context before processing car creation. The car objects are properly parsed from the request data.
57-68: Well-structured validation checks with tenant contextThe implementation correctly sets up validation checks for platform hardware and route existence, ensuring these dependencies exist within the proper tenant context.
69-77: Tenant-aware database operations for car creationThe implementation correctly passes the tenant context to the database operations, ensuring cars are created within the proper tenant context.
81-91: Proper car object construction with tenant contextThe implementation correctly constructs car objects with their states and returns appropriate JSON responses.
99-103: Secure implementation of tenant validation for car deletionThe implementation correctly validates the tenant context before performing car deletion operations, ensuring users can only delete cars within their authorized tenants.
114-123: Proper tenant validation for single car retrievalThe implementation correctly validates the tenant context before retrieving individual car information, with appropriate criteria filtering.
129-129: Updated method call with tenant contextThe code correctly passes the tenant context to the internal method for retrieving car state information.
136-142: Secure implementation of tenant validation for car listingThe implementation properly validates the tenant context before retrieving all cars, ensuring users can only see cars from their authorized tenants.
148-148: Correct tenant context propagationThe code correctly passes the tenant context to the internal method for retrieving car state information during the listing operation.
166-178: Tenant-aware car updates implementationThe implementation correctly validates the tenant context before performing car update operations, ensuring updates are only applied within the proper tenant context.
181-186: Updated internal method signature for tenant contextThe internal method
_get_car_with_last_statehas been correctly updated to accept tenant information, ensuring tenant context is maintained throughout the call chain.
189-203: Tenant-aware state retrieval implementationThe internal method
_get_last_car_statecorrectly incorporates tenant context when retrieving car state information, maintaining proper data isolation.
206-209: Updated helper methods with tenant contextThe helper methods for posting default car states and action states have been properly updated to accept and propagate tenant information, ensuring consistent tenant isolation throughout the application.
Also applies to: 212-217
fleet_management_api/api_impl/controllers/order_state.py (12)
24-28: Appropriate import additions for tenant implementationThese new imports correctly incorporate the request handling classes (
RequestEmpty,RequestJSON) and the tenant management class (AccessibleTenants) needed for the multi-tenancy implementation.
43-46: Good implementation of request validation and tenant extractionThe code properly validates the JSON request using the
_RequestJSON.load()method and correctly initializes the tenant information using_AccessibleTenants. This approach ensures proper request validation before processing.
53-59: Tenant parameter correctly propagated to implementation functionThe tenant context is properly passed to the implementation function, which now correctly accepts the
_AccessibleTenantsparameter. This change is consistent with the multi-tenancy design.
92-96: Tenant parameter correctly passed to database access functionThe tenant context is properly passed to the
_db_access.getfunction, ensuring that only data accessible to the tenant is retrieved.
127-127: Tenant parameter correctly passed to database add functionThe tenant context is properly passed to the
_db_access.addfunction, ensuring that the data is associated with the correct tenant.
132-132: Tenant parameter correctly passed to remove old states functionThe tenant context is properly passed to the
_remove_old_statesfunction, ensuring tenant isolation when performing cleanup operations.
174-178: Proper request validation and tenant extractionThe code correctly validates the request using
_RequestEmpty.load()and initializes the tenant information using_AccessibleTenants, consistent with the multi-tenancy implementation.
180-182: Tenant parameter correctly passed to helper functionThe tenant context is properly passed to the
_get_order_statesfunction in both conditional branches.
199-203: Proper tenant-aware order existence checkThe code correctly validates the request, initializes tenant information, and checks for order existence within the tenant's context.
211-222: Good implementation of tenant-aware order retrievalThe
_existing_ordersfunction has been properly updated to accept and use the tenant context when retrieving orders, ensuring that only data accessible to the tenant is returned.
226-236: Tenant parameter correctly integrated into order states retrievalThe
_get_order_statesfunction now properly accepts and uses the tenant context when retrieving order states, maintaining tenant isolation.
249-256:Details
❌ Incorrect review comment
Missing tenant parameter in delete_n call
While the
_remove_old_statesfunction has been updated to accept the tenant parameter, it's not being passed to the_db_access.delete_nfunction in line 256, unlike other database access calls.Based on the pattern of passing tenant context to other database functions, the
delete_nfunction likely needs the tenant parameter:if delta > 0: response = _db_access.delete_n( + tenants, _db_models.OrderStateDB, n=delta, column_name="timestamp", start_from="minimum", criteria={"order_id": lambda x: x == order_id}, )This omission could lead to incorrect tenant isolation during the deletion of old states.
🏁 Script executed:
#!/bin/bash # Check if delete_n function in db_access.py accepts a tenant parameter rg -A 3 "def delete_n" fleet_management_api/database/db_access.pyLength of output: 138
Clarification on Tenant Parameter Usage in
delete_nCall
The verification confirms that thedelete_n()function infleet_management_api/database/db_access.pyis defined as:def delete_n( base: type[_Base], n: int, column_name: str, ... ):It does not expect a tenant parameter, unlike the
get()function. Based on this, the absence of the tenant parameter in the_remove_old_statesfunction’s call todelete_n()is intentional according to the current API design. If tenant isolation is required for deletion operations, this behavior should be revisited by updatingdelete_n()accordingly. Otherwise, no changes are needed in this specific call.Likely an incorrect or invalid review comment.
|
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
fleet_management_api/api_impl/controllers/order_state.py (1)
254-260:⚠️ Potential issueMissing tenant parameter in delete operation.
Unlike other database operations in this file, the
delete_ncall doesn't include the tenant parameter. This inconsistency could potentially allow deletion of data from other tenants.response = _db_access.delete_n( + tenants, _db_models.OrderStateDB, n=delta, column_name="timestamp", start_from="minimum", criteria={"order_id": lambda x: x == order_id}, )Let's verify this by checking the signature of the
delete_nfunction:#!/bin/bash # Check if delete_n requires a tenant parameter rg "def delete_n" -A 5 "fleet_management_api/database/db_access.py"
🧹 Nitpick comments (1)
fleet_management_api/api_impl/controllers/order_state.py (1)
79-79: Minor grammatical issue in error message.There's a slight grammatical inconsistency in the error message.
- f"Order with IDs={inaccessible_order_ids} are not accessible.", + f"Orders with IDs={inaccessible_order_ids} are not accessible.",
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
fleet_management_api/api_impl/controllers/order_state.py(8 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
fleet_management_api/api_impl/controllers/order_state.py (4)
fleet_management_api/api_impl/load_request.py (2)
RequestJSON(53-63)load(20-36)fleet_management_api/api_impl/tenants.py (2)
AccessibleTenants(51-122)from_dict(112-114)fleet_management_api/models/order_state.py (4)
OrderState(11-147)from_dict(49-57)id(60-67)id(70-78)fleet_management_api/database/db_access.py (2)
get(370-415)add(188-251)
🔇 Additional comments (10)
fleet_management_api/api_impl/controllers/order_state.py (10)
24-28: Appropriate imports added for tenant-aware functionality.Good addition of necessary imports for request handling (
RequestEmpty,RequestJSON) and tenant management (AccessibleTenants). These imports support the multi-tenancy implementation throughout the file.
43-46: Good implementation of request validation.The change from direct JSON checking to using the
_RequestJSON.load()pattern is consistent with best practices for request validation and follows the same pattern used elsewhere in the codebase.
56-60: Proper integration of tenant context in function signature.The function signature has been appropriately updated to include the tenant context as the first parameter, which is consistent with the tenant-aware pattern implemented throughout the API.
68-82: Well-implemented tenant access control mechanism.The implementation properly:
- Retrieves all orders (unrestricted access)
- Retrieves only tenant-accessible orders
- Computes the difference to identify unauthorized access attempts
- Returns an appropriate error response for unauthorized access
This is a robust implementation of multi-tenant access control.
92-96: Tenant-aware database query implementation.Good implementation of tenant context in the database query. This ensures that only data accessible to the current tenant is retrieved from the database.
173-177: Request validation and tenant extraction properly implemented.The addition of request validation using
_RequestEmpty.load()and tenant extraction is consistent with the tenant-aware pattern implemented throughout the API.
198-202: Request validation and tenant extraction properly implemented.Similar to the
get_all_order_statesfunction, appropriate request validation and tenant extraction have been added.
210-212: Tenant context added to helper function.Good update to the
_existing_ordershelper function to include tenant context, which ensures all database operations are tenant-aware.
224-230: Tenant context added to helper function.Good update to the
_get_order_stateshelper function to include tenant context, ensuring tenant-aware behavior is maintained throughout the call chain.
248-248: Tenant context added to helper function.The
_remove_old_statesfunction has been properly updated to accept tenant context.
2 participants
This PR introduces data access management via tenants. Each tenant corresponds to a company owning a set of cars.
The tenants restrict for which company the API client can read and write data.
The details are described in the docs/tenants.md document.
Closes BAIP-244.
Summary by CodeRabbit
New Features
Documentation
Testing & Improvements