From cee26bc52237549e77647002d95a6d1b177d5836 Mon Sep 17 00:00:00 2001 From: Alexandre Teixeira Date: Sun, 1 Nov 2026 02:14:32 +0000 Subject: [PATCH] fix(settings): isolate non-admin image toggle and surface preference cleanup errors - Restrict image generation availability toggle to admin users in imageModels.js, disabling the switch for non-admins and reverting unauthorized changes with an explicit message. - Separate non-admin image model preference saves from global toggle updates. - Track preference cleanup results on admin save in imagePreferences.js, propagating cleanup failures instead of ignoring them. - Require valid preference keys on 403 fallback so toggle-only attempts reject cleanly. - Add /api/prefs route mock to test_image_model_settings_sync_both_directions_and_survive_reopening. - Add regression coverage for admin cleanup failures and non-admin toggle rejection. --- static/js/settings/imageModels.js | 96 +++++++- static/js/settings/imagePreferences.js | 37 ++- ...est_editor_layer_scoped_actions_browser.py | 16 ++ tests/test_settings_save_response_ok_js.py | 226 ++++++++++++++++++ 4 files changed, 356 insertions(+), 19 deletions(-) diff --git a/static/js/settings/imageModels.js b/static/js/settings/imageModels.js index 37fedf666..9a5aa2b89 100644 --- a/static/js/settings/imageModels.js +++ b/static/js/settings/imageModels.js @@ -45,12 +45,16 @@ export async function initImageSettings() { } } } catch (e) { console.warn('Failed to load models for image settings', e); } + let currentEnabled = enabledToggle ? enabledToggle.checked : false; try { const settings = await readImagePreferences(); - if (settings.image_model) modelSel.value = settings.image_model; - if (editModelSel) editModelSel.value = settings.image_edit_model || ''; - if (settings.image_quality) qualSel.value = settings.image_quality; - if (enabledToggle) enabledToggle.checked = settings.image_gen_enabled === true; + if (modelSel && 'image_model' in settings) modelSel.value = settings.image_model || ''; + if (editModelSel && 'image_edit_model' in settings) editModelSel.value = settings.image_edit_model || ''; + if (qualSel && 'image_quality' in settings) qualSel.value = settings.image_quality || 'medium'; + if (enabledToggle) { + enabledToggle.checked = settings.image_gen_enabled === true; + currentEnabled = enabledToggle.checked; + } } catch (e) { console.warn('Failed to load settings', e); } function syncImgDisabled() { @@ -61,21 +65,89 @@ export async function initImageSettings() { } syncImgDisabled(); - async function saveSettings() { + function checkIsAdmin() { + return typeof window !== 'undefined' && !!window._isAdmin; + } + + function applyAdminToggleState() { + if (!enabledToggle) return; + const admin = checkIsAdmin(); + if (!admin) { + enabledToggle.disabled = true; + const adminTitle = 'Admin access is required to change image generation availability.'; + enabledToggle.title = adminTitle; + if (enabledToggle.parentElement) enabledToggle.parentElement.title = adminTitle; + } else { + enabledToggle.disabled = false; + const adminTitle = 'Enable or disable image generation globally'; + enabledToggle.title = adminTitle; + if (enabledToggle.parentElement) enabledToggle.parentElement.title = adminTitle; + } + } + applyAdminToggleState(); + + async function saveModelPreferences() { try { - const res = await _postSettings({ image_gen_enabled: enabledToggle ? enabledToggle.checked : false, image_model: modelSel.value, image_edit_model: editModelSel?.value || '', image_quality: qualSel.value }); + const res = await _postSettings({ + image_model: modelSel ? modelSel.value : '', + image_edit_model: editModelSel?.value || '', + image_quality: qualSel ? qualSel.value : 'medium', + }); if (!res.ok) throw new Error(await res.text().catch(() => `HTTP ${res.status}`)); msg.textContent = 'Saved'; msg.style.color = 'var(--fg)'; setTimeout(() => { msg.textContent = ''; }, 2000); - } catch (e) { msg.textContent = e.status === 403 ? 'Admin access is required to change these settings.' : 'Failed to save'; msg.style.color = 'var(--red)'; } + } catch (e) { + msg.textContent = e.status === 403 ? 'Admin access is required to change these settings.' : 'Failed to save'; + msg.style.color = 'var(--red)'; + } } - modelSel.addEventListener('change', saveSettings); - editModelSel?.addEventListener('change', saveSettings); + + async function saveToggle() { + if (!checkIsAdmin()) { + if (enabledToggle) enabledToggle.checked = currentEnabled; + syncImgDisabled(); + msg.textContent = 'Admin access is required to change these settings.'; + msg.style.color = 'var(--red)'; + return; + } + try { + const res = await _postSettings({ image_gen_enabled: enabledToggle ? enabledToggle.checked : false }); + if (!res.ok) throw new Error(await res.text().catch(() => `HTTP ${res.status}`)); + if (enabledToggle) currentEnabled = enabledToggle.checked; + msg.textContent = 'Saved'; msg.style.color = 'var(--fg)'; setTimeout(() => { msg.textContent = ''; }, 2000); + } catch (e) { + if (enabledToggle) enabledToggle.checked = currentEnabled; + syncImgDisabled(); + msg.textContent = e.status === 403 ? 'Admin access is required to change these settings.' : 'Failed to save'; + msg.style.color = 'var(--red)'; + } + } + + if (modelSel) modelSel.addEventListener('change', saveModelPreferences); + if (editModelSel) editModelSel.addEventListener('change', saveModelPreferences); + if (qualSel) qualSel.addEventListener('change', saveModelPreferences); + if (enabledToggle) { + enabledToggle.addEventListener('change', async function() { + if (!checkIsAdmin()) { + enabledToggle.checked = currentEnabled; + syncImgDisabled(); + msg.textContent = 'Admin access is required to change these settings.'; + msg.style.color = 'var(--red)'; + return; + } + syncImgDisabled(); + await saveToggle(); + }); + } + if (preferencesListener) window.removeEventListener('image-preferences-changed', preferencesListener); preferencesListener = event => { - if ('image_model' in event.detail) modelSel.value = event.detail.image_model || ''; + if ('image_model' in event.detail && modelSel) modelSel.value = event.detail.image_model || ''; if (editModelSel && 'image_edit_model' in event.detail) editModelSel.value = event.detail.image_edit_model || ''; + if ('image_gen_enabled' in event.detail && enabledToggle) { + enabledToggle.checked = event.detail.image_gen_enabled === true; + currentEnabled = enabledToggle.checked; + syncImgDisabled(); + } }; window.addEventListener('image-preferences-changed', preferencesListener); - qualSel.addEventListener('change', saveSettings); - if (enabledToggle) enabledToggle.addEventListener('change', function() { syncImgDisabled(); saveSettings(); }); } diff --git a/static/js/settings/imagePreferences.js b/static/js/settings/imagePreferences.js index 47e20df50..90a07011c 100644 --- a/static/js/settings/imagePreferences.js +++ b/static/js/settings/imagePreferences.js @@ -29,6 +29,11 @@ export function saveImagePreferences(patch) { invalidateSettings(); if (response.status === 403) { const userEntries = Object.entries(patch).filter(([k]) => USER_PREFERENCE_KEYS.includes(k)); + if (userEntries.length === 0) { + const err = new Error('Admin access is required to change these settings.'); + err.status = 403; + throw err; + } for (const [key, value] of userEntries) { const prefRes = await fetch(`/api/prefs/${encodeURIComponent(key)}`, { method: 'PUT', credentials: 'same-origin', @@ -45,13 +50,31 @@ export function saveImagePreferences(patch) { ? new Response(JSON.stringify({ ok: true }), { status: 200, headers: { 'Content-Type': 'application/json' } }) : { ok: true, status: 200, json: async () => ({ ok: true }), text: async () => '{"ok":true}' }; } else if (response.ok) { - await Promise.all(USER_PREFERENCE_KEYS.map(key => - fetch(`/api/prefs/${encodeURIComponent(key)}`, { - method: 'PUT', credentials: 'same-origin', - headers: { 'Content-Type': 'application/json' }, - body: JSON.stringify({ value: null }), - }).catch(() => null) - )); + const clearResults = await Promise.all(USER_PREFERENCE_KEYS.map(async key => { + try { + const res = await fetch(`/api/prefs/${encodeURIComponent(key)}`, { + method: 'PUT', credentials: 'same-origin', + headers: { 'Content-Type': 'application/json' }, + body: JSON.stringify({ value: null }), + }); + if (!res.ok) { + const err = new Error(await res.text().catch(() => `HTTP ${res.status}`)); + err.status = res.status; + return { ok: false, error: err }; + } + return { ok: true }; + } catch (err) { + return { ok: false, error: err }; + } + })); + const failure = clearResults.find(r => !r.ok); + if (failure) { + invalidateSettings(); + const err = new Error(failure.error?.message || 'Could not clear personal preferences'); + err.status = failure.error?.status || 500; + err.cause = failure.error; + throw err; + } } else { throw new Error('Could not save image settings'); } diff --git a/tests/test_editor_layer_scoped_actions_browser.py b/tests/test_editor_layer_scoped_actions_browser.py index 92348cb50..92fca5966 100644 --- a/tests/test_editor_layer_scoped_actions_browser.py +++ b/tests/test_editor_layer_scoped_actions_browser.py @@ -410,6 +410,7 @@ def test_ai_settings_escape_closes_only_the_dialog(page): def test_image_model_settings_sync_both_directions_and_survive_reopening(page): saved = {'image_model':'openai/gpt-5-image', 'image_edit_model':'', 'image_quality':'medium', 'image_gen_enabled':True} writes = [] + user_prefs = {} def settings_api(route): if route.request.method == 'POST': @@ -418,7 +419,22 @@ def test_image_model_settings_sync_both_directions_and_survive_reopening(page): saved.update(patch) route.fulfill(json=dict(saved)) + def prefs_api(route): + url = route.request.url + if route.request.method == 'PUT': + key = url.split('/api/prefs/')[-1] + data = route.request.post_data_json or {} + val = data.get('value') + if val is None: + user_prefs.pop(key, None) + else: + user_prefs[key] = val + route.fulfill(status=200, json={'key': key, 'value': val}) + return + route.fulfill(status=200, json=dict(user_prefs)) + page.route('**/api/auth/settings', settings_api) + page.route('**/api/prefs**', prefs_api) page.route('**/api/model-endpoints', lambda route: route.fulfill(json=[{ 'is_enabled':True, 'online':True, 'model_type':'llm', 'models':['openai/gpt-5-image','openai/gpt-image-2'], diff --git a/tests/test_settings_save_response_ok_js.py b/tests/test_settings_save_response_ok_js.py index 13a96a1af..4d1ccd27b 100644 --- a/tests/test_settings_save_response_ok_js.py +++ b/tests/test_settings_save_response_ok_js.py @@ -237,3 +237,229 @@ def test_image_preferences_js_policy_and_non_admin_fallback(): assert data["loaded"]["image_edit_model"] == "global-edit" assert data["loaded"]["image_quality"] == "low" + +@pytest.mark.skipif(not _HAS_NODE, reason="node binary not on PATH") +def test_admin_cleanup_failure_and_rejection_surfaced(): + """Verify saveImagePreferences surfaces HTTP failures and rejected network requests during admin cleanup.""" + js = """ + globalThis.window = { dispatchEvent: () => {} }; + globalThis.CustomEvent = class CustomEvent {}; + + const { saveImagePreferences } = await import('./static/js/settings/imagePreferences.js'); + + const results = {}; + + // 1. HTTP 500 failure during admin preference cleanup + globalThis.fetch = async (url) => { + if (url === '/api/auth/settings') { + return { status: 200, ok: true, json: async () => ({ ok: true }) }; + } + if (url.startsWith('/api/prefs/')) { + return { status: 500, ok: false, text: async () => 'Database lock error' }; + } + throw new Error('Unexpected URL: ' + url); + }; + try { + await saveImagePreferences({ image_model: 'admin-model', image_gen_enabled: true }); + results.http500Threw = false; + } catch (err) { + results.http500Threw = true; + results.http500Status = err.status; + } + + // 2. Network rejection during admin preference cleanup + globalThis.fetch = async (url) => { + if (url === '/api/auth/settings') { + return { status: 200, ok: true, json: async () => ({ ok: true }) }; + } + if (url.startsWith('/api/prefs/')) { + throw new Error('Network offline'); + } + throw new Error('Unexpected URL: ' + url); + }; + try { + await saveImagePreferences({ image_model: 'admin-model', image_gen_enabled: true }); + results.networkRejectThrew = false; + } catch (err) { + results.networkRejectThrew = true; + results.networkRejectStatus = err.status; + } + + // 3. Successful cleanup + const clearedKeys = []; + globalThis.fetch = async (url, opts = {}) => { + if (url === '/api/auth/settings') { + return { status: 200, ok: true, json: async () => ({ ok: true }) }; + } + if (url.startsWith('/api/prefs/')) { + clearedKeys.push({ url, body: JSON.parse(opts.body) }); + return { status: 200, ok: true, json: async () => ({ ok: true }) }; + } + throw new Error('Unexpected URL: ' + url); + }; + const okRes = await saveImagePreferences({ image_model: 'admin-model', image_gen_enabled: true }); + results.okResStatus = okRes.status; + results.clearedKeysCount = clearedKeys.length; + + console.log(JSON.stringify(results)); + """ + proc = subprocess.run(["node", "--input-type=module", "-e", js], capture_output=True, text=True, check=True) + import json + data = json.loads(proc.stdout) + assert data["http500Threw"] is True + assert data["http500Status"] == 500 + assert data["networkRejectThrew"] is True + assert data["networkRejectStatus"] == 500 + assert data["okResStatus"] == 200 + assert data["clearedKeysCount"] == 3 + + +@pytest.mark.skipif(not _HAS_NODE, reason="node binary not on PATH") +def test_non_admin_toggle_rejection_and_ui_behavior(): + """Verify non-admin cannot change image_gen_enabled via API or UI, and receives unambiguous feedback.""" + js = """ + const { saveImagePreferences } = await import('./static/js/settings/imagePreferences.js'); + + const results = {}; + + // 1. Direct call to saveImagePreferences with only non-admin excluded toggle + globalThis.window = { dispatchEvent: () => {} }; + globalThis.CustomEvent = class CustomEvent {}; + globalThis.fetch = async (url) => { + if (url === '/api/auth/settings') return { status: 403, ok: false, text: async () => 'Forbidden' }; + throw new Error('Unexpected URL: ' + url); + }; + try { + await saveImagePreferences({ image_gen_enabled: true }); + results.toggleOnlyThrew = false; + } catch (err) { + results.toggleOnlyThrew = true; + results.toggleOnlyStatus = err.status; + } + + // 2. Simulated UI interaction in imageModels.js for non-admin + const elements = {}; + function makeElement(id, tag = 'div') { + const listeners = {}; + const el = { + id, tagName: tag.toUpperCase(), + value: '', checked: false, disabled: false, title: '', + style: {}, + parentElement: { title: '', style: {} }, + closest: () => ({ style: {} }), + addEventListener: (evt, cb) => { listeners[evt] = listeners[evt] || []; listeners[evt].push(cb); }, + dispatchEvent: (evt) => Promise.all((listeners[evt] || []).map(cb => cb())), + length: 0, appendChild: () => {}, remove: () => {} + }; + elements[id] = el; + return el; + } + + const modelSel = makeElement('set-imgModelSelect', 'select'); + const editModelSel = makeElement('set-imgEditModelSelect', 'select'); + const qualSel = makeElement('set-imgQualitySelect', 'select'); + const msg = makeElement('set-imgSettingsMsg', 'span'); + const enabledToggle = makeElement('set-imgEnabledToggle', 'input'); + + globalThis.document = { + getElementById: (id) => elements[id] || null, + createElement: (tag) => makeElement('anon-' + Math.random(), tag), + }; + globalThis.window._isAdmin = false; + globalThis.window.addEventListener = () => {}; + globalThis.window.removeEventListener = () => {}; + + const apiCalls = []; + globalThis.fetch = async (url, opts = {}) => { + apiCalls.push({ url, method: opts.method || 'GET' }); + if (url === '/api/model-endpoints') return { ok: true, json: async () => [] }; + if (url === '/api/prefs') return { ok: true, json: async () => ({}) }; + if (url === '/api/auth/settings') { + if (opts.method === 'POST') return { status: 403, ok: false, text: async () => 'Forbidden' }; + return { ok: true, json: async () => ({ image_gen_enabled: true, image_model: 'global-model' }) }; + } + if (url.startsWith('/api/prefs/')) { + return { status: 200, ok: true, json: async () => ({ ok: true }) }; + } + throw new Error('Unexpected URL: ' + url); + }; + + const { initImageSettings } = await import('./static/js/settings/imageModels.js'); + await initImageSettings(); + + // Check non-admin initial UI state + results.nonAdminToggleDisabled = enabledToggle.disabled; + results.nonAdminToggleTitle = enabledToggle.title; + results.nonAdminToggleChecked = enabledToggle.checked; + + // Simulate non-admin attempting to toggle image generation + enabledToggle.checked = false; // user clicked switch to turn off + enabledToggle.dispatchEvent('change'); + results.revertedChecked = enabledToggle.checked; + results.revertedMsgText = msg.textContent; + results.revertedMsgColor = msg.style.color; + + // Simulate non-admin changing model preference + modelSel.value = 'user-selected-model'; + await modelSel.dispatchEvent('change'); + results.modelChangeMsg = msg.textContent; + + // 3. Admin UI interaction in imageModels.js + globalThis.window._isAdmin = true; + const adminToggle = makeElement('set-imgEnabledToggle', 'input'); + const adminMsg = makeElement('set-imgSettingsMsg', 'span'); + elements['set-imgEnabledToggle'] = adminToggle; + elements['set-imgSettingsMsg'] = adminMsg; + + let adminSavedToggle = null; + globalThis.fetch = async (url, opts = {}) => { + if (url === '/api/model-endpoints') return { ok: true, json: async () => [] }; + if (url === '/api/prefs') return { ok: true, json: async () => ({}) }; + if (url === '/api/auth/settings') { + if (opts.method === 'POST') { + const body = JSON.parse(opts.body); + adminSavedToggle = body.image_gen_enabled; + return { status: 200, ok: true, json: async () => ({ ok: true }) }; + } + return { ok: true, json: async () => ({ image_gen_enabled: true, image_model: 'global-model' }) }; + } + if (url.startsWith('/api/prefs/')) { + return { status: 200, ok: true, json: async () => ({ ok: true }) }; + } + throw new Error('Unexpected URL: ' + url); + }; + + await initImageSettings(); + results.adminToggleDisabled = adminToggle.disabled; + adminToggle.checked = false; + await adminToggle.dispatchEvent('change'); + results.adminSavedToggle = adminSavedToggle; + results.adminMsg = adminMsg.textContent; + + console.log(JSON.stringify(results)); + """ + proc = subprocess.run(["node", "--input-type=module", "-e", js], capture_output=True, text=True, check=True) + import json + data = json.loads(proc.stdout) + + # 1. Non-admin toggle-only save threw 403 + assert data["toggleOnlyThrew"] is True + assert data["toggleOnlyStatus"] == 403 + + # 2. Non-admin UI has disabled toggle and rejects changes with warning + assert data["nonAdminToggleDisabled"] is True + assert "Admin access is required" in data["nonAdminToggleTitle"] + assert data["nonAdminToggleChecked"] is True + assert data["revertedChecked"] is True + assert "Admin access is required" in data["revertedMsgText"] + assert data["revertedMsgColor"] == "var(--red)" + + # 3. Non-admin model changes succeed + assert data["modelChangeMsg"] == "Saved" + + # 4. Admin UI allows toggle changes and persists to auth settings + assert data["adminToggleDisabled"] is False + assert data["adminSavedToggle"] is False + assert data["adminMsg"] == "Saved" + +