Project

General

Profile

Bug(バグ) #2588

3.6RC1以降、仮登録時にメールアドレスが登録されないと新規登録がおこなえない

Added by Kiwa Sakai almost 8 years ago. Updated almost 4 years ago.

Status:
Fixed(完了)
Priority:
Urgent(急いで)
Assignee:
Target version:
Start date:
2011-11-08
Due date:
% Done:

100%

3.6 で発生するか:
Unknown (未調査)
3.8 で発生するか:
Unknown (未調査)

Description

Overview (現象)

OpenPNE 3.6RC1 〜 のバージョンだと、仮登録時にメールアドレスが登録されないと新規登録が404エラーとなり、メンバー登録を完了できない。

メンバー登録がおこなえない例

opAuthOpenIDPlugin を利用していて、OpenIDプロバイダ側からメールアドレスが共有されない場合

現象確認バージョン

OpenPNE 3.6.0 (OpenPNE 3.6beta14 では発生せず)

Causes (原因)

Way to fix (修正内容)


Related issues

Related to OpenPNE 3 - Bug(バグ) #2340: 重複登録しないように修正する前の環境で送信した招待メールでは重複登録が可能になってしまっている Fixed(完了) 2011-08-03
Related to opAuthOpenIDPlugin - Bug(バグ) #2587: OpenID の初回ログイン時にプロバイダ側からメールアドレスを共有されないと新規登録に失敗する Invalid(無効) 2011-11-07
Related to OpenPNE 3 - Backport(バックポート) #2369: 重複登録しないように修正する前の環境で送信した招待メールでは重複登録が可能になってしまっている Fixed(完了) 2011-08-03
Related to OpenPNE 3 - Backport(バックポート) #2607: 3.6RC1以降、仮登録時にメールアドレスが登録されないと新規登録がおこなえない Fixed(完了) 2011-11-08

Associated revisions

Revision b6c699ee (diff)
Added by Yuya Watanabe over 7 years ago

(fixes #2588) fixed to validate token when auth plugin is mail address

Revision 4a8b54c2 (diff)
Added by Yuya Watanabe over 7 years ago

(fixes #2588) fixed to validate token when auth plugin is mail address

History

#1 Updated by Kiwa Sakai almost 8 years ago

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

#2 Updated by Yuya Watanabe almost 8 years ago

コード追跡

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   }

#3 Updated by Kousuke Ebihara almost 8 years ago

  • Priority changed from Normal(通常) to Urgent(急いで)

#4 Updated by Yuya Watanabe over 7 years ago

  • Assignee set to Yuya Watanabe
  • Target version set to OpenPNE 3.7.0

#5 Updated by Yuya Watanabe over 7 years ago

前提

もともと仕様として 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;

#6 Updated by Yuya Watanabe over 7 years ago

  • Status changed from New(新規) to Pending Review(レビュー待ち)
  • % Done changed from 0 to 50

更新履歴 b6c699ee2f154bac6bc5357393e92c5f99a272cf で適用されました。

#7 Updated by Kousuke Ebihara over 7 years ago

  • Status changed from Pending Review(レビュー待ち) to Pending Testing(テスト待ち)
  • % Done changed from 50 to 70

#8 Updated by Shouta Kashiwagi over 7 years ago

  • Status changed from Pending Testing(テスト待ち) to Fixed(完了)
  • % Done changed from 70 to 100
  • 3.6 で発生するか set to Unknown (未調査)
  • 3.4 で発生するか set to Unknown (未調査)

テストOKです。

#9 Updated by kaoru n almost 4 years ago

  • 3.8 で発生するか set to Unknown (未調査)

Also available in: Atom PDF