From 51111e286b316340b45b4e6a378646bed0cb0a6a Mon Sep 17 00:00:00 2001 From: Ivan Tashkinov Date: Fri, 29 Nov 2019 18:57:19 +0300 Subject: [PATCH 1/5] [#1427] Initial support for OAuth admin scopes. --- .../web/admin_api/admin_api_controller.ex | 16 ++++----- lib/pleroma/web/oauth/oauth_controller.ex | 10 +++--- lib/pleroma/web/oauth/scopes.ex | 34 ++++++++++++++++--- .../controllers/emoji_api_controller.ex | 2 +- 4 files changed, 43 insertions(+), 19 deletions(-) diff --git a/lib/pleroma/web/admin_api/admin_api_controller.ex b/lib/pleroma/web/admin_api/admin_api_controller.ex index 8c1318d1b..01cd12c96 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: ["read:accounts"]} + %{scopes: ["admin:read:accounts", "read:accounts"]} when action in [:list_users, :user_show, :right_get, :invites] ) plug( OAuthScopesPlug, - %{scopes: ["write:accounts"]} + %{scopes: ["admin:write:accounts", "write:accounts"]} when action in [ :get_invite_token, :revoke_invite, @@ -58,35 +58,35 @@ defmodule Pleroma.Web.AdminAPI.AdminAPIController do plug( OAuthScopesPlug, - %{scopes: ["read:reports"]} when action in [:list_reports, :report_show] + %{scopes: ["admin:read:reports", "read:reports"]} when action in [:list_reports, :report_show] ) plug( OAuthScopesPlug, - %{scopes: ["write:reports"]} + %{scopes: ["admin:reports", "write:reports"]} when action in [:report_update_state, :report_respond] ) plug( OAuthScopesPlug, - %{scopes: ["read:statuses"]} when action == :list_user_statuses + %{scopes: ["admin:read:statuses", "read:statuses"]} when action == :list_user_statuses ) plug( OAuthScopesPlug, - %{scopes: ["write:statuses"]} + %{scopes: ["admin:write:statuses", "write:statuses"]} when action in [:status_update, :status_delete] ) plug( OAuthScopesPlug, - %{scopes: ["read"]} + %{scopes: ["admin:read", "read"]} when action in [:config_show, :migrate_to_db, :migrate_from_db, :list_log] ) plug( OAuthScopesPlug, - %{scopes: ["write"]} + %{scopes: ["admin:write", "write"]} when action in [:relay_follow, :relay_unfollow, :config_update] ) diff --git a/lib/pleroma/web/oauth/oauth_controller.ex b/lib/pleroma/web/oauth/oauth_controller.ex index 2aee8cab2..87acdec97 100644 --- a/lib/pleroma/web/oauth/oauth_controller.ex +++ b/lib/pleroma/web/oauth/oauth_controller.ex @@ -222,7 +222,7 @@ def token_exchange( {:user_active, true} <- {:user_active, !user.deactivated}, {:password_reset_pending, false} <- {:password_reset_pending, user.password_reset_pending}, - {:ok, scopes} <- validate_scopes(app, params), + {:ok, scopes} <- validate_scopes(app, params, user), {:ok, auth} <- Authorization.create_authorization(app, user, scopes), {:ok, token} <- Token.exchange_token(app, auth) do json(conn, Token.Response.build(user, token)) @@ -471,7 +471,7 @@ defp do_create_authorization( {:get_user, (user && {:ok, user}) || Authenticator.get_user(conn)}, %App{} = app <- Repo.get_by(App, client_id: client_id), true <- redirect_uri in String.split(app.redirect_uris), - {:ok, scopes} <- validate_scopes(app, auth_attrs), + {:ok, scopes} <- validate_scopes(app, auth_attrs, user), {:auth_active, true} <- {:auth_active, User.auth_active?(user)} do Authorization.create_authorization(app, user, scopes) end @@ -487,12 +487,12 @@ defp get_session_registration_id(%Plug.Conn{} = conn), do: get_session(conn, :re defp put_session_registration_id(%Plug.Conn{} = conn, registration_id), do: put_session(conn, :registration_id, registration_id) - @spec validate_scopes(App.t(), map()) :: + @spec validate_scopes(App.t(), map(), User.t()) :: {:ok, list()} | {:error, :missing_scopes | :unsupported_scopes} - defp validate_scopes(app, params) do + defp validate_scopes(%App{} = app, params, %User{} = user) do params |> Scopes.fetch_scopes(app.scopes) - |> Scopes.validate(app.scopes) + |> Scopes.validate(app.scopes, user) end def default_redirect_uri(%App{} = app) do diff --git a/lib/pleroma/web/oauth/scopes.ex b/lib/pleroma/web/oauth/scopes.ex index 48bd14407..0c8796ecb 100644 --- a/lib/pleroma/web/oauth/scopes.ex +++ b/lib/pleroma/web/oauth/scopes.ex @@ -7,6 +7,9 @@ defmodule Pleroma.Web.OAuth.Scopes do Functions for dealing with scopes. """ + alias Pleroma.Plugs.OAuthScopesPlug + alias Pleroma.User + @doc """ Fetch scopes from request params. @@ -53,15 +56,36 @@ def to_string(scopes), do: Enum.join(scopes, " ") @doc """ Validates scopes. """ - @spec validate(list() | nil, list()) :: + @spec validate(list() | nil, list(), User.t()) :: {:ok, list()} | {:error, :missing_scopes | :unsupported_scopes} - def validate([], _app_scopes), do: {:error, :missing_scopes} - def validate(nil, _app_scopes), do: {:error, :missing_scopes} + def validate(blank_scopes, _app_scopes, _user) when blank_scopes in [nil, []], + do: {:error, :missing_scopes} - def validate(scopes, app_scopes) do - case Pleroma.Plugs.OAuthScopesPlug.filter_descendants(scopes, app_scopes) do + def validate(scopes, app_scopes, %User{} = user) do + with {:ok, _} <- ensure_scopes_support(scopes, app_scopes), + {:ok, scopes} <- authorize_admin_scopes(scopes, app_scopes, user) do + {:ok, scopes} + end + end + + defp ensure_scopes_support(scopes, app_scopes) do + case OAuthScopesPlug.filter_descendants(scopes, app_scopes) do ^scopes -> {:ok, scopes} _ -> {:error, :unsupported_scopes} end end + + defp authorize_admin_scopes(scopes, app_scopes, %User{} = user) do + if user.is_admin || !contains_admin_scopes?(scopes) || !contains_admin_scopes?(app_scopes) do + {:ok, scopes} + else + {:error, :unsupported_scopes} + end + end + + defp contains_admin_scopes?(scopes) do + scopes + |> OAuthScopesPlug.filter_descendants(["admin"]) + |> Enum.any?() + end end 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 a474d41d4..1e8c62e3a 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: ["write"]} + %{scopes: ["admin:write", "write"]} when action in [ :create, :delete, From af42c00cfffb2cd8e93857cd1cf2901113c45bd2 Mon Sep 17 00:00:00 2001 From: Ivan Tashkinov Date: Fri, 6 Dec 2019 00:25:44 +0300 Subject: [PATCH 2/5] [#1427] Reworked admin scopes support. Requalified users.is_admin flag as legacy accessor to admin actions in case token lacks admin scope(s). --- CHANGELOG.md | 1 + config/config.exs | 5 +++- config/description.exs | 9 ++++++ lib/mix/tasks/pleroma/user.ex | 9 ++---- lib/pleroma/config.ex | 7 +++++ lib/pleroma/plugs/user_is_admin_plug.ex | 28 ++++++++++++++++--- lib/pleroma/user.ex | 28 ++++++++++++++----- .../web/admin_api/admin_api_controller.ex | 18 ++++++------ lib/pleroma/web/oauth/scopes.ex | 2 +- .../controllers/emoji_api_controller.ex | 2 +- .../admin_api/admin_api_controller_test.exs | 3 +- 11 files changed, 82 insertions(+), 30 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6bd835a3d..dac61c174 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -44,6 +44,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/). - Mix task to re-count statuses for all users (`mix pleroma.count_statuses`) - Support for `X-Forwarded-For` and similar HTTP headers which used by reverse proxies to pass a real user IP address to the backend. Must not be enabled unless your instance is behind at least one reverse proxy (such as Nginx, Apache HTTPD or Varnish Cache). - MRF: New module which handles incoming posts based on their age. By default, all incoming posts that are older than 2 days will be unlisted and not shown to their followers. +- OAuth: admin scopes support (relevant setting: `[:auth, :enforce_oauth_admin_scope_usage]`).
API Changes diff --git a/config/config.exs b/config/config.exs index bf2b3f6e2..64397484e 100644 --- a/config/config.exs +++ b/config/config.exs @@ -560,7 +560,10 @@ base_path: "/oauth", providers: ueberauth_providers -config :pleroma, :auth, oauth_consumer_strategies: oauth_consumer_strategies +config :pleroma, + :auth, + enforce_oauth_admin_scope_usage: false, + oauth_consumer_strategies: oauth_consumer_strategies config :pleroma, Pleroma.Emails.Mailer, adapter: Swoosh.Adapters.Sendmail, enabled: false diff --git a/config/description.exs b/config/description.exs index 70e963399..45e4b43f1 100644 --- a/config/description.exs +++ b/config/description.exs @@ -2094,6 +2094,15 @@ type: :group, description: "Authentication / authorization settings", children: [ + %{ + key: :enforce_oauth_admin_scope_usage, + type: :boolean, + description: + "OAuth admin scope requirement toggle. " <> + "If `true`, admin actions explicitly demand admin OAuth scope(s) presence in OAuth token " <> + "(client app must support admin scopes). If `false` and token doesn't have admin scope(s)," <> + "`is_admin` user flag grants access to admin-specific actions." + }, %{ key: :auth_template, type: :string, diff --git a/lib/mix/tasks/pleroma/user.ex b/lib/mix/tasks/pleroma/user.ex index 4e3b80db3..8c4739221 100644 --- a/lib/mix/tasks/pleroma/user.ex +++ b/lib/mix/tasks/pleroma/user.ex @@ -8,7 +8,6 @@ defmodule Mix.Tasks.Pleroma.User do alias Ecto.Changeset alias Pleroma.User alias Pleroma.UserInviteToken - alias Pleroma.Web.OAuth @shortdoc "Manages Pleroma users" @moduledoc File.read!("docs/administration/CLI_tasks/user.md") @@ -354,8 +353,7 @@ def run(["sign_out", nickname]) do start_pleroma() with %User{local: true} = user <- User.get_cached_by_nickname(nickname) do - OAuth.Token.delete_user_tokens(user) - OAuth.Authorization.delete_user_authorizations(user) + User.global_sign_out(user) shell_info("#{nickname} signed out from all apps.") else @@ -375,10 +373,7 @@ defp set_moderator(user, value) do end defp set_admin(user, value) do - {:ok, user} = - user - |> Changeset.change(%{is_admin: value}) - |> User.update_and_set_cache() + {:ok, user} = User.admin_api_update(user, %{is_admin: value}) shell_info("Admin status of #{user.nickname}: #{user.is_admin}") user diff --git a/lib/pleroma/config.ex b/lib/pleroma/config.ex index fcc039710..cadab2f15 100644 --- a/lib/pleroma/config.ex +++ b/lib/pleroma/config.ex @@ -65,4 +65,11 @@ def delete(key) do def oauth_consumer_strategies, do: get([:auth, :oauth_consumer_strategies], []) def oauth_consumer_enabled?, do: oauth_consumer_strategies() != [] + + def enforce_oauth_admin_scope_usage?, do: !!get([:auth, :enforce_oauth_admin_scope_usage]) + + def oauth_admin_scopes(scope) do + ["admin:#{scope}"] ++ + if enforce_oauth_admin_scope_usage?(), do: [], else: [scope] + end end diff --git a/lib/pleroma/plugs/user_is_admin_plug.ex b/lib/pleroma/plugs/user_is_admin_plug.ex index ee808f31f..8814556f1 100644 --- a/lib/pleroma/plugs/user_is_admin_plug.ex +++ b/lib/pleroma/plugs/user_is_admin_plug.ex @@ -5,19 +5,39 @@ 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(%{assigns: %{user: %User{is_admin: true}}} = conn, _) do - conn + 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 + + 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: %User{is_admin: true}}} = conn, _) do + conn + end end def call(conn, _) do + fail(conn) + end + + defp fail(conn) do conn - |> render_error(:forbidden, "User is not admin.") - |> halt + |> 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 fcb1d5143..d05dccd3d 100644 --- a/lib/pleroma/user.ex +++ b/lib/pleroma/user.ex @@ -1732,13 +1732,27 @@ defp truncate_field(%{"name" => name, "value" => value}) do end def admin_api_update(user, params) do - user - |> cast(params, [ - :is_moderator, - :is_admin, - :show_role - ]) - |> update_and_set_cache() + changeset = + cast(user, params, [ + :is_moderator, + :is_admin, + :show_role + ]) + + 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) + global_sign_out(user) + end + + {:ok, updated_user} + end + end + + @doc "Signs user out of all applications" + def global_sign_out(user) do + OAuth.Authorization.delete_user_authorizations(user) + OAuth.Token.delete_user_tokens(user) end def mascot_update(user, url) do diff --git a/lib/pleroma/web/admin_api/admin_api_controller.ex b/lib/pleroma/web/admin_api/admin_api_controller.ex index 01cd12c96..f9ace00d7 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: ["admin:read:accounts", "read:accounts"]} + %{scopes: Pleroma.Config.oauth_admin_scopes("read:accounts")} when action in [:list_users, :user_show, :right_get, :invites] ) plug( OAuthScopesPlug, - %{scopes: ["admin:write:accounts", "write:accounts"]} + %{scopes: Pleroma.Config.oauth_admin_scopes("write:accounts")} when action in [ :get_invite_token, :revoke_invite, @@ -58,35 +58,37 @@ defmodule Pleroma.Web.AdminAPI.AdminAPIController do plug( OAuthScopesPlug, - %{scopes: ["admin:read:reports", "read:reports"]} when action in [:list_reports, :report_show] + %{scopes: Pleroma.Config.oauth_admin_scopes("read:reports")} + when action in [:list_reports, :report_show] ) plug( OAuthScopesPlug, - %{scopes: ["admin:reports", "write:reports"]} + %{scopes: Pleroma.Config.oauth_admin_scopes("write:reports")} when action in [:report_update_state, :report_respond] ) plug( OAuthScopesPlug, - %{scopes: ["admin:read:statuses", "read:statuses"]} when action == :list_user_statuses + %{scopes: Pleroma.Config.oauth_admin_scopes("read:statuses")} + when action == :list_user_statuses ) plug( OAuthScopesPlug, - %{scopes: ["admin:write:statuses", "write:statuses"]} + %{scopes: Pleroma.Config.oauth_admin_scopes("write:statuses")} when action in [:status_update, :status_delete] ) plug( OAuthScopesPlug, - %{scopes: ["admin:read", "read"]} + %{scopes: Pleroma.Config.oauth_admin_scopes("read")} when action in [:config_show, :migrate_to_db, :migrate_from_db, :list_log] ) plug( OAuthScopesPlug, - %{scopes: ["admin:write", "write"]} + %{scopes: Pleroma.Config.oauth_admin_scopes("write")} when action in [:relay_follow, :relay_unfollow, :config_update] ) diff --git a/lib/pleroma/web/oauth/scopes.ex b/lib/pleroma/web/oauth/scopes.ex index 0c8796ecb..5e04652c2 100644 --- a/lib/pleroma/web/oauth/scopes.ex +++ b/lib/pleroma/web/oauth/scopes.ex @@ -83,7 +83,7 @@ defp authorize_admin_scopes(scopes, app_scopes, %User{} = user) do end end - defp contains_admin_scopes?(scopes) do + def contains_admin_scopes?(scopes) do scopes |> OAuthScopesPlug.filter_descendants(["admin"]) |> Enum.any?() 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 1e8c62e3a..6f286032e 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: ["admin:write", "write"]} + %{scopes: Pleroma.Config.oauth_admin_scopes("write")} when action in [ :create, :delete, diff --git a/test/web/admin_api/admin_api_controller_test.exs b/test/web/admin_api/admin_api_controller_test.exs index 3a4c4d65c..fd179e8c2 100644 --- a/test/web/admin_api/admin_api_controller_test.exs +++ b/test/web/admin_api/admin_api_controller_test.exs @@ -1537,7 +1537,8 @@ test "returns 403 when requested by a non-admin" do |> assign(:user, user) |> get("/api/pleroma/admin/reports") - assert json_response(conn, :forbidden) == %{"error" => "User is not admin."} + assert json_response(conn, :forbidden) == + %{"error" => "User is not an admin or OAuth admin scope is not granted."} end test "returns 403 when requested by anonymous" do From 93a80ee9155bf5257aa92afaca2fe017f28aabfa Mon Sep 17 00:00:00 2001 From: Ivan Tashkinov Date: Fri, 6 Dec 2019 16:56:23 +0300 Subject: [PATCH 3/5] [#1427] Bugfix for `enforce_oauth_admin_scope_usage`. Admin API documentation entry. --- docs/API/admin_api.md | 7 +++++++ lib/pleroma/plugs/user_is_admin_plug.ex | 17 ++++++++--------- 2 files changed, 15 insertions(+), 9 deletions(-) diff --git a/docs/API/admin_api.md b/docs/API/admin_api.md index 2cac317de..b19793150 100644 --- a/docs/API/admin_api.md +++ b/docs/API/admin_api.md @@ -2,6 +2,13 @@ Authentication is required and the user must be an admin. +Configuration options: + +* `[:auth, :enforce_oauth_admin_scope_usage]` — OAuth admin scope requirement toggle. + If `true`, admin actions explicitly demand admin OAuth scope(s) presence in OAuth token (client app must support admin scopes). + If `false` and token doesn't have admin scope(s), `is_admin` user flag grants access to admin-specific actions. + Note that client app needs to explicitly support admin scopes and request them when obtaining auth token. + ## `GET /api/pleroma/admin/users` ### List users diff --git a/lib/pleroma/plugs/user_is_admin_plug.ex b/lib/pleroma/plugs/user_is_admin_plug.ex index 8814556f1..814029dce 100644 --- a/lib/pleroma/plugs/user_is_admin_plug.ex +++ b/lib/pleroma/plugs/user_is_admin_plug.ex @@ -6,13 +6,20 @@ 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 + 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 + 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. @@ -23,14 +30,6 @@ def call(%Plug.Conn{assigns: %{token: %OAuth.Token{scopes: oauth_scopes} = _toke end 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: %User{is_admin: true}}} = conn, _) do - conn - end - end - def call(conn, _) do fail(conn) end From 40e1817f707c3c2ef253009c7363cd81b11322a6 Mon Sep 17 00:00:00 2001 From: Ivan Tashkinov Date: Fri, 6 Dec 2019 20:33:47 +0300 Subject: [PATCH 4/5] [#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 @@ def init(%{scopes: _} = options), do: options 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 @@ def init(options) 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 @@ test "Create" 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 @@ test "Cannot create user with exisiting email" 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) From 1770602747ae95d95d12c5601f99ced8699e8947 Mon Sep 17 00:00:00 2001 From: Ivan Tashkinov Date: Sat, 7 Dec 2019 17:49:53 +0300 Subject: [PATCH 5/5] [#1427] Extra check that admin OAuth scope is used by admin. Adjusted tests. --- lib/pleroma/plugs/user_is_admin_plug.ex | 24 ++++++--- lib/pleroma/user.ex | 3 +- test/plugs/user_is_admin_plug_test.exs | 52 +++++++++++++------ .../admin_api/admin_api_controller_test.exs | 15 +++++- 4 files changed, 67 insertions(+), 27 deletions(-) diff --git a/lib/pleroma/plugs/user_is_admin_plug.ex b/lib/pleroma/plugs/user_is_admin_plug.ex index 4a0e43b00..582fb1f92 100644 --- 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 7b8222ce1..1006b5bf9 100644 --- a/lib/pleroma/user.ex +++ b/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 diff --git a/test/plugs/user_is_admin_plug_test.exs b/test/plugs/user_is_admin_plug_test.exs index 154c9b195..bc6fcd73c 100644 --- 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 @@ 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; - # 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 2fc23ad6c..bcab63cf0 100644 --- 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 @@ 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