OData / AspNetCoreOData

ASP.NET Core OData: A server library built upon ODataLib and ASP.NET Core
Other
453 stars 159 forks source link

Authentication should be done before OData validations #734

Open berubejp opened 1 year ago

berubejp commented 1 year ago

Assemblies affected ASP.NET Core OData 8.x

Describe the bug When calling a secured OData endpoint with wrong authentication, I get a 500 error instead of a 401. This occurs when using an invalid key in the URL. Example odata/books/aaaaaaa or odata/books/1' when the expected key is an int.

Reproduce steps I build a new web api project, added OData and basic authentication for testing purposes. View my example here https://github.com/berubejp/odatawebapptest

My issue occurs when I call the endpoint without or with wrong authentication. When calling odata/books, I get a 401 response as expected. When calling odata/books/1, I get a 401 response as expected. When calling odata/books/1', I get a 500 response. Notice the single quote at the end or the URL. When calling odata/books/aaaaaaa, I get a 500 response.

The error is ODataException: The key value (xx) from request is not valid. This exception is triggered before the authentication is handled.

Request/Response Using my example web api, you can call the web api using basic authentication using "admin:admin". However, the issue is triggered if authentication is invalid or absent.

Expected behavior I would expected the authentication to kick and fail before any OData validations resulting in a 401 response. The OData validation error should only be triggered if the authentication succeed.

dnperfors commented 1 year ago

This exception is triggered by the parser of the URL. Since OData knows that the id for Book is an int type, it just throws an exception. In this case this should be a 400 Bad Request since the url is not in the right format. I don't think authentication can be handled after URL Parsing...

julealgon commented 1 year ago

This exception is triggered by the parser of the URL. Since OData knows that the id for Book is an int type, it just throws an exception. In this case this should be a 400 Bad Request since the url is not in the right format.

Are you positive about this? To me, this depends on how the framework wants to deal with it.

For standard MVC endpoints, it works as follows:

Route Template Action Request Result
/api/{value} void Get(int value) /api/1 200
/api/{value} void Get(int value) /api/a 400 "The value 'a' is not valid."
/api/{value:int} void Get(int value) /api/1 200
/api/{value:int} void Get(int value) /api/a 404

If OData wants the EDM to work like a route constraint (which makes some sense in my mind), then the response would be a 404 instead of a 400.

I don't think authentication can be handled after URL Parsing...

Again, using standard MVC as an example, using

This results in an authentication check even though the value a cannot be converted into an int.

But if the same scenario is performed with a constrained route again (/api/{value:int}) then the 404 is returned before auth is attempted.

I'd like OData to both be consistent with MVC, and consistent within its own logic: so either 404 should be returned (if the team feels that the EDM restrictions are like route constraints), or 401 should be returned otherwise.

dnperfors commented 1 year ago

Fair point. I didn't know that /api/{value:int} would return a 404 instead of a 400. I actually never used this contraint. But if this is the case, then yes, a 404 should be returned, or on the authenticated version a 401.