From 6c94b7498b889ffe13691123c94bbe5440786852 Mon Sep 17 00:00:00 2001 From: Ivan Tashkinov Date: Fri, 10 Jan 2020 10:52:21 +0300 Subject: [PATCH] [#1478] OAuth admin tweaks: enforced OAuth admin scopes usage by default, migrated existing OAuth records. Adjusted tests. --- CHANGELOG.md | 1 + config/config.exs | 2 +- lib/pleroma/plugs/user_is_admin_plug.ex | 1 + lib/pleroma/user.ex | 23 ++---- lib/pleroma/web/oauth/oauth_controller.ex | 10 +-- lib/pleroma/web/oauth/scopes.ex | 24 +----- ...add_scopes_to_pleroma_feo_auth_records.exs | 17 ++++ test/web/oauth/oauth_controller_test.exs | 78 ++++++++++--------- .../controllers/emoji_api_controller_test.exs | 4 + 9 files changed, 79 insertions(+), 81 deletions(-) create mode 100644 priv/repo/migrations/20191220174645_add_scopes_to_pleroma_feo_auth_records.exs diff --git a/CHANGELOG.md b/CHANGELOG.md index 22f199b3d..02ddb6213 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/). ### Changed - **Breaking:** Elixir >=1.8 is now required (was >= 1.7) - **Breaking:** attachment links (`config :pleroma, :instance, no_attachment_links` and `config :pleroma, Pleroma.Upload, link_name`) disabled by default +- **Breaking:** OAuth: defaulted `[:auth, :enforce_oauth_admin_scope_usage]` setting to `true` which demands `admin` OAuth scope to perform admin actions (in addition to `is_admin` flag on User); make sure to use bundled or newer versions of AdminFE & PleromaFE to access admin / moderator features. - Replaced [pleroma_job_queue](https://git.pleroma.social/pleroma/pleroma_job_queue) and `Pleroma.Web.Federator.RetryQueue` with [Oban](https://github.com/sorentwo/oban) (see [`docs/config.md`](docs/config.md) on migrating customized worker / retry settings) - Introduced [quantum](https://github.com/quantum-elixir/quantum-core) job scheduler - Enabled `:instance, extended_nickname_format` in the default config diff --git a/config/config.exs b/config/config.exs index c8d42e83e..eeb88174a 100644 --- a/config/config.exs +++ b/config/config.exs @@ -565,7 +565,7 @@ config :pleroma, :auth, - enforce_oauth_admin_scope_usage: false, + enforce_oauth_admin_scope_usage: true, oauth_consumer_strategies: oauth_consumer_strategies config :pleroma, Pleroma.Emails.Mailer, adapter: Swoosh.Adapters.Sendmail, enabled: false diff --git a/lib/pleroma/plugs/user_is_admin_plug.ex b/lib/pleroma/plugs/user_is_admin_plug.ex index 582fb1f92..3190163d3 100644 --- a/lib/pleroma/plugs/user_is_admin_plug.ex +++ b/lib/pleroma/plugs/user_is_admin_plug.ex @@ -23,6 +23,7 @@ def call(%{assigns: %{user: %User{is_admin: true}} = assigns} = conn, _) 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. + # Admin might opt out of admin scope for some apps to block any admin actions from them. conn true -> diff --git a/lib/pleroma/user.ex b/lib/pleroma/user.ex index 706aee2ff..40cc93e03 100644 --- a/lib/pleroma/user.ex +++ b/lib/pleroma/user.ex @@ -1847,22 +1847,13 @@ defp truncate_field(%{"name" => name, "value" => value}) do end def admin_api_update(user, params) do - 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). - # This is an extra safety measure (tokens' admin scopes won't be accepted for non-admins). - global_sign_out(user) - end - - {:ok, updated_user} - end + user + |> cast(params, [ + :is_moderator, + :is_admin, + :show_role + ]) + |> update_and_set_cache() end @doc "Signs user out of all applications" diff --git a/lib/pleroma/web/oauth/oauth_controller.ex b/lib/pleroma/web/oauth/oauth_controller.ex index 87acdec97..d31a3d91c 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, user), + {:ok, scopes} <- validate_scopes(app, params), {: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, user), + {:ok, scopes} <- validate_scopes(app, auth_attrs), {: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(), User.t()) :: + @spec validate_scopes(App.t(), map()) :: {:ok, list()} | {:error, :missing_scopes | :unsupported_scopes} - defp validate_scopes(%App{} = app, params, %User{} = user) do + defp validate_scopes(%App{} = app, params) do params |> Scopes.fetch_scopes(app.scopes) - |> Scopes.validate(app.scopes, user) + |> Scopes.validate(app.scopes) 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 00da225b9..151467494 100644 --- a/lib/pleroma/web/oauth/scopes.ex +++ b/lib/pleroma/web/oauth/scopes.ex @@ -8,7 +8,6 @@ defmodule Pleroma.Web.OAuth.Scopes do """ alias Pleroma.Plugs.OAuthScopesPlug - alias Pleroma.User @doc """ Fetch scopes from request params. @@ -56,35 +55,18 @@ def to_string(scopes), do: Enum.join(scopes, " ") @doc """ Validates scopes. """ - @spec validate(list() | nil, list(), User.t()) :: + @spec validate(list() | nil, list()) :: {:ok, list()} | {:error, :missing_scopes | :unsupported_scopes} - def validate(blank_scopes, _app_scopes, _user) when blank_scopes in [nil, []], + def validate(blank_scopes, _app_scopes) when blank_scopes in [nil, []], do: {:error, :missing_scopes} - 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 + def validate(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 - # Gracefully dropping admin scopes from requested scopes if user isn't an admin (not raising) - scopes = scopes -- OAuthScopesPlug.filter_descendants(scopes, ["admin"]) - validate(scopes, app_scopes, user) - end - end - def contains_admin_scopes?(scopes) do scopes |> OAuthScopesPlug.filter_descendants(["admin"]) diff --git a/priv/repo/migrations/20191220174645_add_scopes_to_pleroma_feo_auth_records.exs b/priv/repo/migrations/20191220174645_add_scopes_to_pleroma_feo_auth_records.exs new file mode 100644 index 000000000..6b160ad16 --- /dev/null +++ b/priv/repo/migrations/20191220174645_add_scopes_to_pleroma_feo_auth_records.exs @@ -0,0 +1,17 @@ +defmodule Pleroma.Repo.Migrations.AddScopesToPleromaFEOAuthRecords do + use Ecto.Migration + + def up do + update_scopes_clause = "SET scopes = '{read,write,follow,push,admin}'" + apps_where = "WHERE apps.client_name like 'PleromaFE_%' or apps.client_name like 'AdminFE_%'" + app_id_subquery_where = "WHERE app_id IN (SELECT apps.id FROM apps #{apps_where})" + + execute("UPDATE apps #{update_scopes_clause} #{apps_where}") + + for table <- ["oauth_authorizations", "oauth_tokens"] do + execute("UPDATE #{table} #{update_scopes_clause} #{app_id_subquery_where}") + end + end + + def down, do: :noop +end diff --git a/test/web/oauth/oauth_controller_test.exs b/test/web/oauth/oauth_controller_test.exs index 901f2ae41..7a629da4f 100644 --- a/test/web/oauth/oauth_controller_test.exs +++ b/test/web/oauth/oauth_controller_test.exs @@ -568,29 +568,34 @@ test "with existing authentication and OOB `redirect_uri`, redirects to app with describe "POST /oauth/authorize" do test "redirects with oauth authorization, " <> - "keeping only non-admin scopes for non-admin user" do - app = insert(:oauth_app, scopes: ["read", "write", "admin"]) + "granting requested app-supported scopes to both admin- and non-admin users" do + app_scopes = ["read", "write", "admin", "secret_scope"] + app = insert(:oauth_app, scopes: app_scopes) redirect_uri = OAuthController.default_redirect_uri(app) non_admin = insert(:user, is_admin: false) admin = insert(:user, is_admin: true) + scopes_subset = ["read:subscope", "write", "admin"] - for {user, expected_scopes} <- %{ - non_admin => ["read:subscope", "write"], - admin => ["read:subscope", "write", "admin"] - } do + # In case scope param is missing, expecting _all_ app-supported scopes to be granted + for user <- [non_admin, admin], + {requested_scopes, expected_scopes} <- + %{scopes_subset => scopes_subset, nil => app_scopes} do conn = - build_conn() - |> post("/oauth/authorize", %{ - "authorization" => %{ - "name" => user.nickname, - "password" => "test", - "client_id" => app.client_id, - "redirect_uri" => redirect_uri, - "scope" => "read:subscope write admin", - "state" => "statepassed" + post( + build_conn(), + "/oauth/authorize", + %{ + "authorization" => %{ + "name" => user.nickname, + "password" => "test", + "client_id" => app.client_id, + "redirect_uri" => redirect_uri, + "scope" => requested_scopes, + "state" => "statepassed" + } } - }) + ) target = redirected_to(conn) assert target =~ redirect_uri @@ -631,34 +636,31 @@ test "returns 401 for wrong credentials", %{conn: conn} do assert result =~ "Invalid Username/Password" end - test "returns 401 for missing scopes " <> - "(including all admin-only scopes for non-admin user)" do + test "returns 401 for missing scopes" do user = insert(:user, is_admin: false) app = insert(:oauth_app, scopes: ["read", "write", "admin"]) redirect_uri = OAuthController.default_redirect_uri(app) - for scope_param <- ["", "admin:read admin:write"] do - result = - build_conn() - |> post("/oauth/authorize", %{ - "authorization" => %{ - "name" => user.nickname, - "password" => "test", - "client_id" => app.client_id, - "redirect_uri" => redirect_uri, - "state" => "statepassed", - "scope" => scope_param - } - }) - |> html_response(:unauthorized) + result = + build_conn() + |> post("/oauth/authorize", %{ + "authorization" => %{ + "name" => user.nickname, + "password" => "test", + "client_id" => app.client_id, + "redirect_uri" => redirect_uri, + "state" => "statepassed", + "scope" => "" + } + }) + |> html_response(:unauthorized) - # Keep the details - assert result =~ app.client_id - assert result =~ redirect_uri + # Keep the details + assert result =~ app.client_id + assert result =~ redirect_uri - # Error message - assert result =~ "This action is outside the authorized scopes" - end + # Error message + assert result =~ "This action is outside the authorized scopes" end test "returns 401 for scopes beyond app scopes hierarchy", %{conn: conn} do diff --git a/test/web/pleroma_api/controllers/emoji_api_controller_test.exs b/test/web/pleroma_api/controllers/emoji_api_controller_test.exs index 3d3becefd..101c4d7d7 100644 --- a/test/web/pleroma_api/controllers/emoji_api_controller_test.exs +++ b/test/web/pleroma_api/controllers/emoji_api_controller_test.exs @@ -14,6 +14,10 @@ defmodule Pleroma.Web.PleromaAPI.EmojiAPIControllerTest do "emoji" ) + clear_config([:auth, :enforce_oauth_admin_scope_usage]) do + Pleroma.Config.put([:auth, :enforce_oauth_admin_scope_usage], false) + end + test "shared & non-shared pack information in list_packs is ok" do conn = build_conn() resp = conn |> get(emoji_api_path(conn, :list_packs)) |> json_response(200)