From d69d29011cf9fe06e50a2c7d65b1ea88ea2d41d5 Mon Sep 17 00:00:00 2001 From: Vladimir Shushlin Date: Tue, 9 Apr 2019 17:46:29 +0000 Subject: Mark unverified pages domains for removal Set pages_domain.remove_at when disabling it Add specs for marking pages domain for removal Notify user that domain is being removed Add documentation --- app/services/verify_pages_domain_service.rb | 15 ++-- app/views/notify/_removal_notification.html.haml | 9 +++ .../notify/pages_domain_disabled_email.html.haml | 4 +- ...ages_domain_verification_failed_email.html.haml | 4 +- .../unreleased/remove-disabled-pages-domains.yml | 5 ++ ...0190312113229_add_remove_at_to_pages_domains.rb | 10 +++ ...2113634_add_remove_at_index_to_pages_domains.rb | 19 +++++ db/schema.rb | 2 + .../project/pages/getting_started_part_three.md | 2 + spec/factories/pages_domains.rb | 4 + spec/mailers/emails/pages_domains_spec.rb | 24 ++++++ spec/services/verify_pages_domain_service_spec.rb | 89 +++++++++++++++++----- 12 files changed, 154 insertions(+), 33 deletions(-) create mode 100644 app/views/notify/_removal_notification.html.haml create mode 100644 changelogs/unreleased/remove-disabled-pages-domains.yml create mode 100644 db/migrate/20190312113229_add_remove_at_to_pages_domains.rb create mode 100644 db/migrate/20190312113634_add_remove_at_index_to_pages_domains.rb diff --git a/app/services/verify_pages_domain_service.rb b/app/services/verify_pages_domain_service.rb index 07f7391f877..b53c3145caf 100644 --- a/app/services/verify_pages_domain_service.rb +++ b/app/services/verify_pages_domain_service.rb @@ -8,6 +8,7 @@ class VerifyPagesDomainService < BaseService # How long verification lasts for VERIFICATION_PERIOD = 7.days + REMOVAL_DELAY = 1.week.freeze attr_reader :domain @@ -36,7 +37,7 @@ class VerifyPagesDomainService < BaseService # Prevent any pre-existing grace period from being truncated reverify = [domain.enabled_until, VERIFICATION_PERIOD.from_now].compact.max - domain.assign_attributes(verified_at: Time.now, enabled_until: reverify) + domain.assign_attributes(verified_at: Time.now, enabled_until: reverify, remove_at: nil) domain.save!(validate: false) if was_disabled @@ -49,18 +50,20 @@ class VerifyPagesDomainService < BaseService end def unverify_domain! - if domain.verified? - domain.assign_attributes(verified_at: nil) - domain.save!(validate: false) + was_verified = domain.verified? - notify(:verification_failed) - end + domain.assign_attributes(verified_at: nil) + domain.remove_at ||= REMOVAL_DELAY.from_now unless domain.enabled? + domain.save!(validate: false) + + notify(:verification_failed) if was_verified error("Couldn't verify #{domain.domain}") end def disable_domain! domain.assign_attributes(verified_at: nil, enabled_until: nil) + domain.remove_at ||= REMOVAL_DELAY.from_now domain.save!(validate: false) notify(:disabled) diff --git a/app/views/notify/_removal_notification.html.haml b/app/views/notify/_removal_notification.html.haml new file mode 100644 index 00000000000..590e0d569aa --- /dev/null +++ b/app/views/notify/_removal_notification.html.haml @@ -0,0 +1,9 @@ +- if @domain.remove_at + %p + Unless you verify your domain by + %strong= @domain.remove_at.strftime('%F %T,') + it will be removed from your GitLab project. +- else + %p + If you no longer wish to use this domain with GitLab Pages, please remove it + from your GitLab project and delete any related DNS records. diff --git a/app/views/notify/pages_domain_disabled_email.html.haml b/app/views/notify/pages_domain_disabled_email.html.haml index 34ce4238a12..224b79bfde8 100644 --- a/app/views/notify/pages_domain_disabled_email.html.haml +++ b/app/views/notify/pages_domain_disabled_email.html.haml @@ -10,6 +10,4 @@ If this domain has been disabled in error, please follow = link_to 'these instructions', help_page_url('user/project/pages/getting_started_part_three.md', anchor: 'dns-txt-record') to verify and re-enable your domain. -%p - If you no longer wish to use this domain with GitLab Pages, please remove it - from your GitLab project and delete any related DNS records. += render 'removal_notification' diff --git a/app/views/notify/pages_domain_verification_failed_email.html.haml b/app/views/notify/pages_domain_verification_failed_email.html.haml index 0bb0eb09fd5..03b298f8e7c 100644 --- a/app/views/notify/pages_domain_verification_failed_email.html.haml +++ b/app/views/notify/pages_domain_verification_failed_email.html.haml @@ -12,6 +12,4 @@ Please visit = link_to 'these instructions', help_page_url('user/project/pages/getting_started_part_three.md', anchor: 'dns-txt-record') for more information about custom domain verification. -%p - If you no longer wish to use this domain with GitLab Pages, please remove it - from your GitLab project and delete any related DNS records. += render 'removal_notification' diff --git a/changelogs/unreleased/remove-disabled-pages-domains.yml b/changelogs/unreleased/remove-disabled-pages-domains.yml new file mode 100644 index 00000000000..e23561329ef --- /dev/null +++ b/changelogs/unreleased/remove-disabled-pages-domains.yml @@ -0,0 +1,5 @@ +--- +title: Mark disabled pages domains for removal, but don't remove them yet +merge_request: 26212 +author: +type: added diff --git a/db/migrate/20190312113229_add_remove_at_to_pages_domains.rb b/db/migrate/20190312113229_add_remove_at_to_pages_domains.rb new file mode 100644 index 00000000000..aa7d2841cd2 --- /dev/null +++ b/db/migrate/20190312113229_add_remove_at_to_pages_domains.rb @@ -0,0 +1,10 @@ +# frozen_string_literal: true + +class AddRemoveAtToPagesDomains < ActiveRecord::Migration[5.0] + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + def change + add_column :pages_domains, :remove_at, :datetime_with_timezone + end +end diff --git a/db/migrate/20190312113634_add_remove_at_index_to_pages_domains.rb b/db/migrate/20190312113634_add_remove_at_index_to_pages_domains.rb new file mode 100644 index 00000000000..b5ccebd9cfa --- /dev/null +++ b/db/migrate/20190312113634_add_remove_at_index_to_pages_domains.rb @@ -0,0 +1,19 @@ +# frozen_string_literal: true + +# See http://doc.gitlab.com/ce/development/migration_style_guide.html +# for more information on how to write migrations for GitLab. + +class AddRemoveAtIndexToPagesDomains < ActiveRecord::Migration[5.0] + include Gitlab::Database::MigrationHelpers + DOWNTIME = false + + disable_ddl_transaction! + + def up + add_concurrent_index :pages_domains, :remove_at + end + + def down + remove_concurrent_index :pages_domains, :remove_at + end +end diff --git a/db/schema.rb b/db/schema.rb index c044fcc90c6..d1b3672725d 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -1547,9 +1547,11 @@ ActiveRecord::Schema.define(version: 20190326164045) do t.datetime_with_timezone "verified_at" t.string "verification_code", null: false t.datetime_with_timezone "enabled_until" + t.datetime_with_timezone "remove_at" t.index ["domain"], name: "index_pages_domains_on_domain", unique: true, using: :btree t.index ["project_id", "enabled_until"], name: "index_pages_domains_on_project_id_and_enabled_until", using: :btree t.index ["project_id"], name: "index_pages_domains_on_project_id", using: :btree + t.index ["remove_at"], name: "index_pages_domains_on_remove_at", using: :btree t.index ["verified_at", "enabled_until"], name: "index_pages_domains_on_verified_at_and_enabled_until", using: :btree t.index ["verified_at"], name: "index_pages_domains_on_verified_at", using: :btree end diff --git a/doc/user/project/pages/getting_started_part_three.md b/doc/user/project/pages/getting_started_part_three.md index fa7ab19ece6..2839f04ae59 100644 --- a/doc/user/project/pages/getting_started_part_three.md +++ b/doc/user/project/pages/getting_started_part_three.md @@ -115,6 +115,8 @@ If using a [DNS A record](#dns-a-record), you can place the TXT record directly under the domain. If using a [DNS CNAME record](#dns-cname-record), the two record types won't co-exist, so you need to place the TXT record in a special subdomain of its own. +If the domain cannot be verified for 7 days, it will be removed from the GitLab project. + #### TL;DR For root domains (`domain.com`), set a DNS `A` record and verify your diff --git a/spec/factories/pages_domains.rb b/spec/factories/pages_domains.rb index 20671da016e..b74f72f2bd3 100644 --- a/spec/factories/pages_domains.rb +++ b/spec/factories/pages_domains.rb @@ -41,6 +41,10 @@ nNp/xedE1YxutQ== enabled_until nil end + trait :scheduled_for_removal do + remove_at { 1.day.from_now } + end + trait :unverified do verified_at nil end diff --git a/spec/mailers/emails/pages_domains_spec.rb b/spec/mailers/emails/pages_domains_spec.rb index c74fd66ad22..050af587061 100644 --- a/spec/mailers/emails/pages_domains_spec.rb +++ b/spec/mailers/emails/pages_domains_spec.rb @@ -26,6 +26,26 @@ describe Emails::PagesDomains do end end + shared_examples 'notification about upcoming domain removal' do + context 'when domain is not scheduled for removal' do + it 'asks user to remove it' do + is_expected.to have_body_text 'please remove it' + end + end + + context 'when domain is scheduled for removal' do + before do + domain.update!(remove_at: 1.week.from_now) + end + it 'notifies user that domain will be removed automatically' do + aggregate_failures do + is_expected.to have_body_text domain.remove_at.strftime('%F %T') + is_expected.to have_body_text "it will be removed from your GitLab project" + end + end + end + end + describe '#pages_domain_enabled_email' do let(:email_subject) { "#{project.path} | GitLab Pages domain '#{domain.domain}' has been enabled" } @@ -43,6 +63,8 @@ describe Emails::PagesDomains do it_behaves_like 'a pages domain email' + it_behaves_like 'notification about upcoming domain removal' + it { is_expected.to have_body_text 'has been disabled' } end @@ -63,6 +85,8 @@ describe Emails::PagesDomains do it_behaves_like 'a pages domain email' + it_behaves_like 'notification about upcoming domain removal' + it 'says verification has failed and when the domain is enabled until' do is_expected.to have_body_text 'Verification has failed' is_expected.to have_body_text domain.enabled_until.strftime('%F %T') diff --git a/spec/services/verify_pages_domain_service_spec.rb b/spec/services/verify_pages_domain_service_spec.rb index ddf9d2b4917..b60b13bb3fc 100644 --- a/spec/services/verify_pages_domain_service_spec.rb +++ b/spec/services/verify_pages_domain_service_spec.rb @@ -27,6 +27,7 @@ describe VerifyPagesDomainService do expect(domain).to be_verified expect(domain).to be_enabled + expect(domain.remove_at).to be_nil end end @@ -48,18 +49,32 @@ describe VerifyPagesDomainService do end end + shared_examples 'unverifies and disables domain' do + it 'unverifies domain' do + expect(service.execute).to eq(error_status) + expect(domain).not_to be_verified + end + + it 'disables domain and shedules it for removal' do + Timecop.freeze do + service.execute + expect(domain).not_to be_enabled + expect(domain.remove_at).to be_within(1.second).of(1.week.from_now) + end + end + end + context 'when domain is disabled(or new)' do let(:domain) { create(:pages_domain, :disabled) } include_examples 'successful enablement and verification' - shared_examples 'unverifies and disables domain' do - it 'unverifies and disables domain' do - expect(service.execute).to eq(error_status) - - expect(domain).not_to be_verified - expect(domain).not_to be_enabled + context 'when txt record does not contain verification code' do + before do + stub_resolver(domain_name => 'something else') end + + include_examples 'unverifies and disables domain' end context 'when txt record does not contain verification code' do @@ -84,16 +99,25 @@ describe VerifyPagesDomainService do include_examples 'successful enablement and verification' - context 'when txt record does not contain verification code' do - before do - stub_resolver(domain_name => 'something else') - end - + shared_examples 'unverifing domain' do it 'unverifies but does not disable domain' do expect(service.execute).to eq(error_status) expect(domain).not_to be_verified expect(domain).to be_enabled end + + it 'does not schedule domain for removal' do + service.execute + expect(domain.remove_at).to be_nil + end + end + + context 'when txt record does not contain verification code' do + before do + stub_resolver(domain_name => 'something else') + end + + include_examples 'unverifing domain' end context 'when no txt records are present' do @@ -101,11 +125,7 @@ describe VerifyPagesDomainService do stub_resolver end - it 'unverifies but does not disable domain' do - expect(service.execute).to eq(error_status) - expect(domain).not_to be_verified - expect(domain).to be_enabled - end + include_examples 'unverifing domain' end end @@ -125,13 +145,40 @@ describe VerifyPagesDomainService do stub_resolver end - it 'disables domain' do - error_status[:message] += '. It is now disabled.' + let(:error_status) { { status: :error, message: "Couldn't verify #{domain.domain}. It is now disabled." } } - expect(service.execute).to eq(error_status) + include_examples 'unverifies and disables domain' + end + end - expect(domain).not_to be_verified - expect(domain).not_to be_enabled + context 'when domain is disabled and scheduled for removal' do + let(:domain) { create(:pages_domain, :disabled, :scheduled_for_removal) } + + context 'when the right code is present' do + before do + stub_resolver(domain.domain => domain.keyed_verification_code) + end + + it 'verifies and enables domain' do + expect(service.execute).to eq(status: :success) + + expect(domain).to be_verified + expect(domain).to be_enabled + end + + it 'prevent domain from being removed' do + expect { service.execute }.to change { domain.remove_at }.to(nil) + end + end + + context 'when the right code is not present' do + before do + stub_resolver + end + + it 'keeps domain scheduled for removal but does not change removal time' do + expect { service.execute }.not_to change { domain.remove_at } + expect(domain.remove_at).to be_present end end end -- cgit v1.2.1