From 40e1817f707c3c2ef253009c7363cd81b11322a6 Mon Sep 17 00:00:00 2001 From: Ivan Tashkinov Date: Fri, 6 Dec 2019 20:33:47 +0300 Subject: [PATCH] [#1427] Fixes / improvements of admin scopes support. Added tests. --- lib/pleroma/plugs/oauth_scopes_plug.ex | 9 ++ lib/pleroma/plugs/user_is_admin_plug.ex | 42 +++---- .../web/admin_api/admin_api_controller.ex | 16 +-- .../controllers/emoji_api_controller.ex | 2 +- test/plugs/user_is_admin_plug_test.exs | 104 ++++++++++++++---- .../admin_api/admin_api_controller_test.exs | 47 +++++++- 6 files changed, 162 insertions(+), 58 deletions(-) diff --git a/lib/pleroma/plugs/oauth_scopes_plug.ex b/lib/pleroma/plugs/oauth_scopes_plug.ex index a3278dbef..3201fb399 100644 --- a/lib/pleroma/plugs/oauth_scopes_plug.ex +++ b/lib/pleroma/plugs/oauth_scopes_plug.ex @@ -6,6 +6,7 @@ defmodule Pleroma.Plugs.OAuthScopesPlug do import Plug.Conn import Pleroma.Web.Gettext + alias Pleroma.Config alias Pleroma.Plugs.EnsurePublicOrAuthenticatedPlug @behaviour Plug @@ -15,6 +16,14 @@ defmodule Pleroma.Plugs.OAuthScopesPlug do def call(%Plug.Conn{assigns: assigns} = conn, %{scopes: scopes} = options) do op = options[:op] || :| token = assigns[:token] + + scopes = + if options[:admin] do + Config.oauth_admin_scopes(scopes) + else + scopes + end + matched_scopes = token && filter_descendants(scopes, token.scopes) cond do diff --git a/lib/pleroma/plugs/user_is_admin_plug.ex b/lib/pleroma/plugs/user_is_admin_plug.ex index 814029dce..4a0e43b00 100644 --- a/lib/pleroma/plugs/user_is_admin_plug.ex +++ b/lib/pleroma/plugs/user_is_admin_plug.ex @@ -12,31 +12,23 @@ defmodule Pleroma.Plugs.UserIsAdminPlug do options end - unless Pleroma.Config.enforce_oauth_admin_scope_usage?() do - # To do: once AdminFE makes use of "admin" scope, disable the following func definition - # (fail on no admin scope(s) in token even if `is_admin` is true) - def call(%Plug.Conn{assigns: %{user: %Pleroma.User{is_admin: true}}} = conn, _) do - conn + def call(%Plug.Conn{assigns: assigns} = conn, _) do + token = assigns[:token] + user = assigns[:user] + + cond do + 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() end end - - def call(%Plug.Conn{assigns: %{token: %OAuth.Token{scopes: oauth_scopes} = _token}} = conn, _) do - if OAuth.Scopes.contains_admin_scopes?(oauth_scopes) do - # Note: checking for _any_ admin scope presence, not necessarily fitting requested action. - # Thus, controller must explicitly invoke OAuthScopesPlug to verify scope requirements. - conn - else - 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/web/admin_api/admin_api_controller.ex b/lib/pleroma/web/admin_api/admin_api_controller.ex index 34d147107..0a63f3fe6 100644 --- a/lib/pleroma/web/admin_api/admin_api_controller.ex +++ b/lib/pleroma/web/admin_api/admin_api_controller.ex @@ -30,13 +30,13 @@ defmodule Pleroma.Web.AdminAPI.AdminAPIController do plug( OAuthScopesPlug, - %{scopes: Pleroma.Config.oauth_admin_scopes("read:accounts")} + %{scopes: ["read:accounts"], admin: true} when action in [:list_users, :user_show, :right_get, :invites] ) plug( OAuthScopesPlug, - %{scopes: Pleroma.Config.oauth_admin_scopes("write:accounts")} + %{scopes: ["write:accounts"], admin: true} when action in [ :get_invite_token, :revoke_invite, @@ -58,37 +58,37 @@ defmodule Pleroma.Web.AdminAPI.AdminAPIController do plug( OAuthScopesPlug, - %{scopes: Pleroma.Config.oauth_admin_scopes("read:reports")} + %{scopes: ["read:reports"], admin: true} when action in [:list_reports, :report_show] ) plug( OAuthScopesPlug, - %{scopes: Pleroma.Config.oauth_admin_scopes("write:reports")} + %{scopes: ["write:reports"], admin: true} when action in [:report_update_state, :report_respond] ) plug( OAuthScopesPlug, - %{scopes: Pleroma.Config.oauth_admin_scopes("read:statuses")} + %{scopes: ["read:statuses"], admin: true} when action == :list_user_statuses ) plug( OAuthScopesPlug, - %{scopes: Pleroma.Config.oauth_admin_scopes("write:statuses")} + %{scopes: ["write:statuses"], admin: true} when action in [:status_update, :status_delete] ) plug( OAuthScopesPlug, - %{scopes: Pleroma.Config.oauth_admin_scopes("read")} + %{scopes: ["read"], admin: true} when action in [:config_show, :migrate_to_db, :migrate_from_db, :list_log] ) plug( OAuthScopesPlug, - %{scopes: Pleroma.Config.oauth_admin_scopes("write")} + %{scopes: ["write"], admin: true} when action in [:relay_follow, :relay_unfollow, :config_update] ) diff --git a/lib/pleroma/web/pleroma_api/controllers/emoji_api_controller.ex b/lib/pleroma/web/pleroma_api/controllers/emoji_api_controller.ex index 6f286032e..69dfa92e3 100644 --- a/lib/pleroma/web/pleroma_api/controllers/emoji_api_controller.ex +++ b/lib/pleroma/web/pleroma_api/controllers/emoji_api_controller.ex @@ -7,7 +7,7 @@ defmodule Pleroma.Web.PleromaAPI.EmojiAPIController do plug( OAuthScopesPlug, - %{scopes: Pleroma.Config.oauth_admin_scopes("write")} + %{scopes: ["write"], admin: true} when action in [ :create, :delete, diff --git a/test/plugs/user_is_admin_plug_test.exs b/test/plugs/user_is_admin_plug_test.exs index 136dcc54e..154c9b195 100644 --- a/test/plugs/user_is_admin_plug_test.exs +++ b/test/plugs/user_is_admin_plug_test.exs @@ -8,36 +8,96 @@ defmodule Pleroma.Plugs.UserIsAdminPlugTest do alias Pleroma.Plugs.UserIsAdminPlug import Pleroma.Factory - test "accepts a user that is admin" do - user = insert(:user, is_admin: true) + describe "unless [:auth, :enforce_oauth_admin_scope_usage]," do + clear_config([:auth, :enforce_oauth_admin_scope_usage]) do + Pleroma.Config.put([:auth, :enforce_oauth_admin_scope_usage], false) + end - conn = - build_conn() - |> assign(:user, user) + test "accepts a user that is admin" do + user = insert(:user, is_admin: true) - ret_conn = - conn - |> UserIsAdminPlug.call(%{}) + conn = assign(build_conn(), :user, user) - assert conn == ret_conn + ret_conn = UserIsAdminPlug.call(conn, %{}) + + assert conn == ret_conn + end + + test "denies a user that isn't admin" do + user = insert(:user) + + conn = + build_conn() + |> assign(:user, user) + |> UserIsAdminPlug.call(%{}) + + assert conn.status == 403 + end + + test "denies when a user isn't set" do + conn = UserIsAdminPlug.call(build_conn(), %{}) + + assert conn.status == 403 + end end - test "denies a user that isn't admin" do - user = insert(:user) + describe "with [:auth, :enforce_oauth_admin_scope_usage]," do + clear_config([:auth, :enforce_oauth_admin_scope_usage]) do + Pleroma.Config.put([:auth, :enforce_oauth_admin_scope_usage], true) + end - conn = - build_conn() - |> assign(:user, user) - |> UserIsAdminPlug.call(%{}) + setup do + admin_user = insert(:user, is_admin: true) + non_admin_user = insert(:user, is_admin: false) + blank_user = nil - assert conn.status == 403 - end + {:ok, %{users: [admin_user, non_admin_user, blank_user]}} + end - test "denies when a user isn't set" do - conn = - build_conn() - |> UserIsAdminPlug.call(%{}) + # 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"]) - assert conn.status == 403 + conn = + build_conn() + |> assign(:user, user) + |> assign(:token, token) + |> UserIsAdminPlug.call(%{}) + + ret_conn = UserIsAdminPlug.call(conn, %{}) + + assert conn == ret_conn + end + end + + test "if token lacks admin scopes, denies users regardless of is_admin flag", + %{users: users} do + for user <- users do + token = insert(:oauth_token, user: user) + + conn = + build_conn() + |> assign(:user, user) + |> assign(:token, token) + |> UserIsAdminPlug.call(%{}) + + assert conn.status == 403 + end + end + + test "if token is missing, denies users regardless of is_admin flag", %{users: users} do + for user <- users do + conn = + build_conn() + |> assign(:user, user) + |> assign(:token, nil) + |> UserIsAdminPlug.call(%{}) + + assert conn.status == 403 + end + end end end diff --git a/test/web/admin_api/admin_api_controller_test.exs b/test/web/admin_api/admin_api_controller_test.exs index d0131fd90..2fc23ad6c 100644 --- a/test/web/admin_api/admin_api_controller_test.exs +++ b/test/web/admin_api/admin_api_controller_test.exs @@ -24,6 +24,49 @@ defmodule Pleroma.Web.AdminAPI.AdminAPIControllerTest do :ok end + clear_config([:auth, :enforce_oauth_admin_scope_usage]) do + Pleroma.Config.put([:auth, :enforce_oauth_admin_scope_usage], false) + end + + describe "with [:auth, :enforce_oauth_admin_scope_usage]," do + clear_config([:auth, :enforce_oauth_admin_scope_usage]) do + Pleroma.Config.put([:auth, :enforce_oauth_admin_scope_usage], true) + end + + test "GET /api/pleroma/admin/users/:nickname requires admin:read:accounts or broader scope" do + user = insert(:user) + admin = insert(:user, is_admin: true) + + good_token1 = insert(:oauth_token, user: admin, scopes: ["admin"]) + good_token2 = insert(:oauth_token, user: admin, scopes: ["admin:read"]) + good_token3 = insert(:oauth_token, user: admin, scopes: ["admin:read:accounts"]) + + bad_token1 = insert(:oauth_token, user: admin, scopes: ["read:accounts"]) + bad_token2 = insert(:oauth_token, user: admin, scopes: ["admin:read:accounts:partial"]) + bad_token3 = nil + + for good_token <- [good_token1, good_token2, good_token3] do + conn = + build_conn() + |> assign(:user, admin) + |> assign(:token, good_token) + |> get("/api/pleroma/admin/users/#{user.nickname}") + + assert json_response(conn, 200) + 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}") + + assert json_response(conn, :forbidden) + end + end + end + describe "DELETE /api/pleroma/admin/users" do test "single user" do admin = insert(:user, is_admin: true) @@ -97,7 +140,7 @@ defmodule Pleroma.Web.AdminAPI.AdminAPIControllerTest do assert ["lain", "lain2"] -- Enum.map(log_entry.data["subjects"], & &1["nickname"]) == [] end - test "Cannot create user with exisiting email" do + test "Cannot create user with existing email" do admin = insert(:user, is_admin: true) user = insert(:user) @@ -128,7 +171,7 @@ defmodule Pleroma.Web.AdminAPI.AdminAPIControllerTest do ] end - test "Cannot create user with exisiting nickname" do + test "Cannot create user with existing nickname" do admin = insert(:user, is_admin: true) user = insert(:user)