From 1605d4fb11a92fb37b465d25fba8f1afca4a53ae Mon Sep 17 00:00:00 2001
From: Simon Walker <github@stwalkerster.co.uk>
Date: Tue, 29 Aug 2023 00:18:14 +0100
Subject: [PATCH] Check user still has access to domain on page load

Fixes #836
---
 includes/Pages/PageDomainSwitch.php           |  2 +-
 includes/Security/DomainAccessManager.php     | 14 ++-----------
 includes/Security/SecurityManager.php         | 21 ++++++++++++++++++-
 includes/WebStart.php                         |  7 ++++---
 .../includes/Security/SecurityManagerTest.php | 12 +++++++++--
 5 files changed, 37 insertions(+), 19 deletions(-)

diff --git a/includes/Pages/PageDomainSwitch.php b/includes/Pages/PageDomainSwitch.php
index 431b4a852..20d346238 100644
--- a/includes/Pages/PageDomainSwitch.php
+++ b/includes/Pages/PageDomainSwitch.php
@@ -39,7 +39,7 @@ protected function main()
             return;
         }
 
-        $this->getDomainAccessManager()->switchDomain($currentUser, $newDomain);
+        $this->getDomainAccessManager()->switchDomain($currentUser, $newDomain, $this->getSecurityManager());
 
         // try to stay on the same page if possible.
         // This only checks basic ACLs and not domain privileges, so this may still result in a 403.
diff --git a/includes/Security/DomainAccessManager.php b/includes/Security/DomainAccessManager.php
index 79d14aa45..61346124f 100644
--- a/includes/Security/DomainAccessManager.php
+++ b/includes/Security/DomainAccessManager.php
@@ -17,16 +17,6 @@
 
 class DomainAccessManager
 {
-    /**
-     * @var SecurityManager
-     */
-    private $securityManager;
-
-    public function __construct(SecurityManager $securityManager)
-    {
-        $this->securityManager = $securityManager;
-    }
-
     /**
      * @param User $user
      *
@@ -41,7 +31,7 @@ public function getAllowedDomains(User $user): array
         return Domain::getDomainByUser($user->getDatabase(), $user, true);
     }
 
-    public function switchDomain(User $user, Domain $newDomain): void
+    public function switchDomain(User $user, Domain $newDomain, SecurityManager $securityManager): void
     {
         $mapToId = function(DataObject $object) {
             return $object->getId();
@@ -53,7 +43,7 @@ public function switchDomain(User $user, Domain $newDomain): void
             WebRequest::setActiveDomain($newDomain);
         }
         else {
-            throw new AccessDeniedException($this->securityManager, $this);
+            throw new AccessDeniedException($securityManager, $this);
         }
     }
 
diff --git a/includes/Security/SecurityManager.php b/includes/Security/SecurityManager.php
index 20a898f84..41edb07c0 100644
--- a/includes/Security/SecurityManager.php
+++ b/includes/Security/SecurityManager.php
@@ -11,6 +11,8 @@
 use Waca\DataObjects\Domain;
 use Waca\DataObjects\User;
 use Waca\DataObjects\UserRole;
+use Waca\Exceptions\AccessDeniedException;
+use Waca\Exceptions\ApplicationLogicException;
 use Waca\IdentificationVerifier;
 
 final class SecurityManager
@@ -25,6 +27,8 @@ final class SecurityManager
      */
     private $roleConfiguration;
 
+    private DomainAccessManager $domainAccessManager;
+
     private $cache = [];
 
     /**
@@ -32,13 +36,16 @@ final class SecurityManager
      *
      * @param IdentificationVerifier $identificationVerifier
      * @param RoleConfiguration      $roleConfiguration
+     * @param DomainAccessManager    $domainAccessManager
      */
     public function __construct(
         IdentificationVerifier $identificationVerifier,
-        RoleConfiguration $roleConfiguration
+        RoleConfiguration $roleConfiguration,
+        DomainAccessManager $domainAccessManager
     ) {
         $this->identificationVerifier = $identificationVerifier;
         $this->roleConfiguration = $roleConfiguration;
+        $this->domainAccessManager = $domainAccessManager;
     }
 
     /**
@@ -181,6 +188,18 @@ public function getActiveRoles(User $user, &$activeRoles, &$inactiveRoles)
 
             if ($user->isActive()) {
                 $domain = Domain::getCurrent($user->getDatabase());
+
+                $userDomains = array_map(fn(Domain $d) => $d->getId(), $this->domainAccessManager->getAllowedDomains($user));
+
+                if (!in_array($domain->getId(), $userDomains)) {
+                    $this->domainAccessManager->switchToDefaultDomain($user);
+                    $domain = Domain::getCurrent($user->getDatabase());
+                }
+
+                if (!in_array($domain->getId(), $userDomains)) {
+                    throw new AccessDeniedException($this, $this->domainAccessManager);
+                }
+
                 $ur = UserRole::getForUser($user->getId(), $user->getDatabase(), $domain->getId());
 
                 // NOTE: public is still in this array.
diff --git a/includes/WebStart.php b/includes/WebStart.php
index 1b0122ef0..071964178 100644
--- a/includes/WebStart.php
+++ b/includes/WebStart.php
@@ -78,7 +78,10 @@ protected function setupHelpers(
                 $page->setTypeAheadHelper(new TypeAheadHelper());
 
                 $identificationVerifier = new IdentificationVerifier($page->getHttpHelper(), $siteConfiguration, $database);
-                $page->setSecurityManager(new SecurityManager($identificationVerifier, new RoleConfiguration()));
+
+                $domainAccessManager = new DomainAccessManager();
+                $page->setDomainAccessManager($domainAccessManager);
+                $page->setSecurityManager(new SecurityManager($identificationVerifier, new RoleConfiguration(), $domainAccessManager));
 
                 if ($siteConfiguration->getTitleBlacklistEnabled()) {
                     $page->setBlacklistHelper(new BlacklistHelper($page->getHttpHelper(), $database));
@@ -86,8 +89,6 @@ protected function setupHelpers(
                 else {
                     $page->setBlacklistHelper(new FakeBlacklistHelper());
                 }
-
-                $page->setDomainAccessManager(new DomainAccessManager($page->getSecurityManager()));
             }
         }
     }
diff --git a/tests/includes/Security/SecurityManagerTest.php b/tests/includes/Security/SecurityManagerTest.php
index 6a91fd418..92b1e2540 100644
--- a/tests/includes/Security/SecurityManagerTest.php
+++ b/tests/includes/Security/SecurityManagerTest.php
@@ -10,8 +10,10 @@
 
 use PHPUnit_Framework_MockObject_MockObject;
 use PHPUnit\Framework\TestCase;
+use Waca\DataObjects\Domain;
 use Waca\DataObjects\User;
 use Waca\IdentificationVerifier;
+use Waca\Security\DomainAccessManager;
 use Waca\Security\RoleConfiguration;
 use Waca\Security\SecurityManager;
 
@@ -51,7 +53,10 @@ public function testPublicAccess()
         ));
         $roleConfiguration->method('roleNeedsIdentification')->willReturn(false);
 
-        $securityManager = new SecurityManager($this->identificationVerifier, $roleConfiguration);
+        $securityManager = new SecurityManager(
+            $this->identificationVerifier,
+            $roleConfiguration,
+            new DomainAccessManager());
         $this->identificationVerifier->method('isUserIdentified')->willReturn(true);
 
         // act
@@ -76,7 +81,10 @@ public function testPublicAccessDenied()
         ));
         $roleConfiguration->method('roleNeedsIdentification')->willReturn(false);
 
-        $securityManager = new SecurityManager($this->identificationVerifier, $roleConfiguration);
+        $securityManager = new SecurityManager(
+            $this->identificationVerifier,
+            $roleConfiguration,
+            new DomainAccessManager());
         $this->identificationVerifier->method('isUserIdentified')->willReturn(true);
 
         // act