Merge pull request #2713 from evazion/fix-2711

Prevent reverting to foreign versions (fixes #2711).

app/controllers/artist_commentaries_controller.rb

@@ -24,8 +24,8 @@ class ArtistCommentariesController < ApplicationController
   end
 
   def revert
-    @artist_commentary = ArtistCommentary.find_by_post_id(params[:id])
-    @version = ArtistCommentaryVersion.find(params[:version_id])
+    @artist_commentary = ArtistCommentary.find_by_post_id!(params[:id])
+    @version = @artist_commentary.versions.find(params[:version_id])
     @artist_commentary.revert_to!(@version)
     respond_with(@artist_commentary)
   end

app/controllers/artists_controller.rb

@@ -3,7 +3,7 @@ class ArtistsController < ApplicationController
   before_filter :member_only, :except => [:index, :show, :banned]
   before_filter :builder_only, :only => [:destroy]
   before_filter :admin_only, :only => [:ban, :unban]
-  before_filter :load_artist, :only => [:ban, :unban, :show, :edit, :update, :destroy, :undelete, :revert]
+  before_filter :load_artist, :only => [:ban, :unban, :show, :edit, :update, :destroy, :undelete]
 
   def new
     @artist = Artist.new_with_defaults(params)
@@ -97,7 +97,8 @@ class ArtistsController < ApplicationController
   end
 
   def revert
-    @version = ArtistVersion.find(params[:version_id])
+    @artist = Artist.find(params[:id])
+    @version = @artist.versions.find(params[:version_id])
     @artist.revert_to!(@version)
     respond_with(@artist)
   end

app/controllers/notes_controller.rb

@@ -54,7 +54,7 @@ class NotesController < ApplicationController
 
   def revert
     @note = Note.find(params[:id])
-    @version = NoteVersion.find(params[:version_id])
+    @version = @note.versions.find(params[:version_id])
     @note.revert_to!(@version)
     respond_with(@note)
   end

app/controllers/pools_controller.rb

@@ -79,7 +79,7 @@ class PoolsController < ApplicationController
 
   def revert
     @pool = Pool.find(params[:id])
-    @version = PoolVersion.find(params[:version_id])
+    @version = @pool.versions.find(params[:version_id])
     @pool.revert_to!(@version)
     flash[:notice] = "Pool reverted"
     respond_with(@pool) do |format|

app/controllers/posts_controller.rb

@@ -59,7 +59,7 @@ class PostsController < ApplicationController
 
   def revert
     @post = Post.find(params[:id])
-    @version = PostVersion.find(params[:version_id])
+    @version = @post.versions.find(params[:version_id])
 
     if @post.visible?
       @post.revert_to!(@version)

app/controllers/wiki_pages_controller.rb

@@ -67,7 +67,7 @@ class WikiPagesController < ApplicationController
 
   def revert
     @wiki_page = WikiPage.find(params[:id])
-    @version = WikiPageVersion.find(params[:version_id])
+    @version = @wiki_page.versions.find(params[:version_id])
     @wiki_page.revert_to!(@version)
     flash[:notice] = "Page was reverted"
     respond_with(@wiki_page)

app/models/artist.rb

@@ -1,4 +1,6 @@
 class Artist < ActiveRecord::Base
+  class RevertError < Exception ; end
+
   before_create :initialize_creator
   before_validation :normalize_name
   after_save :create_version
@@ -173,6 +175,10 @@ class Artist < ActiveRecord::Base
     end
 
     def revert_to!(version)
+      if id != version.artist_id
+        raise RevertError.new("You cannot revert to a previous version of another artist.")
+      end
+
       self.name = version.name
       self.url_string = version.url_string
       self.is_active = version.is_active

app/models/artist_commentary.rb

@@ -1,4 +1,6 @@
 class ArtistCommentary < ActiveRecord::Base
+  class RevertError < Exception ; end
+
   attr_accessor :remove_commentary_tag, :remove_commentary_request_tag, :remove_commentary_check_tag
   attr_accessor :add_commentary_tag, :add_commentary_request_tag, :add_commentary_check_tag
   attr_accessible :post_id, :original_description, :original_title, :translated_description, :translated_title, :remove_commentary_tag, :remove_commentary_request_tag, :add_commentary_tag, :add_commentary_request_tag, :add_commentary_check_tag, :remove_commentary_check_tag
@@ -76,6 +78,10 @@ class ArtistCommentary < ActiveRecord::Base
   end
 
   def revert_to(version)
+    if post_id != version.post_id
+      raise RevertError.new("You cannot revert to a previous artist commentary of another post.")
+    end
+
     self.original_description = version.original_description
     self.original_title = version.original_title
     self.translated_description = version.translated_description

app/models/note.rb

@@ -1,4 +1,6 @@
 class Note < ActiveRecord::Base
+  class RevertError < Exception ; end
+
   attr_accessor :updater_id, :updater_ip_addr, :html_id
   belongs_to :post
   belongs_to :creator, :class_name => "User"
@@ -204,6 +206,10 @@ class Note < ActiveRecord::Base
   end
 
   def revert_to(version)
+    if id != version.note_id
+      raise RevertError.new("You cannot revert to a previous version of another note.")
+    end
+
     self.x = version.x
     self.y = version.y
     self.post_id = version.post_id

app/models/pool.rb

@@ -1,6 +1,8 @@
 require 'ostruct'
 
 class Pool < ActiveRecord::Base
+  class RevertError < Exception ; end
+
   validates_uniqueness_of :name, :case_sensitive => false
   validates_format_of :name, :with => /\A[^,]+\Z/, :message => "cannot have commas"
   validates_inclusion_of :category, :in => %w(series collection)
@@ -194,6 +196,10 @@ class Pool < ActiveRecord::Base
   end
 
   def revert_to!(version)
+    if id != version.pool_id
+      raise RevertError.new("You cannot revert to a previous version of another pool.")
+    end
+
     self.post_ids = version.post_ids
     self.name = version.name
     synchronize!

app/models/post.rb

@@ -4,6 +4,7 @@ require 'google/apis/pubsub_v1'
 class Post < ActiveRecord::Base
   class ApprovalError < Exception ; end
   class DisapprovalError < Exception ; end
+  class RevertError < Exception ; end
   class SearchError < Exception ; end
 
   attr_accessor :old_tag_string, :old_parent_id, :old_source, :old_rating, :has_constraints, :disable_versioning, :view_count
@@ -1390,6 +1391,10 @@ class Post < ActiveRecord::Base
     end
 
     def revert_to(target)
+      if id != target.post_id
+        raise RevertError.new("You cannot revert to a previous version of another post.")
+      end
+
       self.tag_string = target.tags
       self.rating = target.rating
       self.source = target.source

app/models/wiki_page.rb

@@ -1,4 +1,6 @@
 class WikiPage < ActiveRecord::Base
+  class RevertError < Exception ; end
+
   before_save :normalize_title
   before_save :normalize_other_names
   before_validation :initialize_creator, :on => :create
@@ -127,6 +129,10 @@ class WikiPage < ActiveRecord::Base
   end
 
   def revert_to(version)
+    if id != version.wiki_page_id
+      raise RevertError.new("You cannot revert to a previous version of another wiki page.")
+    end
+
     self.title = version.title
     self.body = version.body
     self.is_locked = version.is_locked

test/factories/artist_commentary.rb

@@ -0,0 +1,9 @@
+FactoryGirl.define do
+  factory(:artist_commentary) do
+    post :factory => :post
+    original_title { FFaker::Lorem.sentences.join(" ") }
+    original_description { FFaker::Lorem.sentences.join(" ") }
+    translated_title { FFaker::Lorem.sentences.join(" ") }
+    translated_description { FFaker::Lorem.sentences.join(" ") }
+  end
+end

test/functional/artist_commentaries_controller_test.rb

@@ -0,0 +1,36 @@
+require 'test_helper'
+
+class ArtistCommentariesControllerTest < ActionController::TestCase
+  context "The artist commentaries controller" do
+    setup do
+      @user = FactoryGirl.create(:user)
+      CurrentUser.user = @user
+      CurrentUser.ip_addr = "127.0.0.1"
+    end
+
+    teardown do
+      CurrentUser.user = nil
+    end
+
+    context "revert action" do
+      setup do
+        @commentary1 = FactoryGirl.create(:artist_commentary)
+        @commentary2 = FactoryGirl.create(:artist_commentary)
+      end
+
+      should "return 404 when trying to revert a nonexistent commentary" do
+        post :revert, { :id => -1, :version_id => -1 }, {:user_id => @user.id}
+
+        assert_response 404
+      end
+
+      should "not allow reverting to a previous version of another artist commentary" do
+        post :revert, { :id => @commentary1.post_id, :version_id => @commentary2.versions(true).first.id }, {:user_id => @user.id}
+        @commentary1.reload
+
+        assert_not_equal(@commentary1.original_title, @commentary2.original_title)
+        assert_response :missing
+      end
+    end
+  end
+end

test/functional/artists_controller_test.rb

@@ -136,11 +136,23 @@ class ArtistsControllerTest < ActionController::TestCase
       end
     end
 
-    should "revert an artist" do
-      @artist.update_attributes(:name => "xyz")
-      @artist.update_attributes(:name => "abc")
-      version = @artist.versions.first
-      post :revert, {:id => @artist.id, :version_id => version.id}
+    context "reverting an artist" do
+      should "work" do
+        @artist.update_attributes(:name => "xyz")
+        @artist.update_attributes(:name => "abc")
+        version = @artist.versions.first
+        post :revert, {:id => @artist.id, :version_id => version.id}
+      end
+
+      should "not allow reverting to a previous version of another artist" do
+        @artist2 = FactoryGirl.create(:artist)
+
+        post :revert, { :id => @artist.id, :version_id => @artist2.versions(true).first.id }, {:user_id => @user.id}
+        @artist.reload
+
+        assert_not_equal(@artist.name, @artist2.name)
+        assert_response :missing
+      end
     end
 
     context "when finding an artist" do

test/functional/notes_controller_test.rb

@@ -77,6 +77,16 @@ class NotesControllerTest < ActionController::TestCase
         @note.reload
         assert_equal("000", @note.body)
       end
+
+      should "not allow reverting to a previous version of another note" do
+        @note2 = FactoryGirl.create(:note, :body => "note 2")
+
+        post :revert, { :id => @note.id, :version_id => @note2.versions(true).first.id }, {:user_id => @user.id}
+        @note.reload
+
+        assert_not_equal(@note.body, @note2.body)
+        assert_response :missing
+      end
     end
   end
 end

test/functional/pools_controller_test.rb

@@ -107,6 +107,16 @@ class PoolsControllerTest < ActionController::TestCase
         @pool.reload
         assert_equal([@post.id], @pool.post_id_array)
       end
+
+      should "not allow reverting to a previous version of another pool" do
+        @pool2 = FactoryGirl.create(:pool)
+
+        post :revert, { :id => @pool.id, :version_id => @pool2.versions(true).first.id }, {:user_id => @user.id}
+        @pool.reload
+
+        assert_not_equal(@pool.name, @pool2.name)
+        assert_response :missing
+      end
     end
   end
 end

test/functional/posts_controller_test.rb

@@ -127,6 +127,16 @@ class PostsControllerTest < ActionController::TestCase
         @post.reload
         assert_equal("aaaa", @post.tag_string)
       end
+
+      should "not allow reverting to a previous version of another post" do
+        @post2 = FactoryGirl.create(:post, :uploader_id => @user.id, :tag_string => "herp")
+
+        post :revert, { :id => @post.id, :version_id => @post2.versions.first.id }, {:user_id => @user.id}
+        @post.reload
+
+        assert_not_equal(@post.tag_string, @post2.tag_string)
+        assert_response :missing
+      end
     end
   end
 end

test/functional/wiki_pages_controller_test.rb

@@ -97,6 +97,16 @@ class WikiPagesControllerTest < ActionController::TestCase
         @wiki_page.reload
         assert_equal("1", @wiki_page.body)
       end
+
+      should "not allow reverting to a previous version of another wiki page" do
+        @wiki_page_2 = FactoryGirl.create(:wiki_page)
+
+        post :revert, { :id => @wiki_page.id, :version_id => @wiki_page_2.versions(true).first.id }, {:user_id => @user.id}
+        @wiki_page.reload
+
+        assert_not_equal(@wiki_page.body, @wiki_page_2.body)
+        assert_response :missing
+      end
     end
   end
 end