From d0e3f9c82334582a679062989d2cbed49554e2e6 Mon Sep 17 00:00:00 2001 From: Diogo Cordeiro Date: Fri, 13 Sep 2019 11:56:36 +0100 Subject: [PATCH] [ActivityPub] Fix some bugs with onStartNoticeSearch Refactored Activitypub_profile::ensure_web_finger to Activitypub_profile::ensure_webfinger Do not throw exceptions in the handling of this event because we don't want to stop the regular search just because we were unable to find ActivityPub actors or notes. --- plugins/ActivityPub/ActivityPubPlugin.php | 133 ++++++++++-------- .../classes/Activitypub_profile.php | 33 ++--- plugins/ActivityPub/lib/explorer.php | 23 +-- .../tests/Unit/ActivitypubProfileTest.php | 2 +- 4 files changed, 108 insertions(+), 83 deletions(-) diff --git a/plugins/ActivityPub/ActivityPubPlugin.php b/plugins/ActivityPub/ActivityPubPlugin.php index 057b10afeb..efad81ee01 100644 --- a/plugins/ActivityPub/ActivityPubPlugin.php +++ b/plugins/ActivityPub/ActivityPubPlugin.php @@ -87,13 +87,13 @@ class ActivityPubPlugin extends Plugin /** * Returns a notice from its URL. * - * @author Diogo Cordeiro * @param string $url Notice's URL - * @param bool $grabOnline whether to try online grabbing, defaults to true + * @param bool $grab_online whether to try online grabbing, defaults to true * @return Notice|null The Notice object * @throws Exception This function or provides a Notice, null, or fails with exception + * @author Diogo Cordeiro */ - public static function grab_notice_from_url(string $url, bool $grabOnline = true): ?Notice + public static function grab_notice_from_url(string $url, bool $grab_online = true): ?Notice { /* Offline Grabbing */ try { @@ -114,7 +114,7 @@ class ActivityPubPlugin extends Plugin } } - if ($grabOnline) { + if ($grab_online) { /* Online Grabbing */ $client = new HTTPClient(); $headers = []; @@ -319,7 +319,7 @@ class ActivityPubPlugin extends Plugin /** * Validate AP-recipients for profile page message action addition - * + * * @param Profile $recipient * @return bool hook return value */ @@ -334,7 +334,7 @@ class ActivityPubPlugin extends Plugin /** * Mark an ap_profile object for deletion - * + * * @param Profile profile being deleted * @param array &$related objects with same profile_id to be deleted * @return void @@ -353,7 +353,7 @@ class ActivityPubPlugin extends Plugin } } catch (Exception $e) { // nothing to do - } + } } /** @@ -400,70 +400,91 @@ class ActivityPubPlugin extends Plugin /** * Hack the notice search-box and try to grab remote profiles or notices. - * + * * Note that, on successful grabbing, this function will redirect to the * new profile/notice, so URL searching is directly affected. A good solution * for this is to store the URLs in the notice text without the https/http * prefixes. This would change the queries for URL searching and therefore we * could do both search and grab. - * + * * @param string $query search query - * @return void + * @return bool hook + * @author Bruno Casteleiro */ - public function onStartNoticeSearch(string $query): void + public function onStartNoticeSearch(string $query): bool { if (!common_logged_in()) { - // early return, search-only for non-logged sessions - return; + // early return: Only allow logged users to import/search for remote actors or notes + return true; } - if (!filter_var($query, FILTER_VALIDATE_URL) && - !preg_match('!^((?:\w+\.)*\w+@(?:\w+\.)*\w+(?:\w+\-\w+)*\.\w+)$!', $query)) { - // early return, not an url or webfinger ID - return; - } - - // someone we know about ? - try { - $explorer = new Activitypub_explorer(); - $profile = $explorer->lookup($query, false)[0]; - if ($profile instanceof Profile) { - return; + if (preg_match('!^((?:\w+\.)*\w+@(?:\w+\.)*\w+(?:\w+\-\w+)*\.\w+)$!', $query)) { // WebFinger ID found! + // Try to grab remote actor + $aprofile = self::pull_remote_profile($query); + if ($aprofile instanceof Activitypub_profile) { + $url = common_local_url('userbyid', ['id' => $aprofile->getID()], null, null, false); + common_redirect($url, 303); + return true; } - } catch (Exception $e) { - // nope - } + } elseif (filter_var($query, FILTER_VALIDATE_URL)) { // URL found! + /* Is this an ActivityPub notice? */ - // some notice we know about ? - try { - $notice = self::grab_notice_from_url($query, false); - if ($notice instanceof Notice) { - return; + // If we already know it, just return + try { + $notice = self::grab_notice_from_url($query, false); // Only check locally + if ($notice instanceof Notice) { + return true; + } + } catch (Exception $e) { + // We will next try online } - } catch (Exception $e) { - // nope - } - // try to grab profile - $aprofile = self::pull_remote_profile($query); - if ($aprofile instanceof Activitypub_profile) { - $url = common_local_url('userbyid', ['id' => $aprofile->getID()], null, null, false); - common_redirect($url, 303); - return; - } + // Otherwise, try to grab it + try { + $notice = self::grab_notice_from_url($query); // Unfortunately we will be trying locally again + if ($notice instanceof Notice) { + $url = common_local_url('shownotice', ['notice' => $notice->getID()]); + common_redirect($url, 303); + } + } catch (Exception $e) { + // We will next check if this URL is an actor + } - // try to grab notice - $notice = self::grab_notice_from_url($query); - if ($notice instanceof Notice) { - $url = common_local_url('shownotice', ['notice' => $notice->getID()]); - common_redirect($url, 303); + /* Is this an ActivityPub actor? */ + + // If we already know it, just return + try { + $explorer = new Activitypub_explorer(); + $profile = $explorer->lookup($query, false)[0]; // Only check locally + if ($profile instanceof Profile) { + return true; + } + } catch (Exception $e) { + // We will next try online + } + + // Try to grab remote actor + try { + if (!isset($explorer)) { + $explorer = new Activitypub_explorer(); + } + $profile = $explorer->lookup($query)[0]; // Unfortunately we will be trying locally again + if ($profile instanceof Profile) { + $url = common_local_url('userbyid', ['id' => $profile->getID()], null, null, false); + common_redirect($url, 303); + return true; + } + } catch (Exception $e) { + // Let the search run naturally + } } + return true; } /** * Make sure necessary tables are filled out. * - * @return boolean hook true + * @return bool hook true */ public function onCheckSchema() { @@ -481,17 +502,17 @@ class ActivityPubPlugin extends Plugin /** * Get remote user's ActivityPub_profile via a identifier * - * @author GNU social - * @author Diogo Cordeiro * @param string $arg A remote user identifier * @return Activitypub_profile|null Valid profile in success | null otherwise + * @author GNU social + * @author Diogo Cordeiro */ public static function pull_remote_profile($arg) { if (preg_match('!^((?:\w+\.)*\w+@(?:\w+\.)*\w+(?:\w+\-\w+)*\.\w+)$!', $arg)) { // webfinger lookup try { - return Activitypub_profile::ensure_web_finger($arg); + return Activitypub_profile::ensure_webfinger($arg); } catch (Exception $e) { common_log(LOG_ERR, 'Webfinger lookup failed for ' . $arg . ': ' . $e->getMessage()); @@ -618,7 +639,7 @@ class ActivityPubPlugin extends Plugin $this->log(LOG_INFO, "Checking webfinger person '$target'"); $profile = null; try { - $aprofile = Activitypub_profile::ensure_web_finger($target); + $aprofile = Activitypub_profile::ensure_webfinger($target); $profile = $aprofile->local_profile(); } catch (Exception $e) { $this->log(LOG_ERR, "Webfinger check failed: " . $e->getMessage()); @@ -751,7 +772,7 @@ class ActivityPubPlugin extends Plugin /** * Try to grab and store the remote profile by the given uri - * + * * @param string $uri * @param Profile &$profile * @return bool @@ -979,7 +1000,7 @@ class ActivityPubPlugin extends Plugin if ($notice->reply_to) { try { $parent_notice = $notice->getParent(); - + try { $other[] = Activitypub_profile::from_profile($parent_notice->getProfile()); } catch (Exception $e) { @@ -1005,7 +1026,7 @@ class ActivityPubPlugin extends Plugin /** * Notify remote followers when a user gets deleted - * + * * @param Action $action * @param User $user user being deleted */ diff --git a/plugins/ActivityPub/classes/Activitypub_profile.php b/plugins/ActivityPub/classes/Activitypub_profile.php index 5891b0b4ee..61c77f5e2b 100644 --- a/plugins/ActivityPub/classes/Activitypub_profile.php +++ b/plugins/ActivityPub/classes/Activitypub_profile.php @@ -331,15 +331,16 @@ class Activitypub_profile extends Managed_DataObject /** * Ensures a valid Activitypub_profile when provided with a valid URI. * - * @author Diogo Cordeiro * @param string $url + * @param bool $grab_online whether to try online grabbing, defaults to true * @return Activitypub_profile * @throws Exception if it isn't possible to return an Activitypub_profile + * @author Diogo Cordeiro */ - public static function fromUri($url) + public static function fromUri($url, $grab_online = true) { try { - return self::from_profile(Activitypub_explorer::get_profile_from_url($url)); + return self::from_profile(Activitypub_explorer::get_profile_from_url($url, $grab_online)); } catch (Exception $e) { throw new Exception('No valid ActivityPub profile found for given URI.'); } @@ -347,17 +348,17 @@ class Activitypub_profile extends Managed_DataObject /** * Look up, and if necessary create, an Activitypub_profile for the remote - * entity with the given webfinger address. + * entity with the given WebFinger address. * This should never return null -- you will either get an object or * an exception will be thrown. * * @author GNU social * @author Diogo Cordeiro - * @param string $addr webfinger address + * @param string $addr WebFinger address * @return Activitypub_profile * @throws Exception on error conditions */ - public static function ensure_web_finger($addr) + public static function ensure_webfinger($addr) { // Normalize $addr, i.e. add 'acct:' if missing $addr = Discovery::normalize($addr); @@ -369,12 +370,12 @@ class Activitypub_profile extends Managed_DataObject if (is_null($uri)) { // Negative cache entry // TRANS: Exception. - throw new Exception(_m('Not a valid webfinger address (via cache).')); + throw new Exception(_m('Not a valid WebFinger address (via cache).')); } try { return self::fromUri($uri); } catch (Exception $e) { - common_log(LOG_ERR, sprintf(__METHOD__ . ': Webfinger address cache inconsistent with database, did not find Activitypub_profile uri==%s', $uri)); + common_log(LOG_ERR, sprintf(__METHOD__ . ': WebFinger address cache inconsistent with database, did not find Activitypub_profile uri==%s', $uri)); self::cacheSet(sprintf('activitypub_profile:webfinger:%s', $addr), false); } } @@ -390,13 +391,13 @@ class Activitypub_profile extends Managed_DataObject // @todo FIXME: Distinguish temporary failures? self::cacheSet(sprintf('activitypub_profile:webfinger:%s', $addr), null); // TRANS: Exception. - throw new Exception(_m('Not a valid webfinger address.')); + throw new Exception(_m('Not a valid WebFinger address.')); } $hints = array_merge( - array('webfinger' => $addr), + ['webfinger' => $addr], DiscoveryHints::fromXRD($xrd) - ); + ); // If there's an Hcard, let's grab its info if (array_key_exists('hcard', $hints)) { @@ -419,17 +420,17 @@ class Activitypub_profile extends Managed_DataObject } catch (Exception $e) { common_log(LOG_WARNING, "Failed creating profile from profile URL '$profileUrl': " . $e->getMessage()); // keep looking - // - // @todo FIXME: This means an error discovering from profile page - // may give us a corrupt entry using the webfinger URI, which - // will obscure the correct page-keyed profile later on. + // + // @todo FIXME: This means an error discovering from profile page + // may give us a corrupt entry using the webfinger URI, which + // will obscure the correct page-keyed profile later on. } } // XXX: try hcard // XXX: try FOAF - // TRANS: Exception. %s is a webfinger address. + // TRANS: Exception. %s is a WebFinger address. throw new Exception(sprintf(_m('Could not find a valid profile for "%s".'), $addr)); } diff --git a/plugins/ActivityPub/lib/explorer.php b/plugins/ActivityPub/lib/explorer.php index 1b39137c80..faa04c21b7 100644 --- a/plugins/ActivityPub/lib/explorer.php +++ b/plugins/ActivityPub/lib/explorer.php @@ -43,16 +43,19 @@ class Activitypub_explorer /** * Shortcut function to get a single profile from its URL. * - * @author Diogo Cordeiro * @param string $url + * @param bool $grab_online whether to try online grabbing, defaults to true * @return Profile - * @throws Exception + * @throws HTTP_Request2_Exception + * @throws NoProfileException + * @throws ServerException + * @author Diogo Cordeiro */ - public static function get_profile_from_url($url) + public static function get_profile_from_url($url, $grab_online = true) { $discovery = new Activitypub_explorer; // Get valid Actor object - $actor_profile = $discovery->lookup($url); + $actor_profile = $discovery->lookup($url, $grab_online); if (!empty($actor_profile)) { return $actor_profile[0]; } @@ -65,14 +68,14 @@ class Activitypub_explorer * so that there is no erroneous data * * @param string $url User's url - * @param bool $remote remote lookup? + * @param bool $grab_online whether to try online grabbing, defaults to true * @return array of Profile objects * @throws HTTP_Request2_Exception * @throws NoProfileException * @throws ServerException * @author Diogo Cordeiro */ - public function lookup(string $url, bool $remote = true) + public function lookup(string $url, bool $grab_online = true) { if (in_array($url, ACTIVITYPUB_PUBLIC_TO)) { return []; @@ -81,7 +84,7 @@ class Activitypub_explorer common_debug('ActivityPub Explorer: Started now looking for '.$url); $this->discovered_actor_profiles = []; - return $this->_lookup($url, $remote); + return $this->_lookup($url, $grab_online); } /** @@ -90,7 +93,7 @@ class Activitypub_explorer * $discovered_actor_profiles array * * @param string $url User's url - * @param bool $remote remote lookup? + * @param bool $grab_online whether to try online grabbing, defaults to true * @return array of Profile objects * @throws HTTP_Request2_Exception * @throws NoProfileException @@ -98,13 +101,13 @@ class Activitypub_explorer * @throws Exception * @author Diogo Cordeiro */ - private function _lookup(string $url, bool $remote) + private function _lookup(string $url, bool $grab_online = true) { $grab_local = $this->grab_local_user($url); // First check if we already have it locally and, if so, return it. // If the local fetch fails and remote grab is required: store locally and return. - if (!$grab_local && (!$remote || !$this->grab_remote_user($url))) { + if (!$grab_local && (!$grab_online || !$this->grab_remote_user($url))) { throw new Exception('User not found.'); } diff --git a/plugins/ActivityPub/tests/Unit/ActivitypubProfileTest.php b/plugins/ActivityPub/tests/Unit/ActivitypubProfileTest.php index 0b84e7c834..babd65209e 100644 --- a/plugins/ActivityPub/tests/Unit/ActivitypubProfileTest.php +++ b/plugins/ActivityPub/tests/Unit/ActivitypubProfileTest.php @@ -101,7 +101,7 @@ class ProfileObjectTest extends TestCase /* Test ensure_web_finger() */ // TODO: Maybe elaborate on this function's tests try { - \Activitypub_profile::ensure_web_finger('test1@testinstance.net'); + \Activitypub_profile::ensure_webfinger('test1@testinstance.net'); $this->assertTrue(false); } catch (\Exception $e) { $this->assertTrue($e->getMessage() == 'Not a valid webfinger address.' ||