feat: multi-user login#6
Conversation
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
8 issues found across 16 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="bastion/web/templates/base.html">
<violation number="1" location="bastion/web/templates/base.html:71">
P3: `.menu-user` uses `word-break: break-all`, which breaks at any character boundary regardless of whether overflow is imminent. For a username label in a 168px+ dropdown, `overflow-wrap: break-word` (or the legacy `word-break: break-word`) achieves the same overflow protection but keeps words intact unless they actually exceed the container width, making long ASCII usernames more readable.</violation>
</file>
<file name="bastion/secretkey.py">
<violation number="1" location="bastion/secretkey.py:32">
P2: The `.secret` file is created with the process's default umask permissions (via `open(path, "w")`) before being locked down to `0o600` by the subsequent `os.chmod` call. A local attacker or another process on the system could read the secret during this microsecond window. Use `os.open` with `stat.S_IRUSR | stat.S_IWUSR` to create the file with restricted permissions from the instant it hits disk, eliminating the race.</violation>
</file>
<file name="bastion/users.py">
<violation number="1" location="bastion/users.py:48">
P2: A malformed hash entry in `data/users.json` can crash login/basic-auth checks, because `verify_hash()` only catches split errors but not invalid iteration counts or salt hex. Since this file is best-effort runtime JSON, treating those parse failures as `False` would keep a corrupt user record from causing 500 responses.</violation>
<violation number="2" location="bastion/users.py:79">
P3: `count()`, `exists()`, and `usernames()` each re-read and parse the full `users.json` from disk on every call. The auth middleware calls `count()` up to 6 times per request, each triggering I/O. Consider caching the loaded dict (e.g., keep it in a module-level variable updated by `load()` and invalidated by `save()`) to avoid redundant reads.</violation>
<violation number="3" location="bastion/users.py:96">
P2: Concurrent writes in `add()`, `set_password()`, and `remove()` can lose data. Each function reads the full user store (load), modifies the dict, then writes it back (save). If two admin requests arrive at the same time, the second `load()` may read the first's changes but the second `save()` could overwrite the first's write after the first's `os.replace`. Consider a simple file-lock (fcntl.flock or a threading.Lock) around the load+modify+save sequence.</violation>
<violation number="4" location="bastion/users.py:97">
P1: `add()` silently overwrites an existing user's password instead of rejecting the duplicate. An admin who re-submits the add-user form with an existing username (or a script hitting the POST endpoint) will change that user's password without any error or warning. `add()` should either check `exists(username)` and raise `ValueError`, or the web route should call `exists()` first.</violation>
</file>
<file name="bastion/web/templates/views/settings.html">
<violation number="1" location="bastion/web/templates/views/settings.html:220">
P3: The remove button uses `btn-danger` class, but this standalone template's inline CSS doesn't define `.btn-danger`. The button will render as a plain `.btn` (gray/dark) instead of the intended red/danger style, making it visually indistinguishable from non-destructive buttons. Add `.btn-danger { border-color: #6e2831; background: #3d1a1f; color: #ff7b72; }` and `.btn-danger:hover { background: #5a2027; }` to the page's style block to match the pattern used in base.html.</violation>
</file>
<file name="bastion/web/app.py">
<violation number="1" location="bastion/web/app.py:63">
P1: Removed users can keep accessing the dashboard until their existing `bastion_user` cookie expires, because the multi-user cookie path accepts any valid signed username without checking that the username still exists in `users.json`. It would be safer to verify the session username against the current user store before returning authenticated, so `/settings/users/remove` actually revokes access.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| if not password: | ||
| raise ValueError("password must not be empty") | ||
| users = load() | ||
| users[username] = hash_password(password) |
There was a problem hiding this comment.
P1: add() silently overwrites an existing user's password instead of rejecting the duplicate. An admin who re-submits the add-user form with an existing username (or a script hitting the POST endpoint) will change that user's password without any error or warning. add() should either check exists(username) and raise ValueError, or the web route should call exists() first.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At bastion/users.py, line 97:
<comment>`add()` silently overwrites an existing user's password instead of rejecting the duplicate. An admin who re-submits the add-user form with an existing username (or a script hitting the POST endpoint) will change that user's password without any error or warning. `add()` should either check `exists(username)` and raise `ValueError`, or the web route should call `exists()` first.</comment>
<file context>
@@ -0,0 +1,123 @@
+ if not password:
+ raise ValueError("password must not be empty")
+ users = load()
+ users[username] = hash_password(password)
+ save(users)
+ log.info("user added: %s", username)
</file context>
| """Multi-user mode (any user exists) takes precedence; otherwise fall back | ||
| to the single shared password; otherwise the dashboard is open.""" | ||
| if users.count() > 0: | ||
| if auth.verify_session(request.cookies.get(auth.USER_COOKIE), secretkey.get()): |
There was a problem hiding this comment.
P1: Removed users can keep accessing the dashboard until their existing bastion_user cookie expires, because the multi-user cookie path accepts any valid signed username without checking that the username still exists in users.json. It would be safer to verify the session username against the current user store before returning authenticated, so /settings/users/remove actually revokes access.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At bastion/web/app.py, line 63:
<comment>Removed users can keep accessing the dashboard until their existing `bastion_user` cookie expires, because the multi-user cookie path accepts any valid signed username without checking that the username still exists in `users.json`. It would be safer to verify the session username against the current user store before returning authenticated, so `/settings/users/remove` actually revokes access.</comment>
<file context>
@@ -52,8 +52,30 @@ async def _lifespan(app: FastAPI):
+ """Multi-user mode (any user exists) takes precedence; otherwise fall back
+ to the single shared password; otherwise the dashboard is open."""
+ if users.count() > 0:
+ if auth.verify_session(request.cookies.get(auth.USER_COOKIE), secretkey.get()):
+ return True
+ creds = auth.decode_basic(request.headers.get("authorization"))
</file context>
| return f.read().strip() | ||
| os.makedirs(settings.data_dir, exist_ok=True) | ||
| key = secrets.token_hex(32) | ||
| with open(path, "w", encoding="utf-8") as f: |
There was a problem hiding this comment.
P2: The .secret file is created with the process's default umask permissions (via open(path, "w")) before being locked down to 0o600 by the subsequent os.chmod call. A local attacker or another process on the system could read the secret during this microsecond window. Use os.open with stat.S_IRUSR | stat.S_IWUSR to create the file with restricted permissions from the instant it hits disk, eliminating the race.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At bastion/secretkey.py, line 32:
<comment>The `.secret` file is created with the process's default umask permissions (via `open(path, "w")`) before being locked down to `0o600` by the subsequent `os.chmod` call. A local attacker or another process on the system could read the secret during this microsecond window. Use `os.open` with `stat.S_IRUSR | stat.S_IWUSR` to create the file with restricted permissions from the instant it hits disk, eliminating the race.</comment>
<file context>
@@ -0,0 +1,41 @@
+ return f.read().strip()
+ os.makedirs(settings.data_dir, exist_ok=True)
+ key = secrets.token_hex(32)
+ with open(path, "w", encoding="utf-8") as f:
+ f.write(key)
+ os.chmod(path, 0o600)
</file context>
| raise ValueError(f"invalid username: {username!r}") | ||
| if not password: | ||
| raise ValueError("password must not be empty") | ||
| users = load() |
There was a problem hiding this comment.
P2: Concurrent writes in add(), set_password(), and remove() can lose data. Each function reads the full user store (load), modifies the dict, then writes it back (save). If two admin requests arrive at the same time, the second load() may read the first's changes but the second save() could overwrite the first's write after the first's os.replace. Consider a simple file-lock (fcntl.flock or a threading.Lock) around the load+modify+save sequence.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At bastion/users.py, line 96:
<comment>Concurrent writes in `add()`, `set_password()`, and `remove()` can lose data. Each function reads the full user store (load), modifies the dict, then writes it back (save). If two admin requests arrive at the same time, the second `load()` may read the first's changes but the second `save()` could overwrite the first's write after the first's `os.replace`. Consider a simple file-lock (fcntl.flock or a threading.Lock) around the load+modify+save sequence.</comment>
<file context>
@@ -0,0 +1,123 @@
+ raise ValueError(f"invalid username: {username!r}")
+ if not password:
+ raise ValueError("password must not be empty")
+ users = load()
+ users[username] = hash_password(password)
+ save(users)
</file context>
| candidate = hash_password(password, salt=salt_hex, iterations=int(iters)) | ||
| return hmac.compare_digest(candidate, stored) |
There was a problem hiding this comment.
P2: A malformed hash entry in data/users.json can crash login/basic-auth checks, because verify_hash() only catches split errors but not invalid iteration counts or salt hex. Since this file is best-effort runtime JSON, treating those parse failures as False would keep a corrupt user record from causing 500 responses.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At bastion/users.py, line 48:
<comment>A malformed hash entry in `data/users.json` can crash login/basic-auth checks, because `verify_hash()` only catches split errors but not invalid iteration counts or salt hex. Since this file is best-effort runtime JSON, treating those parse failures as `False` would keep a corrupt user record from causing 500 responses.</comment>
<file context>
@@ -0,0 +1,123 @@
+ return False
+ if algo != "pbkdf2_sha256":
+ return False
+ candidate = hash_password(password, salt=salt_hex, iterations=int(iters))
+ return hmac.compare_digest(candidate, stored)
+
</file context>
| candidate = hash_password(password, salt=salt_hex, iterations=int(iters)) | |
| return hmac.compare_digest(candidate, stored) | |
| try: | |
| candidate = hash_password(password, salt=salt_hex, iterations=int(iters)) | |
| except ValueError: | |
| return False | |
| return hmac.compare_digest(candidate, stored) |
| .profile .menu .ico { width: 16px; text-align: center; color: #7d8590; } | ||
| .profile .menu-user { | ||
| padding: 6px 10px 8px; margin-bottom: 4px; border-bottom: 1px solid #21262d; | ||
| color: #7d8590; font-size: 12px; word-break: break-all; |
There was a problem hiding this comment.
P3: .menu-user uses word-break: break-all, which breaks at any character boundary regardless of whether overflow is imminent. For a username label in a 168px+ dropdown, overflow-wrap: break-word (or the legacy word-break: break-word) achieves the same overflow protection but keeps words intact unless they actually exceed the container width, making long ASCII usernames more readable.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At bastion/web/templates/base.html, line 71:
<comment>`.menu-user` uses `word-break: break-all`, which breaks at any character boundary regardless of whether overflow is imminent. For a username label in a 168px+ dropdown, `overflow-wrap: break-word` (or the legacy `word-break: break-word`) achieves the same overflow protection but keeps words intact unless they actually exceed the container width, making long ASCII usernames more readable.</comment>
<file context>
@@ -66,6 +66,10 @@
.profile .menu .ico { width: 16px; text-align: center; color: #7d8590; }
+ .profile .menu-user {
+ padding: 6px 10px 8px; margin-bottom: 4px; border-bottom: 1px solid #21262d;
+ color: #7d8590; font-size: 12px; word-break: break-all;
+ }
</file context>
| color: #7d8590; font-size: 12px; word-break: break-all; | |
| color: #7d8590; font-size: 12px; overflow-wrap: break-word; |
| os.replace(tmp, path) | ||
|
|
||
|
|
||
| def count() -> int: |
There was a problem hiding this comment.
P3: count(), exists(), and usernames() each re-read and parse the full users.json from disk on every call. The auth middleware calls count() up to 6 times per request, each triggering I/O. Consider caching the loaded dict (e.g., keep it in a module-level variable updated by load() and invalidated by save()) to avoid redundant reads.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At bastion/users.py, line 79:
<comment>`count()`, `exists()`, and `usernames()` each re-read and parse the full `users.json` from disk on every call. The auth middleware calls `count()` up to 6 times per request, each triggering I/O. Consider caching the loaded dict (e.g., keep it in a module-level variable updated by `load()` and invalidated by `save()`) to avoid redundant reads.</comment>
<file context>
@@ -0,0 +1,123 @@
+ os.replace(tmp, path)
+
+
+def count() -> int:
+ return len(load())
+
</file context>
| <form method="post" action="/settings/users/remove" style="display:inline" | ||
| onsubmit="return confirm('{{ t('remove') }} {{ u }}?')"> | ||
| <input type="hidden" name="username" value="{{ u }}"> | ||
| <button type="submit" class="btn btn-danger">{{ t("remove") }}</button> |
There was a problem hiding this comment.
P3: The remove button uses btn-danger class, but this standalone template's inline CSS doesn't define .btn-danger. The button will render as a plain .btn (gray/dark) instead of the intended red/danger style, making it visually indistinguishable from non-destructive buttons. Add .btn-danger { border-color: #6e2831; background: #3d1a1f; color: #ff7b72; } and .btn-danger:hover { background: #5a2027; } to the page's style block to match the pattern used in base.html.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At bastion/web/templates/views/settings.html, line 220:
<comment>The remove button uses `btn-danger` class, but this standalone template's inline CSS doesn't define `.btn-danger`. The button will render as a plain `.btn` (gray/dark) instead of the intended red/danger style, making it visually indistinguishable from non-destructive buttons. Add `.btn-danger { border-color: #6e2831; background: #3d1a1f; color: #ff7b72; }` and `.btn-danger:hover { background: #5a2027; }` to the page's style block to match the pattern used in base.html.</comment>
<file context>
@@ -200,6 +201,69 @@ <h2>{{ t("sec_security") }}</h2>
+ <form method="post" action="/settings/users/remove" style="display:inline"
+ onsubmit="return confirm('{{ t('remove') }} {{ u }}?')">
+ <input type="hidden" name="username" value="{{ u }}">
+ <button type="submit" class="btn btn-danger">{{ t("remove") }}</button>
+ </form>
+ </td>
</file context>
dd530a7 to
d3daee2
Compare
1 participant
Summary
Adds named user accounts on top of the single-password gate.
bastion/users.py— user store indata/users.json, PBKDF2-HMAC-SHA256 hashed passwords (pure hash helpers, tested with fixed salt).bastion/secretkey.py— stable server secret (BASTION_SECRET_KEYenv or generateddata/.secret, 0600) to sign session cookies.auth.sign_session/verify_session+decode_basic.bastion_usercookie); elseBASTION_AUTH_PASSWORD; else open. HTTP Basic auth works with user creds too../data.Test plan
python -m pytest -q→ 145 passed. New:tests/test_users.py, session cases intests/test_auth.py, multi-user flow intests/test_web.py.Notes
data/users.jsonanddata/.secretare gitignored and covered by thebastion_datavolume.Summary by cubic
Add multi-user login with named accounts, PBKDF2-HMAC-SHA256 hashed passwords, and signed session cookies. Falls back to the single shared password when no users exist; runs open only when neither users nor a password are configured.
New Features
bastion_userusing a stable server secret (BASTION_SECRET_KEYor generated atdata/.secret).BASTION_AUTH_PASSWORD; else open.data/users.jsonanddata/.secretare created under the data dir and are gitignored.Migration
BASTION_SECRET_KEYto share sessions across replicas or control rotation; otherwise a key is generated atdata/.secret.Written for commit d3daee2. Summary will update on new commits.