From 9417149e6c5342e6cef7f78026b80efdceef0bf9 Mon Sep 17 00:00:00 2001 From: Sjors Smits Date: Mon, 10 Jun 2024 16:16:43 +0200 Subject: [PATCH 1/9] add the policy class to the not authorized error message --- lib/pundit.rb | 2 +- spec/pundit_spec.rb | 9 +++++---- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/lib/pundit.rb b/lib/pundit.rb index 27dd032a..741d4df1 100644 --- a/lib/pundit.rb +++ b/lib/pundit.rb @@ -36,7 +36,7 @@ def initialize(options = {}) @record = options[:record] @policy = options[:policy] - message = options.fetch(:message) { "not allowed to #{query} this #{record.class}" } + message = options.fetch(:message) { "not allowed to #{policy.class}##{query} this #{record.class}" } end super(message) diff --git a/spec/pundit_spec.rb b/spec/pundit_spec.rb index fadddc85..e9760c97 100644 --- a/spec/pundit_spec.rb +++ b/spec/pundit_spec.rb @@ -57,11 +57,11 @@ expect { Pundit.authorize(user, article_tag, :destroy?) }.to raise_error(Pundit::NotAuthorizedError) end - it "raises an error with a query and action" do + it "raises an error with the policy, query and record" do # rubocop:disable Style/MultilineBlockChain expect do Pundit.authorize(user, post, :destroy?) - end.to raise_error(Pundit::NotAuthorizedError, "not allowed to destroy? this Post") do |error| + end.to raise_error(Pundit::NotAuthorizedError, "not allowed to PostPolicy#destroy? this Post") do |error| expect(error.query).to eq :destroy? expect(error.record).to eq post expect(error.policy).to have_attributes( @@ -73,11 +73,12 @@ # rubocop:enable Style/MultilineBlockChain end - it "raises an error with a the record, query and action when the record is namespaced" do + it "raises an error with the policy, query and record when the record is namespaced" do # rubocop:disable Style/MultilineBlockChain expect do Pundit.authorize(user, [:project, :admin, comment], :destroy?) - end.to raise_error(Pundit::NotAuthorizedError, "not allowed to destroy? this Comment") do |error| + end.to raise_error(Pundit::NotAuthorizedError, + "not allowed to Project::Admin::CommentPolicy#destroy? this Comment") do |error| expect(error.query).to eq :destroy? expect(error.record).to eq comment expect(error.policy).to have_attributes( From fd74aa4eb00930b1f387ae13d5f0c45d461c774c Mon Sep 17 00:00:00 2001 From: Sjors Smits Date: Mon, 10 Jun 2024 16:23:35 +0200 Subject: [PATCH 2/9] update README.md to reflect the actual error message --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index a6807fd7..a3b007b2 100644 --- a/README.md +++ b/README.md @@ -116,7 +116,7 @@ and the given record. It then infers from the action name, that it should call ``` ruby unless PostPolicy.new(current_user, @post).update? - raise Pundit::NotAuthorizedError, "not allowed to update? this #{@post.inspect}" + raise Pundit::NotAuthorizedError, "not allowed to PostPolicy#update? this Post" end ``` From 7f70c66ba810f4a9aacb7d601204512e03b7b2e0 Mon Sep 17 00:00:00 2001 From: Sjors Smits Date: Mon, 10 Jun 2024 22:50:58 +0200 Subject: [PATCH 3/9] added handling for when a class is passed instead of an instance --- lib/pundit.rb | 8 +++++++- spec/pundit_spec.rb | 16 ++++++++++++++++ 2 files changed, 23 insertions(+), 1 deletion(-) diff --git a/lib/pundit.rb b/lib/pundit.rb index 741d4df1..84e9d0ec 100644 --- a/lib/pundit.rb +++ b/lib/pundit.rb @@ -36,7 +36,13 @@ def initialize(options = {}) @record = options[:record] @policy = options[:policy] - message = options.fetch(:message) { "not allowed to #{policy.class}##{query} this #{record.class}" } + record_name = if record.is_a?(Class) + record.name + else + "this #{record.class}" + end + + message = options.fetch(:message) { "not allowed to #{policy.class}##{query} #{record_name}" } end super(message) diff --git a/spec/pundit_spec.rb b/spec/pundit_spec.rb index e9760c97..bcec8633 100644 --- a/spec/pundit_spec.rb +++ b/spec/pundit_spec.rb @@ -90,6 +90,22 @@ # rubocop:enable Style/MultilineBlockChain end + it "raises an error with the policy, query and the class name when a Class is given" do + # rubocop:disable Style/MultilineBlockChain + expect do + Pundit.authorize(user, Post, :destroy?) + end.to raise_error(Pundit::NotAuthorizedError, "not allowed to PostPolicy#destroy? Post") do |error| + expect(error.query).to eq :destroy? + expect(error.record).to eq Post + expect(error.policy).to have_attributes( + user: user, + record: Post + ) + expect(error.policy).to be_a(PostPolicy) + end + # rubocop:enable Style/MultilineBlockChain + end + it "raises an error with a invalid policy constructor" do expect do Pundit.authorize(user, wiki, :update?) From f9d72d12b6167b7b8832a9f4db4aca96777577c9 Mon Sep 17 00:00:00 2001 From: Sjors Smits Date: Mon, 10 Jun 2024 22:55:53 +0200 Subject: [PATCH 4/9] sleeker formatting --- CHANGELOG.md | 2 ++ lib/pundit.rb | 6 +----- 2 files changed, 3 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 96530278..77d6dc3b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,8 @@ ## Unreleased +- + ## 2.3.2 (2024-05-08) - Refactor: First pass of Pundit::Context (#797) diff --git a/lib/pundit.rb b/lib/pundit.rb index 84e9d0ec..fb482dd1 100644 --- a/lib/pundit.rb +++ b/lib/pundit.rb @@ -36,11 +36,7 @@ def initialize(options = {}) @record = options[:record] @policy = options[:policy] - record_name = if record.is_a?(Class) - record.name - else - "this #{record.class}" - end + record_name = if record.is_a?(Class) then record.name else "this #{record.class}" end message = options.fetch(:message) { "not allowed to #{policy.class}##{query} #{record_name}" } end From 944844f7f803e1b0833c9052c9eab3f95a08a2d5 Mon Sep 17 00:00:00 2001 From: Sjors Smits Date: Mon, 10 Jun 2024 23:03:48 +0200 Subject: [PATCH 5/9] Update CHANGELOG.md and add updating the CHANGELOG.md as a step in CONTRIBUTING.md --- CHANGELOG.md | 3 ++- CONTRIBUTING.md | 1 + 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 77d6dc3b..882ef65d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,7 +2,8 @@ ## Unreleased -- +- Improve the `NotAuthorizedError` message to include the policy class. +Furthermore, in the case where the record passed is a class instead of an instance, the class name is given. ## 2.3.2 (2024-05-08) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index a44e1c24..09f2c76f 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -28,3 +28,4 @@ Pundit version, OS version and any stack traces you have are very valuable. - **Send coherent history**. Make sure each individual commit in your pull request is meaningful. If you had to make multiple intermediate commits while developing, please squash them before sending them to us. +- **Update the CHANGELOG.** Don't forget to add your new changes to the CHANGELOG. From e39bbf4ca922daac814ca0ada0cbb570fba45087 Mon Sep 17 00:00:00 2001 From: Sjors Smits Date: Tue, 11 Jun 2024 11:12:29 +0200 Subject: [PATCH 6/9] fix rubocop offense --- lib/pundit.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/pundit.rb b/lib/pundit.rb index fb482dd1..227f6e1a 100644 --- a/lib/pundit.rb +++ b/lib/pundit.rb @@ -36,7 +36,7 @@ def initialize(options = {}) @record = options[:record] @policy = options[:policy] - record_name = if record.is_a?(Class) then record.name else "this #{record.class}" end + record_name = record.is_a?(Class) ? record.name : "this #{record.class}" message = options.fetch(:message) { "not allowed to #{policy.class}##{query} #{record_name}" } end From b0a3bfde7ab1d9841c80aa6a953f2f1a6c853a68 Mon Sep 17 00:00:00 2001 From: Sjors Smits Date: Mon, 17 Jun 2024 13:38:07 +0200 Subject: [PATCH 7/9] Use `to_s` for better compatibility and dont do unnecessary work. Co-authored-by: Kim Burgestrand --- lib/pundit.rb | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/lib/pundit.rb b/lib/pundit.rb index 227f6e1a..a03f4d3a 100644 --- a/lib/pundit.rb +++ b/lib/pundit.rb @@ -36,9 +36,10 @@ def initialize(options = {}) @record = options[:record] @policy = options[:policy] - record_name = record.is_a?(Class) ? record.name : "this #{record.class}" - - message = options.fetch(:message) { "not allowed to #{policy.class}##{query} #{record_name}" } + message = options.fetch(:message) do + record_name = record.is_a?(Class) ? record.to_s : "this #{record.class}" + "not allowed to #{policy.class}##{query} #{record_name}" + end end super(message) From 4daf6e92fdb5e42659c570a27cddf5171f772acc Mon Sep 17 00:00:00 2001 From: Sjors Smits Date: Mon, 17 Jun 2024 13:38:29 +0200 Subject: [PATCH 8/9] Add PR link to CHANGELOG.md Co-authored-by: Kim Burgestrand --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 882ef65d..f699db45 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,7 +2,7 @@ ## Unreleased -- Improve the `NotAuthorizedError` message to include the policy class. +- Improve the `NotAuthorizedError` message to include the policy class. (#812) Furthermore, in the case where the record passed is a class instead of an instance, the class name is given. ## 2.3.2 (2024-05-08) From d84f5f0a11ad651b4c4427f89401abcae4ff2bef Mon Sep 17 00:00:00 2001 From: Sjors Smits Date: Mon, 17 Jun 2024 13:46:32 +0200 Subject: [PATCH 9/9] put link at end of changelog entry --- CHANGELOG.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f699db45..18ad81d0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,8 +2,8 @@ ## Unreleased -- Improve the `NotAuthorizedError` message to include the policy class. (#812) -Furthermore, in the case where the record passed is a class instead of an instance, the class name is given. +- Improve the `NotAuthorizedError` message to include the policy class. +Furthermore, in the case where the record passed is a class instead of an instance, the class name is given. (#812) ## 2.3.2 (2024-05-08)