Project

General

Profile

Bug(バグ) #1182

携帯でSNS強制退会直後にアクセスするとCredentials Required画面が表示される

Added by Kiwa Sakai over 9 years ago. Updated about 4 years ago.

Status:
Fixed(完了)
Priority:
High(高め)
Target version:
Start date:
2010-06-18
Due date:
% Done:

100%

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

Description

携帯端末でログインしたユーザがいる状態で
管理画面からそのユーザを強制退会処理をした後、
ユーザが認証が必要なページにアクセスするとCredentials Required画面が表示される

発生確認バージョン

確認端末

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 アクションを追加した

ticket-1182.patch View (1.78 KB) Rimpei Ogawa, 2011-08-04 21:05


Related issues

Related to OpenPNE 3 - Backport(バックポート) #1229: 携帯でSNS退会直後にアクセスするとCredentials Required画面が表示される Fixed(完了) 2010-06-18 2011-10-06
Related to OpenPNE 3 - Backport(バックポート) #1230: 携帯でSNS退会直後にアクセスするとCredentials Required画面が表示される Works for me(再現せず) 2010-06-18
Related to OpenPNE 3 - Backport(バックポート) #2315: 携帯でSNS強制退会直後にアクセスするとCredentials Required画面が表示される Fixed(完了) 2011-07-29
Related to OpenPNE 3 - Bug(バグ) #1944: 仮登録状態でメンバープロフィールページにアクセスするとFatal Error Fixed(完了) 2011-03-07
Related to OpenPNE 3 - Bug(バグ) #1836: 管理画面でログイン停止にしてもログイン可能 Fixed(完了) 2010-12-07
Related to OpenPNE 3 - Bug(バグ) #1985: Cookie for automatic login is deleted when automatic login is done (自動ログイン時に自動ログイン用のCookieが削除される) Fixed(完了) 2011-03-29

Associated revisions

Revision 982b00d8 (diff)
Added by Shinichi Urabe about 8 years ago

(refs #1182) set authenticated false.

Revision 96765c9b (diff)
Added by Shinichi Urabe about 8 years ago

(refs #1182) I add an action "default/secure" same as a PC for mobile.

Revision ef7d864d (diff)
Added by Shinichi Urabe about 8 years ago

fixed for coding standard (fixes #1182)

Revision decc4e2d (diff)
Added by Shinichi Urabe about 8 years ago

(refs #1182) I made modifications to consider "forced withdrawal and login stop user" which was in a state to be a logout state.

Revision 7cde87c9 (diff)
Added by Shinichi Urabe about 8 years ago

(refs #1182) "opSecurityUser::initializeCredentials()" changed its name to "opSecurityUser::initializeUserStatus()".

History

#1 Updated by Kiwa Sakai over 9 years ago

  • Subject changed from 携帯でSNS退会直後にアクセスするとCredentials Required画面に遷移する to 携帯でSNS退会直後にアクセスするとCredentials Required画面が表示される

#2 Updated by Yuki Yamashita over 9 years ago

  • Target version set to OpenPNE 3.6beta1

#3 Updated by Kousuke Ebihara over 9 years ago

  • Status changed from New(新規) to Accepted(着手)
  • Assignee set to Kousuke Ebihara

#4 Updated by Kousuke Ebihara over 9 years ago

  • Status changed from Accepted(着手) to New(新規)
  • Assignee deleted (Kousuke Ebihara)
  • Target version deleted (OpenPNE 3.6beta1)

現時点の master (bfc73921817b0e82db93 時点) で再現することができませんでした。

#5 Updated by Rimpei Ogawa about 9 years ago

  • 3.6 で発生するか set to Yes

#6 Updated by Shinichi Urabe over 8 years ago

  • Subject changed from 携帯でSNS退会直後にアクセスするとCredentials Required画面が表示される to 携帯でSNS強制退会直後にアクセスするとCredentials Required画面が表示される

#7 Updated by Shinichi Urabe over 8 years ago

  • Target version set to OpenPNE3.6beta11

#8 Updated by Shinichi Urabe over 8 years ago

  • Target version changed from OpenPNE3.6beta11 to OpenPNE 3.7.0

#9 Updated by Shinichi Urabe over 8 years ago

ユーザの操作で、退会処理を実施した場合は 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) を呼ぶのが適切ではないかと考える。

#10 Updated by Shinichi Urabe over 8 years ago

  • Assignee set to Shinichi Urabe

#11 Updated by Shinichi Urabe over 8 years ago

  • Status changed from New(新規) to Accepted(着手)

#12 Updated by Shingo Yamada about 8 years ago

  • Priority changed from Normal(通常) to High(高め)

#13 Updated by Shinichi Urabe about 8 years ago

動作確認した内容

内容 期待値 結果
ユーザ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

#14 Updated by Shinichi Urabe about 8 years ago

  • Status changed from Accepted(着手) to Pending Review(レビュー待ち)
  • % Done changed from 0 to 50

#15 Updated by Shinichi Urabe about 8 years ago

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

#16 Updated by Rimpei Ogawa about 8 years ago

  • File ticket-1182.patch View added
  • Status changed from Pending Review(レビュー待ち) to 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 のログイン停止中の問題も同時に扱ったほうがよいと思います(テスト項目の追加などが必要になります)。

#17 Updated by Shinichi Urabe about 8 years ago

(メモ)

96765c9b のBC用の分岐の default/secure の手前のアクションのクレデンシャルを確認するとき、forward 処理でクレデンシャルの複数指定で OR で指定がされている場合、配列の結果を単純に in_array() だけで妥当性を確認してもいいのか調査する

index:
  credentials: [[SNSMember, A, B]]
index2:
  credentials: [SNSMember, A, B]
index3:
  credentials: [[SNSMember, A, B], C]

#18 Updated by Shingo Yamada about 8 years ago

  • 360対象 set to beta13

#19 Updated by Shinichi Urabe about 8 years ago

  • Status changed from Rejected(差し戻し) to Pending Review(レビュー待ち)

修正内容

  • http://redmine.openpne.jp/issues/1182#note-16 での指摘でのバッチを適用しました。
  • opSecurityUser::initializeCredentials() を抽象的な名称 opSecurityUser::initializeUserStatus() に変更しました。

Member::isOnBlacklist() について

このバッチ適用時の確認として、Member::isOnBlacklist() ブラックリストについては旧 opSecurityUser::initializeCredentials() では対処されておらず、ログイン時のみログインさせない処理となっていることに問題がないか判断ができませんでした。

ブラックリストの機能の意味を考える必要があり、あるべき姿によっては修正が必要と考えます。(別チケット扱いでよいと考えます)

  1. (携帯、PC) のログイン処理時のみ、ログインさせない機能としての位置づけ「現状の状態」
  2. (携帯) のログイン処理時のみ、ログインさせない機能としての位置づけ
  3. (携帯、PC) のログインしているユーザに対しても、ログインさせない機能としての位置づけ
  4. (携帯) のログインしているユーザに対しても、ログインさせない機能としての位置づけ

opAnonymousMember::getId() の返り値について

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 されることは考えなくてもよいと考える

#20 Updated by Minoru Takai about 8 years ago

opAnonymousMember::getId() の返り値について

note-19 のこの内容について確認しましたが、この指摘は「opSecurityUser::getMemberId() が null を返す仕様である」場合に「opAnonymousMember::getId() が 0 を返すのは、前述の仕様と異なり不自然なので opAnonymousMember::getId() で null を返すようにすべき」という内容とのことでした。

しかしながら、この指摘は前提が誤っており( #1944 での書き方が不適切で、 1944-note-19 で補足しました)、 #1944 では opSecurityUser::getMemberId() が opAnonymousMember::getId() を返すように修正する方針となっているので、この指摘は気にしないでよいものであるということを示しておきます。

#21 Updated by Minoru Takai about 8 years ago

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

レビューしました。 OK です。

  • このチケットに紐付くコミットが master ブランチに取り込まれていることを確認しました。
  • 前半のコミット 982b00d8, 96765c9b, ef7d864d
    • 携帯版にも default/secure のページを用意するという修正ですが、修正内容は適切だと思います。
  • 後半のコミット decc4e2d, 7cde87c9
    • 強制退会させられたり、ログイン停止中にされたときに、「そのメンバーがログアウトしない間は操作ができていた」という問題を修正できていることを確認しました。条件式も適切だと思います。

note-19 で、ブラックリストの仕様について、ブラックリストに登録されたときには、「そのメンバーがログアウトしない間は操作ができない」という問題が残ったままになっていることが指摘されています。関連する話であるため併せて対応することも考えられますが、「ブラックリストの仕様が明確でないこと」と「ブラックリストの件を併せて修正することが必須ではないこと」を理由に、このチケットでは扱わなくてよいと判断します。これについては別チケットで対応すればよいと思います。

ログイン中に強制退会させられたメンバーが、そのまま操作を続けた場合に Credentials Required 画面が表示されてしまう問題を解消し、 #1944 の修正と並行して、ログイン停止中にされた場合も含めて、続けて操作ができないようにログアウト処理を追加する修正を行うチケットとして扱うことにします。

#22 Updated by Minoru Takai about 8 years ago

  • Description updated (diff)

#23 Updated by Yuma Sakata almost 8 years ago

  • Status changed from Pending Testing(テスト待ち) to Fixed(完了)
  • % Done changed from 70 to 100

テストOKです。

#24 Updated by kaoru n about 4 years ago

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

Also available in: Atom PDF