From 432f2bbc9cf64d53418c1eb9bb701254803a0e1e Mon Sep 17 00:00:00 2001 From: Vladimir Shushlin Date: Thu, 13 Jun 2019 14:37:45 +0300 Subject: Use project depended feature flag for pages ssl Also add ::Gitlab::LetsEncrypt.enabled? shortcut and simplify it a lot --- app/views/projects/pages_domains/_form.html.haml | 2 +- .../pages_domain_ssl_renewal_cron_worker.rb | 4 +- app/workers/pages_domain_ssl_renewal_worker.rb | 6 +-- lib/gitlab/lets_encrypt.rb | 16 +++++++ lib/gitlab/lets_encrypt/client.rb | 10 +--- spec/lib/gitlab/lets_encrypt/client_spec.rb | 36 -------------- spec/lib/gitlab/lets_encrypt_spec.rb | 56 ++++++++++++++++++++++ .../pages_domain_ssl_renewal_cron_worker_spec.rb | 13 +++-- .../pages_domain_ssl_renewal_worker_spec.rb | 19 ++++++-- 9 files changed, 101 insertions(+), 61 deletions(-) create mode 100644 lib/gitlab/lets_encrypt.rb create mode 100644 spec/lib/gitlab/lets_encrypt_spec.rb diff --git a/app/views/projects/pages_domains/_form.html.haml b/app/views/projects/pages_domains/_form.html.haml index e7edb93f05b..5b657966909 100644 --- a/app/views/projects/pages_domains/_form.html.haml +++ b/app/views/projects/pages_domains/_form.html.haml @@ -11,7 +11,7 @@ - if Gitlab.config.pages.external_https - - auto_ssl_available = ::Gitlab::LetsEncrypt::Client.new.enabled? + - auto_ssl_available = ::Gitlab::LetsEncrypt.enabled?(@domain) - auto_ssl_enabled = @domain.auto_ssl_enabled? - auto_ssl_available_and_enabled = auto_ssl_available && auto_ssl_enabled diff --git a/app/workers/pages_domain_ssl_renewal_cron_worker.rb b/app/workers/pages_domain_ssl_renewal_cron_worker.rb index 4ca9db922b4..40c34d29970 100644 --- a/app/workers/pages_domain_ssl_renewal_cron_worker.rb +++ b/app/workers/pages_domain_ssl_renewal_cron_worker.rb @@ -5,9 +5,9 @@ class PagesDomainSslRenewalCronWorker include CronjobQueue def perform - return unless ::Gitlab::LetsEncrypt::Client.new.enabled? - PagesDomain.need_auto_ssl_renewal.find_each do |domain| + next unless ::Gitlab::LetsEncrypt.enabled?(domain) + PagesDomainSslRenewalWorker.perform_async(domain.id) end end diff --git a/app/workers/pages_domain_ssl_renewal_worker.rb b/app/workers/pages_domain_ssl_renewal_worker.rb index 00c9c4782d8..b32458ca777 100644 --- a/app/workers/pages_domain_ssl_renewal_worker.rb +++ b/app/workers/pages_domain_ssl_renewal_worker.rb @@ -4,11 +4,9 @@ class PagesDomainSslRenewalWorker include ApplicationWorker def perform(domain_id) - return unless ::Gitlab::LetsEncrypt::Client.new.enabled? - domain = PagesDomain.find_by_id(domain_id) - - return unless domain + return unless domain&.enabled? + return unless ::Gitlab::LetsEncrypt.enabled?(domain) ::PagesDomains::ObtainLetsEncryptCertificateService.new(domain).execute end diff --git a/lib/gitlab/lets_encrypt.rb b/lib/gitlab/lets_encrypt.rb new file mode 100644 index 00000000000..cdf24f24647 --- /dev/null +++ b/lib/gitlab/lets_encrypt.rb @@ -0,0 +1,16 @@ +# frozen_string_literal: true + +module Gitlab + module LetsEncrypt + def self.enabled?(pages_domain = nil) + return false unless Gitlab::CurrentSettings.lets_encrypt_terms_of_service_accepted + + return false unless Feature.enabled?(:pages_auto_ssl) + + # If no domain is passed, just check whether we're enabled globally + return true unless pages_domain + + !!pages_domain.project && Feature.enabled?(:pages_auto_ssl_for_project, pages_domain.project) + end + end +end diff --git a/lib/gitlab/lets_encrypt/client.rb b/lib/gitlab/lets_encrypt/client.rb index 66aea137012..ad2921ed555 100644 --- a/lib/gitlab/lets_encrypt/client.rb +++ b/lib/gitlab/lets_encrypt/client.rb @@ -34,14 +34,6 @@ module Gitlab acme_client.terms_of_service end - def enabled? - return false unless Feature.enabled?(:pages_auto_ssl) - - return false unless private_key - - Gitlab::CurrentSettings.lets_encrypt_terms_of_service_accepted - end - private def acme_client @@ -65,7 +57,7 @@ module Gitlab end def ensure_account - raise 'Acme integration is disabled' unless enabled? + raise 'Acme integration is disabled' unless ::Gitlab::LetsEncrypt.enabled? @acme_account ||= acme_client.new_account(contact: contact, terms_of_service_agreed: true) end diff --git a/spec/lib/gitlab/lets_encrypt/client_spec.rb b/spec/lib/gitlab/lets_encrypt/client_spec.rb index 5454d9c1af4..cbb862cb0c9 100644 --- a/spec/lib/gitlab/lets_encrypt/client_spec.rb +++ b/spec/lib/gitlab/lets_encrypt/client_spec.rb @@ -116,42 +116,6 @@ describe ::Gitlab::LetsEncrypt::Client do end end - describe '#enabled?' do - subject { client.enabled? } - - context 'when terms of service are accepted' do - it { is_expected.to eq(true) } - - context "when private_key isn't present and database is read only" do - before do - allow(::Gitlab::Database).to receive(:read_only?).and_return(true) - end - - it 'returns false' do - expect(::Gitlab::CurrentSettings.lets_encrypt_private_key).to eq(nil) - - is_expected.to eq(false) - end - end - - context 'when feature flag is disabled' do - before do - stub_feature_flags(pages_auto_ssl: false) - end - - it { is_expected.to eq(false) } - end - end - - context 'when terms of service are not accepted' do - before do - stub_application_setting(lets_encrypt_terms_of_service_accepted: false) - end - - it { is_expected.to eq(false) } - end - end - describe '#terms_of_service_url' do subject { client.terms_of_service_url } diff --git a/spec/lib/gitlab/lets_encrypt_spec.rb b/spec/lib/gitlab/lets_encrypt_spec.rb new file mode 100644 index 00000000000..674b114e9d3 --- /dev/null +++ b/spec/lib/gitlab/lets_encrypt_spec.rb @@ -0,0 +1,56 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe ::Gitlab::LetsEncrypt do + include LetsEncryptHelpers + + before do + stub_lets_encrypt_settings + end + + describe '.enabled?' do + let(:project) { create(:project) } + let(:pages_domain) { create(:pages_domain, project: project) } + + subject { described_class.enabled?(pages_domain) } + + context 'when terms of service are accepted' do + it { is_expected.to eq(true) } + + context 'when feature flag is disabled' do + before do + stub_feature_flags(pages_auto_ssl: false) + end + + it { is_expected.to eq(false) } + end + end + + context 'when terms of service are not accepted' do + before do + stub_application_setting(lets_encrypt_terms_of_service_accepted: false) + end + + it { is_expected.to eq(false) } + end + + context 'when feature flag for project is disabled' do + before do + stub_feature_flags(pages_auto_ssl_for_project: false) + end + + it 'returns false' do + is_expected.to eq(false) + end + end + + context 'when domain has not project' do + let(:pages_domain) { create(:pages_domain) } + + it 'returns false' do + is_expected.to eq(false) + end + end + end +end diff --git a/spec/workers/pages_domain_ssl_renewal_cron_worker_spec.rb b/spec/workers/pages_domain_ssl_renewal_cron_worker_spec.rb index 2ae4872f51d..08a3511f70b 100644 --- a/spec/workers/pages_domain_ssl_renewal_cron_worker_spec.rb +++ b/spec/workers/pages_domain_ssl_renewal_cron_worker_spec.rb @@ -12,15 +12,18 @@ describe PagesDomainSslRenewalCronWorker do end describe '#perform' do - let!(:domain) { create(:pages_domain) } - let!(:domain_with_enabled_auto_ssl) { create(:pages_domain, auto_ssl_enabled: true) } - let!(:domain_with_obtained_letsencrypt) { create(:pages_domain, :letsencrypt, auto_ssl_enabled: true) } + let(:project) { create :project } + let!(:domain) { create(:pages_domain, project: project) } + let!(:domain_with_enabled_auto_ssl) { create(:pages_domain, project: project, auto_ssl_enabled: true) } + let!(:domain_with_obtained_letsencrypt) do + create(:pages_domain, :letsencrypt, project: project, auto_ssl_enabled: true) + end let!(:domain_without_auto_certificate) do - create(:pages_domain, :without_certificate, :without_key, auto_ssl_enabled: true) + create(:pages_domain, :without_certificate, :without_key, project: project, auto_ssl_enabled: true) end let!(:domain_with_expired_auto_ssl) do - create(:pages_domain, :letsencrypt, :with_expired_certificate) + create(:pages_domain, :letsencrypt, :with_expired_certificate, project: project) end it 'enqueues a PagesDomainSslRenewalWorker for domains needing renewal' do diff --git a/spec/workers/pages_domain_ssl_renewal_worker_spec.rb b/spec/workers/pages_domain_ssl_renewal_worker_spec.rb index a3d33de1b40..3552ff0823a 100644 --- a/spec/workers/pages_domain_ssl_renewal_worker_spec.rb +++ b/spec/workers/pages_domain_ssl_renewal_worker_spec.rb @@ -7,7 +7,8 @@ describe PagesDomainSslRenewalWorker do subject(:worker) { described_class.new } - let(:domain) { create(:pages_domain) } + let(:project) { create(:project) } + let(:domain) { create(:pages_domain, project: project) } before do stub_lets_encrypt_settings @@ -22,14 +23,24 @@ describe PagesDomainSslRenewalWorker do worker.perform(domain.id) end + shared_examples 'does nothing' do + it 'does nothing' do + expect(::PagesDomains::ObtainLetsEncryptCertificateService).not_to receive(:new) + end + end + context 'when domain was deleted' do before do domain.destroy! end - it 'does nothing' do - expect(::PagesDomains::ObtainLetsEncryptCertificateService).not_to receive(:new) - end + include_examples 'does nothing' + end + + context 'when domain is disabled' do + let(:domain) { create(:pages_domain, :disabled) } + + include_examples 'does nothing' end end end -- cgit v1.2.1