From cd0b0c0fb427bff6060c3f785d67784b999610f3 Mon Sep 17 00:00:00 2001 From: Eugene Blikh Date: Sat, 15 Aug 2026 12:16:59 +0300 Subject: [PATCH] web: rename a database from its settings page MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A database's name lives in two places — the metadata row and the on-disk store directory its path names — so a rename has to move both. The order is creation's: the row first, where a name already taken is refused by the unique index before anything on disk changes, then the store, then the handle the remotesapi memoized under the old path. A store that will not move rolls the row back, so the two halves never disagree about where a database lives; only a failed rollback is escalated to a human, because it is the one outcome no later request can repair. storage.MoveStore is one os.Rename behind the containment guard DeleteStore already used, extracted here as containedPath. It refuses an occupied destination outright: os.Rename over an empty directory would succeed and swallow it. No redirect is left behind — the old address simply stops resolving, as on git.sr.ht — so an existing clone needs its remote replaced, and a companion of a git repository is re-provisioned under its old name by the next push to that repository. The README's quickstart now says both. --- README.md | 14 +++ cmd/doltsrht/main.go | 4 + cmd/doltsrht/main_test.go | 4 + db/repos.go | 28 +++++ db/repos_test.go | 65 ++++++++++++ storage/init.go | 86 ++++++++++++--- storage/move_test.go | 150 ++++++++++++++++++++++++++ web/adapters.go | 4 + web/deps.go | 7 ++ web/handlers_settings.go | 106 +++++++++++++++++-- web/templates/settings.html | 16 +++ web/web_test.go | 202 +++++++++++++++++++++++++++++++++++- 12 files changed, 666 insertions(+), 20 deletions(-) create mode 100644 storage/move_test.go diff --git a/README.md b/README.md index cd19d6509233a3460031658986e7bba035e661f9..827d764ba45a5287e37b4407b9868bd6f889f728 100644 --- a/README.md +++ b/README.md @@ -276,6 +276,20 @@ Once the service is up and you are logged into meta: dolt config --global --add creds.add_url https://dolt.srht.bigb.es/settings/keys ``` +4. **Rename a database.** `~/` → **settings** → **Rename**. The + record and the stored data move together, and no redirect is left behind: the + old address stops resolving, so an existing checkout needs its remote + replaced (`dolt remote` has no `set-url`). + + ```sh + dolt remote remove origin + dolt remote add origin https://dolt.srht.bigb.es/~/ + ``` + + A database provisioned as the companion of a git.sr.ht repository is + re-created under its old name by the next push to that repository — the hook + provisions by the *git* repo's name, which renaming here does not change. + Anonymous `dolt clone` works for PUBLIC and UNLISTED databases with no credentials at all; PRIVATE databases return "not found" to unauthorized callers (their existence is not leaked). diff --git a/cmd/doltsrht/main.go b/cmd/doltsrht/main.go index f235b521f9a4865be867a85b02e45a303fa4c20b..fd454be08696d3c521d0ebff10cee4f6fad0d114 100644 --- a/cmd/doltsrht/main.go +++ b/cmd/doltsrht/main.go @@ -84,6 +84,10 @@ func (m *storeManager) DeleteStore(ctx context.Context, root, absPath string) er return storage.DeleteStore(ctx, root, absPath) } +func (m *storeManager) MoveStore(ctx context.Context, root, srcPath, dstPath string) error { + return storage.MoveStore(ctx, root, srcPath, dstPath) +} + func (m *storeManager) Evict(diskPath string) error { return m.cache.Evict(diskPath) } diff --git a/cmd/doltsrht/main_test.go b/cmd/doltsrht/main_test.go index 56c1d0512d45f24493d6ceac03ce8365abbe9b62..6c4f87cee06b614b33ddcfb6000669db81d35579 100644 --- a/cmd/doltsrht/main_test.go +++ b/cmd/doltsrht/main_test.go @@ -263,6 +263,10 @@ func (noStores) DeleteStore(context.Context, string, string) error { panic("cmd/doltsrht: the boot tests must not touch the on-disk stores") } +func (noStores) MoveStore(context.Context, string, string, string) error { + panic("cmd/doltsrht: the boot tests must not touch the on-disk stores") +} + func (noStores) Evict(string) error { panic("cmd/doltsrht: the boot tests must not touch the on-disk stores") } diff --git a/db/repos.go b/db/repos.go index cabb1affe6dfe8dbe8eceaae0e41f1e9865102e8..f4f60a403af88fcdbdb7abab4a1ef1a3a88ee8ed 100644 --- a/db/repos.go +++ b/db/repos.go @@ -218,6 +218,34 @@ WHERE id = $1` return requireOne(res, "update repo") } +// RenameRepo moves a repository to a new name and on-disk path in one +// statement, and bumps updated. Both columns move together on purpose: path is +// derived from (owner, name) by storage.RepoDiskPath, and a row whose name and +// path disagree would be served from the wrong store. +// +// The caller is responsible for validating name (core.ValidateName) and for +// moving the store on disk; this is the metadata half only. A name already +// taken by the same owner comes back as ErrNameTaken — mapped from both unique +// indexes the way CreateRepo maps them, because a rename to an existing +// database trips exactly the same pair — and a missing id as ErrNotFound. +func (s *Store) RenameRepo(ctx context.Context, id int, name, path string) error { + const q = ` +UPDATE repository +SET name = $2, path = $3, updated = $4 +WHERE id = $1` + res, err := s.q.ExecContext(ctx, q, id, name, path, time.Now().UTC()) + if err != nil { + var pqErr *pq.Error + if errors.As(err, &pqErr) && pqErr.Code == "23505" && + (pqErr.Constraint == "uq_repo_owner_id_name" || + pqErr.Constraint == "repository_path_key") { + return ErrNameTaken + } + return fmt.Errorf("rename repo %d: %w", id, err) + } + return requireOne(res, "rename repo") +} + // DeleteRepo removes a repository row (cascading to its access entries). The // on-disk store removal is the caller's responsibility. Returns ErrNotFound if // id does not exist. diff --git a/db/repos_test.go b/db/repos_test.go index dc637a57aa1f3c8eda247575757ec0fa88039fda..b237ac2d2742acabdd801b09b02fbfe3b175d591 100644 --- a/db/repos_test.go +++ b/db/repos_test.go @@ -72,6 +72,71 @@ func TestCreateRepoDuplicateName(t *testing.T) { } } +// A rename moves the name and the on-disk path in one statement: path is +// derived from (owner, name), and a row whose two halves disagree would be +// served out of the wrong store. +func TestRenameRepo(t *testing.T) { + s, sqlDB, cleanup := newTestStore(t) + defer cleanup() + ctx := context.Background() + + insertUser(t, sqlDB, 1, "alice", core.UserTypeUser) + repo := mkRepo(t, s, ctx, 1, "alice", "widgets", core.VisibilityPublic) + + require.NoError(t, s.RenameRepo(ctx, repo.ID, "gadgets", "/var/lib/dolt/~alice/gadgets")) + + got, err := s.GetRepoByID(ctx, repo.ID) + require.NoError(t, err) + assert.Equal(t, "gadgets", got.Name) + assert.Equal(t, "/var/lib/dolt/~alice/gadgets", got.Path) + assert.Equal(t, core.VisibilityPublic, got.Visibility, "a rename must not touch anything else") + + // The old name stops resolving and the new one answers. + _, err = s.GetRepoByOwnerAndName(ctx, "alice", "widgets") + assert.ErrorIs(t, err, ErrNotFound) + byName, err := s.GetRepoByOwnerAndName(ctx, "alice", "gadgets") + require.NoError(t, err) + assert.Equal(t, repo.ID, byName.ID) + + assert.ErrorIs(t, s.RenameRepo(ctx, 999, "x", "/var/lib/dolt/~alice/x"), ErrNotFound) +} + +// Renaming onto a name the owner already uses trips the same pair of unique +// indexes a duplicate create does, and must come back as ErrNameTaken rather +// than a raw 23505 — the web layer answers 409 on that error alone. +func TestRenameRepoOntoATakenName(t *testing.T) { + for _, tc := range []struct { + name string + path string + }{ + {"same path (both indexes, path reported)", "/var/lib/dolt/~alice/taken"}, + {"different path (name index only)", "/srv/dolt/~alice/taken"}, + } { + t.Run(tc.name, func(t *testing.T) { + s, sqlDB, cleanup := newTestStore(t) + defer cleanup() + ctx := context.Background() + + insertUser(t, sqlDB, 1, "alice", core.UserTypeUser) + insertUser(t, sqlDB, 2, "bob", core.UserTypeUser) + repo := mkRepo(t, s, ctx, 1, "alice", "widgets", core.VisibilityPublic) + mkRepo(t, s, ctx, 1, "alice", "taken", core.VisibilityPublic) + + assert.ErrorIs(t, s.RenameRepo(ctx, repo.ID, "taken", tc.path), ErrNameTaken) + + // Nothing moved: the refusal is the whole statement's. + got, err := s.GetRepoByID(ctx, repo.ID) + require.NoError(t, err) + assert.Equal(t, "widgets", got.Name) + + // Uniqueness is per owner — bob's "taken" is no obstacle, and it is + // the path index alone that keeps two owners apart on disk. + mkRepo(t, s, ctx, 2, "bob", "widgets", core.VisibilityPublic) + require.NoError(t, s.RenameRepo(ctx, repo.ID, "gadgets", "/var/lib/dolt/~alice/gadgets")) + }) + } +} + func TestGetRepoNotFound(t *testing.T) { s, _, cleanup := newTestStore(t) defer cleanup() diff --git a/storage/init.go b/storage/init.go index a8372c415c75332b8934518bcef12ae148aa6b53..b192dfdd3449838d9977d2f154ba674095781f58 100644 --- a/storage/init.go +++ b/storage/init.go @@ -129,29 +129,43 @@ func InitEmptyStore(ctx context.Context, absPath string) (err error) { return nil } -// DeleteStore removes the store directory at absPath. It refuses to delete -// anything that is not strictly contained within root, guarding against a -// corrupted or attacker-controlled path escaping the configured repos root. -// Both root and absPath must be absolute. -func DeleteStore(ctx context.Context, root, absPath string) error { +// containedPath checks that absPath is an absolute path strictly inside the +// absolute root and returns both cleaned. It is the guard every destructive +// path operation in this package runs first, so a corrupted or +// attacker-controlled path can never escape the configured repos root — and +// the root itself is never a legal target. op names the caller ("DeleteStore", +// "MoveStore") so a refusal says which operation was stopped. +func containedPath(op, root, absPath string) (cleanRoot, cleanPath string, err error) { if !filepath.IsAbs(root) { - return fmt.Errorf("storage: DeleteStore requires an absolute root, got %q", root) + return "", "", fmt.Errorf("storage: %s requires an absolute root, got %q", op, root) } if !filepath.IsAbs(absPath) { - return fmt.Errorf("storage: DeleteStore requires an absolute path, got %q", absPath) + return "", "", fmt.Errorf("storage: %s requires an absolute path, got %q", op, absPath) } - cleanRoot := filepath.Clean(root) - cleanPath := filepath.Clean(absPath) + cleanRoot = filepath.Clean(root) + cleanPath = filepath.Clean(absPath) if cleanPath == cleanRoot { - return fmt.Errorf("storage: refusing to delete the repos root %q", cleanRoot) + return "", "", fmt.Errorf("storage: %s refuses to act on the repos root %q", op, cleanRoot) } rel, err := filepath.Rel(cleanRoot, cleanPath) if err != nil { - return fmt.Errorf("storage: DeleteStore rel(%q, %q): %w", cleanRoot, cleanPath, err) + return "", "", fmt.Errorf("storage: %s rel(%q, %q): %w", op, cleanRoot, cleanPath, err) } if rel == ".." || strings.HasPrefix(rel, ".."+string(os.PathSeparator)) { - return fmt.Errorf("storage: refusing to delete %q outside repos root %q", cleanPath, cleanRoot) + return "", "", fmt.Errorf("storage: %s refuses %q outside repos root %q", op, cleanPath, cleanRoot) + } + return cleanRoot, cleanPath, nil +} + +// DeleteStore removes the store directory at absPath. It refuses to delete +// anything that is not strictly contained within root, guarding against a +// corrupted or attacker-controlled path escaping the configured repos root. +// Both root and absPath must be absolute. +func DeleteStore(ctx context.Context, root, absPath string) error { + _, cleanPath, err := containedPath("DeleteStore", root, absPath) + if err != nil { + return err } if err := os.RemoveAll(cleanPath); err != nil { @@ -159,3 +173,51 @@ func DeleteStore(ctx context.Context, root, absPath string) error { } return nil } + +// MoveStore relocates the store directory at srcPath to dstPath, the on-disk +// half of a rename. Both paths must be absolute and strictly inside root. +// +// It never overwrites: an existing dstPath is refused before anything is +// touched, because os.Rename over an empty destination directory would succeed +// silently and swallow it. A missing srcPath is likewise an error rather than a +// no-op — a rename whose store never moved would leave the metadata row +// pointing at nothing. +// +// The move itself is one os.Rename, so within a filesystem it is atomic: the +// store is either wholly at the old path or wholly at the new one, never half +// copied. Open handles on the old directory survive it (the inodes move, not +// the files), but they keep resolving new writes against the old path string, +// so the caller must still evict any memoized handle — see Cache.Evict. +func MoveStore(ctx context.Context, root, srcPath, dstPath string) error { + _, cleanSrc, err := containedPath("MoveStore", root, srcPath) + if err != nil { + return err + } + _, cleanDst, err := containedPath("MoveStore", root, dstPath) + if err != nil { + return err + } + if cleanSrc == cleanDst { + return fmt.Errorf("storage: MoveStore source and destination are the same path %q", cleanSrc) + } + + if _, err := os.Stat(cleanSrc); err != nil { + return fmt.Errorf("storage: MoveStore source %q: %w", cleanSrc, err) + } + if _, err := os.Stat(cleanDst); err == nil { + return fmt.Errorf("storage: MoveStore refuses to overwrite an existing store at %q", cleanDst) + } else if !os.IsNotExist(err) { + return fmt.Errorf("storage: MoveStore destination %q: %w", cleanDst, err) + } + + // The owner directory ("/~") already exists for any store that + // is being renamed within its owner's namespace, but creating it keeps the + // operation correct if a future caller ever moves a store across owners. + if err := os.MkdirAll(filepath.Dir(cleanDst), 0o755); err != nil { + return fmt.Errorf("storage: create destination dir %q: %w", filepath.Dir(cleanDst), err) + } + if err := os.Rename(cleanSrc, cleanDst); err != nil { + return fmt.Errorf("storage: move store %q -> %q: %w", cleanSrc, cleanDst, err) + } + return nil +} diff --git a/storage/move_test.go b/storage/move_test.go new file mode 100644 index 0000000000000000000000000000000000000000..8859b852900ad6d5bcf37ebfbf74e3d57c15d241 --- /dev/null +++ b/storage/move_test.go @@ -0,0 +1,150 @@ +package storage + +import ( + "context" + "os" + "path/filepath" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// A moved store must still be a store: MoveStore relocates the directory whole, +// so the same chunks open at the new path. +func TestMoveStoreMovesAServableStore(t *testing.T) { + ctx := context.Background() + root := t.TempDir() + src := RepoDiskPath(root, "alice", "widgets") + dst := RepoDiskPath(root, "alice", "gadgets") + require.NoError(t, InitStore(ctx, src, "alice", "a@b.test")) + + require.NoError(t, MoveStore(ctx, root, src, dst)) + + _, err := os.Stat(src) + assert.True(t, os.IsNotExist(err), "the old path must be gone, stat err = %v", err) + + cache := NewCache(func(context.Context, string, string) (string, error) { return dst, nil }) + defer cache.Close() + cs, err := cache.Get(ctx, "~alice/gadgets", nbfVer) + require.NoError(t, err, "the moved store must open at its new path") + assert.NotNil(t, cs) +} + +// The web rename evicts the memoized handle *after* the directory has moved, +// so eviction has to survive closing a store whose files are no longer where it +// opened them. If it did not, every rename of a database that had been served +// once would end in the eviction-failure branch — a 500 on the routine path +// instead of on the exceptional one. +func TestEvictAfterMoveClosesCleanly(t *testing.T) { + ctx := context.Background() + root := t.TempDir() + src := RepoDiskPath(root, "alice", "widgets") + dst := RepoDiskPath(root, "alice", "gadgets") + require.NoError(t, InitStore(ctx, src, "alice", "a@b.test")) + + cache := NewCache(func(_ context.Context, _, name string) (string, error) { + if name == "widgets" { + return src, nil + } + return dst, nil + }) + defer cache.Close() + + _, err := cache.Get(ctx, "~alice/widgets", nbfVer) // memoize a handle on the old path + require.NoError(t, err) + + require.NoError(t, MoveStore(ctx, root, src, dst)) + require.NoError(t, cache.Evict(src), "evicting a handle whose store has moved must not fail") + + // And the moved store is servable again through its new path. + _, err = cache.Get(ctx, "~alice/gadgets", nbfVer) + require.NoError(t, err) +} + +// os.Rename onto an existing empty directory succeeds and swallows it, so the +// destination is checked before anything is touched. +func TestMoveStoreRefusesAnExistingDestination(t *testing.T) { + ctx := context.Background() + root := t.TempDir() + src := RepoDiskPath(root, "alice", "widgets") + dst := RepoDiskPath(root, "alice", "gadgets") + require.NoError(t, InitStore(ctx, src, "alice", "a@b.test")) + + for _, tc := range []struct { + name string + setup func(t *testing.T) + }{ + {"empty directory", func(t *testing.T) { require.NoError(t, os.MkdirAll(dst, 0o755)) }}, + {"another store", func(t *testing.T) { require.NoError(t, InitStore(ctx, dst, "alice", "a@b.test")) }}, + } { + t.Run(tc.name, func(t *testing.T) { + require.NoError(t, os.RemoveAll(dst)) + tc.setup(t) + + require.Error(t, MoveStore(ctx, root, src, dst), "MoveStore must refuse an occupied destination") + assert.DirExists(t, src, "a refused move must leave the source where it was") + assert.DirExists(t, dst, "a refused move must leave the destination alone") + }) + } +} + +// A source that is not there means the rename would leave the metadata row +// pointing at nothing, so it is an error rather than a silent success. +func TestMoveStoreRefusesAMissingSource(t *testing.T) { + ctx := context.Background() + root := t.TempDir() + + err := MoveStore(ctx, root, RepoDiskPath(root, "alice", "ghost"), RepoDiskPath(root, "alice", "gadgets")) + require.Error(t, err) + assert.NoDirExists(t, RepoDiskPath(root, "alice", "gadgets")) +} + +func TestMoveStoreRefusesTheSamePath(t *testing.T) { + ctx := context.Background() + root := t.TempDir() + src := RepoDiskPath(root, "alice", "widgets") + require.NoError(t, InitStore(ctx, src, "alice", "a@b.test")) + + require.Error(t, MoveStore(ctx, root, src, src)) + assert.DirExists(t, src) +} + +// Both ends of the move are contained: neither a source dragged in from outside +// the repos root nor a destination pointed outside it may be acted on. +func TestMoveStoreRootEscapeGuard(t *testing.T) { + ctx := context.Background() + root := t.TempDir() + outside := t.TempDir() + + canary := filepath.Join(outside, "keep") + require.NoError(t, os.WriteFile(canary, []byte("x"), 0o644)) + + inside := RepoDiskPath(root, "alice", "widgets") + require.NoError(t, InitStore(ctx, inside, "alice", "a@b.test")) + + for name, tc := range map[string]struct{ src, dst string }{ + "source outside the root": {outside, RepoDiskPath(root, "alice", "gadgets")}, + "destination outside": {inside, filepath.Join(outside, "stolen")}, + "traversal destination": {inside, filepath.Join(root, "..", filepath.Base(outside), "stolen")}, + "the root as a source": {root, RepoDiskPath(root, "alice", "gadgets")}, + "the root as a destination": {inside, root}, + "prefix-not-subdir dest": {inside, root + "-evil"}, + "relative source": {"rel/path", RepoDiskPath(root, "alice", "gadgets")}, + "relative destination": {inside, "rel/path"}, + "relative root with abs ends": {inside, RepoDiskPath(root, "alice", "gadgets")}, + } { + t.Run(name, func(t *testing.T) { + useRoot := root + if name == "relative root with abs ends" { + useRoot = "rel/root" + } + require.Error(t, MoveStore(ctx, useRoot, tc.src, tc.dst), + "MoveStore(%q, %q) should have been refused", tc.src, tc.dst) + }) + } + + assert.FileExists(t, canary, "a refused move disturbed something outside the repos root") + assert.DirExists(t, inside, "a refused move disturbed the store it named") + assert.NoDirExists(t, RepoDiskPath(root, "alice", "gadgets")) +} diff --git a/web/adapters.go b/web/adapters.go index e6f3a697af7794012fbf4ebf845d7e38c83322ef..4e0631e12a2eadf31011dc2e774aa97427e7e8ab 100644 --- a/web/adapters.go +++ b/web/adapters.go @@ -51,6 +51,10 @@ func (DBAdapter) UpdateRepo(ctx context.Context, id int, description string, vis return db.FromContext(ctx).UpdateRepo(ctx, id, description, visibility) } +func (DBAdapter) RenameRepo(ctx context.Context, id int, name, path string) error { + return db.FromContext(ctx).RenameRepo(ctx, id, name, path) +} + func (DBAdapter) DeleteRepo(ctx context.Context, id int) error { return db.FromContext(ctx).DeleteRepo(ctx, id) } diff --git a/web/deps.go b/web/deps.go index 0fd9d26700668471b3238c1919f8829bd3a47739..b6eaae544dcd09e40bff740c5fd96bce07b27647 100644 --- a/web/deps.go +++ b/web/deps.go @@ -98,6 +98,10 @@ type StoreManager interface { InitEmptyStore(ctx context.Context, absPath string) error // DeleteStore removes the store at absPath, refusing anything outside root. DeleteStore(ctx context.Context, root, absPath string) error + // MoveStore relocates the store at srcPath to dstPath — the on-disk half of + // a rename — refusing anything outside root and never overwriting an + // existing destination. + MoveStore(ctx context.Context, root, srcPath, dstPath string) error // Evict closes and drops any memoized served handle for diskPath, so a // recreation at the same path never reuses a stale store. Evict(diskPath string) error @@ -122,6 +126,9 @@ type RepoStore interface { ListReposForViewer(ctx context.Context, viewer *core.Caller) ([]*core.Repo, error) ListReposForDashboard(ctx context.Context, userID int) ([]*core.Repo, error) UpdateRepo(ctx context.Context, id int, description string, visibility core.Visibility) error + // RenameRepo moves the row to a new name and on-disk path together; the + // store on disk is moved separately by StoreManager.MoveStore. + RenameRepo(ctx context.Context, id int, name, path string) error DeleteRepo(ctx context.Context, id int) error EffectiveAccess(ctx context.Context, userID, repoID int) (*core.AccessMode, error) diff --git a/web/handlers_settings.go b/web/handlers_settings.go index 44eb2d3436def60b444c4f5f71c20496be66dffb..6e624cfa6c1819c338be23b23d8f380a1aa29f28 100644 --- a/web/handlers_settings.go +++ b/web/handlers_settings.go @@ -80,20 +80,30 @@ func (a *app) renderSettings(w http.ResponseWriter, r *http.Request, status int, a.render(w, status, "settings", view) } -// handleSettings renders the settings page (description/visibility, ACLs, danger -// zone). Owner only. +// handleSettings renders the settings page (name, description/visibility, ACLs, +// danger zone). Owner only. +// +// A completed rename lands here by redirect rather than by rendering in place, +// so the browser's address bar carries the new name; the "renamed" query +// parameter is how the notice survives that redirect. It is echoed back to the +// page, so it is name-validated first — the value is a redirect target we wrote +// ourselves, but nothing stops a reader from hand-editing the URL. func (a *app) handleSettings(w http.ResponseWriter, r *http.Request) { repo, _, ok := a.loadRepoForAdmin(w, r) if !ok { return } - a.renderSettings(w, r, http.StatusOK, repo, "", "") + notice := "" + if from := r.URL.Query().Get("renamed"); from != "" && core.ValidateName(from) == nil { + notice = "Renamed from " + from + "." + } + a.renderSettings(w, r, http.StatusOK, repo, "", notice) } // handleSettingsPost dispatches the settings form on its "action" field: -// update (description + visibility), acl_add, acl_remove, or delete. Owner -// only; the same-origin check is the router's (csrf.Require) and has already -// run. +// update (description + visibility), rename, acl_add, acl_remove, or delete. +// Owner only; the same-origin check is the router's (csrf.Require) and has +// already run. func (a *app) handleSettingsPost(w http.ResponseWriter, r *http.Request) { repo, _, ok := a.loadRepoForAdmin(w, r) if !ok { @@ -108,6 +118,8 @@ func (a *app) handleSettingsPost(w http.ResponseWriter, r *http.Request) { switch form.Get("action") { case "update": a.settingsUpdate(w, r, repo, form) + case "rename": + a.settingsRename(w, r, repo, form) case "acl_add": a.settingsACLAdd(w, r, repo, form) case "acl_remove": @@ -136,6 +148,88 @@ func (a *app) settingsUpdate(w http.ResponseWriter, r *http.Request, repo *core. a.renderSettings(w, r, http.StatusOK, repo, "", "Settings saved.") } +// settingsRename moves the database to a new name. A database is one metadata +// row plus one on-disk store directory, and the name is written into both — the +// row's name column and its path, which storage.RepoDiskPath derives from +// (owner, name). Both must move, and the served handle memoized under the old +// path must go with them. +// +// The order mirrors creation, which inserts the row before it touches disk: the +// row moves first, so a name already taken is caught by the unique index while +// nothing on disk has changed, and once it has moved no request can re-open the +// store under the old path behind us. A failed store move then rolls the row +// back, so metadata and disk never disagree about where a database lives. +// +// Renaming does not leave a redirect behind: the old address stops resolving, +// exactly as it does on git.sr.ht, and clones pointing at it must have their +// remote updated. A companion database provisioned from a git repository will +// also be re-created under its old name by the next push to that repository — +// the hook provisions by the git repo's name, which this rename does not touch. +func (a *app) settingsRename(w http.ResponseWriter, r *http.Request, repo *core.Repo, form url.Values) { + newName := strings.TrimSpace(form.Get("name")) + if newName == repo.Name { + a.renderSettings(w, r, http.StatusOK, repo, "", "That is already the name of this database.") + return + } + if err := core.ValidateName(newName); err != nil { + a.renderSettings(w, r, http.StatusBadRequest, repo, err.Error(), "") + return + } + + oldName, oldPath := repo.Name, repo.Path + newPath := a.cfg.RepoDiskPath(repo.OwnerName, newName) + + if err := a.cfg.Repos.RenameRepo(r.Context(), repo.ID, newName, newPath); err != nil { + switch { + case errors.Is(err, db.ErrNameTaken): + a.renderSettings(w, r, http.StatusConflict, repo, + "You already have a database named "+newName+".", "") + case errors.Is(err, db.ErrNotFound): + a.notFound(w, r) + default: + http.Error(w, "failed to rename database", http.StatusInternalServerError) + } + return + } + + if err := a.cfg.Stores.MoveStore(r.Context(), a.cfg.ReposRoot, oldPath, newPath); err != nil { + // The store-layer error names on-disk paths, which this surface never + // discloses; the reader gets the fact that matters to them — the rename + // did not happen — and the detail goes to the log against the id. + slog.Error("moving a database's on-disk store failed; rolling the renamed record back", + "component", "web", "database", repo.ID, scribe.Err(err)) + if rerr := a.cfg.Repos.RenameRepo(r.Context(), repo.ID, oldName, oldPath); rerr != nil { + // Both halves failed: the row now names a database whose store is + // still at the old path, which no later request can repair on its + // own. This is the one outcome worth escalating to a human. + slog.Error("rolling a database's renamed record back failed; the record and its store disagree", + "component", "web", "database", repo.ID, scribe.Err(rerr)) + http.Error(w, "The database record was renamed, but its on-disk store could not be moved and the record could not be restored. Contact support.", + http.StatusInternalServerError) + return + } + a.renderSettings(w, r, http.StatusInternalServerError, repo, + "The database could not be renamed: its on-disk store could not be moved.", "") + return + } + + repo.Name, repo.Path = newName, newPath + + if err := a.cfg.Stores.Evict(oldPath); err != nil { + // The rename itself is done — record and store are both at the new name + // — and only the memoized handle for the old path outlived it. Kept + // distinct from the failures above for that reason. + slog.Error("evicting a database's cached store handle failed after it was renamed", + "component", "web", "database", repo.ID, scribe.Err(err)) + http.Error(w, "The database was renamed, but the cached handle for its old location could not be evicted. Contact support.", + http.StatusInternalServerError) + return + } + + http.Redirect(w, r, "/~"+repo.OwnerName+"/"+newName+"/settings?renamed="+url.QueryEscape(oldName), + http.StatusSeeOther) +} + // settingsACLAdd grants (or updates) an ACL entry for a username. The grantee is // resolved via the user resolver, which mirrors the meta profile on first sight. func (a *app) settingsACLAdd(w http.ResponseWriter, r *http.Request, repo *core.Repo, form url.Values) { diff --git a/web/templates/settings.html b/web/templates/settings.html index c835f7a73f434d0ea6558c71fd96f8da6bd20266..54268caa0ef611052128b8f42a8db16d35463757 100644 --- a/web/templates/settings.html +++ b/web/templates/settings.html @@ -25,6 +25,22 @@ +
+

Rename

+

+ Renaming moves both this database's record and its stored data. The old + address stops working — no redirect is left behind — so anything cloning or + pushing to ~{{.Repo.OwnerName}}/{{.Repo.Name}} must have its + remote updated. +

+
+ + + +
+

Access control

{{if .ACL}} diff --git a/web/web_test.go b/web/web_test.go index 421eed877da35a4a29ade0dba1bf5f2ec2e70e01..e3d2be19533a8b860cec4a66b9adac7f9c75c12a 100644 --- a/web/web_test.go +++ b/web/web_test.go @@ -54,9 +54,19 @@ type fakeStore struct { // answering — the metadata store unreachable rather than the row missing, // which the pages that resolve one database must not confuse with a // database that does not exist. - getErr error + getErr error + // renameErr, when set, makes RenameRepo fail with it — the metadata half of + // a rename refusing, which must leave the store on disk untouched. + // renameErrAfter lets that many calls through first, which is how a test + // reaches the case where the rename landed and only its rollback fails. + renameErr error + renameErrAfter int + createdCalls []*core.Repo deletedRepos []int + // renameCalls records the new name of every RenameRepo that landed, so a + // rollback (a second call back to the old name) is visible to a test. + renameCalls []string } func newFakeStore() *fakeStore { @@ -160,6 +170,28 @@ func (f *fakeStore) UpdateRepo(_ context.Context, id int, description string, vi return nil } +// RenameRepo mirrors the real store: name and path move together, and the +// owner's namespace is the uniqueness scope, so a taken name is ErrNameTaken. +// renameErr forces the metadata half to fail outright. +func (f *fakeStore) RenameRepo(_ context.Context, id int, name, path string) error { + if f.renameErr != nil && len(f.renameCalls) >= f.renameErrAfter { + return f.renameErr + } + r, ok := f.byID[id] + if !ok { + return db.ErrNotFound + } + if other, taken := f.repos[r.OwnerName+"/"+name]; taken && other.ID != id { + return db.ErrNameTaken + } + delete(f.repos, r.OwnerName+"/"+r.Name) + r.Name = name + r.Path = path + f.repos[r.OwnerName+"/"+name] = r + f.renameCalls = append(f.renameCalls, name) + return nil +} + func (f *fakeStore) DeleteRepo(_ context.Context, id int) error { r, ok := f.byID[id] if !ok { @@ -248,6 +280,7 @@ func (f *fakeStore) DeleteKey(_ context.Context, id, userID int) error { type fakeStoreManager struct { initErr error deleteErr error + moveErr error evictErr error // initCalls records InitStore (with an initial commit); initEmptyCalls // records InitEmptyStore. They are separate so a test can say which of the @@ -255,7 +288,9 @@ type fakeStoreManager struct { initCalls []string initEmptyCalls []string deleteCalls []string - evictCalls []string + // moveCalls records each move as "src -> dst". + moveCalls []string + evictCalls []string } func (m *fakeStoreManager) InitStore(_ context.Context, absPath, _, _ string) error { @@ -270,6 +305,13 @@ func (m *fakeStoreManager) DeleteStore(_ context.Context, _, absPath string) err m.deleteCalls = append(m.deleteCalls, absPath) return m.deleteErr } +func (m *fakeStoreManager) MoveStore(_ context.Context, _, srcPath, dstPath string) error { + if m.moveErr != nil { + return m.moveErr + } + m.moveCalls = append(m.moveCalls, srcPath+" -> "+dstPath) + return nil +} func (m *fakeStoreManager) Evict(diskPath string) error { m.evictCalls = append(m.evictCalls, diskPath) return m.evictErr @@ -1001,6 +1043,162 @@ func TestSettingsDeleteDoesNotPrintTheStoreError(t *testing.T) { }) } +// newRenameHarness is one owner with one database at ~owner/db, ready to be +// renamed. +func newRenameHarness(t *testing.T) (*harness, *core.Repo, *auth.AuthContext) { + t.Helper() + h := newHarness(t) + repo := h.store.add(&core.Repo{Name: "db", OwnerID: 10, OwnerName: "owner", + Path: "/var/lib/dolt/~owner/db", Visibility: core.VisibilityPublic}) + return h, repo, testCaller(10, "owner") +} + +func rename(h *harness, caller *auth.AuthContext, from, to string) *httptest.ResponseRecorder { + return h.do("POST", "/~owner/"+from+"/settings", caller, + url.Values{"action": {"rename"}, "name": {to}}) +} + +// A rename has to move three things together: the metadata row's name, the +// row's on-disk path, and the store directory that path names — and then drop +// the served handle memoized under the old path, or a push would keep landing +// in a store nothing resolves to any more. +func TestSettingsRename(t *testing.T) { + h, repo, owner := newRenameHarness(t) + + rec := rename(h, owner, "db", "widgets") + require.Equal(t, http.StatusSeeOther, rec.Code) + assert.Equal(t, "/~owner/widgets/settings?renamed=db", rec.Header().Get("Location")) + + assert.Equal(t, "widgets", repo.Name) + assert.Equal(t, "/var/lib/dolt/~owner/widgets", repo.Path, + "the row's path must follow the name, or the row would be served from the old store") + assert.Equal(t, []string{"/var/lib/dolt/~owner/db -> /var/lib/dolt/~owner/widgets"}, h.stores.moveCalls) + assert.Equal(t, []string{"/var/lib/dolt/~owner/db"}, h.stores.evictCalls, + "the handle memoized under the old path must be evicted") + + // The old address stops resolving and the new one answers, notice included. + assert.Equal(t, http.StatusNotFound, h.do("GET", "/~owner/db", owner, nil).Code) + after := h.do("GET", "/~owner/widgets/settings?renamed=db", owner, nil) + require.Equal(t, http.StatusOK, after.Code) + assert.Contains(t, after.Body.String(), "Renamed from db.") +} + +// Every refusal must be total: nothing on disk moves, and the row keeps both +// its name and its path. +func TestSettingsRenameRefusals(t *testing.T) { + for _, tc := range []struct { + name string + to string + want int + body string + }{ + {"invalid name", "bad name!", http.StatusBadRequest, "must match"}, + {"traversal", "..", http.StatusBadRequest, "not allowed"}, + {"empty", "", http.StatusBadRequest, "must not be empty"}, + {"name already taken", "taken", http.StatusConflict, "You already have a database named taken."}, + } { + t.Run(tc.name, func(t *testing.T) { + h, repo, owner := newRenameHarness(t) + h.store.add(&core.Repo{Name: "taken", OwnerID: 10, OwnerName: "owner", + Path: "/var/lib/dolt/~owner/taken", Visibility: core.VisibilityPublic}) + + rec := rename(h, owner, "db", tc.to) + require.Equal(t, tc.want, rec.Code) + assert.Contains(t, rec.Body.String(), tc.body) + assert.Equal(t, "db", repo.Name) + assert.Equal(t, "/var/lib/dolt/~owner/db", repo.Path) + assert.Empty(t, h.stores.moveCalls, "a refused rename must not touch disk") + assert.Empty(t, h.stores.evictCalls) + }) + } + + t.Run("the name it already has", func(t *testing.T) { + h, repo, owner := newRenameHarness(t) + rec := rename(h, owner, "db", "db") + require.Equal(t, http.StatusOK, rec.Code) + assert.Contains(t, rec.Body.String(), "That is already the name of this database.") + assert.Equal(t, "db", repo.Name) + assert.Empty(t, h.store.renameCalls, "a no-op rename must not write the row") + assert.Empty(t, h.stores.moveCalls) + }) + + t.Run("not the owner", func(t *testing.T) { + h, repo, _ := newRenameHarness(t) + rec := rename(h, testCaller(99, "intruder"), "db", "widgets") + require.Equal(t, http.StatusForbidden, rec.Code) + assert.Equal(t, "db", repo.Name) + assert.Empty(t, h.stores.moveCalls) + }) +} + +// The row moves before the store does, so a store that will not move leaves a +// row naming a database whose data is somewhere else. The rename is rolled back +// rather than left that way — and, as on the delete path, the store layer's own +// error text (which carries on-disk paths) never reaches the reader. +func TestSettingsRenameRollsBackWhenTheStoreCannotMove(t *testing.T) { + h, repo, owner := newRenameHarness(t) + h.stores.moveErr = errors.New(storeDetail) + + rec := rename(h, owner, "db", "widgets") + require.Equal(t, http.StatusInternalServerError, rec.Code) + + assert.Equal(t, "db", repo.Name, "the row must be back where it started") + assert.Equal(t, "/var/lib/dolt/~owner/db", repo.Path) + assert.Equal(t, []string{"widgets", "db"}, h.store.renameCalls, + "the rename must have been attempted and then undone") + assert.Empty(t, h.stores.evictCalls, "nothing moved, so nothing needs evicting") + + body := rec.Body.String() + assert.Contains(t, body, "The database could not be renamed") + assert.NotContains(t, body, storeDetail) + assert.NotContains(t, body, "/var/lib/dolt", "the store's path must not reach the reader") +} + +// Both halves failing is the one outcome no later request can repair by itself: +// the row names the new database and the store is still at the old path. It is +// reported as such instead of as a plain failed rename. +func TestSettingsRenameRollbackFailure(t *testing.T) { + h, _, owner := newRenameHarness(t) + h.stores.moveErr = errors.New(storeDetail) + h.store.renameErr = errors.New("connection reset by peer") + h.store.renameErrAfter = 1 // the rename lands; only its rollback fails + + rec := rename(h, owner, "db", "widgets") + require.Equal(t, http.StatusInternalServerError, rec.Code) + body := rec.Body.String() + assert.Contains(t, body, "the record could not be restored. Contact support.") + assert.NotContains(t, body, storeDetail) + assert.NotContains(t, body, "connection reset by peer") +} + +// The store moved and the row moved; only the cached handle for the old path +// outlived them. A distinct fact from the two above, told distinctly. +func TestSettingsRenameEvictionFailure(t *testing.T) { + h, repo, owner := newRenameHarness(t) + h.stores.evictErr = errors.New(storeDetail) + + rec := rename(h, owner, "db", "widgets") + require.Equal(t, http.StatusInternalServerError, rec.Code) + assert.Equal(t, "widgets", repo.Name, "the rename itself did happen") + body := rec.Body.String() + assert.Contains(t, body, "The database was renamed, but the cached handle") + assert.NotContains(t, body, storeDetail) + assert.NotContains(t, body, "/var/lib/dolt") +} + +// The notice is built from a query parameter, so it is a reader-controlled +// string on a page the owner is looking at. Only something that could really be +// a database name is echoed back. +func TestSettingsRenamedNoticeIgnoresJunk(t *testing.T) { + h, _, owner := newRenameHarness(t) + + rec := h.do("GET", "/~owner/db/settings?renamed="+url.QueryEscape(""), owner, nil) + require.Equal(t, http.StatusOK, rec.Code) + body := rec.Body.String() + assert.NotContains(t, body, "Renamed from") + assert.NotContains(t, body, "") +} + func TestSettingsACLAddRemove(t *testing.T) { h := newHarness(t) repo := h.store.add(&core.Repo{Name: "db", OwnerID: 10, OwnerName: "owner", Path: "/d", Visibility: core.VisibilityPublic})