Bug（バグ） #1182
完了
携帯でSNS強制退会直後にアクセスするとCredentials Required画面が表示される
100%
説明
携帯端末でログインしたユーザがいる状態で
管理画面からそのユーザを強制退会処理をした後、
ユーザが認証が必要なページにアクセスするとCredentials Required画面が表示される
発生確認バージョン¶
- OpenPNE3.7 (371b27cb02df1a251dda0fcbf1646a388cdd0af3 時点)
- OpenPNE3.6beta10
確認端末¶
au W61CA
モバイルシミュレータ(930SH → Softbank 、P903i → Docomo)
修正方針¶
- opSecurityUser::setIsSNSMember(false) が呼ばれた際に opSecurityUser::setAuthenticated(false) を実行するように対処「その際 SNSMember credentials なども削除される」
- note-16 で示しているが、 setAuthenticated(false) だけではなく logout() が呼ばれるように修正
- モバイルには OpenPNE側の default/secure アクションがないため、PCとあわせ default/secure アクションを追加した
ファイル
Kiwa Sakai さんが14年以上前に更新
- 題名 を 携帯でSNS退会直後にアクセスするとCredentials Required画面に遷移する から 携帯でSNS退会直後にアクセスするとCredentials Required画面が表示される に変更
Kousuke Ebihara さんが14年以上前に更新
- ステータス を New（新規） から Accepted（着手） に変更
- 担当者 を Kousuke Ebihara にセット
Kousuke Ebihara さんが14年以上前に更新
- ステータス を Accepted（着手） から New（新規） に変更
- 担当者 を削除 (
Kousuke Ebihara) - 対象バージョン を削除 (
OpenPNE 3.6beta1)
現時点の master (bfc73921817b0e82db93 時点) で再現することができませんでした。
Shinichi Urabe さんが13年以上前に更新
- 題名 を 携帯でSNS退会直後にアクセスするとCredentials Required画面が表示される から 携帯でSNS強制退会直後にアクセスするとCredentials Required画面が表示される に変更
Shinichi Urabe さんが13年以上前に更新
ユーザの操作で、退会処理を実施した場合は opMemberAction::executeDelete() 内で opSecurityUser::logout() が呼ばれ、そのなかで、opSecurityUser::setAuthenticated(false) が呼ばれるので、未認証状態となり、認証が必要なページにアクセスした場合も、sfBasicSecurityFilter::execute() でログインアクションに飛ばされる。
管理画面からの強制退会を実施した場合、認証セッションが残っているユーザが認証が必要なアクションにアクセスすると opSecurityUser::initializeCredentials() 内で opSecurityUser::setIsSNSMember(false) が呼ばれるものの、SNSMember クレデンシャルが削除されるのみで、認証セッションは維持されている。
そのため sfBasicSecurityFilter::execute() では defaut/secure アクションに飛ばされる (モバイルの場合、OpenPNE側でこのアクションは用意されていないのでsymfonyのデフォルトのページが表示される)
OpenPNE3.6 以降の場合は opSecurityUser::setIsSNSMember(false) がよばれた場合、SNSMember クレデンシャルを削除するのではなく、opSecurityUser::setAuthenticated(false) を呼ぶのが適切ではないかと考える。
Shinichi Urabe さんが13年以上前に更新
動作確認した内容
| 内容 | 期待値 | 結果 |
| ユーザAがPCでログインしている状態で、管理画面からユーザAを強制退会 | 強制退会処理後ユーザAがPC画面を更新すると、ログイン画面に遷移 | OK |
| ユーザBが携帯でログインしている状態で、管理画面からユーザBを強制退会 | 強制退会処理後ユーザBが携帯画面を更新すると、ログイン画面に遷移 | OK |
| プラグインで is_secure:true credentials:tetete の'tetete/tetete'アクションを作り、ユーザCがPCで tetete credentials を持っていない状態で 'tetete/tetete'アクションにアクセス | [このページにはアクセスできません。]と表示される | OK |
| プラグインで is_secure:true credentials:tetete の'tetete/tetete'アクションを作り、ユーザCがPCで tetete credentials が付与された状態で 'tetete/tetete'アクションにアクセス | コンテンツが表示される | OK |
| プラグインで is_secure:true credentials:tetete の'tetete/tetete'アクションを作り、ユーザCが携帯で tetete credentials を持っていない状態で 'tetete/tetete'アクションにアクセス | [このﾍﾟｰｼﾞにはｱｸｾｽできません｡]と表示される | OK |
| プラグインで is_secure:true credentials:tetete の'tetete/tetete'アクションを作り、ユーザCが携帯で tetete credentials が付与された状態で 'tetete/tetete'アクションにアクセス | コンテンツが表示される | OK |
Shinichi Urabe さんが13年以上前に更新
- ステータス を Accepted（着手） から Pending Review（レビュー待ち） に変更
- 進捗率 を 0 から 50 に変更
Rimpei Ogawa さんが13年以上前に更新
- ファイル ticket-1182.patch ticket-1182.patch を追加
- ステータス を Pending Review（レビュー待ち） から Rejected（差し戻し） に変更
修正内容について、
OpenPNE3.6 以降の場合は opSecurityUser::setIsSNSMember(false) がよばれた場合、SNSMember クレデンシャルを削除するのではなく、opSecurityUser::setAuthenticated(false) を呼ぶのが適切ではないかと考える。
クレデンシャルの削除だけよりも、 sfBasicSecurityUser::setAuthenticated(false) を呼ぶ方が適切であることは間違いないですが、もう一歩踏み込んで opSecurityUser::logout() を呼ぶようにするのがより適切な修正方法だと思います。opSecurityUser::logout() には OpenPNE で拡張している Remember Login Cookie の削除や member_id を保存している opSecurityUser namespace のセッションデータの削除の処理が含まれているため、こちらを呼ぶことでより確実に非ログイン状態と同じ状態にすることができます。
ただし、単純に setIsSNSMember() 内で logout() してしまうと仮登録中のメンバーにも影響が出てしまうため、 initializeCredentials() の処理を書き換える必要があります。
opSecurityUser の具体的な修正案をパッチにしたのでこのコメントに添付します。
修正の差分を小さくするためにメソッド名は既存のものをそのまま使っていますが、initializeCredentials() はクレデンシャルの初期化処理以外の役割も担っているため（元からそうでしたが）、メソッド名を変更したほうがよいかもしれません。（そもそも SNSMember クレデンシャルを付与すること自体が OpenPNE 3.6 以降は for BC 扱いになっています）
また、関連チケットのコメント http://redmine.openpne.jp/issues/1944#note-16 で本チケットでの修正内容について触れていますが、
session member member.is_active member.is_login_rejected 備考 1 × - - - 非ログイン状態 2 ○ × - - 退会済み、招待からの仮登録中に招待削除された 3 ○ ○ 0 0 仮登録中 4 ○ ○ 1 0 ログイン中 5 ○ ○ 1 1 ログイン停止中 （中略）
- 2, 5 の場合については、 opSecurityUser のインスタンス初期化時にログアウト処理を行うようにすれば 1 と同じ状態にすることが可能である。
- この対応については、 #1182 にて先に同様の対応がなされているため本チケットでは扱わないこととする。
この表のうち、本チケットで元々対象としている「SNS強制退会直後」の状態は 2 に該当します。
しかし、実際に 982b00d8 で修正されている箇所は 5 の場合についても実行される箇所ですので、本チケットで 5 のログイン停止中の問題も同時に扱ったほうがよいと思います（テスト項目の追加などが必要になります）。
Shinichi Urabe さんが13年以上前に更新
（メモ）
96765c9b のBC用の分岐の default/secure の手前のアクションのクレデンシャルを確認するとき、forward 処理でクレデンシャルの複数指定で OR で指定がされている場合、配列の結果を単純に in_array() だけで妥当性を確認してもいいのか調査する
例
index: credentials: [[SNSMember, A, B]] index2: credentials: [SNSMember, A, B] index3: credentials: [[SNSMember, A, B], C]
Shinichi Urabe さんが13年以上前に更新
- ステータス を Rejected（差し戻し） から Pending Review（レビュー待ち） に変更
修正内容¶
- http://redmine.openpne.jp/issues/1182#note-16 での指摘でのバッチを適用しました。
- opSecurityUser::initializeCredentials() を抽象的な名称 opSecurityUser::initializeUserStatus() に変更しました。
Member::isOnBlacklist() について¶
このバッチ適用時の確認として、Member::isOnBlacklist() ブラックリストについては旧 opSecurityUser::initializeCredentials() では対処されておらず、ログイン時のみログインさせない処理となっていることに問題がないか判断ができませんでした。
ブラックリストの機能の意味を考える必要があり、あるべき姿によっては修正が必要と考えます。（別チケット扱いでよいと考えます）
- (携帯、PC) のログイン処理時のみ、ログインさせない機能としての位置づけ「現状の状態」
- (携帯) のログイン処理時のみ、ログインさせない機能としての位置づけ
- (携帯、PC) のログインしているユーザに対しても、ログインさせない機能としての位置づけ
- (携帯) のログインしているユーザに対しても、ログインさせない機能としての位置づけ
opAnonymousMember::getId() の返り値について¶
- http://redmine.openpne.jp/issues/1944#note-16 で扱う範囲になると思われますが、opAnonymousMember::getId() が 0 を返しているので NULL を返す必要があると思います
default/secure のログインページへの forward 処理について¶
- http://redmine.openpne.jp/issues/1182#note-17 について クレデンシャルの指定を AND と OR の組み合わせで [A, B, [SNSMember, C], D] とした場合に SNSMember と C のみのクレデンシャルがない場合、ログインページではなく、default/secure アクションのページが表示されたままとなる。しかしながら、setIsSNSMember(true) のときは SNSMember クレデンシャルが for BC として付与されるため、また false の場合はそもそも認証状態から外れるため、SNSMember クレデンシャルがない状態で default/secure に forward されることは考えなくてもよいと考える
Minoru Takai さんが13年以上前に更新
opAnonymousMember::getId() の返り値について¶
- http://redmine.openpne.jp/issues/1944#note-16 で扱う範囲になると思われますが、opAnonymousMember::getId() が 0 を返しているので NULL を返す必要があると思います
note-19 のこの内容について確認しましたが、この指摘は「opSecurityUser::getMemberId() が null を返す仕様である」場合に「opAnonymousMember::getId() が 0 を返すのは、前述の仕様と異なり不自然なので opAnonymousMember::getId() で null を返すようにすべき」という内容とのことでした。
しかしながら、この指摘は前提が誤っており（ #1944 での書き方が不適切で、 1944-note-19 で補足しました）、 #1944 では opSecurityUser::getMemberId() が opAnonymousMember::getId() を返すように修正する方針となっているので、この指摘は気にしないでよいものであるということを示しておきます。
Minoru Takai さんが13年以上前に更新
- ステータス を Pending Review（レビュー待ち） から Pending Testing（テスト待ち） に変更
- 進捗率 を 50 から 70 に変更
レビューしました。 OK です。
- このチケットに紐付くコミットが master ブランチに取り込まれていることを確認しました。
- 前半のコミット 982b00d8, 96765c9b, ef7d864d
- 携帯版にも default/secure のページを用意するという修正ですが、修正内容は適切だと思います。
- 後半のコミット decc4e2d, 7cde87c9
- 強制退会させられたり、ログイン停止中にされたときに、「そのメンバーがログアウトしない間は操作ができていた」という問題を修正できていることを確認しました。条件式も適切だと思います。
note-19 で、ブラックリストの仕様について、ブラックリストに登録されたときには、「そのメンバーがログアウトしない間は操作ができない」という問題が残ったままになっていることが指摘されています。関連する話であるため併せて対応することも考えられますが、「ブラックリストの仕様が明確でないこと」と「ブラックリストの件を併せて修正することが必須ではないこと」を理由に、このチケットでは扱わなくてよいと判断します。これについては別チケットで対応すればよいと思います。
ログイン中に強制退会させられたメンバーが、そのまま操作を続けた場合に Credentials Required 画面が表示されてしまう問題を解消し、 #1944 の修正と並行して、ログイン停止中にされた場合も含めて、続けて操作ができないようにログアウト処理を追加する修正を行うチケットとして扱うことにします。
Yuma Sakata さんが約13年前に更新
- ステータス を Pending Testing（テスト待ち） から Fixed（完了） に変更
- 進捗率 を 70 から 100 に変更
テストOKです。
