search

mgomes / api_auth

HMAC authentication for Rails and HTTP Clients
MIT License
480 stars 147 forks source link

Return explicit reasons on bad authentication #120

Closed fwininger closed 7 years ago

fwininger commented 8 years ago

I have one server with a time delay, but it was very complicate to understand and debug the issue in the application because ApiAuth return only false for bad authentication.

Let change that with explicit error.

kjg commented 8 years ago

Thanks so much for taking this on, having the ability to know what reason the request is inauthentic is very valuable.

My suggestion is to first just extract the checks into seperate validations that all run independantly of each other, and when each one passes or fails in can put that reason into a inauthentic_reasons hash or something along those lines. And for now the authentic? method can still be a true or false based on weather anything showed up in inauthentic_reasons.

Does that make sense?

I think having errors get raised for things being inauthentic can also happen, but it can and should be separate from the task of just supplying the exact reason in a new method.

fwininger commented 8 years ago

@kjg can you take a look ? Thanks for every comments.

kjg commented 8 years ago

This is great! Thanks so much! I just want to think about the names a little bit more with your help.

I like the name of the method, but maybe another name might be even more clear. Maybe something like authenticity_report?

I've always worried about using the term "md5_match" because we still consider a request authentic if there is no md5 provided at all. I'm not sure how to name this check using positive language though which is why I've gone with md5_mismatch in the past because lack of md5 doesn't make it a mismatch. Any further thought there?

If we're looking to possibly make request invalid if it's too far in the future too, maybe we can call that check "request_within_time_window" or something along those lines. There might be a better shorter name that I can't think of.

fwininger commented 7 years ago

I don't change md5_match because I prefers to have only false in authenticity_report for a failure.

kjg commented 7 years ago

I totally understand the desire to use only positive assertions like that. Can we come up with a better term than md5_match then, since it is a little misleading to call md5_match a "true" when no md5 was provided. Or maybe we can just leave it out of the report all together if there is no md5?

fwininger commented 7 years ago

I close this PR because we don't agree on the name and in the fact I don't need this anymore with https://github.com/mgomes/api_auth/pull/136.

Spone commented 7 years ago

I still think it's valuable to have access to an explicit error message.