fix(review): surface auth-expiry, harden add-batch rollback, add audit log
Follow-up to the ce-code-review deferred items on the photo editor: - Photo editor fetches now REJECT with a status-bearing error (apiSend) instead of the boolean apiOk that swallowed the HTTP code. Reorder, delete and add paths tell a lapsed login (401/403) apart from a generic failure and prompt the owner to sign in again rather than "try again". - Add-batch rollback: the per-file cleanup DELETEs no longer swallow individual failures. If any rollback DELETE doesn't land (a stray stock-named file could steal the lexicographic cover slot), the owner is told cleanup was incomplete and to reload — instead of a false "rolled back cleanly". - entry-actions delete + reorder routes now emit an owner-attributed audit log line, so a destructive mutation is traceable. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
@@ -63,6 +63,9 @@ class EntryActionsApiController extends AbstractApiController
|
|||||||
|
|
||||||
Folder::delete($path);
|
Folder::delete($path);
|
||||||
$this->grav['cache']->deleteAll();
|
$this->grav['cache']->deleteAll();
|
||||||
|
// Audit trail: entry deletion is destructive and owner-only — record who
|
||||||
|
// did it and to what, so an unexpected disappearance is traceable.
|
||||||
|
$this->grav['log']->info(sprintf('entry-actions: owner "%s" deleted entry "%s"', $user->username, $slug));
|
||||||
|
|
||||||
return ApiResponse::noContent();
|
return ApiResponse::noContent();
|
||||||
}
|
}
|
||||||
@@ -124,6 +127,9 @@ class EntryActionsApiController extends AbstractApiController
|
|||||||
|
|
||||||
PhotoRenumberer::renumber($path, $names);
|
PhotoRenumberer::renumber($path, $names);
|
||||||
$this->grav['cache']->deleteAll();
|
$this->grav['cache']->deleteAll();
|
||||||
|
// Audit trail: mirror deleteEntry — record the owner mutating an entry's
|
||||||
|
// photo order (and how many files the manifest listed).
|
||||||
|
$this->grav['log']->info(sprintf('entry-actions: owner "%s" reordered %d photo(s) for entry "%s"', $user->username, count($names), $slug));
|
||||||
|
|
||||||
return ApiResponse::noContent();
|
return ApiResponse::noContent();
|
||||||
}
|
}
|
||||||
|
|||||||
File diff suppressed because one or more lines are too long
@@ -870,10 +870,28 @@ function initPhotoEditor(route) {
|
|||||||
if (sortable) sortable.option('disabled', b);
|
if (sortable) sortable.option('disabled', b);
|
||||||
}
|
}
|
||||||
|
|
||||||
// Boolean-ok fetch (204 counts as ok for DELETE). Cookies auto-included.
|
// Mutation fetch that RESOLVES on success and REJECTS with a status-bearing
|
||||||
function apiOk(url, opts) {
|
// Error otherwise, so callers can tell an expired login (401/403) apart from a
|
||||||
return fetch(url, Object.assign({ credentials: 'include' }, opts))
|
// generic failure. `okStatuses` lists extra codes to accept as success (e.g.
|
||||||
.then(function (r) { return r.ok || r.status === 204; });
|
// 204 no-content, or 404 already-gone for an idempotent DELETE). Cookies
|
||||||
|
// auto-included. (Superseded the old boolean apiOk, which swallowed the code.)
|
||||||
|
function apiSend(url, opts, okStatuses) {
|
||||||
|
return fetch(url, Object.assign({ credentials: 'include' }, opts)).then(function (r) {
|
||||||
|
if (r.ok || (okStatuses && okStatuses.indexOf(r.status) !== -1)) return r;
|
||||||
|
var e = new Error('HTTP ' + r.status);
|
||||||
|
e.status = r.status;
|
||||||
|
throw e;
|
||||||
|
});
|
||||||
|
}
|
||||||
|
|
||||||
|
// Turn a failed apiSend/fetch into owner-facing copy. A 401/403 almost always
|
||||||
|
// means the login session lapsed mid-edit — say so, because a plain "try
|
||||||
|
// again" wouldn't help until they sign back in.
|
||||||
|
function editErrorMsg(err, fallback) {
|
||||||
|
var s = err && err.status;
|
||||||
|
return (s === 401 || s === 403)
|
||||||
|
? 'Your login session expired — sign in again, then retry.'
|
||||||
|
: fallback;
|
||||||
}
|
}
|
||||||
|
|
||||||
function mediaList() {
|
function mediaList() {
|
||||||
@@ -888,11 +906,11 @@ function initPhotoEditor(route) {
|
|||||||
}
|
}
|
||||||
|
|
||||||
function reorder(order) {
|
function reorder(order) {
|
||||||
return apiOk('/api/v1/entry/' + encodeURIComponent(slug) + '/photos/order', {
|
return apiSend('/api/v1/entry/' + encodeURIComponent(slug) + '/photos/order', {
|
||||||
method: 'POST',
|
method: 'POST',
|
||||||
headers: { 'Content-Type': 'application/json', Accept: 'application/json' },
|
headers: { 'Content-Type': 'application/json', Accept: 'application/json' },
|
||||||
body: JSON.stringify({ order: order })
|
body: JSON.stringify({ order: order })
|
||||||
}).then(function (ok) { if (!ok) throw new Error('reorder failed'); });
|
});
|
||||||
}
|
}
|
||||||
|
|
||||||
function render(list) {
|
function render(list) {
|
||||||
@@ -959,8 +977,8 @@ function initPhotoEditor(route) {
|
|||||||
function (list) { setStatus(''); render(list); },
|
function (list) { setStatus(''); render(list); },
|
||||||
function () { setStatus(''); render(next); } // saved; DOM already shows it
|
function () { setStatus(''); render(next); } // saved; DOM already shows it
|
||||||
);
|
);
|
||||||
}, function () {
|
}, function (err) {
|
||||||
setStatus('Couldn’t save the new order — reverted. Try again.', true);
|
setStatus(editErrorMsg(err, 'Couldn’t save the new order — reverted. Try again.'), true);
|
||||||
render(lastGood); // revert the SortableJS move to last-known-good
|
render(lastGood); // revert the SortableJS move to last-known-good
|
||||||
}).then(function () { setBusy(false); });
|
}).then(function () { setBusy(false); });
|
||||||
}
|
}
|
||||||
@@ -994,9 +1012,8 @@ function initPhotoEditor(route) {
|
|||||||
setStatus('Deleting…');
|
setStatus('Deleting…');
|
||||||
// A 404 means the file is already gone — treat it as success so retrying
|
// A 404 means the file is already gone — treat it as success so retrying
|
||||||
// a ghost cell converges instead of looping on "couldn't delete".
|
// a ghost cell converges instead of looping on "couldn't delete".
|
||||||
fetch('/api/v1/pages' + route + '/media/' + encodeURIComponent(name), { credentials: 'include', method: 'DELETE' })
|
apiSend('/api/v1/pages' + route + '/media/' + encodeURIComponent(name), { method: 'DELETE' }, [204, 404])
|
||||||
.then(function (r) {
|
.then(function () {
|
||||||
if (!(r.ok || r.status === 204 || r.status === 404)) throw new Error('delete failed');
|
|
||||||
// Deleted. Renumber survivors (cover=first), then refresh. A failure
|
// Deleted. Renumber survivors (cover=first), then refresh. A failure
|
||||||
// AFTER this point must NOT resurrect the deleted photo — show the
|
// AFTER this point must NOT resurrect the deleted photo — show the
|
||||||
// survivor set, never lastGood.
|
// survivor set, never lastGood.
|
||||||
@@ -1007,9 +1024,9 @@ function initPhotoEditor(route) {
|
|||||||
render(remaining);
|
render(remaining);
|
||||||
}
|
}
|
||||||
);
|
);
|
||||||
}, function () {
|
}, function (err) {
|
||||||
// The DELETE request itself failed — nothing changed on disk.
|
// The DELETE request itself failed — nothing changed on disk.
|
||||||
setStatus('Couldn’t delete that photo. Try again.', true);
|
setStatus(editErrorMsg(err, 'Couldn’t delete that photo. Try again.'), true);
|
||||||
render(lastGood);
|
render(lastGood);
|
||||||
})
|
})
|
||||||
.then(function () { setBusy(false); });
|
.then(function () { setBusy(false); });
|
||||||
@@ -1035,7 +1052,7 @@ function initPhotoEditor(route) {
|
|||||||
|
|
||||||
function addFiles(files) {
|
function addFiles(files) {
|
||||||
setBusy(true);
|
setBusy(true);
|
||||||
var total = files.length, done = 0, failed = 0;
|
var total = files.length, done = 0, failed = 0, authFailed = false;
|
||||||
var before = photos.slice();
|
var before = photos.slice();
|
||||||
var seq = Promise.resolve();
|
var seq = Promise.resolve();
|
||||||
files.forEach(function (file) {
|
files.forEach(function (file) {
|
||||||
@@ -1045,8 +1062,11 @@ function initPhotoEditor(route) {
|
|||||||
return toWebSafe(file).then(function (prep) {
|
return toWebSafe(file).then(function (prep) {
|
||||||
var fd = new FormData();
|
var fd = new FormData();
|
||||||
fd.append('file', prep.blob, prep.name);
|
fd.append('file', prep.blob, prep.name);
|
||||||
return apiOk('/api/v1/pages' + route + '/media', { method: 'POST', body: fd });
|
return apiSend('/api/v1/pages' + route + '/media', { method: 'POST', body: fd });
|
||||||
}).then(function (ok) { if (!ok) failed++; }, function () { failed++; });
|
}).then(null, function (err) {
|
||||||
|
failed++;
|
||||||
|
if (err && (err.status === 401 || err.status === 403)) authFailed = true;
|
||||||
|
});
|
||||||
});
|
});
|
||||||
});
|
});
|
||||||
seq.then(function () {
|
seq.then(function () {
|
||||||
@@ -1060,22 +1080,43 @@ function initPhotoEditor(route) {
|
|||||||
// One renumber pass after the whole batch. If it fails, auto-retry
|
// One renumber pass after the whole batch. If it fails, auto-retry
|
||||||
// (idempotent — renumber skips files not on disk); if it still fails,
|
// (idempotent — renumber skips files not on disk); if it still fails,
|
||||||
// roll the just-added files back so no orphan stock-named image breaks
|
// roll the just-added files back so no orphan stock-named image breaks
|
||||||
// cover=first, then surface a single error.
|
// cover=first, then surface a single error. Track whether every
|
||||||
|
// rollback DELETE actually landed (204/404 = gone): a swallowed
|
||||||
|
// rollback failure would leave a stray stock-named file that can steal
|
||||||
|
// the lexicographic cover slot, so we warn the owner to reload.
|
||||||
return reorder(order)
|
return reorder(order)
|
||||||
.catch(function () { return reorder(order); })
|
.catch(function () { return reorder(order); })
|
||||||
.catch(function () {
|
.catch(function () {
|
||||||
return Promise.all(added.map(function (n) {
|
return Promise.all(added.map(function (n) {
|
||||||
return apiOk('/api/v1/pages' + route + '/media/' + encodeURIComponent(n), { method: 'DELETE' }).catch(function () {});
|
return apiSend('/api/v1/pages' + route + '/media/' + encodeURIComponent(n), { method: 'DELETE' }, [204, 404])
|
||||||
})).then(function () { var e = new Error('reorder failed'); e.rolledBack = true; throw e; });
|
.then(function () { return true; }, function () { return false; });
|
||||||
|
})).then(function (results) {
|
||||||
|
var e = new Error('reorder failed');
|
||||||
|
e.rolledBack = true;
|
||||||
|
e.rollbackIncomplete = results.indexOf(false) !== -1;
|
||||||
|
throw e;
|
||||||
|
});
|
||||||
})
|
})
|
||||||
.then(mediaList);
|
.then(mediaList);
|
||||||
}).then(function (list) {
|
}).then(function (list) {
|
||||||
if (list) render(list);
|
if (list) render(list);
|
||||||
setStatus(failed ? (failed + ' photo' + (failed > 1 ? 's' : '') + ' couldn’t be added.') : '', !!failed);
|
if (!failed) {
|
||||||
|
setStatus('');
|
||||||
|
} else if (authFailed) {
|
||||||
|
setStatus('Couldn’t add photos — your login session expired. Sign in again, then retry.', true);
|
||||||
|
} else {
|
||||||
|
setStatus(failed + ' photo' + (failed > 1 ? 's' : '') + ' couldn’t be added.', true);
|
||||||
|
}
|
||||||
}).catch(function (err) {
|
}).catch(function (err) {
|
||||||
setStatus(err && err.rolledBack
|
var msg;
|
||||||
? 'Couldn’t finish adding photos — changes were rolled back. Try again.'
|
if (err && err.rolledBack) {
|
||||||
: 'Couldn’t add photos. Please try again.', true);
|
msg = err.rollbackIncomplete
|
||||||
|
? 'Couldn’t finish adding photos and cleanup was incomplete — reload the page and check your photos.'
|
||||||
|
: 'Couldn’t finish adding photos — changes were rolled back. Try again.';
|
||||||
|
} else {
|
||||||
|
msg = 'Couldn’t add photos. Please try again.';
|
||||||
|
}
|
||||||
|
setStatus(msg, true);
|
||||||
return mediaList().then(render, function () { render(before); });
|
return mediaList().then(render, function () { render(before); });
|
||||||
}).then(function () { setBusy(false); });
|
}).then(function () { setBusy(false); });
|
||||||
}
|
}
|
||||||
|
|||||||
Reference in New Issue
Block a user