Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a custom body reader option to Parsers #698

Merged

Conversation

Projects
None yet
2 participants
@ryanwinchester
Copy link
Contributor

commented Apr 21, 2018

Submitting a possible solution as discussed in #691, for further discussion.

Since the body can only be read once, this becomes an issue for anybody who needs the raw body for whatever reason in their application.

This is a common problem and recurring theme that people run into when they need to verify their requests with HTTP Signature Verification oin the body.

This issue effects anybody who uses webhooks to integrate with third-party services like GitHub, Stripe, Dropbox, Samsung SmartThings, and many many more.

By passing in an optional read_body function, the developer can now hook in signature verifications to be used on the raw body before it is discarded.

I'm opening this PR in the hopes of pushing progress on this issue. I am 100% open to completely changing the implementation, and also I will not feel snubbed if somebody opens a better PR.

@ryanwinchester ryanwinchester force-pushed the ryanwinchester:features/config-read_body branch from 82afda2 to 7d0aef6 Apr 21, 2018

@ryanwinchester

This comment has been minimized.

Copy link
Contributor Author

commented Apr 21, 2018

@josevalim said (in the issue mentioned in OP):

I like the idea of the parsers calling Plug.Parsers.read_body that will choose what to do. Or we can pass a read body function on init. But basically, be able to inject the function that reads the body as a dependency.

So, there are still other ways we could consider doing this.

@ryanwinchester ryanwinchester force-pushed the ryanwinchester:features/config-read_body branch from 7d0aef6 to 1de0ed7 Apr 21, 2018

@josevalim

This comment has been minimized.

Copy link
Member

commented Apr 21, 2018

@ryanwinchester I like this as it is the most generic way I would implement this. The only concern is that you cannot pass anonymous functions at compile time. This wouldn't work:

plug Plug.Parsers, body_reader: fn conn, opts -> ... end

That's why we usually use MFAs. Should we use that instead here? Something like:

plug Plug.Parsers, body_reader: {Plug.Conn, :read_body, []}
@ryanwinchester

This comment has been minimized.

Copy link
Contributor Author

commented Apr 21, 2018

@josevalim would you say then, something more like this?

# parsers.ex

  def read_body(conn, {module_name, function_name, extra_args}, opts) do
    apply(module_name, function_name, [conn | opts] ++ extra_args)
  end
# parsers/json.ex

  def init(opts) do
    {decoder, opts} = Keyword.pop(opts, :json_decoder)
    {body_reader, opts} = Keyword.pop(opts, :body_reader, {Plug.Conn, :read_body, []})

    # ...

    {body_reader, decoder, opts}
  end

  def parse(conn, "application", subtype, _headers, {body_reader, decoder, opts}) do
    if subtype == "json" or String.ends_with?(subtype, "+json") do
      conn
      |> Plug.Parsers.read_body(body_reader, opts)
      |> decode(decoder)
    else
      {:next, conn}
    end
  end

Or just stick to forcing the specific parsers applying the MFAs on their own?

Be as nit-picky as you like, I've still got a lot to learn ;)

@josevalim

This comment has been minimized.

Copy link
Member

commented Apr 21, 2018

I don't think we need a helper function. Here is what I would do:

  def init(opts) do
    {decoder, opts} = Keyword.pop(opts, :json_decoder)
    body_reader = Keyword.get(opts, :body_reader, {Plug.Conn, :read_body, [opts]})
    {body_reader, decoder}
  end

  def parse(conn, "application", subtype, _headers, {{mod, fun, args}, decoder}) do
    if subtype == "json" or String.ends_with?(subtype, "+json") do
      apply(mod, fun, [conn | args]) |> decode(decoder)
    else
      {:next, conn}
    end
  end

@ryanwinchester ryanwinchester force-pushed the ryanwinchester:features/config-read_body branch from ea8e5d0 to d25ee7a Apr 21, 2018

@ryanwinchester

This comment has been minimized.

Copy link
Contributor Author

commented Apr 21, 2018

Cool, updated. 😊

My concern now is apply(mod, fun, [conn | args]) vs maybe changing that to something like apply(mod, fun, [conn | args] ++ opts)

Since we used to (and still do in the default case) pass the remaining opts to Plug.Conn.read_body, but now if we override it, it's only the args specifically supplied in the :body_reader MFA.

Thoughts? Not a concern because we can specify what we want in the MFA?

@josevalim

This comment has been minimized.

Copy link
Member

commented Apr 21, 2018

@ryanwinchester we would ever only prepend arguments, because that's the convention, but i understand your concerns. If you want to always pass the options, that's fine by me, but it has to be [conn, opts | args]. :)

@@ -20,6 +24,13 @@ defmodule Plug.Parsers.JSONTest do
end
end

defmodule BodyReader do
def read_body(conn, opts \\ []) do

This comment has been minimized.

Copy link
@josevalim

josevalim Apr 21, 2018

Member

It is better to remove the optional arguments so it is clear which arity the test is invoking.

@josevalim

This comment has been minimized.

Copy link
Member

commented Apr 21, 2018

@ryanwinchester let's go with your suggestion and always pass both conn and opts. Then we just need to add docs and we should be good to go!

@ryanwinchester ryanwinchester force-pushed the ryanwinchester:features/config-read_body branch 3 times, most recently from 08f784d to 3ac844a Apr 21, 2018

Add a custom body reader option to Parsers
Use MFA for body reader options in parsers

@ryanwinchester ryanwinchester force-pushed the ryanwinchester:features/config-read_body branch from 3ac844a to 94e19c4 Apr 21, 2018

@ryanwinchester ryanwinchester force-pushed the ryanwinchester:features/config-read_body branch from 060eb3d to e7c4937 Apr 21, 2018

@josevalim

This comment has been minimized.

Copy link
Member

commented Apr 21, 2018

Beautiful, I have simplified the docs to require less contextual information, and everything else is perfect. Thank you!

@josevalim josevalim merged commit 283e6d5 into elixir-plug:master Apr 21, 2018

1 check was pending

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
@josevalim

This comment has been minimized.

Copy link
Member

commented Apr 21, 2018

❤️ 💚 💙 💛 💜

@ryanwinchester ryanwinchester changed the title (WIP) Add a custom body reader option to Parsers Add a custom body reader option to Parsers Apr 21, 2018

@ryanwinchester ryanwinchester deleted the ryanwinchester:features/config-read_body branch Apr 22, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.