diff options
17 files changed, 358 insertions, 128 deletions
diff --git a/app/services/pages_domains/create_acme_order_service.rb b/app/services/pages_domains/create_acme_order_service.rb index c600f497fa5..8eab5c52432 100644 --- a/app/services/pages_domains/create_acme_order_service.rb +++ b/app/services/pages_domains/create_acme_order_service.rb @@ -3,6 +3,9 @@ module PagesDomains class CreateAcmeOrderService attr_reader :pages_domain + # TODO: remove this hack after https://gitlab.com/gitlab-org/gitlab/issues/30146 is implemented + # This makes GitLab automatically retry the certificate obtaining process every 2 hours if process wasn't finished + SHORT_EXPIRATION_DELAY = 2.hours def initialize(pages_domain) @pages_domain = pages_domain @@ -17,7 +20,7 @@ module PagesDomains private_key = OpenSSL::PKey::RSA.new(4096) saved_order = pages_domain.acme_orders.create!( url: order.url, - expires_at: order.expires, + expires_at: [order.expires, SHORT_EXPIRATION_DELAY.from_now].min, private_key: private_key.to_pem, challenge_token: challenge.token, diff --git a/changelogs/unreleased/acme-order-short-expiration.yml b/changelogs/unreleased/acme-order-short-expiration.yml new file mode 100644 index 00000000000..d2246cfa88a --- /dev/null +++ b/changelogs/unreleased/acme-order-short-expiration.yml @@ -0,0 +1,5 @@ +--- +title: Retry obtaining Let's Encrypt certificates every 2 hours if it wasn't successful +merge_request: 22336 +author: +type: fixed diff --git a/lib/gitlab/database_importers/instance_administrators/create_group.rb b/lib/gitlab/database_importers/instance_administrators/create_group.rb new file mode 100644 index 00000000000..5bf0e5a320d --- /dev/null +++ b/lib/gitlab/database_importers/instance_administrators/create_group.rb @@ -0,0 +1,131 @@ +# frozen_string_literal: true + +module Gitlab + module DatabaseImporters + module InstanceAdministrators + class CreateGroup < ::BaseService + include Stepable + + VISIBILITY_LEVEL = Gitlab::VisibilityLevel::INTERNAL + + steps :validate_application_settings, + :validate_admins, + :create_group, + :save_group_id, + :add_group_members, + :track_event + + def initialize + super(nil) + end + + def execute + execute_steps + end + + private + + def validate_application_settings(result) + return success(result) if application_settings + + log_error('No application_settings found') + error(_('No application_settings found')) + end + + def validate_admins(result) + unless instance_admins.any? + log_error('No active admin user found') + return error(_('No active admin user found')) + end + + success(result) + end + + def create_group(result) + if group_created? + log_info(_('Instance administrators group already exists')) + result[:group] = instance_administrators_group + return success(result) + end + + result[:group] = ::Groups::CreateService.new(instance_admins.first, create_group_params).execute + + if result[:group].persisted? + success(result) + else + log_error("Could not create instance administrators group. Errors: %{errors}" % { errors: result[:group].errors.full_messages }) + error(_('Could not create group')) + end + end + + def save_group_id(result) + return success(result) if group_created? + + response = application_settings.update( + instance_administrators_group_id: result[:group].id + ) + + if response + success(result) + else + log_error("Could not save instance administrators group ID, errors: %{errors}" % { errors: application_settings.errors.full_messages }) + error(_('Could not save group ID')) + end + end + + def add_group_members(result) + group = result[:group] + members = group.add_users(members_to_add(group), Gitlab::Access::MAINTAINER) + errors = members.flat_map { |member| member.errors.full_messages } + + if errors.any? + log_error('Could not add admins as members to self-monitoring project. Errors: %{errors}' % { errors: errors }) + error(_('Could not add admins as members')) + else + success(result) + end + end + + def track_event(result) + ::Gitlab::Tracking.event("instance_administrators_group", "group_created") + + success(result) + end + + def group_created? + instance_administrators_group.present? + end + + def application_settings + @application_settings ||= ApplicationSetting.current_without_cache + end + + def instance_administrators_group + application_settings.instance_administrators_group + end + + def instance_admins + @instance_admins ||= User.admins.active + end + + def members_to_add(group) + # Exclude admins who are already members of group because + # `group.add_users(users)` returns an error if the users parameter contains + # users who are already members of the group. + instance_admins - group.members.collect(&:user) + end + + def create_group_params + { + name: 'GitLab Instance Administrators', + visibility_level: VISIBILITY_LEVEL, + + # The 8 random characters at the end are so that the path does not + # clash with any existing group that the user might have created. + path: "gitlab-instance-administrators-#{SecureRandom.hex(4)}" + } + end + end + end + end +end diff --git a/lib/gitlab/database_importers/self_monitoring/project/create_service.rb b/lib/gitlab/database_importers/self_monitoring/project/create_service.rb index 92a2e504a11..d08afeef3b6 100644 --- a/lib/gitlab/database_importers/self_monitoring/project/create_service.rb +++ b/lib/gitlab/database_importers/self_monitoring/project/create_service.rb @@ -12,11 +12,9 @@ module Gitlab PROJECT_NAME = 'GitLab Instance Administration' steps :validate_application_settings, - :validate_admins, :create_group, :create_project, :save_project_id, - :add_group_members, :add_prometheus_manual_configuration, :track_event @@ -37,28 +35,14 @@ module Gitlab error(_('No application_settings found')) end - def validate_admins(result) - unless instance_admins.any? - log_error('No active admin user found') - return error(_('No active admin user found')) - end - - success(result) - end - def create_group(result) - if project_created? - log_info(_('Instance administrators group already exists')) - result[:group] = self_monitoring_project.owner - return success(result) - end - - result[:group] = ::Groups::CreateService.new(group_owner, create_group_params).execute + create_group_response = + Gitlab::DatabaseImporters::InstanceAdministrators::CreateGroup.new.execute - if result[:group].persisted? - success(result) + if create_group_response[:status] == :success + success(result.merge(create_group_response)) else - error(_('Could not create group')) + error(create_group_response[:message]) end end @@ -69,7 +53,9 @@ module Gitlab return success(result) end - result[:project] = ::Projects::CreateService.new(group_owner, create_project_params(result[:group])).execute + owner = result[:group].owners.first + + result[:project] = ::Projects::CreateService.new(owner, create_project_params(result[:group])).execute if result[:project].persisted? success(result) @@ -94,19 +80,6 @@ module Gitlab end end - def add_group_members(result) - group = result[:group] - members = group.add_users(members_to_add(group), Gitlab::Access::MAINTAINER) - errors = members.flat_map { |member| member.errors.full_messages } - - if errors.any? - log_error('Could not add admins as members to self-monitoring project. Errors: %{errors}' % { errors: errors }) - error(_('Could not add admins as members')) - else - success(result) - end - end - def add_prometheus_manual_configuration(result) return success(result) unless prometheus_enabled? return success(result) unless prometheus_listen_address.present? @@ -140,29 +113,6 @@ module Gitlab ::Gitlab::Prometheus::Internal.listen_address end - def instance_admins - @instance_admins ||= User.admins.active - end - - def group_owner - instance_admins.first - end - - def members_to_add(group) - # Exclude admins who are already members of group because - # `group.add_users(users)` returns an error if the users parameter contains - # users who are already members of the group. - instance_admins - group.members.collect(&:user) - end - - def create_group_params - { - name: 'GitLab Instance Administrators', - path: "gitlab-instance-administrators-#{SecureRandom.hex(4)}", - visibility_level: VISIBILITY_LEVEL - } - end - def docs_path Rails.application.routes.url_helpers.help_page_path( 'administration/monitoring/gitlab_instance_administration_project/index' diff --git a/lib/sentry/client.rb b/lib/sentry/client.rb index 20c919a97ff..490f82c4678 100644 --- a/lib/sentry/client.rb +++ b/lib/sentry/client.rb @@ -33,6 +33,7 @@ module Sentry def request_params { headers: { + 'Content-Type' => 'application/json', 'Authorization' => "Bearer #{@token}" }, follow_redirects: false @@ -47,7 +48,7 @@ module Sentry def http_put(url, params = {}) http_request do - Gitlab::HTTP.put(url, **request_params.merge(body: params)) + Gitlab::HTTP.put(url, **request_params.merge(body: params.to_json)) end end diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 094562a0a29..6bd93a1a5bf 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -5202,6 +5202,9 @@ msgstr "" msgid "Could not revoke personal access token %{personal_access_token_name}." msgstr "" +msgid "Could not save group ID" +msgstr "" + msgid "Could not save project ID" msgstr "" diff --git a/qa/qa/page/project/settings/protected_branches.rb b/qa/qa/page/project/settings/protected_branches.rb index ce17ca986a6..f718311fbf2 100644 --- a/qa/qa/page/project/settings/protected_branches.rb +++ b/qa/qa/page/project/settings/protected_branches.rb @@ -58,10 +58,9 @@ module QA within_element(:"allowed_to_#{action}_dropdown") do click_on allowed[:roles] + allowed[:users].each { |user| click_on user.username } if allowed.key?(:users) + allowed[:groups].each { |group| click_on group.name } if allowed.key?(:groups) end - - # Click the select element again to close the dropdown - click_element :"allowed_to_#{action}_select" end end end diff --git a/qa/qa/specs/features/browser_ui/2_plan/issue/check_mentions_for_xss_spec.rb b/qa/qa/specs/features/browser_ui/2_plan/issue/check_mentions_for_xss_spec.rb index 98178880be3..aa88937504e 100644 --- a/qa/qa/specs/features/browser_ui/2_plan/issue/check_mentions_for_xss_spec.rb +++ b/qa/qa/specs/features/browser_ui/2_plan/issue/check_mentions_for_xss_spec.rb @@ -28,7 +28,6 @@ module QA Flow::Project.add_member(project: project, username: user.username) Resource::Issue.fabricate_via_api! do |issue| - issue.title = 'issue title' issue.project = project end.visit! diff --git a/qa/qa/specs/features/browser_ui/2_plan/issue/collapse_comments_in_discussions_spec.rb b/qa/qa/specs/features/browser_ui/2_plan/issue/collapse_comments_in_discussions_spec.rb index 314a951ba89..e505c0991a6 100644 --- a/qa/qa/specs/features/browser_ui/2_plan/issue/collapse_comments_in_discussions_spec.rb +++ b/qa/qa/specs/features/browser_ui/2_plan/issue/collapse_comments_in_discussions_spec.rb @@ -8,9 +8,7 @@ module QA before do Flow::Login.sign_in - Resource::Issue.fabricate_via_api! do |issue| - issue.title = 'issue title' - end.visit! + Resource::Issue.fabricate_via_api!.visit! Page::Project::Issue::Show.perform do |show| show.select_all_activities_filter diff --git a/qa/qa/specs/features/browser_ui/2_plan/issue/comment_issue_spec.rb b/qa/qa/specs/features/browser_ui/2_plan/issue/comment_issue_spec.rb index 9234227eff7..6c37e3ecbb9 100644 --- a/qa/qa/specs/features/browser_ui/2_plan/issue/comment_issue_spec.rb +++ b/qa/qa/specs/features/browser_ui/2_plan/issue/comment_issue_spec.rb @@ -6,9 +6,7 @@ module QA before do Flow::Login.sign_in - Resource::Issue.fabricate_via_api! do |issue| - issue.title = 'issue title' - end.visit! + Resource::Issue.fabricate_via_api!.visit! end it 'user comments on an issue and edits the comment' do diff --git a/qa/qa/specs/features/browser_ui/2_plan/issue/create_issue_spec.rb b/qa/qa/specs/features/browser_ui/2_plan/issue/create_issue_spec.rb index 32605bc8970..3b231b9930e 100644 --- a/qa/qa/specs/features/browser_ui/2_plan/issue/create_issue_spec.rb +++ b/qa/qa/specs/features/browser_ui/2_plan/issue/create_issue_spec.rb @@ -3,16 +3,12 @@ module QA context 'Plan', :smoke do describe 'Issue creation' do - let(:issue_title) { 'issue title' } - before do Flow::Login.sign_in end it 'user creates an issue' do - issue = Resource::Issue.fabricate_via_browser_ui! do |issue| - issue.title = issue_title - end + issue = Resource::Issue.fabricate_via_browser_ui! Page::Project::Menu.perform(&:click_issues) @@ -28,9 +24,7 @@ module QA end before do - Resource::Issue.fabricate_via_api! do |issue| - issue.title = issue_title - end.visit! + Resource::Issue.fabricate_via_api!.visit! end it 'user comments on an issue with an attachment' do diff --git a/qa/qa/specs/features/browser_ui/2_plan/issue/filter_issue_comments_spec.rb b/qa/qa/specs/features/browser_ui/2_plan/issue/filter_issue_comments_spec.rb index a0652971693..4156ba54785 100644 --- a/qa/qa/specs/features/browser_ui/2_plan/issue/filter_issue_comments_spec.rb +++ b/qa/qa/specs/features/browser_ui/2_plan/issue/filter_issue_comments_spec.rb @@ -3,14 +3,10 @@ module QA context 'Plan' do describe 'filter issue comments activities' do - let(:issue_title) { 'issue title' } - before do Flow::Login.sign_in - Resource::Issue.fabricate_via_api! do |issue| - issue.title = issue_title - end.visit! + Resource::Issue.fabricate_via_api!.visit! end it 'user filters comments and activities in an issue' do diff --git a/qa/qa/specs/features/browser_ui/2_plan/issue/mentions_spec.rb b/qa/qa/specs/features/browser_ui/2_plan/issue/mentions_spec.rb index b323ee26e47..a0647df4097 100644 --- a/qa/qa/specs/features/browser_ui/2_plan/issue/mentions_spec.rb +++ b/qa/qa/specs/features/browser_ui/2_plan/issue/mentions_spec.rb @@ -16,7 +16,6 @@ module QA project.add_member(@user) Resource::Issue.fabricate_via_api! do |issue| - issue.title = 'issue to test mention' issue.project = project end.visit! end diff --git a/spec/lib/gitlab/database_importers/instance_administrators/create_group_spec.rb b/spec/lib/gitlab/database_importers/instance_administrators/create_group_spec.rb new file mode 100644 index 00000000000..97f4a7eec75 --- /dev/null +++ b/spec/lib/gitlab/database_importers/instance_administrators/create_group_spec.rb @@ -0,0 +1,164 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Gitlab::DatabaseImporters::InstanceAdministrators::CreateGroup do + describe '#execute' do + let(:result) { subject.execute } + + context 'without application_settings' do + it 'returns error' do + expect(subject).to receive(:log_error).and_call_original + expect(result).to eq( + status: :error, + message: 'No application_settings found', + last_step: :validate_application_settings + ) + + expect(Group.count).to eq(0) + end + end + + context 'without admin users' do + let(:application_setting) { Gitlab::CurrentSettings.current_application_settings } + + before do + allow(ApplicationSetting).to receive(:current_without_cache) { application_setting } + end + + it 'returns error' do + expect(subject).to receive(:log_error).and_call_original + expect(result).to eq( + status: :error, + message: 'No active admin user found', + last_step: :validate_admins + ) + + expect(Group.count).to eq(0) + end + end + + context 'with application settings and admin users' do + let(:group) { result[:group] } + let(:application_setting) { Gitlab::CurrentSettings.current_application_settings } + + let!(:user) { create(:user, :admin) } + + before do + allow(ApplicationSetting).to receive(:current_without_cache) { application_setting } + end + + it 'returns correct keys' do + expect(result.keys).to contain_exactly( + :status, :group + ) + end + + it "tracks successful install" do + expect(::Gitlab::Tracking).to receive(:event).with( + 'instance_administrators_group', 'group_created' + ) + + result + end + + it 'creates group' do + expect(result[:status]).to eq(:success) + expect(group).to be_persisted + expect(group.name).to eq('GitLab Instance Administrators') + expect(group.path).to start_with('gitlab-instance-administrators') + expect(group.path.split('-').last.length).to eq(8) + expect(group.visibility_level).to eq(described_class::VISIBILITY_LEVEL) + end + + it 'adds all admins as maintainers' do + admin1 = create(:user, :admin) + admin2 = create(:user, :admin) + create(:user) + + expect(result[:status]).to eq(:success) + expect(group.members.collect(&:user)).to contain_exactly(user, admin1, admin2) + expect(group.members.collect(&:access_level)).to contain_exactly( + Gitlab::Access::OWNER, + Gitlab::Access::MAINTAINER, + Gitlab::Access::MAINTAINER + ) + end + + it 'saves the group id' do + expect(result[:status]).to eq(:success) + expect(application_setting.instance_administrators_group_id).to eq(group.id) + end + + it 'returns error when saving group ID fails' do + allow(application_setting).to receive(:save) { false } + + expect(result).to eq( + status: :error, + message: 'Could not save group ID', + last_step: :save_group_id + ) + end + + context 'when group already exists' do + let(:existing_group) { create(:group) } + + before do + admin1 = create(:user, :admin) + admin2 = create(:user, :admin) + + existing_group.add_owner(user) + existing_group.add_users([admin1, admin2], Gitlab::Access::MAINTAINER) + + application_setting.instance_administrators_group_id = existing_group.id + end + + it 'returns success' do + expect(result).to eq( + status: :success, + group: existing_group + ) + + expect(Group.count).to eq(1) + end + end + + context 'when group cannot be created' do + let(:group) { build(:group) } + + before do + group.errors.add(:base, "Test error") + + expect_next_instance_of(::Groups::CreateService) do |group_create_service| + expect(group_create_service).to receive(:execute) + .and_return(group) + end + end + + it 'returns error' do + expect(subject).to receive(:log_error).and_call_original + expect(result).to eq( + status: :error, + message: 'Could not create group', + last_step: :create_group + ) + end + end + + context 'when user cannot be added to group' do + before do + subject.instance_variable_set(:@instance_admins, [user, build(:user, :admin)]) + end + + it 'returns error' do + expect(subject).to receive(:log_error).and_call_original + expect(result).to eq( + status: :error, + message: 'Could not add admins as members', + last_step: :add_group_members + ) + end + end + end + end +end diff --git a/spec/lib/gitlab/database_importers/self_monitoring/project/create_service_spec.rb b/spec/lib/gitlab/database_importers/self_monitoring/project/create_service_spec.rb index 082485f5ddd..10efdd44f20 100644 --- a/spec/lib/gitlab/database_importers/self_monitoring/project/create_service_spec.rb +++ b/spec/lib/gitlab/database_importers/self_monitoring/project/create_service_spec.rb @@ -39,11 +39,10 @@ describe Gitlab::DatabaseImporters::SelfMonitoring::Project::CreateService do end it 'returns error' do - expect(subject).to receive(:log_error).and_call_original expect(result).to eq( status: :error, message: 'No active admin user found', - last_step: :validate_admins + last_step: :create_group ) expect(Project.count).to eq(0) @@ -78,8 +77,8 @@ describe Gitlab::DatabaseImporters::SelfMonitoring::Project::CreateService do it_behaves_like 'has prometheus service', 'http://localhost:9090' it "tracks successful install" do - expect(::Gitlab::Tracking).to receive(:event) - expect(::Gitlab::Tracking).to receive(:event).with("self_monitoring", "project_created") + expect(::Gitlab::Tracking).to receive(:event).twice + expect(::Gitlab::Tracking).to receive(:event).with('self_monitoring', 'project_created') result end @@ -87,10 +86,6 @@ describe Gitlab::DatabaseImporters::SelfMonitoring::Project::CreateService do it 'creates group' do expect(result[:status]).to eq(:success) expect(group).to be_persisted - expect(group.name).to eq('GitLab Instance Administrators') - expect(group.path).to start_with('gitlab-instance-administrators') - expect(group.path.split('-').last.length).to eq(8) - expect(group.visibility_level).to eq(described_class::VISIBILITY_LEVEL) end it 'creates project with internal visibility' do @@ -120,19 +115,9 @@ describe Gitlab::DatabaseImporters::SelfMonitoring::Project::CreateService do expect(File).to exist("doc/#{path}.md") end - it 'adds all admins as maintainers' do - admin1 = create(:user, :admin) - admin2 = create(:user, :admin) - create(:user) - + it 'creates project with group as owner' do expect(result[:status]).to eq(:success) expect(project.owner).to eq(group) - expect(group.members.collect(&:user)).to contain_exactly(user, admin1, admin2) - expect(group.members.collect(&:access_level)).to contain_exactly( - Gitlab::Access::OWNER, - Gitlab::Access::MAINTAINER, - Gitlab::Access::MAINTAINER - ) end it 'saves the project id' do @@ -141,7 +126,10 @@ describe Gitlab::DatabaseImporters::SelfMonitoring::Project::CreateService do end it 'returns error when saving project ID fails' do - allow(application_setting).to receive(:save) { false } + allow(application_setting).to receive(:update).and_call_original + allow(application_setting).to receive(:update) + .with(instance_administration_project_id: anything) + .and_return(false) expect(result).to eq( status: :error, @@ -155,12 +143,7 @@ describe Gitlab::DatabaseImporters::SelfMonitoring::Project::CreateService do let(:existing_project) { create(:project, namespace: existing_group) } before do - admin1 = create(:user, :admin) - admin2 = create(:user, :admin) - - existing_group.add_owner(user) - existing_group.add_users([admin1, admin2], Gitlab::Access::MAINTAINER) - + application_setting.instance_administrators_group_id = existing_group.id application_setting.instance_administration_project_id = existing_project.id end @@ -272,21 +255,6 @@ describe Gitlab::DatabaseImporters::SelfMonitoring::Project::CreateService do end end - context 'when user cannot be added to project' do - before do - subject.instance_variable_set(:@instance_admins, [user, build(:user, :admin)]) - end - - it 'returns error' do - expect(subject).to receive(:log_error).and_call_original - expect(result).to eq( - status: :error, - message: 'Could not add admins as members', - last_step: :add_group_members - ) - end - end - context 'when prometheus manual configuration cannot be saved' do let(:prometheus_settings) do { diff --git a/spec/lib/sentry/client/issue_spec.rb b/spec/lib/sentry/client/issue_spec.rb index 3130bb38cc2..061ebcfdc06 100644 --- a/spec/lib/sentry/client/issue_spec.rb +++ b/spec/lib/sentry/client/issue_spec.rb @@ -30,7 +30,7 @@ describe Sentry::Client::Issue do let(:default_httparty_options) do { follow_redirects: false, - headers: { "Authorization" => "Bearer test-token" } + headers: { 'Content-Type' => 'application/json', 'Authorization' => "Bearer test-token" } } end diff --git a/spec/services/pages_domains/create_acme_order_service_spec.rb b/spec/services/pages_domains/create_acme_order_service_spec.rb index d59aa9b979e..154b3fd5600 100644 --- a/spec/services/pages_domains/create_acme_order_service_spec.rb +++ b/spec/services/pages_domains/create_acme_order_service_spec.rb @@ -45,12 +45,34 @@ describe PagesDomains::CreateAcmeOrderService do expect { OpenSSL::PKey::RSA.new(saved_order.private_key) }.not_to raise_error end - it 'properly saves order attributes' do + it 'properly saves order url' do service.execute saved_order = PagesDomainAcmeOrder.last expect(saved_order.url).to eq(order_double.url) - expect(saved_order.expires_at).to be_like_time(order_double.expires) + end + + context 'when order expires in 2 days' do + it 'sets expiration time in 2 hours' do + Timecop.freeze do + service.execute + + saved_order = PagesDomainAcmeOrder.last + expect(saved_order.expires_at).to be_like_time(2.hours.from_now) + end + end + end + + context 'when order expires in an hour' do + it 'sets expiration time accordingly to order' do + Timecop.freeze do + allow(order_double).to receive(:expires).and_return(1.hour.from_now) + service.execute + + saved_order = PagesDomainAcmeOrder.last + expect(saved_order.expires_at).to be_like_time(1.hour.from_now) + end + end end it 'properly saves challenge attributes' do |