Guardian before and after checks api

Development

AI-generated summary

Angus proposes adding a new server-side plugin API to allow modifying the outcome of Guardian can_* checks via before and after hooks angus. The goal is to enable plugins to alter check results, such as preventing edits on remote posts. pmusaraj raises concerns about security and potential conflicts with multiple plugins using after hooks. Angus acknowledges the risks and suggests limiting the PR to before checks only, which would address immediate needs while reducing complexity angus. pmusaraj agrees to proceed with just the before checks.

1

Something I’ve come across a few times when building plugins is the need to modify the outcome of Guardian can_* checks. I’ve come across it again in the ActivityPub Plugin:

https://github.com/discourse/discourse-activity-pub/blob/main/extensions/discourse_activity_pub_guardian_extension.rb

I’ve just raised a draft PR that adds a new server-side plugin api method that allows you to register before and after checks to guardian can_* methods, affording the ability to change the outcome of the method. For example

add_guardian_check(:before, :edit_post) do |guardian, result, post|
  !post.activity_pub_remote?
end

https://github.com/discourse/discourse/pull/26936

Curious to get feedback on both the approach and the execution before publishing it for review.

2

Just bumping this one back up for whomever is relevant. cc @pmusaraj

3

Thanks, Angus!

I don’t see any issues with the before_* register. The after_* register is a little trickier. Security-wise, the after_* register means plugins can override core in ways that may be unsafe. Obviously plugins can do this in all sorts of ways, but the plugin API shouldn’t facilitate it even further.

Also, what happens if multiple plugins consume the after_* hook? Which one wins?

4

Yeah, I understand what you mean.

The checks accept a priority argument. See this spec

it "respects check priority" do
  plugin.add_guardian_check(:after, :edit_post, 2) { false }
  plugin.add_guardian_check(:after, :edit_post, 0) { true }
  plugin.add_guardian_check(:after, :edit_post, 1) { false }
  expect(Guardian.new(user).can_edit_post?(post)).to be_truthy
end

How about I limit this PR to just before checks? That would serve the immediate needs and reduce the variables in play a bit here.

5

Yes, sure, let’s start with just the before checks. Thanks!