今日から溜まりに溜まったモデルの話をしようと思っていたが、ページの描画などをしているうちに権限関係のメソッドが使いづらいことに気づいた。いろいろなことを考えてモデルやヘルパを先んじて作っているが、実際に使ってみるといろいろと不具合が出てくるものだ。テストは十分に書いているつもりなので、思い切ってガッツリとリファクタリングしてみる。
権限周りの見直し
現在は以下のような手順で権限管理をしている。
- ユーザがログインすると、welcome#index に飛ばされる
- welcome#index で current_user または current_admin に対して set_privileges が呼ばれる
- user の場合、user の権限を調べ、user_session に権限を記録する
- admin の場合、化ける user が設定されている場合、その user の権限を調べ、admin_session に権限を記録する
- view や controller では、controller の action に対応する権限一覧を controller に記録されている hash から取得する
- その後、取得した権限一覧を session に書かれている権限と比較し、権限のあるなしを返却する
ここで問題なのは、最後の確認メソッド has_privileges? を user や admin のインスタンスメソッドにしてしまったことだ。実際には、セッションとの比較をしているだけなので、user や admin の情報は必要ない。簡単に考えると、現在 concerns に記録しているメソッドを User に移動しクラスメソッドとすればよいのだが、これだとオブジェクト指向的に筋が悪い(user 個人の権限を該当者でない User クラスに聞くのは気持ち悪い)。
今回のリファクタリングでは user が権限を持つかではなく、controller が権限を許すかというように発想の転換を行ってみる。そもそも controller から権限一覧の hash を取り出すために constantize をしているため、class は取得できている。この controller class に対して、権限を聞くようにする。上記の手順における最後の二つは以下のように変更する。
- view や controller では、controller class に対して session を渡し、権限のあるなしを返却する。
コントローラテストの追加
上記の user への依存性があったため、テストコードはかなり複雑なものになっていた。has_privileges? については controller に移動する。まずは、このテストを spec/controllers/application_spec.rb に記述する。第一引数でコントローラ、第二引数以降は可変にして、action, 許可する権限、許可しない権限を並べて記述する。設定されていないデータがあった場合に許可しないことも確認しておく。
RSpec.describe ApplicationController, type: :controller do privilege_test Teacher::KyoumusController, :index, %w( is_kyoumu? ), %w( is_staff? ), :no_action, %w(), %w( is_kyoumu? is_staff? ) # 設定してない場合 privilege_test Teacher::YearsController, :index, %w( is_kyoumu? ), %w( is_staff? ), :create, %w( is_kyoumu? ), %w( is_staff? ) end
privilege_test メソッドは spec/rails_helper.rb に記述する。ここで、controller_class に対して、has_privileges? を呼んでいる。
# @param [Controller] controller_class コントローラクラス # @param [Array] test_array 検証するデータセット def privilege_test(controller_class, *array) it "should have correct privileges" do array.each_slice(3) do |(action, permit_privileges, reject_privileges)| permit_privileges.each do |p| expect(controller_class.has_privileges?(action, { p => true })).to be_truthy end reject_privileges.each do |p| expect(controller_class.has_privileges?(action, { p => true })).to be_falsey end end end end
controller の has_privileges? クラスメソッド実装
has_privileges? はほぼすべてのコントローラで使用するので、親クラスである app/controllers/application_controler.rb で実装する。継承しているので、self::Privileges は各子クラスで設定したものが読み込まれるので問題ない。中身は user に書いていたものと同様であるが、controller に記述した hash をわざわざ外部から読み取る必要がなくなった分すっきりしている。
def self.has_privileges?(action, session) privileges = self::Privileges[action] session.values_at(*privileges).compact.first end
これで、上記のテストは通過した。
権限処理の切り替え
コントローラ側での権限管理が作成できたので、現在のユーザ側での権限管理から切替を行う。一つ一つ探していくのは大変なので、モデル側の実装を削除してしまい、問題が起こるテストを一つ一つ潰していけばよい。
まず、app/models/concerns/privilges.rb を削除する。
rm app/models/concerns/privilege.rb
app/models/admin.rb と app/models/users.rb にて include していたので、それらを削除する。
spec/models/user_spec.rb から has_privileges? 関係のテストを削除する。以下のように set_privileges だけのテストになり、見やすくなった。
context 'Each user of users' do context 'checking the privileges' do answers = { hkob: [ nil, true ], skyoumu: [ true, true ], m11001: [ nil, nil ], } UserFactoryHash.keys.each do |user_key| it "#{user_key} should have correct privileges" do user = user_factory(user_key) session = {} user.set_privileges(session) expect(session.values_at(*User::Privileges)).to eq answers[user_key] end end end end
肝心の privilege_check は現在、このようになっている。この時点で怪しいと思うべきだった。
# @note ページの権限を確認して、権限がない場合にはリダイレクト # @return [Boolean] 権限がある場合には true を返す def privilege_check set_campus_id_to_session if ua = current_user_or_admin privileges = self.class::Privileges[self.action_name.to_sym] redirect_to_root_path_with_message(ua.has_privileges?(each_session, privileges)) else redirect_to new_user_session_path, alert: 'ログインしないとこのページは表>示できません' end end
改訂後は以下のように簡単になった。
# @note ページの権限を確認して、権限がない場合にはリダイレクト # @return [Boolean] 権限がある場合には true を返す def privilege_check set_campus_id_to_session if ua = current_user_or_admin has_pr = self.class.has_privileges?(self.action_name, each_session) redirect_to_root_path_with_message(has_pr) else redirect_to new_user_session_path, alert: 'ログインしないとこのページは表示できません' end end
次に、メニュー記述関係の権限を修正する。最初に write_menu のテストを修正する。ユーザがなくなりすっきりした。
it 'should obtain a table line' do ans = write_menu({ 'is_kyoumu?' => true }, teacher_kyoumus_path, 'abc') expect(ans).to eq '<tr><td><a data-method="get" href="/teacher/kyoumus">[教>務室トップページ]</a></td><td>abc</td><td>教務室</td></tr>' ans = write_menu({}, teacher_kyoumus_path, 'abc') expect(ans).to eq '<tr><td>教務室トップページ</td><td>abc</td><td>教務室</td></tr>' end
この実装は以下の様になり、user または admin がなくなった。また、has_privileges_from_path という権限だけをpath から取得するメソッドを別に作った。この部分は別の場所でも使うためである。そういえば、method を渡すのも忘れていたので追記した。
# @param [Hash] es user_session または admin_session # @param [String] path URL # @param [String] detail 説明文字列 # @param [String] method GET 以外の場合は記述 # @return [String] 表示するメニュー行 def write_menu(es, path, detail, method = nil) hp = has_privileges_from_path(es, path) content_tag :tr do concat content_tag(:td, link_to_if(hp, lh(title_from_path(path), hp), path, method: method)) concat content_tag(:td, detail) concat content_tag(:td, privileges_str(privileges)) end end
この has_privileges_from_path のためにテストを記述する。
it 'should obtain has_privileges from path' do kyoumu = { 'is_kyoumu?' => true } expect(has_privileges_from_path(kyoumu, teacher_kyoumus_path)).to be_truthy gaku = { 'is_gaku?' => true } expect(has_privileges_from_path(gaku, teacher_kyoumus_path)).to be_nil end
このテストのための実装は以下の様になる。
# @param [Hash] es user_session または admin_session # @param [String] path URL # @return [Boolean] 権限があるかどうか def has_privileges_from_path(es, path) path_hash = Rails.application.routes.recognize_path(path) ca, a = path_hash.values_at(:controller, :action) klass = "#{ca}_controller".classify.constantize klass.has_privileges?(a.to_sym, es) end
最後に受入テストでエラーが出たので、該当行を修正する。
= write_menu es, teacher_curriculums_path, 'カリキュラムを設定します'
guard ですべてのテストでエラーがなくなったので、リファクタリング終了。あらゆるところで、user が必要なくなるので、テストや view の描画が簡略化すると思われる。
かなり大きい変更なので今日はここまで。
written by iHatenaSync
