PlaceAVote / pav-api

This repository contains the Placeavote API.
0 stars 0 forks source link

Inconsistency in user authentication messages. #27

Open SudoPlz opened 8 years ago

SudoPlz commented 8 years ago

The problem: Inconsistency

When a user tries to log in with a wrong email address, the response from the sever is of type application/json and the response json is this: {"errors":[{"email":"A valid email address is a required"}]}.

On the other hand if a user tries to log in with a wrong password, the response from the server is of type text/plain; charset=UTF-8 and the response text is Invalid Login credientials.

A great solution to all this

It would be much better, and less hackish for us to implement the client side, if the responses were consistent and always responding with application/json.

There are specs for the best practises in server responses here, (its not long to read at all 👍 ) but basically what it proposes is this model of responses:

{
    status : "success",
    data : {
        "post" : { "id" : 1, "title" : "A blog post", "body" : "Some useful content" }
     }
}

where a status can be either a:

johnboy14 commented 8 years ago

I think the proposed solution is a nice approach but it would open a can of worms on the client right now. So maybe when we get to a certain level of maturity we can revisit this idea. I will fix the bug and return the payload in json instead of plaintext

SudoPlz commented 8 years ago

The bug was fixed and I now get {"error":"Invalid Login credientials"} when I send false credentials to the server (which is application/json). However there is still inconsistency because if I give a wrong email, the json will be {"errors":[{"email":"A valid email address is a required"}]}. In the first case we got error being an object, in the other errors being an array. If that is intended behaviour, I will implement the client to recognise both the error and the errors objects, otherwise perhaps the response should be universal.

johnboy14 commented 8 years ago

I will adjust this to always return an array of errors

SudoPlz commented 8 years ago

Awesome, thanks John!

johnboy14 commented 8 years ago

This is a generic error {"error":"Invalid Login credientials"} It doesn't really make reference to a particular error which is safer than spelling out which field is invalid.

It might be better if you handle error and errors seperately