Rails アンチパターン - 使われない権限メソッド（Authorization Astronaut）

はじめに

最近Rails AntiPatternsという本を読んでいました。

https://www.amazon.co.jp/dp/B004C04QE0/ref=dp-kindle-redirect?_encoding=UTF8&btkr=1

幾つかの項目を読んだ所なかなかおもしろそうだったので一冊通して読もうと思っています。
翻訳版はないので、ざっくり訳しつつ自分の感想を書いていこうと思います。 2010年に発売されたもので、サンプルコードやgem等は古いので適宜読み替える必要があります。
また、サンプルコードは一部変更して掲載しています。

Authorization Astronaut

権限ロジックは仕様に基づいて、または将来の要件を予期してプログラムされます。その結果、一般的なユーザー認証では、次のようなユーザー・モデルが作られます。

class User < ApplicationRecord
  has_many :user_roles
  has_many :roles, through: :user_roles

  def has_role?(role)
    roles.exists?(name: role)
  end

  def can_post?
    has_roles?(%w(admin editor associate_editor research_writer))
  end

  def can_review_posts?
    has_roles?(%w(admin editor associate_editor))
  end

  def can_edit_content?
    has_roles?(%w(admin editor associate_editor))
  end

  def can_edit_post?(post)
    self == post.user || has_roles?(%w(admin editor associate_editor))
  end
end

can_*というメソッドは破滅への道です。いつ使われるかわからず、インタフェースも曖昧で一貫性がないです。今後も必要になる前から権限ロジックだけ先に追加されるようになります。また、admin, editor等の権限名がハードコーディングされていて、変更に弱い状態です。

自分もこんな感じのをモデル見たことがあり、ああーなるほどという感じでした。モデルとコントローラを別の人で開発した場合にこんな感じになりがちな印象です。
ハードコーディングやめて定数にすればという人もいますが、本質的には変わらないと思います笑

ユーザにはロールをつけることができます。

class Role < ApplicationRecord
  has_many :user_roles
  has_many :users, through: :user_roles

  validates :name, uniqueness: true, presence: true

  def name=(value)
    write_attribute(:name, value.downcase)
  end

  def self.[](name)
    find_by(name: name.to_s)
  end

  def add_user(user)
    users << user
  end

  def delete_user(user)
    users.delete(user)
  end
end

Roleモデルにはいくつかの問題があります。管理ユーザーがロールを追加または削除できるようにする計画はありません。また、Role.[]というメソッドは識別子の変更を吸収するのに使われるかもしれませんが、その意図で書かれていないため、機能しません。どこでも使われない可能性もあります。
add_user、delete_userも良いインタフェースではありません。

あるあるだw

要するに、これら2つのモデルは独立して書かれました。アプリケーションの仕様が固まる前に、将来何が必要になるかを見越して書かれています。これは認証ロジック実装でかなり発生するようです。仕様を固めている間に、認証ロジックが開発者が事前に開始できるものとして認識されるためです。
これは誤った仮定で、過剰に設計されたコードにつながり、あいまいで矛盾したインターフェイスを提供し、正しく使用されないか、まったく使用されなくなってしまいます。

とはいえ、チームの文化とかいろんな要因で、やりたいことを理解する前（仕様が固まる前とは違う）にロジック部分だけ作ってしまい、こうゆうコードが生まれるのはたまに見かけます。

Solution: Simplify with Simple Flags

前のセクションで説明した複雑さの問題に対処するために、次のようにモデルをリファクタリングすることができます。

class User < ApplicationRecord
end

class AddColumnToUsers < ActiveRecord::Migration[5.1]
  def change
    add_column :users, :admin,  :boolean, null: false, default: false
    add_column :users, :editor, :boolean, null: false, default: false
    add_column :users, :writer, :boolean, null: false, default: false
  end
end

https://github.com/waterlow/authorization_astronaut/pull/2

これによってRoleモデルが削除でき、Userモデルにはadmin?、editor?などのメソッドが使えるようになります。フレームワークの機能をを効果的につかい、「コードが無い」という一番きれいな状態になります。今後１つ２つのロール追加くらいならばこの方法でも問題は無いでしょう。

なかなか衝撃的でした。３つあったテーブルを一つにしてしまって、しかもUserモデルには何も書かないというシンプルさ…。仕事だとこう言う選択は非常に勇気が要りますが、シンプルさを保つ事ができる非常に良い方法だと感じました。

それ以上にロールが追加される場合は、テーブルを増やしましょう。ただし、多対多用の中間テーブルは作らず、has_manyで完結させます。

class User < ApplicationRecord
  has_many :roles

  Role::TYPES.each do |role_type|
    define_method("#{role_type}?") do
      roles.exists?(name: role_type)
    end
  end
end

class Role < ApplicationRecord
  TYPES = %w(admin editor writer guest)
  validates :name, inclusion: { in: TYPES }
end

https://github.com/waterlow/authorization_astronaut/pull/3

これは、不必要なコードを排除し、過剰に設計されていないため、ユーザーの役割を扱う方法について疑問のない一貫したインターフェイスを提供するため、成功しています。

やっぱり前者のadmin?メソッドにこだわっていて、Railsの機能が提供してくれるものが一番シンプルだという考え方ですかね。私もこの辺は同意です。自前で書くよりはよっぽどいい。あと、DBの外部キー制約が使えなさそうだけど、モデルでバリデーション入れてるからいいよねという気持ちだろうと思います。

ここで概説した概念は、ユーザーの役割にのみ適用されるものではありません。また、アプリケーションドメインをモデル化し、モデルによって提供されるインタフェースを定義するときに、他の多くの状況にも適用できます。以下の簡単なガイドラインは、オーバーエンジニアリングを止め、定義されていない仕様とアプリケーションの変更の両方に直面する簡単なインターフェイスを提供するのに役立ちます。

•コードを書くときにアプリケーションの要件を超えて構築しないでください。
•具体的な要件がない場合は、コードを記述しないでください。
•すぐにモデルを作ろうとしないでください。付加的なモデルの追加を避けるために、ブーリアンや非正規化などの単純な方法がよく使用されます。
•データの追加、削除、または管理のためのユーザーインターフェイスがない場合、モデルは必要ありません。ハッシュまたは可能な値の配列で作成された非正規化列で問題ありません。

最後のはシリアライズのことをいっているのかなと思います。UIがない場合はモデル作らなくてもシリアライズで問題ないという考えは、以前ActiveRecord serialize / store の甘い誘惑を断ち切ろうという記事を読んで以来やらないほうがいいと思っていましたが、認可のようなアプリのメイン機能とは異なる場面では使ってもいいかなと思いました。
また、UIがない場合にモデルを追加しないという決断は驚きでした。

個人的な意見としては、空のモデルを作ってもいいかなとは思ったのですが、モデルを作ってしまう時点で将来の要求とはずれてしまうかもしれませんし、やはりモデルを作らないで済むのが一番拡張性の高い状態かなと思いました。

まとめ

仕事でこうゆうリファクタリングをぶっこんでいくのはなかなか難しそうですが、必要になるまで作らないという視点は常に持っておきたいです。

blog.waterlow.work

Ruby, Rails, js, etc...

Rails アンチパターン - 使われない権限メソッド（Authorization Astronaut）

はじめに

Authorization Astronaut

Solution: Simplify with Simple Flags

まとめ