Remove old Filter

coder · Emyrk · Aug 11, 2022 · Aug 10, 2022 · Aug 10, 2022 · Aug 10, 2022
commit bed9f4faf23823d87c63e6de9544ade22a6b6bae
diff --git a/coderd/authorize.go b/coderd/authorize.go
@@ -10,9 +10,27 @@ import (
 	"github.com/coder/coder/coderd/rbac"
 )
 
-func AuthorizeFilter[O rbac.Objecter](api *API, r *http.Request, action rbac.Action, objects []O) []O {
+func AuthorizeFilter[O rbac.Objecter](api *API, r *http.Request, action rbac.Action, objects []O) ([]O, error) {
 	roles := httpmw.AuthorizationUserRoles(r)
-	return rbac.Filter(r.Context(), api.Authorizer, roles.ID.String(), roles.Roles, action, objects)
+
+	if len(objects) == 0 {
+		return objects, nil
+	}
+	objecType := objects[0].RBACObject().Type
+	objects, err := rbac.Filter(r.Context(), api.Authorizer, roles.ID.String(), roles.Roles, action, objecType, objects)
+	if err != nil {
+		api.Logger.Error(r.Context(), "filter failed",
+			slog.Error(err),
+			slog.F("object_type", objecType),
+			slog.F("user_id", roles.ID),
+			slog.F("username", roles.Username),
+			slog.F("route", r.URL.Path),
+			slog.F("action", action),
+		)
+		// Hide the underlying error in case it has sensitive information
+		return nil, xerrors.Errorf("failed to filter requested objects")
+	}
+	return objects, nil
 }
 
 // Authorize will return false if the user is not authorized to do the action.

diff --git a/coderd/provisionerdaemons.go b/coderd/provisionerdaemons.go
@@ -50,7 +50,14 @@ func (api *API) provisionerDaemons(rw http.ResponseWriter, r *http.Request) {
 	if daemons == nil {
 		daemons = []database.ProvisionerDaemon{}
 	}
-	daemons = AuthorizeFilter(api, r, rbac.ActionRead, daemons)
+	daemons, err = AuthorizeFilter(api, r, rbac.ActionRead, daemons)
+	if err != nil {
+		httpapi.Write(rw, http.StatusInternalServerError, codersdk.Response{
+			Message: "Internal error fetching provisioner daemons.",
+			Detail:  err.Error(),
+		})
+		return
+	}
 
 	httpapi.Write(rw, http.StatusOK, daemons)
 }

diff --git a/coderd/rbac/authz.go b/coderd/rbac/authz.go
@@ -19,24 +19,9 @@ type PreparedAuthorized interface {
 }
 
 // Filter takes in a list of objects, and will filter the list removing all
-// the elements the subject does not have permission for.
-// Filter does not allocate a new slice, and will use the existing one
-// passed in. This can cause memory leaks if the slice is held for a prolonged
-// period of time.
-func Filter[O Objecter](ctx context.Context, auth Authorizer, subjID string, subjRoles []string, action Action, objects []O) []O {
-	filtered := make([]O, 0)
-
-	for i := range objects {
-		object := objects[i]
-		err := auth.ByRoleName(ctx, subjID, subjRoles, action, object.RBACObject())
-		if err == nil {
-			filtered = append(filtered, object)
-		}
-	}
-	return filtered
-}
-
-func FilterPart[O Objecter](ctx context.Context, auth Authorizer, subjID string, subjRoles []string, action Action, objectType string, objects []O) ([]O, error) {
+// the elements the subject does not have permission for. All objects must be
+// of the same type.
+func Filter[O Objecter](ctx context.Context, auth Authorizer, subjID string, subjRoles []string, action Action, objectType string, objects []O) ([]O, error) {
 	filtered := make([]O, 0)
 	prepared, err := auth.PrepareByRoleName(ctx, subjID, subjRoles, action, objectType)
 	if err != nil {

diff --git a/coderd/rbac/authz_test.go b/coderd/rbac/authz_test.go
@@ -132,7 +132,7 @@ func TestFilter(t *testing.T) {
 			}
 
 			// Run by filter
-			list, err := rbac.FilterPart(ctx, auth, tc.SubjectID, tc.Roles, tc.Action, tc.ObjectType, localObjects)
+			list, err := rbac.Filter(ctx, auth, tc.SubjectID, tc.Roles, tc.Action, tc.ObjectType, localObjects)
 			require.NoError(t, err)
 			require.Equal(t, allowedCount, len(list), "expected number of allowed")
 			for _, obj := range list {

diff --git a/coderd/rbac/builtin_test.go b/coderd/rbac/builtin_test.go
@@ -73,10 +73,10 @@ func BenchmarkRBACFilter(b *testing.B) {
 		require.NoError(b, err)
 	}
 	for _, c := range benchCases {
-		b.Run(c.Name+"Queries", func(b *testing.B) {
+		b.Run(c.Name, func(b *testing.B) {
 			objects := benchmarkSetup(orgs, users, b.N)
 			b.ResetTimer()
-			allowed, err := rbac.FilterPart(context.Background(), authorizer, c.UserID.String(), c.Roles, rbac.ActionRead, objects[0].Type, objects)
+			allowed, err := rbac.Filter(context.Background(), authorizer, c.UserID.String(), c.Roles, rbac.ActionRead, objects[0].Type, objects)
 			require.NoError(b, err)
 			var _ = allowed
 		})

diff --git a/coderd/templates.go b/coderd/templates.go
@@ -273,7 +273,14 @@ func (api *API) templatesByOrganization(rw http.ResponseWriter, r *http.Request)
 	}
 
 	// Filter templates based on rbac permissions
-	templates = AuthorizeFilter(api, r, rbac.ActionRead, templates)
+	templates, err = AuthorizeFilter(api, r, rbac.ActionRead, templates)
+	if err != nil {
+		httpapi.Write(rw, http.StatusInternalServerError, codersdk.Response{
+			Message: "Internal error fetching templates.",
+			Detail:  err.Error(),
+		})
+		return
+	}
 
 	templateIDs := make([]uuid.UUID, 0, len(templates))
 

diff --git a/coderd/users.go b/coderd/users.go
@@ -155,7 +155,15 @@ func (api *API) users(rw http.ResponseWriter, r *http.Request) {
 		return
 	}
 
-	users = AuthorizeFilter(api, r, rbac.ActionRead, users)
+	users, err = AuthorizeFilter(api, r, rbac.ActionRead, users)
+	if err != nil {
+		httpapi.Write(rw, http.StatusInternalServerError, codersdk.Response{
+			Message: "Internal error fetching users.",
+			Detail:  err.Error(),
+		})
+		return
+	}
+
 	userIDs := make([]uuid.UUID, 0, len(users))
 	for _, user := range users {
 		userIDs = append(userIDs, user.ID)
@@ -489,7 +497,14 @@ func (api *API) userRoles(rw http.ResponseWriter, r *http.Request) {
 	}
 
 	// Only include ones we can read from RBAC.
-	memberships = AuthorizeFilter(api, r, rbac.ActionRead, memberships)
+	memberships, err = AuthorizeFilter(api, r, rbac.ActionRead, memberships)
+	if err != nil {
+		httpapi.Write(rw, http.StatusInternalServerError, codersdk.Response{
+			Message: "Internal error fetching memberships.",
+			Detail:  err.Error(),
+		})
+		return
+	}
 
 	for _, mem := range memberships {
 		// If we can read the org member, include the roles.
@@ -610,7 +625,14 @@ func (api *API) organizationsByUser(rw http.ResponseWriter, r *http.Request) {
 	}
 
 	// Only return orgs the user can read.
-	organizations = AuthorizeFilter(api, r, rbac.ActionRead, organizations)
+	organizations, err = AuthorizeFilter(api, r, rbac.ActionRead, organizations)
+	if err != nil {
+		httpapi.Write(rw, http.StatusInternalServerError, codersdk.Response{
+			Message: "Internal error fetching organizations.",
+			Detail:  err.Error(),
+		})
+		return
+	}
 
 	publicOrganizations := make([]codersdk.Organization, 0, len(organizations))
 	for _, organization := range organizations {

diff --git a/coderd/workspaces.go b/coderd/workspaces.go
@@ -145,7 +145,14 @@ func (api *API) workspaces(rw http.ResponseWriter, r *http.Request) {
 	}
 
 	// Only return workspaces the user can read
-	workspaces = AuthorizeFilter(api, r, rbac.ActionRead, workspaces)
+	workspaces, err = AuthorizeFilter(api, r, rbac.ActionRead, workspaces)
+	if err != nil {
+		httpapi.Write(rw, http.StatusInternalServerError, codersdk.Response{
+			Message: "Internal error fetching workspaces.",
+			Detail:  err.Error(),
+		})
+		return
+	}
 
 	apiWorkspaces, err := convertWorkspaces(r.Context(), api.Database, workspaces)
 	if err != nil {