feat: add PushSecret and DeleteSecret to onepassword provider (#2646)

* feat: add PushSecret and DeleteSecret to onepassword provider

Signed-off-by: Bryce Thuilot <bryce@thuilot.io>

* refactor: clean code based on suggestions

Signed-off-by: Bryce Thuilot <bryce@thuilot.io>

* refactor: make suggested sonar cube changes

Signed-off-by: Bryce Thuilot <bryce@thuilot.io>

---------

Signed-off-by: Bryce Thuilot <bryce@thuilot.io>

docs/introduction/stability-support.md

@@ -72,7 +72,7 @@ The following table show the support for features across different providers.
 | Alibaba Cloud KMS         |              |              |                      |                         |        x         |             |                             |
 | Oracle Vault              |              |              |                      |                         |        x         |             |                             |
 | Akeyless                  |      x       |      x       |                      |                         |        x         |             |                             |
-| 1Password                 |      x       |              |                      |                         |        x         |             |                             |
+| 1Password                 |      x       |              |                      |                         |        x         |      x      |              x              |
 | Generic Webhook           |              |              |                      |                         |                  |             |              x              |
 | senhasegura DSM           |              |              |                      |                         |        x         |             |                             |
 | Doppler                   |      x       |              |                      |                         |        x         |             |                             |

pkg/provider/onepassword/fake/fake.go

@@ -22,10 +22,13 @@ import (
 
 // OnePasswordMockClient is a fake connect.Client.
 type OnePasswordMockClient struct {
-	MockVaults       map[string][]onepassword.Vault
+	MockVaults             map[string][]onepassword.Vault
-	MockItems        map[string][]onepassword.Item // ID and Title only
+	MockItems              map[string][]onepassword.Item // ID and Title only
-	MockItemFields   map[string]map[string][]*onepassword.ItemField
+	MockItemFields         map[string]map[string][]*onepassword.ItemField
-	MockFileContents map[string][]byte
+	MockFileContents       map[string][]byte
+	UpdateItemValidateFunc func(*onepassword.Item, string) (*onepassword.Item, error)
+	CreateItemValidateFunc func(*onepassword.Item, string) (*onepassword.Item, error)
+	DeleteItemValidateFunc func(*onepassword.Item, string) error
 }
 
 // NewMockClient returns an instantiated mock client.
@@ -116,18 +119,27 @@ func (mockClient *OnePasswordMockClient) GetItemsByTitle(itemUUID, vaultUUID str
 	return items, nil
 }
 
-// CreateItem unused fake.
+// CreateItem will call a validation function if set.
-func (mockClient *OnePasswordMockClient) CreateItem(_ *onepassword.Item, _ string) (*onepassword.Item, error) {
+func (mockClient *OnePasswordMockClient) CreateItem(i *onepassword.Item, s string) (*onepassword.Item, error) {
+	if mockClient.CreateItemValidateFunc != nil {
+		return mockClient.CreateItemValidateFunc(i, s)
+	}
 	return &onepassword.Item{}, nil
 }
 
-// UpdateItem unused fake.
+// UpdateItem will call a validation function if set.
-func (mockClient *OnePasswordMockClient) UpdateItem(_ *onepassword.Item, _ string) (*onepassword.Item, error) {
+func (mockClient *OnePasswordMockClient) UpdateItem(i *onepassword.Item, s string) (*onepassword.Item, error) {
+	if mockClient.UpdateItemValidateFunc != nil {
+		return mockClient.UpdateItemValidateFunc(i, s)
+	}
 	return &onepassword.Item{}, nil
 }
 
-// DeleteItem unused fake.
+// DeleteItem will call a validation function if set.
-func (mockClient *OnePasswordMockClient) DeleteItem(_ *onepassword.Item, _ string) error {
+func (mockClient *OnePasswordMockClient) DeleteItem(i *onepassword.Item, s string) error {
+	if mockClient.DeleteItemValidateFunc != nil {
+		return mockClient.DeleteItemValidateFunc(i, s)
+	}
 	return nil
 }
 

pkg/provider/onepassword/onepassword.go

@@ -15,6 +15,7 @@ package onepassword
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"net/url"
 	"sort"
@@ -45,17 +46,35 @@ const (
 	errFetchK8sSecret                             = "could not fetch ConnectToken Secret: %w"
 	errMissingToken                               = "missing Secret Token"
 	errGetVault                                   = "error finding 1Password Vault: %w"
-	errExpectedOneItem                            = "expected one 1Password Item matching %w"
-	errGetItem                                    = "error finding 1Password Item: %w"
-	errKeyNotFound                                = "key not found in 1Password Vaults: %w"
-	errDocumentNotFound                           = "error finding 1Password Document: %w"
-	errExpectedOneField                           = "expected one 1Password ItemField matching %w"
-	errTagsNotImplemented                         = "'find.tags' is not implemented in the 1Password provider"
-	errVersionNotImplemented                      = "'remoteRef.version' is not implemented in the 1Password provider"
 
-	documentCategory      = "DOCUMENT"
+	errGetItem               = "error finding 1Password Item: %w"
-	fieldsWithLabelFormat = "'%s' in '%s', got %d"
+	errUpdateItem            = "error updating 1Password Item: %w"
-	incorrectCountFormat  = "'%s', got %d"
+	errDocumentNotFound      = "error finding 1Password Document: %w"
+	errTagsNotImplemented    = "'find.tags' is not implemented in the 1Password provider"
+	errVersionNotImplemented = "'remoteRef.version' is not implemented in the 1Password provider"
+	errCreateItem            = "error creating 1Password Item: %w"
+	errDeleteItem            = "error deleting 1Password Item: %w"
+	// custom error messages.
+	errKeyNotFoundMsg       = "key not found in 1Password Vaults"
+	errNoVaultsMsg          = "no vaults found"
+	errNoChangesMsg         = "no changes made to 1Password Item"
+	errExpectedOneItemMsg   = "expected one 1Password Item matching"
+	errExpectedOneFieldMsg  = "expected one 1Password ItemField matching"
+	errExpectedOneFieldMsgF = "%w: '%s' in '%s', got %d"
+
+	documentCategory = "DOCUMENT"
+)
+
+// Custom Errors //.
+var (
+	// ErrKeyNotFound is returned when a key is not found in the 1Password Vaults.
+	ErrKeyNotFound = errors.New(errKeyNotFoundMsg)
+	// ErrNoVaults is returned when no vaults are found in the 1Password provider.
+	ErrNoVaults = errors.New(errNoVaultsMsg)
+	// ErrExpectedOneField is returned when more than 1 field is found in the 1Password Vaults.
+	ErrExpectedOneField = errors.New(errExpectedOneFieldMsg)
+	// ErrExpectedOneItem is returned when more than 1 item is found in the 1Password Vaults.
+	ErrExpectedOneItem = errors.New(errExpectedOneItemMsg)
 )
 
 // ProviderOnePassword is a provider for 1Password.
@@ -152,13 +171,160 @@ func validateStore(store esv1beta1.GenericStore) error {
 	return nil
 }
 
-func (provider *ProviderOnePassword) DeleteSecret(_ context.Context, _ esv1beta1.PushSecretRemoteRef) error {
+func deleteField(fields []*onepassword.ItemField, label string) ([]*onepassword.ItemField, error) {
-	return fmt.Errorf("not implemented")
+	// This will always iterate over all items
+	// but its done to ensure that two fields with the same label
+	// exist resulting in undefined behavior
+	var (
+		found   bool
+		fieldsF = make([]*onepassword.ItemField, 0, len(fields))
+	)
+	for _, item := range fields {
+		if item.Label == label {
+			if found {
+				return nil, ErrExpectedOneField
+			}
+			found = true
+			continue
+		}
+		fieldsF = append(fieldsF, item)
+	}
+	return fieldsF, nil
 }
 
-// Not Implemented PushSecret.
+func (provider *ProviderOnePassword) DeleteSecret(_ context.Context, ref esv1beta1.PushSecretRemoteRef) error {
-func (provider *ProviderOnePassword) PushSecret(_ context.Context, _ *corev1.Secret, _ esv1beta1.PushSecretData) error {
+	providerItem, err := provider.findItem(ref.GetRemoteKey())
-	return fmt.Errorf("not implemented")
+	if err != nil {
+		return err
+	}
+
+	providerItem.Fields, err = deleteField(providerItem.Fields, ref.GetProperty())
+	if err != nil {
+		return fmt.Errorf(errUpdateItem, err)
+	}
+
+	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 {
+			return fmt.Errorf(errDeleteItem, err)
+		}
+		return nil
+	}
+
+	if _, err = provider.client.UpdateItem(providerItem, providerItem.Vault.ID); err != nil {
+		return fmt.Errorf(errDeleteItem, err)
+	}
+	return nil
+}
+
+const (
+	passwordLabel = "password"
+)
+
+// createItem creates a new item in the first vault. If no vaults exist, it returns an error.
+func (provider *ProviderOnePassword) createItem(val []byte, ref esv1beta1.PushSecretData) error {
+	// Get the first vault
+	sortedVaults := sortVaults(provider.vaults)
+	if len(sortedVaults) == 0 {
+		return ErrNoVaults
+	}
+	vaultID := sortedVaults[0]
+	// Get the label
+	label := ref.GetProperty()
+	if label == "" {
+		label = passwordLabel
+	}
+
+	// Create the item
+	item := &onepassword.Item{
+		Title:    ref.GetRemoteKey(),
+		Category: onepassword.Server,
+		Vault: onepassword.ItemVault{
+			ID: vaultID,
+		},
+		Fields: []*onepassword.ItemField{
+			generateNewItemField(label, string(val)),
+		},
+	}
+
+	_, err := provider.client.CreateItem(item, vaultID)
+	return err
+}
+
+// updateFieldValue updates the fields value of an item with the given label.
+// If the label does not exist, a new field is created. If the label exists but
+// the value is different, the value is updated. If the label exists and the
+// value is the same, nothing is done.
+func updateFieldValue(fields []*onepassword.ItemField, label, newVal string) ([]*onepassword.ItemField, error) {
+	// This will always iterate over all items
+	// but its done to ensure that two fields with the same label
+	// exist resulting in undefined behavior
+	var (
+		found bool
+		index int
+	)
+	for i, item := range fields {
+		if item.Label == label {
+			if found {
+				return nil, ErrExpectedOneField
+			}
+			found = true
+			index = i
+		}
+	}
+	if !found {
+		return append(fields, generateNewItemField(label, newVal)), nil
+	}
+	if field := fields[index]; newVal != field.Value {
+		field.Value = newVal
+		fields[index] = field
+	}
+
+	return fields, nil
+}
+
+// generateNewItemField generates a new item field with the given label and value.
+func generateNewItemField(label, newVal string) *onepassword.ItemField {
+	field := &onepassword.ItemField{
+		Label: label,
+		Value: newVal,
+		Type:  onepassword.FieldTypeConcealed,
+	}
+
+	return field
+}
+
+func (provider *ProviderOnePassword) PushSecret(_ context.Context, secret *corev1.Secret, ref esv1beta1.PushSecretData) error {
+	val, ok := secret.Data[ref.GetSecretKey()]
+	if !ok {
+		return ErrKeyNotFound
+	}
+
+	title := ref.GetRemoteKey()
+	providerItem, err := provider.findItem(title)
+	if errors.Is(err, ErrKeyNotFound) {
+		if err = provider.createItem(val, ref); err != nil {
+			return fmt.Errorf(errCreateItem, err)
+		}
+		return nil
+	} else if err != nil {
+		return err
+	}
+
+	label := ref.GetProperty()
+	if label == "" {
+		label = passwordLabel
+	}
+
+	providerItem.Fields, err = updateFieldValue(providerItem.Fields, label, string(val))
+	if err != nil {
+		return fmt.Errorf(errUpdateItem, err)
+	}
+
+	if _, err = provider.client.UpdateItem(providerItem, providerItem.Vault.ID); err != nil {
+		return fmt.Errorf(errUpdateItem, err)
+	}
+	return nil
 }
 
 // GetSecret returns a single secret from the provider.
@@ -260,11 +426,11 @@ func (provider *ProviderOnePassword) findItem(name string) (*onepassword.Item, e
 		case len(items) == 1:
 			return provider.client.GetItemByUUID(items[0].ID, items[0].Vault.ID)
 		case len(items) > 1:
-			return nil, fmt.Errorf(errExpectedOneItem, fmt.Errorf(incorrectCountFormat, name, len(items)))
+			return nil, fmt.Errorf("%w: '%s', got %d", ErrExpectedOneItem, name, len(items))
 		}
 	}
 
-	return nil, fmt.Errorf(errKeyNotFound, fmt.Errorf("%s in: %v", name, provider.vaults))
+	return nil, fmt.Errorf("%w: %s in: %v", ErrKeyNotFound, name, provider.vaults)
 }
 
 func (provider *ProviderOnePassword) getField(item *onepassword.Item, property string) ([]byte, error) {
@@ -275,7 +441,7 @@ func (provider *ProviderOnePassword) getField(item *onepassword.Item, property s
 	}
 
 	if length := countFieldsWithLabel(fieldLabel, item.Fields); length != 1 {
-		return nil, fmt.Errorf(errExpectedOneField, fmt.Errorf(fieldsWithLabelFormat, fieldLabel, item.Title, length))
+		return nil, fmt.Errorf("%w: '%s' in '%s', got %d", ErrExpectedOneField, fieldLabel, item.Title, length)
 	}
 
 	// caution: do not use client.GetValue here because it has undesirable behavior on keys with a dot in them
@@ -297,7 +463,7 @@ func (provider *ProviderOnePassword) getFields(item *onepassword.Item, property
 			continue
 		}
 		if length := countFieldsWithLabel(field.Label, item.Fields); length != 1 {
-			return nil, fmt.Errorf(errExpectedOneField, fmt.Errorf(fieldsWithLabelFormat, field.Label, item.Title, length))
+			return nil, fmt.Errorf(errExpectedOneFieldMsgF, ErrExpectedOneField, field.Label, item.Title, length)
 		}
 
 		// caution: do not use client.GetValue here because it has undesirable behavior on keys with a dot in them
@@ -315,7 +481,7 @@ func (provider *ProviderOnePassword) getAllFields(item onepassword.Item, ref esv
 	item = *i
 	for _, field := range item.Fields {
 		if length := countFieldsWithLabel(field.Label, item.Fields); length != 1 {
-			return fmt.Errorf(errExpectedOneField, fmt.Errorf(fieldsWithLabelFormat, field.Label, item.Title, length))
+			return fmt.Errorf(errExpectedOneFieldMsgF, ErrExpectedOneField, field.Label, item.Title, length)
 		}
 		if ref.Name != nil {
 			matcher, err := find.New(*ref.Name)

pkg/provider/onepassword/onepassword_test.go

@@ -15,11 +15,14 @@ package onepassword
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"reflect"
 	"testing"
 
 	"github.com/1Password/connect-sdk-go/onepassword"
+	corev1 "k8s.io/api/core/v1"
+	apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	pointer "k8s.io/utils/ptr"
 
@@ -65,6 +68,8 @@ const (
 	getAllSecretsErrFormat    = "%s: onepassword.GetAllSecrets(...): -expected, +got:\n-%#v\n+%#v\n"
 	validateStoreErrFormat    = "%s: onepassword.validateStore(...): -expected, +got:\n-%#v\n+%#v\n"
 	findItemErrFormat         = "%s: onepassword.findItem(...): -expected, +got:\n-%#v\n+%#v\n"
+	errFromErrMsgF            = "%w: %s"
+	errDoesNotMatchMsgF       = "%s: error did not match: -expected, +got:\\n-%#v\\n+%#v\\n"
 )
 
 func TestFindItem(t *testing.T) {
@@ -187,7 +192,7 @@ func TestFindItem(t *testing.T) {
 				{
 					checkNote:    "no exist",
 					findItemName: "my-item-no-exist",
-					expectedErr:  fmt.Errorf(errKeyNotFound, fmt.Errorf("my-item-no-exist in: map[my-vault:1]")),
+					expectedErr:  fmt.Errorf("%w: my-item-no-exist in: map[my-vault:1]", ErrKeyNotFound),
 				},
 			},
 		},
@@ -208,7 +213,7 @@ func TestFindItem(t *testing.T) {
 				{
 					checkNote:    "multiple match",
 					findItemName: myItem,
-					expectedErr:  fmt.Errorf(errExpectedOneItem, fmt.Errorf("'my-item', got 2")),
+					expectedErr:  fmt.Errorf(errFromErrMsgF, ErrExpectedOneItem, "'my-item', got 2"),
 				},
 			},
 		},
@@ -781,7 +786,7 @@ func TestGetSecret(t *testing.T) {
 						Key:      myItem,
 						Property: key1,
 					},
-					expectedErr: fmt.Errorf(errExpectedOneField, fmt.Errorf("'key1' in 'my-item', got 2")),
+					expectedErr: fmt.Errorf(errFromErrMsgF, ErrExpectedOneField, "'key1' in 'my-item', got 2"),
 				},
 			},
 		},
@@ -948,7 +953,7 @@ func TestGetSecretMap(t *testing.T) {
 						Key: myItem,
 					},
 					expectedMap: nil,
-					expectedErr: fmt.Errorf(errExpectedOneField, fmt.Errorf("'key1' in 'my-item', got 2")),
+					expectedErr: fmt.Errorf(errFromErrMsgF, ErrExpectedOneField, "'key1' in 'my-item', got 2"),
 				},
 			},
 		},
@@ -1407,3 +1412,697 @@ func TestHasUniqueVaultNumbers(t *testing.T) {
 		}
 	}
 }
+
+type fakeRef struct {
+	key       string
+	prop      string
+	secretKey string
+}
+
+func (f fakeRef) GetRemoteKey() string {
+	return f.key
+}
+
+func (f fakeRef) GetProperty() string {
+	return f.prop
+}
+
+func (f fakeRef) GetSecretKey() string {
+	return f.secretKey
+}
+
+func (f fakeRef) GetMetadata() *apiextensionsv1.JSON {
+	return nil
+}
+
+func validateItem(t *testing.T, expectedItem, actualItem *onepassword.Item) {
+	t.Helper()
+	if !reflect.DeepEqual(expectedItem, actualItem) {
+		t.Errorf("expected item %v, got %v", expectedItem, actualItem)
+	}
+}
+
+func TestProviderOnePasswordCreateItem(t *testing.T) {
+	type testCase struct {
+		vaults             map[string]int
+		expectedErr        error
+		setupNote          string
+		val                []byte
+		createValidateFunc func(*testing.T, *onepassword.Item, string) (*onepassword.Item, error)
+		ref                esv1beta1.PushSecretData
+	}
+	const vaultName = "vault1"
+
+	thridPartyErr := errors.New("third party error")
+
+	testCases := []testCase{
+		{
+			setupNote: "standard create",
+			val:       []byte("value"),
+			ref: fakeRef{
+				key:  "testing",
+				prop: "prop",
+			},
+			expectedErr: nil,
+			vaults: map[string]int{
+				vaultName: 1,
+			},
+			createValidateFunc: func(t *testing.T, item *onepassword.Item, s string) (*onepassword.Item, error) {
+				validateItem(t, &onepassword.Item{
+					Title:    "testing",
+					Category: onepassword.Server,
+					Vault: onepassword.ItemVault{
+						ID: vaultName,
+					},
+					Fields: []*onepassword.ItemField{
+						generateNewItemField("prop", "value"),
+					},
+				}, item)
+				return item, nil
+			},
+		},
+		{
+			setupNote: "standard create with no property",
+			val:       []byte("value2"),
+			ref: fakeRef{
+				key:  "testing2",
+				prop: "",
+			},
+			vaults: map[string]int{
+				vaultName: 2,
+			},
+			createValidateFunc: func(t *testing.T, item *onepassword.Item, s string) (*onepassword.Item, error) {
+				validateItem(t, &onepassword.Item{
+					Title:    "testing2",
+					Category: onepassword.Server,
+					Vault: onepassword.ItemVault{
+						ID: vaultName,
+					},
+					Fields: []*onepassword.ItemField{
+						generateNewItemField("password", "value2"),
+					},
+				}, item)
+				return item, nil
+			},
+		},
+		{
+			setupNote: "no vaults",
+			val:       []byte("value"),
+			ref: fakeRef{
+				key:  "testing",
+				prop: "prop",
+			},
+			vaults:      map[string]int{},
+			expectedErr: ErrNoVaults,
+			createValidateFunc: func(t *testing.T, item *onepassword.Item, s string) (*onepassword.Item, error) {
+				t.Errorf("onepassword.createItem(...): should not have been called")
+				return nil, nil
+			},
+		},
+		{
+			setupNote: "error on create",
+			val:       []byte("testing"),
+			ref: fakeRef{
+				key:  "another",
+				prop: "property",
+			},
+			vaults: map[string]int{
+				vaultName: 1,
+			},
+			expectedErr: thridPartyErr,
+			createValidateFunc: func(t *testing.T, item *onepassword.Item, s string) (*onepassword.Item, error) {
+				validateItem(t, &onepassword.Item{
+					Title:    "another",
+					Category: onepassword.Server,
+					Vault: onepassword.ItemVault{
+						ID: vaultName,
+					},
+					Fields: []*onepassword.ItemField{
+						generateNewItemField("property", "testing"),
+					},
+				}, item)
+				return nil, thridPartyErr
+			},
+		},
+	}
+	provider := &ProviderOnePassword{}
+	for _, tc := range testCases {
+		// setup
+		mockClient := fake.NewMockClient()
+		mockClient.CreateItemValidateFunc = func(item *onepassword.Item, s string) (*onepassword.Item, error) {
+			i, e := tc.createValidateFunc(t, item, s)
+			return i, e
+		}
+		provider.client = mockClient
+		provider.vaults = tc.vaults
+
+		err := provider.createItem(tc.val, tc.ref)
+		if !errors.Is(err, tc.expectedErr) {
+			t.Errorf(errDoesNotMatchMsgF, tc.setupNote, tc.expectedErr, err)
+		}
+	}
+}
+
+func TestProviderOnePasswordDeleteItem(t *testing.T) {
+	type testCase struct {
+		inputFields    []*onepassword.ItemField
+		fieldName      string
+		expectedErr    error
+		expectedFields []*onepassword.ItemField
+		setupNote      string
+	}
+
+	field1, field2, field3, field4 := "field1", "field2", "field3", "field4"
+	testCases := []testCase{
+		{
+			setupNote: "one field to remove",
+			inputFields: []*onepassword.ItemField{
+				{
+					ID:    field1,
+					Label: field1,
+					Type:  onepassword.FieldTypeAddress,
+				},
+				{
+					ID:    field2,
+					Label: field2,
+					Type:  onepassword.FieldTypeString,
+				},
+				{
+					ID:    field3,
+					Label: field3,
+					Type:  onepassword.FieldTypeConcealed,
+				},
+			},
+			fieldName: field2,
+			expectedFields: []*onepassword.ItemField{
+				{
+					ID:    field1,
+					Label: field1,
+					Type:  onepassword.FieldTypeAddress,
+				},
+				{
+					ID:    field3,
+					Label: field3,
+					Type:  onepassword.FieldTypeConcealed,
+				},
+			},
+		},
+		{
+			setupNote: "no fields to remove",
+			inputFields: []*onepassword.ItemField{
+				{
+					ID:    field1,
+					Label: field1,
+					Type:  onepassword.FieldTypeAddress,
+				},
+				{
+					ID:    field2,
+					Label: field2,
+					Type:  onepassword.FieldTypeString,
+				},
+				{
+					ID:    field3,
+					Label: field3,
+					Type:  onepassword.FieldTypeConcealed,
+				},
+			},
+			expectedErr: nil,
+			fieldName:   field4,
+			expectedFields: []*onepassword.ItemField{
+				{
+					ID:    field1,
+					Label: field1,
+					Type:  onepassword.FieldTypeAddress,
+				},
+				{
+					ID:    field2,
+					Label: field2,
+					Type:  onepassword.FieldTypeString,
+				},
+				{
+					ID:    field3,
+					Label: field3,
+					Type:  onepassword.FieldTypeConcealed,
+				},
+			},
+		},
+		{
+			setupNote: "multiple fields to remove",
+			inputFields: []*onepassword.ItemField{
+				{
+					ID:    field3,
+					Label: field3,
+					Type:  onepassword.FieldTypeConcealed,
+				},
+				{
+					ID:    field1,
+					Label: field1,
+					Type:  onepassword.FieldTypeAddress,
+				},
+				{
+					ID:    field3,
+					Label: field3,
+					Type:  onepassword.FieldTypeCreditCardType,
+				},
+				{
+					ID:    field2,
+					Label: field2,
+					Type:  onepassword.FieldTypeString,
+				},
+				{
+					ID:    field3,
+					Label: field3,
+					Type:  onepassword.FieldTypeGender,
+				},
+			},
+			fieldName:      field3,
+			expectedErr:    ErrExpectedOneField,
+			expectedFields: nil,
+		},
+	}
+
+	// run the tests
+	for _, tc := range testCases {
+		actualOutput, err := deleteField(tc.inputFields, tc.fieldName)
+		if len(actualOutput) != len(tc.expectedFields) {
+			t.Errorf("%s: length fields did not match: -expected, +got:\n-%#v\n+%#v\n", tc.setupNote, tc.expectedFields, actualOutput)
+			return
+		}
+		if !errors.Is(err, tc.expectedErr) {
+			t.Errorf(errDoesNotMatchMsgF, tc.setupNote, tc.expectedErr, err)
+		}
+		for i, check := range tc.expectedFields {
+			if len(actualOutput) <= i {
+				continue
+			}
+			if !reflect.DeepEqual(check, actualOutput[i]) {
+				t.Errorf("%s: fields at position %d did not match: -expected, +got:\n-%#v\n+%#v\n", tc.setupNote, i, check, actualOutput[i])
+			}
+		}
+	}
+}
+
+func TestUpdateFields(t *testing.T) {
+	type testCase struct {
+		inputFields    []*onepassword.ItemField
+		fieldName      string
+		newVal         string
+		expectedErr    error
+		expectedFields []*onepassword.ItemField
+		setupNote      string
+	}
+
+	field1, field2, field3, field4 := "field1", "field2", "field3", "field4"
+	testCases := []testCase{
+		{
+			setupNote: "one field to update",
+			inputFields: []*onepassword.ItemField{
+				{
+					ID:    field1,
+					Label: field1,
+					Value: value1,
+					Type:  onepassword.FieldTypeAddress,
+				},
+				{
+					ID:    field2,
+					Label: field2,
+					Value: value2,
+					Type:  onepassword.FieldTypeString,
+				},
+				{
+					ID:    field3,
+					Label: field3,
+					Value: value3,
+					Type:  onepassword.FieldTypeConcealed,
+				},
+			},
+			fieldName: field2,
+			newVal:    "testing",
+			expectedFields: []*onepassword.ItemField{
+				{
+					ID:    field1,
+					Label: field1,
+					Value: value1,
+					Type:  onepassword.FieldTypeAddress,
+				},
+				{
+					ID:    field2,
+					Label: field2,
+					Value: "testing",
+					Type:  onepassword.FieldTypeString,
+				},
+				{
+					ID:    field3,
+					Label: field3,
+					Value: value3,
+					Type:  onepassword.FieldTypeConcealed,
+				},
+			},
+		},
+		{
+			setupNote: "add field",
+			inputFields: []*onepassword.ItemField{
+				{
+					ID:    field1,
+					Value: value1,
+					Label: field1,
+					Type:  onepassword.FieldTypeAddress,
+				},
+				{
+					ID:    field2,
+					Label: field2,
+					Value: value2,
+					Type:  onepassword.FieldTypeString,
+				},
+			},
+			fieldName: field4,
+			newVal:    value4,
+			expectedFields: []*onepassword.ItemField{
+				{
+					ID:    field1,
+					Label: field1,
+					Value: value1,
+					Type:  onepassword.FieldTypeAddress,
+				},
+				{
+					ID:    field2,
+					Label: field2,
+					Value: value2,
+					Type:  onepassword.FieldTypeString,
+				},
+				{
+					Label: field4,
+					Value: value4,
+					Type:  onepassword.FieldTypeConcealed,
+				},
+			},
+		},
+		{
+			setupNote: "no changes",
+			inputFields: []*onepassword.ItemField{
+				{
+					ID:    field1,
+					Label: field1,
+					Value: value1,
+					Type:  onepassword.FieldTypeAddress,
+				},
+				{
+					ID:    field2,
+					Label: field2,
+					Value: value2,
+					Type:  onepassword.FieldTypeString,
+				},
+			},
+			fieldName:   field1,
+			newVal:      value1,
+			expectedErr: nil,
+			expectedFields: []*onepassword.ItemField{
+				{
+					ID:    field1,
+					Label: field1,
+					Value: value1,
+					Type:  onepassword.FieldTypeAddress,
+				},
+				{
+					ID:    field2,
+					Label: field2,
+					Value: value2,
+					Type:  onepassword.FieldTypeString,
+				},
+			},
+		},
+		{
+			setupNote: "multiple fields to remove",
+			inputFields: []*onepassword.ItemField{
+				{
+					ID:    field3,
+					Label: field3,
+					Value: value3,
+					Type:  onepassword.FieldTypeConcealed,
+				},
+				{
+					ID:    field1,
+					Label: field1,
+					Value: value1,
+					Type:  onepassword.FieldTypeAddress,
+				},
+				{
+					ID:    field3,
+					Label: field3,
+					Value: value3,
+					Type:  onepassword.FieldTypeCreditCardType,
+				},
+				{
+					ID:    field2,
+					Label: field2,
+					Value: value2,
+					Type:  onepassword.FieldTypeString,
+				},
+				{
+					ID:    field3,
+					Label: field3,
+					Value: value3,
+					Type:  onepassword.FieldTypeGender,
+				},
+			},
+			fieldName:      field3,
+			expectedErr:    ErrExpectedOneField,
+			expectedFields: nil,
+		},
+	}
+
+	// run the tests
+	for _, tc := range testCases {
+		actualOutput, err := updateFieldValue(tc.inputFields, tc.fieldName, tc.newVal)
+		if len(actualOutput) != len(tc.expectedFields) {
+			t.Errorf("%s: length fields did not match: -expected, +got:\n-%#v\n+%#v\n", tc.setupNote, tc.expectedFields, actualOutput)
+			return
+		}
+		if !errors.Is(err, tc.expectedErr) {
+			t.Errorf(errDoesNotMatchMsgF, tc.setupNote, tc.expectedErr, err)
+		}
+		for i, check := range tc.expectedFields {
+			if len(actualOutput) <= i {
+				continue
+			}
+			if !reflect.DeepEqual(check, actualOutput[i]) {
+				t.Errorf("%s: fields at position %d did not match: -expected, +got:\n-%#v\n+%#v\n", tc.setupNote, i, check, actualOutput[i])
+			}
+		}
+	}
+}
+
+func TestGenerateNewItemField(t *testing.T) {
+	field := generateNewItemField("property", "testing")
+	if !reflect.DeepEqual(field, &onepassword.ItemField{
+		Label: "property",
+		Type:  onepassword.FieldTypeConcealed,
+		Value: "testing",
+	}) {
+		t.Errorf("field did not match: -expected, +got:\n-%#v\n+%#v\n", &onepassword.ItemField{
+			Label: "property",
+			Type:  onepassword.FieldTypeConcealed,
+			Value: "testing",
+		}, field)
+	}
+}
+
+func TestProviderOnePasswordPushSecret(t *testing.T) {
+	// Most logic is tested in the createItem and updateField functions
+	// This test is just to make sure the correct functions are called.
+	// the correct values are passed to them, and errors are propagated
+	type testCase struct {
+		vaults              map[string]int
+		expectedErr         error
+		setupNote           string
+		existingItems       []onepassword.Item
+		val                 *corev1.Secret
+		existingItemsFields map[string][]*onepassword.ItemField
+		createValidateFunc  func(*onepassword.Item, string) (*onepassword.Item, error)
+		updateValidateFunc  func(*onepassword.Item, string) (*onepassword.Item, error)
+		ref                 fakeRef
+	}
+	var (
+		vaultName = "vault1"
+		vault     = onepassword.Vault{
+			ID: vaultName,
+		}
+	)
+	testCases := []testCase{
+		{
+			vaults: map[string]int{
+				vaultName: 1,
+			},
+			expectedErr: ErrExpectedOneItem,
+			setupNote:   "find item error",
+			existingItems: []onepassword.Item{
+				{
+					Title: key1,
+				}, {
+					Title: key1,
+				}, // can be empty, testing for error with length
+			},
+			ref: fakeRef{
+				key:       key1,
+				secretKey: key1,
+			},
+			val: &corev1.Secret{Data: map[string][]byte{key1: []byte("testing")}},
+		},
+		{
+			setupNote:   "create item error",
+			expectedErr: ErrNoVaults,
+			val:         &corev1.Secret{Data: map[string][]byte{key1: []byte("testing")}},
+			ref:         fakeRef{secretKey: key1},
+			vaults:      nil,
+		},
+		{
+			setupNote:   "key not in data",
+			expectedErr: ErrKeyNotFound,
+			val:         &corev1.Secret{Data: map[string][]byte{}},
+			ref:         fakeRef{secretKey: key1},
+			vaults:      nil,
+		},
+		{
+			setupNote:   "create item success",
+			expectedErr: nil,
+			val: &corev1.Secret{Data: map[string][]byte{
+				key1: []byte("testing"),
+			}},
+			ref: fakeRef{
+				key:       key1,
+				prop:      "prop",
+				secretKey: key1,
+			},
+			vaults: map[string]int{
+				vaultName: 1,
+			},
+			createValidateFunc: func(item *onepassword.Item, s string) (*onepassword.Item, error) {
+				validateItem(t, &onepassword.Item{
+					Title:    key1,
+					Category: onepassword.Server,
+					Vault: onepassword.ItemVault{
+						ID: vaultName,
+					},
+					Fields: []*onepassword.ItemField{
+						generateNewItemField("prop", "testing"),
+					},
+				}, item)
+				return item, nil
+			},
+		},
+		{
+			setupNote:   "update fields error",
+			expectedErr: ErrExpectedOneField,
+			val: &corev1.Secret{Data: map[string][]byte{
+				"key2": []byte("testing"),
+			}},
+			ref: fakeRef{
+				key:       key1,
+				prop:      "prop",
+				secretKey: "key2",
+			},
+			vaults: map[string]int{
+				vaultName: 1,
+			},
+			existingItemsFields: map[string][]*onepassword.ItemField{
+				key1: {
+					{
+						Label: "prop",
+					},
+					{
+						Label: "prop",
+					},
+				},
+			},
+			existingItems: []onepassword.Item{
+				{
+					Vault: onepassword.ItemVault{
+						ID: vaultName,
+					},
+					ID:    key1,
+					Title: key1,
+				},
+			},
+		},
+		{
+			setupNote:   "standard update",
+			expectedErr: nil,
+			val: &corev1.Secret{Data: map[string][]byte{
+				"key3": []byte("testing2"),
+			}},
+			ref: fakeRef{
+				key:       key1,
+				prop:      "",
+				secretKey: "key3",
+			},
+			vaults: map[string]int{
+				vaultName: 1,
+			},
+			existingItemsFields: map[string][]*onepassword.ItemField{
+				key1: {
+					{
+						Label: "not-prop",
+					},
+				},
+			},
+			updateValidateFunc: func(item *onepassword.Item, s string) (*onepassword.Item, error) {
+				validateItem(t, &onepassword.Item{
+					Vault: onepassword.ItemVault{
+						ID: vaultName,
+					},
+					ID:    key1,
+					Title: key1,
+					Fields: []*onepassword.ItemField{
+						{
+							Label: "not-prop",
+						},
+						{
+							Label: "password",
+							Value: "testing2",
+							Type:  onepassword.FieldTypeConcealed,
+						},
+					},
+				}, item)
+				return nil, nil
+			},
+			existingItems: []onepassword.Item{
+				{
+					Vault: onepassword.ItemVault{
+						ID: vaultName,
+					},
+					ID:    key1,
+					Title: key1,
+				},
+			},
+		},
+	}
+	provider := &ProviderOnePassword{}
+	for _, tc := range testCases {
+		t.Run(tc.setupNote, func(t *testing.T) {
+			// setup
+			mockClient := fake.NewMockClient()
+			mockClient.MockVaults = map[string][]onepassword.Vault{
+				vaultName: {vault},
+			}
+			mockClient.MockItems = map[string][]onepassword.Item{
+				vaultName: tc.existingItems,
+			}
+			mockClient.MockItemFields = map[string]map[string][]*onepassword.ItemField{
+				vaultName: tc.existingItemsFields,
+			}
+			mockClient.CreateItemValidateFunc = func(item *onepassword.Item, s string) (*onepassword.Item, error) {
+				return tc.createValidateFunc(item, s)
+			}
+			mockClient.UpdateItemValidateFunc = func(item *onepassword.Item, s string) (*onepassword.Item, error) {
+				return tc.updateValidateFunc(item, s)
+			}
+			provider.client = mockClient
+			provider.vaults = tc.vaults
+
+			err := provider.PushSecret(context.Background(), tc.val, tc.ref)
+			if !errors.Is(err, tc.expectedErr) {
+				t.Errorf(errDoesNotMatchMsgF, tc.setupNote, tc.expectedErr, err)
+			}
+		})
+	}
+}