diff --git a/cmd/keeporg/commands.go b/cmd/keeporg/commands.go
index 19b9ea2..0479ed9 100644
--- a/cmd/keeporg/commands.go
+++ b/cmd/keeporg/commands.go
@@ -412,7 +412,8 @@ func cmdDB(ctx context.Context, args []string) error {
"merge strategy: merge, server-wins, client-wins or manual")
id := fs.String("id", "", "database id, for -strategy, -rename or -delete")
rename := fs.String("rename", "", "new name for -id")
- del := fs.Bool("delete", false, "delete the database given by -id")
+ del := fs.Bool("delete", false,
+ "delete the database given by -id, every stored version of it included")
storePassword := fs.Bool("store-password", false,
"give the server this vault's password so it can merge the vault")
forgetPassword := fs.Bool("forget-password", false,
@@ -463,7 +464,19 @@ func cmdDB(ctx context.Context, args []string) error {
}
fmt.Printf("%s now uses %s\n", db.Name, db.MergeStrategy)
case *id != "" && *del:
- if err := api.DeleteDatabase(ctx, *id); err != nil {
+ password, perr := readPassword("Your account password: ")
+ if perr != nil {
+ return perr
+ }
+ err := api.DeleteDatabase(ctx, *id, password, "")
+ if client.Code(err) == "totp_required" || client.Code(err) == "totp_invalid" {
+ code, cerr := prompt("One-time code: ")
+ if cerr != nil {
+ return cerr
+ }
+ err = api.DeleteDatabase(ctx, *id, password, code)
+ }
+ if err != nil {
return err
}
fmt.Println("deleted on the server; local files were left in place")
diff --git a/docs/HANDOVER.md b/docs/HANDOVER.md
index d23e8b5..c17b34c 100644
--- a/docs/HANDOVER.md
+++ b/docs/HANDOVER.md
@@ -353,6 +353,7 @@ browsing, by design.
| Page | What it does |
| --- | --- |
| `/web/vaults` | the vaults you can reach, the create-a-vault form, your own account password |
+| `/web/vaults/{id}/delete` | what deleting a vault destroys, and the password that confirms it |
| `/web/groups` | the group tree you can see, and a form to add a group inside one you administer |
| `/web/groups/{id}` | one group: members and their roles, the vaults it reaches and where that access comes from, grants given and revoked, deletion |
| `/web/users` | the accounts you may see; for a server administrator, disable, enable, clear the second factor, promote, demote, delete |
@@ -393,6 +394,25 @@ No page carries any JavaScript. Each control is its own small form with the
CSRF token and one action in it, which is what makes the interface work in a
locked-down browser and keeps its attack surface down to that token.
+**Deleting a vault asks for the password again.** Every other control on
+these pages is undoable or additive; this one erases every stored version of
+a file. A live session is what an unlocked laptop leaves lying around, so the
+confirmation page states what is about to go — owner, versions, grants — and
+takes the administrator's own account password, plus their one-time code when
+they have one. The same rule covers the API and `keeporg db -delete`, because
+it lives in `confirmDeletion` rather than in the page.
+
+An account that signs in only through an identity provider has no password to
+type. Refusing those accounts would mean a deployment where nobody can delete
+anything, so they confirm by typing the vault's exact name. That is a weaker
+gate, and it is deliberately the fallback rather than the rule.
+
+Deleting takes the record, the grants pointing at it, every stored version
+and — for a merge vault — the key the server held. It does not touch the
+copies in people's mirror directories: the server stops tracking the vault, it
+does not reach into anybody's home directory. Clients are told over SSE, so
+they stop expecting the file rather than discovering it at the next save.
+
Creating a vault is one transaction: the server builds an empty KDBX 4
database, encrypts it with the password from the form, stores it as version
1, and — for a merge vault — wraps the password under the master key. If the
diff --git a/docs/PROTOCOL.md b/docs/PROTOCOL.md
index b23aa29..9e29971 100644
--- a/docs/PROTOCOL.md
+++ b/docs/PROTOCOL.md
@@ -116,7 +116,7 @@ the group named or any group above it.
| DELETE | `/api/v1/groups/{id}/members/{user}` | – | group admin |
| POST | `/api/v1/databases` | `{name, owner_group_id, merge_strategy}` | group admin of the owner |
| PATCH | `/api/v1/databases/{id}` | `{name?, merge_strategy?}` | database admin |
-| DELETE | `/api/v1/databases/{id}` | – | database admin; deletes every stored version |
+| DELETE | `/api/v1/databases/{id}` | `{password, totp?}` | database admin, re-proving their own password; deletes every stored version |
| POST | `/api/v1/grants` | `{group_id, database_id, perm}` | database admin |
| DELETE | `/api/v1/grants` | `?group_id=&database_id=` | database admin |
| POST | `/api/v1/invites` | `{group_id, role, ttl_hours, max_uses}` | group admin |
@@ -131,6 +131,14 @@ Switching to `merge` requires a stored key and is otherwise refused with
`400`. `perm` is one of `read`, `write`, `admin`. `role` is `member` or
`admin`.
+Deleting a database is the one call that asks for credentials again. A
+session token is enough to read and write a vault and not enough to destroy
+every version of it, so the body carries the caller's own account password,
+and `totp` as well when that account has a second factor enrolled — the
+server answers `401 totp_required` when it is missing. An account that signs
+in only through an identity provider has no password to send and passes
+`{"confirm_name": ""}` instead.
+
An account `action` is one of `disable`, `enable`, `promote`, `demote`,
`clear-totp` or `delete`. `disable` and `delete` also revoke every session
that account holds. The three that would lock the caller out — `disable`,
diff --git a/internal/client/admin.go b/internal/client/admin.go
index 36ee7b6..7ab3a1f 100644
--- a/internal/client/admin.go
+++ b/internal/client/admin.go
@@ -103,8 +103,13 @@ func (a *API) RenameDatabase(ctx context.Context, dbID, name string) (*Database,
map[string]string{"name": name}, &out)
}
-func (a *API) DeleteDatabase(ctx context.Context, dbID string) error {
- return a.do(ctx, http.MethodDelete, "/api/v1/databases/"+url.PathEscape(dbID), nil, nil)
+// DeleteDatabase removes a database and every version of it stored on the
+// server. The caller's own account password is required, and the one-time
+// code too when that account has one enrolled; the server answers
+// totp_required when it is missing.
+func (a *API) DeleteDatabase(ctx context.Context, dbID, password, totp string) error {
+ return a.do(ctx, http.MethodDelete, "/api/v1/databases/"+url.PathEscape(dbID),
+ map[string]string{"password": password, "totp": totp}, nil)
}
func (a *API) SetGrant(ctx context.Context, groupID, dbID, perm string) error {
diff --git a/internal/server/admin.go b/internal/server/admin.go
index 2b7e426..cfc1328 100644
--- a/internal/server/admin.go
+++ b/internal/server/admin.go
@@ -19,6 +19,23 @@ var errForbidden = errors.New("forbidden")
// may do what, and the only durable way to make them agree is for both to
// call the same function.
+// denyAuth answers a request that failed to re-prove its credentials. The
+// cases stay distinct so a client knows to prompt for the factor it is
+// missing rather than reporting a flat refusal.
+func denyAuth(w http.ResponseWriter, err error) {
+ switch {
+ case errors.Is(err, errTOTPRequired):
+ fail(w, http.StatusUnauthorized, "totp_required", "second factor required")
+ case errors.Is(err, errTOTPInvalid):
+ fail(w, http.StatusUnauthorized, "totp_invalid", "second factor rejected")
+ case errors.Is(err, errBadCredentials):
+ fail(w, http.StatusUnauthorized, "bad_credentials",
+ "your own account password is required for this")
+ default:
+ deny(w, err)
+ }
+}
+
// deny maps a domain error onto a response.
func deny(w http.ResponseWriter, err error) {
switch {
@@ -457,29 +474,86 @@ func (s *Server) handlePatchDatabase(w http.ResponseWriter, r *http.Request, u *
writeJSON(w, http.StatusOK, out)
}
-func (s *Server) handleDeleteDatabase(w http.ResponseWriter, r *http.Request, u *store.User) {
- id := r.PathValue("id")
- var watchers []string
+// errConfirmName is the answer when an account with no password of its own
+// did not type the name of the thing it is about to destroy.
+var errConfirmName = errors.New("type the name exactly to confirm")
+
+// confirmDeletion re-proves who the caller is before something irreversible.
+// Holding a session is enough to read and write a vault; it is not enough to
+// destroy every version of one, because a session is what an unlocked laptop
+// leaves lying around.
+//
+// An account that signs in only through an identity provider has no password
+// to type. Refusing those accounts would mean a deployment where nobody can
+// delete anything, so they confirm by typing the exact name instead. That is
+// a weaker gate and it is deliberately the fallback, not the rule.
+func confirmDeletion(st *store.State, u *store.User, name, password, totpCode, typed string) error {
+ target := st.Users[u.ID]
+ if target == nil {
+ return errBadCredentials
+ }
+ if target.PassHash == "" {
+ if strings.TrimSpace(typed) != name {
+ return errConfirmName
+ }
+ return nil
+ }
+ _, err := checkPassword(st, target.Username, password, totpCode)
+ return err
+}
+
+// deleteDatabase removes a database, its grants and every stored version of
+// the file. Clients keep whatever they already have on disk: the server can
+// stop tracking a vault, but it has no business reaching into somebody's
+// home directory.
+func (s *Server) deleteDatabase(u *store.User, id, password, totpCode, typed string) (string, error) {
+ var (
+ name string
+ watchers []string
+ )
err := s.st.Update(func(st *store.State) error {
- if st.Databases[id] == nil {
+ db := st.Databases[id]
+ if db == nil {
return errors.New("no such database")
}
if st.DatabasePerm(u.ID, id) != store.PermAdmin {
return errForbidden
}
+ if cerr := confirmDeletion(st, u, db.Name, password, totpCode, typed); cerr != nil {
+ return cerr
+ }
+ name = db.Name
+ // Taken before the delete: the people to tell are the ones who are
+ // about to stop having access.
watchers = st.Watchers(id)
delete(st.Databases, id)
st.Grants = filterGrants(st.Grants, func(g store.Grant) bool { return g.DatabaseID != id })
return nil
})
if err != nil {
- deny(w, err)
- return
+ return "", err
}
- if err := s.blobs.Forget(id); err != nil {
- s.log.Printf("delete blobs for %s: %v", id, err)
+ if ferr := s.blobs.Forget(id); ferr != nil {
+ s.log.Printf("delete blobs for %s: %v", id, ferr)
}
s.hub.Publish(watchers, Event{Type: EventChanged, DatabaseID: id})
+ s.log.Printf("%s deleted database %q (%s) and every stored version", u.Username, name, id)
+ return name, nil
+}
+
+func (s *Server) handleDeleteDatabase(w http.ResponseWriter, r *http.Request, u *store.User) {
+ var req struct {
+ Password string `json:"password"`
+ TOTP string `json:"totp"`
+ Confirm string `json:"confirm_name"`
+ }
+ if r.ContentLength > 0 && !readJSON(w, r, &req) {
+ return
+ }
+ if _, err := s.deleteDatabase(u, r.PathValue("id"), req.Password, req.TOTP, req.Confirm); err != nil {
+ denyAuth(w, err)
+ return
+ }
writeJSON(w, http.StatusOK, map[string]string{"status": "deleted"})
}
diff --git a/internal/server/delete_test.go b/internal/server/delete_test.go
new file mode 100644
index 0000000..00b1800
--- /dev/null
+++ b/internal/server/delete_test.go
@@ -0,0 +1,237 @@
+package server
+
+import (
+ "net/http"
+ "net/url"
+ "os"
+ "path/filepath"
+ "strings"
+ "testing"
+ "time"
+
+ "cc.tiredsysadmin.keeporg/internal/secret"
+ "cc.tiredsysadmin.keeporg/internal/store"
+ "cc.tiredsysadmin.keeporg/internal/totp"
+)
+
+// blobsExist reports whether the server still keeps any version of a
+// database on disk.
+func (h *harness) blobsExist(dbID string) bool {
+ h.t.Helper()
+ _, err := os.Stat(filepath.Join(h.dataDir, "blobs", dbID))
+ return err == nil
+}
+
+func TestWebDeletesAVaultOnlyWithTheAdminsPassword(t *testing.T) {
+ h := newHarness(t)
+ b := newBrowser(t, h)
+ csrf := b.signIn()
+ admin, _, _ := h.login("admin", adminPassword, "")
+ id := h.makeVault(admin, "Doomed")
+ if !h.blobsExist(id) {
+ t.Fatal("the new vault has no stored version")
+ }
+ // A grant so the delete has something to clean up besides the record.
+ if status, body := h.call("POST", "/api/v1/grants", admin, map[string]any{
+ "group_id": "everyone", "database_id": id, "perm": "read",
+ }); status != http.StatusOK {
+ t.Fatalf("grant: %d %v", status, body)
+ }
+
+ status, page := b.get("/web/vaults/" + id + "/delete")
+ if status != http.StatusOK || !strings.Contains(page, "Delete Doomed.kdbx?") {
+ t.Fatalf("the confirmation page = %d\n%s", status, page)
+ }
+ if !strings.Contains(page, "Your account password") {
+ t.Fatalf("the page does not ask for a password:\n%s", page)
+ }
+
+ _, page = b.post("/web/vaults/"+id+"/delete", url.Values{
+ "csrf": {csrf}, "password": {"not-the-password"}})
+ if !strings.Contains(page, "not your account password") {
+ t.Fatalf("a wrong password was not reported:\n%s", page)
+ }
+ h.srv.Store().Read(func(st *store.State) {
+ if st.Databases[id] == nil {
+ t.Fatal("the vault was deleted on a wrong password")
+ }
+ })
+ if !h.blobsExist(id) {
+ t.Fatal("the stored versions went on a wrong password")
+ }
+
+ _, page = b.post("/web/vaults/"+id+"/delete", url.Values{
+ "csrf": {csrf}, "password": {adminPassword}})
+ if !strings.Contains(page, "Deleted Doomed") {
+ t.Fatalf("delete:\n%s", page)
+ }
+ if strings.Contains(page, "Doomed.kdbx") {
+ t.Fatal("the deleted vault is still listed")
+ }
+ h.srv.Store().Read(func(st *store.State) {
+ if st.Databases[id] != nil {
+ t.Fatal("the record survived")
+ }
+ for _, g := range st.Grants {
+ if g.DatabaseID == id {
+ t.Fatal("a grant outlived the vault")
+ }
+ }
+ })
+ if h.blobsExist(id) {
+ t.Fatal("the stored versions outlived the vault")
+ }
+}
+
+// Write access is not administration: it must not reach the page, and must
+// not delete even with the right password.
+func TestWebDeleteIsRefusedWithoutAdminRights(t *testing.T) {
+ h := newHarness(t)
+ b := newBrowser(t, h)
+ csrf := b.signIn()
+ admin, _, _ := h.login("admin", adminPassword, "")
+ id := h.makeVault(admin, "Theirs")
+
+ h.join(b.invite(csrf, "everyone", "member"), "wally", "wally-password-1")
+ if status, body := h.call("POST", "/api/v1/grants", admin, map[string]any{
+ "group_id": "everyone", "database_id": id, "perm": "write",
+ }); status != http.StatusOK {
+ t.Fatalf("grant: %d %v", status, body)
+ }
+
+ w := newBrowser(t, h)
+ wCSRF := w.signInAs("wally", "wally-password-1")
+ _, page := w.get("/web/vaults/" + id + "/delete")
+ if !strings.Contains(page, "you do not administer it") {
+ t.Fatalf("a writer reached the delete page:\n%s", page)
+ }
+ _, page = w.post("/web/vaults/"+id+"/delete", url.Values{
+ "csrf": {wCSRF}, "password": {"wally-password-1"}})
+ if !strings.Contains(page, "you do not administer it") {
+ t.Fatalf("a writer's delete was not refused:\n%s", page)
+ }
+ h.srv.Store().Read(func(st *store.State) {
+ if st.Databases[id] == nil {
+ t.Fatal("a writer deleted the vault")
+ }
+ })
+ // The vault list offers them no way in either.
+ _, page = w.get("/web/vaults")
+ if strings.Contains(page, "/delete") {
+ t.Fatalf("a writer was offered a delete link:\n%s", page)
+ }
+}
+
+func TestAPIDeleteNeedsThePassword(t *testing.T) {
+ h := newHarness(t)
+ admin, _, _ := h.login("admin", adminPassword, "")
+ id := h.makeVault(admin, "By API")
+
+ status, body := h.call("DELETE", "/api/v1/databases/"+id, admin, nil)
+ if status != http.StatusUnauthorized || body["code"] != "bad_credentials" {
+ t.Fatalf("delete with no password: %d %v", status, body)
+ }
+ status, body = h.call("DELETE", "/api/v1/databases/"+id, admin,
+ map[string]string{"password": "wrong"})
+ if status != http.StatusUnauthorized {
+ t.Fatalf("delete with a wrong password: %d %v", status, body)
+ }
+ h.srv.Store().Read(func(st *store.State) {
+ if st.Databases[id] == nil {
+ t.Fatal("the vault went without a password")
+ }
+ })
+ if status, body = h.call("DELETE", "/api/v1/databases/"+id, admin,
+ map[string]string{"password": adminPassword}); status != http.StatusOK {
+ t.Fatalf("delete with the password: %d %v", status, body)
+ }
+ if h.blobsExist(id) {
+ t.Fatal("the stored versions outlived the vault")
+ }
+}
+
+func TestDeleteAsksForTheSecondFactorToo(t *testing.T) {
+ h := newHarness(t)
+ admin, _, _ := h.login("admin", adminPassword, "")
+ id := h.makeVault(admin, "Guarded")
+
+ status, body := h.call("POST", "/api/v1/auth/totp/enroll", admin, nil)
+ if status != http.StatusOK {
+ t.Fatalf("enroll: %d %v", status, body)
+ }
+ sec := body["secret"].(string)
+ code, err := totp.Code(sec, time.Now())
+ if err != nil {
+ t.Fatal(err)
+ }
+ if status, body = h.call("POST", "/api/v1/auth/totp/confirm", admin,
+ map[string]string{"code": code}); status != http.StatusOK {
+ t.Fatalf("confirm: %d %v", status, body)
+ }
+
+ if status, body = h.call("DELETE", "/api/v1/databases/"+id, admin,
+ map[string]string{"password": adminPassword}); status != http.StatusUnauthorized ||
+ body["code"] != "totp_required" {
+ t.Fatalf("delete without a code: %d %v", status, body)
+ }
+ code, _ = totp.Code(sec, time.Now())
+ if status, body = h.call("DELETE", "/api/v1/databases/"+id, admin,
+ map[string]string{"password": adminPassword, "totp": code}); status != http.StatusOK {
+ t.Fatalf("delete with the code: %d %v", status, body)
+ }
+}
+
+// An account that only ever signs in through an identity provider has no
+// password to re-type, so it confirms with the vault's name instead.
+func TestSingleSignOnAccountConfirmsByName(t *testing.T) {
+ h := newHarness(t)
+ admin, _, _ := h.login("admin", adminPassword, "")
+ id := h.makeVault(admin, "Federated")
+
+ sso := &store.User{ID: secret.ID(), Username: "sso", OIDCIssuer: "https://id.example.com",
+ OIDCSubject: "abc", ServerAdmin: true, CreatedAt: time.Now()}
+ if err := h.srv.Store().Update(func(st *store.State) error {
+ st.Users[sso.ID] = sso
+ return nil
+ }); err != nil {
+ t.Fatal(err)
+ }
+
+ if _, err := h.srv.deleteDatabase(sso, id, "", "", ""); err != errConfirmName {
+ t.Fatalf("delete with nothing typed = %v", err)
+ }
+ if _, err := h.srv.deleteDatabase(sso, id, "", "", "federated"); err != errConfirmName {
+ t.Fatalf("delete with the wrong case = %v", err)
+ }
+ if _, err := h.srv.deleteDatabase(sso, id, "", "", "Federated"); err != nil {
+ t.Fatalf("delete with the name typed = %v", err)
+ }
+ h.srv.Store().Read(func(st *store.State) {
+ if st.Databases[id] != nil {
+ t.Fatal("the vault survived a confirmed delete")
+ }
+ })
+}
+
+// Deleting a vault tells everyone who could reach it, so their client stops
+// expecting the file rather than finding out at the next save.
+func TestDeleteNotifiesTheWatchers(t *testing.T) {
+ h := newHarness(t)
+ admin, _, _ := h.login("admin", adminPassword, "")
+ id := h.makeVault(admin, "Watched")
+
+ sub, events := h.srv.hub.Subscribe("admin")
+ defer h.srv.hub.Unsubscribe(sub)
+ if _, err := h.srv.deleteDatabase(&store.User{ID: "admin", Username: "admin"},
+ id, adminPassword, "", ""); err != nil {
+ t.Fatal(err)
+ }
+ select {
+ case ev := <-events:
+ if ev.DatabaseID != id || ev.Type != EventChanged {
+ t.Fatalf("event = %+v", ev)
+ }
+ case <-time.After(2 * time.Second):
+ t.Fatal("no event reached a watcher")
+ }
+}
diff --git a/internal/server/pages.go b/internal/server/pages.go
index cd8fbc9..be88a00 100644
--- a/internal/server/pages.go
+++ b/internal/server/pages.go
@@ -71,8 +71,10 @@ a{color:var(--accent)}
td form{display:inline;margin:0}
td button{width:auto;margin:0 .25rem .25rem 0;padding:.3rem .5rem;font-size:.8rem;font-weight:500;
background:transparent;color:var(--accent);border:1px solid var(--line)}
-td button.danger{color:#b02a37}
-@media(prefers-color-scheme:dark){td button.danger{color:#f2b8c0}}
+td button.danger,a.danger{color:#b02a37}
+button.destroy{background:#b02a37}
+@media(prefers-color-scheme:dark){td button.danger,a.danger{color:#f2b8c0}
+button.destroy{background:#a3313c}}
.inline{display:flex;gap:.6rem;align-items:flex-end;flex-wrap:wrap;margin:0}
.inline>div{flex:1 1 9rem}
.inline label{margin-top:0}
@@ -133,7 +135,7 @@ var vaultsTmpl = template.Must(template.Must(pages.Clone()).Parse(`
{{if .Notice}}
{{.Notice}}
{{end}}
{{if .Vaults}}
-
Vault
Access
On conflict
Version
Last saved
+
Vault
Access
On conflict
Version
Last saved
{{range .Vaults}}
{{.Name}}.kdbx
@@ -141,6 +143,7 @@ var vaultsTmpl = template.Must(template.Must(pages.Clone()).Parse(`
Every stored version of this file is erased from the server,
+along with the grants that point at it. Copies already in people's mirror
+directories stay on their computers: the server stops tracking the vault, it
+does not reach into anybody's home directory. If the vault is merged here,
+the key the server holds goes with it.