SchedulerControllerの認証方法とテストコードの見直し

[JunichiIto]

https://bootcamp.fjord.jp/questions/1766#answer_6533 でコメントした内容です。

問題点1：環境変数TOKENが未設定だと、認証がパスしてしまう

bootcamp/app/controllers/scheduler_controller.rb

Line 11 in e660538

return if ENV['TOKEN'] == params[:token]

上記コードで環境変数TOKENが未設定かつ、GETパラメータのtokenが未指定だと、nil == nilになって認証がパスしてしまう。
このせいで、tokenを指定しなくてもテストがパスしてしまっている。

例

bootcamp/test/system/notification/regular_events_test.rb

Line 70 in e660538

visit '/scheduler/daily/notify_coming_soon_regular_events'

たとえば、以下のようなコードに変えて、TOKENが設定されていなければ常に認証NGとした方がセキュリティ的に安全。
（本番環境でも万が一のTOKEN設定漏れを防げる）

return if ENV['TOKEN'].present? && ENV['TOKEN'] == params[:token]

その上で、スケジューラーを呼び出す既存のテストコードをtoken認証する形式に修正する。

問題点2：ユーザー認証でスケジューラーを呼び出すテストコードがある

以下のテストコードではtoken認証ではなく、ユーザー認証でスケジューラーのURLを呼びだしてしまっている。

bootcamp/test/system/notification/questions_test.rb

Line 306 in 70bb35d

visit_with_auth '/scheduler/daily/notify_certain_period_passed_after_last_answer', 'kimura'

そのため、該当コントーラーがSchedulerControllerを継承していない問題を検知できていないので、token認証するテストコードに修正が必要。

[JunichiIto]

#6929 でも同じような不具合があることがわかりました。

スケジューラーがApplicationControllerを直接継承していた場合、今の実装だとスケジューラの実行結果が200になり、異常終了していることに気づけません。
今後も起こりうるうっかりミスなので、たとえば「/schdulerで始まるパスをリクエストされた && self が SchedulerController を継承していないなら 500エラーとする」みたいなポカヨケロジックをApplicationControllerを組み込んだ方がいいかもしれません。

[ogawa-tomo]

本件については、下記PRで対応。
#6970

scheduler配下のパスにある定期処理について、実装を確認。

bootcamp/config/routes/scheduler.rb

Lines 4 to 16 in 2e6e84b

	namespace :scheduler do
	resource :statistic, only: %i(show), controller: "statistic"
	resource :link_checker, only: %i(show), controller: "link_checker"
	resource :validator, only: %i(show), controller: "validator"
	namespace :daily do
	resource :notify_coming_soon_regular_events, only: %i(show), controller: "notify_coming_soon_regular_events"
	resource :notify_certain_period_passed_after_last_answer, only: %i(show), controller: "notify_certain_period_passed_after_last_answer"
	resource :notify_product_review_not_completed, only: %i(show), controller: "notify_product_review_not_completed"
	resource :send_message, only: %i(show), controller: "send_message"
	resource :auto_retire, only: %i(show), controller: "auto_retire"
	resource :fetch_external_entry, only: %i(show), controller: "fetch_external_entry"
	end
	end

SchedulerControllerを継承していないのは以下の2つだが、それぞれ既に対応中。

• notify_certain_period_passed_after_last_answer
  • ベストアンサーなし通知が本番で動作しないバグを修正 #6970
• fetch_external_entry
  • みんなのブログが更新されない。 #6929

以下のパスについては定期処理のweb APIのテスト自体がないので、追加する必要がある。

• statistic
• link_checker
• validator
• notify_product_review_not_completed
• send_message

[ogawa-tomo]

本イシューについてはこのPRで対応することとし、Web APIのテストがない定期処理へのテスト追加は、下記のイシューに切り出しました。

• 定期処理にWeb APIのテストを追加する #6993

[ogawa-tomo]

こちらのPRで対応完了しましたのでこのイシューはクローズします。
#6970