fix(grpc/session-server): authorise to token introspection endpoint

[alienvspredator]

fix(grpc/session-server): call token introspection endpoint with authorisation

Summary by CodeRabbit

Release Notes

• Bug Fixes

  • Server startup errors are now properly reported instead of being suppressed, improving troubleshooting and debugging
  • Transport credential initialization failures now include descriptive error messages for easier diagnosis and configuration

• Improvements

  • Server initialization process includes more robust credential handling and validation during startup

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: c27a9357-e260-429d-b114-c18a9f84228f

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

Walkthrough

The pull request refactors server construction by moving credentials building before gRPC session server instantiation and extracting session server options into a dedicated file. The NewSessionServer constructor is updated to accept an additional clientID parameter and explicit options instead of variadic option slices. A new credentials.Builder type alias is introduced in the credentials package, replacing the local CredentialsBuilder type previously defined in the session package. The manager and related code are updated to use the centralized credentials builder type. All test call sites are updated to match the new constructor signature.

Suggested reviewers

• apatsap
• pabloszel11
• cb80
• OlegGitH

Poem

🐰 Hops of joy through credential flows,
Options grouped where order grows,
Client IDs now find their place,
Refactored with architectural grace!
BuildersUnite in credentials package bright, ✨

🚥 Pre-merge checks | ✅ 1 | ❌ 2

❌ Failed checks (2 warnings)

Check name	Status	Explanation	Resolution
Description check	⚠️ Warning	The pull request description is minimal and lacks required template sections (What this PR does, Special notes, Release note). While it repeats the title information, it does not follow the repository's description template structure.	Complete the description using the repository template: add a detailed explanation under 'What this PR does / why we need it', include any 'Special notes for your reviewer', and specify the 'Release note' section.
Docstring Coverage	⚠️ Warning	Docstring coverage is 5.88% 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 clearly describes the main change: adding authorization credentials to token introspection endpoint calls in the gRPC session server, which aligns with the refactoring across multiple files.

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

---

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

• X
• Mastodon
• Reddit
• LinkedIn

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

[coderabbitai]

🧹 Nitpick comments (2)

internal/grpc/session.go (2)

235-241: Consider reusing HTTP clients for connection pooling.

A new http.Client is created for each introspection call (after cache miss). While functionally correct, this prevents connection reuse and pooling. For high-throughput scenarios, consider caching HTTP clients per clientID.

This is a minor optimization and may not be necessary depending on expected load.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/grpc/session.go` around lines 235 - 241, The code creates a new
http.Client for each introspection provider by calling s.httpClient(oidcTrust)
before oidc.NewProvider, which prevents connection pooling; modify s.httpClient
(or add a new method) to cache and return an *http.Client per client identifier
(e.g., oidcTrust.ClientID) using a protected map (sync.Mutex or sync.RWMutex)
stored on the session object so subsequent calls to oidc.NewProvider reuse the
same client; update callers (where s.httpClient is invoked) to pass the clientID
or let s.httpClient derive it from oidcTrust so connection reuse and pooling are
achieved.

---

212-217: Add timeout to HTTP client for introspection requests.

The http.Client is created without explicit Timeout configuration. While the introspection call uses context-based timeouts (line 249: provider.IntrospectToken(ctx, token)), adding an HTTP client timeout provides an additional safety net against slow or unresponsive OIDC introspection endpoints, preventing potential resource exhaustion.

♻️ Proposed fix

 func (s *SessionServer) httpClient(mapping *trust.OIDCMapping) *http.Client {
 	creds := s.newCreds(s.getClientID(mapping))
 	return &http.Client{
 		Transport: creds.Transport(),
+		Timeout:   30 * time.Second,
 	}
 }

Note: An identical httpClient method exists in internal/session/manager.go (lines 611-615) and should receive the same treatment.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/grpc/session.go` around lines 212 - 217, The http.Client returned by
SessionServer.httpClient lacks a Timeout and should set a sensible timeout to
guard OIDC introspection calls; update the SessionServer.httpClient (which uses
newCreds(s.getClientID(mapping)) and is used around
provider.IntrospectToken(ctx, token)) to return &http.Client{Transport:
creds.Transport(), Timeout: <reasonable duration>} and apply the same change to
the duplicate httpClient in internal/session/manager.go so both places have an
explicit timeout value (use a constant or config-driven duration rather than a
magic literal).

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@internal/grpc/session.go`:
- Around line 235-241: The code creates a new http.Client for each introspection
provider by calling s.httpClient(oidcTrust) before oidc.NewProvider, which
prevents connection pooling; modify s.httpClient (or add a new method) to cache
and return an *http.Client per client identifier (e.g., oidcTrust.ClientID)
using a protected map (sync.Mutex or sync.RWMutex) stored on the session object
so subsequent calls to oidc.NewProvider reuse the same client; update callers
(where s.httpClient is invoked) to pass the clientID or let s.httpClient derive
it from oidcTrust so connection reuse and pooling are achieved.
- Around line 212-217: The http.Client returned by SessionServer.httpClient
lacks a Timeout and should set a sensible timeout to guard OIDC introspection
calls; update the SessionServer.httpClient (which uses
newCreds(s.getClientID(mapping)) and is used around
provider.IntrospectToken(ctx, token)) to return &http.Client{Transport:
creds.Transport(), Timeout: <reasonable duration>} and apply the same change to
the duplicate httpClient in internal/session/manager.go so both places have an
explicit timeout value (use a constant or config-driven duration rather than a
magic literal).

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 565c0133-40c7-4424-a9f0-1366555a6a4a

📥 Commits

Reviewing files that changed from the base of the PR and between 88da356 and 5e93bc7.

📒 Files selected for processing (9)

• internal/business/business.go
• internal/business/server/grpc_server_test.go
• internal/credentials/credentials.go
• internal/grpc/options.go
• internal/grpc/session.go
• internal/grpc/session_test.go
• internal/session/manager.go
• internal/session/manager_test.go
• internal/session/options.go

[cb80]

lgtm

[coderabbitai]

🧹 Nitpick comments (1)

internal/business/business.go (1)

100-103: Consider a more accurate error message.

The error message says "failed to load http client" but this code builds transport credentials, not an HTTP client. A more precise message like "failed to build transport credentials: %w" would better describe the actual failure point and aid debugging.

This same message is used at line 134 in initSessionManager, so both could be updated for consistency.

Suggested improvement

 credsBuilder, err := newCredsBuilder(cfg)
 if err != nil {
-    return fmt.Errorf("failed to load http client: %w", err)
+    return fmt.Errorf("failed to build transport credentials: %w", err)
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/business/business.go` around lines 100 - 103, Update the error
messages to accurately reflect the failure to build transport credentials:
replace the generic "failed to load http client" with "failed to build transport
credentials: %w" in the block where newCredsBuilder(cfg) is called (symbol:
newCredsBuilder) and make the same change in initSessionManager where the
identical message is used so both locations consistently report failures
constructing transport credentials.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@internal/business/business.go`:
- Around line 100-103: Update the error messages to accurately reflect the
failure to build transport credentials: replace the generic "failed to load http
client" with "failed to build transport credentials: %w" in the block where
newCredsBuilder(cfg) is called (symbol: newCredsBuilder) and make the same
change in initSessionManager where the identical message is used so both
locations consistently report failures constructing transport credentials.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 8039748e-1f5f-47f1-a419-28bc8e7eaca4

📥 Commits

Reviewing files that changed from the base of the PR and between 239b97e and 74e28e4.

📒 Files selected for processing (11)

• integration/grpc_test.go
• integration/session_grpc_test.go
• internal/business/business.go
• internal/business/server/grpc_server_test.go
• internal/credentials/credentials.go
• internal/grpc/options.go
• internal/grpc/session.go
• internal/grpc/session_test.go
• internal/session/manager.go
• internal/session/manager_test.go
• internal/session/options.go

✅ Files skipped from review due to trivial changes (1)

• internal/grpc/session_test.go

🚧 Files skipped from review as they are similar to previous changes (8)

• internal/credentials/credentials.go
• internal/session/options.go
• integration/session_grpc_test.go
• internal/business/server/grpc_server_test.go
• internal/session/manager.go
• internal/grpc/options.go
• internal/session/manager_test.go
• internal/grpc/session.go