refactor password/login-timestamp handling into a User object

CHANGELOG.md

@@ -2,6 +2,11 @@
 
 ## untagged
 
+- Migrate and remove sqlite database in favor of password/lastlogin tracking 
+  in a user's maildir.  This removes the need for "passdb" setting in ini file
+  which was introduced through #351 below. 
+  ([#379](https://github.com/deltachat/chatmail/pull/379))
+
 - BREAKING: new required chatmail.ini values:
 
   mailboxes_dir = /home/vmail/mail/{mail_domain}
@@ -14,10 +19,6 @@
   which removes users from database and mails after 100 days without any login. 
   ([#350](https://github.com/deltachat/chatmail/pull/350))
 
-- Fix and refine "last-login" tracking which now happens via a dedicated
-  dovecot dictproxy with state kept in "$USERDIR/last-login" files. 
-  ([#354](https://github.com/deltachat/chatmail/pull/354))
-
 - Refine DNS checking to distinguish between "required" and "recommended" settings 
   ([#372](https://github.com/deltachat/chatmail/pull/372))
 

chatmaild/src/chatmaild/config.py

@@ -2,6 +2,8 @@ from pathlib import Path
 
 import iniconfig
 
+from chatmaild.user import User
+
 echobot_password_path = Path("/run/echobot/password")
 
 
@@ -38,16 +40,17 @@ class Config:
     def _getbytefile(self):
         return open(self._inipath, "rb")
 
-    def get_user_maildir(self, addr):
-        if addr and addr != "." and addr != ".." and "/" not in addr:
-            return self.mailboxes_dir.joinpath(addr)
-        raise ValueError(f"invalid address {addr!r}")
+    def get_user(self, addr):
+        if not addr or "@" not in addr or "/" in addr:
+            raise ValueError(f"invalid address {addr!r}")
 
-    def get_user_dict(self, addr, enc_password):
-        home = self.get_user_maildir(addr)
-        return dict(
-            addr=addr, home=str(home), uid="vmail", gid="vmail", password=enc_password
-        )
+        maildir = self.mailboxes_dir.joinpath(addr)
+        if addr.startswith("echo@"):
+            password_path = echobot_password_path
+        else:
+            password_path = maildir.joinpath("password")
+
+        return User(maildir, addr, password_path, uid="vmail", gid="vmail")
 
 
 def write_initial_config(inipath, mail_domain, overrides):

chatmaild/src/chatmaild/delete_inactive_users.py

@@ -2,19 +2,27 @@
 Remove inactive users
 """
 
+import os
 import shutil
 import sys
 import time
 
 from .config import read_config
-from .lastlogin import get_last_login_from_userdir
 
 
 def delete_inactive_users(config):
     cutoff_date = time.time() - config.delete_inactive_users_after * 86400
-    for userdir in config.mailboxes_dir.iterdir():
-        if get_last_login_from_userdir(userdir) < cutoff_date:
-            shutil.rmtree(userdir, ignore_errors=True)
+    for addr in os.listdir(config.mailboxes_dir):
+        try:
+            user = config.get_user(addr)
+        except ValueError:
+            continue
+
+        read_timestamp = user.get_last_login_timestamp()
+        if read_timestamp and read_timestamp < cutoff_date:
+            path = config.mailboxes_dir.joinpath(addr)
+            assert path == user.maildir
+            shutil.rmtree(path, ignore_errors=True)
 
 
 def main():

chatmaild/src/chatmaild/doveauth.py

@@ -4,9 +4,8 @@ import logging
 import os
 import sys
 
-from .config import Config, echobot_password_path, read_config
+from .config import Config, read_config
 from .dictproxy import DictProxy
-from .lastlogin import set_user_password
 from .migrate_db import migrate_from_db_to_maildir
 
 NOCREATE_FILE = "/etc/chatmail-nocreate"
@@ -128,37 +127,22 @@ class AuthDictProxy(DictProxy):
 
     def iter_userdb(self) -> list:
         """Get a list of all user addresses."""
-        getuserpaths = self.config.mailboxes_dir.iterdir()
-        return [x.name for x in getuserpaths if "@" in x.name]
+        return [x for x in os.listdir(self.config.mailboxes_dir) if "@" in x]
 
-    def lookup_userdb(self, user):
-        userdir = self.config.get_user_maildir(user)
-        if user.startswith("echo@"):
-            password_path = echobot_password_path
-        else:
-            password_path = userdir.joinpath("password")
+    def lookup_userdb(self, addr):
+        return self.config.get_user(addr).get_userdb_dict()
 
-        try:
-            enc_password = password_path.read_text()
-        except FileNotFoundError:
-            return {}
-        else:
-            if not enc_password:
-                # writing the password might have crashed and file is empty
-                return {}
-            return self.config.get_user_dict(user, enc_password=enc_password)
-
-    def lookup_passdb(self, user, cleartext_password):
-        userdata = self.lookup_userdb(user)
+    def lookup_passdb(self, addr, cleartext_password):
+        user = self.config.get_user(addr)
+        userdata = user.get_userdb_dict()
         if userdata:
             return userdata
-        if not is_allowed_to_create(self.config, user, cleartext_password):
+        if not is_allowed_to_create(self.config, addr, cleartext_password):
             return
 
-        enc_password = encrypt_password(cleartext_password)
-        set_user_password(self.config, user, enc_password=enc_password)
+        user.set_password(encrypt_password(cleartext_password))
         print(f"Created address: {user}", file=sys.stderr)
-        return self.config.get_user_dict(user, enc_password=enc_password)
+        return user.get_userdb_dict()
 
 
 def main():

chatmaild/src/chatmaild/lastlogin.py

@@ -1,48 +1,8 @@
-import os
 import sys
-import time
-
-import filelock
 
 from .config import read_config
 from .dictproxy import DictProxy
 
-# this file's mtime reflects the last login-time for a user
-LAST_LOGIN = "password"
-
-
-def get_daytimestamp(timestamp) -> int:
-    return int(timestamp) // 86400 * 86400
-
-
-def write_last_login_to_userdir(userdir, timestamp):
-    target = userdir.joinpath(LAST_LOGIN)
-    timestamp = get_daytimestamp(timestamp)
-    try:
-        s = os.stat(target)
-    except FileNotFoundError:
-        pass
-    else:
-        if int(s.st_mtime) != timestamp:
-            os.utime(target, (timestamp, timestamp))
-
-
-def get_last_login_from_userdir(userdir) -> int:
-    if "@" not in userdir.name or userdir.name.startswith("echo@"):
-        return get_daytimestamp(time.time())
-    target = userdir.joinpath(LAST_LOGIN)
-    return int(target.stat().st_mtime)
-
-
-def set_user_password(config, addr, enc_password):
-    assert not addr.startswith("echo@"), addr
-    userdir = config.get_user_maildir(addr)
-    userdir.mkdir(exist_ok=True)
-    password_path = userdir.joinpath("password")
-    lock_path = password_path.with_suffix(".lock")
-    with filelock.FileLock(lock_path):
-        password_path.write_bytes(enc_password.encode("ascii"))
-
 
 class LastLoginDictProxy(DictProxy):
     def __init__(self, config):
@@ -58,8 +18,8 @@ class LastLoginDictProxy(DictProxy):
                 return
             addr = keyname[2]
             timestamp = int(value)
-            userdir = self.config.get_user_maildir(addr)
-            write_last_login_to_userdir(userdir, timestamp)
+            user = self.config.get_user(addr)
+            user.set_last_login_timestamp(timestamp)
         else:
             # Transaction failed.
             self.transactions[transaction_id]["res"] = "F\n"

chatmaild/src/chatmaild/migrate_db.py

@@ -2,13 +2,13 @@
 migration code from old sqlite databases into per-maildir "password" files
 where mtime reflects and is updated to be the "last-login" time.
 """
+
 import logging
 import os
 import sqlite3
 import sys
 
 from chatmaild.config import read_config
-from chatmaild.lastlogin import set_user_password, write_last_login_to_userdir
 
 
 def get_all_rows(path):
@@ -29,6 +29,7 @@ def migrate_from_db_to_maildir(config, chunking=10000):
 
     all_rows = get_all_rows(path)
 
+    # don't transfer special/CI accounts
     rows = [row for row in all_rows if row[0][:3] not in ("ci-", "ac_")]
 
     logging.info(f"ignoring {len(all_rows)-len(rows)} CI accounts")
@@ -36,15 +37,13 @@ def migrate_from_db_to_maildir(config, chunking=10000):
 
     for i, row in enumerate(rows):
         addr = row[0]
-        # don't transfer special/CI accounts (IOLO)
-        if addr.startswith("echo@"):
-            continue
         enc_password = row[1]
-        set_user_password(config, addr, enc_password=enc_password)
+        user = config.get_user(addr)
+        user.set_password(enc_password)
+
         if len(row) == 3 and row[2]:
-            homedir = config.mailboxes_dir.joinpath(addr)
             timestamp = int(row[2])
-            write_last_login_to_userdir(homedir, timestamp)
+            user.set_last_login_timestamp(timestamp)
 
         if i > 0 and i % chunking == 0:
             logging.info(f"migration-progress: {i} passwords transferred")

chatmaild/src/chatmaild/tests/plugin.py

@@ -35,6 +35,11 @@ def maildomain(example_config):
     return example_config.mail_domain
 
 
+@pytest.fixture
+def testaddr(maildomain):
+    return f"user.name@{maildomain}"
+
+
 @pytest.fixture
 def gencreds(maildomain):
     count = itertools.count()

chatmaild/src/chatmaild/tests/test_config.py

@@ -41,32 +41,21 @@ def test_config_userstate_paths(make_config, tmp_path):
     assert passdb_path.name == "passdb.sqlite"
     assert passdb_path.is_relative_to(tmp_path)
     assert config.mail_domain == "something.testrun.org"
-    path = config.get_user_maildir("user1@something.testrun.org")
+    path = config.get_user("user1@something.testrun.org").maildir
     assert not path.exists()
     assert path == mailboxes_dir.joinpath("user1@something.testrun.org")
 
     with pytest.raises(ValueError):
-        config.get_user_maildir("")
+        config.get_user("")
 
     with pytest.raises(ValueError):
-        config.get_user_maildir(None)
+        config.get_user(None)
 
     with pytest.raises(ValueError):
-        config.get_user_maildir("../some@something.testrun.org")
+        config.get_user("../some@something.testrun.org").maildir
 
     with pytest.raises(ValueError):
-        config.get_user_maildir("..")
+        config.get_user("..").maildir
 
     with pytest.raises(ValueError):
-        config.get_user_maildir(".")
-
-
-def test_config_get_user_dict(make_config, tmp_path):
-    config = make_config("something.testrun.org")
-    addr = "user1@something.org"
-    enc_password = "l1k2j31lk2j3l1k23j123"
-    data = config.get_user_dict(addr, enc_password=enc_password)
-    assert addr in str(data["home"])
-    assert data["uid"] == "vmail"
-    assert data["gid"] == "vmail"
-    assert data["password"] == enc_password
+        config.get_user(".")

chatmaild/src/chatmaild/tests/test_delete_inactive_users.py

@@ -2,28 +2,17 @@ import time
 
 from chatmaild.delete_inactive_users import delete_inactive_users
 from chatmaild.doveauth import AuthDictProxy
-from chatmaild.lastlogin import get_last_login_from_userdir, write_last_login_to_userdir
 
 
-def test_login_timestamps(tmp_path):
-    userdir = tmp_path.joinpath("someuser@chat.example.org")
-    userdir.mkdir()
-    userdir.joinpath("password").touch()
-    write_last_login_to_userdir(userdir, timestamp=100000)
-    assert get_last_login_from_userdir(userdir) == 86400
+def test_login_timestamps(example_config):
+    testaddr = "someuser@chat.example.org"
+    user = example_config.get_user(testaddr)
 
-    write_last_login_to_userdir(userdir, timestamp=200000)
-    assert get_last_login_from_userdir(userdir) == 86400 * 2
-
-    write_last_login_to_userdir(userdir, timestamp=200000)
-    assert get_last_login_from_userdir(userdir) == 86400 * 2
-
-
-def test_delete_skips_non_email_dir(example_config):
-    userdir = example_config.get_user_maildir("something")
-    userdir.mkdir()
-    get_last_login_from_userdir(userdir)
-    assert not list(userdir.iterdir())
+    # password file needs to be set because it's mtime tracks last-login time
+    user.set_password("1l2k3j1l2k3j123")
+    for i in range(10):
+        user.set_last_login_timestamp(86400 * 4 + i)
+        assert user.get_last_login_timestamp() == 86400 * 4
 
 
 def test_delete_inactive_users(example_config):
@@ -33,10 +22,10 @@ def test_delete_inactive_users(example_config):
 
     def create_user(addr, last_login):
         dictproxy.lookup_passdb(addr, "q9mr3faue")
-        md = example_config.get_user_maildir(addr)
-        md.joinpath("cur").mkdir()
-        md.joinpath("cur", "something").mkdir()
-        write_last_login_to_userdir(md, timestamp=last_login)
+        user = example_config.get_user(addr)
+        user.maildir.joinpath("cur").mkdir()
+        user.maildir.joinpath("cur", "something").mkdir()
+        user.set_last_login_timestamp(timestamp=last_login)
 
     # create some stale and some new accounts
     to_remove = []
@@ -54,7 +43,7 @@ def test_delete_inactive_users(example_config):
     # check pre and post-conditions for delete_inactive_users()
 
     for addr in to_remove:
-        assert example_config.get_user_maildir(addr).exists()
+        assert example_config.get_user(addr).maildir.exists()
 
     delete_inactive_users(example_config)
 
@@ -62,9 +51,9 @@ def test_delete_inactive_users(example_config):
         assert not p.name.startswith("old")
 
     for addr in to_remove:
-        assert not example_config.get_user_maildir(addr).exists()
+        assert not example_config.get_user(addr).maildir.exists()
 
     for addr in remain:
-        userdir = example_config.get_user_maildir(addr)
+        userdir = example_config.get_user(addr).maildir
         assert userdir.exists()
         assert userdir.joinpath("password").read_text()

chatmaild/src/chatmaild/tests/test_lastlogin.py

@@ -1,42 +1,35 @@
 import time
 
-import pytest
 from chatmaild.doveauth import AuthDictProxy
 from chatmaild.lastlogin import (
     LastLoginDictProxy,
-    get_last_login_from_userdir,
-    write_last_login_to_userdir,
 )
 
 
-@pytest.fixture
-def testaddr():
-    return "user.name@example.org"
-
-
 def test_handle_dovecot_request_last_login(testaddr, example_config):
     dictproxy = LastLoginDictProxy(config=example_config)
 
     authproxy = AuthDictProxy(config=example_config)
     authproxy.lookup_passdb(testaddr, "1l2k3j1l2k3jl123")
 
-    userdir = dictproxy.config.get_user_maildir(testaddr)
-
-    # set last-login info for user
+    # Begin transaction
     tx = "1111"
     msg = f"B{tx}\t{testaddr}"
     res = dictproxy.handle_dovecot_request(msg)
     assert not res
     assert dictproxy.transactions == {tx: dict(addr=testaddr, res="O\n")}
 
+    # set last-login info for user
+    user = dictproxy.config.get_user(testaddr)
     timestamp = int(time.time())
     msg = f"S{tx}\tshared/last-login/{testaddr}\t{timestamp}"
     res = dictproxy.handle_dovecot_request(msg)
     assert not res
     assert len(dictproxy.transactions) == 1
-    read_timestamp = get_last_login_from_userdir(userdir)
+    read_timestamp = user.get_last_login_timestamp()
     assert read_timestamp == timestamp // 86400 * 86400
 
+    # finish transaction
     msg = f"C{tx}"
     res = dictproxy.handle_dovecot_request(msg)
     assert res == "O\n"
@@ -49,7 +42,7 @@ def test_handle_dovecot_request_last_login_echobot(example_config):
     authproxy = AuthDictProxy(config=example_config)
     testaddr = f"echo@{example_config.mail_domain}"
     authproxy.lookup_passdb(testaddr, "ignore")
-    userdir = dictproxy.config.get_user_maildir(testaddr)
+    user = dictproxy.config.get_user(testaddr)
 
     # set last-login info for user
     tx = "1111"
@@ -63,17 +56,5 @@ def test_handle_dovecot_request_last_login_echobot(example_config):
     res = dictproxy.handle_dovecot_request(msg)
     assert not res
     assert len(dictproxy.transactions) == 1
-    read_timestamp = get_last_login_from_userdir(userdir)
-    assert read_timestamp == time.time() // 86400 * 86400
-
-
-def test_login_timestamp(testaddr, example_config):
-    dictproxy = LastLoginDictProxy(config=example_config)
-    authproxy = AuthDictProxy(config=example_config)
-    authproxy.lookup_passdb(testaddr, "1l2k3j1l2k3jl123")
-    userdir = dictproxy.config.get_user_maildir(testaddr)
-    write_last_login_to_userdir(userdir, timestamp=100000)
-    assert get_last_login_from_userdir(userdir) == 86400
-
-    write_last_login_to_userdir(userdir, timestamp=200000)
-    assert get_last_login_from_userdir(userdir) == 86400 * 2
+    read_timestamp = user.get_last_login_timestamp()
+    assert read_timestamp is None

chatmaild/src/chatmaild/tests/test_metadata.py

@@ -33,11 +33,6 @@ def dictproxy(notifier, metadata):
     return MetadataDictProxy(notifier=notifier, metadata=metadata)
 
 
-@pytest.fixture
-def testaddr():
-    return "user.name@example.org"
-
-
 @pytest.fixture
 def testaddr2():
     return "user2@example.org"

chatmaild/src/chatmaild/tests/test_migrate_db.py

@@ -1,6 +1,5 @@
 import sqlite3
 
-from chatmaild.lastlogin import get_last_login_from_userdir
 from chatmaild.migrate_db import migrate_from_db_to_maildir
 
 
@@ -59,9 +58,10 @@ def test_migration(tmp_path, example_config, caplog):
         if "@" not in path.name:
             continue
         password, last_login = all.pop(path.name)
+        user = example_config.get_user(path.name)
         if last_login:
-            assert get_last_login_from_userdir(path) == last_login
-        assert password == path.joinpath("password").read_text()
+            assert user.get_last_login_timestamp() == last_login
+        assert password == user.get_userdb_dict()["password"]
 
     assert not all
     assert not example_config.passdb_path.exists()

chatmaild/src/chatmaild/tests/test_user.py

@@ -0,0 +1,32 @@
+def test_login_timestamp(testaddr, example_config):
+    user = example_config.get_user(testaddr)
+    user.set_password("someeqkjwelkqwjleqwe")
+    user.set_last_login_timestamp(100000)
+    assert user.get_last_login_timestamp() == 86400
+
+    user.set_last_login_timestamp(200000)
+    assert user.get_last_login_timestamp() == 86400 * 2
+
+
+def test_get_user_dict_not_set(testaddr, example_config, caplog):
+    user = example_config.get_user(testaddr)
+    assert not caplog.records
+    assert user.get_userdb_dict() == {}
+    assert len(caplog.records) == 1
+
+    user.set_password("")
+    assert user.get_userdb_dict() == {}
+    assert len(caplog.records) == 2
+
+
+def test_get_user_dict(make_config, tmp_path):
+    config = make_config("something.testrun.org")
+    addr = "user1@something.org"
+    user = config.get_user(addr)
+    enc_password = "l1k2j31lk2j3l1k23j123"
+    user.set_password(enc_password)
+    data = user.get_userdb_dict()
+    assert addr in str(data["home"])
+    assert data["uid"] == "vmail"
+    assert data["gid"] == "vmail"
+    assert data["password"] == enc_password

chatmaild/src/chatmaild/user.py

@@ -0,0 +1,77 @@
+import logging
+import os
+
+import filelock
+
+
+def get_daytimestamp(timestamp) -> int:
+    return int(timestamp) // 86400 * 86400
+
+
+class User:
+    def __init__(self, maildir, addr, password_path, uid, gid):
+        self.maildir = maildir
+        self.addr = addr
+        self.password_path = password_path
+        self.uid = uid
+        self.gid = gid
+
+    @property
+    def can_track(self):
+        return "@" in self.addr and not self.addr.startswith("echo@")
+
+    def get_userdb_dict(self):
+        """Return a non-empty dovecot 'userdb' style dict
+        if the user has an existing non-empty password"""
+        try:
+            pw = self.password_path.read_text()
+        except FileNotFoundError:
+            logging.error(f"password not set for: {self.addr}")
+            return {}
+
+        if not pw:
+            logging.error(f"password is empty for: {self.addr}")
+            return {}
+
+        home = str(self.maildir)
+        return dict(addr=self.addr, home=home, uid=self.uid, gid=self.gid, password=pw)
+
+    def set_password(self, enc_password):
+        """Set the specified password for this user.
+
+        If called concurrently from multiple threads
+        the last password set call will be persisted.
+        """
+        self.maildir.mkdir(exist_ok=True)
+        lock_path = self.maildir.joinpath("password.lock")
+        password = enc_password.encode("ascii")
+
+        with filelock.FileLock(lock_path):
+            try:
+                self.password_path.write_bytes(password)
+            except PermissionError:
+                if not self.addr.startswith("echo@"):
+                    logging.error(f"could not write password for: {self.addr}")
+                    raise
+
+    def set_last_login_timestamp(self, timestamp):
+        """Track login time with daily granularity
+        to minimize touching files and to minimize metadata leakage."""
+        if not self.can_track:
+            return
+        try:
+            mtime = int(os.stat(self.password_path).st_mtime)
+        except FileNotFoundError:
+            logging.error(f"Can not get last login timestamp for {self.addr}")
+            return
+
+        timestamp = get_daytimestamp(timestamp)
+        if mtime != timestamp:
+            os.utime(self.password_path, (timestamp, timestamp))
+
+    def get_last_login_timestamp(self):
+        if self.can_track:
+            try:
+                return int(self.password_path.stat().st_mtime)
+            except FileNotFoundError:
+                pass

cmdeploy/src/cmdeploy/tests/plugin.py

@@ -261,7 +261,6 @@ def gencreds(chatmail_config):
     return lambda domain=None: next(gen(domain))
 
 
-
 #
 # Delta Chat testplugin re-use
 # use the cmfactory fixture to get chatmail instance accounts