From 44cfa73d729a60f8e6767ca46e777492a0be8872 Mon Sep 17 00:00:00 2001
From: Kim Burgestrand <kim@burgestrand.se>
Date: Fri, 11 Feb 2022 12:37:43 +0100
Subject: [PATCH] Require users to explicitly define Scope#resolve
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Closes https://github.com/varvet/pundit/pull/711 (original issue and pull request)

> A01:2021-Broken Access Control is the category with the most serious web
application security risk.
>
> Using scope.all in templates violates the principle of least privilege
or deny by default, where access should only be granted for particular
capabilities, roles, or users.
>
> This change improves the security of default templates
>
> Ref: https://owasp.org/Top10/A01_2021-Broken_Access_Control/
— by @tagliala (github.com/tagliala)

Co-authored-by: Duncan Stuart <dgmstuart@gmail.com>
---
 CHANGELOG.md                                  |  4 ++
 .../install/templates/application_policy.rb   |  2 +-
 .../pundit/policy/templates/policy.rb         |  7 +--
 pundit.gemspec                                |  1 +
 spec/generators_spec.rb                       | 43 +++++++++++++++++++
 5 files changed, 53 insertions(+), 4 deletions(-)
 create mode 100644 spec/generators_spec.rb

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 5710063..7ff3000 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -6,6 +6,10 @@
 
 - Using `policy_class` and a namespaced record now passes only the record when instantiating the policy. (#697, #689, #694, #666)
 
+### Changed
+
+- Require users to explicitly define Scope#resolve in generated policies (#711, #722)
+
 ### Deprecated
 
 - Deprecate `include Pundit` in favor of `include Pundit::Authorization` (#621)
diff --git a/lib/generators/pundit/install/templates/application_policy.rb b/lib/generators/pundit/install/templates/application_policy.rb
index d989e9b..e000cba 100644
--- a/lib/generators/pundit/install/templates/application_policy.rb
+++ b/lib/generators/pundit/install/templates/application_policy.rb
@@ -43,7 +43,7 @@ class ApplicationPolicy
     end
 
     def resolve
-      scope.all
+      raise NotImplementedError, "You must define #resolve in #{self.class}"
     end
 
     private
diff --git a/lib/generators/pundit/policy/templates/policy.rb b/lib/generators/pundit/policy/templates/policy.rb
index 8eb9def..6798550 100644
--- a/lib/generators/pundit/policy/templates/policy.rb
+++ b/lib/generators/pundit/policy/templates/policy.rb
@@ -1,9 +1,10 @@
 <% module_namespacing do -%>
 class <%= class_name %>Policy < ApplicationPolicy
   class Scope < Scope
-    def resolve
-      scope.all
-    end
+    # NOTE: Be explicit about which records you allow access to!
+    # def resolve
+    #   scope.all
+    # end
   end
 end
 <% end -%>
diff --git a/pundit.gemspec b/pundit.gemspec
index 2cfdb7d..44c8e8f 100644
--- a/pundit.gemspec
+++ b/pundit.gemspec
@@ -24,6 +24,7 @@ Gem::Specification.new do |gem|
   gem.add_development_dependency "activemodel", ">= 3.0.0"
   gem.add_development_dependency "bundler"
   gem.add_development_dependency "pry"
+  gem.add_development_dependency "railties", ">= 3.0.0"
   gem.add_development_dependency "rake"
   gem.add_development_dependency "rspec", ">= 3.0.0"
   gem.add_development_dependency "rubocop", "1.24.0"
diff --git a/spec/generators_spec.rb b/spec/generators_spec.rb
new file mode 100644
index 0000000..5f15877
--- /dev/null
+++ b/spec/generators_spec.rb
@@ -0,0 +1,43 @@
+# frozen_string_literal: true
+
+require "spec_helper"
+require "tmpdir"
+
+require "rails/generators"
+require "generators/pundit/install/install_generator"
+require "generators/pundit/policy/policy_generator"
+
+RSpec.describe "generators" do
+  before(:all) do
+    @tmpdir = Dir.mktmpdir
+
+    Dir.chdir(@tmpdir) do
+      Pundit::Generators::InstallGenerator.new([], { quiet: true }).invoke_all
+      Pundit::Generators::PolicyGenerator.new(%w[Widget], { quiet: true }).invoke_all
+
+      require "./app/policies/application_policy"
+      require "./app/policies/widget_policy"
+    end
+  end
+
+  after(:all) do
+    FileUtils.remove_entry(@tmpdir)
+  end
+
+  describe "WidgetPolicy", type: :policy do
+    permissions :index?, :show?, :create?, :new?, :update?, :edit?, :destroy? do
+      it "has safe defaults" do
+        expect(WidgetPolicy).not_to permit(double("User"), double("Widget"))
+      end
+    end
+
+    describe "WidgetPolicy::Scope" do
+      describe "#resolve" do
+        it "raises a descriptive error" do
+          scope = WidgetPolicy::Scope.new(double("User"), double("User.all"))
+          expect { scope.resolve }.to raise_error(NotImplementedError, /WidgetPolicy::Scope/)
+        end
+      end
+    end
+  end
+end