Fix profiles losing their selected users
Starting an M365 scan wrote the sidebar state back into the active profile via _save_settings(), replacing its user_ids with the sidebar selection (minus Google-only users and anything hidden by the role filter). With no profile active it overwrote profiles[0]. Profiles are loaders, so scan start no longer writes them; profile_id is audit-only. The profile editor built its account list from S._allUsers only, so manually added accounts, users not yet loaded, and users gone from the directory were not rendered and got dropped on the next save. They now render as checked "Manual" / "Not loaded" rows, and user_ids "all" is kept when saved before users load. Applying a profile cleared the pending user IDs before the un-awaited Google merge had added google:<email> users, leaving them unticked. Pending IDs are now kept until all are matched or the user list is final, and _mergeGoogleUsers() re-applies them. A saved google:<email> also matches an M365 user merged to platform 'both'. Adds regression tests for scan start leaving profiles untouched. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
1 parent
b77bf70ff5
commit
bbe9c62ff9
10 files changed
+146
-17
No files matched your search
@@ -532,6 +532,8 @@
|
||||
"m365_profile_save_tip": "Gem aktuelle indstillinger som profil",
|
||||
"m365_profile_save_prompt": "Profilnavn:",
|
||||
"m365_profile_applied": "Profil indlæst",
|
||||
"m365_profile_acct_manual": "Manuel",
|
||||
"m365_profile_acct_unresolved": "Ikke indlæst",
|
||||
"m365_profile_saved": "Profil gemt",
|
||||
"m365_profile_manage_btn": "Profiler",
|
||||
"m365_profile_clear_btn": "Ryd",
|
||||
|
||||
@@ -532,6 +532,8 @@
|
||||
"m365_profile_save_tip": "Aktuelle Einstellungen als Profil speichern",
|
||||
"m365_profile_save_prompt": "Profilname:",
|
||||
"m365_profile_applied": "Profil geladen",
|
||||
"m365_profile_acct_manual": "Manuell",
|
||||
"m365_profile_acct_unresolved": "Nicht geladen",
|
||||
"m365_profile_saved": "Profil gespeichert",
|
||||
"m365_profile_manage_btn": "Profile",
|
||||
"m365_profile_clear_btn": "Löschen",
|
||||
|
||||
@@ -532,6 +532,8 @@
|
||||
"m365_profile_save_tip": "Save current settings as a profile",
|
||||
"m365_profile_save_prompt": "Profile name:",
|
||||
"m365_profile_applied": "Profile loaded",
|
||||
"m365_profile_acct_manual": "Manual",
|
||||
"m365_profile_acct_unresolved": "Not loaded",
|
||||
"m365_profile_saved": "Profile saved",
|
||||
"m365_profile_manage_btn": "Profiles",
|
||||
"m365_profile_clear_btn": "Clear",
|
||||
|
||||
+2
-1
@@ -115,5 +115,6 @@ Optional AI-powered NER replacing spaCy. Activated via `config.json` keys `claud
|
||||
## Gotchas
|
||||
|
||||
- **`_load_settings()` return** — does NOT include `file_sources`. Returns only: sources, user_ids, options, retention_years, fiscal_year_end, email_to.
|
||||
- **`_save_settings()` clobbers profile fields** — called on every M365 scan start with only M365 sources/user_ids/options. The fix in `app_config.py` preserves `google_sources` and `file_sources` and rebuilds `sources` as `m365_src + google_src + file_src`. Do not simplify away this merge logic.
|
||||
- **`POST /api/scan/start` must not write profiles** — it used to call `_save_settings()` with the sidebar state, which overwrote the active profile's `user_ids` (dropping Google-only users and role-filtered-out users) and, with no active profile, clobbered `profiles[0]`. Profiles change only via `/api/profiles/save`. `profile_id` in the start payload is audit-only. Do not re-add the call.
|
||||
- **`_save_settings()` merge logic** — still used by `/api/settings/save`. It preserves `google_sources` and `file_sources` and rebuilds `sources` as `m365_src + google_src + file_src`. Do not simplify away this merge logic.
|
||||
- **`loadLastScanSummary()` timing** — must only be called after the first `/api/scan/status` poll resolves (inside `_sseWatchdog` in `results.js`, guarded by `_initialStatusChecked`). Calling it on `DOMContentLoaded` shows a stale "no results" card during a live scan after a hard refresh.
|
||||
+3
-5
@@ -103,12 +103,10 @@ def scan_start():
|
||||
return jsonify({"error": "scan already running"}), 409
|
||||
options = request.get_json() or {}
|
||||
state._scan_abort.clear()
|
||||
# Profiles are loaders: a scan must never write the sidebar state back into
|
||||
# a profile (it overwrote user_ids, dropped Google-only users, and hit
|
||||
# profiles[0] when no profile was active). profile_id is audit-only here.
|
||||
profile_id = options.pop("profile_id", None)
|
||||
_save_settings({
|
||||
"sources": options.get("sources", []),
|
||||
"user_ids": options.get("user_ids", []),
|
||||
"options": options.get("options", {}),
|
||||
}, profile_id=profile_id)
|
||||
def _run():
|
||||
from scan_engine import run_scan
|
||||
try:
|
||||
|
||||
+2
-1
@@ -80,7 +80,8 @@ Never revert to `!!window._googleConnected` / `_fileSources.length > 0` — thos
|
||||
|
||||
- **`scheduler.js` strings must use `t()`** — frequency labels, "Next", "Running...", "Disabled", empty-job text, and empty-history text all have translation keys. Do not hard-code English strings in `schedLoad()` or `schedRenderJobs()`.
|
||||
- **Scheduler UI — `schedToggleReportOnly()`** — dims the Profile row, shows/hides `#schedReportOnlyHint`, and forces `#schedAutoEmail` checked. Called from the checkbox `onchange` handler and at the start of `schedAddJob()` / `schedEditJob()`.
|
||||
- **Profile editor accounts** — default to unchecked. Only explicitly saved `user_ids` are checked.
|
||||
- **Profile editor accounts** — default to unchecked. Only explicitly saved `user_ids` are checked. Saved IDs not in `S._allUsers` (`manual:<email>`, users not loaded yet, users gone from the directory) are rendered by `_unresolvedAccountRows()` as checked rows so the next save keeps them — do not go back to building rows from `S._allUsers` alone (that silently dropped them on save). `user_ids: "all"` is kept as `"all"` when saved before users have loaded.
|
||||
- **Pending profile users wait for the Google merge** — `_applyPendingProfileUsers()` keeps `window._pendingProfileUserIds` until every ID is matched or `_userListFinal()` (users loaded, and Google either not connected or `S._googleUsersMerged`). `_mergeGoogleUsers()` re-applies in its `finally`. A saved `google:<email>` also matches an M365 user merged to `platform: 'both'` via `u.googleEmail`. Do not clear the pending IDs unconditionally — Google-only users would stay unticked.
|
||||
- **Date presets** — stored as `years * 365` (integer days). Do not use `* 365.25`.
|
||||
- **`copyTokenLink` is async** — called from `onclick` as fire-and-forget. Do not make it synchronous.
|
||||
- **Escape scan-derived strings with `esc()`** — `results.js` defines `esc()` (escapes `& < > " '`). Every value that originates from scanned content (`f.name`, `f.account_name`, `f.folder`, `f.source`, `f.modified`, `label`, image `alt`, and the same fields on `item`/related rows) must pass through `esc()` before going into `innerHTML` or a `title=`/`alt=` attribute. These are attacker-influenceable (e.g. a file named with markup), so an unescaped interpolation is stored XSS — including in shared read-only viewer sessions. Numeric counts (`cpr_count`, `size_kb`) don't need it. When embedding an object in an `onclick` payload, also `.replace(/"/g,'"')` the `JSON.stringify(...)`.
|
||||
+52
-10
@@ -225,13 +225,23 @@ function _applyProfile(profile) {
|
||||
log(t('m365_profile_applied', 'Profile loaded') + ': ' + profile.name);
|
||||
}
|
||||
|
||||
// Google-only users (id "google:<email>") arrive later than M365 users, via the
|
||||
// un-awaited _mergeGoogleUsers(). Keep the pending IDs until every one is
|
||||
// matched or the user list is final, else those users stay unticked.
|
||||
function _userListFinal() {
|
||||
return S._allUsers.length > 0 && (!window._googleConnected || S._googleUsersMerged);
|
||||
}
|
||||
|
||||
function _applyPendingProfileUsers() {
|
||||
const ids = window._pendingProfileUserIds;
|
||||
if (!ids || !ids.length || !S._allUsers.length) return;
|
||||
// Select only the users listed in the profile
|
||||
S._allUsers.forEach(u => { u.selected = ids.includes(u.id); });
|
||||
const wanted = new Set(ids);
|
||||
// A saved google:<email> may since have merged onto an M365 user (platform 'both')
|
||||
const matches = u => wanted.has(u.id) || (u.googleEmail && wanted.has('google:' + u.googleEmail));
|
||||
S._allUsers.forEach(u => { u.selected = matches(u); });
|
||||
renderAccountList();
|
||||
window._pendingProfileUserIds = null;
|
||||
const found = S._allUsers.filter(matches).length;
|
||||
if (found >= wanted.size || _userListFinal()) window._pendingProfileUserIds = null;
|
||||
}
|
||||
|
||||
async function saveCurrentAsProfile() {
|
||||
@@ -365,19 +375,23 @@ function _openEditorForProfile(profile) {
|
||||
const srcCheck = (id) => allSources.includes(id) ? 'checked' : '';
|
||||
|
||||
// Build account list from S._allUsers
|
||||
const savedIds = new Set((profile.user_ids || []).map(u => u.id || u));
|
||||
// If no saved IDs match current users, treat as all-selected (new profile or users changed)
|
||||
const anyMatch = savedIds.size > 0 && S._allUsers.some(u => savedIds.has(u.id));
|
||||
const savedAll = profile.user_ids === 'all';
|
||||
const savedList = Array.isArray(profile.user_ids) ? profile.user_ids : [];
|
||||
const savedIds = new Set(savedList.map(u => u.id || u));
|
||||
const isSaved = u => savedAll || savedIds.has(u.id)
|
||||
|| !!(u.googleEmail && savedIds.has('google:' + u.googleEmail));
|
||||
// Remember "all" so saving before the user list has loaded doesn't turn it into []
|
||||
window._pmgmtSavedAll = savedAll;
|
||||
const accountRows = S._allUsers.map(u => {
|
||||
// Only check if the user was explicitly saved — default to unchecked like the main window
|
||||
const checked = anyMatch && savedIds.has(u.id) ? 'checked' : '';
|
||||
const checked = isSaved(u) ? 'checked' : '';
|
||||
const platBadge = u.platform === 'both' ? '<span style="font-size:9px;padding:1px 5px;border-radius:10px;background:linear-gradient(90deg,#E6F1FB 50%,#EAF3DE 50%);color:#1a4a1a;font-weight:500;border:0.5px solid #b5d4b5">M365+GWS</span>'
|
||||
: (u.platform || 'm365') === 'google' ? '<span style="font-size:9px;padding:1px 5px;border-radius:10px;background:#EAF3DE;color:#3B6D11;font-weight:500">GWS</span>'
|
||||
: '<span style="font-size:9px;padding:1px 5px;border-radius:10px;background:#E6F1FB;color:#185FA5;font-weight:500">M365</span>';
|
||||
const roleBadge = u.userRole === 'student' ? t('role_student','Elev') : u.userRole === 'staff' ? t('role_staff','Ansat') : t('role_other','Anden');
|
||||
const roleOverrideStyle = u.roleOverride ? 'color:var(--color-text-info);outline:1px solid var(--color-border-info);' : '';
|
||||
return `<label class="pmgmt-acct-row" data-uid="${_esc(u.id)}" data-role="${_esc(u.userRole || 'other')}"><input type="checkbox" ${checked} data-uid="${_esc(u.id)}"><span style="flex:1;color:var(--color-text-primary);overflow:hidden;text-overflow:ellipsis;white-space:nowrap">${_esc(u.displayName)}</span>${platBadge}<button type="button" class="pmgmt-role-badge" data-uid="${_esc(u.id)}" onclick="_pmgmtCycleRole(this.getAttribute('data-uid'),event)" style="font-size:9px;padding:1px 5px;border-radius:10px;background:#D3D1C7;border:none;cursor:pointer;${roleOverrideStyle}">${roleBadge}</button></label>`;
|
||||
}).join('');
|
||||
}).join('') + _unresolvedAccountRows(savedList);
|
||||
|
||||
body.innerHTML = `
|
||||
<div>
|
||||
@@ -464,6 +478,28 @@ function _openEditorForProfile(profile) {
|
||||
_renderEditorSources((profile.sources || []).concat(profile.google_sources || []).concat(profile.file_sources || []));
|
||||
}
|
||||
|
||||
// Saved accounts that aren't in S._allUsers — manually added ("manual:<email>"),
|
||||
// not loaded yet, or gone from the directory. Rendered as checked rows so the
|
||||
// next save keeps them instead of silently dropping them.
|
||||
function _unresolvedAccountRows(savedList) {
|
||||
const known = new Set();
|
||||
S._allUsers.forEach(u => {
|
||||
known.add(u.id);
|
||||
if (u.googleEmail) known.add('google:' + u.googleEmail);
|
||||
});
|
||||
const seen = new Set();
|
||||
return savedList.map(u => {
|
||||
const id = u.id || u;
|
||||
if (!id || known.has(id) || seen.has(id)) return '';
|
||||
seen.add(id);
|
||||
const isManual = id.startsWith('manual:');
|
||||
const label = (typeof u === 'object' && u.displayName) || id.replace(/^(manual|google):/, '');
|
||||
const badge = isManual ? t('m365_profile_acct_manual', 'Manual')
|
||||
: t('m365_profile_acct_unresolved', 'Not loaded');
|
||||
return `<label class="pmgmt-acct-row" data-uid="${_esc(id)}" data-role="${_esc((typeof u === 'object' && u.userRole) || 'other')}"><input type="checkbox" checked data-uid="${_esc(id)}"><span style="flex:1;color:var(--text);overflow:hidden;text-overflow:ellipsis;white-space:nowrap" title="${_esc(id)}">${_esc(label)}</span><span style="font-size:9px;padding:1px 5px;border-radius:10px;background:#D3D1C7;color:#444441">${_esc(badge)}</span></label>`;
|
||||
}).join('');
|
||||
}
|
||||
|
||||
function _peSetDate(val) {
|
||||
if (!val) return;
|
||||
const ms = new Date() - new Date(val);
|
||||
@@ -601,7 +637,9 @@ function _pmgmtAddManual() {
|
||||
if (list.querySelector(`input[data-uid="${id}"]`)) return; // already exists
|
||||
const lbl = document.createElement('label');
|
||||
lbl.className = 'pmgmt-acct-row';
|
||||
lbl.innerHTML = `<input type="checkbox" checked data-uid="${_esc(id)}"><span style="flex:1;color:var(--text);overflow:hidden;text-overflow:ellipsis;white-space:nowrap">${_esc(email.trim())}</span><span style="font-size:9px;padding:1px 5px;border-radius:10px;background:#D3D1C7;color:#444441">Manuel</span>`;
|
||||
lbl.dataset.uid = id;
|
||||
lbl.dataset.role = 'other';
|
||||
lbl.innerHTML = `<input type="checkbox" checked data-uid="${_esc(id)}"><span style="flex:1;color:var(--text);overflow:hidden;text-overflow:ellipsis;white-space:nowrap">${_esc(email.trim())}</span><span style="font-size:9px;padding:1px 5px;border-radius:10px;background:#D3D1C7;color:#444441">${_esc(t('m365_profile_acct_manual', 'Manual'))}</span>`;
|
||||
list.appendChild(lbl);
|
||||
}
|
||||
|
||||
@@ -635,9 +673,11 @@ async function _pmgmtSaveFullEdit() {
|
||||
const effectiveGoogleSources = googleRendered ? googleSources : (profile.google_sources || []);
|
||||
const effectiveFileSources = fileRendered ? fileSources : (profile.file_sources || []);
|
||||
const allSources = m365Sources.concat(effectiveGoogleSources).concat(effectiveFileSources);
|
||||
const user_ids = Array.from(document.querySelectorAll('#pmgmtAcctList input[type=checkbox]:checked'))
|
||||
let user_ids = Array.from(document.querySelectorAll('#pmgmtAcctList input[type=checkbox]:checked'))
|
||||
.map(cb => cb.dataset.uid)
|
||||
.filter(Boolean);
|
||||
// "all" profile opened before users loaded: there were no rows to tick, so keep "all"
|
||||
if (window._pmgmtSavedAll && !S._allUsers.length && !user_ids.length) user_ids = 'all';
|
||||
const updated = {
|
||||
...profile,
|
||||
name,
|
||||
@@ -769,6 +809,8 @@ window._esc = _esc;
|
||||
window._pmgmtUse = _pmgmtUse;
|
||||
window._pmgmtOpenEditor = _pmgmtOpenEditor;
|
||||
window._openEditorForProfile = _openEditorForProfile;
|
||||
window._unresolvedAccountRows = _unresolvedAccountRows;
|
||||
window._userListFinal = _userListFinal;
|
||||
window._peSetDate = _peSetDate;
|
||||
window._peSetYear = _peSetYear;
|
||||
window._renderEditorSources = _renderEditorSources;
|
||||
|
||||
@@ -19,6 +19,7 @@ export const S = {
|
||||
_progressCurrentUser: '',
|
||||
// Users
|
||||
_allUsers: [],
|
||||
_googleUsersMerged: false, // true once _mergeGoogleUsers() has added Google users
|
||||
// Auth
|
||||
_currentAppMode: null,
|
||||
// Profiles
|
||||
|
||||
@@ -107,6 +107,12 @@ async function _mergeGoogleUsers() {
|
||||
S._allUsers = S._allUsers.concat(googleUsers);
|
||||
renderAccountList();
|
||||
} catch(e) { /* Google users unavailable */ }
|
||||
finally {
|
||||
// Google users are now as complete as they'll get (even on error/early
|
||||
// return) — finish a profile apply that was waiting on google:<email> IDs.
|
||||
S._googleUsersMerged = true;
|
||||
_applyPendingProfileUsers();
|
||||
}
|
||||
}
|
||||
|
||||
let _activeRoleFilter = ''; // '' = all, 'staff', 'student'
|
||||
|
||||
@@ -661,3 +661,77 @@ class TestProfileRoutes:
|
||||
def test_delete_missing_key_returns_400(self, client):
|
||||
r = client.post("/api/profiles/delete", json={})
|
||||
assert r.status_code == 400
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Scan start must not write profiles
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
class TestScanStartLeavesProfilesAlone:
|
||||
"""
|
||||
Regression: POST /api/scan/start used to call _save_settings() with the
|
||||
sidebar state, overwriting the active profile's user_ids (dropping
|
||||
Google-only users) — or profiles[0] when no profile was active.
|
||||
"""
|
||||
|
||||
@pytest.fixture(autouse=True)
|
||||
def _isolate(self, tmp_path, monkeypatch):
|
||||
import app_config, scan_engine
|
||||
monkeypatch.setattr(app_config, "_SETTINGS_PATH", tmp_path / "settings.json")
|
||||
monkeypatch.setattr(scan_engine, "run_scan", lambda opts: None)
|
||||
|
||||
@staticmethod
|
||||
def _wait_for_lock():
|
||||
from routes import state
|
||||
deadline = time.time() + 2.0
|
||||
while not state._scan_lock.acquire(blocking=False):
|
||||
assert time.time() < deadline, "scan lock was never released"
|
||||
time.sleep(0.05)
|
||||
state._scan_lock.release()
|
||||
|
||||
def _save(self, client, name, user_ids):
|
||||
r = client.post("/api/profiles/save", json={
|
||||
"name": name, "sources": ["email", "gmail"],
|
||||
"google_sources": ["gmail"], "user_ids": user_ids, "options": {},
|
||||
})
|
||||
return r.get_json()["profile"]
|
||||
|
||||
def _get(self, client, pid):
|
||||
return client.get(f"/api/profiles/get?id={pid}").get_json()["profile"]
|
||||
|
||||
def test_active_profile_users_unchanged(self, client, mock_connector):
|
||||
users = ["m365-a", "google:b@school.dk", "manual:c@school.dk"]
|
||||
p = self._save(client, "Staff", users)
|
||||
r = client.post("/api/scan/start", json={
|
||||
"sources": ["email"],
|
||||
"user_ids": [{"id": "m365-z", "displayName": "Z"}],
|
||||
"options": {"older_than_days": 0},
|
||||
"profile_id": p["id"],
|
||||
})
|
||||
assert r.status_code == 200
|
||||
self._wait_for_lock()
|
||||
after = self._get(client, p["id"])
|
||||
assert after["user_ids"] == users
|
||||
assert after["sources"] == ["email", "gmail"]
|
||||
|
||||
def test_no_active_profile_leaves_first_profile_unchanged(self, client, mock_connector):
|
||||
first = self._save(client, "First", ["m365-a", "m365-b"])
|
||||
self._save(client, "Second", ["m365-c"])
|
||||
r = client.post("/api/scan/start", json={
|
||||
"sources": ["email"],
|
||||
"user_ids": [{"id": "m365-z", "displayName": "Z"}],
|
||||
"options": {},
|
||||
"profile_id": None,
|
||||
})
|
||||
assert r.status_code == 200
|
||||
self._wait_for_lock()
|
||||
assert self._get(client, first["id"])["user_ids"] == ["m365-a", "m365-b"]
|
||||
assert len(client.get("/api/profiles").get_json()["profiles"]) == 2
|
||||
|
||||
def test_scan_start_creates_no_profile(self, client, mock_connector):
|
||||
r = client.post("/api/scan/start", json={
|
||||
"sources": ["email"], "user_ids": [{"id": "m365-z"}], "options": {},
|
||||
})
|
||||
assert r.status_code == 200
|
||||
self._wait_for_lock()
|
||||
assert client.get("/api/profiles").get_json()["profiles"] == []
|
||||
Reference in new issue
Block a user