OAuth passthrough support

[aangelisc]

Adding OAuth passthrough support to the GCL data source.

This functionality will allow users that are logged into Grafana using Google OAuth to also authenticate the data source with their OAuth token rather than static credentials. This allows for improved security and more granular data access.

I've also refactored the config editor a little to make it clearer what authentication type is being used. This can be a little confusing for the authentication methods that come from the SDK itself and I'm happy to take feedback here.

I've updated the docs accordingly and have tested that this works as expected.

[google-cla]

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

[xiangshen-dk]

The AI I am using provided the following review. When you get a chance, can you please take a look and let me know your thoughts? Can we address at least the critical issues? Thanks!

PR #145 Code Review: OAuth Passthrough Authentication

Summary

This PR adds OAuth Passthrough authentication support, allowing users to authenticate to Google Cloud Logging using their browser OAuth token from Grafana's Google authentication.

---

🔴 Critical Issues (Must Fix)

1. Race Condition + CRITICAL SECURITY VULNERABILITY (Confused Deputy / Data Leakage) (pkg/plugin/plugin.go)

func (d *CloudLoggingDatasource) SetOauthClient(ctx context.Context, headers map[string]string) error {
    client, err := cloudlogging.NewClientWithPassThrough(ctx, headers)
    if err != nil {
        return err
    }
    d.client = client  // ⚠️ RACE CONDITION + SECURITY VULNERABILITY
    return nil
}

Problem: The d.client field is mutated on every request when using OAuth passthrough. Since CloudLoggingDatasource is shared across ALL users of the datasource, this creates a critical security vulnerability:

1. User A sends a request with their OAuth token
2. SetOauthClient sets d.client to User A's client
3. User B sends a request concurrently
4. Before User A's query executes, SetOauthClient might be called for User B (or vice versa)
5. Result: User B could execute a query using User A's credentials, potentially seeing logs they are NOT authorized to access

This is a Confused Deputy vulnerability that can lead to unauthorized data access between users.

Recommendation:

• Do NOT store the client on the struct. Instead, create a per-request client and pass it through the call chain
• Example pattern:

func (d *CloudLoggingDatasource) QueryData(ctx context.Context, req *backend.QueryDataRequest) (*backend.QueryDataResponse, error) {
    var client cloudlogging.API
    if d.oauthPassThrough {
        var err error
        client, err = cloudlogging.NewClientWithPassThrough(ctx, req.Headers)
        if err != nil {
            return nil, err
        }
        defer client.Close()  // Ensure cleanup
    } else {
        client = d.client
    }
    // Use 'client' for all operations in this request
}

2. Resource Leak - Clients Not Closed (pkg/plugin/plugin.go)

func (d *CloudLoggingDatasource) SetOauthClient(ctx context.Context, headers map[string]string) error {
    client, err := cloudlogging.NewClientWithPassThrough(ctx, headers)
    // Previous d.client is never closed!
    d.client = client
    return nil
}

Problem: Each request creates a new client but the previous client is never closed, causing gRPC connection leaks that will eventually exhaust system resources.

Recommendation: When fixing Issue #1 with per-request clients, ensure defer client.Close() is called after each request.

3. Nil Pointer Dereference in Dispose() (pkg/plugin/plugin.go)

func (d *CloudLoggingDatasource) Dispose() {
    if err := d.client.Close(); err != nil {  // ⚠️ d.client is nil for oauthPassthrough
        log.DefaultLogger.Error("failed closing client", "error", err)
    }
}

Problem: When oauthPassthrough is used, d.client is nil at initialization, causing a panic when Dispose() is called.

Recommendation: Add nil check:

func (d *CloudLoggingDatasource) Dispose() {
    if d.client != nil {
        if err := d.client.Close(); err != nil {
            log.DefaultLogger.Error("failed closing client", "error", err)
        }
    }
}

4. Missing Token Validation (pkg/plugin/cloudlogging/client.go)

func NewClientWithPassThrough(ctx context.Context, headers map[string]string) (*Client, error) {
    token, _ := strings.CutPrefix(headers["Authorization"], "Bearer ")
    // No validation if token is empty!
    oauthOpt := option.WithTokenSource(...)

Problem: If the Authorization header is missing or malformed, an empty token is used silently, leading to confusing authentication errors downstream.

Recommendation: Validate the token exists:

token, found := strings.CutPrefix(headers["Authorization"], "Bearer ")
if !found || token == "" {
    return nil, errors.New("missing or invalid Authorization header")
}

---

🟡 Medium Issues

5. Inconsistent Header Key Casing (pkg/plugin/plugin.go & client.go)

// In plugin.go - CallResource uses map[string]string with original casing
headers[k] = strings.Join(v, ",")

// In client.go - expects "Authorization" with exact casing
token, _ := strings.CutPrefix(headers["Authorization"], "Bearer ")

Problem: HTTP headers are case-insensitive, but Go maps are case-sensitive. The header might come as authorization or AUTHORIZATION.

Recommendation: Use case-insensitive header lookup:

func getHeader(headers map[string]string, key string) string {
    for k, v := range headers {
        if strings.EqualFold(k, key) {
            return v
        }
    }
    return ""
}

6. Multiple Authorization Headers Edge Case (pkg/plugin/plugin.go)

// In CallResource
headers[k] = strings.Join(v, ",")

Problem: If multiple Authorization headers are present (rare, but possible in some proxy configurations), joining them with a comma creates an invalid token string (e.g., "Bearer token1,Bearer token2").

Recommendation: For Authorization headers, strictly select the first value:

for k, v := range req.Headers {
    if strings.EqualFold(k, "Authorization") && len(v) > 0 {
        headers[k] = v[0]  // Take only the first Authorization header
    } else {
        headers[k] = strings.Join(v, ",")
    }
}

7. Missing User-Agent (pkg/plugin/cloudlogging/client.go)

func NewClientWithPassThrough(ctx context.Context, headers map[string]string) (*Client, error) {
    // Missing: option.WithUserAgent("googlecloud-logging-datasource")
    client, err := logging.NewClient(ctx, oauthOpt)

Problem: Other client constructors include WithUserAgent, but this one doesn't, causing inconsistent API tracking.

Recommendation: Add option.WithUserAgent("googlecloud-logging-datasource") to all client creations in NewClientWithPassThrough.

8. State Synchronization Issue in ConfigEditor (src/ConfigEditor.tsx)

state = {
    isChecked: this.props.options.jsonData.usingImpersonation || false,
    sa: this.props.options.jsonData.serviceAccountToImpersonate || '',
    authenticationMethod: this.props.options.jsonData.authenticationType || GoogleAuthType.JWT,
};

Problem: authenticationMethod is stored in state but never updated when handleAuthTypeChange is called, causing potential UI inconsistencies.

Recommendation: Either remove unused state or update it in handleAuthTypeChange.

---

🟢 Minor Issues / Suggestions

9. Unnecessary break Statement (pkg/plugin/plugin.go)

case oauthpassthroughAuthentication:
    oauthPassThrough = true
    break  // Unnecessary in Go switch

10. Missing Tests

No unit tests were added for the new OAuth passthrough functionality. Consider adding:

• Test for NewClientWithPassThrough
• Test for SetOauthClient
• Test for OAuth passthrough flow in QueryData and CheckHealth

11. Documentation Improvement

The README mentions OAuth passthrough but doesn't document the security implications of using user tokens.

---

Performance Concerns

1. Client Creation Per Request: Creating a new GCP client for every request is expensive (involves gRPC connection setup). If implementing caching in the future:

   • IMPORTANT: Do NOT use the request's context.Context for cached clients. Use context.Background() for client creation, as request contexts are short-lived and will cancel the client when the request completes.
   • Consider token-based client caching with TTL matching token expiry.

2. No Connection Pooling: Each request creates 3 new clients (logging, resourcemanager, config). This could be optimized with connection pooling.

---

Implementation Plan

Phase 1: Fix Critical Security & Correctness Issues

1. Refactor to per-request client pattern (Fixes Issues update release tag #1, Update release file #2)

   • Remove SetOauthClient method
   • Create client in each handler method (QueryData, CallResource, CheckHealth)
   • Pass client through call chain or use a request-scoped pattern
   • Ensure defer client.Close() for cleanup

2. Add nil check in Dispose() (Fixes Issue Update plugin.json #3)

3. Add token validation in NewClientWithPassThrough (Fixes Issue Update the files based on review feedback. #4)

Phase 2: Fix Medium Issues

4. Implement case-insensitive header lookup (Fixes Issue update version in plugin.json #5)
5. Handle multiple Authorization headers (Fixes Issue Adds Cloud Logging Service Endpoint option to configuration #6)
6. Add User-Agent to NewClientWithPassThrough (Fixes Issue prepare for rel 1.1.0 #7)
7. Fix ConfigEditor state management (Fixes Issue Fixes bad datasource structure in datasource test #8)

Phase 3: Polish

8. Remove unnecessary break statement (Fixes Issue Fixes GitHub CI react dependency issues on yarn test #9)
9. Add unit tests for OAuth passthrough
10. Update documentation with security considerations

---

Recommendation

Do not merge until at minimum the critical issues (1-4) are addressed. Issue #1 is a critical security vulnerability that could lead to unauthorized data access between users. The current implementation is fundamentally unsafe for multi-user environments.

[aangelisc]

Hi @xiangshen-dk, I've updated based on the review, PTAL