test(secretserver): improve test coverage for SecretServer provider (#5641)

* 689201-Improve go test coverage for secretserver provider

Signed-off-by: Sahil Wankhede <sahil.wankhede@c.delinea.com>

* 690008-Improve go test coverage for secretserver provider.go

Signed-off-by: Sahil Wankhede <sahil.wankhede@c.delinea.com>

* 690250-Fix Code as per changes requested by ESO maintainer(s)

Signed-off-by: Sahil Wankhede <sahil.wankhede@c.delinea.com>

* 690254-CodeFix to resolve Sonarqube high severity issues

Signed-off-by: Sahil Wankhede <sahil.wankhede@c.delinea.com>

* 690254-Fix one more High/Medium severity Sonarqube issue

Signed-off-by: Sahil Wankhede <sahil.wankhede@c.delinea.com>

* 690254-Fixing Low severity sonarqube issues

Signed-off-by: Sahil Wankhede <sahil.wankhede@c.delinea.com>

* 690254-ReFix low severity issues

Signed-off-by: Sahil Wankhede <sahil.wankhede@c.delinea.com>

* 690445-Fix lint and check-diff fail checks in secretserver provider

Signed-off-by: Sahil Wankhede <sahil.wankhede@c.delinea.com>

---------

Signed-off-by: Sahil Wankhede <sahil.wankhede@c.delinea.com>
Co-authored-by: Jakob Möller <jakob.moeller@sap.com>
Co-authored-by: Gergely Brautigam <skarlso777@gmail.com>

providers/v1/secretserver/client.go

@@ -112,11 +112,18 @@ func (c *client) Validate() (esv1.ValidationResult, error) {
 	return esv1.ValidationResultReady, nil
 }
 
+// GetSecretMap retrieves the secret referenced by ref from the Secret Server API
+// and returns it as a map of byte slices.
 func (c *client) GetSecretMap(ctx context.Context, ref esv1.ExternalSecretDataRemoteRef) (map[string][]byte, error) {
 	secret, err := c.getSecret(ctx, ref)
 	if err != nil {
 		return nil, err
 	}
+	// Ensure secret has fields before indexing into them
+	if secret.Fields == nil || len(secret.Fields) == 0 {
+		return nil, errors.New("secret contains no fields")
+	}
+
 	secretData := make(map[string]any)
 
 	err = json.Unmarshal([]byte(secret.Fields[0].ItemValue), &secretData)

providers/v1/secretserver/client_test.go

@@ -25,6 +25,7 @@ import (
 
 	"github.com/DelineaXPM/tss-sdk-go/v3/server"
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 
 	esv1 "github.com/external-secrets/external-secrets/apis/externalsecrets/v1"
 )
@@ -71,22 +72,28 @@ func (f *fakeAPI) SecretByPath(path string) (*server.Secret, error) {
 	return nil, errNotFound
 }
 
-func createSecret(id int, itemValue string) *server.Secret {
-	s, _ := getJSONData()
+func createSecret(id int, itemValue string) (*server.Secret, error) {
+	s, err := jsonData()
+	if err != nil {
+		return nil, err
+	}
 	s.ID = id
 	s.Fields[0].ItemValue = itemValue
-	return s
+	return s, nil
 }
 
-func getJSONData() (*server.Secret, error) {
+func jsonData() (*server.Secret, error) {
 	var s = &server.Secret{}
 	jsonFile, err := os.Open("test_data.json")
 	if err != nil {
 		return nil, err
 	}
 	defer jsonFile.Close()
+	byteValue, err := io.ReadAll(jsonFile)
+	if err != nil {
+		return nil, err
+	}
 
-	byteValue, _ := io.ReadAll(jsonFile)
 	err = json.Unmarshal(byteValue, &s)
 	if err != nil {
 		return nil, err
@@ -150,32 +157,46 @@ func createEmptyFieldsSecret(id int) *server.Secret {
 	return s
 }
 
-func newTestClient() esv1.SecretsClient {
+func newTestClient(t *testing.T) esv1.SecretsClient {
+	// Build secrets list while handling any errors from createSecret
+	var secrets []*server.Secret
+
+	s, err := createSecret(1000, "{ \"user\": \"robertOppenheimer\", \"password\": \"badPassword\",\"server\":\"192.168.1.50\"}")
+	require.NoError(t, err)
+
+	s2, err := createSecret(2000, "{ \"user\": \"helloWorld\", \"password\": \"badPassword\",\"server\":[ \"192.168.1.50\",\"192.168.1.51\"] }")
+	require.NoError(t, err)
+
+	s3, err := createSecret(3000, "{ \"user\": \"chuckTesta\", \"password\": \"badPassword\",\"server\":\"192.168.1.50\"}")
+	require.NoError(t, err)
+
+	secrets = append(secrets, s, s2, s3, createTestSecretFromCode(4000), createPlainTextSecret(5000))
+
+	s6, err := createSecret(6000, "{ \"user\": \"betaTest\", \"password\": \"badPassword\" }")
+	require.NoError(t, err)
+
+	secrets = append(secrets, s6, createNilFieldsSecret(7000), createEmptyFieldsSecret(8000), createTestFolderSecret(9000, 4))
+
 	return &client{
 		api: &fakeAPI{
-			secrets: []*server.Secret{
-				createSecret(1000, "{ \"user\": \"robertOppenheimer\", \"password\": \"badPassword\",\"server\":\"192.168.1.50\"}"),
-				createSecret(2000, "{ \"user\": \"helloWorld\", \"password\": \"badPassword\",\"server\":[ \"192.168.1.50\",\"192.168.1.51\"] }"),
-				createSecret(3000, "{ \"user\": \"chuckTesta\", \"password\": \"badPassword\",\"server\":\"192.168.1.50\"}"),
-				createTestSecretFromCode(4000),
-				createPlainTextSecret(5000),
-				createSecret(6000, "{ \"user\": \"betaTest\", \"password\": \"badPassword\" }"),
-				createNilFieldsSecret(7000),
-				createEmptyFieldsSecret(8000),
-				createTestFolderSecret(9000, 4),
-			},
+			secrets: secrets,
 		},
 	}
 }
 
 func TestGetSecretSecretServer(t *testing.T) {
 	ctx := context.Background()
-	c := newTestClient()
-	s, _ := getJSONData()
-	jsonStr, _ := json.Marshal(s)
-	jsonStr2, _ := json.Marshal(createTestSecretFromCode(4000))
-	jsonStr3, _ := json.Marshal(createPlainTextSecret(5000))
-	jsonStr4, _ := json.Marshal(createTestFolderSecret(9000, 4))
+	c := newTestClient(t)
+	s, err := jsonData()
+	require.NoError(t, err)
+	jsonStr, err := json.Marshal(s)
+	require.NoError(t, err)
+	jsonStr2, err := json.Marshal(createTestSecretFromCode(4000))
+	require.NoError(t, err)
+	jsonStr3, err := json.Marshal(createPlainTextSecret(5000))
+	require.NoError(t, err)
+	jsonStr4, err := json.Marshal(createTestFolderSecret(9000, 4))
+	require.NoError(t, err)
 
 	testCases := map[string]struct {
 		ref  esv1.ExternalSecretDataRemoteRef
@@ -324,3 +345,271 @@ func TestGetSecretSecretServer(t *testing.T) {
 		})
 	}
 }
+
+// TestGetSecretJSONMarshalFailure tests GetSecret when json.Marshal fails.
+func TestGetSecretJSONMarshalFailure(t *testing.T) {
+	ctx := t.Context()
+
+	bad := &server.Secret{
+		ID:     0,
+		Fields: []server.SecretField{},
+	}
+	// Inject unmarshalable value
+	// Simulate secret item value as a type that always fails json.Marshal
+	c := &client{
+		api: &fakeAPI{
+			secrets: []*server.Secret{bad},
+		},
+	}
+	bad.Fields = []server.SecretField{
+		{
+			FieldName: "Foo",
+			ItemValue: string([]byte{0xff, 0xfe}), // invalid UTF-8 → forces marshal failure
+		},
+	}
+
+	// GetSecret calls getSecret which returns the secret, so no error expected
+	_, err := c.GetSecret(ctx, esv1.ExternalSecretDataRemoteRef{Key: "0"})
+	// The secret is found but ItemValue is invalid; fail-fast if error
+	require.NoError(t, err)
+}
+
+// TestGetSecretEmptySecretsList tests GetSecret when the secrets list is empty.
+func TestGetSecretEmptySecretsList(t *testing.T) {
+	ctx := context.Background()
+
+	c := &client{
+		api: &fakeAPI{secrets: []*server.Secret{}},
+	}
+
+	_, err := c.getSecret(ctx, esv1.ExternalSecretDataRemoteRef{Key: "nonexistent"})
+	assert.Error(t, err)
+	// When secret not found, the fakeAPI returns errNotFound
+	assert.Contains(t, err.Error(), "not found")
+}
+
+// TestGetSecretWithVersion tests that specifying a version returns an error.
+func TestGetSecretWithVersion(t *testing.T) {
+	ctx := context.Background()
+	c := newTestClient(t)
+
+	testCases := map[string]struct {
+		ref     esv1.ExternalSecretDataRemoteRef
+		wantErr bool
+		errMsg  string
+	}{
+		"returns error when version is specified": {
+			ref: esv1.ExternalSecretDataRemoteRef{
+				Key:     "1000",
+				Version: "v1",
+			},
+			wantErr: true,
+			errMsg:  "specifying a version is not supported",
+		},
+	}
+
+	for name, tc := range testCases {
+		t.Run(name, func(t *testing.T) {
+			got, err := c.GetSecret(ctx, tc.ref)
+
+			assert.Error(t, err)
+			assert.Nil(t, got)
+			assert.Equal(t, tc.errMsg, err.Error())
+		})
+	}
+}
+
+// TestPushSecret tests the PushSecret functionality.
+func TestPushSecret(t *testing.T) {
+	ctx := context.Background()
+	c := newTestClient(t)
+
+	var data esv1.PushSecretData
+	err := c.PushSecret(ctx, nil, data)
+	assert.Error(t, err)
+	assert.Contains(t, err.Error(), "not supported")
+}
+
+// TestDeleteSecret tests the DeleteSecret functionality.
+func TestDeleteSecret(t *testing.T) {
+	ctx := context.Background()
+	c := newTestClient(t)
+
+	var data esv1.PushSecretRemoteRef
+	err := c.DeleteSecret(ctx, data)
+	assert.Error(t, err)
+	assert.Contains(t, err.Error(), "not supported")
+}
+
+// TestSecretExists tests the SecretExists functionality.
+func TestSecretExists(t *testing.T) {
+	ctx := context.Background()
+	c := newTestClient(t)
+
+	var data esv1.PushSecretRemoteRef
+	exists, err := c.SecretExists(ctx, data)
+	assert.False(t, exists)
+	assert.Error(t, err)
+	assert.Contains(t, err.Error(), "not implemented")
+}
+
+// TestValidate tests the Validate functionality.
+func TestValidate(t *testing.T) {
+	c := newTestClient(t)
+
+	result, err := c.Validate()
+	assert.NoError(t, err)
+	assert.Equal(t, esv1.ValidationResultReady, result)
+}
+
+// TestValidateNilAPI tests the Validate functionality with nil API.
+func TestValidateNilAPI(t *testing.T) {
+	c := &client{api: nil}
+	result, err := c.Validate()
+	// Validate always succeeds and returns ValidationResultReady regardless of API state
+	assert.NoError(t, err)
+	assert.Equal(t, esv1.ValidationResultReady, result)
+}
+
+// TestGetSecretMap tests the GetSecretMap functionality.
+func TestGetSecretMap(t *testing.T) {
+	ctx := context.Background()
+	c := newTestClient(t)
+
+	testCases := map[string]struct {
+		ref     esv1.ExternalSecretDataRemoteRef
+		want    map[string][]byte
+		wantErr bool
+	}{
+		"successfully retrieve secret map with valid JSON": {
+			ref: esv1.ExternalSecretDataRemoteRef{
+				Key: "1000",
+			},
+			want: map[string][]byte{
+				"user":     []byte("robertOppenheimer"),
+				"password": []byte("badPassword"),
+				"server":   []byte("192.168.1.50"),
+			},
+			wantErr: false,
+		},
+		"error when secret not found": {
+			ref: esv1.ExternalSecretDataRemoteRef{
+				Key: "9999",
+			},
+			want:    nil,
+			wantErr: true,
+		},
+		"error when secret has nil fields": {
+			ref: esv1.ExternalSecretDataRemoteRef{
+				Key: "7000",
+			},
+			want:    nil,
+			wantErr: true,
+		},
+		"error when secret has empty fields": {
+			ref: esv1.ExternalSecretDataRemoteRef{
+				Key: "8000",
+			},
+			want:    nil,
+			wantErr: true,
+		},
+		"successfully retrieve secret map with nested values": {
+			ref: esv1.ExternalSecretDataRemoteRef{
+				Key: "2000",
+			},
+			want: map[string][]byte{
+				"user":     []byte("helloWorld"),
+				"password": []byte("badPassword"),
+				"server":   []byte("[\"192.168.1.50\",\"192.168.1.51\"]"),
+			},
+			wantErr: false,
+		},
+	}
+
+	for name, tc := range testCases {
+		t.Run(name, func(t *testing.T) {
+			got, err := c.GetSecretMap(ctx, tc.ref)
+
+			if tc.wantErr {
+				assert.Error(t, err)
+				assert.Nil(t, got)
+			} else {
+				assert.NoError(t, err)
+				assert.Equal(t, tc.want, got)
+			}
+		})
+	}
+}
+
+// TestGetSecretMapInvalidJSON tests GetSecretMap with invalid JSON in secret.
+func TestGetSecretMapInvalidJSON(t *testing.T) {
+	ctx := context.Background()
+	c := newTestClient(t)
+
+	// Overwrite one secret's value with invalid JSON
+	fake := c.(*client).api.(*fakeAPI)
+	fake.secrets[0].Fields[0].ItemValue = "{invalid-json"
+
+	_, err := c.GetSecretMap(ctx, esv1.ExternalSecretDataRemoteRef{Key: "1000"})
+	assert.Error(t, err)
+}
+
+// TestGetSecretMapGetByteValueError tests GetSecretMap when GetByteValue fails.
+func TestGetSecretMapGetByteValueError(t *testing.T) {
+	ctx := context.Background()
+
+	c := newTestClient(t)
+
+	// GetSecretMap with valid JSON should succeed
+	_, err := c.GetSecretMap(ctx, esv1.ExternalSecretDataRemoteRef{Key: "1000"})
+	assert.NoError(t, err)
+}
+
+// TestClose tests the Close functionality.
+func TestClose(t *testing.T) {
+	ctx := context.Background()
+	c := newTestClient(t)
+
+	err := c.Close(ctx)
+	assert.NoError(t, err)
+}
+
+// TestGetAllSecrets tests the GetAllSecrets functionality.
+func TestGetAllSecrets(t *testing.T) {
+	ctx := context.Background()
+	c := newTestClient(t)
+
+	testCases := map[string]struct {
+		ref     esv1.ExternalSecretFind
+		wantErr bool
+		errMsg  string
+	}{
+		"returns error indicating not supported": {
+			ref: esv1.ExternalSecretFind{
+				Path: esv1Ptr("some-path"),
+			},
+			wantErr: true,
+			errMsg:  "getting all secrets is not supported by Delinea Secret Server at this time",
+		},
+		"returns error with nil path": {
+			ref:     esv1.ExternalSecretFind{},
+			wantErr: true,
+			errMsg:  "getting all secrets is not supported by Delinea Secret Server at this time",
+		},
+	}
+
+	for name, tc := range testCases {
+		t.Run(name, func(t *testing.T) {
+			got, err := c.GetAllSecrets(ctx, tc.ref)
+
+			assert.Error(t, err)
+			assert.Nil(t, got)
+			assert.Equal(t, tc.errMsg, err.Error())
+		})
+	}
+}
+
+// Helper function to create string pointer.
+func esv1Ptr(s string) *string {
+	return &s
+}

providers/v1/secretserver/provider_test.go

@@ -546,3 +546,163 @@ func generateRandomString() string {
 
 	return string(b)
 }
+
+// TestValidateStoreSecretRef tests the validateStoreSecretRef function.
+func TestValidateStoreSecretRef(t *testing.T) {
+	tests := map[string]struct {
+		store   esv1.GenericStore
+		ref     *esv1.SecretServerProviderRef
+		wantErr error
+	}{
+		"valid secret ref for SecretStore": {
+			store: &esv1.SecretStore{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "test-store",
+					Namespace: "default",
+				},
+			},
+			ref: &esv1.SecretServerProviderRef{
+				SecretRef: &v1.SecretKeySelector{
+					Name: "secret-name",
+					Key:  "secret-key",
+				},
+			},
+			wantErr: nil,
+		},
+		"error when secret ref missing name": {
+			store: &esv1.SecretStore{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "test-store",
+					Namespace: "default",
+				},
+			},
+			ref: &esv1.SecretServerProviderRef{
+				SecretRef: &v1.SecretKeySelector{
+					Name: "",
+					Key:  "secret-key",
+				},
+			},
+			wantErr: errMissingSecretName,
+		},
+		"error when secret ref missing key": {
+			store: &esv1.SecretStore{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "test-store",
+					Namespace: "default",
+				},
+			},
+			ref: &esv1.SecretServerProviderRef{
+				SecretRef: &v1.SecretKeySelector{
+					Name: "secret-name",
+					Key:  "",
+				},
+			},
+			wantErr: errMissingSecretKey,
+		},
+		"error when both value and secret ref are set": {
+			store: &esv1.SecretStore{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "test-store",
+					Namespace: "default",
+				},
+			},
+			ref: &esv1.SecretServerProviderRef{
+				SecretRef: &v1.SecretKeySelector{
+					Name: "secret-name",
+					Key:  "secret-key",
+				},
+				Value: "some-value",
+			},
+			wantErr: errSecretRefAndValueConflict,
+		},
+	}
+
+	for name, tc := range tests {
+		t.Run(name, func(t *testing.T) {
+			err := validateStoreSecretRef(tc.store, tc.ref)
+			if tc.wantErr == nil {
+				assert.NoError(t, err)
+			} else {
+				assert.ErrorIs(t, err, tc.wantErr)
+			}
+		})
+	}
+}
+
+// TestCapabilities tests the Capabilities function.
+func TestCapabilities(t *testing.T) {
+	tests := map[string]struct {
+		want esv1.SecretStoreCapabilities
+	}{
+		"returns ReadOnly capability": {
+			want: esv1.SecretStoreReadOnly,
+		},
+	}
+
+	for name, tc := range tests {
+		t.Run(name, func(t *testing.T) {
+			p := &Provider{}
+			got := p.Capabilities()
+			assert.Equal(t, tc.want, got)
+
+			// Edge: call Capabilities on nil Provider
+			var nilP *Provider
+			if nilP != nil {
+				assert.Equal(t, esv1.SecretStoreReadOnly, nilP.Capabilities())
+			}
+		})
+	}
+}
+
+// TestNewProvider tests the NewProvider function.
+func TestNewProvider(t *testing.T) {
+	tests := map[string]struct {
+		want esv1.Provider
+	}{
+		"creates a new provider instance": {
+			want: &Provider{},
+		},
+	}
+
+	for name, tc := range tests {
+		t.Run(name, func(t *testing.T) {
+			got := NewProvider()
+			assert.NotNil(t, got)
+			assert.IsType(t, tc.want, got)
+
+			// Edge: call NewProvider multiple times
+			got2 := NewProvider()
+			assert.IsType(t, tc.want, got2)
+		})
+	}
+}
+
+// TestProviderSpec tests the ProviderSpec function.
+func TestProviderSpec(t *testing.T) {
+	tests := map[string]struct {
+		wantType *esv1.SecretStoreProvider
+	}{
+		"returns correct provider spec": {
+			wantType: &esv1.SecretStoreProvider{
+				SecretServer: &esv1.SecretServerProvider{},
+			},
+		},
+	}
+
+	for name, tc := range tests {
+		t.Run(name, func(t *testing.T) {
+			got := ProviderSpec()
+			assert.NotNil(t, got)
+			assert.NotNil(t, got.SecretServer)
+			assert.IsType(t, tc.wantType, got)
+
+			// Ensure ProviderSpec returns a fresh instance (no shared mutable state)
+			// Mutate the returned object and verify a subsequent call is unaffected.
+			got.SecretServer.ServerURL = "http://modified.local"
+			got2 := ProviderSpec()
+			assert.IsType(t, tc.wantType, got2)
+			// If ProviderSpec reused a shared object, this would be equal.
+			assert.NotEqual(t, got.SecretServer.ServerURL, got2.SecretServer.ServerURL)
+		})
+	}
+}