Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

deal with 404 or 405 validator error #1499

Merged
merged 3 commits into from
Sep 20, 2024
Merged

Conversation

n2ygk
Copy link
Member

@n2ygk n2ygk commented Sep 16, 2024

Fixes #1497 unrelated tox failures

Description of the Change

The test mocked validation endpoint sometimes returns a 404 or a 405. It's not clear where this changed
but it first appeared in #1497 which was unrelated.

It turns out the test neglected to mock the POST endpoint so the POST was being sent to the live example.com website. Apparently example.com changed their response from a 404 to a 405.

The fix mocks the endpoint to always return a 404 Not Found.

Checklist

  • PR only contains one change (considered splitting up PR)
  • unit-test added
  • documentation updated
  • CHANGELOG.md updated (only for user relevant changes)
  • author name in AUTHORS

@n2ygk n2ygk requested a review from a team September 16, 2024 20:48
@dopry
Copy link
Contributor

dopry commented Sep 18, 2024

I would be more confident this is an appropriate fix if we knew why we were getting the 405. If this varies with Django Version it would like to see a check against the django version to determine the correct assertion, or a comment explaining why we get differing response. Is this solely the result of the django version or is pip resolving a different version of OAuthLib for different django versions?

@n2ygk
Copy link
Member Author

n2ygk commented Sep 18, 2024

I would be more confident this is an appropriate fix if we knew why we were getting the 405. If this varies with Django Version it would like to see a check against the django version to determine the correct assertion, or a comment explaining why we get differing response. Is this solely the result of the django version or is pip resolving a different version of OAuthLib for different django versions?

I've been trying to find the source. It "suddenly" broke for < djmain which is unexpected.

@dopry
Copy link
Contributor

dopry commented Sep 18, 2024

Maybe this filtered in from a dependency. Can the stack trace lead us back to the raise to figure out which module the 405 is originating in?

@n2ygk
Copy link
Member Author

n2ygk commented Sep 18, 2024

Maybe this filtered in from a dependency. Can the stack trace lead us back to the raise to figure out which module the 405 is originating in?

I've been trying to find it. It's a response from a mocked HTTP server so the stack trace ends at the request. I tried drilling down for quite a while and was unable to find where it's coming from. Must be in Django somewhere since it varies based on django version

@dopry
Copy link
Contributor

dopry commented Sep 18, 2024

Where and how is the mocked http server setup? Maybe we can get into it? I looked into OAuthLib no new releases there. Doesn't look like anything changed in Django so maybe the mocking library changed it's default response.

@n2ygk
Copy link
Member Author

n2ygk commented Sep 19, 2024

@dopry I'll do some more digging...

@n2ygk
Copy link
Member Author

n2ygk commented Sep 19, 2024

I'm not sure where something changed, but now I am unable to reproduce locally. All flavors of py*-dj* are consistently returning a 405. I'm rerunning the jobs for #1497 with debug logging to see what happens.

@n2ygk
Copy link
Member Author

n2ygk commented Sep 19, 2024

@dopry I think I found it. https://www.python.org/downloads/release/python-31015/ was released 9/7/24. However the Python 10 I have locally is Python 3.10.13. Mock is part of cpython. There are a lot of bug fixes listed referencing mock. Investigating further....

@dopry
Copy link
Contributor

dopry commented Sep 19, 2024

I wonder if it could have something to do with

not explicitly setting a status?

@n2ygk
Copy link
Member Author

n2ygk commented Sep 19, 2024

I wonder if it could have something to do with

not explicitly setting a status?

I tried to set a breakpoint there and the function is never reached. I think this is happening way earlier as the 405 is a method not allowed (POST I assume). I will try digging further but honestly I don't really understand what the test case is supposed to be testing for.

@dopry
Copy link
Contributor

dopry commented Sep 19, 2024

I've been poking a but more and getting a 405 Method Not Allowed is a pretty big change from getting a 404 Not Found. For a hot second I thought it was a CORS related issue, but I sent an option request in _get_token_from_authentication_server and it returned

'Allow': 'OPTIONS, GET, HEAD, POST', 

so that looks right, but the 405 response on the POST request doesn't return the Allow header which it should...

so I'm starting to share the suspicion that the mock server is misbehaving. I still don't quite understand how the mock server get stood up. I'm also not having any luck stepping into the requests.post with my debugger to figure our how the mocked server is working.. I'm used to the server being mocked the the TestCase and using self.client to call the django instance under test... I'm not quite sure how that works in pytest. I get the sense this mocked server may be setup by pytest... and maybe it's mocking has changes somehow... or a library it depends on has changed.

@n2ygk
Copy link
Member Author

n2ygk commented Sep 19, 2024

Yeah I wonder if this test case was always just wrong because there is no mock server. If you look at other test cases they use MagicMock(wraps=Request) (which is oauthlib's Request).

class TestOAuth2Validator(TransactionTestCase):
def setUp(self):
self.user = UserModel.objects.create_user("user", "[email protected]", "123456")
self.request = mock.MagicMock(wraps=Request)
self.request.user = self.user
self.request.grant_type = "not client"
self.validator = OAuth2Validator()

This might have been an attempt to show what happens when a bad url is used or something. I don't really understand the logic since a 404 is also not useful. It's not testing if a bad Bearer token was provided.... This test was added in #848 and I'm thinking something in the mock/response/whatever environment has changed. Still very confusing. Why's it a 404 for djmain but suddenly a 405 for lower versions?

@dopry
Copy link
Contributor

dopry commented Sep 19, 2024

looking at the test itself it's just ensuring we're logging an error when we get any response other than 200... from that perspective your approach to resolving the issue seems 'Good Enough'. While I'm still concerned about what changed and what it might mean for our test infrastructure, I'm gonna go ahead an approve this.

@n2ygk
Copy link
Member Author

n2ygk commented Sep 19, 2024

HAHA! found it:

django-oauth-toolkit$ curl  -v -X POST http://example.com/token/introspection
* Host example.com:80 was resolved.
* IPv6: (none)
* IPv4: 93.184.215.14
*   Trying 93.184.215.14:80...
* Connected to example.com (93.184.215.14) port 80
> POST /token/introspection HTTP/1.1
> Host: example.com
> User-Agent: curl/8.7.1
> Accept: */*
> 
* Request completely sent off
< HTTP/1.1 405 Method Not Allowed
< Content-Type: text/html; charset=UTF-8
< Date: Thu, 19 Sep 2024 18:51:30 GMT
< Server: ECAcc (nyd/D12F)
< Content-Length: 0
< 
* Connection #0 to host example.com left intact
* 

Example.com changed their web server to return a 405. I guess. Still not sure why 404 sometimes. Maybe to do with headers?

Saw these headers in the debugger for the response:

{'Content-Type': 'text/html; charset=UTF-8', 'Date': 'Thu, 19 Sep 2024 18:46:54 GMT', 'Server': 'ECAcc (nyd/D136)', 'Content-Length': '0'}

@n2ygk
Copy link
Member Author

n2ygk commented Sep 19, 2024

This test is just incorrectly connecting to example.com. We need to fix that.

@dopry
Copy link
Contributor

dopry commented Sep 19, 2024

oh that sounds like we need to mock the request... Are you up to doing the fix?

@n2ygk n2ygk requested a review from dopry September 20, 2024 15:32
@n2ygk n2ygk merged commit a153823 into jazzband:master Sep 20, 2024
19 checks passed
@n2ygk n2ygk deleted the bug/1497/404_405 branch September 20, 2024 15:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants