White list modules #27
Conversation
![sourcelevel-bot[bot]](https://avatars.githubusercontent.com/in/597?s=60&v=4){kind=small}
lib/live_view_demo/white_list.ex
Outdated
| def validate(command) do | ||
| {_, result} = | ||
| Code.string_to_quoted!(command) | ||
| |> Macro.prewalk([], &valid?(&1, &2)) |
There was a problem hiding this comment.
Use a function call when a pipeline is only one function long
There was a problem hiding this comment.
@iaguirre88 I didn't understand it at a first glance, so I'm sharing here what I would try:
command
|> Code.string_to_quoted!
|> Macro.prewalk([], &valid?(&1, &2))
lib/live_view_demo/white_list.ex
Outdated
| def validate(command) do | ||
| {_, result} = | ||
| Code.string_to_quoted!(command) | ||
| |> Macro.prewalk([], &valid?(&1, &2)) |
There was a problem hiding this comment.
@iaguirre88 I didn't understand it at a first glance, so I'm sharing here what I would try:
command
|> Code.string_to_quoted!
|> Macro.prewalk([], &valid?(&1, &2))
lib/live_view_demo/white_list.ex
Outdated
| {:ok, command} | ||
|
|
||
| invalid_modules -> | ||
| {:error, "Invalid modules: #{inspect(invalid_modules)}"} |
There was a problem hiding this comment.
Nit: "Invalid module" (in singular)
There was a problem hiding this comment.
My bad.. just realized a list of modules is returned. It's fine to use plural.
Note: I just created a story to have something like the following in the future:
| command = "List.first([1,2,3])" | ||
| expected_result = {:ok, command} | ||
|
|
||
| assert WhiteList.validate(command) == expected_result |
There was a problem hiding this comment.
Style question.. do you feel it is better to use command and expected_result variables in cases where the test could fit in one line, like the following?
test "returns :ok when the command is valid" do
assert WhiteList.validate("List.first([1,2,3])") == {:ok, command}
endI don't have strong feelings, just looking to be consistent (I can follow this style in other tests, if you still like your way that includes more explicit variables)
There was a problem hiding this comment.
I prefer the approach you are suggesting so I'm gonna change it
lib/live_view_demo/white_list.ex
Outdated
| @@ -0,0 +1,31 @@ | |||
| defmodule LiveViewDemo.WhiteList do | |||
There was a problem hiding this comment.
Great idea to extract this to its own module 👏 👏 👏
There was a problem hiding this comment.
Just a comment.. we probably want to organize all sandbox modules under LiveViewDemo.Sandbox namespace. We can do it now or later in other PR. This would be LiveViewDemo.Sandbox.WhiteList
| use ExUnit.Case | ||
| alias LiveViewDemo.WhiteList | ||
|
|
||
| describe "validate/2" do |
There was a problem hiding this comment.
I'm not using describe a lot, but this is inspiring me to start using it more often in this project
lib/live_view_demo/white_list.ex
Outdated
| @@ -0,0 +1,31 @@ | |||
| defmodule LiveViewDemo.WhiteList do | |||
| @moduledoc """ | |||
| Analize the ast to filter out non white-listed modules and kernel functions | |||
Walks the AST to check whether a module is allowed to be used or not. Eventually, we'll have a curated list of safe modules.