Commit 1c69353a authored by David Fernandez's avatar David Fernandez

Merge branch '349743-phase2-jwt' into 'master'

Restrict JWT requests when importing repositories

See merge request gitlab-org/gitlab!78489
parents 7a2593d9 c6c6d609
...@@ -146,6 +146,10 @@ class ContainerRepository < ApplicationRecord ...@@ -146,6 +146,10 @@ class ContainerRepository < ApplicationRecord
update!(expiration_policy_started_at: Time.zone.now) update!(expiration_policy_started_at: Time.zone.now)
end end
def migration_importing?
migration_state == 'importing'
end
def self.build_from_path(path) def self.build_from_path(path)
self.new(project: path.repository_project, self.new(project: path.repository_project,
name: path.repository_name) name: path.repository_name)
...@@ -169,6 +173,11 @@ class ContainerRepository < ApplicationRecord ...@@ -169,6 +173,11 @@ class ContainerRepository < ApplicationRecord
self.find_by!(project: path.repository_project, self.find_by!(project: path.repository_project,
name: path.repository_name) name: path.repository_name)
end end
def self.find_by_path(path)
self.find_by(project: path.repository_project,
name: path.repository_name)
end
end end
ContainerRepository.prepend_mod_with('ContainerRepository') ContainerRepository.prepend_mod_with('ContainerRepository')
...@@ -14,6 +14,10 @@ module Auth ...@@ -14,6 +14,10 @@ module Auth
:build_destroy_container_image :build_destroy_container_image
].freeze ].freeze
FORBIDDEN_IMPORTING_SCOPES = %w[push delete *].freeze
ActiveImportError = Class.new(StandardError)
def execute(authentication_abilities:) def execute(authentication_abilities:)
@authentication_abilities = authentication_abilities @authentication_abilities = authentication_abilities
...@@ -26,12 +30,22 @@ module Auth ...@@ -26,12 +30,22 @@ module Auth
end end
{ token: authorized_token(*scopes).encoded } { token: authorized_token(*scopes).encoded }
rescue ActiveImportError
error(
'DENIED',
status: 403,
message: 'Your repository is currently being migrated to a new platform and writes are temporarily disabled. Go to https://gitlab.com/groups/gitlab-org/-/epics/5523 to learn more.'
)
end end
def self.full_access_token(*names) def self.full_access_token(*names)
access_token(%w(*), names) access_token(%w(*), names)
end end
def self.import_access_token(*names)
access_token(%w(import), names)
end
def self.pull_access_token(*names) def self.pull_access_token(*names)
access_token(['pull'], names) access_token(['pull'], names)
end end
...@@ -104,6 +118,8 @@ module Auth ...@@ -104,6 +118,8 @@ module Auth
def process_repository_access(type, path, actions) def process_repository_access(type, path, actions)
return unless path.valid? return unless path.valid?
raise ActiveImportError if actively_importing?(actions, path)
requested_project = path.repository_project requested_project = path.repository_project
return unless requested_project return unless requested_project
...@@ -129,6 +145,15 @@ module Auth ...@@ -129,6 +145,15 @@ module Auth
}.compact }.compact
end end
def actively_importing?(actions, path)
return false if FORBIDDEN_IMPORTING_SCOPES.intersection(actions).empty?
container_repository = ContainerRepository.find_by_path(path)
return false unless container_repository
container_repository.migration_importing?
end
def self.migration_eligible(project: nil, repository_path: nil) def self.migration_eligible(project: nil, repository_path: nil)
return unless Feature.enabled?(:container_registry_migration_phase1) return unless Feature.enabled?(:container_registry_migration_phase1)
......
...@@ -330,6 +330,52 @@ RSpec.describe ContainerRepository do ...@@ -330,6 +330,52 @@ RSpec.describe ContainerRepository do
end end
end end
describe '.find_by_path' do
let_it_be(:container_repository) { create(:container_repository) }
let_it_be(:repository_path) { container_repository.project.full_path }
let(:path) { ContainerRegistry::Path.new(repository_path + '/' + container_repository.name) }
subject { described_class.find_by_path(path) }
context 'when repository exists' do
it 'finds the repository' do
expect(subject).to eq(container_repository)
end
end
context 'when repository does not exist' do
let(:path) { ContainerRegistry::Path.new(repository_path + '/some/image') }
it 'returns nil' do
expect(subject).to be_nil
end
end
end
describe '.find_by_path!' do
let_it_be(:container_repository) { create(:container_repository) }
let_it_be(:repository_path) { container_repository.project.full_path }
let(:path) { ContainerRegistry::Path.new(repository_path + '/' + container_repository.name) }
subject { described_class.find_by_path!(path) }
context 'when repository exists' do
it 'finds the repository' do
expect(subject).to eq(container_repository)
end
end
context 'when repository does not exist' do
let(:path) { ContainerRegistry::Path.new(repository_path + '/some/image') }
it 'raises an exception' do
expect { subject }.to raise_error(ActiveRecord::RecordNotFound)
end
end
end
describe '.build_root_repository' do describe '.build_root_repository' do
let(:repository) do let(:repository) do
described_class.build_root_repository(project) described_class.build_root_repository(project)
...@@ -458,6 +504,24 @@ RSpec.describe ContainerRepository do ...@@ -458,6 +504,24 @@ RSpec.describe ContainerRepository do
it { is_expected.to eq([repository]) } it { is_expected.to eq([repository]) }
end end
describe '#migration_importing?' do
let_it_be_with_reload(:container_repository) { create(:container_repository, migration_state: 'importing')}
subject { container_repository.migration_importing? }
it { is_expected.to eq(true) }
context 'when not importing' do
before do
# Technical debt: when the state machine is added, we should iterate through all possible states
# https://gitlab.com/gitlab-org/gitlab/-/merge_requests/78499
container_repository.update_column(:migration_state, 'default')
end
it { is_expected.to eq(false) }
end
end
context 'with repositories' do context 'with repositories' do
let_it_be_with_reload(:repository) { create(:container_repository, :cleanup_unscheduled) } let_it_be_with_reload(:repository) { create(:container_repository, :cleanup_unscheduled) }
let_it_be(:other_repository) { create(:container_repository, :cleanup_unscheduled) } let_it_be(:other_repository) { create(:container_repository, :cleanup_unscheduled) }
......
...@@ -167,7 +167,7 @@ RSpec.shared_examples 'a container registry auth service' do ...@@ -167,7 +167,7 @@ RSpec.shared_examples 'a container registry auth service' do
stub_feature_flags(container_registry_cdn_redirect: false) stub_feature_flags(container_registry_cdn_redirect: false)
end end
describe '#full_access_token' do describe '.full_access_token' do
let_it_be(:project) { create(:project) } let_it_be(:project) { create(:project) }
let(:token) { described_class.full_access_token(project.full_path) } let(:token) { described_class.full_access_token(project.full_path) }
...@@ -181,7 +181,21 @@ RSpec.shared_examples 'a container registry auth service' do ...@@ -181,7 +181,21 @@ RSpec.shared_examples 'a container registry auth service' do
it_behaves_like 'not a container repository factory' it_behaves_like 'not a container repository factory'
end end
describe '#pull_access_token' do describe '.import_access_token' do
let_it_be(:project) { create(:project) }
let(:token) { described_class.import_access_token(project.full_path) }
subject { { token: token } }
it_behaves_like 'an accessible' do
let(:actions) { ['import'] }
end
it_behaves_like 'not a container repository factory'
end
describe '.pull_access_token' do
let_it_be(:project) { create(:project) } let_it_be(:project) { create(:project) }
let(:token) { described_class.pull_access_token(project.full_path) } let(:token) { described_class.pull_access_token(project.full_path) }
...@@ -1126,4 +1140,72 @@ RSpec.shared_examples 'a container registry auth service' do ...@@ -1126,4 +1140,72 @@ RSpec.shared_examples 'a container registry auth service' do
end end
end end
end end
context 'when importing' do
let_it_be(:container_repository) { create(:container_repository, :root, migration_state: 'importing') }
let_it_be(:current_project) { container_repository.project }
let_it_be(:current_user) { create(:user) }
before do
current_project.add_developer(current_user)
end
shared_examples 'containing the import error' do
it 'includes a helpful error message' do
expect(subject[:errors].first).to include(message: /Your repository is currently being migrated/)
end
end
context 'push request' do
let(:current_params) do
{ scopes: ["repository:#{container_repository.path}:push"] }
end
it_behaves_like 'a forbidden' do
it_behaves_like 'containing the import error'
end
end
context 'delete request' do
let(:current_params) do
{ scopes: ["repository:#{container_repository.path}:delete"] }
end
it_behaves_like 'a forbidden' do
it_behaves_like 'containing the import error'
end
end
context '* request' do
let(:current_params) do
{ scopes: ["repository:#{container_repository.path}:*"] }
end
it_behaves_like 'a forbidden' do
it_behaves_like 'containing the import error'
end
end
context 'pull request' do
let(:current_params) do
{ scopes: ["repository:#{container_repository.path}:pull"] }
end
let(:project) { current_project }
it_behaves_like 'a pullable'
end
context 'mixed request' do
let(:current_params) do
{ scopes: ["repository:#{container_repository.path}:pull,push"] }
end
let(:project) { current_project }
it_behaves_like 'a forbidden' do
it_behaves_like 'containing the import error'
end
end
end
end end
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment