-
Notifications
You must be signed in to change notification settings - Fork 2
Description
This issue tracks work around a breaking change that will not have backwards compatible support. Right now the lib/hooks/app/api.rb
has a bit of setup that looks like so:
api_class = Class.new(Grape::API) do
content_type :json, "application/json"
content_type :txt, "text/plain"
content_type :xml, "application/xml"
content_type :any, "*/*"
format :txt # TODO: make this configurable via config[:format] (defaults to :json in the future)
default_format :txt # TODO: make this configurable via config[:default_format] (defaults to :json in the future)
end
Right now, the API defaults to assuming all requests are plain text payloads and then doing a best effort attempt to parse them into JSON. In a similar manner, it assumes all responses are plain text as well.
Instead, we should assume that all requests are JSON by default and always return a JSON response by default as well (setting the correct content type headers as well of course).
This should be reflected in two new global config options:
format: json # defaults to json if unset
default_format: defaults to json if unset
Here is what a sample CURL request looks like to the server right now (this is just a demo):
$ curl -v --request POST --url http://localhost:8081/webhooks/shared_secret_demo -- application/json' --header 'foo: bar' --header 'super-secret: 123' --data '{
"hello": "world",
"foo": ["bar", "baz"],
"truthy": true,
"coffee": {"is": "good"}
}'
Note: Unnecessary use of -X or --request, POST is already inferred.
* Host localhost:8081 was resolved.
* IPv6: ::1
* IPv4: 127.0.0.1
* Trying [::1]:8081...
* Connected to localhost (::1) port 8081
> POST /webhooks/shared_secret_demo HTTP/1.1
> Host: localhost:8081
> User-Agent: curl/8.5.0
> Accept: */*
> content-type: application/json
> foo: bar
> super-secret: 123
> Content-Length: 93
>
< HTTP/1.1 401 Unauthorized
< content-type: text/plain
< Content-Length: 21
<
* Connection #0 to host localhost left intact
*
As you can see, the content-type
is text/plain
and the response body is just the plain text authentication failed
. By going towards native JSON responses, we should expect the response to look like this instead:
{
"error": "authentication_failed",
"message": "authentication failed",
"request_id": "<uuid_here>"
}
This can be seen in the source code by looking at auth.rb
(example):
unless auth_class.valid?(payload:, headers:, config: endpoint_config)
log.warn("authentication failed for request with auth_class: #{auth_class.name} - request_id: #{request_id}")
error!({
error: "authentication_failed",
message: "authentication failed",
request_id:
}, 401)
end
Right now, we truly are trying to return JSON responses with the error!
method, but since our Grape API is hard coded to plain text responses, it is translated. This needs to be configurable and default to JSON going forward. This is a breaking change and that is okay.
script/test
must passscript/integration
must passscript/acceptance
must pass- 100% code coverage is required
script/lint -A
must pass