chore

Author: tchapi

config/packages/security.yaml

@@ -21,6 +21,8 @@ security:
             logout:
                 path: app_logout
                 target: dashboard
+                # Without this, any page the admin visits can log them out with a plain GET
+                enable_csrf: true
 
 
     access_control:

templates/_partials/navigation.html.twig

@@ -22,7 +22,7 @@
             👤 {{ app.user.username }}
           </a>
           <div class="dropdown-menu" aria-labelledby="navUserMenu">
-            <a class="dropdown-item" href="{{ path('app_logout') }}">{{ "logout"|trans }}</a>
+            <a class="dropdown-item" href="{{ path('app_logout', {_csrf_token: csrf_token('logout')}) }}">{{ "logout"|trans }}</a>
           </div>
         </li>
         <li class="nav-item dropdown">

templates/security/login.html.twig

@@ -5,7 +5,7 @@
 
 {% if app.user %}
     <div class="mb-3">
-        {{ "login.already"|trans({username: app.user.username}) }}, <a href="{{ path('app_logout') }}">{{ "logout"|trans }}</a>
+        {{ "login.already"|trans({username: app.user.username}) }}, <a href="{{ path('app_logout', {_csrf_token: csrf_token('logout')}) }}">{{ "logout"|trans }}</a>
     </div>
 {% else %}
     <div class="row justify-content-md-center">

tests/Functional/Controllers/AddressBookControllerTest.php

@@ -206,4 +206,15 @@ public function testAddressBookWithoutADisplayNameFallsBackToItsUri(): void
         $this->assertResponseIsSuccessful();
         $this->assertAnySelectorTextContains('h5', 'nameless-book');
     }
+
+    public function testAddressBookPagesAreNotReachableAnonymously(): void
+    {
+        $client = static::createClient();
+
+        foreach (['/addressbooks/1', '/addressbooks/1/new', '/addressbooks/1/edit/1'] as $url) {
+            $client->request('GET', $url);
+
+            $this->assertResponseRedirects('/login', null, $url.' must not be public');
+        }
+    }
 }

tests/Functional/Controllers/CalendarControllerTest.php

@@ -313,4 +313,15 @@ public function testCalendarNewIgnoresASubmittedOwner(): void
         $calendarRepository = static::getContainer()->get(CalendarInstanceRepository::class);
         $this->assertNull($calendarRepository->findOneBy(['uri' => 'hijack']));
     }
+
+    public function testCalendarPagesAreNotReachableAnonymously(): void
+    {
+        $client = static::createClient();
+
+        foreach (['/calendars/1', '/calendars/1/new', '/calendars/1/edit/1', '/calendars/1/shares/1'] as $url) {
+            $client->request('GET', $url);
+
+            $this->assertResponseRedirects('/login', null, $url.' must not be public');
+        }
+    }
 }

tests/Functional/Controllers/DashboardTest.php

@@ -2,6 +2,7 @@
 
 namespace App\Tests\Functional;
 
+use App\Security\AdminUser;
 use Symfony\Bundle\FrameworkBundle\Test\WebTestCase;
 
 class DashboardTest extends WebTestCase
@@ -91,4 +92,31 @@ public function testLoginCorrect(): void
         $this->assertSelectorTextContains('h3.environment', 'Configured environment');
         $this->assertSelectorExists('nav.navbar');
     }
+
+    /**
+     * A plain `GET /logout` from any page the admin happens to visit used to end their session.
+     */
+    public function testLogoutRequiresACsrfToken(): void
+    {
+        $client = static::createClient();
+        $client->loginUser(new AdminUser('admin', 'test'));
+
+        $client->request('GET', '/logout');
+        $this->assertResponseStatusCodeSame(403);
+
+        $client->request('GET', '/dashboard');
+        $this->assertResponseIsSuccessful('The session must survive a logout without a token');
+    }
+
+    public function testLogoutWorksFromTheMenuLink(): void
+    {
+        $client = static::createClient();
+        $client->loginUser(new AdminUser('admin', 'test'));
+
+        $crawler = $client->request('GET', '/dashboard');
+        $client->click($crawler->filter('a.dropdown-item')->selectLink('Logout')->link());
+
+        $client->request('GET', '/dashboard');
+        $this->assertResponseRedirects('/login');
+    }
 }

tests/Functional/Controllers/UserControllerTest.php

@@ -253,4 +253,19 @@ public function testUserCreationRejectsAUsernameThatBreaksThePrincipalUri(): voi
             static::getContainer()->get('doctrine.orm.entity_manager')->getRepository(User::class)->findOneByUsername('bad/user')
         );
     }
+
+    /**
+     * Every admin test authenticates first, so a missing `access_control` entry would go
+     * unnoticed — which is exactly how the `^/adressbooks` typo of #268 shipped.
+     */
+    public function testUserPagesAreNotReachableAnonymously(): void
+    {
+        $client = static::createClient();
+
+        foreach (['/users/', '/users/new', '/users/edit/1', '/users/delegates/1'] as $url) {
+            $client->request('GET', $url);
+
+            $this->assertResponseRedirects('/login', null, $url.' must not be public');
+        }
+    }
 }