From b8be8192fbfe8c27b457094e8d64ccb1c3a29f7f Mon Sep 17 00:00:00 2001 From: FloatingGhost Date: Fri, 16 Dec 2022 03:25:14 +0000 Subject: [PATCH] do not allow non-admins to register tokens with admin scopes this didn't actually _do_ anything in the past, the users would be prevented from accessing the resource, but they shouldn't be able to even create them --- lib/pleroma/web/o_auth/o_auth_controller.ex | 17 +-- lib/pleroma/web/o_auth/scopes.ex | 16 ++- .../web/o_auth/o_auth_controller_test.exs | 119 +++++++++++++----- 3 files changed, 110 insertions(+), 42 deletions(-) diff --git a/lib/pleroma/web/o_auth/o_auth_controller.ex b/lib/pleroma/web/o_auth/o_auth_controller.ex index 8f32e7219..3943ca449 100644 --- a/lib/pleroma/web/o_auth/o_auth_controller.ex +++ b/lib/pleroma/web/o_auth/o_auth_controller.ex @@ -211,11 +211,11 @@ defp handle_create_authorization_error( {:error, scopes_issue}, %{"authorization" => _} = params ) - when scopes_issue in [:unsupported_scopes, :missing_scopes] do + when scopes_issue in [:unsupported_scopes, :missing_scopes, :user_is_not_an_admin] do # Per https://github.com/tootsuite/mastodon/blob/ # 51e154f5e87968d6bb115e053689767ab33e80cd/app/controllers/api/base_controller.rb#L39 conn - |> put_flash(:error, dgettext("errors", "This action is outside the authorized scopes")) + |> put_flash(:error, dgettext("errors", "This action is outside of authorized scopes")) |> put_status(:unauthorized) |> authorize(params) end @@ -605,7 +605,7 @@ defp do_create_authorization( defp do_create_authorization(%User{} = user, %App{} = app, requested_scopes) when is_list(requested_scopes) do with {:account_status, :active} <- {:account_status, User.account_status(user)}, - {:ok, scopes} <- validate_scopes(app, requested_scopes), + {:ok, scopes} <- validate_scopes(user, app, requested_scopes), {:ok, auth} <- Authorization.create_authorization(app, user, scopes) do {:ok, auth} end @@ -637,15 +637,16 @@ defp build_and_response_mfa_token(user, auth) do end end - @spec validate_scopes(App.t(), map() | list()) :: + @spec validate_scopes(User.t(), App.t(), map() | list()) :: {:ok, list()} | {:error, :missing_scopes | :unsupported_scopes} - defp validate_scopes(%App{} = app, params) when is_map(params) do + defp validate_scopes(%User{} = user, %App{} = app, params) when is_map(params) do requested_scopes = Scopes.fetch_scopes(params, app.scopes) - validate_scopes(app, requested_scopes) + validate_scopes(user, app, requested_scopes) end - defp validate_scopes(%App{} = app, requested_scopes) when is_list(requested_scopes) do - Scopes.validate(requested_scopes, app.scopes) + defp validate_scopes(%User{} = user, %App{} = app, requested_scopes) + when is_list(requested_scopes) do + Scopes.validate(requested_scopes, app.scopes, user) end def default_redirect_uri(%App{} = app) do diff --git a/lib/pleroma/web/o_auth/scopes.ex b/lib/pleroma/web/o_auth/scopes.ex index ada43eae9..7fe04b912 100644 --- a/lib/pleroma/web/o_auth/scopes.ex +++ b/lib/pleroma/web/o_auth/scopes.ex @@ -56,12 +56,20 @@ def to_string(scopes), do: Enum.join(scopes, " ") @doc """ Validates scopes. """ - @spec validate(list() | nil, list()) :: - {:ok, list()} | {:error, :missing_scopes | :unsupported_scopes} - def validate(blank_scopes, _app_scopes) when blank_scopes in [nil, []], + @spec validate(list() | nil, list(), Pleroma.User.t()) :: + {:ok, list()} | {:error, :missing_scopes | :unsupported_scopes, :user_is_not_an_admin} + def validate(blank_scopes, _app_scopes, _user) when blank_scopes in [nil, []], do: {:error, :missing_scopes} - def validate(scopes, app_scopes) do + def validate(scopes, app_scopes, %Pleroma.User{is_admin: is_admin}) do + if !is_admin && contains_admin_scopes?(scopes) do + {:error, :user_is_not_an_admin} + else + validate_scopes_are_supported(scopes, app_scopes) + end + end + + defp validate_scopes_are_supported(scopes, app_scopes) do case OAuthScopesPlug.filter_descendants(scopes, app_scopes) do ^scopes -> {:ok, scopes} _ -> {:error, :unsupported_scopes} diff --git a/test/pleroma/web/o_auth/o_auth_controller_test.exs b/test/pleroma/web/o_auth/o_auth_controller_test.exs index 5a1258ec3..19a7dea60 100644 --- a/test/pleroma/web/o_auth/o_auth_controller_test.exs +++ b/test/pleroma/web/o_auth/o_auth_controller_test.exs @@ -693,45 +693,76 @@ test "with existing authentication and OOB `redirect_uri`, redirects to app with describe "POST /oauth/authorize" do test "redirects with oauth authorization, " <> - "granting requested app-supported scopes to both admin- and non-admin users" do + "granting requested app-supported scopes to both admin users" do app_scopes = ["read", "write", "admin", "secret_scope"] app = insert(:oauth_app, scopes: app_scopes) redirect_uri = OAuthController.default_redirect_uri(app) + scopes_subset = ["read:subscope", "write", "admin"] + admin = insert(:user, is_admin: true) + + # In case scope param is missing, expecting _all_ app-supported scopes to be granted + conn = + post( + build_conn(), + "/oauth/authorize", + %{ + "authorization" => %{ + "name" => admin.nickname, + "password" => "test", + "client_id" => app.client_id, + "redirect_uri" => redirect_uri, + "scope" => scopes_subset, + "state" => "statepassed" + } + } + ) + + target = redirected_to(conn) + assert target =~ redirect_uri + + query = URI.parse(target).query |> URI.query_decoder() |> Map.new() + + assert %{"state" => "statepassed", "code" => code} = query + auth = Repo.get_by(Authorization, token: code) + assert auth + assert auth.scopes == scopes_subset + end + + test "redirects with oauth authorization, " <> + "granting requested app-supported scopes for non-admin users" do + app_scopes = ["read", "write", "secret_scope", "admin"] + 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"] + scopes_subset = ["read:subscope", "write"] # 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 = - 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" - } + conn = + post( + build_conn(), + "/oauth/authorize", + %{ + "authorization" => %{ + "name" => non_admin.nickname, + "password" => "test", + "client_id" => app.client_id, + "redirect_uri" => redirect_uri, + "scope" => scopes_subset, + "state" => "statepassed" } - ) + } + ) - target = redirected_to(conn) - assert target =~ redirect_uri + target = redirected_to(conn) + assert target =~ redirect_uri - query = URI.parse(target).query |> URI.query_decoder() |> Map.new() + query = URI.parse(target).query |> URI.query_decoder() |> Map.new() - assert %{"state" => "statepassed", "code" => code} = query - auth = Repo.get_by(Authorization, token: code) - assert auth - assert auth.scopes == expected_scopes - end + assert %{"state" => "statepassed", "code" => code} = query + auth = Repo.get_by(Authorization, token: code) + assert auth + assert auth.scopes == scopes_subset end test "authorize from cookie" do @@ -739,6 +770,7 @@ test "authorize from cookie" do app = insert(:oauth_app) oauth_token = insert(:oauth_token, user: user, app: app) redirect_uri = OAuthController.default_redirect_uri(app) + IO.inspect(app) conn = build_conn() @@ -831,6 +863,33 @@ test "returns 401 for wrong credentials", %{conn: conn} do assert result =~ "Invalid Username/Password" end + test "returns 401 when attempting to use an admin scope with a non-admin", %{conn: conn} do + user = insert(:user) + app = insert(:oauth_app, scopes: ["admin"]) + redirect_uri = OAuthController.default_redirect_uri(app) + + result = + conn + |> post("/oauth/authorize", %{ + "authorization" => %{ + "name" => user.nickname, + "password" => "test", + "client_id" => app.client_id, + "redirect_uri" => redirect_uri, + "state" => "statepassed", + "scope" => Enum.join(app.scopes, " ") + } + }) + |> html_response(:unauthorized) + + # Keep the details + assert result =~ app.client_id + assert result =~ redirect_uri + + # Error message + assert result =~ "outside of authorized scopes" + end + test "returns 401 for missing scopes" do user = insert(:user, is_admin: false) app = insert(:oauth_app, scopes: ["read", "write", "admin"]) @@ -855,7 +914,7 @@ test "returns 401 for missing scopes" do assert result =~ redirect_uri # Error message - assert result =~ "This action is outside the authorized scopes" + assert result =~ "This action is outside of authorized scopes" end test "returns 401 for scopes beyond app scopes hierarchy", %{conn: conn} do @@ -882,7 +941,7 @@ test "returns 401 for scopes beyond app scopes hierarchy", %{conn: conn} do assert result =~ redirect_uri # Error message - assert result =~ "This action is outside the authorized scopes" + assert result =~ "This action is outside of authorized scopes" end end