Refactor secrets form (#1721)

lib/livebook/ecto_types/hex_color.ex

@@ -2,11 +2,17 @@ defmodule Livebook.EctoTypes.HexColor do
   @moduledoc false
   use Ecto.Type
 
+  @impl true
   def type, do: :string
 
+  @impl true
   def load(value), do: {:ok, value}
+
+  @impl true
+
   def dump(value), do: {:ok, value}
 
+  @impl true
   def cast(value) do
     if valid?(value) do
       {:ok, value}

lib/livebook/ecto_types/secret_origin.ex

@@ -1,45 +1,54 @@
 defmodule Livebook.EctoTypes.SecretOrigin do
   @moduledoc false
+
   use Ecto.Type
 
   @type t :: nil | :session | :startup | :app | {:hub, String.t()}
 
+  @impl true
   def type, do: :string
 
-  def load("session"), do: {:ok, :session}
-  def load("app"), do: {:ok, :app}
-  def load("startup"), do: {:ok, :startup}
-
-  def load("hub-" <> id) do
-    if hub_secret?(id),
-      do: {:ok, {:hub, id}},
-      else: :error
-  end
-
-  def load(_), do: :error
+  @impl true
+  def load(origin), do: decode(origin)
 
+  @impl true
   def dump(:session), do: {:ok, "session"}
   def dump(:app), do: {:ok, "app"}
   def dump(:startup), do: {:ok, "startup"}
-
-  def dump({:hub, id}) when is_binary(id) do
-    if hub_secret?(id), do: {:ok, "hub-#{id}"}, else: :error
-  end
-
+  def dump({:hub, id}), do: {:ok, "hub-#{id}"}
   def dump(_), do: :error
 
+  @impl true
   def cast(:session), do: {:ok, :session}
   def cast(:app), do: {:ok, :app}
   def cast(:startup), do: {:ok, :startup}
-  def cast({:hub, id}) when is_binary(id), do: cast(id)
+  def cast({:hub, id}), do: {:hub, id}
 
-  def cast(id) when is_binary(id) do
-    if hub_secret?(id),
-      do: {:ok, {:hub, id}},
-      else: {:error, message: "does not exists"}
+  def cast(encoded) when is_binary(encoded) do
+    case decode(encoded) do
+      {:ok, origin} -> {:ok, origin}
+      :error -> {:error, message: "is invalid"}
+    end
   end
 
   def cast(_), do: {:error, message: "is invalid"}
 
-  defdelegate hub_secret?(id), to: Livebook.Hubs, as: :hub_exists?
+  @doc """
+  Encodes origin into string representation.
+  """
+  @spec encode(t()) :: String.t()
+  def encode(:session), do: "session"
+  def encode(:app), do: "app"
+  def encode(:startup), do: "startup"
+  def encode({:hub, id}), do: "hub-#{id}"
+
+  @doc """
+  Decodes origin from string representation.
+  """
+  @spec decode(String.t()) :: {:ok, t()} | :error
+  def decode("session"), do: {:ok, :session}
+  def decode("app"), do: {:ok, :app}
+  def decode("startup"), do: {:ok, :startup}
+  def decode("hub-" <> id), do: {:ok, {:hub, id}}
+  def decode(_other), do: :error
 end

lib/livebook/secrets.ex

@@ -1,8 +1,6 @@
 defmodule Livebook.Secrets do
   @moduledoc false
 
-  import Ecto.Changeset, only: [apply_action: 2, add_error: 3, get_field: 2, put_change: 3]
-
   alias Livebook.Storage
   alias Livebook.Secrets.Secret
 
@@ -49,41 +47,22 @@ defmodule Livebook.Secrets do
     Storage.fetch(:secrets, id) != :error
   end
 
-  @doc """
-  Validates a secret map and either returns a tuple.
-
-  ## Examples
-
-      iex> validate_secret(%{name: "FOO", value: "bar", origin: "session"})
-      {:ok, %Secret{}}
-
-      iex> validate_secret(%{})
-      {:error, %Ecto.Changeset{}}
-
-  """
-  @spec validate_secret(map()) :: {:ok, Secret.t()} | {:error, Ecto.Changeset.t()}
-  def validate_secret(attrs) do
-    %Secret{}
-    |> Secret.changeset(attrs)
-    |> apply_action(:validate)
-  end
-
   @doc """
   Returns an `%Ecto.Changeset{}` for tracking secret changes.
   """
   @spec change_secret(Secret.t(), map()) :: Ecto.Changeset.t()
   def change_secret(%Secret{} = secret, attrs) do
-    secret
-    |> Secret.changeset(attrs)
-    |> Map.replace!(:action, :validate)
-    |> normalize_origin()
+    Secret.changeset(secret, attrs)
   end
 
-  defp normalize_origin(changeset) do
-    case get_field(changeset, :origin) do
-      {:hub, id} -> put_change(changeset, :origin, "hub-#{id}")
-      _ -> changeset
-    end
+  @doc """
+  Updates secret with the given changes.
+  """
+  @spec update_secret(Secret.t(), map()) :: {:ok, Secret.t()} | {:error, Ecto.Changeset.t()}
+  def update_secret(%Secret{} = secret, attrs) do
+    secret
+    |> Secret.changeset(attrs)
+    |> Ecto.Changeset.apply_action(:update)
   end
 
   @doc """
@@ -94,11 +73,11 @@ defmodule Livebook.Secrets do
   def add_secret_error(%Secret{} = secret, field, message) do
     secret
     |> change_secret(%{})
-    |> add_error(field, message)
+    |> Ecto.Changeset.add_error(field, message)
   end
 
   def add_secret_error(%Ecto.Changeset{} = changeset, field, message) do
-    add_error(changeset, field, message)
+    Ecto.Changeset.add_error(changeset, field, message)
   end
 
   @doc """

lib/livebook_web/components/form_components.ex

@@ -224,10 +224,11 @@ defmodule LivebookWeb.FormComponents do
 
     ~H"""
     <div phx-feedback-for={@name} class={[@errors != [] && "show-errors"]}>
-      <div class="flex items-center gap-2 text-gray-600">
-        <label :for={{value, description} <- @options}>
+      <div class="flex gap-4 text-gray-600">
+        <label :for={{value, description} <- @options} class="flex items-center gap-2 cursor-pointer">
           <input
             type="radio"
+            class="radio-base"
             name={@name}
             id={@id || @name}
             value={value}

lib/livebook_web/live/session_live/secrets_component.ex

@@ -5,15 +5,30 @@ defmodule LivebookWeb.SessionLive.SecretsComponent do
   alias Livebook.Secrets
   alias Livebook.Secrets.Secret
   alias Livebook.Session
+  alias Livebook.EctoTypes.SecretOrigin
+
+  @impl true
+  def mount(socket) do
+    {:ok, assign(socket, title: title(socket), hubs: Livebook.Hubs.get_hubs([:secrets]))}
+  end
 
   @impl true
   def update(assigns, socket) do
+    socket = assign(socket, assigns)
+
+    secret_name = socket.assigns[:prefill_secret_name]
+
     socket =
       socket
-      |> assign(assigns)
-      |> assign(hubs: Livebook.Hubs.get_hubs([:secrets]))
+      |> assign_new(:changeset, fn ->
+        attrs = %{name: secret_name, value: nil, origin: :session}
+        Secrets.change_secret(%Secret{}, attrs)
+      end)
+      |> assign_new(:grant_access_secret, fn ->
+        Enum.find(socket.assigns.saved_secrets, &(&1.name == secret_name))
+      end)
 
-    {:ok, assign(socket, prefill_assigns(socket))}
+    {:ok, socket}
   end
 
   @impl true
@@ -24,9 +39,8 @@ defmodule LivebookWeb.SessionLive.SecretsComponent do
         <%= @title %>
       </h3>
       <.grant_access_message
-        :if={@grant_access_name}
-        secret_name={@grant_access_name}
-        secret_origin={@grant_access_origin}
+        :if={@grant_access_secret}
+        secret={@grant_access_secret}
         target={@myself}
       />
       <div class="flex flex-columns gap-4">
@@ -39,19 +53,16 @@ defmodule LivebookWeb.SessionLive.SecretsComponent do
               <.secret_with_badge
                 :for={{secret_name, _} <- Enum.sort(@secrets)}
                 secret_name={secret_name}
-                secret_origin="session"
+                secret_origin={:session}
                 stored="Session"
-                action="select_secret"
                 active={secret_name == @prefill_secret_name}
                 target={@myself}
               />
               <.secret_with_badge
                 :for={secret <- @saved_secrets}
                 secret_name={secret.name}
-                secret_store={store(secret)}
-                secret_origin={origin(secret)}
+                secret_origin={secret.origin}
                 stored={stored(secret)}
-                action="select_secret"
                 active={false}
                 target={@myself}
               />
@@ -83,7 +94,7 @@ defmodule LivebookWeb.SessionLive.SecretsComponent do
             <.text_field
               field={f[:name]}
               label="Name (alphanumeric and underscore)"
-              autofocus={not @has_prefill}
+              autofocus={@prefill_secret_name == nil}
               spellcheck="false"
               autocomplete="off"
               phx-debounce="blur"
@@ -91,7 +102,7 @@ defmodule LivebookWeb.SessionLive.SecretsComponent do
             <.text_field
               field={f[:value]}
               label="Value"
-              autofocus={@has_prefill}
+              autofocus={@prefill_secret_name != nil}
               spellcheck="false"
               autocomplete="off"
               phx-debounce="blur"
@@ -109,7 +120,7 @@ defmodule LivebookWeb.SessionLive.SecretsComponent do
               }
             />
             <div class="flex space-x-2">
-              <button class="button-base button-blue" type="submit" disabled={f.errors != []}>
+              <button class="button-base button-blue" type="submit" disabled={not @changeset.valid?}>
                 <.remix_icon icon="add-line" class="align-middle" />
                 <span class="font-normal">Add</span>
               </button>
@@ -124,46 +135,6 @@ defmodule LivebookWeb.SessionLive.SecretsComponent do
     """
   end
 
-  defp secret_with_badge(%{secret_store: "hub"} = assigns) do
-    ~H"""
-    <div
-      role="button"
-      class={[
-        "flex justify-between w-full font-mono text-sm p-2 border-b cursor-pointer",
-        if @active do
-          "bg-blue-100 text-blue-700"
-        else
-          "text-gray-700 hover:bg-gray-100"
-        end
-      ]}
-      phx-value-name={@secret_name}
-      phx-value-origin={"hub-" <> @secret_origin}
-      phx-target={@target}
-      phx-click={@action}
-    >
-      <%= @secret_name %>
-      <span class={[
-        "inline-flex items-center font-sans rounded-full px-2.5 py-0.5 text-xs font-medium bg-gray-100",
-        if @active do
-          "bg-indigo-100 text-blue-800"
-        else
-          "bg-gray-100 text-gray-800"
-        end
-      ]}>
-        <svg
-          :if={@active}
-          class="-ml-0.5 mr-1.5 h-2 w-2 text-blue-400"
-          fill="currentColor"
-          viewBox="0 0 8 8"
-        >
-          <circle cx="4" cy="4" r="3" />
-        </svg>
-        <%= @stored %>
-      </span>
-    </div>
-    """
-  end
-
   defp secret_with_badge(assigns) do
     ~H"""
     <div
@@ -177,9 +148,9 @@ defmodule LivebookWeb.SessionLive.SecretsComponent do
         end
       ]}
       phx-value-name={@secret_name}
-      phx-value-origin={@secret_store}
+      phx-value-origin={SecretOrigin.encode(@secret_origin)}
       phx-target={@target}
-      phx-click={@action}
+      phx-click="select_secret"
     >
       <%= @secret_name %>
       <span class={[
@@ -215,41 +186,29 @@ defmodule LivebookWeb.SessionLive.SecretsComponent do
                 icon="error-warning-fill"
                 class="align-middle text-2xl flex text-gray-100 rounded-lg py-2"
               />
-              <%= if @secret_origin in ["app", "startup"] do %>
+              <%= if @secret.origin in [:app, :startup] do %>
                 <span class="ml-2 text-sm font-normal text-gray-100">
                   There is a secret named
-                  <span class="font-semibold text-white"><%= @secret_name %></span>
+                  <span class="font-semibold text-white"><%= @secret.name %></span>
                   in your Livebook app. Allow this session to access it?
                 </span>
               <% else %>
                 <span class="ml-2 text-sm font-normal text-gray-100">
                   There is a secret named
-                  <span class="font-semibold text-white"><%= @secret_name %></span>
+                  <span class="font-semibold text-white"><%= @secret.name %></span>
                   in your Livebook Hub. Allow this session to access it?
                 </span>
               <% end %>
             </div>
-            <%= if @secret_origin in ["app", "startup"] do %>
-              <button
-                class="button-base button-gray"
-                phx-click="grant_access"
-                phx-value-name={@secret_name}
-                phx-value-origin={@secret_origin}
-                phx-target={@target}
-              >
-                Grant access
-              </button>
-            <% else %>
-              <button
-                class="button-base button-gray"
-                phx-click="grant_access"
-                phx-value-name={@secret_name}
-                phx-value-origin={"hub-" <> @secret_origin}
-                phx-target={@target}
-              >
-                Grant access
-              </button>
-            <% end %>
+            <button
+              class="button-base button-gray"
+              phx-click="grant_access"
+              phx-value-name={@secret.name}
+              phx-value-origin={SecretOrigin.encode(@secret.origin)}
+              phx-target={@target}
+            >
+              Grant access
+            </button>
           </div>
         </div>
       </div>
@@ -257,42 +216,12 @@ defmodule LivebookWeb.SessionLive.SecretsComponent do
     """
   end
 
-  defp prefill_assigns(socket) do
-    secret_name = socket.assigns[:prefill_secret_name]
-    attrs = %{name: secret_name, value: nil, origin: "session"}
-
-    assigns = %{
-      changeset: Secrets.change_secret(%Secret{}, attrs),
-      title: title(socket),
-      grant_access_name: nil,
-      grant_access_origin: "app",
-      has_prefill: !is_nil(secret_name)
-    }
-
-    case Enum.find(socket.assigns.saved_secrets, &(&1.name == secret_name)) do
-      %Secret{name: name, origin: {:hub, id}} ->
-        %{assigns | grant_access_name: name, grant_access_origin: id}
-
-      %Secret{name: name, origin: origin} ->
-        %{assigns | grant_access_name: name, grant_access_origin: to_string(origin)}
-
-      nil ->
-        assigns
-    end
-  end
-
-  defp store(%{origin: {:hub, _id}}), do: "hub"
-  defp store(%{origin: origin}), do: to_string(origin)
-
-  defp origin(%{origin: {:hub, id}}), do: id
-  defp origin(%{origin: origin}), do: to_string(origin)
-
   defp stored(%{origin: {:hub, _}}), do: "Hub"
   defp stored(%{origin: origin}) when origin in [:app, :startup], do: "Livebook"
 
   @impl true
   def handle_event("save", %{"secret" => attrs}, socket) do
-    with {:ok, secret} <- Secrets.validate_secret(build_attrs(attrs)),
+    with {:ok, secret} <- Secrets.update_secret(%Secret{}, attrs),
          :ok <- set_secret(socket, secret) do
       {:noreply,
        socket
@@ -304,8 +233,9 @@ defmodule LivebookWeb.SessionLive.SecretsComponent do
     end
   end
 
-  def handle_event("select_secret", %{"name" => secret_name} = attrs, socket) do
-    grant_access(socket.assigns.saved_secrets, secret_name, build_origin(attrs), socket)
+  def handle_event("select_secret", %{"name" => secret_name, "origin" => origin}, socket) do
+    {:ok, origin} = SecretOrigin.decode(origin)
+    grant_access(socket.assigns.saved_secrets, secret_name, origin, socket)
 
     {:noreply,
      socket
@@ -314,11 +244,17 @@ defmodule LivebookWeb.SessionLive.SecretsComponent do
   end
 
   def handle_event("validate", %{"secret" => attrs}, socket) do
-    {:noreply, assign(socket, changeset: Secrets.change_secret(%Secret{}, build_attrs(attrs)))}
+    changeset =
+      %Secret{}
+      |> Secrets.change_secret(attrs)
+      |> Map.put(:action, :validate)
+
+    {:noreply, assign(socket, changeset: changeset)}
   end
 
-  def handle_event("grant_access", %{"name" => secret_name} = attrs, socket) do
-    grant_access(socket.assigns.saved_secrets, secret_name, build_origin(attrs), socket)
+  def handle_event("grant_access", %{"name" => secret_name, "origin" => origin}, socket) do
+    {:ok, origin} = SecretOrigin.decode(origin)
+    grant_access(socket.assigns.saved_secrets, secret_name, origin, socket)
 
     {:noreply,
      socket
@@ -336,13 +272,6 @@ defmodule LivebookWeb.SessionLive.SecretsComponent do
   defp title(%{assigns: %{select_secret_options: %{"title" => title}}}), do: title
   defp title(_), do: "Select secret"
 
-  defp build_origin(%{"origin" => "hub-" <> id}), do: {:hub, id}
-  defp build_origin(%{"origin" => store}), do: String.to_existing_atom(store)
-
-  defp build_attrs(%{"name" => name, "value" => value} = attrs) do
-    %{name: name, value: value, origin: build_origin(attrs)}
-  end
-
   defp set_secret(socket, %Secret{origin: :session} = secret) do
     Session.set_secret(socket.assigns.session.pid, secret)
   end

test/livebook/secrets_test.exs

@@ -3,6 +3,7 @@ defmodule Livebook.SecretsTest do
   use Livebook.DataCase
 
   alias Livebook.Secrets
+  alias Livebook.Secrets.Secret
 
   describe "get_secrets/0" do
     test "returns a list of secrets from storage" do
@@ -51,11 +52,11 @@ defmodule Livebook.SecretsTest do
     Secrets.unset_secret("FOO")
   end
 
-  describe "validate_secret/1" do
+  describe "update_secret/2" do
     test "returns a valid secret" do
       attrs = params_for(:secret, name: "FOO", value: "111")
 
-      assert {:ok, secret} = Secrets.validate_secret(attrs)
+      assert {:ok, secret} = Secrets.update_secret(%Secret{}, attrs)
       assert attrs.name == secret.name
       assert attrs.value == secret.value
       assert attrs.origin == secret.origin
@@ -63,11 +64,11 @@ defmodule Livebook.SecretsTest do
 
     test "returns changeset error" do
       attrs = params_for(:secret, name: nil, value: "111")
-      assert {:error, changeset} = Secrets.validate_secret(attrs)
+      assert {:error, changeset} = Secrets.update_secret(%Secret{}, attrs)
       assert "can't be blank" in errors_on(changeset).name
 
       attrs = params_for(:secret, name: "@inavalid", value: "111")
-      assert {:error, changeset} = Secrets.validate_secret(attrs)
+      assert {:error, changeset} = Secrets.update_secret(%Secret{}, attrs)
 
       assert "should contain only alphanumeric characters and underscore" in errors_on(changeset).name
     end