Enhance project access control in backend handlers

[Gkrumbach07]

• Updated GetProject, UpdateProject, and DeleteProject functions to use K8sClientProjects for namespace retrieval, ensuring consistent access checks.
• Introduced permission verification for viewing and modifying projects using checkUserCanViewProject and checkUserCanModifyProject functions.
• Removed deprecated access check function and added error handling for permission verification failures.
• Improved error responses for unauthorized access attempts.

These changes enhance security and ensure proper access control for project management operations.

[github-actions]

Code Review - PR #268: Enhance project access control in backend handlers

Overall Assessment

This PR improves RBAC enforcement for project management operations by introducing fine-grained permission checks using projectsettings resource permissions. The approach is sound and aligns with Kubernetes-native RBAC patterns.

---

✅ Strengths

1. Proper RBAC Granularity

The PR correctly distinguishes between view (GET) and modify (UPDATE) permissions on projectsettings:

• checkUserCanViewProject - Checks GET permission for read operations
• checkUserCanModifyProject - Checks UPDATE permission for write operations

This is more precise than the previous checkUserCanAccessNamespace which only checked list agenticsessions.

2. Security-First Design

• Uses user-scoped clients (reqK8s) for permission checks ✅
• Uses backend SA (K8sClientProjects) only for namespace retrieval ✅
• Maintains proper separation of concerns per CLAUDE.md guidelines

3. Backward Compatibility

The deprecated checkUserCanAccessNamespace function is preserved and redirects to the new implementation, preventing breaking changes.

4. Proper Error Handling

• Returns 403 Forbidden when permissions are denied
• Returns 500 Internal Server Error when permission checks fail
• Logs unauthorized access attempts for audit purposes

---

⚠️ Issues & Concerns

1. CRITICAL: Inconsistent Authentication Pattern

The changes introduce a security inconsistency that violates CLAUDE.md guidelines:

In GetProject and UpdateProject:

// BEFORE (Correct per CLAUDE.md):
ns, err := reqK8s.CoreV1().Namespaces().Get(ctx, projectName, v1.GetOptions{})

// AFTER (Violates guidelines):
ns, err := K8sClientProjects.CoreV1().Namespaces().Get(ctx, projectName, v1.GetOptions{})

Problem: The PR switches from user-scoped client to backend SA for namespace retrieval, then adds a separate permission check. This creates a race condition where:

1. Backend SA retrieves namespace (always succeeds if namespace exists)
2. User permission check happens afterward
3. A malicious user could gain information about namespace existence even without permissions

CLAUDE.md violation (lines 454-458):

Critical Rules (Never Violate)

1. User Token Authentication Required
   • FORBIDDEN: Using backend service account for user-initiated API operations
   • REQUIRED: Always use GetK8sClientsForRequest(c) to get user-scoped K8s clients

Recommended Fix:
Keep the original pattern where reqK8s retrieves the namespace. The Kubernetes API will naturally enforce RBAC, and you can add the projectsettings check as an additional layer afterward:

// Use user-scoped client (enforces namespace-level RBAC)
ns, err := reqK8s.CoreV1().Namespaces().Get(ctx, projectName, v1.GetOptions{})
if err != nil {
    if errors.IsNotFound(err) || errors.IsForbidden(err) {
        c.JSON(http.StatusNotFound, gin.H{"error": "Project not found"})
        return
    }
    log.Printf("Failed to get Namespace %s: %v", projectName, err)
    c.JSON(http.StatusInternalServerError, gin.H{"error": "Failed to get project"})
    return
}

// Additional check: verify user can GET projectsettings (finer-grained control)
canView, err := checkUserCanViewProject(reqK8s, projectName)
if err != nil || !canView {
    c.JSON(http.StatusForbidden, gin.H{"error": "Unauthorized to view project"})
    return
}

This approach:

• ✅ Uses user token for all data retrieval (CLAUDE.md compliant)
• ✅ Prevents information disclosure about namespace existence
• ✅ Provides defense-in-depth with two RBAC checks

---

2. Removed Forbidden Error Handling

The PR removes explicit handling of errors.IsForbidden(err):

// REMOVED:
if errors.IsForbidden(err) {
    log.Printf("User forbidden to access Namespace %s: %v", projectName, err)
    c.JSON(http.StatusForbidden, gin.H{"error": "Unauthorized to access project"})
    return
}

Impact: If K8sClientProjects (backend SA) lacks permissions to get namespaces, the error will be caught by the generic error handler and return 500 instead of a more specific error. However, since backend SA should have cluster-level permissions, this is less critical.

Recommendation: If keeping the backend SA approach, this is acceptable. If reverting to user-scoped client (recommended), restore this error handling.

---

3. Test Coverage Missing

Per CLAUDE.md, backend changes should have corresponding tests:

Pre-Commit Checklist for Backend/Operator

• Tests: Added/updated tests for new functionality

Missing Test Scenarios:

• User with GET projectsettings permission can view project
• User without GET projectsettings permission gets 403
• User with UPDATE projectsettings permission can modify/delete project
• User without UPDATE projectsettings permission gets 403
• Permission check failure returns 500

Recommendation: Add unit tests in components/backend/handlers/projects_test.go using table-driven tests. Reference CLAUDE.md section on Go testing standards.

---

4. Performance Consideration

The new implementation adds an additional API call (SelfSubjectAccessReview) to GetProject, UpdateProject, and DeleteProject.

Current flow:

1. Get namespace (via backend SA or user token)
2. Check managed label
3. Check projectsettings permission (+1 API call)

Impact: Minimal - SSAR checks are typically fast (<50ms). The 2-second timeout is appropriate.

Optimization opportunity: If using user-scoped client for namespace retrieval, the first RBAC check happens implicitly, so the added latency is justified for defense-in-depth.

---

5. Inconsistency with Middleware

ValidateProjectContext middleware (middleware.go:255-265) checks:

Verb:      "list",
Resource:  "agenticsessions",

But project handlers now check projectsettings permissions. This creates inconsistency:

• Middleware: "Can user list agenticsessions?"
• Handlers: "Can user GET/UPDATE projectsettings?"

Question: Should ValidateProjectContext also check projectsettings permissions for project-scoped endpoints? Or is the current two-layer approach intentional?

---

🔍 Code Quality

Positive:

• ✅ Clear function names (checkUserCanViewProject, checkUserCanModifyProject)
• ✅ Proper context usage with 2-second timeout
• ✅ Structured logging with context (namespace, operation)
• ✅ Deprecation notice on old function

Minor Issues:

• Missing error context: When permission check fails in UpdateProject/DeleteProject, the error message is generic. Consider adding more context:

  log.Printf("UpdateProject: User failed projectsettings UPDATE check for %s: allowed=%v err=%v", projectName, canModify, err)

---

🔐 Security Assessment

Strengths:

• Fine-grained RBAC using projectsettings resource
• Logs unauthorized access attempts for audit trails
• Uses SelfSubjectAccessReview (Kubernetes-native)

Concerns:

• Information Disclosure: Switching to backend SA for namespace retrieval may leak namespace existence to unauthorized users
• TOCTOU (Time-of-Check-Time-of-Use): Small window between permission check and actual operation, but this is inherent to RBAC and acceptable

---

📋 Recommendations Summary

Must Fix (Security):

1. Revert to user-scoped client for namespace retrieval in GetProject/UpdateProject to comply with CLAUDE.md and prevent information disclosure
2. Keep the projectsettings permission check as an additional layer

Should Fix (Quality):

3. Add unit tests covering permission scenarios
4. Restore errors.IsForbidden handling if using user-scoped clients
5. Document the relationship between middleware RBAC checks and handler-level checks

Nice to Have (Enhancement):

6. Consider adding metric instrumentation for permission denials
7. Add integration test validating the full RBAC chain

---

📚 Reference

• CLAUDE.md Backend Development Standards (lines 126-458)
• Established pattern: components/backend/handlers/sessions.go:227, 449

---

Overall: Good security improvement, but requires a critical fix to maintain compliance with established authentication patterns. The switch to backend SA for data retrieval undermines the security model defined in CLAUDE.md.

Recommendation: Request changes, specifically reverting namespace retrieval to use reqK8s while keeping the new permission checks.

[github-actions]

Code Review: Enhance Project Access Control

Overview

This PR improves RBAC enforcement by switching from namespace-level to projectsettings resource-based permissions.

Strengths

1. Proper Separation: Backend SA retrieves namespace, user token validates RBAC (lines 472, 491, 546, 564)
2. Granular Permissions: checkUserCanViewProject (GET) vs checkUserCanModifyProject (UPDATE)
3. Security Logging: Good audit trail at lines 499, 574, 663
4. Backward Compatibility: Deprecated function preserved with notice

Issues and Recommendations

1. Critical: Permission Check Order

Issue: GetProject/UpdateProject retrieve namespace with backend SA BEFORE checking user permissions. This could leak namespace existence information.

Recommendation: Check permissions FIRST, then retrieve namespace.

2. Missing Test Coverage

Required: Add tests for:

• checkUserCanViewProject validates GET projectsettings
• checkUserCanModifyProject validates UPDATE projectsettings
• 403 responses when denied
• RBAC check error handling

Reference: CLAUDE.md mentions tests/integration/ and make test-permissions

3. Timeout Context

Issue: Permission checks use 2s timeout, namespace ops use 10s. Consider coordinating or reusing parent context.

4. Error Messages

Line 494 returns generic error without distinguishing infrastructure failures from authorization denials.

5. Verify RBAC Rules

Confirm RoleBindings grant get/update verbs for vteam.ambient-code/projectsettings resource.

Pre-Merge Checklist

• Uses GetK8sClientsForRequest: YES
• Returns 401 for invalid token: YES
• Logs errors with context: YES
• No panic calls: YES
• Tests for new functions: NO (needs attention)
• Permission-first approach: NO (should consider)

Run: cd components/backend && gofmt -l . && go vet ./... && golangci-lint run && make test

Overall: Approve with Recommendations

Core improvement is sound. Required before merge:

1. Add test coverage
2. Consider permission-check-first pattern

Great work on the granular permission model!

[github-actions]

Code Review: Enhance Project Access Control

Thank you for this security improvement! I've reviewed the changes against the project's CLAUDE.md guidelines. Here's my feedback:

✅ Strengths

1. Improved RBAC Granularity: Using projectsettings resource permissions (get for view, update for modify) provides more fine-grained access control than the previous namespace-level checks.

2. Proper SelfSubjectAccessReview Usage: The new checkUserCanViewProject and checkUserCanModifyProject functions correctly use the K8s RBAC engine to determine access from all sources (RoleBindings, ClusterRoleBindings, groups).

3. Consistent Pattern: Both functions follow the same structure with proper timeout handling and error propagation.

4. Backward Compatibility: Deprecated function is maintained with a compatibility shim.

⚠️ Critical Issues

1. VIOLATION: User Token Authentication Rule

Problem: The PR switches from user-scoped client (reqK8s) to service account client (K8sClientProjects) for namespace retrieval in GetProject, UpdateProject, and DeleteProject.

Current code (lines 466, 526):

ns, err := reqK8s.CoreV1().Namespaces().Get(ctx, projectName, v1.GetOptions{})

PR changes to:

ns, err := K8sClientProjects.CoreV1().Namespaces().Get(ctx, projectName, v1.GetOptions{})

Why this is wrong: According to CLAUDE.md Backend Development Standards:

User Token Authentication Required

• FORBIDDEN: Using backend service account for user-initiated API operations
• REQUIRED: Always use GetK8sClientsForRequest(c) to get user-scoped K8s clients
• Exception: Backend service account ONLY for CR writes and token minting

Impact:

• Bypasses Kubernetes RBAC for namespace access
• Users could potentially view/modify namespaces they shouldn't have access to
• The explicit permission checks added later don't fully compensate because they only check projectsettings resource, not namespace access

Recommended fix: Keep using reqK8s for namespace retrieval, but add the new permission checks as an additional layer of validation. The original IsForbidden check removal is acceptable since the new permission checks are more specific.

2. Security Gap: Different Resources, Different Permissions

The code now checks projectsettings permissions but fetches the namespace resource. In Kubernetes RBAC, these are separate resources with potentially different permissions:

• A user might have get projectsettings but not get namespaces
• Or vice versa

Recommendation:

• Continue using reqK8s for namespace retrieval (enforces namespace RBAC)
• Keep the new projectsettings checks as additional validation
• This provides defense in depth: both resources must be accessible

📝 Minor Issues

3. Inconsistent Error Messages

The error message "Failed to get project" (line 465 in PR) is returned for multiple scenarios:

• K8sClientProjects == nil
• Namespace retrieval failure

Suggestion: Use more specific messages:

if K8sClientProjects == nil {
    c.JSON(http.StatusInternalServerError, gin.H{"error": "Backend service misconfigured"})
    return
}

4. Missing Test Coverage

No test files found in components/backend/. According to CLAUDE.md:

Testing Strategy > Backend Tests (Go)

• Unit tests (tests/unit/): Isolated component logic
• Contract tests (tests/contract/): API contract validation
• Integration tests (tests/integration/): End-to-end with real k8s cluster

Recommendation: Add permission tests to verify:

• Users with get projectsettings can view projects
• Users without update projectsettings cannot modify/delete
• RBAC boundaries are respected

5. Code Quality: Function Ordering

The deprecated checkUserCanAccessNamespace function is placed after the new functions but should be grouped with them or at the end of the file for clarity.

🔒 Security Considerations

Current Approach Risk

By using the backend service account for namespace retrieval:

1. ✅ You avoid issues where users lack namespace get permissions
2. ❌ You bypass Kubernetes RBAC for a core security boundary
3. ❌ You create a potential privilege escalation vector

Recommended Approach

Use user-scoped client + projectsettings checks:

1. ✅ Honors both namespace RBAC and projectsettings RBAC
2. ✅ Defense in depth
3. ✅ Aligns with CLAUDE.md standards
4. ❌ Requires users to have appropriate namespace permissions (but this is correct behavior)

📋 Pre-Commit Checklist Status

Based on CLAUDE.md Backend checklist:

• Authentication: All endpoints use GetK8sClientsForRequest(c) ✅
• Authorization: ❌ RBAC bypassed by using service account for namespace access
• Error Handling: All errors logged with context ✅
• Token Security: No token leakage ✅
• Type Safety: Proper type usage ✅
• Tests: ❌ No test coverage for new permission checks
• Logging: Structured logs with context ✅
• Code Quality: ❌ Should run golangci-lint

🎯 Recommended Changes

func GetProject(c *gin.Context) {
    projectName := c.Param("projectName")
    reqK8s, _ := GetK8sClientsForRequest(c)

    if reqK8s == nil {
        c.JSON(http.StatusUnauthorized, gin.H{"error": "Invalid or missing token"})
        return
    }

    isOpenShift := isOpenShiftCluster()

    ctx, cancel := context.WithTimeout(context.Background(), defaultK8sTimeout)
    defer cancel()

    // Use user's token for namespace access (enforces namespace RBAC)
    ns, err := reqK8s.CoreV1().Namespaces().Get(ctx, projectName, v1.GetOptions{})
    if err != nil {
        if errors.IsNotFound(err) {
            c.JSON(http.StatusNotFound, gin.H{"error": "Project not found"})
            return
        }
        // Keep this check - it's valid when using user token
        if errors.IsForbidden(err) {
            log.Printf("User forbidden to access Namespace %s: %v", projectName, err)
            c.JSON(http.StatusForbidden, gin.H{"error": "Unauthorized to access project"})
            return
        }
        log.Printf("Failed to get Namespace %s: %v", projectName, err)
        c.JSON(http.StatusInternalServerError, gin.H{"error": "Failed to get project"})
        return
    }

    // Validate it's an Ambient-managed namespace
    if ns.Labels["ambient-code.io/managed"] != "true" {
        log.Printf("SECURITY: User attempted to access non-managed namespace: %s", projectName)
        c.JSON(http.StatusNotFound, gin.H{"error": "Project not found or not an Ambient project"})
        return
    }

    // ADDITIONAL layer: Verify projectsettings access
    canView, err := checkUserCanViewProject(reqK8s, projectName)
    if err != nil {
        log.Printf("GetProject: Failed to check projectsettings access for %s: %v", projectName, err)
        c.JSON(http.StatusInternalServerError, gin.H{"error": "Failed to verify permissions"})
        return
    }

    if !canView {
        log.Printf("User attempted to view project %s without GET projectsettings permission", projectName)
        c.JSON(http.StatusForbidden, gin.H{"error": "Unauthorized to view project"})
        return
    }

    project := projectFromNamespace(ns, isOpenShift)
    c.JSON(http.StatusOK, project)
}

Apply similar pattern to UpdateProject and DeleteProject.

📊 Overall Assessment

Concept: ⭐⭐⭐⭐⭐ Excellent idea to add fine-grained permission checks
Implementation: ⭐⭐⚠️ Good helper functions, but bypasses critical security controls
Adherence to Standards: ⭐⭐ Violates CLAUDE.md authentication requirements

Next Steps

1. Revert namespace retrieval to use reqK8s instead of K8sClientProjects
2. Keep the new permission check functions as additional validation
3. Add test coverage for permission boundaries
4. Run golangci-lint before pushing

Would you like me to provide a complete corrected version of the file, or would you prefer to make these changes yourself?

[github-actions]

Code Review: Enhance Project Access Control

Thank you for improving the security of project operations! This PR enhances RBAC enforcement by using more fine-grained permission checks. Here's my detailed review:

---

✅ Strengths

1. More Granular Permission Checks: Using checkUserCanViewProject (GET projectsettings) and checkUserCanModifyProject (UPDATE projectsettings) is better than the generic namespace access check.

2. Consistent Pattern: The new permission checks follow established patterns in the codebase using SelfSubjectAccessReview.

3. Good Error Logging: Proper logging of unauthorized access attempts aids security auditing.

4. Backward Compatibility: Deprecated checkUserCanAccessNamespace maintains compatibility while redirecting to new function.

---

⚠️ Critical Issues

1. VIOLATED: User Token Authentication Required (CLAUDE.md Rule #1)

Problem: GetProject, UpdateProject, and DeleteProject now use K8sClientProjects (backend service account) to retrieve namespaces instead of the user's token (reqK8s).

Current code (lines 463-479):

// ❌ WRONG: Using backend SA for namespace retrieval
ns, err := K8sClientProjects.CoreV1().Namespaces().Get(ctx, projectName, v1.GetOptions{})

Why this is a security issue:

• CLAUDE.md states: "FORBIDDEN: Using backend service account for user-initiated API operations"
• CLAUDE.md states: "REQUIRED: Always use GetK8sClientsForRequest(c) to get user-scoped K8s clients"
• The backend SA can access all namespaces, bypassing user RBAC entirely
• You then check permissions after retrieving the namespace, which is backwards
• An unauthorized user could trigger errors that reveal namespace existence

Correct pattern (from CLAUDE.md and existing code):

// ✅ CORRECT: Use user's token for namespace retrieval
ns, err := reqK8s.CoreV1().Namespaces().Get(ctx, projectName, v1.GetOptions{})
if err != nil {
    if errors.IsNotFound(err) {
        c.JSON(http.StatusNotFound, gin.H{"error": "Project not found"})
        return
    }
    if errors.IsForbidden(err) {
        log.Printf("User forbidden to access Namespace %s: %v", projectName, err)
        c.JSON(http.StatusForbidden, gin.H{"error": "Unauthorized to access project"})
        return
    }
    log.Printf("Failed to get Namespace %s: %v", projectName, err)
    c.JSON(http.StatusInternalServerError, gin.H{"error": "Failed to get project"})
    return
}
// Now you already know user has namespace access via RBAC
// Additional projectsettings checks are fine-grained validation

Why the old code was better:

• The original code used reqK8s (user token) for namespace retrieval
• Kubernetes RBAC engine checked permissions during the operation
• errors.IsForbidden(err) naturally caught unauthorized access
• No namespace data leaked to unauthorized users

Recommendation: REVERT the switch from reqK8s to K8sClientProjects for namespace GET operations. Keep the additional checkUserCanViewProject/checkUserCanModifyProject checks for fine-grained validation, but always retrieve the namespace with the user's token first.

---

2. Incorrect Permission Check Timing

Current flow:

1. Backend SA retrieves namespace (bypasses user RBAC) ✗
2. Validate namespace is Ambient-managed ✓
3. Check if user can view/modify projectsettings ✓

Correct flow (Defense in Depth):

1. User token retrieves namespace (enforces namespace-level RBAC) ✓
2. Validate namespace is Ambient-managed ✓
3. Check if user can view/modify projectsettings (fine-grained validation) ✓

The permission check should be a second layer of validation, not the only layer.

---

🔧 Recommended Changes

For GetProject (line 465):

- ns, err := K8sClientProjects.CoreV1().Namespaces().Get(ctx, projectName, v1.GetOptions{})
+ ns, err := reqK8s.CoreV1().Namespaces().Get(ctx, projectName, v1.GetOptions{})
  if err != nil {
      if errors.IsNotFound(err) {
          c.JSON(http.StatusNotFound, gin.H{"error": "Project not found"})
          return
      }
+     if errors.IsForbidden(err) {
+         log.Printf("User forbidden to access Namespace %s: %v", projectName, err)
+         c.JSON(http.StatusForbidden, gin.H{"error": "Unauthorized to access project"})
+         return
+     }
      log.Printf("Failed to get Namespace %s: %v", projectName, err)
      c.JSON(http.StatusInternalServerError, gin.H{"error": "Failed to get project"})
      return
  }

For UpdateProject (line 525):

- ns, err := K8sClientProjects.CoreV1().Namespaces().Get(ctx, projectName, v1.GetOptions{})
+ ns, err := reqK8s.CoreV1().Namespaces().Get(ctx, projectName, v1.GetOptions{})
  // Same error handling as GetProject

For DeleteProject (line 606):

Keep using K8sClientProjects here since you're already checking permissions with checkUserCanModifyProject before this point, and the actual deletion needs elevated permissions.

---

📝 Minor Issues

3. Missing Nil Check Removal

Lines 464-467 and 534-537 still have the K8sClientProjects == nil check, but this should be checked once at handler initialization, not on every request.

Recommendation: Remove these checks or handle at initialization time.

---

4. Deprecated Function Implementation

Line 740 (deprecated checkUserCanAccessNamespace):

func checkUserCanAccessNamespace(userClient *kubernetes.Clientset, namespace string) (bool, error) {
    return checkUserCanViewProject(userClient, namespace)
}

This maintains backward compatibility, which is good. Consider adding a timeline for removal in the comment.

---

🧪 Testing Recommendations

1. Add unit tests for the new permission check functions:

   • checkUserCanViewProject
   • checkUserCanModifyProject

2. Add integration tests (components/backend/tests/integration/):

   • User with namespace access but no projectsettings access
   • User with projectsettings GET but not UPDATE
   • User with no access attempting operations

3. Add permission tests (components/backend/tests/permissions/):

   • Verify user can't access projects in other namespaces
   • Verify backend SA isn't used for user operations

Example test structure:

func TestGetProject_UnauthorizedUser(t *testing.T) {
    // Setup: User with no namespace access
    // Call: GetProject
    // Assert: 403 Forbidden (not 404)
}

func TestGetProject_CanViewButNotModify(t *testing.T) {
    // Setup: User with GET projectsettings only
    // Call: GetProject → expect 200
    // Call: UpdateProject → expect 403
}

---

📋 Pre-Commit Checklist (from CLAUDE.md)

Please verify:

• Authentication: All endpoints use GetK8sClientsForRequest(c) for user operations ❌ (violated)
• Authorization: RBAC checks performed before resource access ⚠️ (after, not before)
• Error Handling: All errors logged with context ✅
• Token Security: No tokens in logs ✅
• Tests: Added/updated tests ❌ (none found)
• Code Quality: Ran gofmt, go vet, golangci-lint ⚠️ (please confirm)

---

🎯 Summary

Main Issue: The PR switches from user-token-based namespace retrieval to backend-SA-based retrieval, which violates CLAUDE.md Rule #1 and weakens security.

Solution: Keep using reqK8s for namespace GET operations, and use the new checkUserCanViewProject/checkUserCanModifyProject functions as additional fine-grained validation.

Overall Assessment: The intent is excellent (more granular permissions), but the implementation inadvertently introduces a security regression by bypassing user RBAC during namespace retrieval.

---

📚 References

• CLAUDE.md: "Backend and Operator Development Standards" section
• CLAUDE.md Rule Outcome: Reduce Refinement Time with agent System #1: "User Token Authentication Required"
• Existing code pattern: components/backend/handlers/sessions.go:227 (proper user/SA client usage)

Please let me know if you'd like me to prepare a corrected version or if you have questions about the recommended approach!

[sallyom]

LGTM