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 docs related to auth when using Kino.Proxy #433

Merged
merged 4 commits into from
Jun 3, 2024

Conversation

hugobarauna
Copy link
Member

No description provided.


Kino.Proxy.listen(fn
%{path_info: ["export", "data"]} = conn ->
["Bearer " <> ^token] = Plug.Conn.get_req_header(conn, "authorization")
data = "some data"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this case, using bearer tokens may be the best example, since tokens are most common for APIs (and Kino.Proxy would most likely be used for APIs?)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed.

I'll change from HTTP basic auth to HTTP Bearer.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought about leaving this example as simple as possible, with no auth. And the auth would be below, inside the admonition block.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I see. Keeping it simple is a good idea.

Copy link
Member Author

@hugobarauna hugobarauna left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@josevalim maybe this version?

Although I'm not 100% satisfied with it. I accept suggestions.


Kino.Proxy.listen(fn
%{path_info: ["export", "data"]} = conn ->
["Bearer " <> ^token] = Plug.Conn.get_req_header(conn, "authorization")
data = "some data"
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought about leaving this example as simple as possible, with no auth. And the auth would be below, inside the admonition block.

> implement your own authentication mechanism. Here's a simple example.
>
> ```elixir
> Kino.Proxy.listen(fn conn ->
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@josevalim what about this code example with a simple Bearer auth?

Kino.Proxy.listen(fn conn ->
  api_token = "my-secret-api-token"

  with ["Bearer " <> client_token] <- Plug.Conn.get_req_header(conn, "authorization"),
       true <- api_token == client_token do
    Plug.Conn.send_resp(conn, 200, "hello")
  else
    _ ->
      conn
      |> Plug.Conn.put_resp_header("www-authenticate", "Bearer")
      |> Plug.Conn.send_resp(401, "Unauthorized")
  end
end)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Kino.Proxy.listen(fn conn ->
  api_token = "my-secret-api-token"

  case Plug.Conn.get_req_header(conn, "authorization") do
    ["Bearer " <> ^api_token] ->
      Plug.Conn.send_resp(conn, 200, "hello")
    _ ->
      conn
      |> Plug.Conn.put_resp_header("www-authenticate", "Bearer")
      |> Plug.Conn.send_resp(401, "Unauthorized")
  end
end)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, the example above is unsafe, we want to use Plug.Crypto.secure_compare :( It may be worth keeping it as user:pass just for simplicity. Sorry for the back and forth. :(

Comment on lines +52 to +53
> The paths exposed by `Kino.Proxy` don't use the authentication mechanisms
> defined in your Livebook instance.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@josevalim I started to wonder if we should put them in the /public namespace. The main downside is that it differs more from the base app/session path.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can always add more routes in the future. I think we have a good starting set. I can also see it being used for coordinating tasks inside their own infrastructure, but still not exposing it to the world.

lib/kino/proxy.ex Outdated Show resolved Hide resolved
@hugobarauna
Copy link
Member Author

@josevalim can I merge this?

@josevalim
Copy link
Contributor

@hugobarauna to merge this, you should either use Plug.BasicAuth, or keep the token aprpoach, but use Plug.Crypto.secure_compare(user_token, expected_token)., Then feel free to ship it!

@hugobarauna
Copy link
Member Author

@josevalim now it should be ready to merge. Please take a last look.

@hugobarauna hugobarauna merged commit bfd6be2 into main Jun 3, 2024
1 check passed
@hugobarauna hugobarauna deleted the hb-kino-proxy-docs-auth branch June 3, 2024 13:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants