diff options
author | Jason Robinson <mail@jasonrobinson.me> | 2014-09-30 00:20:36 +0400 |
---|---|---|
committer | Jason Robinson <mail@jasonrobinson.me> | 2014-10-16 23:53:08 +0400 |
commit | 69c35669585896249db1eb3e3487a30ae09cff46 (patch) | |
tree | 941488aaa4cbb611c926a626a8e02e939a281472 | |
parent | ef71c5719072d6dc314bf567c4300eb96e0dddfc (diff) |
Maintenance feature to remove old users
Add Sidetiq webview to the Sidekiq monitoring panel
Add rake task maintenance:queue_users_for_removal
This basically just triggers an immediate run of the normal maintenance remove old users functionality that is normally (if enabled) scheduled to run once a day via sidetiq
Add extra safety when checking for user removal due to inactivity.
Now also user.last_seen will also be checked to make sure a user will not be removed in the event that the Devise rememember me login functionality has stopped the users remove_after timestamp from being removed.
Add initializer for maintenance job.
Add warning about mail being disabled if remove_old_users maintenance is enabled.
-rw-r--r-- | Changelog.md | 6 | ||||
-rw-r--r-- | Gemfile | 4 | ||||
-rw-r--r-- | Gemfile.lock | 6 | ||||
-rw-r--r-- | app/mailers/maintenance.rb | 15 | ||||
-rw-r--r-- | app/models/user.rb | 18 | ||||
-rw-r--r-- | app/views/maintenance/account_removal_warning.markerb | 2 | ||||
-rw-r--r-- | app/workers/queue_users_for_removal.rb | 41 | ||||
-rw-r--r-- | app/workers/remove_old_user.rb | 27 | ||||
-rw-r--r-- | config/defaults.yml | 6 | ||||
-rw-r--r-- | config/diaspora.yml.example | 15 | ||||
-rw-r--r-- | config/initializers/maintenance.rb | 16 | ||||
-rw-r--r-- | config/locales/diaspora/en.yml | 16 | ||||
-rw-r--r-- | config/routes.rb | 1 | ||||
-rw-r--r-- | db/migrate/20141001162851_add_remove_after_to_users.rb | 5 | ||||
-rw-r--r-- | db/schema.rb | 9 | ||||
-rw-r--r-- | lib/tasks/maintenance.rake | 9 | ||||
-rw-r--r-- | spec/mailers/maintenance_spec.rb | 41 | ||||
-rw-r--r-- | spec/models/user_spec.rb | 26 | ||||
-rw-r--r-- | spec/workers/queue_users_for_removal_spec.rb | 82 | ||||
-rw-r--r-- | spec/workers/remove_old_user_spec.rb | 52 |
20 files changed, 392 insertions, 5 deletions
diff --git a/Changelog.md b/Changelog.md index b59673f8b..473c1bf57 100644 --- a/Changelog.md +++ b/Changelog.md @@ -37,6 +37,11 @@ The way services are shown in the `statistics.json` route is changing. The keys The keys will still be available in the root level within the 0.5 release. The old keys will be removed in the 0.6 release. +## New maintenance feature to automatically expire inactive accounts +Removing of old inactive users can now be done automatically by background processing. The amount of inactivity is set by `after_days`. A warning email will be sent to the user and after an additional `warn_days`, the account will be automatically closed. + +This maintenance is not enabled by default. Podmins can enable it by for example copying over the new settings under `settings.maintenance` to their `diaspora.yml` file and setting it enabled. The default setting is to expire accounts that have been inactive for 2 years (no login). + ## Refactor * Redesign contacts page [#5153](https://github.com/diaspora/diaspora/pull/5153) * Improve profile page design on mobile [#5084](https://github.com/diaspora/diaspora/pull/5084) @@ -86,6 +91,7 @@ The keys will still be available in the root level within the 0.5 release. The o * Strip search query from leading and trailing whitespace [#5317](https://github.com/diaspora/diaspora/pull/5317) * Add the "network" key to statistics.json and set it to "Diaspora" [#5308](https://github.com/diaspora/diaspora/pull/5308) * Infinite scrolling in the notifications dropdown [#5237](https://github.com/diaspora/diaspora/pull/5237) +* Maintenance feature to automatically expire inactive accounts [#5288](https://github.com/diaspora/diaspora/pull/5288) # 0.4.1.1 @@ -32,6 +32,10 @@ gem 'simple_captcha2', '0.3.2', :require => 'simple_captcha' gem 'sidekiq', '3.2.5' gem 'sinatra', '1.3.3' +# Scheduled processing + +gem 'sidetiq', '0.6.3' + # Compression gem 'uglifier', '2.5.3' diff --git a/Gemfile.lock b/Gemfile.lock index efd3b0152..90c360cf5 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -243,6 +243,7 @@ GEM actionpack (>= 3.0.0) i18n-inflector (~> 2.6) railties (>= 3.0.0) + ice_cube (0.11.1) inflecto (0.0.2) ipaddress (0.8.0) jasmine (2.0.3) @@ -465,6 +466,10 @@ GEM json redis (>= 3.0.6) redis-namespace (>= 1.3.1) + sidetiq (0.6.3) + celluloid (>= 0.14.1) + ice_cube (= 0.11.1) + sidekiq (>= 3.0.0) simple_captcha2 (0.3.2) rails (>= 4.1) simple_oauth (0.3.0) @@ -613,6 +618,7 @@ DEPENDENCIES sass-rails (= 4.0.3) selenium-webdriver (= 2.43.0) sidekiq (= 3.2.5) + sidetiq (= 0.6.3) simple_captcha2 (= 0.3.2) sinatra (= 1.3.3) sinon-rails (= 1.10.3) diff --git a/app/mailers/maintenance.rb b/app/mailers/maintenance.rb new file mode 100644 index 000000000..d44b28f51 --- /dev/null +++ b/app/mailers/maintenance.rb @@ -0,0 +1,15 @@ +class Maintenance < ActionMailer::Base + default :from => AppConfig.mail.sender_address + + def account_removal_warning(user) + @user = user + @login_url = new_user_session_path + @pod_url = AppConfig.environment.url + @after_days = AppConfig.settings.maintenance.remove_old_users.after_days.to_s + @remove_after = @user.remove_after + mail(:to => @user.email, :subject => I18n.t('notifier.remove_old_user.subject')) do |format| + format.text + format.html + end + end +end diff --git a/app/models/user.rb b/app/models/user.rb index 5f7a5fd22..35fe032f9 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -481,12 +481,28 @@ class User < ActiveRecord::Base end end + def flag_for_removal(remove_after) + # flag inactive user for future removal + if AppConfig.settings.maintenance.remove_old_users.enable? + self.remove_after = remove_after + self.save + end + end + + def after_database_authentication + # remove any possible remove_after timestamp flag set by maintenance.remove_old_users + unless self.remove_after.nil? + self.remove_after = nil + self.save + end + end + private def clearable_fields self.attributes.keys - ["id", "username", "encrypted_password", "created_at", "updated_at", "locked_at", "serialized_private_key", "getting_started", "disable_mail", "show_community_spotlight_in_stream", - "email"] + "email", "remove_after"] end end diff --git a/app/views/maintenance/account_removal_warning.markerb b/app/views/maintenance/account_removal_warning.markerb new file mode 100644 index 000000000..7303bafae --- /dev/null +++ b/app/views/maintenance/account_removal_warning.markerb @@ -0,0 +1,2 @@ +<%= t('notifier.remove_old_user.body', pod_url: @pod_url, login_url: @login_url, after_days: @after_days, remove_after: @remove_after) %> + diff --git a/app/workers/queue_users_for_removal.rb b/app/workers/queue_users_for_removal.rb new file mode 100644 index 000000000..63efa3365 --- /dev/null +++ b/app/workers/queue_users_for_removal.rb @@ -0,0 +1,41 @@ +# Copyright (c) 2010-2011, Diaspora Inc. This file is +# licensed under the Affero General Public License version 3 or later. See +# the COPYRIGHT file. + +module Workers + class QueueUsersForRemoval < Base + include Sidetiq::Schedulable + + sidekiq_options queue: :maintenance + + recurrence { daily } + + def perform + # Queue users for removal due to inactivity + if AppConfig.settings.maintenance.remove_old_users.enable? + users = User.where("last_seen < ? and locked_at is null and remove_after is null", + Time.now - (AppConfig.settings.maintenance.remove_old_users.after_days.to_i).days) + .order(:last_seen) + .limit(AppConfig.settings.maintenance.remove_old_users.limit_removals_to_per_day) + + # deliver to be closed emails to account holders + # and queue accounts for closing to sidekiq + # for those who have not signed in, skip warning and queue removal + # in +1 days + users.find_each do |user| + if user.sign_in_count > 0 + remove_at = Time.now + AppConfig.settings.maintenance.remove_old_users.warn_days.to_i.days + else + remove_at = Time.now + end + user.flag_for_removal(remove_at) + if user.sign_in_count > 0 + # send a warning + Maintenance.account_removal_warning(user).deliver + end + Workers::RemoveOldUser.perform_in(remove_at+1.day, user.id) + end + end + end + end +end
\ No newline at end of file diff --git a/app/workers/remove_old_user.rb b/app/workers/remove_old_user.rb new file mode 100644 index 000000000..d8619b4bf --- /dev/null +++ b/app/workers/remove_old_user.rb @@ -0,0 +1,27 @@ +# Copyright (c) 2010-2011, Diaspora Inc. This file is +# licensed under the Affero General Public License version 3 or later. See +# the COPYRIGHT file. + +module Workers + class RemoveOldUser < Base + sidekiq_options queue: :maintenance + + def safe_remove_after + # extra safety time to compare in addition to remove_after + Time.now- + (AppConfig.settings.maintenance.remove_old_users.after_days.to_i).days- + (AppConfig.settings.maintenance.remove_old_users.warn_days.to_i).days + end + + def perform(user_id) + # if user has been flagged as to be removed (see settings.maintenance.remove_old_users) + # and hasn't logged in since that flag has been set, we remove the user + if AppConfig.settings.maintenance.remove_old_users.enable? + user = User.find(user_id) + if user.remove_after < Time.now and user.last_seen < self.safe_remove_after + user.close_account! + end + end + end + end +end
\ No newline at end of file diff --git a/config/defaults.yml b/config/defaults.yml index 01e2599be..841fc2643 100644 --- a/config/defaults.yml +++ b/config/defaults.yml @@ -93,6 +93,12 @@ defaults: enable: false jurisdiction: false minimum_age: false + maintenance: + remove_old_users: + enable: false + after_days: 730 + warn_days: 30 + limit_removals_to_per_day: 100 services: facebook: enable: false diff --git a/config/diaspora.yml.example b/config/diaspora.yml.example index 0555ee9bc..3a4153edf 100644 --- a/config/diaspora.yml.example +++ b/config/diaspora.yml.example @@ -330,6 +330,21 @@ configuration: ## Section ## Set a number to activate this setting. This age limit is shown ## in the default ToS document. #minimum_age: false + + ## Maintenance + ## Various pod maintenance related settings are controlled from here. + maintenance: ## Section + ## Removing of old inactive users can be done automatically by background + ## processing. The amount of inactivity is set by `after_days`. A warning + ## email will be sent to the user and after an additional `warn_days`, the + ## account will be automatically closed. + ## This maintenance is not enabled by default. + remove_old_users: ## Section + #enable: true + #after_days: 730 + #warn_days: 30 + ## Limit queuing for removal per day. + #limit_removals_to_per_day: 100 ## Posting from Diaspora to external services (all are disabled by default) services: ## Section diff --git a/config/initializers/maintenance.rb b/config/initializers/maintenance.rb new file mode 100644 index 000000000..76bc58aae --- /dev/null +++ b/config/initializers/maintenance.rb @@ -0,0 +1,16 @@ +# Copyright (c) 2010-2011, Diaspora Inc. This file is +# licensed under the Affero General Public License version 3 or later. See +# the COPYRIGHT file. + +unless AppConfig.mail.enable? + if AppConfig.settings.maintenance.remove_old_users.enable? + # The maintenance job remove_old_users will warn users + # of inactivity removal before removing the users. + # Warn podmins here that enable it but don't have mail enabled. + puts " +WARNING: Maintenance that removes inactive users is enabled +but mail is disabled! This means there will be no warning email +sent to users whose accounts are flagged for removal! +See configuration setting 'settings.maintenance.remove_old_users'." + end +end
\ No newline at end of file diff --git a/config/locales/diaspora/en.yml b/config/locales/diaspora/en.yml index 014841156..b078c33d5 100644 --- a/config/locales/diaspora/en.yml +++ b/config/locales/diaspora/en.yml @@ -802,6 +802,22 @@ en: The diaspora* email robot! [1]: %{invite_url} + remove_old_user: + subject: "Your diaspora* account has been flagged for removal due to inactivity" + body: |- + Hello, + + Due to inactivity on your diaspora* account at %{pod_url}, we regret to inform you that the system has flagged this account for automated removal. This happens automatically after an inactivity period of more than %{after_days} days. + + You can avoid the loss of this account by logging into the account before %{remove_after}, in which case removal will be automatically cancelled. + + This maintenance is done to ensure our active users get the most out of the resources of this diaspora* instance. Thank you for your understanding. + + If you would like to keep your account, please login here: %{login_url} + + Hoping to see you again, + + The diaspora* email robot! people: zero: "no people" one: "1 person" diff --git a/config/routes.rb b/config/routes.rb index dbb06bde4..08da714cb 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -3,6 +3,7 @@ # the COPYRIGHT file. require 'sidekiq/web' +require 'sidetiq/web' Diaspora::Application.routes.draw do resources :report, :except => [:edit, :new] diff --git a/db/migrate/20141001162851_add_remove_after_to_users.rb b/db/migrate/20141001162851_add_remove_after_to_users.rb new file mode 100644 index 000000000..c63b916b6 --- /dev/null +++ b/db/migrate/20141001162851_add_remove_after_to_users.rb @@ -0,0 +1,5 @@ +class AddRemoveAfterToUsers < ActiveRecord::Migration + def change + add_column :users, :remove_after, :datetime + end +end diff --git a/db/schema.rb b/db/schema.rb index 80202933c..a19ba2b5f 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: 20140906192846) do +ActiveRecord::Schema.define(version: 20141001162851) do create_table "account_deletions", force: true do |t| t.string "diaspora_handle" @@ -219,8 +219,8 @@ ActiveRecord::Schema.define(version: 20140906192846) do create_table "open_graph_caches", force: true do |t| t.string "title" t.string "ob_type" - t.text "image" - t.text "url" + t.text "image" + t.text "url" t.text "description" end @@ -530,6 +530,7 @@ ActiveRecord::Schema.define(version: 20140906192846) do t.text "hidden_shareables" t.datetime "reset_password_sent_at" t.datetime "last_seen" + t.datetime "remove_after" end add_index "users", ["authentication_token"], name: "index_users_on_authentication_token", unique: true, using: :btree @@ -570,4 +571,4 @@ ActiveRecord::Schema.define(version: 20140906192846) do add_foreign_key "share_visibilities", "contacts", name: "post_visibilities_contact_id_fk", dependent: :delete -end
\ No newline at end of file +end diff --git a/lib/tasks/maintenance.rake b/lib/tasks/maintenance.rake index fd8c9e59d..746a773f5 100644 --- a/lib/tasks/maintenance.rake +++ b/lib/tasks/maintenance.rake @@ -36,4 +36,13 @@ RUBY puts "Could not install logrotate configs. Perhaps you should try running this task as root and ensuring logrotate is installed:\n#{logrotate_conf}" end end + + desc "Queue users for removal" + task :queue_users_for_removal => :environment do + # Queue users for removal due to inactivity + # Note! settings.maintenance.remove_old_users + # must still be enabled, this only bypasses + # scheduling to run the queuing immediately + Workers::QueueUsersForRemoval.perform_async + end end diff --git a/spec/mailers/maintenance_spec.rb b/spec/mailers/maintenance_spec.rb new file mode 100644 index 000000000..1ca07c513 --- /dev/null +++ b/spec/mailers/maintenance_spec.rb @@ -0,0 +1,41 @@ +# Copyright (c) 2010-2011, Diaspora Inc. This file is +# licensed under the Affero General Public License version 3 or later. See +# the COPYRIGHT file. + +require 'spec_helper' + +describe Maintenance, :type => :mailer do + describe 'create warning' do + before do + @removal_timestamp = Time.now + 3.days + @user = FactoryGirl.create(:user_with_aspect, :username => "local", :remove_after => @removal_timestamp) + end + + it "#should deliver successfully" do + expect { + Maintenance.account_removal_warning(@user).deliver + }.to_not raise_error + end + + it "#should be added to the delivery queue" do + expect { + Maintenance.account_removal_warning(@user).deliver + }.to change(ActionMailer::Base.deliveries, :size).by(1) + end + + it "#should include correct recipient" do + Maintenance.account_removal_warning(@user).deliver + expect(ActionMailer::Base.deliveries.last.to[0]).to include(@user.email) + end + + it "#should include after inactivity days from settings" do + Maintenance.account_removal_warning(@user).deliver + expect(ActionMailer::Base.deliveries.last.body.parts[0].body.raw_source).to include("more than "+AppConfig.settings.maintenance.remove_old_users.after_days.to_s+" days") + end + + it "#should include timestamp for account removal" do + Maintenance.account_removal_warning(@user).deliver + expect(ActionMailer::Base.deliveries.last.body.parts[0].body.raw_source).to include("logging into the account before "+@removal_timestamp.utc.to_s) + end + end +end diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index b6bcbf802..4648b3196 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -1025,4 +1025,30 @@ describe User, :type => :model do @user.sign_up end end + + describe "maintenance" do + before do + @user = bob + AppConfig.settings.maintenance.remove_old_users.enable = true + end + + it "#flags user for removal" do + remove_at = Time.now+5.days + @user.flag_for_removal(remove_at) + expect(@user.remove_after).to eq(remove_at) + end + end + + describe "#auth database auth maintenance" do + before do + @user = bob + @user.remove_after = Time.now + @user.save + end + + it "remove_after is cleared" do + @user.after_database_authentication + expect(@user.remove_after).to eq(nil) + end + end end diff --git a/spec/workers/queue_users_for_removal_spec.rb b/spec/workers/queue_users_for_removal_spec.rb new file mode 100644 index 000000000..6e00fe76c --- /dev/null +++ b/spec/workers/queue_users_for_removal_spec.rb @@ -0,0 +1,82 @@ +require 'spec_helper' + +describe Workers::QueueUsersForRemoval do + describe 'remove_old_users is active' do + before do + AppConfig.settings.maintenance.remove_old_users.enable = true + ActionMailer::Base.deliveries = nil + Timecop.freeze + end + + it '#does not queue user that is not inactive' do + user = FactoryGirl.create(:user, :last_seen => Time.now-728.days, :sign_in_count => 5) + Workers::QueueUsersForRemoval.new.perform + user.reload + expect(user.remove_after).to eq(nil) + expect(ActionMailer::Base.deliveries.count).to eq(0) + end + + it '#queues user that is inactive' do + removal_date = Time.now + (AppConfig.settings.maintenance.remove_old_users.warn_days.to_i).days + user = FactoryGirl.create(:user, :last_seen => Time.now-732.days, :sign_in_count => 5) + Workers::QueueUsersForRemoval.new.perform + user.reload + expect(user.remove_after.to_i).to eq(removal_date.utc.to_i) + expect(ActionMailer::Base.deliveries.count).to eq(1) + end + + it '#queues user that is inactive and has not logged in' do + removal_date = Time.now + user = FactoryGirl.create(:user, :last_seen => Time.now-735.days, :sign_in_count => 0) + Workers::QueueUsersForRemoval.new.perform + user.reload + expect(user.remove_after.to_i).to eq(removal_date.utc.to_i) + expect(ActionMailer::Base.deliveries.count).to eq(0) # no email sent + end + + it '#does not queue user that is not inactive and has not logged in' do + user = FactoryGirl.create(:user, :last_seen => Time.now-728.days, :sign_in_count => 0) + Workers::QueueUsersForRemoval.new.perform + user.reload + expect(user.remove_after).to eq(nil) + expect(ActionMailer::Base.deliveries.count).to eq(0) + end + + it '#does not queue user that has already been flagged for removal' do + removal_date = Date.today + 5.days + user = FactoryGirl.create(:user, :last_seen => Time.now-735.days, :sign_in_count => 5, :remove_after => removal_date) + Workers::QueueUsersForRemoval.new.perform + user.reload + expect(user.remove_after).to eq(removal_date) + expect(ActionMailer::Base.deliveries.count).to eq(0) + end + + after do + Timecop.return + end + end + + describe 'remove_old_users is inactive' do + before do + AppConfig.settings.maintenance.remove_old_users.enable = false + ActionMailer::Base.deliveries = nil + end + + it '#does not queue user that is not inactive' do + user = FactoryGirl.create(:user, :last_seen => Time.now-728.days, :sign_in_count => 5) + Workers::QueueUsersForRemoval.new.perform + user.reload + expect(user.remove_after).to eq(nil) + expect(ActionMailer::Base.deliveries.count).to eq(0) + end + + it '#does not queue user that is inactive' do + user = FactoryGirl.create(:user, :last_seen => Time.now-735.days, :sign_in_count => 5) + Workers::QueueUsersForRemoval.new.perform + user.reload + expect(user.remove_after).to eq(nil) + expect(ActionMailer::Base.deliveries.count).to eq(0) + end + + end +end diff --git a/spec/workers/remove_old_user_spec.rb b/spec/workers/remove_old_user_spec.rb new file mode 100644 index 000000000..12e0fad57 --- /dev/null +++ b/spec/workers/remove_old_user_spec.rb @@ -0,0 +1,52 @@ +require 'spec_helper' + +describe Workers::RemoveOldUser do + describe 'remove_old_users is active' do + before do + AppConfig.settings.maintenance.remove_old_users.enable = true + end + + it '#removes user whose remove_after timestamp has passed' do + user = double(id: 1, remove_after: 1.day.ago, last_seen: 1000.days.ago) + allow(User).to receive(:find).with(user.id).and_return(user) + expect(user).to receive(:close_account!) + Workers::RemoveOldUser.new.perform(user.id) + end + + it '#doesnt remove user whose remove_after timestamp hasnt passed' do + user = double(id: 1, remove_after: 1.day.from_now, last_seen: 1000.days.ago) + allow(User).to receive(:find).with(user.id).and_return(user) + expect(user).to_not receive(:close_account!) + Workers::RemoveOldUser.new.perform(user.id) + end + + it '#doesnt remove user whose remove_after timestamp has passed but last_seen is recent' do + user = double(id: 1, remove_after: 1.day.ago, last_seen: 1.day.ago) + allow(User).to receive(:find).with(user.id).and_return(user) + expect(user).to_not receive(:close_account!) + Workers::RemoveOldUser.new.perform(user.id) + end + + end + + describe 'remove_old_users is inactive' do + before do + AppConfig.settings.maintenance.remove_old_users.enable = false + end + + it '#doesnt remove user whose remove_after timestamp has passed' do + user = double(id: 1, remove_after: 1.day.ago, last_seen: 1000.days.ago) + allow(User).to receive(:find).with(user.id).and_return(user) + expect(user).to_not receive(:close_account!) + Workers::RemoveOldUser.new.perform(user.id) + end + + it '#doesnt remove user whose remove_after timestamp hasnt passed' do + user = double(id: 1, remove_after: 1.day.from_now, last_seen: 1000.days.ago) + allow(User).to receive(:find).with(user.id).and_return(user) + expect(user).to_not receive(:close_account!) + Workers::RemoveOldUser.new.perform(user.id) + end + + end +end |