Implement API create World #248
Conversation
Reviewer's Guide by SourceryThis pull request implements an API endpoint for creating and updating World objects. It introduces a new CreateWorldView class that handles POST requests to create or update a World, including authentication and permission checks. The changes also involve updates to URL routing and settings to support this new functionality. Sequence diagram for CreateWorldView POST requestsequenceDiagram
actor User
participant CreateWorldView
participant World
User->>CreateWorldView: POST /create-world/
CreateWorldView->>CreateWorldView: get_payload_from_token(request)
alt User has permission
CreateWorldView->>World: Check if World exists
alt World exists
CreateWorldView->>World: Update World
else World does not exist
CreateWorldView->>World: Create new World
end
World-->>CreateWorldView: World object
CreateWorldView-->>User: 201 Created
else User lacks permission
CreateWorldView-->>User: 403 Forbidden
end
Architecture diagram for API endpoint integrationgraph TD;
subgraph Django Application
A[CreateWorldView]
B[World Model]
end
subgraph Django Settings
C[DOMAIN_PATH]
D[BASE_PATH]
end
A --> B
A --> C
A --> D
A -.->|POST /create-world/| User
Class diagram for CreateWorldViewclassDiagram
class CreateWorldView {
+post(request, *args, **kwargs) JsonResponse
+get_payload_from_token(request)
}
class World {
+id
+title
+domain
+locale
+timezone
+config
+save()
+create(id, title, domain, locale, timezone, config)
}
CreateWorldView --> World : uses
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @odkhang - I've reviewed your changes and found some issues that need to be addressed.
Blocking issues:
- Implement proper authentication and authorization for CreateWorldView (link)
Overall Comments:
- Consider implementing proper authentication and authorization instead of disabling them entirely for the CreateWorldView. This would improve the security of the API endpoint.
- The create and update operations for a World are currently mixed in a single block. Consider separating these into distinct methods for better code organization and maintainability.
Here's what I looked at during the review
- 🟡 General issues: 3 issues found
- 🔴 Security: 1 blocking issue, 1 other issue
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| @@ -115,6 +119,70 @@ def get(self, request, **kwargs): | |||
| ) | |||
|
|
|||
|
|
|||
| class CreateWorldView(APIView): | |||
There was a problem hiding this comment.
🚨 suggestion (security): Implement proper authentication and authorization for CreateWorldView
The current implementation with empty authentication_classes and permission_classes could allow unauthorized access to world creation. Consider implementing appropriate authentication and permission checks.
class CreateWorldView(APIView):
authentication_classes = [SessionAuthentication, BasicAuthentication]
permission_classes = [IsAdminUser]
| timezone=request.data.get('timezone'), | ||
| config=config, | ||
| ) | ||
| except Exception as e: |
There was a problem hiding this comment.
suggestion (bug_risk): Implement more granular exception handling
Catching a broad Exception can mask specific errors. Consider catching and handling specific exceptions (e.g., IntegrityError, ValidationError) to provide more informative error messages and easier debugging.
except (IntegrityError, ValidationError) as e:
logger.error(f"Error creating world: {e}")
return JsonResponse({'error': str(e)}, status=400)
except Exception as e:
logger.error(f"Unexpected error creating world: {e}")
return JsonResponse({'error': 'An unexpected error occurred'}, status=500)
There was a problem hiding this comment.
This is a bad practice to catch a very general exception class. This has been reminded before.
There was a problem hiding this comment.
Thank you for your feedback, we will generate fewer comments like this in the future according to the following instructions:
- Avoid suggesting changes that are already well-understood best practices unless there is a specific context or evidence that the current implementation is causing issues.
- Ensure that suggestions for code improvements are backed by specific examples or evidence of potential problems in the current code.
- Provide actionable and context-specific advice rather than general coding best practices.
server/venueless/api/views.py
Outdated
| raise exceptions.AuthenticationFailed( | ||
| "Invalid token header. Token string should not contain spaces." | ||
| ) | ||
| payload = jwt.decode(auth_header[1], settings.SECRET_KEY, algorithms=["HS256"]) |
There was a problem hiding this comment.
suggestion: Add exception handling for JWT decoding
JWT decoding can raise exceptions if the token is invalid or expired. Implement try-except blocks to handle potential jwt.DecodeError and jwt.ExpiredSignatureError exceptions.
try:
payload = jwt.decode(auth_header[1], settings.SECRET_KEY, algorithms=["HS256"])
except jwt.ExpiredSignatureError:
raise exceptions.AuthenticationFailed("Token has expired")
except jwt.DecodeError:
raise exceptions.AuthenticationFailed("Invalid token")
return payload
server/venueless/api/views.py
Outdated
| # if world already exists, update it, otherwise create a new world | ||
| try: | ||
| if World.objects.filter(id=request.data.get('id')).exists(): | ||
| world = World.objects.get(id=request.data.get('id')) |
There was a problem hiding this comment.
suggestion (bug_risk): Use .get() with default value or check key existence
Using .get() without a default value on request.data can raise KeyError if the key doesn't exist. Consider using .get('id', None) or check if 'id' in request.data before accessing it.
world_id = request.data.get('id')
if world_id is not None:
world = World.objects.get(id=world_id)
server/venueless/settings.py
Outdated
| @@ -40,6 +40,7 @@ | |||
| "VENUELESS_DJANGO_SECRET", config.get("django", "secret", fallback="") | |||
| ) | |||
| BASE_PATH = config.get("venueless", "base_path", fallback="") | |||
| DOMAIN_PATH = config.get("venueless", "domain_path", fallback="https://app.eventyay.com") | |||
There was a problem hiding this comment.
🚨 suggestion (security): Consider using environment variables for configuration
Using environment variables for configuration settings like DOMAIN_PATH can improve security and flexibility, especially when deploying in different environments.
| DOMAIN_PATH = config.get("venueless", "domain_path", fallback="https://app.eventyay.com") | |
| DOMAIN_PATH = os.environ.get('VENUELESS_DOMAIN_PATH', 'https://app.eventyay.com') |
server/venueless/api/views.py
Outdated
| ] | ||
| } | ||
|
|
||
| title_default = { |
There was a problem hiding this comment.
This impementation leave to many holes:
data.get()can returnNoneandNone.items()will throw exception.set.pop()will throw exception ifsetis empty.
Also, if you don't use key, retrieving values from dict instead.
server/venueless/api/views.py
Outdated
|
|
||
| # if world already exists, update it, otherwise create a new world | ||
| try: | ||
| if World.objects.filter(id=request.data.get("id")).exists(): |
There was a problem hiding this comment.
Data received from outside work (form, HTTP request) have to be validated.
You are calling
World.objects.get(id=request.data.get("id")but what if some attackers pass "id=abc"? This code will throw exception.
| timezone=request.data.get('timezone'), | ||
| config=config, | ||
| ) | ||
| except Exception as e: |
There was a problem hiding this comment.
This is a bad practice to catch a very general exception class. This has been reminded before.
server/venueless/api/views.py
Outdated
| raise exceptions.AuthenticationFailed( | ||
| "Invalid token header. Token string should not contain spaces." | ||
| ) | ||
| payload = jwt.decode(auth_header[1], settings.SECRET_KEY, algorithms=["HS256"]) |
server/venueless/api/views.py
Outdated
| world = World.objects.create( | ||
| id=request.data.get("id"), | ||
| title=request.data.get("title")[request.data.get("locale")] | ||
| or request.data.get("title")["en"] |
There was a problem hiding this comment.
What if data(get('title')) returns None or something not a dict. Validation needs to be applied to make sure the incoming data has the expected data type and shape.
As part of issue fossasia/eventyay-tickets#358
This PR is implement API create world which using by eventyay-common
Summary by Sourcery
Implement a new API endpoint to create or update a 'World' entity, including permission checks and JWT-based authentication. Update the settings to include a new 'DOMAIN_PATH' configuration for constructing world URLs.
New Features:
Enhancements: