test: Add unit test verifying acl behavior against all normal cases.

- bug: OrgAdmin could not make new groups - Also refactor some function names
coder · Emyrk · Nov 28, 2022 · Nov 20, 2022 · Nov 20, 2022 · Nov 20, 2022
commit a0790b04004904cbfe542dd82d667def79a9430a
diff --git a/coderd/coderdtest/authorize.go b/coderd/coderdtest/authorize.go
@@ -551,10 +551,10 @@ func (f *fakePreparedAuthorizer) Authorize(ctx context.Context, object rbac.Obje
 	return f.Original.ByRoleName(ctx, f.SubjectID, f.Roles, f.Scope, f.Groups, f.Action, object)
 }
 
-// Compile returns a compiled version of the authorizer that will work for
+// CompileToSQL returns a compiled version of the authorizer that will work for
 // in memory databases. This fake version will not work against a SQL database.
-func (f *fakePreparedAuthorizer) Compile(_ regosql.ConvertConfig) (rbac.AuthorizeFilter, error) {
-	return f, nil
+func (f *fakePreparedAuthorizer) CompileToSQL(_ regosql.ConvertConfig) (string, error) {
+	return "", fmt.Errorf("not implemented")
 }
 
 func (f *fakePreparedAuthorizer) Eval(object rbac.Object) bool {
@@ -567,10 +567,3 @@ func (f fakePreparedAuthorizer) RegoString() string {
 	}
 	panic("not implemented")
 }
-
-func (f fakePreparedAuthorizer) SQLString() string {
-	if f.HardCodedSQLString != "" {
-		return f.HardCodedSQLString
-	}
-	panic("not implemented")
-}
diff --git a/coderd/database/databasefake/databasefake.go b/coderd/database/databasefake/databasefake.go
@@ -488,11 +488,20 @@ func (q *fakeQuerier) GetFilteredUserCount(ctx context.Context, arg database.Get
 	return count, err
 }
 
-func (q *fakeQuerier) GetAuthorizedUserCount(_ context.Context, params database.GetFilteredUserCountParams, prepared rbac.PreparedAuthorized) (int64, error) {
+func (q *fakeQuerier) GetAuthorizedUserCount(ctx context.Context, params database.GetFilteredUserCountParams, prepared rbac.PreparedAuthorized) (int64, error) {
 	q.mutex.RLock()
 	defer q.mutex.RUnlock()
 
-	users := append([]database.User{}, q.users...)
+	users := make([]database.User, 0, len(q.users))
+
+	for _, user := range q.users {
+		// If the filter exists, ensure the object is authorized.
+		if prepared != nil && prepared.Authorize(ctx, user.RBACObject()) != nil {
+			continue
+		}
+
+		users = append(users, user)
+	}
 
 	if params.Deleted {
 		tmp := make([]database.User, 0, len(users))
@@ -539,22 +548,6 @@ func (q *fakeQuerier) GetAuthorizedUserCount(_ context.Context, params database.
 		users = usersFilteredByRole
 	}
 
-	var authorizedFilter rbac.AuthorizeFilter
-	var err error
-	if prepared != nil {
-		authorizedFilter, err = prepared.Compile(rbac.ConfigWithoutACL())
-		if err != nil {
-			return -1, xerrors.Errorf("compile authorized filter: %w", err)
-		}
-	}
-
-	for _, user := range q.workspaces {
-		// If the filter exists, ensure the object is authorized.
-		if authorizedFilter != nil && !authorizedFilter.Eval(user.RBACObject()) {
-			continue
-		}
-	}
-
 	return int64(len(users)), nil
 }
 
@@ -763,11 +756,6 @@ func (q *fakeQuerier) GetAuthorizedWorkspaces(ctx context.Context, arg database.
 	q.mutex.RLock()
 	defer q.mutex.RUnlock()
 
-	filter, err := prepared.Compile(rbac.ConfigWithoutACL())
-	if err != nil {
-		return nil, xerrors.Errorf("compile authorized filter: %w", err)
-	}
-
 	workspaces := make([]database.Workspace, 0)
 	for _, workspace := range q.workspaces {
 		if arg.OwnerID != uuid.Nil && workspace.OwnerID != arg.OwnerID {
@@ -899,7 +887,7 @@ func (q *fakeQuerier) GetAuthorizedWorkspaces(ctx context.Context, arg database.
 		}
 
 		// If the filter exists, ensure the object is authorized.
-		if filter != nil && !filter.Eval(workspace.RBACObject()) {
+		if prepared != nil && prepared.Authorize(ctx, workspace.RBACObject()) != nil {
 			continue
 		}
 		workspaces = append(workspaces, workspace)
@@ -1447,12 +1435,20 @@ func (q *fakeQuerier) UpdateTemplateMetaByID(_ context.Context, arg database.Upd
 	return database.Template{}, sql.ErrNoRows
 }
 
-func (q *fakeQuerier) GetTemplatesWithFilter(_ context.Context, arg database.GetTemplatesWithFilterParams) ([]database.Template, error) {
+func (q *fakeQuerier) GetTemplatesWithFilter(ctx context.Context, arg database.GetTemplatesWithFilterParams) ([]database.Template, error) {
+	return q.GetAuthorizedTemplates(ctx, arg, nil)
+}
+
+func (q *fakeQuerier) GetAuthorizedTemplates(ctx context.Context, arg database.GetTemplatesWithFilterParams, prepared rbac.PreparedAuthorized) ([]database.Template, error) {
 	q.mutex.RLock()
 	defer q.mutex.RUnlock()
 
 	var templates []database.Template
 	for _, template := range q.templates {
+		if prepared != nil && prepared.Authorize(ctx, template.RBACObject()) != nil {
+			continue
+		}
+
 		if template.Deleted != arg.Deleted {
 			continue
 		}

diff --git a/coderd/database/modelqueries.go b/coderd/database/modelqueries.go
@@ -23,10 +23,63 @@ type customQuerier interface {
 }
 
 type templateQuerier interface {
+	GetAuthorizedTemplates(ctx context.Context, arg GetTemplatesWithFilterParams, prepared rbac.PreparedAuthorized) ([]Template, error)
 	GetTemplateGroupRoles(ctx context.Context, id uuid.UUID) ([]TemplateGroup, error)
 	GetTemplateUserRoles(ctx context.Context, id uuid.UUID) ([]TemplateUser, error)
 }
 
+func (q *sqlQuerier) GetAuthorizedTemplates(ctx context.Context, arg GetTemplatesWithFilterParams, prepared rbac.PreparedAuthorized) ([]Template, error) {
+	authorizedFilter, err := prepared.CompileToSQL(rbac.ConfigWithACL())
+	if err != nil {
+		return nil, xerrors.Errorf("compile authorized filter: %w", err)
+	}
+
+	filter := strings.Replace(getTemplatesWithFilter, "-- @authorize_filter", fmt.Sprintf(" AND %s", authorizedFilter), 1)
+	// The name comment is for metric tracking
+	query := fmt.Sprintf("-- name: GetAuthorizedTemplates :many\n%s", filter)
+	rows, err := q.db.QueryContext(ctx, query,
+		arg.Deleted,
+		arg.OrganizationID,
+		arg.ExactName,
+		pq.Array(arg.IDs),
+	)
+	if err != nil {
+		return nil, err
+	}
+	defer rows.Close()
+	var items []Template
+	for rows.Next() {
+		var i Template
+		if err := rows.Scan(
+			&i.ID,
+			&i.CreatedAt,
+			&i.UpdatedAt,
+			&i.OrganizationID,
+			&i.Deleted,
+			&i.Name,
+			&i.Provisioner,
+			&i.ActiveVersionID,
+			&i.Description,
+			&i.DefaultTTL,
+			&i.CreatedBy,
+			&i.Icon,
+			&i.UserACL,
+			&i.GroupACL,
+			&i.DisplayName,
+		); err != nil {
+			return nil, err
+		}
+		items = append(items, i)
+	}
+	if err := rows.Close(); err != nil {
+		return nil, err
+	}
+	if err := rows.Err(); err != nil {
+		return nil, err
+	}
+	return items, nil
+}
+
 type TemplateUser struct {
 	User
 	Actions Actions `db:"actions"`
@@ -119,14 +172,14 @@ type workspaceQuerier interface {
 // This code is copied from `GetWorkspaces` and adds the authorized filter WHERE
 // clause.
 func (q *sqlQuerier) GetAuthorizedWorkspaces(ctx context.Context, arg GetWorkspacesParams, prepared rbac.PreparedAuthorized) ([]GetWorkspacesRow, error) {
-	authorizedFilter, err := prepared.Compile(rbac.ConfigWithoutACL())
+	authorizedFilter, err := prepared.CompileToSQL(rbac.ConfigWithoutACL())
 	if err != nil {
 		return nil, xerrors.Errorf("compile authorized filter: %w", err)
 	}
 
 	// In order to properly use ORDER BY, OFFSET, and LIMIT, we need to inject the
 	// authorizedFilter between the end of the where clause and those statements.
-	filter := strings.Replace(getWorkspaces, "-- @authorize_filter", fmt.Sprintf(" AND %s", authorizedFilter.SQLString()), 1)
+	filter := strings.Replace(getWorkspaces, "-- @authorize_filter", fmt.Sprintf(" AND %s", authorizedFilter), 1)
 	// The name comment is for metric tracking
 	query := fmt.Sprintf("-- name: GetAuthorizedWorkspaces :many\n%s", filter)
 	rows, err := q.db.QueryContext(ctx, query,
@@ -179,12 +232,12 @@ type userQuerier interface {
 }
 
 func (q *sqlQuerier) GetAuthorizedUserCount(ctx context.Context, arg GetFilteredUserCountParams, prepared rbac.PreparedAuthorized) (int64, error) {
-	authorizedFilter, err := prepared.Compile(rbac.ConfigWithoutACL())
+	authorizedFilter, err := prepared.CompileToSQL(rbac.ConfigWithoutACL())
 	if err != nil {
 		return -1, xerrors.Errorf("compile authorized filter: %w", err)
 	}
 
-	filter := strings.Replace(getFilteredUserCount, "-- @authorize_filter", fmt.Sprintf(" AND %s", authorizedFilter.SQLString()), 1)
+	filter := strings.Replace(getFilteredUserCount, "-- @authorize_filter", fmt.Sprintf(" AND %s", authorizedFilter), 1)
 	query := fmt.Sprintf("-- name: GetAuthorizedUserCount :one\n%s", filter)
 	row := q.db.QueryRowContext(ctx, query,
 		arg.Deleted,

diff --git a/coderd/database/queries.sql.go b/coderd/database/queries.sql.go
diff --git a/coderd/database/queries/templates.sql b/coderd/database/queries/templates.sql
@@ -34,6 +34,8 @@ WHERE
 			id = ANY(@ids)
 		ELSE true
 	END
+  -- Authorize Filter clause will be injected below in GetAuthorizedTemplates
+  -- @authorize_filter
 ORDER BY (name, id) ASC
 ;
 

diff --git a/coderd/rbac/authz.go b/coderd/rbac/authz.go
@@ -21,7 +21,7 @@ type Authorizer interface {
 
 type PreparedAuthorized interface {
 	Authorize(ctx context.Context, object Object) error
-	Compile(cfg regosql.ConvertConfig) (AuthorizeFilter, error)
+	CompileToSQL(cfg regosql.ConvertConfig) (string, error)
 }
 
 // Filter takes in a list of objects, and will filter the list removing all

diff --git a/coderd/rbac/partial.go b/coderd/rbac/partial.go
@@ -29,12 +29,12 @@ type PartialAuthorizer struct {
 
 var _ PreparedAuthorized = (*PartialAuthorizer)(nil)
 
-func (pa *PartialAuthorizer) Compile(cfg regosql.ConvertConfig) (AuthorizeFilter, error) {
+func (pa *PartialAuthorizer) CompileToSQL(cfg regosql.ConvertConfig) (string, error) {
 	filter, err := Compile(cfg, pa)
 	if err != nil {
-		return nil, xerrors.Errorf("compile: %w", err)
+		return "", xerrors.Errorf("compile: %w", err)
 	}
-	return filter, nil
+	return filter.SQLString(), nil
 }
 
 func (pa *PartialAuthorizer) Authorize(ctx context.Context, object Object) error {

diff --git a/coderd/rbac/query.go b/coderd/rbac/query.go
@@ -14,9 +14,6 @@ import (
 
 type AuthorizeFilter interface {
 	SQLString() string
-	// Eval is required for the fake in memory database to work. The in memory
-	// database can use this function to filter the results.
-	Eval(object Object) bool
 }
 
 type authorizedSQLFilter struct {

diff --git a/enterprise/coderd/groups.go b/enterprise/coderd/groups.go
@@ -31,7 +31,7 @@ func (api *API) postGroupByOrganization(rw http.ResponseWriter, r *http.Request)
 	)
 	defer commitAudit()
 
-	if !api.Authorize(r, rbac.ActionCreate, rbac.ResourceGroup) {
+	if !api.Authorize(r, rbac.ActionCreate, rbac.ResourceGroup.InOrg(org.ID)) {
 		http.NotFound(rw, r)
 		return
 	}