diff options
author | Vladimir Shushlin <vshushlin@gitlab.com> | 2019-06-21 12:06:12 +0000 |
---|---|---|
committer | Nick Thomas <nick@gitlab.com> | 2019-06-21 12:06:12 +0000 |
commit | 6119d5ad7518f547af216d3bdc3d1fcffcfc9c71 (patch) | |
tree | cc1a1e74efb5e44bd5c5d152da85b7fed4ff2107 /spec | |
parent | 176164d37423ffb39d293341799aff757f050d7c (diff) | |
download | gitlab-ce-6119d5ad7518f547af216d3bdc3d1fcffcfc9c71.tar.gz |
Don't show private keys for letsencrypt certs
Adds enum certificate_source to pages_domains table
with default manually_uploaded
Mark certificates as 'gitlab_provided'
if the were obtained through Let's Encrypt
Mark certificates as 'user_provided' if they were uploaded through
controller or api
Only show private key in domain edit form if it is 'user_provided'
Only show LetsEncrypt option if is enabled by application settings
(and feature flag)
Refactor and fix some specs to match new logic
Don't show Let's Encrypt certificates as well
Diffstat (limited to 'spec')
-rw-r--r-- | spec/controllers/projects/pages_domains_controller_spec.rb | 64 | ||||
-rw-r--r-- | spec/factories/pages_domains.rb | 4 | ||||
-rw-r--r-- | spec/features/projects/pages_lets_encrypt_spec.rb | 153 | ||||
-rw-r--r-- | spec/features/projects/pages_spec.rb | 16 | ||||
-rw-r--r-- | spec/models/pages_domain_spec.rb | 96 | ||||
-rw-r--r-- | spec/requests/api/pages_domains_spec.rb | 8 | ||||
-rw-r--r-- | spec/services/pages_domains/obtain_lets_encrypt_certificate_service_spec.rb | 6 |
7 files changed, 234 insertions, 113 deletions
diff --git a/spec/controllers/projects/pages_domains_controller_spec.rb b/spec/controllers/projects/pages_domains_controller_spec.rb index ff3afd51cd8..032f4f1418f 100644 --- a/spec/controllers/projects/pages_domains_controller_spec.rb +++ b/spec/controllers/projects/pages_domains_controller_spec.rb @@ -15,7 +15,10 @@ describe Projects::PagesDomainsController do end let(:pages_domain_params) do - build(:pages_domain, domain: 'my.otherdomain.com').slice(:key, :certificate, :domain) + attributes_for(:pages_domain, domain: 'my.otherdomain.com').slice(:key, :certificate, :domain).tap do |params| + params[:user_provided_key] = params.delete(:key) + params[:user_provided_certificate] = params.delete(:certificate) + end end before do @@ -84,48 +87,59 @@ describe Projects::PagesDomainsController do controller.instance_variable_set(:@domain, pages_domain) end - let(:pages_domain_params) do - attributes_for(:pages_domain).slice(:key, :certificate) - end - let(:params) do request_params.merge(id: pages_domain.domain, pages_domain: pages_domain_params) end - it 'updates the domain' do - expect(pages_domain) - .to receive(:update) - .with(ActionController::Parameters.new(pages_domain_params).permit!) - .and_return(true) + context 'with valid params' do + let(:pages_domain_params) do + attributes_for(:pages_domain, :with_trusted_chain).slice(:key, :certificate).tap do |params| + params[:user_provided_key] = params.delete(:key) + params[:user_provided_certificate] = params.delete(:certificate) + end + end + + it 'updates the domain' do + expect do + patch(:update, params: params) + end.to change { pages_domain.reload.certificate }.to(pages_domain_params[:user_provided_certificate]) + end + + it 'redirects to the project page' do + patch(:update, params: params) - patch(:update, params: params) + expect(flash[:notice]).to eq 'Domain was updated' + expect(response).to redirect_to(project_pages_path(project)) + end end - it 'redirects to the project page' do - patch(:update, params: params) + context 'with key parameter' do + before do + pages_domain.update!(key: nil, certificate: nil, certificate_source: 'gitlab_provided') + end - expect(flash[:notice]).to eq 'Domain was updated' - expect(response).to redirect_to(project_pages_path(project)) + it 'marks certificate as provided by user' do + expect do + patch(:update, params: params) + end.to change { pages_domain.reload.certificate_source }.from('gitlab_provided').to('user_provided') + end end context 'the domain is invalid' do - it 'renders the edit action' do - allow(pages_domain).to receive(:update).and_return(false) + let(:pages_domain_params) { { user_provided_certificate: 'blabla' } } + it 'renders the edit action' do patch(:update, params: params) expect(response).to render_template('edit') end end - context 'the parameters include the domain' do - it 'renders 400 Bad Request' do - expect(pages_domain) - .to receive(:update) - .with(hash_not_including(:domain)) - .and_return(true) - - patch(:update, params: params.deep_merge(pages_domain: { domain: 'abc' })) + context 'when parameters include the domain' do + it 'does not update domain' do + expect do + patch(:update, params: params.deep_merge(pages_domain: { domain: 'abc' })) + end.not_to change { pages_domain.reload.domain } end end end diff --git a/spec/factories/pages_domains.rb b/spec/factories/pages_domains.rb index db8384877b0..8da19a37a6a 100644 --- a/spec/factories/pages_domains.rb +++ b/spec/factories/pages_domains.rb @@ -180,5 +180,9 @@ Iy6oRpHaCF/2obZdIdgf9rlyz0fkqyHJc9GkioSoOhJZxEV2SgAkap8yS0sX2tJ9 ZDXgrA== -----END CERTIFICATE-----' end + + trait :letsencrypt do + certificate_source { :gitlab_provided } + end end end diff --git a/spec/features/projects/pages_lets_encrypt_spec.rb b/spec/features/projects/pages_lets_encrypt_spec.rb index baa217cbe58..a5f8702302c 100644 --- a/spec/features/projects/pages_lets_encrypt_spec.rb +++ b/spec/features/projects/pages_lets_encrypt_spec.rb @@ -2,124 +2,119 @@ require 'spec_helper' describe "Pages with Let's Encrypt", :https_pages_enabled do + include LetsEncryptHelpers + let(:project) { create(:project) } let(:user) { create(:user) } let(:role) { :maintainer } - let(:certificate_pem) do - <<~PEM - -----BEGIN CERTIFICATE----- - MIICGzCCAYSgAwIBAgIBATANBgkqhkiG9w0BAQUFADAbMRkwFwYDVQQDExB0ZXN0 - LWNlcnRpZmljYXRlMB4XDTE2MDIxMjE0MzIwMFoXDTIwMDQxMjE0MzIwMFowGzEZ - MBcGA1UEAxMQdGVzdC1jZXJ0aWZpY2F0ZTCBnzANBgkqhkiG9w0BAQEFAAOBjQAw - gYkCgYEApL4J9L0ZxFJ1hI1LPIflAlAGvm6ZEvoT4qKU5Xf2JgU7/2geNR1qlNFa - SvCc08Knupp5yTgmvyK/Xi09U0N82vvp4Zvr/diSc4A/RA6Mta6egLySNT438kdT - nY2tR5feoTLwQpX0t4IMlwGQGT5h6Of2fKmDxzuwuyffcIHqLdsCAwEAAaNvMG0w - DAYDVR0TAQH/BAIwADAdBgNVHQ4EFgQUxl9WSxBprB0z0ibJs3rXEk0+95AwCwYD - VR0PBAQDAgXgMBEGCWCGSAGG+EIBAQQEAwIGQDAeBglghkgBhvhCAQ0EERYPeGNh - IGNlcnRpZmljYXRlMA0GCSqGSIb3DQEBBQUAA4GBAGC4T8SlFHK0yPSa+idGLQFQ - joZp2JHYvNlTPkRJ/J4TcXxBTJmArcQgTIuNoBtC+0A/SwdK4MfTCUY4vNWNdese - 5A4K65Nb7Oh1AdQieTBHNXXCdyFsva9/ScfQGEl7p55a52jOPs0StPd7g64uvjlg - YHi2yesCrOvVXt+lgPTd - -----END CERTIFICATE----- - PEM - end + let(:certificate_pem) { attributes_for(:pages_domain)[:certificate] } - let(:certificate_key) do - <<~KEY - -----BEGIN PRIVATE KEY----- - MIICdgIBADANBgkqhkiG9w0BAQEFAASCAmAwggJcAgEAAoGBAKS+CfS9GcRSdYSN - SzyH5QJQBr5umRL6E+KilOV39iYFO/9oHjUdapTRWkrwnNPCp7qaeck4Jr8iv14t - PVNDfNr76eGb6/3YknOAP0QOjLWunoC8kjU+N/JHU52NrUeX3qEy8EKV9LeCDJcB - kBk+Yejn9nypg8c7sLsn33CB6i3bAgMBAAECgYA2D26w80T7WZvazYr86BNMePpd - j2mIAqx32KZHzt/lhh40J/SRtX9+Kl0Y7nBoRR5Ja9u/HkAIxNxLiUjwg9r6cpg/ - uITEF5nMt7lAk391BuI+7VOZZGbJDsq2ulPd6lO+C8Kq/PI/e4kXcIjeH6KwQsuR - 5vrXfBZ3sQfflaiN4QJBANBt8JY2LIGQF8o89qwUpRL5vbnKQ4IzZ5+TOl4RLR7O - AQpJ81tGuINghO7aunctb6rrcKJrxmEH1whzComybrMCQQDKV49nOBudRBAIgG4K - EnLzsRKISUHMZSJiYTYnablof8cKw1JaQduw7zgrUlLwnroSaAGX88+Jw1f5n2Lh - Vlg5AkBDdUGnrDLtYBCDEQYZHblrkc7ZAeCllDOWjxUV+uMqlCv8A4Ey6omvY57C - m6I8DkWVAQx8VPtozhvHjUw80rZHAkB55HWHAM3h13axKG0htCt7klhPsZHpx6MH - EPjGlXIT+aW2XiPmK3ZlCDcWIenE+lmtbOpI159Wpk8BGXs/s/xBAkEAlAY3ymgx - 63BDJEwvOb2IaP8lDDxNsXx9XJNVvQbv5n15vNsLHbjslHfAhAbxnLQ1fLhUPqSi - nNp/xedE1YxutQ== - -----END PRIVATE KEY----- - KEY - end + let(:certificate_key) { attributes_for(:pages_domain)[:key] } before do allow(Gitlab.config.pages).to receive(:enabled).and_return(true) + stub_lets_encrypt_settings + project.add_role(user, role) sign_in(user) project.namespace.update(owner: user) allow_any_instance_of(Project).to receive(:pages_deployed?) { true } end - context 'when the page_auto_ssl feature flag is enabled' do - before do - stub_feature_flags(pages_auto_ssl: true) + context 'when the auto SSL management is initially disabled' do + let(:domain) do + create(:pages_domain, auto_ssl_enabled: false, project: project) end - context 'when the auto SSL management is initially disabled' do - let(:domain) do - create(:pages_domain, auto_ssl_enabled: false, project: project) - end + it 'enables auto SSL and dynamically updates the form accordingly', :js do + visit edit_project_pages_domain_path(project, domain) - it 'enables auto SSL and dynamically updates the form accordingly', :js do - visit edit_project_pages_domain_path(project, domain) + expect(domain.auto_ssl_enabled).to eq false - expect(domain.auto_ssl_enabled).to eq false + expect(find("#pages_domain_auto_ssl_enabled", visible: false).value).to eq 'false' + expect(page).to have_field 'Certificate (PEM)', type: 'textarea' + expect(page).to have_field 'Key (PEM)', type: 'textarea' - expect(find("#pages_domain_auto_ssl_enabled", visible: false).value).to eq 'false' - expect(page).to have_field 'Certificate (PEM)', type: 'textarea' - expect(page).to have_field 'Key (PEM)', type: 'textarea' + find('.js-auto-ssl-toggle-container .project-feature-toggle').click - find('.js-auto-ssl-toggle-container .project-feature-toggle').click + expect(find("#pages_domain_auto_ssl_enabled", visible: false).value).to eq 'true' + expect(page).not_to have_field 'Certificate (PEM)', type: 'textarea' + expect(page).not_to have_field 'Key (PEM)', type: 'textarea' - expect(find("#pages_domain_auto_ssl_enabled", visible: false).value).to eq 'true' - expect(page).not_to have_field 'Certificate (PEM)', type: 'textarea' - expect(page).not_to have_field 'Key (PEM)', type: 'textarea' - expect(page).to have_content "The certificate will be shown here once it has been obtained from Let's Encrypt. This process may take up to an hour to complete." + click_on 'Save Changes' - click_on 'Save Changes' + expect(domain.reload.auto_ssl_enabled).to eq true + end + end - expect(domain.reload.auto_ssl_enabled).to eq true - end + context 'when the auto SSL management is initially enabled' do + let(:domain) do + create(:pages_domain, :letsencrypt, auto_ssl_enabled: true, project: project) end - context 'when the auto SSL management is initially enabled' do - let(:domain) do - create(:pages_domain, auto_ssl_enabled: true, project: project) - end + it 'disables auto SSL and dynamically updates the form accordingly', :js do + visit edit_project_pages_domain_path(project, domain) - it 'disables auto SSL and dynamically updates the form accordingly', :js do - visit edit_project_pages_domain_path(project, domain) + expect(find("#pages_domain_auto_ssl_enabled", visible: false).value).to eq 'true' + expect(page).not_to have_field 'Certificate (PEM)', type: 'textarea' + expect(page).not_to have_field 'Key (PEM)', type: 'textarea' - expect(find("#pages_domain_auto_ssl_enabled", visible: false).value).to eq 'true' - expect(page).to have_field 'Certificate (PEM)', type: 'textarea', disabled: true - expect(page).not_to have_field 'Key (PEM)', type: 'textarea' + find('.js-auto-ssl-toggle-container .project-feature-toggle').click - find('.js-auto-ssl-toggle-container .project-feature-toggle').click + expect(find("#pages_domain_auto_ssl_enabled", visible: false).value).to eq 'false' + expect(page).to have_field 'Certificate (PEM)', type: 'textarea' + expect(page).to have_field 'Key (PEM)', type: 'textarea' - expect(find("#pages_domain_auto_ssl_enabled", visible: false).value).to eq 'false' - expect(page).to have_field 'Certificate (PEM)', type: 'textarea' - expect(page).to have_field 'Key (PEM)', type: 'textarea' + fill_in 'Certificate (PEM)', with: certificate_pem + fill_in 'Key (PEM)', with: certificate_key - fill_in 'Certificate (PEM)', with: certificate_pem - fill_in 'Key (PEM)', with: certificate_key + click_on 'Save Changes' - click_on 'Save Changes' + expect(domain.reload.auto_ssl_enabled).to eq false + end + end - expect(domain.reload.auto_ssl_enabled).to eq false + shared_examples 'user sees private keys only for user provided certificate' do + before do + visit edit_project_pages_domain_path(project, domain) + end + + shared_examples 'user do not see private key' do + it 'user do not see private key' do + expect(find_field('Key (PEM)', visible: :all, disabled: :all).value).to be_blank + end + end + + context 'when auto_ssl is enabled for domain' do + let(:domain) { create(:pages_domain, :letsencrypt, project: project, auto_ssl_enabled: true) } + + include_examples 'user do not see private key' + end + + context 'when auto_ssl is disabled for domain' do + let(:domain) { create(:pages_domain, :letsencrypt, project: project) } + + include_examples 'user do not see private key' + end + + context 'when certificate is provided by user' do + let(:domain) { create(:pages_domain, project: project) } + + it 'user sees private key' do + expect(find_field('Key (PEM)').value).not_to be_blank end end end - context 'when the page_auto_ssl feature flag is disabled' do + include_examples 'user sees private keys only for user provided certificate' + + context 'when letsencrypt is disabled' do let(:domain) do create(:pages_domain, auto_ssl_enabled: false, project: project) end before do - stub_feature_flags(pages_auto_ssl: false) + stub_application_setting(lets_encrypt_terms_of_service_accepted: false) visit edit_project_pages_domain_path(project, domain) end @@ -127,5 +122,7 @@ describe "Pages with Let's Encrypt", :https_pages_enabled do it "does not render the Let's Encrypt field", :js do expect(page).not_to have_selector '.js-auto-ssl-toggle-container' end + + include_examples 'user sees private keys only for user provided certificate' end end diff --git a/spec/features/projects/pages_spec.rb b/spec/features/projects/pages_spec.rb index 9bb0ba81ef5..c4b3ddb2088 100644 --- a/spec/features/projects/pages_spec.rb +++ b/spec/features/projects/pages_spec.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true require 'spec_helper' -shared_examples 'pages domain editing' do +shared_examples 'pages settings editing' do let(:project) { create(:project) } let(:user) { create(:user) } let(:role) { :maintainer } @@ -321,19 +321,15 @@ shared_examples 'pages domain editing' do end describe 'Pages' do - context 'when pages_auto_ssl feature flag is disabled' do - before do - stub_feature_flags(pages_auto_ssl: false) - end + include LetsEncryptHelpers - include_examples 'pages domain editing' - end + include_examples 'pages settings editing' - context 'when pages_auto_ssl feature flag is enabled' do + context 'when letsencrypt support is enabled' do before do - stub_feature_flags(pages_auto_ssl: true) + stub_lets_encrypt_settings end - include_examples 'pages domain editing' + include_examples 'pages settings editing' end end diff --git a/spec/models/pages_domain_spec.rb b/spec/models/pages_domain_spec.rb index fdc81359d34..4fb7b71a3c7 100644 --- a/spec/models/pages_domain_spec.rb +++ b/spec/models/pages_domain_spec.rb @@ -356,6 +356,102 @@ describe PagesDomain do end end + describe '#user_provided_key' do + subject { domain.user_provided_key } + + context 'when certificate is provided by user' do + let(:domain) { create(:pages_domain) } + + it 'returns key' do + is_expected.to eq(domain.key) + end + end + + context 'when certificate is provided by gitlab' do + let(:domain) { create(:pages_domain, :letsencrypt) } + + it 'returns nil' do + is_expected.to be_nil + end + end + end + + describe '#user_provided_certificate' do + subject { domain.user_provided_certificate } + + context 'when certificate is provided by user' do + let(:domain) { create(:pages_domain) } + + it 'returns key' do + is_expected.to eq(domain.certificate) + end + end + + context 'when certificate is provided by gitlab' do + let(:domain) { create(:pages_domain, :letsencrypt) } + + it 'returns nil' do + is_expected.to be_nil + end + end + end + + shared_examples 'certificate setter' do |attribute, setter_name, old_certificate_source, new_certificate_source| + let(:domain) do + create(:pages_domain, certificate_source: old_certificate_source) + end + + let(:old_value) { domain.public_send(attribute) } + + subject { domain.public_send(setter_name, new_value) } + + context 'when value has been changed' do + let(:new_value) { 'new_value' } + + it "assignes new value to #{attribute}" do + expect do + subject + end.to change { domain.public_send(attribute) }.from(old_value).to('new_value') + end + + it 'changes certificate source' do + expect do + subject + end.to change { domain.certificate_source }.from(old_certificate_source).to(new_certificate_source) + end + end + + context 'when value has not been not changed' do + let(:new_value) { old_value } + + it 'does not change certificate source' do + expect do + subject + end.not_to change { domain.certificate_source }.from(old_certificate_source) + end + end + end + + describe '#user_provided_key=' do + include_examples('certificate setter', 'key', 'user_provided_key=', + 'gitlab_provided', 'user_provided') + end + + describe '#gitlab_provided_key=' do + include_examples('certificate setter', 'key', 'gitlab_provided_key=', + 'user_provided', 'gitlab_provided') + end + + describe '#user_provided_certificate=' do + include_examples('certificate setter', 'certificate', 'user_provided_certificate=', + 'gitlab_provided', 'user_provided') + end + + describe '#gitlab_provided_certificate=' do + include_examples('certificate setter', 'certificate', 'gitlab_provided_certificate=', + 'user_provided', 'gitlab_provided') + end + describe '.for_removal' do subject { described_class.for_removal } diff --git a/spec/requests/api/pages_domains_spec.rb b/spec/requests/api/pages_domains_spec.rb index 3eb68a6abb6..449032b95b7 100644 --- a/spec/requests/api/pages_domains_spec.rb +++ b/spec/requests/api/pages_domains_spec.rb @@ -359,6 +359,14 @@ describe API::PagesDomains do expect(pages_domain_secure.certificate).to eq(params_secure_nokey[:certificate]) end + it 'updates certificate source to user_provided if is changed' do + pages_domain.update!(certificate_source: 'gitlab_provided') + + expect do + put api(route_domain, user), params: params_secure + end.to change { pages_domain.reload.certificate_source }.from('gitlab_provided').to('user_provided') + end + it 'fails to update pages domain adding certificate without key' do put api(route_domain, user), params: params_secure_nokey diff --git a/spec/services/pages_domains/obtain_lets_encrypt_certificate_service_spec.rb b/spec/services/pages_domains/obtain_lets_encrypt_certificate_service_spec.rb index 6d7be27939c..d5f77f3354b 100644 --- a/spec/services/pages_domains/obtain_lets_encrypt_certificate_service_spec.rb +++ b/spec/services/pages_domains/obtain_lets_encrypt_certificate_service_spec.rb @@ -137,6 +137,12 @@ describe PagesDomains::ObtainLetsEncryptCertificateService do expect(pages_domain.certificate).to eq(certificate) end + it 'marks certificate as gitlab_provided' do + service.execute + + expect(pages_domain.certificate_source).to eq("gitlab_provided") + end + it 'removes order from database' do service.execute |