From d1a15b2a65edfcf5949675c81de7188a299ba183 Mon Sep 17 00:00:00 2001 From: gyanranjanpanda Date: Sun, 17 May 2026 22:33:09 +0530 Subject: [PATCH] fix(connectors): replace panics with error returns and add HTTP status code validation Replace panic() calls with proper error returns and add HTTP response status code validation in all API client methods that previously crashed on non-2xx responses. Changes: - GetKeycloakURL: add status check, replace panics, guard type assertions - CreateTestResult: add status check, replace panics, guard id assertion - GetTestResult: add status check, replace panic (prevents silent false failures) - DownloadArtifact: replace remaining ReadAll panic - ConnectAndGetToken: add status check, replace panics, guard assertion - GetOIDCConfig: add status check, replace panics, guard assertions, fix swallowed error - ConnectAndGetTokenAndRefreshToken: add status check, replace panics, guard assertions, fix swallowed error Fixes #401 Signed-off-by: gyanranjanpanda --- pkg/connectors/keycloak_client.go | 58 ++++-- pkg/connectors/keycloak_client_test.go | 127 +++++++++++++ pkg/connectors/microcks_client.go | 64 +++++-- pkg/connectors/microcks_client_error_test.go | 180 +++++++++++++++++++ 4 files changed, 394 insertions(+), 35 deletions(-) create mode 100644 pkg/connectors/keycloak_client_test.go create mode 100644 pkg/connectors/microcks_client_error_test.go diff --git a/pkg/connectors/keycloak_client.go b/pkg/connectors/keycloak_client.go index 5d6aeed..81712b4 100644 --- a/pkg/connectors/keycloak_client.go +++ b/pkg/connectors/keycloak_client.go @@ -97,26 +97,32 @@ func (c *keycloakClient) ConnectAndGetToken() (string, error) { body, err := io.ReadAll(resp.Body) if err != nil { - panic(err.Error()) + return "", fmt.Errorf("failed to read token response: %w", err) + } + + if resp.StatusCode/100 != 2 { + return "", fmt.Errorf("token request returned HTTP %d: %s", resp.StatusCode, string(body)) } var openIDResp map[string]interface{} if err := json.Unmarshal(body, &openIDResp); err != nil { - panic(err) + return "", fmt.Errorf("failed to parse token response: %w", err) } - accessToken := openIDResp["access_token"].(string) - return accessToken, err + accessToken, ok := openIDResp["access_token"].(string) + if !ok { + return "", fmt.Errorf("token response missing required field \"access_token\"") + } + return accessToken, nil } func (c *keycloakClient) GetOIDCConfig() (*oauth2.Config, error) { rel := &url.URL{Path: ".well-known/openid-configuration"} u := c.BaseURL.ResolveReference(rel) - // Create HTTP request req, err := http.NewRequest("GET", u.String(), nil) if err != nil { - fmt.Println("Error creating request:", err) + return nil, fmt.Errorf("failed to create OIDC config request: %w", err) } resp, err := c.httpClient.Do(req) @@ -127,16 +133,26 @@ func (c *keycloakClient) GetOIDCConfig() (*oauth2.Config, error) { body, err := io.ReadAll(resp.Body) if err != nil { - panic(err.Error()) + return nil, fmt.Errorf("failed to read OIDC config response: %w", err) + } + + if resp.StatusCode/100 != 2 { + return nil, fmt.Errorf("OIDC config request returned HTTP %d: %s", resp.StatusCode, string(body)) } var openIDResp map[string]interface{} if err := json.Unmarshal(body, &openIDResp); err != nil { - panic(err) + return nil, fmt.Errorf("failed to parse OIDC config response: %w", err) } - authURL := openIDResp["authorization_endpoint"].(string) - tokenURL := openIDResp["token_endpoint"].(string) + authURL, ok := openIDResp["authorization_endpoint"].(string) + if !ok { + return nil, fmt.Errorf("OIDC config response missing required field \"authorization_endpoint\"") + } + tokenURL, ok := openIDResp["token_endpoint"].(string) + if !ok { + return nil, fmt.Errorf("OIDC config response missing required field \"token_endpoint\"") + } return &oauth2.Config{ Endpoint: oauth2.Endpoint{ @@ -157,13 +173,11 @@ func (c *keycloakClient) ConnectAndGetTokenAndRefreshToken(username, password st data.Set("username", username) data.Set("password", password) data.Set("grant_type", "password") - // Create HTTP request req, err := http.NewRequest("POST", u.String(), bytes.NewBufferString(data.Encode())) if err != nil { - fmt.Println("Error creating request:", err) + return "", "", fmt.Errorf("failed to create password token request: %w", err) } - // Set headers req.Header.Set("Content-Type", "application/x-www-form-urlencoded") resp, err := c.httpClient.Do(req) @@ -174,16 +188,26 @@ func (c *keycloakClient) ConnectAndGetTokenAndRefreshToken(username, password st body, err := io.ReadAll(resp.Body) if err != nil { - panic(err.Error()) + return "", "", fmt.Errorf("failed to read password token response: %w", err) + } + + if resp.StatusCode/100 != 2 { + return "", "", fmt.Errorf("password token request returned HTTP %d: %s", resp.StatusCode, string(body)) } var openIDResp map[string]interface{} if err := json.Unmarshal(body, &openIDResp); err != nil { - panic(err) + return "", "", fmt.Errorf("failed to parse password token response: %w", err) } - authToken := openIDResp["access_token"].(string) - refreshToken := openIDResp["refresh_token"].(string) + authToken, ok := openIDResp["access_token"].(string) + if !ok { + return "", "", fmt.Errorf("password token response missing required field \"access_token\"") + } + refreshToken, ok := openIDResp["refresh_token"].(string) + if !ok { + return "", "", fmt.Errorf("password token response missing required field \"refresh_token\"") + } return authToken, refreshToken, nil } diff --git a/pkg/connectors/keycloak_client_test.go b/pkg/connectors/keycloak_client_test.go new file mode 100644 index 0000000..d0a8391 --- /dev/null +++ b/pkg/connectors/keycloak_client_test.go @@ -0,0 +1,127 @@ +package connectors + +import ( + "net/http" + "net/http/httptest" + "strings" + "testing" +) + +func TestConnectAndGetTokenReturnsErrorOnNon2xx(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusUnauthorized) + w.Write([]byte(`{"error":"unauthorized_client"}`)) + })) + defer server.Close() + + kc := NewKeycloakClient(server.URL+"/", "bad-id", "bad-secret") + _, err := kc.ConnectAndGetToken() + if err == nil { + t.Fatal("expected error for 401 response, got nil") + } + if !strings.Contains(err.Error(), "HTTP 401") { + t.Fatalf("expected error to mention HTTP 401, got: %s", err.Error()) + } +} + +func TestConnectAndGetTokenReturnsErrorOnMalformedJSON(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusOK) + w.Write([]byte("not json")) + })) + defer server.Close() + + kc := NewKeycloakClient(server.URL+"/", "id", "secret") + _, err := kc.ConnectAndGetToken() + if err == nil { + t.Fatal("expected error for malformed JSON, got nil") + } + if !strings.Contains(err.Error(), "failed to parse") { + t.Fatalf("expected parse error, got: %s", err.Error()) + } +} + +func TestConnectAndGetTokenReturnsErrorOnMissingAccessToken(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusOK) + w.Write([]byte(`{"token_type":"bearer"}`)) + })) + defer server.Close() + + kc := NewKeycloakClient(server.URL+"/", "id", "secret") + _, err := kc.ConnectAndGetToken() + if err == nil { + t.Fatal("expected error for missing access_token, got nil") + } + if !strings.Contains(err.Error(), "missing required field") { + t.Fatalf("expected missing field error, got: %s", err.Error()) + } +} + +func TestGetOIDCConfigReturnsErrorOnNon2xx(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusNotFound) + w.Write([]byte("not found")) + })) + defer server.Close() + + kc := NewKeycloakClient(server.URL+"/", "", "") + _, err := kc.GetOIDCConfig() + if err == nil { + t.Fatal("expected error for 404 response, got nil") + } + if !strings.Contains(err.Error(), "HTTP 404") { + t.Fatalf("expected error to mention HTTP 404, got: %s", err.Error()) + } +} + +func TestGetOIDCConfigReturnsErrorOnMissingFields(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusOK) + w.Write([]byte(`{"issuer":"https://auth.example.com"}`)) + })) + defer server.Close() + + kc := NewKeycloakClient(server.URL+"/", "", "") + _, err := kc.GetOIDCConfig() + if err == nil { + t.Fatal("expected error for missing OIDC fields, got nil") + } + if !strings.Contains(err.Error(), "missing required field") { + t.Fatalf("expected missing field error, got: %s", err.Error()) + } +} + +func TestConnectAndGetTokenAndRefreshTokenReturnsErrorOnNon2xx(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusBadRequest) + w.Write([]byte(`{"error":"invalid_grant"}`)) + })) + defer server.Close() + + kc := NewKeycloakClient(server.URL+"/", "client-id", "client-secret") + _, _, err := kc.ConnectAndGetTokenAndRefreshToken("user", "wrong-pass") + if err == nil { + t.Fatal("expected error for 400 response, got nil") + } + if !strings.Contains(err.Error(), "HTTP 400") { + t.Fatalf("expected error to mention HTTP 400, got: %s", err.Error()) + } +} + +func TestConnectAndGetTokenAndRefreshTokenReturnsErrorOnMissingFields(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusOK) + w.Write([]byte(`{"access_token":"tok123"}`)) + })) + defer server.Close() + + kc := NewKeycloakClient(server.URL+"/", "client-id", "client-secret") + _, _, err := kc.ConnectAndGetTokenAndRefreshToken("user", "pass") + if err == nil { + t.Fatal("expected error for missing refresh_token field, got nil") + } + if !strings.Contains(err.Error(), "refresh_token") { + t.Fatalf("expected error to mention refresh_token, got: %s", err.Error()) + } +} diff --git a/pkg/connectors/microcks_client.go b/pkg/connectors/microcks_client.go index 8ecd420..b3a3eac 100644 --- a/pkg/connectors/microcks_client.go +++ b/pkg/connectors/microcks_client.go @@ -35,7 +35,6 @@ import ( "github.com/coreos/go-oidc/v3/oidc" "github.com/golang-jwt/jwt/v4" "github.com/microcks/microcks-cli/pkg/config" - "github.com/microcks/microcks-cli/pkg/errors" "golang.org/x/oauth2" ) @@ -239,24 +238,37 @@ func (c *microcksClient) GetKeycloakURL() (string, error) { body, err := io.ReadAll(resp.Body) if err != nil { - panic(err.Error()) + return "", fmt.Errorf("failed to read Keycloak config response: %w", err) + } + + if resp.StatusCode/100 != 2 { + return "", fmt.Errorf("Keycloak config request returned HTTP %d: %s", resp.StatusCode, string(body)) } var configResp map[string]interface{} if err := json.Unmarshal(body, &configResp); err != nil { - panic(err) + return "", fmt.Errorf("failed to parse Keycloak config response: %w", err) } // Retrieve auth server url and realm name. - enabled := configResp["enabled"].(bool) - authServerURL := configResp["auth-server-url"].(string) - realmName := configResp["realm"].(string) + enabled, ok := configResp["enabled"].(bool) + if !ok { + return "", fmt.Errorf("Keycloak config response missing required field \"enabled\"") + } + if !enabled { + return "null", nil + } - // Return a proper URL or 'null' if Keycloak is disables. - if enabled { - return authServerURL + "/realms/" + realmName + "/", nil + authServerURL, ok := configResp["auth-server-url"].(string) + if !ok { + return "", fmt.Errorf("Keycloak config response missing required field \"auth-server-url\"") } - return "null", nil + realmName, ok := configResp["realm"].(string) + if !ok { + return "", fmt.Errorf("Keycloak config response missing required field \"realm\"") + } + + return authServerURL + "/realms/" + realmName + "/", nil } func (c *microcksClient) refreshAuthToken(localCfg *config.LocalConfig, ctxName, configPath string) error { @@ -304,10 +316,15 @@ func (c *microcksClient) refreshAuthToken(localCfg *config.LocalConfig, ctxName, func (c *microcksClient) redeemRefreshToken(auth config.Auth) (string, string, error) { keyCloakUrl, err := c.GetKeycloakURL() - errors.CheckError(err) + if err != nil { + return "", "", err + } + kc := NewKeycloakClient(keyCloakUrl, "", "") oauth2Conf, err := kc.GetOIDCConfig() - errors.CheckError(err) + if err != nil { + return "", "", err + } oauth2Conf.ClientID = auth.ClientId oauth2Conf.ClientSecret = auth.ClientSecret @@ -381,16 +398,23 @@ func (c *microcksClient) CreateTestResult(serviceID string, testEndpoint string, body, err := io.ReadAll(resp.Body) if err != nil { - panic(err.Error()) + return "", fmt.Errorf("failed to read create test response: %w", err) + } + + if resp.StatusCode/100 != 2 { + return "", fmt.Errorf("create test request returned HTTP %d: %s", resp.StatusCode, string(body)) } var createTestResp map[string]interface{} if err := json.Unmarshal(body, &createTestResp); err != nil { - panic(err) + return "", fmt.Errorf("failed to parse create test response: %w", err) } - testID := createTestResp["id"].(string) - return testID, err + testID, ok := createTestResp["id"].(string) + if !ok { + return "", fmt.Errorf("create test response missing required field \"id\"") + } + return testID, nil } func (c *microcksClient) GetTestResult(testResultID string) (*TestResultSummary, error) { @@ -420,7 +444,11 @@ func (c *microcksClient) GetTestResult(testResultID string) (*TestResultSummary, body, err := io.ReadAll(resp.Body) if err != nil { - panic(err.Error()) + return nil, fmt.Errorf("failed to read test result response: %w", err) + } + + if resp.StatusCode/100 != 2 { + return nil, fmt.Errorf("get test result request returned HTTP %d: %s", resp.StatusCode, string(body)) } result := TestResultSummary{} @@ -553,7 +581,7 @@ func (c *microcksClient) DownloadArtifact(artifactURL string, mainArtifact bool, respBody, err := io.ReadAll(resp.Body) if err != nil { - panic(err.Error()) + return "", fmt.Errorf("failed to read download artifact response: %w", err) } // Raise exception if not created. diff --git a/pkg/connectors/microcks_client_error_test.go b/pkg/connectors/microcks_client_error_test.go new file mode 100644 index 0000000..2cba706 --- /dev/null +++ b/pkg/connectors/microcks_client_error_test.go @@ -0,0 +1,180 @@ +package connectors + +import ( + "encoding/json" + "net/http" + "net/http/httptest" + "strings" + "testing" + + "github.com/microcks/microcks-cli/pkg/config" +) + +func TestGetKeycloakURLReturnsErrorOnNon2xx(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusInternalServerError) + w.Write([]byte("internal error")) + })) + defer server.Close() + + client := NewMicrocksClient(server.URL) + _, err := client.GetKeycloakURL() + if err == nil { + t.Fatal("expected error for non-2xx response, got nil") + } + if !strings.Contains(err.Error(), "HTTP 500") { + t.Fatalf("expected error to mention HTTP 500, got: %s", err.Error()) + } +} + +func TestGetKeycloakURLReturnsErrorOnMalformedJSON(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusOK) + w.Write([]byte("not json")) + })) + defer server.Close() + + client := NewMicrocksClient(server.URL) + _, err := client.GetKeycloakURL() + if err == nil { + t.Fatal("expected error for malformed JSON, got nil") + } + if !strings.Contains(err.Error(), "failed to parse") { + t.Fatalf("expected parse error, got: %s", err.Error()) + } +} + +func TestGetKeycloakURLReturnsErrorOnMissingFields(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusOK) + w.Write([]byte(`{"enabled": true}`)) + })) + defer server.Close() + + client := NewMicrocksClient(server.URL) + _, err := client.GetKeycloakURL() + if err == nil { + t.Fatal("expected error for missing fields, got nil") + } + if !strings.Contains(err.Error(), "missing required field") { + t.Fatalf("expected missing field error, got: %s", err.Error()) + } +} + +func TestGetKeycloakURLReturnsNullWhenKeycloakDisabled(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusOK) + w.Write([]byte(`{"enabled": false}`)) + })) + defer server.Close() + + client := NewMicrocksClient(server.URL) + keycloakURL, err := client.GetKeycloakURL() + if err != nil { + t.Fatalf("expected no error for disabled Keycloak config, got: %s", err.Error()) + } + if keycloakURL != "null" { + t.Fatalf("expected disabled Keycloak URL to be null, got: %s", keycloakURL) + } +} + +func TestRedeemRefreshTokenReturnsKeycloakConfigError(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusInternalServerError) + w.Write([]byte("config unavailable")) + })) + defer server.Close() + + client := NewMicrocksClient(server.URL).(*microcksClient) + _, _, err := client.redeemRefreshToken(config.Auth{}) + if err == nil { + t.Fatal("expected error for Keycloak config failure, got nil") + } + if !strings.Contains(err.Error(), "HTTP 500") { + t.Fatalf("expected error to mention HTTP 500, got: %s", err.Error()) + } +} + +func TestCreateTestResultReturnsErrorOnNon2xx(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path == "/api/keycloak/config" { + json.NewEncoder(w).Encode(map[string]interface{}{ + "enabled": false, + "auth-server-url": "", + "realm": "", + }) + return + } + w.WriteHeader(http.StatusUnauthorized) + w.Write([]byte("unauthorized")) + })) + defer server.Close() + + client := NewMicrocksClient(server.URL) + client.SetOAuthToken("expired-token") + + _, err := client.CreateTestResult("svc:1.0", "http://endpoint", "HTTP", "", 5000, "", "", "") + if err == nil { + t.Fatal("expected error for non-2xx response, got nil") + } + if !strings.Contains(err.Error(), "HTTP 401") { + t.Fatalf("expected error to mention HTTP 401, got: %s", err.Error()) + } +} + +func TestCreateTestResultReturnsErrorOnMissingID(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusOK) + w.Write([]byte(`{"status": "created"}`)) + })) + defer server.Close() + + client := NewMicrocksClient(server.URL) + client.SetOAuthToken("test-token") + + _, err := client.CreateTestResult("svc:1.0", "http://endpoint", "HTTP", "", 5000, "", "", "") + if err == nil { + t.Fatal("expected error for missing id field, got nil") + } + if !strings.Contains(err.Error(), "missing required field") { + t.Fatalf("expected missing field error, got: %s", err.Error()) + } +} + +func TestGetTestResultReturnsErrorOnNon2xx(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusForbidden) + w.Write([]byte("forbidden")) + })) + defer server.Close() + + client := NewMicrocksClient(server.URL) + client.SetOAuthToken("test-token") + + _, err := client.GetTestResult("abc-123") + if err == nil { + t.Fatal("expected error for non-2xx response, got nil") + } + if !strings.Contains(err.Error(), "HTTP 403") { + t.Fatalf("expected error to mention HTTP 403, got: %s", err.Error()) + } +} + +func TestDownloadArtifactReturnsErrorOnNon2xx(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusNotFound) + w.Write([]byte("not found")) + })) + defer server.Close() + + client := NewMicrocksClient(server.URL) + client.SetOAuthToken("test-token") + + _, err := client.DownloadArtifact("https://example.com/spec.yaml", true, "") + if err == nil { + t.Fatal("expected error for non-2xx response, got nil") + } + if !strings.Contains(err.Error(), "not found") { + t.Fatalf("expected error body in message, got: %s", err.Error()) + } +}