FIX: Also check if user can see post or topic prior to letting them localize it (#36749)

There exists a `localization_guardian` that checks if a user can
localize based on settings like
- `content_localization_allowed_groups`
- `content_localization_allow_author_localization`

However, it missed out checking if the user can even see the model.

This commit fixes that by adding the checks. This issue was found as I was
adding a new model (`tags`) and discovered they were absent for the
older models.

This commit also introduces a small refactor that `.find`s the model first
on the controller and passes the object, so that the subsequent services
do not have to `.find` them again.

app/controllers/post_localizations_controller.rb

@@ -39,14 +39,17 @@ class PostLocalizationsController < ApplicationController
   def create_or_update
     post_id, locale, raw = params.require(%i[post_id locale raw])
 
-    guardian.ensure_can_localize_post!(post_id)
+    post = Post.find_by(id: post_id)
+    raise Discourse::NotFound unless post
 
-    localization = PostLocalization.find_by(post_id:, locale:)
+    guardian.ensure_can_localize_post!(post)
+
+    localization = PostLocalization.find_by(post_id: post.id, locale:)
     if localization
-      PostLocalizationUpdater.update(post_id: post_id, locale:, raw:, user: current_user)
+      PostLocalizationUpdater.update(post:, locale:, raw:, user: current_user)
       render json: success_json, status: :ok
     else
-      PostLocalizationCreator.create(post_id: post_id, locale:, raw:, user: current_user)
+      PostLocalizationCreator.create(post:, locale:, raw:, user: current_user)
       render json: success_json, status: :created
     end
   end
@@ -54,9 +57,12 @@ class PostLocalizationsController < ApplicationController
   def destroy
     post_id, locale = params.require(%i[post_id locale])
 
-    guardian.ensure_can_localize_post!(post_id)
+    post = Post.find_by(id: post_id)
+    raise Discourse::NotFound unless post
 
-    PostLocalizationDestroyer.destroy(post_id:, locale:, acting_user: current_user)
+    guardian.ensure_can_localize_post!(post)
+
+    PostLocalizationDestroyer.destroy(post:, locale:, acting_user: current_user)
     head :no_content
   end
 end

app/controllers/topic_localizations_controller.rb

@@ -6,19 +6,22 @@ class TopicLocalizationsController < ApplicationController
   def create_or_update
     topic_id, locale, title = params.require(%i[topic_id locale title])
 
-    guardian.ensure_can_localize_topic!(topic_id)
+    topic = Topic.find_by(id: topic_id)
+    raise Discourse::NotFound unless topic
 
-    topic_localization = TopicLocalization.find_by(topic_id: topic_id, locale: params[:locale])
+    guardian.ensure_can_localize_topic!(topic)
+
+    topic_localization = TopicLocalization.find_by(topic_id: topic.id, locale: params[:locale])
     if topic_localization
       TopicLocalizationUpdater.update(
-        topic_id: topic_id,
+        topic:,
         locale: params[:locale],
         title: params[:title],
         user: current_user,
       )
       render json: success_json, status: :ok
     else
-      TopicLocalizationCreator.create(topic_id:, locale:, title:, user: current_user)
+      TopicLocalizationCreator.create(topic:, locale:, title:, user: current_user)
       render json: success_json, status: :created
     end
   end
@@ -26,9 +29,12 @@ class TopicLocalizationsController < ApplicationController
   def destroy
     topic_id, locale = params.require(%i[topic_id locale])
 
-    guardian.ensure_can_localize_topic!(topic_id)
+    topic = Topic.find_by(id: topic_id)
+    raise Discourse::NotFound unless topic
 
-    TopicLocalizationDestroyer.destroy(topic_id:, locale:, acting_user: current_user)
+    guardian.ensure_can_localize_topic!(topic)
+
+    TopicLocalizationDestroyer.destroy(topic:, locale:, acting_user: current_user)
     head :no_content
   end
 end

lib/guardian/localization_guardian.rb

@@ -13,22 +13,26 @@ module LocalizationGuardian
     return false if !SiteSetting.content_localization_enabled
     return false if anonymous?
 
+    post = post_or_post_id.is_a?(Post) ? post_or_post_id : Post.find_by(id: post_or_post_id)
+    return false if !can_see_post?(post)
+
     return true if @user.in_any_groups?(SiteSetting.content_localization_allowed_groups_map)
     return false if !SiteSetting.content_localization_allow_author_localization
 
-    post = post_or_post_id.is_a?(Post) ? post_or_post_id : Post.find_by(id: post_or_post_id)
-    post&.user_id == @user.id
+    post.user_id == @user.id
   end
 
   def can_localize_topic?(topic_or_topic_id)
     return false if !SiteSetting.content_localization_enabled
     return false if anonymous?
 
+    topic =
+      topic_or_topic_id.is_a?(Topic) ? topic_or_topic_id : Topic.find_by(id: topic_or_topic_id)
+    return false if !can_see_topic?(topic)
+
     return true if @user.in_any_groups?(SiteSetting.content_localization_allowed_groups_map)
     return false if !SiteSetting.content_localization_allow_author_localization
 
-    topic =
-      topic_or_topic_id.is_a?(Topic) ? topic_or_topic_id : Topic.find_by(id: topic_or_topic_id)
-    topic&.user_id == @user.id
+    topic.user_id == @user.id
   end
 end

lib/post_localization_creator.rb

@@ -1,11 +1,8 @@
 # frozen_string_literal: true
 
 class PostLocalizationCreator
-  def self.create(post_id:, locale:, raw:, user:)
-    post = Post.find_by(id: post_id)
-
+  def self.create(post:, locale:, raw:, user:)
     Guardian.new(user).ensure_can_localize_post!(post)
-    raise Discourse::NotFound unless post
 
     localization =
       PostLocalization.create!(

lib/post_localization_destroyer.rb

@@ -1,15 +1,13 @@
 # frozen_string_literal: true
 
 class PostLocalizationDestroyer
-  def self.destroy(post_id:, locale:, acting_user:)
-    Guardian.new(acting_user).ensure_can_localize_post!(post_id)
+  def self.destroy(post:, locale:, acting_user:)
+    Guardian.new(acting_user).ensure_can_localize_post!(post)
 
-    localization = PostLocalization.find_by(post_id:, locale:)
+    localization = PostLocalization.find_by(post_id: post.id, locale:)
     raise Discourse::NotFound unless localization
 
     localization.destroy!
-
-    post = localization.post
-    post.publish_change_to_clients! :revised if post
+    post.publish_change_to_clients! :revised
   end
 end

lib/post_localization_updater.rb

@@ -1,13 +1,11 @@
 # frozen_string_literal: true
 
 class PostLocalizationUpdater
-  def self.update(post_id:, locale:, raw:, user:)
-    Guardian.new(user).ensure_can_localize_post!(post_id)
+  def self.update(post:, locale:, raw:, user:)
+    Guardian.new(user).ensure_can_localize_post!(post)
 
-    localization = PostLocalization.find_by(post_id: post_id, locale: locale)
+    localization = PostLocalization.find_by(post_id: post.id, locale: locale)
     raise Discourse::NotFound unless localization
-    post = localization.post
-    raise Discourse::NotFound unless post
 
     return localization if localization.raw == raw
 

lib/topic_localization_creator.rb

@@ -1,11 +1,11 @@
 # frozen_string_literal: true
 
 class TopicLocalizationCreator
-  def self.create(topic_id:, locale:, title:, user:)
-    Guardian.new(user).ensure_can_localize_topic!(topic_id)
+  def self.create(topic:, locale:, title:, user:)
+    Guardian.new(user).ensure_can_localize_topic!(topic)
 
     TopicLocalization.create!(
-      topic_id: topic_id,
+      topic_id: topic.id,
       locale: locale,
       title: title,
       fancy_title: Topic.fancy_title(title),

lib/topic_localization_destroyer.rb

@@ -1,10 +1,10 @@
 # frozen_string_literal: true
 
 class TopicLocalizationDestroyer
-  def self.destroy(topic_id:, locale:, acting_user:)
-    Guardian.new(acting_user).ensure_can_localize_topic!(topic_id)
+  def self.destroy(topic:, locale:, acting_user:)
+    Guardian.new(acting_user).ensure_can_localize_topic!(topic)
 
-    localization = TopicLocalization.find_by(topic_id: topic_id, locale: locale)
+    localization = TopicLocalization.find_by(topic_id: topic.id, locale: locale)
     raise Discourse::NotFound unless localization
 
     localization.destroy

lib/topic_localization_updater.rb

@@ -1,10 +1,10 @@
 # frozen_string_literal: true
 
 class TopicLocalizationUpdater
-  def self.update(topic_id:, locale:, title:, user:)
-    Guardian.new(user).ensure_can_localize_topic!(topic_id)
+  def self.update(topic:, locale:, title:, user:)
+    Guardian.new(user).ensure_can_localize_topic!(topic)
 
-    localization = TopicLocalization.find_by(topic_id: topic_id, locale: locale)
+    localization = TopicLocalization.find_by(topic_id: topic.id, locale: locale)
     raise Discourse::NotFound unless localization
 
     return localization if localization.title == title

spec/lib/guardian/localization_guardian_spec.rb

@@ -34,6 +34,32 @@ describe LocalizationGuardian do
       expect(Guardian.new(user).can_localize_post?(post)).to eq(false)
     end
 
+    context "when user cannot see the post" do
+      fab!(:private_category) { Fabricate(:private_category, group: Fabricate(:group)) }
+      fab!(:private_topic) { Fabricate(:topic, category: private_category) }
+      fab!(:private_post) { Fabricate(:post, topic: private_topic) }
+
+      before do
+        SiteSetting.content_localization_allowed_groups = "#{Group::AUTO_GROUPS[:everyone]}"
+      end
+
+      it "returns false for posts in private categories the user cannot access" do
+        expect(Guardian.new(user).can_localize_post?(private_post)).to eq(false)
+        expect(Guardian.new(user).can_localize_post?(private_post.id)).to eq(false)
+      end
+
+      it "returns false for posts in private messages the user is not part of" do
+        pm = Fabricate(:private_message_topic)
+        pm_post = Fabricate(:post, topic: pm)
+        expect(Guardian.new(user).can_localize_post?(pm_post)).to eq(false)
+        expect(Guardian.new(user).can_localize_post?(pm_post.id)).to eq(false)
+      end
+
+      it "returns false for non-existent posts" do
+        expect(Guardian.new(user).can_localize_post?(999_999_999)).to eq(false)
+      end
+    end
+
     context "when user is in allowed groups" do
       before { SiteSetting.content_localization_allowed_groups = "#{Group::AUTO_GROUPS[:admins]}" }
 
@@ -93,6 +119,30 @@ describe LocalizationGuardian do
       expect(Guardian.new(user).can_localize_topic?(topic)).to eq(false)
     end
 
+    context "when user cannot see the topic" do
+      fab!(:private_category) { Fabricate(:private_category, group: Fabricate(:group)) }
+      fab!(:private_topic) { Fabricate(:topic, category: private_category) }
+
+      before do
+        SiteSetting.content_localization_allowed_groups = "#{Group::AUTO_GROUPS[:everyone]}"
+      end
+
+      it "returns false for topics in private categories the user cannot access" do
+        expect(Guardian.new(user).can_localize_topic?(private_topic)).to eq(false)
+        expect(Guardian.new(user).can_localize_topic?(private_topic.id)).to eq(false)
+      end
+
+      it "returns false for private messages the user is not part of" do
+        pm = Fabricate(:private_message_topic)
+        expect(Guardian.new(user).can_localize_topic?(pm)).to eq(false)
+        expect(Guardian.new(user).can_localize_topic?(pm.id)).to eq(false)
+      end
+
+      it "returns false for non-existent topics" do
+        expect(Guardian.new(user).can_localize_topic?(999_999_999)).to eq(false)
+      end
+    end
+
     context "when user is in allowed groups" do
       before { SiteSetting.content_localization_allowed_groups = "#{Group::AUTO_GROUPS[:admins]}" }
 

spec/lib/post_localization_creator_spec.rb

@@ -15,7 +15,7 @@ describe PostLocalizationCreator do
   end
 
   it "creates a post localization record" do
-    localization = described_class.create(post_id: post.id, locale:, raw:, user:)
+    localization = described_class.create(post:, locale:, raw:, user:)
 
     expect(PostLocalization.find(localization.id)).to have_attributes(
       post_id: post.id,
@@ -27,17 +27,11 @@ describe PostLocalizationCreator do
   end
 
   it "enqueues ProcessLocalizedCook job" do
-    loc = described_class.create(post_id: post.id, locale:, raw:, user:)
+    loc = described_class.create(post:, locale:, raw:, user:)
 
     expect_job_enqueued(job: :process_localized_cooked, args: { post_localization_id: loc.id })
   end
 
-  it "raises not found if the post is missing" do
-    expect { described_class.create(post_id: -1, locale:, raw:, user:) }.to raise_error(
-      Discourse::NotFound,
-    )
-  end
-
   context "with author localization" do
     fab!(:author, :user)
     fab!(:author_post) { Fabricate(:post, user: author) }
@@ -46,7 +40,7 @@ describe PostLocalizationCreator do
     before { SiteSetting.content_localization_allow_author_localization = true }
 
     it "allows post author to create localization for their own post" do
-      localization = described_class.create(post_id: author_post.id, locale:, raw:, user: author)
+      localization = described_class.create(post: author_post, locale:, raw:, user: author)
 
       expect(localization).to have_attributes(
         post_id: author_post.id,
@@ -58,7 +52,7 @@ describe PostLocalizationCreator do
 
     it "raises permission error if user is not the post author" do
       expect {
-        described_class.create(post_id: other_post.id, locale:, raw:, user: author)
+        described_class.create(post: other_post, locale:, raw:, user: author)
       }.to raise_error(Discourse::InvalidAccess)
     end
   end

spec/lib/post_localization_destroyer_spec.rb

@@ -15,22 +15,22 @@ describe PostLocalizationDestroyer do
   end
 
   it "deletes the localization" do
-    expect {
-      described_class.destroy(post_id: post.id, locale: locale, acting_user: user)
-    }.to change { PostLocalization.count }.by(-1)
+    expect { described_class.destroy(post:, locale:, acting_user: user) }.to change {
+      PostLocalization.count
+    }.by(-1)
     expect { PostLocalization.find(localization.id) }.to raise_error(ActiveRecord::RecordNotFound)
   end
 
   it "raises not found if the localization is missing" do
-    expect {
-      described_class.destroy(post_id: post.id, locale: "nope", acting_user: user)
-    }.to raise_error(Discourse::NotFound)
+    expect { described_class.destroy(post:, locale: "nope", acting_user: user) }.to raise_error(
+      Discourse::NotFound,
+    )
   end
 
   it "publishes MessageBus notification" do
     messages =
       MessageBus.track_publish("/topic/#{post.topic_id}") do
-        described_class.destroy(post_id: post.id, locale: locale, acting_user: user)
+        described_class.destroy(post:, locale:, acting_user: user)
       end
 
     expect(messages.length).to eq(1)
@@ -46,15 +46,15 @@ describe PostLocalizationDestroyer do
 
     before { SiteSetting.content_localization_allow_author_localization = true }
 
-    it "allows post author to create localization for their own post" do
-      localization = described_class.destroy(post_id: author_post.id, locale:, acting_user: author)
+    it "allows post author to destroy localization for their own post" do
+      localization = described_class.destroy(post: author_post, locale:, acting_user: author)
 
       expect(localization).to be_nil
     end
 
     it "raises permission error if user is not the post author" do
       expect {
-        described_class.destroy(post_id: other_post.id, locale:, acting_user: author)
+        described_class.destroy(post: other_post, locale:, acting_user: author)
       }.to raise_error(Discourse::InvalidAccess)
     end
   end

spec/lib/post_localization_updater_spec.rb

@@ -18,8 +18,7 @@ describe PostLocalizationUpdater do
   end
 
   it "updates an existing localization" do
-    localization =
-      described_class.update(post_id: post.id, locale: locale, raw: new_raw, user: user)
+    localization = described_class.update(post:, locale:, raw: new_raw, user:)
 
     expect(localization).to have_attributes(
       raw: new_raw,
@@ -30,29 +29,22 @@ describe PostLocalizationUpdater do
   end
 
   it "returns the localization unchanged if the raw content is the same" do
-    localization =
-      described_class.update(post_id: post.id, locale:, raw: post_localization.raw, user:)
+    localization = described_class.update(post:, locale:, raw: post_localization.raw, user:)
 
     expect(localization.id).to eq(post_localization.id)
     expect(localization.localizer_user_id).not_to eq(user.id)
   end
 
   it "enqueues ProcessLocalizedCook job" do
-    loc = described_class.update(post_id: post.id, locale: locale, raw: new_raw, user: user)
+    loc = described_class.update(post:, locale:, raw: new_raw, user:)
 
     expect_job_enqueued(job: :process_localized_cooked, args: { post_localization_id: loc.id })
   end
 
   it "raises not found if the localization is missing" do
-    expect {
-      described_class.update(post_id: post.id, locale: "nope", raw: new_raw, user: user)
-    }.to raise_error(Discourse::NotFound)
-  end
-
-  it "raises not found if the post is missing" do
-    expect {
-      described_class.update(post_id: -1, locale: locale, raw: new_raw, user: user)
-    }.to raise_error(Discourse::NotFound)
+    expect { described_class.update(post:, locale: "nope", raw: new_raw, user:) }.to raise_error(
+      Discourse::NotFound,
+    )
   end
 
   context "with author localization" do
@@ -64,8 +56,7 @@ describe PostLocalizationUpdater do
     before { SiteSetting.content_localization_allow_author_localization = true }
 
     it "allows post author to create localization for their own post" do
-      localization =
-        described_class.update(post_id: author_post.id, locale:, raw: new_raw, user: author)
+      localization = described_class.update(post: author_post, locale:, raw: new_raw, user: author)
 
       expect(localization).to have_attributes(
         post_id: author_post.id,
@@ -77,7 +68,7 @@ describe PostLocalizationUpdater do
 
     it "raises permission error if user is not the post author" do
       expect {
-        described_class.update(post_id: other_post.id, locale:, raw: new_raw, user: author)
+        described_class.update(post: other_post, locale:, raw: new_raw, user: author)
       }.to raise_error(Discourse::InvalidAccess)
     end
   end

spec/lib/topic_localization_creator_spec.rb

@@ -15,7 +15,7 @@ describe TopicLocalizationCreator do
   end
 
   it "creates a topic localization record" do
-    localization = described_class.create(topic_id: topic.id, locale:, title:, user:)
+    localization = described_class.create(topic:, locale:, title:, user:)
 
     expect(TopicLocalization.find(localization.id)).to have_attributes(
       topic_id: topic.id,
@@ -28,7 +28,7 @@ describe TopicLocalizationCreator do
 
   it "raises permission error if user not in allowed groups" do
     group.remove(user)
-    expect { described_class.create(topic_id: topic.id, locale:, title:, user:) }.to raise_error(
+    expect { described_class.create(topic:, locale:, title:, user:) }.to raise_error(
       Discourse::InvalidAccess,
     )
   end
@@ -44,8 +44,7 @@ describe TopicLocalizationCreator do
     end
 
     it "allows topic author to create localization for their own topic" do
-      localization =
-        described_class.create(topic_id: author_topic.id, locale:, title:, user: author)
+      localization = described_class.create(topic: author_topic, locale:, title:, user: author)
 
       expect(localization).to have_attributes(
         topic_id: author_topic.id,
@@ -57,7 +56,7 @@ describe TopicLocalizationCreator do
 
     it "raises permission error if user is not the topic author" do
       expect {
-        described_class.create(topic_id: other_topic.id, locale:, title:, user: author)
+        described_class.create(topic: other_topic, locale:, title:, user: author)
       }.to raise_error(Discourse::InvalidAccess)
     end
   end

spec/lib/topic_localization_destroyer_spec.rb

@@ -15,23 +15,23 @@ describe TopicLocalizationDestroyer do
   end
 
   it "deletes the localization" do
-    expect {
-      described_class.destroy(topic_id: topic.id, locale: locale, acting_user: user)
-    }.to change { TopicLocalization.count }.by(-1)
+    expect { described_class.destroy(topic:, locale:, acting_user: user) }.to change {
+      TopicLocalization.count
+    }.by(-1)
     expect { TopicLocalization.find(localization.id) }.to raise_error(ActiveRecord::RecordNotFound)
   end
 
   it "raises not found if the localization is missing" do
-    expect {
-      described_class.destroy(topic_id: topic.id, locale: "nope", acting_user: user)
-    }.to raise_error(Discourse::NotFound)
+    expect { described_class.destroy(topic:, locale: "nope", acting_user: user) }.to raise_error(
+      Discourse::NotFound,
+    )
   end
 
   it "raises permission error if user not in allowed groups" do
     group.remove(user)
-    expect {
-      described_class.destroy(topic_id: topic.id, locale: locale, acting_user: user)
-    }.to raise_error(Discourse::InvalidAccess)
+    expect { described_class.destroy(topic:, locale:, acting_user: user) }.to raise_error(
+      Discourse::InvalidAccess,
+    )
   end
 
   context "with author localization" do
@@ -46,7 +46,7 @@ describe TopicLocalizationDestroyer do
 
     it "allows topic author to destroy localization for their own topic" do
       expect {
-        described_class.destroy(topic_id: author_topic.id, locale: "ja", acting_user: author)
+        described_class.destroy(topic: author_topic, locale: "ja", acting_user: author)
       }.to change { TopicLocalization.count }.by(-1)
       expect { TopicLocalization.find(author_localization.id) }.to raise_error(
         ActiveRecord::RecordNotFound,
@@ -54,9 +54,9 @@ describe TopicLocalizationDestroyer do
     end
 
     it "raises permission error if user is not the topic author" do
-      expect {
-        described_class.destroy(topic_id: topic.id, locale: locale, acting_user: author)
-      }.to raise_error(Discourse::InvalidAccess)
+      expect { described_class.destroy(topic:, locale:, acting_user: author) }.to raise_error(
+        Discourse::InvalidAccess,
+      )
     end
   end
 end

spec/lib/topic_localization_updater_spec.rb

@@ -18,8 +18,7 @@ describe TopicLocalizationUpdater do
   end
 
   it "updates an existing localization" do
-    localization =
-      described_class.update(topic_id: topic.id, locale: locale, title: new_title, user: user)
+    localization = described_class.update(topic:, locale:, title: new_title, user:)
 
     expect(localization).to have_attributes(
       title: new_title,
@@ -29,8 +28,7 @@ describe TopicLocalizationUpdater do
   end
 
   it "returns the localization unchanged if the title is the same" do
-    localization =
-      described_class.update(topic_id: topic.id, locale:, title: topic_localization.title, user:)
+    localization = described_class.update(topic:, locale:, title: topic_localization.title, user:)
 
     expect(localization.id).to eq(topic_localization.id)
     expect(localization.localizer_user_id).not_to eq(user.id)
@@ -38,21 +36,15 @@ describe TopicLocalizationUpdater do
 
   it "raises not found if the localization is missing" do
     expect {
-      described_class.update(topic_id: topic.id, locale: "nope", title: new_title, user: user)
-    }.to raise_error(Discourse::NotFound)
-  end
-
-  it "raises not found if the topic is missing" do
-    expect {
-      described_class.update(topic_id: -1, locale: locale, title: new_title, user: user)
+      described_class.update(topic:, locale: "nope", title: new_title, user:)
     }.to raise_error(Discourse::NotFound)
   end
 
   it "raises permission error if user not in allowed groups" do
     group.remove(user)
-    expect {
-      described_class.update(topic_id: topic.id, locale: locale, title: new_title, user: user)
-    }.to raise_error(Discourse::InvalidAccess)
+    expect { described_class.update(topic:, locale:, title: new_title, user:) }.to raise_error(
+      Discourse::InvalidAccess,
+    )
   end
 
   context "with author localization" do
@@ -69,12 +61,7 @@ describe TopicLocalizationUpdater do
 
     it "allows topic author to update localization for their own topic" do
       localization =
-        described_class.update(
-          topic_id: author_topic.id,
-          locale: "ja",
-          title: new_title,
-          user: author,
-        )
+        described_class.update(topic: author_topic, locale: "ja", title: new_title, user: author)
 
       expect(localization).to have_attributes(
         title: new_title,
@@ -85,7 +72,7 @@ describe TopicLocalizationUpdater do
 
     it "raises permission error if user is not the topic author" do
       expect {
-        described_class.update(topic_id: topic.id, locale: locale, title: new_title, user: author)
+        described_class.update(topic:, locale:, title: new_title, user: author)
       }.to raise_error(Discourse::InvalidAccess)
     end
   end