Commit ff0bbef0 authored by Stan Hu's avatar Stan Hu

Gracefully recover from deleted LFS file

If an LFS object exists in the database but the underlying file has been
deleted, pushing that same object from another project may result in a
confusing "403 Access Forbidden" error.

Now, if the file no longer exists, we replace the file entry with the
newly-uploaded file so that we can gracefully recover from this case.

Closes https://gitlab.com/gitlab-org/gitlab/-/issues/25852
parent 1aac1232
...@@ -62,7 +62,10 @@ module Repositories ...@@ -62,7 +62,10 @@ module Repositories
# rubocop: disable CodeReuse/ActiveRecord # rubocop: disable CodeReuse/ActiveRecord
def store_file!(oid, size) def store_file!(oid, size)
object = LfsObject.find_by(oid: oid, size: size) object = LfsObject.find_by(oid: oid, size: size)
unless object&.file&.exists?
if object
replace_file!(object) unless object.file&.exists?
else
object = create_file!(oid, size) object = create_file!(oid, size)
end end
...@@ -79,6 +82,16 @@ module Repositories ...@@ -79,6 +82,16 @@ module Repositories
LfsObject.create!(oid: oid, size: size, file: uploaded_file) LfsObject.create!(oid: oid, size: size, file: uploaded_file)
end end
def replace_file!(lfs_object)
uploaded_file = params[:file]
raise UploadedFile::InvalidPathError unless uploaded_file.is_a?(UploadedFile)
Gitlab::AppJsonLogger.info(message: "LFS file replaced because it did not exist", oid: oid, size: size)
lfs_object.file = uploaded_file
lfs_object.save!
end
def link_to_project!(object) def link_to_project!(object)
return unless object return unless object
......
---
title: Gracefully recover from deleted LFS file
merge_request: 45459
author:
type: fixed
...@@ -127,6 +127,41 @@ RSpec.describe Repositories::LfsStorageController do ...@@ -127,6 +127,41 @@ RSpec.describe Repositories::LfsStorageController do
end end
end end
context 'when existing file has been deleted' do
let(:lfs_object) { create(:lfs_object, :with_file) }
before do
FileUtils.rm(lfs_object.file.path)
params[:oid] = lfs_object.oid
params[:size] = lfs_object.size
end
it 'replaces the file' do
expect(Gitlab::AppJsonLogger).to receive(:info).with(message: "LFS file replaced because it did not exist", oid: lfs_object.oid, size: lfs_object.size)
subject
expect(response).to have_gitlab_http_status(:ok)
expect(lfs_object.reload.file).to exist
end
context 'with invalid file' do
before do
allow_next_instance_of(ActionController::Parameters) do |params|
allow(params).to receive(:[]).and_call_original
allow(params).to receive(:[]).with(:file).and_return({})
end
end
it 'renders LFS forbidden' do
subject
expect(response).to have_gitlab_http_status(:forbidden)
expect(lfs_object.reload.file).not_to exist
end
end
end
context 'when file is not stored' do context 'when file is not stored' do
it 'renders unprocessable entity' do it 'renders unprocessable entity' do
expect(controller).to receive(:store_file!).and_return(nil) expect(controller).to receive(:store_file!).and_return(nil)
......
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