From bbe9c62ff97463b4dd19e1266b3990d721bd8769 Mon Sep 17 00:00:00 2001 From: StyxX65 <150797939+StyxX65@users.noreply.github.com> Date: Mon, 28 Sep 2026 09:22:32 +0200 Subject: [PATCH 1/2] 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: 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: 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 --- lang/da.json | 2 + lang/de.json | 2 + lang/en.json | 2 + routes/CLAUDE.md | 3 +- routes/scan.py | 8 ++-- static/js/CLAUDE.md | 3 +- static/js/profiles.js | 62 ++++++++++++++++++++++----- static/js/state.js | 1 + static/js/users.js | 6 +++ tests/test_route_integration.py | 74 +++++++++++++++++++++++++++++++++ 10 files changed, 146 insertions(+), 17 deletions(-) diff --git a/lang/da.json b/lang/da.json index d8b96fb..03922bc 100644 --- a/lang/da.json +++ b/lang/da.json @@ -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", diff --git a/lang/de.json b/lang/de.json index 80a880a..20512c6 100644 --- a/lang/de.json +++ b/lang/de.json @@ -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", diff --git a/lang/en.json b/lang/en.json index 3e17284..c8caa83 100644 --- a/lang/en.json +++ b/lang/en.json @@ -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", diff --git a/routes/CLAUDE.md b/routes/CLAUDE.md index 715004e..99eec0c 100644 --- a/routes/CLAUDE.md +++ b/routes/CLAUDE.md @@ -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. diff --git a/routes/scan.py b/routes/scan.py index 02db7e4..bc9ec3f 100644 --- a/routes/scan.py +++ b/routes/scan.py @@ -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: diff --git a/static/js/CLAUDE.md b/static/js/CLAUDE.md index d7cbf57..c55ad23 100644 --- a/static/js/CLAUDE.md +++ b/static/js/CLAUDE.md @@ -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:`, 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:` 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(...)`. diff --git a/static/js/profiles.js b/static/js/profiles.js index 18f9cff..223c9a9 100644 --- a/static/js/profiles.js +++ b/static/js/profiles.js @@ -225,13 +225,23 @@ function _applyProfile(profile) { log(t('m365_profile_applied', 'Profile loaded') + ': ' + profile.name); } +// Google-only users (id "google:") 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: 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' ? 'M365+GWS' : (u.platform || 'm365') === 'google' ? 'GWS' : 'M365'; 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 ``; - }).join(''); + }).join('') + _unresolvedAccountRows(savedList); body.innerHTML = `
@@ -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:"), +// 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 ``; + }).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 = `${_esc(email.trim())}Manuel`; + lbl.dataset.uid = id; + lbl.dataset.role = 'other'; + lbl.innerHTML = `${_esc(email.trim())}${_esc(t('m365_profile_acct_manual', 'Manual'))}`; 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; diff --git a/static/js/state.js b/static/js/state.js index 1050201..b3a98ec 100644 --- a/static/js/state.js +++ b/static/js/state.js @@ -19,6 +19,7 @@ export const S = { _progressCurrentUser: '', // Users _allUsers: [], + _googleUsersMerged: false, // true once _mergeGoogleUsers() has added Google users // Auth _currentAppMode: null, // Profiles diff --git a/static/js/users.js b/static/js/users.js index 2b54a6d..af33622 100644 --- a/static/js/users.js +++ b/static/js/users.js @@ -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: IDs. + S._googleUsersMerged = true; + _applyPendingProfileUsers(); + } } let _activeRoleFilter = ''; // '' = all, 'staff', 'student' diff --git a/tests/test_route_integration.py b/tests/test_route_integration.py index 4cadfef..e48c5e9 100644 --- a/tests/test_route_integration.py +++ b/tests/test_route_integration.py @@ -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"] == [] -- 2.47.3 From 1c79ac15b7e0270a14f71aa5838f891696a75c54 Mon Sep 17 00:00:00 2001 From: StyxX65 <150797939+StyxX65@users.noreply.github.com> Date: Mon, 28 Sep 2026 09:27:56 +0200 Subject: [PATCH 2/2] Scan manually added accounts Manual accounts were never scanned: - Sidebar "add user" entries had no platform, and getSelectedUsers() required platform === 'm365', so they were dropped from every scan. - Profile-editor manual accounts were stored as "manual:", which is not a Graph user id, and applying the profile never put them in the sidebar list, so they could not be selected. Now the editor looks the email up via /api/users/lookup and stores the directory id plus name/email (falling back to manual: only when the lookup fails). Applying a profile adds its manual accounts to the sidebar and resolves legacy manual: entries, logging any that can't be found. The scan engine resolves remaining manual: ids (e.g. from scheduled scans) via the new M365Connector.lookup_user(), skipping and reporting failures without widening to all users. Co-Authored-By: Claude Opus 5.5 --- lang/da.json | 1 + lang/de.json | 1 + lang/en.json | 1 + m365_connector.py | 4 ++ routes/users.py | 2 +- scan_engine.py | 32 ++++++++++++ static/js/CLAUDE.md | 6 +++ static/js/profiles.js | 87 ++++++++++++++++++++++++++----- static/js/state.js | 1 + static/js/users.js | 13 +++-- tests/test_manual_users.py | 104 +++++++++++++++++++++++++++++++++++++ 11 files changed, 234 insertions(+), 18 deletions(-) create mode 100644 tests/test_manual_users.py diff --git a/lang/da.json b/lang/da.json index 03922bc..8c962a6 100644 --- a/lang/da.json +++ b/lang/da.json @@ -534,6 +534,7 @@ "m365_profile_applied": "Profil indlæst", "m365_profile_acct_manual": "Manuel", "m365_profile_acct_unresolved": "Ikke indlæst", + "m365_profile_manual_not_found": "Manuel konto ikke fundet i M365", "m365_profile_saved": "Profil gemt", "m365_profile_manage_btn": "Profiler", "m365_profile_clear_btn": "Ryd", diff --git a/lang/de.json b/lang/de.json index 20512c6..a5fabbf 100644 --- a/lang/de.json +++ b/lang/de.json @@ -534,6 +534,7 @@ "m365_profile_applied": "Profil geladen", "m365_profile_acct_manual": "Manuell", "m365_profile_acct_unresolved": "Nicht geladen", + "m365_profile_manual_not_found": "Manuelles Konto in M365 nicht gefunden", "m365_profile_saved": "Profil gespeichert", "m365_profile_manage_btn": "Profile", "m365_profile_clear_btn": "Löschen", diff --git a/lang/en.json b/lang/en.json index c8caa83..ecbc541 100644 --- a/lang/en.json +++ b/lang/en.json @@ -534,6 +534,7 @@ "m365_profile_applied": "Profile loaded", "m365_profile_acct_manual": "Manual", "m365_profile_acct_unresolved": "Not loaded", + "m365_profile_manual_not_found": "Manual account not found in M365", "m365_profile_saved": "Profile saved", "m365_profile_manage_btn": "Profiles", "m365_profile_clear_btn": "Clear", diff --git a/m365_connector.py b/m365_connector.py index 210f259..ce3c31a 100644 --- a/m365_connector.py +++ b/m365_connector.py @@ -224,6 +224,10 @@ class M365Connector: return {"displayName": "App (service account)", "id": "", "mail": ""} return self._get("/me") + def lookup_user(self, upn: str) -> dict: + """Look up one directory user by UPN or email (requires User.Read.All).""" + return self._get(f"/users/{upn}", {"$select": "id,displayName,mail,userPrincipalName"}) + def list_users(self, top: int = 999) -> list: """List licensed domain users in the tenant (requires User.Read.All). diff --git a/routes/users.py b/routes/users.py index 19f4306..ebd6ed2 100644 --- a/routes/users.py +++ b/routes/users.py @@ -176,7 +176,7 @@ def lookup_user(): if not upn: return jsonify({"error": "upn required"}), 400 try: - data = state.connector._get(f"/users/{upn}", {"$select": "id,displayName,mail,userPrincipalName"}) + data = state.connector.lookup_user(upn) _email = data.get("mail") or data.get("userPrincipalName", upn) return jsonify({ "id": data["id"], diff --git a/scan_engine.py b/scan_engine.py index a56a33f..1bef525 100644 --- a/scan_engine.py +++ b/scan_engine.py @@ -41,6 +41,37 @@ except ImportError: from routes import state as _state +_MANUAL_PREFIX = "manual:" + + +def _resolve_manual_users(conn, user_ids: list[dict]) -> list[dict]: + """Resolve profile accounts saved as "manual:" to their directory id. + + Older profile-editor saves (and saves made while M365 was not connected) + store manual accounts by email only; Graph needs the user id. Accounts that + can't be resolved are reported and skipped — never widened to "all users". + """ + resolved, seen = [], set() + for u in user_ids: + uid = u.get("id", "") + if uid.startswith(_MANUAL_PREFIX): + upn = uid[len(_MANUAL_PREFIX):] + try: + data = conn.lookup_user(upn) + _email = data.get("mail") or data.get("userPrincipalName", upn) + u = {**u, "id": data["id"], + "displayName": _resolve_display_name(data.get("displayName", ""), _email, upn)} + except Exception as e: + logger.warning("[run_scan] manual account %r not found: %s", upn, e) + broadcast("scan_error", {"file": upn, "error": f"Manual account not found in M365: {e}"}) + continue + if u["id"] in seen: + continue + seen.add(u["id"]) + resolved.append(u) + return resolved + + def _get_scan_abort(): return _state._scan_abort @@ -645,6 +676,7 @@ def run_scan(options: dict): me_info.get("displayName", ""), me_info.get("mail") or me_info.get("userPrincipalName", "me"))}] else: + user_ids = _resolve_manual_users(conn, user_ids) sample = user_ids[0] if user_ids else None logger.info("[run_scan] user_ids: %d entries, type=%s, sample=%s", len(user_ids), type(user_ids).__name__, sample) diff --git a/static/js/CLAUDE.md b/static/js/CLAUDE.md index c55ad23..058671e 100644 --- a/static/js/CLAUDE.md +++ b/static/js/CLAUDE.md @@ -81,6 +81,12 @@ 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. Saved IDs not in `S._allUsers` (`manual:`, 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. +- **Manual accounts must be scannable** — they aren't in the `/api/users` directory list, so: + - `getSelectedUsers()` treats a missing `platform` as `'m365'`. Sidebar `addUserManually()` users historically had no platform and were silently excluded from every scan. Do not go back to a strict `u.platform === 'm365'` check. + - The profile editor's "+ Tilføj konto" looks the email up via `/api/users/lookup` and stores `{id: , displayName, email, manual: true}`. It falls back to `"manual:"` only when the lookup fails. Checkboxes with `data-manual` save as objects; everything else saves as plain id strings. + - `_applyProfile()` → `_addProfileManualUsers()` pushes manual accounts into `S._allUsers` (`manual: true`, `platform: 'm365'`). Legacy `manual:` entries are looked up and the pending ID is rewritten to the resolved one. Without this the account can never be ticked in the sidebar, so it's never scanned. + - `_userListFinal()` keys off `S._m365UsersLoaded`, not `S._allUsers.length`, because manual users can be pushed before the directory loads. + - Server side, `scan_engine._resolve_manual_users()` resolves any remaining `manual:` (e.g. from scheduled scans) via `conn.lookup_user()`. It skips and reports failures and never widens to "all users". - **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:` 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. diff --git a/static/js/profiles.js b/static/js/profiles.js index 223c9a9..87e1e94 100644 --- a/static/js/profiles.js +++ b/static/js/profiles.js @@ -215,6 +215,7 @@ function _applyProfile(profile) { } } else if (Array.isArray(profile.user_ids) && profile.user_ids.length) { window._pendingProfileUserIds = profile.user_ids.map(u => u.id || u); + _addProfileManualUsers(profile.user_ids); _applyPendingProfileUsers(); } else if (Array.isArray(profile.user_ids) && profile.user_ids.length === 0) { // Explicitly empty list — deselect everyone so previous sidebar state doesn't persist @@ -229,7 +230,7 @@ function _applyProfile(profile) { // 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); + return S._m365UsersLoaded && (!window._googleConnected || S._googleUsersMerged); } function _applyPendingProfileUsers() { @@ -244,6 +245,54 @@ function _applyPendingProfileUsers() { if (found >= wanted.size || _userListFinal()) window._pendingProfileUserIds = null; } +function _isManualEntry(u) { + return typeof u === 'object' ? !!u.manual : String(u).startsWith('manual:'); +} + +// Manual accounts aren't in the /api/users directory list, so they must be added +// to S._allUsers (as sidebar manual users) or they can never be selected/scanned. +// Legacy "manual:" entries are looked up to get their directory id. +function _addProfileManualUsers(entries) { + entries.filter(_isManualEntry).forEach(async function(u) { + if (typeof u === 'object' && !String(u.id).startsWith('manual:')) { + if (!S._allUsers.some(x => x.id === u.id)) { + S._allUsers.push({id: u.id, displayName: u.displayName || u.email || u.id, + email: u.email || '', userRole: u.userRole || 'other', + platform: 'm365', manual: true, selected: false}); + } + return; // selection is handled by _applyPendingProfileUsers() + } + const legacyId = u.id || u; + try { + const email = legacyId.slice('manual:'.length); + const d = await _lookupM365User(email); + if (!d) { + // Not connected / not found — scheduled scans retry the lookup at scan time + log(t('m365_profile_manual_not_found', 'Manual account not found in M365') + ': ' + email, 'err'); + return; + } + let user = S._allUsers.find(x => x.id === d.id); + if (!user) { + user = {...d, platform: 'm365', manual: true}; + S._allUsers.push(user); + } + user.selected = true; + // Pending apply may re-run later — make it select the resolved id, not the legacy one + const pend = window._pendingProfileUserIds; + if (pend) window._pendingProfileUserIds = pend.map(x => x === legacyId ? d.id : x); + renderAccountList(); + } catch(e) { /* leave for the scan-time fallback */ } + }); +} + +// Returns {id, displayName, email} or null when M365 isn't connected / the user doesn't exist. +async function _lookupM365User(upn) { + const r = await fetch('/api/users/lookup?upn=' + encodeURIComponent(upn)); + if (!r.ok) return null; + const d = await r.json(); + return d && d.id ? d : null; +} + async function saveCurrentAsProfile() { const name = prompt(t('m365_profile_save_prompt', 'Profile name:'), S._activeProfileId @@ -385,12 +434,14 @@ function _openEditorForProfile(profile) { const accountRows = S._allUsers.map(u => { // Only check if the user was explicitly saved — default to unchecked like the main window const checked = isSaved(u) ? 'checked' : ''; + const manualAttrs = u.manual + ? ` data-manual="1" data-name="${_esc(u.displayName || '')}" data-email="${_esc(u.email || '')}"` : ''; const platBadge = u.platform === 'both' ? 'M365+GWS' : (u.platform || 'm365') === 'google' ? 'GWS' : 'M365'; 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 ``; + return ``; }).join('') + _unresolvedAccountRows(savedList); body.innerHTML = ` @@ -492,11 +543,13 @@ function _unresolvedAccountRows(savedList) { const id = u.id || u; if (!id || known.has(id) || seen.has(id)) return ''; seen.add(id); - const isManual = id.startsWith('manual:'); + const isManual = _isManualEntry(u); 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 ``; + const manualAttrs = _isManualEntry(u) && typeof u === 'object' + ? ` data-manual="1" data-name="${_esc(u.displayName || '')}" data-email="${_esc(u.email || '')}"` : ''; + return ``; }).join(''); } @@ -628,18 +681,24 @@ function _pmgmtRoleFilter(role) { _pmgmtFilterAccounts(document.getElementById('pmgmtAcctSearch')?.value || ''); } -function _pmgmtAddManual() { - const email = prompt('E-mail adresse:'); - if (!email || !email.trim()) return; +async function _pmgmtAddManual() { + const email = (prompt('E-mail adresse:') || '').trim(); + if (!email) return; const list = document.getElementById('pmgmtAcctList'); if (!list) return; - const id = 'manual:' + email.trim().toLowerCase(); - if (list.querySelector(`input[data-uid="${id}"]`)) return; // already exists + // Resolve to the directory id so the scan can use it. If M365 isn't reachable + // right now, fall back to "manual:" — the scan engine resolves that later. + let d = null; + try { d = await _lookupM365User(email); } catch(e) { /* offline — use fallback */ } + const id = d ? d.id : 'manual:' + email.toLowerCase(); + const label = d ? (d.displayName || email) : email; + const existing = [...list.querySelectorAll('input[data-uid]')].find(cb => cb.dataset.uid === id); + if (existing) { existing.checked = true; return; } // already listed — just tick it const lbl = document.createElement('label'); lbl.className = 'pmgmt-acct-row'; lbl.dataset.uid = id; lbl.dataset.role = 'other'; - lbl.innerHTML = `${_esc(email.trim())}${_esc(t('m365_profile_acct_manual', 'Manual'))}`; + lbl.innerHTML = `${_esc(label)}${_esc(t('m365_profile_acct_manual', 'Manual'))}`; list.appendChild(lbl); } @@ -674,8 +733,10 @@ async function _pmgmtSaveFullEdit() { const effectiveFileSources = fileRendered ? fileSources : (profile.file_sources || []); const allSources = m365Sources.concat(effectiveGoogleSources).concat(effectiveFileSources); let user_ids = Array.from(document.querySelectorAll('#pmgmtAcctList input[type=checkbox]:checked')) - .map(cb => cb.dataset.uid) - .filter(Boolean); + .filter(cb => cb.dataset.uid) + .map(cb => cb.dataset.manual + ? {id: cb.dataset.uid, displayName: cb.dataset.name || '', email: cb.dataset.email || '', manual: true} + : cb.dataset.uid); // "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 = { @@ -810,6 +871,8 @@ window._pmgmtUse = _pmgmtUse; window._pmgmtOpenEditor = _pmgmtOpenEditor; window._openEditorForProfile = _openEditorForProfile; window._unresolvedAccountRows = _unresolvedAccountRows; +window._addProfileManualUsers = _addProfileManualUsers; +window._lookupM365User = _lookupM365User; window._userListFinal = _userListFinal; window._peSetDate = _peSetDate; window._peSetYear = _peSetYear; diff --git a/static/js/state.js b/static/js/state.js index b3a98ec..57f107d 100644 --- a/static/js/state.js +++ b/static/js/state.js @@ -19,6 +19,7 @@ export const S = { _progressCurrentUser: '', // Users _allUsers: [], + _m365UsersLoaded: false, // true once loadUsers() has fetched the directory _googleUsersMerged: false, // true once _mergeGoogleUsers() has added Google users // Auth _currentAppMode: null, diff --git a/static/js/users.js b/static/js/users.js index af33622..ae76c2d 100644 --- a/static/js/users.js +++ b/static/js/users.js @@ -28,6 +28,7 @@ async function loadUsers() { u.selected = prevSelected.has(u.id) ? prevSelected.get(u.id) : false; }); S._allUsers = [...fetched, ...toAdd]; + S._m365UsersLoaded = true; // Apply deferred "select all" from a profile chosen before users loaded if (window._pendingProfileAllUsers) { S._allUsers.forEach(u => { u.selected = true; }); @@ -421,7 +422,7 @@ async function addUserManually() { const d = await r.json(); if (d.error) { alert('User not found: ' + d.error); return; } if (S._allUsers.find(u => u.id === d.id)) { alert('User already in list.'); return; } - S._allUsers.push({...d, manual: true}); + S._allUsers.push({...d, platform: 'm365', manual: true}); input.value = ''; renderAccountList(S._allUsers.filter(u => !u.manual).length <= 1); } catch(e) { @@ -447,16 +448,18 @@ function selectAllAccounts(checked) { function getSelectedUsers() { // Only return M365 users — Google users are handled separately via selectedGoogleEmails - let selected = S._allUsers.filter(u => u.selected !== false && (u.platform === 'm365' || u.platform === 'both')); + // Manually added users may have no platform — they are M365 lookups, so treat missing as 'm365' + let selected = S._allUsers.filter(u => u.selected !== false && ['m365', 'both'].includes(u.platform || 'm365')); // Respect the active role filter — hidden users must not sneak into the scan // even if they were checked before the filter was applied. if (_activeRoleFilter) { selected = selected.filter(u => (u.userRole || 'other') === _activeRoleFilter); } if (selected.length) { - return selected.map(u => ({ - id: u.id, displayName: u.displayName, userRole: u.userRole || 'other' - })); + // manual/email let a profile saved from this selection re-add manual users on apply + return selected.map(u => Object.assign( + { id: u.id, displayName: u.displayName, userRole: u.userRole || 'other' }, + u.manual ? { manual: true, email: u.email || '' } : {})); } // Fallback to DOM if S._allUsers not yet populated return Array.from(document.querySelectorAll('.account-check:checked')).map(cb => ({ diff --git a/tests/test_manual_users.py b/tests/test_manual_users.py new file mode 100644 index 0000000..94cf94c --- /dev/null +++ b/tests/test_manual_users.py @@ -0,0 +1,104 @@ +""" +Manual accounts in scan profiles. + +The profile editor stores manually added accounts either with their directory +id (looked up via GET /api/users/lookup) or, when M365 wasn't reachable / for +profiles saved by older versions, as "manual:". The scan engine must +resolve the latter to a Graph user id or they are never scanned. +""" +from __future__ import annotations +from unittest.mock import MagicMock + +import pytest + +import scan_engine + + +class _Conn: + def __init__(self, directory: dict[str, dict]): + self.directory = directory + self.lookups: list[str] = [] + + def lookup_user(self, upn: str) -> dict: + self.lookups.append(upn) + if upn not in self.directory: + raise RuntimeError("Request_ResourceNotFound") + return self.directory[upn] + + +@pytest.fixture() +def broadcasts(monkeypatch): + sent = [] + monkeypatch.setattr(scan_engine, "broadcast", lambda ev, data: sent.append((ev, data))) + return sent + + +class TestResolveManualUsers: + def test_manual_email_resolved_to_directory_id(self, broadcasts): + conn = _Conn({"ann@school.dk": {"id": "guid-ann", "displayName": "Ann Andersen", + "mail": "ann@school.dk"}}) + out = scan_engine._resolve_manual_users(conn, [ + {"id": "manual:ann@school.dk", "displayName": "manual:ann@school.dk", "userRole": "other"}, + ]) + assert out == [{"id": "guid-ann", "displayName": "Ann Andersen", "userRole": "other"}] + assert conn.lookups == ["ann@school.dk"] + assert broadcasts == [] + + def test_regular_and_resolved_manual_ids_pass_through_without_lookup(self, broadcasts): + conn = _Conn({}) + users = [{"id": "guid-1", "displayName": "One"}, + {"id": "guid-2", "displayName": "Two", "manual": True, "email": "two@school.dk"}] + assert scan_engine._resolve_manual_users(conn, users) == users + assert conn.lookups == [] + + def test_unknown_manual_account_skipped_and_reported(self, broadcasts): + conn = _Conn({}) + out = scan_engine._resolve_manual_users(conn, [ + {"id": "guid-1", "displayName": "One"}, + {"id": "manual:ghost@school.dk", "displayName": "ghost"}, + ]) + assert out == [{"id": "guid-1", "displayName": "One"}] + assert broadcasts and broadcasts[0][0] == "scan_error" + assert "ghost@school.dk" in broadcasts[0][1]["file"] + + def test_manual_account_already_selected_is_deduplicated(self, broadcasts): + conn = _Conn({"ann@school.dk": {"id": "guid-ann", "displayName": "Ann"}}) + out = scan_engine._resolve_manual_users(conn, [ + {"id": "guid-ann", "displayName": "Ann"}, + {"id": "manual:ann@school.dk", "displayName": "ann"}, + ]) + assert [u["id"] for u in out] == ["guid-ann"] + + def test_all_manual_unresolvable_gives_empty_list_not_all_users(self, broadcasts): + # run_scan resolves inside the non-empty branch, so an empty result must + # stay empty — it must never widen to "scan every tenant user". + conn = _Conn({}) + assert scan_engine._resolve_manual_users( + conn, [{"id": "manual:x@school.dk", "displayName": "x"}]) == [] + + +class TestLookupRoute: + @pytest.fixture() + def client(self): + import gdpr_scanner + gdpr_scanner.app.config["TESTING"] = True + with gdpr_scanner.app.test_client() as c: + yield c + + def test_lookup_uses_connector(self, client, monkeypatch): + from routes import state + conn = MagicMock() + conn.lookup_user.return_value = {"id": "guid-ann", "displayName": "Ann", + "mail": "ann@school.dk"} + monkeypatch.setattr(state, "connector", conn) + r = client.get("/api/users/lookup?upn=ann@school.dk") + assert r.status_code == 200 + assert r.get_json()["id"] == "guid-ann" + conn.lookup_user.assert_called_once_with("ann@school.dk") + + def test_lookup_not_found_is_404(self, client, monkeypatch): + from routes import state + conn = MagicMock() + conn.lookup_user.side_effect = RuntimeError("Request_ResourceNotFound") + monkeypatch.setattr(state, "connector", conn) + assert client.get("/api/users/lookup?upn=ghost@school.dk").status_code == 404 -- 2.47.3