[#1427] Extra check that admin OAuth scope is used by admin. Adjusted tests.

diff --git a/lib/pleroma/plugs/user_is_admin_plug.ex b/lib/pleroma/plugs/user_is_admin_plug.ex
index 4a0e43b003e805e182bc462e342bdce18434a18c..582fb1f9275ca6babdf930f0abc3a0225206cc15 100644 (file)
--- a/lib/pleroma/plugs/user_is_admin_plug.ex
+++ b/lib/pleroma/plugs/user_is_admin_plug.ex
@@ -6,29 +6,37 @@ defmodule Pleroma.Plugs.UserIsAdminPlug do
   import Pleroma.Web.TranslationHelpers
   import Plug.Conn
 
+  alias Pleroma.User
   alias Pleroma.Web.OAuth
 
   def init(options) do
     options
   end
 
-  def call(%Plug.Conn{assigns: assigns} = conn, _) do
+  def call(%{assigns: %{user: %User{is_admin: true}} = assigns} = conn, _) do
     token = assigns[:token]
-    user = assigns[:user]
 
     cond do
+      not Pleroma.Config.enforce_oauth_admin_scope_usage?() ->
+        conn
+
       token && OAuth.Scopes.contains_admin_scopes?(token.scopes) ->
         # Note: checking for _any_ admin scope presence, not necessarily fitting requested action.
         #   Thus, controller must explicitly invoke OAuthScopesPlug to verify scope requirements.
         conn
 
-      user && user.is_admin && !Pleroma.Config.enforce_oauth_admin_scope_usage?() ->
-        conn
-
       true ->
-        conn
-        |> render_error(:forbidden, "User is not an admin or OAuth admin scope is not granted.")
-        |> halt()
+        fail(conn)
     end
   end
+
+  def call(conn, _) do
+    fail(conn)
+  end
+
+  defp fail(conn) do
+    conn
+    |> render_error(:forbidden, "User is not an admin or OAuth admin scope is not granted.")
+    |> halt()
+  end
 end

diff --git a/lib/pleroma/user.ex b/lib/pleroma/user.ex
index 7b8222ce170db28c093617640c9da6226ba2363f..1006b5bf9bb41e4211a0a479f2c5ee37eefef195 100644 (file)
--- a/lib/pleroma/user.ex
+++ b/lib/pleroma/user.ex
@@ -1736,7 +1736,8 @@ defmodule Pleroma.User do
 
     with {:ok, updated_user} <- update_and_set_cache(changeset) do
       if user.is_admin && !updated_user.is_admin do
-        # Tokens & authorizations containing any admin scopes must be revoked (revoking all)
+        # Tokens & authorizations containing any admin scopes must be revoked (revoking all).
+        # This is an extra safety measure (tokens' admin scopes won't be accepted for non-admins).
         global_sign_out(user)
       end
 

diff --git a/test/plugs/user_is_admin_plug_test.exs b/test/plugs/user_is_admin_plug_test.exs
index 154c9b195ed1798fe057c59633d737d61353bc3d..bc6fcd73caa2809cd03883d045bc01fa99f8dc02 100644 (file)
--- a/test/plugs/user_is_admin_plug_test.exs
+++ b/test/plugs/user_is_admin_plug_test.exs
@@ -13,7 +13,7 @@ defmodule Pleroma.Plugs.UserIsAdminPlugTest do
       Pleroma.Config.put([:auth, :enforce_oauth_admin_scope_usage], false)
     end
 
-    test "accepts a user that is admin" do
+    test "accepts a user that is an admin" do
       user = insert(:user, is_admin: true)
 
       conn = assign(build_conn(), :user, user)
@@ -23,7 +23,7 @@ defmodule Pleroma.Plugs.UserIsAdminPlugTest do
       assert conn == ret_conn
     end
 
-    test "denies a user that isn't admin" do
+    test "denies a user that isn't an admin" do
       user = insert(:user)
 
       conn =
@@ -54,23 +54,43 @@ defmodule Pleroma.Plugs.UserIsAdminPlugTest do
       {:ok, %{users: [admin_user, non_admin_user, blank_user]}}
     end
 
-    # Note: in real-life scenarios only users with is_admin flag can possess admin-scoped tokens;
-    #   however, the following test stresses out that is_admin flag is not checked if we got token
-    test "if token has any of admin scopes, accepts users regardless of is_admin flag",
-         %{users: users} do
-      for user <- users do
-        token = insert(:oauth_token, user: user, scopes: ["admin:something"])
+    test "if token has any of admin scopes, accepts a user that is an admin", %{conn: conn} do
+      user = insert(:user, is_admin: true)
+      token = insert(:oauth_token, user: user, scopes: ["admin:something"])
 
-        conn =
-          build_conn()
-          |> assign(:user, user)
-          |> assign(:token, token)
-          |> UserIsAdminPlug.call(%{})
+      conn =
+        conn
+        |> assign(:user, user)
+        |> assign(:token, token)
 
-        ret_conn = UserIsAdminPlug.call(conn, %{})
+      ret_conn = UserIsAdminPlug.call(conn, %{})
 
-        assert conn == ret_conn
-      end
+      assert conn == ret_conn
+    end
+
+    test "if token has any of admin scopes, denies a user that isn't an admin", %{conn: conn} do
+      user = insert(:user, is_admin: false)
+      token = insert(:oauth_token, user: user, scopes: ["admin:something"])
+
+      conn =
+        conn
+        |> assign(:user, user)
+        |> assign(:token, token)
+        |> UserIsAdminPlug.call(%{})
+
+      assert conn.status == 403
+    end
+
+    test "if token has any of admin scopes, denies when a user isn't set", %{conn: conn} do
+      token = insert(:oauth_token, scopes: ["admin:something"])
+
+      conn =
+        conn
+        |> assign(:user, nil)
+        |> assign(:token, token)
+        |> UserIsAdminPlug.call(%{})
+
+      assert conn.status == 403
     end
 
     test "if token lacks admin scopes, denies users regardless of is_admin flag",

diff --git a/test/web/admin_api/admin_api_controller_test.exs b/test/web/admin_api/admin_api_controller_test.exs
index 2fc23ad6c567cb4806cc0d2d5fd893538921b128..bcab63cf05193b34f015b1d15344cc8dd83be34f 100644 (file)
--- a/test/web/admin_api/admin_api_controller_test.exs
+++ b/test/web/admin_api/admin_api_controller_test.exs
@@ -36,6 +36,7 @@ defmodule Pleroma.Web.AdminAPI.AdminAPIControllerTest do
     test "GET /api/pleroma/admin/users/:nickname requires admin:read:accounts or broader scope" do
       user = insert(:user)
       admin = insert(:user, is_admin: true)
+      url = "/api/pleroma/admin/users/#{user.nickname}"
 
       good_token1 = insert(:oauth_token, user: admin, scopes: ["admin"])
       good_token2 = insert(:oauth_token, user: admin, scopes: ["admin:read"])
@@ -50,17 +51,27 @@ defmodule Pleroma.Web.AdminAPI.AdminAPIControllerTest do
           build_conn()
           |> assign(:user, admin)
           |> assign(:token, good_token)
-          |> get("/api/pleroma/admin/users/#{user.nickname}")
+          |> get(url)
 
         assert json_response(conn, 200)
       end
 
+      for good_token <- [good_token1, good_token2, good_token3] do
+        conn =
+          build_conn()
+          |> assign(:user, nil)
+          |> assign(:token, good_token)
+          |> get(url)
+
+        assert json_response(conn, :forbidden)
+      end
+
       for bad_token <- [bad_token1, bad_token2, bad_token3] do
         conn =
           build_conn()
           |> assign(:user, admin)
           |> assign(:token, bad_token)
-          |> get("/api/pleroma/admin/users/#{user.nickname}")
+          |> get(url)
 
         assert json_response(conn, :forbidden)
       end