From 545e6925650b9722258d642d706f0802b9f65d47 Mon Sep 17 00:00:00 2001
From: ghreprimand <github@jrpmail.ca>
Date: Fri, 5 Jun 2026 13:27:10 -0500
Subject: [PATCH] fix(auth): distinguish empty model allowlists (#2938)

Co-authored-by: ghreprimand <203024559+ghreprimand@users.noreply.github.com>
---
 core/auth.py               |  2 ++
 routes/chat_helpers.py     |  8 +++--
 static/js/admin.js         | 26 +++++++++-------
 tests/test_chat_helpers.py | 64 +++++++++++++++++++++++++++++++++++++-
 4 files changed, 85 insertions(+), 15 deletions(-)

diff --git a/core/auth.py b/core/auth.py
index 3c7669de4..ed083b008 100644
--- a/core/auth.py
+++ b/core/auth.py
@@ -30,10 +30,12 @@ DEFAULT_PRIVILEGES = {
     "can_manage_memory": True,
     "max_messages_per_day": 0,
     "allowed_models": [],
+    "allowed_models_restricted": False,
 }
 
 # Admins get everything
 ADMIN_PRIVILEGES = {k: (True if isinstance(v, bool) else (0 if isinstance(v, int) else [])) for k, v in DEFAULT_PRIVILEGES.items()}
+ADMIN_PRIVILEGES["allowed_models_restricted"] = False
 
 DEFAULT_AUTH_PATH = os.path.join(
     Path(__file__).parent.parent, "data", "auth.json"
diff --git a/routes/chat_helpers.py b/routes/chat_helpers.py
index 0929b699d..c62d3452d 100644
--- a/routes/chat_helpers.py
+++ b/routes/chat_helpers.py
@@ -75,7 +75,7 @@ def _enforce_chat_privileges(request, sess) -> None:
     allowlist, or HTTPException(429) if the user has hit their daily message
     cap. No-op for unauthenticated callers or when auth_manager is absent
     (single-user mode). Admins receive ADMIN_PRIVILEGES from get_privileges,
-    which means empty allowed_models / zero cap → no-op for them.
+    which means unrestricted allowed_models / zero cap -> no-op for them.
     """
     try:
         user = get_current_user(request)
@@ -88,8 +88,10 @@ def _enforce_chat_privileges(request, sess) -> None:
         return
 
     privs = auth_manager.get_privileges(user) or {}
-    allowed = privs.get("allowed_models") or []
-    if allowed and sess.model and sess.model not in allowed:
+    allowed_raw = privs.get("allowed_models")
+    allowed = allowed_raw if isinstance(allowed_raw, list) else []
+    restricted = bool(privs.get("allowed_models_restricted")) or bool(allowed)
+    if restricted and sess.model and sess.model not in allowed:
         raise HTTPException(403, f"Your account is not allowed to use model '{sess.model}'.")
 
     cap = int(privs.get("max_messages_per_day") or 0)
diff --git a/static/js/admin.js b/static/js/admin.js
index 5019096af..5211bf62d 100644
--- a/static/js/admin.js
+++ b/static/js/admin.js
@@ -87,8 +87,11 @@ async function loadUsers() {
           <input type="number" min="0" value="${maxMsg}" data-priv="max_messages_per_day" data-user="${esc(u.username)}" style="width:70px;padding:4px 6px;background:var(--bg);border:1px solid var(--border);border-radius:4px;color:var(--fg);font-size:12px;text-align:center;">
         </div>`;
         // Allowed models — checkbox list
-        const allowedSet = new Set((u.privileges && u.privileges.allowed_models) || []);
-        const allEmpty = allowedSet.size === 0;
+        const allowedModels = Array.isArray(u.privileges && u.privileges.allowed_models)
+          ? u.privileges.allowed_models
+          : [];
+        const allowedSet = new Set(allowedModels);
+        const modelsRestricted = !!(u.privileges && u.privileges.allowed_models_restricted);
         html += `<div style="padding:4px 0;">
           <div style="display:flex;align-items:center;justify-content:space-between;">
             <span style="font-size:12px;">Allowed models</span>
@@ -97,7 +100,7 @@ async function loadUsers() {
               <a href="#" class="priv-models-none" data-user="${esc(u.username)}" style="font-size:10px;opacity:0.5;">None</a>
             </div>
           </div>
-          <div style="font-size:10px;opacity:0.4;margin-bottom:4px;">${allEmpty ? 'All models allowed (no restrictions)' : allowedSet.size + ' model(s) allowed'}</div>
+          <div style="font-size:10px;opacity:0.4;margin-bottom:4px;">${!modelsRestricted ? 'All models allowed (no restrictions)' : (allowedSet.size === 0 ? 'No models allowed' : allowedSet.size + ' model(s) allowed')}</div>
           <div class="priv-models-list" data-user="${esc(u.username)}">
             <span style="opacity:0.4;font-size:11px;">Loading models...</span>
           </div>
@@ -119,7 +122,7 @@ async function loadUsers() {
           // Load models list on first expand
           if (!_modelsLoaded && !privPanel.classList.contains('hidden')) {
             _modelsLoaded = true;
-            _loadModelsForUser(u.username, allowedSet, privPanel);
+            _loadModelsForUser(u.username, allowedSet, modelsRestricted, privPanel);
           }
         });
 
@@ -199,7 +202,7 @@ async function loadUsers() {
   } catch (e) { list.innerHTML = '<div class="admin-error">Failed to load users</div>'; }
 }
 
-async function _loadModelsForUser(username, allowedSet, privPanel) {
+async function _loadModelsForUser(username, allowedSet, modelsRestricted, privPanel) {
   const listEl = privPanel.querySelector(`.priv-models-list[data-user="${username}"]`);
   if (!listEl) return;
   try {
@@ -216,9 +219,9 @@ async function _loadModelsForUser(username, allowedSet, privPanel) {
       listEl.innerHTML = '<span style="opacity:0.4;font-size:11px;">No models available</span>';
       return;
     }
-    const allEmpty = allowedSet.size === 0;
+    let restricted = modelsRestricted;
     listEl.innerHTML = sortModelObjects(allModels).map(m => {
-      const checked = allEmpty || allowedSet.has(m.mid) ? 'checked' : '';
+      const checked = !restricted || allowedSet.has(m.mid) ? 'checked' : '';
       return `<label>
         <input type="checkbox" class="priv-model-cb" data-mid="${esc(m.mid)}" ${checked}>
         <span>${esc(m.display)}</span>
@@ -232,14 +235,15 @@ async function _loadModelsForUser(username, allowedSet, privPanel) {
       listEl.querySelectorAll('.priv-model-cb').forEach(cb => {
         if (cb.checked) checked.push(cb.dataset.mid);
       });
-      // If all are checked, send empty array (= no restrictions)
-      const value = checked.length === allModels.length ? [] : checked;
+      // All checked means unrestricted; zero checked means explicitly no models.
+      restricted = checked.length !== allModels.length;
+      const value = restricted ? checked : [];
       const hint = privPanel.querySelector('.priv-models-list[data-user]')?.previousElementSibling?.querySelector('div[style*="opacity"]');
-      if (hint) hint.textContent = value.length === 0 ? 'All models allowed (no restrictions)' : value.length + ' model(s) allowed';
+      if (hint) hint.textContent = !restricted ? 'All models allowed (no restrictions)' : (value.length === 0 ? 'No models allowed' : value.length + ' model(s) allowed');
       fetch(`/api/auth/users/${encodeURIComponent(username)}/privileges`, {
         method: 'PUT', credentials: 'same-origin',
         headers: { 'Content-Type': 'application/json' },
-        body: JSON.stringify({ allowed_models: value }),
+        body: JSON.stringify({ allowed_models: value, allowed_models_restricted: restricted }),
       }).catch(() => {});
     }
     listEl.querySelectorAll('.priv-model-cb').forEach(cb => cb.addEventListener('change', _saveModels));
diff --git a/tests/test_chat_helpers.py b/tests/test_chat_helpers.py
index 7a7ed28cb..ba23e43ce 100644
--- a/tests/test_chat_helpers.py
+++ b/tests/test_chat_helpers.py
@@ -1,5 +1,67 @@
 import pytest
-from routes.chat_helpers import clean_thinking_for_save, needs_auto_name
+from fastapi import HTTPException
+
+from routes.chat_helpers import _enforce_chat_privileges, clean_thinking_for_save, needs_auto_name
+
+
+class _AuthManager:
+    def __init__(self, privileges):
+        self._privileges = privileges
+
+    def get_privileges(self, username):
+        assert username == "alice"
+        return self._privileges
+
+
+class _Request:
+    def __init__(self, privileges):
+        self.app = type("App", (), {})()
+        self.app.state = type("State", (), {"auth_manager": _AuthManager(privileges)})()
+
+
+class _Session:
+    def __init__(self, model):
+        self.model = model
+
+
+def test_allowed_models_legacy_empty_list_remains_unrestricted(monkeypatch):
+    monkeypatch.setattr("routes.chat_helpers.get_current_user", lambda request: "alice")
+
+    _enforce_chat_privileges(
+        _Request({"allowed_models": [], "max_messages_per_day": 0}),
+        _Session("provider/model-a"),
+    )
+
+
+def test_allowed_models_explicit_empty_restricted_list_blocks_all_models(monkeypatch):
+    monkeypatch.setattr("routes.chat_helpers.get_current_user", lambda request: "alice")
+
+    with pytest.raises(HTTPException) as exc:
+        _enforce_chat_privileges(
+            _Request({
+                "allowed_models": [],
+                "allowed_models_restricted": True,
+                "max_messages_per_day": 0,
+            }),
+            _Session("provider/model-a"),
+        )
+
+    assert exc.value.status_code == 403
+    assert "provider/model-a" in exc.value.detail
+
+
+def test_allowed_models_nonempty_list_still_restricts_without_new_flag(monkeypatch):
+    monkeypatch.setattr("routes.chat_helpers.get_current_user", lambda request: "alice")
+
+    _enforce_chat_privileges(
+        _Request({"allowed_models": ["provider/model-a"], "max_messages_per_day": 0}),
+        _Session("provider/model-a"),
+    )
+    with pytest.raises(HTTPException):
+        _enforce_chat_privileges(
+            _Request({"allowed_models": ["provider/model-a"], "max_messages_per_day": 0}),
+            _Session("provider/model-b"),
+        )
 
 
 @pytest.mark.parametrize("name,expected", [