search

kumar303 / hawkrest

Hawk HTTP Authorization for Django Rest Framework
BSD 3-Clause "New" or "Revised" License
19 stars 8 forks source link

Error when there is no content-type available (e.g. DELETE requests). #46

Open theyosh opened 4 years ago

theyosh commented 4 years ago

Hi,

we make a DELETE action with the Django REST framework. And then there is no data returned. And then a message is '204 No content'. As there is no content, there is also no content-type meta data in the response.

So now the signing is broken at https://github.com/kumar303/hawkrest/blob/master/hawkrest/middleware.py#L33 and is causing errors when you want to make a DELETE action to the Django REST API.

You should check if the header is actually set. If not, leave it out, or give it some default value.

kumar303 commented 4 years ago

Hi. Thanks for the bug report. What is the exact traceback you get? Is response['Content-Type'] an empty string? If it is, I think it should work as expected (but maybe there is still a bug to fix).

I feel like we addressed this kind of thing better in https://github.com/kumar303/mohawk/issues/51 but maybe hawkrest needs some updates to support it, too.

theyosh commented 4 years ago

Hi, sorry for missing some details.

No, the key 'Content-type' does not exist in the dict response. So you get a KeyError on the dict. (I do not have the exact error message right now).

I am not sure if my conclusion is right, but when you do a REST DELETE call, you will not get any data back from the REST framework. The REST framework returns a 204 status. Which is no content. So the header 'Content-Type' is not available. Because there is no content. And then I get an error on line 33 due to the fact you try to read a key from a dict that does not exist.

So I solved it just for testing with: response.get('Content-Type','application/json') But I am not sure if this is valid. For me, it did the trick, but then, it was also the end of the day. So not sure if using a default like 'application/json' is valid. I think it is better to use None. Or change the line of code, so that you check if the key does exist, and then create the hash.

As far I can see, is the code on line 33 already from the beginning. Not sure why this is not happening earlier. Could be that the Django REST framework has changed this in time. And that it is now an issue....

kumar303 commented 4 years ago

Here?

https://github.com/kumar303/hawkrest/blob/dce22be5a0ebbbf61e4496f38fc13478220a66b9/hawkrest/middleware.py#L33

Hmm, yeah, I guess response.get('Content-Type', None) is the safest but we'd have to see if that solves it. It might need to be an empty string. If you have time to try a patch I can review and merge it.

theyosh commented 4 years ago

Yup, that is the line. I first tried application/json as a default, as I am making JSON calls. That did work for me. I did not extensive testing, as it was the last day for my holiday.

So at the moment I do not have enough knowledge about the HAWK protocol what kind of response header is valid. It just did the trick for me. Also making a PR for it, is a lot of work for me, as it is for you just change a single line of code. Forking a repository for a small bug is not the right way. For feature requests you can ask for PR, but for bugs, that is not the right way. That is a lot of overhead.

kumar303 commented 4 years ago

No worries, I appreciate you taking the time to submit a bug report. I don't really use hawkrest or mohawk for anything in production which is why I suggested submitting a PR for the fix you want (I can help). Otherwise, I'm not really sure who will do it, especially in testing the fix.

We can maybe use the verification tool to test it out: https://hawkrest.readthedocs.io/en/latest/usage.html#verification-tool

However, I think that code will need updating to pass a custom content-type or at least do DELETE requests correctly.

ghost commented 3 years ago

Sorry for not responding earlier, but I had holidays :P

I am not sure if the verification tool can check this, but I think I found another way of 'testing' it. I also use the library https://github.com/mozilla-services/requests-hawk which has a HAWK implementation in it.

So I use that to make my calls to Django, and when I make a delete call with a content type None, it works. There is no error from the requests-hawk module, which has to check the REST call?

But not sure if you can valid test it, as Django returns no data, and as far I can judge, HAWK needs a body to do its magic.

And I am unable to use the verification as it relays on a configuration DICT. Where I have implemented a database token system. So it is hard to say what the right value should be for the content type.

But what if we just start with response.get('Content-Type', None) as that is working (for me). If more people start complaining about this, we could change it to an empty string?

theyosh commented 3 years ago

Hmm, I mixed two accounts... :)

kumar303 commented 3 years ago

Hi, thanks for the follow up.

But what if we just start with response.get('Content-Type', None) as that is working (for me). If more people start complaining about this, we could change it to an empty string?

If you can propose it in a patch, I'll take a look. I'd just want to make sure that it doesn't circumvent content-type verification for other types of requests. If you got it working like this using requests-hawk then that's a good clue it's the right thing to do!