diff --git a/change_implementation/concept_location.md b/change_implementation/concept_location.md new file mode 100644 index 0000000000..33849ce09b --- /dev/null +++ b/change_implementation/concept_location.md @@ -0,0 +1,93 @@ +# DRF Issue #6855 Concept Location + +## Problem Summary + +The browsable API can crash while rendering an extra action page when the response data comes from a serializer whose `instance` is not a normal model object for the current view's permission class. + +The failing path is in the browsable API renderer, not in view dispatch: + +1. The extra action returns `Response(serializer.data)`. +2. `serializer.data` is a `ReturnDict` or `ReturnList` that keeps a backlink to the serializer. +3. `BrowsableAPIRenderer` inspects that serializer during HTML rendering. +4. While building placeholder UI for other HTTP methods, it calls `view.check_object_permissions(request, instance)`. +5. `instance` may be a non-model object from the serializer, which can be incompatible with the permission class and crash with `AttributeError`. + +## Primary Source Locations + +### `rest_framework/renderers.py` + +This is the main issue location. + +- `BrowsableAPIRenderer.get_context()` + - Always asks for `put_form`, `post_form`, `delete_form`, and `options_form`. + - This means a normal `GET` render also evaluates synthetic UI for other methods. + +- `BrowsableAPIRenderer.get_rendered_html_form()` + - Pulls `serializer = getattr(data, 'serializer', None)`. + - Extracts `instance = getattr(serializer, 'instance', None)` when `many=False`. + - Calls `show_form_for_method()` before the current `DELETE` / `OPTIONS` bailout. + - This ordering is what exposes the bug. + +- `BrowsableAPIRenderer.show_form_for_method()` + - Calls `view.check_permissions(request)`. + - If `obj is not None`, also calls `view.check_object_permissions(request, obj)`. + - Only catches `APIException`, so an unexpected `AttributeError` from a permission class bubbles up and crashes rendering. + +## Supporting Locations + +### `rest_framework/utils/serializer_helpers.py` + +- `ReturnDict` +- `ReturnList` + +These wrappers preserve `data.serializer`, which is why renderers can see the original serializer and its `.instance`. + +### `rest_framework/views.py` + +- `APIView.check_object_permissions()` + +This loops through permission classes and directly passes the provided object to `has_object_permission()`. It assumes the caller supplied the correct domain object. + +### `rest_framework/request.py` + +- `override_method` + +`get_rendered_html_form()` uses this context manager while probing alternate HTTP methods such as `OPTIONS` and `DELETE`. + +## UI/Template Locations + +### `rest_framework/templates/rest_framework/base.html` + +- `options_form` +- `delete_form` + +These booleans control whether the browsable API shows the `OPTIONS` button and `DELETE` button/modal. + +### `rest_framework/templates/rest_framework/admin.html` + +- `delete_form` + +The admin renderer also consumes the truthiness of `delete_form`. + +## Existing Test Locations To Extend + +### `tests/test_renderers.py` + +Contains `BrowsableAPIRendererTests`, which is the closest existing unit/integration coverage for renderer behavior and extra actions. + +### `tests/browsable_api/test_form_rendering.py` + +Already has regression coverage around browsable API form generation and serializer shapes, including `many=True`. + +### `tests/browsable_api/views.py` + +Contains an example object-permission class that accesses nested attributes on the object and is useful as a pattern for reproducing this class of failure. + +## Key Conceptual Insight + +The renderer is mixing two different concepts: + +- the object that was serialized for display +- the object that should be used for permission checks when deciding whether to show action UI + +For issue #6855, the immediate crash is triggered by the synthetic `OPTIONS` UI path, where no object-level edit form is actually rendered, so reusing `serializer.instance` is not appropriate. diff --git a/change_implementation/impact_analysis.md b/change_implementation/impact_analysis.md new file mode 100644 index 0000000000..638c1ac4f1 --- /dev/null +++ b/change_implementation/impact_analysis.md @@ -0,0 +1,102 @@ +# DRF Issue #6855 Impact Analysis + +## Scope Of The Bug + +The bug affects HTML rendering through `BrowsableAPIRenderer`. It does not affect normal JSON responses or the primary view execution path. + +The issue appears when all of the following are true: + +1. A browsable API page is rendered. +2. The response data came from `serializer.data`, so the renderer can access `data.serializer`. +3. The serializer has `many=False`, so `serializer.instance` is used. +4. The serializer instance is not a safe object for the view's `has_object_permission()` logic. +5. The renderer probes another method, especially `OPTIONS`, during page construction. + +## Observable User Impact + +- The browsable API page crashes instead of rendering successfully. +- The failure happens after the view has already produced a valid response payload. +- API clients using non-HTML renderers are unaffected, so the bug can be invisible in automated API tests unless browsable rendering is exercised. + +## Why Extra Actions Are A Common Trigger + +Extra actions are more likely to serialize data that is not the same object type returned by `get_object()`. Examples include: + +- projection objects +- service-layer result objects +- denormalized DTO-like objects +- serializer-backed views that do not call `get_object()` + +That makes `serializer.instance` an unreliable proxy for object-permission checks during browsable API rendering. + +## Method-Level Impact + +### `OPTIONS` + +This is the most important path for issue #6855. + +- `OPTIONS` is typically present in `view.allowed_methods`. +- `get_context()` asks for `options_form` on normal page render. +- `get_rendered_html_form()` currently runs permission checks before its `OPTIONS` early return. +- No real serializer-bound form is produced for `OPTIONS`, so the object-level permission check is not necessary for rendering the placeholder button. + +### `DELETE` + +This method shares the same code shape, but the impact is different. + +- `DELETE` may or may not be allowed on the route. +- `delete_form` is used to decide whether to show destructive UI. +- Unlike `OPTIONS`, object-level permission checks may still be meaningful here because the button reflects an action against a resource. + +This means a broad "skip object checks for both `DELETE` and `OPTIONS`" change would be riskier than a focused `OPTIONS` fix. + +### `PUT` / `PATCH` + +These are not the direct cause of the reported crash for a GET-only extra action because they usually fail the `allowed_methods` check earlier. They remain important regression surface because they rely on `instance` to prepopulate edit forms. + +## Compatibility Risks + +### Low-risk changes + +- Preventing object-permission checks for the synthetic `OPTIONS` path. +- Keeping existing permission behavior for edit and delete affordances. + +### Medium-risk changes + +- Replacing `serializer.instance` with `view.get_object()` inside the renderer. + +This could: + +- trigger additional database queries +- fail on views that do not implement `get_object()` +- introduce side effects in renderer code +- blur the boundary between response rendering and view object retrieval + +### Higher-risk changes + +- Skipping object-permission checks for `DELETE` +- Removing `delete_form` behavior entirely + +Either could change which users see destructive UI and would need stronger review. + +## Testing Impact + +Regression coverage should include: + +1. A browsable API request to an extra action that returns `serializer.data` for a non-model object. +2. A permission class whose `has_object_permission()` would fail if handed that serializer instance. +3. Confirmation that the HTML response renders successfully instead of crashing. +4. Confirmation that existing delete/edit form behavior is not unintentionally broadened. + +Useful secondary coverage: + +- a direct renderer test for the `OPTIONS` path +- confirmation that `many=True` behavior remains unchanged + +## Documentation Impact + +No public API documentation change is likely required if the fix is internal and behavior-preserving. A release note entry may still be appropriate because the change resolves a user-visible browsable API crash. + +## Recommended Impact Conclusion + +The smallest safe fix is to narrow the change to the synthetic `OPTIONS` form-generation path. That addresses the reported crash while minimizing risk to object-permission-sensitive UI such as delete and edit actions. diff --git a/change_implementation/other_useful_info.md b/change_implementation/other_useful_info.md new file mode 100644 index 0000000000..1db9c80553 --- /dev/null +++ b/change_implementation/other_useful_info.md @@ -0,0 +1,71 @@ +## Quick Overview of Important Classes + +Inside `rest_framework/`, common areas are: + +`serializers.py`: converts Python/Django objects to API data and validates input. +`views.py`, `generics.py`, `viewsets.py`: request handling abstractions. +`routers.py`: URL routing for viewsets. +`permissions.py`, `authentication.py`, `throttling.py`: access control and API policies. +`renderers.py`, `parsers.py`: output/input formats like JSON and the browsable API. +`response.py`, `request.py`: DRF’s request/response wrappers. + +## Issue in Plain English + +Issue #6855 is a Browsable API bug. The actual API endpoint works, but the HTML page crashes while DRF is trying to decide which buttons/forms to show. + +The key flow is: + +- A view action returns `Response(serializer.data)`. +- In browsable mode, DRF looks at `data.serializer`. +- It pulls `serializer.instance`. +- It uses that instance for permission checks while deciding whether to show forms/buttons. +- For some extra actions, that serializer.instance is not the real model object the permission logic expects. +- Result: DRF calls object-permission logic on the wrong kind of object and crashes. + +This is the critical code path: + +```python +# renderers.py +def show_form_for_method(self, view, method, request, obj): + """ + Returns True if a form should be shown for this method. + """ + if method not in view.allowed_methods: + return # Not a valid method + + try: + view.check_permissions(request) + if obj is not None: + view.check_object_permissions(request, obj) +``` + +And this is where the renderer derives the object from the serializer and then asks for forms for several methods: + +```python +# renderers.py +# See issue #2089 for refactoring this. +serializer = getattr(data, 'serializer', None) +if serializer and not getattr(serializer, 'many', False): + instance = getattr(serializer, 'instance', None) + if isinstance(instance, Page): + instance = None +else: + instance = None + +with override_method(view, request, method) as request: + if not self.show_form_for_method(view, method, request, instance): + return + + if method in ('DELETE', 'OPTIONS'): + return True # Don't actually need to return a form +``` + +And the browsable page always probes multiple methods, even on a normal GET render: + +```python +# renderers.py +'put_form': self.get_rendered_html_form(data, view, 'PUT', request), +'post_form': self.get_rendered_html_form(data, view, 'POST', request), +'delete_form': self.get_rendered_html_form(data, view, 'DELETE', request), +'options_form': self.get_rendered_html_form(data, view, 'OPTIONS', request), +``` diff --git a/change_implementation/source_code_strategy.md b/change_implementation/source_code_strategy.md new file mode 100644 index 0000000000..f613d8ffc3 --- /dev/null +++ b/change_implementation/source_code_strategy.md @@ -0,0 +1,93 @@ +# DRF Issue #6855 Source Code Strategy + +## Goal + +Fix the browsable API crash for extra actions that return serializer-backed data whose `serializer.instance` is not a valid object for the view's object-permission logic. + +## Recommended Strategy + +Implement a focused change in `rest_framework/renderers.py` so that the synthetic `OPTIONS` button path does not call `check_object_permissions()` on `serializer.instance`. + +## Why This Strategy + +It directly addresses the reported crash with the smallest behavioral change. + +It also preserves current behavior for methods where object-level permission checks are still meaningful: + +- `PUT` +- `PATCH` +- `DELETE` + +## Proposed Code Change + +### Primary change + +Refactor `BrowsableAPIRenderer.get_rendered_html_form()` so that `OPTIONS` is handled before `show_form_for_method(view, method, request, instance)` is called. + +Recommended behavior: + +1. Enter `override_method(view, request, method)` as today. +2. If `method == 'OPTIONS'`: + - return early when the method is not allowed + - run `view.check_permissions(request)` + - do not run `view.check_object_permissions(request, instance)` + - return `True` so the template can render the `OPTIONS` button +3. For all other methods, keep the existing `show_form_for_method()` flow. +4. Keep the existing `DELETE` truthy return after permission gating. + +## Why Not Use `view.get_object()` + +Avoid changing the renderer to retrieve a new object from the view. + +Reasons: + +- not every relevant view implements `get_object()` +- extra actions may intentionally not be tied to the routed object +- renderer-time object lookup can introduce extra queries and side effects +- it is a larger semantic change than needed for this bug + +## Why Not Skip Checks For Both `DELETE` And `OPTIONS` + +That would be simpler mechanically, but it risks changing existing UI authorization behavior for delete actions. + +`OPTIONS` is different because the renderer does not build an object-bound form for it. It only needs enough permission context to decide whether the method is generally accessible. + +## Suggested Test Plan + +### Regression test + +Add a renderer-focused regression test in `tests/test_renderers.py` near `BrowsableAPIRendererTests`. + +Suggested test shape: + +1. Define a `ViewSet` with browsable API enabled. +2. Add a detail extra action that returns `Response(serializer.data)`. +3. Use a serializer whose `instance` is a custom object without the attributes expected by object permissions. +4. Attach a permission class whose `has_object_permission()` would raise `AttributeError` if called with that custom object. +5. Request the extra action with `HTTP_ACCEPT='text/html'`. +6. Assert that the response renders successfully with status `200`. + +### Guardrail test + +Add a small test confirming that delete UI still respects the existing permission gate, or at minimum ensure no existing delete-related renderer test regresses when the new test is added. + +### Optional unit test + +If maintainers prefer tighter isolation, add a direct unit test around `get_rendered_html_form(..., method='OPTIONS', ...)` to assert the method returns truthy without consulting object permissions. + +## Implementation Notes + +- Keep the fix local to `BrowsableAPIRenderer`; avoid changing core permission APIs. +- Do not broaden exception handling to swallow arbitrary `AttributeError`. That would hide real bugs instead of correcting the invalid permission-check input. +- Preserve the current `many=True` behavior, where `instance` is already treated as `None`. + +## Validation Steps + +1. Run the new regression test. +2. Run existing browsable API renderer tests. +3. Verify no change in non-HTML renderer behavior. +4. Manually confirm that an authenticated browsable API extra action page now renders instead of crashing. + +## Fallback Option + +If reviewers want a more centralized fix, a secondary approach is to teach `show_form_for_method()` to skip object-permission checks only for `OPTIONS`. That is still acceptable, but the `get_rendered_html_form()`-level change keeps the special case closest to the synthetic-form behavior that causes the bug.