kumar303 / hawkrest

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

Make AuthenticationFailed more specific for bad headers/expired tokens #16

Closed edmorley closed 8 years ago

edmorley commented 8 years ago

Whilst many of the possible mohawk exception types should not be revealed directly in the AuthenticationFailed exception message (since they would give clues to attackers - eg let them determine valid client ids), there are some that are useful (and safe) to surface in the response to the client.

Fixes #14.

The existing tests for the "dangerous to reveal" cases have also been updated to ensure they exact-match against the string, since previously they were only checking for the substring (since assertRaisesRegexp() uses re.search()).

kumar303 commented 8 years ago

Could you add some tests for the BadHeaderValue and TokenExpire cases that check the exception message? They don't have to be exact, something like

self.assertRaisesRegexp(
    AuthenticationFailed, 
    '^Hawk authentication failed: The token has expired .*')

These will serve two purposes: 1) they'll make sure we don't regress showing extra output in special cases and 2) they'll cover the else blocks in these cases so that no exceptions get raised by future refactoring.

kumar303 commented 8 years ago

There are no existing tests for the bad header and bad timestamp cases or I would have updated those too

They should be easy to add. I'd suggest copying test_hawk_post_wrong_sig but use mock to patch Receiver() and give it a side_effect for each exception. Let me know if you get stuck. Because of how mock patching works you may need to edit the code to do receiver = mohawk.Receiver so you can patch 'mohawk.Receiver' (i.e. a direct reference).

edmorley commented 8 years ago

PR updated with tests :-)

I ended up not mocking Receiver in the end, since it was easy to do without, and this way we ensure that mohawk still raises those exceptions for the specific cases we expect.

kumar303 commented 8 years ago

Awesome, thanks! The added tests look great.

edmorley commented 8 years ago

Thank you :-)

edmorley commented 8 years ago

@kumar303 - would it be possible to make a new release for this at some point in the next week or two? :-)