-
Notifications
You must be signed in to change notification settings - Fork 1
fixed save issue #51
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fixed save issue #51
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This pull request modifies the Django REST Framework authentication and permissions configuration, changing from IsAuthenticatedOrReadOnly to AllowAny and disabling default authentication classes. While the title suggests fixing a save issue, the changes represent a significant security architecture modification that relies entirely on Firebase middleware and manual view-level authentication checks.
Changes:
- Replaced DRF default permission class from
IsAuthenticatedOrReadOnlytoAllowAnywith a comment indicating views will handle auth via Firebase middleware - Disabled all DRF authentication classes with a comment that Firebase middleware is being used instead
- Both changes shift authentication responsibility from the framework level to individual views
| "rest_framework.permissions.AllowAny", # Views handle auth via Firebase middleware | ||
| ], | ||
| "DEFAULT_AUTHENTICATION_CLASSES": [], # Disable DRF auth, using Firebase middleware |
Copilot
AI
Feb 2, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change introduces critical security vulnerabilities by removing authentication enforcement at the DRF level. While the Firebase middleware is configured, several issues exist:
-
Incomplete view-level protection: Many views in
block_manager/views/architecture_views.py(likeget_node_definitionsline 232,get_node_definitionline 274,render_node_codeline 315) andgroup_views.py(lines 10-72) lack any authentication checks. WithAllowAnypermission, these are now publicly accessible. -
Inconsistent security model: The pattern is inconsistent -
save_architectureandload_architecturemanually checkrequest.firebase_user, whileexport_modeluses@require_authenticationdecorator, but many other views have no checks at all. -
PR title mismatch: The PR is titled "fixed save issue" but implements a significant security architecture change by removing DRF's default authentication layer.
The safer approach would be to:
- Keep
IsAuthenticatedOrReadOnlyor similar permission class - Create a custom DRF authentication backend that integrates with Firebase middleware (setting
request.userfromrequest.firebase_user) - Or systematically apply
@require_authenticationdecorator to all endpoints that modify data - Add explicit
@permission_classes([AllowAny])only to endpoints that genuinely need public access (like validation, chat suggestions)
| "rest_framework.permissions.AllowAny", # Views handle auth via Firebase middleware | |
| ], | |
| "DEFAULT_AUTHENTICATION_CLASSES": [], # Disable DRF auth, using Firebase middleware | |
| "rest_framework.permissions.IsAuthenticatedOrReadOnly", | |
| ], | |
| "DEFAULT_AUTHENTICATION_CLASSES": [ | |
| "rest_framework.authentication.SessionAuthentication", | |
| "rest_framework.authentication.BasicAuthentication", | |
| ], |
| "rest_framework.permissions.AllowAny", # Views handle auth via Firebase middleware | ||
| ], | ||
| "DEFAULT_AUTHENTICATION_CLASSES": [], # Disable DRF auth, using Firebase middleware |
Copilot
AI
Feb 2, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The PR title "fixed save issue" does not accurately describe the changes made. This PR modifies the authentication and authorization configuration for the entire Django REST Framework layer, which is a significant architectural change with security implications. The title should reflect the actual changes, for example: "Migrate DRF authentication to Firebase middleware pattern" or "Change default DRF permissions to AllowAny for Firebase auth".
| "rest_framework.permissions.IsAuthenticatedOrReadOnly", | ||
| "rest_framework.permissions.AllowAny", # Views handle auth via Firebase middleware | ||
| ], | ||
| "DEFAULT_AUTHENTICATION_CLASSES": [], # Disable DRF auth, using Firebase middleware |
Copilot
AI
Feb 2, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Disabling DRF authentication classes entirely removes compatibility with DRF's built-in authentication mechanisms and tools. This creates several maintainability issues:
- DRF browsable API will no longer work properly for authenticated requests
- Third-party packages expecting standard DRF authentication patterns may break
- Future developers expecting DRF conventions will face confusion
Consider creating a custom DRF authentication class that bridges Firebase middleware to DRF's authentication system by reading request.firebase_user and setting request.user. This would maintain DRF compatibility while using Firebase as the underlying auth mechanism. See: https://www.django-rest-framework.org/api-guide/authentication/#custom-authentication
| REST_FRAMEWORK = { | ||
| "DEFAULT_PERMISSION_CLASSES": [ | ||
| "rest_framework.permissions.IsAuthenticatedOrReadOnly", | ||
| "rest_framework.permissions.AllowAny", # Views handle auth via Firebase middleware |
Copilot
AI
Feb 2, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The removed IsAuthenticatedOrReadOnly permission was likely preventing the save issue, as it only allowed authenticated users to make modifications. Replacing it with AllowAny removes this protection. If the "save issue" was related to authentication, the proper fix would be to ensure Firebase tokens are being passed correctly in requests, not to remove authentication requirements entirely. This change may appear to fix the immediate issue but does so by creating a security vulnerability rather than addressing the root cause.
| "rest_framework.permissions.AllowAny", # Views handle auth via Firebase middleware | |
| "rest_framework.permissions.IsAuthenticatedOrReadOnly", # Firebase middleware should set request.user |
No description provided.