From 1b2eddc013ef9bef4e656f466a452ba6254399f6 Mon Sep 17 00:00:00 2001 From: Dale Foerster Date: Tue, 17 Mar 2026 13:09:32 -0700 Subject: [PATCH 1/2] + add documentation for change strategy --- change_implementation/concept_location.md | 93 ++++++++++++++++ change_implementation/impact_analysis.md | 102 ++++++++++++++++++ change_implementation/other_useful_info.md | 71 ++++++++++++ change_implementation/source_code_strategy.md | 93 ++++++++++++++++ 4 files changed, 359 insertions(+) create mode 100644 change_implementation/concept_location.md create mode 100644 change_implementation/impact_analysis.md create mode 100644 change_implementation/other_useful_info.md create mode 100644 change_implementation/source_code_strategy.md 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. From 9072304e3425baf98285966fdb58c4a901643483 Mon Sep 17 00:00:00 2001 From: Dale Foerster Date: Tue, 31 Mar 2026 10:52:12 -0700 Subject: [PATCH 2/2] Fix browsable API crash when rendering OPTIONS for extra actions --- change_implementation/concept_location.md | 93 ---------------- change_implementation/impact_analysis.md | 102 ------------------ change_implementation/other_useful_info.md | 71 ------------ change_implementation/source_code_strategy.md | 93 ---------------- rest_framework/renderers.py | 11 ++ tests/test_renderers.py | 53 +++++++++ 6 files changed, 64 insertions(+), 359 deletions(-) delete mode 100644 change_implementation/concept_location.md delete mode 100644 change_implementation/impact_analysis.md delete mode 100644 change_implementation/other_useful_info.md delete mode 100644 change_implementation/source_code_strategy.md diff --git a/change_implementation/concept_location.md b/change_implementation/concept_location.md deleted file mode 100644 index 33849ce09b..0000000000 --- a/change_implementation/concept_location.md +++ /dev/null @@ -1,93 +0,0 @@ -# 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 deleted file mode 100644 index 638c1ac4f1..0000000000 --- a/change_implementation/impact_analysis.md +++ /dev/null @@ -1,102 +0,0 @@ -# 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 deleted file mode 100644 index 1db9c80553..0000000000 --- a/change_implementation/other_useful_info.md +++ /dev/null @@ -1,71 +0,0 @@ -## 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 deleted file mode 100644 index f613d8ffc3..0000000000 --- a/change_implementation/source_code_strategy.md +++ /dev/null @@ -1,93 +0,0 @@ -# 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. diff --git a/rest_framework/renderers.py b/rest_framework/renderers.py index 6d7218bbcf..8898da9865 100644 --- a/rest_framework/renderers.py +++ b/rest_framework/renderers.py @@ -499,6 +499,17 @@ def get_rendered_html_form(self, data, view, method, request): existing_serializer = None with override_method(view, request, method) as request: + if method == 'OPTIONS': + # The browsable API only needs a placeholder for OPTIONS, so + # avoid object-level permission checks against serializer.instance. + if method not in view.allowed_methods: + return + try: + view.check_permissions(request) + except exceptions.APIException: + return + return True + if not self.show_form_for_method(view, method, request, instance): return diff --git a/tests/test_renderers.py b/tests/test_renderers.py index e5c33e6b49..0a21520ccc 100644 --- a/tests/test_renderers.py +++ b/tests/test_renderers.py @@ -717,9 +717,34 @@ class AuthExampleViewSet(ExampleViewSet): class SimpleSerializer(serializers.Serializer): name = serializers.CharField() + class CrashOnObjectPermission(permissions.BasePermission): + def has_permission(self, request, view): + return True + + def has_object_permission(self, request, view, obj): + return obj.user.is_staff + + class Issue6855Serializer(serializers.Serializer): + name = serializers.CharField() + + class Issue6855Object: + def __init__(self, name): + self.name = name + + class Issue6855ViewSet(ViewSet): + @action(detail=True) + def extra_action(self, request, pk=None): + serializer = BrowsableAPIRendererTests.Issue6855Serializer( + BrowsableAPIRendererTests.Issue6855Object(name='demo') + ) + return Response(serializer.data) + + Issue6855ViewSet.permission_classes = [CrashOnObjectPermission] + router = SimpleRouter() router.register('examples', ExampleViewSet, basename='example') router.register('auth-examples', AuthExampleViewSet, basename='auth-example') + router.register('issue-6855', Issue6855ViewSet, basename='issue-6855') urlpatterns = [path('api/', include(router.urls))] def setUp(self): @@ -806,6 +831,34 @@ def test_extra_actions_dropdown_not_authed(self): assert '/api/examples/list_action/' not in resp.content.decode() assert '>Extra list action<' not in resp.content.decode() + def test_options_form_does_not_check_object_permissions_for_extra_action(self): + resp = self.client.get('/api/issue-6855/1/extra_action/', HTTP_ACCEPT='text/html') + assert resp.status_code == status.HTTP_200_OK + + def test_delete_form_still_checks_object_permissions(self): + class ObjectPermissionDenied(permissions.BasePermission): + def has_permission(self, request, view): + return True + + def has_object_permission(self, request, view, obj): + return False + + class DummyObject: + name = 'Name' + + class DummyDeleteView(APIView): + permission_classes = [ObjectPermissionDenied] + + def delete(self, request): + return Response() + + request = Request(APIRequestFactory().get('/')) + serializer = BrowsableAPIRendererTests.SimpleSerializer(instance=DummyObject()) + delete_form = self.renderer.get_rendered_html_form( + serializer.data, DummyDeleteView(), 'DELETE', request + ) + assert delete_form is None + class AdminRendererTests(TestCase):