Respond with 401 for requests with bad credentials

[sarayourfriend]

Fixes

Fixes #3626 by @sarayourfriend
Fixes #4127 by @obulat

Description

Extends the oauth2 provider's DRF authentication class to raise the authentication error exception. Refer to DRF documentation for how these authentication classes are meant to work: https://www.django-rest-framework.org/api-guide/authentication/

Testing Instructions

Checkout the branch and run the API using just api/up && just api/init. Run just logs web in a separate console, and then follow the registration documentation in the API to create a registration token locally. You'll need the verification URL printed in the logs instead of sent in a real email after you register.

Confirm you can send requests with tokens. Confirm you can send anonymous requests. Confirm requests with invalid tokens are rejected with 401.

Checklist

• My pull request has a descriptive title (not a vague title likeUpdate index.md).
• My pull request targets the default branch of the repository (main) or a parent feature branch.
• My commit messages follow best practices.
• My code follows the established code style of the repository.
• I added or updated tests for the changes I made (if applicable).
• I added or updated documentation (if applicable).
• I tried running the project locally and verified that there are no visible errors.
• [N/A] I ran the DAG documentation generator (if applicable).

Developer Certificate of Origin

Developer Certificate of Origin

Developer Certificate of Origin
Version 1.1

Copyright (C) 2004, 2006 The Linux Foundation and its contributors.
1 Letterman Drive
Suite D4700
San Francisco, CA, 94129

Everyone is permitted to copy and distribute verbatim copies of this
license document, but changing it is not allowed.


Developer's Certificate of Origin 1.1

By making a contribution to this project, I certify that:

(a) The contribution was created in whole or in part by me and I
    have the right to submit it under the open source license
    indicated in the file; or

(b) The contribution is based upon previous work that, to the best
    of my knowledge, is covered under an appropriate open source
    license and I have the right under that license to submit that
    work with modifications, whether created in whole or in part
    by me, under the same open source license (unless I am
    permitted to submit under a different license), as indicated
    in the file; or

(c) The contribution was provided directly to me by some other
    person who certified (a), (b) or (c) and I have not modified
    it.

(d) I understand and agree that this project and the contribution
    are public and that a record of the contribution (including all
    personal information I submit with it, including my sign-off) is
    maintained indefinitely and may be redistributed consistent with
    this project or the open source license(s) involved.

[obulat]

Tested this PR locally, and it works well. There is, however the following warning:

[2024-04-16 10:02:40,469 - log_request_id.middleware -  55][INFO] [77950d359f2642a488e49639f60e7c19] method=GET path=/v1/ status=200
/api/api/views/audio_views.py: Warning [AudioViewSet]: could not resolve authenticator <class 'conf.oauth2_extensions.OAuth2Authentication'>. There was no OpenApiAuthenticationExtension registered for that class. Try creating one by subclassing it. Ignoring for now.

I'm not sure how this can be fixed, though.

[sarayourfriend]

Ah, I didn't notice that warning. I'll look into it, I've probably missed some configuration detail.

[sarayourfriend]

I ended up going down a bit of a rabbit hole fixing that warning, and ended up also fixing #4127 @obulat.

I'll push a commit shortly with those changes.

[sarayourfriend]

Well, I got stuck trying to fix the documentation errors.

I fixed some of them, and corrected the authentication classes and permissions on the token views to align with the authentication class... but now the test_schema tests fail with bizarre errors about invalid HTTP requests, that I just don't understand and cannot replicate locally 😢

Do you have any ideas @obulat ?

[obulat]

I fixed some of them, and corrected the authentication classes and permissions on the token views to align with the authentication class... but now the test_schema tests fail with bizarre errors about invalid HTTP requests, that I just don't understand and cannot replicate locally 😢

You can see what [Redacted] means when you run the tests locally. Schemathesis is sending Bearer \x1f token, which is probably causing an error somewhere in the auth class or in Django itself, because it's a non-letter Unit Separator character .

This discussion suggests removing all "Authorization" headers from cases. I tried only removing the "Bearer \x1f", and almost all cases passed.

@schema.parametrize()
def test_schema(case):
    if case.headers.get("Authorization") == "Bearer \x1f":  # This is the token that's causing 400. We will never generate such a token, so we don't need to test it, right?
        case.headers.pop("Authorization", None)
    case.call_and_validate()

There seems to be only one undocumented case left with an invalid client:

case = Case(body={'client_id': '', 'client_secret': '', 'grant_type': 'client_credentials'})

    @schema.parametrize()
    def test_schema(case):
        if case.headers.get("Authorization") == "Bearer \x1f":
            case.headers.pop("Authorization", None)
>       case.call_and_validate()
E       schemathesis.exceptions.CheckFailed: 
E       
E       1. Undocumented HTTP status code
E       
E           Received: 401
E           Documented: 200, 400
E       
E       [401] Unauthorized:
E       
E           `{"error":"invalid_client"}`
E       
E       Reproduce with: 
E       
E           curl -X POST -H 'Content-Type: application/x-www-form-urlencoded' -d 'client_id=&client_secret=&grant_type=client_credentials' http://localhost:50280/v1/auth_tokens/token/

[obulat]

This should probably be added back for the "invalid_client" error.

[sarayourfriend]

Thanks for finding these solutions! I'll push them next week, drafting this for now.

[sarayourfriend]

You can see what [Redacted] means when you run the tests locally. Schemathesis is sending Bearer \x1f token, which is probably causing an error somewhere in the auth class or in Django itself, because it's a non-letter Unit Separator character .

Yes, but when I try to send that myself I can't reproduce the issue 🤔 I can't tell if it's a real problem though, does this open up a vulnerability for someone to overwhelm the service with bad authentication tokens and cause a bunch of errors?

[Stranger6667]

FYI, I am going to add a config option to Schemathesis so it is easier to tune what header values it should generate (schemathesis/schemathesis#2137), so you won’t have to manually adjust the headers

[obulat]

Yes, but when I try to send that myself I can't reproduce the issue 🤔 I can't tell if it's a real problem though, does this open up a vulnerability for someone to overwhelm the service with bad authentication tokens and cause a bunch of errors?

I couldn't reproduce this locally either. I also couldn't reproduce this error in prod with \x1, so I guess it will not cause a vulnerability

[sarayourfriend]

Thanks, @Stranger6667! I've subscribed to the issue 🙂

Rebasing this PR now and will push the changes Olga suggested as a temporary workaround. Thanks again, Olga.

[dhruvkb]

LGTM 🚀!

I don't feel too good about having to specify 401 for each authenticated endpoint individually but that's an extremely small nit and definitely should not shadow all the good things in this PR.

[sarayourfriend]

I don't feel too good about having to specify 401 for each authenticated endpoint individually but that's an extremely small nit and definitely should not shadow all the good things in this PR.

There's probably some way to get drf-spectacular to infer it based on the authentication classes on a view, but our current way of detaching docs/schema extensions from the views themselves made it hard to reason about, and I didn't spend much time on it. Feel free to open an issue for that, though, would be nice if we could make this a little less fiddly, configuration wise.