From f362836742aabd5b60b92c1296f2bbb6d83a3d59 Mon Sep 17 00:00:00 2001 From: Egor Kislitsyn Date: Fri, 24 Apr 2020 14:46:59 +0400 Subject: [PATCH 01/18] Support validation for inline OpenAPI schema and automatic tests for examples --- .../web/api_spec/operations/app_operation.ex | 60 +++++++++++++++++-- .../operations/custom_emoji_operation.ex | 40 ++++++++++++- .../operations/domain_block_operation.ex | 31 ++++++++-- .../api_spec/schemas/app_create_request.ex | 33 ---------- .../api_spec/schemas/app_create_response.ex | 33 ---------- .../schemas/custom_emojis_response.ex | 42 ------------- .../api_spec/schemas/domain_block_request.ex | 20 ------- .../schemas/domain_blocks_response.ex | 16 ----- lib/pleroma/web/oauth/scopes.ex | 6 +- test/support/api_spec_helpers.ex | 57 ++++++++++++++++++ test/support/conn_case.ex | 36 +++++++++++ test/web/api_spec/app_operation_test.exs | 45 -------------- test/web/api_spec/schema_examples_test.exs | 43 +++++++++++++ .../controllers/app_controller_test.exs | 4 +- .../custom_emoji_controller_test.exs | 17 +----- .../domain_block_controller_test.exs | 28 +++------ 16 files changed, 267 insertions(+), 244 deletions(-) delete mode 100644 lib/pleroma/web/api_spec/schemas/app_create_request.ex delete mode 100644 lib/pleroma/web/api_spec/schemas/app_create_response.ex delete mode 100644 lib/pleroma/web/api_spec/schemas/custom_emojis_response.ex delete mode 100644 lib/pleroma/web/api_spec/schemas/domain_block_request.ex delete mode 100644 lib/pleroma/web/api_spec/schemas/domain_blocks_response.ex create mode 100644 test/support/api_spec_helpers.ex delete mode 100644 test/web/api_spec/app_operation_test.exs create mode 100644 test/web/api_spec/schema_examples_test.exs diff --git a/lib/pleroma/web/api_spec/operations/app_operation.ex b/lib/pleroma/web/api_spec/operations/app_operation.ex index 26d8dbd42..035ef2470 100644 --- a/lib/pleroma/web/api_spec/operations/app_operation.ex +++ b/lib/pleroma/web/api_spec/operations/app_operation.ex @@ -6,8 +6,6 @@ defmodule Pleroma.Web.ApiSpec.AppOperation do alias OpenApiSpex.Operation alias OpenApiSpex.Schema alias Pleroma.Web.ApiSpec.Helpers - alias Pleroma.Web.ApiSpec.Schemas.AppCreateRequest - alias Pleroma.Web.ApiSpec.Schemas.AppCreateResponse @spec open_api_operation(atom) :: Operation.t() def open_api_operation(action) do @@ -22,9 +20,9 @@ def create_operation do summary: "Create an application", description: "Create a new application to obtain OAuth2 credentials", operationId: "AppController.create", - requestBody: Helpers.request_body("Parameters", AppCreateRequest, required: true), + requestBody: Helpers.request_body("Parameters", create_request(), required: true), responses: %{ - 200 => Operation.response("App", "application/json", AppCreateResponse), + 200 => Operation.response("App", "application/json", create_response()), 422 => Operation.response( "Unprocessable Entity", @@ -93,4 +91,58 @@ def verify_credentials_operation do } } end + + defp create_request do + %Schema{ + title: "AppCreateRequest", + description: "POST body for creating an app", + type: :object, + properties: %{ + client_name: %Schema{type: :string, description: "A name for your application."}, + redirect_uris: %Schema{ + type: :string, + description: + "Where the user should be redirected after authorization. To display the authorization code to the user instead of redirecting to a web page, use `urn:ietf:wg:oauth:2.0:oob` in this parameter." + }, + scopes: %Schema{ + type: :string, + description: "Space separated list of scopes", + default: "read" + }, + website: %Schema{type: :string, description: "A URL to the homepage of your app"} + }, + required: [:client_name, :redirect_uris], + example: %{ + "client_name" => "My App", + "redirect_uris" => "https://myapp.com/auth/callback", + "website" => "https://myapp.com/" + } + } + end + + defp create_response do + %Schema{ + title: "AppCreateResponse", + description: "Response schema for an app", + type: :object, + properties: %{ + id: %Schema{type: :string}, + name: %Schema{type: :string}, + client_id: %Schema{type: :string}, + client_secret: %Schema{type: :string}, + redirect_uri: %Schema{type: :string}, + vapid_key: %Schema{type: :string}, + website: %Schema{type: :string, nullable: true} + }, + example: %{ + "id" => "123", + "name" => "My App", + "client_id" => "TWhM-tNSuncnqN7DBJmoyeLnk6K3iJJ71KKXxgL1hPM", + "client_secret" => "ZEaFUFmF0umgBX1qKJDjaU99Q31lDkOU8NutzTOoliw", + "vapid_key" => + "BCk-QqERU0q-CfYZjcuB6lnyyOYfJ2AifKqfeGIm7Z-HiTU5T9eTG5GxVA0_OH5mMlI4UkkDTpaZwozy0TzdZ2M=", + "website" => "https://myapp.com/" + } + } + end end diff --git a/lib/pleroma/web/api_spec/operations/custom_emoji_operation.ex b/lib/pleroma/web/api_spec/operations/custom_emoji_operation.ex index cf2215823..a117fe460 100644 --- a/lib/pleroma/web/api_spec/operations/custom_emoji_operation.ex +++ b/lib/pleroma/web/api_spec/operations/custom_emoji_operation.ex @@ -4,7 +4,8 @@ defmodule Pleroma.Web.ApiSpec.CustomEmojiOperation do alias OpenApiSpex.Operation - alias Pleroma.Web.ApiSpec.Schemas.CustomEmojisResponse + alias OpenApiSpex.Schema + alias Pleroma.Web.ApiSpec.Schemas.CustomEmoji def open_api_operation(action) do operation = String.to_existing_atom("#{action}_operation") @@ -18,8 +19,43 @@ def index_operation do description: "Returns custom emojis that are available on the server.", operationId: "CustomEmojiController.index", responses: %{ - 200 => Operation.response("Custom Emojis", "application/json", CustomEmojisResponse) + 200 => Operation.response("Custom Emojis", "application/json", custom_emojis_resposnse()) } } end + + defp custom_emojis_resposnse do + %Schema{ + title: "CustomEmojisResponse", + description: "Response schema for custom emojis", + type: :array, + items: CustomEmoji, + example: [ + %{ + "category" => "Fun", + "shortcode" => "blank", + "static_url" => "https://lain.com/emoji/blank.png", + "tags" => ["Fun"], + "url" => "https://lain.com/emoji/blank.png", + "visible_in_picker" => false + }, + %{ + "category" => "Gif,Fun", + "shortcode" => "firefox", + "static_url" => "https://lain.com/emoji/Firefox.gif", + "tags" => ["Gif", "Fun"], + "url" => "https://lain.com/emoji/Firefox.gif", + "visible_in_picker" => true + }, + %{ + "category" => "pack:mixed", + "shortcode" => "sadcat", + "static_url" => "https://lain.com/emoji/mixed/sadcat.png", + "tags" => ["pack:mixed"], + "url" => "https://lain.com/emoji/mixed/sadcat.png", + "visible_in_picker" => true + } + ] + } + end end diff --git a/lib/pleroma/web/api_spec/operations/domain_block_operation.ex b/lib/pleroma/web/api_spec/operations/domain_block_operation.ex index dd14837c3..3b7f51ceb 100644 --- a/lib/pleroma/web/api_spec/operations/domain_block_operation.ex +++ b/lib/pleroma/web/api_spec/operations/domain_block_operation.ex @@ -6,8 +6,6 @@ defmodule Pleroma.Web.ApiSpec.DomainBlockOperation do alias OpenApiSpex.Operation alias OpenApiSpex.Schema alias Pleroma.Web.ApiSpec.Helpers - alias Pleroma.Web.ApiSpec.Schemas.DomainBlockRequest - alias Pleroma.Web.ApiSpec.Schemas.DomainBlocksResponse def open_api_operation(action) do operation = String.to_existing_atom("#{action}_operation") @@ -22,7 +20,13 @@ def index_operation do security: [%{"oAuth" => ["follow", "read:blocks"]}], operationId: "DomainBlockController.index", responses: %{ - 200 => Operation.response("Domain blocks", "application/json", DomainBlocksResponse) + 200 => + Operation.response("Domain blocks", "application/json", %Schema{ + description: "Response schema for domain blocks", + type: :array, + items: %Schema{type: :string}, + example: ["google.com", "facebook.com"] + }) } } end @@ -40,7 +44,7 @@ def create_operation do - prevent following new users from it (but does not remove existing follows) """, operationId: "DomainBlockController.create", - requestBody: Helpers.request_body("Parameters", DomainBlockRequest, required: true), + requestBody: domain_block_request(), security: [%{"oAuth" => ["follow", "write:blocks"]}], responses: %{ 200 => Operation.response("Empty object", "application/json", %Schema{type: :object}) @@ -54,11 +58,28 @@ def delete_operation do summary: "Unblock a domain", description: "Remove a domain block, if it exists in the user's array of blocked domains.", operationId: "DomainBlockController.delete", - requestBody: Helpers.request_body("Parameters", DomainBlockRequest, required: true), + requestBody: domain_block_request(), security: [%{"oAuth" => ["follow", "write:blocks"]}], responses: %{ 200 => Operation.response("Empty object", "application/json", %Schema{type: :object}) } } end + + defp domain_block_request do + Helpers.request_body( + "Parameters", + %Schema{ + type: :object, + properties: %{ + domain: %Schema{type: :string} + }, + required: [:domain] + }, + required: true, + example: %{ + "domain" => "facebook.com" + } + ) + end end diff --git a/lib/pleroma/web/api_spec/schemas/app_create_request.ex b/lib/pleroma/web/api_spec/schemas/app_create_request.ex deleted file mode 100644 index 8a83abef3..000000000 --- a/lib/pleroma/web/api_spec/schemas/app_create_request.ex +++ /dev/null @@ -1,33 +0,0 @@ -# Pleroma: A lightweight social networking server -# Copyright © 2017-2020 Pleroma Authors -# SPDX-License-Identifier: AGPL-3.0-only - -defmodule Pleroma.Web.ApiSpec.Schemas.AppCreateRequest do - alias OpenApiSpex.Schema - require OpenApiSpex - - OpenApiSpex.schema(%{ - title: "AppCreateRequest", - description: "POST body for creating an app", - type: :object, - properties: %{ - client_name: %Schema{type: :string, description: "A name for your application."}, - redirect_uris: %Schema{ - type: :string, - description: - "Where the user should be redirected after authorization. To display the authorization code to the user instead of redirecting to a web page, use `urn:ietf:wg:oauth:2.0:oob` in this parameter." - }, - scopes: %Schema{ - type: :string, - description: "Space separated list of scopes. If none is provided, defaults to `read`." - }, - website: %Schema{type: :string, description: "A URL to the homepage of your app"} - }, - required: [:client_name, :redirect_uris], - example: %{ - "client_name" => "My App", - "redirect_uris" => "https://myapp.com/auth/callback", - "website" => "https://myapp.com/" - } - }) -end diff --git a/lib/pleroma/web/api_spec/schemas/app_create_response.ex b/lib/pleroma/web/api_spec/schemas/app_create_response.ex deleted file mode 100644 index f290fb031..000000000 --- a/lib/pleroma/web/api_spec/schemas/app_create_response.ex +++ /dev/null @@ -1,33 +0,0 @@ -# Pleroma: A lightweight social networking server -# Copyright © 2017-2020 Pleroma Authors -# SPDX-License-Identifier: AGPL-3.0-only - -defmodule Pleroma.Web.ApiSpec.Schemas.AppCreateResponse do - alias OpenApiSpex.Schema - - require OpenApiSpex - - OpenApiSpex.schema(%{ - title: "AppCreateResponse", - description: "Response schema for an app", - type: :object, - properties: %{ - id: %Schema{type: :string}, - name: %Schema{type: :string}, - client_id: %Schema{type: :string}, - client_secret: %Schema{type: :string}, - redirect_uri: %Schema{type: :string}, - vapid_key: %Schema{type: :string}, - website: %Schema{type: :string, nullable: true} - }, - example: %{ - "id" => "123", - "name" => "My App", - "client_id" => "TWhM-tNSuncnqN7DBJmoyeLnk6K3iJJ71KKXxgL1hPM", - "client_secret" => "ZEaFUFmF0umgBX1qKJDjaU99Q31lDkOU8NutzTOoliw", - "vapid_key" => - "BCk-QqERU0q-CfYZjcuB6lnyyOYfJ2AifKqfeGIm7Z-HiTU5T9eTG5GxVA0_OH5mMlI4UkkDTpaZwozy0TzdZ2M=", - "website" => "https://myapp.com/" - } - }) -end diff --git a/lib/pleroma/web/api_spec/schemas/custom_emojis_response.ex b/lib/pleroma/web/api_spec/schemas/custom_emojis_response.ex deleted file mode 100644 index 01582a63d..000000000 --- a/lib/pleroma/web/api_spec/schemas/custom_emojis_response.ex +++ /dev/null @@ -1,42 +0,0 @@ -# Pleroma: A lightweight social networking server -# Copyright © 2017-2020 Pleroma Authors -# SPDX-License-Identifier: AGPL-3.0-only - -defmodule Pleroma.Web.ApiSpec.Schemas.CustomEmojisResponse do - alias Pleroma.Web.ApiSpec.Schemas.CustomEmoji - - require OpenApiSpex - - OpenApiSpex.schema(%{ - title: "CustomEmojisResponse", - description: "Response schema for custom emojis", - type: :array, - items: CustomEmoji, - example: [ - %{ - "category" => "Fun", - "shortcode" => "blank", - "static_url" => "https://lain.com/emoji/blank.png", - "tags" => ["Fun"], - "url" => "https://lain.com/emoji/blank.png", - "visible_in_picker" => true - }, - %{ - "category" => "Gif,Fun", - "shortcode" => "firefox", - "static_url" => "https://lain.com/emoji/Firefox.gif", - "tags" => ["Gif", "Fun"], - "url" => "https://lain.com/emoji/Firefox.gif", - "visible_in_picker" => true - }, - %{ - "category" => "pack:mixed", - "shortcode" => "sadcat", - "static_url" => "https://lain.com/emoji/mixed/sadcat.png", - "tags" => ["pack:mixed"], - "url" => "https://lain.com/emoji/mixed/sadcat.png", - "visible_in_picker" => true - } - ] - }) -end diff --git a/lib/pleroma/web/api_spec/schemas/domain_block_request.ex b/lib/pleroma/web/api_spec/schemas/domain_block_request.ex deleted file mode 100644 index ee9238361..000000000 --- a/lib/pleroma/web/api_spec/schemas/domain_block_request.ex +++ /dev/null @@ -1,20 +0,0 @@ -# Pleroma: A lightweight social networking server -# Copyright © 2017-2020 Pleroma Authors -# SPDX-License-Identifier: AGPL-3.0-only - -defmodule Pleroma.Web.ApiSpec.Schemas.DomainBlockRequest do - alias OpenApiSpex.Schema - require OpenApiSpex - - OpenApiSpex.schema(%{ - title: "DomainBlockRequest", - type: :object, - properties: %{ - domain: %Schema{type: :string} - }, - required: [:domain], - example: %{ - "domain" => "facebook.com" - } - }) -end diff --git a/lib/pleroma/web/api_spec/schemas/domain_blocks_response.ex b/lib/pleroma/web/api_spec/schemas/domain_blocks_response.ex deleted file mode 100644 index d895aca4e..000000000 --- a/lib/pleroma/web/api_spec/schemas/domain_blocks_response.ex +++ /dev/null @@ -1,16 +0,0 @@ -# Pleroma: A lightweight social networking server -# Copyright © 2017-2020 Pleroma Authors -# SPDX-License-Identifier: AGPL-3.0-only - -defmodule Pleroma.Web.ApiSpec.Schemas.DomainBlocksResponse do - require OpenApiSpex - alias OpenApiSpex.Schema - - OpenApiSpex.schema(%{ - title: "DomainBlocksResponse", - description: "Response schema for domain blocks", - type: :array, - items: %Schema{type: :string}, - example: ["google.com", "facebook.com"] - }) -end diff --git a/lib/pleroma/web/oauth/scopes.ex b/lib/pleroma/web/oauth/scopes.ex index 1023f16d4..6f06f1431 100644 --- a/lib/pleroma/web/oauth/scopes.ex +++ b/lib/pleroma/web/oauth/scopes.ex @@ -17,12 +17,8 @@ defmodule Pleroma.Web.OAuth.Scopes do """ @spec fetch_scopes(map() | struct(), list()) :: list() - def fetch_scopes(%Pleroma.Web.ApiSpec.Schemas.AppCreateRequest{scopes: scopes}, default) do - parse_scopes(scopes, default) - end - def fetch_scopes(params, default) do - parse_scopes(params["scope"] || params["scopes"], default) + parse_scopes(params["scope"] || params["scopes"] || params[:scopes], default) end def parse_scopes(scopes, _default) when is_list(scopes) do diff --git a/test/support/api_spec_helpers.ex b/test/support/api_spec_helpers.ex new file mode 100644 index 000000000..80c69c788 --- /dev/null +++ b/test/support/api_spec_helpers.ex @@ -0,0 +1,57 @@ +# Pleroma: A lightweight social networking server +# Copyright © 2017-2020 Pleroma Authors +# SPDX-License-Identifier: AGPL-3.0-only + +defmodule Pleroma.Tests.ApiSpecHelpers do + @moduledoc """ + OpenAPI spec test helpers + """ + + import ExUnit.Assertions + + alias OpenApiSpex.Cast.Error + alias OpenApiSpex.Reference + alias OpenApiSpex.Schema + + def assert_schema(value, schema) do + api_spec = Pleroma.Web.ApiSpec.spec() + + case OpenApiSpex.cast_value(value, schema, api_spec) do + {:ok, data} -> + data + + {:error, errors} -> + errors = + Enum.map(errors, fn error -> + message = Error.message(error) + path = Error.path_to_string(error) + "#{message} at #{path}" + end) + + flunk( + "Value does not conform to schema #{schema.title}: #{Enum.join(errors, "\n")}\n#{ + inspect(value) + }" + ) + end + end + + def resolve_schema(%Schema{} = schema), do: schema + + def resolve_schema(%Reference{} = ref) do + schemas = Pleroma.Web.ApiSpec.spec().components.schemas + Reference.resolve_schema(ref, schemas) + end + + def api_operations do + paths = Pleroma.Web.ApiSpec.spec().paths + + Enum.flat_map(paths, fn {_, path_item} -> + path_item + |> Map.take([:delete, :get, :head, :options, :patch, :post, :put, :trace]) + |> Map.values() + |> Enum.reject(&is_nil/1) + |> Enum.uniq() + end) + end +end diff --git a/test/support/conn_case.ex b/test/support/conn_case.ex index 064874201..781622476 100644 --- a/test/support/conn_case.ex +++ b/test/support/conn_case.ex @@ -51,6 +51,42 @@ defp oauth_access(scopes, opts \\ []) do %{user: user, token: token, conn: conn} end + defp json_response_and_validate_schema(conn, status \\ nil) do + content_type = + conn + |> Plug.Conn.get_resp_header("content-type") + |> List.first() + |> String.split(";") + |> List.first() + + status = status || conn.status + + %{private: %{open_api_spex: %{operation_id: op_id, operation_lookup: lookup, spec: spec}}} = + conn + + schema = lookup[op_id].responses[status].content[content_type].schema + json = json_response(conn, status) + + case OpenApiSpex.cast_value(json, schema, spec) do + {:ok, _data} -> + json + + {:error, errors} -> + errors = + Enum.map(errors, fn error -> + message = OpenApiSpex.Cast.Error.message(error) + path = OpenApiSpex.Cast.Error.path_to_string(error) + "#{message} at #{path}" + end) + + flunk( + "Response does not conform to schema of #{op_id} operation: #{ + Enum.join(errors, "\n") + }\n#{inspect(json)}" + ) + end + end + defp ensure_federating_or_authenticated(conn, url, user) do initial_setting = Config.get([:instance, :federating]) on_exit(fn -> Config.put([:instance, :federating], initial_setting) end) diff --git a/test/web/api_spec/app_operation_test.exs b/test/web/api_spec/app_operation_test.exs deleted file mode 100644 index 5b96abb44..000000000 --- a/test/web/api_spec/app_operation_test.exs +++ /dev/null @@ -1,45 +0,0 @@ -# Pleroma: A lightweight social networking server -# Copyright © 2017-2020 Pleroma Authors -# SPDX-License-Identifier: AGPL-3.0-only - -defmodule Pleroma.Web.ApiSpec.AppOperationTest do - use Pleroma.Web.ConnCase, async: true - - alias Pleroma.Web.ApiSpec - alias Pleroma.Web.ApiSpec.Schemas.AppCreateRequest - alias Pleroma.Web.ApiSpec.Schemas.AppCreateResponse - - import OpenApiSpex.TestAssertions - import Pleroma.Factory - - test "AppCreateRequest example matches schema" do - api_spec = ApiSpec.spec() - schema = AppCreateRequest.schema() - assert_schema(schema.example, "AppCreateRequest", api_spec) - end - - test "AppCreateResponse example matches schema" do - api_spec = ApiSpec.spec() - schema = AppCreateResponse.schema() - assert_schema(schema.example, "AppCreateResponse", api_spec) - end - - test "AppController produces a AppCreateResponse", %{conn: conn} do - api_spec = ApiSpec.spec() - app_attrs = build(:oauth_app) - - json = - conn - |> put_req_header("content-type", "application/json") - |> post( - "/api/v1/apps", - Jason.encode!(%{ - client_name: app_attrs.client_name, - redirect_uris: app_attrs.redirect_uris - }) - ) - |> json_response(200) - - assert_schema(json, "AppCreateResponse", api_spec) - end -end diff --git a/test/web/api_spec/schema_examples_test.exs b/test/web/api_spec/schema_examples_test.exs new file mode 100644 index 000000000..88b6f07cb --- /dev/null +++ b/test/web/api_spec/schema_examples_test.exs @@ -0,0 +1,43 @@ +# Pleroma: A lightweight social networking server +# Copyright © 2017-2020 Pleroma Authors +# SPDX-License-Identifier: AGPL-3.0-only + +defmodule Pleroma.Web.ApiSpec.SchemaExamplesTest do + use ExUnit.Case, async: true + import Pleroma.Tests.ApiSpecHelpers + + @content_type "application/json" + + for operation <- api_operations() do + describe operation.operationId <> " Request Body" do + if operation.requestBody do + @media_type operation.requestBody.content[@content_type] + @schema resolve_schema(@media_type.schema) + + if @media_type.example do + test "request body media type example matches schema" do + assert_schema(@media_type.example, @schema) + end + end + + if @schema.example do + test "request body schema example matches schema" do + assert_schema(@schema.example, @schema) + end + end + end + end + + for {status, response} <- operation.responses do + describe "#{operation.operationId} - #{status} Response" do + @schema resolve_schema(response.content[@content_type].schema) + + if @schema.example do + test "example matches schema" do + assert_schema(@schema.example, @schema) + end + end + end + end + end +end diff --git a/test/web/mastodon_api/controllers/app_controller_test.exs b/test/web/mastodon_api/controllers/app_controller_test.exs index e7b11d14e..a0b8b126c 100644 --- a/test/web/mastodon_api/controllers/app_controller_test.exs +++ b/test/web/mastodon_api/controllers/app_controller_test.exs @@ -27,7 +27,7 @@ test "apps/verify_credentials", %{conn: conn} do "vapid_key" => Push.vapid_config() |> Keyword.get(:public_key) } - assert expected == json_response(conn, 200) + assert expected == json_response_and_validate_schema(conn, 200) end test "creates an oauth app", %{conn: conn} do @@ -55,6 +55,6 @@ test "creates an oauth app", %{conn: conn} do "vapid_key" => Push.vapid_config() |> Keyword.get(:public_key) } - assert expected == json_response(conn, 200) + assert expected == json_response_and_validate_schema(conn, 200) end end diff --git a/test/web/mastodon_api/controllers/custom_emoji_controller_test.exs b/test/web/mastodon_api/controllers/custom_emoji_controller_test.exs index 0b2ffa470..4222556a4 100644 --- a/test/web/mastodon_api/controllers/custom_emoji_controller_test.exs +++ b/test/web/mastodon_api/controllers/custom_emoji_controller_test.exs @@ -5,15 +5,13 @@ defmodule Pleroma.Web.MastodonAPI.CustomEmojiControllerTest do use Pleroma.Web.ConnCase, async: true alias Pleroma.Web.ApiSpec - alias Pleroma.Web.ApiSpec.Schemas.CustomEmoji - alias Pleroma.Web.ApiSpec.Schemas.CustomEmojisResponse import OpenApiSpex.TestAssertions test "with tags", %{conn: conn} do assert resp = conn |> get("/api/v1/custom_emojis") - |> json_response(200) + |> json_response_and_validate_schema(200) assert [emoji | _body] = resp assert Map.has_key?(emoji, "shortcode") @@ -23,19 +21,6 @@ test "with tags", %{conn: conn} do assert Map.has_key?(emoji, "category") assert Map.has_key?(emoji, "url") assert Map.has_key?(emoji, "visible_in_picker") - assert_schema(resp, "CustomEmojisResponse", ApiSpec.spec()) assert_schema(emoji, "CustomEmoji", ApiSpec.spec()) end - - test "CustomEmoji example matches schema" do - api_spec = ApiSpec.spec() - schema = CustomEmoji.schema() - assert_schema(schema.example, "CustomEmoji", api_spec) - end - - test "CustomEmojisResponse example matches schema" do - api_spec = ApiSpec.spec() - schema = CustomEmojisResponse.schema() - assert_schema(schema.example, "CustomEmojisResponse", api_spec) - end end diff --git a/test/web/mastodon_api/controllers/domain_block_controller_test.exs b/test/web/mastodon_api/controllers/domain_block_controller_test.exs index d66190c90..01a24afcf 100644 --- a/test/web/mastodon_api/controllers/domain_block_controller_test.exs +++ b/test/web/mastodon_api/controllers/domain_block_controller_test.exs @@ -6,11 +6,8 @@ defmodule Pleroma.Web.MastodonAPI.DomainBlockControllerTest do use Pleroma.Web.ConnCase alias Pleroma.User - alias Pleroma.Web.ApiSpec - alias Pleroma.Web.ApiSpec.Schemas.DomainBlocksResponse import Pleroma.Factory - import OpenApiSpex.TestAssertions test "blocking / unblocking a domain" do %{user: user, conn: conn} = oauth_access(["write:blocks"]) @@ -21,7 +18,7 @@ test "blocking / unblocking a domain" do |> put_req_header("content-type", "application/json") |> post("/api/v1/domain_blocks", %{"domain" => "dogwhistle.zone"}) - assert %{} = json_response(ret_conn, 200) + assert %{} == json_response_and_validate_schema(ret_conn, 200) user = User.get_cached_by_ap_id(user.ap_id) assert User.blocks?(user, other_user) @@ -30,7 +27,7 @@ test "blocking / unblocking a domain" do |> put_req_header("content-type", "application/json") |> delete("/api/v1/domain_blocks", %{"domain" => "dogwhistle.zone"}) - assert %{} = json_response(ret_conn, 200) + assert %{} == json_response_and_validate_schema(ret_conn, 200) user = User.get_cached_by_ap_id(user.ap_id) refute User.blocks?(user, other_user) end @@ -41,21 +38,10 @@ test "getting a list of domain blocks" do {:ok, user} = User.block_domain(user, "bad.site") {:ok, user} = User.block_domain(user, "even.worse.site") - conn = - conn - |> assign(:user, user) - |> get("/api/v1/domain_blocks") - - domain_blocks = json_response(conn, 200) - - assert "bad.site" in domain_blocks - assert "even.worse.site" in domain_blocks - assert_schema(domain_blocks, "DomainBlocksResponse", ApiSpec.spec()) - end - - test "DomainBlocksResponse example matches schema" do - api_spec = ApiSpec.spec() - schema = DomainBlocksResponse.schema() - assert_schema(schema.example, "DomainBlocksResponse", api_spec) + assert ["even.worse.site", "bad.site"] == + conn + |> assign(:user, user) + |> get("/api/v1/domain_blocks") + |> json_response_and_validate_schema(200) end end From cb12585098e0cc1e2e85d253812e1898e8034b7f Mon Sep 17 00:00:00 2001 From: lain Date: Fri, 24 Apr 2020 14:37:53 +0200 Subject: [PATCH 02/18] Announcements: Prevent race condition. --- lib/pleroma/web/activity_pub/activity_pub.ex | 1 + test/web/common_api/common_api_test.exs | 27 ++++++++++++++++++++ 2 files changed, 28 insertions(+) diff --git a/lib/pleroma/web/activity_pub/activity_pub.ex b/lib/pleroma/web/activity_pub/activity_pub.ex index c67b3335d..4cce4f13c 100644 --- a/lib/pleroma/web/activity_pub/activity_pub.ex +++ b/lib/pleroma/web/activity_pub/activity_pub.ex @@ -438,6 +438,7 @@ def announce( defp do_announce(user, object, activity_id, local, public) do with true <- is_announceable?(object, user, public), + object <- Object.get_by_id(object.id), announce_data <- make_announce_data(user, object, activity_id, public), {:ok, activity} <- insert(announce_data, local), {:ok, object} <- add_announce_to_object(activity, object), diff --git a/test/web/common_api/common_api_test.exs b/test/web/common_api/common_api_test.exs index e87193c83..1758662b0 100644 --- a/test/web/common_api/common_api_test.exs +++ b/test/web/common_api/common_api_test.exs @@ -48,6 +48,33 @@ test "favoriting race condition" do assert object.data["like_count"] == 20 end + test "repeating race condition" do + user = insert(:user) + users_serial = insert_list(10, :user) + users = insert_list(10, :user) + + {:ok, activity} = CommonAPI.post(user, %{"status" => "."}) + + users_serial + |> Enum.map(fn user -> + CommonAPI.repeat(activity.id, user) + end) + + object = Object.get_by_ap_id(activity.data["object"]) + assert object.data["announcement_count"] == 10 + + users + |> Enum.map(fn user -> + Task.async(fn -> + CommonAPI.repeat(activity.id, user) + end) + end) + |> Enum.map(&Task.await/1) + + object = Object.get_by_ap_id(activity.data["object"]) + assert object.data["announcement_count"] == 20 + end + test "when replying to a conversation / participation, it will set the correct context id even if no explicit reply_to is given" do user = insert(:user) {:ok, activity} = CommonAPI.post(user, %{"status" => ".", "visibility" => "direct"}) From 6e625a427cdc829714ad0365560d79aa4ee9c2e5 Mon Sep 17 00:00:00 2001 From: Alexander Date: Wed, 4 Dec 2019 09:49:17 +0300 Subject: [PATCH 03/18] reply filtering --- CHANGELOG.md | 1 + benchmarks/load_testing/fetcher.ex | 53 ++ docs/API/differences_in_mastoapi_responses.md | 2 +- lib/pleroma/user.ex | 14 + lib/pleroma/user/query.ex | 4 +- lib/pleroma/web/activity_pub/activity_pub.ex | 77 ++- lib/pleroma/web/common_api/activity_draft.ex | 16 +- .../controllers/timeline_controller.ex | 1 + test/web/activity_pub/activity_pub_test.exs | 486 ++++++++++++++++++ 9 files changed, 636 insertions(+), 18 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 702c58180..affabcd95 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,6 +19,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/). API Changes - Mastodon API: Support for `include_types` in `/api/v1/notifications`. - Mastodon API: Added `/api/v1/notifications/:id/dismiss` endpoint. +- Mastodon API: Add support for filtering replies in public and friends timelines - Admin API: endpoints for create/update/delete OAuth Apps. diff --git a/benchmarks/load_testing/fetcher.ex b/benchmarks/load_testing/fetcher.ex index 786929ace..3aa82b48a 100644 --- a/benchmarks/load_testing/fetcher.ex +++ b/benchmarks/load_testing/fetcher.ex @@ -495,4 +495,57 @@ defp render_long_thread(user) do formatters: formatters() ) end + + def query_replies(user) do + public_params = %{ + "type" => ["Create", "Announce"], + "local_only" => false, + "blocking_user" => user, + "muting_user" => user, + "count" => 20 + } + + Benchee.run(%{ + "Public timeline without reply filtering" => fn -> + ActivityPub.fetch_public_activities(public_params) + end, + "Public timeline with reply filtering - following" => fn -> + public_params + |> Map.put("reply_visibility", "following") + |> Map.put("user", user) + |> ActivityPub.fetch_public_activities() + end, + "Public timeline with reply filtering - self" => fn -> + public_params + |> Map.put("reply_visibility", "self") + |> Map.put("user", user) + |> ActivityPub.fetch_public_activities() + end + }) + + private_params = %{ + "type" => ["Create", "Announce"], + "blocking_user" => user, + "muting_user" => user, + "user" => user, + "count" => 20 + } + + recipients = [user.ap_id | User.following(user)] + + Benchee.run(%{ + "Home timeline without reply filtering" => fn -> + ActivityPub.fetch_activities(recipients, private_params) + end, + "Home timeline with reply filtering - following" => fn -> + private_params = Map.put(private_params, "reply_visibility", "following") + + ActivityPub.fetch_activities(recipients, private_params) + end, + "Home timeline with reply filtering - self" => fn -> + private_params = Map.put(private_params, "reply_visibility", "self") + ActivityPub.fetch_activities(recipients, private_params) + end + }) + end end diff --git a/docs/API/differences_in_mastoapi_responses.md b/docs/API/differences_in_mastoapi_responses.md index 1059155cf..c97fb8c56 100644 --- a/docs/API/differences_in_mastoapi_responses.md +++ b/docs/API/differences_in_mastoapi_responses.md @@ -14,7 +14,7 @@ Some apps operate under the assumption that no more than 4 attachments can be re Adding the parameter `with_muted=true` to the timeline queries will also return activities by muted (not by blocked!) users. Adding the parameter `exclude_visibilities` to the timeline queries will exclude the statuses with the given visibilities. The parameter accepts an array of visibility types (`public`, `unlisted`, `private`, `direct`), e.g., `exclude_visibilities[]=direct&exclude_visibilities[]=private`. - +Adding the parameter `reply_visibility` to the public and friends timelines quieries will filter replies. Possible values: without parameter (default) shows all replies, `following` - replies directed to you or users you follow, `self` - replies directed to you. ## Statuses - `visibility`: has an additional possible value `list` diff --git a/lib/pleroma/user.ex b/lib/pleroma/user.ex index 477237756..b451202b2 100644 --- a/lib/pleroma/user.ex +++ b/lib/pleroma/user.ex @@ -832,6 +832,7 @@ def set_cache({:error, err}), do: {:error, err} def set_cache(%User{} = user) do Cachex.put(:user_cache, "ap_id:#{user.ap_id}", user) Cachex.put(:user_cache, "nickname:#{user.nickname}", user) + Cachex.put(:user_cache, "friends_ap_ids:#{user.nickname}", get_user_friends_ap_ids(user)) {:ok, user} end @@ -847,9 +848,22 @@ def update_and_set_cache(changeset) do end end + def get_user_friends_ap_ids(user) do + from(u in User.get_friends_query(user), select: u.ap_id) + |> Repo.all() + end + + @spec get_cached_user_friends_ap_ids(User.t()) :: [String.t()] + def get_cached_user_friends_ap_ids(user) do + Cachex.fetch!(:user_cache, "friends_ap_ids:#{user.ap_id}", fn _ -> + get_user_friends_ap_ids(user) + end) + end + def invalidate_cache(user) do Cachex.del(:user_cache, "ap_id:#{user.ap_id}") Cachex.del(:user_cache, "nickname:#{user.nickname}") + Cachex.del(:user_cache, "friends_ap_ids:#{user.ap_id}") end @spec get_cached_by_ap_id(String.t()) :: User.t() | nil diff --git a/lib/pleroma/user/query.ex b/lib/pleroma/user/query.ex index ec88088cf..ac77aab71 100644 --- a/lib/pleroma/user/query.ex +++ b/lib/pleroma/user/query.ex @@ -54,13 +54,13 @@ defmodule Pleroma.User.Query do select: term(), limit: pos_integer() } - | %{} + | map() @ilike_criteria [:nickname, :name, :query] @equal_criteria [:email] @contains_criteria [:ap_id, :nickname] - @spec build(criteria()) :: Query.t() + @spec build(Query.t(), criteria()) :: Query.t() def build(query \\ base_query(), criteria) do prepare_query(query, criteria) end diff --git a/lib/pleroma/web/activity_pub/activity_pub.ex b/lib/pleroma/web/activity_pub/activity_pub.ex index c67b3335d..8b170b7f8 100644 --- a/lib/pleroma/web/activity_pub/activity_pub.ex +++ b/lib/pleroma/web/activity_pub/activity_pub.ex @@ -270,9 +270,9 @@ defp do_create(%{to: to, actor: actor, context: context, object: object} = param ), {:ok, activity} <- insert(create_data, local, fake), {:fake, false, activity} <- {:fake, fake, activity}, + {:quick_insert, false, activity} <- {:quick_insert, quick_insert?, activity}, _ <- increase_replies_count_if_reply(create_data), _ <- increase_poll_votes_if_vote(create_data), - {:quick_insert, false, activity} <- {:quick_insert, quick_insert?, activity}, {:ok, _actor} <- increase_note_count_if_public(actor, activity), :ok <- maybe_federate(activity) do {:ok, activity} @@ -700,12 +700,14 @@ def fetch_activities_for_context_query(context, opts) do do: [opts["user"].ap_id | User.following(opts["user"])] ++ public, else: public + opts = Map.put(opts, "user", opts["user"]) + from(activity in Activity) |> maybe_preload_objects(opts) |> maybe_preload_bookmarks(opts) |> maybe_set_thread_muted_field(opts) |> restrict_blocked(opts) - |> restrict_recipients(recipients, opts["user"]) + |> restrict_recipients(recipients, opts) |> where( [activity], fragment( @@ -740,7 +742,10 @@ def fetch_latest_activity_id_for_context(context, opts \\ %{}) do @spec fetch_public_activities(map(), Pagination.type()) :: [Activity.t()] def fetch_public_activities(opts \\ %{}, pagination \\ :keyset) do - opts = Map.drop(opts, ["user"]) + opts = + opts + |> Map.put("reply_user", opts["user"]) + |> Map.delete("user") [Constants.as_public()] |> fetch_activities_query(opts) @@ -976,13 +981,65 @@ defp restrict_tag(query, %{"tag" => tag}) when is_binary(tag) do defp restrict_tag(query, _), do: query - defp restrict_recipients(query, [], _user), do: query - - defp restrict_recipients(query, recipients, nil) do - from(activity in query, where: fragment("? && ?", ^recipients, activity.recipients)) + defp reply_recipients(user, "following") do + [user.ap_id | User.get_cached_user_friends_ap_ids(user)] end - defp restrict_recipients(query, recipients, user) do + defp reply_recipients(user, "self"), do: [user.ap_id] + + defp restrict_recipients(query, [], _opts), do: query + + defp restrict_recipients( + query, + recipients, + %{"user" => nil, "reply_user" => user, "reply_visibility" => visibility} + ) + when not is_nil(user) and visibility in ["following", "self"] do + reply_recipients = reply_recipients(user, visibility) + + from([activity, object] in query, + where: + fragment( + "? && ? AND (?->>'inReplyTo' IS NULL OR array_remove(?, ?) && ? OR ? = ?)", + ^recipients, + activity.recipients, + object.data, + activity.recipients, + activity.actor, + ^reply_recipients, + activity.actor, + ^user.ap_id + ) + ) + end + + defp restrict_recipients(query, recipients, %{"user" => nil}) do + from(activity in query, + where: fragment("? && ?", ^recipients, activity.recipients) + ) + end + + defp restrict_recipients(query, recipients, %{"user" => user, "reply_visibility" => visibility}) + when visibility in ["following", "self"] do + reply_recipients = reply_recipients(user, visibility) + + from( + [activity, object] in query, + where: + fragment( + "? && ? AND (?->>'inReplyTo' IS NULL OR array_remove(?, ?) && ?)", + ^recipients, + activity.recipients, + object.data, + activity.recipients, + activity.actor, + ^reply_recipients + ), + or_where: activity.actor == ^user.ap_id + ) + end + + defp restrict_recipients(query, recipients, %{"user" => user}) do from( activity in query, where: fragment("? && ?", ^recipients, activity.recipients), @@ -1254,13 +1311,15 @@ def fetch_activities_query(recipients, opts \\ %{}) do skip_thread_containment: Config.get([:instance, :skip_thread_containment]) } + opts = Map.put(opts, "user", opts["user"]) + Activity |> maybe_preload_objects(opts) |> maybe_preload_bookmarks(opts) |> maybe_preload_report_notes(opts) |> maybe_set_thread_muted_field(opts) |> maybe_order(opts) - |> restrict_recipients(recipients, opts["user"]) + |> restrict_recipients(recipients, opts) |> restrict_tag(opts) |> restrict_tag_reject(opts) |> restrict_tag_all(opts) diff --git a/lib/pleroma/web/common_api/activity_draft.ex b/lib/pleroma/web/common_api/activity_draft.ex index c1cd15bb2..244cf2be5 100644 --- a/lib/pleroma/web/common_api/activity_draft.ex +++ b/lib/pleroma/web/common_api/activity_draft.ex @@ -84,14 +84,18 @@ defp attachments(%{params: params} = draft) do %__MODULE__{draft | attachments: attachments} end - defp in_reply_to(draft) do - case Map.get(draft.params, "in_reply_to_status_id") do - "" -> draft - nil -> draft - id -> %__MODULE__{draft | in_reply_to: Activity.get_by_id(id)} - end + defp in_reply_to(%{params: %{"in_reply_to_status_id" => ""}} = draft), do: draft + + defp in_reply_to(%{params: %{"in_reply_to_status_id" => id}} = draft) when is_binary(id) do + %__MODULE__{draft | in_reply_to: Activity.get_by_id(id)} end + defp in_reply_to(%{params: %{"in_reply_to_status_id" => %Activity{} = in_reply_to}} = draft) do + %__MODULE__{draft | in_reply_to: in_reply_to} + end + + defp in_reply_to(draft), do: draft + defp in_reply_to_conversation(draft) do in_reply_to_conversation = Participation.get(draft.params["in_reply_to_conversation_id"]) %__MODULE__{draft | in_reply_to_conversation: in_reply_to_conversation} diff --git a/lib/pleroma/web/mastodon_api/controllers/timeline_controller.ex b/lib/pleroma/web/mastodon_api/controllers/timeline_controller.ex index b3c58005e..a2ac9301e 100644 --- a/lib/pleroma/web/mastodon_api/controllers/timeline_controller.ex +++ b/lib/pleroma/web/mastodon_api/controllers/timeline_controller.ex @@ -100,6 +100,7 @@ def public(%{assigns: %{user: user}} = conn, params) do |> Map.put("local_only", local_only) |> Map.put("blocking_user", user) |> Map.put("muting_user", user) + |> Map.put("user", user) |> ActivityPub.fetch_public_activities() conn diff --git a/test/web/activity_pub/activity_pub_test.exs b/test/web/activity_pub/activity_pub_test.exs index 53176917e..8a1638a23 100644 --- a/test/web/activity_pub/activity_pub_test.exs +++ b/test/web/activity_pub/activity_pub_test.exs @@ -1910,4 +1910,490 @@ test "old user must be in the new user's `also_known_as` list" do ActivityPub.move(old_user, new_user) end end + + test "doesn't retrieve replies activities with exclude_replies" do + user = insert(:user) + + {:ok, activity} = CommonAPI.post(user, %{"status" => "yeah"}) + + {:ok, _reply} = + CommonAPI.post(user, %{"status" => "yeah", "in_reply_to_status_id" => activity.id}) + + [result] = ActivityPub.fetch_public_activities(%{"exclude_replies" => "true"}) + + assert result.id == activity.id + + assert length(ActivityPub.fetch_public_activities()) == 2 + end + + describe "replies filtering with public messages" do + setup :public_messages + + test "public timeline", %{users: %{u1: user}} do + activities_ids = + %{} + |> Map.put("type", ["Create", "Announce"]) + |> Map.put("local_only", false) + |> Map.put("blocking_user", user) + |> Map.put("muting_user", user) + |> Map.put("user", user) + |> ActivityPub.fetch_public_activities() + |> Enum.map(& &1.id) + + assert length(activities_ids) == 16 + end + + test "public timeline with reply_visibility `following`", %{ + users: %{u1: user}, + u1: u1, + u2: u2, + u3: u3, + u4: u4, + activities: activities + } do + activities_ids = + %{} + |> Map.put("type", ["Create", "Announce"]) + |> Map.put("local_only", false) + |> Map.put("blocking_user", user) + |> Map.put("muting_user", user) + |> Map.put("reply_visibility", "following") + |> Map.put("user", user) + |> ActivityPub.fetch_public_activities() + |> Enum.map(& &1.id) + + assert length(activities_ids) == 14 + + visible_ids = + Map.values(u1) ++ Map.values(u2) ++ Map.values(u4) ++ Map.values(activities) ++ [u3[:r1]] + + assert Enum.all?(visible_ids, &(&1 in activities_ids)) + end + + test "public timeline with reply_visibility `self`", %{ + users: %{u1: user}, + u1: u1, + u2: u2, + u3: u3, + u4: u4, + activities: activities + } do + activities_ids = + %{} + |> Map.put("type", ["Create", "Announce"]) + |> Map.put("local_only", false) + |> Map.put("blocking_user", user) + |> Map.put("muting_user", user) + |> Map.put("reply_visibility", "self") + |> Map.put("user", user) + |> ActivityPub.fetch_public_activities() + |> Enum.map(& &1.id) + + assert length(activities_ids) == 10 + visible_ids = Map.values(u1) ++ [u2[:r1], u3[:r1], u4[:r1]] ++ Map.values(activities) + assert Enum.all?(visible_ids, &(&1 in activities_ids)) + end + + test "home timeline", %{ + users: %{u1: user}, + activities: activities, + u1: u1, + u2: u2, + u3: u3, + u4: u4 + } do + params = + %{} + |> Map.put("type", ["Create", "Announce"]) + |> Map.put("blocking_user", user) + |> Map.put("muting_user", user) + |> Map.put("user", user) + + activities_ids = + ActivityPub.fetch_activities([user.ap_id | User.following(user)], params) + |> Enum.map(& &1.id) + + assert length(activities_ids) == 13 + + visible_ids = + Map.values(u1) ++ + Map.values(u3) ++ + [ + activities[:a1], + activities[:a2], + activities[:a4], + u2[:r1], + u2[:r3], + u4[:r1], + u4[:r2] + ] + + assert Enum.all?(visible_ids, &(&1 in activities_ids)) + end + + test "home timeline with reply_visibility `following`", %{ + users: %{u1: user}, + activities: activities, + u1: u1, + u2: u2, + u3: u3, + u4: u4 + } do + params = + %{} + |> Map.put("type", ["Create", "Announce"]) + |> Map.put("blocking_user", user) + |> Map.put("muting_user", user) + |> Map.put("user", user) + |> Map.put("reply_visibility", "following") + + activities_ids = + ActivityPub.fetch_activities([user.ap_id | User.following(user)], params) + |> Enum.map(& &1.id) + + assert length(activities_ids) == 11 + + visible_ids = + Map.values(u1) ++ + [ + activities[:a1], + activities[:a2], + activities[:a4], + u2[:r1], + u2[:r3], + u3[:r1], + u4[:r1], + u4[:r2] + ] + + assert Enum.all?(visible_ids, &(&1 in activities_ids)) + end + + test "home timeline with reply_visibility `self`", %{ + users: %{u1: user}, + activities: activities, + u1: u1, + u2: u2, + u3: u3, + u4: u4 + } do + params = + %{} + |> Map.put("type", ["Create", "Announce"]) + |> Map.put("blocking_user", user) + |> Map.put("muting_user", user) + |> Map.put("user", user) + |> Map.put("reply_visibility", "self") + + activities_ids = + ActivityPub.fetch_activities([user.ap_id | User.following(user)], params) + |> Enum.map(& &1.id) + + assert length(activities_ids) == 9 + + visible_ids = + Map.values(u1) ++ + [ + activities[:a1], + activities[:a2], + activities[:a4], + u2[:r1], + u3[:r1], + u4[:r1] + ] + + assert Enum.all?(visible_ids, &(&1 in activities_ids)) + end + end + + describe "replies filtering with private messages" do + setup :private_messages + + test "public timeline", %{users: %{u1: user}} do + activities_ids = + %{} + |> Map.put("type", ["Create", "Announce"]) + |> Map.put("local_only", false) + |> Map.put("blocking_user", user) + |> Map.put("muting_user", user) + |> Map.put("user", user) + |> ActivityPub.fetch_public_activities() + |> Enum.map(& &1.id) + + assert activities_ids == [] + end + + test "public timeline with default reply_visibility `following`", %{users: %{u1: user}} do + activities_ids = + %{} + |> Map.put("type", ["Create", "Announce"]) + |> Map.put("local_only", false) + |> Map.put("blocking_user", user) + |> Map.put("muting_user", user) + |> Map.put("reply_visibility", "following") + |> Map.put("user", user) + |> ActivityPub.fetch_public_activities() + |> Enum.map(& &1.id) + + assert activities_ids == [] + end + + test "public timeline with default reply_visibility `self`", %{users: %{u1: user}} do + activities_ids = + %{} + |> Map.put("type", ["Create", "Announce"]) + |> Map.put("local_only", false) + |> Map.put("blocking_user", user) + |> Map.put("muting_user", user) + |> Map.put("reply_visibility", "self") + |> Map.put("user", user) + |> ActivityPub.fetch_public_activities() + |> Enum.map(& &1.id) + + assert activities_ids == [] + end + + test "home timeline", %{users: %{u1: user}} do + params = + %{} + |> Map.put("type", ["Create", "Announce"]) + |> Map.put("blocking_user", user) + |> Map.put("muting_user", user) + |> Map.put("user", user) + + activities_ids = + ActivityPub.fetch_activities([user.ap_id | User.following(user)], params) + |> Enum.map(& &1.id) + + assert length(activities_ids) == 12 + end + + test "home timeline with default reply_visibility `following`", %{users: %{u1: user}} do + params = + %{} + |> Map.put("type", ["Create", "Announce"]) + |> Map.put("blocking_user", user) + |> Map.put("muting_user", user) + |> Map.put("user", user) + |> Map.put("reply_visibility", "following") + + activities_ids = + ActivityPub.fetch_activities([user.ap_id | User.following(user)], params) + |> Enum.map(& &1.id) + + assert length(activities_ids) == 12 + end + + test "home timeline with default reply_visibility `self`", %{ + users: %{u1: user}, + activities: activities, + u1: u1, + u2: u2, + u3: u3, + u4: u4 + } do + params = + %{} + |> Map.put("type", ["Create", "Announce"]) + |> Map.put("blocking_user", user) + |> Map.put("muting_user", user) + |> Map.put("user", user) + |> Map.put("reply_visibility", "self") + + activities_ids = + ActivityPub.fetch_activities([user.ap_id | User.following(user)], params) + |> Enum.map(& &1.id) + + assert length(activities_ids) == 10 + + visible_ids = + Map.values(u1) ++ Map.values(u4) ++ [u2[:r1], u3[:r1]] ++ Map.values(activities) + + assert Enum.all?(visible_ids, &(&1 in activities_ids)) + end + end + + defp public_messages(_) do + [u1, u2, u3, u4] = insert_list(4, :user) + {:ok, u1} = User.follow(u1, u2) + {:ok, u2} = User.follow(u2, u1) + {:ok, u1} = User.follow(u1, u4) + {:ok, u4} = User.follow(u4, u1) + + {:ok, u2} = User.follow(u2, u3) + {:ok, u3} = User.follow(u3, u2) + + {:ok, a1} = CommonAPI.post(u1, %{"status" => "Status"}) + + {:ok, r1_1} = + CommonAPI.post(u2, %{ + "status" => "@#{u1.nickname} reply from u2 to u1", + "in_reply_to_status_id" => a1.id + }) + + {:ok, r1_2} = + CommonAPI.post(u3, %{ + "status" => "@#{u1.nickname} reply from u3 to u1", + "in_reply_to_status_id" => a1.id + }) + + {:ok, r1_3} = + CommonAPI.post(u4, %{ + "status" => "@#{u1.nickname} reply from u4 to u1", + "in_reply_to_status_id" => a1.id + }) + + {:ok, a2} = CommonAPI.post(u2, %{"status" => "Status"}) + + {:ok, r2_1} = + CommonAPI.post(u1, %{ + "status" => "@#{u2.nickname} reply from u1 to u2", + "in_reply_to_status_id" => a2.id + }) + + {:ok, r2_2} = + CommonAPI.post(u3, %{ + "status" => "@#{u2.nickname} reply from u3 to u2", + "in_reply_to_status_id" => a2.id + }) + + {:ok, r2_3} = + CommonAPI.post(u4, %{ + "status" => "@#{u2.nickname} reply from u4 to u2", + "in_reply_to_status_id" => a2.id + }) + + {:ok, a3} = CommonAPI.post(u3, %{"status" => "Status"}) + + {:ok, r3_1} = + CommonAPI.post(u1, %{ + "status" => "@#{u3.nickname} reply from u1 to u3", + "in_reply_to_status_id" => a3.id + }) + + {:ok, r3_2} = + CommonAPI.post(u2, %{ + "status" => "@#{u3.nickname} reply from u2 to u3", + "in_reply_to_status_id" => a3.id + }) + + {:ok, r3_3} = + CommonAPI.post(u4, %{ + "status" => "@#{u3.nickname} reply from u4 to u3", + "in_reply_to_status_id" => a3.id + }) + + {:ok, a4} = CommonAPI.post(u4, %{"status" => "Status"}) + + {:ok, r4_1} = + CommonAPI.post(u1, %{ + "status" => "@#{u4.nickname} reply from u1 to u4", + "in_reply_to_status_id" => a4.id + }) + + {:ok, r4_2} = + CommonAPI.post(u2, %{ + "status" => "@#{u4.nickname} reply from u2 to u4", + "in_reply_to_status_id" => a4.id + }) + + {:ok, r4_3} = + CommonAPI.post(u3, %{ + "status" => "@#{u4.nickname} reply from u3 to u4", + "in_reply_to_status_id" => a4.id + }) + + {:ok, + users: %{u1: u1, u2: u2, u3: u3, u4: u4}, + activities: %{a1: a1.id, a2: a2.id, a3: a3.id, a4: a4.id}, + u1: %{r1: r1_1.id, r2: r1_2.id, r3: r1_3.id}, + u2: %{r1: r2_1.id, r2: r2_2.id, r3: r2_3.id}, + u3: %{r1: r3_1.id, r2: r3_2.id, r3: r3_3.id}, + u4: %{r1: r4_1.id, r2: r4_2.id, r3: r4_3.id}} + end + + defp private_messages(_) do + [u1, u2, u3, u4] = insert_list(4, :user) + {:ok, u1} = User.follow(u1, u2) + {:ok, u2} = User.follow(u2, u1) + {:ok, u1} = User.follow(u1, u3) + {:ok, u3} = User.follow(u3, u1) + {:ok, u1} = User.follow(u1, u4) + {:ok, u4} = User.follow(u4, u1) + + {:ok, u2} = User.follow(u2, u3) + {:ok, u3} = User.follow(u3, u2) + + {:ok, a1} = CommonAPI.post(u1, %{"status" => "Status", "visibility" => "private"}) + + {:ok, r1_1} = + CommonAPI.post(u2, %{ + "status" => "@#{u1.nickname} reply from u2 to u1", + "in_reply_to_status_id" => a1.id, + "visibility" => "private" + }) + + {:ok, r1_2} = + CommonAPI.post(u3, %{ + "status" => "@#{u1.nickname} reply from u3 to u1", + "in_reply_to_status_id" => a1.id, + "visibility" => "private" + }) + + {:ok, r1_3} = + CommonAPI.post(u4, %{ + "status" => "@#{u1.nickname} reply from u4 to u1", + "in_reply_to_status_id" => a1.id, + "visibility" => "private" + }) + + {:ok, a2} = CommonAPI.post(u2, %{"status" => "Status", "visibility" => "private"}) + + {:ok, r2_1} = + CommonAPI.post(u1, %{ + "status" => "@#{u2.nickname} reply from u1 to u2", + "in_reply_to_status_id" => a2.id, + "visibility" => "private" + }) + + {:ok, r2_2} = + CommonAPI.post(u3, %{ + "status" => "@#{u2.nickname} reply from u3 to u2", + "in_reply_to_status_id" => a2.id, + "visibility" => "private" + }) + + {:ok, a3} = CommonAPI.post(u3, %{"status" => "Status", "visibility" => "private"}) + + {:ok, r3_1} = + CommonAPI.post(u1, %{ + "status" => "@#{u3.nickname} reply from u1 to u3", + "in_reply_to_status_id" => a3.id, + "visibility" => "private" + }) + + {:ok, r3_2} = + CommonAPI.post(u2, %{ + "status" => "@#{u3.nickname} reply from u2 to u3", + "in_reply_to_status_id" => a3.id, + "visibility" => "private" + }) + + {:ok, a4} = CommonAPI.post(u4, %{"status" => "Status", "visibility" => "private"}) + + {:ok, r4_1} = + CommonAPI.post(u1, %{ + "status" => "@#{u4.nickname} reply from u1 to u4", + "in_reply_to_status_id" => a4.id, + "visibility" => "private" + }) + + {:ok, + users: %{u1: u1, u2: u2, u3: u3, u4: u4}, + activities: %{a1: a1.id, a2: a2.id, a3: a3.id, a4: a4.id}, + u1: %{r1: r1_1.id, r2: r1_2.id, r3: r1_3.id}, + u2: %{r1: r2_1.id, r2: r2_2.id}, + u3: %{r1: r3_1.id, r2: r3_2.id}, + u4: %{r1: r4_1.id}} + end end From be34672d6768bdc9ece96669e07e940a98c9d933 Mon Sep 17 00:00:00 2001 From: Alexander Date: Wed, 4 Dec 2019 10:29:26 +0300 Subject: [PATCH 04/18] formatting --- docs/API/differences_in_mastoapi_responses.md | 1 + 1 file changed, 1 insertion(+) diff --git a/docs/API/differences_in_mastoapi_responses.md b/docs/API/differences_in_mastoapi_responses.md index c97fb8c56..92086136f 100644 --- a/docs/API/differences_in_mastoapi_responses.md +++ b/docs/API/differences_in_mastoapi_responses.md @@ -15,6 +15,7 @@ Some apps operate under the assumption that no more than 4 attachments can be re Adding the parameter `with_muted=true` to the timeline queries will also return activities by muted (not by blocked!) users. Adding the parameter `exclude_visibilities` to the timeline queries will exclude the statuses with the given visibilities. The parameter accepts an array of visibility types (`public`, `unlisted`, `private`, `direct`), e.g., `exclude_visibilities[]=direct&exclude_visibilities[]=private`. Adding the parameter `reply_visibility` to the public and friends timelines quieries will filter replies. Possible values: without parameter (default) shows all replies, `following` - replies directed to you or users you follow, `self` - replies directed to you. + ## Statuses - `visibility`: has an additional possible value `list` From 1a75ef63b2f6fef96b9bf9d07b4963fb217d4017 Mon Sep 17 00:00:00 2001 From: Alexander Strizhakov Date: Fri, 24 Apr 2020 09:24:08 +0300 Subject: [PATCH 05/18] updating benchmarks --- benchmarks/load_testing/activities.ex | 2 +- benchmarks/load_testing/fetcher.ex | 85 +++++++++++++-------------- 2 files changed, 41 insertions(+), 46 deletions(-) diff --git a/benchmarks/load_testing/activities.ex b/benchmarks/load_testing/activities.ex index 23ee2b987..2b032943b 100644 --- a/benchmarks/load_testing/activities.ex +++ b/benchmarks/load_testing/activities.ex @@ -313,7 +313,7 @@ defp insert_activity("simple_thread", visibility, group, user, friends, non_frie tasks = get_reply_tasks(visibility, group) {:ok, activity} = - CommonAPI.post(user, %{"status" => "Simple status", "visibility" => "unlisted"}) + CommonAPI.post(user, %{"status" => "Simple status", "visibility" => visibility}) acc = {activity.id, ["@" <> actor.nickname, "reply to status"]} insert_replies(tasks, visibility, user, friends, non_friends, acc) diff --git a/benchmarks/load_testing/fetcher.ex b/benchmarks/load_testing/fetcher.ex index 3aa82b48a..6503deb41 100644 --- a/benchmarks/load_testing/fetcher.ex +++ b/benchmarks/load_testing/fetcher.ex @@ -41,6 +41,7 @@ defp fetch_timelines(user) do fetch_notifications(user) fetch_favourites(user) fetch_long_thread(user) + fetch_timelines_with_reply_filtering(user) end defp render_views(user) do @@ -496,56 +497,50 @@ defp render_long_thread(user) do ) end - def query_replies(user) do - public_params = %{ - "type" => ["Create", "Announce"], - "local_only" => false, - "blocking_user" => user, - "muting_user" => user, - "count" => 20 - } + defp fetch_timelines_with_reply_filtering(user) do + public_params = opts_for_public_timeline(user) - Benchee.run(%{ - "Public timeline without reply filtering" => fn -> - ActivityPub.fetch_public_activities(public_params) - end, - "Public timeline with reply filtering - following" => fn -> - public_params - |> Map.put("reply_visibility", "following") - |> Map.put("user", user) - |> ActivityPub.fetch_public_activities() - end, - "Public timeline with reply filtering - self" => fn -> - public_params - |> Map.put("reply_visibility", "self") - |> Map.put("user", user) - |> ActivityPub.fetch_public_activities() - end - }) + Benchee.run( + %{ + "Public timeline without reply filtering" => fn -> + ActivityPub.fetch_public_activities(public_params) + end, + "Public timeline with reply filtering - following" => fn -> + public_params + |> Map.put("reply_visibility", "following") + |> Map.put("user", user) + |> ActivityPub.fetch_public_activities() + end, + "Public timeline with reply filtering - self" => fn -> + public_params + |> Map.put("reply_visibility", "self") + |> Map.put("user", user) + |> ActivityPub.fetch_public_activities() + end + }, + formatters: formatters() + ) - private_params = %{ - "type" => ["Create", "Announce"], - "blocking_user" => user, - "muting_user" => user, - "user" => user, - "count" => 20 - } + private_params = opts_for_home_timeline(user) recipients = [user.ap_id | User.following(user)] - Benchee.run(%{ - "Home timeline without reply filtering" => fn -> - ActivityPub.fetch_activities(recipients, private_params) - end, - "Home timeline with reply filtering - following" => fn -> - private_params = Map.put(private_params, "reply_visibility", "following") + Benchee.run( + %{ + "Home timeline without reply filtering" => fn -> + ActivityPub.fetch_activities(recipients, private_params) + end, + "Home timeline with reply filtering - following" => fn -> + private_params = Map.put(private_params, "reply_visibility", "following") - ActivityPub.fetch_activities(recipients, private_params) - end, - "Home timeline with reply filtering - self" => fn -> - private_params = Map.put(private_params, "reply_visibility", "self") - ActivityPub.fetch_activities(recipients, private_params) - end - }) + ActivityPub.fetch_activities(recipients, private_params) + end, + "Home timeline with reply filtering - self" => fn -> + private_params = Map.put(private_params, "reply_visibility", "self") + ActivityPub.fetch_activities(recipients, private_params) + end + }, + formatters: formatters() + ) end end From 375ab05234a3590c161a2a5a4f715fbc61dafb34 Mon Sep 17 00:00:00 2001 From: Alexander Strizhakov Date: Fri, 24 Apr 2020 09:57:30 +0300 Subject: [PATCH 06/18] bench sync --- benchmarks/load_testing/activities.ex | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/benchmarks/load_testing/activities.ex b/benchmarks/load_testing/activities.ex index 2b032943b..482e42fc1 100644 --- a/benchmarks/load_testing/activities.ex +++ b/benchmarks/load_testing/activities.ex @@ -279,7 +279,7 @@ defp insert_activity("like", visibility, group, user, friends, non_friends, opts actor = get_actor(group, user, friends, non_friends) with activity_id when not is_nil(activity_id) <- get_random_create_activity_id(), - {:ok, _activity, _object} <- CommonAPI.favorite(activity_id, actor) do + {:ok, _activity} <- CommonAPI.favorite(actor, activity_id) do :ok else {:error, _} -> From 8480f84615b696965d3c1ca34b5847af99fbdece Mon Sep 17 00:00:00 2001 From: lain Date: Fri, 24 Apr 2020 10:26:54 +0000 Subject: [PATCH 07/18] Update differences_in_mastoapi_responses.md --- docs/API/differences_in_mastoapi_responses.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/API/differences_in_mastoapi_responses.md b/docs/API/differences_in_mastoapi_responses.md index 92086136f..41ceda26b 100644 --- a/docs/API/differences_in_mastoapi_responses.md +++ b/docs/API/differences_in_mastoapi_responses.md @@ -14,7 +14,7 @@ Some apps operate under the assumption that no more than 4 attachments can be re Adding the parameter `with_muted=true` to the timeline queries will also return activities by muted (not by blocked!) users. Adding the parameter `exclude_visibilities` to the timeline queries will exclude the statuses with the given visibilities. The parameter accepts an array of visibility types (`public`, `unlisted`, `private`, `direct`), e.g., `exclude_visibilities[]=direct&exclude_visibilities[]=private`. -Adding the parameter `reply_visibility` to the public and friends timelines quieries will filter replies. Possible values: without parameter (default) shows all replies, `following` - replies directed to you or users you follow, `self` - replies directed to you. +Adding the parameter `reply_visibility` to the public and home timelines queries will filter replies. Possible values: without parameter (default) shows all replies, `following` - replies directed to you or users you follow, `self` - replies directed to you. ## Statuses From e2f3030c868ca087915294909dee304f7de1730f Mon Sep 17 00:00:00 2001 From: lain Date: Fri, 24 Apr 2020 10:27:51 +0000 Subject: [PATCH 08/18] Apply suggestion to CHANGELOG.md --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index affabcd95..ccc6a5bd4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,7 +19,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/). API Changes - Mastodon API: Support for `include_types` in `/api/v1/notifications`. - Mastodon API: Added `/api/v1/notifications/:id/dismiss` endpoint. -- Mastodon API: Add support for filtering replies in public and friends timelines +- Mastodon API: Add support for filtering replies in public and home timelines - Admin API: endpoints for create/update/delete OAuth Apps. From d89cd0a19733eec27b79b768df2e30a68bfc6d6b Mon Sep 17 00:00:00 2001 From: lain Date: Fri, 24 Apr 2020 18:25:26 +0200 Subject: [PATCH 09/18] Reply Filtering: Refactor. --- benchmarks/load_testing/fetcher.ex | 15 ++- benchmarks/mix/tasks/pleroma/load_testing.ex | 1 + lib/pleroma/web/activity_pub/activity_pub.ex | 114 +++++++----------- .../controllers/timeline_controller.ex | 3 +- test/web/activity_pub/activity_pub_test.exs | 13 +- 5 files changed, 69 insertions(+), 77 deletions(-) diff --git a/benchmarks/load_testing/fetcher.ex b/benchmarks/load_testing/fetcher.ex index 6503deb41..12c30f6f5 100644 --- a/benchmarks/load_testing/fetcher.ex +++ b/benchmarks/load_testing/fetcher.ex @@ -508,13 +508,13 @@ defp fetch_timelines_with_reply_filtering(user) do "Public timeline with reply filtering - following" => fn -> public_params |> Map.put("reply_visibility", "following") - |> Map.put("user", user) + |> Map.put("reply_filtering_user", user) |> ActivityPub.fetch_public_activities() end, "Public timeline with reply filtering - self" => fn -> public_params |> Map.put("reply_visibility", "self") - |> Map.put("user", user) + |> Map.put("reply_filtering_user", user) |> ActivityPub.fetch_public_activities() end }, @@ -531,12 +531,19 @@ defp fetch_timelines_with_reply_filtering(user) do ActivityPub.fetch_activities(recipients, private_params) end, "Home timeline with reply filtering - following" => fn -> - private_params = Map.put(private_params, "reply_visibility", "following") + private_params = + private_params + |> Map.put("reply_filtering_user", user) + |> Map.put("reply_visibility", "following") ActivityPub.fetch_activities(recipients, private_params) end, "Home timeline with reply filtering - self" => fn -> - private_params = Map.put(private_params, "reply_visibility", "self") + private_params = + private_params + |> Map.put("reply_filtering_user", user) + |> Map.put("reply_visibility", "self") + ActivityPub.fetch_activities(recipients, private_params) end }, diff --git a/benchmarks/mix/tasks/pleroma/load_testing.ex b/benchmarks/mix/tasks/pleroma/load_testing.ex index 72b225f09..388883240 100644 --- a/benchmarks/mix/tasks/pleroma/load_testing.ex +++ b/benchmarks/mix/tasks/pleroma/load_testing.ex @@ -44,6 +44,7 @@ defmodule Mix.Tasks.Pleroma.LoadTesting do ] def run(args) do + Logger.configure(level: :error) Mix.Pleroma.start_pleroma() clean_tables() {opts, _} = OptionParser.parse!(args, strict: @switches, aliases: @aliases) diff --git a/lib/pleroma/web/activity_pub/activity_pub.ex b/lib/pleroma/web/activity_pub/activity_pub.ex index 8b170b7f8..9ec31fb03 100644 --- a/lib/pleroma/web/activity_pub/activity_pub.ex +++ b/lib/pleroma/web/activity_pub/activity_pub.ex @@ -270,9 +270,9 @@ defp do_create(%{to: to, actor: actor, context: context, object: object} = param ), {:ok, activity} <- insert(create_data, local, fake), {:fake, false, activity} <- {:fake, fake, activity}, - {:quick_insert, false, activity} <- {:quick_insert, quick_insert?, activity}, _ <- increase_replies_count_if_reply(create_data), _ <- increase_poll_votes_if_vote(create_data), + {:quick_insert, false, activity} <- {:quick_insert, quick_insert?, activity}, {:ok, _actor} <- increase_note_count_if_public(actor, activity), :ok <- maybe_federate(activity) do {:ok, activity} @@ -700,14 +700,12 @@ def fetch_activities_for_context_query(context, opts) do do: [opts["user"].ap_id | User.following(opts["user"])] ++ public, else: public - opts = Map.put(opts, "user", opts["user"]) - from(activity in Activity) |> maybe_preload_objects(opts) |> maybe_preload_bookmarks(opts) |> maybe_set_thread_muted_field(opts) |> restrict_blocked(opts) - |> restrict_recipients(recipients, opts) + |> restrict_recipients(recipients, opts["user"]) |> where( [activity], fragment( @@ -742,10 +740,7 @@ def fetch_latest_activity_id_for_context(context, opts \\ %{}) do @spec fetch_public_activities(map(), Pagination.type()) :: [Activity.t()] def fetch_public_activities(opts \\ %{}, pagination \\ :keyset) do - opts = - opts - |> Map.put("reply_user", opts["user"]) - |> Map.delete("user") + opts = Map.drop(opts, ["user"]) [Constants.as_public()] |> fetch_activities_query(opts) @@ -981,65 +976,13 @@ defp restrict_tag(query, %{"tag" => tag}) when is_binary(tag) do defp restrict_tag(query, _), do: query - defp reply_recipients(user, "following") do - [user.ap_id | User.get_cached_user_friends_ap_ids(user)] + defp restrict_recipients(query, [], _user), do: query + + defp restrict_recipients(query, recipients, nil) do + from(activity in query, where: fragment("? && ?", ^recipients, activity.recipients)) end - defp reply_recipients(user, "self"), do: [user.ap_id] - - defp restrict_recipients(query, [], _opts), do: query - - defp restrict_recipients( - query, - recipients, - %{"user" => nil, "reply_user" => user, "reply_visibility" => visibility} - ) - when not is_nil(user) and visibility in ["following", "self"] do - reply_recipients = reply_recipients(user, visibility) - - from([activity, object] in query, - where: - fragment( - "? && ? AND (?->>'inReplyTo' IS NULL OR array_remove(?, ?) && ? OR ? = ?)", - ^recipients, - activity.recipients, - object.data, - activity.recipients, - activity.actor, - ^reply_recipients, - activity.actor, - ^user.ap_id - ) - ) - end - - defp restrict_recipients(query, recipients, %{"user" => nil}) do - from(activity in query, - where: fragment("? && ?", ^recipients, activity.recipients) - ) - end - - defp restrict_recipients(query, recipients, %{"user" => user, "reply_visibility" => visibility}) - when visibility in ["following", "self"] do - reply_recipients = reply_recipients(user, visibility) - - from( - [activity, object] in query, - where: - fragment( - "? && ? AND (?->>'inReplyTo' IS NULL OR array_remove(?, ?) && ?)", - ^recipients, - activity.recipients, - object.data, - activity.recipients, - activity.actor, - ^reply_recipients - ), - or_where: activity.actor == ^user.ap_id - ) - end - - defp restrict_recipients(query, recipients, %{"user" => user}) do + defp restrict_recipients(query, recipients, user) do from( activity in query, where: fragment("? && ?", ^recipients, activity.recipients), @@ -1104,6 +1047,41 @@ defp restrict_replies(query, %{"exclude_replies" => val}) when val == "true" or ) end + defp restrict_replies(query, %{ + "reply_filtering_user" => user, + "reply_visibility" => "self" + }) do + from( + [activity, object] in query, + where: + fragment( + "?->>'inReplyTo' is null OR ? = ANY(?)", + object.data, + ^user.ap_id, + activity.recipients + ) + ) + end + + defp restrict_replies(query, %{ + "reply_filtering_user" => user, + "reply_visibility" => "following" + }) do + from( + [activity, object] in query, + where: + fragment( + "?->>'inReplyTo' is null OR ? && array_remove(?, ?) OR ? = ?", + object.data, + ^[user.ap_id | User.get_cached_user_friends_ap_ids(user)], + activity.recipients, + activity.actor, + activity.actor, + ^user.ap_id + ) + ) + end + defp restrict_replies(query, _), do: query defp restrict_reblogs(query, %{"exclude_reblogs" => val}) when val == "true" or val == "1" do @@ -1311,15 +1289,14 @@ def fetch_activities_query(recipients, opts \\ %{}) do skip_thread_containment: Config.get([:instance, :skip_thread_containment]) } - opts = Map.put(opts, "user", opts["user"]) - Activity |> maybe_preload_objects(opts) |> maybe_preload_bookmarks(opts) |> maybe_preload_report_notes(opts) |> maybe_set_thread_muted_field(opts) |> maybe_order(opts) - |> restrict_recipients(recipients, opts) + |> restrict_recipients(recipients, opts["user"]) + |> restrict_replies(opts) |> restrict_tag(opts) |> restrict_tag_reject(opts) |> restrict_tag_all(opts) @@ -1334,7 +1311,6 @@ def fetch_activities_query(recipients, opts \\ %{}) do |> restrict_media(opts) |> restrict_visibility(opts) |> restrict_thread_visibility(opts, config) - |> restrict_replies(opts) |> restrict_reblogs(opts) |> restrict_pinned(opts) |> restrict_muted_reblogs(restrict_muted_reblogs_opts) diff --git a/lib/pleroma/web/mastodon_api/controllers/timeline_controller.ex b/lib/pleroma/web/mastodon_api/controllers/timeline_controller.ex index a2ac9301e..403d500e0 100644 --- a/lib/pleroma/web/mastodon_api/controllers/timeline_controller.ex +++ b/lib/pleroma/web/mastodon_api/controllers/timeline_controller.ex @@ -37,6 +37,7 @@ def home(%{assigns: %{user: user}} = conn, params) do |> Map.put("type", ["Create", "Announce"]) |> Map.put("blocking_user", user) |> Map.put("muting_user", user) + |> Map.put("reply_filtering_user", user) |> Map.put("user", user) recipients = [user.ap_id | User.following(user)] @@ -100,7 +101,7 @@ def public(%{assigns: %{user: user}} = conn, params) do |> Map.put("local_only", local_only) |> Map.put("blocking_user", user) |> Map.put("muting_user", user) - |> Map.put("user", user) + |> Map.put("reply_filtering_user", user) |> ActivityPub.fetch_public_activities() conn diff --git a/test/web/activity_pub/activity_pub_test.exs b/test/web/activity_pub/activity_pub_test.exs index 8a1638a23..edd7dfb22 100644 --- a/test/web/activity_pub/activity_pub_test.exs +++ b/test/web/activity_pub/activity_pub_test.exs @@ -1936,7 +1936,7 @@ test "public timeline", %{users: %{u1: user}} do |> Map.put("local_only", false) |> Map.put("blocking_user", user) |> Map.put("muting_user", user) - |> Map.put("user", user) + |> Map.put("reply_filtering_user", user) |> ActivityPub.fetch_public_activities() |> Enum.map(& &1.id) @@ -1958,7 +1958,7 @@ test "public timeline with reply_visibility `following`", %{ |> Map.put("blocking_user", user) |> Map.put("muting_user", user) |> Map.put("reply_visibility", "following") - |> Map.put("user", user) + |> Map.put("reply_filtering_user", user) |> ActivityPub.fetch_public_activities() |> Enum.map(& &1.id) @@ -1985,7 +1985,7 @@ test "public timeline with reply_visibility `self`", %{ |> Map.put("blocking_user", user) |> Map.put("muting_user", user) |> Map.put("reply_visibility", "self") - |> Map.put("user", user) + |> Map.put("reply_filtering_user", user) |> ActivityPub.fetch_public_activities() |> Enum.map(& &1.id) @@ -2008,6 +2008,7 @@ test "home timeline", %{ |> Map.put("blocking_user", user) |> Map.put("muting_user", user) |> Map.put("user", user) + |> Map.put("reply_filtering_user", user) activities_ids = ActivityPub.fetch_activities([user.ap_id | User.following(user)], params) @@ -2046,6 +2047,7 @@ test "home timeline with reply_visibility `following`", %{ |> Map.put("muting_user", user) |> Map.put("user", user) |> Map.put("reply_visibility", "following") + |> Map.put("reply_filtering_user", user) activities_ids = ActivityPub.fetch_activities([user.ap_id | User.following(user)], params) @@ -2084,6 +2086,7 @@ test "home timeline with reply_visibility `self`", %{ |> Map.put("muting_user", user) |> Map.put("user", user) |> Map.put("reply_visibility", "self") + |> Map.put("reply_filtering_user", user) activities_ids = ActivityPub.fetch_activities([user.ap_id | User.following(user)], params) @@ -2131,6 +2134,7 @@ test "public timeline with default reply_visibility `following`", %{users: %{u1: |> Map.put("blocking_user", user) |> Map.put("muting_user", user) |> Map.put("reply_visibility", "following") + |> Map.put("reply_filtering_user", user) |> Map.put("user", user) |> ActivityPub.fetch_public_activities() |> Enum.map(& &1.id) @@ -2146,6 +2150,7 @@ test "public timeline with default reply_visibility `self`", %{users: %{u1: user |> Map.put("blocking_user", user) |> Map.put("muting_user", user) |> Map.put("reply_visibility", "self") + |> Map.put("reply_filtering_user", user) |> Map.put("user", user) |> ActivityPub.fetch_public_activities() |> Enum.map(& &1.id) @@ -2176,6 +2181,7 @@ test "home timeline with default reply_visibility `following`", %{users: %{u1: u |> Map.put("muting_user", user) |> Map.put("user", user) |> Map.put("reply_visibility", "following") + |> Map.put("reply_filtering_user", user) activities_ids = ActivityPub.fetch_activities([user.ap_id | User.following(user)], params) @@ -2199,6 +2205,7 @@ test "home timeline with default reply_visibility `self`", %{ |> Map.put("muting_user", user) |> Map.put("user", user) |> Map.put("reply_visibility", "self") + |> Map.put("reply_filtering_user", user) activities_ids = ActivityPub.fetch_activities([user.ap_id | User.following(user)], params) From 0d05e1fe397d75d4381d9059bd1c049ab7030085 Mon Sep 17 00:00:00 2001 From: Ivan Tashkinov Date: Sat, 25 Apr 2020 18:24:10 +0300 Subject: [PATCH 10/18] [#1706] Prevented error on unresolved activity actors for timeline actions. --- lib/pleroma/web/admin_api/views/status_view.ex | 18 +++--------------- .../web/mastodon_api/views/status_view.ex | 13 ++++++++++--- 2 files changed, 13 insertions(+), 18 deletions(-) diff --git a/lib/pleroma/web/admin_api/views/status_view.ex b/lib/pleroma/web/admin_api/views/status_view.ex index 360ddc22c..3637dee24 100644 --- a/lib/pleroma/web/admin_api/views/status_view.ex +++ b/lib/pleroma/web/admin_api/views/status_view.ex @@ -8,15 +8,16 @@ defmodule Pleroma.Web.AdminAPI.StatusView do require Pleroma.Constants alias Pleroma.User + alias Pleroma.Web.MastodonAPI.StatusView def render("index.json", opts) do safe_render_many(opts.activities, __MODULE__, "show.json", opts) end def render("show.json", %{activity: %{data: %{"object" => _object}} = activity} = opts) do - user = get_user(activity.data["actor"]) + user = StatusView.get_user(activity.data["actor"]) - Pleroma.Web.MastodonAPI.StatusView.render("show.json", opts) + StatusView.render("show.json", opts) |> Map.merge(%{account: merge_account_views(user)}) end @@ -26,17 +27,4 @@ defp merge_account_views(%User{} = user) do end defp merge_account_views(_), do: %{} - - defp get_user(ap_id) do - cond do - user = User.get_cached_by_ap_id(ap_id) -> - user - - user = User.get_by_guessed_nickname(ap_id) -> - user - - true -> - User.error_user(ap_id) - end - end end diff --git a/lib/pleroma/web/mastodon_api/views/status_view.ex b/lib/pleroma/web/mastodon_api/views/status_view.ex index b5850e1ae..b0c53acd9 100644 --- a/lib/pleroma/web/mastodon_api/views/status_view.ex +++ b/lib/pleroma/web/mastodon_api/views/status_view.ex @@ -45,7 +45,7 @@ defp get_replied_to_activities(activities) do end) end - defp get_user(ap_id) do + def get_user(ap_id, fake_record_fallback \\ true) do cond do user = User.get_cached_by_ap_id(ap_id) -> user @@ -53,8 +53,11 @@ defp get_user(ap_id) do user = User.get_by_guessed_nickname(ap_id) -> user - true -> + fake_record_fallback -> + # TODO: refactor (fake records is never a good idea) User.error_user(ap_id) + + true -> nil end end @@ -97,7 +100,11 @@ def render("index.json", opts) do UserRelationship.view_relationships_option(nil, []) true -> - actors = Enum.map(activities ++ parent_activities, &get_user(&1.data["actor"])) + # Note: unresolved users are filtered out + actors = + (activities ++ parent_activities) + |> Enum.map(&get_user(&1.data["actor"], false)) + |> Enum.filter(& &1) UserRelationship.view_relationships_option(reading_user, actors, source_mutes_only: opts[:skip_relationships] From e16437ff191f17b7ec59504d3c38e582ba76eedc Mon Sep 17 00:00:00 2001 From: Ivan Tashkinov Date: Sat, 25 Apr 2020 18:42:08 +0300 Subject: [PATCH 11/18] [#1706] Formatting fix. --- lib/pleroma/web/mastodon_api/views/status_view.ex | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/lib/pleroma/web/mastodon_api/views/status_view.ex b/lib/pleroma/web/mastodon_api/views/status_view.ex index b0c53acd9..1d9082c09 100644 --- a/lib/pleroma/web/mastodon_api/views/status_view.ex +++ b/lib/pleroma/web/mastodon_api/views/status_view.ex @@ -57,7 +57,8 @@ def get_user(ap_id, fake_record_fallback \\ true) do # TODO: refactor (fake records is never a good idea) User.error_user(ap_id) - true -> nil + true -> + nil end end From 1bd9749a8f31e5f087b0d0ca75b13f4baf461997 Mon Sep 17 00:00:00 2001 From: Alex Gleason Date: Sun, 26 Apr 2020 00:28:57 -0500 Subject: [PATCH 12/18] Let blob: pass CSP --- docs/configuration/hardening.md | 2 +- lib/pleroma/plugs/http_security_plug.ex | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/configuration/hardening.md b/docs/configuration/hardening.md index b54c28850..d3bfc4e4a 100644 --- a/docs/configuration/hardening.md +++ b/docs/configuration/hardening.md @@ -36,7 +36,7 @@ content-security-policy: default-src 'none'; base-uri 'self'; frame-ancestors 'none'; - img-src 'self' data: https:; + img-src 'self' data: blob: https:; media-src 'self' https:; style-src 'self' 'unsafe-inline'; font-src 'self'; diff --git a/lib/pleroma/plugs/http_security_plug.ex b/lib/pleroma/plugs/http_security_plug.ex index 81e6b4f2a..6462797b6 100644 --- a/lib/pleroma/plugs/http_security_plug.ex +++ b/lib/pleroma/plugs/http_security_plug.ex @@ -75,7 +75,7 @@ defp csp_string do "default-src 'none'", "base-uri 'self'", "frame-ancestors 'none'", - "img-src 'self' data: https:", + "img-src 'self' data: blob: https:", "media-src 'self' https:", "style-src 'self' 'unsafe-inline'", "font-src 'self'", From a626cb682cc8fd6cad91484db064ed22646960af Mon Sep 17 00:00:00 2001 From: fence Date: Mon, 27 Apr 2020 17:55:33 +0200 Subject: [PATCH 13/18] secure mongoose auth endpoint --- .../web/mongooseim/mongoose_im_controller.ex | 37 +++++++++++++------ 1 file changed, 26 insertions(+), 11 deletions(-) diff --git a/lib/pleroma/web/mongooseim/mongoose_im_controller.ex b/lib/pleroma/web/mongooseim/mongoose_im_controller.ex index 04d823b36..744cf5227 100644 --- a/lib/pleroma/web/mongooseim/mongoose_im_controller.ex +++ b/lib/pleroma/web/mongooseim/mongoose_im_controller.ex @@ -26,21 +26,36 @@ def user_exists(conn, %{"user" => username}) do end def check_password(conn, %{"user" => username, "pass" => password}) do - with %User{password_hash: password_hash} <- - Repo.get_by(User, nickname: username, local: true), - true <- Pbkdf2.checkpw(password, password_hash) do - conn - |> json(true) - else - false -> - conn - |> put_status(:forbidden) - |> json(false) + user = Repo.get_by(User, nickname: username, local: true) - _ -> + case User.account_status(user) do + :deactivated -> conn |> put_status(:not_found) |> json(false) + + :confirmation_pending -> + conn + |> put_status(:not_found) + |> json(false) + + _ -> + with %User{password_hash: password_hash} <- + user, + true <- Pbkdf2.checkpw(password, password_hash) do + conn + |> json(true) + else + false -> + conn + |> put_status(:forbidden) + |> json(false) + + _ -> + conn + |> put_status(:not_found) + |> json(false) + end end end end From 5c7cc109172c84b991fad7eebbdd51e75f0c5382 Mon Sep 17 00:00:00 2001 From: fence Date: Mon, 27 Apr 2020 18:31:00 +0200 Subject: [PATCH 14/18] add tests for deactivated users for mongoose auth --- lib/pleroma/web/mongooseim/mongoose_im_controller.ex | 7 ++++++- test/web/mongooseim/mongoose_im_controller_test.exs | 9 +++++++++ 2 files changed, 15 insertions(+), 1 deletion(-) diff --git a/lib/pleroma/web/mongooseim/mongoose_im_controller.ex b/lib/pleroma/web/mongooseim/mongoose_im_controller.ex index 744cf5227..c15b4bfb8 100644 --- a/lib/pleroma/web/mongooseim/mongoose_im_controller.ex +++ b/lib/pleroma/web/mongooseim/mongoose_im_controller.ex @@ -27,8 +27,13 @@ def user_exists(conn, %{"user" => username}) do def check_password(conn, %{"user" => username, "pass" => password}) do user = Repo.get_by(User, nickname: username, local: true) + + state = case user do + nil -> nil + _ -> User.account_status(user) + end - case User.account_status(user) do + case state do :deactivated -> conn |> put_status(:not_found) diff --git a/test/web/mongooseim/mongoose_im_controller_test.exs b/test/web/mongooseim/mongoose_im_controller_test.exs index 291ae54fc..5987111e5 100644 --- a/test/web/mongooseim/mongoose_im_controller_test.exs +++ b/test/web/mongooseim/mongoose_im_controller_test.exs @@ -34,6 +34,7 @@ test "/user_exists", %{conn: conn} do test "/check_password", %{conn: conn} do user = insert(:user, password_hash: Comeonin.Pbkdf2.hashpwsalt("cool")) + _deactivated_user = insert(:user, nickname: "konata", local: false, deactivated: true) res = conn @@ -49,6 +50,14 @@ test "/check_password", %{conn: conn} do assert res == false + res = + conn + |> get(mongoose_im_path(conn, :check_password), user: "konata", pass: "1337") + |> json_response(404) + + assert res == false + + res = conn |> get(mongoose_im_path(conn, :check_password), user: "nobody", pass: "cool") From cc1e2e8d0f5fa27d051a0a21740a8052b95ce1a5 Mon Sep 17 00:00:00 2001 From: fence Date: Mon, 27 Apr 2020 19:11:03 +0200 Subject: [PATCH 15/18] requested changes to mongoose_im_controller.ex --- .../web/mongooseim/mongoose_im_controller.ex | 45 ++++++------------- 1 file changed, 13 insertions(+), 32 deletions(-) diff --git a/lib/pleroma/web/mongooseim/mongoose_im_controller.ex b/lib/pleroma/web/mongooseim/mongoose_im_controller.ex index c15b4bfb8..7123153c5 100644 --- a/lib/pleroma/web/mongooseim/mongoose_im_controller.ex +++ b/lib/pleroma/web/mongooseim/mongoose_im_controller.ex @@ -14,7 +14,7 @@ defmodule Pleroma.Web.MongooseIM.MongooseIMController do plug(RateLimiter, [name: :authentication, params: ["user"]] when action == :check_password) def user_exists(conn, %{"user" => username}) do - with %User{} <- Repo.get_by(User, nickname: username, local: true) do + with %User{} <- Repo.get_by(User, nickname: username, local: true, deactivated: false) do conn |> json(true) else @@ -26,41 +26,22 @@ def user_exists(conn, %{"user" => username}) do end def check_password(conn, %{"user" => username, "pass" => password}) do - user = Repo.get_by(User, nickname: username, local: true) - - state = case user do - nil -> nil - _ -> User.account_status(user) - end - - case state do - :deactivated -> + with %User{password_hash: password_hash, deactivated: false} <- + Repo.get_by(User, nickname: username, local: true), + true <- Pbkdf2.checkpw(password, password_hash) do + conn + |> json(true) + else + false -> conn - |> put_status(:not_found) - |> json(false) - - :confirmation_pending -> - conn - |> put_status(:not_found) + |> put_status(:forbidden) |> json(false) _ -> - with %User{password_hash: password_hash} <- - user, - true <- Pbkdf2.checkpw(password, password_hash) do - conn - |> json(true) - else - false -> - conn - |> put_status(:forbidden) - |> json(false) - - _ -> - conn - |> put_status(:not_found) - |> json(false) - end + conn + |> put_status(:not_found) + |> json(false) end end end + From 935ca2c1329fd4b4f2b40a5ed7f2c1fa5f95b9bd Mon Sep 17 00:00:00 2001 From: fence Date: Mon, 27 Apr 2020 19:16:05 +0200 Subject: [PATCH 16/18] requested changes to mongoose test --- test/web/mongooseim/mongoose_im_controller_test.exs | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/test/web/mongooseim/mongoose_im_controller_test.exs b/test/web/mongooseim/mongoose_im_controller_test.exs index 5987111e5..2b4d124af 100644 --- a/test/web/mongooseim/mongoose_im_controller_test.exs +++ b/test/web/mongooseim/mongoose_im_controller_test.exs @@ -9,6 +9,7 @@ defmodule Pleroma.Web.MongooseIMController do test "/user_exists", %{conn: conn} do _user = insert(:user, nickname: "lain") _remote_user = insert(:user, nickname: "alice", local: false) + _deactivated_user = insert(:user, nickname: "konata", deactivated: true) res = conn @@ -30,11 +31,18 @@ test "/user_exists", %{conn: conn} do |> json_response(404) assert res == false + + res = + conn + |> get(mongoose_im_path(conn, :user_exists), user: "konata") + |> json_response(404) + + assert res == false end test "/check_password", %{conn: conn} do user = insert(:user, password_hash: Comeonin.Pbkdf2.hashpwsalt("cool")) - _deactivated_user = insert(:user, nickname: "konata", local: false, deactivated: true) + _deactivated_user = insert(:user, nickname: "konata", deactivated: true) res = conn @@ -52,7 +60,7 @@ test "/check_password", %{conn: conn} do res = conn - |> get(mongoose_im_path(conn, :check_password), user: "konata", pass: "1337") + |> get(mongoose_im_path(conn, :check_password), user: "konata", pass: "cool") |> json_response(404) assert res == false From d607b4d84002ff14f51713f1ac74a4971e2dffac Mon Sep 17 00:00:00 2001 From: fence Date: Mon, 27 Apr 2020 19:32:58 +0200 Subject: [PATCH 17/18] mongooseim test: explicitly set password for the deactivated used --- test/web/mongooseim/mongoose_im_controller_test.exs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/web/mongooseim/mongoose_im_controller_test.exs b/test/web/mongooseim/mongoose_im_controller_test.exs index 2b4d124af..d17f8dbb4 100644 --- a/test/web/mongooseim/mongoose_im_controller_test.exs +++ b/test/web/mongooseim/mongoose_im_controller_test.exs @@ -42,7 +42,7 @@ test "/user_exists", %{conn: conn} do test "/check_password", %{conn: conn} do user = insert(:user, password_hash: Comeonin.Pbkdf2.hashpwsalt("cool")) - _deactivated_user = insert(:user, nickname: "konata", deactivated: true) + _deactivated_user = insert(:user, nickname: "konata", deactivated: true, password_hash: Comeonin.Pbkdf2.hashpwsalt("cool")) res = conn From 5ff20793e739daa962cdc1623c01dc6ec1ff8a61 Mon Sep 17 00:00:00 2001 From: fence Date: Tue, 28 Apr 2020 01:29:31 +0200 Subject: [PATCH 18/18] formating --- lib/pleroma/web/mongooseim/mongoose_im_controller.ex | 1 - test/web/mongooseim/mongoose_im_controller_test.exs | 9 +++++++-- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/lib/pleroma/web/mongooseim/mongoose_im_controller.ex b/lib/pleroma/web/mongooseim/mongoose_im_controller.ex index 7123153c5..1ed6ee521 100644 --- a/lib/pleroma/web/mongooseim/mongoose_im_controller.ex +++ b/lib/pleroma/web/mongooseim/mongoose_im_controller.ex @@ -44,4 +44,3 @@ def check_password(conn, %{"user" => username, "pass" => password}) do end end end - diff --git a/test/web/mongooseim/mongoose_im_controller_test.exs b/test/web/mongooseim/mongoose_im_controller_test.exs index d17f8dbb4..1ac2f2c27 100644 --- a/test/web/mongooseim/mongoose_im_controller_test.exs +++ b/test/web/mongooseim/mongoose_im_controller_test.exs @@ -42,7 +42,13 @@ test "/user_exists", %{conn: conn} do test "/check_password", %{conn: conn} do user = insert(:user, password_hash: Comeonin.Pbkdf2.hashpwsalt("cool")) - _deactivated_user = insert(:user, nickname: "konata", deactivated: true, password_hash: Comeonin.Pbkdf2.hashpwsalt("cool")) + + _deactivated_user = + insert(:user, + nickname: "konata", + deactivated: true, + password_hash: Comeonin.Pbkdf2.hashpwsalt("cool") + ) res = conn @@ -65,7 +71,6 @@ test "/check_password", %{conn: conn} do assert res == false - res = conn |> get(mongoose_im_path(conn, :check_password), user: "nobody", pass: "cool")