Repository: jeregrine/jsonapi (elixir)

Issue:
Improper checking of headers
In [this](https://github.com/jeregrine/jsonapi/blob/1f9e45aee4058ca6b3a8a55aaec6eebcada525a6/lib/jsonapi/plugs/content_type_negotiation.ex) file, it forces the headers to conform to the JSONAPI media type improperly.

To [quote](http://jsonapi.org/format/#content-negotiation-servers):

```
Servers MUST send all JSON API data in response documents with the header Content-Type: application/vnd.api+json without any media type parameters.
```

The server MUST respond with the proper headers, the library handles this okay.

```
Servers MUST respond with a 415 Unsupported Media Type status code if a request specifies the header Content-Type: application/vnd.api+json with any media type parameters.
```

This seems to be worded intentionally, such that **if** a request specifies the `Content-Type: application/vnd.api+json` header **with** any additional parameters, it should return a 415. The server itself is not required to force the client to send the `Content-Type: application/vnd.api+json` header (just to enforce that it's done properly if the client does send it), though [the client is required by the spec to send it](http://jsonapi.org/format/#content-negotiation-clients).

```
Servers MUST respond with a 406 Not Acceptable status code if a request’s Accept header contains the JSON API media type and all instances of that media type are modified with media type parameters.
```

This also seems to be worded intentionally, such that **if** a request specifies an `Accept` header with the proper type, but ALL instances of that media type are modified (meaning it can have multiple media types, and even multiple declarations of the JSONAPI media type), throw a 406. Again, the server is not required to force the client to send the `Accept: application/vnd.api+json` header (just to enforce that it's done properly if the client does send it -- and only to enforce it if they use the `application/vnd.api+json` media type, **and each time it's specified** it has modified media type parameters), and actually, [_the client isn't required to send this header at all, they're just required to **not** modify the media type with parameters on at least one instance of it if they do pass it_](http://jsonapi.org/format/#content-negotiation-clients).

Just wanted to clarify that. Great library!


Pull request:
Check headers according to spec close #90
I think this is close, but not exactly what we want.  

#90 

Patch:
diff --git a/lib/jsonapi/page.ex b/lib/jsonapi/page.ex
index 01fefba..767e113 100644
--- a/lib/jsonapi/page.ex
+++ b/lib/jsonapi/page.ex
@@ -8,4 +8,4 @@ defmodule JSONAPI.Page do
             size: nil,
             page: nil,
             cursor: nil
-end
\ No newline at end of file
+end
diff --git a/lib/jsonapi/plugs/content_type_negotiation.ex b/lib/jsonapi/plugs/content_type_negotiation.ex
index 3dea8fb..84e6a52 100644
--- a/lib/jsonapi/plugs/content_type_negotiation.ex
+++ b/lib/jsonapi/plugs/content_type_negotiation.ex
@@ -33,8 +33,6 @@ defmodule JSONAPI.ContentTypeNegotiation do
     accepts =
       conn
       |> get_req_header("accept")
-      |> Enum.flat_map(&(String.split(&1, ",")))
-      |> Enum.map(&String.strip/1)
       |> List.first
 
     {conn, content_type, accepts}
@@ -49,11 +47,25 @@ defmodule JSONAPI.ContentTypeNegotiation do
     {conn, content_type}
   end
 
-  defp respond({conn, @jsonapi, @jsonapi}) do
+  defp respond({conn, content_type, accepts}) do
+    cond do
+      validate_header(content_type) and validate_header(accepts) == true ->
+        add_header_to_resp(conn)
+      validate_header(content_type) == false ->
+        send_error(conn, 415)
+      validate_header(accepts) == false ->
+        send_error(conn, 406)
+    end
+  end
+
+  defp validate_header(string) when is_binary(string) do
+    string |> String.split(",") |> Enum.map(&String.trim/1) |> Enum.member?(@jsonapi)
+  end
+  defp validate_header(nil), do: true
+
+  defp add_header_to_resp(conn) do
     register_before_send(conn, fn conn -> update_resp_header(conn, "content-type", @jsonapi, &(&1)) end)
 
     conn
   end
-  defp respond({conn, @jsonapi, _accepts}), do: send_error(conn, 406)
-  defp respond({conn, _content_type, _accepts}), do: send_error(conn, 415)
 end
diff --git a/lib/jsonapi/query_parser.ex b/lib/jsonapi/query_parser.ex
index 35e629c..0e0db0e 100644
--- a/lib/jsonapi/query_parser.ex
+++ b/lib/jsonapi/query_parser.ex
@@ -3,6 +3,7 @@ defmodule JSONAPI.QueryParser do
   alias JSONAPI.Config
   alias JSONAPI.Page
   alias JSONAPI.Exceptions.InvalidQuery
+  alias Plug.Conn
   import JSONAPI.Utils.IncludeTree
 
   @moduledoc """
@@ -65,7 +66,7 @@ defmodule JSONAPI.QueryParser do
 
   def call(conn, opts) do
     query_params_config_struct = conn
-      |> Plug.Conn.fetch_query_params()
+      |> Conn.fetch_query_params()
       |> Map.get(:query_params)
       |> struct_from_map(%Config{})
 
@@ -76,7 +77,7 @@ defmodule JSONAPI.QueryParser do
     |> parse_sort(query_params_config_struct.sort)
     |> parse_pagination(query_params_config_struct.page)
 
-    Plug.Conn.assign(conn, :jsonapi_query, config)
+    Conn.assign(conn, :jsonapi_query, config)
   end
 
   def parse_pagination(config, map) when map_size(map) == 0, do: config
diff --git a/lib/jsonapi/serializer.ex b/lib/jsonapi/serializer.ex
index b134846..ea83767 100644
--- a/lib/jsonapi/serializer.ex
+++ b/lib/jsonapi/serializer.ex
@@ -81,7 +81,8 @@ defmodule JSONAPI.Serializer do
     if {rel_view, :include} == valid_include_view && is_data_loaded?(rel_data) do
       rel_query_includes =
         if is_list(query_includes) do
-          Enum.reduce(query_includes, [], fn
+          query_includes
+          |> Enum.reduce([], fn
             {^key, value}, acc -> acc ++ [value]
             _, acc -> acc
           end)
@@ -89,7 +90,6 @@ defmodule JSONAPI.Serializer do
         else
           []
         end
-      #TODO Possibly only return a list of data + view, and encode it after the fact once instead of N times.
       {rel_included, encoded_rel} = encode_data(rel_view, rel_data, conn, rel_query_includes)
       {rel_included ++ [encoded_rel], acc}
     else
@@ -154,7 +154,7 @@ defmodule JSONAPI.Serializer do
 
   defp get_default_includes(view) do
     rels = view.relationships()
-    default_includes = rels |> Enum.filter(fn
+    Enum.filter(rels, fn
       {_k, {_v, :include}} -> true
       _ -> false
     end)
@@ -162,7 +162,8 @@ defmodule JSONAPI.Serializer do
 
   defp get_query_includes(view, query_includes) do
     rels = view.relationships()
-    Enum.map(query_includes, fn
+    query_includes
+    |> Enum.map(fn
       {include, _} -> Keyword.take(rels, [include])
       include -> Keyword.take(rels, [include])
     end)


Test patch:
diff --git a/test/jsonapi/plugs/content_type_negotiation_test.exs b/test/jsonapi/plugs/content_type_negotiation_test.exs
index abfc893..fa3860a 100644
--- a/test/jsonapi/plugs/content_type_negotiation_test.exs
+++ b/test/jsonapi/plugs/content_type_negotiation_test.exs
@@ -4,35 +4,154 @@ defmodule JSONAPI.ContentTypeNegotiationTest do
 
   alias JSONAPI.ContentTypeNegotiation
 
-  test "halts and returns an error if content-type header is incorrect" do
+  test "passes request through" do
+    conn =
+      :post
+      |> conn("/example", "")
+      |> Plug.Conn.put_req_header("content-type", "application/vnd.api+json")
+      |> Plug.Conn.put_req_header("accept", "application/vnd.api+json")
+      |> ContentTypeNegotiation.call([])
+
+    refute conn.halted
+  end
+
+  test "halts and returns an error if no content-type or accept header" do
+    conn =
+      :post
+      |> conn("/example", "")
+      |> ContentTypeNegotiation.call([])
+
+    refute conn.halted
+  end
+
+  test "passes request through if only content-type header" do
+    conn =
+      :post
+      |> conn("/example", "")
+      |> Plug.Conn.put_req_header("content-type", "application/vnd.api+json")
+      |> ContentTypeNegotiation.call([])
+
+    refute conn.halted
+  end
+
+  test "passes request through if only accept header" do
+    conn =
+      :post
+      |> conn("/example", "")
+      |> Plug.Conn.put_req_header("accept", "application/vnd.api+json")
+      |> ContentTypeNegotiation.call([])
+
+    refute conn.halted
+  end
+
+  test "passes request through if multiple accept header" do
+    conn =
+      :post
+      |> conn("/example", "")
+      |> Plug.Conn.put_req_header("accept", "application/vnd.api+json, application/vnd.api+json; version=1.0")
+      |> ContentTypeNegotiation.call([])
+
+    refute conn.halted
+  end
+
+  test "passes request through if correct content-type header is last" do
+    conn =
+      :post
+      |> conn("/example", "")
+      |> Plug.Conn.put_req_header("content-type", "application/vnd.api+json; version=1.0, application/vnd.api+json")
+      |> ContentTypeNegotiation.call([])
+
+    refute conn.halted
+  end
+
+  test "passes request through if correct accept header is last" do
+    conn =
+      :post
+      |> conn("/example", "")
+      |> Plug.Conn.put_req_header("accept", "application/vnd.api+json; version=1.0, application/vnd.api+json")
+      |> ContentTypeNegotiation.call([])
+
+    refute conn.halted
+  end
+
+  test "halts and returns an error if content-type header contains other media type" do
+    conn =
+      :post
+      |> conn("/example", "")
+      |> Plug.Conn.put_req_header("content-type", "text/html")
+      |> ContentTypeNegotiation.call([])
+
+    assert conn.halted
+    assert 415 == conn.status
+  end
+
+  test "halts and returns an error if content-type header contains other media type params" do
+    conn =
+      :post
+      |> conn("/example", "")
+      |> Plug.Conn.put_req_header("content-type", "application/vnd.api+json; version=1.0")
+      |> ContentTypeNegotiation.call([])
+
+    assert conn.halted
+    assert 415 == conn.status
+  end
+
+  test "halts and returns an error if content-type header contains other media type params (multiple)" do
     conn =
       :post
       |> conn("/example", "")
+      |> Plug.Conn.put_req_header("content-type", "application/vnd.api+json; version=1.0, application/vnd.api+json; version=1.0")
       |> ContentTypeNegotiation.call([])
 
     assert conn.halted
     assert 415 == conn.status
   end
 
-  test "halts and returns an error if accept header is incorrect" do
+  test "halts and returns an error if content-type header contains other media type params with correct accept header" do
+    conn =
+      :post
+      |> conn("/example", "")
+      |> Plug.Conn.put_req_header("content-type", "application/vnd.api+json; version=1.0")
+      |> Plug.Conn.put_req_header("accept", "application/vnd.api+json")
+      |> ContentTypeNegotiation.call([])
+
+    assert conn.halted
+    assert 415 == conn.status
+  end
+
+  test "halts and returns an error if accept header contains other media type params" do
     conn =
       :post
       |> conn("/example", "")
       |> Plug.Conn.put_req_header("content-type", "application/vnd.api+json")
+      |> Plug.Conn.put_req_header("accept", "application/vnd.api+json charset=utf-8")
       |> ContentTypeNegotiation.call([])
 
     assert conn.halted
     assert 406 == conn.status
   end
 
-  test "passes request through" do
+  test "halts and returns an error if all accept header media types contain media type params with no content-type" do
+    conn =
+      :post
+      |> conn("/example", "")
+      |> Plug.Conn.put_req_header("accept", "application/vnd.api+json; version=1.0, application/vnd.api+json; version=1.0")
+      |> ContentTypeNegotiation.call([])
+
+    assert conn.halted
+    assert 406 == conn.status
+  end
+
+  test "halts and returns an error if all accept header media types contain media type params" do
     conn =
       :post
       |> conn("/example", "")
       |> Plug.Conn.put_req_header("content-type", "application/vnd.api+json")
-      |> Plug.Conn.put_req_header("accept", "application/vnd.api+json")
+      |> Plug.Conn.put_req_header("accept", "application/vnd.api+json; version=1.0, application/vnd.api+json; version=1.0")
       |> ContentTypeNegotiation.call([])
 
-    refute conn.halted
+    assert conn.halted
+    assert 406 == conn.status
   end
+
 end