From c5485849d56aeed094aaf189707c00d1be08272f Mon Sep 17 00:00:00 2001
From: Rossco Hellmans <rosscohellmans@catalyst-au.net>
Date: Fri, 27 Sep 2024 12:34:36 +1000
Subject: [PATCH] Fix: make access key and allowed ips OR conditional

Access keys and allowed IPs should be indpendant of each other and when
at least one of them is satisfied the user should be allowed in.
---
 README.md                      |  2 ++
 classes/local/outagelib.php    | 12 ++--------
 tests/local/outagelib_test.php | 40 ++++++++++++++--------------------
 3 files changed, 20 insertions(+), 34 deletions(-)

diff --git a/README.md b/README.md
index 01db01a..d98a8fa 100644
--- a/README.md
+++ b/README.md
@@ -192,6 +192,8 @@ Note: these restrictions build on each other; If both are enabled, users must me
 Only allow users from a certain IP or range of ips to enter.
 ## Access key
 Users provide an access key in the URL params on first page load, which is then stored as a cookie for 24 hours. If the access key matches the one setup for the outage, they are allowed in.
+## Using IP restriction with access key
+Users will be allowed if they are from the configured allowed ips OR if they provide the correct access key.
 
 
 Feedback and issues
diff --git a/classes/local/outagelib.php b/classes/local/outagelib.php
index 7163926..94638fb 100644
--- a/classes/local/outagelib.php
+++ b/classes/local/outagelib.php
@@ -321,10 +321,10 @@ public static function create_climaintenancephp_code($starttime, $stoptime, $all
 
     $ipblocked = !remoteip_in_list('{{ALLOWEDIPS}}');
     $accesskeyblocked = $useraccesskey != '{{ACCESSKEY}}';
-    $blocked = ({{USEACCESSKEY}} && $accesskeyblocked) || ({{USEALLOWEDIPS}} && $ipblocked);
+    $allowed = ({{USEACCESSKEY}} && !$accesskeyblocked) || ({{USEALLOWEDIPS}} && !$ipblocked);
     $isphpunit = defined('PHPUNIT_TEST');
 
-    if ($blocked) {
+    if (!$allowed) {
         if (!$isphpunit) {
             header($_SERVER['SERVER_PROTOCOL'] . ' 503 Moodle under maintenance');
             header('Status: 503 Moodle under maintenance');
@@ -347,19 +347,11 @@ public static function create_climaintenancephp_code($starttime, $stoptime, $all
         if ({{USEALLOWEDIPS}} && $ipblocked) {
             echo '<!-- Blocked by ip, your ip: '.getremoteaddr('n/a').' -->';
         }
-        
-        if ({{USEALLOWEDIPS}} && !$ipblocked) {
-            echo '<!-- Your IP is allowed: '.getremoteaddr('n/a').' -->';
-        }
 
         if ({{USEACCESSKEY}} && $accesskeyblocked) {
             echo '<!-- Blocked by missing or incorrect access key, access key given: '. $useraccesskey .' -->';
         }
 
-        if ({{USEACCESSKEY}} && !$accesskeyblocked) {
-            echo '<!-- Your access key is allowed: '. $useraccesskey .' -->';
-        }
-
         if (!$isphpunit) {
             if (file_exists($CFG->dataroot.'/climaintenance.template.html')) {
                 require($CFG->dataroot.'/climaintenance.template.html');
diff --git a/tests/local/outagelib_test.php b/tests/local/outagelib_test.php
index 4dcd8d8..9337cec 100644
--- a/tests/local/outagelib_test.php
+++ b/tests/local/outagelib_test.php
@@ -335,10 +335,10 @@ public function test_createmaintenancephpcode() {
 a.b.c.d
 e.e.e.e/20');
     $accesskeyblocked = $useraccesskey != '12345';
-    $blocked = (true && $accesskeyblocked) || (true && $ipblocked);
+    $allowed = (true && !$accesskeyblocked) || (true && !$ipblocked);
     $isphpunit = defined('PHPUNIT_TEST');
 
-    if ($blocked) {
+    if (!$allowed) {
         if (!$isphpunit) {
             header($_SERVER['SERVER_PROTOCOL'] . ' 503 Moodle under maintenance');
             header('Status: 503 Moodle under maintenance');
@@ -361,19 +361,11 @@ public function test_createmaintenancephpcode() {
         if (true && $ipblocked) {
             echo '<!-- Blocked by ip, your ip: '.getremoteaddr('n/a').' -->';
         }
-        
-        if (true && !$ipblocked) {
-            echo '<!-- Your IP is allowed: '.getremoteaddr('n/a').' -->';
-        }
 
         if (true && $accesskeyblocked) {
             echo '<!-- Blocked by missing or incorrect access key, access key given: '. $useraccesskey .' -->';
         }
 
-        if (true && !$accesskeyblocked) {
-            echo '<!-- Your access key is allowed: '. $useraccesskey .' -->';
-        }
-
         if (!$isphpunit) {
             if (file_exists($CFG->dataroot.'/climaintenance.template.html')) {
                 require($CFG->dataroot.'/climaintenance.template.html');
@@ -422,10 +414,10 @@ public function test_createmaintenancephpcode_withoutage($configkey) {
 
     $ipblocked = !remoteip_in_list('127.0.0.1');
     $accesskeyblocked = $useraccesskey != '5678';
-    $blocked = (true && $accesskeyblocked) || (true && $ipblocked);
+    $allowed = (true && !$accesskeyblocked) || (true && !$ipblocked);
     $isphpunit = defined('PHPUNIT_TEST');
 
-    if ($blocked) {
+    if (!$allowed) {
         if (!$isphpunit) {
             header($_SERVER['SERVER_PROTOCOL'] . ' 503 Moodle under maintenance');
             header('Status: 503 Moodle under maintenance');
@@ -448,19 +440,11 @@ public function test_createmaintenancephpcode_withoutage($configkey) {
         if (true && $ipblocked) {
             echo '<!-- Blocked by ip, your ip: '.getremoteaddr('n/a').' -->';
         }
-        
-        if (true && !$ipblocked) {
-            echo '<!-- Your IP is allowed: '.getremoteaddr('n/a').' -->';
-        }
 
         if (true && $accesskeyblocked) {
             echo '<!-- Blocked by missing or incorrect access key, access key given: '. $useraccesskey .' -->';
         }
 
-        if (true && !$accesskeyblocked) {
-            echo '<!-- Your access key is allowed: '. $useraccesskey .' -->';
-        }
-
         if (!$isphpunit) {
             if (file_exists($CFG->dataroot.'/climaintenance.template.html')) {
                 require($CFG->dataroot.'/climaintenance.template.html');
@@ -679,12 +663,11 @@ private function create_outage() {
      * @return array
      */
     public static function evaluation_maintenancepage_provider(): array {
-        $allowedipout = '<!-- Your IP is allowed:';
         $blockedipout = '<!-- Blocked by ip, your ip:';
-        $allowedaccesskeyout = '<!-- Your access key is allowed:';
         $blockedaccesskeyout = '<!-- Blocked by missing or incorrect access key, access key given:';
 
         return [
+            // IP set up, access key not set up.
             'ip allowed, no access key setup' => [
                 'allowedips' => '127.0.0.1',
                 'iptouse' => '127.0.0.1',
@@ -699,6 +682,7 @@ public static function evaluation_maintenancepage_provider(): array {
                 'accesskeytouse' => null,
                 'expectedoutputs' => [$blockedipout],
             ],
+            // IP not set up, access key set up.
             'access key incorrect, no ip setup' => [
                 'allowedips' => null,
                 'iptouse' => null,
@@ -713,19 +697,27 @@ public static function evaluation_maintenancepage_provider(): array {
                 'accesskeytouse' => '12345',
                 'expectedoutputs' => [],
             ],
+            // Both IP and access key set up.
+            'access key incorrect, ip incorrect' => [
+                'allowedips' => '127.0.0.1',
+                'iptouse' => '5.5.5.5',
+                'accesskey' => '12345',
+                'accesskeytouse' => 'wrong',
+                'expectedoutputs' => [$blockedipout, $blockedaccesskeyout],
+            ],
             'access key correct, ip incorrect' => [
                 'allowedips' => '127.0.0.1',
                 'iptouse' => '5.5.5.5',
                 'accesskey' => '12345',
                 'accesskeytouse' => '12345',
-                'expectedoutputs' => [$allowedaccesskeyout, $blockedipout],
+                'expectedoutputs' => [],
             ],
             'access key incorrect, ip correct' => [
                 'allowedips' => '127.0.0.1',
                 'iptouse' => '127.0.0.1',
                 'accesskey' => '12345',
                 'accesskeytouse' => 'wrong',
-                'expectedoutputs' => [$blockedaccesskeyout, $allowedipout],
+                'expectedoutputs' => [],
             ],
             'access key correct, ip correct' => [
                 'allowedips' => '127.0.0.1',