Refactor api tokens (#1370)

Related to #1364

Co-authored-by: Jamil Bou Kheir <jamilbk@users.noreply.github.com>

apps/fz_http/lib/fz_http/api_tokens.ex

@@ -1,47 +1,49 @@
 defmodule FzHttp.ApiTokens do
-  @moduledoc """
-  The ApiTokens context.
-  """
-
-  import Ecto.Query, warn: false
-  alias FzHttp.Repo
-
+  alias FzHttp.{Repo, Validator}
+  alias FzHttp.Users
   alias FzHttp.ApiTokens.ApiToken
 
   def count_by_user_id(user_id) do
-    Repo.aggregate(from(a in ApiToken, where: a.user_id == ^user_id), :count)
+    ApiToken.Query.by_user_id(user_id)
+    |> Repo.aggregate(:count)
   end
 
   def list_api_tokens do
-    Repo.all(ApiToken)
+    ApiToken.Query.all()
+    |> Repo.list()
   end
 
-  def list_api_tokens(user_id) do
-    Repo.all(from a in ApiToken, where: a.user_id == ^user_id)
+  def list_api_tokens_by_user_id(user_id) do
+    ApiToken.Query.by_user_id(user_id)
+    |> Repo.list()
   end
 
-  def get_api_token(id), do: Repo.get(ApiToken, id)
+  def fetch_api_token_by_id(id) do
+    if Validator.valid_uuid?(id) do
+      ApiToken.Query.by_id(id)
+      |> Repo.fetch()
+    else
+      {:error, :not_found}
+    end
+  end
 
-  def get_api_token!(id), do: Repo.get!(ApiToken, id)
-
-  def get_unexpired_api_token(api_token_id) do
-    now = DateTime.utc_now()
-
-    Repo.one(
-      from a in ApiToken,
-        where: a.id == ^api_token_id and a.expires_at >= ^now
-    )
+  def fetch_unexpired_api_token_by_id(id) do
+    if Validator.valid_uuid?(id) do
+      ApiToken.Query.by_id(id)
+      |> ApiToken.Query.not_expired()
+      |> Repo.fetch()
+    else
+      {:error, :not_found}
+    end
   end
 
   def new_api_token(attrs \\ %{}) do
-    ApiToken.create_changeset(attrs)
+    ApiToken.Changeset.changeset(attrs)
   end
 
   def create_user_api_token(%FzHttp.Users.User{} = user, params) do
-    changeset =
-      params
-      |> Enum.into(%{"user_id" => user.id})
-      |> ApiToken.create_changeset(count_per_user: count_by_user_id(user.id))
+    count_by_user_id = count_by_user_id(user.id)
+    changeset = ApiToken.Changeset.create_changeset(user, params, max: count_by_user_id)
 
     with {:ok, api_token} <- Repo.insert(changeset) do
       FzHttp.Telemetry.create_api_token()
@@ -53,10 +55,14 @@ defmodule FzHttp.ApiTokens do
     DateTime.diff(api_token.expires_at, DateTime.utc_now()) < 0
   end
 
-  def delete_api_token(%ApiToken{} = api_token) do
-    with {:ok, api_token} <- Repo.delete(api_token) do
-      FzHttp.Telemetry.delete_api_token(api_token)
-      {:ok, api_token}
+  def delete_api_token_by_id(api_token_id, %Users.User{} = user) do
+    with {:ok, api_token} <- fetch_api_token_by_id(api_token_id),
+         # A user can only delete his/her own MFA method!
+         true <- api_token.user_id == user.id do
+      {:ok, Repo.delete!(api_token)}
+    else
+      {:error, :not_found} -> {:error, :not_found}
+      false -> {:error, :not_found}
     end
   end
 end

apps/fz_http/lib/fz_http/api_tokens/api_token.ex

@@ -1,8 +1,5 @@
 defmodule FzHttp.ApiTokens.ApiToken do
   use FzHttp, :schema
-  import Ecto.Changeset
-
-  @max_per_user 25
 
   schema "api_tokens" do
     field :expires_at, :utc_datetime_usec
@@ -14,38 +11,4 @@ defmodule FzHttp.ApiTokens.ApiToken do
 
     timestamps(updated_at: false)
   end
-
-  def create_changeset(attrs, opts \\ []) do
-    %__MODULE__{}
-    |> cast(attrs, ~w[
-      user_id
-      expires_in
-      expires_at
-    ]a)
-    |> validate_required([:user_id, :expires_in])
-    |> validate_number(:expires_in, greater_than_or_equal_to: 1, less_than_or_equal_to: 90)
-    |> resolve_expires_at()
-    |> validate_required(:expires_at)
-    |> assoc_constraint(:user)
-    |> maybe_validate_count_per_user(@max_per_user, opts[:count_per_user])
-  end
-
-  def max_per_user, do: @max_per_user
-
-  defp resolve_expires_at(changeset) do
-    expires_at =
-      DateTime.utc_now()
-      |> DateTime.add(get_field(changeset, :expires_in), :day)
-
-    put_change(changeset, :expires_at, expires_at)
-  end
-
-  defp maybe_validate_count_per_user(changeset, max, num) when is_integer(num) and num >= max do
-    # XXX: This suffers from a race condition because the count happens in a separate transaction.
-    # At the moment it's not a big concern. Fixing it would require locking against INSERTs or DELETEs
-    # while counts are happening.
-    add_error(changeset, :base, "token limit of #{@max_per_user} reached")
-  end
-
-  defp maybe_validate_count_per_user(changeset, _, _), do: changeset
 end

apps/fz_http/lib/fz_http/api_tokens/api_token/changeset.ex

@@ -0,0 +1,44 @@
+defmodule FzHttp.ApiTokens.ApiToken.Changeset do
+  use FzHttp, :changeset
+  alias FzHttp.ApiTokens.ApiToken
+
+  @max_per_user 25
+
+  def create_changeset(user, attrs, opts \\ []) do
+    changeset(attrs)
+    |> put_change(:user_id, user.id)
+    |> assoc_constraint(:user)
+    |> maybe_validate_count_per_user(@max_per_user, opts[:max])
+  end
+
+  def changeset(api_token \\ %ApiToken{}, attrs) do
+    api_token
+    |> cast(attrs, ~w[
+      expires_in
+      expires_at
+    ]a)
+    |> validate_required([:expires_in])
+    |> validate_number(:expires_in, greater_than_or_equal_to: 1, less_than_or_equal_to: 90)
+    |> resolve_expires_at()
+    |> validate_required(:expires_at)
+  end
+
+  def max_per_user, do: @max_per_user
+
+  defp resolve_expires_at(changeset) do
+    expires_at =
+      DateTime.utc_now()
+      |> DateTime.add(get_field(changeset, :expires_in), :day)
+
+    put_change(changeset, :expires_at, expires_at)
+  end
+
+  defp maybe_validate_count_per_user(changeset, max, num) when is_integer(num) and num >= max do
+    # XXX: This suffers from a race condition because the count happens in a separate transaction.
+    # At the moment it's not a big concern. Fixing it would require locking against INSERTs or DELETEs
+    # while counts are happening.
+    add_error(changeset, :base, "token limit of #{@max_per_user} reached")
+  end
+
+  defp maybe_validate_count_per_user(changeset, _, _), do: changeset
+end

apps/fz_http/lib/fz_http/api_tokens/api_token/query.ex

@@ -0,0 +1,19 @@
+defmodule FzHttp.ApiTokens.ApiToken.Query do
+  use FzHttp, :query
+
+  def all do
+    from(api_tokens in FzHttp.ApiTokens.ApiToken, as: :api_tokens)
+  end
+
+  def by_id(queryable \\ all(), id) do
+    where(queryable, [api_tokens: api_tokens], api_tokens.id == ^id)
+  end
+
+  def by_user_id(queryable \\ all(), user_id) do
+    where(queryable, [api_tokens: api_tokens], api_tokens.user_id == ^user_id)
+  end
+
+  def not_expired(queryable \\ all()) do
+    where(queryable, [api_tokens: api_tokens], api_tokens.expires_at >= fragment("NOW()"))
+  end
+end

apps/fz_http/lib/fz_http_web/auth/json/authentication.ex

@@ -18,12 +18,10 @@ defmodule FzHttpWeb.Auth.JSON.Authentication do
 
   @impl Guardian
   def resource_from_claims(%{"api" => api_token_id}) do
-    with %ApiTokens.ApiToken{} = api_token <- ApiTokens.get_unexpired_api_token(api_token_id),
+    with {:ok, %ApiTokens.ApiToken{} = api_token} <-
+           ApiTokens.fetch_unexpired_api_token_by_id(api_token_id),
          {:ok, %Users.User{} = user} <- Users.fetch_user_by_id(api_token.user_id) do
       {:ok, user}
-    else
-      _ ->
-        {:error, :resource_not_found}
     end
   end
 

apps/fz_http/lib/fz_http_web/live/setting_live/account.html.heex

@@ -200,7 +200,7 @@
     <% end %>
   </div>
 
-  <%= if length(@api_tokens) < FzHttp.ApiTokens.ApiToken.max_per_user() do %>
+  <%= if length(@api_tokens) < FzHttp.ApiTokens.ApiToken.Changeset.max_per_user() do %>
     <.link patch={~p"/settings/account/api_token"} class="button">
       <span class="icon is-small">
         <i class="mdi mdi-plus"></i>

apps/fz_http/lib/fz_http_web/live/setting_live/account_live.ex

@@ -23,13 +23,14 @@ defmodule FzHttpWeb.SettingLive.Account do
   def mount(params, _session, socket) do
     Endpoint.subscribe(@live_sessions_topic)
     {:ok, methods} = MFA.list_methods_for_user(socket.assigns.current_user)
+    {:ok, api_tokens} = ApiTokens.list_api_tokens_by_user_id(socket.assigns.current_user.id)
 
     socket =
       socket
       |> assign(:api_token_id, params["api_token_id"])
       |> assign(:subscribe_link, subscribe_link())
       |> assign(:allow_delete, Users.count_by_role(:admin) > 1)
-      |> assign(:api_tokens, ApiTokens.list_api_tokens(socket.assigns.current_user.id))
+      |> assign(:api_tokens, api_tokens)
       |> assign(:changeset, Users.change_user(socket.assigns.current_user))
       |> assign(:methods, methods)
       |> assign(:page_title, @page_title)
@@ -45,30 +46,32 @@ defmodule FzHttpWeb.SettingLive.Account do
 
   @impl Phoenix.LiveView
   def handle_params(%{"api_token_id" => api_token_id}, _url, socket) do
-    {:noreply,
-     socket
-     |> assign(:api_token, ApiTokens.get_api_token!(api_token_id))}
+    {:ok, api_token} = ApiTokens.fetch_unexpired_api_token_by_id(api_token_id)
+    {:noreply, assign(socket, :api_token, api_token)}
   end
 
   @impl Phoenix.LiveView
   def handle_params(_params, _url, socket) do
-    {:noreply,
-     socket
-     |> assign(:allow_delete, Users.count_by_role(:admin) > 1)
-     |> assign(:api_tokens, ApiTokens.list_api_tokens(socket.assigns.current_user.id))}
+    {:ok, api_tokens} = ApiTokens.list_api_tokens_by_user_id(socket.assigns.current_user.id)
+
+    socket =
+      socket
+      |> assign(:allow_delete, Users.count_by_role(:admin) > 1)
+      |> assign(:api_tokens, api_tokens)
+
+    {:noreply, socket}
   end
 
   @impl Phoenix.LiveView
   def handle_event("delete_api_token", %{"id" => id}, socket) do
-    api_token = ApiTokens.get_api_token!(id)
+    case ApiTokens.delete_api_token_by_id(id, socket.assigns.current_user) do
+      {:ok, _api_token} ->
+        {:ok, api_tokens} = ApiTokens.list_api_tokens_by_user_id(socket.assigns.current_user.id)
+        {:noreply, assign(socket, :api_tokens, api_tokens)}
 
-    if api_token.user_id == socket.assigns.current_user.id do
-      {:ok, _deleted} = ApiTokens.delete_api_token(api_token)
+      {:error, :not_found} ->
+        {:noreply, socket}
     end
-
-    {:noreply,
-     socket
-     |> assign(:api_tokens, ApiTokens.list_api_tokens(socket.assigns.current_user.id))}
   end
 
   @impl Phoenix.LiveView

apps/fz_http/lib/fz_http_web/live/setting_live/oidc_form_component.ex

@@ -149,7 +149,9 @@ defmodule FzHttpWeb.SettingLive.OIDCFormComponent do
 
           <div class="level">
             <div class="level-left">
-              <p class="help">Automatically create users when signing in for the first time.</p>
+              <p class="help">
+                Automatically provision users when signing in for the first time.
+              </p>
               <p class="help is-danger">
                 <%= error_tag(f, :auto_create_users) %>
               </p>

apps/fz_http/lib/fz_http_web/live/setting_live/saml_form_component.ex

@@ -171,7 +171,9 @@ defmodule FzHttpWeb.SettingLive.SAMLFormComponent do
 
           <div class="level">
             <div class="level-left">
-              <p class="help">Automatically create users when signing in for the first time.</p>
+              <p class="help">
+                Automatically provision users when signing in for the first time.
+              </p>
               <p class="help is-danger">
                 <%= error_tag(f, :auto_create_users) %>
               </p>

apps/fz_http/lib/fz_http_web/user_from_auth.ex

@@ -6,6 +6,7 @@ defmodule FzHttpWeb.UserFromAuth do
   alias FzHttp.Users
   alias FzHttpWeb.Auth.HTML.Authentication
 
+  # Local auth
   def find_or_create(
         %Ueberauth.Auth{
           provider: :identity,

apps/fz_http/test/fz_http/api_tokens_test.exs

@@ -1,50 +1,142 @@
 defmodule FzHttp.ApiTokensTest do
   use FzHttp.DataCase
-
+  alias FzHttp.ApiTokensFixtures
+  alias FzHttp.UsersFixtures
   alias FzHttp.ApiTokens
+  alias FzHttp.ApiTokens.ApiToken
 
-  describe "api_tokens" do
-    alias FzHttp.ApiTokens.ApiToken
-
-    import FzHttp.ApiTokensFixtures
-    import FzHttp.UsersFixtures
-
-    @invalid_params %{"expires_in" => 0}
-
-    test "list_api_tokens/0 returns all api_tokens" do
-      api_token = api_token()
-      assert ApiTokens.list_api_tokens() == [api_token]
+  describe "count_by_user_id/1" do
+    test "returns 0 when no user exist" do
+      assert ApiTokens.count_by_user_id(Ecto.UUID.generate()) == 0
     end
 
-    test "list_api_tokens/1 returns api_tokens scoped to a user" do
-      api_token1 = api_token()
-      api_token2 = api_token()
-      assert [api_token1] == ApiTokens.list_api_tokens(api_token1.user_id)
-      assert [api_token2] == ApiTokens.list_api_tokens(api_token2.user_id)
+    test "returns the number of api_tokens for a user" do
+      user = UsersFixtures.create_user()
+      assert ApiTokens.count_by_user_id(user.id) == 0
+
+      ApiTokensFixtures.create_api_token(user: user)
+      assert ApiTokens.count_by_user_id(user.id) == 1
+
+      ApiTokensFixtures.create_api_token(user: user)
+      assert ApiTokens.count_by_user_id(user.id) == 2
+    end
+  end
+
+  describe "list_api_tokens/0" do
+    test "returns empty list when no api tokens" do
+      assert ApiTokens.list_api_tokens() == {:ok, []}
     end
 
-    test "get_api_token!/1 returns the api_token with given id" do
-      api_token = api_token()
-      assert ApiTokens.get_api_token!(api_token.id) == api_token
+    test "returns all api_tokens" do
+      assert ApiTokens.list_api_tokens() == {:ok, []}
+
+      api_token = ApiTokensFixtures.create_api_token()
+      assert ApiTokens.list_api_tokens() == {:ok, [api_token]}
+    end
+  end
+
+  describe "list_api_tokens_by_user_id/1" do
+    test "returns api tokens scoped to a user" do
+      api_token1 = ApiTokensFixtures.create_api_token()
+      api_token2 = ApiTokensFixtures.create_api_token()
+
+      assert ApiTokens.list_api_tokens_by_user_id(api_token1.user_id) == {:ok, [api_token1]}
+      assert ApiTokens.list_api_tokens_by_user_id(api_token2.user_id) == {:ok, [api_token2]}
+    end
+  end
+
+  describe "fetch_api_token_by_id/1" do
+    test "returns error when UUID is invalid" do
+      assert ApiTokens.fetch_api_token_by_id("foo") == {:error, :not_found}
     end
 
-    test "create_user_api_token/2 with valid data creates a api_token" do
+    test "returns api token by id" do
+      api_token = ApiTokensFixtures.create_api_token()
+      assert ApiTokens.fetch_api_token_by_id(api_token.id) == {:ok, api_token}
+    end
+  end
+
+  describe "fetch_unexpired_api_token_by_id/1" do
+    test "fetches the unexpired token" do
+      api_token = ApiTokensFixtures.create_api_token()
+      assert ApiTokens.fetch_unexpired_api_token_by_id(api_token.id) == {:ok, api_token}
+    end
+
+    test "returns error for expired token" do
+      api_token =
+        ApiTokensFixtures.create_api_token(%{"expires_in" => 1})
+        |> ApiTokensFixtures.expire_api_token()
+
+      assert ApiTokens.fetch_unexpired_api_token_by_id(api_token.id) == {:error, :not_found}
+    end
+  end
+
+  describe "create_user_api_token/2" do
+    test "creates an api_token" do
+      user = UsersFixtures.create_user()
+
       valid_params = %{
         "expires_in" => 1
       }
 
-      assert {:ok, %ApiToken{} = api_token} =
-               ApiTokens.create_user_api_token(user(), valid_params)
+      assert {:ok, %ApiToken{} = api_token} = ApiTokens.create_user_api_token(user, valid_params)
 
       # Within 10 seconds
       assert_in_delta DateTime.to_unix(api_token.expires_at),
                       DateTime.to_unix(DateTime.add(DateTime.utc_now(), 1, :day)),
                       10
+
+      assert api_token.user_id == user.id
+      assert api_token.expires_in == 1
     end
 
-    test "create_user_api_token/2 with invalid data returns error changeset" do
-      assert {:error, %Ecto.Changeset{}} =
-               ApiTokens.create_user_api_token(user(), @invalid_params)
+    test "returns changeset error on invalid data" do
+      user = UsersFixtures.create_user()
+
+      assert {:error, %Ecto.Changeset{} = changeset} =
+               ApiTokens.create_user_api_token(user, %{"expires_in" => 0})
+
+      assert changeset.valid? == false
+      assert errors_on(changeset) == %{expires_in: ["must be greater than or equal to 1"]}
+    end
+  end
+
+  describe "api_token_expired?/1" do
+    test "returns true when expired" do
+      api_token =
+        ApiTokensFixtures.create_api_token(%{"expires_in" => 1})
+        |> ApiTokensFixtures.expire_api_token()
+
+      assert ApiTokens.api_token_expired?(api_token) == true
+    end
+
+    test "returns false when not expired" do
+      api_token = ApiTokensFixtures.create_api_token(%{"expires_in" => 1})
+      assert ApiTokens.api_token_expired?(api_token) == false
+    end
+  end
+
+  describe "delete_api_token_by_id/1" do
+    test "deletes the api token" do
+      user = UsersFixtures.create_user()
+      api_token = ApiTokensFixtures.create_api_token(user: user)
+
+      assert {:ok, deleted_api_token} = ApiTokens.delete_api_token_by_id(api_token.id, user)
+
+      assert deleted_api_token.id == api_token.id
+      refute Repo.one(ApiTokens.ApiToken)
+    end
+
+    test "returns error when api token did not belong to user" do
+      user = UsersFixtures.create_user()
+      api_token = ApiTokensFixtures.create_api_token()
+
+      assert ApiTokens.delete_api_token_by_id(api_token.id, user) == {:error, :not_found}
+    end
+
+    test "returns error when api token does not exist" do
+      user = UsersFixtures.create_user()
+      assert ApiTokens.delete_api_token_by_id(Ecto.UUID.generate(), user) == {:error, :not_found}
     end
   end
 end

apps/fz_http/test/fz_http_web/auth/json/authentication_test.exs

@@ -0,0 +1,33 @@
+defmodule FzHttpWeb.Auth.JSON.AuthenticationTest do
+  use FzHttpWeb.ApiCase, async: true
+  alias FzHttp.UsersFixtures
+  import FzHttpWeb.ApiCase
+
+  test "renders error when api token is invalid" do
+    conn =
+      api_conn()
+      |> Plug.Conn.put_req_header("authorization", "bearer invalid")
+      |> FzHttpWeb.Auth.JSON.Pipeline.call([])
+
+    assert json_response(conn, 401) == %{"errors" => %{"auth" => "invalid_token"}}
+  end
+
+  test "renders error when api token resource is invalid" do
+    user = UsersFixtures.user(%{role: :admin})
+
+    claims = %{
+      "api" => Ecto.UUID.generate(),
+      "exp" => DateTime.to_unix(DateTime.utc_now() |> DateTime.add(1, :hour))
+    }
+
+    {:ok, token, _claims} =
+      Guardian.encode_and_sign(FzHttpWeb.Auth.JSON.Authentication, user, claims)
+
+    conn =
+      api_conn()
+      |> Plug.Conn.put_req_header("authorization", "bearer #{token}")
+      |> FzHttpWeb.Auth.JSON.Pipeline.call([])
+
+    assert json_response(conn, 401) == %{"errors" => %{"auth" => "no_resource_found"}}
+  end
+end

apps/fz_http/test/support/api_case.ex

@@ -51,7 +51,7 @@ defmodule FzHttpWeb.ApiCase do
 
   def authed_conn do
     user = UsersFixtures.user(%{role: :admin})
-    api_token = ApiTokensFixtures.api_token(%{"user_id" => user.id})
+    api_token = ApiTokensFixtures.create_api_token(user: user)
 
     {:ok, token, _claims} = FzHttpWeb.Auth.JSON.Authentication.fz_encode_and_sign(api_token, user)
 

apps/fz_http/test/support/fixtures/api_tokens_fixtures.ex

@@ -1,25 +1,21 @@
 defmodule FzHttp.ApiTokensFixtures do
-  @moduledoc """
-  This module defines test helpers for creating
-  entities via the `FzHttp.ApiTokens` context.
-  """
+  alias FzHttp.UsersFixtures
 
-  @doc """
-  Generate a api_token.
-  """
-  def api_token(params \\ %{}) do
-    user_id =
-      Map.get_lazy(
-        params,
-        "user_id",
-        fn ->
-          FzHttp.UsersFixtures.user().id
-        end
-      )
-
-    {:ok, api_token} =
-      FzHttp.ApiTokens.create_user_api_token(%FzHttp.Users.User{id: user_id}, params)
+  def api_token_attrs(attrs \\ %{}) do
+    Enum.into(attrs, %{})
+  end
 
+  def create_api_token(attrs \\ %{}) do
+    attrs = api_token_attrs(attrs)
+    {user, attrs} = Map.pop_lazy(attrs, :user, fn -> UsersFixtures.user() end)
+    {:ok, api_token} = FzHttp.ApiTokens.create_user_api_token(user, attrs)
     api_token
   end
+
+  def expire_api_token(api_token) do
+    one_second_ago = DateTime.utc_now() |> DateTime.add(-1, :second)
+
+    Ecto.Changeset.change(api_token, expires_at: one_second_ago)
+    |> FzHttp.Repo.update!()
+  end
 end