fix: deleting the whole secret when it is empty (#5927)

providers/v1/onepassword/onepassword.go

@@ -222,6 +222,10 @@ func (provider *ProviderOnePassword) DeleteSecret(_ context.Context, ref esv1.Pu
 		return fmt.Errorf(errUpdateItem, err)
 	}
 
+	if len(providerItem.Sections) == 1 && providerItem.Sections[0].ID == "" && providerItem.Sections[0].Label == "" {
+		providerItem.Sections = nil
+	}
+
 	if len(providerItem.Fields) == 0 && len(providerItem.Files) == 0 && len(providerItem.Sections) == 0 {
 		// Delete the item if there are no fields, files or sections
 		if err = provider.client.DeleteItem(providerItem, providerItem.Vault.ID); err != nil {

providers/v1/onepassword/onepassword_test.go

@@ -2467,6 +2467,95 @@ func (m *mockClient) LoadStructFromItem(config interface{}, itemQuery, vaultQuer
 }
 func (m *mockClient) LoadStruct(config interface{}) error { return nil }
 
+func TestDeleteSecretWithEmptySections(t *testing.T) {
+	const vaultName = "vault1"
+	vault := onepassword.Vault{
+		ID:   vaultName,
+		Name: vaultName,
+	}
+
+	t.Run("item with empty section should be deleted when last field is removed", func(t *testing.T) {
+		deleteCalled := false
+		updateCalled := false
+
+		mockClient := fake.NewMockClient()
+		mockClient.MockVaults = map[string][]onepassword.Vault{
+			vaultName: {vault},
+		}
+		mockClient.MockItems = map[string][]onepassword.Item{
+			vaultName: {
+				{
+					ID:    "item-id",
+					Title: "test-item",
+					Vault: onepassword.ItemVault{ID: vaultName},
+					Sections: []*onepassword.ItemSection{
+						{ID: "", Label: ""},
+					},
+				},
+			},
+		}
+		mockClient.MockItemFields = map[string]map[string][]*onepassword.ItemField{
+			vaultName: {
+				"item-id": {
+					{ID: "field-1", Label: "password", Value: "secret"},
+				},
+			},
+		}
+		mockClient.DeleteItemValidateFunc = func(item *onepassword.Item, s string) error {
+			deleteCalled = true
+			return nil
+		}
+		mockClient.UpdateItemValidateFunc = func(item *onepassword.Item, s string) (*onepassword.Item, error) {
+			updateCalled = true
+			return item, nil
+		}
+
+		provider := &ProviderOnePassword{
+			vaults: map[string]int{vaultName: 1},
+			client: mockClient,
+		}
+
+		err := provider.DeleteSecret(context.Background(), fakeRef{
+			key:  "test-item",
+			prop: "password",
+		})
+
+		if err != nil {
+			t.Errorf("expected no error, got %v", err)
+		}
+		if !deleteCalled {
+			t.Error("expected DeleteItem to be called when item has no fields and only empty sections")
+		}
+		if updateCalled {
+			t.Error("expected UpdateItem not to be called")
+		}
+	})
+
+	t.Run("item not found should not error", func(t *testing.T) {
+		mockClient := fake.NewMockClient()
+		mockClient.MockVaults = map[string][]onepassword.Vault{
+			vaultName: {vault},
+		}
+		mockClient.MockItems = map[string][]onepassword.Item{
+			vaultName: {},
+		}
+
+		provider := &ProviderOnePassword{
+			vaults: map[string]int{vaultName: 1},
+			client: mockClient,
+		}
+
+		err := provider.DeleteSecret(context.Background(), fakeRef{
+			key:  "non-existent-item",
+			prop: "password",
+		})
+
+		if err != nil {
+			t.Errorf("expected no error when item not found, got %v", err)
+		}
+	})
+}
+
 func TestRetryClient(t *testing.T) {
 	tests := []struct {
 		name        string

providers/v1/onepasswordsdk/client.go

@@ -78,6 +78,9 @@ func (p *Provider) Close(_ context.Context) error {
 // DeleteSecret implements Secret Deletion on the provider when PushSecret.spec.DeletionPolicy=Delete.
 func (p *Provider) DeleteSecret(ctx context.Context, ref esv1.PushSecretRemoteRef) error {
 	providerItem, err := p.findItem(ctx, ref.GetRemoteKey())
+	if errors.Is(err, ErrKeyNotFound) {
+		return nil
+	}
 	if err != nil {
 		return err
 	}

providers/v1/onepasswordsdk/client_test.go

@@ -19,6 +19,7 @@ package onepasswordsdk
 import (
 	"context"
 	"errors"
+	"fmt"
 	"testing"
 	"time"
 
@@ -652,6 +653,69 @@ func (f *fakeFileLister) ReplaceDocument(ctx context.Context, item onepassword.I
 
 var _ onepassword.ItemsFilesAPI = (*fakeFileLister)(nil)
 
+type statefulFakeLister struct {
+	listAllResult []onepassword.ItemOverview
+	items         map[string]onepassword.Item
+	deletedItems  map[string]bool
+	createCalled  bool
+	putCalled     bool
+	deleteCalled  bool
+	fileLister    onepassword.ItemsFilesAPI
+}
+
+func (f *statefulFakeLister) Create(ctx context.Context, params onepassword.ItemCreateParams) (onepassword.Item, error) {
+	f.createCalled = true
+	return onepassword.Item{}, nil
+}
+
+func (f *statefulFakeLister) Get(ctx context.Context, vaultID, itemID string) (onepassword.Item, error) {
+	if f.deletedItems != nil && f.deletedItems[itemID] {
+		return onepassword.Item{}, fmt.Errorf("item not found")
+	}
+	if item, ok := f.items[itemID]; ok {
+		return item, nil
+	}
+	return onepassword.Item{}, fmt.Errorf("item not found")
+}
+
+func (f *statefulFakeLister) Put(ctx context.Context, item onepassword.Item) (onepassword.Item, error) {
+	f.putCalled = true
+	if f.items == nil {
+		f.items = make(map[string]onepassword.Item)
+	}
+	f.items[item.ID] = item
+	return item, nil
+}
+
+func (f *statefulFakeLister) Delete(ctx context.Context, vaultID, itemID string) error {
+	f.deleteCalled = true
+	if f.deletedItems == nil {
+		f.deletedItems = make(map[string]bool)
+	}
+	f.deletedItems[itemID] = true
+	delete(f.items, itemID)
+	f.listAllResult = nil
+	return nil
+}
+
+func (f *statefulFakeLister) Archive(ctx context.Context, vaultID, itemID string) error {
+	return nil
+}
+
+func (f *statefulFakeLister) List(ctx context.Context, vaultID string, opts ...onepassword.ItemListFilter) ([]onepassword.ItemOverview, error) {
+	return f.listAllResult, nil
+}
+
+func (f *statefulFakeLister) Shares() onepassword.ItemsSharesAPI {
+	return nil
+}
+
+func (f *statefulFakeLister) Files() onepassword.ItemsFilesAPI {
+	return f.fileLister
+}
+
+var _ onepassword.ItemsAPI = (*statefulFakeLister)(nil)
+
 type fakeClient struct {
 	resolveResult   string
 	resolveError    error
@@ -673,6 +737,87 @@ func (f *fakeClient) ResolveAll(ctx context.Context, secretReferences []string)
 	return f.resolveAll, f.resolveAllError
 }
 
+func TestDeleteMultipleFieldsFromSameItem(t *testing.T) {
+	fc := &fakeClient{
+		listAllResult: []onepassword.VaultOverview{
+			{
+				ID:    "test",
+				Title: "test",
+			},
+		},
+	}
+
+	t.Run("deleting second field after item was deleted should not error", func(t *testing.T) {
+		fl := &statefulFakeLister{
+			listAllResult: []onepassword.ItemOverview{
+				{
+					ID:       "test-item-id",
+					Title:    "key",
+					Category: "login",
+					VaultID:  "vault-id",
+				},
+			},
+			items: map[string]onepassword.Item{
+				"test-item-id": {
+					ID:       "test-item-id",
+					Title:    "key",
+					Category: "login",
+					VaultID:  "vault-id",
+					Fields: []onepassword.ItemField{
+						{
+							ID:        "field-1",
+							Title:     "username",
+							FieldType: onepassword.ItemFieldTypeConcealed,
+							Value:     "testuser",
+						},
+						{
+							ID:        "field-2",
+							Title:     "password",
+							FieldType: onepassword.ItemFieldTypeConcealed,
+							Value:     "testpass",
+						},
+					},
+				},
+			},
+		}
+
+		p := &Provider{
+			client: &onepassword.Client{
+				SecretsAPI: fc,
+				VaultsAPI:  fc,
+				ItemsAPI:   fl,
+			},
+		}
+
+		ctx := t.Context()
+
+		err := p.DeleteSecret(ctx, &v1alpha1.PushSecretRemoteRef{
+			RemoteKey: "key",
+			Property:  "username",
+		})
+		require.NoError(t, err, "first field deletion should succeed")
+		assert.True(t, fl.putCalled, "Put should have been called to update the item")
+		assert.False(t, fl.deleteCalled, "Delete should not have been called yet")
+
+		fl.putCalled = false
+
+		err = p.DeleteSecret(ctx, &v1alpha1.PushSecretRemoteRef{
+			RemoteKey: "key",
+			Property:  "password",
+		})
+		require.NoError(t, err, "second field deletion should succeed")
+		assert.True(t, fl.deleteCalled, "Delete should have been called to remove the item")
+
+		fl.listAllResult = nil
+
+		err = p.DeleteSecret(ctx, &v1alpha1.PushSecretRemoteRef{
+			RemoteKey: "key",
+			Property:  "some-other-field",
+		})
+		require.NoError(t, err, "deleting a field from an already-deleted item should not error (this is the bug!)")
+	})
+}
+
 func TestCachingGetSecret(t *testing.T) {
 	t.Run("cache hit returns cached value", func(t *testing.T) {
 		fcWithCounter := &fakeClientWithCounter{