OpenPNE 3

http://redmine.openpne.jp/issues/1895#note-19 に関連するバグなのでコメントしておきます。

このエラーは opSecurityUser::getMember() が登録画面を開いたが登録完了していないユーザーの場合に opAnonymousMember のインスタンスではなく false を返すこと（もしくは opDynamicAclRoute がそれを想定していないこと）が原因です。

また、エラー個所の直後にある opDynamicAclRoute::getCurrentMemberId() の呼び出しは http://redmine.openpne.jp/issues/1895#note-19 の opDiaryPlugin と同様に、認証状態や is_active フラグの確認なしに opSecurityUser::getMemberId() を利用される危険性があると思われます。

これらの影響範囲について他に問題がないかどうか調査する必要がありますが、調査の結果このチケットにある「権限がない場合に白画面になる」だけが問題なのであれば今回の緊急リリースに入れる必要はないと考えます。

「権限がない場合に白画面になる」だけが問題なのであれば今回の緊急リリースに入れる必要はないと考えます。

自分も同意です。
再現手法を公開してしまうことは問題だと思いますので、今回のリリースには含めないが、Security Issueで対応するが望ましいと思います。

追加テストメモ¶

（違う問題かもしれませんが手順が類似しているため、あえて同じチケットに報告しています）
仮登録状態のメンバーにフレンド申請をすると、下記のエラーが発生します。
アクセス出来ませんなどのエラーを表示するのが望ましいかと思います。

蜀咏悄
Fatal error: Call to a member function getImageFileName() on a non-object in /home/imamura/sns/36x.bughunt.uh-openpne5.pne.jp/apps/pc_frontend/modules/friend/templates/linkInput.php on line 5

再現手順¶

1. 管理画面のメンバーリストを確認し一番最後のメンバーIDを確認する（メンバーID：56とする）
2. 管理画面からメンバー A 宛に招待メールを送信する
3. メンバー A がその招待メールに記述された URL にアクセスする
4. 別のブラウザを開き登録済みのユーザーでメンバーID：57にフレンド申請をする
   （一番最後に登録されたメンバーIDが56だった場合、次に登録されるIDは57になるはずなので）

アクセスしたURL↓
http://36x.bughunt.uh-openpne5.pne.jp/pc_frontend_dev.php/friend/link/id/57

http://redmine.openpne.jp/issues/1895#note-21 と同様、 OpenPNE 3.6.0 開発期間中のどこかで進める方針で作業をおこないたいです。

再現手法を公開してしまうことは問題だと思いますので、今回のリリースには含めないが、Security Issueで対応するが望ましいと思います。

とありますが、本件は脆弱性とは言えない（秘密にしておく強い理由がない）ため、 Security Issue で対応するのは反対です。今回の緊急リリース後にチケットを公開状態にするというのはどうでしょう。
（ただ、この問題を利用した悪用可能な脆弱性が発覚する可能性はあります。したがって、公開する前にこの問題が悪用可能でないか調べることは必要です）

http://redmine.openpne.jp/issues/1944#note-4 のとおりこのチケットを公開状態にしました。

本チケットの問題¶

• lib/user/opSecurityUser.class.php
  

  61-  public function getMember($inactive = false)
  62-  {
  63-    if (!$this->getMemberId())
  64-    {
  65-      return new opAnonymousMember();
  66-    }
  67-
  68-    if ($inactive)
  69-    {
  70-      return Doctrine::getTable('Member')->findInactive($this->getMemberId());
  71-    }
  72-
  73-    if ($this->serializedMember)
  74-    {
  75-      return unserialize($this->serializedMember);
  76-    }
  77-
  78-    $result = Doctrine::getTable('Member')->find($this->getMemberId());
  79-    if ($result)
  80-    {
  81-      $this->serializedMember = serialize($result);
  82-    }
  83-
  84-    return $result;
  85-  }
  

• この 79-82 行目の if 文に対する else 文で（65行目のように） return new opAnonymousMember(); を行ってしまえばよいように思えるが、現状このように実装されている経緯をもう少し追ってみる必要があるかもしれない。
  • 79-82 行目の if 文が記述されたのは issue/940 issue/960 でのコミット であり、このコメントでの修正方針が正しければその issue/940 issue/960 のコミット時点で else 文が追加されていても不自然ではないように思える。
  • しかしそうなっていないのは、単に『このエラーは opSecurityUser::getMember() が登録画面を開いたが登録完了していないユーザーの場合に opAnonymousMember のインスタンスではなく false を返すこと（もしくは opDynamicAclRoute がそれを想定していないこと）が原因です。』と示されているように、仮登録状態を考慮し忘れていただけのように見える。

note-3 の問題¶

追加テストメモ¶

（違う問題かもしれませんが手順が類似しているため、あえて同じチケットに報告しています）
仮登録状態のメンバーにフレンド申請をすると、下記のエラーが発生します。
アクセス出来ませんなどのエラーを表示するのが望ましいかと思います。

[...]

再現手順¶

1. 管理画面のメンバーリストを確認し一番最後のメンバーIDを確認する（メンバーID：56とする）
2. 管理画面からメンバー A 宛に招待メールを送信する
3. メンバー A がその招待メールに記述された URL にアクセスする
4. 別のブラウザを開き登録済みのユーザーでメンバーID：57にフレンド申請をする
   （一番最後に登録されたメンバーIDが56だった場合、次に登録されるIDは57になるはずなので）

アクセスしたURL↓
http://36x.bughunt.uh-openpne5.pne.jp/pc_frontend_dev.php/friend/link/id/57

この問題は OpenPNE-3.6beta8 で発生することは確認できましたが、現在の master では発生しないようです。この差分の間に問題を解消する修正が含まれているかもしれません。

Minoru Takai は書きました:

本チケットの問題¶

• lib/user/opSecurityUser.class.php
  [...]
• この 79-82 行目の if 文に対する else 文で（65行目のように） return new opAnonymousMember(); を行ってしまえばよいように思えるが、現状このように実装されている経緯をもう少し追ってみる必要があるかもしれない。
  • 79-82 行目の if 文が記述されたのは issue/940 でのコミット であり、このコメントでの修正方針が正しければその issue/940 のコミット時点で else 文が追加されていても不自然ではないように思える。
  • しかしそうなっていないのは、単に『このエラーは opSecurityUser::getMember() が登録画面を開いたが登録完了していないユーザーの場合に opAnonymousMember のインスタンスではなく false を返すこと（もしくは opDynamicAclRoute がそれを想定していないこと）が原因です。』と示されているように、仮登録状態を考慮し忘れていただけのように見える。

opSecurityUser::getMember() が false ではなく opAnonymousMember のインスタンスを返すようにすることで本チケット表題の問題は解決するかもしれませんが、 note-1 のコメントで触れたとおり opDynamicAclRoute::getCurrentMemberId() から呼ばれている opSecurityUser::getMemberId() が値を返すことによる危険性は残ったままとなります。

根本的な対応としては、登録完了前は opSecurityUser::getMemberId() が値を返さないようにする、そのために登録完了前はセッションストレージに member_id を保存しないという実装をすべきだと考えます。
（別チケットでの対応が妥当かもしれませんが、getMemberId() を先に修正すれば getMember() は修正の必要がない可能性があります）

#2285 との関連が無いかを調べる必要があります。

まず、 note-12 で示されている内容の以下の項目について同意見であり、とくに認識の違う部分はないことを表明しておきます。

• 「(P1) のみを扱う」とせず、「(P1) および (P2)」を解消するのがよい
• (s2) または (s3) での対応を行うと (P1) も併せて問題が解消される

(P3) に関しては詳しく追っていませんが、同時に直るものでなければ別チケット扱いで構わないと思います。

残るは (s2) と (s3) のどちらの修正方針が適切かという部分だと思います。

(s2) と (s3) の二者に関して、(s3) が適切だと判断される理由（登録完了前はセッションストレージに member_id を保存しないようにすることの副作用はないか、なぜこれまでは保存していたのか）を明確にしておきたいと思います。

セッションストレージの member_id の値は現状では opSecurityUser::getMemberId() 以外から直接参照されることはないため（要確認）、保存しないことによる副作用はないのではないかと思っています。

// "副作用" という意味では (s2), (s3) に共通している opSecurityUser::getMemberId() が値を返さないようにすること自体にはかなりあると思われます（ただし、これに関しては副作用を見極めて仕様変更および影響部分の修正をすべきと考えます）

また、 http://redmine.openpne.jp/issues/1895#note-19 のコメントに書いたとおり、

登録中は register token を引き回す仕様であるため、本来 member_id をセッションストレージに保持する必要はないはず

という認識ですので、この認識が正しければ「これまでセッションストレージに保存していた」のは冗長な処理だったのではないかという理由から (s3) の修正方針の方が適切だという意見です。

ただし、バージョンアップなどで既にセッションストレージに member_id が保存されている場合を考慮して (s2) の修正を単体もしくは (s3) との併用で行うのは有効かもしれません。

再度、修正方針について検討した結果、 (s3) の対応は影響範囲が広いため本チケットでは難しいと判断し、 (s2) をベースとした具体的な修正方針についてまとめました。

opSecurityUser の問題¶

本チケットで扱う (P1), (P2) の問題を opSecurityUser で書き直してみます。

• (P1') opSecurityUser::getMember() が false を返してしまう。
  • Member のインスタンス（opAnonymousMember 含む）以外の値を返してしまうことにより、それを考慮しないメソッドコールがあった場合に Fatal error が発生してしまう。
• (P2') opSecurityUser::getMemberId() が、仮登録中やログイン停止中、DBに存在しない場合でも、セッションデータ内のメンバーIDをそのまま返してしまう。
  • 取得した値を登録済み、ログイン中のメンバーIDとしてそのまま使ってしまう危険性がある。

以下の5つの状態のメンバーについて opSecurityMember の現在の実装と、修正案を示します。

opSecurityMember 現在の実装¶

opSecurityMember 修正案¶

• 2, 5 の場合については、 opSecurityUser のインスタンス初期化時にログアウト処理を行うようにすれば 1 と同じ状態にすることが可能である。
  • この対応については、 #1182 にて先に同様の対応がなされているため本チケットでは扱わないこととする。
• getMemberId() に getMember() と同様に引数 $inactive を追加する。
  • $inactive を指定しない場合は、仮登録中の member_id を返さないように仕様変更する。
  • $inactive を true に指定した場合は仮登録中の member_id が取得できるようにする（member_id は仮登録中もセッションストレージに保存する）。
    • 前コメントの (s3) の方針と相反するが、仮登録中に member_id を利用するすべての箇所で register token から得た member_id を利用するように書き換えることが容易ではないことがわかったため、仮登録中の処理を変更少なく正常に動作させるためにこの引数を導入する。
    • opSecurityUser::getMemberId() が修正されていれば Bug の直接の原因になることはないと考えられるため、仮登録中に member_id をセッションストレージに保存しないようにする変更は別チケットで Enhancement として扱う。
• getMember() は必ず Member もしくは opAnonymousMember のインスタンスを返すようにする。

opSecurityUser 修正例¶

--- a/lib/user/opSecurityUser.class.php
+++ b/lib/user/opSecurityUser.class.php
@@ -42,9 +42,9 @@ class opSecurityUser extends opAdaptableUser
     $this->serializedMember = '';
   }

-  public function getMemberId()
+  public function getMemberId($inactive = false)
   {
-    return $this->getAttribute('member_id', null, 'opSecurityUser');
+    return $this->getMember($inactive)->getId();
   }

   public function setMemberId($memberId)
@@ -54,14 +54,24 @@ class opSecurityUser extends opAdaptableUser

   public function getMember($inactive = false)
   {
-    if (!$this->getMemberId())
+    $memberId = $this->getAttribute('member_id', null, 'opSecurityUser');
+
+    if (!$memberId)
     {
       return new opAnonymousMember();
     }

     if ($inactive)
     {
-      return Doctrine::getTable('Member')->findInactive($this->getMemberId());
+      $member = Doctrine::getTable('Member')->findInactive($memberId);
+      if ($member)
+      {
+        return $member;
+      }
+      else
+      {
+        return new opAnonymousMember();
+      }
     }

     if ($this->serializedMember)
@@ -69,11 +79,15 @@ class opSecurityUser extends opAdaptableUser
       return unserialize($this->serializedMember);
     }

-    $result = Doctrine::getTable('Member')->find($this->getMemberId());
+    $result = Doctrine::getTable('Member')->find($memberId);
     if ($result)
     {
       $this->serializedMember = serialize($result);
     }
+    else
+    {
+      return new opAnonymousMember();
+    }

     return $result;
   }

これに加えて opSecurityUser::getMemberId(), opSecurityUser::getMember() を利用しており仕様変更の影響がある箇所を修正する必要がある。

特に以下の箇所を確認する必要がある。

• 仮登録中に opSecurityUser::getMemberId() を呼び出している箇所
• opSecurityUser::getMember() に引数として true を渡して使用している箇所

note-16 の方針で修正を行います。

調査¶

• 仮登録中に opSecurityUser::getMemberId() を呼び出している箇所

仮登録中に、（仮登録メンバーの member_id を取得する目的で） opSecurityUser::getMemberId() を呼んでいる箇所は、 opSecurityUser クラス内以外にはなさそうでした。

opSecurityUser クラスでは、以下の 3 箇所で呼び出されています。少なくとも isRegisterBegin() メソッドでは、仮登録中メンバーの member_id を取得しようとしているので併せて修正する必要がありそうです（ isRegisterFinish() はどうなのだろう）。

• setRememberLoginCookie() メソッド
  

  131- /**
  132-  * set remember login cookie
  133-  */
  134-  protected function setRememberLoginCookie($isDeleteCookie = false)
   :
  158-    else
  159-    {
  160-      $rememberKey = opToolkit::getRandom();
  161:      if (!$this->getMemberId())
  162-      {
  163-        throw new LogicException('No login');
  164-      }
  165-      $this->getMember()->setConfig('remember_key', $rememberKey);
  166-
  167:      $value = base64_encode(serialize(array($this->getMemberId(), $rememberKey)));
  168-      $expire = time() + sfConfig::get('op_remember_login_limit', 60*60*24*30);
  169-    }
  

• isRegisterBegin() メソッド
  

  341-  public function isRegisterBegin()
  342-  {
  343:    return $this->getAuthAdapter()->isRegisterBegin($this->getMemberId());
  344-  }
  

• isRegisterFinish() メソッド
  

  346-  public function isRegisterFinish()
  347-  {
  348:    return $this->getAuthAdapter()->isRegisterFinish($this->getMemberId());
  349-  }
  

なお、 opAuthMailAddressPlugin 側では、

plugins/opAuthMailAddressPlugin/apps/pc_frontend/modules/opAuthMailAddress/actions/actions.class.php
63:    $this->getUser()->setMemberId($memberConfig->getMemberId());

plugins/opAuthMailAddressPlugin/apps/mobile_frontend/modules/opAuthMailAddress/actions/actions.class.php
51:    $this->getUser()->setMemberId($memberConfig->getMemberId());

このような記述はありますが、これは $memberConfig = Doctrine::getTable('MemberConfig')->retrieveByNameAndValue('register_token', $token); から呼ばれているもの（つまり MemberConfig テーブルの memberId カラム値から取得しているもの）であり、セッションストレージに仮登録メンバーの member_id をセットする処理です。なお、

前コメントの (s3) の方針と相反するが、仮登録中に member_id を利用するすべての箇所で register token から得た member_id を利用するように書き換えることが容易ではないことがわかったため、仮登録中の処理を変更少なく正常に動作させるためにこの引数を導入する。

と示されている通り、セッションストレージに仮登録メンバーの member_id をセットする処理があることで、仮登録メンバーの member_id を取得することが容易になるため、この記述（セッションストレージへの member_id の保存）は残しておきます。

• opSecurityUser::getMember(true) をコールしている箇所について列挙します。

lib/model/doctrine/Gadget.class.php
73-
74-    $controller = sfContext::getInstance()->getController();
75-    if (!$controller->componentExists($this->getComponentModule(), $this->getComponentAction()))
76-    {
77-      return false;
78-    }
79-
80-    $member = sfContext::getInstance()->getUser()->getMember();
81-    if (!$member) // 状態[4]ログイン中としてのメンバー取得に失敗した場合に
82-    {
         // 状態[3]仮登録中としてのメンバーを取得している：今回の修正で動作が変わるので【要確認】
83:      $member = sfContext::getInstance()->getUser()->getMember(true); // ==== HERE ====
84-    }
85-
       // $member を条件に書く必要がなくなる：【改善可能】
86-    if (!$member || !$this->isAllowed($member, 'view'))
87-    {
88-      return false;
89-    }
90-
91-    return true;
92-  }
93-  // ここでは、ログイン中のメンバーのみ isAllowed() をチェックすればよいのではないか
     // $member = sfContext::getInstance()->getUser()->getMember();
     // return $this->isAllowed($member, 'view')

lib/user/opAuthAdapter.class.php
346-  }
347-
348-  public function isInvitedRegisterable()
349-  {
350-    // is this adapter allowed registration?
351-    if (!in_array($this->getAuthModeName(), sfContext::getInstance()->getUser()->getAuthModes()))
352-    {
353-      return false;
354-    }
355-
356:    $member = sfContext::getInstance()->getUser()->getMember(true); // ==== HERE ====
357-    if (!$member) // ここで false を返すケースとして何を想定しているのか【要確認】
358-    {
359-      return false; // ここは仮登録中のメンバーの取得に失敗した場合を想定したものと推測
360-    } // そうであれば以下 362 行目の if 文で return false されるので、ここを削除できる
361-
362-    if ($member->getConfig('is_admin_invited', false) && $this->getAuthModeName() !== $member->getConfig('register_auth_mode'))
363-    {
364-      return false;
365-    }
366-

lib/user/opSecurityUser.class.php
336-    return $this->isSNSMember();
337-  }
338-
339-  public function isSNSMember()
340-  {
341-    return ($this->getMember() && $this->getMember()->getIsActive());
342-  }
343-
344-  public function isInvited()
345-  {
346:    $member = $this->getMember(true); // ==== HERE ====
347-
        // 影響なし（ $member は常にオブジェクトなので条件に書く必要がなくなる：【改善可能】）
348-    return ($member && ($member->getConfig('is_admin_invited', false) || $member->getInviteMemberId()));
349-  }
350-
351-  public function isRegisterBegin()
352-  {
353-    return $this->getAuthAdapter()->isRegisterBegin($this->getMemberId());
354-  }
355-
356-  public function isRegisterFinish()

lib/action/opAuthAction.class.php
14- * @package    OpenPNE
15- * @subpackage action
16- * @author     Kousuke Ebihara <ebihara@tejimaya.com>
17- */
18-class opAuthAction extends sfActions
19-{
20-  public function executeRegisterEnd(sfWebRequest $request)
21-  {
22-    $this->forward404Unless($this->getUser()->setRegisterToken($request['token']));
23-
24:    $member = $this->getUser()->getMember(true); // ==== HERE ====
25-
26-    if (opConfig::get('retrieve_uid') == 3
27-      && !sfConfig::get('app_is_mobile', false)
         // getUser()->getMember(true) が常に getConfig をコールできることを想定している（影響なし）
28-      && !$member->getConfig('mobile_uid') 
29-    )
30-    {
31-      $this->forward('member', 'registerMobileToRegisterEnd');
32-    }
33-
34-    $this->getUser()->getAuthAdapter()->activate();

plugins/opAuthOpenIDPlugin/lib/opAuthAdapterOpenID.class.php
98-    {
99-      header('Location: '.$this->getAuthForm()->getRedirectUrl());
100-      exit;
101-    }
102-
103-    if ($this->getAuthForm()->isValid()
104-      && $this->getAuthForm()->getValue('openid')
105-      && !$result
106-      && $this->isRegisterable())
107-    {
108:      $member = $context->getUser()->getMember(true); // ==== HERE ====
          // 影響なし（ $member を条件に書く必要がなくなる：【改善可能】）
109-      if (!$member || !$member->getId())
110-      {
111-        $member = Doctrine::getTable('Member')->createPre();
112-        $member->generateRegisterToken();
113-      }
114-
115-      $member->setConfig('openid', $this->getAuthForm()->getValue('openid'));
116-      $this->appendMemberInformationFromProvider($member);
117-
118-      $member->save();

note-16 の修正案の表についてですが、

opSecurityMember 修正案

	getMemberId()	getMemberId(true)	getMember()	getMember(true)	備考
1	null	null	opAnonymousMember	opAnonymousMember
2	null	null	opAnonymousMember	opAnonymousMember	ログアウトさせる（1と同じ状態にする）
3	null	member_id	opAnonymousMember	Member
4	member_id	null	Member	opAnonymousMember
5	null	null	opAnonymousMember	opAnonymousMember	ログアウトさせる（1と同じ状態にする）

「opSecurityUser::getMember() が opAnonymousMember を返す場合には opSecurityUser::getMemberId() は null を返すようにする」という表に見えますが、これは適切ではありません。

この問題に関係なく、もともとの仕様で opAnonymousMember::getId() は 0 が返るようになっており、上記の getMemberId() の表の値 null は、「仮登録中やログイン停止中に、そのメンバーの ID が返らないようにする」ことを示したもので、 null を返すべきという意味のものではありませんでした。

つまり、適切な表は以下になります。

note-16 の方針に沿って修正を行いました。

28cda015 で、 opSecurityUser::getMember() と getMemberId() の仕様を変更しました

• これまで単に getMemberId() をコールしていれば、セッションストレージから member_id を取得しようとしていましたが、今回の修正で getMember($inactive) が Member オブジェクトを返さない場合には opAnonymousMember::getId() が返るようになりました。
• このため、 opSecurityUser::isRegisterBegin() では仮登録中に member_id が取得できなくなり、招待状のURLからプロフィール入力画面へ遷移できなくなるため、 opActivateBehavior を用いて member_id を取得するようにしておきました。
  • getMemberId(true) をコールしてもよいかもしれませんが、修正前のコードが「仮登録中の member_id 」を取得することを意識していないように見えたので、「仮登録中でなくログイン中であっても member_id が取得できる」ような形で修正しておきました。
• opSecurityUser::isRegisterFinish() がどのように使われているのか分からなかったので isRegisterBegin() にとりあえず合わせておきました（ isRegisterFinish() では仮登録中の member_id を取得することを想定しているのか）。

91afd1ff で、条件式で $member を用いている箇所を削除しました

• $member = opSecurityUser::getMember() では、今回の修正で常に Member オブジェクトか opAnonymousMember オブジェクトが取得できるので、オブジェクトが取得できているかを確認する必要がないためです。
• 今回修正した箇所は、もともと getMember(true) を用いていた周辺と、 opSecurityUser クラスのファイル全体です。それ以外の箇所については、この不要な条件式の削除は行っていません。
• ※もしプラグイン側で不要そうな if ($member) を削除する場合は注意が必要です。 if ($member) を削除するには、このチケットで扱った opSecurityUser::getMember() の仕様が変更されていることが前提となります。プラグインが、この仕様変更が入っていないバージョンの OpenPNE で使われる場合、 opSecurityUser::getMember() がオブジェクトを返さないことを考慮しないと問題が起こるかもしれません。

e7d09771 で、コメントの追加と、2 個目のコミットによって isSNSMember() の返り値の型が変わってしまった点を修正しました。

Methods¶

ところで、「ログイン中のメンバーであるかどうか」を判定するための方法として、以下の方法がありますが、

• if ($sf_user->isSNSMember())
• if ($sf_user->hasCredential('SNSMember'))
• if ($sf_user->isAuthenticated())

これら 3 種類について、使い分けを示しておきます（ $sf_user は opSecurityUser クラスのオブジェクトである前提です）。

if ($sf_user->isSNSMember())¶

• 現在の仕様では、内部では opSecurityUser::getMember()->getIsActive() の結果を返します。
• opActivateBehavior::disable(); の直後に opSecurityUser::getMember() がコールされた場合、「ログイン中」か「仮登録中」に Member オブジェクト（opAnonymousMember オブジェクトではない）が返る。この状態で本当に「ログイン中」かどうかを判定するために isActive の値をチェックしている。
• opSecurityUser::getMember() が返す Member または opAnonymousMember オブジェクトに対して、 isActive の値をチェックしている。
  • opAnonymousMember::getIsActive() は lib/user/opAnonymousMember.class.php で定義されていないので DB アクセスが発生しているかもしれない。
    • #548 で lib/user/opAnonymousMember.class.php が作成されているが、 opAnonymousMember として値が分かっている Member テーブルのカラム値があれば get**() を用意したほうが良いかもしれない。

if ($sf_user->hasCredential('SNSMember'))¶

• その時点のユーザのクレデンシャルが 'SNSMember' であるかどうかの結果を返します。
• ログイン時に SNSMember のクレデンシャルが付与されるようにはなっているが、OpenPNE-3.6.x では後方互換のためでありクレデンシャルは使わない方針にしていくので、これは 3.6.x 以降には使わないようにすることが好ましい。今後のクレデンシャルの扱いに関する詳細は、次の記事を参照してください。
  • https://groups.google.com/group/openpne-dev/msg/16ce15c694bbc531?hl=ja OpenPNE3 の master の認証プラグイン用の API を大幅に変更しました - openpne-dev | Google グループ
  • https://groups.google.com/group/openpne-dev/msg/6411294628277782?hl=ja 「 OpenPNE3 の master の認証プラグイン用の API を大幅に変更しました」についての詳細 - openpne-dev | Google グループ
    

    そもそもクレデンシャルは静的な権限を扱うものであって OpenPNE のように DB と紐付いた動的な権限の管理には向いていません。
    今回のようなバグを引き起こす危険性や、クレデンシャルの付与を誤った場合などのトラブルも考えられます。
    登録処理のような用途でクレデンシャルを使い続けるのは不適切であり誰もメリットを享受し得ないことから廃止し、
    トークンを引き回して認証を継続する形に置き換えることにしました。 
    

• 3.6.x での opSecurityUser::setIsSNSMember() メソッド
  

  374:  public function setIsSNSMember($isSNSMember)
  375-  {
  376-    $this->setAuthenticated($isSNSMember);
  377-
  378-    // for BC
  379-    if ($isSNSMember)
  380-    {
  381-      $this->addCredential('SNSMember');
  382-    }
  383-  }
  

if ($sf_user->isAuthenticated())¶

• その時点のユーザがログイン中であるかどうかの結果を返します。
  • 3.4.x 以前は isAuthenticated() が true になるロールとして「ログイン中」と「仮登録中」の 2 通りがあり、「ログイン中かどうか」の判定には hasCredentials('SNSMember') を用いていた。
  • 3.6.x 以降では「仮登録中」であるかどうかを別の方法で扱うようにしたため、クレデンシャルは情報として重要ではなくなり、 isAuthenticated() が true であれば「ログイン中」であることが保証されるようになった。
• opSecurityUser::initialize() の中で opSecurityUser::initializeUserStatus() がコールされており、そこでは opSecurityUser::isSNSMember() と同等の処理を行い、その結果を $this->setIsSNSMember() で保持している（上記のメソッド参照）。
  • 3.6.x での opSecurityUser::initializeUserStatus() メソッド（旧 opSecurityUser::initializeCredentials() メソッド）
    

    313- /**
    314-  * Initializes all credentials associated and status with this user.
    315-  */
    316:  public function initializeUserStatus()
    317-  {
    318-    opActivateBehavior::disable();
    319-    $member = $this->getMember();
    320-    opActivateBehavior::enable();
    321-
    322-    if ($member instanceof opAnonymousMember || $member->getIsLoginRejected())
    323-    {
    324-      $this->logout();
    325-      $isSNSMember = false;
    326-    }
    327-    else
    328-    {
    329-      $isSNSMember = (bool)$member->getIsActive();
    330-    }
    331-
    332-    $this->setIsSNSMember($isSNSMember);
    333-    if ($isSNSMember)
    334-    {
    335-      $member->updateLastLoginTime();
    336-    }
    337-  }
    

note-21 を見直しましたが、 91afd1ff のコミットで、 (#1009) 8a8b44f4 を打ち消してしまっている可能性がありますが、(#1009) 8a8b44f4 で何を修正したのかを的確に認識できていません。

(#1009) 8a8b44f4 ではガジェットについて修正していますが、それについてここで少しまとめます。

• #1009 の修正前の動作（推測）

  	Web全体に公開	SNS全員に公開
  非ログイン	表示	非表示
  仮登録中	非表示	非表示(※)
  ログイン中	表示	表示

• #1009 の修正後の動作（推測）

  	Web全体に公開	SNS全員に公開
  非ログイン	表示	非表示
  仮登録中	表示	非表示(※)
  ログイン中	表示	表示

もしこの動作のために (#1009) 8a8b44f4 のコミットがあるのであれば、今回の仕様変更 28cda015 と不要な条件式の削除 91afd1ff を行った後でも想定する動作となっているはずなので、打ち消してしまっているように見える点は問題ないだろうということをコメントしておきます。

※ #2285 のチケットがあるように、非表示であるべき箇所でガジェットが表示されている問題があったようです。しかしこれは、このコメントで記述している内容を基に、このチケットで修正しています。

補足¶

OpenPNE では、あるアクター（ユーザ）が OpenPNE 上でどのようなロール（メンバー）なのかを判断するため、しばしば次のような判定をしていました。

$sf_user;                        // $sf_user = sfContext::getInstance()->getUser();
                                 // $sf_user instanceof opSecurityUser
$member = $sf_user->getMember(); // $member instanceof Member || $member instanceof opAnonymousMember || false

(1.) if ($sf_user->getMemberId())
(2.) if ($sf_user->isMember())
(3.) if ($sf_user->isSNSMember())
(4.) if ($sf_user->hasCredential('SNSMember'))
(5.) if ($sf_user->isAuthenticated())
(6.) if ($member && $member->getId())
(7.) if ($member && $member->getIsActive())
(8.) if ($member && !($member instanceof opAnonymousMember))
...

こうした判定は、箇所によっては意図した動作に反する結果が生じる可能性がありました。少なくとも上記のそれぞれで結果がどのように異なるのか区別できていない場合、例えば仮登録中のメンバーがその箇所でどのようなロールとして判定されるのか（ if 文を通ってしまうのか）について考慮されていない可能性があります。

このチケットの修正前は $sf_user->getMember() が Member を返すのか opAnonymousMember を返すのか false を返すのかがよく分からない状況であり、インスタンスであることを保証（if ($member) の条件判定）した上で getId() するなどして「ログイン中のメンバー」であると判定したりしていました。

しかし、修正前の $sf_user->getMember()->getId() と $sf_user->getMemberId() では結果が異なり、「ログイン中のメンバー」に限って許可したい処理が、ログイン中以外のロールであっても通ってしまう問題が各所で生じていました。

この修正前および修正後における「ログイン中メンバーであるか」の判定として適切と思われるものを示します：

• 本チケットでの修正前
  

  ==== $sf_user を基に判定する場合：
  // ログイン中メンバーであることが保証される（ログイン停止中の可能性がある）
  (2.) if ($sf_user->isMember())                 // isSNSMember() へのエイリアス
  (3.) if ($sf_user->isSNSMember())              // 推奨（Member レコードから判定）
  (4.) if ($sf_user->hasCredential('SNSMember')) // 3.4.x までは推奨（ログイン状態から判定）
  (5.) if ($sf_user->isAuthenticated())          // 3.4.x では仮登録中のメンバーも含まれてしまう
  
  // 仮登録中のメンバーであっても条件式が真になってしまうので、これ単体では用いてはならない
  (1.) if ($sf_user->getMemberId())
  
  ==== $member = $sf_user->getMember() を基に判定する場合：
  // ログイン中メンバーであることが保証される（ログイン停止中の可能性がある）
  (7.) if ($member && $member->getIsActive())
  
  // opActivateBehavior::disable();
  // $member = $sf_user->getMember();
  // opActivateBehavior::enable();
  // このように opActivateBehavior が disabled の場合は、仮登録中のメンバーであっても条件式が真になってしまう
  (6.) if ($member && $member->getId())
  (8.) if ($member && !($member instanceof opAnonymousMember))
  
  

• 本チケットでの修正後
  

  ==== $sf_user を基に判定する場合：
  // ログイン中メンバーであることが保証される
  (2.) if ($sf_user->isMember())        // isSNSMember() へのエイリアス
  (3.) if ($sf_user->isSNSMember())     // 推奨（Member レコードから判定）
  (5.) if ($sf_user->isAuthenticated()) // 推奨（ログイン状態から判定）
  
  // opActivateBehavior::disable();
  // $memberId = $sf_user->getMemberId();
  // opActivateBehavior::enable();
  // このように opActivateBehavior が disabled の場合は、仮登録中のメンバーであっても条件式が真になってしまう
  (1.) if ($sf_user->getMemberId())
  
  // OpenPNE-3.4.x 以前では、クレデンシャルを使って「仮登録中」と「ログイン中」のロールを区別していた
  // OpenPNE-3.6.x 以降では、仮登録処理でクレデンシャルを使わずに「仮登録中」かどうかを扱えるようにした
  // このため、クレデンシャルの情報は 3.6.x 以降では使う必要のないものとなる
  // hasCredential('SNSMember') は isAuthenticated() に代わることとなる
  
  // 非推奨だが、for BC (Backward Compatibility, 後方互換性) が考慮されているため、これでも「ログイン中」と判断できる
  (4.) if ($sf_user->hasCredential('SNSMember')) // 非推奨
  
  ※プラグインやコアの古い部分では hasCredential('SNSMember') を用いて「ログイン中」であるかを判定している
  ※そのような箇所は isAuthenticated() か isSNSMember() に書き換えていくことが好ましい
  ※コア側は、（古いコードで）クレデンシャルのチェックを行っている箇所がある以上、後方互換のために
    これまでクレデンシャルを付与していた部分についてはそのままにしておく方針となっている
  
  // isAuthenticated() が true になるのはログイン中であり、ログイン中であれば isSNSMember() は true になる
  // isSNSMember() の定義を（カスタマイズなどで）変更しないかぎり、 isAuthenticated() と isSNSMember() は交換可能
  // 詳細は http://redmine.openpne.jp/issues/1944#Methods を参照
  
  ==== $member = $sf_user->getMember() を基に判定する場合：
  // ログイン中メンバーであることが保証される
  (7.) if ($member->getIsActive())
  
  // opActivateBehavior::disable();
  // $member = $sf_user->getMember();
  // opActivateBehavior::enable();
  // このように opActivateBehavior が disabled の場合は、仮登録中のメンバーであっても条件式が真になってしまう
  (6.) if ($member->getId())
  (8.) if (!($member instanceof opAnonymousMember))
  
  opActivateBehavior が有効である前提であれば、(6.),(7.),(8.) はどれでも期待する結果となる。
  (6.),(7.),(8.) のどれが適切かは判断できない（ログイン中メンバーであるかどうかを判定する用のメソッドがない）。
  

$sf_user (opSecurityUser クラス) には opSecurityUser::isSNSMember() メソッドが用意されていますが、 $member (Member クラス)　には Member::isSNSMember() メソッドが用意されていません。現状は、結果さえ意図しているものになることが期待されれば、上記 (6.),(7.),(8.) のどれが使われても良いようになっています。

今回の仕様変更で「 opSecurityUser::getMember() が必ずオブジェクト（Member かそれを継承した opAnonymousMember）を返すようになった」ことを保証することにするのであれば、ついでに「ログイン中メンバーであるかどうかを判定する用の Member クラスのメソッド Member::isSNSMember() 」を追加して、それを使って判定できるようにした方がよいかもしれません（もし対応するとしても、別チケットにすべきかもしれません）。

ところで、 opSecurityUser::isSNSMember() の実装についてですが、 メソッドの使い分け でも示しましたが、現在以下のように実装されています。

  public function isSNSMember()
  {
    return (bool)$this->getMember()->getIsActive();
  }

しかしながら、「ログイン中かどうか」の判定は常に isSNSMember() で行うようにし、次のような実装をしておくことも可能かもしれません。

lib/user/opSecurityUser.class.php

  public function isSNSMember()
  {
    return $this->isAuthenticated();
  }

lib/model/doctrine/Member.class.php 

  public function isSNSMember()
  {
    return (bool)$this->getIsActive();

    // 以下は sfContext::getInstance()->getUser() で opSecurityUser が得られる箇所で、
    // かつ Member がアクターと一致している場合しか使えませんね（ Member から opSecurityUser を逆引きできない）
    // return sfContext::getInstance()->getUser()->isSNSMember();
  }

ちなみに、「 opSecurityUser::getMember() が必ずオブジェクト（Member かそれを継承した opAnonymousMember）を返すようにした」という仕様変更は、（コンテンツの外部公開機能を追加した）OpenPNE-3.6.x 以上で opAnonymousMember クラスが用意されていることを前提に、これを利用して Member オブジェクトに対して NullObject パターンを実現させてしまおうという発想に基づいています。

つまり、 opAnonymousMember クラスが存在しない OpenPNE-3.4.x 以前については、 opSecurityUser::getMember() が Member オブジェクトを返さない場合に false あるいは null を返す仕様は、これまでがそうであったように自然なものと言えます（このチケットでの改善内容を 3.4.x 向けにバックポートすることは難しいかもしれません）。

(P3) について¶

note-3 で指摘されており、 note-12 で

• (P3) 仮登録状態のメンバーに対してフレンド申請を行うと表示されるべきでないエラーが吐かれる

と示した問題：

note-3

追加テストメモ¶

（違う問題かもしれませんが手順が類似しているため、あえて同じチケットに報告しています）
仮登録状態のメンバーにフレンド申請をすると、下記のエラーが発生します。
アクセス出来ませんなどのエラーを表示するのが望ましいかと思います。

[...]

再現手順¶

1. 管理画面のメンバーリストを確認し一番最後のメンバーIDを確認する（メンバーID：56とする）
2. 管理画面からメンバー A 宛に招待メールを送信する
3. メンバー A がその招待メールに記述された URL にアクセスする
4. 別のブラウザを開き登録済みのユーザーでメンバーID：57にフレンド申請をする
   （一番最後に登録されたメンバーIDが56だった場合、次に登録されるIDは57になるはずなので）

アクセスしたURL↓
http://36x.bughunt.uh-openpne5.pne.jp/pc_frontend_dev.php/friend/link/id/57

について調査しました。master ブランチでは生じず stable-3.6.x では生じる問題であり、コードを追ったところ、 #368 で示された問題そのものでした。これは master ブランチでしか対応していませんでしたが #2190 でバックポートチケットが作られており、対応予定となっています。

つまり、このチケットで扱った問題は全て解消される予定ということになります。

レビュワーの方へ：

• このチケットでは、getMember() などの仕様を note-19 の表の通りに変更する修正を行いました。
  • note-21 で修正しています。
• note-16 で 2011 年 8 月ごろに考察した最新の内容を示しています。
  • note-15 以前は 5 月ごろの考察内容であり note-16 以降とは異なる見解を示しています。