department-of-veterans-affairs / notification-api

Notification API
MIT License
16 stars 8 forks source link

BUG - POST api-key route returns 500 Internal Server Errors instead of a meaningful error #1031

Open kalbfled opened 1 year ago

kalbfled commented 1 year ago

Description

When posting to this route to create a new api key with the above properties set with bad values, since we know the problem, we should return 400 Bad Request and a meaningful response message.

There is a though that this was done for 'security by obscurity reasons,' but we can still return a 400 instead of a 500 with an obscured message directing them to the API Docs if we are concerned about returning the real reason.

Steps to Reproduce

Bad key_type value: POST /service/{{service_id}}/api-key with the following payload in Postman

{
  "name": "some_unique_string",
  "created_by": "{{user_id}}",
  "key_type": "foobar"
}

Bad name value: POST /service/{{service_id}}/api-key with the following payload in Postman, replace "some_not_unique_string" with a real APIkey name from that service.

{
  "name": "some_not_unique_string",
  "created_by": {{user_id}},
  "key_type": "foobar"
}

Observe that the response is a 500 Internal Server Error

Workaround

Is there something we can do to work around this issue in the meantime? Leave blank if n/a.

Impact/Urgency

Required. Describe the impact this bug has on our system, clients, and/or team.

Expected Behavior

When I post to create a new API Key Using a bad value for name or key_type, Then I expect a meaningful 400 Bad Request response and a meaningful if possibly obscurred error message.

QA Considerations

Additional Info & Resources

If applicable, include extra information to assist with troubleshooting, like screenshots, log snippets, links to applicable code files, and/or articles/websites that have relevant info on the issue. Leave blank if n/a.

Note that the 400 Bad Request error responses for all of the api-keys routes are not the same as the Not Found error responses for other service routes nor do they match the swagger responses for 400s.

For now, the 400 Bad Request message should at least match what the rest of the api_keys Bad Request responses are, which should be more or less

{
  "message": {
    "key_type": ["String not defined in enum: [ normal, team ]."],
    "result": "error"
  }
}

and

{
  "message": {
    "name": ["This name already exists."],
    "result": "error"
  }
}
kalbfled commented 1 year ago

Several people have suggested that the existing behavior might be intentional for security purposes. At a minimum, we should log the actual error.

mjones-oddball commented 1 year ago

Just to clarify: The name needs to be unique per service

mjones-oddball commented 1 year ago

API creation/mgmt is slated to be moved to Portal Q1 or Q2 2023, so we should hold off on API changes related to this.

mjones-oddball commented 1 year ago

Hey team! Please add your planning poker estimate with Zenhub @cris-oddball @EvanParish @jakehova @justaskdavidb2 @k-macmillan @kalbfled @ldraney