From b3ff08fedf8c9f37fb4b7e1f99d26ca407140ef3 Mon Sep 17 00:00:00 2001 From: evazion Date: Tue, 17 Mar 2020 04:36:05 -0500 Subject: [PATCH] pundit: convert wiki pages to pundit. --- app/controllers/wiki_pages_controller.rb | 29 ++++++------ app/models/wiki_page.rb | 7 --- app/policies/wiki_page_policy.rb | 17 +++++++ app/views/wiki_pages/_form.html.erb | 2 +- .../wiki_pages/_secondary_links.html.erb | 6 +-- test/functional/wiki_pages_controller_test.rb | 44 ++++++++++++++++++- test/unit/wiki_page_test.rb | 39 ---------------- 7 files changed, 78 insertions(+), 66 deletions(-) create mode 100644 app/policies/wiki_page_policy.rb diff --git a/app/controllers/wiki_pages_controller.rb b/app/controllers/wiki_pages_controller.rb index a3ed76e0d..09212fab6 100644 --- a/app/controllers/wiki_pages_controller.rb +++ b/app/controllers/wiki_pages_controller.rb @@ -1,31 +1,32 @@ class WikiPagesController < ApplicationController respond_to :html, :xml, :json, :js - before_action :member_only, :except => [:index, :search, :show, :show_or_new] before_action :normalize_search_params, :only => [:index] layout "sidebar" def new - @wiki_page = WikiPage.new(wiki_page_params(:create)) + @wiki_page = authorize WikiPage.new(permitted_attributes(WikiPage)) respond_with(@wiki_page) end def edit @wiki_page, _found_by = WikiPage.find_by_id_or_title(params[:id]) + authorize @wiki_page respond_with(@wiki_page) end def index - @wiki_pages = WikiPage.paginated_search(params) - + @wiki_pages = authorize WikiPage.paginated_search(params) respond_with(@wiki_pages) end def search + authorize WikiPage render layout: "default" end def show @wiki_page, found_by = WikiPage.find_by_id_or_title(params[:id]) + if request.format.html? && @wiki_page.blank? && found_by == :title @wiki_page = WikiPage.new(title: params[:id]) respond_with @wiki_page, status: 404 @@ -39,13 +40,16 @@ class WikiPagesController < ApplicationController end def create - @wiki_page = WikiPage.create(wiki_page_params(:create)) + @wiki_page = authorize WikiPage.new(permitted_attributes(WikiPage)) + @wiki_page.save respond_with(@wiki_page) end def update @wiki_page, _found_by = WikiPage.find_by_id_or_title(params[:id]) - @wiki_page.update(wiki_page_params(:update)) + authorize @wiki_page + + @wiki_page.update(permitted_attributes(@wiki_page)) flash[:notice] = @wiki_page.warnings.full_messages.join(".\n \n") if @wiki_page.warnings.any? respond_with(@wiki_page) @@ -53,12 +57,16 @@ class WikiPagesController < ApplicationController def destroy @wiki_page, _found_by = WikiPage.find_by_id_or_title(params[:id]) + authorize @wiki_page + @wiki_page.update(is_deleted: true) respond_with(@wiki_page) end def revert @wiki_page, _found_by = WikiPage.find_by_id_or_title(params[:id]) + authorize @wiki_page + @version = @wiki_page.versions.find(params[:version_id]) @wiki_page.revert_to!(@version) flash[:notice] = "Page was reverted" @@ -67,7 +75,7 @@ class WikiPagesController < ApplicationController def show_or_new if params[:title].blank? - redirect_to new_wiki_page_path(wiki_page_params(:create)) + redirect_to new_wiki_page_path(permitted_attributes(WikiPage)) else redirect_to wiki_page_path(params[:title]) end @@ -89,11 +97,4 @@ class WikiPagesController < ApplicationController params[:search][:title] = params.delete(:title) end end - - def wiki_page_params(context) - permitted_params = %i[title body other_names other_names_string is_deleted] - permitted_params += %i[is_locked] if CurrentUser.is_builder? - - params.fetch(:wiki_page, {}).permit(permitted_params) - end end diff --git a/app/models/wiki_page.rb b/app/models/wiki_page.rb index 53f8f7536..6c6c70f43 100644 --- a/app/models/wiki_page.rb +++ b/app/models/wiki_page.rb @@ -11,7 +11,6 @@ class WikiPage < ApplicationRecord validates_presence_of :title validates_presence_of :body, :unless => -> { is_deleted? || other_names.present? } validate :validate_rename - validate :validate_not_locked array_attribute :other_names has_one :tag, :foreign_key => "name", :primary_key => "title" @@ -116,12 +115,6 @@ class WikiPage < ApplicationRecord extend SearchMethods include ApiMethods - def validate_not_locked - if is_locked? && !CurrentUser.is_builder? - errors.add(:is_locked, "and cannot be updated") - end - end - def validate_rename return unless title_changed? diff --git a/app/policies/wiki_page_policy.rb b/app/policies/wiki_page_policy.rb new file mode 100644 index 000000000..81b897fcc --- /dev/null +++ b/app/policies/wiki_page_policy.rb @@ -0,0 +1,17 @@ +class WikiPagePolicy < ApplicationPolicy + def update? + unbanned? && (can_edit_locked? || !record.is_locked?) + end + + def revert? + update? + end + + def can_edit_locked? + user.is_builder? + end + + def permitted_attributes + [:title, :body, :other_names, :other_names_string, :is_deleted, (:is_locked if can_edit_locked?)].compact + end +end diff --git a/app/views/wiki_pages/_form.html.erb b/app/views/wiki_pages/_form.html.erb index b9944bdf4..bb0100e7d 100644 --- a/app/views/wiki_pages/_form.html.erb +++ b/app/views/wiki_pages/_form.html.erb @@ -7,7 +7,7 @@ <%= dtext_field "wiki_page", "body" %> - <% if CurrentUser.is_builder? %> + <% if policy(@wiki_page).can_edit_locked? %> <%= f.input :is_locked, label: "Locked", hint: "Locked wikis can only be edited by Builders." %> <% end %> diff --git a/app/views/wiki_pages/_secondary_links.html.erb b/app/views/wiki_pages/_secondary_links.html.erb index 35f0d03dc..276591965 100644 --- a/app/views/wiki_pages/_secondary_links.html.erb +++ b/app/views/wiki_pages/_secondary_links.html.erb @@ -2,7 +2,7 @@ <%= quick_search_form_for(:title_normalize, wiki_pages_path, "wiki pages", autocomplete: "wiki-page", redirect: true) %> <%= subnav_link_to "Listing", wiki_pages_path %> <%= subnav_link_to "Search", search_wiki_pages_path %> - <% if CurrentUser.is_member? %> + <% if policy(WikiPage).new? %> <%= subnav_link_to "New", new_wiki_page_path %> <% end %> <%= subnav_link_to "Help", wiki_page_path("help:wiki") %> @@ -13,7 +13,7 @@
  • |
    • <%= subnav_link_to "Posts (#{@wiki_page.tag.try(:post_count) || 0})", posts_path(:tags => @wiki_page.title) %> <%= subnav_link_to "History", wiki_page_versions_path(:search => {:wiki_page_id => @wiki_page.id}) %> - <% if CurrentUser.is_member? %> + <% if policy(@wiki_page).edit? %> <%= subnav_link_to "Edit", edit_wiki_page_path(@wiki_page.id), "data-shortcut": "e" %> <% if @wiki_page.is_deleted? %> @@ -28,7 +28,7 @@ <% if @wiki_page_version.previous %> <%= subnav_link_to "Diff", diff_wiki_page_versions_path(:otherpage => @wiki_page_version.id, :thispage => @wiki_page_version.previous.id) %> <% end %> - <% if CurrentUser.is_member? %> + <% if policy(@wiki_page_version.wiki_page).revert? %> <%= subnav_link_to "Revert to", revert_wiki_page_path(@wiki_page_version.wiki_page_id, :version_id => @wiki_page_version.id), :method => :put, :data => {:confirm => "Are you sure you want to revert to this version?"} %> <% end %> <% end %> diff --git a/test/functional/wiki_pages_controller_test.rb b/test/functional/wiki_pages_controller_test.rb index 6d495f756..e23d89c0d 100644 --- a/test/functional/wiki_pages_controller_test.rb +++ b/test/functional/wiki_pages_controller_test.rb @@ -33,6 +33,13 @@ class WikiPagesControllerTest < ActionDispatch::IntegrationTest end end + context "search action" do + should "work" do + get search_wiki_pages_path + assert_response :success + end + end + context "show action" do setup do as_user do @@ -146,6 +153,7 @@ class WikiPagesControllerTest < ActionDispatch::IntegrationTest should "create a wiki_page" do assert_difference("WikiPage.count", 1) do post_auth wiki_pages_path, @user, params: {:wiki_page => {:title => "abc", :body => "abc"}} + assert_redirected_to(wiki_page_path(WikiPage.last)) end end end @@ -155,13 +163,45 @@ class WikiPagesControllerTest < ActionDispatch::IntegrationTest as_user do @tag = create(:tag, name: "foo", post_count: 42) @wiki_page = create(:wiki_page, title: "foo") + @builder = create(:builder_user) end end should "update a wiki_page" do put_auth wiki_page_path(@wiki_page), @user, params: {:wiki_page => {:body => "xyz"}} - @wiki_page.reload - assert_equal("xyz", @wiki_page.body) + + assert_redirected_to wiki_page_path(@wiki_page) + assert_equal("xyz", @wiki_page.reload.body) + end + + should "not allow members to edit locked wiki pages" do + as(@user) { @wiki_page.update!(is_locked: true) } + put_auth wiki_page_path(@wiki_page), @user, params: { wiki_page: { body: "xyz" }} + + assert_response 403 + assert_not_equal("xyz", @wiki_page.reload.body) + end + + should "allow builders to edit locked wiki pages" do + as(@builder) { @wiki_page.update!(is_locked: true) } + put_auth wiki_page_path(@wiki_page), @builder, params: { wiki_page: { body: "xyz" }} + + assert_redirected_to wiki_page_path(@wiki_page) + assert_equal("xyz", @wiki_page.reload.body) + end + + should "not allow members to edit the is_locked flag" do + put_auth wiki_page_path(@wiki_page), @user, params: { wiki_page: { is_locked: true }} + + assert_response 403 + assert_equal(false, @wiki_page.reload.is_locked) + end + + should "allow builders to edit the is_locked flag" do + put_auth wiki_page_path(@wiki_page), @builder, params: { wiki_page: { is_locked: true }} + + assert_redirected_to wiki_page_path(@wiki_page) + assert_equal(true, @wiki_page.reload.is_locked) end should "warn about renaming a wiki page with a non-empty tag" do diff --git a/test/unit/wiki_page_test.rb b/test/unit/wiki_page_test.rb index 96b07ddb5..bd52fc719 100644 --- a/test/unit/wiki_page_test.rb +++ b/test/unit/wiki_page_test.rb @@ -11,38 +11,6 @@ class WikiPageTest < ActiveSupport::TestCase end context "A wiki page" do - context "that is locked" do - should "not be editable by a member" do - CurrentUser.user = FactoryBot.create(:moderator_user) - @wiki_page = FactoryBot.create(:wiki_page, :is_locked => true) - CurrentUser.user = FactoryBot.create(:user) - @wiki_page.update(body: "hello") - assert_equal(["Is locked and cannot be updated"], @wiki_page.errors.full_messages) - end - - should "be editable by a moderator" do - CurrentUser.user = FactoryBot.create(:moderator_user) - @wiki_page = FactoryBot.create(:wiki_page, :is_locked => true) - CurrentUser.user = FactoryBot.create(:moderator_user) - @wiki_page.update(body: "hello") - assert_equal([], @wiki_page.errors.full_messages) - end - end - - context "updated by a moderator" do - setup do - @user = FactoryBot.create(:moderator_user) - CurrentUser.user = @user - @wiki_page = FactoryBot.create(:wiki_page) - end - - should "allow the is_locked attribute to be updated" do - @wiki_page.update(is_locked: true) - @wiki_page.reload - assert_equal(true, @wiki_page.is_locked?) - end - end - context "updated by a regular user" do setup do @user = FactoryBot.create(:user) @@ -50,13 +18,6 @@ class WikiPageTest < ActiveSupport::TestCase @wiki_page = FactoryBot.create(:wiki_page, :title => "HOT POTATO", :other_names => "foo*bar baz") end - should "not allow the is_locked attribute to be updated" do - @wiki_page.update(is_locked: true) - assert_equal(["Is locked and cannot be updated"], @wiki_page.errors.full_messages) - @wiki_page.reload - assert_equal(false, @wiki_page.is_locked?) - end - should "normalize its title" do assert_equal("hot_potato", @wiki_page.title) end