Add JF_S3_PATH_PREFIX to config and restrict s3_config to be provided… (

#170)

* Restrict available codecs in room for recording component

* Restrict recording component to one per room

* Add JF_S3_PATH_PREFIX to config and restrict s3_config to be provided through only one method

* Requested changes

config/runtime.exs

@@ -35,7 +35,8 @@ config :jellyfish,
   dist_config: ConfigReader.read_dist_config(),
   webrtc_config: ConfigReader.read_webrtc_config(),
   sip_config: ConfigReader.read_sip_config(),
-  s3_credentials: ConfigReader.read_s3_credentials(),
+  recording_config: ConfigReader.read_recording_config(),
+  s3_config: ConfigReader.read_s3_config(),
   git_commit: ConfigReader.read_git_commit()
 
 case System.get_env("JF_SERVER_API_TOKEN") do

lib/jellyfish/component/recording.ex

@@ -13,11 +13,19 @@ defmodule Jellyfish.Component.Recording do
 
   @impl true
   def config(%{engine_pid: engine} = options) do
-    with {:ok, serialized_opts} <- serialize_options(options, Options.schema()),
-         {:ok, credentials} <- get_credentials(serialized_opts) do
-      path_prefix = serialized_opts.path_prefix
-      output_dir = Path.join(get_base_path(), path_prefix)
+    recording_config = Application.fetch_env!(:jellyfish, :recording_config)
+    sink_config = Application.fetch_env!(:jellyfish, :s3_config)
+
+    unless recording_config[:recording_used?],
+      do:
+        raise("""
+        Recording components can only be used if JF_RECORDING_USED environmental variable is set to \"true\"
+        """)
 
+    with {:ok, serialized_opts} <- serialize_options(options, Options.schema()),
+         {:ok, credentials} <- get_credentials(serialized_opts, sink_config),
+         {:ok, path_prefix} <- get_path_prefix(serialized_opts, sink_config) do
+      output_dir = get_base_path()
       File.mkdir_p!(output_dir)
 
       file_storage = {Recording.Storage.File, %{output_dir: output_dir}}
@@ -39,13 +47,24 @@ defmodule Jellyfish.Component.Recording do
     end
   end
 
-  defp get_credentials(%{credentials: nil}) do
-    case Application.fetch_env!(:jellyfish, :s3_credentials) do
-      nil -> {:error, :missing_s3_credentials}
-      credentials -> {:ok, Enum.into(credentials, %{})}
+  defp get_credentials(%{credentials: credentials}, s3_config) do
+    case {credentials, s3_config[:credentials]} do
+      {nil, nil} -> {:error, :missing_s3_credentials}
+      {nil, credentials} -> {:ok, Enum.into(credentials, %{})}
+      {credentials, nil} -> {:ok, credentials}
+      _else -> {:error, :overridding_credentials}
+    end
+  end
+
+  defp get_path_prefix(%{path_prefix: path_prefix}, s3_config) do
+    case {path_prefix, s3_config[:path_prefix]} do
+      {nil, nil} -> {:ok, ""}
+      {nil, path_prefix} -> {:ok, path_prefix}
+      {path_prefix, nil} -> {:ok, path_prefix}
+      _else -> {:error, :overridding_path_prefix}
     end
   end
 
-  defp get_credentials(%{credentials: credentials}), do: {:ok, credentials}
-  defp get_base_path(), do: Application.fetch_env!(:jellyfish, :media_files_path)
+  defp get_base_path(),
+    do: :jellyfish |> Application.fetch_env!(:media_files_path) |> Path.join("raw_recordings")
 end

lib/jellyfish/config_reader.ex

@@ -143,32 +143,48 @@ defmodule Jellyfish.ConfigReader do
     end
   end
 
-  def read_s3_credentials() do
+  def read_recording_config() do
+    [
+      recording_used?: read_boolean("JF_RECORDING_USED") != false
+    ]
+  end
+
+  def read_s3_config() do
     credentials = [
       bucket: System.get_env("JF_S3_BUCKET"),
       region: System.get_env("JF_S3_REGION"),
       access_key_id: System.get_env("JF_S3_ACCESS_KEY_ID"),
       secret_access_key: System.get_env("JF_S3_SECRET_ACCESS_KEY")
     ]
 
-    cond do
-      Enum.all?(credentials, fn {_key, val} -> not is_nil(val) end) ->
-        credentials
-
-      Enum.all?(credentials, fn {_key, val} -> is_nil(val) end) ->
-        nil
+    path_prefix = System.get_env("JF_S3_PATH_PREFIX")
 
-      true ->
-        missing_envs =
+    credentials =
+      cond do
+        Enum.all?(credentials, fn {_key, val} -> not is_nil(val) end) ->
           credentials
-          |> Enum.filter(fn {_key, val} -> val == nil end)
-          |> Enum.map(fn {key, _val} -> "JF_" <> (key |> Atom.to_string() |> String.upcase()) end)
 
-        raise """
-        Either all S3 credentials have to be set: `JF_S3_BUCKET`, `JF_S3_REGION`, `JF_S3_ACCESS_KEY_ID`, `JF_S3_SECRET_ACCESS_KEY`, or none must be set.
-        Currently, the following required credentials are missing: #{inspect(missing_envs)}.
-        """
-    end
+        Enum.all?(credentials, fn {_key, val} -> is_nil(val) end) ->
+          nil
+
+        true ->
+          missing_envs =
+            credentials
+            |> Enum.filter(fn {_key, val} -> val == nil end)
+            |> Enum.map(fn {key, _val} ->
+              "JF_" <> (key |> Atom.to_string() |> String.upcase())
+            end)
+
+          raise """
+          Either all S3 credentials have to be set: `JF_S3_BUCKET`, `JF_S3_REGION`, `JF_S3_ACCESS_KEY_ID`, `JF_S3_SECRET_ACCESS_KEY`, or none must be set.
+          Currently, the following required credentials are missing: #{inspect(missing_envs)}.
+          """
+      end
+
+    [
+      path_prefix: path_prefix,
+      credentials: credentials
+    ]
   end
 
   def read_dist_config() do

lib/jellyfish/room.ex

@@ -318,6 +318,14 @@ defmodule Jellyfish.Room do
         Logger.warning("Unable to add component: missing s3 credentials")
         {:reply, {:error, :missing_s3_credentials}, state}
 
+      {:error, :overridding_credentials} ->
+        Logger.warning("Unable to add component: tried to override s3 credentials")
+        {:reply, {:error, :overridding_credentials}, state}
+
+      {:error, :overridding_path_prefix} ->
+        Logger.warning("Unable to add component: tried to override s3 path_prefix")
+        {:reply, {:error, :overridding_path_prefix}, state}
+
       {:error, reason} ->
         Logger.warning("Unable to add component: #{inspect(reason)}")
         {:reply, :error, state}
@@ -791,8 +799,8 @@ defmodule Jellyfish.Room do
 
   defp check_component_allowed(RTSP, %{config: %{video_codec: video_codec}}) do
     # Right now, RTSP component can only publish H264, so there's no point adding it
-    # to a room which enforces another video codec, e.g. VP8
-    if video_codec in [:h264, nil],
+    # to a room which allows another video codec, e.g. VP8
+    if video_codec == :h264,
       do: :ok,
       else: {:error, :incompatible_codec}
   end

lib/jellyfish_web/controllers/component_controller.ex

@@ -83,6 +83,14 @@ defmodule JellyfishWeb.ComponentController do
         {:error, :bad_request,
          "S3 credentials has to be passed either by request or at application startup as envs"}
 
+      {:error, :overridding_credentials} ->
+        {:error, :bad_request,
+         "Conflicting S3 credentials supplied via environment variables and the REST API. Overrides on existing values are disallowed"}
+
+      {:error, :overridding_path_prefix} ->
+        {:error, :bad_request,
+         "Conflicting S3 path prefix supplied via environment variables and the REST API. Overrides on existing values are disallowed"}
+
       {:error, :invalid_type} ->
         {:error, :bad_request, "Invalid component type"}
 

test/jellyfish/config_reader_test.exs

@@ -143,22 +143,34 @@ defmodule Jellyfish.ConfigReaderTest do
     end
   end
 
-  test "read_s3_credentials/0" do
-    with_env ["JF_S3_BUCKET", "JF_S3_ACCESS_KEY_ID", "JF_S3_SECRET_ACCESS_KEY", "JF_S3_REGION"] do
-      assert ConfigReader.read_s3_credentials() == nil
+  test "read_s3_config/0" do
+    with_env [
+      "JF_S3_BUCKET",
+      "JF_S3_ACCESS_KEY_ID",
+      "JF_S3_SECRET_ACCESS_KEY",
+      "JF_S3_REGION",
+      "JF_S3_PATH_PREFIX"
+    ] do
+      assert ConfigReader.read_s3_config() == [path_prefix: nil, credentials: nil]
+
+      System.put_env("JF_S3_PATH_PREFIX", "path_prefix")
+      assert ConfigReader.read_s3_config() == [path_prefix: "path_prefix", credentials: nil]
 
       System.put_env("JF_S3_BUCKET", "bucket")
-      assert_raise RuntimeError, fn -> ConfigReader.read_s3_credentials() end
+      assert_raise RuntimeError, fn -> ConfigReader.read_s3_config() end
 
       System.put_env("JF_S3_ACCESS_KEY_ID", "id")
       System.put_env("JF_S3_SECRET_ACCESS_KEY", "key")
       System.put_env("JF_S3_REGION", "region")
 
-      assert ConfigReader.read_s3_credentials() == [
-               bucket: "bucket",
-               region: "region",
-               access_key_id: "id",
-               secret_access_key: "key"
+      assert ConfigReader.read_s3_config() == [
+               path_prefix: "path_prefix",
+               credentials: [
+                 bucket: "bucket",
+                 region: "region",
+                 access_key_id: "id",
+                 secret_access_key: "key"
+               ]
              ]
     end
   end

test/jellyfish_web/controllers/component/recording_component_test.exs

@@ -52,7 +52,7 @@ defmodule JellyfishWeb.Component.RecordingComponentTest do
       room_id: room_id
     } do
       mock_http_request()
-      Application.put_env(:jellyfish, :s3_credentials, @s3_credentials)
+      put_s3_envs(path_prefix: nil, credentials: @s3_credentials)
 
       conn =
         post(conn, ~p"/room/#{room_id}/component",
@@ -70,7 +70,43 @@ defmodule JellyfishWeb.Component.RecordingComponentTest do
 
       assert_component_created(conn, room_id, id, "recording")
 
-      Application.put_env(:jellyfish, :s3_credentials, nil)
+      clean_s3_envs()
+    end
+
+    test "renders error when credentials are passed both in config and request", %{
+      conn: conn,
+      room_id: room_id
+    } do
+      put_s3_envs(path_prefix: nil, credentials: @s3_credentials)
+
+      conn =
+        post(conn, ~p"/room/#{room_id}/component",
+          type: "recording",
+          options: %{credentials: @s3_credentials}
+        )
+
+      assert model_response(conn, :bad_request, "Error")["errors"] ==
+               "Conflicting S3 credentials supplied via environment variables and the REST API. Overrides on existing values are disallowed"
+
+      clean_s3_envs()
+    end
+
+    test "renders error when path prefix is passed both in config and request", %{
+      conn: conn,
+      room_id: room_id
+    } do
+      put_s3_envs(path_prefix: @path_prefix, credentials: @s3_credentials)
+
+      conn =
+        post(conn, ~p"/room/#{room_id}/component",
+          type: "recording",
+          options: %{path_prefix: @path_prefix}
+        )
+
+      assert model_response(conn, :bad_request, "Error")["errors"] ==
+               "Conflicting S3 path prefix supplied via environment variables and the REST API. Overrides on existing values are disallowed"
+
+      clean_s3_envs()
     end
 
     test "renders errors when required options are missing", %{conn: conn, room_id: room_id} do
@@ -85,7 +121,8 @@ defmodule JellyfishWeb.Component.RecordingComponentTest do
     end
 
     test "renders errors when video codec is different than h264 - vp8", %{conn: conn} do
-      Application.put_env(:jellyfish, :s3_credentials, @s3_credentials)
+      put_s3_envs(path_prefix: nil, credentials: @s3_credentials)
+
       conn = post(conn, ~p"/room", videoCodec: "vp8")
 
       assert %{"id" => room_id} =
@@ -97,7 +134,7 @@ defmodule JellyfishWeb.Component.RecordingComponentTest do
                "Incompatible video codec enforced in room #{room_id}"
 
       RoomService.delete_room(room_id)
-      Application.put_env(:jellyfish, :s3_credentials, nil)
+      clean_s3_envs()
     end
   end
 
@@ -110,4 +147,15 @@ defmodule JellyfishWeb.Component.RecordingComponentTest do
       {:ok, %{status_code: 200, headers: %{}}}
     end)
   end
+
+  defp put_s3_envs(path_prefix: path_prefix, credentials: credentials) do
+    Application.put_env(:jellyfish, :s3_config,
+      path_prefix: path_prefix,
+      credentials: credentials
+    )
+  end
+
+  defp clean_s3_envs() do
+    Application.put_env(:jellyfish, :s3_config, path_prefix: nil, credentials: nil)
+  end
 end

test/jellyfish_web/controllers/component/rtsp_component_test.exs

@@ -23,6 +23,8 @@ defmodule JellyfishWeb.Component.RTSPComponentTest do
   @rtsp_custom_properties @rtsp_custom_options |> map_keys_to_string()
 
   describe "create rtsp component" do
+    setup [:create_h264_room]
+
     test "renders component with required options", %{conn: conn, room_id: room_id} do
       conn =
         post(conn, ~p"/room/#{room_id}/component",