Skip to content

fix: enhance token refresh handling and improve error reporting - #138

Open
alexlovelltroy wants to merge 1 commit into
mainfrom
fix/smd-token-refresh-hardening
Open

alexlovelltroy wants to merge 1 commit into
mainfrom
fix/smd-token-refresh-hardening

Conversation

@alexlovelltroy

Copy link
Copy Markdown
Member

Description

This pull request improves the reliability and test coverage of the SMD client’s access token refresh logic. The most important changes include stricter error handling and validation in token refresh, dedicated HTTP client usage for token requests, and new tests to ensure correct behavior when token refresh fails.

Error Handling and Validation Improvements:

  • The refreshTokenWithContext method now returns detailed errors for various failure modes, such as missing HTTP client, non-2xx responses, malformed or empty tokens, and JSON decoding errors. It also trims whitespace from the received token and rejects empty tokens.
  • In getSMD, if the token refresh fails after a rejected access token, the error is now returned immediately instead of being silently ignored, providing better visibility and preventing retries with invalid tokens.

HTTP Client Usage and Struct Changes:

  • Added a dedicated tokenClient field to SMDClient, ensuring token refresh requests use a separate HTTP client from regular SMD requests. Constructors and tests are updated to initialize this field appropriately. [1] [2] [3] [4]

Testing Improvements:

  • Added tests to verify that failed token refresh attempts (due to non-2xx responses, empty access tokens, or malformed JSON) do not overwrite the previous access token.
  • Added a test to ensure that getSMD returns a refresh failure error without retrying, and that the access token remains unchanged.

Code Consistency:

  • Standardized struct field names in oidcTokenData to use Go naming conventions (e.g., AccessToken instead of Access_token).

Checklist

  • My code follows the style guidelines of this project
  • I have added/updated comments where needed
  • I have added tests that prove my fix is effective or my feature works
  • I have run make test (or equivalent) locally and all tests pass
  • I have updated the relevant documentation (CLI examples, man pages, README, other docs, etc.)
  • DCO Sign-off: All commits are signed off (git commit -s) with my real name and email
  • REUSE Compliance:
    • Each new/modified source file has SPDX copyright and license headers
    • Any non-commentable files include a <filename>.license sidecar
    • All referenced licenses are present in the LICENSES/ directory

Type of Change

  • Bug fix
  • New feature
  • Breaking change
  • Documentation update
  • Dependency update
  • Build system/CI

Signed-off-by: Alex Lovell-Troy <alovelltroy@lanl.gov>

@synackd synackd left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, just one concern (with suggested patch) about separate HTTP client for token refresh.

client := &SMDClient{
clusterName: clusterName,
smdClient: c,
tokenClient: &http.Client{Timeout: 10 * time.Second},

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably reuse the SMDClient transport here so TLS can be used for token handling. We can also reuse the timeout value too.

diff --git a/internal/smdclient/SMDclient.go b/internal/smdclient/SMDclient.go
index 82ba48a..64319aa 100644
--- a/internal/smdclient/SMDclient.go
+++ b/internal/smdclient/SMDclient.go
@@ -94,7 +94,7 @@ func NewSMDClient(clusterName, baseurl, jwtURL, accessToken, certPath string, in
 		if err != nil {
 			return nil, fmt.Errorf("failed to read cert from path %s: %v", certPath, err)
 		}
-		certPool := x509.NewCertPool()
+		certPool = x509.NewCertPool()
 		certPool.AppendCertsFromPEM(cacert)
 	}
 
@@ -114,9 +114,12 @@ func NewSMDClient(clusterName, baseurl, jwtURL, accessToken, certPath string, in
 	}
 
 	client := &SMDClient{
-		clusterName:       clusterName,
-		smdClient:         c,
-		tokenClient:       &http.Client{Timeout: 10 * time.Second},
+		clusterName: clusterName,
+		smdClient:   c,
+		tokenClient: &http.Client{
+			Transport: c.Transport,
+			Timeout:   c.Timeout,
+		},
 		smdBaseURL:        baseurl,
 		tokenEndpoint:     jwtURL,
 		accessToken:       accessToken,

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants