From 76068873dbf9da191dd2487158ca88df198b811a Mon Sep 17 00:00:00 2001 From: Ivan Tashkinov Date: Tue, 17 Sep 2019 22:19:39 +0300 Subject: [PATCH] [#1234] Defined admin OAuth scopes, refined other scopes. Added tests. --- .../web/admin_api/admin_api_controller.ex | 71 +++--- .../controllers/mastodon_api_controller.ex | 23 +- test/plugs/oauth_scopes_plug_test.exs | 227 ++++++++++++------ 3 files changed, 210 insertions(+), 111 deletions(-) diff --git a/lib/pleroma/web/admin_api/admin_api_controller.ex b/lib/pleroma/web/admin_api/admin_api_controller.ex index 0a508d40e..fa69a23d9 100644 --- a/lib/pleroma/web/admin_api/admin_api_controller.ex +++ b/lib/pleroma/web/admin_api/admin_api_controller.ex @@ -24,38 +24,20 @@ defmodule Pleroma.Web.AdminAPI.AdminAPIController do require Logger - plug(OAuthScopesPlug, %{scopes: ["read:statuses"]} when action == :list_user_statuses) - plug( OAuthScopesPlug, - %{scopes: ["write:statuses"]} when action in [:status_update, :status_delete] + %{scopes: ["admin:read:accounts", "read:accounts"]} + when action in [:list_users, :user_show, :right_get, :invites] ) plug( OAuthScopesPlug, - %{scopes: ["read"]} + %{scopes: ["admin:write", "write:accounts"]} when action in [ - :list_reports, - :report_show, - :right_get, :get_invite_token, - :invites, + :revoke_invite, + :email_invite, :get_password_reset, - :list_users, - :user_show, - :config_show, - :migrate_to_db, - :migrate_from_db, - :list_log - ] - ) - - plug( - OAuthScopesPlug, - %{scopes: ["write"]} - when action in [ - :report_update_state, - :report_respond, :user_follow, :user_unfollow, :user_delete, @@ -65,15 +47,44 @@ defmodule Pleroma.Web.AdminAPI.AdminAPIController do :untag_users, :right_add, :right_delete, - :set_activation_status, - :relay_follow, - :relay_unfollow, - :revoke_invite, - :email_invite, - :config_update + :set_activation_status ] ) + plug( + OAuthScopesPlug, + %{scopes: ["admin:read:reports", "read:reports"]} when action in [:list_reports, :report_show] + ) + + plug( + OAuthScopesPlug, + %{scopes: ["admin:write:reports", "write:reports"]} + when action in [:report_update_state, :report_respond] + ) + + plug( + OAuthScopesPlug, + %{scopes: ["admin:read:statuses", "read:statuses"]} when action == :list_user_statuses + ) + + plug( + OAuthScopesPlug, + %{scopes: ["admin:write:statuses", "write:statuses"]} + when action in [:status_update, :status_delete] + ) + + plug( + OAuthScopesPlug, + %{scopes: ["admin:read", "read"]} + when action in [:config_show, :migrate_to_db, :migrate_from_db, :list_log] + ) + + plug( + OAuthScopesPlug, + %{scopes: ["admin:write", "write"]} + when action in [:relay_follow, :relay_unfollow, :config_update] + ) + @users_page_size 50 action_fallback(:errors) @@ -451,7 +462,7 @@ def email_invite(%{assigns: %{user: user}} = conn, %{"email" => email} = params) end end - @doc "Get a account registeration invite token (base64 string)" + @doc "Get a account registration invite token (base64 string)" def get_invite_token(conn, params) do options = params["invite"] || %{} {:ok, invite} = UserInviteToken.create_invite(options) diff --git a/lib/pleroma/web/mastodon_api/controllers/mastodon_api_controller.ex b/lib/pleroma/web/mastodon_api/controllers/mastodon_api_controller.ex index c5632bb5e..d7a83a2f5 100644 --- a/lib/pleroma/web/mastodon_api/controllers/mastodon_api_controller.ex +++ b/lib/pleroma/web/mastodon_api/controllers/mastodon_api_controller.ex @@ -53,13 +53,13 @@ defmodule Pleroma.Web.MastodonAPI.MastodonAPIController do require Logger require Pleroma.Constants - plug(Pleroma.Plugs.EnsurePublicOrAuthenticatedPlug when action != :index) - @unauthenticated_access %{fallback: :proceed_unauthenticated, scopes: []} + # Note: :index action handles attempt of unauthenticated access to private instance with redirect plug( OAuthScopesPlug, - %{scopes: ["read"], skip_instance_privacy_check: true} when action == :index + Map.merge(@unauthenticated_access, %{scopes: ["read"], skip_instance_privacy_check: true}) + when action == :index ) plug( @@ -220,6 +220,23 @@ defmodule Pleroma.Web.MastodonAPI.MastodonAPIController do %{scopes: ["write:bookmarks"]} when action in [:bookmark_status, :unbookmark_status] ) + # An extra safety measure for possible actions not guarded by OAuth permissions specification + plug( + Pleroma.Plugs.EnsurePublicOrAuthenticatedPlug + when action not in [ + :account_register, + :create_app, + :index, + :login, + :logout, + :password_reset, + :account_confirmation_resend, + :masto_instance, + :peers, + :custom_emojis + ] + ) + @rate_limited_relations_actions ~w(follow unfollow)a @rate_limited_status_actions ~w(reblog_status unreblog_status fav_status unfav_status diff --git a/test/plugs/oauth_scopes_plug_test.exs b/test/plugs/oauth_scopes_plug_test.exs index 9b0a2e702..3b895a6e4 100644 --- a/test/plugs/oauth_scopes_plug_test.exs +++ b/test/plugs/oauth_scopes_plug_test.exs @@ -6,23 +6,47 @@ defmodule Pleroma.Plugs.OAuthScopesPlugTest do use Pleroma.Web.ConnCase, async: true alias Pleroma.Plugs.OAuthScopesPlug + alias Pleroma.Plugs.EnsurePublicOrAuthenticatedPlug alias Pleroma.Repo + import Mock import Pleroma.Factory - test "proceeds with no op if `assigns[:token]` is nil", %{conn: conn} do - conn = - conn - |> assign(:user, insert(:user)) - |> OAuthScopesPlug.call(%{scopes: ["read"]}) - - refute conn.halted - assert conn.assigns[:user] + setup_with_mocks([{EnsurePublicOrAuthenticatedPlug, [], [call: fn conn, _ -> conn end]}]) do + :ok end - test "proceeds with no op if `token.scopes` fulfill specified 'any of' conditions", %{ - conn: conn - } do + describe "when `assigns[:token]` is nil, " do + test "with :skip_instance_privacy_check option, proceeds with no op", %{conn: conn} do + conn = + conn + |> assign(:user, insert(:user)) + |> OAuthScopesPlug.call(%{scopes: ["read"], skip_instance_privacy_check: true}) + + refute conn.halted + assert conn.assigns[:user] + + refute called(EnsurePublicOrAuthenticatedPlug.call(conn, :_)) + end + + test "without :skip_instance_privacy_check option, calls EnsurePublicOrAuthenticatedPlug", %{ + conn: conn + } do + conn = + conn + |> assign(:user, insert(:user)) + |> OAuthScopesPlug.call(%{scopes: ["read"]}) + + refute conn.halted + assert conn.assigns[:user] + + assert called(EnsurePublicOrAuthenticatedPlug.call(conn, :_)) + end + end + + test "if `token.scopes` fulfills specified 'any of' conditions, " <> + "proceeds with no op", + %{conn: conn} do token = insert(:oauth_token, scopes: ["read", "write"]) |> Repo.preload(:user) conn = @@ -35,9 +59,9 @@ test "proceeds with no op if `token.scopes` fulfill specified 'any of' condition assert conn.assigns[:user] end - test "proceeds with no op if `token.scopes` fulfill specified 'all of' conditions", %{ - conn: conn - } do + test "if `token.scopes` fulfills specified 'all of' conditions, " <> + "proceeds with no op", + %{conn: conn} do token = insert(:oauth_token, scopes: ["scope1", "scope2", "scope3"]) |> Repo.preload(:user) conn = @@ -50,82 +74,112 @@ test "proceeds with no op if `token.scopes` fulfill specified 'all of' condition assert conn.assigns[:user] end - test "proceeds with cleared `assigns[:user]` if `token.scopes` doesn't fulfill specified 'any of' conditions " <> - "and `fallback: :proceed_unauthenticated` option is specified", - %{conn: conn} do - token = insert(:oauth_token, scopes: ["read", "write"]) |> Repo.preload(:user) + describe "with `fallback: :proceed_unauthenticated` option, " do + test "if `token.scopes` doesn't fulfill specified 'any of' conditions, " <> + "clears `assigns[:user]` and calls EnsurePublicOrAuthenticatedPlug", + %{conn: conn} do + token = insert(:oauth_token, scopes: ["read", "write"]) |> Repo.preload(:user) - conn = - conn - |> assign(:user, token.user) - |> assign(:token, token) - |> OAuthScopesPlug.call(%{scopes: ["follow"], fallback: :proceed_unauthenticated}) + conn = + conn + |> assign(:user, token.user) + |> assign(:token, token) + |> OAuthScopesPlug.call(%{scopes: ["follow"], fallback: :proceed_unauthenticated}) - refute conn.halted - refute conn.assigns[:user] + refute conn.halted + refute conn.assigns[:user] + + assert called(EnsurePublicOrAuthenticatedPlug.call(conn, :_)) + end + + test "if `token.scopes` doesn't fulfill specified 'all of' conditions, " <> + "clears `assigns[:user] and calls EnsurePublicOrAuthenticatedPlug", + %{conn: conn} do + token = insert(:oauth_token, scopes: ["read", "write"]) |> Repo.preload(:user) + + conn = + conn + |> assign(:user, token.user) + |> assign(:token, token) + |> OAuthScopesPlug.call(%{ + scopes: ["read", "follow"], + op: :&, + fallback: :proceed_unauthenticated + }) + + refute conn.halted + refute conn.assigns[:user] + + assert called(EnsurePublicOrAuthenticatedPlug.call(conn, :_)) + end + + test "with :skip_instance_privacy_check option, " <> + "if `token.scopes` doesn't fulfill specified conditions, " <> + "clears `assigns[:user]` and does not call EnsurePublicOrAuthenticatedPlug", + %{conn: conn} do + token = insert(:oauth_token, scopes: ["read:statuses", "write"]) |> Repo.preload(:user) + + conn = + conn + |> assign(:user, token.user) + |> assign(:token, token) + |> OAuthScopesPlug.call(%{ + scopes: ["read"], + fallback: :proceed_unauthenticated, + skip_instance_privacy_check: true + }) + + refute conn.halted + refute conn.assigns[:user] + + refute called(EnsurePublicOrAuthenticatedPlug.call(conn, :_)) + end end - test "proceeds with cleared `assigns[:user]` if `token.scopes` doesn't fulfill specified 'all of' conditions " <> - "and `fallback: :proceed_unauthenticated` option is specified", - %{conn: conn} do - token = insert(:oauth_token, scopes: ["read", "write"]) |> Repo.preload(:user) + describe "without :fallback option, " do + test "if `token.scopes` does not fulfill specified 'any of' conditions, " <> + "returns 403 and halts", + %{conn: conn} do + token = insert(:oauth_token, scopes: ["read", "write"]) + any_of_scopes = ["follow"] - conn = - conn - |> assign(:user, token.user) - |> assign(:token, token) - |> OAuthScopesPlug.call(%{ - scopes: ["read", "follow"], - op: :&, - fallback: :proceed_unauthenticated - }) + conn = + conn + |> assign(:token, token) + |> OAuthScopesPlug.call(%{scopes: any_of_scopes}) - refute conn.halted - refute conn.assigns[:user] - end + assert conn.halted + assert 403 == conn.status - test "returns 403 and halts " <> - "in case of no :fallback option and `token.scopes` not fulfilling specified 'any of' conditions", - %{conn: conn} do - token = insert(:oauth_token, scopes: ["read", "write"]) - any_of_scopes = ["follow"] + expected_error = "Insufficient permissions: #{Enum.join(any_of_scopes, ", ")}." + assert Jason.encode!(%{error: expected_error}) == conn.resp_body + end - conn = - conn - |> assign(:token, token) - |> OAuthScopesPlug.call(%{scopes: any_of_scopes}) + test "if `token.scopes` does not fulfill specified 'all of' conditions, " <> + "returns 403 and halts", + %{conn: conn} do + token = insert(:oauth_token, scopes: ["read", "write"]) + all_of_scopes = ["write", "follow"] - assert conn.halted - assert 403 == conn.status + conn = + conn + |> assign(:token, token) + |> OAuthScopesPlug.call(%{scopes: all_of_scopes, op: :&}) - expected_error = "Insufficient permissions: #{Enum.join(any_of_scopes, ", ")}." - assert Jason.encode!(%{error: expected_error}) == conn.resp_body - end + assert conn.halted + assert 403 == conn.status - test "returns 403 and halts " <> - "in case of no :fallback option and `token.scopes` not fulfilling specified 'all of' conditions", - %{conn: conn} do - token = insert(:oauth_token, scopes: ["read", "write"]) - all_of_scopes = ["write", "follow"] + expected_error = + "Insufficient permissions: #{Enum.join(all_of_scopes -- token.scopes, ", ")}." - conn = - conn - |> assign(:token, token) - |> OAuthScopesPlug.call(%{scopes: all_of_scopes, op: :&}) - - assert conn.halted - assert 403 == conn.status - - expected_error = - "Insufficient permissions: #{Enum.join(all_of_scopes -- token.scopes, ", ")}." - - assert Jason.encode!(%{error: expected_error}) == conn.resp_body + assert Jason.encode!(%{error: expected_error}) == conn.resp_body + end end describe "with hierarchical scopes, " do - test "proceeds with no op if `token.scopes` fulfill specified 'any of' conditions", %{ - conn: conn - } do + test "if `token.scopes` fulfills specified 'any of' conditions, " <> + "proceeds with no op", + %{conn: conn} do token = insert(:oauth_token, scopes: ["read", "write"]) |> Repo.preload(:user) conn = @@ -138,9 +192,9 @@ test "proceeds with no op if `token.scopes` fulfill specified 'any of' condition assert conn.assigns[:user] end - test "proceeds with no op if `token.scopes` fulfill specified 'all of' conditions", %{ - conn: conn - } do + test "if `token.scopes` fulfills specified 'all of' conditions, " <> + "proceeds with no op", + %{conn: conn} do token = insert(:oauth_token, scopes: ["scope1", "scope2", "scope3"]) |> Repo.preload(:user) conn = @@ -153,4 +207,21 @@ test "proceeds with no op if `token.scopes` fulfill specified 'all of' condition assert conn.assigns[:user] end end + + describe "filter_descendants/2" do + test "filters scopes which directly match or are ancestors of supported scopes" do + f = fn scopes, supported_scopes -> + OAuthScopesPlug.filter_descendants(scopes, supported_scopes) + end + + assert f.(["read", "follow"], ["write", "read"]) == ["read"] + + assert f.(["read", "write:something", "follow"], ["write", "read"]) == + ["read", "write:something"] + + assert f.(["admin:read"], ["write", "read"]) == [] + + assert f.(["admin:read"], ["write", "admin"]) == ["admin:read"] + end + end end