diff options
author | Rob Watson <rob@mixlr.com> | 2018-01-03 11:07:03 +0300 |
---|---|---|
committer | Rob Watson <rob@mixlr.com> | 2018-03-22 21:58:36 +0300 |
commit | 9d45951fcaeda4f01a2e4be2480d980a3e7cd37e (patch) | |
tree | 536b456729edad79ba718d987a39f235587d4dfb /spec | |
parent | 53d352aaf4ce7f0d2bcaf04cce5252b753ef7938 (diff) |
Add HTTPS-only pages
Closes #28857
Diffstat (limited to 'spec')
-rw-r--r-- | spec/controllers/projects/pages_controller_spec.rb | 37 | ||||
-rw-r--r-- | spec/controllers/projects/pages_domains_controller_spec.rb | 4 | ||||
-rw-r--r-- | spec/factories/pages_domains.rb | 48 | ||||
-rw-r--r-- | spec/features/projects/pages_spec.rb | 90 | ||||
-rw-r--r-- | spec/lib/gitlab/import_export/safe_model_attributes.yml | 1 | ||||
-rw-r--r-- | spec/models/pages_domain_spec.rb | 146 | ||||
-rw-r--r-- | spec/models/project_spec.rb | 45 | ||||
-rw-r--r-- | spec/requests/api/pages_domains_spec.rb | 14 | ||||
-rw-r--r-- | spec/services/projects/update_service_spec.rb | 21 | ||||
-rw-r--r-- | spec/spec_helper.rb | 16 |
10 files changed, 322 insertions, 100 deletions
diff --git a/spec/controllers/projects/pages_controller_spec.rb b/spec/controllers/projects/pages_controller_spec.rb index 4705c50de7e..11f54eef531 100644 --- a/spec/controllers/projects/pages_controller_spec.rb +++ b/spec/controllers/projects/pages_controller_spec.rb @@ -65,4 +65,41 @@ describe Projects::PagesController do end end end + + describe 'PATCH update' do + let(:request_params) do + { + namespace_id: project.namespace, + project_id: project, + project: { pages_https_only: false } + } + end + + let(:update_service) { double(execute: { status: :success }) } + + before do + allow(Projects::UpdateService).to receive(:new) { update_service } + end + + it 'returns 302 status' do + patch :update, request_params + + expect(response).to have_gitlab_http_status(:found) + end + + it 'redirects back to the pages settings' do + patch :update, request_params + + expect(response).to redirect_to(project_pages_path(project)) + end + + it 'calls the update service' do + expect(Projects::UpdateService) + .to receive(:new) + .with(project, user, request_params[:project]) + .and_return(update_service) + + patch :update, request_params + end + end end diff --git a/spec/controllers/projects/pages_domains_controller_spec.rb b/spec/controllers/projects/pages_domains_controller_spec.rb index 83a3799e883..d4058a5c515 100644 --- a/spec/controllers/projects/pages_domains_controller_spec.rb +++ b/spec/controllers/projects/pages_domains_controller_spec.rb @@ -13,7 +13,7 @@ describe Projects::PagesDomainsController do end let(:pages_domain_params) do - build(:pages_domain, :with_certificate, :with_key, domain: 'my.otherdomain.com').slice(:key, :certificate, :domain) + build(:pages_domain, domain: 'my.otherdomain.com').slice(:key, :certificate, :domain) end before do @@ -68,7 +68,7 @@ describe Projects::PagesDomainsController do end let(:pages_domain_params) do - attributes_for(:pages_domain, :with_certificate, :with_key).slice(:key, :certificate) + attributes_for(:pages_domain).slice(:key, :certificate) end let(:params) do diff --git a/spec/factories/pages_domains.rb b/spec/factories/pages_domains.rb index 35b44e1c52e..20671da016e 100644 --- a/spec/factories/pages_domains.rb +++ b/spec/factories/pages_domains.rb @@ -4,25 +4,7 @@ FactoryBot.define do verified_at { Time.now } enabled_until { 1.week.from_now } - trait :disabled do - verified_at nil - enabled_until nil - end - - trait :unverified do - verified_at nil - end - - trait :reverify do - enabled_until { 1.hour.from_now } - end - - trait :expired do - enabled_until { 1.hour.ago } - end - - trait :with_certificate do - certificate '-----BEGIN CERTIFICATE----- + certificate '-----BEGIN CERTIFICATE----- MIICGzCCAYSgAwIBAgIBATANBgkqhkiG9w0BAQUFADAbMRkwFwYDVQQDExB0ZXN0 LWNlcnRpZmljYXRlMB4XDTE2MDIxMjE0MzIwMFoXDTIwMDQxMjE0MzIwMFowGzEZ MBcGA1UEAxMQdGVzdC1jZXJ0aWZpY2F0ZTCBnzANBgkqhkiG9w0BAQEFAAOBjQAw @@ -36,10 +18,8 @@ joZp2JHYvNlTPkRJ/J4TcXxBTJmArcQgTIuNoBtC+0A/SwdK4MfTCUY4vNWNdese 5A4K65Nb7Oh1AdQieTBHNXXCdyFsva9/ScfQGEl7p55a52jOPs0StPd7g64uvjlg YHi2yesCrOvVXt+lgPTd -----END CERTIFICATE-----' - end - trait :with_key do - key '-----BEGIN PRIVATE KEY----- + key '-----BEGIN PRIVATE KEY----- MIICdgIBADANBgkqhkiG9w0BAQEFAASCAmAwggJcAgEAAoGBAKS+CfS9GcRSdYSN SzyH5QJQBr5umRL6E+KilOV39iYFO/9oHjUdapTRWkrwnNPCp7qaeck4Jr8iv14t PVNDfNr76eGb6/3YknOAP0QOjLWunoC8kjU+N/JHU52NrUeX3qEy8EKV9LeCDJcB @@ -55,6 +35,30 @@ EPjGlXIT+aW2XiPmK3ZlCDcWIenE+lmtbOpI159Wpk8BGXs/s/xBAkEAlAY3ymgx 63BDJEwvOb2IaP8lDDxNsXx9XJNVvQbv5n15vNsLHbjslHfAhAbxnLQ1fLhUPqSi nNp/xedE1YxutQ== -----END PRIVATE KEY-----' + + trait :disabled do + verified_at nil + enabled_until nil + end + + trait :unverified do + verified_at nil + end + + trait :reverify do + enabled_until { 1.hour.from_now } + end + + trait :expired do + enabled_until { 1.hour.ago } + end + + trait :without_certificate do + certificate nil + end + + trait :without_key do + key nil end trait :with_missing_chain do diff --git a/spec/features/projects/pages_spec.rb b/spec/features/projects/pages_spec.rb index 233d2e67b9d..020738ae865 100644 --- a/spec/features/projects/pages_spec.rb +++ b/spec/features/projects/pages_spec.rb @@ -40,11 +40,6 @@ feature 'Pages' do end context 'when support for external domains is disabled' do - before do - allow(Gitlab.config.pages).to receive(:external_http).and_return(nil) - allow(Gitlab.config.pages).to receive(:external_https).and_return(nil) - end - it 'renders message that support is disabled' do visit project_pages_path(project) @@ -52,7 +47,9 @@ feature 'Pages' do end end - context 'when pages are exposed on external HTTP address' do + context 'when pages are exposed on external HTTP address', :http_pages_enabled do + given(:project) { create(:project, pages_https_only: false) } + shared_examples 'adds new domain' do it 'adds new domain' do visit new_project_pages_domain_path(project) @@ -64,11 +61,6 @@ feature 'Pages' do end end - before do - allow(Gitlab.config.pages).to receive(:external_http).and_return(['1.1.1.1:80']) - allow(Gitlab.config.pages).to receive(:external_https).and_return(nil) - end - it 'allows to add new domain' do visit project_pages_path(project) @@ -80,13 +72,13 @@ feature 'Pages' do context 'when project in group namespace' do it_behaves_like 'adds new domain' do let(:group) { create :group } - let(:project) { create :project, namespace: group } + let(:project) { create(:project, namespace: group, pages_https_only: false) } end end context 'when pages domain is added' do before do - project.pages_domains.create!(domain: 'my.test.domain.com') + create(:pages_domain, project: project, domain: 'my.test.domain.com') visit new_project_pages_domain_path(project) end @@ -104,7 +96,7 @@ feature 'Pages' do end end - context 'when pages are exposed on external HTTPS address' do + context 'when pages are exposed on external HTTPS address', :https_pages_enabled do let(:certificate_pem) do <<~PEM -----BEGIN CERTIFICATE----- @@ -145,11 +137,6 @@ feature 'Pages' do KEY end - before do - allow(Gitlab.config.pages).to receive(:external_http).and_return(['1.1.1.1:80']) - allow(Gitlab.config.pages).to receive(:external_https).and_return(['1.1.1.1:443']) - end - it 'adds new domain with certificate' do visit new_project_pages_domain_path(project) @@ -163,7 +150,7 @@ feature 'Pages' do describe 'updating the certificate for an existing domain' do let!(:domain) do - create(:pages_domain, :with_key, :with_certificate, project: project) + create(:pages_domain, project: project) end it 'allows the certificate to be updated' do @@ -237,6 +224,69 @@ feature 'Pages' do it_behaves_like 'no pages deployed' end + describe 'HTTPS settings', :js, :https_pages_enabled do + background do + project.namespace.update(owner: user) + + allow_any_instance_of(Project).to receive(:pages_deployed?) { true } + end + + scenario 'tries to change the setting' do + visit project_pages_path(project) + expect(page).to have_content("Force domains with SSL certificates to use HTTPS") + + uncheck :project_pages_https_only + + click_button 'Save' + + expect(page).to have_text('Your changes have been saved') + expect(page).not_to have_checked_field('project_pages_https_only') + end + + context 'setting could not be updated' do + before do + allow_any_instance_of(Projects::UpdateService) + .to receive(:execute) + .and_return(status: :error) + end + + scenario 'tries to change the setting' do + visit project_pages_path(project) + + uncheck :project_pages_https_only + + click_button 'Save' + + expect(page).to have_text('Something went wrong on our end') + end + end + + context 'non-HTTPS domain exists' do + given(:project) { create(:project, pages_https_only: false) } + + before do + create(:pages_domain, :without_key, :without_certificate, project: project) + end + + scenario 'the setting is disabled' do + visit project_pages_path(project) + + expect(page).to have_field(:project_pages_https_only, disabled: true) + expect(page).not_to have_button('Save') + end + end + + context 'HTTPS pages are disabled', :https_pages_disabled do + scenario 'the setting is unavailable' do + visit project_pages_path(project) + + expect(page).not_to have_field(:project_pages_https_only) + expect(page).not_to have_content('Force domains with SSL certificates to use HTTPS') + expect(page).not_to have_button('Save') + end + end + end + describe 'Remove page' do context 'when user is the owner' do let(:project) { create :project, :repository } diff --git a/spec/lib/gitlab/import_export/safe_model_attributes.yml b/spec/lib/gitlab/import_export/safe_model_attributes.yml index 0b938892da5..44e4c6ff94b 100644 --- a/spec/lib/gitlab/import_export/safe_model_attributes.yml +++ b/spec/lib/gitlab/import_export/safe_model_attributes.yml @@ -458,6 +458,7 @@ Project: - merge_requests_ff_only_enabled - merge_requests_rebase_enabled - jobs_cache_index +- pages_https_only Author: - name ProjectFeature: diff --git a/spec/models/pages_domain_spec.rb b/spec/models/pages_domain_spec.rb index 95713d8b85b..4b85c5e8720 100644 --- a/spec/models/pages_domain_spec.rb +++ b/spec/models/pages_domain_spec.rb @@ -18,24 +18,63 @@ describe PagesDomain do it { is_expected.to validate_uniqueness_of(:domain).case_insensitive } end - { - 'my.domain.com' => true, - '123.456.789' => true, - '0x12345.com' => true, - '0123123' => true, - '_foo.com' => false, - 'reserved.com' => false, - 'a.reserved.com' => false, - nil => false - }.each do |value, validity| - context "domain #{value.inspect} validity" do - before do - allow(Settings.pages).to receive(:host).and_return('reserved.com') + describe "hostname" do + { + 'my.domain.com' => true, + '123.456.789' => true, + '0x12345.com' => true, + '0123123' => true, + '_foo.com' => false, + 'reserved.com' => false, + 'a.reserved.com' => false, + nil => false + }.each do |value, validity| + context "domain #{value.inspect} validity" do + before do + allow(Settings.pages).to receive(:host).and_return('reserved.com') + end + + let(:domain) { value } + + it { expect(pages_domain.valid?).to eq(validity) } + end + end + end + + describe "HTTPS-only" do + using RSpec::Parameterized::TableSyntax + + let(:domain) { 'my.domain.com' } + + let(:project) do + instance_double(Project, pages_https_only?: pages_https_only) + end + + let(:pages_domain) do + build(:pages_domain, certificate: certificate, key: key).tap do |pd| + allow(pd).to receive(:project).and_return(project) + pd.valid? end + end - let(:domain) { value } + where(:pages_https_only, :certificate, :key, :errors_on) do + attributes = attributes_for(:pages_domain) + cert, key = attributes.fetch_values(:certificate, :key) + + true | nil | nil | %i(certificate key) + true | cert | nil | %i(key) + true | nil | key | %i(certificate key) + true | cert | key | [] + false | nil | nil | [] + false | cert | nil | %i(key) + false | nil | key | %i(key) + false | cert | key | [] + end - it { expect(pages_domain.valid?).to eq(validity) } + with_them do + it "is adds the expected errors" do + expect(pages_domain.errors.keys).to eq errors_on + end end end end @@ -43,26 +82,26 @@ describe PagesDomain do describe 'validate certificate' do subject { domain } - context 'when only certificate is specified' do - let(:domain) { build(:pages_domain, :with_certificate) } + context 'with matching key' do + let(:domain) { build(:pages_domain) } - it { is_expected.not_to be_valid } + it { is_expected.to be_valid } end - context 'when only key is specified' do - let(:domain) { build(:pages_domain, :with_key) } + context 'when no certificate is specified' do + let(:domain) { build(:pages_domain, :without_certificate) } it { is_expected.not_to be_valid } end - context 'with matching key' do - let(:domain) { build(:pages_domain, :with_certificate, :with_key) } + context 'when no key is specified' do + let(:domain) { build(:pages_domain, :without_key) } - it { is_expected.to be_valid } + it { is_expected.not_to be_valid } end context 'for not matching key' do - let(:domain) { build(:pages_domain, :with_missing_chain, :with_key) } + let(:domain) { build(:pages_domain, :with_missing_chain) } it { is_expected.not_to be_valid } end @@ -103,30 +142,26 @@ describe PagesDomain do describe '#url' do subject { domain.url } - context 'without the certificate' do - let(:domain) { build(:pages_domain, certificate: '') } + let(:domain) { build(:pages_domain) } - it { is_expected.to eq("http://#{domain.domain}") } - end + it { is_expected.to eq("https://#{domain.domain}") } - context 'with a certificate' do - let(:domain) { build(:pages_domain, :with_certificate) } + context 'without the certificate' do + let(:domain) { build(:pages_domain, :without_certificate) } - it { is_expected.to eq("https://#{domain.domain}") } + it { is_expected.to eq("http://#{domain.domain}") } end end describe '#has_matching_key?' do subject { domain.has_matching_key? } - context 'for matching key' do - let(:domain) { build(:pages_domain, :with_certificate, :with_key) } + let(:domain) { build(:pages_domain) } - it { is_expected.to be_truthy } - end + it { is_expected.to be_truthy } context 'for invalid key' do - let(:domain) { build(:pages_domain, :with_missing_chain, :with_key) } + let(:domain) { build(:pages_domain, :with_missing_chain) } it { is_expected.to be_falsey } end @@ -136,7 +171,7 @@ describe PagesDomain do subject { domain.has_intermediates? } context 'for self signed' do - let(:domain) { build(:pages_domain, :with_certificate) } + let(:domain) { build(:pages_domain) } it { is_expected.to be_truthy } end @@ -162,7 +197,7 @@ describe PagesDomain do subject { domain.expired? } context 'for valid' do - let(:domain) { build(:pages_domain, :with_certificate) } + let(:domain) { build(:pages_domain) } it { is_expected.to be_falsey } end @@ -175,7 +210,7 @@ describe PagesDomain do end describe '#subject' do - let(:domain) { build(:pages_domain, :with_certificate) } + let(:domain) { build(:pages_domain) } subject { domain.subject } @@ -183,7 +218,7 @@ describe PagesDomain do end describe '#certificate_text' do - let(:domain) { build(:pages_domain, :with_certificate) } + let(:domain) { build(:pages_domain) } subject { domain.certificate_text } @@ -191,6 +226,18 @@ describe PagesDomain do it { is_expected.not_to be_empty } end + describe "#https?" do + context "when a certificate is present" do + subject { build(:pages_domain) } + it { is_expected.to be_https } + end + + context "when no certificate is present" do + subject { build(:pages_domain, :without_certificate) } + it { is_expected.not_to be_https } + end + end + describe '#update_daemon' do it 'runs when the domain is created' do domain = build(:pages_domain) @@ -267,29 +314,30 @@ describe PagesDomain do end context 'TLS configuration' do - set(:domain_with_tls) { create(:pages_domain, :with_key, :with_certificate) } + set(:domain_without_tls) { create(:pages_domain, :without_certificate, :without_key) } + set(:domain) { create(:pages_domain) } - let(:cert1) { domain_with_tls.certificate } + let(:cert1) { domain.certificate } let(:cert2) { cert1 + ' ' } - let(:key1) { domain_with_tls.key } + let(:key1) { domain.key } let(:key2) { key1 + ' ' } it 'updates when added' do - expect(domain).to receive(:update_daemon) + expect(domain_without_tls).to receive(:update_daemon) - domain.update!(key: key1, certificate: cert1) + domain_without_tls.update!(key: key1, certificate: cert1) end it 'updates when changed' do - expect(domain_with_tls).to receive(:update_daemon) + expect(domain).to receive(:update_daemon) - domain_with_tls.update!(key: key2, certificate: cert2) + domain.update!(key: key2, certificate: cert2) end it 'updates when removed' do - expect(domain_with_tls).to receive(:update_daemon) + expect(domain).to receive(:update_daemon) - domain_with_tls.update!(key: nil, certificate: nil) + domain.update!(key: nil, certificate: nil) end end end diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index 4cf8d861595..c522ab7c447 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -3479,4 +3479,49 @@ describe Project do end end end + + describe "#pages_https_only?" do + subject { build(:project) } + + context "when HTTPS pages are disabled" do + it { is_expected.not_to be_pages_https_only } + end + + context "when HTTPS pages are enabled", :https_pages_enabled do + it { is_expected.to be_pages_https_only } + end + end + + describe "#pages_https_only? validation", :https_pages_enabled do + subject(:project) do + # set-up dirty object: + create(:project, pages_https_only: false).tap do |p| + p.pages_https_only = true + end + end + + context "when no domains are associated" do + it { is_expected.to be_valid } + end + + context "when domains including keys and certificates are associated" do + before do + allow(project) + .to receive(:pages_domains) + .and_return([instance_double(PagesDomain, https?: true)]) + end + + it { is_expected.to be_valid } + end + + context "when domains including no keys or certificates are associated" do + before do + allow(project) + .to receive(:pages_domains) + .and_return([instance_double(PagesDomain, https?: false)]) + end + + it { is_expected.not_to be_valid } + end + end end diff --git a/spec/requests/api/pages_domains_spec.rb b/spec/requests/api/pages_domains_spec.rb index dc3a116c060..a9ccbb32666 100644 --- a/spec/requests/api/pages_domains_spec.rb +++ b/spec/requests/api/pages_domains_spec.rb @@ -1,17 +1,17 @@ require 'rails_helper' describe API::PagesDomains do - set(:project) { create(:project, path: 'my.project') } + set(:project) { create(:project, path: 'my.project', pages_https_only: false) } set(:user) { create(:user) } set(:admin) { create(:admin) } - set(:pages_domain) { create(:pages_domain, domain: 'www.domain.test', project: project) } - set(:pages_domain_secure) { create(:pages_domain, :with_certificate, :with_key, domain: 'ssl.domain.test', project: project) } - set(:pages_domain_expired) { create(:pages_domain, :with_expired_certificate, :with_key, domain: 'expired.domain.test', project: project) } + set(:pages_domain) { create(:pages_domain, :without_key, :without_certificate, domain: 'www.domain.test', project: project) } + set(:pages_domain_secure) { create(:pages_domain, domain: 'ssl.domain.test', project: project) } + set(:pages_domain_expired) { create(:pages_domain, :with_expired_certificate, domain: 'expired.domain.test', project: project) } - let(:pages_domain_params) { build(:pages_domain, domain: 'www.other-domain.test').slice(:domain) } - let(:pages_domain_secure_params) { build(:pages_domain, :with_certificate, :with_key, domain: 'ssl.other-domain.test', project: project).slice(:domain, :certificate, :key) } - let(:pages_domain_secure_key_missmatch_params) {build(:pages_domain, :with_trusted_chain, :with_key, project: project).slice(:domain, :certificate, :key) } + let(:pages_domain_params) { build(:pages_domain, :without_key, :without_certificate, domain: 'www.other-domain.test').slice(:domain) } + let(:pages_domain_secure_params) { build(:pages_domain, domain: 'ssl.other-domain.test', project: project).slice(:domain, :certificate, :key) } + let(:pages_domain_secure_key_missmatch_params) {build(:pages_domain, :with_trusted_chain, project: project).slice(:domain, :certificate, :key) } let(:pages_domain_secure_missing_chain_params) {build(:pages_domain, :with_missing_chain, project: project).slice(:certificate) } let(:route) { "/projects/#{project.id}/pages/domains" } diff --git a/spec/services/projects/update_service_spec.rb b/spec/services/projects/update_service_spec.rb index f3f97b6b921..497c1949256 100644 --- a/spec/services/projects/update_service_spec.rb +++ b/spec/services/projects/update_service_spec.rb @@ -241,6 +241,27 @@ describe Projects::UpdateService do }) end end + + context 'when updating #pages_https_only', :https_pages_enabled do + subject(:call_service) do + update_project(project, admin, pages_https_only: false) + end + + it 'updates the attribute' do + expect { call_service } + .to change { project.pages_https_only? } + .to(false) + end + + it 'calls Projects::UpdatePagesConfigurationService' do + expect(Projects::UpdatePagesConfigurationService) + .to receive(:new) + .with(project) + .and_call_original + + call_service + end + end end describe '#run_auto_devops_pipeline?' do diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index 9f6f0204a16..5051cd34564 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -197,6 +197,22 @@ RSpec.configure do |config| Ability.allowed?(*args) end end + + config.before(:each, :http_pages_enabled) do |_| + allow(Gitlab.config.pages).to receive(:external_http).and_return(['1.1.1.1:80']) + end + + config.before(:each, :https_pages_enabled) do |_| + allow(Gitlab.config.pages).to receive(:external_https).and_return(['1.1.1.1:443']) + end + + config.before(:each, :http_pages_disabled) do |_| + allow(Gitlab.config.pages).to receive(:external_http).and_return(false) + end + + config.before(:each, :https_pages_disabled) do |_| + allow(Gitlab.config.pages).to receive(:external_https).and_return(false) + end end # add simpler way to match asset paths containing digest strings |