start adding some tests for API auth #28394
Conversation
corehq/apps/api/tests/test_auth.py
Outdated
| class LoginAndDomainAuthenticationTest(AuthenticationTestBase): | ||
|
|
||
| def test_login_with_domain(self): | ||
| self.assertAuthenticationSuccess(LoginAndDomainAuthentication(), self._get_request(domain=self.domain)) |
There was a problem hiding this comment.
shouldn't this fail since it has no auth?
There was a problem hiding this comment.
eagle eyes. Thanks, it was returning a 401 HttpResponse which was not failing on assertTrue. This whole "sometimes it's true or false and sometimes it's an HttpResponse" thing is really bugging me. I'm pretty sure it's entirely unneccessary, but just haven't gone through enough code to change it yet. Hopefully soon. 🙏
(fixed in the latest 3 commits, which revealed some other issues with the domain checks...)
There was a problem hiding this comment.
I wonder if the reason we do this is to expose things like this to the API callers, and whether it's worth keeping it that way because of that: https://github.com/dimagi/commcare-hq/blob/master/corehq/apps/domain/decorators.py#L433-L437
There was a problem hiding this comment.
Seems like this is by design in Tastypie. Here's part of the docstring from is_authenticated:
Should return either ``True`` if allowed, ``False`` if not or an
``HttpResponse`` if you need something custom.
I guess the question is whether we should try and be more consistent and always return an HTTP response. I do think the custom messages are useful at times, particularly the one you linked.
There was a problem hiding this comment.
Yeah agreed, now that I get the use case I think the tradeoff between improvement in API docs is worth the additional code complexity
Co-authored-by: Daniel Miller <dmiller@dimagi.com>
|
added some more checks. think this is ready for re-review |
Related to #28388 I thought it'd be good to have a test suite we can use to ensure we don't mess anything up.
We do have some tests around this functionality spread around, but they're all integration tests at the API-level and I thought it'd be better to be able to test the primitives.
This is just a start - planning on expanding it out tomorrow (but fine to review/merge as is)