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

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 ->
+        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
 end

lib/pleroma/user.ex

@@ -1736,7 +1736,8 @@ def admin_api_update(user, params) 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
 

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 @@ test "accepts a user that is admin" 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 @@ test "denies when a user isn't set" 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;
+    test "if token has any of admin scopes, accepts a user that is an admin", %{conn: conn} do
-    #   however, the following test stresses out that is_admin flag is not checked if we got token
+      user = insert(:user, is_admin: true)
-    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"])
 
       conn =
-          build_conn()
+        conn
         |> assign(:user, user)
         |> assign(:token, token)
-          |> UserIsAdminPlug.call(%{})
 
       ret_conn = UserIsAdminPlug.call(conn, %{})
 
       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",

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 @@ test "GET /api/pleroma/admin/users/:nickname requires admin:read:accounts or bro
           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