Merge pull request #4354 from manyfold3d/fix-permission-bug

Fix permission bug where contributors could edit things they didn't have permission to

Author: Floppy

Gemfile

@@ -156,7 +156,7 @@ gem "devise_zxcvbn", "~> 6.0"
 
 gem "federails", git: "https://gitlab.com/experimentslabs/federails.git", branch: "merge-contexts"
 gem "federails-moderation", "~> 0.3"
-gem "caber"
+gem "caber", github: "manyfold3d/caber"
 
 gem "nanoid", "~> 2.0"
 

Gemfile.lock

@@ -7,6 +7,13 @@ GIT
       capybara (>= 1.0, < 4)
       launchy
 
+GIT
+  remote: https://github.com/manyfold3d/caber.git
+  revision: 501a9f4dba0df4772f41713fc69cfc8ec470a7ba
+  specs:
+    caber (0.4.0)
+      rails (>= 7.1.4)
+
 GIT
   remote: https://github.com/manyfold3d/rubocop-pundit.git
   revision: 10ac81bbec63a9abd3d58be84dc47d9c490b29c6
@@ -182,8 +189,6 @@ GEM
       activesupport (>= 3.0.0)
       uniform_notifier (~> 1.11)
     byebug (12.0.0)
-    caber (0.4.0)
-      rails (>= 7.1.4)
     capybara (3.40.0)
       addressable
       matrix
@@ -910,7 +915,7 @@ DEPENDENCIES
   brakeman (~> 7.0)
   bullet (~> 8.0)
   byebug
-  caber
+  caber!
   capybara
   capybara-screenshot!
   climate_control (~> 1.2)

app/components/model_card.rb

@@ -7,11 +7,10 @@ class Components::ModelCard < Components::Base
 
   register_output_helper :status_badges
   register_output_helper :server_indicator
+  register_value_helper :policy
 
-  def initialize(model:, can_edit: false, can_destroy: false)
+  def initialize(model:)
     @model = model
-    @can_destroy = can_destroy
-    @can_edit = can_edit
   end
 
   def view_template
@@ -111,8 +110,8 @@ def actions
         end
         div class: "col col-auto" do
           BurgerMenu do
-            DropdownItem(icon: "pencil", label: t("components.model_card.edit_button.text"), path: edit_model_path(@model), aria_label: translate("components.model_card.edit_button.label", name: @model.name)) if @can_edit
-            DropdownItem(icon: "trash", label: t("components.model_card.delete_button.text"), path: model_path(@model), method: :delete, aria_label: translate("components.model_card.delete_button.label", name: @model.name), confirm: translate("models.destroy.confirm")) if @can_destroy
+            DropdownItem(icon: "pencil", label: t("components.model_card.edit_button.text"), path: edit_model_path(@model), aria_label: translate("components.model_card.edit_button.label", name: @model.name)) if policy(@model).edit?
+            DropdownItem(icon: "trash", label: t("components.model_card.delete_button.text"), path: model_path(@model), method: :delete, aria_label: translate("components.model_card.delete_button.label", name: @model.name), confirm: translate("models.destroy.confirm")) if policy(@model).destroy?
             DropdownItem(icon: "flag", label: t("general.report", type: ""), path: new_model_report_path(@model)) if SiteSettings.multiuser_enabled?
           end
         end

app/controllers/concerns/model_listable.rb

@@ -9,10 +9,6 @@ module ModelListable
   private
 
   def prepare_model_list
-    # Work out policies for showing buttons up front
-    @can_destroy = policy(Model).destroy?
-    @can_edit = policy(Model).edit?
-
     # Ordering
     @models = case session["order"]
     when "recent"

app/views/models/_list.html.erb

@@ -14,7 +14,7 @@
         <% end %>
       </div>
       <div class="row row-cols-1 row-cols-md-2 row-cols-lg-3 mb-4">
-        <%= render_component_collection Components::ModelCard, :model, @models, can_edit: @can_edit, can_destroy: @can_destroy %>
+        <%= render_component_collection Components::ModelCard, :model, @models %>
       </div>
       <% if pagination_settings["models"] %>
         <%= paginate @models %>

spec/policies/collection_policy_spec.rb

@@ -0,0 +1,7 @@
+require "rails_helper"
+
+describe CollectionPolicy do
+  let(:target_class) { Collection }
+
+  it_behaves_like "ApplicationPolicy"
+end

spec/policies/creator_policy_spec.rb

@@ -0,0 +1,7 @@
+require "rails_helper"
+
+describe CreatorPolicy do
+  let(:target_class) { Creator }
+
+  it_behaves_like "ApplicationPolicy"
+end

spec/policies/model_policy_spec.rb

@@ -3,10 +3,14 @@
 describe ModelPolicy do
   subject(:policy) { described_class }
 
-  let(:member) { create(:user) }
-  let(:moderator) { create(:moderator) }
-  let(:model) { create(:model) }
+  let(:target_class) { Model }
+
   let(:nested) { create(:model) }
+  let(:model) { create(:model) }
+  let(:moderator) { create(:moderator) }
+  let(:member) { create(:user) }
+
+  it_behaves_like "ApplicationPolicy"
 
   permissions :destroy? do
     it "denies deletion to users with member role" do

spec/policies/application_policy_spec.rb renamed to spec/policies/shared/application_policy_shared.rb

@@ -1,14 +1,12 @@
-require "rails_helper"
-
-describe ApplicationPolicy do
+shared_examples "ApplicationPolicy" do
   subject(:policy) { described_class }
 
   let(:member) { create(:user) }
-  let(:model) { create(:model) }
+  let(:object) { create(target_class.to_s.underscore.to_sym) }
 
   permissions :index?, :show? do
     it "allows users with member role by default" do
-      expect(policy).to permit(member, model)
+      expect(policy).to permit(member, object)
     end
 
     it "falls back to member role if ReBAC isn't available on the record" do
@@ -18,27 +16,27 @@
 
     context "when default member role access is removed" do
       before do
-        model.revoke_permission("view", Role.find_by(name: :member))
+        object.revoke_permission("view", Role.find_by(name: :member))
       end
 
       it "denies users without individual view permission" do
-        expect(policy).not_to permit(member, model)
+        expect(policy).not_to permit(member, object)
       end
 
       it "allows users with individual view permission" do
-        model.grant_permission_to "view", member
-        expect(policy).to permit(member, model)
+        object.grant_permission_to "view", member
+        expect(policy).to permit(member, object)
       end
     end
 
     context "with an unknown user" do
       it "denies access if public view permission isn't set" do
-        expect(policy).not_to permit(nil, model)
+        expect(policy).not_to permit(nil, object)
       end
 
       it "allows access if public view permission is set" do
-        model.grant_permission_to "view", nil
-        expect(policy).to permit(nil, model)
+        object.grant_permission_to "view", nil
+        expect(policy).to permit(nil, object)
       end
     end
   end
@@ -64,26 +62,26 @@
     let(:contributor) { create(:contributor) }
 
     it "allows all users with moderator role" do
-      expect(policy).to permit(moderator, model)
+      expect(policy).to permit(moderator, object)
     end
 
     it "denies users with contributor role" do
-      expect(policy).not_to permit(contributor, model)
+      expect(policy).not_to permit(contributor, object)
     end
 
     it "allows users with granted edit permission" do
-      model.grant_permission_to "edit", member
-      expect(policy).to permit(member, model)
+      object.grant_permission_to "edit", member
+      expect(policy).to permit(member, object)
     end
 
     it "allows users with granted owner permission" do
-      model.grant_permission_to "own", member
-      expect(policy).to permit(member, model)
+      object.grant_permission_to "own", member
+      expect(policy).to permit(member, object)
     end
 
-    it "denies unknown users on public models" do
-      model.grant_permission_to "view", nil
-      expect(policy).not_to permit(nil, model)
+    it "denies unknown users on public objects" do
+      object.grant_permission_to "view", nil
+      expect(policy).not_to permit(nil, object)
     end
   end
 end