diff --git a/Changelog.md b/Changelog.md index 884e51348..ce7f4d2e7 100644 --- a/Changelog.md +++ b/Changelog.md @@ -61,6 +61,7 @@ Ruby 2.0 is no longer officially supported. * Improved logging source [#6041](https://github.com/diaspora/diaspora/pull/6041) * Gracefully handle duplicate entry while receiving share-visibility in parallel [#6068](https://github.com/diaspora/diaspora/pull/6068) * Update twitter gem to get rid of deprecation warnings [#6083](https://github.com/diaspora/diaspora/pull/6083) +* Refactor photos federation to get rid of some hacks [#6082](https://github.com/diaspora/diaspora/pull/6082) ## Bug fixes * Disable auto follow back on aspect deletion [#5846](https://github.com/diaspora/diaspora/pull/5846) @@ -93,6 +94,7 @@ Ruby 2.0 is no longer officially supported. * Only strip text direction codepoints around hashtags [#6067](https://github.com/diaspora/diaspora/issues/6067) * Fix selected week on admin weekly stats page [#6079](https://github.com/diaspora/diaspora/pull/6079) * Fix that some unread conversations may be hidden [#6060](https://github.com/diaspora/diaspora/pull/6060) +* Fix photo links in the mobile interface [#6082](https://github.com/diaspora/diaspora/pull/6082) ## Features * Hide post title of limited post in comment notification email [#5843](https://github.com/diaspora/diaspora/pull/5843) diff --git a/app/controllers/status_messages_controller.rb b/app/controllers/status_messages_controller.rb index f830fbf9b..e65b0a7c7 100644 --- a/app/controllers/status_messages_controller.rb +++ b/app/controllers/status_messages_controller.rb @@ -73,11 +73,6 @@ class StatusMessagesController < ApplicationController current_user.dispatch_post(@status_message, :url => short_post_url(@status_message.guid), :service_types => receiving_services) - #this is done implicitly, somewhere else, but it doesnt work, says max. :'( - @status_message.photos.each do |photo| - current_user.dispatch_post(photo) - end - current_user.participate!(@status_message) if coming_from_profile_page? && !own_profile_page? # if this is a post coming from a profile page diff --git a/app/models/status_message.rb b/app/models/status_message.rb index 1d7b26f86..4dacee42c 100644 --- a/app/models/status_message.rb +++ b/app/models/status_message.rb @@ -116,6 +116,7 @@ class StatusMessage < Post def update_and_dispatch_attached_photos(sender) if self.photos.any? + logger.info "dispatch photos for StatusMessage:#{guid}" Photo.where(status_message_guid: guid).update_all(:public => self.public) self.photos.each do |photo| if photo.pending diff --git a/db/migrate/20150607143809_fix_photo_public_flag.rb b/db/migrate/20150607143809_fix_photo_public_flag.rb new file mode 100644 index 000000000..fa75167ef --- /dev/null +++ b/db/migrate/20150607143809_fix_photo_public_flag.rb @@ -0,0 +1,9 @@ +class FixPhotoPublicFlag < ActiveRecord::Migration + def up + Photo.joins(:status_message).where(posts: {public: true}).update_all(public: true) + end + + def down + raise ActiveRecord::IrreversibleMigration + end +end diff --git a/db/schema.rb b/db/schema.rb index 90367fa20..fd77691b3 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -11,7 +11,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 20150531005120) do +ActiveRecord::Schema.define(version: 20150607143809) do create_table "account_deletions", force: :cascade do |t| t.string "diaspora_handle", limit: 255 diff --git a/lib/diaspora/federated/base.rb b/lib/diaspora/federated/base.rb index ff93dc61a..70f46dc79 100644 --- a/lib/diaspora/federated/base.rb +++ b/lib/diaspora/federated/base.rb @@ -23,11 +23,13 @@ module Diaspora module InstanceMethods def to_diaspora_xml + xml = to_xml + ::Logging::Logger["XMLLogger"].debug "to_xml: #{xml}" <<-XML - #{to_xml.to_s} + #{xml} - XML + XML end def x(input) diff --git a/lib/diaspora/federated/shareable.rb b/lib/diaspora/federated/shareable.rb index 26dad7db8..0b29da8d8 100644 --- a/lib/diaspora/federated/shareable.rb +++ b/lib/diaspora/federated/shareable.rb @@ -44,6 +44,16 @@ module Diaspora end end + # @return [void] + def receive_public + local_shareable = persisted_shareable + if local_shareable + update_existing_sharable(local_shareable) if verify_persisted_shareable(local_shareable) + else + save! + end + end + # The list of people that should receive this Shareable. # # @param [User] user The context, or dispatching user. diff --git a/lib/diaspora/parser.rb b/lib/diaspora/parser.rb index 62420f9b8..4142b542a 100644 --- a/lib/diaspora/parser.rb +++ b/lib/diaspora/parser.rb @@ -8,6 +8,7 @@ module Diaspora doc = Nokogiri::XML(xml) {|cfg| cfg.noblanks } return unless body = doc.xpath("/XML/post").children.first class_name = body.name.gsub("-", "/") + ::Logging::Logger["XMLLogger"].debug "from_xml: #{body}" begin class_name.camelize.constantize.from_xml body.to_s rescue NameError => e diff --git a/lib/postzord/receiver.rb b/lib/postzord/receiver.rb index deb44216f..ecdf02f6c 100644 --- a/lib/postzord/receiver.rb +++ b/lib/postzord/receiver.rb @@ -14,12 +14,19 @@ class Postzord::Receiver self.receive! end + private + def author_does_not_match_xml_author? - if (@author.diaspora_handle != xml_author) - logger.warn "event=receive status=abort reason='author in xml does not match retrieved person' " \ - "payload_type=#{@object.class} sender=#{@author.diaspora_handle}" - return true - end + return false unless @author.diaspora_handle != xml_author + logger.error "event=receive status=abort reason='author in xml does not match retrieved person' " \ + "type=#{@object.class} sender=#{@author.diaspora_handle}" + true + end + + def relayable_without_parent? + return false unless @object.respond_to?(:relayable?) && @object.parent.nil? + logger.error "event=receive status=abort reason='no corresponding post' type=#{@object.class} " \ + "sender=#{@author.diaspora_handle}" + true end end - diff --git a/lib/postzord/receiver/private.rb b/lib/postzord/receiver/private.rb index 98f211d91..378380b85 100644 --- a/lib/postzord/receiver/private.rb +++ b/lib/postzord/receiver/private.rb @@ -9,30 +9,28 @@ class Postzord::Receiver::Private < Postzord::Receiver @user_person = @user.person @salmon_xml = opts[:salmon_xml] - @sender = opts[:person] || Webfinger.new(self.salmon.author_id).fetch - @author = @sender + @author = opts[:person] || Webfinger.new(salmon.author_id).fetch @object = opts[:object] end def receive! - if @sender && self.salmon.verified_for_key?(@sender.public_key) + if @author && salmon.verified_for_key?(@author.public_key) parse_and_receive(salmon.parsed_data) else logger.error "event=receive status=abort reason='not_verified for key' " \ "recipient=#{@user.diaspora_handle} sender=#{@salmon.author_id}" end rescue => e - logger.error "failed to receive #{@object.class} from sender:#{@sender.id} for user:#{@user.id}: #{e.message}\n" \ + logger.error "failed to receive #{@object.class} from sender:#{@author.id} for user:#{@user.id}: #{e.message}\n" \ "#{@object.inspect}" raise e end def parse_and_receive(xml) @object ||= Diaspora::Parser.from_xml(xml) - return if @object.nil? - logger.info "user:#{@user.id} starting private receive from person:#{@sender.guid}" + logger.info "user:#{@user.id} starting private receive from person:#{@author.guid}" validate_object set_author! @@ -43,27 +41,17 @@ class Postzord::Receiver::Private < Postzord::Receiver def receive_object obj = @object.receive(@user, @author) Notification.notify(@user, obj, @author) if obj.respond_to?(:notification_type) - logger.info "user:#{@user.id} successfully received #{@object.class} from person #{@sender.guid}" \ + logger.info "user:#{@user.id} successfully received #{@object.class} from person #{@author.guid}" \ "#{": #{@object.guid}" if @object.respond_to?(:guid)}" logger.debug "received: #{@object.inspect}" end protected + def salmon @salmon ||= Salmon::EncryptedSlap.from_xml(@salmon_xml, @user) end - def validate_object - raise Diaspora::ContactRequiredUnlessRequest if contact_required_unless_request - raise Diaspora::RelayableObjectWithoutParent if relayable_without_parent? - - assign_sender_handle_if_request - - raise Diaspora::AuthorXMLAuthorMismatch if author_does_not_match_xml_author? - - @object - end - def xml_author if @object.respond_to?(:relayable?) #if A and B are friends, and A sends B a comment from C, we delegate the validation to the owner of the post being commented on @@ -84,19 +72,22 @@ class Postzord::Receiver::Private < Postzord::Receiver private - #validations - def relayable_without_parent? - if @object.respond_to?(:relayable?) && @object.parent.nil? - logger.error "event=receive status=abort reason='no corresponding post' type=#{@object.class} " \ - "recipient=#{@user_person.diaspora_handle} sender=#{@sender.diaspora_handle}" - return true - end + # validations + + def validate_object + raise Diaspora::XMLNotParseable if @object.nil? + raise Diaspora::ContactRequiredUnlessRequest if contact_required_unless_request + raise Diaspora::RelayableObjectWithoutParent if relayable_without_parent? + + assign_sender_handle_if_request + + raise Diaspora::AuthorXMLAuthorMismatch if author_does_not_match_xml_author? end def contact_required_unless_request - unless @object.is_a?(Request) || @user.contact_for(@sender) + unless @object.is_a?(Request) || @user.contact_for(@author) logger.error "event=receive status=abort reason='sender not connected to recipient' type=#{@object.class} " \ - "recipient=#{@user_person.diaspora_handle} sender=#{@sender.diaspora_handle}" + "recipient=#{@user_person.diaspora_handle} sender=#{@author.diaspora_handle}" return true end end @@ -104,7 +95,7 @@ class Postzord::Receiver::Private < Postzord::Receiver def assign_sender_handle_if_request #special casey if @object.is_a?(Request) - @object.sender_handle = @sender.diaspora_handle + @object.sender_handle = @author.diaspora_handle end end end diff --git a/lib/postzord/receiver/public.rb b/lib/postzord/receiver/public.rb index afd9622af..c96f0d340 100644 --- a/lib/postzord/receiver/public.rb +++ b/lib/postzord/receiver/public.rb @@ -9,8 +9,6 @@ class Postzord::Receiver::Public < Postzord::Receiver def initialize(xml) @salmon = Salmon::Slap.from_xml(xml) @author = Webfinger.new(@salmon.author_id).fetch - - logger.info "Receiving public object from person:#{@author.id}" end # @return [Boolean] @@ -23,7 +21,7 @@ class Postzord::Receiver::Public < Postzord::Receiver return unless verified_signature? # return false unless account_deletion_is_from_author - return unless save_object + parse_and_receive(@salmon.parsed_data) logger.info "received a #{@object.inspect}" if @object.is_a?(SignedRetraction) # feels like a hack @@ -51,15 +49,23 @@ class Postzord::Receiver::Public < Postzord::Receiver receiver.notify_users end - # @return [Object] - def save_object - @object = Diaspora::Parser.from_xml(@salmon.parsed_data) - raise Diaspora::XMLNotParseable if @object.nil? - raise Diaspora::NonPublic if object_can_be_public_and_it_is_not? - raise Diaspora::RelayableObjectWithoutParent if object_must_have_parent_and_does_not? - raise Diaspora::AuthorXMLAuthorMismatch if author_does_not_match_xml_author? - @object.save! if @object.respond_to?(:save!) - @object + # @return [void] + def parse_and_receive(xml) + @object = Diaspora::Parser.from_xml(xml) + + logger.info "starting public receive from person:#{@author.guid}" + + validate_object + receive_object + end + + # @return [void] + def receive_object + if @object.respond_to?(:receive_public) + @object.receive_public + elsif @object.respond_to?(:save!) + @object.save! + end end # @return [Array] User ids @@ -78,6 +84,15 @@ class Postzord::Receiver::Public < Postzord::Receiver private + # validations + + def validate_object + raise Diaspora::XMLNotParseable if @object.nil? + raise Diaspora::NonPublic if object_can_be_public_and_it_is_not? + raise Diaspora::RelayableObjectWithoutParent if relayable_without_parent? + raise Diaspora::AuthorXMLAuthorMismatch if author_does_not_match_xml_author? + end + def account_deletion_is_from_author return true unless @object.is_a?(AccountDeletion) return false if @object.diaspora_handle != @author.diaspora_handle @@ -88,12 +103,4 @@ class Postzord::Receiver::Public < Postzord::Receiver def object_can_be_public_and_it_is_not? @object.respond_to?(:public) && !@object.public? end - - def object_must_have_parent_and_does_not? - if @object.respond_to?(:relayable?) # comment, like - @object.parent.nil? - else - false - end - end end diff --git a/spec/lib/postzord/receiver/private_spec.rb b/spec/lib/postzord/receiver/private_spec.rb index 2e54f861c..717dbe8c1 100644 --- a/spec/lib/postzord/receiver/private_spec.rb +++ b/spec/lib/postzord/receiver/private_spec.rb @@ -18,7 +18,7 @@ describe Postzord::Receiver::Private do zord = Postzord::Receiver::Private.new(bob, :person => alice.person, :object => @alices_post) expect(zord.instance_variable_get(:@user)).not_to be_nil - expect(zord.instance_variable_get(:@sender)).not_to be_nil + expect(zord.instance_variable_get(:@author)).not_to be_nil expect(zord.instance_variable_get(:@object)).not_to be_nil end @@ -32,7 +32,7 @@ describe Postzord::Receiver::Private do zord = Postzord::Receiver::Private.new(bob, :salmon_xml => @salmon_xml) expect(zord.instance_variable_get(:@user)).not_to be_nil - expect(zord.instance_variable_get(:@sender)).not_to be_nil + expect(zord.instance_variable_get(:@author)).not_to be_nil expect(zord.instance_variable_get(:@salmon_xml)).not_to be_nil end end @@ -45,13 +45,13 @@ describe Postzord::Receiver::Private do context "does not parse and receive" do it "if the salmon author does not exist" do - @zord.instance_variable_set(:@sender, nil) + @zord.instance_variable_set(:@author, nil) expect(@zord).not_to receive(:parse_and_receive) @zord.receive! end it "if the author does not match the signature" do - @zord.instance_variable_set(:@sender, FactoryGirl.create(:person)) + @zord.instance_variable_set(:@author, FactoryGirl.create(:person)) expect(@zord).not_to receive(:parse_and_receive) @zord.receive! end diff --git a/spec/lib/postzord/receiver/public_spec.rb b/spec/lib/postzord/receiver/public_spec.rb index f4aadcc0f..090d67358 100644 --- a/spec/lib/postzord/receiver/public_spec.rb +++ b/spec/lib/postzord/receiver/public_spec.rb @@ -47,7 +47,7 @@ describe Postzord::Receiver::Public do it "does not save the object if signature is not verified" do expect(@receiver).to receive(:verified_signature?).and_return(false) - expect(@receiver).not_to receive(:save_object) + expect(@receiver).not_to receive(:parse_and_receive) @receiver.perform! end @@ -58,7 +58,7 @@ describe Postzord::Receiver::Public do end it 'saves the parsed object' do - expect(@receiver).to receive(:save_object) + expect(@receiver).to receive(:parse_and_receive).and_call_original @receiver.perform! end @@ -120,14 +120,15 @@ describe Postzord::Receiver::Public do end end - describe "#save_object" do + describe "#parse_and_receive" do before do @receiver = Postzord::Receiver::Public.new(@xml) + @parsed_salmon = Salmon::Slap.from_xml(@xml) end it "should raise a Diaspora::XMLNotParseable when the parsed object is nil" do expect(Diaspora::Parser).to receive(:from_xml).and_return(nil) - expect { @receiver.save_object }.to raise_error(Diaspora::XMLNotParseable) + expect { @receiver.parse_and_receive(@parsed_salmon.parsed_data) }.to raise_error(Diaspora::XMLNotParseable) end end end diff --git a/spec/lib/postzord/receiver_spec.rb b/spec/lib/postzord/receiver_spec.rb index a7dca6b1d..9dc2ceac7 100644 --- a/spec/lib/postzord/receiver_spec.rb +++ b/spec/lib/postzord/receiver_spec.rb @@ -2,7 +2,7 @@ # licensed under the Affero General Public License version 3 or later. See # the COPYRIGHT file. -require 'spec_helper' +require "spec_helper" describe Postzord::Receiver do before do @@ -14,10 +14,53 @@ describe Postzord::Receiver do allow(@receiver).to receive(:receive!).and_return(true) end - it 'calls receive!' do + it "calls receive!" do expect(@receiver).to receive(:receive!) @receiver.perform! end end -end + describe "#author_does_not_match_xml_author?" do + before do + @receiver.instance_variable_set(:@author, alice.person) + allow(@receiver).to receive(:xml_author).and_return(alice.diaspora_handle) + end + + it "should return false if the author matches" do + allow(@receiver).to receive(:xml_author).and_return(alice.diaspora_handle) + expect(@receiver.send(:author_does_not_match_xml_author?)).to be_falsey + end + + it "should return true if the author does not match" do + allow(@receiver).to receive(:xml_author).and_return(bob.diaspora_handle) + expect(@receiver.send(:author_does_not_match_xml_author?)).to be_truthy + end + end + + describe "#relayable_without_parent?" do + before do + @receiver.instance_variable_set(:@author, alice.person) + end + + it "should return false if object is not relayable" do + @receiver.instance_variable_set(:@object, nil) + expect(@receiver.send(:relayable_without_parent?)).to be_falsey + end + + context "if object is relayable" do + before do + @comment = bob.build_comment(text: "yo", post: FactoryGirl.create(:status_message)) + @receiver.instance_variable_set(:@object, @comment) + end + + it "should return false if object has parent" do + expect(@receiver.send(:relayable_without_parent?)).to be_falsey + end + + it "should return true if object has no parent" do + @comment.parent = nil + expect(@receiver.send(:relayable_without_parent?)).to be_truthy + end + end + end +end diff --git a/spec/models/photo_spec.rb b/spec/models/photo_spec.rb index 72bab80e9..7f4902c0d 100644 --- a/spec/models/photo_spec.rb +++ b/spec/models/photo_spec.rb @@ -277,4 +277,19 @@ describe Photo, :type => :model do }.to_not change(StatusMessage, :count) end end + + describe "#receive_public" do + it "updates the photo if it is already persisted" do + allow(@photo).to receive(:persisted_shareable).and_return(@photo2) + expect(@photo2).to receive(:update_attributes) + @photo.receive_public + end + + it "does not update the photo if the author mismatches" do + @photo.author = bob.person + allow(@photo).to receive(:persisted_shareable).and_return(@photo2) + expect(@photo).not_to receive(:update_existing_sharable) + @photo.receive_public + end + end end diff --git a/spec/models/poll_participation_spec.rb b/spec/models/poll_participation_spec.rb index 3caa16e25..d56d25e27 100644 --- a/spec/models/poll_participation_spec.rb +++ b/spec/models/poll_participation_spec.rb @@ -2,7 +2,6 @@ require 'spec_helper' require Rails.root.join("spec", "shared_behaviors", "relayable") describe PollParticipation, :type => :model do - before do @alices_aspect = alice.aspects.first @status = bob.post(:status_message, :text => "hello", :to => bob.aspects.first.id) @@ -98,7 +97,8 @@ describe PollParticipation, :type => :model do allow_any_instance_of(Person).to receive(:local?).and_return(false) expect{ salmon = Salmon::Slap.create_by_user_and_activity(alice, @poll_participation_alice.to_diaspora_xml).xml_for(@poll_participant) - Postzord::Receiver::Public.new(salmon).save_object + parsed_salmon = Salmon::Slap.from_xml(salmon) + Postzord::Receiver::Public.new(salmon).parse_and_receive(parsed_salmon.parsed_data) }.to_not raise_error end end diff --git a/spec/models/post_spec.rb b/spec/models/post_spec.rb index eeee9ffa0..00fbe98e2 100644 --- a/spec/models/post_spec.rb +++ b/spec/models/post_spec.rb @@ -336,6 +336,22 @@ describe Post, :type => :model do end end + describe "#receive_public" do + it "saves the post if the post is unknown" do + @post = FactoryGirl.create(:status_message, author: bob.person) + allow(@post).to receive(:persisted_shareable).and_return(nil) + expect(@post).to receive(:save!) + @post.receive_public + end + + it "does not update the post because not mutable" do + @post = FactoryGirl.create(:status_message, author: bob.person) + expect(@post).to receive(:update_existing_sharable).and_call_original + expect(@post).not_to receive(:update_attributes) + @post.receive_public + end + end + describe '#reshares_count' do before :each do @post = @user.post :status_message, :text => "hello", :to => @aspect.id, :public => true