BooksControllerでのログイン有無チェックを修正 #5921

peno022 · 2022-12-10T03:59:14Z

Issue

概要

booksのページでエラーが起きているのを修正する #5788 について

http://localhost:3000/books はログインしないと閲覧できないページですが、 BooksControllerではログインの確認がされていなかったため、ログインせずにアクセスするとcurrent_userがnilの状態でviewが呼び出され、 /app/app/views/books/index.html.slimでNoMethodError: undefined method admin_or_mentor? for nil:NilClassのエラーが発生していました。
同件のバグを防ぐことも考慮し、app/controllers/application_controller.rbにbefore_action :require_loginを追加して、ログインが不要なページのみrequire_loginをskipする方針で修正しました。

ログイン必須の指定が多すぎる #5984 について

#5788 の対応で行った

app/controllers/application_controller.rbにbefore_action :require_loginを追加して、ログインが不要なページのみrequire_loginをskipする方針で修正

の修正について、本PRで対応することは変わらないですが、別issueとして切り出していただいています。

ログインせずにユーザーのポートフォリオページにアクセスすると500エラー #5992 について

修正を進める中で、ポートフォリオページでもbooksページと同様のバグが発生しているのを見つけたので、あわせて修正しました。ログイン必須ページとなるように修正しています。

修正対象のファイルが多いですが、バグとなっていたapp/controllers/books_controller.rb、app/controllers/users/works_controller.rb以外は、仕様が変わらないよう機械的な修正になっています。

app/controllers/books_controller.rbについては、2点見直し
- http://localhost:3000/books アクセス時にログイン済みかどうかチェックがされるようにすること
- deleteアクションもadminまたはmentor権限が必要なはずなので、before_action: require_admin_or_mentor_loginの対象にすること
app/controllers/users/works_controller.rbについては、1点見直し
- http://localhost:3000//users/{user.id}/portfolio アクセス時にログイン済みかどうかチェックがされるようにすること
それ以外のファイルについては、現状のログインチェック内容から変わらないように、下記の手順に従って機械的に修正したものになるので、手順が正しいかどうか→手順に合わない修正が含まれていないかどうか、という観点で見ていただければと思います。

修正した手順

app/controllers/application_controller.rbにbefore_action :require_loginを追記
< ApplicationControllerを含む全ファイルを検索（75ファイル）
対象の75ファイルのコントローラに対して、before_action :require_loginがあるものは削除
ないもののうち、他の権限でのログイン確認も含まれていないものにはskip_before_action :require_login, raise: falseを追加（対象アクションが絞られているものはそれに従う）

変更確認方法

bug/nomethoderror-on-books-without-loginをローカルに取り込む。
ログインしていない状態で http://localhost:3000/books にアクセスする。
下記2点が確認できればOK。

「500: Internal Server Error」のエラーが出なくなっていること
「ログインしてください」のメッセージとともにトップページに遷移すること

Screenshot

ログインしていない状態で http://localhost:3000/books にアクセスした時：

変更前

変更後

peno022 · 2022-12-10T05:02:16Z

@akingo55
おつかれさまです！こちらのPRのレビューをお願いしてもよいでしょうか？
（直近時間取れなくて難しいなどありましたら遠慮なくおっしゃってください🙏）

akingo55

確認しました！問題なさそうかと思います。

今回バグとなっていたBooksController以外については、現状のログインチェックの内容が正しいかどうかの検討は実施していません。

コード上はBooksController以外も修正しているので、それらも動作確認が必要かと思いました！（また別のバグを生む可能性もあるかと思うので）
私の方では問題なさそうでしたので、Approveします！

peno022 · 2022-12-11T22:13:22Z

@akingo55
ご確認ありがとうございました！🙏

今回バグとなっていたBooksController以外については、現状のログインチェックの内容が正しいかどうかの検討は実施していません。

こちらについて、すみません書き方がわるかったかもしれないのですが、「仕様が正しいかを改めて確認するということはせずに、現状の仕様のままになるように修正した」という意図でした。
とはいえ動作確認項目に書けてなかったので追加します🙇‍♀️ご指摘ありがとうございます🙇‍♀️

peno022 · 2022-12-13T04:46:52Z

@komagata
おつかれさまです！本PRに動作確認をどこまで含めるかご相談させてください🙏
バグ対象となっていたBooksControllerについてはもちろん動作確認・テスト実施するのですが、それ以外の修正ファイルの動作確認についてです。
修正ファイル数が64ファイルあり、全ファイル分手元で動作確認、またはテストの有無を確認してなければ追加、をやろうとするとかなり時間がかかりそうです。

とはいえ全件テスト書く
単純にbefore_action :require_loginを削除またはskip_before_action :require_login, raise: falseを追加しただけ、というファイルはコードだけ確認してOKにし、only〜〜、admin条件など、その他の条件があるものだけ対象にしてテストを書く（対象8ファイル）
コードだけの確認にして、あとは今のテストが通っていればOKにする‥？

くらいの選択肢が考えられるかなあと思ったのですが、2で進めることは問題あるでしょうか・・？

komagata · 2022-12-15T07:16:56Z

@peno022 最初にお話しいただいた時に、これが念頭にあるのでやるかどうか迷ったのですが、やるからには全件テストで行きたいと思います。

分量が多くなるのでポイントを増やしておきたいと思います。

peno022 · 2022-12-15T22:52:44Z

@komagata 承知しました！🙇‍♀️

peno022 · 2022-12-20T08:36:58Z

@komagata
たびたびすみません、追加で一点質問させてくださいm
ログイン確認のテストが下記2ファイルに分かれているのですが、なにか背景があるでしょうか？

test/system/require_login_test.rb
test/system/login_requirements_test.rb

1には /users/#{@user_id} 配下のものしか書かれていないので今回は2にテスト追加する必要があるのか、そもそもまとめてしまってよいファイルなのか確認したいです。

komagata · 2022-12-20T16:23:52Z

@peno022

ログイン確認のテストが下記2ファイルに分かれているのですが、なにか背景があるでしょうか？

本来はどちらかに統一すべきところを把握できていないので別のファイルに書いてしまったのだと思います。

もし作業時に問題なければ統一してしまっていただければ助かります〜。

peno022 · 2022-12-26T06:26:46Z

@komagata
おつかれさまです！
テストの追加を実施中なのですが、量が多いため、まだ途中ですが方針について確認させてください🙇‍♀️
各パターン、一部のテストを追加した状態でいったんコミットしており、方針に問題がなければこのまま全件分進めようと思っています。

下記の形でテスト追加に着手したのですが、懸念あるでしょうか？
ご確認いただけますと助かります🙇‍♀️

①before_action :require_loginを削除したのみのcontrollerのテスト
→test/system/require_login_test.rbに、どれか1つのアクションに対して、ログインなしでアクセスすると「ログインしてください」が出ることを確認するテストを書く

②skip_before_action :require_login, raise: falseを追加したのみのcontrollerのテスト
→同じくtest/system/require_login_test.rbに、どれか1つのアクションに対して、ログインなしでアクセスしてもページのコンテンツが表示されることを確認するテストを書く

③単純な削除/追加になっておらず、only/except等条件をつけたcontrollerのテスト
→各ページのシステムテストに追加（現在はtest/system/articles_test.rbのみテスト追加してあります）
対象：
app/controllers/articles_controller.rb
app/controllers/hibernation_controller.rb
app/controllers/retirement_controller.rb
app/controllers/users_controller.rb

④その他
app/controllers/api/admin/base_controller.rb
app/controllers/api/base_controller.rb
の変更については、apiのテストで権限確認をしているのでテスト追加なし。

komagata · 2022-12-27T22:02:26Z

@peno022

1

OKです〜。

2

できれば「どれか1つのアクション」ではなく全部のアクションをテストしたいです〜。

3

基本的にはOKです〜。

ログイン必須のテストはtest/system/require_loginディレクトリ以下にarticles_test.rbとかファイルを置いてく形でまとめるとわかりやすいかもですね。

もしくは、「ログインしてないとXXXという表示が出る」「ログインしていればXXXという表示がでない」というテストであればURLを渡すだけでテストできるような形に共通化して一気に処理できちゃうかもですね。

4

OKです〜。

すみません、認証関連の書き換えの件を以前は同じPRでいいといっちゃったんですが、共通化などを考えるとやはり別のIssueにした方がいいと思い直したので #5984 というIssueを作成しました。PRはこのままで大丈夫ですが、こちらの新しいIssueのポイントもGETできる感じで考えていただければ〜。

peno022 · 2022-12-29T03:04:41Z

@komagata
ご確認いただきありがとうございます！
2,3はご指摘いただいた形に修正して進めます🙏
ログイン処理共通化については別issueにして、同じPRで対応するという形にすることも承知しました🙏

…ーラでだけskip_before_actionを書く形に修正する

…st.rb をまとめる

peno022 · 2022-12-30T08:26:49Z

@komagata
おつかれさまです！年の瀬にすみません〜
テスト追加の対応を実施しましたので、PRのレビューをお願いしたいです🙇‍♀️

追記：
修正中に #5992 のバグを見つけたので、issue起票のうえあわせて修正しました。🙏

komagata · 2023-01-03T17:22:51Z

test/system/require_login/users_test.rb

+class UsersLoginTest < ApplicationSystemTestCase
+  include LoginAssertHelper
+  test 'cannot access users list without login' do
+    assert_login_required('/users')


Suggested change

assert_login_required('/users')

assert_login_required '/users'

複雑な場合以外はかっこはなくていいかもです〜

修正しました！

komagata · 2023-01-03T17:23:44Z

test/system/require_login_test.rb

-    assert_text 'ログインしてください'
+  include LoginAssertHelper
+  test 'redirect to welcome page and show message when you visit login required pages without login' do
+    assert_login_required('/announcements')


とってもいい感じだと思います〜！

pathの配列を受け取るようにしてもいいかもですね。

peno022

@komagata
レビューありがとうございます！修正しましたので再度ご確認お願いします🙏

pathの配列を受け取る形にしてもいいかも、というご指摘については、そうできていなかった背景の懸念をコメントに記載させていただきました🙇‍♀️　ご確認いただけますと幸いです。

peno022 · 2023-01-04T12:46:48Z

test/system/require_login_test.rb

+  test 'redirect to welcome page and show message when you visit login required pages without login' do
+    assert_login_required '/announcements'
+    assert_login_required '/books'
+    assert_login_required "/companies/#{companies(:company1).id}/products"


こちらについて、assert_login_requiredをたくさん書くことになるのでご指摘いただいたように配列を渡す形も検討したのですが、

paths = ['/announcements', '/books', '/companies'] paths.each do |path| assert_login_required(path) end

もしくは

def assert_login_required(paths) paths.each do |path| visit path assert_text 'ログインしてください' assert_selector 'h3', text: 'フィヨルドブートキャンプとは？' end end

のようにした場合、記述としては簡単になりますが、テストが失敗した時に、どのパスを渡した時でも同じ行がエラー行としてログに出てくるので、どれで失敗したかが分かりづらくなりそう、というのを懸念して、上記の形にはしませんでした。

配列を渡すにしても上記の問題は回避できる方法があれば、ご指摘いただけますと嬉しいです🙇‍♀️

@peno022 なるほどですね。

でしたらそのままでいいと思います〜。

komagata

確認させて頂きました。OKです〜🙆‍♂️

komagata · 2023-01-05T03:52:49Z

test/system/require_login_test.rb

+  test 'redirect to welcome page and show message when you visit login required pages without login' do
+    assert_login_required '/announcements'
+    assert_login_required '/books'
+    assert_login_required "/companies/#{companies(:company1).id}/products"


@peno022 なるほどですね。

でしたらそのままでいいと思います〜。

komagata · 2023-01-05T03:53:37Z

@peno022 作業量が多くて大変なIssueありがとうございます！お疲れ様です〜！

peno022 · 2023-01-05T11:50:00Z

@komagata 大きいPRになってしまいましたが、レビューありがとうございましたー！！🙌

peno022 changed the title ~~[WIP]application_controllerでログイン確認をするように修正~~ BooksControllerでのログイン有無チェックを修正 Dec 10, 2022

peno022 marked this pull request as ready for review December 10, 2022 04:58

peno022 requested a review from akingo55 December 10, 2022 05:00

peno022 self-assigned this Dec 10, 2022

akingo55 approved these changes Dec 11, 2022

View reviewed changes

peno022 force-pushed the bug/nomethoderror-on-books-without-login branch from 16eb8ed to 4941816 Compare December 26, 2022 06:04

peno022 added 12 commits December 30, 2022 17:08

before_action :require_loginをapplication_controllerに追加しておいて、不要なコントロ…

f220fef

…ーラでだけskip_before_actionを書く形に修正する

articles_controllerにログイン認証のskip漏れがあったので追加

eeeba44

BooksControllerのdeleteアクションもrequire_admin_or_mentor_loginの対象にするよう修正

af96339

余分な空白がrubocopエラーになっているので削除

4bb6786

articles一覧、詳細ページはログインなしでもアクセスできることを確認するテストを追加

1bf8619

books一覧は生徒アカウントでも表示できることを確認するテストを追加

a480960

test/system/require_login_test.rb と test/system/login_requirements_te…

e1f60e8

…st.rb をまとめる

ログイン必須ページのテストを追加

49caba9

ログインなしでアクセスできることを確認するテストを追加（一部）

27f8b7f

文字化けが起きていたので修正

d65bd18

require_loginディレクトリにログイン確認テストを切り出す

381f294

ログイン必須テスト、ログインなしでアクセス可のテストを追加

d02a605

peno022 force-pushed the bug/nomethoderror-on-books-without-login branch from f66ed5c to d02a605 Compare December 30, 2022 08:08

komagata reviewed Jan 3, 2023

View reviewed changes

関数の不要な()を削除

44ddd0d

peno022 commented Jan 4, 2023

View reviewed changes

komagata approved these changes Jan 5, 2023

View reviewed changes

komagata merged commit 6e369ee into main Jan 5, 2023

komagata deleted the bug/nomethoderror-on-books-without-login branch January 5, 2023 03:53

github-actions bot mentioned this pull request Jan 5, 2023

Release 2023-01-12 00:54:59 #5964

Merged

21 tasks

sadanora mentioned this pull request Jan 18, 2023

休会中・退会後でもログインが必要なページにアクセスできてしまう問題を修正 #6010

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BooksControllerでのログイン有無チェックを修正 #5921

BooksControllerでのログイン有無チェックを修正 #5921

peno022 commented Dec 10, 2022 •

edited

peno022 commented Dec 10, 2022

akingo55 left a comment

peno022 commented Dec 11, 2022

peno022 commented Dec 13, 2022

komagata commented Dec 15, 2022

peno022 commented Dec 15, 2022

peno022 commented Dec 20, 2022

komagata commented Dec 20, 2022

peno022 commented Dec 26, 2022

komagata commented Dec 27, 2022 •

edited

peno022 commented Dec 29, 2022

peno022 commented Dec 30, 2022 •

edited

komagata Jan 3, 2023

peno022 Jan 4, 2023

komagata Jan 3, 2023

peno022 left a comment

peno022 Jan 4, 2023

komagata Jan 5, 2023

komagata left a comment

komagata Jan 5, 2023

komagata commented Jan 5, 2023

peno022 commented Jan 5, 2023 •

edited

	assert_login_required('/users')
	assert_login_required '/users'

BooksControllerでのログイン有無チェックを修正 #5921

BooksControllerでのログイン有無チェックを修正 #5921

Conversation

peno022 commented Dec 10, 2022 • edited

Issue

概要

修正した手順

変更確認方法

Screenshot

変更前

変更後

peno022 commented Dec 10, 2022

akingo55 left a comment

Choose a reason for hiding this comment

peno022 commented Dec 11, 2022

peno022 commented Dec 13, 2022

komagata commented Dec 15, 2022

peno022 commented Dec 15, 2022

peno022 commented Dec 20, 2022

komagata commented Dec 20, 2022

peno022 commented Dec 26, 2022

komagata commented Dec 27, 2022 • edited

peno022 commented Dec 29, 2022

peno022 commented Dec 30, 2022 • edited

komagata Jan 3, 2023

Choose a reason for hiding this comment

peno022 Jan 4, 2023

Choose a reason for hiding this comment

komagata Jan 3, 2023

Choose a reason for hiding this comment

peno022 left a comment

Choose a reason for hiding this comment

peno022 Jan 4, 2023

Choose a reason for hiding this comment

komagata Jan 5, 2023

Choose a reason for hiding this comment

komagata left a comment

Choose a reason for hiding this comment

komagata Jan 5, 2023

Choose a reason for hiding this comment

komagata commented Jan 5, 2023

peno022 commented Jan 5, 2023 • edited

peno022 commented Dec 10, 2022 •

edited

komagata commented Dec 27, 2022 •

edited

peno022 commented Dec 30, 2022 •

edited

peno022 commented Jan 5, 2023 •

edited