Merge pull request #345 from catalyst/access-key-fix

Fix: make access key and allowed ips OR conditional
catalyst · Sep 30, 2024 · a40e198 · a40e198
2 parents cba55cd + c548584
commit a40e198
Show file tree

Hide file tree

Showing 3 changed files with 20 additions and 34 deletions.
diff --git 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
@@ -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
@@ -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',