Auth subsystem refactoring and tweaks.
authorIvan Tashkinov <ivantashkinov@gmail.com>
Sat, 31 Oct 2020 10:38:35 +0000 (13:38 +0300)
committerIvan Tashkinov <ivantashkinov@gmail.com>
Sat, 31 Oct 2020 10:38:35 +0000 (13:38 +0300)
Added proper OAuth skipping for SessionAuthenticationPlug. Integrated LegacyAuthenticationPlug into AuthenticationPlug. Adjusted tests & docs.

15 files changed:
docs/dev.md
lib/pleroma/helpers/auth_helper.ex [new file with mode: 0644]
lib/pleroma/web/plugs/admin_secret_authentication_plug.ex
lib/pleroma/web/plugs/authentication_plug.ex
lib/pleroma/web/plugs/basic_auth_decoder_plug.ex
lib/pleroma/web/plugs/ensure_user_key_plug.ex
lib/pleroma/web/plugs/legacy_authentication_plug.ex [deleted file]
lib/pleroma/web/plugs/session_authentication_plug.ex
lib/pleroma/web/plugs/set_user_session_id_plug.ex
lib/pleroma/web/plugs/user_fetcher_plug.ex
lib/pleroma/web/router.ex
test/pleroma/web/plugs/admin_secret_authentication_plug_test.exs
test/pleroma/web/plugs/authentication_plug_test.exs
test/pleroma/web/plugs/legacy_authentication_plug_test.exs [deleted file]
test/pleroma/web/plugs/session_authentication_plug_test.exs

index 22e0691f14e35e906bbb4a1a546874e4d0606b77..ba2718673168bdaf152cea9bb3781621c5d99ae7 100644 (file)
@@ -14,9 +14,9 @@ This document contains notes and guidelines for Pleroma developers.
 
   For `:api` pipeline routes, it'll be verified whether `OAuthScopesPlug` was called or explicitly skipped, and if it was not then auth information will be dropped for request. Then `EnsurePublicOrAuthenticatedPlug` will be called to ensure that either the instance is not private or user is authenticated (unless explicitly skipped). Such automated checks help to prevent human errors and result in higher security / privacy for users.
 
-## [HTTP Basic Authentication](https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Authorization)
+## Non-OAuth authentication
 
-* With HTTP Basic Auth, OAuth scopes check is _not_ performed for any action (since password is provided during the auth, requester is able to obtain a token with full permissions anyways). `Pleroma.Web.Plugs.AuthenticationPlug` and `Pleroma.Web.Plugs.LegacyAuthenticationPlug` both call `Pleroma.Web.Plugs.OAuthScopesPlug.skip_plug(conn)` when password is provided.
+* With non-OAuth authentication ([HTTP Basic Authentication](https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Authorization) or HTTP header- or params-provided auth), OAuth scopes check is _not_ performed for any action (since password is provided during the auth, requester is able to obtain a token with full permissions anyways); auth plugs invoke `Pleroma.Helpers.AuthHelper.skip_oauth(conn)` in this case.
 
 ## Auth-related configuration, OAuth consumer mode etc.
 
diff --git a/lib/pleroma/helpers/auth_helper.ex b/lib/pleroma/helpers/auth_helper.ex
new file mode 100644 (file)
index 0000000..6e29c00
--- /dev/null
@@ -0,0 +1,17 @@
+# Pleroma: A lightweight social networking server
+# Copyright © 2017-2020 Pleroma Authors <https://pleroma.social/>
+# SPDX-License-Identifier: AGPL-3.0-only
+
+defmodule Pleroma.Helpers.AuthHelper do
+  alias Pleroma.Web.Plugs.OAuthScopesPlug
+
+  @doc """
+  Skips OAuth permissions (scopes) checks, assigns nil `:token`.
+  Intended to be used with explicit authentication and only when OAuth token cannot be determined.
+  """
+  def skip_oauth(conn) do
+    conn
+    |> Plug.Conn.assign(:token, nil)
+    |> OAuthScopesPlug.skip_plug()
+  end
+end
index d7d4e4092b4ba51631262de2b8b168acb49f5d0e..ff49801f4cca8692160b90e15e835fe32a658e30 100644 (file)
@@ -5,8 +5,8 @@
 defmodule Pleroma.Web.Plugs.AdminSecretAuthenticationPlug do
   import Plug.Conn
 
+  alias Pleroma.Helpers.AuthHelper
   alias Pleroma.User
-  alias Pleroma.Web.Plugs.OAuthScopesPlug
   alias Pleroma.Web.Plugs.RateLimiter
 
   def init(options) do
@@ -51,7 +51,7 @@ defmodule Pleroma.Web.Plugs.AdminSecretAuthenticationPlug do
   defp assign_admin_user(conn) do
     conn
     |> assign(:user, %User{is_admin: true})
-    |> OAuthScopesPlug.skip_plug()
+    |> AuthHelper.skip_oauth()
   end
 
   defp handle_bad_token(conn) do
index e2a8b1b69366062b8571f861ed17db1e56fd403c..a7b8a9bfe4ee5546b9e1244397751b171f0a5f94 100644 (file)
@@ -3,6 +3,9 @@
 # SPDX-License-Identifier: AGPL-3.0-only
 
 defmodule Pleroma.Web.Plugs.AuthenticationPlug do
+  @moduledoc "Password authentication plug."
+
+  alias Pleroma.Helpers.AuthHelper
   alias Pleroma.User
 
   import Plug.Conn
@@ -11,6 +14,30 @@ defmodule Pleroma.Web.Plugs.AuthenticationPlug do
 
   def init(options), do: options
 
+  def call(%{assigns: %{user: %User{}}} = conn, _), do: conn
+
+  def call(
+        %{
+          assigns: %{
+            auth_user: %{password_hash: password_hash} = auth_user,
+            auth_credentials: %{password: password}
+          }
+        } = conn,
+        _
+      ) do
+    if checkpw(password, password_hash) do
+      {:ok, auth_user} = maybe_update_password(auth_user, password)
+
+      conn
+      |> assign(:user, auth_user)
+      |> AuthHelper.skip_oauth()
+    else
+      conn
+    end
+  end
+
+  def call(conn, _), do: conn
+
   def checkpw(password, "$6" <> _ = password_hash) do
     :crypt.crypt(password, password_hash) == password_hash
   end
@@ -40,40 +67,6 @@ defmodule Pleroma.Web.Plugs.AuthenticationPlug do
   def maybe_update_password(user, _), do: {:ok, user}
 
   defp do_update_password(user, password) do
-    user
-    |> User.password_update_changeset(%{
-      "password" => password,
-      "password_confirmation" => password
-    })
-    |> Pleroma.Repo.update()
+    User.reset_password(user, %{password: password, password_confirmation: password})
   end
-
-  def call(%{assigns: %{user: %User{}}} = conn, _), do: conn
-
-  def call(
-        %{
-          assigns: %{
-            auth_user: %{password_hash: password_hash} = auth_user,
-            auth_credentials: %{password: password}
-          }
-        } = conn,
-        _
-      ) do
-    if checkpw(password, password_hash) do
-      {:ok, auth_user} = maybe_update_password(auth_user, password)
-
-      conn
-      |> assign(:user, auth_user)
-      |> Pleroma.Web.Plugs.OAuthScopesPlug.skip_plug()
-    else
-      conn
-    end
-  end
-
-  def call(%{assigns: %{auth_credentials: %{password: _}}} = conn, _) do
-    Pbkdf2.no_user_verify()
-    conn
-  end
-
-  def call(conn, _), do: conn
 end
index 4dadfb0004526355f704f60ee7c4962b94225f3e..97529aedbae6f0d919f91e0333b0a57cf09b46a3 100644 (file)
@@ -3,6 +3,12 @@
 # SPDX-License-Identifier: AGPL-3.0-only
 
 defmodule Pleroma.Web.Plugs.BasicAuthDecoderPlug do
+  @moduledoc """
+  Decodes HTTP Basic Auth information and assigns `:auth_credentials`.
+
+  NOTE: no checks are performed at this step, auth_credentials/username could be easily faked.
+  """
+
   import Plug.Conn
 
   def init(options) do
index 70d3091f0268623735b1675d5a498e4108961ea9..31608dbbf2e0b6259b7f7cb6f7b5a43b6d6fd8ba 100644 (file)
@@ -5,6 +5,8 @@
 defmodule Pleroma.Web.Plugs.EnsureUserKeyPlug do
   import Plug.Conn
 
+  @moduledoc "Ensures `conn.assigns.user` is initialized."
+
   def init(opts) do
     opts
   end
@@ -12,7 +14,6 @@ defmodule Pleroma.Web.Plugs.EnsureUserKeyPlug do
   def call(%{assigns: %{user: _}} = conn, _), do: conn
 
   def call(conn, _) do
-    conn
-    |> assign(:user, nil)
+    assign(conn, :user, nil)
   end
 end
diff --git a/lib/pleroma/web/plugs/legacy_authentication_plug.ex b/lib/pleroma/web/plugs/legacy_authentication_plug.ex
deleted file mode 100644 (file)
index 2a54d0b..0000000
+++ /dev/null
@@ -1,41 +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.Plugs.LegacyAuthenticationPlug do
-  import Plug.Conn
-
-  alias Pleroma.User
-
-  def init(options) do
-    options
-  end
-
-  def call(%{assigns: %{user: %User{}}} = conn, _), do: conn
-
-  def call(
-        %{
-          assigns: %{
-            auth_user: %{password_hash: "$6$" <> _ = password_hash} = auth_user,
-            auth_credentials: %{password: password}
-          }
-        } = conn,
-        _
-      ) do
-    with ^password_hash <- :crypt.crypt(password, password_hash),
-         {:ok, user} <-
-           User.reset_password(auth_user, %{password: password, password_confirmation: password}) do
-      conn
-      |> assign(:auth_user, user)
-      |> assign(:user, user)
-      |> Pleroma.Web.Plugs.OAuthScopesPlug.skip_plug()
-    else
-      _ ->
-        conn
-    end
-  end
-
-  def call(conn, _) do
-    conn
-  end
-end
index 6e176d5537d12bcac64c5ad69c63ec9ad1c1cb64..51704e2734544bd49f34c439f63890b3a2cb6104 100644 (file)
@@ -3,17 +3,27 @@
 # SPDX-License-Identifier: AGPL-3.0-only
 
 defmodule Pleroma.Web.Plugs.SessionAuthenticationPlug do
+  @moduledoc """
+  Authenticates user by session-stored `:user_id` and request-contained username.
+  Username can be provided via HTTP Basic Auth (the password is not checked and can be anything).
+  """
+
   import Plug.Conn
 
+  alias Pleroma.Helpers.AuthHelper
+
   def init(options) do
     options
   end
 
+  def call(%{assigns: %{user: %Pleroma.User{}}} = conn, _), do: conn
+
   def call(conn, _) do
     with saved_user_id <- get_session(conn, :user_id),
          %{auth_user: %{id: ^saved_user_id}} <- conn.assigns do
       conn
       |> assign(:user, conn.assigns.auth_user)
+      |> AuthHelper.skip_oauth()
     else
       _ -> conn
     end
index e520159e4aaa5211e254e47c9e610a18bc80e307..6ddb6b5e58fe9db12445da3260ed4cf2d3b467e2 100644 (file)
@@ -11,8 +11,7 @@ defmodule Pleroma.Web.Plugs.SetUserSessionIdPlug do
   end
 
   def call(%{assigns: %{user: %User{id: id}}} = conn, _) do
-    conn
-    |> put_session(:user_id, id)
+    put_session(conn, :user_id, id)
   end
 
   def call(conn, _), do: conn
index 4039600daff9ecae0d135f2ff798ef62441956ea..89e16b49f0b278a92b57483ee248354ee49c94d1 100644 (file)
@@ -3,6 +3,12 @@
 # SPDX-License-Identifier: AGPL-3.0-only
 
 defmodule Pleroma.Web.Plugs.UserFetcherPlug do
+  @moduledoc """
+  Assigns `:auth_user` basing on `:auth_credentials`.
+
+  NOTE: no checks are performed at this step, auth_credentials/username could be easily faked.
+  """
+
   alias Pleroma.User
   import Plug.Conn
 
index 76ca2c9b51ab3a9be87b109dbba5a2e9a6fb5af2..9da10f1e5ff130b61f7b0e1efa10dead0912b9fc 100644 (file)
@@ -49,7 +49,6 @@ defmodule Pleroma.Web.Router do
     plug(Pleroma.Web.Plugs.BasicAuthDecoderPlug)
     plug(Pleroma.Web.Plugs.UserFetcherPlug)
     plug(Pleroma.Web.Plugs.SessionAuthenticationPlug)
-    plug(Pleroma.Web.Plugs.LegacyAuthenticationPlug)
     plug(Pleroma.Web.Plugs.AuthenticationPlug)
   end
 
index 33394722a5e39da6d63581834067391042ab2ab8..23498badfe59e9c73dd4ef276ab53e026afe20ed 100644 (file)
@@ -49,6 +49,7 @@ defmodule Pleroma.Web.Plugs.AdminSecretAuthenticationPlugTest do
         |> AdminSecretAuthenticationPlug.call(%{})
 
       assert conn.assigns[:user].is_admin
+      assert conn.assigns[:token] == nil
       assert PlugHelper.plug_skipped?(conn, OAuthScopesPlug)
     end
 
@@ -69,6 +70,7 @@ defmodule Pleroma.Web.Plugs.AdminSecretAuthenticationPlugTest do
         |> AdminSecretAuthenticationPlug.call(%{})
 
       assert conn.assigns[:user].is_admin
+      assert conn.assigns[:token] == nil
       assert PlugHelper.plug_skipped?(conn, OAuthScopesPlug)
     end
   end
index af39352e261a39bcf5a349e678352e49d5d5cebd..3dedd38b279cb162e763a54d471d1b9946b7fc1f 100644 (file)
@@ -48,6 +48,7 @@ defmodule Pleroma.Web.Plugs.AuthenticationPlugTest do
       |> AuthenticationPlug.call(%{})
 
     assert conn.assigns.user == conn.assigns.auth_user
+    assert conn.assigns.token == nil
     assert PlugHelper.plug_skipped?(conn, OAuthScopesPlug)
   end
 
@@ -62,6 +63,7 @@ defmodule Pleroma.Web.Plugs.AuthenticationPlugTest do
       |> AuthenticationPlug.call(%{})
 
     assert conn.assigns.user.id == conn.assigns.auth_user.id
+    assert conn.assigns.token == nil
     assert PlugHelper.plug_skipped?(conn, OAuthScopesPlug)
 
     user = User.get_by_id(user.id)
@@ -83,6 +85,7 @@ defmodule Pleroma.Web.Plugs.AuthenticationPlugTest do
       |> AuthenticationPlug.call(%{})
 
     assert conn.assigns.user.id == conn.assigns.auth_user.id
+    assert conn.assigns.token == nil
     assert PlugHelper.plug_skipped?(conn, OAuthScopesPlug)
 
     user = User.get_by_id(user.id)
diff --git a/test/pleroma/web/plugs/legacy_authentication_plug_test.exs b/test/pleroma/web/plugs/legacy_authentication_plug_test.exs
deleted file mode 100644 (file)
index 2016a31..0000000
+++ /dev/null
@@ -1,82 +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.Plugs.LegacyAuthenticationPlugTest do
-  use Pleroma.Web.ConnCase
-
-  import Pleroma.Factory
-
-  alias Pleroma.User
-  alias Pleroma.Web.Plugs.LegacyAuthenticationPlug
-  alias Pleroma.Web.Plugs.OAuthScopesPlug
-  alias Pleroma.Web.Plugs.PlugHelper
-
-  setup do
-    user =
-      insert(:user,
-        password: "password",
-        password_hash:
-          "$6$9psBWV8gxkGOZWBz$PmfCycChoxeJ3GgGzwvhlgacb9mUoZ.KUXNCssekER4SJ7bOK53uXrHNb2e4i8yPFgSKyzaW9CcmrDXWIEMtD1"
-      )
-
-    %{user: user}
-  end
-
-  test "it does nothing if a user is assigned", %{conn: conn, user: user} do
-    conn =
-      conn
-      |> assign(:auth_credentials, %{username: "dude", password: "password"})
-      |> assign(:auth_user, user)
-      |> assign(:user, %User{})
-
-    ret_conn =
-      conn
-      |> LegacyAuthenticationPlug.call(%{})
-
-    assert ret_conn == conn
-  end
-
-  @tag :skip_on_mac
-  test "if `auth_user` is present and password is correct, " <>
-         "it authenticates the user, resets the password, marks OAuthScopesPlug as skipped",
-       %{
-         conn: conn,
-         user: user
-       } do
-    conn =
-      conn
-      |> assign(:auth_credentials, %{username: "dude", password: "password"})
-      |> assign(:auth_user, user)
-
-    conn = LegacyAuthenticationPlug.call(conn, %{})
-
-    assert conn.assigns.user.id == user.id
-    assert PlugHelper.plug_skipped?(conn, OAuthScopesPlug)
-  end
-
-  @tag :skip_on_mac
-  test "it does nothing if the password is wrong", %{
-    conn: conn,
-    user: user
-  } do
-    conn =
-      conn
-      |> assign(:auth_credentials, %{username: "dude", password: "wrong_password"})
-      |> assign(:auth_user, user)
-
-    ret_conn =
-      conn
-      |> LegacyAuthenticationPlug.call(%{})
-
-    assert conn == ret_conn
-  end
-
-  test "with no credentials or user it does nothing", %{conn: conn} do
-    ret_conn =
-      conn
-      |> LegacyAuthenticationPlug.call(%{})
-
-    assert ret_conn == conn
-  end
-end
index 2b4d5bc0cfacc9f1a93f17169cc0b181e872e43d..d027331a9f9012d20943ff7a110ceb1825826b67 100644 (file)
@@ -6,6 +6,8 @@ defmodule Pleroma.Web.Plugs.SessionAuthenticationPlugTest do
   use Pleroma.Web.ConnCase, async: true
 
   alias Pleroma.User
+  alias Pleroma.Web.Plugs.OAuthScopesPlug
+  alias Pleroma.Web.Plugs.PlugHelper
   alias Pleroma.Web.Plugs.SessionAuthenticationPlug
 
   setup %{conn: conn} do
@@ -18,24 +20,20 @@ defmodule Pleroma.Web.Plugs.SessionAuthenticationPlugTest do
     conn =
       conn
       |> Plug.Session.call(Plug.Session.init(session_opts))
-      |> fetch_session
+      |> fetch_session()
       |> assign(:auth_user, %User{id: 1})
 
     %{conn: conn}
   end
 
   test "it does nothing if a user is assigned", %{conn: conn} do
-    conn =
-      conn
-      |> assign(:user, %User{})
-
-    ret_conn =
-      conn
-      |> SessionAuthenticationPlug.call(%{})
+    conn = assign(conn, :user, %User{})
+    ret_conn = SessionAuthenticationPlug.call(conn, %{})
 
     assert ret_conn == conn
   end
 
+  # Scenario: requester has the cookie and knows the username (not necessarily knows the password)
   test "if the auth_user has the same id as the user_id in the session, it assigns the user", %{
     conn: conn
   } do
@@ -45,19 +43,23 @@ defmodule Pleroma.Web.Plugs.SessionAuthenticationPlugTest do
       |> SessionAuthenticationPlug.call(%{})
 
     assert conn.assigns.user == conn.assigns.auth_user
+    assert conn.assigns.token == nil
+    assert PlugHelper.plug_skipped?(conn, OAuthScopesPlug)
   end
 
+  # Scenario: requester has the cookie but doesn't know the username
   test "if the auth_user has a different id as the user_id in the session, it does nothing", %{
     conn: conn
   } do
-    conn =
-      conn
-      |> put_session(:user_id, -1)
-
-    ret_conn =
-      conn
-      |> SessionAuthenticationPlug.call(%{})
+    conn = put_session(conn, :user_id, -1)
+    ret_conn = SessionAuthenticationPlug.call(conn, %{})
 
     assert ret_conn == conn
   end
+
+  test "if the session does not contain user_id, it does nothing", %{
+    conn: conn
+  } do
+    assert conn == SessionAuthenticationPlug.call(conn, %{})
+  end
 end