fix bug: handle non-UUID path segment in POST/GET/DELETE /apikeys - #786
Conversation
| def routeMethods(resourceIdentity: Option[UUID]): Route = { | ||
| post { | ||
| exchAuth(TUser(resource, resourceIdentity), Access.WRITE, validIdentity = identity) { _ => | ||
| postUserApiKey(identity, organization, username, resourceIdentity) | ||
| pathEnd { | ||
| post { |
There was a problem hiding this comment.
Wouldn't this pathEnd guard here be enough?
There was a problem hiding this comment.
If we only add a pathEnd here, requests like GET /orgs/testorg/users/testuser/apikeys/123 or DELETE /orgs/testorg/users/testuser/apikeys/123 will not mismatch POST and return 405 Method Not Allowed as Krystian reported. Instead, it will return a 404 Unrecognised route. This because the previous implementation, GET and DELETE only accepts UUIDs, so a non-UUID string will not match any route.
In my current commit these requests will return 400 Invalid UUID format for API key ID. I personally feel this is a more appropriate response. Let me know what you think.
There was a problem hiding this comment.
Thanks for explanation. I definitely think that 404 Unrecognised route or 405 Method Not Allowed would be out of place. There should be returned either404 Resource Not found or your 400 Bad Request. I think your solution is more user-friendly (in case of a typo).
|
@naphelps Please merge. |
3365737 to
bbe0c6d
Compare
Signed-off-by: wenyang-cao <wenyang.cao@ibm.com>
bbe0c6d to
b093fc8
Compare
3 participants
Issue: #785
Expected
POSTonly matches exact/apikeys(pathEnd)GETandDELETEshould validatekeyidas UUID and return400if malformed@naphelps @kroczi
Please take a look.
Since the change involves wrapping at a higher level, there's a lot of indentation diff — please review with ?w=1: https://github.com/open-horizon/exchange-api/pull/786/files?w=1
Haven't updated changelog since I am not sure what version it would be when it is ready to be merged.