[CORE][Actor][Settings] Fix nickname change and refactor Core Form::handle so it's harder to repeat these mistakes again

Minor improvements on actor->getLocal

components/Group/Controller/Group.php

@@ -99,7 +99,7 @@ class Group extends Controller
             return [
                 '_template'          => 'group/settings.html.twig',
                 'group'              => $group_actor,
-                'personal_info_form' => ActorForms::personalInfo($request, $actor, $local_group)->createView(),
+                'personal_info_form' => ActorForms::personalInfo(request: $request, scope: $actor, target: $group_actor)->createView(),
                 'open_details_query' => $this->string('open'),
             ];
         } else {

components/Group/Entity/LocalGroup.php

@@ -117,7 +117,7 @@ class LocalGroup extends Entity
 
     public function getActor()
     {
-        return DB::findOneBy('actor', ['id' => $this->actor_id]);
+        return DB::findOneBy(Actor::class, ['id' => $this->actor_id]);
     }
 
     public static function getByNickname(string $nickname): ?self

components/Person/Controller/PersonSettings.php

@@ -94,7 +94,7 @@ class PersonSettings extends Controller
         $user  = Common::ensureLoggedIn();
         $actor = $user->getActor();
 
-        $personal_form            = ActorForms::personalInfo($request, $actor, $user);
+        $personal_form            = ActorForms::personalInfo(request: $request, scope: $actor, target: $actor);
         $email_form               = self::email($request);
         $password_form            = self::password($request);
         $notifications_form_array = self::notifications($request);

src/Core/Form.php

@@ -136,12 +136,21 @@ abstract class Form
     }
 
     /**
-     * Handle the full life cycle of a form. Creates it with @see
+     * Handle the full life cycle of a form. Creates it with @param array $form_definition
+     * @param Request $request
+     * @param object|null $target
+     * @param array $extra_args If specified, is used as $target->set($data[$key], $extra_args[$key])
+     * @param callable|null $after_step
+     * @param array $create_args
+     * @param SymfForm|null $testing_only_form
+     * @return mixed
+     * @throws RedirectException
+     * @throws ServerException
+     * @see
      * self::create and inserts the submitted values into the database
      *
-     * @throws ServerException
      */
-    public static function handle(array $form_definition, Request $request, ?object $target, array $extra_args = [], ?callable $extra_step = null, array $create_args = [], ?SymfForm $testing_only_form = null): mixed
+    public static function handle(array $form_definition, Request $request, ?object $target, array $extra_args = [], ?callable $before_step = null, ?callable $after_step = null, array $create_args = [], ?SymfForm $testing_only_form = null): mixed
     {
         $form = $testing_only_form ?? self::create($form_definition, $target, ...$create_args);
 
@@ -162,6 +171,13 @@ abstract class Form
                 }
 
                 unset($data['translation_domain'], $data['save']);
+
+                if (isset($before_step)) {
+                    // @codeCoverageIgnoreStart
+                    $before_step($data, $extra_args);
+                    // @codeCoverageIgnoreEnd
+                }
+
                 foreach ($data as $key => $val) {
                     $method = 'set' . ucfirst(Formatting::snakeCaseToCamelCase($key));
                     if (method_exists($target, $method)) {
@@ -175,9 +191,9 @@ abstract class Form
                     }
                 }
 
-                if (isset($extra_step)) {
+                if (isset($after_step)) {
                     // @codeCoverageIgnoreStart
-                    $extra_step($data, $extra_args);
+                    $after_step($data, $extra_args);
                     // @codeCoverageIgnoreEnd
                 }
 

src/Entity/Actor.php

@@ -32,10 +32,12 @@ use App\Core\Router\Router;
 use App\Util\Exception\BugFoundException;
 use App\Util\Exception\NicknameException;
 use App\Util\Exception\NotFoundException;
+use App\Util\Exception\NotImplementedException;
 use App\Util\Formatting;
 use App\Util\Nickname;
 use Component\Avatar\Avatar;
 use Component\Group\Entity\GroupMember;
+use Component\Group\Entity\LocalGroup;
 use Component\Language\Entity\ActorLanguage;
 use Component\Language\Entity\Language;
 use Component\Subscription\Entity\ActorSubscription;
@@ -264,14 +266,25 @@ class Actor extends Entity
         ];
     }
 
-    public function getLocalUser(): ?LocalUser
+    /**
+     * Note: You will receive a Local from Database, it's a persisted instance
+     *
+     * @return LocalUser|LocalGroup  Returns the local, if actor is local, null otherwise
+     */
+    public function getLocal(): LocalUser|LocalGroup|null
     {
         if ($this->getIsLocal()) {
-            return DB::findOneBy('local_user', ['id' => $this->getId()]);
+            switch ($this->getType()) {
-        } else {
+                case Actor::PERSON:
-            throw new NotFoundException('This is a remote actor.');
+                    return DB::findOneBy(LocalUser::class, ['id' => $this->getId()]);
+                case Actor::GROUP:
+                    return DB::findOneBy(LocalGroup::class, ['actor_id' => $this->getId()]);
+                case Actor::BOT:
+                    throw new NotImplementedException('We don\'t exactly have a local application abstraction yet...');
             }
         }
+        return null;
+    }
 
     public function getAvatarUrl(string $size = 'medium')
     {

src/Util/Form/ActorForms.php

@@ -59,7 +59,10 @@ class ActorForms
     /**
      * Actor personal information panel
      *
-     * @throws \App\Util\Exception\NicknameException
+     * @param Request $request
+     * @param Actor $scope The perpetrator of the change
+     * @param Actor $target The victim of changes
+     * @return mixed
      * @throws ClientException
      * @throws NicknameEmptyException
      * @throws NicknameInvalidException
@@ -67,9 +70,15 @@ class ActorForms
      * @throws NicknameTakenException
      * @throws NicknameTooLongException
      * @throws ServerException
+     * @throws \App\Util\Exception\NicknameException
      */
-    public static function personalInfo(Request $request, Actor $target, LocalUser|LocalGroup $user): mixed
+    public static function personalInfo(Request $request, Actor $scope, Actor $target): mixed
     {
+        // Is $target in $scope's sight?
+        if (!$scope->canModerate($target)) {
+            throw new ClientException(_m('You do not have permissions to change :nickname\'s settings', [':nickname' => $target->getNickname()]));
+        }
+
         // Defining the various form fields
         $form_definition = [
             ['nickname', TextType::class, ['label' => _m('Nickname'), 'required' => true, 'help' => _m('1-64 lowercase letters or numbers, no punctuation or spaces.')]],
@@ -82,14 +91,26 @@ class ActorForms
         ];
 
         // Setting nickname normalised and setting actor cache
-        $extra_step = static function ($data, $extra_args) use ($user, $target) {
+        $before_step = static function ($data, $extra_args) use ($target) {
-            if (!strcmp($user->getNickname(), $data['nickname'])) {
+            if ($target->getNickname() !== $data['nickname']) {
-                $data['nickname'] = Nickname::normalize($data['nickname'], check_already_used: false, which: Nickname::CHECK_LOCAL_GROUP, check_is_allowed: true);
+                // We must only check if is both already used and allowed if the actor is local
+                $check_is_allowed = $target->getIsLocal();
+                if ($target->isGroup()) {
+                    $data['nickname'] = Nickname::normalize($data['nickname'], check_already_used: $check_is_allowed, which: Nickname::CHECK_LOCAL_GROUP, check_is_allowed: $check_is_allowed);
+                } else {
+                    $data['nickname'] = Nickname::normalize($data['nickname'], check_already_used: $check_is_allowed, which: Nickname::CHECK_LOCAL_USER, check_is_allowed: $check_is_allowed);
                 }
-            if (!strcmp($target->getFullname(), $data['full_name'])) {
+
+                // We will set $target actor's nickname in the form::handle,
+                // but if it is local, we must update the local reference as well
+                if (!is_null($local = $target->getLocal())) {
+                    $local->setNickname($data['nickname']);
+                }
+            }
+            if ($target->getFullname() !== $data['full_name']) {
                 $data['full_name'] = trim($data['full_name']);
                 if (mb_strlen($data['full_name']) > 64) {
-                    throw new ClientException('Fullname cannot be more than 64 character long.');
+                    throw new ClientException(_m('Full name cannot be more than 64 character long.'));
                 }
             }
 
@@ -104,7 +125,7 @@ class ActorForms
             $request,
             target: $target,
             extra_args: [],
-            extra_step: $extra_step,
+            before_step: $before_step,
         );
     }
 }

tests/Core/FormTest.php

@@ -115,12 +115,12 @@ class FormTest extends GNUsocialTestCase
         $mock_form->method('isSubmitted')->willReturn(true);
         $mock_form->method('isValid')->willReturn(true);
         $mock_form->method('getData')->willReturn($data);
-        $ret = Form::handle(form_definition: [/* not normal usage */], request: $mock_request, target: null, extra_args: [], extra_step: null, create_args: [], testing_only_form: $mock_form);
+        $ret = Form::handle(form_definition: [/* not normal usage */], request: $mock_request, target: null, extra_args: [], before_step: null, after_step: null, create_args: [], testing_only_form: $mock_form);
         static::assertSame($data, $ret);
 
         $actor = Actor::create(['nickname' => 'form_testing_new_user', 'is_local' => false]);
         DB::persist($actor);
-        $ret = Form::handle(form_definition: [/* not normal usage */], request: $mock_request, target: $actor, extra_args: [], extra_step: null, create_args: [], testing_only_form: $mock_form);
+        $ret = Form::handle(form_definition: [/* not normal usage */], request: $mock_request, target: $actor, extra_args: [], before_step: null, after_step: null, create_args: [], testing_only_form: $mock_form);
         static::assertSame($mock_form, $ret);
         static::assertSame($data['fullname'], $actor->getFullname());
         static::assertSame($data['homepage'], $actor->getHomepage());