Merge branch 'release/0.7.1.0' into next-minor

This commit is contained in:
Benjamin Neff 2017-10-17 01:39:39 +02:00
commit 1ac0dfbbbc
No known key found for this signature in database
GPG key ID: 971464C3F1A90194
20 changed files with 314 additions and 88 deletions

View file

@ -8,6 +8,13 @@
# 0.7.1.0
## Ensure account deletions are run
There were some issues causing accounts deletions to not properly perform in some cases, see
[#7631](https://github.com/diaspora/diaspora/issues/7631) and [#7639](https://github.com/diaspora/diaspora/pull/7639).
To ensure these are reexecuted properly, please run `RAILS_ENV=production bin/rake migrations:run_account_deletions`
after you've upgraded.
## Refactor
* Remove title from profile photo upload button [#7551](https://github.com/diaspora/diaspora/pull/7551)
* Remove Internet Explorer workarounds [#7557](https://github.com/diaspora/diaspora/pull/7557)
@ -22,6 +29,7 @@
* Remove `rails_admin_histories` table [#7597](https://github.com/diaspora/diaspora/pull/7597)
* Optimize memory usage on profile export [#7627](https://github.com/diaspora/diaspora/pull/7627)
* Limit the number of parallel exports [#7629](https://github.com/diaspora/diaspora/pull/7629)
* Reduce memory usage for account deletion [#7639](https://github.com/diaspora/diaspora/pull/7639)
## Bug fixes
* Fix displaying polls with long answers [#7579](https://github.com/diaspora/diaspora/pull/7579)
@ -34,6 +42,12 @@
* Fix local migration run without old private key [#7558](https://github.com/diaspora/diaspora/pull/7558)
* Fix export not downloadable because the filename was resetted on access [#7622](https://github.com/diaspora/diaspora/pull/7622)
* Delete invalid oEmbed caches with binary titles [#7620](https://github.com/diaspora/diaspora/pull/7620)
* Delete invalid diaspora IDs from friendica [#7630](https://github.com/diaspora/diaspora/pull/7630)
* Cleanup relayables where the signature is missing [#7637](https://github.com/diaspora/diaspora/pull/7637)
* Avoid page to jump to top after a post deletion [#7638](https://github.com/diaspora/diaspora/pull/7638)
* Handle duplicate account deletions [#7639](https://github.com/diaspora/diaspora/pull/7639)
* Handle duplicate account migrations [#7641](https://github.com/diaspora/diaspora/pull/7641)
* Handle bugs related to missing users [#7632](https://github.com/diaspora/diaspora/pull/7632)
## Features
* Ask for confirmation when leaving a submittable comment field [#7530](https://github.com/diaspora/diaspora/pull/7530)

View file

@ -78,7 +78,8 @@ app.views.PostControls = app.views.Base.extend({
}.bind(this));
},
destroyModel: function() {
destroyModel: function(evt) {
if (evt) { evt.preventDefault(); }
this.post.destroyModel();
}
});

View file

@ -5,16 +5,13 @@
# the COPYRIGHT file.
class AspectMembership < ApplicationRecord
belongs_to :aspect
belongs_to :contact
has_one :user, :through => :contact
has_one :person, :through => :contact
before_destroy do
if self.contact && self.contact.aspects.size == 1
self.user.disconnect(self.contact)
end
user&.disconnect(contact) if contact&.aspects&.size == 1
true
end

View file

@ -6,7 +6,7 @@
#
class Notification < ApplicationRecord
belongs_to :recipient, class_name: "User"
has_many :notification_actors, dependent: :destroy
has_many :notification_actors, dependent: :delete_all
has_many :actors, class_name: "Person", through: :notification_actors, source: :person
belongs_to :target, polymorphic: true

View file

@ -5,8 +5,6 @@
# the COPYRIGHT file.
class NotificationActor < ApplicationRecord
belongs_to :notification
belongs_to :person
end

View file

@ -90,6 +90,10 @@ class User < ApplicationRecord
after_save :remove_invalid_unconfirmed_emails
before_destroy do
raise "Never destroy users!"
end
def self.all_sharing_with_person(person)
User.joins(:contacts).where(:contacts => {:person_id => person.id})
end

View file

@ -34,6 +34,7 @@ class User
logger.info "event=disconnect user=#{diaspora_handle} target=#{contact.person.diaspora_handle}"
if contact.person.local?
raise "FATAL: user entry is missing from the DB. Aborting" if contact.person.owner.nil?
contact.person.owner.disconnected_by(contact.user.person)
else
ContactRetraction.for(contact).defer_dispatch(self)

View file

@ -0,0 +1,25 @@
# frozen_string_literal: true
class CleanupInvalidDiasporaIds < ActiveRecord::Migration[5.1]
def up
ids = Person.where("diaspora_handle LIKE '%@%/%'").ids
return if ids.empty?
AspectMembership.joins(:contact).where(contacts: {person_id: ids}).delete_all
Person.where(id: ids).each do |person|
destroy_notifications_for_person(person)
person.destroy
end
end
def destroy_notifications_for_person(person)
Notification.joins(:notification_actors).where(notification_actors: {person_id: person.id}).each do |notification|
if notification.notification_actors.count > 1
notification.notification_actors.where(person_id: person.id).delete_all
else
notification.destroy
end
end
end
end

View file

@ -0,0 +1,81 @@
# frozen_string_literal: true
class CleanupRelayablesWithoutSignature < ActiveRecord::Migration[5.1]
class Comment < ApplicationRecord
belongs_to :commentable, polymorphic: true
before_destroy do
Like.where(target_type: "Comment", target_id: id).destroy_all
ActsAsTaggableOn::Tagging.where(taggable_type: "Comment", taggable_id: id).destroy_all
end
after_destroy do
commentable.update_comments_counter
end
end
class Like < ApplicationRecord
belongs_to :target, polymorphic: true
has_one :signature, class_name: "LikeSignature", dependent: :delete
after_destroy do
target.update_likes_counter
end
end
class PollParticipation < ApplicationRecord
belongs_to :poll_answer, counter_cache: :vote_count
end
def up
cleanup_comments
cleanup_likes
cleanup_poll_participations
end
def cleanup_comments
Comment.joins("INNER JOIN posts as post ON post.id = comments.commentable_id AND " \
"comments.commentable_type = 'Post'")
.joins("INNER JOIN people as comment_author ON comment_author.id = comments.author_id")
.joins("INNER JOIN people as post_author ON post_author.id = post.author_id")
.where("comment_author.owner_id IS NULL AND post_author.owner_id IS NOT NULL " \
"AND NOT EXISTS(" \
"SELECT NULL FROM comment_signatures WHERE comment_signatures.comment_id = comments.id" \
")")
.destroy_all
end
def cleanup_likes
Like.joins("INNER JOIN posts as post ON post.id = likes.target_id AND likes.target_type = 'Post'")
.joins("INNER JOIN people as like_author ON like_author.id = likes.author_id")
.joins("INNER JOIN people as post_author ON post_author.id = post.author_id")
.where("like_author.owner_id IS NULL AND post_author.owner_id IS NOT NULL " \
"AND NOT EXISTS(" \
"SELECT NULL FROM like_signatures WHERE like_signatures.like_id = likes.id" \
")")
.destroy_all
Like.joins("INNER JOIN comments as comment ON comment.id = likes.target_id AND likes.target_type = 'Comment'")
.joins("INNER JOIN posts as post ON post.id = comment.commentable_id AND comment.commentable_type = 'Post'")
.joins("INNER JOIN people as like_author ON like_author.id = likes.author_id")
.joins("INNER JOIN people as post_author ON post_author.id = post.author_id")
.where("like_author.owner_id IS NULL AND post_author.owner_id IS NOT NULL " \
"AND NOT EXISTS(" \
"SELECT NULL FROM like_signatures WHERE like_signatures.like_id = likes.id" \
")")
.destroy_all
end
def cleanup_poll_participations
PollParticipation.joins("INNER JOIN polls as poll ON poll.id = poll_participations.poll_id")
.joins("INNER JOIN posts as post ON post.id = poll.status_message_id")
.joins("INNER JOIN people as pp_author ON pp_author.id = poll_participations.author_id")
.joins("INNER JOIN people as post_author ON post_author.id = post.author_id")
.where("pp_author.owner_id IS NULL AND post_author.owner_id IS NOT NULL " \
"AND NOT EXISTS(" \
"SELECT NULL FROM poll_participation_signatures " \
"WHERE poll_participation_signatures.poll_participation_id = poll_participations.id" \
")")
.destroy_all
end
end

View file

@ -0,0 +1,44 @@
# frozen_string_literal: true
class CleanupInvalidContacts < ActiveRecord::Migration[5.1]
class Contact < ApplicationRecord
belongs_to :user
belongs_to :person
has_many :aspect_memberships, dependent: :delete_all
before_destroy :destroy_notifications
def destroy_notifications
Notification.where(
target_type: "Person",
target_id: person_id,
recipient_id: user_id,
type: "Notifications::StartedSharing"
).destroy_all
end
end
class User < ApplicationRecord
end
class Person < ApplicationRecord
belongs_to :owner, class_name: "User", optional: true
end
class Notification < ApplicationRecord
self.inheritance_column = nil
has_many :notification_actors, dependent: :delete_all
end
class NotificationActor < ApplicationRecord
end
class AspectMembership < ApplicationRecord
end
def up
Contact.left_outer_joins(:user).where("users.id is NULL").destroy_all
Contact.left_outer_joins(person: :owner).where("people.owner_id is NOT NULL").where("users.id is NULL").destroy_all
end
end

View file

@ -5,7 +5,6 @@
# the COPYRIGHT file.
class AccountDeleter
# Things that are not removed from the database:
# - Comments
# - Likes
@ -25,17 +24,14 @@ class AccountDeleter
end
def perform!
ActiveRecord::Base.transaction do
#person
delete_standard_person_associations
remove_conversation_visibilities
delete_contacts_of_me
tombstone_person_and_profile
# close person
delete_standard_person_associations
delete_contacts_of_me
tombstone_person_and_profile
close_user if user
close_user if user
mark_account_deletion_complete
end
mark_account_deletion_complete
end
# user deletion methods
@ -46,30 +42,29 @@ class AccountDeleter
tombstone_user
end
#user deletions
# user deletions
def normal_ar_user_associates_to_delete
%i[tag_followings services aspects user_preferences
notifications blocks authorizations o_auth_applications pairwise_pseudonymous_identifiers]
end
def special_ar_user_associations
%i[person profile contacts auto_follow_back_aspect]
end
def ignored_ar_user_associations
%i[followed_tags invited_by invited_users contact_people aspect_memberships
ignored_people share_visibilities conversation_visibilities conversations reports]
end
def delete_standard_user_associations
normal_ar_user_associates_to_delete.each do |asso|
self.user.send(asso).each{|model| model.destroy }
user.send(asso).ids.each_slice(20) do |ids|
User.reflect_on_association(asso).class_name.constantize.where(id: ids).destroy_all
end
end
end
def normal_ar_person_associates_to_delete
%i[posts photos mentions participations roles blocks conversation_visibilities]
end
def delete_standard_person_associations
normal_ar_person_associates_to_delete.each do |asso|
self.person.send(asso).destroy_all
person.send(asso).ids.each_slice(20) do |ids|
Person.reflect_on_association(asso).class_name.constantize.where(id: ids).destroy_all
end
end
end
@ -80,33 +75,20 @@ class AccountDeleter
# Currently this would get deleted due to the db foreign key constrainsts,
# but we'll keep this method here for completeness
def remove_share_visibilities_on_contacts_posts
ShareVisibility.for_a_user(user).destroy_all
end
def remove_conversation_visibilities
ConversationVisibility.where(:person_id => person.id).destroy_all
ShareVisibility.for_a_user(user).find_each(batch_size: 20, &:destroy)
end
def tombstone_person_and_profile
self.person.lock_access!
self.person.clear_profile!
person.lock_access!
person.clear_profile!
end
def tombstone_user
self.user.clear_account!
user.clear_account!
end
def delete_contacts_of_me
Contact.all_contacts_of_person(self.person).destroy_all
end
def normal_ar_person_associates_to_delete
%i[posts photos mentions participations roles blocks]
end
def ignored_or_special_ar_person_associations
%i[comments likes poll_participations contacts notification_actors notifications owner profile
conversation_visibilities pod conversations messages]
Contact.all_contacts_of_person(person).find_each(batch_size: 20, &:destroy)
end
def mark_account_deletion_complete

View file

@ -10,15 +10,22 @@ module Diaspora
end
def self.account_deletion(entity)
AccountDeletion.create!(person: author_of(entity))
person = author_of(entity)
AccountDeletion.create!(person: person) unless AccountDeletion.where(person: person).exists?
rescue => e # rubocop:disable Lint/RescueWithoutErrorClass
raise e unless AccountDeletion.where(person: person).exists?
logger.warn "ignoring error on receive AccountDeletion:#{entity.author}: #{e.class}: #{e.message}"
end
def self.account_migration(entity)
old_person = author_of(entity)
profile = profile(entity.profile)
AccountMigration.create!(
old_person: Person.by_account_identifier(entity.author),
new_person: profile.person
)
return if AccountMigration.where(old_person: old_person, new_person: profile.person).exists?
AccountMigration.create!(old_person: old_person, new_person: profile.person)
rescue => e # rubocop:disable Lint/RescueWithoutErrorClass
raise e unless AccountMigration.where(old_person: old_person, new_person: profile.person).exists?
logger.warn "ignoring error on receive #{entity}: #{e.class}: #{e.message}"
nil
end
def self.comment(entity)

View file

@ -59,4 +59,15 @@ namespace :migrations do
queue.clear
end
end
desc "Run uncompleted account deletions"
task run_account_deletions: :environment do
if AccountDeletion.uncompleted.count > 0
puts "Running account deletions.."
AccountDeletion.uncompleted.find_each(&:perform!)
puts "OK."
else
puts "No acccount deletions to run."
end
end
end

View file

@ -29,7 +29,7 @@ describe PostsController, type: :controller do
msg = alice.post(:status_message, text: "Mention @{User ; #{user.diaspora_handle}}", public: true)
expect(msg.mentioned_people.count).to eq(1)
user.destroy
user.close_account!
get :show, params: {id: msg.id}
expect(response).to be_success

View file

@ -58,11 +58,12 @@ describe "Receive federation messages feature" do
expect(AccountMigration.find_by(old_person: sender.person, new_person: new_user.person)).to be_performed
end
it "doesn't accept the same migration for the second time" do
it "doesn't run the same migration for the second time" do
run_migration
expect {
run_migration
}.to raise_error(ActiveRecord::RecordInvalid)
expect_any_instance_of(AccountMigration).not_to receive(:perform!)
run_migration
expect(AccountMigration.where(old_person: sender.person, new_person: new_user.person).count).to eq(1)
expect(AccountMigration.find_by(old_person: sender.person, new_person: new_user.person)).to be_performed
end
it "doesn't accept second migration for the same sender" do

View file

@ -20,7 +20,6 @@ describe AccountDeleter do
delete_contacts_of_me
delete_standard_person_associations
tombstone_person_and_profile
remove_conversation_visibilities
]
context "user deletion" do
@ -96,8 +95,12 @@ describe AccountDeleter do
it 'removes all standard user associaltions' do
@account_deletion.normal_ar_user_associates_to_delete.each do |asso|
association_double = double
expect(association_double).to receive(:destroy)
expect(bob).to receive(asso).and_return([association_double])
expect(association_double).to receive(:ids).and_return([42])
expect(bob).to receive(asso).and_return(association_double)
batch_double = double
expect(User.reflect_on_association(asso).class_name.constantize).to receive(:where)
.with(id: [42]).and_return(batch_double)
expect(batch_double).to receive(:destroy_all)
end
@account_deletion.delete_standard_user_associations
@ -111,8 +114,12 @@ describe AccountDeleter do
it 'removes all standard person associaltions' do
@account_deletion.normal_ar_person_associates_to_delete.each do |asso|
association_double = double
expect(association_double).to receive(:destroy_all)
expect(association_double).to receive(:ids).and_return([42])
expect(bob.person).to receive(asso).and_return(association_double)
batch_double = double
expect(Person.reflect_on_association(asso).class_name.constantize).to receive(:where)
.with(id: [42]).and_return(batch_double)
expect(batch_double).to receive(:destroy_all)
end
@account_deletion.delete_standard_person_associations
@ -133,7 +140,7 @@ describe AccountDeleter do
it 'deletes all the local contact objects where deleted account is the person' do
contacts = double
expect(Contact).to receive(:all_contacts_of_person).with(bob.person).and_return(contacts)
expect(contacts).to receive(:destroy_all)
expect(contacts).to receive(:find_each).with(batch_size: 20)
@account_deletion.delete_contacts_of_me
end
end
@ -149,21 +156,13 @@ describe AccountDeleter do
@account_deletion.tombstone_person_and_profile
end
end
describe "#remove_conversation_visibilities" do
it "removes the conversation visibility for the deleted user" do
vis = double
expect(ConversationVisibility).to receive(:where).with(hash_including(:person_id => bob.person.id)).and_return(vis)
expect(vis).to receive(:destroy_all)
@account_deletion.remove_conversation_visibilities
end
end
end
describe "#remove_share_visibilities_by_contacts_of_user" do
it "removes the share visibilities for a user" do
s_vis = double
expect(ShareVisibility).to receive(:for_a_user).with(bob).and_return(s_vis)
expect(s_vis).to receive(:destroy_all)
expect(s_vis).to receive(:find_each).with(batch_size: 20)
@account_deletion.remove_share_visibilities_on_contacts_posts
end
@ -176,14 +175,20 @@ describe AccountDeleter do
end
end
it 'has all user association keys accounted for' do
all_keys = (@account_deletion.normal_ar_user_associates_to_delete + @account_deletion.special_ar_user_associations + @account_deletion.ignored_ar_user_associations)
expect(all_keys.sort{|x, y| x.to_s <=> y.to_s}).to eq(User.reflections.keys.sort{|x, y| x.to_s <=> y.to_s}.map(&:to_sym))
it "has all user association keys accounted for" do
special_ar_user_associations = %i[person profile contacts auto_follow_back_aspect]
ignored_ar_user_associations = %i[followed_tags invited_by invited_users contact_people aspect_memberships
ignored_people share_visibilities conversation_visibilities conversations reports]
all_keys = @account_deletion.normal_ar_user_associates_to_delete +
special_ar_user_associations + ignored_ar_user_associations
expect(all_keys.sort_by(&:to_s)).to eq(User.reflections.keys.sort_by(&:to_s).map(&:to_sym))
end
it 'has all person association keys accounted for' do
all_keys = (@account_deletion.normal_ar_person_associates_to_delete + @account_deletion.ignored_or_special_ar_person_associations)
expect(all_keys.sort{|x, y| x.to_s <=> y.to_s}).to eq(Person.reflections.keys.sort{|x, y| x.to_s <=> y.to_s}.map(&:to_sym))
it "has all person association keys accounted for" do
ignored_or_special_ar_person_associations = %i[comments likes poll_participations contacts notification_actors
notifications owner profile pod conversations messages]
all_keys = @account_deletion.normal_ar_person_associates_to_delete + ignored_or_special_ar_person_associations
expect(all_keys.sort_by(&:to_s)).to eq(Person.reflections.keys.sort_by(&:to_s).map(&:to_sym))
end
end

View file

@ -12,6 +12,27 @@ describe Diaspora::Federation::Receive do
expect(AccountDeletion.exists?(person: sender)).to be_truthy
end
it "ignores duplicate the account deletion" do
AccountDeletion.create(person: sender)
expect(AccountDeletion).not_to receive(:create!)
Diaspora::Federation::Receive.account_deletion(account_deletion_entity)
expect(AccountDeletion.exists?(person: sender)).to be_truthy
end
it "handles race conditions on parallel receive" do
expect(AccountDeletion).to receive(:create!) do
AccountDeletion.create(person: sender)
raise "Some database error"
end
Diaspora::Federation::Receive.account_deletion(account_deletion_entity)
expect(AccountDeletion.exists?(person: sender)).to be_truthy
end
end
describe ".account_migration" do
@ -26,6 +47,27 @@ describe Diaspora::Federation::Receive do
expect(AccountMigration.exists?(old_person: sender, new_person: new_person)).to be_truthy
end
it "ignores duplicate the account migrations" do
AccountMigration.create(old_person: sender, new_person: new_person)
expect(AccountMigration).not_to receive(:create!)
expect(Diaspora::Federation::Receive.account_migration(account_migration_entity)).to be_nil
expect(AccountMigration.exists?(old_person: sender, new_person: new_person)).to be_truthy
end
it "handles race conditions on parallel receive" do
expect(AccountMigration).to receive(:create!) do
AccountMigration.create(old_person: sender, new_person: new_person)
raise "Some database error"
end
expect(Diaspora::Federation::Receive.account_migration(account_migration_entity)).to be_nil
expect(AccountMigration.exists?(old_person: sender, new_person: new_person)).to be_truthy
end
end
describe ".comment" do

View file

@ -26,5 +26,13 @@ describe AspectMembership, :type => :model do
alice.add_contact_to_aspect(contact, aspect)
aspect_membership.destroy
end
it "doesn't fail destruction if the user entry was deleted in prior" do
aspect_membership.user.delete
id = aspect_membership.id
expect { AspectMembership.find(id).destroy }.not_to raise_error
expect(AspectMembership.where(id: id)).not_to exist
end
end
end

View file

@ -104,6 +104,16 @@ describe User::Connecting, type: :model do
alice.disconnect(contact)
}.to change(contact.aspects, :count).from(2).to(0)
end
it "raises when a contact for an improperly deleted user was passed" do
contact = alice.contact_for(bob.person)
bob.delete
expect {
alice.disconnect(contact)
}.to raise_error "FATAL: user entry is missing from the DB. Aborting"
expect(Contact.where(id: contact.id)).to exist
end
end
end

View file

@ -565,16 +565,11 @@ describe User, :type => :model do
end
end
describe 'account deletion' do
describe '#destroy' do
it 'removes all service connections' do
Services::Facebook.create(:access_token => 'what', :user_id => alice.id)
expect {
alice.destroy
}.to change {
alice.services.count
}.by(-1)
end
describe "#destroy" do
it "raises error" do
expect {
alice.destroy
}.to raise_error "Never destroy users!"
end
end