pundit: convert dmails to pundit.

app/controllers/dmails_controller.rb

@@ -1,29 +1,26 @@
 class DmailsController < ApplicationController
   respond_to :html, :xml, :js, :json
-  before_action :member_only, except: [:index, :show, :update, :mark_all_as_read]
 
   def new
     if params[:respond_to_id]
-      parent = Dmail.find(params[:respond_to_id])
-      check_show_privilege(parent)
+      parent = authorize Dmail.find(params[:respond_to_id]), :show?
       @dmail = parent.build_response(:forward => params[:forward])
     else
-      @dmail = Dmail.new(dmail_params(:create))
+      @dmail = authorize Dmail.new(permitted_attributes(Dmail))
     end
 
     respond_with(@dmail)
   end
 
   def index
-    @dmails = Dmail.visible(CurrentUser.user).paginated_search(params, count_pages: true)
+    @dmails = authorize Dmail.visible(CurrentUser.user).paginated_search(params, count_pages: true)
     @dmails = @dmails.includes(:owner, :to, :from) if request.format.html?
 
     respond_with(@dmails)
   end
 
   def show
-    @dmail = Dmail.find(params[:id])
-    check_show_privilege(@dmail)
+    @dmail = authorize Dmail.find(params[:id])
 
     if request.format.html? && @dmail.owner == CurrentUser.user
       @dmail.update!(is_read: true)
@@ -33,38 +30,20 @@ class DmailsController < ApplicationController
   end
 
   def create
-    @dmail = Dmail.create_split(from: CurrentUser.user, creator_ip_addr: CurrentUser.ip_addr, **dmail_params(:create))
+    @dmail = authorize(Dmail).create_split(from: CurrentUser.user, creator_ip_addr: CurrentUser.ip_addr, **permitted_attributes(Dmail))
     respond_with(@dmail)
   end
 
   def update
-    @dmail = Dmail.find(params[:id])
-    check_update_privilege(@dmail)
-    @dmail.update(dmail_params(:update))
+    @dmail = authorize Dmail.find(params[:id])
+    @dmail.update(permitted_attributes(@dmail))
     flash[:notice] = "Dmail updated"
 
     respond_with(@dmail)
   end
 
   def mark_all_as_read
-    @dmails = CurrentUser.user.dmails.mark_all_as_read
+    @dmails = authorize(CurrentUser.user.dmails).mark_all_as_read
     respond_with(@dmails)
   end
-
-  private
-
-  def check_show_privilege(dmail)
-    raise User::PrivilegeError unless dmail.visible_to?(CurrentUser.user, params[:key])
-  end
-
-  def check_update_privilege(dmail)
-    raise User::PrivilegeError unless dmail.owner == CurrentUser.user
-  end
-
-  def dmail_params(context)
-    permitted_params = %i[title body to_name to_id] if context == :create
-    permitted_params = %i[is_read is_deleted] if context == :update
-
-    params.fetch(:dmail, {}).permit(permitted_params)
-  end
 end

app/models/dmail.rb

@@ -1,9 +1,7 @@
 require 'digest/sha1'
 
 class Dmail < ApplicationRecord
-
   validates_presence_of :title, :body, on: :create
-  validate :validate_sender_is_not_banned, on: :create
 
   belongs_to :owner, :class_name => "User"
   belongs_to :to, :class_name => "User"
@@ -123,10 +121,6 @@ class Dmail < ApplicationRecord
     def valid_key?(key)
       id == verifier.verified(key)
     end
-
-    def visible_to?(user, key)
-      owner_id == user.id || valid_key?(key)
-    end
   end
 
   include AddressMethods
@@ -137,12 +131,6 @@ class Dmail < ApplicationRecord
     unread.update(is_read: true)
   end
 
-  def validate_sender_is_not_banned
-    if from.try(:is_banned?)
-      errors[:base] << "Sender is banned and cannot send messages"
-    end
-  end
-
   def quoted_body
     "[quote]\n#{from.pretty_name} said:\n\n#{body}\n[/quote]\n\n"
   end

app/policies/dmail_policy.rb

@@ -0,0 +1,29 @@
+class DmailPolicy < ApplicationPolicy
+  def create?
+    unbanned?
+  end
+
+  def index?
+    user.is_member?
+  end
+
+  def mark_all_as_read?
+    user.is_member?
+  end
+
+  def update?
+    user.is_member? && record.owner_id == user.id
+  end
+
+  def show?
+    user.is_member? && (record.owner_id == user.id || record.valid_key?(request.params[:key]))
+  end
+
+  def permitted_attributes_for_create
+    [:title, :body, :to_name, :to_id]
+  end
+
+  def permitted_attributes_for_update
+    [:is_read, :is_deleted]
+  end
+end

test/functional/dmails_controller_test.rb

@@ -22,9 +22,8 @@ class DmailsControllerTest < ActionDispatch::IntegrationTest
       end
 
       context "with a respond_to_id" do
-        should "check privileges" do
-          @user2 = create(:user)
-          get_auth new_dmail_path, @user2, params: {:respond_to_id => @dmail.id}
+        should "not allow users to quote dmails belonging to unrelated users " do
+          get_auth new_dmail_path, @unrelated_user, params: {:respond_to_id => @dmail.id}
           assert_response 403
         end
 
@@ -134,6 +133,16 @@ class DmailsControllerTest < ActionDispatch::IntegrationTest
         assert_redirected_to Dmail.last
         assert_enqueued_emails 1
       end
+
+      should "not allow banned users to send dmails" do
+        create(:ban, user: @user)
+        @user.reload
+
+        assert_difference("Dmail.count", 0) do
+          post_auth dmails_path, @user, params: { dmail: { to_id: @unrelated_user.id, title: "abc", body: "abc" }}
+          assert_response 403
+        end
+      end
     end
 
     context "update action" do

test/unit/dmail_test.rb

@@ -25,16 +25,6 @@ class DmailTest < ActiveSupport::TestCase
       end
     end
 
-    context "from a banned user" do
-      should "not validate" do
-        user = create(:banned_user)
-        dmail = build(:dmail, owner: user, from: user)
-
-        assert_equal(false, dmail.valid?)
-        assert_equal(["Sender is banned and cannot send messages"], dmail.errors.full_messages)
-      end
-    end
-
     context "search" do
       should "return results based on title contents" do
         dmail = FactoryBot.create(:dmail, :title => "xxx", :owner => @user)