jenkins-bot has submitted this change and it was merged.
Change subject: (Bug 61715) Make history entries look at most recent revision
......................................................................
(Bug 61715) Make history entries look at most recent revision
The problem with history thingies is that "restore" should not show up for users
that can not view the permissions for what is restored.
isAllowed will check the most recent revision to get some sort of "global
allowed state" for a revision. Combined with the above, this would make any
revision of a just-restored post be invisible.
For that reason, I had made history (and RC) permission checks only do
isRevisionAllowed. That's incorrect; they should also check last revision (but
in a way that is actually correct - where the restore-aspect is ignored for the
"global" moderation state)
Bug: 61715
Change-Id: Ia27c94176bb8f40f2fe683a54ab9ddafb4f98fc0
---
M FlowActions.php
M includes/Block/Header.php
M includes/Block/Topic.php
M includes/Formatter/CheckUser.php
M includes/Formatter/Contributions.php
M includes/Formatter/RecentChanges.php
M includes/Log/Formatter.php
M includes/RevisionActionPermissions.php
M includes/Templating.php
M tests/RevisionCollectionPermissionsTest.php
10 files changed, 78 insertions(+), 59 deletions(-)
Approvals:
EBernhardson: Looks good to me, approved
jenkins-bot: Verified
diff --git a/FlowActions.php b/FlowActions.php
index a799372..b0731c2 100644
--- a/FlowActions.php
+++ b/FlowActions.php
@@ -371,14 +371,47 @@
'log_type' => false,
'permissions' => array(
PostRevision::MODERATED_NONE => function(
AbstractRevision $revision, RevisionActionPermissions $permissions ) {
- // if a revision was the result of a
restore-action, we have
- // to look at the previous revision what the
original moderation
- // status was; permissions for the
restore-actions visibility
- // is the same as the moderation (e.g. if user
can't see
- // suppress actions, he can't see restores from
suppress
+ static $previousCollectionId;
+
+ /*
+ * To check permissions, both the current
revision (revision-
+ * specific moderation state)& the last
revision (global
+ * collection moderation state) will always be
checked.
+ * This one has special checks to make sure
"restore" actions
+ * are hidden when the user has no permissions
to see the
+ * moderation state they were restored from.
+ * We don't want that test to happen;
otherwise, when a post
+ * has just been restored in the most recent
revisions, that
+ * would result in none of the previous
revisions being
+ * available (because a user would need
permissions for the the
+ * state the last revision was restored from)
+ */
+ $collection = $revision->getCollection();
+ if ( $previousCollectionId &&
$collection->getId()->equals( $previousCollectionId ) ) {
+ // doublecheck that this run is indeed
against the most
+ // recent revision, to get the global
collection state
+ try {
+ $lastRevision =
$collection->getLastRevision();
+ if (
$revision->getRevisionId()->equals( $lastRevision->getRevisionId() ) ) {
+ $previousCollectionId =
null;
+ return '';
+ }
+ } catch ( Exception $e ) {
+ // nothing to do here; if
fetching last revision failed,
+ // we're just not testing any
stored revision; that's ok
+ }
+ }
+ $previousCollectionId = $collection->getId();
+
+ /*
+ * If a revision was the result of a
restore-action, we have
+ * to look at the previous revision what the
original moderation
+ * status was; permissions for the
restore-actions visibility
+ * is the same as the moderation (e.g. if user
can't see
+ * suppress actions, he can't see restores from
suppress.
+ */
if ( $revision->getChangeType() ==
'restore-post' ) {
- $revisionable =
$revision->getCollection();
- $previous =
$revisionable->getPrevRevision( $revision );
+ $previous =
$collection->getPrevRevision( $revision );
if ( $previous->getModerationState()
=== AbstractRevision::MODERATED_NONE ) {
return '';
diff --git a/includes/Block/Header.php b/includes/Block/Header.php
index f7aac7f..785a109 100644
--- a/includes/Block/Header.php
+++ b/includes/Block/Header.php
@@ -206,8 +206,10 @@
// get rid of history entries user doesn't have sufficient
permissions for
foreach ( $history as $i => $revision ) {
+ /** @var PostRevision|Header $revision */
+
// only check against the specific revision, ignoring
the most recent
- if ( !$this->permissions->isRevisionAllowed( $revision,
'post-history' ) ) {
+ if ( !$this->permissions->isAllowed( $revision,
'post-history' ) ) {
unset( $history[$i] );
}
}
diff --git a/includes/Block/Topic.php b/includes/Block/Topic.php
index 40534ab..2bee4eb 100644
--- a/includes/Block/Topic.php
+++ b/includes/Block/Topic.php
@@ -755,8 +755,10 @@
if ( $history ) {
// get rid of history entries user doesn't have
sufficient permissions for
foreach ( $history as $i => $revision ) {
+ /** @var PostRevision $revision */
+
// only check against the specific revision,
ignoring the most recent
- if ( !$this->permissions->isRevisionAllowed(
$revision, 'post-history' ) ) {
+ if ( !$this->permissions->isAllowed( $revision,
'post-history' ) ) {
unset( $history[$i] );
}
}
@@ -869,8 +871,10 @@
if ( $history ) {
// get rid of history entries user doesn't have
sufficient permissions for
foreach ( $history as $i => $revision ) {
+ /** @var PostRevision $revision */
+
// only check against the specific revision,
ignoring the most recent
- if ( !$this->permissions->isRevisionAllowed(
$revision, 'topic-history' ) ) {
+ if ( !$this->permissions->isAllowed( $revision,
'topic-history' ) ) {
unset( $history[$i] );
}
}
diff --git a/includes/Formatter/CheckUser.php b/includes/Formatter/CheckUser.php
index 8010e12..d359f9e 100644
--- a/includes/Formatter/CheckUser.php
+++ b/includes/Formatter/CheckUser.php
@@ -22,7 +22,7 @@
// @todo: this currently only implements CU links
// we'll probably want to add a hook to CheckUser that lets us
blank out
- // the entire line for entries that !isRevisionAllowed(
<this-revision>, 'checkuser' )
+ // the entire line for entries that !isAllowed(
<this-revision>, 'checkuser' )
$data = explode( ',', $row->cuc_comment );
$post = null;
diff --git a/includes/Formatter/Contributions.php
b/includes/Formatter/Contributions.php
index 13f70e8..e96dc23 100644
--- a/includes/Formatter/Contributions.php
+++ b/includes/Formatter/Contributions.php
@@ -30,7 +30,7 @@
$user = $pager->getUser();
$title = $workflow->getArticleTitle();
- if ( !$this->getPermissions( $user )->isRevisionAllowed(
$revision, 'contributions' ) ) {
+ if ( !$this->getPermissions( $user )->isAllowed( $revision,
'contributions' ) ) {
return false;
}
diff --git a/includes/Formatter/RecentChanges.php
b/includes/Formatter/RecentChanges.php
index 2081c31..99a1137 100644
--- a/includes/Formatter/RecentChanges.php
+++ b/includes/Formatter/RecentChanges.php
@@ -68,7 +68,7 @@
return false;
}
- if ( !$this->getPermissions( $user )->isRevisionAllowed(
$revision, 'recentchanges' ) ) {
+ if ( !$this->getPermissions( $user )->isAllowed( $revision,
'recentchanges' ) ) {
return false;
}
diff --git a/includes/Log/Formatter.php b/includes/Log/Formatter.php
index 5de483c..ca3450c 100644
--- a/includes/Log/Formatter.php
+++ b/includes/Log/Formatter.php
@@ -20,11 +20,11 @@
$skin = $this->plaintext ? null : $this->context->getSkin();
$params = $this->entry->getParameters();
- // @todo: we should probably check if user isRevisionAllowed(
<this-revision>, 'log' )
+ // @todo: we should probably check if user isAllowed(
<this-revision>, 'log' )
// unlike RC, Contributions, ... this one does not batch-load
all Flow
// revisions & does not use the same Formatter, i18n message
text, etc
// I assume this will change with
https://trello.com/c/S10KfqBm/62-8-history-watchlist-rc-and-contribs-changes
- // Then, we should also add in the isRevisionAllowed check!
+ // Then, we should also add in the isAllowed check!
// FIXME this is ugly. Why were we treating log parameters as
// URL GET parameters in the first place?
diff --git a/includes/RevisionActionPermissions.php
b/includes/RevisionActionPermissions.php
index 2d59074..bb3eb56 100644
--- a/includes/RevisionActionPermissions.php
+++ b/includes/RevisionActionPermissions.php
@@ -63,17 +63,13 @@
try {
// Also check if the user would be allowed to perform
this against
- // against the most recent revision (unless it's
already the most recent
- // revision) - the last revision is the current state
of a revision, so
- // checking against a revision at one point in time
alone isn't enough.
+ // against the most recent revision - the last revision
is the
+ // current state of an object, so checking against a
revision at one
+ // point in time alone isn't enough.
$last = $revision->getCollection()->getLastRevision();
+ return $allowed && $this->isRevisionAllowed( $last,
$action );
- // check if $revision is not already the most recent,
to prevent
- // infinite recursion in this method
- $isLastRevision = $last->getRevisionId()->equals(
$revision->getRevisionId() );
- return $allowed && ( $isLastRevision ||
$this->isAllowed( $last, $action ) );
-
- // If data is not in storage, just return that
revision's status
+ // If data is not in storage, just return that revision's status
} catch ( InvalidDataException $e ) {
return $allowed;
}
@@ -113,7 +109,7 @@
* @param string $action
* @return bool
*/
- public function isRevisionAllowed( AbstractRevision $revision = null,
$action ) {
+ protected function isRevisionAllowed( AbstractRevision $revision =
null, $action ) {
// Users must have the core 'edit' permission to perform any
write action in flow
$performsWrites = $this->actions->getValue( $action,
'performs-writes' );
if ( $performsWrites && ( !$this->user->isAllowed( 'edit' ) ||
$this->user->isBlocked() ) ) {
@@ -133,33 +129,6 @@
array( $this->user, 'isAllowedAny' ),
(array) $permission
);
- }
-
- /**
- * Check if a user is allowed to perform certain actions, only against 1
- * specific revision (whereas the default isAllowed() will check if the
- * given $action is allowed for both given and the most current
revision)
- *
- * @param AbstractRevision|null $revision
- * @param string $action... Multiple parameters to check if either of
the provided actions are allowed
- * @return bool
- */
- public function isRevisionAllowedAny( AbstractRevision $revision =
null, $action /* [, $action2 [, ... ]] */ ) {
- $actions = func_get_args();
- // Pull $revision out of the actions list
- array_shift( $actions );
- $allowed = false;
-
- foreach ( $actions as $action ) {
- $allowed |= $this->isRevisionAllowed( $revision,
$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 1cd5966..ef26b16 100644
--- a/includes/Templating.php
+++ b/includes/Templating.php
@@ -307,7 +307,7 @@
* @return string
*/
public function getUserText( AbstractRevision $revision ) {
- if ( $this->permissions->isRevisionAllowed( $revision, 'view' )
) {
+ if ( $this->permissions->isAllowed( $revision, 'view' ) ) {
return $this->usernames->get( wfWikiId(),
$revision->getUserId(), $revision->getUserIp() );
} else {
$username = $this->usernames->get(
@@ -337,7 +337,7 @@
* @return string HTML
*/
public function getUserLinks( AbstractRevision $revision ) {
- if ( $this->permissions->isRevisionAllowed( $revision, 'view' )
) {
+ if ( $this->permissions->isAllowed( $revision, 'view' ) ) {
$userid = $revision->getUserId();
$username = $this->usernames->get( wfWikiId(),
$revision->getUserId(), $revision->getUserIp() );
return Linker::userLink( $userid, $username ) .
Linker::userToolLinks( $userid, $username );
@@ -374,7 +374,7 @@
* @return string
*/
public function getCreatorText( PostRevision $revision ) {
- if ( $this->permissions->isRevisionAllowed( $revision, 'view' )
) {
+ if ( $this->permissions->isAllowed( $revision, 'view' ) ) {
return $this->usernames->get(
wfWikiId(),
$revision->getCreatorId(),
@@ -414,7 +414,7 @@
* @return string HTML
*/
public function getContent( AbstractRevision $revision, $format =
'html' ) {
- if ( $this->permissions->isRevisionAllowed( $revision, 'view' )
) {
+ if ( $this->permissions->isAllowed( $revision, 'view' ) ) {
$content = $revision->getContent( $format );
if ( $format === 'html' ) {
diff --git a/tests/RevisionCollectionPermissionsTest.php
b/tests/RevisionCollectionPermissionsTest.php
index 62637af..604f423 100644
--- a/tests/RevisionCollectionPermissionsTest.php
+++ b/tests/RevisionCollectionPermissionsTest.php
@@ -147,13 +147,24 @@
array( 'restore-post' => true ),
array( 'suppress-post' => true ),
) ),
+
+ // bug 61715
+ array( $this->confirmedUser(), 'topic-history', array(
+ array( 'new-post' => false ),
+ array( 'suppress-post' => false ),
+ ) ),
+ array( $this->confirmedUser(), 'topic-history', array(
+ array( 'new-post' => true ),
+ array( 'suppress-post' => false ),
+ array( 'restore-post' => false ),
+ ) ),
);
}
/**
* @dataProvider permissionsProvider
*/
- public function testPermissions( User $user, $action, $actions ) {
+ public function testPermissions( User $user, $permisisonAction,
$actions ) {
$permissions = new RevisionActionPermissions( $this->actions,
$user );
// we'll have to process this in 2 steps: first do all of the
actions,
@@ -171,8 +182,8 @@
$revision = array_shift( $revisions );
$this->assertEquals(
$expected,
- $permissions->isAllowed( $revision, 'view' ),
- 'User ' . $user->getName() . ' should ' . (
$expected ? '' : 'not' ) . ' be allowed to view revision ' . key( $action )
+ $permissions->isAllowed( $revision,
$permisisonAction ),
+ 'User ' . $user->getName() . ' should ' . (
$expected ? '' : 'not ' ) . 'be allowed action ' . $permisisonAction . ' on
revision ' . key( $action )
);
}
}
--
To view, visit https://gerrit.wikimedia.org/r/114679
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: Ia27c94176bb8f40f2fe683a54ab9ddafb4f98fc0
Gerrit-PatchSet: 2
Gerrit-Project: mediawiki/extensions/Flow
Gerrit-Branch: master
Gerrit-Owner: Matthias Mullie <mmul...@wikimedia.org>
Gerrit-Reviewer: EBernhardson <ebernhard...@wikimedia.org>
Gerrit-Reviewer: jenkins-bot <>
_______________________________________________
MediaWiki-commits mailing list
MediaWiki-commits@lists.wikimedia.org
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
