Skip to content

ROX-33256: add console user to requester claim#19683

Draft
stehessel wants to merge 1 commit intomasterfrom
ROX-33256/console-user-in-requester
Draft

ROX-33256: add console user to requester claim#19683
stehessel wants to merge 1 commit intomasterfrom
ROX-33256/console-user-in-requester

Conversation

@stehessel
Copy link
Copy Markdown
Collaborator

Description

change me!

User-facing documentation

Testing and quality

  • the change is production ready: the change is GA, or otherwise the functionality is gated by a feature flag
  • CI results are inspected

Automated testing

  • added unit tests
  • added e2e tests
  • added regression tests
  • added compatibility tests
  • modified existing tests

How I validated my change

change me!

@openshift-ci
Copy link
Copy Markdown

openshift-ci bot commented Mar 30, 2026

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

Copy link
Copy Markdown
Contributor

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

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

Hey - I've found 2 issues, and left some high level feedback:

  • The tokenCacheKey.cacheKeyString implementation (scope + "|" + username) can collide if either field contains "|"; consider using a delimiter that cannot appear in either value (e.g., "\x00") or a more robust encoding (e.g., JSON or fmt.Sprintf("%q|%q", ...)) to ensure unique coalescer keys.
Prompt for AI Agents
Please address the comments from this code review:

## Overall Comments
- The `tokenCacheKey.cacheKeyString` implementation (`scope + "|" + username`) can collide if either field contains `"|"`; consider using a delimiter that cannot appear in either value (e.g., `"\x00"`) or a more robust encoding (e.g., JSON or `fmt.Sprintf("%q|%q", ...)`) to ensure unique coalescer keys.

## Individual Comments

### Comment 1
<location path="sensor/common/centralproxy/transport.go" line_range="186-187" />
<code_context>
+}
+
+// cacheKeyString returns a string representation for coalescer keys.
+func (k tokenCacheKey) cacheKeyString() string {
+	return k.scope + "|" + k.username
+}
+
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Consider making the coalescer key construction robust against delimiter collisions or future changes in username format.

The current `scope + "|" + username` key relies on `"|"` never appearing in `username`, but this isn’t enforced. If that assumption changes, distinct `(scope, username)` pairs could collide in the coalescer. To avoid this, either pick a delimiter that’s guaranteed invalid for both fields, encode each component before joining (e.g., URL-escape/base64), or use a structured key type in the coalescer instead of flattening to a string.

Suggested implementation:

```golang
	// cacheKeyString returns a string representation for coalescer keys.
	// We URL-escape each component to ensure the "|" delimiter cannot be
	// produced by the encoded values, avoiding collisions even if scope
	// or username formats change in the future.
func (k tokenCacheKey) cacheKeyString() string {
	return url.QueryEscape(k.scope) + "|" + url.QueryEscape(k.username)
}

	// Buffer the request body upfront so we can replay it on retry.
	var bodyBytes []byte
		req.Body = io.NopCloser(bytes.NewBuffer(bodyBytes))

```

To compile successfully, ensure that `net/url` is imported at the top of `sensor/common/centralproxy/transport.go`:

1. Add `net/url` to the existing import block, for example:
   - From:
     `import ( "bytes" "io" ... )`
   - To:
     `import ( "bytes" "io" "net/url" ... )`

No other changes should be required, assuming `cacheKeyString()` is only used as a string key and not relied upon to be human-readable.
</issue_to_address>

### Comment 2
<location path="sensor/common/centralproxy/transport.go" line_range="250" />
<code_context>
 		}

-		log.Debugf("Token cache miss for namespace scope %q, requesting from Central", namespaceScope)
+		log.Debugf("Token cache miss for namespace scope %q (user %q), requesting from Central", key.scope, key.username)

 		// Use a background context with timeout to ensure the shared function is independent
</code_context>
<issue_to_address>
**🚨 suggestion (security):** Revisit logging of the raw username in debug logs to avoid leaking potentially sensitive identifiers.

This now logs the console username for every cache miss, which may end up in long‑retention aggregated logs. To reduce privacy/compliance risk, consider hashing or redacting `username`, or omitting it and logging only the scope or a non-identifying token for correlation.

```suggestion
		log.Debugf("Token cache miss for namespace scope %q, requesting from Central", key.scope)
```
</issue_to_address>

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

Comment on lines +186 to +187
func (k tokenCacheKey) cacheKeyString() string {
return k.scope + "|" + k.username
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.

suggestion (bug_risk): Consider making the coalescer key construction robust against delimiter collisions or future changes in username format.

The current scope + "|" + username key relies on "|" never appearing in username, but this isn’t enforced. If that assumption changes, distinct (scope, username) pairs could collide in the coalescer. To avoid this, either pick a delimiter that’s guaranteed invalid for both fields, encode each component before joining (e.g., URL-escape/base64), or use a structured key type in the coalescer instead of flattening to a string.

Suggested implementation:

	// cacheKeyString returns a string representation for coalescer keys.
	// We URL-escape each component to ensure the "|" delimiter cannot be
	// produced by the encoded values, avoiding collisions even if scope
	// or username formats change in the future.
func (k tokenCacheKey) cacheKeyString() string {
	return url.QueryEscape(k.scope) + "|" + url.QueryEscape(k.username)
}

	// Buffer the request body upfront so we can replay it on retry.
	var bodyBytes []byte
		req.Body = io.NopCloser(bytes.NewBuffer(bodyBytes))

To compile successfully, ensure that net/url is imported at the top of sensor/common/centralproxy/transport.go:

  1. Add net/url to the existing import block, for example:
    • From:
      import ( "bytes" "io" ... )
    • To:
      import ( "bytes" "io" "net/url" ... )

No other changes should be required, assuming cacheKeyString() is only used as a string key and not relied upon to be human-readable.

}

log.Debugf("Token cache miss for namespace scope %q, requesting from Central", namespaceScope)
log.Debugf("Token cache miss for namespace scope %q (user %q), requesting from Central", key.scope, key.username)
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.

🚨 suggestion (security): Revisit logging of the raw username in debug logs to avoid leaking potentially sensitive identifiers.

This now logs the console username for every cache miss, which may end up in long‑retention aggregated logs. To reduce privacy/compliance risk, consider hashing or redacting username, or omitting it and logging only the scope or a non-identifying token for correlation.

Suggested change
log.Debugf("Token cache miss for namespace scope %q (user %q), requesting from Central", key.scope, key.username)
log.Debugf("Token cache miss for namespace scope %q, requesting from Central", key.scope)

@rhacs-bot
Copy link
Copy Markdown
Contributor

rhacs-bot commented Mar 30, 2026

Images are ready for the commit at 56e55bd.

To use with deploy scripts, first export MAIN_IMAGE_TAG=4.11.x-473-g56e55bd6e5.

@codecov
Copy link
Copy Markdown

codecov bot commented Mar 30, 2026

Codecov Report

❌ Patch coverage is 94.73684% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 49.65%. Comparing base (f3362c5) to head (56e55bd).
⚠️ Report is 16 commits behind head on master.

Files with missing lines Patch % Lines
sensor/common/centralproxy/transport.go 93.54% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master   #19683      +/-   ##
==========================================
+ Coverage   49.38%   49.65%   +0.27%     
==========================================
  Files        2743     2747       +4     
  Lines      207037   207278     +241     
==========================================
+ Hits       102236   102921     +685     
+ Misses      97218    96701     -517     
- Partials     7583     7656      +73     
Flag Coverage Δ
go-unit-tests 49.65% <94.73%> (+0.27%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@stehessel stehessel force-pushed the ROX-33256/console-user-in-requester branch from e105ffb to ff09221 Compare March 30, 2026 14:52
@stehessel
Copy link
Copy Markdown
Collaborator Author

@sourcery-ai review

Copy link
Copy Markdown
Contributor

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

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

Hey - I've found 1 issue

Prompt for AI Agents
Please address the comments from this code review:

## Individual Comments

### Comment 1
<location path="sensor/common/centralproxy/transport_test.go" line_range="434-443" />
<code_context>
+
+		provider := newTestTokenProvider(fakeClient, "test-cluster-id")
+
+		keyA := tokenCacheKey{scope: "shared-scope", username: "user-a"}
+		keyB := tokenCacheKey{scope: "shared-scope", username: "user-b"}
+
+		// Request for user-a.
+		tokenA, err := provider.getTokenForScope(context.Background(), keyA)
+		require.NoError(t, err)
+		assert.Equal(t, "token-user-a", tokenA)
+
+		// Request for user-b (same scope, different user, should get new token).
+		tokenB, err := provider.getTokenForScope(context.Background(), keyB)
+		require.NoError(t, err)
+		assert.Equal(t, "token-user-b", tokenB)
+
+		// Request for user-a again (should use cached).
+		tokenACached, err := provider.getTokenForScope(context.Background(), keyA)
+		require.NoError(t, err)
</code_context>
<issue_to_address>
**suggestion (testing):** Consider adding an invalidateToken test that differentiates between users with the same scope.

To fully validate the new per-user invalidation in `invalidateToken(tokenCacheKey)`, please add a corresponding test in `TestTokenProvider_InvalidateToken` that:

- Caches tokens for two keys with the same scope but different `username` values.
- Calls `invalidateToken` for only one key.
- Asserts that the invalidated user triggers a new RPC on the next call, while the other user still uses the cached token.

This complements the existing "invalidate only specific scope" test by covering the user dimension as well.

Suggested implementation:

```golang
		require.NotNil(t, resp)

		assert.Equal(t, "console-admin", fakeClient.lastRequest.GetRequester())
	})

}

// Verifies that invalidating a token for one user in a shared scope does not
// affect other users with the same scope, and that only the invalidated user
// triggers a new RPC.
func TestTokenProvider_InvalidateToken_DifferentUsersSameScope(t *testing.T) {
	ctx := context.Background()

	tokenIndex := 0
	tokens := []string{
		"token-user-a-1", // first token for user-a
		"token-user-b-1", // first token for user-b
		"token-user-a-2", // refreshed token for user-a after invalidation
	}

	fakeClient := &dynamicFakeTokenServiceClient{
		getToken: func() string {
			token := tokens[tokenIndex]
			tokenIndex++
			return token
		},
	}

	provider := newTestTokenProvider(fakeClient, "test-cluster-id")

	keyA := tokenCacheKey{scope: "shared-scope", username: "user-a"}
	keyB := tokenCacheKey{scope: "shared-scope", username: "user-b"}

	// Initial requests populate the cache for both users.
	tokenA1, err := provider.getTokenForScope(ctx, keyA)
	require.NoError(t, err)
	assert.Equal(t, "token-user-a-1", tokenA1)

	tokenB1, err := provider.getTokenForScope(ctx, keyB)
	require.NoError(t, err)
	assert.Equal(t, "token-user-b-1", tokenB1)

	// Invalidate only user-a's token.
	provider.invalidateToken(keyA)

	// Next request for user-a should trigger a new RPC and return a new token.
	tokenA2, err := provider.getTokenForScope(ctx, keyA)
	require.NoError(t, err)
	assert.Equal(t, "token-user-a-2", tokenA2)

	// User-b should still use the cached token and not trigger a new RPC.
	tokenB2, err := provider.getTokenForScope(ctx, keyB)
	require.NoError(t, err)
	assert.Equal(t, "token-user-b-1", tokenB2)

	// We expect exactly three RPCs: initial A, initial B, refreshed A.
	assert.Equal(t, 3, tokenIndex)
}

	t.Run("different users with same scope get separate cache entries", func(t *testing.T) {
		tokenIndex := 0
		tokens := []string{"token-user-a", "token-user-b"}
		fakeClient := &dynamicFakeTokenServiceClient{
			getToken: func() string {

```

If `context` is not already imported in this file, add:

```go
import "context"
```

to the import block. The new test assumes `newTestTokenProvider`, `dynamicFakeTokenServiceClient`, `tokenCacheKey`, `require`, and `assert` are already defined and imported as in the existing tests. Adjust the token strings or assertion style if your existing tests follow a different naming or assertion convention.
</issue_to_address>

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

@stehessel stehessel force-pushed the ROX-33256/console-user-in-requester branch from ff09221 to 56e55bd Compare March 30, 2026 21:49
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Mar 30, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Central YAML (base), Organization UI (inherited)

Review profile: CHILL

Plan: Pro

Run ID: 6ae81789-11b3-41ad-a9ae-8b0ceb1a84db

📥 Commits

Reviewing files that changed from the base of the PR and between 574b0ee and 56e55bd.

📒 Files selected for processing (4)
  • sensor/common/centralproxy/handler.go
  • sensor/common/centralproxy/handler_test.go
  • sensor/common/centralproxy/transport.go
  • sensor/common/centralproxy/transport_test.go

📝 Walkthrough

Summary by CodeRabbit

  • New Features

    • Authenticated console username is now tracked and used in token management, with tokens cached separately per user and namespace scope for improved resource isolation between users.
  • Tests

    • Added comprehensive test coverage for user-specific token handling, cache separation, and proper request forwarding.

Walkthrough

Added console username context propagation through request handling and token caching layers. Introduces context key helpers in handler to attach authenticated username, derives username in transport's RoundTrip method, uses composite cache keys combining namespace scope and username for token caching, and includes requester field in token generation requests to Central.

Changes

Cohort / File(s) Summary
Context Management
sensor/common/centralproxy/handler.go, sensor/common/centralproxy/handler_test.go
Introduced typed context key and helper functions (contextWithConsoleUser, consoleUserFromContext) to attach and retrieve authenticated username from request context. Added test coverage verifying username propagation through the request lifecycle.
Token Caching and Coalescing
sensor/common/centralproxy/transport.go, sensor/common/centralproxy/transport_test.go
Refactored token caching to use composite keys combining namespace scope and console username. Updated RoundTrip to derive username from context and build composite cache keys. Modified token provider cache type and related function signatures. Strips internal stackroxNamespaceHeader before forwarding requests. Extended test suite to verify requester propagation, user-specific cache separation, and header stripping behavior.

Sequence Diagram

sequenceDiagram
    participant Client
    participant Handler
    participant Transport
    participant TokenProvider
    participant Central

    Client->>Handler: HTTP Request with Bearer Token
    Handler->>Handler: Authenticate & Extract Username
    Handler->>Handler: Create Context with Username
    Handler->>Transport: ServeHTTP with Updated Context
    Transport->>Transport: Derive Username from Context
    Transport->>Transport: Build Composite Cache Key<br/>(Namespace + Username)
    Transport->>TokenProvider: Check Token Cache<br/>for Composite Key
    alt Cache Hit
        TokenProvider-->>Transport: Return Cached Token
    else Cache Miss
        Transport->>TokenProvider: Request Token with Username
        TokenProvider->>Central: GenerateTokenForPermissionsAndScopeRequest<br/>(Requester: username)
        Central-->>TokenProvider: Token Response
        TokenProvider->>TokenProvider: Store in Cache<br/>(Composite Key)
        TokenProvider-->>Transport: Return New Token
    end
    Transport->>Transport: Strip Internal Headers
    Transport->>Central: Forward Request with Token
    Central-->>Transport: Response
    Transport-->>Client: Response
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~30 minutes

🚥 Pre-merge checks | ✅ 1 | ❌ 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Description check ⚠️ Warning The description contains only placeholder text ('change me!') with unchecked documentation and testing checkboxes, providing no actual details about the changes, validation, or testing approach. Replace placeholder text with a detailed description of the changes, clarify testing and validation approach (noting that unit tests were added), and check appropriate documentation/testing checkboxes.
Docstring Coverage ⚠️ Warning Docstring coverage is 25.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (1 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately and concisely describes the main change: adding console user information to the requester claim in the central proxy handler.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch ROX-33256/console-user-in-requester

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants