diff --git a/admin/admin.go b/admin/admin.go index 6484c0b..f997d01 100644 --- a/admin/admin.go +++ b/admin/admin.go @@ -10,6 +10,7 @@ import ( "encoding/pem" "errors" "fmt" + "io/fs" "log/slog" "os" "path/filepath" @@ -661,6 +662,15 @@ func AccountAdd(ctx context.Context, account, address string) (rerr error) { return fmt.Errorf("%w: account already present", ErrRequest) } + // Ensure the directory does not exist, e.g. due to pending account removal, or an + // otherwise failed cleanup. + accountDir := filepath.Join(mox.DataDirPath("accounts"), account) + if _, err := os.Stat(accountDir); err == nil { + return fmt.Errorf("%w: account directory %q already/still exists", ErrRequest, accountDir) + } else if !errors.Is(err, fs.ErrNotExist) { + return fmt.Errorf(`%w: stat account directory %q, expected "does not exist": %v`, ErrRequest, accountDir, err) + } + if err := checkAddressAvailable(addr); err != nil { return fmt.Errorf("%w: address not available: %v", ErrRequest, err) } @@ -690,12 +700,20 @@ func AccountRemove(ctx context.Context, account string) (rerr error) { } }() + // Open account now. The deferred Close MUST happen after the dynamic unlock, + // because during tests the consistency checker takes the same lock. + acc, err := store.OpenAccount(log, account, false) + if err != nil { + return fmt.Errorf("%w: open account: %v", ErrRequest, err) + } + defer func() { + err := acc.Close() + log.Check(err, "closing account after error") + }() + defer mox.Conf.DynamicLockUnlock()() c := mox.Conf.Dynamic - if _, ok := c.Accounts[account]; !ok { - return fmt.Errorf("%w: account does not exist", ErrRequest) - } // Compose new config without modifying existing data structures. If we fail, we // leave no trace. @@ -711,28 +729,11 @@ func AccountRemove(ctx context.Context, account string) (rerr error) { return fmt.Errorf("writing domains.conf: %w", err) } - odir := filepath.Join(mox.DataDirPath("accounts"), account) - tmpdir := filepath.Join(mox.DataDirPath("tmp"), "oldaccount-"+account) - if err := os.Rename(odir, tmpdir); err != nil { - log.Errorx("moving old account data directory out of the way", err, slog.String("account", account)) - return fmt.Errorf("account removed, but account data directory %q could not be moved out of the way: %v", odir, err) - } - if err := os.RemoveAll(tmpdir); err != nil { - log.Errorx("removing old account data directory", err, slog.String("account", account)) - return fmt.Errorf("account removed, its data directory moved to %q, but removing failed: %v", odir, err) + if err := acc.Remove(context.Background()); err != nil { + return fmt.Errorf("account removed from configuration file, but scheduling account directory for removal failed: %v", err) } - if err := store.TLSPublicKeyRemoveForAccount(context.Background(), account); err != nil { - log.Errorx("removing tls public keys for removed account", err) - return fmt.Errorf("account removed, but removing tls public keys failed: %v", err) - } - - if err := store.LoginAttemptRemoveAccount(context.Background(), account); err != nil { - log.Errorx("removing historic login attempts for removed account", err) - return fmt.Errorf("account removed, but removing historic login attempts failed: %v", err) - } - - log.Info("account removed", slog.String("account", account)) + log.Info("account marked for removal", slog.String("account", account)) return nil } diff --git a/store/account.go b/store/account.go index 3a18483..e87fe03 100644 --- a/store/account.go +++ b/store/account.go @@ -939,8 +939,9 @@ type Account struct { // Reference count, while >0, this account is alive and shared. Protected by // openAccounts, not by account wlock. - nused int - closed chan struct{} // Closed when last reference is gone. + nused int + removed bool // Marked for removal. Last close removes the account directory. + closed chan struct{} // Closed when last reference is gone. } type Upgrade struct { @@ -972,20 +973,34 @@ var openAccounts = struct { } func closeAccount(acc *Account) (rerr error) { + // If we need to remove the account files, we do so without the accounts lock. + remove := false + defer func() { + if remove { + log := mlog.New("store", nil) + err := removeAccount(log, acc.Name) + if rerr == nil { + rerr = err + } + close(acc.closed) + } + }() + openAccounts.Lock() defer openAccounts.Unlock() acc.nused-- if acc.nused > 0 { return } - - // threadsCompleted must be closed now because it increased nused. + remove = acc.removed defer func() { err := acc.DB.Close() acc.DB = nil delete(openAccounts.names, acc.Name) - close(acc.closed) + if !remove { + close(acc.closed) + } if rerr == nil { rerr = err @@ -999,9 +1014,51 @@ func closeAccount(acc *Account) (rerr error) { } else if len(l) > 0 { return fmt.Errorf("messageerase records still present after last account reference is gone: %v", l) } + return nil } +// removeAccount moves the account directory for an account away and removes +// all files, and removes the AccountRemove struct from the database. +func removeAccount(log mlog.Log, accountName string) error { + log = log.With(slog.String("account", accountName)) + log.Info("removing account directory and files") + + // First move the account directory away. + odir := filepath.Join(mox.DataDirPath("accounts"), accountName) + tmpdir := filepath.Join(mox.DataDirPath("tmp"), "oldaccount-"+accountName) + if err := os.Rename(odir, tmpdir); err != nil { + return fmt.Errorf("moving account data directory %q out of the way to %q (account not removed): %v", odir, tmpdir, err) + } + + var errs []error + + // Commit removal to database. + err := AuthDB.Write(context.Background(), func(tx *bstore.Tx) error { + if err := tx.Delete(&AccountRemove{accountName}); err != nil { + return fmt.Errorf("deleting account removal request: %v", err) + } + if err := tlsPublicKeyRemoveForAccount(tx, accountName); err != nil { + return fmt.Errorf("removing tls public keys for account: %v", err) + } + + if err := loginAttemptRemoveAccount(tx, accountName); err != nil { + return fmt.Errorf("removing historic login attempts for account: %v", err) + } + return nil + }) + if err != nil { + errs = append(errs, fmt.Errorf("remove account from database: %w", err)) + } + + // Remove the account directory and its message and other files. + if err := os.RemoveAll(tmpdir); err != nil { + errs = append(errs, fmt.Errorf("removing account data directory %q that was moved to %q: %v", odir, tmpdir, err)) + } + + return errors.Join(errs...) +} + // OpenAccount opens an account by name. // // No additional data path prefix or ".db" suffix should be added to the name. @@ -1010,6 +1067,10 @@ func OpenAccount(log mlog.Log, name string, checkLoginDisabled bool) (*Account, openAccounts.Lock() defer openAccounts.Unlock() if acc, ok := openAccounts.names[name]; ok { + if acc.removed { + return nil, fmt.Errorf("account has been removed") + } + acc.nused++ return acc, nil } @@ -1077,6 +1138,7 @@ func OpenAccountDB(log mlog.Log, accountDir, accountName string) (a *Account, re if err := initAccount(db); err != nil { return nil, fmt.Errorf("initializing account: %v", err) } + close(acc.threadsCompleted) return acc, nil } @@ -1576,6 +1638,20 @@ func initAccount(db *bstore.DB) error { }) } +// Remove schedules an account for removal. New opens will fail. When the last +// reference is closed, the account files are removed. +func (a *Account) Remove(ctx context.Context) error { + openAccounts.Lock() + defer openAccounts.Unlock() + + if err := AuthDB.Insert(ctx, &AccountRemove{AccountName: a.Name}); err != nil { + return fmt.Errorf("inserting account removal: %w", err) + } + a.removed = true + + return nil +} + // WaitClosed waits until the last reference to this account is gone and the // account is closed. Used during tests, to ensure the consistency checks run after // expunged messages have been erased. diff --git a/store/account_test.go b/store/account_test.go index 4c9e00f..bb30de6 100644 --- a/store/account_test.go +++ b/store/account_test.go @@ -2,6 +2,8 @@ package store import ( "context" + "errors" + "io/fs" "os" "path/filepath" "reflect" @@ -411,3 +413,72 @@ func TestNextMessageID(t *testing.T) { tcheck(t, err, "closing account") acc.WaitClosed() } + +func TestRemove(t *testing.T) { + log := mlog.New("store", nil) + os.RemoveAll("../testdata/store/data") + mox.ConfigStaticPath = filepath.FromSlash("../testdata/store/mox.conf") + mox.MustLoadConfig(true, false) + err := Init(ctxbg) + tcheck(t, err, "init") + defer func() { + err := Close() + tcheck(t, err, "close") + }() + defer Switchboard()() + + // Note: we are not removing the account from the config file. Nothing currently + // has a problem with that. + + // Ensure account exists. + acc, err := OpenAccount(log, "mjl", false) + tcheck(t, err, "open account") + + // Mark account removed. It will only be removed when we close the account. + err = acc.Remove(context.Background()) + tcheck(t, err, "remove account") + + p := filepath.Join(mox.DataDirPath("accounts"), "mjl") + _, err = os.Stat(p) + tcheck(t, err, "stat account dir") + + err = acc.Close() + tcheck(t, err, "closing account") + acc.WaitClosed() + acc = nil + + if _, err := os.Stat(p); err == nil || !errors.Is(err, fs.ErrNotExist) { + t.Fatalf(`got stat err %v for account directory, expected "does not exist"`, err) + } + + // Recreate files and directories. We will reinitialize store/ without closing our + // account reference. This will apply the account removal. We only drop our (now + // broken) account reference when done. + acc, err = OpenAccount(log, "mjl", false) + tcheck(t, err, "open account") + defer func() { + acc.Close() // Ignore errors. + acc.WaitClosed() + CheckConsistencyOnClose = true + }() + + // Init below will remove the directory, we are no longer consistent. + CheckConsistencyOnClose = false + + err = acc.Remove(context.Background()) + tcheck(t, err, "remove account") + + _, err = os.Stat(p) + tcheck(t, err, "stat account dir") + + err = Close() + tcheck(t, err, "close store") + err = Init(ctxbg) + tcheck(t, err, "init store") + if _, err := os.Stat(p); err == nil || !errors.Is(err, fs.ErrNotExist) { + t.Fatalf(`got stat err %v for account directory, expected "does not exist"`, err) + } + exists, err := bstore.QueryDB[AccountRemove](ctxbg, AuthDB).Exists() + tcheck(t, err, "checking for account removals") + tcompare(t, exists, false) +} diff --git a/store/init.go b/store/init.go index 9c3126c..d84f76f 100644 --- a/store/init.go +++ b/store/init.go @@ -17,9 +17,15 @@ import ( "github.com/mjl-/mox/moxvar" ) +// AccountRemove represents the scheduled removal of an account, when its last +// reference goes away. +type AccountRemove struct { + AccountName string +} + // AuthDB and AuthDBTypes are exported for ../backup.go. var AuthDB *bstore.DB -var AuthDBTypes = []any{TLSPublicKey{}, LoginAttempt{}, LoginAttemptState{}} +var AuthDBTypes = []any{TLSPublicKey{}, LoginAttempt{}, LoginAttemptState{}, AccountRemove{}} var loginAttemptCleanerStop chan chan struct{} @@ -38,6 +44,18 @@ func Init(ctx context.Context) error { return err } + // List pending account removals, and process them one by one, committing each + // individually. + removals, err := bstore.QueryDB[AccountRemove](ctx, AuthDB).List() + if err != nil { + return fmt.Errorf("listing scheduled account removals: %v", err) + } + for _, removal := range removals { + if err := removeAccount(pkglog, removal.AccountName); err != nil { + pkglog.Errorx("removing old account", err, slog.String("account", removal.AccountName)) + } + } + startLoginAttemptWriter() loginAttemptCleanerStop = make(chan chan struct{}) diff --git a/store/loginattempt.go b/store/loginattempt.go index 6621351..0641e89 100644 --- a/store/loginattempt.go +++ b/store/loginattempt.go @@ -313,15 +313,13 @@ func LoginAttemptCleanup(ctx context.Context) error { }) } -// LoginAttemptRemoveAccount removes all LoginAttempt records for an account +// loginAttemptRemoveAccount removes all LoginAttempt records for an account // (value must be non-empty). -func LoginAttemptRemoveAccount(ctx context.Context, accountName string) error { - return AuthDB.Write(ctx, func(tx *bstore.Tx) error { - q := bstore.QueryTx[LoginAttempt](tx) - q.FilterNonzero(LoginAttempt{AccountName: accountName}) - _, err := q.Delete() - return err - }) +func loginAttemptRemoveAccount(tx *bstore.Tx, accountName string) error { + q := bstore.QueryTx[LoginAttempt](tx) + q.FilterNonzero(LoginAttempt{AccountName: accountName}) + _, err := q.Delete() + return err } // LoginAttemptList returns LoginAttempt records for the accountName. If diff --git a/store/loginattempt_test.go b/store/loginattempt_test.go index 8c467af..642c583 100644 --- a/store/loginattempt_test.go +++ b/store/loginattempt_test.go @@ -8,6 +8,8 @@ import ( "testing" "time" + "github.com/mjl-/bstore" + "github.com/mjl-/mox/mox-" ) @@ -87,7 +89,9 @@ func TestLoginAttempt(t *testing.T) { tcompare(t, len(l), 2) // Removing account will keep last entry for mjl2. - err = LoginAttemptRemoveAccount(ctxbg, "mjl1") + err = AuthDB.Write(ctxbg, func(tx *bstore.Tx) error { + return loginAttemptRemoveAccount(tx, "mjl1") + }) tcheck(t, err, "remove login attempt account") l, err = LoginAttemptList(ctxbg, "", 0) tcheck(t, err, "list login attempts") diff --git a/store/tlspubkey.go b/store/tlspubkey.go index 729d320..0ccfaeb 100644 --- a/store/tlspubkey.go +++ b/store/tlspubkey.go @@ -127,9 +127,9 @@ func TLSPublicKeyRemove(ctx context.Context, fingerprint string) error { return AuthDB.Delete(ctx, &k) } -// TLSPublicKeyRemoveForAccount removes all tls public keys for an account. -func TLSPublicKeyRemoveForAccount(ctx context.Context, account string) error { - q := bstore.QueryDB[TLSPublicKey](ctx, AuthDB) +// tlsPublicKeyRemoveForAccount removes all tls public keys for an account. +func tlsPublicKeyRemoveForAccount(tx *bstore.Tx, account string) error { + q := bstore.QueryTx[TLSPublicKey](tx) q.FilterNonzero(TLSPublicKey{Account: account}) _, err := q.Delete() return err