chore: Ensure Authorized SQL clause

Emyrk · Emyrk · commit bba2cdbe94d5 · 2022-11-04T16:37:20.000-05:00
In authorized database calls we want to ensure the authorized
sql string is always present. If it is not, the sql query
has no Authorized filter. This is just a safeguard
diff --git a/coderd/database/modelqueries.go b/coderd/database/modelqueries.go
@@ -121,7 +121,10 @@ type workspaceQuerier interface {
 func (q *sqlQuerier) GetAuthorizedWorkspaces(ctx context.Context, arg GetWorkspacesParams, authorizedFilter rbac.AuthorizeFilter) ([]Workspace, error) {
 	// 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(rbac.NoACLConfig())), 1)
+	filter, err := insertAuthorizedFilter(getWorkspaceCount, authorizedFilter, rbac.NoACLConfig())
+	if err != nil {
+		return nil, err
+	}
 	// The name comment is for metric tracking
 	query := fmt.Sprintf("-- name: GetAuthorizedWorkspaces :many\n%s", filter)
 	rows, err := q.db.QueryContext(ctx, query,
@@ -171,7 +174,10 @@ func (q *sqlQuerier) GetAuthorizedWorkspaces(ctx context.Context, arg GetWorkspa
 func (q *sqlQuerier) GetAuthorizedWorkspaceCount(ctx context.Context, arg GetWorkspaceCountParams, authorizedFilter rbac.AuthorizeFilter) (int64, error) {
 	// 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(getWorkspaceCount, "-- @authorize_filter", fmt.Sprintf(" AND %s", authorizedFilter.SQLString(rbac.NoACLConfig())), 1)
+	filter, err := insertAuthorizedFilter(getWorkspaceCount, authorizedFilter, rbac.NoACLConfig())
+	if err != nil {
+		return -1, err
+	}
 	// The name comment is for metric tracking
 	query := fmt.Sprintf("-- name: GetAuthorizedWorkspaceCount :one\n%s", filter)
 	row := q.db.QueryRowContext(ctx, query,
@@ -184,6 +190,19 @@ func (q *sqlQuerier) GetAuthorizedWorkspaceCount(ctx context.Context, arg GetWor
 		arg.Name,
 	)
 	var count int64
-	err := row.Scan(&count)
+	err = row.Scan(&count)
 	return count, err
 }
+
+// insertAuthorizedFilter is used to replace the @authorized_filter placeholder.
+// It is crucial the placeholder exists, otherwise no auth checks are done. This
+// is to prevent that mistake. If this function returns an error, it is
+// a developer error.
+func insertAuthorizedFilter(query string, authorizedFilter rbac.AuthorizeFilter, config rbac.SQLConfig) (string, error) {
+	replace := "-- @authorize_filter"
+	if !strings.Contains(query, replace) {
+		return "", xerrors.Errorf("query does not contain authorized replace string, this is not an authorized query")
+	}
+	filter := strings.Replace(getWorkspaces, "-- @authorize_filter", fmt.Sprintf(" AND %s", authorizedFilter.SQLString(config)), 1)
+	return filter, nil
+}
diff --git a/coderd/database/modelqueries_internal_test.go b/coderd/database/modelqueries_internal_test.go
@@ -0,0 +1,14 @@
+package database
+
+import (
+	"testing"
+
+	"github.com/coder/coder/coderd/rbac"
+	"github.com/stretchr/testify/require"
+)
+
+func TestInsertAuthorized(t *testing.T) {
+	query := `SELECT true;`
+	_, err := insertAuthorizedFilter(query, nil, rbac.NoACLConfig())
+	require.ErrorContains(t, err, "does not contain authorized replace string", "ensure replace string")
+}
