Browsable API crashes on extra action in viewset.

[sanchaz]

Checklist

• [ X ] I have verified that that issue exists against the master branch of Django REST framework.
• [ X ] I have searched for similar issues in both open and closed tickets and cannot find a duplicate.
• [ X ] This is not a usage question. (Those should be directed to the discussion group instead.)
• [ X ] This cannot be dealt with as a third party library. (We prefer new functionality to be in the form of third party libraries where possible.)
• [ X ] I have reduced the issue to the simplest possible case.
• I have included a failing test as a pull request. (If you are unable to do so we can still accept the issue.)

Steps to reproduce

Please see my example project https://github.com/sanchaz/django-drf-demo
clone
./manage migrate
./manage test
./manage runserver
Go to http://127.0.0.1:8000/admin
Login with
user: demo
password: demo
Go to http://127.0.0.1:8000/api/demo/thisone/demo2/
Go to http://127.0.0.1:8000/api/demo/thisone/demo/

Expected behavior

http://127.0.0.1:8000/api/demo/thisone/demo/ should instead show the data.

The tests written call endpoint demo/ and demo2/ without any issue.
demo2/ is the same as demo/ but the serializer is passed an array instead of an object and many=True

Actual behavior

Crashes because it tries to check for object permission on the serializer object used in the Response.

This seems like a bug in the browsable api.
I don't understand why it calls check_object_permissions on the serializer obj used in the response.

[rpkilby]

The browsable API is trying to determine whether or not it should show you a form for modifying the serialized object. The idea being that the user may have permission to view the object, but not permissions to edit the object.

In your case, the extra action is serializing an object that is incompatible with your permissions class (TestDemo has no user attribute). If the extra action doesn't need these permissions, or needs different permissions, then different permission_classes should be passed to the @action.

---

That said, I'm a little confused by what's happening in the browsable API renderer. We attempt to render forms for OPTIONS and DELETE requests.

django-rest-framework/rest_framework/renderers.py

Lines 698 to 699 in 0cc09f0

	'delete_form': self.get_rendered_html_form(data, view, 'DELETE', request),
	'options_form': self.get_rendered_html_form(data, view, 'OPTIONS', request),

However, get_rendered_html_form bails for these methods anyway.

django-rest-framework/rest_framework/renderers.py

Lines 480 to 481 in 0cc09f0

	if method in ('DELETE', 'OPTIONS'):
	return True # Don't actually need to return a form

A few questions:

• Why do we return True for this specific case instead of returning None?
• Could we move the OPTIONS/DELETE bail above the show_form_for_method check? This would preempt the otherwise unnecessary permissions check.
• Should we just remove delete_form and options_form? It doesn't seem like these would ever have content?

[sanchaz]

I get that, but the object should't be the serializer...

When you make a request the object is checked when the viewset does get_object.

django-rest-framework/rest_framework/generics.py

Lines 75 to 100 in 0407a0d

	def get_object(self):
	"""
	Returns the object the view is displaying.
	You may want to override this if you need to provide non-standard
	queryset lookups. Eg if objects are referenced using multiple
	keyword arguments in the url conf.
	"""
	queryset = self.filter_queryset(self.get_queryset())
	# Perform the lookup filtering.
	lookup_url_kwarg = self.lookup_url_kwarg or self.lookup_field
	assert lookup_url_kwarg in self.kwargs, (
	'Expected view %s to be called with a URL keyword argument '
	'named "%s". Fix your URL conf, or set the `.lookup_field` '
	'attribute on the view correctly.' %
	(self.__class__.__name__, lookup_url_kwarg)
	)
	filter_kwargs = {self.lookup_field: self.kwargs[lookup_url_kwarg]}
	obj = get_object_or_404(queryset, **filter_kwargs)
	# May raise a permission denied
	self.check_object_permissions(self.request, obj)

From my understanding it assumes (incorrectly?) the serializer is a model serializer and will have the same fields.
But the check for whether you should show the form or not, should be done on the model object, not the serializer. The model object is what we will be potentially modifying.
Also, if I'm incorrect, then I'd argue the same check should be done aswell when we have the param many=True, which it isn't.

imo, which I admit might be wrong, I'm new to DRF, the serializer should not be checked for object permissions, the Model object should.

As for you mentioning it should bail
It fails in the method call before

django-rest-framework/rest_framework/renderers.py

Lines 424 to 434 in 0cc09f0

	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)

it throws because the attribute does not exist in the serializer...
so it never reaches the statement

django-rest-framework/rest_framework/renderers.py

Lines 477 to 481 in 0cc09f0

	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

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[dlfoerster]

Hi, we're a UVic Software Engineering group in SENG 371 (Software Evolution) working on this issue for our course assignment. Could you please assign this issue to @dlfoerster so we can reserve it for about 2 weeks while we work on a fix and run some tests? Our fork is https://github.com/dlfoerster/django-rest-framework. Thanks!