From 480f39c34afb62bf86e9e83f6bdf33dc5272afcc Mon Sep 17 00:00:00 2001 From: evazion Date: Wed, 18 Mar 2020 01:01:40 -0500 Subject: [PATCH] pundit: convert dmails to pundit. --- app/controllers/dmails_controller.rb | 37 +++++------------------ app/models/dmail.rb | 12 -------- app/policies/dmail_policy.rb | 29 ++++++++++++++++++ test/functional/dmails_controller_test.rb | 15 +++++++-- test/unit/dmail_test.rb | 10 ------ 5 files changed, 49 insertions(+), 54 deletions(-) create mode 100644 app/policies/dmail_policy.rb diff --git a/app/controllers/dmails_controller.rb b/app/controllers/dmails_controller.rb index 869238bec..9fe2eb362 100644 --- a/app/controllers/dmails_controller.rb +++ b/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 diff --git a/app/models/dmail.rb b/app/models/dmail.rb index bd76a8466..86417d78f 100644 --- a/app/models/dmail.rb +++ b/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 diff --git a/app/policies/dmail_policy.rb b/app/policies/dmail_policy.rb new file mode 100644 index 000000000..effb614d1 --- /dev/null +++ b/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 diff --git a/test/functional/dmails_controller_test.rb b/test/functional/dmails_controller_test.rb index 50eef56b1..c2e055416 100644 --- a/test/functional/dmails_controller_test.rb +++ b/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 diff --git a/test/unit/dmail_test.rb b/test/unit/dmail_test.rb index 5f9f24c78..3c5b65415 100644 --- a/test/unit/dmail_test.rb +++ b/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)