OpenPNE 3

再現方法はこちらをご覧ください #2587

コード追跡¶

lib/action/opMemberAction.class.php 128行目

123   public function executeRegisterInput($request)
124   {
125     $this->forward404Unless(opToolkit::isEnabledRegistration((sfConfig::get('app_is_mobile') ? 'mobile' : 'pc')));
126 
127     $this->token = $request['token'];
128     $member = $this->getUser()->setRegisterToken($this->token);
129 
130     $this->forward404Unless($member && $this->getUser()->isRegisterBegin());
131 
132     opActivateBehavior::disable();
133     $this->form = $this->getUser()->getAuthAdapter()->getAuthRegisterForm();
134     opActivateBehavior::enable();
135 
136     if ($request->isMethod('post'))
137     {
138       $this->form->bindAll($request);
139 
140       if ($this->form->isValidAll())
141       {
142         $result = $this->getUser()->register($this->form);
143         $this->redirectIf($result, $this->getUser()->getRegisterEndAction($this->token));
144       }
145     }
146 
147     return sfView::SUCCESS;
148   }

lib/user/opSecurityUser.class.php 420行目

418   public function setRegisterToken($token)
419   {
420     $member = Doctrine::getTable('Member')->findByValidRegisterToken($token);
421     if (!$member)
422     {
423       return false;
424     }
425 
426     $this->setMemberId($member->getId());
427 
428     $authMode = $member->getConfig('register_auth_mode');
429     if ($authMode)
430     {
431       $this->setCurrentAuthMode($authMode);
432     }
433 
434     return $member;
435   }
436 
437   public function setSid($sid, $isRememberLogin = false)
438   {
439     if ($this->isAuthenticated())
440     {
441       return false;
442     }
443 
444     session_write_close();
445 
446     // set session id from request
447     session_id($sid);
448     session_start();
449     session_write_close();
450 
451     if ($isRememberLogin)
452     {
453       $this->setRememberLoginCookie();
454     }
455   }

lib/model/doctrine/MemberTable.class.php

156   public function findByValidRegisterToken($token)
157   {
158     $member = $this->findByRegisterToken($token);
159     if (!$member)
160     {
161       return false;
162     }
163 
164     $configTable = Doctrine::getTable('MemberConfig');
165 
166     $mailTypes = array("pc_address", "pc_address_pre", "mobile_address", "mobile_address_pre");
167     $query = $configTable->createQuery('m');
168     foreach ($mailTypes as $mailType)
169     {
170       $hash = $configTable->generateNameValueHash($mailType, $member->getConfig($mailType));
171       $query->orWhere('m.name_value_hash = ?', $hash);
172     }
173     $configs = $query->fetchArray();
174 
175     $memberIds = array();
176     $updateTimes = array();
177     foreach ($configs as $config)
178     {
179       $memberIds[] = $memberId = $config['member_id'];
180       $updateTimes[] = $configTable
181         ->createQuery('m')
182         ->where('m.member_id = ?', $memberId)
183         ->AndWhere('m.name = "register_token"')
184         ->fetchOne()
185         ->getUpdatedAt();
186     }
187     array_multisort($updateTimes, $memberIds);
188 
189     if ($member->getId() !== $memberIds[count($memberIds)-1])
190     {
191       return false;
192     }
193 
194     return $member;
195   }

前提¶

もともと仕様として OpenPNE 内で登録されるメールアドレス設定は重複不可であるということが与えられている．このとき他の設定がどうであるかは考慮していない．仕様としてはそうであったが， #1816 修正前では重複したデータが存在する可能性があったため #1816 で修正が行われた．しかし，その修正は新たに設定されるメールアドレスに関して重複しないようにする修正であったため，その修正以前の状態である状態では重複したものが存在することに対応する必要があった． この修正を #2340 で行ったが，この修正はトークンに紐付くメールアドレスがすでに SNS 内に存在しないもの，かつ，pc_address_pre や mobile_address_pre などまだ登録されていない状態のものにメールアドレスが存在する場合に最新のトークンであるかを判定することを行った．このときトークンに紐付く設定がメールアドレスのみを考慮していた．

原因¶

「前提」に記述した通り，トークンに紐付く設定をメールアドレスのみ考慮していたため， lib/model/doctrine/MemberTable.class.php の findByValidRegisterToken() メソッドでメールアドレスのみを用いて register_token が正しいものかどうかを判定していた．しかし登録時には regsiger_token と紐付くデータを認証プラグインに合うように本体側でチェックすることはできず，すなわち本体側でメールアドレスのみによって正しさを決定することは適切ではない．

修正案¶

トークンが正しいかどうかをプラグイン側で判定する実装が正しいと思われる．プラグイン側で実装するべき理由として，実際にはトークンに紐付く重複が不可としていることを保証できないことと，トークンの正しさに最新かどうかを仕様として含めるかどうかが不明なことが挙げられる．しかし，今回の実装では暫定的に，設定がユニークであると本体側で決まっているメールアドレス設定を用いた認証である opAuthMailAddressPlugin による認証の場合のみでトークンが正しいかどうかを調べる方針とする．これはあくまでも暫定的であるため該当部分は今後改善チケットとして削除されなければならない．

実装案¶

https://redmine.openpne.jp/issues/2607#note-3

• 認証プラグインがopAuthMailAddressPluginの場合のみトークンが正しいかどうかを判定する処理を設ける．
• opAuthMailAddressPluginに依存しない実装にした場合に変更しなければならない部分を減らすために$mailTypesをより呼び出し側に近いところに持ってきた．
• findByValidRegisterToken()をMemberConfig名群について最新のregister_tokenを持っているかどうかを調べる汎用的なメソッドに修正した．
• findByValidRegisterToken()内で必要とするデータはすべてMemberConfigTableであるが，findByRegisterToken()がMemberTable内にあるため本修正案ではクラスをまたいだメソッドの移動は採用していない．

diff --git a/lib/model/doctrine/MemberTable.class.php b/lib/model/doctrine/MemberTable.class.php
index 4ec3816..8ad8a28 100644
--- a/lib/model/doctrine/MemberTable.class.php
+++ b/lib/model/doctrine/MemberTable.class.php
@@ -153,21 +153,19 @@ class MemberTable extends opAccessControlDoctrineTable
     return $this->findInactive($config->getMemberId());
   }

-  public function findByValidRegisterToken($token)
+  public function findByValidRegisterToken($token, $configNames)
   {
     $member = $this->findByRegisterToken($token);
     if (!$member)
     {
       return false;
     }
-
     $configTable = Doctrine::getTable('MemberConfig');

-    $mailTypes = array("pc_address", "pc_address_pre", "mobile_address", "mobile_address_pre");
     $query = $configTable->createQuery('m');
-    foreach ($mailTypes as $mailType)
+    foreach ($configNames as $configName)
     {
-      $hash = $configTable->generateNameValueHash($mailType, $member->getConfig($mailType));
+      $hash = $configTable->generateNameValueHash($configName, $member->getConfig($configName));
       $query->orWhere('m.name_value_hash = ?', $hash);
     }
     $configs = $query->fetchArray();
diff --git a/lib/user/opSecurityUser.class.php b/lib/user/opSecurityUser.class.php
index be67cc6..60ff898 100644
--- a/lib/user/opSecurityUser.class.php
+++ b/lib/user/opSecurityUser.class.php
@@ -417,7 +417,15 @@ class opSecurityUser extends opAdaptableUser

   public function setRegisterToken($token)
   {
-    $member = Doctrine::getTable('Member')->findByValidRegisterToken($token);
+    if ('MailAddress' === $this->getAuthAdapter()->getAuthModeName())
+    {
+      $mailTypes = array("pc_address", "pc_address_pre", "mobile_address", "mobile_address_pre");
+      $member = Doctrine::getTable('Member')->findByValidRegisterToken($token, $mailTypes);
+    }
+    else
+    {
+      $member = Doctrine::getTable('Member')->findByRegisterToken($token);
+    }
     if (!$member)
     {
       return false;