Merge branch 'feature/1790-oban-overuse-clear-oauth-token' into 'develop'

Feature/1790 removing expired tokens through Oban scheduled jobs

See merge request pleroma/pleroma!2957

CHANGELOG.md

@@ -13,6 +13,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/).
 ### Removed
 
 - **Breaking:** Removed `Pleroma.Workers.Cron.StatsWorker` setting from Oban `:crontab`.
+- **Breaking:** Removed `Pleroma.Workers.Cron.ClearOauthTokenWorker` setting from Oban `:crontab` config.
 
 ## [2.1.1] - 2020-09-08
 

config/config.exs

@@ -530,6 +530,7 @@ config :pleroma, Oban,
   log: false,
   queues: [
     activity_expiration: 10,
+    token_expiration: 5,
     federator_incoming: 50,
     federator_outgoing: 50,
     web_push: 50,
@@ -543,7 +544,6 @@ config :pleroma, Oban,
   ],
   plugins: [Oban.Plugins.Pruner],
   crontab: [
-    {"0 0 * * *", Pleroma.Workers.Cron.ClearOauthTokenWorker},
     {"* * * * *", Pleroma.Workers.Cron.PurgeExpiredActivitiesWorker},
     {"0 0 * * 0", Pleroma.Workers.Cron.DigestEmailsWorker},
     {"0 0 * * *", Pleroma.Workers.Cron.NewUsersDigestWorker}

config/description.exs

@@ -2290,7 +2290,6 @@ config :pleroma, :config_description, [
         type: {:list, :tuple},
         description: "Settings for cron background jobs",
         suggestions: [
-          {"0 0 * * *", Pleroma.Workers.Cron.ClearOauthTokenWorker},
           {"* * * * *", Pleroma.Workers.Cron.PurgeExpiredActivitiesWorker},
           {"0 0 * * 0", Pleroma.Workers.Cron.DigestEmailsWorker},
           {"0 0 * * *", Pleroma.Workers.Cron.NewUsersDigestWorker}

docs/configuration/cheatsheet.md

@@ -691,9 +691,8 @@ Pleroma has the following queues:
 
 Pleroma has these periodic job workers:
 
-`Pleroma.Workers.Cron.ClearOauthTokenWorker` - a job worker to cleanup expired oauth tokens.
+* `Pleroma.Workers.Cron.DigestEmailsWorker` - digest emails for users with new mentions and follows
-
+* `Pleroma.Workers.Cron.NewUsersDigestWorker` - digest emails for admins with new registrations
-Example:
 
 ```elixir
 config :pleroma, Oban,
@@ -705,7 +704,8 @@ config :pleroma, Oban,
     federator_outgoing: 50
   ],
   crontab: [
-    {"0 0 * * *", Pleroma.Workers.Cron.ClearOauthTokenWorker}
+    {"0 0 * * 0", Pleroma.Workers.Cron.DigestEmailsWorker},
+    {"0 0 * * *", Pleroma.Workers.Cron.NewUsersDigestWorker}
   ]
 ```
 
@@ -972,7 +972,7 @@ Configure OAuth 2 provider capabilities:
 
 * `token_expires_in` - The lifetime in seconds of the access token.
 * `issue_new_refresh_token` - Keeps old refresh token or generate new refresh token when to obtain an access token.
-* `clean_expired_tokens` - Enable a background job to clean expired oauth tokens. Defaults to `false`. Interval settings sets in configuration periodic jobs [`Oban.Cron`](#obancron)
+* `clean_expired_tokens` - Enable a background job to clean expired oauth tokens. Defaults to `false`.
 
 ## Link parsing
 

lib/pleroma/config/oban.ex

@@ -5,7 +5,7 @@ defmodule Pleroma.Config.Oban do
     oban_config = Pleroma.Config.get(Oban)
 
     crontab =
-      [Pleroma.Workers.Cron.StatsWorker]
+      [Pleroma.Workers.Cron.StatsWorker, Pleroma.Workers.Cron.ClearOauthTokenWorker]
       |> Enum.reduce(oban_config[:crontab], fn removed_worker, acc ->
         with acc when is_list(acc) <- acc,
              setting when is_tuple(setting) <-

lib/pleroma/mfa/token.ex

@@ -10,10 +10,11 @@ defmodule Pleroma.MFA.Token do
   alias Pleroma.Repo
   alias Pleroma.User
   alias Pleroma.Web.OAuth.Authorization
-  alias Pleroma.Web.OAuth.Token, as: OAuthToken
 
   @expires 300
 
+  @type t() :: %__MODULE__{}
+
   schema "mfa_tokens" do
     field(:token, :string)
     field(:valid_until, :naive_datetime_usec)
@@ -24,6 +25,7 @@ defmodule Pleroma.MFA.Token do
     timestamps()
   end
 
+  @spec get_by_token(String.t()) :: {:ok, t()} | {:error, :not_found}
   def get_by_token(token) do
     from(
       t in __MODULE__,
@@ -33,33 +35,40 @@ defmodule Pleroma.MFA.Token do
     |> Repo.find_resource()
   end
 
-  def validate(token) do
+  @spec validate(String.t()) :: {:ok, t()} | {:error, :not_found} | {:error, :expired_token}
-    with {:fetch_token, {:ok, token}} <- {:fetch_token, get_by_token(token)},
+  def validate(token_str) do
-         {:expired, false} <- {:expired, is_expired?(token)} do
+    with {:ok, token} <- get_by_token(token_str),
+         false <- expired?(token) do
       {:ok, token}
-    else
-      {:expired, _} -> {:error, :expired_token}
-      {:fetch_token, _} -> {:error, :not_found}
-      error -> {:error, error}
     end
   end
 
-  def create_token(%User{} = user) do
+  defp expired?(%__MODULE__{valid_until: valid_until}) do
-    %__MODULE__{}
+    with true <- NaiveDateTime.diff(NaiveDateTime.utc_now(), valid_until) > 0 do
-    |> change
+      {:error, :expired_token}
-    |> assign_user(user)
+    end
-    |> put_token
-    |> put_valid_until
-    |> Repo.insert()
   end
 
-  def create_token(user, authorization) do
+  @spec create(User.t(), Authorization.t() | nil) :: {:ok, t()} | {:error, Ecto.Changeset.t()}
+  def create(user, authorization \\ nil) do
+    with {:ok, token} <- do_create(user, authorization) do
+      Pleroma.Workers.PurgeExpiredToken.enqueue(%{
+        token_id: token.id,
+        valid_until: DateTime.from_naive!(token.valid_until, "Etc/UTC"),
+        mod: __MODULE__
+      })
+
+      {:ok, token}
+    end
+  end
+
+  defp do_create(user, authorization) do
     %__MODULE__{}
-    |> change
+    |> change()
     |> assign_user(user)
-    |> assign_authorization(authorization)
+    |> maybe_assign_authorization(authorization)
-    |> put_token
+    |> put_token()
-    |> put_valid_until
+    |> put_valid_until()
     |> Repo.insert()
   end
 
@@ -69,15 +78,19 @@ defmodule Pleroma.MFA.Token do
     |> validate_required([:user])
   end
 
-  defp assign_authorization(changeset, authorization) do
+  defp maybe_assign_authorization(changeset, %Authorization{} = authorization) do
     changeset
     |> put_assoc(:authorization, authorization)
     |> validate_required([:authorization])
   end
 
+  defp maybe_assign_authorization(changeset, _), do: changeset
+
   defp put_token(changeset) do
+    token = Pleroma.Web.OAuth.Token.Utils.generate_token()
+
     changeset
-    |> change(%{token: OAuthToken.Utils.generate_token()})
+    |> change(%{token: token})
     |> validate_required([:token])
     |> unique_constraint(:token)
   end
@@ -89,18 +102,4 @@ defmodule Pleroma.MFA.Token do
     |> change(%{valid_until: expires_in})
     |> validate_required([:valid_until])
   end
-
-  def is_expired?(%__MODULE__{valid_until: valid_until}) do
-    NaiveDateTime.diff(NaiveDateTime.utc_now(), valid_until) > 0
-  end
-
-  def is_expired?(_), do: false
-
-  def delete_expired_tokens do
-    from(
-      q in __MODULE__,
-      where: fragment("?", q.valid_until) < ^Timex.now()
-    )
-    |> Repo.delete_all()
-  end
 end

lib/pleroma/web/oauth/oauth_controller.ex

@@ -200,7 +200,7 @@ defmodule Pleroma.Web.OAuth.OAuthController do
          {:mfa_required, user, auth, _},
          params
        ) do
-    {:ok, token} = MFA.Token.create_token(user, auth)
+    {:ok, token} = MFA.Token.create(user, auth)
 
     data = %{
       "mfa_token" => token.token,
@@ -582,7 +582,7 @@ defmodule Pleroma.Web.OAuth.OAuthController do
     do: put_session(conn, :registration_id, registration_id)
 
   defp build_and_response_mfa_token(user, auth) do
-    with {:ok, token} <- MFA.Token.create_token(user, auth) do
+    with {:ok, token} <- MFA.Token.create(user, auth) do
       MFAView.render("mfa_response.json", %{token: token, user: user})
     end
   end

lib/pleroma/web/oauth/token.ex

@@ -50,7 +50,7 @@ defmodule Pleroma.Web.OAuth.Token do
          true <- auth.app_id == app.id do
       user = if auth.user_id, do: User.get_cached_by_id(auth.user_id), else: %User{}
 
-      create_token(
+      create(
         app,
         user,
         %{scopes: auth.scopes}
@@ -83,8 +83,22 @@ defmodule Pleroma.Web.OAuth.Token do
     |> validate_required([:valid_until])
   end
 
-  @spec create_token(App.t(), User.t(), map()) :: {:ok, Token} | {:error, Changeset.t()}
+  @spec create(App.t(), User.t(), map()) :: {:ok, Token} | {:error, Changeset.t()}
-  def create_token(%App{} = app, %User{} = user, attrs \\ %{}) do
+  def create(%App{} = app, %User{} = user, attrs \\ %{}) do
+    with {:ok, token} <- do_create(app, user, attrs) do
+      if Pleroma.Config.get([:oauth2, :clean_expired_tokens]) do
+        Pleroma.Workers.PurgeExpiredToken.enqueue(%{
+          token_id: token.id,
+          valid_until: DateTime.from_naive!(token.valid_until, "Etc/UTC"),
+          mod: __MODULE__
+        })
+      end
+
+      {:ok, token}
+    end
+  end
+
+  defp do_create(app, user, attrs) do
     %__MODULE__{user_id: user.id, app_id: app.id}
     |> cast(%{scopes: attrs[:scopes] || app.scopes}, [:scopes])
     |> validate_required([:scopes, :app_id])
@@ -105,11 +119,6 @@ defmodule Pleroma.Web.OAuth.Token do
     |> Repo.delete_all()
   end
 
-  def delete_expired_tokens do
-    Query.get_expired_tokens()
-    |> Repo.delete_all()
-  end
-
   def get_user_tokens(%User{id: user_id}) do
     Query.get_by_user(user_id)
     |> Query.preload([:app])

lib/pleroma/web/oauth/token/clean_worker.ex

@@ -1,38 +0,0 @@
-# Pleroma: A lightweight social networking server
-# Copyright © 2017-2020 Pleroma Authors <https://pleroma.social/>
-# SPDX-License-Identifier: AGPL-3.0-only
-
-defmodule Pleroma.Web.OAuth.Token.CleanWorker do
-  @moduledoc """
-  The module represents functions to clean an expired OAuth and MFA tokens.
-  """
-  use GenServer
-
-  @ten_seconds 10_000
-  @one_day 86_400_000
-
-  alias Pleroma.MFA
-  alias Pleroma.Web.OAuth
-  alias Pleroma.Workers.BackgroundWorker
-
-  def start_link(_), do: GenServer.start_link(__MODULE__, %{})
-
-  def init(_) do
-    Process.send_after(self(), :perform, @ten_seconds)
-    {:ok, nil}
-  end
-
-  @doc false
-  def handle_info(:perform, state) do
-    BackgroundWorker.enqueue("clean_expired_tokens", %{})
-    interval = Pleroma.Config.get([:oauth2, :clean_expired_tokens_interval], @one_day)
-
-    Process.send_after(self(), :perform, interval)
-    {:noreply, state}
-  end
-
-  def perform(:clean) do
-    OAuth.Token.delete_expired_tokens()
-    MFA.Token.delete_expired_tokens()
-  end
-end

lib/pleroma/web/oauth/token/query.ex

@@ -33,12 +33,6 @@ defmodule Pleroma.Web.OAuth.Token.Query do
     from(q in query, where: q.id == ^id)
   end
 
-  @spec get_expired_tokens(query, DateTime.t() | nil) :: query
-  def get_expired_tokens(query \\ Token, date \\ nil) do
-    expired_date = date || Timex.now()
-    from(q in query, where: fragment("?", q.valid_until) < ^expired_date)
-  end
-
   @spec get_by_user(query, String.t()) :: query
   def get_by_user(query \\ Token, user_id) do
     from(q in query, where: q.user_id == ^user_id)

lib/pleroma/web/oauth/token/strategy/refresh_token.ex

@@ -46,7 +46,7 @@ defmodule Pleroma.Web.OAuth.Token.Strategy.RefreshToken do
   defp create_access_token({:error, error}, _), do: {:error, error}
 
   defp create_access_token({:ok, token}, %{app: app, user: user} = token_params) do
-    Token.create_token(app, user, add_refresh_token(token_params, token.refresh_token))
+    Token.create(app, user, add_refresh_token(token_params, token.refresh_token))
   end
 
   defp add_refresh_token(params, token) do

lib/pleroma/web/twitter_api/controllers/remote_follow_controller.ex

@@ -135,7 +135,7 @@ defmodule Pleroma.Web.TwitterAPI.RemoteFollowController do
   end
 
   defp handle_follow_error(conn, {:mfa_required, followee, user, _} = _) do
-    {:ok, %{token: token}} = MFA.Token.create_token(user)
+    {:ok, %{token: token}} = MFA.Token.create(user)
     render(conn, "follow_mfa.html", %{followee: followee, mfa_token: token, error: false})
   end
 

lib/pleroma/workers/cron/clear_oauth_token_worker.ex

@@ -1,23 +0,0 @@
-# Pleroma: A lightweight social networking server
-# Copyright © 2017-2020 Pleroma Authors <https://pleroma.social/>
-# SPDX-License-Identifier: AGPL-3.0-only
-
-defmodule Pleroma.Workers.Cron.ClearOauthTokenWorker do
-  @moduledoc """
-  The worker to cleanup expired oAuth tokens.
-  """
-
-  use Oban.Worker, queue: "background"
-
-  alias Pleroma.Config
-  alias Pleroma.Web.OAuth.Token
-
-  @impl Oban.Worker
-  def perform(_job) do
-    if Config.get([:oauth2, :clean_expired_tokens], false) do
-      Token.delete_expired_tokens()
-    end
-
-    :ok
-  end
-end

lib/pleroma/workers/purge_expired_token.ex

@@ -0,0 +1,29 @@
+# Pleroma: A lightweight social networking server
+# Copyright © 2017-2020 Pleroma Authors <https://pleroma.social/>
+# SPDX-License-Identifier: AGPL-3.0-only
+
+defmodule Pleroma.Workers.PurgeExpiredToken do
+  @moduledoc """
+  Worker which purges expired OAuth tokens
+  """
+
+  use Oban.Worker, queue: :token_expiration, max_attempts: 1
+
+  @spec enqueue(%{token_id: integer(), valid_until: DateTime.t(), mod: module()}) ::
+          {:ok, Oban.Job.t()} | {:error, Ecto.Changeset.t()}
+  def enqueue(args) do
+    {scheduled_at, args} = Map.pop(args, :valid_until)
+
+    args
+    |> __MODULE__.new(scheduled_at: scheduled_at)
+    |> Oban.insert()
+  end
+
+  @impl true
+  def perform(%Oban.Job{args: %{"token_id" => id, "mod" => module}}) do
+    module
+    |> String.to_existing_atom()
+    |> Pleroma.Repo.get(id)
+    |> Pleroma.Repo.delete()
+  end
+end

priv/repo/migrations/20200907084956_remove_cron_clear_oauth_token_worker_from_oban_config.exs

@@ -0,0 +1,19 @@
+defmodule Pleroma.Repo.Migrations.RemoveCronClearOauthTokenWorkerFromObanConfig do
+  use Ecto.Migration
+
+  def change do
+    with %Pleroma.ConfigDB{} = config <-
+           Pleroma.ConfigDB.get_by_params(%{group: :pleroma, key: Oban}),
+         crontab when is_list(crontab) <- config.value[:crontab],
+         index when is_integer(index) <-
+           Enum.find_index(crontab, fn {_, worker} ->
+             worker == Pleroma.Workers.Cron.ClearOauthTokenWorker
+           end) do
+      updated_value = Keyword.put(config.value, :crontab, List.delete_at(crontab, index))
+
+      config
+      |> Ecto.Changeset.change(value: updated_value)
+      |> Pleroma.Repo.update()
+    end
+  end
+end

priv/repo/migrations/20200907092050_move_tokens_expiration_into_oban.exs

@@ -0,0 +1,36 @@
+defmodule Pleroma.Repo.Migrations.MoveTokensExpirationIntoOban do
+  use Ecto.Migration
+
+  import Ecto.Query, only: [from: 2]
+
+  def change do
+    Supervisor.start_link([{Oban, Pleroma.Config.get(Oban)}],
+      strategy: :one_for_one,
+      name: Pleroma.Supervisor
+    )
+
+    if Pleroma.Config.get([:oauth2, :clean_expired_tokens]) do
+      from(t in Pleroma.Web.OAuth.Token, where: t.valid_until > ^NaiveDateTime.utc_now())
+      |> Pleroma.Repo.stream()
+      |> Stream.each(fn token ->
+        Pleroma.Workers.PurgeExpiredToken.enqueue(%{
+          token_id: token.id,
+          valid_until: DateTime.from_naive!(token.valid_until, "Etc/UTC"),
+          mod: Pleroma.Web.OAuth.Token
+        })
+      end)
+      |> Stream.run()
+    end
+
+    from(t in Pleroma.MFA.Token, where: t.valid_until > ^NaiveDateTime.utc_now())
+    |> Pleroma.Repo.stream()
+    |> Stream.each(fn token ->
+      Pleroma.Workers.PurgeExpiredToken.enqueue(%{
+        token_id: token.id,
+        valid_until: DateTime.from_naive!(token.valid_until, "Etc/UTC"),
+        mod: Pleroma.MFA.Token
+      })
+    end)
+    |> Stream.run()
+  end
+end

test/plugs/oauth_plug_test.exs

@@ -16,7 +16,7 @@ defmodule Pleroma.Plugs.OAuthPlugTest do
 
   setup %{conn: conn} do
     user = insert(:user)
-    {:ok, %{token: token}} = Pleroma.Web.OAuth.Token.create_token(insert(:oauth_app), user)
+    {:ok, %{token: token}} = Pleroma.Web.OAuth.Token.create(insert(:oauth_app), user)
     %{user: user, token: token, conn: conn}
   end
 

test/web/oauth/token_test.exs

@@ -69,17 +69,4 @@ defmodule Pleroma.Web.OAuth.TokenTest do
 
     assert tokens == 2
   end
-
-  test "deletes expired tokens" do
-    insert(:oauth_token, valid_until: Timex.shift(Timex.now(), days: -3))
-    insert(:oauth_token, valid_until: Timex.shift(Timex.now(), days: -3))
-    t3 = insert(:oauth_token)
-    t4 = insert(:oauth_token, valid_until: Timex.shift(Timex.now(), minutes: 10))
-    {tokens, _} = Token.delete_expired_tokens()
-    assert tokens == 2
-    available_tokens = Pleroma.Repo.all(Token)
-
-    token_ids = available_tokens |> Enum.map(& &1.id)
-    assert token_ids == [t3.id, t4.id]
-  end
 end

test/web/twitter_api/password_controller_test.exs

@@ -37,7 +37,7 @@ defmodule Pleroma.Web.TwitterAPI.PasswordControllerTest do
     test "it returns HTTP 200", %{conn: conn} do
       user = insert(:user)
       {:ok, token} = PasswordResetToken.create_token(user)
-      {:ok, _access_token} = Token.create_token(insert(:oauth_app), user, %{})
+      {:ok, _access_token} = Token.create(insert(:oauth_app), user, %{})
 
       params = %{
         "password" => "test",
@@ -62,7 +62,7 @@ defmodule Pleroma.Web.TwitterAPI.PasswordControllerTest do
       user = insert(:user, password_reset_pending: true)
 
       {:ok, token} = PasswordResetToken.create_token(user)
-      {:ok, _access_token} = Token.create_token(insert(:oauth_app), user, %{})
+      {:ok, _access_token} = Token.create(insert(:oauth_app), user, %{})
 
       params = %{
         "password" => "test",

test/web/twitter_api/remote_follow_controller_test.exs

@@ -227,7 +227,7 @@ defmodule Pleroma.Web.TwitterAPI.RemoteFollowControllerTest do
           }
         )
 
-      {:ok, %{token: token}} = MFA.Token.create_token(user)
+      {:ok, %{token: token}} = MFA.Token.create(user)
 
       user2 = insert(:user)
       otp_token = TOTP.generate_token(otp_secret)
@@ -256,7 +256,7 @@ defmodule Pleroma.Web.TwitterAPI.RemoteFollowControllerTest do
           }
         )
 
-      {:ok, %{token: token}} = MFA.Token.create_token(user)
+      {:ok, %{token: token}} = MFA.Token.create(user)
 
       user2 = insert(:user)
       otp_token = TOTP.generate_token(TOTP.generate_secret())

test/workers/cron/clear_oauth_token_worker_test.exs

@@ -1,22 +0,0 @@
-# Pleroma: A lightweight social networking server
-# Copyright © 2017-2020 Pleroma Authors <https://pleroma.social/>
-# SPDX-License-Identifier: AGPL-3.0-only
-
-defmodule Pleroma.Workers.Cron.ClearOauthTokenWorkerTest do
-  use Pleroma.DataCase
-
-  import Pleroma.Factory
-  alias Pleroma.Workers.Cron.ClearOauthTokenWorker
-
-  setup do: clear_config([:oauth2, :clean_expired_tokens])
-
-  test "deletes expired tokens" do
-    insert(:oauth_token,
-      valid_until: NaiveDateTime.add(NaiveDateTime.utc_now(), -60 * 10)
-    )
-
-    Pleroma.Config.put([:oauth2, :clean_expired_tokens], true)
-    ClearOauthTokenWorker.perform(%Oban.Job{})
-    assert Pleroma.Repo.all(Pleroma.Web.OAuth.Token) == []
-  end
-end

test/workers/purge_expired_token_test.exs

@@ -0,0 +1,51 @@
+# Pleroma: A lightweight social networking server
+# Copyright © 2017-2020 Pleroma Authors <https://pleroma.social/>
+# SPDX-License-Identifier: AGPL-3.0-only
+
+defmodule Pleroma.Workers.PurgeExpiredTokenTest do
+  use Pleroma.DataCase, async: true
+  use Oban.Testing, repo: Pleroma.Repo
+
+  import Pleroma.Factory
+
+  setup do: clear_config([:oauth2, :clean_expired_tokens], true)
+
+  test "purges expired oauth token" do
+    user = insert(:user)
+    app = insert(:oauth_app)
+
+    {:ok, %{id: id}} = Pleroma.Web.OAuth.Token.create(app, user)
+
+    assert_enqueued(
+      worker: Pleroma.Workers.PurgeExpiredToken,
+      args: %{token_id: id, mod: Pleroma.Web.OAuth.Token}
+    )
+
+    assert {:ok, %{id: ^id}} =
+             perform_job(Pleroma.Workers.PurgeExpiredToken, %{
+               token_id: id,
+               mod: Pleroma.Web.OAuth.Token
+             })
+
+    assert Repo.aggregate(Pleroma.Web.OAuth.Token, :count, :id) == 0
+  end
+
+  test "purges expired mfa token" do
+    authorization = insert(:oauth_authorization)
+
+    {:ok, %{id: id}} = Pleroma.MFA.Token.create(authorization.user, authorization)
+
+    assert_enqueued(
+      worker: Pleroma.Workers.PurgeExpiredToken,
+      args: %{token_id: id, mod: Pleroma.MFA.Token}
+    )
+
+    assert {:ok, %{id: ^id}} =
+             perform_job(Pleroma.Workers.PurgeExpiredToken, %{
+               token_id: id,
+               mod: Pleroma.MFA.Token
+             })
+
+    assert Repo.aggregate(Pleroma.MFA.Token, :count, :id) == 0
+  end
+end