Commit 9d45951f authored by Rob Watson's avatar Rob Watson

Add HTTPS-only pages

Closes #28857
parent 53d352aa
......@@ -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
......@@ -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
......@@ -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
......
......@@ -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
......
......@@ -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
......
......@@ -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
......@@ -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
......
= 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'
......@@ -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'
......
---
title: Add HTTPS-only pages
merge_request: 16273
author: rfwatson
type: added
......@@ -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
......
class AddPagesHttpsOnlyToProjects < ActiveRecord::Migration
include Gitlab::Database::MigrationHelpers
DOWNTIME = false
def change
add_column :projects, :pages_https_only, :boolean
end
end
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
......@@ -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
......
......@@ -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
......@@ -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
......
......@@ -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
......
......@@ -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 }
......
......@@ -458,6 +458,7 @@ Project:
- merge_requests_ff_only_enabled
- merge_requests_rebase_enabled
- jobs_cache_index
- pages_https_only
Author:
- name
ProjectFeature:
......
......@@ -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