diff --git a/.github/changelog/fix-account-move-verification b/.github/changelog/fix-account-move-verification new file mode 100644 index 0000000000..dd9955c119 --- /dev/null +++ b/.github/changelog/fix-account-move-verification @@ -0,0 +1,4 @@ +Significance: patch +Type: fixed + +Fix Fediverse account migration so a move is verified before it takes effect and reliably reaches your followers on other servers. diff --git a/.github/changelog/fix-move-canonical-target-id b/.github/changelog/fix-move-canonical-target-id new file mode 100644 index 0000000000..152837877b --- /dev/null +++ b/.github/changelog/fix-move-canonical-target-id @@ -0,0 +1,4 @@ +Significance: patch +Type: fixed + +Fix Fediverse account migration being ignored by some servers when the new account is entered using an alternate address. diff --git a/includes/class-move.php b/includes/class-move.php index 9222075f52..211793430c 100644 --- a/includes/class-move.php +++ b/includes/class-move.php @@ -12,6 +12,7 @@ use Activitypub\Collection\Actors; use Activitypub\Model\Blog; use Activitypub\Model\User; +use Activitypub\Scheduler\Actor as Actor_Scheduler; /** * ActivityPub (Account) Move Class @@ -81,13 +82,6 @@ public static function externally( $from, $to ) { return $user; } - // Update the movedTo property. - if ( $user->get__id() > 0 ) { - \update_user_option( $user->get__id(), 'activitypub_moved_to', $to ); - } else { - \update_option( 'activitypub_blog_user_moved_to', $to ); - } - $response = Http::get_remote_object( $to ); if ( \is_wp_error( $response ) ) { @@ -97,21 +91,53 @@ public static function externally( $from, $to ) { $target_actor = new Actor(); $target_actor->from_array( $response ); - // Check if the `Move` Activity is valid. + // The canonical id is both federated and advertised, so a target that declares none cannot be moved to. + $target_id = $target_actor->get_id(); + if ( ! $target_id ) { + return new \WP_Error( 'invalid_target', \__( 'Invalid target', 'activitypub' ) ); + } + + /* + * The move is only valid if the target links back. Receiving servers accept it only when the + * id we send as the Move's `object` is listed in the target's `alsoKnownAs`, so verify that + * exact id, not the (possibly non-canonical) input URL. + */ $also_known_as = $target_actor->get_also_known_as() ?? array(); - if ( ! \in_array( $from, $also_known_as, true ) ) { + if ( ! \in_array( $user->get_id(), $also_known_as, true ) ) { return new \WP_Error( 'invalid_target', \__( 'Invalid target', 'activitypub' ) ); } + /* + * Advertise the move only after the target is verified, so a failed attempt never leaves the + * actor pointing at an unverified target. Store the canonical id, not the input URL: receivers + * match the advertised `movedTo` against the Move's `target` and skip the move when they differ. + */ + if ( $user->get__id() > 0 ) { + \update_user_option( $user->get__id(), 'activitypub_moved_to', $target_id ); + } else { + \update_option( 'activitypub_blog_user_moved_to', $target_id ); + } + $activity = new Activity(); $activity->set_type( 'Move' ); $activity->set_actor( $user->get_id() ); $activity->set_origin( $user->get_id() ); $activity->set_object( $user->get_id() ); - $activity->set_target( $target_actor->get_id() ); + $activity->set_target( $target_id ); + + $outbox_id = add_to_outbox( $activity, null, $user->get__id(), ACTIVITYPUB_CONTENT_VISIBILITY_PUBLIC ); - // Add to outbox. - return add_to_outbox( $activity, null, $user->get__id(), ACTIVITYPUB_CONTENT_VISIBILITY_PUBLIC ); + if ( ! $outbox_id || \is_wp_error( $outbox_id ) ) { + return $outbox_id; + } + + /* + * Notify followers of the new movedTo by federating a profile Update (FEP-7628). Queue it + * after the Move so a follower that reacts to `movedTo` still processes the migration first. + */ + Actor_Scheduler::schedule_profile_update( $user->get__id() ); + + return $outbox_id; } /** @@ -139,15 +165,27 @@ public static function internally( $from, $to ) { return $user; } - // Add the old account URL to alsoKnownAs. + // Point the old actor at the new one. if ( $user->get__id() > 0 ) { - self::update_user_also_known_as( $user->get__id(), $from ); \update_user_option( $user->get__id(), 'activitypub_moved_to', $to ); } else { - self::update_blog_also_known_as( $from ); \update_option( 'activitypub_blog_user_moved_to', $to ); } + /* + * The old account URL belongs in the *target's* alsoKnownAs, not the source's: receiving + * servers accept the Move only when the new actor links back to the old one. For a domain + * change the source and target resolve to the same actor, so it is still recorded there. + */ + $target = Actors::get_by_various( $to ); + if ( ! \is_wp_error( $target ) ) { + if ( $target->get__id() > 0 ) { + self::update_user_also_known_as( $target->get__id(), $from ); + } else { + self::update_blog_also_known_as( $from ); + } + } + // check if `$from` is a URL or an ID. if ( \filter_var( $from, FILTER_VALIDATE_URL ) ) { $actor = $from; @@ -162,7 +200,22 @@ public static function internally( $from, $to ) { $activity->set_object( $actor ); $activity->set_target( $to ); - return add_to_outbox( $activity, null, $user->get__id(), ACTIVITYPUB_CONTENT_VISIBILITY_QUIET_PUBLIC ); + $outbox_id = add_to_outbox( $activity, null, $user->get__id(), ACTIVITYPUB_CONTENT_VISIBILITY_QUIET_PUBLIC ); + + if ( ! $outbox_id || \is_wp_error( $outbox_id ) ) { + return $outbox_id; + } + + /* + * Notify followers of the changed profile on both actors by federating an Update (FEP-7628). + * Queued after the Move so a follower that reacts to `movedTo` still processes the migration first. + */ + Actor_Scheduler::schedule_profile_update( $user->get__id() ); + if ( ! \is_wp_error( $target ) && $target->get__id() !== $user->get__id() ) { + Actor_Scheduler::schedule_profile_update( $target->get__id() ); + } + + return $outbox_id; } /** diff --git a/tests/phpunit/tests/includes/class-test-move.php b/tests/phpunit/tests/includes/class-test-move.php index 9d52081eed..9affdfc228 100644 --- a/tests/phpunit/tests/includes/class-test-move.php +++ b/tests/phpunit/tests/includes/class-test-move.php @@ -8,6 +8,7 @@ namespace Activitypub\Tests; use Activitypub\Collection\Actors; +use Activitypub\Collection\Outbox; use Activitypub\Move; /** @@ -40,11 +41,26 @@ public function test_account_with_valid_input() { $from = Actors::get_by_id( self::$user_id )->get_id(); $to = 'https://newsite.com/user/1'; - add_filter( 'pre_http_request', '__return_false' ); + $filter = function () use ( $from, $to ) { + return array( + 'body' => wp_json_encode( + array( + 'id' => $to, + 'type' => 'Person', + 'alsoKnownAs' => array( $from ), + ) + ), + 'response' => array( 'code' => 200 ), + ); + }; + add_filter( 'pre_http_request', $filter ); + Move::externally( $from, $to ); $moved_to = Actors::get_by_id( self::$user_id )->get_moved_to(); $this->assertEquals( $to, $moved_to ); + + remove_filter( 'pre_http_request', $filter ); } /** @@ -81,7 +97,203 @@ public function test_account_with_invalid_target() { $this->assertWPError( $result ); $this->assertEquals( 'http_request_failed', $result->get_error_code() ); + // A move that never verified must not leave the actor pointing at the target. + $this->assertNull( Actors::get_by_id( self::$user_id )->get_moved_to() ); + + \remove_filter( 'activitypub_pre_http_get_remote_object', $filter ); + } + + /** + * A verified move federates a profile Update so followers refresh the actor (FEP-7628). + * + * @covers ::externally + */ + public function test_move_federates_profile_update() { + $from = Actors::get_by_id( self::$user_id )->get_id(); + $to = 'https://newsite.com/user/1'; + + $filter = function () use ( $from, $to ) { + return array( + 'body' => wp_json_encode( + array( + 'id' => $to, + 'type' => 'Person', + 'alsoKnownAs' => array( $from ), + ) + ), + 'response' => array( 'code' => 200 ), + ); + }; + add_filter( 'pre_http_request', $filter ); + + Move::externally( $from, $to ); + + remove_filter( 'pre_http_request', $filter ); + + $updates = get_posts( + array( + 'post_type' => Outbox::POST_TYPE, + 'post_status' => 'any', + 'author' => self::$user_id, + // phpcs:ignore WordPress.DB.SlowDBQuery.slow_db_query_meta_query + 'meta_query' => array( + array( + 'key' => '_activitypub_activity_type', + 'value' => 'Update', + ), + ), + ) + ); + + $this->assertCount( 1, $updates, 'A move should federate exactly one profile Update.' ); + } + + /** + * When the Move itself is not federated, no follower notification is sent. + * + * @covers ::externally + */ + public function test_move_does_not_notify_followers_when_move_fails() { + $from = Actors::get_by_id( self::$user_id )->get_id(); + $to = 'https://newsite.com/user/1'; + + // Target links back, so verification passes. + $http = function () use ( $from, $to ) { + return array( + 'body' => wp_json_encode( + array( + 'id' => $to, + 'type' => 'Person', + 'alsoKnownAs' => array( $from ), + ) + ), + 'response' => array( 'code' => 200 ), + ); + }; + add_filter( 'pre_http_request', $http ); + + // Fail only the Move's outbox insert; a follow-up profile Update would still succeed. + $fail_move = function ( $maybe_empty, $postarr ) { + $data = json_decode( stripslashes( (string) ( $postarr['post_content'] ?? '' ) ), true ); + + return ( is_array( $data ) && isset( $data['type'] ) && 'Move' === $data['type'] ) ? true : $maybe_empty; + }; + add_filter( 'wp_insert_post_empty_content', $fail_move, 10, 2 ); + + $result = Move::externally( $from, $to ); + + remove_filter( 'wp_insert_post_empty_content', $fail_move, 10 ); + remove_filter( 'pre_http_request', $http ); + + $this->assertTrue( empty( $result ) || \is_wp_error( $result ), 'A Move that could not be federated must not return a success id.' ); + + $updates = get_posts( + array( + 'post_type' => Outbox::POST_TYPE, + 'post_status' => 'any', + 'author' => self::$user_id, + // phpcs:ignore WordPress.DB.SlowDBQuery.slow_db_query_meta_query + 'meta_query' => array( + array( + 'key' => '_activitypub_activity_type', + 'value' => 'Update', + ), + ), + ) + ); + + $this->assertEmpty( $updates, 'No profile Update should be federated when the Move was not.' ); + } + + /** + * A target that does not link back is rejected and the actor is not moved. + * + * @covers ::externally + */ + public function test_account_rejects_unlinked_target() { + $from = Actors::get_by_id( self::$user_id )->get_id(); + $to = 'https://newsite.com/user/1'; + + // Target resolves, but its alsoKnownAs does not list this actor. + $filter = function () use ( $to ) { + return array( + 'body' => wp_json_encode( + array( + 'id' => $to, + 'type' => 'Person', + 'alsoKnownAs' => array( 'https://newsite.com/user/999' ), + ) + ), + 'response' => array( 'code' => 200 ), + ); + }; + \add_filter( 'pre_http_request', $filter ); + + $result = Move::externally( $from, $to ); + + $this->assertWPError( $result ); + $this->assertEquals( 'invalid_target', $result->get_error_code() ); + $this->assertNull( Actors::get_by_id( self::$user_id )->get_moved_to() ); + + \remove_filter( 'pre_http_request', $filter ); + } + + /** + * The advertised `movedTo` is the target's canonical id, not the URL the move was requested with. + * + * Receiving servers match the advertised `movedTo` against the Move's `target` and skip the move + * when the two differ, so an alias or redirecting input URL must not end up as the stored value. + * + * @covers ::externally + */ + public function test_account_stores_canonical_target_id() { + $from = Actors::get_by_id( self::$user_id )->get_id(); + $alias = 'https://newsite.com/alias/1'; + $canonical = 'https://newsite.com/user/1'; + + $filter = function () use ( $from, $canonical ) { + return array( + 'id' => $canonical, + 'type' => 'Person', + 'alsoKnownAs' => array( $from ), + ); + }; + \add_filter( 'activitypub_pre_http_get_remote_object', $filter ); + + $outbox_id = Move::externally( $from, $alias ); + \remove_filter( 'activitypub_pre_http_get_remote_object', $filter ); + + $this->assertEquals( $canonical, Actors::get_by_id( self::$user_id )->get_moved_to() ); + + $activity = \json_decode( \get_post_field( 'post_content', $outbox_id ) ); + $this->assertEquals( $canonical, $activity->target, 'The federated target must match the advertised movedTo.' ); + } + + /** + * A target document that declares no id cannot be federated, so the move is rejected. + * + * @covers ::externally + */ + public function test_account_rejects_target_without_id() { + $from = Actors::get_by_id( self::$user_id )->get_id(); + $to = 'https://newsite.com/user/1'; + + $filter = function () use ( $from ) { + return array( + 'type' => 'Person', + 'alsoKnownAs' => array( $from ), + ); + }; + \add_filter( 'activitypub_pre_http_get_remote_object', $filter ); + + $result = Move::externally( $from, $to ); + + \remove_filter( 'activitypub_pre_http_get_remote_object', $filter ); + + $this->assertWPError( $result ); + $this->assertEquals( 'invalid_target', $result->get_error_code() ); + $this->assertNull( Actors::get_by_id( self::$user_id )->get_moved_to() ); } /** @@ -95,9 +307,15 @@ public function test_account_with_duplicate_moves() { \update_user_option( self::$user_id, 'activitypub_also_known_as', array( 'https://old.example.com/user/1' ) ); - $filter = function () use ( $from ) { + $filter = function () use ( $from, $to ) { return array( - 'body' => wp_json_encode( array( 'alsoKnownAs' => array( $from ) ) ), + 'body' => wp_json_encode( + array( + 'id' => $to, + 'type' => 'Person', + 'alsoKnownAs' => array( $from ), + ) + ), 'response' => array( 'code' => 200 ), ); }; @@ -123,11 +341,26 @@ public function test_account_with_blog_author_as_actor() { $from = Actors::get_by_id( Actors::BLOG_USER_ID )->get_id(); $to = 'https://newsite.com/user/0'; + $filter = function () use ( $from, $to ) { + return array( + 'body' => wp_json_encode( + array( + 'id' => $to, + 'type' => 'Person', + 'alsoKnownAs' => array( $from ), + ) + ), + 'response' => array( 'code' => 200 ), + ); + }; + \add_filter( 'pre_http_request', $filter ); + Move::externally( $from, $to ); $moved_to = Actors::get_by_id( Actors::BLOG_USER_ID )->get_moved_to(); $this->assertEquals( $to, $moved_to ); + \remove_filter( 'pre_http_request', $filter ); \delete_option( 'activitypub_actor_mode' ); } @@ -153,6 +386,29 @@ public function test_internally_with_valid_input() { $this->assertContains( $from, $also_known_as ); } + /** + * An internal move between two different local users links the target back to the source. + * + * @covers ::internally + */ + public function test_internally_between_distinct_users() { + $target_id = self::factory()->user->create( array( 'role' => 'author' ) ); + + $from = Actors::get_by_id( self::$user_id )->get_id(); + $to = Actors::get_by_id( $target_id )->get_id(); + + Move::internally( $from, $to ); + + wp_cache_delete( self::$user_id, 'users' ); + wp_cache_delete( $target_id, 'users' ); + + // The source points at the target. + $this->assertEquals( $to, Actors::get_by_id( self::$user_id )->get_moved_to() ); + + // The target links back to the source via alsoKnownAs, so receiving servers accept the move. + $this->assertContains( $from, Actors::get_by_id( $target_id )->get_also_known_as() ); + } + /** * Test that the Move Activity created by internally() has the correct properties. *