Notebooks 2.0 // Backend // Refactor Auth Proposal

[Adembc]

Summary

After reviewing auth logic in #202, I propose refactoring the auth validation mechanism to improve structure, maintainability, and security. The current approach requires calling a.requireAuth(w, r, authPolicies) manually in every (protected) handler, which can lead to security risks if a developer forgets to include it.

Current Issue

The current approach requires explicit authentication checks in each protected handler:

// =========================== AUTH ===========================  
authPolicies := []*auth.ResourcePolicy{  
    auth.NewResourcePolicy(  
       auth.ResourceVerbGet,  
       &kubefloworgv1beta1.WorkspaceKind{  
          ObjectMeta: metav1.ObjectMeta{Name: name},  
       },  
    ),  
}  
if success := a.requireAuth(w, r, authPolicies); !success {  
    return  
}  
// ============================================================

This approach has several drawbacks:

• Requires developers to manually insert auth checks in every handler.
• If omitted, it could result in security issues.
• Authentication logic is scattered across multiple handlers instead of being centrally managed.

Proposed Solution

Introduce a middleware-based approach that enforces authentication centrally. The middleware will:

1. Define authentication policies for all routes in a single place.
2. Automatically apply auth validation based on predefined policies.
3. Provide an option to skip authentication for specific routes (e.g., /healthcheck).
4. Ensure that if a route lacks a defined policy, an internal error is returned to avoid accidental unprotected routes.

Policy Definition

type ResourceValueFunc func(ps httprouter.Params) string  
  
func GetNamespaceFromParam(ps httprouter.Params) string {  
    return ps.ByName(NamespacePathParam)  
}  
  
type AuthSpec struct {  
    SkipAuth bool  
    Policies []*Policy  
}  
  
type Policy struct {  
    Verb                  auth.ResourceVerb  
    Object                client.Object  
    ResourceNameFunc      ResourceValueFunc  
    ResourceNamespaceFunc ResourceValueFunc  
}  
  
var RoutesAuthPolicies = map[string]map[string]*AuthSpec{  
    HealthCheckPath: {  
       http.MethodGet: {  
          SkipAuth: true,  
       },  
    },  
    AllWorkspacesPath: {  
       http.MethodGet: {  
          Policies: []*Policy{  
             {  
                Verb:   auth.ResourceVerbList,  
                Object: &kubefloworgv1beta1.Workspace{},  
             },  
          },  
          SkipAuth: false, // could be removed as it is the default value  
       },  
    },  
    WorkspacesByNamespacePath: {  
       http.MethodGet: {  
          Policies: []*Policy{  
             {  
                Verb:                  auth.ResourceVerbList,  
                Object:                &kubefloworgv1beta1.Workspace{},  
                ResourceNamespaceFunc: GetNamespaceFromParam,  
             },  
          },  
       },  
    },  
}

Middleware Implementation

unc (a *App) validateUserGrants(next http.Handler) http.Handler {  
    return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {  
  
       // Get method and path from request  
       method := r.Method  
       // Will be replaced by path pattern (e.g. /workspace/:name) instead of the path (/workspace/test)  
       path := r.URL.Path  
  
       grants, ok := RoutesAuthPolicies[path]  
       if !ok {  
          err := fmt.Errorf("no policies found for this path: %s", path)  
          a.serverErrorResponse(w, r, err)  
          return  
       }  
       methodGrant, ok := grants[method]  
       if !ok {  
          err := fmt.Errorf("no policies found for this path %s and method: %s", path, method)  
          a.serverErrorResponse(w, r, err)  
          return  
       }  
       if methodGrant.SkipAuth {  
          next.ServeHTTP(w, r)  
          return  
       }  
  
       policies := make([]*auth.ResourcePolicy, len(methodGrant.Policies))  
       for i, policy := range methodGrant.Policies {  
          if policy.ResourceNameFunc != nil {  
             resourceName := policy.ResourceNameFunc(httprouter.ParamsFromContext(r.Context()))  
             policy.Object.SetName(resourceName)  
          }  
          if policy.ResourceNamespaceFunc != nil {  
             resourceNamespace := policy.ResourceNamespaceFunc(httprouter.ParamsFromContext(r.Context()))  
             policy.Object.SetNamespace(resourceNamespace)  
          }  
          policies[i] = auth.NewResourcePolicy(policy.Verb, policy.Object)  
       }  
  
       if success := a.requireAuth(w, r, policies); !success {  
          return  
       }  
  
       next.ServeHTTP(w, r)  
    })  
}

Middleware Usage

return a.recoverPanic(a.enableCORS(a.validateUserGrants(router)))

Note: This code is for demo purposes and will be improved.

Benefits of This Approach

1. Improved Security: Eliminates the risk of forgetting to add auth validation in handlers.
2. Centralized Policy Management: All auth policies are defined in a single place.
3. Better Maintainability: Easier to modify policies without modifying individual handlers.
4. Failsafe for Undefined Routes: Ensures that all routes require an explicit auth policy, preventing accidental exposure.

Next Steps

If this proposal is acceptable, I will raise a PR to implement this refactor. Let me know your thoughts!

[Adembc]

CC: @thesuperzapper , @ederign

[github-actions]

This issue has been automatically marked as stale because it has not had recent activity.
It will be closed if no further activity occurs.
Thank you for your contributions.

Members may comment /lifecycle frozen to prevent this issue from being marked as stale.

[github-actions]

This issue has been automatically closed because it has not had recent activity.

Members may comment /reopen to reopen it.