diff options
24 files changed, 447 insertions, 107 deletions
diff --git a/app/controllers/projects/pages_controller.rb b/app/controllers/projects/pages_controller.rb index d421b1a8eb5..cae6e2c40b8 100644 --- a/app/controllers/projects/pages_controller.rb +++ b/app/controllers/projects/pages_controller.rb @@ -21,4 +21,26 @@ class Projects::PagesController < Projects::ApplicationController end end end + + def update + result = Projects::UpdateService.new(@project, current_user, project_params).execute + + respond_to do |format| + format.html do + if result[:status] == :success + flash[:notice] = 'Your changes have been saved' + else + flash[:alert] = 'Something went wrong on our end' + end + + redirect_to project_pages_path(@project) + end + end + end + + private + + def project_params + params.require(:project).permit(:pages_https_only) + end end diff --git a/app/helpers/projects_helper.rb b/app/helpers/projects_helper.rb index da9fe734f1c..15f48e43a28 100644 --- a/app/helpers/projects_helper.rb +++ b/app/helpers/projects_helper.rb @@ -531,4 +531,22 @@ module ProjectsHelper def can_show_last_commit_in_list?(project) can?(current_user, :read_cross_project) && project.commit end + + def pages_https_only_disabled? + !@project.pages_domains.all?(&:https?) + end + + def pages_https_only_title + return unless pages_https_only_disabled? + + "You must enable HTTPS for all your domains first" + end + + def pages_https_only_label_class + if pages_https_only_disabled? + "list-label disabled" + else + "list-label" + end + end end diff --git a/app/models/pages_domain.rb b/app/models/pages_domain.rb index 588bd50ed77..2e478a24778 100644 --- a/app/models/pages_domain.rb +++ b/app/models/pages_domain.rb @@ -6,8 +6,10 @@ class PagesDomain < ActiveRecord::Base validates :domain, hostname: { allow_numeric_hostname: true } validates :domain, uniqueness: { case_sensitive: false } - validates :certificate, certificate: true, allow_nil: true, allow_blank: true - validates :key, certificate_key: true, allow_nil: true, allow_blank: true + validates :certificate, presence: { message: 'must be present if HTTPS-only is enabled' }, if: ->(domain) { domain.project&.pages_https_only? } + validates :certificate, certificate: true, if: ->(domain) { domain.certificate.present? } + validates :key, presence: { message: 'must be present if HTTPS-only is enabled' }, if: ->(domain) { domain.project&.pages_https_only? } + validates :key, certificate_key: true, if: ->(domain) { domain.key.present? } validates :verification_code, presence: true, allow_blank: false validate :validate_pages_domain @@ -46,6 +48,10 @@ class PagesDomain < ActiveRecord::Base !Gitlab::CurrentSettings.pages_domain_verification_enabled? || enabled_until.present? end + def https? + certificate.present? + end + def to_param domain end diff --git a/app/models/project.rb b/app/models/project.rb index 250680e2a2c..48a81ddb82e 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -267,6 +267,7 @@ class Project < ActiveRecord::Base validate :visibility_level_allowed_by_group validate :visibility_level_allowed_as_fork validate :check_wiki_path_conflict + validate :validate_pages_https_only, if: -> { changes.has_key?(:pages_https_only) } validates :repository_storage, presence: true, inclusion: { in: ->(_object) { Gitlab.config.repositories.storages.keys } } @@ -737,6 +738,26 @@ class Project < ActiveRecord::Base end end + def pages_https_only + return false unless Gitlab.config.pages.external_https + + super + end + + def pages_https_only? + return false unless Gitlab.config.pages.external_https + + super + end + + def validate_pages_https_only + return unless pages_https_only? + + unless pages_domains.all?(&:https?) + errors.add(:pages_https_only, "cannot be enabled unless all domains have TLS certificates") + end + end + def to_param if persisted? && errors.include?(:path) path_was diff --git a/app/services/projects/update_pages_configuration_service.rb b/app/services/projects/update_pages_configuration_service.rb index 52ff64cc938..25017c5cbe3 100644 --- a/app/services/projects/update_pages_configuration_service.rb +++ b/app/services/projects/update_pages_configuration_service.rb @@ -18,7 +18,8 @@ module Projects def pages_config { - domains: pages_domains_config + domains: pages_domains_config, + https_only: project.pages_https_only? } end @@ -27,7 +28,8 @@ module Projects { domain: domain.domain, certificate: domain.certificate, - key: domain.key + key: domain.key, + https_only: project.pages_https_only? && domain.https? } end end diff --git a/app/services/projects/update_service.rb b/app/services/projects/update_service.rb index 5f2615a2c01..679f4a9cb62 100644 --- a/app/services/projects/update_service.rb +++ b/app/services/projects/update_service.rb @@ -24,6 +24,8 @@ module Projects system_hook_service.execute_hooks_for(project, :update) end + update_pages_config if changing_pages_https_only? + success else model_errors = project.errors.full_messages.to_sentence @@ -67,5 +69,13 @@ module Projects log_error("Could not create wiki for #{project.full_name}") Gitlab::Metrics.counter(:wiki_can_not_be_created_total, 'Counts the times we failed to create a wiki') end + + def update_pages_config + Projects::UpdatePagesConfigurationService.new(project).execute + end + + def changing_pages_https_only? + project.previous_changes.include?(:pages_https_only) + end end end diff --git a/app/validators/certificate_validator.rb b/app/validators/certificate_validator.rb index 5239e70a326..b0c9a1b92a4 100644 --- a/app/validators/certificate_validator.rb +++ b/app/validators/certificate_validator.rb @@ -16,8 +16,6 @@ class CertificateValidator < ActiveModel::EachValidator private def valid_certificate_pem?(value) - return false unless value - OpenSSL::X509::Certificate.new(value).present? rescue OpenSSL::X509::CertificateError false diff --git a/app/views/projects/pages/_https_only.html.haml b/app/views/projects/pages/_https_only.html.haml new file mode 100644 index 00000000000..6a3ffce949f --- /dev/null +++ b/app/views/projects/pages/_https_only.html.haml @@ -0,0 +1,10 @@ += form_for @project, url: namespace_project_pages_path(@project.namespace.becomes(Namespace), @project), html: { class: 'inline', title: pages_https_only_title } do |f| + = f.check_box :pages_https_only, class: 'pull-left', disabled: pages_https_only_disabled? + + .prepend-left-20 + = f.label :pages_https_only, class: pages_https_only_label_class do + %strong Force domains with SSL certificates to use HTTPS + + - unless pages_https_only_disabled? + .prepend-top-10 + = f.submit 'Save', class: 'btn btn-success' diff --git a/app/views/projects/pages/show.html.haml b/app/views/projects/pages/show.html.haml index 04e647c0dc6..f17d9d24db6 100644 --- a/app/views/projects/pages/show.html.haml +++ b/app/views/projects/pages/show.html.haml @@ -13,6 +13,9 @@ Combined with the power of GitLab CI and the help of GitLab Runner you can deploy static pages for your individual projects, your user or your group. +- if Gitlab.config.pages.external_https + = render 'https_only' + %hr.clearfix = render 'access' diff --git a/changelogs/unreleased/pages_force_https.yml b/changelogs/unreleased/pages_force_https.yml new file mode 100644 index 00000000000..da7e29087f3 --- /dev/null +++ b/changelogs/unreleased/pages_force_https.yml @@ -0,0 +1,5 @@ +--- +title: Add HTTPS-only pages +merge_request: 16273 +author: rfwatson +type: added diff --git a/config/routes/project.rb b/config/routes/project.rb index c803737d40b..f50b9aded8d 100644 --- a/config/routes/project.rb +++ b/config/routes/project.rb @@ -52,7 +52,7 @@ constraints(::Constraints::ProjectUrlConstrainer.new) do end end - resource :pages, only: [:show, :destroy] do + resource :pages, only: [:show, :update, :destroy] do resources :domains, except: :index, controller: 'pages_domains', constraints: { id: %r{[^/]+} } do member do post :verify diff --git a/db/migrate/20180102220145_add_pages_https_only_to_projects.rb b/db/migrate/20180102220145_add_pages_https_only_to_projects.rb new file mode 100644 index 00000000000..ef6bc6896c0 --- /dev/null +++ b/db/migrate/20180102220145_add_pages_https_only_to_projects.rb @@ -0,0 +1,9 @@ +class AddPagesHttpsOnlyToProjects < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + + def change + add_column :projects, :pages_https_only, :boolean + end +end diff --git a/db/migrate/20180109183319_change_default_value_for_pages_https_only.rb b/db/migrate/20180109183319_change_default_value_for_pages_https_only.rb new file mode 100644 index 00000000000..c242e1b0d24 --- /dev/null +++ b/db/migrate/20180109183319_change_default_value_for_pages_https_only.rb @@ -0,0 +1,13 @@ +class ChangeDefaultValueForPagesHttpsOnly < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + + def up + change_column_default :projects, :pages_https_only, true + end + + def down + change_column_default :projects, :pages_https_only, nil + end +end diff --git a/db/schema.rb b/db/schema.rb index 56116a2d241..1be0570f85a 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -1513,6 +1513,7 @@ ActiveRecord::Schema.define(version: 20180320182229) do t.boolean "merge_requests_ff_only_enabled", default: false t.boolean "merge_requests_rebase_enabled", default: false, null: false t.integer "jobs_cache_index" + t.boolean "pages_https_only", default: true end add_index "projects", ["ci_id"], name: "index_projects_on_ci_id", using: :btree 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 |