tfranzel / drf-spectacular

Sane and flexible OpenAPI 3 schema generation for Django REST framework.
https://drf-spectacular.readthedocs.io
BSD 3-Clause "New" or "Revised" License
2.39k stars 264 forks source link

Response code 401 missing #622

Open jayvdb opened 2 years ago

jayvdb commented 2 years ago

Describe the bug When a DRF viewset has permission_classes, a 401 response should be in the schema for the endpoints. This is a special case of https://github.com/tfranzel/drf-spectacular/issues/101 , and it is both a simpler case than other 400 codes because the response structure is typically constant, and also more prominent that other 400 codes as 401 is encountered in normal/correct use of the schema .

drf-spectacular already includes security: in the endpoints, so it already knows the endpoint uses authentication and that 401 is expected.

To Reproduce Generate a schema of an endpoint which used stock standard DRF authentication:

class FooViewSet(ModelViewSet):
    permission_classes = [IsAuthenticated]
    queryset = Foo.objects.all()
    serializer_class = FooSerializer

Expected behavior The schema should include a 401 in responses

It can be as simple as responses['401'] = {'description': ''}. That at least is sufficient for schemathesis's status_code_conformance check.

tfranzel commented 2 years ago

Hey @jayvdb, i see! Since you also commented on #101, you are probably aware this entails a whole package things. I believe we should have a setting to turn on default error status code responses. 401 is one of the easier sub-tasks there. I did not tackle easy sub-tasks to make this a cohesive feature. backtracking bad decisions is hard as you know.

I will have a look at this again, when I have some spare time.

sergei-maertens commented 2 years ago

Note that permission_classes can also be set to (permissions.AllowAny,) which does not require authentication, so the 401 response should not be included in those cases.

With other possible custom permission classes (which we have, based on request.session), it's not correct to conclude "any permission classes set -> 401 should be included". It's also not possible (to my knowledge) to introspect the permission classes to figure out if they require authentication or not.

Furthermore, DRF itself does some stuff w/r to HTTP 401 responses - it depends on how the authentication class is implement. So, to automatically document 401 responses, I believe the correct approach would be to look at authentication_classes rather than permission_classes, and introspect those. See https://github.com/encode/django-rest-framework/blob/master/rest_framework/views.py#L455 for the DRF implementation

tfranzel commented 2 years ago

Note that permission_classes can also be set to (permissions.AllowAny,) which does not require authentication, so the 401 response should not be included in those cases.

if you have a authentication_class and e.g. provide an invalid token, you will get a 401 even though you have AllowAny and it would have worked without sending auth credentials. So that statement is not really correct.

It's also not possible (to my knowledge) to introspect the permission classes to figure out if they require authentication or not.

yeah... no way to know for sure without extra mechanics for extensions that explicitly state the possible error this auth might incur.