From: Andy Allan Date: Wed, 22 Nov 2023 15:07:34 +0000 (+0000) Subject: Merge pull request #4313 from AntonKhorev/account-delete-delay X-Git-Tag: live~1075 X-Git-Url: https://git.openstreetmap.org./rails.git/commitdiff_plain/5fdddf2a8a98362ee646fb9cd22fadeaea185b46?hp=f08fb4f30f83288a5d485d422b94c4301c2a86b5 Merge pull request #4313 from AntonKhorev/account-delete-delay Account deletion cool-down period --- diff --git a/.rubocop_todo.yml b/.rubocop_todo.yml index e3407c6dc..bd2f92309 100644 --- a/.rubocop_todo.yml +++ b/.rubocop_todo.yml @@ -61,7 +61,7 @@ Metrics/BlockNesting: # Offense count: 26 # Configuration parameters: CountComments, CountAsOne. Metrics/ClassLength: - Max: 286 + Max: 297 # Offense count: 59 # Configuration parameters: AllowedMethods, AllowedPatterns. diff --git a/app/controllers/accounts_controller.rb b/app/controllers/accounts_controller.rb index 63da1293f..db9721010 100644 --- a/app/controllers/accounts_controller.rb +++ b/app/controllers/accounts_controller.rb @@ -53,12 +53,16 @@ class AccountsController < ApplicationController end def destroy - current_user.soft_destroy! + if current_user.deletion_allowed? + current_user.soft_destroy! - session.delete(:user) - session_expires_automatically + session.delete(:user) + session_expires_automatically - flash[:notice] = t ".success" - redirect_to root_path + flash[:notice] = t ".success" + redirect_to root_path + else + head :bad_request + end end end diff --git a/app/models/changeset.rb b/app/models/changeset.rb index ce0943824..137de18fd 100644 --- a/app/models/changeset.rb +++ b/app/models/changeset.rb @@ -14,11 +14,12 @@ # # Indexes # -# changesets_bbox_idx (min_lat,max_lat,min_lon,max_lon) USING gist -# changesets_closed_at_idx (closed_at) -# changesets_created_at_idx (created_at) -# changesets_user_id_created_at_idx (user_id,created_at) -# changesets_user_id_id_idx (user_id,id) +# changesets_bbox_idx (min_lat,max_lat,min_lon,max_lon) USING gist +# changesets_closed_at_idx (closed_at) +# changesets_created_at_idx (created_at) +# changesets_user_id_created_at_idx (user_id,created_at) +# changesets_user_id_id_idx (user_id,id) +# index_changesets_on_user_id_and_closed_at (user_id,closed_at) # # Foreign Keys # diff --git a/app/models/user.rb b/app/models/user.rb index 7571dd9dc..1942a25cc 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -419,6 +419,18 @@ class User < ApplicationRecord end end + def deletion_allowed_at + unless Settings.user_account_deletion_delay.nil? + last_changeset = changesets.reorder(:closed_at => :desc).first + return last_changeset.closed_at.utc + Settings.user_account_deletion_delay.hours if last_changeset + end + creation_time.utc + end + + def deletion_allowed? + deletion_allowed_at <= Time.now.utc + end + private def encrypt_password diff --git a/app/views/account/deletions/show.html.erb b/app/views/account/deletions/show.html.erb index ddc821677..0ed4d663f 100644 --- a/app/views/account/deletions/show.html.erb +++ b/app/views/account/deletions/show.html.erb @@ -31,5 +31,13 @@
  • <%= t ".retain_email" %>
    • -<%= link_to t(".delete_account"), account_path, { :method => :delete, :class => "btn btn-danger", :data => { :confirm => t(".confirm_delete") } } %> +<% if current_user.deletion_allowed? %> + <%= link_to t(".delete_account"), account_path, { :method => :delete, :class => "btn btn-danger", :data => { :confirm => t(".confirm_delete") } } %> +<% else %> +
    + <%= t ".recent_editing_html", :time => friendly_date(current_user.deletion_allowed_at) %> +
    + +<% end %> + <%= link_to t(".cancel"), edit_account_path, :class => "btn btn-link" %> diff --git a/config/initializers/config.rb b/config/initializers/config.rb index c1cc522a5..f36e6ac1a 100644 --- a/config/initializers/config.rb +++ b/config/initializers/config.rb @@ -79,6 +79,7 @@ Config.setup do |config| required(:max_number_of_relation_members).filled(:int?) required(:max_issues_count).filled(:int?) required(:api_timeout).filled(:int?) + required(:user_account_deletion_delay).maybe(:number?) required(:imagery_blacklist).maybe(:array?) required(:status).filled(:str?, :included_in? => ALLOWED_STATUS) required(:avatar_storage).filled(:str?) diff --git a/config/locales/en.yml b/config/locales/en.yml index 1a41dcce8..56d722f43 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -256,6 +256,7 @@ en: retain_notes: Your map notes and note comments, if any, will be retained but hidden from view. retain_changeset_discussions: Your changeset discussions, if any, will be retained. retain_email: Your email address will be retained. + recent_editing_html: "As you have edited recently your account cannot currently be deleted. Deletion will be possible in %{time}." confirm_delete: Are you sure? cancel: Cancel accounts: diff --git a/config/settings.yml b/config/settings.yml index 87c467c88..1c9c7e0a1 100644 --- a/config/settings.yml +++ b/config/settings.yml @@ -53,6 +53,8 @@ api_timeout: 300 web_timeout: 30 # Periods (in hours) which are allowed for user blocks user_block_periods: [0, 1, 3, 6, 12, 24, 48, 96, 168, 336, 731, 4383, 8766, 87660] +# Account deletion cooldown period (in hours) since last changeset close; null to disable, 0 to make sure there aren't any open changesets when the deletion happens +user_account_deletion_delay: null # Rate limit for message sending max_messages_per_hour: 60 # Rate limit for friending diff --git a/db/migrate/20231117170422_add_closed_at_index_to_changesets.rb b/db/migrate/20231117170422_add_closed_at_index_to_changesets.rb new file mode 100644 index 000000000..e9d7e62f7 --- /dev/null +++ b/db/migrate/20231117170422_add_closed_at_index_to_changesets.rb @@ -0,0 +1,7 @@ +class AddClosedAtIndexToChangesets < ActiveRecord::Migration[7.1] + disable_ddl_transaction! + + def change + add_index :changesets, [:user_id, :closed_at], :algorithm => :concurrently + end +end diff --git a/db/structure.sql b/db/structure.sql index 56e778523..f74d4d571 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -2499,6 +2499,13 @@ CREATE INDEX index_changeset_comments_on_changeset_id_and_created_at ON public.c CREATE INDEX index_changeset_comments_on_created_at ON public.changeset_comments USING btree (created_at); +-- +-- Name: index_changesets_on_user_id_and_closed_at; Type: INDEX; Schema: public; Owner: - +-- + +CREATE INDEX index_changesets_on_user_id_and_closed_at ON public.changesets USING btree (user_id, closed_at); + + -- -- Name: index_changesets_subscribers_on_changeset_id; Type: INDEX; Schema: public; Owner: - -- @@ -3499,6 +3506,7 @@ INSERT INTO "schema_migrations" (version) VALUES ('23'), ('22'), ('21'), +('20231117170422'), ('20231101222146'), ('20231029151516'), ('20231010194809'), diff --git a/test/controllers/accounts_controller_test.rb b/test/controllers/accounts_controller_test.rb index 7546c3797..131292f41 100644 --- a/test/controllers/accounts_controller_test.rb +++ b/test/controllers/accounts_controller_test.rb @@ -152,4 +152,23 @@ class AccountsControllerTest < ActionDispatch::IntegrationTest # Make sure we have a button to "go public" assert_select "form.button_to[action='/user/go_public']", true end + + def test_destroy_allowed + user = create(:user) + session_for(user) + + delete account_path + assert_response :redirect + end + + def test_destroy_not_allowed + with_user_account_deletion_delay(24) do + user = create(:user) + create(:changeset, :user => user, :created_at => Time.now.utc) + session_for(user) + + delete account_path + assert_response :bad_request + end + end end diff --git a/test/models/user_test.rb b/test/models/user_test.rb index a4ed07e09..5c48bb969 100644 --- a/test/models/user_test.rb +++ b/test/models/user_test.rb @@ -282,4 +282,62 @@ class UserTest < ActiveSupport::TestCase oauth_access_token.reload assert_predicate oauth_access_token, :revoked? end + + def test_deletion_allowed_when_no_changesets + with_user_account_deletion_delay(10000) do + user = create(:user) + assert_predicate user, :deletion_allowed? + end + end + + def test_deletion_allowed_without_delay + with_user_account_deletion_delay(nil) do + user = create(:user) + create(:changeset, :user => user) + user.reload + assert_predicate user, :deletion_allowed? + end + end + + def test_deletion_allowed_past_delay + with_user_account_deletion_delay(10) do + user = create(:user) + create(:changeset, :user => user, :created_at => Time.now.utc - 12.hours, :closed_at => Time.now.utc - 10.hours) + user.reload + assert_predicate user, :deletion_allowed? + end + end + + def test_deletion_allowed_during_delay + with_user_account_deletion_delay(10) do + user = create(:user) + create(:changeset, :user => user, :created_at => Time.now.utc - 11.hours, :closed_at => Time.now.utc - 9.hours) + user.reload + assert_not_predicate user, :deletion_allowed? + assert_equal Time.now.utc + 1.hour, user.deletion_allowed_at + end + end + + def test_deletion_allowed_past_zero_delay + with_user_account_deletion_delay(0) do + user = create(:user) + create(:changeset, :user => user, :created_at => Time.now.utc, :closed_at => Time.now.utc + 1.hour) + travel 90.minutes do + user.reload + assert_predicate user, :deletion_allowed? + end + end + end + + def test_deletion_allowed_during_zero_delay + with_user_account_deletion_delay(0) do + user = create(:user) + create(:changeset, :user => user, :created_at => Time.now.utc, :closed_at => Time.now.utc + 1.hour) + travel 30.minutes do + user.reload + assert_not_predicate user, :deletion_allowed? + assert_equal Time.now.utc + 30.minutes, user.deletion_allowed_at + end + end + end end diff --git a/test/system/account_deletion_test.rb b/test/system/account_deletion_test.rb index 87e981c64..e6517dccc 100644 --- a/test/system/account_deletion_test.rb +++ b/test/system/account_deletion_test.rb @@ -41,4 +41,59 @@ class AccountDeletionTest < ApplicationSystemTestCase assert_content "Account Deleted" end + + test "can delete with any delay setting value if the user has no changesets" do + with_user_account_deletion_delay(10000) do + travel 1.hour do + visit edit_account_path + + click_link "Delete Account..." + + assert_no_content "cannot currently be deleted" + end + end + end + + test "can delete with delay disabled" do + with_user_account_deletion_delay(nil) do + create(:changeset, :user => @user) + + travel 1.hour do + visit edit_account_path + + click_link "Delete Account..." + + assert_no_content "cannot currently be deleted" + end + end + end + + test "can delete when last changeset is old enough" do + with_user_account_deletion_delay(10) do + create(:changeset, :user => @user, :created_at => Time.now.utc, :closed_at => Time.now.utc + 1.hour) + + travel 12.hours do + visit edit_account_path + + click_link "Delete Account..." + + assert_no_content "cannot currently be deleted" + end + end + end + + test "can't delete when last changeset isn't old enough" do + with_user_account_deletion_delay(10) do + create(:changeset, :user => @user, :created_at => Time.now.utc, :closed_at => Time.now.utc + 1.hour) + + travel 10.hours do + visit edit_account_path + + click_link "Delete Account..." + + assert_content "cannot currently be deleted" + assert_content "in about 1 hour" + end + end + end end diff --git a/test/test_helper.rb b/test/test_helper.rb index 68749c0f7..19e1a2784 100644 --- a/test/test_helper.rb +++ b/test/test_helper.rb @@ -371,5 +371,16 @@ module ActiveSupport el << tag_el end end + + def with_user_account_deletion_delay(value) + freeze_time + default_value = Settings.user_account_deletion_delay + Settings.user_account_deletion_delay = value + + yield + + Settings.user_account_deletion_delay = default_value + unfreeze_time + end end end