From 846f01e221a9dd379ce40d680e4a6b414d785000 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Thu, 25 Oct 2012 16:27:49 -0700
Subject: [PATCH] Correct self-review check in Differential

Summary: This check is currently wrong -- the actor is only //coincidentally// the owner (and only most of the time). It also raises at parse time, preventing any user from parsing a message with their own name in the "Reviewers" field. Instead, check against the right owner PHID and raise it only if a revision is available. See https://github.com/facebook/arcanist/issues/54 and next diff.

Test Plan: Tried to add myself as a reviewer to revisions I own via web and Conduit, got rejected. Parsed a message with myself in the "Reviewers:" field, it worked correctly.

Reviewers: btrahan, vrana

Reviewed By: btrahan

CC: aran

Differential Revision: https://secure.phabricator.com/D3820
---
 .../DifferentialFieldSpecification.php        | 11 +++++++++
 ...ifferentialReviewersFieldSpecification.php | 23 +++++++++++++------
 2 files changed, 27 insertions(+), 7 deletions(-)

diff --git a/src/applications/differential/field/specification/DifferentialFieldSpecification.php b/src/applications/differential/field/specification/DifferentialFieldSpecification.php
index 414e1e9169..046704d139 100644
--- a/src/applications/differential/field/specification/DifferentialFieldSpecification.php
+++ b/src/applications/differential/field/specification/DifferentialFieldSpecification.php
@@ -841,6 +841,17 @@ abstract class DifferentialFieldSpecification {
     return $this->revision;
   }
 
+
+  /**
+   * Determine if revision context is currently available.
+   *
+   * @task context
+   */
+  final protected function hasRevision() {
+    return (bool)$this->revision;
+  }
+
+
   /**
    * @task context
    */
diff --git a/src/applications/differential/field/specification/DifferentialReviewersFieldSpecification.php b/src/applications/differential/field/specification/DifferentialReviewersFieldSpecification.php
index 47783a9d43..c6afb6e93d 100644
--- a/src/applications/differential/field/specification/DifferentialReviewersFieldSpecification.php
+++ b/src/applications/differential/field/specification/DifferentialReviewersFieldSpecification.php
@@ -61,14 +61,23 @@ final class DifferentialReviewersFieldSpecification
   }
 
   public function validateField() {
-    $allow_self_accept = PhabricatorEnv::getEnvConfig(
-       'differential.allow-self-accept', false);
-    if (!$allow_self_accept
-        && in_array($this->getUser()->getPHID(), $this->reviewers)) {
-      $this->error = 'Invalid';
-      throw new DifferentialFieldValidationException(
-        "You may not review your own revision!");
+    if (!$this->hasRevision()) {
+      return;
     }
+
+    $self = PhabricatorEnv::getEnvConfig('differential.allow-self-accept');
+    if ($self) {
+      return;
+    }
+
+    $author_phid = $this->getRevision()->getAuthorPHID();
+    if (!in_array($author_phid, $this->reviewers)) {
+      return;
+    }
+
+    $this->error = 'Invalid';
+    throw new DifferentialFieldValidationException(
+      "The owner of a revision may not be a reviewer.");
   }
 
   public function renderEditControl() {