/api/events endpoint can be used to receive events not only from client #8799
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe changes introduce a new permission scope, Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
cvat/apps/events/views.py (1)
50-54: Enhance OpenAPI documentation with error responsesThe endpoint documentation should include possible error responses for better API understanding.
Add error responses to the
@extend_schemadecorator:@extend_schema(summary='Log external events', description='Sends logs to the Clickhouse if it is connected', parameters=ORGANIZATION_OPEN_API_PARAMETERS, - responses={'201': EventSerializer()}) + responses={ + '201': EventSerializer(), + '400': OpenApiResponse(description='Invalid event data'), + '403': OpenApiResponse(description='Permission denied'), + '429': OpenApiResponse(description='Too many requests'), + })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
cvat/apps/events/permissions.py(2 hunks)cvat/apps/events/views.py(3 hunks)
🔇 Additional comments (3)
cvat/apps/events/permissions.py (1)
18-18: LGTM! Permission scope addition is well-structured
The new SEND_EXTERNAL_EVENTS scope and its mapping are consistently implemented, following the existing patterns and naming conventions.
Also applies to: 54-54
cvat/apps/events/views.py (2)
10-10: LGTM! Clean import organization
The new imports are properly organized and necessary for the added functionality.
Also applies to: 19-19, 22-22
61-63: LGTM! Proper validation and event recording
The implementation correctly validates input data before recording the event.
cvat/apps/events/views.py
Outdated
| @extend_schema(summary='Log external events', | ||
| description='Sends logs to the Clickhouse if it is connected', | ||
| parameters=ORGANIZATION_OPEN_API_PARAMETERS, | ||
| responses={'201': EventSerializer()}) | ||
| @action( | ||
| detail=False, | ||
| url_path='external', | ||
| methods=['POST'], | ||
| serializer_class=EventSerializer, | ||
| ) | ||
| def external(self, request): | ||
| serializer = EventSerializer(data=request.data) | ||
| serializer.is_valid(raise_exception=True) | ||
| record_server_event(**serializer.validated_data) | ||
|
|
||
| return Response(serializer.validated_data, status=status.HTTP_201_CREATED) |
There was a problem hiding this comment.
💡 Codebase verification
Add rate limiting and request validation for external events endpoint
The external events endpoint currently lacks rate limiting and request size validation. While the system has some global settings, this endpoint needs specific protections:
- Add throttling class similar to
ResendOrganizationInvitationThrottlebut with appropriate rate for events - Add request size validation (current global limit is 100MB which is too high for events)
- Consider adding field-level validation in the serializer
Example implementation:
class EventThrottleClass(UserRateThrottle):
rate = '60/minute' # Adjust based on requirements
@action(
detail=False,
url_path='external',
methods=['POST'],
throttle_classes=[EventThrottleClass],
serializer_class=EventSerializer,
)
def external(self, request):
# ... existing code🔗 Analysis chain
Consider adding rate limiting and validation safeguards
While the implementation is structurally sound, consider adding the following security measures:
- Rate limiting to prevent DoS attacks
- Maximum size validation for event data
- Input sanitization for sensitive fields
Consider implementing rate limiting using Django's built-in tools or a dedicated rate-limiting middleware.
Let's check if rate limiting is implemented elsewhere in the codebase:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for rate limiting implementations
rg -A 3 "ratelimit|rate_limit|RateLimit"
Length of output: 42
Script:
#!/bin/bash
# Let's check for any throttling or rate limiting related code using broader patterns
rg -i "(throttle|limit.*request|request.*limit)" -A 3
# Also check Django settings and middleware configurations
fd "settings|middleware" --type f --extension py --exec rg -l "(throttle|rate|limit)"
# Check for DRF throttling configurations
rg "throttle" --type py -A 3
Length of output: 80865
Script:
#!/bin/bash
# Let's check Django REST framework throttling configurations in settings
rg -A 5 "REST_FRAMEWORK.*=.*{" cvat/settings/base.py
# Check for any rate limiting middleware
rg -l "class.*Throttle|class.*RateLimit" --type py
# Check for any rate limiting decorators or configurations
rg "@.*throttle|@.*rate_limit" --type py
Length of output: 475
Script:
#!/bin/bash
# Let's check the content of the throttle class and settings
rg -A 10 "REST_FRAMEWORK" cvat/settings/base.py
# Check the throttle implementation
cat cvat/apps/organizations/throttle.py
# Check if there are any size limits in request configurations
rg -A 3 "DATA_UPLOAD_MAX_MEMORY_SIZE|DATA_UPLOAD_MAX_NUMBER_FIELDS|MAX_" cvat/settings/base.py
Length of output: 1721
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #8799 +/- ##
===========================================
- Coverage 73.93% 73.89% -0.04%
===========================================
Files 409 409
Lines 43930 43930
Branches 3986 3986
===========================================
- Hits 32478 32461 -17
- Misses 11452 11469 +17
|
cvat/apps/events/views.py
Outdated
| @@ -44,6 +48,26 @@ def create(self, request): | |||
|
|
|||
| return Response(serializer.validated_data, status=status.HTTP_201_CREATED) | |||
|
|
|||
| @extend_schema(summary='Log external events', | |||
There was a problem hiding this comment.
Do we really need several end-points to send events (add a row into Clickhouse)?
There was a problem hiding this comment.
reused existing endpoint
cvat/apps/events/permissions.py
Outdated
| class EventsPermission(OpenPolicyAgentPermission): | ||
| class Scopes(StrEnum): | ||
| SEND_EVENTS = 'send:events' | ||
| DUMP_EVENTS = 'dump:events' | ||
| SEND_EXTERNAL_EVENTS = 'send-external:events' |
There was a problem hiding this comment.
Please update the rego rules to reflect this change
There was a problem hiding this comment.
I planned it to be only accessible by admins, and rego rules do not need to be changed for it.
But due to comments above I will try to use the existing endpoint somehow, and just drop this scope
|
Consider adding some tests for this endpoint |
Eldies
changed the title
|
@Eldies LGTM, but please add a changelog entry |
done |
|
Motivation and context
How has this been tested?
Checklist
developbranch(cvat-canvas,
cvat-core,
cvat-data and
cvat-ui)
License
Feel free to contact the maintainers if that's a concern.
Summary by CodeRabbit
New Features
Bug Fixes