move OAuth route to project group

[MichaelClifford]

No description provided.

[github-actions]

Claude Code Review

Summary

This PR moves the OAuth URL generation endpoint from an unauthenticated route to the project group with proper authentication and authorization. This is a security improvement that adds user token validation and RBAC checks to the OAuth flow.

Issues by Severity

🚫 Blocker Issues

None - the PR is safe to merge from a security perspective.

🔴 Critical Issues

None identified.

🟡 Major Issues

1. Missing Import for schema Package

Location: components/backend/handlers/oauth.go:115-120

The code uses schema.GroupVersionResource but doesn't import it in the file:

gvr := schema.GroupVersionResource{
    Group:    "vteam.ambient-code",
    Version:  "v1alpha1",
    Resource: "agenticsessions",
}

However, looking at the imports (line 21), schema is already imported:

"k8s.io/apimachinery/pkg/runtime/schema"

So this is actually not an issue - the import exists. My mistake on initial review.

🔵 Minor Issues

1. Duplicate GVR Definition

Location: components/backend/handlers/oauth.go:116-120 and oauth.go:695-699

The GroupVersionResource for AgenticSession is defined inline in two places:

• Line 116-120 in GetOAuthURL
• Line 695-699 in storeCredentialsInSecret

Recommendation: Extract this to a helper function or use a shared constant pattern like in types/ package:

// In handlers/helpers.go or types/session.go
func GetAgenticSessionGVR() schema.GroupVersionResource {
    return schema.GroupVersionResource{
        Group:    "vteam.ambient-code",
        Version:  "v1alpha1",
        Resource: "agenticsessions",
    }
}

This follows the DRY principle and matches patterns in the operator code.

2. Error Handling Pattern Inconsistency

Location: components/backend/handlers/oauth.go:127

The code checks for errors.IsForbidden(err) explicitly:

if errors.IsForbidden(err) {
    c.JSON(http.StatusForbidden, gin.H{"error": "Access denied to session"})
    return
}

However, when using user-scoped clients (reqDyn), a forbidden error typically manifests as IsNotFound due to RBAC filtering, not as IsForbidden. The IsForbidden check may be redundant.

Pattern from CLAUDE.md (line 535-538):

if errors.IsNotFound(err) {
    c.JSON(http.StatusNotFound, gin.H{"error": "Session not found"})
    return
}

The current code is safer (checks both), so this is minor. Consider removing the explicit IsForbidden check since it's unlikely to trigger with user-scoped clients.

Positive Highlights

✅ Excellent Security Improvement: Moving the OAuth endpoint behind authentication prevents unauthorized users from initiating OAuth flows for sessions they don't own.

✅ Follows User Token Authentication Pattern: Correctly uses GetK8sClientsForRequest(c) to verify user identity and permissions (CLAUDE.md line 426).

✅ Proper Error Handling: All error paths are logged with context and return appropriate HTTP status codes (matches error-handling.md patterns).

✅ RBAC-Compliant: By using user-scoped clients, the endpoint respects Kubernetes RBAC permissions automatically.

✅ Maintains Backward Compatibility: The route path stays the same (/oauth/:provider/url), just moved to authenticated group.

✅ Clear Logging: Line 132 logs failures with full context (projectName, sessionName).

✅ Type-Safe K8s Access: Uses proper Get() with error checking, not direct type assertions.

Recommendations

High Priority

1. Verify Import Statement ✅ (Already correct - schema is imported)

Medium Priority

2. Extract GVR to Helper Function
   • Create GetAgenticSessionGVR() in handlers/helpers.go
   • Replace inline GVR definitions in oauth.go (lines 116-120, 695-699)
   • Consider doing this across the entire backend for consistency

Low Priority

3. Simplify Error Handling

   • Consider removing the IsForbidden check at line 127 since user-scoped clients won't return it
   • Or keep it for defense-in-depth (current approach is fine)

4. Add Integration Test

   • Test that unauthenticated requests return 401
   • Test that users without session access return 404 (not 403)
   • Test that authorized users successfully get OAuth URL

Security Review

✅ User Token Authentication: Correctly implemented
✅ RBAC Enforcement: Implicit through user-scoped client
✅ No Token Leaks: No sensitive data in logs
✅ Proper HTTP Status Codes: 401 for auth failure, 404 for not found
✅ Error Context Logging: Yes, with project and session names
✅ Input Validation: Provider name validated via getOAuthProvider()

Architectural Alignment

This change aligns with:

• ADR-0002 (User Token Authentication): All API operations use user tokens ✅
• Backend Development Standards: Follows GetK8sClientsForRequest pattern ✅
• Security Standards: RBAC enforcement, token handling, input validation ✅
• Error Handling Patterns: Log + return appropriate status codes ✅

Pre-Commit Checklist Status

• User-facing endpoints use GetK8sClientsForRequest
• RBAC checks performed (implicit via user client)
• Errors logged with context
• No tokens in logs
• Type-safe K8s access
• Appropriate HTTP status codes
• Extract duplicate GVR definition (optional improvement)

Verdict

✅ APPROVE - This is a solid security improvement that correctly implements user token authentication for the OAuth flow. The code follows established patterns from CLAUDE.md and the security standards. The minor issues noted are opportunities for future refactoring, not blockers.

The PR successfully:

1. Prevents unauthorized OAuth flow initiation
2. Respects RBAC permissions
3. Follows established backend patterns
4. Maintains proper error handling

Recommended Action: Merge after addressing the GVR duplication in a follow-up PR if desired (not blocking).

---

🔍 View AI decision process (logs available for 90 days)

📋 View memory system files loaded (click to expand)

What Amber Loaded for Code Review

Amber automatically loaded these repository standards from the memory system:

1. CLAUDE.md - Master project instructions, development standards
2. backend-development.md - Go backend, K8s integration patterns
3. frontend-development.md - NextJS, Shadcn UI, React Query patterns
4. security-standards.md - Auth, RBAC, token handling
5. k8s-client-usage.md - User token vs service account patterns
6. error-handling.md - Consistent error patterns
7. react-query-usage.md - Data fetching patterns

Impact: This review used your repository's specific code quality standards, security patterns, and best practices from the memory system (PRs #359, #360) - not just generic code review guidelines.