Merge branch 'tombstoning' into develop

.github/workflows/docker-image.yml

@@ -105,11 +105,11 @@ jobs:
     - uses: actions/checkout@v4
     - uses: oven-sh/setup-bun@v1
       with:
-        bun-version: 1.1.3
+        bun-version: 1.1.26
     - run: bun install --frozen-lockfile
     - run: bun run bundle --prod
     - name: Check for outdated NPM packages
-      run: bun x npm-check-updates --root --format group >> "${GITHUB_STEP_SUMMARY}"
+      run: ./scripts/bun_outdated.sh | tee "${GITHUB_STEP_SUMMARY}"
   build:
     runs-on: ubuntu-latest
     steps:

.pre-commit-config.yaml

@@ -16,7 +16,7 @@ repos:
   hooks:
     - id: ruff
 - repo: https://github.com/astral-sh/uv-pre-commit
-  rev: 0.1.29
+  rev: 0.4.7
   hooks:
     - id: pip-compile
       name: "pip-compile: requirements.txt"

doc/_static/js/.gitattributes

@@ -0,0 +1,2 @@
+mermaid.min.js* linguist-vendored
+mermaid.min.js -diff

doc/_templates/logo-text.html

@@ -1,4 +1,4 @@
 <a href="{{ homepage() }}" class="text-logo">
-  <img src="{{ pathto('_static/' + logo, 1) }}" class="logo" alt="Pycroft" height="32px" />
+  <img src="{{ logo_url }}" class="logo" alt="Pycroft" height="32px" />
   {{ theme_project_nav_name or shorttitle }}
 </a>

doc/api/pycroft.model.rst

@@ -17,7 +17,6 @@ pycroft.model
 .. automodule:: pycroft.model.functions
 .. automodule:: pycroft.model.hades
 .. automodule:: pycroft.model.host
-.. automodule:: pycroft.model.interval_base
 .. automodule:: pycroft.model.logging
 .. automodule:: pycroft.model.net
 .. automodule:: pycroft.model.port
@@ -28,5 +27,6 @@ pycroft.model
 .. automodule:: pycroft.model.task_serialization
 .. automodule:: pycroft.model.traffic
 .. automodule:: pycroft.model.types
+.. automodule:: pycroft.model.unix_account
 .. automodule:: pycroft.model.user
 .. automodule:: pycroft.model.webstorage

doc/arch/adr-006.rst

@@ -0,0 +1,148 @@
+ADR006
+======
+
+:Number: 006
+:Title: Combined tombstoning of uid and login
+:Author: Lukas Juhrich, Jakob Müller
+:Created: 2023-01-15
+:Status: Postulated
+
+.. contents:: Table of Contents
+
+Context
+-------
+In the light of data economy, we should not store the login of a :class:`User` –
+which implicitly is a (partial) property of the :class:`UnixAccount` –
+more than necessary.
+
+The only reason to keep the login is to avoid assigning the same mail address
+to multiple people across time; otherwise,
+mails might be send to the wrong addressee.
+
+Thus it has been decided (external from the development team) to store
+the login in form of a hash whenever it stops being required for providing
+services to a user.
+
+Requirements
+~~~~~~~~~~~~
+The implementation has to ensure
+
+1. absence of the ``login`` whenever it is not needed *(legal requirement)*
+2. that no login is doubly assigned *(legal / business requirement)*
+
+Consequently, we need to track the ``login`` hash in some kind of “tombstone”,
+such that
+
+* it is referenced by a ``User`` or a ``UnixAccount``
+* it exists in all scenarios and is never deleted
+
+Since the ``login`` and the ``uid`` have the same lifecycle, and both
+require tombstoning –
+albeit the ``uid`` needs to be kept only for technical reasons,
+not for legal ones –
+it makes sense to let such a ``tombstone`` maintain both pieces of data.
+
+Current model
+~~~~~~~~~~~~~
+
+In the current model, the relevant entities are related as follows:
+
+1. ``User``: has a ``login`` (nullable) and a ``unix_account_id`` (nullable).
+2. ``UnixAccount``: Has the columns ``uid``, ``gid``, ``home``, and ``login_shell``,
+   but no ``login``.
+3. An ldap account (Derived entity):
+   If a ``User`` has a ``UnixAccount`` and the correct permissions,
+   this causes an LDAP account to be exported by the ldap syncer.
+
+Since ``user.unix_account`` is a foreign key constraint,
+we have the following possible states (``U``: User, ``UA``: UnixAccount):
+
+========== === ========= =================== ====
+#          ∃U? ∃U.login? ∃U.unix_account_id? ∃UA?
+========== === ========= =================== ====
+1          ✓   ✗         ✗                   ✗
+2          ✓   ✗         ✓                   ✓
+3          ✓   ✓         ✗                   ✗
+4          ✓   ✓         ✓                   ✓
+---------- --- --------- ------------------- ----
+5          ✗   ✗         ✗                   ✓
+6          ✗   ✗         ✗                   ✗
+========== === ========= =================== ====
+
+With regards to tombstoning, the states imply the following requirements:
+
+State 1. No login, no unix account
+    No requirements.
+
+State 2. No login, but unix account
+    This is currently allowed, but issues a warning in the ldap syncer.
+    Indeed, without the login the unix account cannot be exported.
+    With tombstones however, we require
+
+    * :math:`(T_u)` there shall exist a tombstone with the same uid as the account
+
+State 3. Login, but no unix account
+    * :math:`(T_l)` there shall exist a tombstone with the implied login hash
+
+State 4. Login and unix account
+    * :math:`(T_u)` there shall exist a tombstone with the same uid as the account
+    * :math:`(T_l)` there shall exist a tombstone with the implied login hash
+    * :math:`(E_{ul})` both tombstones in question should be equal
+
+State 5. Unix account without user
+    * :math:`(T_u)` there shall exist a tombstone with the same uid as the account
+
+State 6. Tautological state
+    Nothing exists
+
+
+Decision
+--------
+
+There shall be
+
+1. A new entity ``unix_tombstone(uid int, login_hash text)`` satisfying
+
+   * ``uid`` is unique
+   * ``login_hash`` is unique
+   * Either column may be ``null``, but not both
+   * ``uid`` and ``login_hash`` form the primary key
+
+2. A generated column ``login_hash`` on the ``user`` relation
+   (see :class:`sqlalchemy.schema.Computed`)
+3. :math:`(T_l)`: A foreign key constraint ``User.login_hash → UnixTombstone.login_hash``
+4. :math:`(T_l)`: A foreign key constraint ``UnixAccount.uid → UnixTombstone.uid``
+5. :math:`(E_{ul})`: A constraint checking consistency for users with login and unix account:
+   In this case, the tombstone induced by the ``account.uid`` should agree
+   with the tombstone induced by the ``user.login_hash``
+
+Consequences
+------------
+
+* That the ``login_hash`` is optional allows for
+  ``unix_accounts`` which don't have a ``unix_login`` associated to them
+  to have valid tombstones as well.
+  However, this implies that were one to couple these accounts to users again,
+  the tombstone has to be modified to reflect the user's ``login`` (if it exists).
+
+* using a combined entity instead of an entity for the ``login`` and ``uid``,
+  respectively, has the advantage that one can identify ``login`` tombstones
+  which never had a respective ``unix_account``.
+  Database administrators can then decide on whether to keep these entries or not,
+  since technically these logins have not been used anywhere.
+  This might not serve any particular purpose but the
+
+* In the most frequent use case of creating a user with login and unix account,
+  A tombstone has to be created as well.  This is slightly more effort
+  than the current implementation.
+  To avoid this, triggers may be created that take care of this automatically.
+
+  .. note:: This ADR does not take a stance on whether or not to add triggers
+      as it is mainly concerned with ensuring the critical legal and business
+      requirements.
+
+* Tests have to be written to ensure that with any state change of the ``user``
+  or ``unix_account`` relations,
+  the information contained in the ``tombstone`` tables is monotonous,
+  i.e that neither does a tombstone get deleted via a cascade
+  nor is a field set to ``null`` when it has been non-null before.

doc/conf.py

@@ -30,6 +30,7 @@
 # Add any Sphinx extension module names here, as strings. They can be extensions
 # coming with Sphinx (named 'sphinx.ext.*') or your custom ones.
 extensions = [
+    "sphinx_toolbox.more_autodoc.autonamedtuple",
     'sphinx.ext.autodoc',
     'sphinx.ext.doctest',
     'sphinx.ext.todo',
@@ -40,6 +41,7 @@
     "sphinx_paramlinks",
     "sphinxcontrib.fulltoc",
     "sphinxcontrib.autohttp.flask",
+    "sphinxcontrib.mermaid",
 ]
 
 # Add any paths that contain templates here, relative to this directory.
@@ -168,6 +170,11 @@
 # so a file named "default.css" will overwrite the builtin "default.css".
 html_static_path = ['_static']
 
+mermaid_version = ""
+html_js_files = [
+    "js/mermaid.min.js",
+]
+
 # If not '', a 'Last updated on:' timestamp is inserted at every page bottom,
 # using the given strftime format.
 #html_last_updated_fmt = '%b %d, %Y'

doc/guides/docker.rst

@@ -7,6 +7,7 @@ Installing Docker and Docker Compose
 ------------------------------------
 Prerequisites
     *nothing*
+
 You need to install
 
 * `Docker-engine <https://docs.docker.com/engine/install/>`__ ``≥19.03.0``

doc/guides/setup.rst

@@ -40,6 +40,7 @@ you only need to do the following:
 #. Set up a virtual environment
 
    .. code:: shell
+
        uv venv
        uv pip sync requirements.dev.txt && uv pip install -e deps/wtforms-widgets -e '.[dev]'
 

docker-compose.dev.yml

@@ -57,7 +57,7 @@ services:
       "--watch", "doc/_static",
       "--watch", "ldap_sync",
       "--watch", "web",
-    ]
+      ]
     healthcheck:
       test: curl --fail http://localhost:8000
     ports:

docker/dev.Dockerfile

@@ -7,9 +7,9 @@ RUN --mount=type=cache,target=/var/cache/apk,sharing=locked \
     apk add unzip curl
 RUN <<EOF ash
     set -euo pipefail
-    curl -sSfLO https://github.com/oven-sh/bun/releases/download/bun-v1.1.3/bun-linux-x64-baseline.zip
+    curl -sSfLO https://github.com/oven-sh/bun/releases/download/bun-v1.1.26/bun-linux-x64-baseline.zip
     unzip -j bun-linux-x64-baseline.zip bun-linux-x64-baseline/bun -d /opt
-    echo "e1c94765691f95ca593cf921c89d7bba951cd6e876d28f67ee37a3feeb288f55  /opt/bun" \
+    echo "610bf0daf21cbb7a80be18b2bdb67c0cdcb9e83c680afa082e70a970db78f895  /opt/bun" \
         | sha256sum -c -
 EOF
 

justfile

@@ -93,24 +93,30 @@ _ensure_schema_dir:
 run: (_up "dev-app")
 
 # spawn a shell in the `test-app` container
+[positional-arguments]
 test-shell *args:
-    {{ drc }} run --rm test-app shell {{ args }}
+    {{ drc }} run --rm test-app shell "$@"
 
 # spawn a shell in the `dev-app` container
+[positional-arguments]
 dev-shell *args:
-    {{ drc }} run --rm dev-app shell {{ args }}
+    {{ drc }} run --rm dev-app shell "$@"
 
 # run an alembic command against the `dev-db`
-alembic command *args:
-    {{ drc }} --progress=none run --rm dev-migrate shell flask alembic {{ command }} {{ args }}
+[positional-arguments]
+alembic *args:
+    {{ drc }} --progress=none run --rm dev-migrate shell flask alembic "$@"
+
 
 # run an interactive postgres shell in the dev-db container
+[positional-arguments]
 dev-psql *args: (_up "dev-db")
-    {{ dev-psql }} {{ args }}
+    {{ dev-psql }} "$@"
 
 # run an interactive postgres shell in the test-db container
+[positional-arguments]
 test-psql *args: (_up "test-db")
-    {{ test-psql }} {{ args }}
+    {{ test-psql }} "$@"
 
 # give a quick overview over the schema in the dev-db.
 
@@ -120,10 +126,14 @@ schema-status: (_up "dev-db")
     	`{{ dev-psql }} -q -t -c 'table pycroft.alembic_version'`
     @echo "Schema version in {{ sql_dump }}: " \
     	`grep 'COPY.*alembic_version' -A1 {{ sql_dump }} | sed -n '2p'`
-    {{ drc }} --progress=none run --rm dev-app alembic check 2>&1 | tail -n1
+    {{ drc }} --progress=none run --rm dev-app flask alembic check 2>&1 | tail -n1
+
+schema-diff: (_up "dev-db") (alembic "diff")
 
 # upgrade the (imported or created) schema to the current revision
 schema-upgrade: (_up "dev-db") (alembic "upgrade" "head")
+
+# extract `requirements` lockfiles from `pyproject.toml` dependency spec
 deps-compile:
     uv pip compile pyproject.toml --generate-hashes -o requirements.txt
     uv pip compile pyproject.toml --generate-hashes --extra dev -o requirements.dev.txt

ldap_sync/sources/db.py

@@ -47,7 +47,7 @@ def _fetch_db_users(
 ) -> list[_UserProxyType]:
     """Fetch users to be synced, plus whether ``ldap_login_enabled`` is set.
 
-    If the `` ldap_login_enabled`` flag is not present,
+    If the ``ldap_login_enabled`` flag is not present,
     we interpret this as ``should_be_blocked``.
 
     :param session: The SQLAlchemy session to use
@@ -199,7 +199,7 @@ class _PropertyProxyType(NamedTuple):
 def _fetch_db_properties(session: Session) -> list[_PropertyProxyType]:
     """Fetch the groups who should be synced.
 
-    Explicitly, this returns everything in :ref:`EXPORTED_PROPERTIES` together with
+    Explicitly, this returns everything in :data:`EXPORTED_PROPERTIES` together with
     the current users having the respective property as members.
 
     :param session: The SQLAlchemy session to use
@@ -246,6 +246,7 @@ def fetch_db_properties(
         )
 
 
+#: The properties of a user we export to LDAP.
 EXPORTED_PROPERTIES = frozenset(
     [
         "network_access",

package.json

@@ -51,7 +51,6 @@
     "esbuild-plugin-clean": "^1.0.1",
     "esbuild-plugin-summary": "^0.0.2",
     "eslint": "^8.16.0",
-    "npm-check-updates": "^16.14.18",
     "typescript": "^4.1.3"
   },
   "scripts": {

pycroft/helpers/user.py

@@ -8,6 +8,7 @@
 import random
 import string
 import typing as t
+from hashlib import sha512
 
 from passlib.apps import ldap_context
 from passlib.context import CryptContext
@@ -66,6 +67,11 @@ def verify_password(plaintext_password: str, hash: str) -> bool:
         return False
 
 
+def login_hash(login: str) -> bytes:
+    """Hashes a login with sha512, as is done in the `User.login_hash` generated column."""
+    return sha512(login.encode()).digest()
+
+
 def generate_random_str(length: int) -> str:
     """
     Generates an aplhanumeric random string