cpjk / canary

:hatching_chick: Elixir authorization and resource-loading library for Plug applications.
MIT License
474 stars 51 forks source link

Ecto.Query.CastError when :id is not an integer / does not match the field type #69

Open JamieREvans opened 7 years ago

JamieREvans commented 7 years ago

If I put in "/users/foo", Canary will throw an exception in trying to access the resource as "foo" is clearly not an acceptable where query for id.

     ** (Ecto.Query.CastError) /my_app/deps/ecto/lib/ecto/repo/queryable.ex:331: value `"foo"` in `where` cannot be cast to type :id in query:

     from g in MyApp.Account.User,
       where: g.id == ^"foo",
       select: g

This exception should be caught and we should have an :unprocessable_entity handler in the error controller, rather than letting the application implode.

cpjk commented 7 years ago

You might be able to accomplish what you're looking for with https://github.com/cpjk/canary#specifing-database-field

However, if you want a more custom authorization solution, it is fairly simple to do it yourself using custom plugs (and Canada as well if you like the can? user, :action, thing API). :smiley_cat:

JamieREvans commented 7 years ago

Sorry for any confusion, @cpjk. What I meant was that if someone using canary with the default id field, which, in Ecto, only supports numbers, if a string is passed in the id field, Canary throws an exception in the plug before we're able to even catch / wrap it. We'd have to build a plug ahead of Canary for all connections to make sure the ID field isn't a string, which seems a little crazy.

I would suggest having Canary catch common exceptions like this, especially if it's just for a simple web app that uses basic Phoenix routing with object ids in the url, since the urls would be very susceptible to having bad IDs.

It is just a shame to see a server crash for something like this.

cpjk commented 7 years ago

Ah, I understand. What do you think would be the appropriate catching behaviour from canary? Maybe return a 404? Maybe forbidden so as to not give anything away about URL structure.

JamieREvans commented 7 years ago

I think a 422, unprocessable entity, is the right exception for the error handler to receive. 400 means the HTTP request was faulty, but 422 means that the request cannot be served as requested. It says less than 404 or 403, in my mind.