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.
This commit is contained in:
Alexandre Teixeira
2026-11-01 02:14:32 +00:00
parent 0d25d49d15
commit cee26bc522
4 changed files with 356 additions and 19 deletions
+84 -12
View File
@@ -45,12 +45,16 @@ export async function initImageSettings() {
} }
} }
} catch (e) { console.warn('Failed to load models for image settings', e); } } catch (e) { console.warn('Failed to load models for image settings', e); }
let currentEnabled = enabledToggle ? enabledToggle.checked : false;
try { try {
const settings = await readImagePreferences(); const settings = await readImagePreferences();
if (settings.image_model) modelSel.value = settings.image_model; if (modelSel && 'image_model' in settings) modelSel.value = settings.image_model || '';
if (editModelSel) editModelSel.value = settings.image_edit_model || ''; if (editModelSel && 'image_edit_model' in settings) editModelSel.value = settings.image_edit_model || '';
if (settings.image_quality) qualSel.value = settings.image_quality; if (qualSel && 'image_quality' in settings) qualSel.value = settings.image_quality || 'medium';
if (enabledToggle) enabledToggle.checked = settings.image_gen_enabled === true; if (enabledToggle) {
enabledToggle.checked = settings.image_gen_enabled === true;
currentEnabled = enabledToggle.checked;
}
} catch (e) { console.warn('Failed to load settings', e); } } catch (e) { console.warn('Failed to load settings', e); }
function syncImgDisabled() { function syncImgDisabled() {
@@ -61,21 +65,89 @@ export async function initImageSettings() {
} }
syncImgDisabled(); 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 { 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}`)); 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); 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); if (preferencesListener) window.removeEventListener('image-preferences-changed', preferencesListener);
preferencesListener = event => { 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 (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); window.addEventListener('image-preferences-changed', preferencesListener);
qualSel.addEventListener('change', saveSettings);
if (enabledToggle) enabledToggle.addEventListener('change', function() { syncImgDisabled(); saveSettings(); });
} }
+30 -7
View File
@@ -29,6 +29,11 @@ export function saveImagePreferences(patch) {
invalidateSettings(); invalidateSettings();
if (response.status === 403) { if (response.status === 403) {
const userEntries = Object.entries(patch).filter(([k]) => USER_PREFERENCE_KEYS.includes(k)); 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) { for (const [key, value] of userEntries) {
const prefRes = await fetch(`/api/prefs/${encodeURIComponent(key)}`, { const prefRes = await fetch(`/api/prefs/${encodeURIComponent(key)}`, {
method: 'PUT', credentials: 'same-origin', 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' } }) ? 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}' }; : { ok: true, status: 200, json: async () => ({ ok: true }), text: async () => '{"ok":true}' };
} else if (response.ok) { } else if (response.ok) {
await Promise.all(USER_PREFERENCE_KEYS.map(key => const clearResults = await Promise.all(USER_PREFERENCE_KEYS.map(async key => {
fetch(`/api/prefs/${encodeURIComponent(key)}`, { try {
method: 'PUT', credentials: 'same-origin', const res = await fetch(`/api/prefs/${encodeURIComponent(key)}`, {
headers: { 'Content-Type': 'application/json' }, method: 'PUT', credentials: 'same-origin',
body: JSON.stringify({ value: null }), headers: { 'Content-Type': 'application/json' },
}).catch(() => null) 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 { } else {
throw new Error('Could not save image settings'); throw new Error('Could not save image settings');
} }
@@ -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): 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} saved = {'image_model':'openai/gpt-5-image', 'image_edit_model':'', 'image_quality':'medium', 'image_gen_enabled':True}
writes = [] writes = []
user_prefs = {}
def settings_api(route): def settings_api(route):
if route.request.method == 'POST': if route.request.method == 'POST':
@@ -418,7 +419,22 @@ def test_image_model_settings_sync_both_directions_and_survive_reopening(page):
saved.update(patch) saved.update(patch)
route.fulfill(json=dict(saved)) 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/auth/settings', settings_api)
page.route('**/api/prefs**', prefs_api)
page.route('**/api/model-endpoints', lambda route: route.fulfill(json=[{ page.route('**/api/model-endpoints', lambda route: route.fulfill(json=[{
'is_enabled':True, 'online':True, 'model_type':'llm', 'is_enabled':True, 'online':True, 'model_type':'llm',
'models':['openai/gpt-5-image','openai/gpt-image-2'], 'models':['openai/gpt-5-image','openai/gpt-image-2'],
+226
View File
@@ -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_edit_model"] == "global-edit"
assert data["loaded"]["image_quality"] == "low" 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"