euniversal / umei-api-specification

UMEI API specifications
Apache License 2.0
3 stars 0 forks source link

Add ResourceNotFoundError #74

Closed cdmNSIDE closed 2 years ago

cdmNSIDE commented 2 years ago

Hello Narve,

I think we should explicitly write in the UMEI that a 404 error code will be returned if the path parameter provided (ex: order ID) does not correspond to something that exists in the database. No ?

narve commented 2 years ago

Well, yes, but note that a 404 should be returned either if the item does not exist (e.g. the order), or if the item exist but you are not allowed to see it. This is standard security practise, so as not to leak ids or other private information.

cdmNSIDE commented 2 years ago

Well for the moment the situation is a bit different in the UMEI. If the item exists but you are not allowed to see it you will have a 403 ForbiddenError (or a 401 UnauthenticatedError)

cdmNSIDE commented 2 years ago

Do you agree with the changes suggested in the pull request ?

narve commented 2 years ago

I clarified the ResourceNotFound description in error codes. I'm ok with this PR.

cdmNSIDE commented 2 years ago

@narve Does that mean that we have to remove all 403 Forbidden errors from the UMEI, if we prefer to use 404 Not Found for that ?

narve commented 2 years ago

@narve Does that mean that we have to remove all 403 Forbidden errors from the UMEI, if we prefer to use 404 Not Found for that ?

No, we should keep both. Consider a user that is not allowed to view order number 123, but not order number 456 (the user really should not know that there is such an order number 456 at all!). There is no order number 789 at all.

Then

If we returned 404 for 789 but 403 for 456, we would be "leaking" information - revealing to the user that there actually is such an 456 order. Normally not a big deal, but securitywise it is recommended to keep it the way I've sketched above. Especially if the order ids are guessable, e.g. based on integers or some other predictable entity.

narve commented 2 years ago

So, if you agree, we can merge this PR.

cdmNSIDE commented 2 years ago

OK,thanks, can be merged