Commit `a6378c178c`

a6378c178c4f0f6b59039e3501fe9b7e2b962098

parent: 970614872d

Verified · cmc

cmc <hello@cleberg.net> · 2026-09-29 15:30 UTC

store: deletes take grants and deploy keys naming the deleted row

Deleting an account or org removes its repo_access rows, and deleting a
repository removes its deploy keys, in the delete's transaction. A
migration removes those left by earlier deletes and the daemon logs the
counts with the schema change.

Ref #306

Layout: unified · split

cmd/gitbayd/main.go +10 −1

		@@ -77,7 +77,16 @@ func openStore(cfg config.Config) (*store.Store, error) {
77	77	return nil, err
78	78	}
79	79	if after != before {
80		slog.Info("schema migrated", "from", before, "to", after)
	80	note, err := s.TakeMigrationNote()
	81	if err != nil {
	82	s.Close()
	83	return nil, err
	84	}
	85	args := []any{"from", before, "to", after}
	86	if note != "" {
	87	args = append(args, "note", note)
	88	}
	89	slog.Info("schema migrated", args...)
81	90	}
82	91	return s, nil
83	92	}

cmd/gitbayd/main_test.go +35

		@@ -3,9 +3,12 @@ package main
3	3	import (
4	4	"bytes"
5	5	"log/slog"
	6	"path/filepath"
6	7	"strconv"
7	8	"strings"
8	9	"testing"
	10
	11	"gitbay.org/gitbay/internal/store"
9	12	)
10	13
11	14	// A restart that moves the schema says so. Migrations used to run in silence,
		@@ -52,3 +55,35 @@ func TestOpenStoreLogsSchemaMigration(t *testing.T) {
52	55	t.Errorf("logged a migration on an up-to-date database:\n%s", buf.String())
53	56	}
54	57	}
	58
	59	// A note a migration leaves goes in the log line and is then dropped.
	60	func TestOpenStoreLogsMigrationNote(t *testing.T) {
	61	cfg := testConfig(t)
	62	st, err := store.Open(filepath.Join(cfg.Server.Root, "gitbay.db"))
	63	if err != nil {
	64	t.Fatal(err)
	65	}
	66	if err := st.MigrateTo(1); err != nil {
	67	t.Fatal(err)
	68	}
	69	if _, err := st.DB.Exec("INSERT INTO settings (key, value) VALUES ('migration_note', 'removed 2 things')"); err != nil {
	70	t.Fatal(err)
	71	}
	72	st.Close()
	73
	74	var buf bytes.Buffer
	75	prev := slog.Default()
	76	slog.SetDefault(slog.New(slog.NewTextHandler(&buf, nil)))
	77	defer slog.SetDefault(prev)
	78	s, err := openStore(cfg)
	79	if err != nil {
	80	t.Fatalf("openStore: %v", err)
	81	}
	82	defer s.Close()
	83	if !strings.Contains(buf.String(), `note="removed 2 things"`) {
	84	t.Fatalf("note not logged:\n%s", buf.String())
	85	}
	86	if note, err := s.TakeMigrationNote(); err != nil \|\| note != "" {
	87	t.Fatalf("note left behind: %q, %v", note, err)
	88	}
	89	}

internal/control/idreuse_test.go +5 −5

		@@ -8,9 +8,9 @@ import (
8	8	"gitbay.org/gitbay/internal/policy"
9	9	)
10	10
11		// A deploy key names its repository by id in its scope, and deleting
12		// the repository leaves the key. The next repository does not take the
13		// id, so the key opens nothing (#306).
	11	// A deploy key names its repository by id in its scope. Deleting the
	12	// repository removes the key, and the next repository does not take the
	13	// id, so the key opens nothing either way (#306).
14	14	func TestDeployKeyOfDeletedRepository(t *testing.T) {
15	15	st, _, uid := newQueueTestRepo(t)
16	16	goneID, err := st.CreateRepo("user", uid, "gone", "private")
		@@ -33,8 +33,8 @@ func TestDeployKeyOfDeletedRepository(t *testing.T) {
33	33	}
34	34	}
35	35
36		// A grant names its account by id and outlives the account. The next
37		// account does not take the id, so it does not inherit the grant (#306).
	36	// A grant names its account by id. Deleting the account removes the
	37	// grant, and the next account does not take the id (#306).
38	38	func TestGrantOfDeletedAccount(t *testing.T) {
39	39	st, repo, _ := newQueueTestRepo(t)
40	40	carol, err := st.CreateUser("carol", false)

internal/store/idreuse_test.go +168 −6

		@@ -81,16 +81,14 @@ func seedForIDs(t testing.T, s Store) (goneRepo, goneUser int64) {
81	81	goneRepo = mustExec(t, s, "INSERT INTO repos (owner_kind, owner_id, name, visibility) VALUES ('user', ?, 'gone', 'private')", alice)
82	82	mustExec(t, s, "INSERT INTO ssh_keys (user_id, fingerprint, algo, blob, scope) VALUES (?, 'SHA256:d', 'ssh-ed25519', x'00', ?)",
83	83	alice, "deploy:"+itoa(goneRepo)+":rw")
84		if err := s.DeleteRepo(goneRepo); err != nil {
85		t.Fatal(err)
86		}
	84	// Raw deletes: DeleteRepo and DeleteUser now take the deploy key and
	85	// the grant with them, and these orphans stand for ones left earlier.
	86	mustExec(t, s, "DELETE FROM repos WHERE id = ?", goneRepo)
87	87	goneUser = mustExec(t, s, "INSERT INTO users (username) VALUES ('carol')")
88	88	if err := s.GrantAccess(repo, goneUser, "write"); err != nil {
89	89	t.Fatal(err)
90	90	}
91		if err := s.DeleteUser(goneUser); err != nil {
92		t.Fatal(err)
93		}
	91	mustExec(t, s, "DELETE FROM users WHERE id = ?", goneUser)
94	92	return goneRepo, goneUser
95	93	}
96	94
		@@ -320,3 +318,167 @@ func TestInFlightMarksAfterCascade(t *testing.T) {
320	318	t.Fatal("the removed device's push was marked on the next device's")
321	319	}
322	320	}
	321
	322	// Deleting a repository takes the deploy keys scoped to it, and only
	323	// those; deleting an account or an org takes its grants (#306).
	324	func TestDeletesTakeGrantsAndDeployKeys(t *testing.T) {
	325	s := open(t)
	326	if err := s.MigrateUp(); err != nil {
	327	t.Fatal(err)
	328	}
	329	var revoked []Revoked
	330	s.OnRevoke(func(r Revoked) { revoked = append(revoked, r) })
	331	alice := mustExec(t, s, "INSERT INTO users (username) VALUES ('alice')")
	332	var repos []int64
	333	for i := range 12 {
	334	repos = append(repos, mustExec(t, s,
	335	"INSERT INTO repos (owner_kind, owner_id, name, visibility) VALUES ('user', ?, ?, 'public')", alice, "r"+itoa(int64(i))))
	336	}
	337	// repos[0] is 2 and repos[10] is 12: a prefix of one id must not
	338	// match the other.
	339	gone, other := repos[0], repos[len(repos)-2]
	340	if !strings.HasPrefix(itoa(other), itoa(gone)) {
	341	t.Fatalf("ids %d and %d do not share a prefix", gone, other)
	342	}
	343	goneKey := mustExec(t, s, "INSERT INTO ssh_keys (user_id, fingerprint, algo, blob, scope) VALUES (?, 'SHA256:g', 'a', x'00', ?)",
	344	alice, "deploy:"+itoa(gone)+":rw")
	345	mustExec(t, s, "INSERT INTO ssh_keys (user_id, fingerprint, algo, blob, scope) VALUES (?, 'SHA256:o', 'a', x'00', ?)",
	346	alice, "deploy:"+itoa(other)+":ro")
	347	if err := s.DeleteRepo(gone); err != nil {
	348	t.Fatal(err)
	349	}
	350	var n int
	351	s.DB.QueryRow("SELECT COUNT(*) FROM ssh_keys WHERE fingerprint = 'SHA256:g'").Scan(&n)
	352	if n != 0 {
	353	t.Fatal("the deleted repository's deploy key survived")
	354	}
	355	s.DB.QueryRow("SELECT COUNT(*) FROM ssh_keys WHERE fingerprint = 'SHA256:o'").Scan(&n)
	356	if n != 1 {
	357	t.Fatal("another repository's deploy key went with it")
	358	}
	359	if len(revoked) != 1 \|\| len(revoked[0].KeyIDs) != 1 \|\| revoked[0].KeyIDs[0] != goneKey {
	360	t.Fatalf("revocations announced: %+v", revoked)
	361	}
	362	if err := s.DeleteRepo(gone); err != ErrNotFound {
	363	t.Fatalf("deleting it again: %v", err)
	364	}
	365
	366	bob := mustExec(t, s, "INSERT INTO users (username) VALUES ('bob')")
	367	org := mustExec(t, s, "INSERT INTO orgs (name) VALUES ('acme')")
	368	if err := s.GrantAccess(other, bob, "write"); err != nil {
	369	t.Fatal(err)
	370	}
	371	mustExec(t, s, "INSERT INTO repo_access (repo_id, subject_kind, subject_id, role) VALUES (?, 'org', ?, 'read')", other, org)
	372	// An org with the same id as bob's keeps its grant when bob goes.
	373	mustExec(t, s, "INSERT INTO repo_access (repo_id, subject_kind, subject_id, role) VALUES (?, 'org', ?, 'read')", repos[1], bob)
	374	mustExec(t, s, `INSERT INTO profile_about_backfill (owner_kind, owner_id, about, about_format)
	375	VALUES ('user', ?, 'a', 'md'), ('org', ?, 'b', 'md'), ('org', ?, 'c', 'md')`, bob, org, bob)
	376	if err := s.DeleteUser(bob); err != nil {
	377	t.Fatal(err)
	378	}
	379	s.DB.QueryRow("SELECT COUNT(*) FROM repo_access WHERE subject_kind = 'user' AND subject_id = ?", bob).Scan(&n)
	380	if n != 0 {
	381	t.Fatal("the deleted account's grant survived")
	382	}
	383	s.DB.QueryRow("SELECT COUNT(*) FROM profile_about_backfill WHERE owner_kind = 'user' AND owner_id = ?", bob).Scan(&n)
	384	if n != 0 {
	385	t.Fatal("the deleted account's about text survived")
	386	}
	387	s.DB.QueryRow("SELECT COUNT(*) FROM repo_access WHERE subject_kind = 'org' AND subject_id = ?", bob).Scan(&n)
	388	if n != 1 {
	389	t.Fatal("an org grant went with the account of the same id")
	390	}
	391	if err := s.DeleteOrg(org); err != nil {
	392	t.Fatal(err)
	393	}
	394	s.DB.QueryRow("SELECT COUNT(*) FROM repo_access WHERE subject_kind = 'org' AND subject_id = ?", org).Scan(&n)
	395	if n != 0 {
	396	t.Fatal("the deleted org's grant survived")
	397	}
	398	s.DB.QueryRow("SELECT COUNT(*) FROM profile_about_backfill WHERE owner_kind = 'org'").Scan(&n)
	399	if n != 1 {
	400	t.Fatalf("org about texts after deleting acme: %d, want only the one with bob's id", n)
	401	}
	402	}
	403
	404	// The cleanup migration removes grants and deploy keys left by earlier
	405	// deletes, keeps live ones, and leaves a note with the counts.
	406	func TestOrphanCleanupMigration(t *testing.T) {
	407	ms, err := loadMigrations()
	408	if err != nil {
	409	t.Fatal(err)
	410	}
	411	v := 0
	412	for _, m := range ms {
	413	if m.name == "orphan_grants_deploy_keys" {
	414	v = m.version
	415	}
	416	}
	417	if v == 0 {
	418	t.Fatal("no orphan_grants_deploy_keys migration")
	419	}
	420	s := open(t)
	421	if err := s.MigrateTo(v - 1); err != nil {
	422	t.Fatal(err)
	423	}
	424	alice := mustExec(t, s, "INSERT INTO users (username) VALUES ('alice')")
	425	repo := mustExec(t, s, "INSERT INTO repos (owner_kind, owner_id, name, visibility) VALUES ('user', ?, 'r', 'public')", alice)
	426	mustExec(t, s, "INSERT INTO repo_access (repo_id, subject_kind, subject_id, role) VALUES (?, 'user', ?, 'read')", repo, alice)
	427	mustExec(t, s, "INSERT INTO repo_access (repo_id, subject_kind, subject_id, role) VALUES (?, 'user', 999, 'write')", repo)
	428	mustExec(t, s, "INSERT INTO repo_access (repo_id, subject_kind, subject_id, role) VALUES (?, 'org', 998, 'read')", repo)
	429	mustExec(t, s, "INSERT INTO ssh_keys (user_id, fingerprint, algo, blob, scope) VALUES (?, 'SHA256:live', 'a', x'00', ?)",
	430	alice, "deploy:"+itoa(repo)+":rw")
	431	mustExec(t, s, "INSERT INTO ssh_keys (user_id, fingerprint, algo, blob, scope) VALUES (?, 'SHA256:dead', 'a', x'00', 'deploy:997:ro')", alice)
	432	mustExec(t, s, "INSERT INTO ssh_keys (user_id, fingerprint, algo, blob) VALUES (?, 'SHA256:user', 'a', x'00')", alice)
	433	mustExec(t, s, `INSERT INTO profile_about_backfill (owner_kind, owner_id, about, about_format)
	434	VALUES ('user', ?, 'live', 'md'), ('user', 996, 'dead', 'md'), ('org', 995, 'dead', 'md')`, alice)
	435	var epoch int
	436	s.DB.QueryRow("SELECT value FROM settings WHERE key = 'key_epoch'").Scan(&epoch)
	437	if err := s.MigrateTo(v); err != nil {
	438	t.Fatal(err)
	439	}
	440	if got := count(t, s, "repo_access"); got != 1 {
	441	t.Fatalf("repo_access: %d rows, want the live grant", got)
	442	}
	443	var about string
	444	if err := s.DB.QueryRow("SELECT group_concat(about) FROM profile_about_backfill").Scan(&about); err != nil \|\| about != "live" {
	445	t.Fatalf("about texts after cleanup: %q, %v", about, err)
	446	}
	447	var fps []string
	448	rows, err := s.DB.Query("SELECT fingerprint FROM ssh_keys ORDER BY fingerprint")
	449	if err != nil {
	450	t.Fatal(err)
	451	}
	452	for rows.Next() {
	453	var fp string
	454	rows.Scan(&fp)
	455	fps = append(fps, fp)
	456	}
	457	rows.Close()
	458	if strings.Join(fps, " ") != "SHA256:live SHA256:user" {
	459	t.Fatalf("keys after cleanup: %v", fps)
	460	}
	461	var after int
	462	s.DB.QueryRow("SELECT value FROM settings WHERE key = 'key_epoch'").Scan(&after)
	463	if after != epoch+1 {
	464	t.Fatalf("key_epoch %d, want %d", after, epoch+1)
	465	}
	466	note, err := s.TakeMigrationNote()
	467	if err != nil \|\| note != "removed grants of deleted accounts or organizations: 2; deploy keys of deleted repositories: 1; profile about texts of deleted accounts or organizations: 2" {
	468	t.Fatalf("note %q, %v", note, err)
	469	}
	470	if note, _ := s.TakeMigrationNote(); note != "" {
	471	t.Fatalf("note not cleared: %q", note)
	472	}
	473
	474	// Nothing to remove, no note.
	475	if err := s.MigrateTo(v - 1); err != nil {
	476	t.Fatal(err)
	477	}
	478	if err := s.MigrateTo(v); err != nil {
	479	t.Fatal(err)
	480	}
	481	if note, _ := s.TakeMigrationNote(); note != "" {
	482	t.Fatalf("note with nothing removed: %q", note)
	483	}
	484	}

internal/store/migrations/0075_orphan_grants_deploy_keys.down.sql added +2

		@@ -0,0 +1,2 @@
	1	-- The removed rows are not restored.
	2	DELETE FROM settings WHERE key = 'migration_note';

internal/store/migrations/0075_orphan_grants_deploy_keys.up.sql added +36

		@@ -0,0 +1,36 @@
	1	-- Grants and parked profile about texts of deleted accounts and
	2	-- organizations, and deploy keys of deleted repositories, name their
	3	-- subject by id with no foreign key; deletes left them behind until #306. The counts go in a note the
	4	-- daemon logs with the migration and then drops.
	5	INSERT INTO settings (key, value)
	6	SELECT 'migration_note', 'removed grants of deleted accounts or organizations: ' \|\| g.n
	7	\|\| '; deploy keys of deleted repositories: ' \|\| k.n
	8	\|\| '; profile about texts of deleted accounts or organizations: ' \|\| b.n
	9	FROM (SELECT COUNT(*) AS n FROM repo_access a
	10	WHERE (a.subject_kind = 'user' AND NOT EXISTS (SELECT 1 FROM users u WHERE u.id = a.subject_id))
	11	OR (a.subject_kind = 'org' AND NOT EXISTS (SELECT 1 FROM orgs o WHERE o.id = a.subject_id))) g,
	12	(SELECT COUNT(*) AS n FROM ssh_keys
	13	WHERE scope LIKE 'deploy:%' AND CAST(substr(scope, 8, instr(substr(scope, 8), ':') - 1) AS INTEGER)
	14	NOT IN (SELECT id FROM repos)) k,
	15	(SELECT COUNT(*) AS n FROM profile_about_backfill p
	16	WHERE (p.owner_kind = 'user' AND NOT EXISTS (SELECT 1 FROM users u WHERE u.id = p.owner_id))
	17	OR (p.owner_kind = 'org' AND NOT EXISTS (SELECT 1 FROM orgs o WHERE o.id = p.owner_id))) b
	18	WHERE g.n + k.n + b.n > 0
	19	ON CONFLICT (key) DO UPDATE SET value = excluded.value;
	20
	21	DELETE FROM repo_access
	22	WHERE (subject_kind = 'user' AND NOT EXISTS (SELECT 1 FROM users u WHERE u.id = repo_access.subject_id))
	23	OR (subject_kind = 'org' AND NOT EXISTS (SELECT 1 FROM orgs o WHERE o.id = repo_access.subject_id));
	24
	25	UPDATE settings SET value = value + 1
	26	WHERE key = 'key_epoch' AND EXISTS (SELECT 1 FROM ssh_keys
	27	WHERE scope LIKE 'deploy:%' AND CAST(substr(scope, 8, instr(substr(scope, 8), ':') - 1) AS INTEGER)
	28	NOT IN (SELECT id FROM repos));
	29
	30	DELETE FROM ssh_keys
	31	WHERE scope LIKE 'deploy:%' AND CAST(substr(scope, 8, instr(substr(scope, 8), ':') - 1) AS INTEGER)
	32	NOT IN (SELECT id FROM repos);
	33
	34	DELETE FROM profile_about_backfill
	35	WHERE (owner_kind = 'user' AND NOT EXISTS (SELECT 1 FROM users u WHERE u.id = profile_about_backfill.owner_id))
	36	OR (owner_kind = 'org' AND NOT EXISTS (SELECT 1 FROM orgs o WHERE o.id = profile_about_backfill.owner_id));

internal/store/orgs.go +17 −2

		@@ -195,8 +195,23 @@ func (s *Store) DeleteOrg(orgID int64) error {
195	195	if n > 0 {
196	196	return fmt.Errorf("the organization still owns %d repositories; delete or transfer them first", n)
197	197	}
198		_, err := s.DB.Exec("DELETE FROM orgs WHERE id = ?", orgID)
199		return err
	198	// Grants and a parked about text name the org by id with no foreign
	199	// key (#306).
	200	tx, err := s.DB.Begin()
	201	if err != nil {
	202	return err
	203	}
	204	defer tx.Rollback()
	205	if _, err := tx.Exec("DELETE FROM repo_access WHERE subject_kind = 'org' AND subject_id = ?", orgID); err != nil {
	206	return err
	207	}
	208	if _, err := tx.Exec("DELETE FROM profile_about_backfill WHERE owner_kind = 'org' AND owner_id = ?", orgID); err != nil {
	209	return err
	210	}
	211	if _, err := tx.Exec("DELETE FROM orgs WHERE id = ?", orgID); err != nil {
	212	return err
	213	}
	214	return tx.Commit()
200	215	}
201	216
202	217	// RenameOrg changes an org's name, holding the shared owner-namespace

internal/store/repos.go +37 −1

		@@ -180,14 +180,50 @@ func (s *Store) SetForkOf(repoID, parentID int64) error {
180	180	return err
181	181	}
182	182
	183	// DeleteRepo removes the repository row, what cascades from it, and
	184	// the deploy keys scoped to it, which name it by id in their scope
	185	// rather than by a foreign key (#306).
183	186	func (s *Store) DeleteRepo(repoID int64) error {
184		res, err := s.DB.Exec("DELETE FROM repos WHERE id = ?", repoID)
	187	tx, err := s.DB.Begin()
	188	if err != nil {
	189	return err
	190	}
	191	defer tx.Rollback()
	192	rows, err := tx.Query("DELETE FROM ssh_keys WHERE scope LIKE 'deploy:' \|\| ? \|\| ':%' RETURNING id", repoID)
	193	if err != nil {
	194	return err
	195	}
	196	var keyIDs []int64
	197	for rows.Next() {
	198	var id int64
	199	if err := rows.Scan(&id); err != nil {
	200	rows.Close()
	201	return err
	202	}
	203	keyIDs = append(keyIDs, id)
	204	}
	205	rows.Close()
	206	if err := rows.Err(); err != nil {
	207	return err
	208	}
	209	res, err := tx.Exec("DELETE FROM repos WHERE id = ?", repoID)
185	210	if err != nil {
186	211	return err
187	212	}
188	213	if n, _ := res.RowsAffected(); n == 0 {
189	214	return ErrNotFound
190	215	}
	216	if len(keyIDs) > 0 {
	217	if err := bumpKeyEpoch(tx); err != nil {
	218	return err
	219	}
	220	}
	221	if err := tx.Commit(); err != nil {
	222	return err
	223	}
	224	if len(keyIDs) > 0 {
	225	s.announce(Revoked{KeyIDs: keyIDs})
	226	}
191	227	return nil
192	228	}
193	229

internal/store/store.go +11

		@@ -174,6 +174,17 @@ func (s *Store) Version() (int, error) {
174	174	return v, err
175	175	}
176	176
	177	// TakeMigrationNote returns and clears what a migration left to be
	178	// logged with it, "" for nothing.
	179	func (s *Store) TakeMigrationNote() (string, error) {
	180	var note string
	181	err := s.DB.QueryRow("DELETE FROM settings WHERE key = 'migration_note' RETURNING value").Scan(&note)
	182	if errors.Is(err, sql.ErrNoRows) {
	183	return "", nil
	184	}
	185	return note, err
	186	}
	187
177	188	// MigrateUp applies all pending migrations.
178	189	func (s *Store) MigrateUp() error { return s.migrateTo(-1) }
179	190

internal/store/users.go +17 −1

		@@ -99,13 +99,29 @@ func (s *Store) DeleteUser(id int64) error {
99	99	return fmt.Errorf("account still anchors: %s — transfer or delete those first, or disable the account instead",
100	100	strings.Join(blockers, ", "))
101	101	}
102		res, err := s.DB.Exec("DELETE FROM users WHERE id = ?", id)
	102	// Grants and a parked about text name the account by id with no
	103	// foreign key, so they go in the same transaction (#306).
	104	tx, err := s.DB.Begin()
	105	if err != nil {
	106	return err
	107	}
	108	defer tx.Rollback()
	109	if _, err := tx.Exec("DELETE FROM repo_access WHERE subject_kind = 'user' AND subject_id = ?", id); err != nil {
	110	return err
	111	}
	112	if _, err := tx.Exec("DELETE FROM profile_about_backfill WHERE owner_kind = 'user' AND owner_id = ?", id); err != nil {
	113	return err
	114	}
	115	res, err := tx.Exec("DELETE FROM users WHERE id = ?", id)
103	116	if err != nil {
104	117	return err
105	118	}
106	119	if n, _ := res.RowsAffected(); n == 0 {
107	120	return ErrNotFound
108	121	}
	122	if err := tx.Commit(); err != nil {
	123	return err
	124	}
109	125	s.announce(Revoked{UserID: id})
110	126	return nil
111	127	}

Commit a6378c178c

Commit `a6378c178c`