From d13669716ab0c31ce9039ae9f7f073e33a4dc40f Mon Sep 17 00:00:00 2001 From: Toon Claes Date: Tue, 19 Sep 2017 09:44:58 +0200 Subject: Create idea of read-only database In GitLab EE, a GitLab instance can be read-only (e.g. when it's a Geo secondary node). But in GitLab CE it also might be useful to have the "read-only" idea around. So port it back to GitLab CE. Also having the principle of read-only in GitLab CE would hopefully lead to less errors introduced, doing write operations when there aren't allowed for read-only calls. Closes gitlab-org/gitlab-ce#37534. --- spec/factories/projects.rb | 2 +- spec/lib/banzai/renderer_spec.rb | 9 +- spec/lib/gitlab/git_access_spec.rb | 23 ++++ spec/lib/gitlab/git_access_wiki_spec.rb | 29 +++-- spec/lib/gitlab/middleware/read_only_spec.rb | 142 +++++++++++++++++++++ .../app/git_user_default_ssh_config_check_spec.rb | 8 ++ spec/models/concerns/cache_markdown_field_spec.rb | 72 ++++++----- spec/models/concerns/routable_spec.rb | 10 ++ spec/models/project_spec.rb | 42 +++++- spec/requests/lfs_http_spec.rb | 28 ++++ .../hashed_storage_migration_service_spec.rb | 2 +- spec/services/users/activity_service_spec.rb | 12 ++ 12 files changed, 329 insertions(+), 50 deletions(-) create mode 100644 spec/lib/gitlab/middleware/read_only_spec.rb (limited to 'spec') diff --git a/spec/factories/projects.rb b/spec/factories/projects.rb index 958d62181a2..4034e7905ad 100644 --- a/spec/factories/projects.rb +++ b/spec/factories/projects.rb @@ -149,7 +149,7 @@ FactoryGirl.define do end end - trait :readonly do + trait :read_only do repository_read_only true end diff --git a/spec/lib/banzai/renderer_spec.rb b/spec/lib/banzai/renderer_spec.rb index da42272bbef..81a04a2d46d 100644 --- a/spec/lib/banzai/renderer_spec.rb +++ b/spec/lib/banzai/renderer_spec.rb @@ -31,7 +31,14 @@ describe Banzai::Renderer do let(:object) { fake_object(fresh: false) } it 'caches and returns the result' do - expect(object).to receive(:refresh_markdown_cache!).with(do_update: true) + expect(object).to receive(:refresh_markdown_cache!) + + is_expected.to eq('field_html') + end + + it "skips database caching on a GitLab read-only instance" do + allow(Gitlab::Database).to receive(:read_only?).and_return(true) + expect(object).to receive(:refresh_markdown_cache!) is_expected.to eq('field_html') end diff --git a/spec/lib/gitlab/git_access_spec.rb b/spec/lib/gitlab/git_access_spec.rb index 458627ee4de..c54327bd2e4 100644 --- a/spec/lib/gitlab/git_access_spec.rb +++ b/spec/lib/gitlab/git_access_spec.rb @@ -598,6 +598,19 @@ describe Gitlab::GitAccess do admin: { push_protected_branch: false, push_all: false, merge_into_protected_branch: false })) end end + + context "when in a read-only GitLab instance" do + before do + create(:protected_branch, name: 'feature', project: project) + allow(Gitlab::Database).to receive(:read_only?) { true } + end + + # Only check admin; if an admin can't do it, other roles can't either + matrix = permissions_matrix[:admin].dup + matrix.each { |key, _| matrix[key] = false } + + run_permission_checks(admin: matrix) + end end describe 'build authentication abilities' do @@ -632,6 +645,16 @@ describe Gitlab::GitAccess do end end + context 'when the repository is read only' do + let(:project) { create(:project, :repository, :read_only) } + + it 'denies push access' do + project.add_master(user) + + expect { push_access_check }.to raise_unauthorized('The repository is temporarily read-only. Please try again later.') + end + end + describe 'deploy key permissions' do let(:key) { create(:deploy_key, user: user, can_push: can_push) } let(:actor) { key } diff --git a/spec/lib/gitlab/git_access_wiki_spec.rb b/spec/lib/gitlab/git_access_wiki_spec.rb index 0376b4ee783..1056074264a 100644 --- a/spec/lib/gitlab/git_access_wiki_spec.rb +++ b/spec/lib/gitlab/git_access_wiki_spec.rb @@ -4,6 +4,7 @@ describe Gitlab::GitAccessWiki do let(:access) { described_class.new(user, project, 'web', authentication_abilities: authentication_abilities, redirected_path: redirected_path) } let(:project) { create(:project, :repository) } let(:user) { create(:user) } + let(:changes) { ['6f6d7e7ed 570e7b2ab refs/heads/master'] } let(:redirected_path) { nil } let(:authentication_abilities) do [ @@ -13,19 +14,27 @@ describe Gitlab::GitAccessWiki do ] end - describe 'push_allowed?' do - before do - create(:protected_branch, name: 'master', project: project) - project.team << [user, :developer] - end + describe '#push_access_check' do + context 'when user can :create_wiki' do + before do + create(:protected_branch, name: 'master', project: project) + project.team << [user, :developer] + end - subject { access.check('git-receive-pack', changes) } + subject { access.check('git-receive-pack', changes) } - it { expect { subject }.not_to raise_error } - end + it { expect { subject }.not_to raise_error } + + context 'when in a read-only GitLab instance' do + before do + allow(Gitlab::Database).to receive(:read_only?) { true } + end - def changes - ['6f6d7e7ed 570e7b2ab refs/heads/master'] + it 'does not give access to upload wiki code' do + expect { subject }.to raise_error(Gitlab::GitAccess::UnauthorizedError, "You can't push code to a read-only GitLab instance.") + end + end + end end describe '#access_check_download!' do diff --git a/spec/lib/gitlab/middleware/read_only_spec.rb b/spec/lib/gitlab/middleware/read_only_spec.rb new file mode 100644 index 00000000000..742a792a1af --- /dev/null +++ b/spec/lib/gitlab/middleware/read_only_spec.rb @@ -0,0 +1,142 @@ +require 'spec_helper' + +describe Gitlab::Middleware::ReadOnly do + include Rack::Test::Methods + + RSpec::Matchers.define :be_a_redirect do + match do |response| + response.status == 301 + end + end + + RSpec::Matchers.define :disallow_request do + match do |middleware| + flash = middleware.send(:rack_flash) + flash['alert'] && flash['alert'].include?('You cannot do writing operations') + end + end + + RSpec::Matchers.define :disallow_request_in_json do + match do |response| + json_response = JSON.parse(response.body) + response.body.include?('You cannot do writing operations') && json_response.key?('message') + end + end + + let(:rack_stack) do + rack = Rack::Builder.new do + use ActionDispatch::Session::CacheStore + use ActionDispatch::Flash + use ActionDispatch::ParamsParser + end + + rack.run(subject) + rack.to_app + end + + subject { described_class.new(fake_app) } + + let(:request) { Rack::MockRequest.new(rack_stack) } + + context 'normal requests to a read-only Gitlab instance' do + let(:fake_app) { lambda { |env| [200, { 'Content-Type' => 'text/plain' }, ['OK']] } } + + before do + allow(Gitlab::Database).to receive(:read_only?) { true } + end + + it 'expects PATCH requests to be disallowed' do + response = request.patch('/test_request') + + expect(response).to be_a_redirect + expect(subject).to disallow_request + end + + it 'expects PUT requests to be disallowed' do + response = request.put('/test_request') + + expect(response).to be_a_redirect + expect(subject).to disallow_request + end + + it 'expects POST requests to be disallowed' do + response = request.post('/test_request') + + expect(response).to be_a_redirect + expect(subject).to disallow_request + end + + it 'expects a internal POST request to be allowed after a disallowed request' do + response = request.post('/test_request') + + expect(response).to be_a_redirect + + response = request.post("/api/#{API::API.version}/internal") + + expect(response).not_to be_a_redirect + end + + it 'expects DELETE requests to be disallowed' do + response = request.delete('/test_request') + + expect(response).to be_a_redirect + expect(subject).to disallow_request + end + + context 'whitelisted requests' do + it 'expects DELETE request to logout to be allowed' do + response = request.delete('/users/sign_out') + + expect(response).not_to be_a_redirect + expect(subject).not_to disallow_request + end + + it 'expects a POST internal request to be allowed' do + response = request.post("/api/#{API::API.version}/internal") + + expect(response).not_to be_a_redirect + expect(subject).not_to disallow_request + end + + it 'expects a POST LFS request to batch URL to be allowed' do + response = request.post('/root/rouge.git/info/lfs/objects/batch') + + expect(response).not_to be_a_redirect + expect(subject).not_to disallow_request + end + end + end + + context 'json requests to a read-only GitLab instance' do + let(:fake_app) { lambda { |env| [200, { 'Content-Type' => 'application/json' }, ['OK']] } } + let(:content_json) { { 'CONTENT_TYPE' => 'application/json' } } + + before do + allow(Gitlab::Database).to receive(:read_only?) { true } + end + + it 'expects PATCH requests to be disallowed' do + response = request.patch('/test_request', content_json) + + expect(response).to disallow_request_in_json + end + + it 'expects PUT requests to be disallowed' do + response = request.put('/test_request', content_json) + + expect(response).to disallow_request_in_json + end + + it 'expects POST requests to be disallowed' do + response = request.post('/test_request', content_json) + + expect(response).to disallow_request_in_json + end + + it 'expects DELETE requests to be disallowed' do + response = request.delete('/test_request', content_json) + + expect(response).to disallow_request_in_json + end + end +end diff --git a/spec/lib/system_check/app/git_user_default_ssh_config_check_spec.rb b/spec/lib/system_check/app/git_user_default_ssh_config_check_spec.rb index a0fb86345f3..b4b83b70d1c 100644 --- a/spec/lib/system_check/app/git_user_default_ssh_config_check_spec.rb +++ b/spec/lib/system_check/app/git_user_default_ssh_config_check_spec.rb @@ -39,6 +39,14 @@ describe SystemCheck::App::GitUserDefaultSSHConfigCheck do it { is_expected.to eq(expected_result) } end + + it 'skips GitLab read-only instances' do + stub_user + stub_home_dir + allow(Gitlab::Database).to receive(:read_only?).and_return(true) + + is_expected.to be_truthy + end end describe '#check?' do diff --git a/spec/models/concerns/cache_markdown_field_spec.rb b/spec/models/concerns/cache_markdown_field_spec.rb index 40bbb10eaac..129dfa07f15 100644 --- a/spec/models/concerns/cache_markdown_field_spec.rb +++ b/spec/models/concerns/cache_markdown_field_spec.rb @@ -178,57 +178,59 @@ describe CacheMarkdownField do end end - describe '#refresh_markdown_cache!' do + describe '#refresh_markdown_cache' do before do thing.foo = updated_markdown end - context 'do_update: false' do - it 'fills all html fields' do - thing.refresh_markdown_cache! + it 'fills all html fields' do + thing.refresh_markdown_cache - expect(thing.foo_html).to eq(updated_html) - expect(thing.foo_html_changed?).to be_truthy - expect(thing.baz_html_changed?).to be_truthy - end + expect(thing.foo_html).to eq(updated_html) + expect(thing.foo_html_changed?).to be_truthy + expect(thing.baz_html_changed?).to be_truthy + end - it 'does not save the result' do - expect(thing).not_to receive(:update_columns) + it 'does not save the result' do + expect(thing).not_to receive(:update_columns) - thing.refresh_markdown_cache! - end + thing.refresh_markdown_cache + end - it 'updates the markdown cache version' do - thing.cached_markdown_version = nil - thing.refresh_markdown_cache! + it 'updates the markdown cache version' do + thing.cached_markdown_version = nil + thing.refresh_markdown_cache - expect(thing.cached_markdown_version).to eq(CacheMarkdownField::CACHE_VERSION) - end + expect(thing.cached_markdown_version).to eq(CacheMarkdownField::CACHE_VERSION) end + end - context 'do_update: true' do - it 'fills all html fields' do - thing.refresh_markdown_cache!(do_update: true) + describe '#refresh_markdown_cache!' do + before do + thing.foo = updated_markdown + end - expect(thing.foo_html).to eq(updated_html) - expect(thing.foo_html_changed?).to be_truthy - expect(thing.baz_html_changed?).to be_truthy - end + it 'fills all html fields' do + thing.refresh_markdown_cache! - it 'skips saving if not persisted' do - expect(thing).to receive(:persisted?).and_return(false) - expect(thing).not_to receive(:update_columns) + expect(thing.foo_html).to eq(updated_html) + expect(thing.foo_html_changed?).to be_truthy + expect(thing.baz_html_changed?).to be_truthy + end - thing.refresh_markdown_cache!(do_update: true) - end + it 'skips saving if not persisted' do + expect(thing).to receive(:persisted?).and_return(false) + expect(thing).not_to receive(:update_columns) - it 'saves the changes using #update_columns' do - expect(thing).to receive(:persisted?).and_return(true) - expect(thing).to receive(:update_columns) - .with("foo_html" => updated_html, "baz_html" => "", "cached_markdown_version" => CacheMarkdownField::CACHE_VERSION) + thing.refresh_markdown_cache! + end - thing.refresh_markdown_cache!(do_update: true) - end + it 'saves the changes using #update_columns' do + expect(thing).to receive(:persisted?).and_return(true) + expect(thing).to receive(:update_columns) + .with("foo_html" => updated_html, "baz_html" => "", "cached_markdown_version" => CacheMarkdownField::CACHE_VERSION) + + thing.refresh_markdown_cache! end end diff --git a/spec/models/concerns/routable_spec.rb b/spec/models/concerns/routable_spec.rb index b463d12e448..ab8773b7ede 100644 --- a/spec/models/concerns/routable_spec.rb +++ b/spec/models/concerns/routable_spec.rb @@ -12,6 +12,16 @@ describe Group, 'Routable' do it { is_expected.to have_many(:redirect_routes).dependent(:destroy) } end + describe 'GitLab read-only instance' do + it 'does not save route if route is not present' do + group.route.path = '' + allow(Gitlab::Database).to receive(:read_only?).and_return(true) + expect(group).to receive(:update_route).and_call_original + + expect { group.full_path }.to change { Route.count }.by(0) + end + end + describe 'Callbacks' do it 'creates route record on create' do expect(group.route.path).to eq(group.path) diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index 9b470e79a76..ec700215521 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -692,6 +692,44 @@ describe Project do project.cache_has_external_issue_tracker end.to change { project.has_external_issue_tracker}.to(false) end + + it 'does not cache data when in a read-only GitLab instance' do + allow(Gitlab::Database).to receive(:read_only?) { true } + + expect do + project.cache_has_external_issue_tracker + end.not_to change { project.has_external_issue_tracker } + end + end + + describe '#cache_has_external_wiki' do + let(:project) { create(:project, has_external_wiki: nil) } + + it 'stores true if there is any external_wikis' do + services = double(:service, external_wikis: [ExternalWikiService.new]) + expect(project).to receive(:services).and_return(services) + + expect do + project.cache_has_external_wiki + end.to change { project.has_external_wiki}.to(true) + end + + it 'stores false if there is no external_wikis' do + services = double(:service, external_wikis: []) + expect(project).to receive(:services).and_return(services) + + expect do + project.cache_has_external_wiki + end.to change { project.has_external_wiki}.to(false) + end + + it 'does not cache data when in a read-only GitLab instance' do + allow(Gitlab::Database).to receive(:read_only?) { true } + + expect do + project.cache_has_external_wiki + end.not_to change { project.has_external_wiki } + end end describe '#has_wiki?' do @@ -2446,7 +2484,7 @@ describe Project do expect(project.migrate_to_hashed_storage!).to be_truthy end - it 'flags as readonly' do + it 'flags as read-only' do expect { project.migrate_to_hashed_storage! }.to change { project.repository_read_only }.to(true) end @@ -2573,7 +2611,7 @@ describe Project do expect(project.migrate_to_hashed_storage!).to be_nil end - it 'does not flag as readonly' do + it 'does not flag as read-only' do expect { project.migrate_to_hashed_storage! }.not_to change { project.repository_read_only } end end diff --git a/spec/requests/lfs_http_spec.rb b/spec/requests/lfs_http_spec.rb index 27d09b8202e..eb4017e116b 100644 --- a/spec/requests/lfs_http_spec.rb +++ b/spec/requests/lfs_http_spec.rb @@ -824,6 +824,34 @@ describe 'Git LFS API and storage' do end end + describe 'when handling lfs batch request on a read-only GitLab instance' do + let(:authorization) { authorize_user } + let(:project) { create(:project) } + let(:path) { "#{project.http_url_to_repo}/info/lfs/objects/batch" } + let(:body) do + { 'objects' => [{ 'oid' => sample_oid, 'size' => sample_size }] } + end + + before do + allow(Gitlab::Database).to receive(:read_only?) { true } + project.team << [user, :master] + enable_lfs + end + + it 'responds with a 200 message on download' do + post_lfs_json path, body.merge('operation' => 'download'), headers + + expect(response).to have_gitlab_http_status(200) + end + + it 'responds with a 403 message on upload' do + post_lfs_json path, body.merge('operation' => 'upload'), headers + + expect(response).to have_gitlab_http_status(403) + expect(json_response).to include('message' => 'You cannot write to this read-only GitLab instance.') + end + end + describe 'when pushing a lfs object' do before do enable_lfs diff --git a/spec/services/projects/hashed_storage_migration_service_spec.rb b/spec/services/projects/hashed_storage_migration_service_spec.rb index 1b61207b550..aa1988d29d6 100644 --- a/spec/services/projects/hashed_storage_migration_service_spec.rb +++ b/spec/services/projects/hashed_storage_migration_service_spec.rb @@ -20,7 +20,7 @@ describe Projects::HashedStorageMigrationService do expect(gitlab_shell.exists?(project.repository_storage_path, "#{hashed_storage.disk_path}.wiki.git")).to be_truthy end - it 'updates project to be hashed and not readonly' do + it 'updates project to be hashed and not read-only' do service.execute expect(project.hashed_storage?).to be_truthy diff --git a/spec/services/users/activity_service_spec.rb b/spec/services/users/activity_service_spec.rb index fef4da0c76e..17eabad73be 100644 --- a/spec/services/users/activity_service_spec.rb +++ b/spec/services/users/activity_service_spec.rb @@ -38,6 +38,18 @@ describe Users::ActivityService do end end end + + context 'when in GitLab read-only instance' do + before do + allow(Gitlab::Database).to receive(:read_only?).and_return(true) + end + + it 'does not update last_activity_at' do + service.execute + + expect(last_hour_user_ids).to eq([]) + end + end end def last_hour_user_ids -- cgit v1.2.1