Skip to content
This repository has been archived by the owner on Apr 17, 2023. It is now read-only.

Commit

Permalink
Restrict deletes into the repository
Browse files Browse the repository at this point in the history
Deletes of registry objects are referenced by their digest, but in some
situations, we accidentally deleted objects from other places. For
example, if we pushed the same image:tag into multiple namespaces, then
a delete on one of them removed also the others from other namespaces.

This commits restricts this into the given namespace/repository.

Fixes #1125

Signed-off-by: Miquel Sabaté Solà <[email protected]>
  • Loading branch information
mssola committed Sep 19, 2018
1 parent d1bc09b commit 8117995
Show file tree
Hide file tree
Showing 2 changed files with 25 additions and 5 deletions.
8 changes: 6 additions & 2 deletions app/models/tag.rb
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ def delete_by_digest!(actor)
dig = fetch_digest
return false if dig.blank?

Tag.where(digest: dig).update_all(marked: true)
Tag.where(repository: repository, digest: dig).update_all(marked: true)

begin
Registry.get.client.delete(repository.full_name, dig, "manifests")
Expand All @@ -73,7 +73,11 @@ def delete_by_digest!(actor)
return false
end

Tag.where(digest: dig).map { |t| t.delete_by!(actor) }
success = true
Tag.where(repository: repository, digest: dig).find_each do |tag|
success &&= tag&.delete_by!(actor)
end
success
end

# Delete this tag and update its activity.
Expand Down
22 changes: 19 additions & 3 deletions spec/models/tag_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -34,9 +34,10 @@ def fetch_digest_test
end

describe Tag do
let!(:registry) { create(:registry, hostname: "registry.test.lan") }
let!(:user) { create(:admin) }
let!(:repository) { create(:repository, namespace: registry.global_namespace, name: "repo") }
let!(:registry) { create(:registry, hostname: "registry.test.lan") }
let!(:user) { create(:admin) }
let!(:repository) { create(:repository, namespace: registry.global_namespace, name: "repo") }
let!(:repository1) { create(:repository, namespace: registry.global_namespace, name: "repo1") }

it { is_expected.to belong_to(:repository) }
it { is_expected.to belong_to(:author) }
Expand Down Expand Up @@ -71,6 +72,7 @@ def fetch_digest_test
describe "#delete_by_digest!" do
let!(:tag) { create(:tag, name: "tag1", repository: repository, digest: "1") }
let!(:tag2) { create(:tag, name: "tag2", repository: repository, digest: "2") }
let!(:tag3) { create(:tag, name: "tag3", repository: repository1, digest: "2") }

it "returns false if there is no digest" do
allow_any_instance_of(described_class).to receive(:fetch_digest).and_return(nil)
Expand Down Expand Up @@ -151,6 +153,20 @@ def fetch_digest_test

expect(described_class.find_by(name: "t")).to be_nil
end

it "does not remove tags from other repositories" do
allow_any_instance_of(Tag).to receive(:fetch_digest).and_return("2")
allow_any_instance_of(Portus::RegistryClient).to(
receive(:delete)
.with(repository.full_name, "2", "manifests")
.and_return(true)
)

tag2.delete_by_digest!(user)

expect(repository.tags.count).to eq 1
expect(repository1.tags.count).to eq 1
end
end

# NOTE: lots of cases are being left out on purpose because they are already
Expand Down

0 comments on commit 8117995

Please sign in to comment.