Merge branch 'remove-disabled-pages-domains' into 'master'

Mark disabled pages domain for removal

See merge request gitlab-org/gitlab-ce!26212

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)

app/views/notify/_removal_notification.html.haml

+- 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.

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'

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'

changelogs/unreleased/remove-disabled-pages-domains.yml

+---
+title: Mark disabled pages domains for removal, but don't remove them yet
+merge_request: 26212
+author:
+type: added

db/migrate/20190312113229_add_remove_at_to_pages_domains.rb

+# 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

db/migrate/20190312113634_add_remove_at_index_to_pages_domains.rb

+# 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

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

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

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

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')

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