Fix stuff

- fix conversion errors while handling `UpdateActorMessage`s - fix errors when handling add and remove messages (array access, not flushing entities) - specify when to ignore an `MagazineModeratorAddedEvent` and `MagazineModeratorRemovedEvent` - correctly display the moderator count (ignore the owner for remote magazines) - add a uuid as id in `MagazineModeratorsController` so no server will cache the response
MbinOrg · Nov 12, 2023 · 70f6c11 · 70f6c11
1 parent 6c92f32
commit 70f6c11
Show file tree

Hide file tree

Showing 11 changed files with 61 additions and 72 deletions.
diff --git a/src/Controller/ActivityPub/Magazine/MagazineModeratorsController.php b/src/Controller/ActivityPub/Magazine/MagazineModeratorsController.php
@@ -6,12 +6,14 @@
 
 use App\Entity\Contracts\ActivityPubActivityInterface;
 use App\Entity\Magazine;
+use App\Entity\Moderator;
 use App\Repository\MagazineRepository;
 use App\Service\ActivityPubManager;
 use JetBrains\PhpStorm\ArrayShape;
 use Symfony\Component\HttpFoundation\JsonResponse;
 use Symfony\Component\HttpFoundation\Request;
 use Symfony\Component\Routing\Generator\UrlGeneratorInterface;
+use Symfony\Component\Uid\Uuid;
 
 class MagazineModeratorsController
 {
@@ -40,23 +42,21 @@ public function __invoke(Magazine $magazine, Request $request): JsonResponse
     ])]
     private function getCollectionItems(Magazine $magazine): array
     {
-        $moderatorsCount = $this->magazineRepository->findModerators($magazine)->count();
-        $moderators = $this->magazineRepository->findModerators($magazine, perPage: $moderatorsCount);
-        $actors = array_map(fn ($mod) => $mod->user, iterator_to_array($moderators->getCurrentPageResults()));
+        $moderators = $this->magazineRepository->findModerators($magazine, perPage: $magazine->moderators->count());
 
         $items = [];
-        foreach ($actors as $actor) {
+        foreach ($moderators->getCurrentPageResults() as /* @var Moderator $mod */ $mod) {
+            $actor = $mod->user;
             $items[] = $this->manager->getActorProfileId($actor);
         }
 
-        $routeName = 'ap_magazine_moderators';
-        $routeParams = ['name' => $magazine->name];
+        $id = Uuid::v4()->toRfc4122();
 
         return [
             '@context' => ActivityPubActivityInterface::CONTEXT_URL,
             'type' => 'OrderedCollection',
-            'id' => $this->urlGenerator->generate($routeName, $routeParams, UrlGeneratorInterface::ABSOLUTE_URL),
-            'totalItems' => $moderatorsCount,
+            'id' => $this->urlGenerator->generate('ap_object', ['id' => $id], UrlGeneratorInterface::ABSOLUTE_URL),
+            'totalItems' => \sizeof($items),
             'orderedItems' => $items,
         ];
     }

diff --git a/src/Controller/Magazine/MagazineModController.php b/src/Controller/Magazine/MagazineModController.php
@@ -6,7 +6,6 @@
 
 use App\Controller\AbstractController;
 use App\Entity\Magazine;
-use App\Entity\Moderator;
 use App\Repository\MagazineRepository;
 use Symfony\Component\HttpFoundation\Request;
 use Symfony\Component\HttpFoundation\Response;
@@ -15,19 +14,13 @@ class MagazineModController extends AbstractController
 {
     public function __invoke(Magazine $magazine, MagazineRepository $repository, Request $request): Response
     {
-        $moderatorsWithoutOwner = [];
-        foreach ($repository->findModerators($magazine, $this->getPageNb($request)) as /* @var $mod Moderator */ $mod) {
-            // only include the owner if it is a local magazine, for remote magazines the owner is always the admin
-            if (!$mod->isOwner or null === $magazine->apId) {
-                $moderatorsWithoutOwner[] = $mod;
-            }
-        }
+        $moderators = $repository->findModerators($magazine, $this->getPageNb($request));
 
         return $this->render(
             'magazine/moderators.html.twig',
             [
                 'magazine' => $magazine,
-                'moderators' => $moderatorsWithoutOwner,
+                'moderators' => $moderators,
             ]
         );
     }

diff --git a/src/Entity/Magazine.php b/src/Entity/Magazine.php
@@ -191,20 +191,6 @@ public function getUserAsModeratorOrNull(User $user): ?Moderator
         return null;
     }
 
-    public function removeUserAsModerator(User $user): void
-    {
-        $user->moderatorTokens->get(-1);
-
-        $criteria = Criteria::create()
-            ->where(Criteria::expr()->eq('magazine', $this));
-
-        foreach ($user->moderatorTokens->matching($criteria) as $item) {
-            /** @var Moderator $mod */
-            $mod = $item;
-            $this->moderators->remove($mod->getId());
-        }
-    }
-
     public function userIsOwner(User $user): bool
     {
         $user->moderatorTokens->get(-1);
@@ -232,6 +218,14 @@ public function getOwner(): User
         return $this->moderators->matching($criteria)->first()->user;
     }
 
+    public function getModeratorCount(): int
+    {
+        $criteria = Criteria::create()
+            ->where(Criteria::expr()->eq('isOwner', false));
+
+        return $this->moderators->matching($criteria)->count();
+    }
+
     public function addEntry(Entry $entry): self
     {
         if (!$this->entries->contains($entry)) {
@@ -330,7 +324,7 @@ public function updateSubscriptionsCount(): void
     {
         if (null !== $this->apFollowersCount) {
             $criteria = Criteria::create()
-                ->where(Criteria::expr()->gt('createdAt', $this->apFetchedAt));
+                ->where(Criteria::expr()->gt('createdAt', \DateTimeImmutable::createFromMutable($this->apFetchedAt)));
 
             $newSubscribers = $this->subscriptions->matching($criteria)->count();
             $this->subscriptionsCount = $this->apFollowersCount + $newSubscribers;

diff --git a/src/Entity/User.php b/src/Entity/User.php
@@ -465,8 +465,10 @@ public function isFollowing(User $user): bool
     public function updateFollowCounts(): void
     {
         if (null !== $this->apFollowersCount) {
-            $criteria = Criteria::create()
-                ->where(Criteria::expr()->gt('createdAt', $this->apFetchedAt));
+            $criteria = Criteria::create();
+            if ($this->apFetchedAt) {
+                $criteria->where(Criteria::expr()->gt('createdAt', \DateTimeImmutable::createFromMutable($this->apFetchedAt)));
+            }
 
             $newFollowers = $this->followers->matching($criteria)->count();
             $this->followersCount = $this->apFollowersCount + $newFollowers;

diff --git a/src/EventSubscriber/ActivityPub/MagazineModeratorAddedRemovedSubscriber.php b/src/EventSubscriber/ActivityPub/MagazineModeratorAddedRemovedSubscriber.php
@@ -20,16 +20,16 @@ public function __construct(
 
     public function onModeratorAdded(MagazineModeratorAddedEvent $event): void
     {
-        // if the magazine is local then we have authority over it, otherwise the addedBy user has to be defined
-        if (!$event->magazine->apId or null !== $event->addedBy) {
+        // if the magazine is local then we have authority over it, otherwise the addedBy user has to be a local user
+        if (!$event->magazine->apId or (null !== $event->addedBy and !$event->addedBy->apId)) {
             $this->bus->dispatch(new AddMessage($event->addedBy->getId(), $event->magazine->getId(), $event->user->getId()));
         }
     }
 
     public function onModeratorRemoved(MagazineModeratorRemovedEvent $event): void
     {
-        // if the magazine is local then we have authority over it, otherwise the removedBy user has to be defined
-        if (!$event->magazine->apId or null !== $event->removedBy) {
+        // if the magazine is local then we have authority over it, otherwise the removedBy user has to be a local user
+        if (!$event->magazine->apId or (null !== $event->removedBy and !$event->removedBy->apId)) {
             $this->bus->dispatch(new RemoveMessage($event->removedBy->getId(), $event->magazine->getId(), $event->user->getId()));
         }
     }

diff --git a/src/MessageHandler/ActivityPub/Inbox/AddHandler.php b/src/MessageHandler/ActivityPub/Inbox/AddHandler.php
@@ -5,7 +5,6 @@
 namespace App\MessageHandler\ActivityPub\Inbox;
 
 use App\DTO\ModeratorDto;
-use App\Entity\Moderator;
 use App\Message\ActivityPub\Inbox\AddMessage;
 use App\Repository\MagazineRepository;
 use App\Service\ActivityPubManager;
@@ -26,28 +25,24 @@ public function __construct(
 
     public function __invoke(AddMessage $message): void
     {
-        $actor = $this->activityPubManager->findUserActorOrCreateOrThrow($message['actor']);
-        $targetMag = $this->magazineRepository->getMagazineFromModeratorsUrl($message['target']);
+        $payload = $message->payload;
+        $actor = $this->activityPubManager->findUserActorOrCreateOrThrow($payload['actor']);
+        $targetMag = $this->magazineRepository->getMagazineFromModeratorsUrl($payload['target']);
         if (!$targetMag) {
-            throw new \LogicException("could not find a magazine with moderators url like: '{$message['target']}'");
+            throw new \LogicException("could not find a magazine with moderators url like: '{$payload['target']}'");
         }
         if (!$targetMag->userIsModerator($actor) and !$targetMag->hasSameHostAsUser($actor)) {
             throw new \LogicException("the user '$actor->username' ({$actor->getId()}) is not a moderator of $targetMag->name ({$targetMag->getId()}) and is not from the same instance. He can therefore not add moderators");
         }
 
-        $object = $this->activityPubManager->findUserActorOrCreateOrThrow($message['object']);
+        $object = $this->activityPubManager->findUserActorOrCreateOrThrow($payload['object']);
 
         if ($targetMag->userIsModerator($object)) {
-            $this->logger->warning("the user '$object->username' ({$object->getId()}) already is a moderator of $targetMag->name ({$targetMag->getId()}. Discarding message");
-        }
-        $this->logger->info("'$actor->username' ({$actor->getId()}) added '$object->username' ({$object->getId()}) as moderator to '$targetMag->name' ({$targetMag->getId()}");
-        if ($targetMag->apId) {
-            // don't federate the added moderator if the magazine is not local
-            $targetMag->addModerator(new Moderator($targetMag, $object, $actor, false, true));
-        } else {
-            // use the manager to trigger outbox federation for the new moderator
-            // use case for this: a remote moderator added a new moderator
-            $this->magazineManager->addModerator(new ModeratorDto($targetMag, $object, $actor));
+            $this->logger->warning("the user '$object->username' ({$object->getId()}) already is a moderator of $targetMag->name ({$targetMag->getId()}). Discarding message");
+
+            return;
         }
+        $this->logger->info("'$actor->username' ({$actor->getId()}) added '$object->username' ({$object->getId()}) as moderator to '$targetMag->name' ({$targetMag->getId()})");
+        $this->magazineManager->addModerator(new ModeratorDto($targetMag, $object, $actor));
     }
 }
diff --git a/src/MessageHandler/ActivityPub/Inbox/RemoveHandler.php b/src/MessageHandler/ActivityPub/Inbox/RemoveHandler.php
@@ -24,29 +24,23 @@ public function __construct(
 
     public function __invoke(RemoveMessage $message): void
     {
-        $actor = $this->activityPubManager->findUserActorOrCreateOrThrow($message['actor']);
-        $targetMag = $this->magazineRepository->getMagazineFromModeratorsUrl($message['target']);
+        $payload = $message->payload;
+        $actor = $this->activityPubManager->findUserActorOrCreateOrThrow($payload['actor']);
+        $targetMag = $this->magazineRepository->getMagazineFromModeratorsUrl($payload['target']);
         if (!$targetMag) {
-            throw new \LogicException("could not find a magazine with moderators url like: '{$message['target']}'");
+            throw new \LogicException("could not find a magazine with moderators url like: '{$payload['target']}'");
         }
         if (!$targetMag->userIsModerator($actor) and !$targetMag->hasSameHostAsUser($actor)) {
             throw new \LogicException("the user '$actor->username' ({$actor->getId()}) is not a moderator of $targetMag->name ({$targetMag->getId()}) and is not from the same instance. He can therefore not remove moderators");
         }
 
-        $object = $this->activityPubManager->findUserActorOrCreateOrThrow($message['object']);
+        $object = $this->activityPubManager->findUserActorOrCreateOrThrow($payload['object']);
         $objectMod = $targetMag->getUserAsModeratorOrNull($object);
 
         if (null === $objectMod) {
-            $this->logger->warning("the user '$object->username' ({$object->getId()}) is not a moderator of $targetMag->name ({$targetMag->getId()} and can therefore not be removed as one. Discarding message");
-        }
-        $this->logger->info("'$actor->username' ({$actor->getId()}) removed '$object->username' ({$object->getId()}) as moderator from '$targetMag->name' ({$targetMag->getId()}");
-        if ($targetMag->apId) {
-            // don't federate the added moderator if the magazine is not local
-            $targetMag->removeUserAsModerator($object);
-        } else {
-            // use the manager to trigger outbox federation for the new moderator
-            // use case for this: a remote moderator added a new moderator
-            $this->magazineManager->removeModerator($objectMod);
+            $this->logger->warning("the user '$object->username' ({$object->getId()}) is not a moderator of $targetMag->name ({$targetMag->getId()}) and can therefore not be removed as one. Discarding message");
         }
+        $this->logger->info("'$actor->username' ({$actor->getId()}) removed '$object->username' ({$object->getId()}) as moderator from '$targetMag->name' ({$targetMag->getId()})");
+        $this->magazineManager->removeModerator($objectMod, $actor);
     }
 }
diff --git a/src/Repository/MagazineRepository.php b/src/Repository/MagazineRepository.php
@@ -202,6 +202,10 @@ public function findModerators(
     ): PagerfantaInterface {
         $criteria = Criteria::create()->orderBy(['createdAt' => 'ASC']);
 
+        if ($magazine->apId) {
+            $criteria->where(Criteria::expr()->eq('isOwner', false));
+        }
+
         $moderators = new Pagerfanta(new SelectableAdapter($magazine->moderators, $criteria));
         try {
             $moderators->setMaxPerPage($perPage);

diff --git a/src/Service/ActivityPubManager.php b/src/Service/ActivityPubManager.php
@@ -26,6 +26,7 @@
 use App\Service\ActivityPub\ApHttpClient;
 use App\Service\ActivityPub\Webfinger\WebFinger;
 use App\Service\ActivityPub\Webfinger\WebFingerFactory;
+use Doctrine\Common\Collections\Criteria;
 use Doctrine\ORM\EntityManagerInterface;
 use League\HTMLToMarkdown\HtmlConverter;
 use Psr\Log\LoggerInterface;
@@ -428,6 +429,8 @@ public function updateMagazine(string $actorUrl): ?Magazine
                         $items = $attributedObj['orderedItems'];
                     }
 
+                    $this->logger->debug("got moderator items for magazine '$magazine->name': ".json_encode($attributedObj));
+
                     if (null !== $items) {
                         $moderatorsToRemove = [];
                         foreach ($magazine->moderators as /* @var $mod Moderator */ $mod) {
@@ -448,6 +451,7 @@ public function updateMagazine(string $actorUrl): ?Magazine
                                         }
                                     }
                                     if (!$magazine->userIsModerator($user)) {
+                                        $this->logger->info("adding '$user->username' as moderator in '$magazine->name' because they are a mod upstream, but not locally");
                                         $this->magazineManager->addModerator(new ModeratorDto($magazine, $user, null));
                                     }
                                 }
@@ -462,7 +466,10 @@ public function updateMagazine(string $actorUrl): ?Magazine
                             if (null === $modToRemove) {
                                 continue;
                             }
-                            $magazine->removeUserAsModerator($modToRemove);
+                            $criteria = Criteria::create()->where(Criteria::expr()->eq('magazine', $magazine));
+                            $modObject = $modToRemove->moderatorTokens->matching($criteria)->first();
+                            $this->logger->info("removing '$modToRemove->username' from '$magazine->name' as mod locally because they are no longer mod upstream");
+                            $this->magazineManager->removeModerator($modObject, null);
                         }
                     } else {
                         $this->logger->warning("could not update the moderators of $actorUrl, the response doesn't have a 'items' or 'orderedItems' property or it is not an array");

diff --git a/src/Service/MagazineManager.php b/src/Service/MagazineManager.php
@@ -240,15 +240,15 @@ private function clearCommentsCache(User $user)
         ]);
     }
 
-    public function removeModerator(Moderator $moderator): void
+    public function removeModerator(Moderator $moderator, ?User $removedBy): void
     {
         $user = $moderator->user;
 
         $this->entityManager->remove($moderator);
         $this->entityManager->flush();
 
         $this->clearCommentsCache($user);
-        $this->dispatcher->dispatch(new MagazineModeratorRemovedEvent($moderator->magazine, $moderator->user, $moderator->addedByUser));
+        $this->dispatcher->dispatch(new MagazineModeratorRemovedEvent($moderator->magazine, $moderator->user, $removedBy));
     }
 
     public function changeTheme(MagazineThemeDto $dto): Magazine
@@ -329,7 +329,7 @@ public function toggleOwnershipRequest(Magazine $magazine, User $user): void
 
     public function acceptOwnershipRequest(Magazine $magazine, User $user, ?User $addedBy): void
     {
-        $this->removeModerator($magazine->getOwnerModerator());
+        $this->removeModerator($magazine->getOwnerModerator(), $addedBy);
 
         $this->addModerator(new ModeratorDto($magazine, $user, $addedBy), true);
 

diff --git a/templates/components/magazine_box.html.twig b/templates/components/magazine_box.html.twig
@@ -48,7 +48,7 @@
             {{ _self.meta_item( 'comments'|trans, path('magazine_entry_comments', {'name': computed.magazine.name}), computed.magazine.entryCommentCount) }}
             {{ _self.meta_item( 'posts'|trans, path('magazine_posts', {'name': computed.magazine.name}), computed.magazine.postCount) }}
             {{ _self.meta_item( 'replies'|trans, path('magazine_posts', {'name': computed.magazine.name}), computed.magazine.postCommentCount) }}
-            {{ _self.meta_item( 'moderators'|trans, path('magazine_moderators', {'name': computed.magazine.name}), computed.magazine.moderators|length) }}
+            {{ _self.meta_item( 'moderators'|trans, path('magazine_moderators', {'name': computed.magazine.name}), computed.magazine.getModeratorCount()) }}
             {{ _self.meta_item( 'mod_log'|trans, path('magazine_modlog', {'name': computed.magazine.name}), computed.magazine.logs|length) }}
         </ul>
     {% endif %}