From 10b393687c4510afae9eaa4e0e5030a68795fc6e Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 3 Oct 2026 11:00:48 +0000 Subject: [PATCH] Mitarbeiter-Zugang: Review-Punkte (fail closed, Kunden-Konto bleibt, Probelauf robuster) - removeEmployeeEntry: nur 404 heisst "kein Konto", jeder andere Fehler bricht ab - Mitarbeiter loeschen (Admin-Panel): Konto ohne Mitarbeiter-Label (Kunde mit altem employees-Eintrag) bleibt bestehen, nur der Eintrag geht - Mitarbeiter bearbeiten gibt nie automatisch das Label (der Eintrag kann von einem Kunden stammen); nur Neuanlage bekommt "mitarbeiter" - --rechte: --offen-lassen , unlesbare Listen als Fehler statt Abbruch (--anwenden verweigert dann), --sicherung + docker-cp-Hinweis, Bucket-Einstellung transformations wird mitgeschickt - getEmployeeByEmail entfernt (unbenutzt) - Tests: write/users-unverified, zweiter Lauf idempotent, >100 Dokumente, Loeschen mit Kunden- und Mitarbeiter-Konto Co-Authored-By: Claude Opus 5.5 --- scripts/mitarbeiter-zugang.mjs | 15 ++++-- server/services/appwriteAdmin.js | 9 ---- server/services/employeeAdmin.js | 26 ++++++--- server/services/staffAccessAdmin.js | 40 ++++++++++---- test/staffAccess.test.js | 83 +++++++++++++++++++++++++---- test/support/appwriteMock.js | 6 +++ 6 files changed, 139 insertions(+), 40 deletions(-) diff --git a/scripts/mitarbeiter-zugang.mjs b/scripts/mitarbeiter-zugang.mjs index 70d4d23..a369b82 100644 --- a/scripts/mitarbeiter-zugang.mjs +++ b/scripts/mitarbeiter-zugang.mjs @@ -6,8 +6,8 @@ * docker exec -it -w /app webklar-kundenbereich-server node scripts/mitarbeiter-zugang.mjs --status * ... --label [,...] [--yes] Label "mitarbeiter" vergeben (nur ausdrueckliche Konten) * ... --eintrag-loeschen [--yes] employees-Eintrag eines Kunden loeschen (Konto bleibt) - * ... --rechte Probelauf: welche Rechte sich aendern - * ... --rechte --anwenden [--yes] Rechte umstellen, Sicherung als JSON (Pfad wird ausgegeben) + * ... --rechte [--offen-lassen [,...]] Probelauf: welche Rechte sich aendern (IDs bleiben offen) + * ... --rechte --anwenden [--sicherung ] [--yes] Rechte umstellen, vorher Sicherung als JSON * ... --rechte --zurueck [--yes] Rechte aus einer Sicherung wiederherstellen */ import 'dotenv/config' @@ -99,14 +99,19 @@ async function permissions() { return report(await applyPermissionChanges(changes, { direction: 'before' })) } - const changes = await planPermissionChanges() + const keep = (flag('--offen-lassen') || '').split(',').map((id) => id.trim()).filter(Boolean) + const { changes, errors } = await planPermissionChanges(undefined, { keep }) + for (const e of errors) console.log(`NICHT LESBAR ${e.path}: ${e.error}`) + if (keep.length) console.log(`Bleiben offen: ${keep.join(', ')}`) if (!changes.length) return console.log('Keine offenen Rechte (any/guests/users) gefunden.') printChanges(changes) if (!args.includes('--anwenden')) return console.log(`Probelauf: ${changes.length} Stellen. Mit --anwenden umstellen.`) + if (errors.length) throw new Error('Nicht alles lesbar (siehe oben) - erst den Key-Scope klären, dann erneut.') - const backup = join(tmpdir(), `mitarbeiter-rechte-${new Date().toISOString().replace(/[:.]/g, '-')}.json`) + const backup = flag('--sicherung') || join(tmpdir(), `mitarbeiter-rechte-${new Date().toISOString().replace(/[:.]/g, '-')}.json`) writeFileSync(backup, JSON.stringify(changes, null, 2)) console.log(`Sicherung: ${backup}`) + console.log(` aus dem Container holen: docker cp webklar-kundenbereich-server:${backup} /root/reports/`) if (!(await confirm(`${changes.length} Stellen nur noch für Mitarbeiter freigeben?`))) return console.log('Abgebrochen.') report(await applyPermissionChanges(changes)) } @@ -118,7 +123,7 @@ async function main() { const employeeId = flag('--eintrag-loeschen') if (employeeId) return removeEntry(employeeId) if (args.includes('--rechte')) return permissions() - console.log('Usage: --status | --label [,...] | --eintrag-loeschen | --rechte [--anwenden | --zurueck ] [--yes]') + console.log('Usage: --status | --label [,...] | --eintrag-loeschen | --rechte [--offen-lassen ,...] [--anwenden [--sicherung ] | --zurueck ] [--yes]') } main().catch((err) => { diff --git a/server/services/appwriteAdmin.js b/server/services/appwriteAdmin.js index a237c98..e7c14fb 100644 --- a/server/services/appwriteAdmin.js +++ b/server/services/appwriteAdmin.js @@ -223,15 +223,6 @@ export async function getEmployeeByUserId(userId) { return docs[0] || null } -export async function getEmployeeByEmail(email) { - if (!email) return null - const docs = await listDocuments(config.collections.employees, [ - Query.equal('email', email.trim().toLowerCase()), - Query.limit(1), - ]) - return docs[0] || null -} - export async function getPortalAccessByCustomerId(customerId) { const docs = await listDocuments(config.collections.customerPortalAccess, [ Query.equal('customerId', customerId), diff --git a/server/services/employeeAdmin.js b/server/services/employeeAdmin.js index d53b2eb..596fbfb 100644 --- a/server/services/employeeAdmin.js +++ b/server/services/employeeAdmin.js @@ -14,7 +14,7 @@ import { updateDocument, Query, } from './appwriteAdmin.js' -import { ADMIN_LABEL, STAFF_LABEL, isAdminAccount } from './staffLabels.js' +import { ADMIN_LABEL, STAFF_LABEL, isAdminAccount, isStaffAccount } from './staffLabels.js' function httpError(status, message) { const error = new Error(message) @@ -46,12 +46,13 @@ async function employeeWithUser(employee) { return sanitizeEmployee(employee, user) } -// Jeder Mitarbeiter traegt das Label "mitarbeiter" (Zugang zum Ticketsystem, siehe staffLabels.js); -// isAdmin undefined laesst "admin" unveraendert -async function setStaffLabels(userId, isAdmin) { +// Neue Mitarbeiter bekommen das Label "mitarbeiter" (Zugang zum Ticketsystem, siehe staffLabels.js). Beim +// Bearbeiten nie automatisch: ein employees-Eintrag kann von einem Kunden stammen (frueher selbst anlegbar); +// Zugang fuer Bestandskonten nur per scripts/mitarbeiter-zugang.mjs --label +async function setStaffLabels(userId, { isAdmin, grantStaff = false }) { const user = await getUserById(userId) const labels = new Set(user.labels || []) - labels.add(STAFF_LABEL) + if (grantStaff) labels.add(STAFF_LABEL) if (isAdmin === true) labels.add(ADMIN_LABEL) if (isAdmin === false) labels.delete(ADMIN_LABEL) await updateAppwriteUserLabels(userId, [...labels]) @@ -81,7 +82,7 @@ export async function createEmployeeWithLogin(payload) { const user = await createAppwriteUser({ email, password, name: displayName }) let employee try { - await setStaffLabels(user.$id, isAdmin) + await setStaffLabels(user.$id, { isAdmin, grantStaff: true }) employee = await createDocument(config.collections.employees, { userId: user.$id, displayName, @@ -135,7 +136,9 @@ export async function updateEmployeeWithLogin(employeeId, payload, actorUserId) if (password) { await updateAppwriteUserPassword(employee.userId, password) } - await setStaffLabels(employee.userId, payload?.isAdmin === undefined ? undefined : Boolean(payload.isAdmin)) + if (payload?.isAdmin !== undefined) { + await setStaffLabels(employee.userId, { isAdmin: Boolean(payload.isAdmin) }) + } const updated = await updateDocument(config.collections.employees, employeeId, { displayName, @@ -153,7 +156,16 @@ export async function deleteEmployeeWithLogin(employeeId, actorUserId) { throw httpError(400, 'Der eigene angemeldete Account kann nicht gelöscht werden') } + // Konto ohne Mitarbeiter-Label (z. B. Kunde mit altem employees-Eintrag): nur den Eintrag loeschen, das Konto ist + // sein Portal-Login. Konto nicht lesbar -> abbrechen statt zu raten + const user = employee.userId + ? await getUserById(employee.userId).catch((err) => { + if (err.status === 404) return null + throw err + }) + : null await deleteDocument(config.collections.employees, employeeId) + if (!isStaffAccount(user)) return { success: true, accountKept: Boolean(user) } await deleteAppwriteUser(employee.userId).catch((error) => { error.message = `Mitarbeiter entfernt, Login konnte nicht gelöscht werden: ${error.message}` throw error diff --git a/server/services/staffAccessAdmin.js b/server/services/staffAccessAdmin.js index c24cc74..dd30036 100644 --- a/server/services/staffAccessAdmin.js +++ b/server/services/staffAccessAdmin.js @@ -92,7 +92,13 @@ export async function grantStaffLabel(userId, request = appwriteAdminRequest) { export async function removeEmployeeEntry(employeeId, request = appwriteAdminRequest) { const path = `/databases/${config.appwrite.databaseId}/collections/${config.collections.employees}/documents/${employeeId}` const employee = await request(path) - const user = employee.userId ? await request(`/users/${employee.userId}`).catch(() => null) : null + // Nur "Konto gibt es nicht" (404) zaehlt als kein Mitarbeiter; jeder andere Fehler bricht ab (fail closed) + const user = employee.userId + ? await request(`/users/${employee.userId}`).catch((err) => { + if (err.status === 404) return null + throw err + }) + : null if (isStaffAccount(user)) throw new Error(`${employee.displayName || employeeId} hat das Mitarbeiter-Label - Eintrag bleibt.`) await request(path, { method: 'DELETE' }) return { employeeId, name: employee.displayName || '', userId: employee.userId || '' } @@ -100,18 +106,31 @@ export async function removeEmployeeEntry(employeeId, request = appwriteAdminReq /** * Alle Stellen mit offenen Rechten: Collections und Buckets, bei Dokument- bzw. Datei-Sicherheit auch einzelne - * Dokumente und Dateien. Jede Aenderung traegt before/after, damit sie sich zuruecknehmen laesst. + * Dokumente und Dateien. Jede Aenderung traegt before/after, damit sie sich zuruecknehmen laesst. keep = IDs von + * Collections/Buckets, die bewusst offen bleiben (samt ihrer Dokumente/Dateien). Eine Liste, die sich nicht lesen + * laesst (z. B. fehlender Key-Scope), landet in errors statt den ganzen Lauf abzubrechen. */ -export async function planPermissionChanges(request = appwriteAdminRequest) { +export async function planPermissionChanges(request = appwriteAdminRequest, { keep = [] } = {}) { const changes = [] + const errors = [] + const kept = new Set(keep) const add = (change, before) => { const after = staffOnlyPermissions(before) if (!samePermissions(before, after)) changes.push({ ...change, before, after }) } + const tryList = async (path, key) => { + try { + return await listAll(request, path, key) + } catch (err) { + errors.push({ path, error: err.message }) + return [] + } + } - for (const database of await listAll(request, '/databases', 'databases')) { + for (const database of await tryList('/databases', 'databases')) { const collectionsPath = `/databases/${database.$id}/collections` - for (const collection of await listAll(request, collectionsPath, 'collections')) { + for (const collection of await tryList(collectionsPath, 'collections')) { + if (kept.has(collection.$id)) continue add( { kind: 'collection', @@ -123,13 +142,14 @@ export async function planPermissionChanges(request = appwriteAdminRequest) { collection.$permissions || [] ) if (!collection.documentSecurity) continue - for (const document of await listAll(request, `${collectionsPath}/${collection.$id}/documents`, 'documents')) { + for (const document of await tryList(`${collectionsPath}/${collection.$id}/documents`, 'documents')) { add({ kind: 'document', databaseId: database.$id, collectionId: collection.$id, id: document.$id }, document.$permissions || []) } } } - for (const bucket of await listAll(request, '/storage/buckets', 'buckets')) { + for (const bucket of await tryList('/storage/buckets', 'buckets')) { + if (kept.has(bucket.$id)) continue add( { kind: 'bucket', @@ -144,16 +164,18 @@ export async function planPermissionChanges(request = appwriteAdminRequest) { compression: bucket.compression, encryption: bucket.encryption, antivirus: bucket.antivirus, + // neuere Appwrite-Versionen: Bild-Transformationen; mitschicken, sonst setzt das PUT den Standard + ...(bucket.transformations === undefined ? {} : { transformations: bucket.transformations }), }, }, bucket.$permissions || [] ) if (!bucket.fileSecurity) continue - for (const file of await listAll(request, `/storage/buckets/${bucket.$id}/files`, 'files')) { + for (const file of await tryList(`/storage/buckets/${bucket.$id}/files`, 'files')) { add({ kind: 'file', bucketId: bucket.$id, id: file.$id }, file.$permissions || []) } } - return changes + return { changes, errors } } function writeChange(change, permissions, request) { diff --git a/test/staffAccess.test.js b/test/staffAccess.test.js index 328d38c..4c668a4 100644 --- a/test/staffAccess.test.js +++ b/test/staffAccess.test.js @@ -17,7 +17,7 @@ const { staffOnlyPermissions, } = await import('../server/services/staffAccessAdmin.js') const { isStaffAccount, isAdminAccount } = await import('../server/services/staffLabels.js') -const { createEmployeeWithLogin, updateEmployeeWithLogin } = await import('../server/services/employeeAdmin.js') +const { createEmployeeWithLogin, deleteEmployeeWithLogin, updateEmployeeWithLogin } = await import('../server/services/employeeAdmin.js') after(() => Promise.all([aw.close(), st.close()])) const STAFF = ['label:mitarbeiter', 'label:admin'] @@ -44,6 +44,15 @@ test('staffOnlyPermissions: any/guests/users -> Mitarbeiter-Labels, andere Rolle ) assert.deepEqual(staffOnlyPermissions(['read("label:mitarbeiter")', 'read("team:x")']), ['read("label:mitarbeiter")', 'read("team:x")']) assert.deepEqual(staffOnlyPermissions([]), []) + assert.deepEqual(staffOnlyPermissions(['write("users")', 'read("users/unverified")']), [ + ...STAFF.map((r) => `write("${r}")`), + ...STAFF.map((r) => `read("${r}")`), + ]) +}) + +test('staffOnlyPermissions: zweiter Lauf aendert nichts mehr', () => { + const once = staffOnlyPermissions(['read("users")', 'create("any")', 'delete("user:u1")']) + assert.deepEqual(staffOnlyPermissions(once), once) }) // Nachbau der Appwrite-Verwaltungs-API fuer Rechte (Datenbanken, Collections, Dokumente, Buckets, Dateien) @@ -62,7 +71,7 @@ function fakeAdminApi() { allowedFileExtensions: ['pdf'], compression: 'none', encryption: true, antivirus: true, $permissions: ['read("users")', 'create("users")'], } const writes = [] - const request = async (path, { method = 'GET', body } = {}) => { + const request = async (path, { method = 'GET', body, queries = [] } = {}) => { if (method !== 'GET') { if (path.endsWith('/collections/workorders') && body.name === undefined) throw new Error('name fehlt') if (path.startsWith('/storage/') && method === 'PUT' && path.endsWith('/broken')) throw new Error('missing scope (buckets.write)') @@ -71,16 +80,22 @@ function fakeAdminApi() { } if (path === '/databases') return { databases: [{ $id: 'woms-database' }] } if (path === '/databases/woms-database/collections') return { collections: Object.values(collections) } - if (path === '/databases/woms-database/collections/notes/documents') return { documents } + if (path === '/databases/woms-database/collections/notes/documents') { + // Seiten zu 100 per cursorAfter wie Appwrite + const cursor = queries.map((q) => (typeof q === 'string' ? JSON.parse(q) : q)).find((q) => q.method === 'cursorAfter')?.values[0] + const start = cursor ? documents.findIndex((d) => d.$id === cursor) + 1 : 0 + return { documents: documents.slice(start, start + 100) } + } if (path === '/storage/buckets') return { buckets: [bucket, { ...bucket, $id: 'broken', name: 'Kaputt' }] } throw new Error(`unerwartet ${path}`) } - return { request, writes } + return { request, writes, documents } } test('planPermissionChanges: offene Collections, Dokumente und Buckets, geschlossene nicht', async () => { const api = fakeAdminApi() - const changes = await planPermissionChanges(api.request) + const { changes, errors } = await planPermissionChanges(api.request) + assert.deepEqual(errors, []) assert.deepEqual(changes.map((c) => `${c.kind}:${c.id}`), ['collection:workorders', 'document:n2', 'bucket:legal-files', 'bucket:broken']) const tickets = changes[0] assert.deepEqual(tickets.before, ['read("users")', 'update("users")']) @@ -88,9 +103,28 @@ test('planPermissionChanges: offene Collections, Dokumente und Buckets, geschlos assert.deepEqual(tickets.settings, { name: 'Work Orders', documentSecurity: false, enabled: true }) }) +test('planPermissionChanges: liest ueber 100 Dokumente seitenweise, offen-lassen und unlesbare Listen', async () => { + const api = fakeAdminApi() + const many = Array.from({ length: 230 }, (_, i) => ({ $id: `d${String(i).padStart(3, '0')}`, $permissions: ['read("users")'] })) + api.documents.splice(0, api.documents.length, ...many) + const all = await planPermissionChanges(api.request) + assert.equal(all.changes.filter((c) => c.kind === 'document').length, 230) + + const kept = await planPermissionChanges(api.request, { keep: ['notes', 'legal-files'] }) + assert.deepEqual(kept.changes.map((c) => `${c.kind}:${c.id}`), ['collection:workorders', 'bucket:broken']) + + const failing = async (path, options) => { + if (path === '/storage/buckets') throw new Error('missing scope (buckets.read)') + return api.request(path, options) + } + const partial = await planPermissionChanges(failing) + assert.deepEqual(partial.errors, [{ path: '/storage/buckets', error: 'missing scope (buckets.read)' }]) + assert.ok(partial.changes.some((c) => c.id === 'workorders')) +}) + test('applyPermissionChanges: schreibt mit allen Einstellungen, meldet Fehler je Stelle, Ruecknahme schreibt before', async () => { const api = fakeAdminApi() - const changes = await planPermissionChanges(api.request) + const { changes } = await planPermissionChanges(api.request) const results = await applyPermissionChanges(changes, { request: api.request }) assert.deepEqual(results.map((r) => r.ok), [true, true, true, false]) assert.match(results[3].error, /buckets\.write/) @@ -155,6 +189,33 @@ test('removeEmployeeEntry: loescht nur den Eintrag eines Kunden, nie das Konto; assert.equal(aw.docs('employees').has('e_m2'), true) }) +test('removeEmployeeEntry: Konto-Abruf gestoert -> Abbruch, Eintrag bleibt (fail closed)', async () => { + aw.seed('employees', { $id: 'e_m3', userId: 'u_m2', displayName: 'M2' }) + aw.failUserLookups(true) + try { + await assert.rejects(removeEmployeeEntry('e_m3')) + } finally { + aw.failUserLookups(false) + } + assert.equal(aw.docs('employees').has('e_m3'), true) +}) + +test('Mitarbeiter loeschen (Admin-Panel): Eintrag eines Kunden weg, sein Konto bleibt; Mitarbeiter-Konto wird geloescht', async () => { + aw.addUser({ $id: 'u_k3', email: 'k3@example.invalid', name: 'K3' }) + aw.seed('employees', { $id: 'e_k3', userId: 'u_k3', displayName: 'K3' }) + const res = await deleteEmployeeWithLogin('e_k3', 'u_kenso') + assert.equal(res.accountKept, true) + assert.equal(aw.docs('employees').has('e_k3'), false) + assert.ok(aw.user('u_k3')) + assert.ok(!aw.userCalls().includes('delete:u_k3')) + + aw.addUser({ $id: 'u_m4', email: 'm4@webklar.com', name: 'M4', labels: ['mitarbeiter'] }) + aw.seed('employees', { $id: 'e_m4', userId: 'u_m4', displayName: 'M4' }) + await deleteEmployeeWithLogin('e_m4', 'u_kenso') + assert.equal(aw.user('u_m4'), null) + assert.ok(aw.userCalls().includes('delete:u_m4')) +}) + test('Mitarbeiter anlegen: Label mitarbeiter (+ admin), Bearbeiten ergaenzt es und laesst admin ohne Angabe stehen', async () => { const { employee } = await createEmployeeWithLogin({ displayName: 'Neu', email: 'neu@webklar.com', password: 'neu-neu-neu', isAdmin: false }) assert.deepEqual(aw.user(employee.userId).labels, ['mitarbeiter']) @@ -163,11 +224,13 @@ test('Mitarbeiter anlegen: Label mitarbeiter (+ admin), Bearbeiten ergaenzt es u assert.deepEqual(aw.user(chef.userId).labels.sort(), ['admin', 'mitarbeiter']) assert.equal(chef.isAdmin, true) - // Altbestand ohne Label: jede Bearbeitung holt es nach, admin bleibt ohne Angabe unveraendert - aw.addUser({ $id: 'u_alt', email: 'alt@webklar.com', name: 'Alt', labels: ['admin'] }) + // Bearbeiten gibt nie automatisch Zugang (der Eintrag kann von einem Kunden stammen); admin nur auf Angabe + aw.addUser({ $id: 'u_alt', email: 'alt@webklar.com', name: 'Alt', labels: [] }) aw.seed('employees', { $id: 'e_alt', userId: 'u_alt', displayName: 'Alt', email: 'alt@webklar.com' }) await updateEmployeeWithLogin('e_alt', { shortcode: 'ALT' }, 'u_kenso') - assert.deepEqual(aw.user('u_alt').labels.sort(), ['admin', 'mitarbeiter']) + assert.deepEqual(aw.user('u_alt').labels, []) + await updateEmployeeWithLogin('e_alt', { isAdmin: true }, 'u_kenso') + assert.deepEqual(aw.user('u_alt').labels, ['admin']) await updateEmployeeWithLogin('e_alt', { isAdmin: false }, 'u_kenso') - assert.deepEqual(aw.user('u_alt').labels, ['mitarbeiter']) + assert.deepEqual(aw.user('u_alt').labels, []) }) diff --git a/test/support/appwriteMock.js b/test/support/appwriteMock.js index 2ca945b..4d102c7 100644 --- a/test/support/appwriteMock.js +++ b/test/support/appwriteMock.js @@ -236,6 +236,12 @@ export async function startAppwriteMock({ schema = {}, datetimes = {}, defaultLi userCalls.push(`create:${userId}`) return [201, publicUser(users.get(userId))] } + const deleteUserPath = url.pathname.match(/^\/v1\/users\/([^/]+)$/) + if (deleteUserPath && req.method === 'DELETE') { + if (!users.delete(deleteUserPath[1])) return [404, { message: 'User not found', code: 404, type: 'user_not_found' }] + userCalls.push(`delete:${deleteUserPath[1]}`) + return [204, null] + } const labelsPath = url.pathname.match(/^\/v1\/users\/([^/]+)\/labels$/) if (labelsPath && req.method === 'PUT') { const user = users.get(labelsPath[1])