jenkins-bot has submitted this change and it was merged.
Change subject: Consolidate action access to a single location
......................................................................
Consolidate action access to a single location
Views and blocks were using different ways to decide what a user can and cannot
do. This pulls the related code out of the view helper and reuses in both the
view helpers and the blocks.
Change-Id: I03956a78c493fc56e5aad11aab9ffce17e56bba5
---
M Flow.php
M includes/Block/Topic.php
M includes/Model/PostRevision.php
A includes/PostActionPermissions.php
M includes/Templating.php
M includes/View/PostActionMenu.php
6 files changed, 233 insertions(+), 102 deletions(-)
Approvals:
Matthias Mullie: Looks good to me, approved
jenkins-bot: Verified
diff --git a/Flow.php b/Flow.php
index 451b463..eab4dbe 100755
--- a/Flow.php
+++ b/Flow.php
@@ -72,6 +72,7 @@
$wgAutoloadClasses['Flow\TalkpageManager'] = $dir .
'includes/TalkpageManager.php';
$wgAutoloadClasses['Flow\NotificationFormatter'] = $dir .
'includes/Notifications/Formatter.php';
$wgAutoloadClasses['Flow\NotificationController'] = $dir .
'includes/Notifications/Controller.php';
+$wgAutoloadClasses['Flow\PostActionPermissions'] = $dir .
'includes/PostActionPermissions.php';
// Classes that model our data
$wgAutoloadClasses['Flow\Model\Definition'] = $dir .
'includes/Model/Definition.php';
diff --git a/includes/Block/Topic.php b/includes/Block/Topic.php
index 6cec6dc..6bfa7e0 100644
--- a/includes/Block/Topic.php
+++ b/includes/Block/Topic.php
@@ -10,6 +10,7 @@
use Flow\Model\AbstractRevision;
use Flow\Model\PostRevision;
use Flow\NotificationController;
+use Flow\PostActionPermissions;
use Flow\Templating;
use EchoEvent;
use User;
@@ -34,6 +35,12 @@
'hide-topic', 'edit-title',
);
+ /**
+ * @var PostActionPermissions $permissions Allows or denies actions to
be performed
+ */
+ protected $permissions;
+
+
public function __construct( Workflow $workflow, ManagerGroup $storage,
NotificationController $notificationController, $root ) {
parent::__construct( $workflow, $storage,
$notificationController );
if ( $root instanceof PostRevision ) {
@@ -45,6 +52,11 @@
'Expected PostRevision or RootPostLoader,
received: ' . is_object( $root ) ? get_class( $root ) : gettype( $root )
);
}
+ }
+
+ public function init( $action, $user ) {
+ parent::init( $action, $user );
+ $this->permissions = new PostActionPermissions( $user );
}
protected function validate() {
@@ -97,6 +109,10 @@
if ( !$topicTitle ) {
throw new \Exception( 'No revision associated
with workflow?' );
}
+ if ( !$this->permissions->isAllowed( $topicTitle,
'edit-title' ) ) {
+ $this->errors['permissions'] = wfMessage(
'flow-error-not-allowed' );
+ return;
+ }
$this->newRevision = $topicTitle->newNextRevision(
$this->user, $this->submitted['content'], 'flow-rev-message-edit-title' );
@@ -122,6 +138,9 @@
$post = $this->storage->get( 'PostRevision',
$this->submitted['replyTo'] );
if ( !$post ) {
$this->errors['replyTo'] = wfMessage(
'flow-error-invalid-replyto' );
+ } elseif ( !$this->permissions->isAllowed( $post,
'reply' ) ) {
+ // Or should the check be rolled into the
!$post condition?
+ $this->errors['permissions'] = wfMessage(
'flow-error-not-allowed' );
} else {
// TODO: assert post belongs to this tree?
Does it really matter?
// answer: might not belong, and probably does
matter due to inter-wiki interaction
@@ -154,6 +173,9 @@
if ( !$post ) {
$this->errors['moderate-post'] = wfMessage(
'flow-error-invalid-postId' );
return;
+ } elseif ( !$this->permissions->isAllowed( $post,
"{$moderationState}-post" ) ) {
+ $this->errors['permissions'] = wfMessage(
'flow-error-not-allowed' );
+ return;
}
$this->newRevision = $post->moderate( $this->user,
$moderationState );
@@ -170,6 +192,9 @@
$post = $this->loadRequestedPost( $this->submitted['postId'] );
if ( !$post ) {
$this->errors['restore-post'] = wfMessage(
'flow-error-invalid-postId' );
+ return;
+ } elseif ( !$this->permissions->isAllowed( $post,
"restore-post" ) ) {
+ $this->errors['permissions'] = wfMessage(
'flow-error-not-allowed' );
return;
}
@@ -193,8 +218,8 @@
$this->errors['edit-post'] = wfMessage(
'flow-post-not-found' );
return;
}
- if ( !$post->isAllowedToEdit( $this->user ) ) {
- $this->errors['edit-post'] = wfMessage(
'flow-error-edit-restricted' );
+ if ( !$this->permissions->isAllowed( $post, 'edit-post' ) ) {
+ $this->errors['permissions'] = wfMessage(
'flow-error-not-allowed' );
return;
}
@@ -269,6 +294,10 @@
return $this->renderPostHistory( $templating, $options,
$return );
case 'topic-history':
+ $history = $this->loadTopicHistory();
+ if ( !$this->permissions->isAllowed( reset( $history ),
'post-history' ) ) {
+ throw new \MWException( 'Not Allowed' );
+ }
return $templating->render(
"flow:topic-history.html.php", array(
'block' => $this,
'topic' => $this->workflow,
@@ -279,6 +308,10 @@
return $this->renderEditPost( $templating, $options,
$return );
case 'edit-title':
+ $topicTitle = $this->loadTopicTitle();
+ if ( !$this->permissions->isAllowed( $topicTitle,
'edit-post' ) ) {
+ throw new \MWException( 'Not Allowed' );
+ }
return $templating->render( "flow:edit-title.html.php",
array(
'block' => $this,
'topic' => $this->workflow,
@@ -288,10 +321,14 @@
default:
$root = $this->loadRootPost();
+ if ( !$this->permissions->isAllowed( $root, 'view' ) ) {
+ throw new \MWException( 'Not Allowed' );
+ }
+
if ( isset( $options['postId'] ) ) {
$indexDescendant = $root->registerDescendant(
$options['postId'] );
$post = $root->getRecursiveResult(
$indexDescendant );
- if ( $post === false ) {
+ if ( $post === null ) {
throw new \MWException( 'Requested
postId is not available within post tree' );
}
@@ -314,6 +351,10 @@
if ( !isset( $options['postId'] ) ) {
throw new \Exception( 'No postId provided' );
}
+ $history = $this->getHistory( $options['postId'] );
+ if ( !$this->permissions->isAllowed( reset( $history ),
'post-history' ) ) {
+ throw new \MWException( 'Not Allowed' );
+ }
return $templating->render( "flow:post-history.html.php", array(
'block' => $this,
'topic' => $this->workflow,
@@ -326,8 +367,8 @@
throw new \Exception( 'No postId provided' );
}
$post = $this->loadRequestedPost( $options['postId'] );
- if ( $post->isModerated() ) {
- throw new \Exception( 'Cannot edit restricted post.
Restore first.' );
+ if ( !$this->permissions->isAllowed( $post, 'edit-post' ) ) {
+ throw new \MWException( 'Not Allowed' );
}
return $templating->render( "flow:edit-post.html.php", array(
'block' => $this,
@@ -347,12 +388,20 @@
}
if ( ! $post ) {
- throw new MWException( "Requested post could
not be found" );
+ throw new \MWException( "Requested post could
not be found" );
}
- return array( $this->renderPostAPI( $templating, $post,
$options ) );
+ $res = $this->renderPostAPI( $templating, $post,
$options );
+ if ( $res === null ) {
+ throw new \MWException( 'Not Allowed' );
+ }
+ return array( $res );
} else {
- return $this->renderTopicAPI( $templating, $options );
+ $output = $this->renderTopicAPI( $templating, $options
);
+ if ( $output === null ) {
+ throw new \MWException( 'Not Allowed' );
+ }
+ return $output;
}
}
@@ -361,6 +410,9 @@
$rootPost = $this->loadRootPost();
$topic = $this->workflow;
+ if ( !$this->permissions->isAllowed( $rootPost, 'view' ) ) {
+ throw new \MWException( 'Not Allowed' );
+ }
$output = array(
'_element' => 'post',
'title' => $rootPost->getContent( null, 'wikitext' ),
@@ -389,15 +441,23 @@
}
foreach( $rootPost->getChildren() as $child ) {
- $output[] = $this->renderPostAPI( $templating, $child,
$options );
+ $res = $this->renderPostAPI( $templating, $child,
$options );
+ if ( $res !== null ) {
+ $output[] = $res;
+ }
}
return $output;
}
protected function renderPostAPI( Templating $templating, PostRevision
$post, array $options ) {
- $output = array();
+ if ( !$this->permissions->isAllowed( $post, 'view' ) ) {
+ // we have to return null, or we would have to
duplicate this call when rendering children.
+ // callers must check for null and do as appropriate
+ return null;
+ }
+ $output = array();
$output['post-id'] = $post->getPostId()->getHex();
$contentFormat = 'wikitext';
@@ -412,14 +472,17 @@
'*' => $post->getContent( null, $contentFormat
),
'format' => $contentFormat
);
- $output['user'] = $post->getUserText();
+ $output['user'] = $post->getCreatorText();
}
if ( ! isset( $options['no-children'] ) ) {
$children = array( '_element' => 'post' );
foreach( $post->getChildren() as $child ) {
- $children[] = $this->renderPostAPI(
$templating, $child, $options );
+ $res = $this->renderPostAPI( $templating,
$child, $options );
+ if ( $res !== null ) {
+ $children[] = $res;
+ }
}
if ( count( $children ) > 1 ) {
@@ -442,11 +505,13 @@
$output['post-id'] = $postId;
foreach( $history as $revision ) {
- $output[] = array(
- 'revision-id' =>
$revision->getRevisionId()->getHex(),
- 'revision-author' => $revision->getUserText(),
- 'revision-change-type' =>
$revision->getChangeType(),
- );
+ if ( $this->permissions->isAllowed( $revision, 'view' )
) {
+ $output[] = array(
+ 'revision-id' =>
$revision->getRevisionId()->getHex(),
+ 'revision-author' =>
$revision->getUserText(),
+ 'revision-change-type' =>
$revision->getChangeType(),
+ );
+ }
}
return $output;
diff --git a/includes/Model/PostRevision.php b/includes/Model/PostRevision.php
index 9f878c5..7e74c5d 100644
--- a/includes/Model/PostRevision.php
+++ b/includes/Model/PostRevision.php
@@ -88,7 +88,7 @@
/**
* Get the user ID of the user who created this post.
- * Checks permissions and returns false
+ * Checks permissions and returns false
*
* @param $user User The user to check permissions for.
* @return int|bool The user ID, or false
@@ -383,10 +383,10 @@
}
}
- public function isAllowedToEdit( $user ) {
+ public function isCreator( $user ) {
if ( $user->isAnon() ) {
return false;
}
- return $user->getId() == $this->getCreatorId() ||
$user->isAllowed( 'flow-edit-post' );
+ return $user->getId() == $this->getCreatorId();
}
}
diff --git a/includes/PostActionPermissions.php
b/includes/PostActionPermissions.php
new file mode 100644
index 0000000..9a3e727
--- /dev/null
+++ b/includes/PostActionPermissions.php
@@ -0,0 +1,136 @@
+<?php
+
+namespace Flow;
+
+use Flow\Model\PostRevision;
+use Closure;
+
+/**
+ * role based security for posts based on moderation state
+ */
+class PostActionPermissions {
+
+ public function __construct( $user ) {
+ $this->user = $user;
+
+ $this->actions = array(
+ 'hide-post' => array(
+ // Permissions required to perform action. The
key is the moderation state
+ // of the post to perform the action against.
The value is a string or array
+ // of user rights that can allow this action.
+ PostRevision::MODERATED_NONE => 'flow-hide',
+ ),
+
+ 'delete-post' => array(
+ PostRevision::MODERATED_NONE => 'flow-delete',
+ PostRevision::MODERATED_HIDDEN => 'flow-delete',
+ ),
+
+ 'censor-post' => array(
+ PostRevision::MODERATED_NONE => 'flow-censor',
+ PostRevision::MODERATED_HIDDEN => 'flow-censor',
+ PostRevision::MODERATED_DELETED =>
'flow-censor',
+ ),
+
+ 'restore-post' => array(
+ PostRevision::MODERATED_HIDDEN => array(
'flow-hide', 'flow-delete', 'flow-censor' ),
+ PostRevision::MODERATED_DELETED => array(
'flow-delete', 'flow-censor' ),
+ PostRevision::MODERATED_CENSORED =>
'flow-censor',
+ ),
+
+ 'post-history' => array(
+ PostRevision::MODERATED_NONE => '',
+ PostRevision::MODERATED_HIDDEN => '',
+ PostRevision::MODERATED_DELETED => '',
+ PostRevision::MODERATED_CENSORED =>
'flow-censor',
+ ),
+
+ 'edit-post' => function( PostRevision $post ) use (
$user ) {
+ // no permissions needed for own posts
+ return array(
+ PostRevision::MODERATED_NONE =>
$post->isCreator( $user ) ? '' : 'flow-edit-post',
+ );
+ },
+
+ 'view' => array(
+ PostRevision::MODERATED_NONE => '',
+ PostRevision::MODERATED_HIDDEN => array(
'flow-hide', 'flow-delete', 'flow-censor' ),
+ PostRevision::MODERATED_DELETED => array(
'flow-delete', 'flow-censor' ),
+ PostRevision::MODERATED_CENSORED =>
'flow-censor',
+ ),
+ );
+ }
+
+ /**
+ * Get the name of all the actions the user is allowed to perform.
+ *
+ * @param PostRevision $post The post to check permissions against
+ * @return array Array of action names that are allowed
+ */
+ public function getAllowedActions( PostRevision $post ) {
+ $allowed = array();
+ foreach( array_keys( $this->actions ) as $action ) {
+ if ( $this->isAllowedAny( $post, $action ) ) {
+ $allowed[] = $action;
+ }
+ }
+ return $allowed;
+ }
+
+ /**
+ * Check if a user is allowed to perform a certain action.
+ *
+ * @param PostRevision $post
+ * @param string $action
+ * @return bool
+ */
+ public function isAllowed( PostRevision $post, $action ) {
+ if ( !isset( $this->actions[$action] ) ) {
+ return false;
+ }
+ $permissions = $this->actions[$action];
+ if ( $permissions instanceof Closure ) {
+ $permissions = $permissions( $post );
+ }
+ $state = $post->getModerationState();
+ // If no permission is defined for this state, then the action
is not allowed
+ // check if permission is set for this action
+ if ( !isset( $permissions[$state] ) ) {
+ return false;
+ }
+
+ // check if user is allowed to perform action
+ $res = call_user_func_array(
+ array( $this->user, 'isAllowedAny' ),
+ (array) $permissions[$state]
+ );
+ return $res;
+ }
+
+ /**
+ * Check if a user is allowed to perform certain actions.
+ *
+ * @param PostRevision $post
+ * @param string $action
+ * @param string[optional] $action2 Overloadable to check if either of
the provided actions are allowed
+ * @return bool
+ */
+ public function isAllowedAny( PostRevision $post, $action /* [,
$action2 [, ... ]] */ ) {
+ $actions = func_get_args();
+ // Pull $post out of the actions list
+ array_shift( $actions );
+ $allowed = false;
+
+ foreach ( $actions as $action ) {
+ $allowed |= $this->isAllowed( $post, $action );
+
+ // as soon as we've found one that is allowed, break
+ if ( $allowed ) {
+ break;
+ }
+ }
+
+ return $allowed;
+ }
+}
+
diff --git a/includes/Templating.php b/includes/Templating.php
index 250404c..74ab02b 100644
--- a/includes/Templating.php
+++ b/includes/Templating.php
@@ -98,7 +98,7 @@
// class has too many responsibilities to keep
receiving all required objects in the constructor.
'postActionMenu' => new PostActionMenu(
$this->urlGenerator,
- $wgUser,
+ new PostActionPermissions( $wgUser ),
$block,
$post,
$wgUser->getEditToken( $wgFlowTokenSalt
)
@@ -220,7 +220,7 @@
* Gets a Flow-formatted plaintext human-readable identifier for a user.
* Usually the user's name, but it can also return "an anonymous user",
* or information about an item's moderation state.
- *
+ *
* @param User $user The User object to get a
description for.
* @param AbstractRevision $rev An AbstractRevision object to
retrieve moderation state from.
* @param bool $showIPs Whether or not to show IP
addresses for anonymous users
diff --git a/includes/View/PostActionMenu.php b/includes/View/PostActionMenu.php
index 547862d..745c0ba 100644
--- a/includes/View/PostActionMenu.php
+++ b/includes/View/PostActionMenu.php
@@ -4,9 +4,9 @@
use Flow\Block\Block;
use Flow\Model\PostRevision;
+use Flow\PostActionPermissions;
use Flow\UrlGenerator;
use Html;
-use User;
class PostActionMenu {
// Received via constructor
@@ -16,9 +16,9 @@
protected $post;
protected $user;
- public function __construct( UrlGenerator $urlGenerator, User $user,
Block $block, PostRevision $post, $editToken ) {
+ public function __construct( UrlGenerator $urlGenerator,
PostActionPermissions $permissions, Block $block, PostRevision $post,
$editToken ) {
$this->urlGenerator = $urlGenerator;
- $this->user = $user;
+ $this->permissions = $permissions;
$this->block = $block;
$this->post = $post;
$this->editToken = $editToken;
@@ -34,60 +34,24 @@
$actions = array(
'hide-post' => array(
'method' => 'POST',
- 'permissions' => array(
- PostRevision::MODERATED_NONE =>
'flow-hide',
- ),
),
'delete-post' => array(
'method' => 'POST',
- 'permissions' => array(
- PostRevision::MODERATED_NONE =>
'flow-delete',
- PostRevision::MODERATED_HIDDEN =>
'flow-delete',
- ),
),
'censor-post' => array(
'method' => 'POST',
- 'permissions' => array(
- PostRevision::MODERATED_NONE =>
'flow-censor',
- PostRevision::MODERATED_HIDDEN =>
'flow-censor',
- PostRevision::MODERATED_DELETED =>
'flow-censor',
- ),
),
'restore-post' => array(
'method' => 'POST',
- 'permissions' => array(
- PostRevision::MODERATED_HIDDEN =>
array( 'flow-hide', 'flow-delete', 'flow-censor' ),
- PostRevision::MODERATED_DELETED =>
array( 'flow-delete', 'flow-censor' ),
- PostRevision::MODERATED_CENSORED =>
'flow-censor',
- ),
),
'post-history' => array(
'method' => 'GET',
- 'permissions' => array(
- PostRevision::MODERATED_NONE => '',
- PostRevision::MODERATED_HIDDEN => '',
- PostRevision::MODERATED_DELETED => '',
- PostRevision::MODERATED_CENSORED =>
'flow-censor',
- ),
),
'edit-post' => array(
'method' => 'GET',
- 'permissions' => array(
- // no permissions needed for own posts
- PostRevision::MODERATED_NONE =>
$this->post->isAllowedToEdit( $this->user ) ? '' : 'flow-edit-post',
- PostRevision::MODERATED_HIDDEN =>
$this->post->isAllowedToEdit( $this->user ) ? '' : 'flow-edit-post',
- PostRevision::MODERATED_DELETED =>
$this->post->isAllowedToEdit( $this->user ) ? '' : 'flow-edit-post',
- PostRevision::MODERATED_CENSORED =>
$this->post->isAllowedToEdit( $this->user ) ? '' : 'flow-edit-post',
- ),
),
'view' => array(
'method' => 'GET',
- 'permissions' => array(
- PostRevision::MODERATED_NONE => '',
- PostRevision::MODERATED_HIDDEN => '',
- PostRevision::MODERATED_DELETED => '',
- PostRevision::MODERATED_CENSORED => '',
- ),
),
);
@@ -103,13 +67,11 @@
* @return string|bool Button HTML or false on failure
*/
public function getButton( $action, $content, $class ) {
- $details = $this->getActionDetails( $action );
-
- if ( !$this->isAllowed( $action ) ) {
+ if ( !$this->permissions->isAllowed( $this->post, $action ) ) {
return false;
}
-
$data = array( $this->block->getName() . '[postId]' =>
$this->post->getPostId()->getHex() );
+ $details = $this->getActionDetails( $action );
if ( $details['method'] === 'POST' ) {
return $this->postAction( $action, $data, $content,
$class );
} else {
@@ -117,48 +79,15 @@
}
}
- /**
- * Check if a user is allowed to perform (a) certain action(s).
- *
- * @param string $action
- * @return bool
- */
public function isAllowed( $action ) {
- $details = $this->getActionDetails( $action );
-
- // check if permission is set for this action
- if ( !isset(
$details['permissions'][$this->post->getModerationState()] ) ) {
- return false;
- }
-
- // check if user is allowed to perform action
- return call_user_func_array(
- array( $this->user, 'isAllowedAny' ),
- (array)
$details['permissions'][$this->post->getModerationState()]
- );
+ return $this->permissions->isAllowed( $this->post, $action );
}
- /**
- * Check if a user is allowed to perform certain actions.
- *
- * @param string $action
- * @param string[optional] $action2 Overloadable to check if either of
the provided actions are allowed
- * @return bool
- */
public function isAllowedAny( $action /* [, $action2 [, ... ]] */ ) {
- $actions = func_get_args();
- $allowed = false;
+ $arguments = func_get_args();
+ array_unshift( $arguments, $this->post );
- foreach ( $actions as $action ) {
- $allowed |= $this->isAllowed( $action );
-
- // as soon as we've found one that is allowed, break
- if ( $allowed ) {
- break;
- }
- }
-
- return $allowed;
+ return call_user_func_array( array( $this->permissions,
'isAllowedAny' ), $arguments );
}
/**
--
To view, visit https://gerrit.wikimedia.org/r/91562
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I03956a78c493fc56e5aad11aab9ffce17e56bba5
Gerrit-PatchSet: 2
Gerrit-Project: mediawiki/extensions/Flow
Gerrit-Branch: master
Gerrit-Owner: EBernhardson <[email protected]>
Gerrit-Reviewer: Bsitu <[email protected]>
Gerrit-Reviewer: Matthias Mullie <[email protected]>
Gerrit-Reviewer: Werdna <[email protected]>
Gerrit-Reviewer: jenkins-bot
_______________________________________________
MediaWiki-commits mailing list
[email protected]
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
