infisical: fix error handling which previously failed silently (missing secrets, incorrect auth, etc.) (#4304)

* add error handling to Infisical provider

Signed-off-by: Joey Pereira <joey@pereira.io>

* add access token error handling

While adding test cases for RevokeAccessToken, I realized
that the tests were simply exiting early because of the access
token. Instead, let's be explicit and return an error.

Signed-off-by: Joey Pereira <joey@pereira.io>

* add assertion for ImportedSecrets

Signed-off-by: Joey Pereira <joey@pereira.io>

* rewrite tests to use httptest

Signed-off-by: Joey Pereira <joey@pereira.io>

* refactor API calls to simplify common code

Signed-off-by: Joey Pereira <joey@pereira.io>

* better handle responses that cannot unmarshal; only return NoSecretError on GetSecretByKeyV3

Signed-off-by: Joey Pereira <joey@pereira.io>

* cleanup tests

Additionally, this correctly plumbs through more of the
error response data and avoids leaking a 200 response
on unmarshal errors

Signed-off-by: Joey Pereira <joey@pereira.io>

* fix provider.go test (given swap away from improper infisical API impl)

Signed-off-by: Joey Pereira <joey@pereira.io>

* improve details format

Signed-off-by: Joey Pereira <joey@pereira.io>

* bin/golangci-lint fixes

Signed-off-by: Joey Pereira <joey@pereira.io>

* address feedback

Signed-off-by: Joey Pereira <joey@pereira.io>

* address quality issue

Signed-off-by: Joey Pereira <joey@pereira.io>

* address comments

Signed-off-by: Joey Pereira <joey@pereira.io>

* replace reflect call with simpler zero-value check

Signed-off-by: Joey Pereira <joey@pereira.io>

---------

Signed-off-by: Joey Pereira <joey@pereira.io>
Co-authored-by: Gergely Brautigam <182850+Skarlso@users.noreply.github.com>

pkg/provider/infisical/api/api.go

@@ -19,10 +19,10 @@ import (
 	"encoding/json"
 	"errors"
 	"fmt"
+	"io"
 	"net/http"
 	"net/url"
 	"strconv"
-	"time"
 
 	esv1beta1 "github.com/external-secrets/external-secrets/apis/externalsecrets/v1beta1"
 	"github.com/external-secrets/external-secrets/pkg/metrics"
@@ -42,10 +42,71 @@ type InfisicalApis interface {
 	RevokeAccessToken() error
 }
 
+const (
+	machineIdentityLoginViaUniversalAuth = "MachineIdentityLoginViaUniversalAuth"
+	getSecretsV3                         = "GetSecretsV3"
+	getSecretByKeyV3                     = "GetSecretByKeyV3"
+	revokeAccessToken                    = "RevokeAccessToken"
+)
+
 const UserAgentName = "k8-external-secrets-operator"
-const errJSONSecretUnmarshal = "unable to unmarshal secret: %w"
 
-func NewAPIClient(baseURL string) (*InfisicalClient, error) {
+var errJSONUnmarshal = errors.New("unable to unmarshal API response")
+var errNoAccessToken = errors.New("unexpected error: no access token available to revoke")
+var errAccessTokenAlreadyRetrieved = errors.New("unexpected error: access token was already retrieved")
+
+type InfisicalAPIError struct {
+	StatusCode int
+	Err        any
+	Message    any
+	Details    any
+}
+
+func (e *InfisicalAPIError) Error() string {
+	if e.Details != nil {
+		detailsJSON, _ := json.Marshal(e.Details)
+		return fmt.Sprintf("API error (%d): error=%v message=%v, details=%s", e.StatusCode, e.Err, e.Message, string(detailsJSON))
+	} else {
+		return fmt.Sprintf("API error (%d): error=%v message=%v", e.StatusCode, e.Err, e.Message)
+	}
+}
+
+// checkError checks for an error on the http response and generates an appropriate error if one is
+// found.
+func checkError(resp *http.Response) error {
+	if resp.StatusCode >= 200 && resp.StatusCode < 400 {
+		return nil
+	}
+
+	var buf bytes.Buffer
+	_, err := buf.ReadFrom(resp.Body)
+	if err != nil {
+		return fmt.Errorf("API error (%d) and failed to read response body: %w", resp.StatusCode, err)
+	}
+
+	// Attempt to unmarshal the response body into an InfisicalAPIErrorResponse.
+	var errRes InfisicalAPIErrorResponse
+	err = json.Unmarshal(buf.Bytes(), &errRes)
+	// Non-200 errors that cannot be unmarshaled must be handled, as errors could come from outside of
+	// Infisical.
+	if err != nil {
+		return fmt.Errorf("API error (%d), could not unmarshal error response: %w", resp.StatusCode, err)
+	} else if errRes.StatusCode == 0 {
+		// When the InfisicalResponse has a zero-value status code, then the
+		// response was either malformed or not from Infisical. Instead, just return
+		// the error string from the response.
+		return fmt.Errorf("API error (%d): %s", resp.StatusCode, buf.String())
+	} else {
+		return &InfisicalAPIError{
+			StatusCode: resp.StatusCode,
+			Message:    errRes.Message,
+			Err:        errRes.Error,
+			Details:    errRes.Details,
+		}
+	}
+}
+
+func NewAPIClient(baseURL string, client *http.Client) (*InfisicalClient, error) {
 	baseParsedURL, err := url.Parse(baseURL)
 	if err != nil {
 		return nil, err
@@ -53,9 +114,7 @@ func NewAPIClient(baseURL string) (*InfisicalClient, error) {
 
 	api := &InfisicalClient{
 		BaseURL: baseParsedURL,
-		client: &http.Client{
-			Timeout: time.Second * 15,
-		},
+		client:  client,
 	}
 
 	return api, nil
@@ -63,13 +122,22 @@ func NewAPIClient(baseURL string) (*InfisicalClient, error) {
 
 func (a *InfisicalClient) SetTokenViaMachineIdentity(clientID, clientSecret string) error {
 	if a.token != "" {
-		return nil
+		return errAccessTokenAlreadyRetrieved
 	}
 
-	loginResponse, err := a.MachineIdentityLoginViaUniversalAuth(MachineIdentityUniversalAuthLoginRequest{
-		ClientID:     clientID,
-		ClientSecret: clientSecret,
-	})
+	var loginResponse MachineIdentityDetailsResponse
+	err := a.do(
+		"api/v1/auth/universal-auth/login",
+		http.MethodPost,
+		map[string]string{},
+		MachineIdentityUniversalAuthLoginRequest{
+			ClientID:     clientID,
+			ClientSecret: clientSecret,
+		},
+		&loginResponse,
+	)
+	metrics.ObserveAPICall(constants.ProviderName, machineIdentityLoginViaUniversalAuth, err)
+
 	if err != nil {
 		return err
 	}
@@ -80,9 +148,20 @@ func (a *InfisicalClient) SetTokenViaMachineIdentity(clientID, clientSecret stri
 
 func (a *InfisicalClient) RevokeAccessToken() error {
 	if a.token == "" {
-		return nil
+		return errNoAccessToken
 	}
-	if _, err := a.RevokeMachineIdentityAccessToken(RevokeMachineIdentityAccessTokenRequest{AccessToken: a.token}); err != nil {
+
+	var revokeResponse RevokeMachineIdentityAccessTokenResponse
+	err := a.do(
+		"api/v1/auth/token/revoke",
+		http.MethodPost,
+		map[string]string{},
+		RevokeMachineIdentityAccessTokenRequest{AccessToken: a.token},
+		&revokeResponse,
+	)
+	metrics.ObserveAPICall(constants.ProviderName, revokeAccessToken, err)
+
+	if err != nil {
 		return err
 	}
 
@@ -94,97 +173,85 @@ func (a *InfisicalClient) resolveEndpoint(path string) string {
 	return a.BaseURL.ResolveReference(&url.URL{Path: path}).String()
 }
 
-func (a *InfisicalClient) do(r *http.Request) (*http.Response, error) {
+func (a *InfisicalClient) addHeaders(r *http.Request) {
 	if a.token != "" {
 		r.Header.Add("Authorization", "Bearer "+a.token)
 	}
 	r.Header.Add("User-Agent", UserAgentName)
 	r.Header.Add("Content-Type", "application/json")
-
-	return a.client.Do(r)
 }
 
-func (a *InfisicalClient) MachineIdentityLoginViaUniversalAuth(data MachineIdentityUniversalAuthLoginRequest) (*MachineIdentityDetailsResponse, error) {
-	endpointURL := a.resolveEndpoint("api/v1/auth/universal-auth/login")
-	body, err := MarshalReqBody(data)
-	if err != nil {
-		return nil, err
-	}
+// do is a generic function that makes an API call to the Infisical API, and handle the response
+// (including if an API error is returned).
+func (a *InfisicalClient) do(endpoint, method string, params map[string]string, body, response any) error {
+	endpointURL := a.resolveEndpoint(endpoint)
 
-	req, err := http.NewRequest(http.MethodPost, endpointURL, body)
-	metrics.ObserveAPICall(constants.ProviderName, "MachineIdentityLoginViaUniversalAuth", err)
+	bodyReader, err := MarshalReqBody(body)
 	if err != nil {
-		return nil, err
+		return err
 	}
 
-	rawRes, err := a.do(req)
+	r, err := http.NewRequest(method, endpointURL, bodyReader)
 	if err != nil {
-		return nil, err
+		return err
 	}
 
-	var res MachineIdentityDetailsResponse
-	err = ReadAndUnmarshal(rawRes, &res)
-	if err != nil {
-		return nil, fmt.Errorf(errJSONSecretUnmarshal, err)
+	a.addHeaders(r)
+
+	q := r.URL.Query()
+	for key, value := range params {
+		q.Add(key, value)
 	}
-	return &res, nil
-}
+	r.URL.RawQuery = q.Encode()
 
-func (a *InfisicalClient) RevokeMachineIdentityAccessToken(data RevokeMachineIdentityAccessTokenRequest) (*RevokeMachineIdentityAccessTokenResponse, error) {
-	endpointURL := a.resolveEndpoint("api/v1/auth/token/revoke")
-	body, err := MarshalReqBody(data)
+	resp, err := a.client.Do(r)
 	if err != nil {
-		return nil, err
+		return err
 	}
+	defer resp.Body.Close()
 
-	req, err := http.NewRequest(http.MethodPost, endpointURL, body)
-	metrics.ObserveAPICall(constants.ProviderName, "RevokeMachineIdentityAccessToken", err)
-	if err != nil {
-		return nil, err
+	if err := checkError(resp); err != nil {
+		return err
 	}
 
-	rawRes, err := a.do(req)
+	bodyBytes, err := io.ReadAll(resp.Body)
 	if err != nil {
-		return nil, err
+		return err
 	}
 
-	var res RevokeMachineIdentityAccessTokenResponse
-	err = ReadAndUnmarshal(rawRes, &res)
+	err = json.Unmarshal(bodyBytes, response)
 	if err != nil {
-		return nil, fmt.Errorf(errJSONSecretUnmarshal, err)
+		// Importantly, we do not include the response in the actual error to avoid
+		// leaking anything sensitive.
+		return errJSONUnmarshal
 	}
-	return &res, nil
+
+	return nil
 }
 
 func (a *InfisicalClient) GetSecretsV3(data GetSecretsV3Request) (map[string]string, error) {
-	endpointURL := a.resolveEndpoint("api/v3/secrets/raw")
-
-	req, err := http.NewRequest(http.MethodGet, endpointURL, http.NoBody)
-	metrics.ObserveAPICall(constants.ProviderName, "GetSecretsV3", err)
-	if err != nil {
-		return nil, err
+	params := map[string]string{
+		"workspaceSlug":          data.ProjectSlug,
+		"environment":            data.EnvironmentSlug,
+		"secretPath":             data.SecretPath,
+		"include_imports":        "true",
+		"expandSecretReferences": "true",
+		"recursive":              strconv.FormatBool(data.Recursive),
 	}
 
-	q := req.URL.Query()
-	q.Add("workspaceSlug", data.ProjectSlug)
-	q.Add("environment", data.EnvironmentSlug)
-	q.Add("secretPath", data.SecretPath)
-	q.Add("include_imports", "true")
-	q.Add("expandSecretReferences", "true")
-	q.Add("recursive", strconv.FormatBool(data.Recursive))
-	req.URL.RawQuery = q.Encode()
-
-	rawRes, err := a.do(req)
+	res := GetSecretsV3Response{}
+	err := a.do(
+		"api/v3/secrets/raw",
+		http.MethodGet,
+		params,
+		http.NoBody,
+		&res,
+	)
+	metrics.ObserveAPICall(constants.ProviderName, getSecretsV3, err)
 	if err != nil {
 		return nil, err
 	}
 
-	var res GetSecretsV3Response
-	err = ReadAndUnmarshal(rawRes, &res)
-	if err != nil {
-		return nil, fmt.Errorf(errJSONSecretUnmarshal, err)
-	}
-
 	secrets := make(map[string]string)
 	for _, s := range res.ImportedSecrets {
 		for _, el := range s.Secrets {
@@ -199,42 +266,30 @@ func (a *InfisicalClient) GetSecretsV3(data GetSecretsV3Request) (map[string]str
 }
 
 func (a *InfisicalClient) GetSecretByKeyV3(data GetSecretByKeyV3Request) (string, error) {
-	endpointURL := a.resolveEndpoint(fmt.Sprintf("api/v3/secrets/raw/%s", data.SecretKey))
-
-	req, err := http.NewRequest(http.MethodGet, endpointURL, http.NoBody)
-	metrics.ObserveAPICall(constants.ProviderName, "GetSecretByKeyV3", err)
-	if err != nil {
-		return "", err
+	params := map[string]string{
+		"workspaceSlug":   data.ProjectSlug,
+		"environment":     data.EnvironmentSlug,
+		"secretPath":      data.SecretPath,
+		"include_imports": "true",
 	}
 
-	q := req.URL.Query()
-	q.Add("workspaceSlug", data.ProjectSlug)
-	q.Add("environment", data.EnvironmentSlug)
-	q.Add("secretPath", data.SecretPath)
-	q.Add("include_imports", "true")
-	req.URL.RawQuery = q.Encode()
-
-	rawRes, err := a.do(req)
+	endpointURL := fmt.Sprintf("api/v3/secrets/raw/%s", data.SecretKey)
+
+	res := GetSecretByKeyV3Response{}
+	err := a.do(
+		endpointURL,
+		http.MethodGet,
+		params,
+		http.NoBody,
+		&res,
+	)
+	metrics.ObserveAPICall(constants.ProviderName, getSecretByKeyV3, err)
 	if err != nil {
-		return "", err
-	}
-	if rawRes.StatusCode == 400 {
-		var errRes InfisicalAPIErrorResponse
-		err = ReadAndUnmarshal(rawRes, &errRes)
-		if err != nil {
-			return "", fmt.Errorf(errJSONSecretUnmarshal, err)
-		}
-
-		if errRes.Message == "Secret not found" {
+		var apiErr *InfisicalAPIError
+		if errors.As(err, &apiErr) && apiErr.StatusCode == 404 {
 			return "", esv1beta1.NoSecretError{}
 		}
-		return "", errors.New(errRes.Message)
-	}
-
-	var res GetSecretByKeyV3Response
-	err = ReadAndUnmarshal(rawRes, &res)
-	if err != nil {
-		return "", fmt.Errorf(errJSONSecretUnmarshal, err)
+		return "", err
 	}
 
 	return res.Secret.SecretValue, nil
@@ -247,13 +302,3 @@ func MarshalReqBody(data any) (*bytes.Reader, error) {
 	}
 	return bytes.NewReader(body), nil
 }
-
-func ReadAndUnmarshal(resp *http.Response, target any) error {
-	var buf bytes.Buffer
-	defer resp.Body.Close()
-	_, err := buf.ReadFrom(resp.Body)
-	if err != nil {
-		return err
-	}
-	return json.Unmarshal(buf.Bytes(), target)
-}

pkg/provider/infisical/api/api_fake.go

@@ -0,0 +1,47 @@
+/*
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package api
+
+import (
+	"encoding/json"
+	"net/http"
+	"net/http/httptest"
+)
+
+func newMockServer(status int, data any) *httptest.Server {
+	body, err := json.Marshal(data)
+	if err != nil {
+		panic(err)
+	}
+
+	return httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		w.WriteHeader(status)
+		_, err := w.Write(body)
+		if err != nil {
+			panic(err)
+		}
+	}))
+}
+
+// NewMockClient creates an InfisicalClient with a mocked HTTP client that has a
+// fixed response.
+func NewMockClient(status int, data any) (*InfisicalClient, func()) {
+	server := newMockServer(status, data)
+	client, err := NewAPIClient(server.URL, server.Client())
+	if err != nil {
+		panic(err)
+	}
+	return client, server.Close
+}

pkg/provider/infisical/api/api_models.go

@@ -84,5 +84,7 @@ type ImportedSecretV3 struct {
 type InfisicalAPIErrorResponse struct {
 	StatusCode int    `json:"statusCode"`
 	Message    string `json:"message"`
-	Error      any    `json:"error"`
+	Error      string `json:"error"`
+	// According to Infisical's API docs, `details` are only returned for 403 errors.
+	Details any `json:"details,omitempty"`
 }

pkg/provider/infisical/api/api_test.go

@@ -0,0 +1,317 @@
+/*
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package api
+
+import (
+	"errors"
+	"reflect"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+
+	esv1beta1 "github.com/external-secrets/external-secrets/apis/externalsecrets/v1beta1"
+)
+
+const (
+	fakeClientID        = "client-id"
+	fakeClientSecret    = "client-secret"
+	fakeToken           = "token"
+	fakeProjectSlug     = "first-project"
+	fakeEnvironmentSlug = "dev"
+)
+
+func TestAPIClientDo(t *testing.T) {
+	apiURL := "foo"
+	httpMethod := "bar"
+
+	testCases := []struct {
+		Name             string
+		MockStatusCode   int
+		MockResponse     any
+		ExpectedResponse any
+		ExpectedError    error
+	}{
+		{
+			Name:           "Success",
+			MockStatusCode: 200,
+			MockResponse: MachineIdentityDetailsResponse{
+				AccessToken: "foobar",
+			},
+			ExpectedResponse: MachineIdentityDetailsResponse{
+				AccessToken: "foobar",
+			},
+			ExpectedError: nil,
+		},
+		{
+			Name:           "Error when response cannot be unmarshalled",
+			MockStatusCode: 500,
+			MockResponse:   []byte("not-json"),
+			ExpectedError:  errors.New("API error (500), could not unmarshal error response: json: cannot unmarshal string into Go value of type api.InfisicalAPIErrorResponse"),
+		},
+		{
+			Name:           "Error when non-Infisical error response received",
+			MockStatusCode: 500,
+			MockResponse:   map[string]string{"foo": "bar"},
+			ExpectedError:  errors.New("API error (500): {\"foo\":\"bar\"}"),
+		},
+		{
+			Name:           "Do: Error when non-200 response received",
+			MockStatusCode: 401,
+			MockResponse: InfisicalAPIErrorResponse{
+				StatusCode: 401,
+				Error:      "Unauthorized",
+			},
+			ExpectedError: &InfisicalAPIError{StatusCode: 401, Err: "Unauthorized", Message: ""},
+		},
+		{
+			Name:           "Error when arbitrary details are returned",
+			MockStatusCode: 401,
+			MockResponse: InfisicalAPIErrorResponse{
+				StatusCode: 401,
+				Error:      "Unauthorized",
+				Details:    map[string]string{"foo": "details"},
+			},
+			ExpectedError: &InfisicalAPIError{StatusCode: 401, Err: "Unauthorized", Message: "", Details: map[string]string{"foo": "details"}},
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.Name, func(t *testing.T) {
+			apiClient, closeFunc := NewMockClient(tc.MockStatusCode, tc.MockResponse)
+			defer closeFunc()
+
+			// Automatically pluck out the expected response type using reflection to create a new empty value for unmarshalling.
+			var actualResponse any
+			if tc.ExpectedResponse != nil {
+				actualResponse = reflect.New(reflect.TypeOf(tc.ExpectedResponse)).Interface()
+			}
+
+			err := apiClient.do(apiURL, httpMethod, nil, nil, actualResponse)
+			if tc.ExpectedError != nil {
+				assert.Error(t, err)
+				assert.Equal(t, tc.ExpectedError.Error(), err.Error())
+			} else {
+				assert.NoError(t, err)
+				assert.Equal(t, tc.ExpectedResponse, reflect.ValueOf(actualResponse).Elem().Interface())
+			}
+		})
+	}
+}
+
+// TestAPIClientDoInvalidResponse tests the case where the response is a 200 but does not unmarshal
+// correctly.
+func TestAPIClientDoInvalidResponse(t *testing.T) {
+	apiClient, closeFunc := NewMockClient(200, []byte("not-json"))
+	defer closeFunc()
+
+	err := apiClient.do("foo", "bar", nil, nil, nil)
+	assert.ErrorIs(t, err, errJSONUnmarshal)
+}
+
+func TestSetTokenViaMachineIdentity(t *testing.T) {
+	t.Run("Success", func(t *testing.T) {
+		apiClient, closeFunc := NewMockClient(200, MachineIdentityDetailsResponse{
+			AccessToken: "foobar",
+		})
+		defer closeFunc()
+
+		err := apiClient.SetTokenViaMachineIdentity(fakeClientID, fakeClientSecret)
+		assert.NoError(t, err)
+		assert.Equal(t, apiClient.token, "foobar")
+	})
+
+	t.Run("SetTokenViaMachineIdentity: Error when non-200 response received", func(t *testing.T) {
+		apiClient, closeFunc := NewMockClient(401, InfisicalAPIErrorResponse{
+			StatusCode: 401,
+			Error:      "Unauthorized",
+		})
+		defer closeFunc()
+
+		err := apiClient.SetTokenViaMachineIdentity(fakeClientID, fakeClientSecret)
+		assert.Error(t, err)
+		var apiErr *InfisicalAPIError
+		assert.True(t, errors.As(err, &apiErr))
+		assert.Equal(t, 401, apiErr.StatusCode)
+		assert.Equal(t, "Unauthorized", apiErr.Err)
+	})
+
+	t.Run("Error when token already set", func(t *testing.T) {
+		apiClient, closeFunc := NewMockClient(401, nil)
+		defer closeFunc()
+
+		apiClient.token = fakeToken
+
+		err := apiClient.SetTokenViaMachineIdentity(fakeClientID, fakeClientSecret)
+		assert.ErrorIs(t, err, errAccessTokenAlreadyRetrieved)
+	})
+}
+
+func TestRevokeAccessToken(t *testing.T) {
+	t.Run("Success", func(t *testing.T) {
+		apiClient, closeFunc := NewMockClient(200, RevokeMachineIdentityAccessTokenResponse{
+			Message: "Success",
+		})
+		defer closeFunc()
+
+		apiClient.token = fakeToken
+
+		err := apiClient.RevokeAccessToken()
+		assert.NoError(t, err)
+		// Verify that the access token was unset.
+		assert.Equal(t, apiClient.token, "")
+	})
+
+	t.Run("RevokeAccessToken: Error when non-200 response received", func(t *testing.T) {
+		apiClient, closeFunc := NewMockClient(401, InfisicalAPIErrorResponse{
+			StatusCode: 401,
+			Error:      "Unauthorized",
+		})
+		defer closeFunc()
+
+		apiClient.token = fakeToken
+
+		err := apiClient.RevokeAccessToken()
+		assert.Error(t, err)
+		var apiErr *InfisicalAPIError
+		assert.True(t, errors.As(err, &apiErr))
+		assert.Equal(t, 401, apiErr.StatusCode)
+		assert.Equal(t, "Unauthorized", apiErr.Err)
+	})
+
+	t.Run("Error when no access token is set", func(t *testing.T) {
+		apiClient, closeFunc := NewMockClient(401, nil)
+		defer closeFunc()
+
+		err := apiClient.RevokeAccessToken()
+		assert.ErrorIs(t, err, errNoAccessToken)
+	})
+}
+
+func TestGetSecretsV3(t *testing.T) {
+	t.Run("Works with secrets", func(t *testing.T) {
+		apiClient, closeFunc := NewMockClient(200, GetSecretsV3Response{
+			Secrets: []SecretsV3{
+				{SecretKey: "foo", SecretValue: "bar"},
+			},
+		})
+		defer closeFunc()
+
+		secrets, err := apiClient.GetSecretsV3(GetSecretsV3Request{
+			ProjectSlug:     fakeProjectSlug,
+			EnvironmentSlug: fakeEnvironmentSlug,
+			SecretPath:      "/",
+			Recursive:       true,
+		})
+		assert.NoError(t, err)
+		assert.Equal(t, secrets, map[string]string{"foo": "bar"})
+	})
+
+	t.Run("Works with imported secrets", func(t *testing.T) {
+		apiClient, closeFunc := NewMockClient(200, GetSecretsV3Response{
+			ImportedSecrets: []ImportedSecretV3{{
+				Secrets: []SecretsV3{{SecretKey: "foo", SecretValue: "bar"}},
+			}},
+		})
+		defer closeFunc()
+
+		secrets, err := apiClient.GetSecretsV3(GetSecretsV3Request{
+			ProjectSlug:     fakeProjectSlug,
+			EnvironmentSlug: fakeEnvironmentSlug,
+			SecretPath:      "/",
+			Recursive:       true,
+		})
+		assert.NoError(t, err)
+		assert.Equal(t, secrets, map[string]string{"foo": "bar"})
+	})
+
+	t.Run("GetSecretsV3: Error when non-200 response received", func(t *testing.T) {
+		apiClient, closeFunc := NewMockClient(401, InfisicalAPIErrorResponse{
+			StatusCode: 401,
+			Error:      "Unauthorized",
+		})
+		defer closeFunc()
+
+		_, err := apiClient.GetSecretsV3(GetSecretsV3Request{
+			ProjectSlug:     fakeProjectSlug,
+			EnvironmentSlug: fakeEnvironmentSlug,
+			SecretPath:      "/",
+			Recursive:       true,
+		})
+		assert.Error(t, err)
+		var apiErr *InfisicalAPIError
+		assert.True(t, errors.As(err, &apiErr))
+		assert.Equal(t, 401, apiErr.StatusCode)
+		assert.Equal(t, "Unauthorized", apiErr.Err)
+	})
+}
+func TestGetSecretByKeyV3(t *testing.T) {
+	t.Run("Works", func(t *testing.T) {
+		apiClient, closeFunc := NewMockClient(200, GetSecretByKeyV3Response{
+			Secret: SecretsV3{
+				SecretKey:   "foo",
+				SecretValue: "bar",
+			},
+		})
+		defer closeFunc()
+
+		secret, err := apiClient.GetSecretByKeyV3(GetSecretByKeyV3Request{
+			ProjectSlug:     fakeProjectSlug,
+			EnvironmentSlug: fakeEnvironmentSlug,
+			SecretPath:      "/",
+			SecretKey:       "foo",
+		})
+		assert.NoError(t, err)
+		assert.Equal(t, "bar", secret)
+	})
+
+	t.Run("Error when secret is not found", func(t *testing.T) {
+		apiClient, closeFunc := NewMockClient(404, InfisicalAPIErrorResponse{
+			StatusCode: 404,
+			Error:      "Not Found",
+		})
+		defer closeFunc()
+
+		_, err := apiClient.GetSecretByKeyV3(GetSecretByKeyV3Request{
+			ProjectSlug:     fakeProjectSlug,
+			EnvironmentSlug: fakeEnvironmentSlug,
+			SecretPath:      "/",
+			SecretKey:       "foo",
+		})
+		assert.Error(t, err)
+		// Importantly, we return the standard error for no secrets found.
+		assert.ErrorIs(t, err, esv1beta1.NoSecretError{})
+	})
+
+	// Test case where the request is unauthorized
+	t.Run("ErrorHandlingUnauthorized", func(t *testing.T) {
+		apiClient, closeFunc := NewMockClient(401, InfisicalAPIErrorResponse{
+			StatusCode: 401,
+			Error:      "Unauthorized",
+		})
+		defer closeFunc()
+
+		_, err := apiClient.GetSecretByKeyV3(GetSecretByKeyV3Request{
+			ProjectSlug:     fakeProjectSlug,
+			EnvironmentSlug: fakeEnvironmentSlug,
+			SecretPath:      "/",
+			SecretKey:       "foo",
+		})
+		assert.Error(t, err)
+		var apiErr *InfisicalAPIError
+		assert.True(t, errors.As(err, &apiErr))
+		assert.Equal(t, 401, apiErr.StatusCode)
+		assert.Equal(t, "Unauthorized", apiErr.Err)
+	})
+}

pkg/provider/infisical/fake/fake.go

@@ -1,58 +0,0 @@
-/*
-Licensed under the Apache License, Version 2.0 (the "License");
-you may not use this file except in compliance with the License.
-You may obtain a copy of the License at
-
-	http://www.apache.org/licenses/LICENSE-2.0
-
-Unless required by applicable law or agreed to in writing, software
-distributed under the License is distributed on an "AS IS" BASIS,
-WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or impliec.
-See the License for the specific language governing permissions and
-limitations under the License.
-*/
-package fake
-
-import (
-	"errors"
-	"time"
-
-	"github.com/external-secrets/external-secrets/pkg/provider/infisical/api"
-)
-
-var (
-	ErrMissingMockImplementation = errors.New("missing mock implmentation")
-)
-
-type MockInfisicalClient struct {
-	MockedGetSecretV3      func(data api.GetSecretsV3Request) (map[string]string, error)
-	MockedGetSecretByKeyV3 func(data api.GetSecretByKeyV3Request) (string, error)
-}
-
-func (a *MockInfisicalClient) MachineIdentityLoginViaUniversalAuth(data api.MachineIdentityUniversalAuthLoginRequest) (*api.MachineIdentityDetailsResponse, error) {
-	return &api.MachineIdentityDetailsResponse{
-		AccessToken:       "test-access-token",
-		ExpiresIn:         int(time.Hour * 24),
-		TokenType:         "bearer",
-		AccessTokenMaxTTL: int(time.Hour * 24 * 2),
-	}, nil
-}
-
-func (a *MockInfisicalClient) GetSecretsV3(data api.GetSecretsV3Request) (map[string]string, error) {
-	if a.MockedGetSecretV3 == nil {
-		return nil, ErrMissingMockImplementation
-	}
-
-	return a.MockedGetSecretV3(data)
-}
-
-func (a *MockInfisicalClient) GetSecretByKeyV3(data api.GetSecretByKeyV3Request) (string, error) {
-	if a.MockedGetSecretByKeyV3 == nil {
-		return "", ErrMissingMockImplementation
-	}
-	return a.MockedGetSecretByKeyV3(data)
-}
-
-func (a *MockInfisicalClient) RevokeAccessToken() error {
-	return nil
-}

pkg/provider/infisical/provider.go

@@ -18,6 +18,8 @@ import (
 	"context"
 	"errors"
 	"fmt"
+	"net/http"
+	"time"
 
 	ctrl "sigs.k8s.io/controller-runtime"
 	kclient "sigs.k8s.io/controller-runtime/pkg/client"
@@ -36,7 +38,7 @@ var (
 )
 
 type Provider struct {
-	apiClient api.InfisicalApis
+	apiClient *api.InfisicalClient
 	apiScope  *InfisicalClientScope
 }
 
@@ -70,7 +72,9 @@ func (p *Provider) NewClient(ctx context.Context, store esv1beta1.GenericStore,
 
 	infisicalSpec := storeSpec.Provider.Infisical
 
-	apiClient, err := api.NewAPIClient(infisicalSpec.HostAPI)
+	apiClient, err := api.NewAPIClient(infisicalSpec.HostAPI, &http.Client{
+		Timeout: time.Second * 15,
+	})
 	if err != nil {
 		return nil, err
 	}

pkg/provider/infisical/provider_test.go

@@ -25,7 +25,6 @@ import (
 	esv1beta1 "github.com/external-secrets/external-secrets/apis/externalsecrets/v1beta1"
 	esv1meta "github.com/external-secrets/external-secrets/apis/meta/v1"
 	"github.com/external-secrets/external-secrets/pkg/provider/infisical/api"
-	"github.com/external-secrets/external-secrets/pkg/provider/infisical/fake"
 )
 
 type storeModifier func(*esv1beta1.SecretStore) *esv1beta1.SecretStore
@@ -38,61 +37,65 @@ var apiScope = InfisicalClientScope{
 
 type TestCases struct {
 	Name           string
-	MockClient     *fake.MockInfisicalClient
-	PropertyAccess string
+	MockStatusCode int
+	MockResponse   any
+	Property       string
 	Error          error
 	Output         any
 }
 
 func TestGetSecret(t *testing.T) {
+	key := "foo"
+
 	testCases := []TestCases{
 		{
-			Name: "Get_valid_key",
-			MockClient: &fake.MockInfisicalClient{
-				MockedGetSecretByKeyV3: func(data api.GetSecretByKeyV3Request) (string, error) {
-					return "value", nil
+			Name:           "Get_valid_key",
+			MockStatusCode: 200,
+			MockResponse: api.GetSecretByKeyV3Response{
+				Secret: api.SecretsV3{
+					SecretKey:   key,
+					SecretValue: "bar",
 				},
 			},
-			Error:  nil,
-			Output: []byte("value"),
+			Output: []byte("bar"),
 		},
 		{
-			Name: "Get_property_key",
-			MockClient: &fake.MockInfisicalClient{
-				MockedGetSecretByKeyV3: func(data api.GetSecretByKeyV3Request) (string, error) {
-					return `{"key":"value"}`, nil
+			Name:           "Get_property_key",
+			MockStatusCode: 200,
+			MockResponse: api.GetSecretByKeyV3Response{
+				Secret: api.SecretsV3{
+					SecretKey:   key,
+					SecretValue: `{"bar": "value"}`,
 				},
 			},
-			Error:  nil,
-			Output: []byte("value"),
+			Property: "bar",
+			Output:   []byte("value"),
 		},
 		{
-			Name: "Key_not_found",
-			MockClient: &fake.MockInfisicalClient{
-				MockedGetSecretByKeyV3: func(data api.GetSecretByKeyV3Request) (string, error) {
-					// from server
-					return "", errors.New("Secret not found")
-				},
+			Name:           "Key_not_found",
+			MockStatusCode: 404,
+			MockResponse: api.InfisicalAPIError{
+				StatusCode: 404,
+				Err:        "Not Found",
+				Message:    "Secret not found",
 			},
-			Error:  errors.New("Secret not found"),
+			Error:  esv1beta1.NoSecretError{},
 			Output: "",
 		},
 	}
 
 	for _, tc := range testCases {
 		t.Run(tc.Name, func(t *testing.T) {
+			apiClient, closeFunc := api.NewMockClient(tc.MockStatusCode, tc.MockResponse)
+			defer closeFunc()
 			p := &Provider{
-				apiClient: tc.MockClient,
+				apiClient: apiClient,
 				apiScope:  &apiScope,
 			}
-			var property string
-			if tc.Name == "Get_property_key" {
-				property = "key"
-			}
 
 			output, err := p.GetSecret(context.Background(), esv1beta1.ExternalSecretDataRemoteRef{
-				Key:      "key",
-				Property: property,
+				Key:      key,
+				Property: tc.Property,
 			})
 
 			if tc.Error == nil {
@@ -106,39 +109,39 @@ func TestGetSecret(t *testing.T) {
 }
 
 func TestGetSecretMap(t *testing.T) {
+	key := "foo"
 	testCases := []TestCases{
 		{
-			Name: "Get_valid_key_map",
-			MockClient: &fake.MockInfisicalClient{
-				MockedGetSecretByKeyV3: func(data api.GetSecretByKeyV3Request) (string, error) {
-					return `{"key":"value"}`, nil
+			Name:           "Get_valid_key_map",
+			MockStatusCode: 200,
+			MockResponse: api.GetSecretByKeyV3Response{
+				Secret: api.SecretsV3{
+					SecretKey:   key,
+					SecretValue: `{"bar": "value"}`,
 				},
 			},
-			Error: nil,
 			Output: map[string][]byte{
-				"key": []byte("value"),
+				"bar": []byte("value"),
 			},
 		},
 		{
-			Name: "Get_invalid_map",
-			MockClient: &fake.MockInfisicalClient{
-				MockedGetSecretByKeyV3: func(data api.GetSecretByKeyV3Request) (string, error) {
-					return ``, nil
-				},
-			},
-			Error:  errors.New("unexpected end of JSON input"),
-			Output: nil,
+			Name:           "Get_invalid_map",
+			MockStatusCode: 200,
+			MockResponse:   []byte(``),
+			Error:          errors.New("unable to unmarshal secret foo"),
 		},
 	}
 
 	for _, tc := range testCases {
 		t.Run(tc.Name, func(t *testing.T) {
+			apiClient, closeFunc := api.NewMockClient(tc.MockStatusCode, tc.MockResponse)
+			defer closeFunc()
 			p := &Provider{
-				apiClient: tc.MockClient,
+				apiClient: apiClient,
 				apiScope:  &apiScope,
 			}
 			output, err := p.GetSecretMap(context.Background(), esv1beta1.ExternalSecretDataRemoteRef{
-				Key: "key",
+				Key: key,
 			})
 			if tc.Error == nil {
 				assert.NoError(t, err)