From 480336a332edb3421967e8a704976494c582e76c Mon Sep 17 00:00:00 2001
From: Andreas Gohr <andi@splitbrain.org>
Date: Fri, 31 Mar 2017 17:49:15 +0200
Subject: [PATCH] no longer rely on actionOk when checking if actions are
disabled
loadAction() and checkAction() are now public and could be used within
actionOK(). However some weird circular references prevent that. In
addition, actionOK is also used to check for things that aren't Actions
(yet) like 'rss' and 'top'.
---
_test/tests/inc/Action/general.test.php | 12 +++---
inc/Action/AbstractAclAction.php | 2 +
inc/Action/AbstractAction.php | 3 +-
inc/Action/Login.php | 1 -
inc/Action/Logout.php | 10 +++++
inc/Action/Profile.php | 10 +++++
inc/Action/ProfileDelete.php | 10 +++++
inc/Action/Register.php | 14 ++++++-
inc/Action/Resendpwd.php | 12 ++++++
inc/Action/Subscribe.php | 10 +++++
inc/ActionRouter.php | 55 ++++++++++++++++++-------
11 files changed, 114 insertions(+), 25 deletions(-)
diff --git a/_test/tests/inc/Action/general.test.php b/_test/tests/inc/Action/general.test.php
index 116128a62..5e6402836 100644
--- a/_test/tests/inc/Action/general.test.php
+++ b/_test/tests/inc/Action/general.test.php
@@ -87,6 +87,9 @@ class action_general extends DokuWikiTest {
* @param $name
*/
public function testBaseClassActionOkPermission($name) {
+ $this->assertTrue(true); // mark as not risky
+ if($name == 'Show') return; // disabling show does not work
+
$classname = 'dokuwiki\\Action\\' . $name;
/** @var \dokuwiki\Action\AbstractAction $class */
$class = new $classname();
@@ -95,9 +98,10 @@ class action_general extends DokuWikiTest {
$conf['useacl'] = 1;
$conf['subscribers'] = 1;
$conf['disableactions'] = '';
+ $_SERVER['REMOTE_USER'] = 'someone';
try {
- $class->checkPermissions();
+ \dokuwiki\ActionRouter::getInstance(true)->checkAction($class);
} catch(\Exception $e) {
$this->assertNotSame(ActionDisabledException::class, get_class($e));
}
@@ -105,12 +109,10 @@ class action_general extends DokuWikiTest {
$conf['disableactions'] = $class->getActionName();
try {
- $class->checkPermissions();
+ \dokuwiki\ActionRouter::getInstance(true)->checkAction($class);
} catch(\Exception $e) {
- $this->assertSame(ActionDisabledException::class, get_class($e));
+ $this->assertSame(ActionDisabledException::class, get_class($e), $e);
}
-
- $this->assertTrue(true); // mark as not risky
}
/**
diff --git a/inc/Action/AbstractAclAction.php b/inc/Action/AbstractAclAction.php
index 7fec77a2e..76639b066 100644
--- a/inc/Action/AbstractAclAction.php
+++ b/inc/Action/AbstractAclAction.php
@@ -17,7 +17,9 @@ abstract class AbstractAclAction extends AbstractAction {
public function checkPermissions() {
parent::checkPermissions();
global $conf;
+ global $auth;
if(!$conf['useacl']) throw new ActionAclRequiredException();
+ if(!$auth) throw new ActionAclRequiredException();
}
}
diff --git a/inc/Action/AbstractAction.php b/inc/Action/AbstractAction.php
index fe139892a..c19167d79 100644
--- a/inc/Action/AbstractAction.php
+++ b/inc/Action/AbstractAction.php
@@ -23,7 +23,7 @@ abstract class AbstractAction {
*
* @param string $actionname the name of this action (see getActioName() for caveats)
*/
- public function __construct($actionname='') {
+ public function __construct($actionname = '') {
if($actionname !== '') {
$this->actionname = $actionname;
} else {
@@ -50,7 +50,6 @@ abstract class AbstractAction {
* @return void
*/
public function checkPermissions() {
- if(!actionOK($this->actionname)) throw new ActionDisabledException();
}
/**
diff --git a/inc/Action/Login.php b/inc/Action/Login.php
index 982e3c7c2..6e4aeb01a 100644
--- a/inc/Action/Login.php
+++ b/inc/Action/Login.php
@@ -26,7 +26,6 @@ class Login extends AbstractAclAction {
// nothing to do
throw new ActionException();
}
- // FIXME auth login capabilities
}
/** @inheritdoc */
diff --git a/inc/Action/Logout.php b/inc/Action/Logout.php
index 88f2673bd..2abf968f6 100644
--- a/inc/Action/Logout.php
+++ b/inc/Action/Logout.php
@@ -2,6 +2,7 @@
namespace dokuwiki\Action;
+use dokuwiki\Action\Exception\ActionDisabledException;
use dokuwiki\Action\Exception\ActionException;
/**
@@ -18,6 +19,15 @@ class Logout extends AbstractUserAction {
return AUTH_NONE;
}
+ /** @inheritdoc */
+ public function checkPermissions() {
+ parent::checkPermissions();
+
+ /** @var \DokuWiki_Auth_Plugin $auth */
+ global $auth;
+ if(!$auth->canDo('logout')) throw new ActionDisabledException();
+ }
+
/** @inheritdoc */
public function preProcess() {
global $ID;
diff --git a/inc/Action/Profile.php b/inc/Action/Profile.php
index 6931a2589..1ebe51fec 100644
--- a/inc/Action/Profile.php
+++ b/inc/Action/Profile.php
@@ -3,6 +3,7 @@
namespace dokuwiki\Action;
use dokuwiki\Action\Exception\ActionAbort;
+use dokuwiki\Action\Exception\ActionDisabledException;
/**
* Class Profile
@@ -18,6 +19,15 @@ class Profile extends AbstractUserAction {
return AUTH_NONE;
}
+ /** @inheritdoc */
+ public function checkPermissions() {
+ parent::checkPermissions();
+
+ /** @var \DokuWiki_Auth_Plugin $auth */
+ global $auth;
+ if(!$auth->canDo('Profile')) throw new ActionDisabledException();
+ }
+
/** @inheritdoc */
public function preProcess() {
global $lang;
diff --git a/inc/Action/ProfileDelete.php b/inc/Action/ProfileDelete.php
index 40336fba1..5be5ff578 100644
--- a/inc/Action/ProfileDelete.php
+++ b/inc/Action/ProfileDelete.php
@@ -3,6 +3,7 @@
namespace dokuwiki\Action;
use dokuwiki\Action\Exception\ActionAbort;
+use dokuwiki\Action\Exception\ActionDisabledException;
/**
* Class ProfileDelete
@@ -18,6 +19,15 @@ class ProfileDelete extends AbstractUserAction {
return AUTH_NONE;
}
+ /** @inheritdoc */
+ public function checkPermissions() {
+ parent::checkPermissions();
+
+ /** @var \DokuWiki_Auth_Plugin $auth */
+ global $auth;
+ if(!$auth->canDo('delUser')) throw new ActionDisabledException();
+ }
+
/** @inheritdoc */
public function preProcess() {
global $lang;
diff --git a/inc/Action/Register.php b/inc/Action/Register.php
index 50fa2894d..c97d3f858 100644
--- a/inc/Action/Register.php
+++ b/inc/Action/Register.php
@@ -3,6 +3,7 @@
namespace dokuwiki\Action;
use dokuwiki\Action\Exception\ActionAbort;
+use dokuwiki\Action\Exception\ActionDisabledException;
/**
* Class Register
@@ -11,13 +12,24 @@ use dokuwiki\Action\Exception\ActionAbort;
*
* @package dokuwiki\Action
*/
-class Register extends AbstractAction {
+class Register extends AbstractAclAction {
/** @inheritdoc */
public function minimumPermission() {
return AUTH_NONE;
}
+ /** @inheritdoc */
+ public function checkPermissions() {
+ parent::checkPermissions();
+
+ /** @var \DokuWiki_Auth_Plugin $auth */
+ global $auth;
+ global $conf;
+ if(isset($conf['openregister']) && !$conf['openregister']) throw new ActionDisabledException();
+ if(!$auth->canDo('addUser')) throw new ActionDisabledException();
+ }
+
/** @inheritdoc */
public function preProcess() {
if(register()) { // FIXME could be moved from auth to here
diff --git a/inc/Action/Resendpwd.php b/inc/Action/Resendpwd.php
index 16548f383..466e078a4 100644
--- a/inc/Action/Resendpwd.php
+++ b/inc/Action/Resendpwd.php
@@ -3,6 +3,7 @@
namespace dokuwiki\Action;
use dokuwiki\Action\Exception\ActionAbort;
+use dokuwiki\Action\Exception\ActionDisabledException;
/**
* Class Resendpwd
@@ -18,6 +19,17 @@ class Resendpwd extends AbstractAclAction {
return AUTH_NONE;
}
+ /** @inheritdoc */
+ public function checkPermissions() {
+ parent::checkPermissions();
+
+ /** @var \DokuWiki_Auth_Plugin $auth */
+ global $auth;
+ global $conf;
+ if(isset($conf['resendpasswd']) && !$conf['resendpasswd']) throw new ActionDisabledException(); //legacy option
+ if(!$auth->canDo('modPass')) throw new ActionDisabledException();
+ }
+
/** @inheritdoc */
public function preProcess() {
if($this->resendpwd()) {
diff --git a/inc/Action/Subscribe.php b/inc/Action/Subscribe.php
index 762c32783..b7f62c46e 100644
--- a/inc/Action/Subscribe.php
+++ b/inc/Action/Subscribe.php
@@ -2,6 +2,8 @@
namespace dokuwiki\Action;
+use dokuwiki\Action\Exception\ActionDisabledException;
+
/**
* Class Subscribe
*
@@ -16,6 +18,14 @@ class Subscribe extends AbstractUserAction {
return AUTH_READ;
}
+ /** @inheritdoc */
+ public function checkPermissions() {
+ parent::checkPermissions();
+
+ global $conf;
+ if(isset($conf['subscribers']) && !$conf['subscribers']) throw new ActionDisabledException();
+ }
+
/** @inheritdoc */
public function preProcess() {
$act = $this->actionname;
diff --git a/inc/ActionRouter.php b/inc/ActionRouter.php
index 2e7e9a07c..01e670c9d 100644
--- a/inc/ActionRouter.php
+++ b/inc/ActionRouter.php
@@ -27,6 +27,9 @@ class ActionRouter {
/** maximum loop */
const MAX_TRANSITIONS = 5;
+ /** @var string[] the actions disabled in the configuration */
+ protected $disabled;
+
/**
* ActionRouter constructor. Singleton, thus protected!
*
@@ -35,6 +38,12 @@ class ActionRouter {
*/
protected function __construct() {
global $ACT;
+ global $conf;
+
+ $this->disabled = explode(',', $conf['disableactions']);
+ $this->disabled = array_map('trim', $this->disabled);
+ $this->transitions = 0;
+
$ACT = act_clean($ACT);
$this->setupAction($ACT);
$ACT = $this->action->getActionName();
@@ -67,8 +76,7 @@ class ActionRouter {
try {
$this->action = $this->loadAction($actionname);
- $this->action->checkPermissions();
- $this->ensureMinimumPermission($this->action->minimumPermission());
+ $this->checkAction($this->action);
$this->action->preProcess();
} catch(ActionException $e) {
@@ -139,19 +147,6 @@ class ActionRouter {
$this->setupAction($to);
}
- /**
- * Check that the given minimum permissions are reached
- *
- * @param int $permneed
- * @throws ActionException
- */
- protected function ensureMinimumPermission($permneed) {
- global $INFO;
- if($INFO['perm'] < $permneed) {
- throw new ActionException('denied');
- }
- }
-
/**
* Aborts all processing with a message
*
@@ -183,7 +178,7 @@ class ActionRouter {
* @return AbstractAction
* @throws NoActionException
*/
- protected function loadAction($actionname) {
+ public function loadAction($actionname) {
$actionname = strtolower($actionname); // FIXME is this needed here? should we run a cleanup somewhere else?
$parts = explode('_', $actionname);
while($parts) {
@@ -198,6 +193,34 @@ class ActionRouter {
throw new NoActionException();
}
+ /**
+ * Execute all the checks to see if this action can be executed
+ *
+ * @param AbstractAction $action
+ * @throws ActionDisabledException
+ * @throws ActionException
+ */
+ public function checkAction(AbstractAction $action) {
+ global $INFO;
+ global $ID;
+
+ if(in_array($action->getActionName(), $this->disabled)) {
+ throw new ActionDisabledException();
+ }
+
+ $action->checkPermissions();
+
+ if(isset($INFO)) {
+ $perm = $INFO['perm'];
+ } else {
+ $perm = auth_quickaclcheck($ID);
+ }
+
+ if($perm < $action->minimumPermission()) {
+ throw new ActionException('denied');
+ }
+ }
+
/**
* Returns the action handling the current request
*
--
GitLab