Policies are now unique for (team_id, name). (#16501)

#13643 

Updating the `policies` table to use a checksum column for uniqueness.
The checksum is computed with team_id (which may be null) and name. This
change is modeled on the checksum in the software table.

# Checklist for submitter

If some of the following don't apply, delete the relevant line.

<!-- Note that API documentation changes are now addressed by the
product design team. -->

- [x] Changes file added for user-visible changes in `changes/` or
`orbit/changes/`.
See [Changes
files](https://fleetdm.com/docs/contributing/committing-changes#changes-files)
for more information.
- [x] Added/updated tests
- [x] If database migrations are included, checked table schema to
confirm autoupdate
- For database migrations:
- [x] Checked schema for all modified table for columns that will
auto-update timestamps during migration.
- [x] Confirmed that updating the timestamps is acceptable, and will not
cause unwanted side effects.
- [x] Manual QA for all new/changed functionality

changes/13643-policy-name-uniqueness

@@ -0,0 +1 @@
+Policy names are now unique per team -- different teams can have policies with the same name.

server/datastore/mysql/hosts.go

@@ -2824,7 +2824,7 @@ func (ds *Datastore) ListPoliciesForHost(ctx context.Context, host *fleet.Host)
 		// We log to help troubleshooting in case this happens.
 		level.Error(ds.logger).Log("err", "unrecognized platform", "hostID", host.ID, "platform", host.Platform) //nolint:errcheck
 	}
-	query := `SELECT p.*,
+	query := `SELECT p.id, p.team_id, p.resolution, p.name, p.query, p.description, p.author_id, p.platforms, p.critical, p.created_at, p.updated_at,
 		COALESCE(u.name, '<deleted>') AS author_name,
 		COALESCE(u.email, '') AS author_email,
 		CASE

server/datastore/mysql/migrations/tables/20240131083822_AddUniqueHashToPolicies.go

@@ -0,0 +1,74 @@
+package tables
+
+import (
+	"database/sql"
+	"fmt"
+)
+
+func init() {
+	MigrationClient.AddMigration(Up_20240131083822, Down_20240131083822)
+}
+
+func Up_20240131083822(tx *sql.Tx) error {
+	// binary(16) is the efficient way to store md5 hashes,
+	// see https://dev.mysql.com/doc/refman/8.0/en/encryption-functions.html
+	// We store it using UNHEX(MD5(<the string value to hash>)).
+	//
+	// We use md5 for consistency as we already use it in the software table and
+	// for configuration profiles. So instead of using different hashing
+	// algorithms, we'll stick to md5.
+	//
+	// This approach closely matches the one used in the software table.
+	_, err := tx.Exec(`ALTER TABLE policies ADD COLUMN checksum BINARY(16) DEFAULT NULL`)
+	if err != nil {
+		return fmt.Errorf("failed to add checksum column to policies table: %w", err)
+	}
+
+	// fill the checksum for existing rows - order of column used to generate the
+	// checksum is important, we will need to use the same everywhere. The logic
+	// of that computed checksum is captured in
+	// mysql.policiesChecksumComputedColumn, but we don't use it here because if
+	// the function's implementation changes in the future, it should not affect
+	// this DB migration (e.g. the function might use columns that don't exist at
+	// the point in time when this migration is run).
+	_, err = tx.Exec(
+		`
+	UPDATE
+		policies
+	SET
+		checksum = UNHEX(
+			MD5(
+				-- concatenate with separator \x00
+				CONCAT_WS(CHAR(0),
+					COALESCE(team_id, ''),
+					name
+				)
+			)
+		)
+	`,
+	)
+	if err != nil {
+		return fmt.Errorf("failed to update policies table to fill the checksum column: %w", err)
+	}
+
+	// now that every row has a checksum, make it non-nullable and unique
+	_, err = tx.Exec(
+		`ALTER TABLE policies
+		CHANGE COLUMN checksum checksum BINARY(16) NOT NULL,
+		ADD UNIQUE INDEX idx_policies_checksum (checksum)`,
+	)
+	if err != nil {
+		return fmt.Errorf("failed to make checksum column NOT NULL and UNIQUE in policies table: %w", err)
+	}
+
+	// remove the old unique index on (name)
+	_, err = tx.Exec(`ALTER TABLE policies DROP INDEX idx_policies_unique_name`)
+	if err != nil {
+		return fmt.Errorf("failed to drop unique index on name column in policies table: %w", err)
+	}
+	return nil
+}
+
+func Down_20240131083822(tx *sql.Tx) error {
+	return nil
+}

server/datastore/mysql/migrations/tables/20240131083822_AddUniqueHashToPolicies_test.go

@@ -0,0 +1,61 @@
+package tables
+
+import (
+	"context"
+	"github.com/stretchr/testify/require"
+	"testing"
+)
+
+func TestUp_20240131083822(t *testing.T) {
+	db := applyUpToPrev(t)
+
+	// Create a team
+	const insertTeamStmt = `INSERT INTO teams (name) VALUES (?)`
+	teamID := execNoErrLastID(t, db, insertTeamStmt, "team1")
+
+	// add some policies entries
+	const insertStmt = `INSERT INTO policies
+		(team_id, name, query, description)
+	VALUES
+		(?, ?, ?, ?)`
+
+	policy1 := execNoErrLastID(t, db, "INSERT INTO policies (name, query, description) VALUES (?,?,?)", "policy1", "", "")
+	policy2 := execNoErrLastID(t, db, insertStmt, teamID, "policy2", "", "")
+	policy3 := execNoErrLastID(t, db, insertStmt, teamID, "policy3", "", "")
+
+	// Apply current migration.
+	applyNext(t, db)
+
+	var policyCheck []struct {
+		ID       int64  `db:"id"`
+		Name     string `db:"name"`
+		Checksum string `db:"checksum"`
+	}
+	err := db.SelectContext(context.Background(), &policyCheck, `SELECT id, name, HEX(checksum) AS checksum FROM policies ORDER BY id`)
+	require.NoError(t, err)
+	wantIDs := []int64{policy1, policy2, policy3}
+	require.Len(t, policyCheck, len(wantIDs))
+
+	gotIDs := make([]int64, len(wantIDs))
+	for i, pc := range policyCheck {
+		if pc.ID == policy1 {
+			require.Equal(t, pc.Name, "policy1")
+		} else if pc.ID == policy2 {
+			require.Equal(t, pc.Name, "policy2")
+		} else {
+			require.Equal(t, pc.Name, "policy3")
+		}
+		gotIDs[i] = pc.ID
+		require.NotEmpty(t, pc.Checksum)
+		require.Len(t, pc.Checksum, 32)
+	}
+	require.Equal(t, wantIDs, gotIDs)
+
+	// Now insert a policy with the same name but different team_id
+	const insertStmtWithChecksum = `INSERT INTO policies
+		(team_id, name, query, description, checksum)
+	VALUES
+		(?, ?, ?, ?, ?)`
+	_ = execNoErrLastID(t, db, insertStmtWithChecksum, "1", "policy1", "", "", "checksum")
+
+}

server/datastore/mysql/policies.go

@@ -34,7 +34,10 @@ func (ds *Datastore) NewGlobalPolicy(ctx context.Context, authorID *uint, args f
 		args.Description = q.Description
 	}
 	res, err := ds.writer(ctx).ExecContext(ctx,
-		`INSERT INTO policies (name, query, description, resolution, author_id, platforms, critical) VALUES (?, ?, ?, ?, ?, ?, ?)`,
+		fmt.Sprintf(
+			`INSERT INTO policies (name, query, description, resolution, author_id, platforms, critical, checksum) VALUES (?, ?, ?, ?, ?, ?, ?, %s)`,
+			policiesChecksumComputedColumn(),
+		),
 		args.Name, args.Query, args.Description, args.Resolution, authorID, args.Platform, args.Critical,
 	)
 	switch {
@@ -52,6 +55,18 @@ func (ds *Datastore) NewGlobalPolicy(ctx context.Context, authorID *uint, args f
 	return policyDB(ctx, ds.writer(ctx), uint(lastIdInt64), nil)
 }
 
+func policiesChecksumComputedColumn() string {
+	// concatenate with separator \x00
+	return ` UNHEX(
+		MD5(
+			CONCAT_WS(CHAR(0),
+				COALESCE(team_id, ''),
+				name
+			)
+		)
+	) `
+}
+
 func (ds *Datastore) Policy(ctx context.Context, id uint) (*fleet.Policy, error) {
 	return policyDB(ctx, ds.reader(ctx), id, nil)
 }
@@ -520,7 +535,10 @@ func (ds *Datastore) NewTeamPolicy(ctx context.Context, teamID uint, authorID *u
 		args.Description = q.Description
 	}
 	res, err := ds.writer(ctx).ExecContext(ctx,
-		`INSERT INTO policies (name, query, description, team_id, resolution, author_id, platforms, critical) VALUES (?, ?, ?, ?, ?, ?, ?, ?)`,
+		fmt.Sprintf(
+			`INSERT INTO policies (name, query, description, team_id, resolution, author_id, platforms, critical, checksum) VALUES (?, ?, ?, ?, ?, ?, ?, ?, %s)`,
+			policiesChecksumComputedColumn(),
+		),
 		args.Name, args.Query, args.Description, teamID, args.Resolution, authorID, args.Platform, args.Critical)
 	switch {
 	case err == nil:
@@ -567,7 +585,8 @@ func (ds *Datastore) TeamPolicy(ctx context.Context, teamID uint, policyID uint)
 // Currently ApplyPolicySpecs does not allow updating the team of an existing policy.
 func (ds *Datastore) ApplyPolicySpecs(ctx context.Context, authorID uint, specs []*fleet.PolicySpec) error {
 	return ds.withRetryTxx(ctx, func(tx sqlx.ExtContext) error {
-		query := `
+		query := fmt.Sprintf(
+			`
 		INSERT INTO policies (
 			name,
 			query,
@@ -576,8 +595,9 @@ func (ds *Datastore) ApplyPolicySpecs(ctx context.Context, authorID uint, specs
 			resolution,
 			team_id,
 			platforms,
-			critical
-		) VALUES ( ?, ?, ?, ?, ?, (SELECT IFNULL(MIN(id), NULL) FROM teams WHERE name = ?), ?, ?)
+			critical,
+			checksum
+		) VALUES ( ?, ?, ?, ?, ?, (SELECT IFNULL(MIN(id), NULL) FROM teams WHERE name = ?), ?, ?, %s)
 		ON DUPLICATE KEY UPDATE
 			name = VALUES(name),
 			query = VALUES(query),
@@ -586,7 +606,8 @@ func (ds *Datastore) ApplyPolicySpecs(ctx context.Context, authorID uint, specs
 			resolution = VALUES(resolution),
 			platforms = VALUES(platforms),
 			critical = VALUES(critical)
-		`
+		`, policiesChecksumComputedColumn(),
+		)
 		for _, spec := range specs {
 
 			// Validate that the team is not being changed

server/datastore/mysql/policies_test.go

@@ -586,7 +586,7 @@ func testTeamPolicyProprietary(t *testing.T, ds *Datastore) {
 	})
 	require.NoError(t, err)
 
-	// Can't create a team policy with an existing name.
+	// Can't create a team policy with same team id and name.
 	_, err = ds.NewTeamPolicy(ctx, team1.ID, &user1.ID, fleet.PolicyPayload{
 		Name:  "query1",
 		Query: "select 1;",
@@ -597,16 +597,8 @@ func testTeamPolicyProprietary(t *testing.T, ds *Datastore) {
 	}
 	require.True(t, errors.As(err, &isExist) && isExist.IsExists(), err)
 
-	// Can't create a global policy with an existing name.
+	// Can't create a global policy with an existing global name.
 	_, err = ds.NewGlobalPolicy(ctx, &user1.ID, fleet.PolicyPayload{
-		Name:  "query1",
-		Query: "select 1;",
-	})
-	require.Error(t, err)
-	require.True(t, errors.As(err, &isExist) && isExist.IsExists(), err)
-
-	// Can't create a team policy with an existing global name.
-	_, err = ds.NewTeamPolicy(ctx, team1.ID, &user1.ID, fleet.PolicyPayload{
 		Name:  "existing-query-global-1",
 		Query: "select 1;",
 	})
@@ -1073,7 +1065,9 @@ func testPolicyQueriesForHost(t *testing.T, ds *Datastore) {
 
 	// Manually insert a global policy with null resolution.
 	res, err := ds.writer(context.Background()).ExecContext(
-		context.Background(), `INSERT INTO policies (name, query, description) VALUES (?, ?, ?)`, q.Name+"2", q.Query, q.Description+"2",
+		context.Background(),
+		fmt.Sprintf(`INSERT INTO policies (name, query, description, checksum) VALUES (?, ?, ?, %s)`, policiesChecksumComputedColumn()),
+		q.Name+"2", q.Query, q.Description+"2",
 	)
 	require.NoError(t, err)
 	id, err := res.LastInsertId()
@@ -2044,7 +2038,10 @@ func testPolicyViolationDays(t *testing.T, ds *Datastore) {
 		hosts[i] = h
 	}
 
-	createPolStmt := `INSERT INTO policies (name, query, description, author_id, platforms, created_at, updated_at) VALUES (?, ?, '', ?, ?, ?, ?)`
+	createPolStmt := fmt.Sprintf(
+		`INSERT INTO policies (name, query, description, author_id, platforms, created_at, updated_at, checksum) VALUES (?, ?, '', ?, ?, ?, ?, %s)`,
+		policiesChecksumComputedColumn(),
+	)
 	res, err := ds.writer(ctx).ExecContext(ctx, createPolStmt, "test_pol", "select 1", user.ID, "", then, then)
 	require.NoError(t, err)
 	id, _ := res.LastInsertId()
@@ -2139,8 +2136,10 @@ func testPolicyCleanupPolicyMembership(t *testing.T, ds *Datastore) {
 	}
 
 	// create some policies, using direct insert statements to control the timestamps
-	createPolStmt := `INSERT INTO policies (name, query, description, author_id, platforms, created_at, updated_at)
-                    VALUES (?, ?, '', ?, ?, ?, ?)`
+	createPolStmt := fmt.Sprintf(
+		`INSERT INTO policies (name, query, description, author_id, platforms, created_at, updated_at, checksum)
+                    VALUES (?, ?, '', ?, ?, ?, ?, %s)`, policiesChecksumComputedColumn(),
+	)
 
 	jan2020 := time.Date(2020, 1, 1, 0, 0, 0, 0, time.UTC)
 	feb2020 := time.Date(2020, 2, 1, 0, 0, 0, 0, time.UTC)
@@ -2256,11 +2255,13 @@ func testPolicyCleanupPolicyMembership(t *testing.T, ds *Datastore) {
 }
 
 func updatePolicyWithTimestamp(t *testing.T, ds *Datastore, p *fleet.Policy, ts time.Time) {
-	sql := `
+	sqlStmt := `
 		UPDATE policies
 			SET name = ?, query = ?, description = ?, resolution = ?, platforms = ?, updated_at = ?
 			WHERE id = ?`
-	_, err := ds.writer(context.Background()).ExecContext(context.Background(), sql, p.Name, p.Query, p.Description, p.Resolution, p.Platform, ts, p.ID)
+	_, err := ds.writer(context.Background()).ExecContext(
+		context.Background(), sqlStmt, p.Name, p.Query, p.Description, p.Resolution, p.Platform, ts, p.ID,
+	)
 	require.NoError(t, err)
 }
 

server/service/async/async_policy_test.go

@@ -4,6 +4,7 @@ import (
 	"context"
 	"database/sql"
 	"fmt"
+	"strconv"
 	"testing"
 	"time"
 
@@ -525,7 +526,10 @@ func createPolicies(t *testing.T, ds *mysql.Datastore, count int) []uint {
 	ids := make([]uint, count)
 	mysql.ExecAdhocSQL(t, ds, func(tx sqlx.ExtContext) error {
 		for i := 0; i < count; i++ {
-			res, err := tx.ExecContext(ctx, `INSERT INTO policies (name, description, query) VALUES (?, ?, ?)`, fmt.Sprintf("%s-%d", t.Name(), i), t.Name(), "SELECT 1")
+			res, err := tx.ExecContext(
+				ctx, `INSERT INTO policies (name, description, query, checksum) VALUES (?, ?, ?, ?)`,
+				fmt.Sprintf("%s-%d", t.Name(), i), t.Name(), "SELECT 1", strconv.Itoa(i),
+			)
 			if err != nil {
 				return err
 			}