OpenPNE 3

#4012-7 のコメントを転記

lib/model/doctrine/MemberConfig.class.php:

@@ -117,6 +117,19 @@ public function getSetting()
     return $config[$this->getName()];
   }

+  public function checkUniqueness()

メソッド名からはユニーク性をチェックするように見えますが、内部の処理では設定値からユニーク判定が不要な場合も true と返されますので、メソッド名と処理に差異があるように感じます。

lib/model/doctrine/MemberConfig.class.php:

+    $duplicate = $this->getTable()->retrieveByNameAndValue($this->name, $this->value);

(既に重複しているデータがある前提となりますが) 取得したレコードが複数存在した場合で且つ、
opDoctrineQuery::fetchOne() で取得したレコードがたまたま、ユーザ自身のレコードであった場合、重複判定が回避されます。

lib/action/opMemberAction.class.php:

@@ -309,6 +309,13 @@ public function executeConfigComplete(opWebRequest $request)
         }
         $config->setValue($pre->getValue());

+        if (!$config->checkUniqueness())
+        {
+          $this->getUser()->setFlash('error', 'The inputted value is already exist.');

エラーメッセージが親切であるのは良いことだと思いますが、該当メールアドレスがすでに SNS 上の誰かに紐付いて存在するという情報の暴露には繋がらないでしょうか。
変更しようとして受信したメールアドレスの持ち主自身での操作になると思いますので、難しいところですが。

#4012-9 の指摘について:

lib/model/doctrine/MemberConfig.class.php:

@@ -117,6 +117,19 @@ public function getSetting()
     return $config[$this->getName()];
   }

+  public function checkUniqueness()

メソッド名からはユニーク性をチェックするように見えますが、内部の処理では設定値からユニーク判定が不要な場合も true と返されますので、メソッド名と処理に差異があるように感じます。

修正後のメソッド名として validateUniqueness で考えています。
メソッド名以外は変更なしで、設定値にエラーが無いか重複チェックが不要な場合は true、設定値の重複によるエラーが発生した場合は false が返ります。

lib/model/doctrine/MemberConfig.class.php:

+    $duplicate = $this->getTable()->retrieveByNameAndValue($this->name, $this->value);

(既に重複しているデータがある前提となりますが) 取得したレコードが複数存在した場合で且つ、
opDoctrineQuery::fetchOne() で取得したレコードがたまたま、ユーザ自身のレコードであった場合、重複判定が回避されます。

当チケットの修正前は他のメンバーと重複した pc_address, mobile_address が設定可能な状態であったため、この指摘について考慮が必要となります。

例えば sns@example.com がPCメールアドレスとして重複登録されている場合、現在の修正通りに retrieveByNameAndValue メソッドで name = 'pc_address' AND value = 'sns@example.com' (実際は name_value_hash の値で比較される) に合致するレコードを 1 件だけ抽出すると、上記の表でいう id = 1001 または id = 1002 のどちらのレコードが返るか不定になります。

これを踏まえて「取得したレコードがたまたま、ユーザ自身のレコードであった場合、重複判定が回避されます」というのは、

• member_id = 1 のメンバーがPCメールアドレスを sns@example.com から sns@example.com に変更
• sns@example.com が他のメンバー (member_id = 2) と重複して登録されている
• 重複チェック時に $duplicate に取得されたレコードが id = 1001 であった

の条件が揃った場合になります。
あまり現実味の無い状況ではありますが、以下のように member_id の条件を fetchOne で絞り込むより前に移動することで対処することが可能です。

--- a/lib/model/doctrine/MemberConfig.class.php
+++ b/lib/model/doctrine/MemberConfig.class.php
@@ -125,9 +125,12 @@
       return true;
     }

-    $duplicate = $this->getTable()->retrieveByNameAndValue($this->name, $this->value);
-
-    return !$duplicate || $duplicate->member_id === $this->member_id;
+    $duplicate = $this->getTable()->createQuery('mc')
+      ->andWhere('mc.member_id <> ?', $this->member_id)
+      ->andWhere('mc.name_value_hash = ?', $this->getTable()->generateNameValueHash($this->name, $this->value))
+      ->fetchOne();
+
+    return !$duplicate;
   }

   public function generateRoleId(Member $member)

lib/action/opMemberAction.class.php:

@@ -309,6 +309,13 @@ public function executeConfigComplete(opWebRequest $request)
         }
         $config->setValue($pre->getValue());

+        if (!$config->checkUniqueness())
+        {
+          $this->getUser()->setFlash('error', 'The inputted value is already exist.');

エラーメッセージが親切であるのは良いことだと思いますが、該当メールアドレスがすでに SNS 上の誰かに紐付いて存在するという情報の暴露には繋がらないでしょうか。
変更しようとして受信したメールアドレスの持ち主自身での操作になると思いますので、難しいところですが。

この箇所については、変更後のメールアドレスの所有者自身でなければこのエラーメッセージに辿り着くことは無いため、現状の修正のままで問題は無いと考えています。