Skip to content

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.

Sign up for GitHub

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

Merged
devgunnu merged 1 commit into from
Feb 2, 2026
Merged

fixed save issue #51

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion project/backend/settings.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -154,8 +154,9 @@

REST_FRAMEWORK = {
"DEFAULT_PERMISSION_CLASSES": [
"rest_framework.permissions.IsAuthenticatedOrReadOnly",
"rest_framework.permissions.AllowAny", # Views handle auth via Firebase middleware
Copy link

Copilot AI Feb 2, 2026

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.

Suggested change
"rest_framework.permissions.AllowAny", # Views handle auth via Firebase middleware
"rest_framework.permissions.IsAuthenticatedOrReadOnly", # Firebase middleware should set request.user

Copilot uses AI. Check for mistakes.
],
"DEFAULT_AUTHENTICATION_CLASSES": [], # Disable DRF auth, using Firebase middleware
Comment on lines +157 to +159
Copy link

Copilot AI Feb 2, 2026

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:

  1. Incomplete view-level protection: Many views in block_manager/views/architecture_views.py (like get_node_definitions line 232, get_node_definition line 274, render_node_code line 315) and group_views.py (lines 10-72) lack any authentication checks. With AllowAny permission, these are now publicly accessible.

  2. Inconsistent security model: The pattern is inconsistent - save_architecture and load_architecture manually check request.firebase_user, while export_model uses @require_authentication decorator, but many other views have no checks at all.

  3. 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 IsAuthenticatedOrReadOnly or similar permission class
  • Create a custom DRF authentication backend that integrates with Firebase middleware (setting request.user from request.firebase_user)
  • Or systematically apply @require_authentication decorator to all endpoints that modify data
  • Add explicit @permission_classes([AllowAny]) only to endpoints that genuinely need public access (like validation, chat suggestions)
Suggested change
"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",
],

Copilot uses AI. Check for mistakes.
Comment on lines +157 to +159
Copy link

Copilot AI Feb 2, 2026

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".

Copilot uses AI. Check for mistakes.
Copy link

Copilot AI Feb 2, 2026

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:

  1. DRF browsable API will no longer work properly for authenticated requests
  2. Third-party packages expecting standard DRF authentication patterns may break
  3. 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

Copilot uses AI. Check for mistakes.
"DEFAULT_THROTTLE_CLASSES": [
"rest_framework.throttling.AnonRateThrottle",
"rest_framework.throttling.UserRateThrottle",
Expand Down
Loading