Merge pull request #115 from theta42/release/1.6.3

Release 1.6.3

CHANGELOG.md

@@ -4,6 +4,22 @@ All notable changes to this project are documented here. Format loosely
 follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/); versions
 correspond to git tags (`vX.Y.Z`) and `nodejs/package.json`'s `version`.
 
+## [1.6.3] - 2026-07-28
+
+### Fixed
+- **Group membership changes (`PUT`/`DELETE /api/group/:group/:uid`) didn't invalidate the User cache**, so `isServiceAccount` (and anything else derived from `memberOf`) could stay stale for up to 5 minutes after a change. This is what caused a real "lost user" report — the account had landed in `app_sso_service_account` (which `users.ejs`'s People tab filters out entirely) and looked exactly like data loss, though nothing was ever deleted.
+
+### Added
+- **A confirmation before adding anyone to `app_sso_service_account`** via the Groups page — that group's whole purpose is to hide an account from the People tab, and there was no guardrail against doing that to a real person by mistake (which is how the bug above happened). Every other group's add-member flow is unchanged.
+
+## [1.6.2] - 2026-07-28
+
+### Fixed
+- **`DELETE /api/oauth/client/:id` 500'd** (`client.remove is not a function`) — `OAuthClient` wraps `@simpleworkjs/orm`'s `Resource` model, whose instance delete method is `.delete()`, not `.remove()`. The Directory Management UI was unaffected (its own delete routes already used `.delete()` correctly); only this legacy/raw API endpoint was broken. Found live against a real deployment's SSO API.
+
+### Added
+- **Regression tests**: PUT/DELETE on `/api/oauth/client/:id` now verify persistence with a follow-up GET rather than trusting the mutating response alone (this is what would have caught the bug above). A static check across all views/client-side scripts fails CI if any native `alert()`/`confirm()`/`prompt()` call appears — these block all further browser events on the page and were fully removed in 1.6.1.
+
 ## [1.6.1] - 2026-07-27
 
 ### Fixed

nodejs/package.json

@@ -1,6 +1,6 @@
 {
   "name": "t42-sso-manager",
-  "version": "1.6.1",
+  "version": "1.6.3",
   "description": "A very simple LDAP management and SSO system",
   "author": [
     {

nodejs/routes/group.js

@@ -82,8 +82,13 @@ router.put('/:group/:uid', async function(req, res, next){
 
 		var group = await Group.get(req.params.group);
 		var user = await User.get(req.params.uid);
+		const results = await group.addMember(user);
+		// Group membership feeds directly into cached-User-derived state
+		// (isServiceAccount, isAdmin, group-gated nav/UI) -- without this,
+		// a membership change here is invisible for up to the cache's TTL.
+		User.clearCache();
 		return res.json({
-			results: await group.addMember(user),
+			results,
 			message: `Added user ${req.params.uid} to ${req.params.group} group.`
 		});
 	}catch(error){
@@ -98,8 +103,10 @@ router.delete('/:group/:uid', async function(req, res, next){
 
 		var group = await Group.get(req.params.group);
 		var user = await User.get(req.params.uid);
+		const results = await group.removeMember(user);
+		User.clearCache();
 		return res.json({
-			results: await group.removeMember(user),
+			results,
 			message: `Removed user ${req.params.uid} from ${req.params.group} group.`
 		});
 	}catch(error){

nodejs/routes/oauth_client.js

@@ -97,7 +97,7 @@ router.delete('/:client_id', async function(req, res, next) {
 		await permission.byGroup(req.user, [ADMIN_GROUP]);
 
 		const client = await OAuthClient.get(req.params.client_id);
-		await client.remove();
+		await client.delete();
 
 		return res.json({
 			client_id: req.params.client_id,

nodejs/tests/group.test.js

@@ -151,6 +151,32 @@ describe('Groups — member management', () => {
 		const members = Array.isArray(group.member) ? group.member : [group.member];
 		expect(members.some(dn => dn && dn.includes(MEMBER_UID))).toBe(false);
 	});
+
+	// Regression: adding/removing a member here didn't clear User's LRU
+	// cache (ttl 5 minutes), so isServiceAccount -- derived from
+	// app_sso_service_account membership at GET /api/user/:uid time -- could
+	// stay wrong for up to 5 minutes after the group change. In production
+	// this hid a real person's account from the Users page's "People" tab
+	// (it filters out anything with isServiceAccount) for however long the
+	// stale cache entry lived, which looked exactly like the account had
+	// vanished.
+	test('PUT app_sso_service_account/:uid immediately flips isServiceAccount (no stale cache)', async () => {
+		const added = await request(app)
+			.put(`/api/group/app_sso_service_account/${MEMBER_UID}`)
+			.set('auth-token', token);
+		expect(added.status).toBe(200);
+
+		const afterAdd = await request(app).get(`/api/user/${MEMBER_UID}`).set('auth-token', token);
+		expect(afterAdd.body.results.isServiceAccount).toBeTruthy();
+
+		const removed = await request(app)
+			.delete(`/api/group/app_sso_service_account/${MEMBER_UID}`)
+			.set('auth-token', token);
+		expect(removed.status).toBe(200);
+
+		const afterRemove = await request(app).get(`/api/user/${MEMBER_UID}`).set('auth-token', token);
+		expect(afterRemove.body.results.isServiceAccount).toBeFalsy();
+	});
 });
 
 describe('Groups — owner management', () => {

nodejs/tests/no_native_dialogs.test.js

@@ -0,0 +1,45 @@
+'use strict';
+
+// Regression guard: native alert()/confirm()/prompt() calls block all further
+// browser events on the page (found live, mid browser-automation testing, on
+// directory.ejs's "Rotate Client Secret" — it froze the tab entirely) and are
+// visually inconsistent with the rest of the UI. Every call site was removed
+// in favor of app.messages.action/confirm/toast and app.modal.open; this test
+// keeps it that way.
+
+const fs = require('fs');
+const path = require('path');
+
+const ROOTS = ['views', 'public/js', 'public/lib/js'].map((d) => path.join(__dirname, '..', d));
+
+// Matches a bare alert(/confirm(/prompt( call, but not app.messages.*,
+// app.modal.*, or identifiers merely containing these words (e.g.
+// "confirmation", ".confirmed").
+const NATIVE_DIALOG_RE = /(^|[^.\w$])(alert|confirm|prompt)\s*\(/g;
+
+function walk(dir) {
+	let files = [];
+	if (!fs.existsSync(dir)) return files;
+	for (const entry of fs.readdirSync(dir, { withFileTypes: true })) {
+		const full = path.join(dir, entry.name);
+		if (entry.isDirectory()) files = files.concat(walk(full));
+		else if (/\.(ejs|js)$/.test(entry.name)) files.push(full);
+	}
+	return files;
+}
+
+test('no view or client-side script calls native alert()/confirm()/prompt()', () => {
+	const offenders = [];
+	for (const root of ROOTS) {
+		for (const file of walk(root)) {
+			const src = fs.readFileSync(file, 'utf8');
+			let m;
+			NATIVE_DIALOG_RE.lastIndex = 0;
+			while ((m = NATIVE_DIALOG_RE.exec(src))) {
+				const line = src.slice(0, m.index).split('\n').length;
+				offenders.push(`${path.relative(path.join(__dirname, '..'), file)}:${line} — ${m[2]}(`);
+			}
+		}
+	}
+	expect(offenders).toEqual([]);
+});

nodejs/tests/oauth.test.js

@@ -69,6 +69,51 @@ describe('OAuth client management API — /api/oauth/client', () => {
 		expect(res.body.results).not.toHaveProperty('client_secret_hash');
 	});
 
+	test('PUT persists — a changed name survives a fresh GET', async () => {
+		const created = await request(app)
+			.post('/api/oauth/client/')
+			.set('auth-token', token)
+			.send({ name: 'put-persist-test', redirect_uris: REDIRECT_URI });
+		expect(created.status).toBe(200);
+		const id = created.body.results.client_id;
+
+		const updated = await request(app)
+			.put(`/api/oauth/client/${id}`)
+			.set('auth-token', token)
+			.send({ name: 'put-persist-test-renamed' });
+		expect(updated.status).toBe(200);
+		expect(updated.body.results.name).toBe('put-persist-test-renamed');
+
+		const fetched = await request(app).get(`/api/oauth/client/${id}`).set('auth-token', token);
+		expect(fetched.status).toBe(200);
+		expect(fetched.body.results.name).toBe('put-persist-test-renamed');
+
+		await request(app).delete(`/api/oauth/client/${id}`).set('auth-token', token);
+	});
+
+	// Regression: this route called client.remove(), but OAuthClient wraps
+	// @simpleworkjs/orm's Resource model, whose instance method is .delete()
+	// — .remove() doesn't exist on it (unlike the model-redis Tables
+	// elsewhere in this app, e.g. api_token.js, which really do have
+	// .remove()). The route's try/catch turned the resulting TypeError into
+	// a plain 500 JSON response rather than a thrown exception, so every
+	// prior DELETE call in this file's cleanup hooks silently "succeeded"
+	// from Jest's point of view while leaving the client un-deleted.
+	test('DELETE persists — the client is actually gone, not just a 200', async () => {
+		const created = await request(app)
+			.post('/api/oauth/client/')
+			.set('auth-token', token)
+			.send({ name: 'delete-persist-test', redirect_uris: REDIRECT_URI });
+		expect(created.status).toBe(200);
+		const id = created.body.results.client_id;
+
+		const deleted = await request(app).delete(`/api/oauth/client/${id}`).set('auth-token', token);
+		expect(deleted.status).toBe(200);
+
+		const fetched = await request(app).get(`/api/oauth/client/${id}`).set('auth-token', token);
+		expect(fetched.status).toBe(404);
+	});
+
 	test('list then rotate a client by its returned client_id (the bootstrap path)', async () => {
 		// Reproduces exactly what the theta-env bootstrap does: create, list,
 		// find by name, rotate by the client_id from the list response. Uses a
@@ -90,7 +135,11 @@ describe('OAuth client management API — /api/oauth/client', () => {
 		expect(rotated.status).toBe(200);
 		expect(rotated.body.client_secret).toBeTruthy();
 
-		await request(app).delete(`/api/oauth/client/${found.client_id}`).set('auth-token', token);
+		const deleted = await request(app).delete(`/api/oauth/client/${found.client_id}`).set('auth-token', token);
+		expect(deleted.status).toBe(200);
+
+		const afterDelete = await request(app).get(`/api/oauth/client/${found.client_id}`).set('auth-token', token);
+		expect(afterDelete.status).toBe(404);
 	});
 
 	test('GET /:id unknown id returns 404, not 500', async () => {

nodejs/views/groups.ejs

@@ -32,6 +32,33 @@
 		return value;
 	}
 
+	// app_sso_service_account is a marker group: membership hides an account
+	// from the Users page's People tab entirely (see users.ejs), which is
+	// exactly right for a non-person account but has silently made a real
+	// person's account look "gone" before (nothing else about it changes).
+	// Everywhere else in this dropdown just fires the PUT directly; only
+	// this one group gets a confirmation first.
+	function addMemberClick(event, groupCN, uid, el){
+		event.preventDefault();
+		const $el = $(el);
+		(async function(){
+			if (groupCN === 'app_sso_service_account') {
+				const ok = await app.messages.confirm(
+					`Mark "${uid}" as a service account? This hides them from the Users page's People tab (Service Accounts tab only) — only do this for a non-person account.`,
+					$el.closest('.card'), 'warning'
+				);
+				if (!ok) return;
+			}
+			try {
+				const data = await app.api.put(`group/${groupCN}/${uid}`, {});
+				await addedUser(data.message, groupCN, uid, $el);
+			} catch(e) {
+				app.messages.action(e.message || 'Failed to add member', $el.closest('.card'), 'danger');
+			}
+		})();
+		return false;
+	}
+
 	async function addedUser(message, group, user, $form){
 		let data = await app.group.get(group);
 		$.scope.groupCard.update('cn', group, processGroup(data.results));
@@ -214,7 +241,7 @@
 								</button>
 								<div class="dropdown-menu shadow-lg" aria-labelledby="group_add_member">
 								{{ #toAdd }}{{#.}}
-									<a class="dropdown-item" action="group/{{groupCN}}/{{uid}}" method="put" onclick="formAJAX(this)" evalAJAX="addedUser(data.message, '{{groupCN}}', '{{uid}}', $form);">
+									<a class="dropdown-item" href="#" onclick="return addMemberClick(event, '{{groupCN}}', '{{uid}}', this);">
 										<i class="fa-solid fa-user"></i> {{uid}}
 									</a>
 								{{/.}}{{ /toAdd }}