fix(categories): address PR #790 review — event ownership, migration renumber, nits
- 🔴 Event ownership: GET /event/:eventId and DELETE /reorder/:eventId now use requireEventOwnership; POST /reorder (event_id in body) gets the equivalent inline check (super_admin bypasses; others limited to owned/ownerless events). New test covers a settings.edit-holding non-super_admin blocked (403) on all three per-event routes. - 🔴 Migration renumber: 158→159, 159→160 (upstream #788 already took 158); headers + the test's require path updated. - 🟢 Nits: stale inline "Drag the arrows" fallback → "Use the arrows" (matches en.json; control is click-only); invalid bg-accent-dark/150 → bg-accent-dark.
This commit is contained in:
Luca
@@ -2,9 +2,9 @@
|
||||
* Layered per-event category ordering (#782).
|
||||
*
|
||||
* Two ordering layers, resolved per event:
|
||||
* - GLOBAL default — photo_categories.display_order (migration 158),
|
||||
* - GLOBAL default — photo_categories.display_order (migration 159),
|
||||
* set via POST /reorder-global; applies everywhere.
|
||||
* - PER-EVENT override — event_category_order (migration 159), set via
|
||||
* - PER-EVENT override — event_category_order (migration 160), set via
|
||||
* POST /reorder; overrides the default for one gallery.
|
||||
* - DELETE /reorder/:eventId clears an event's override.
|
||||
*
|
||||
@@ -58,7 +58,7 @@ describe('category ordering (#782)', () => {
|
||||
|
||||
const getEvent = (eventId) => auth(request(app).get(`/api/admin/categories/event/${eventId}`)).expect(200);
|
||||
|
||||
describe('migration 158 backfill', () => {
|
||||
describe('migration 159 backfill', () => {
|
||||
it('seeds display_order from alphabetical order, scoped per event', async () => {
|
||||
const eventId = await insertEvent('backfill-ev');
|
||||
await insertCat('Reception', { event_id: eventId });
|
||||
@@ -67,7 +67,7 @@ describe('category ordering (#782)', () => {
|
||||
|
||||
// Re-run the migration: addColumn is guarded (no-op); the backfill loop
|
||||
// re-runs and assigns per-scope alphabetical order — what an upgrade does.
|
||||
await require('../../migrations/core/158_add_category_display_order').up(db);
|
||||
await require('../../migrations/core/159_add_category_display_order').up(db);
|
||||
|
||||
const evCats = await db('photo_categories').where({ event_id: eventId }).orderBy('display_order', 'asc');
|
||||
expect(evCats.map((c) => c.name)).toEqual(['Ceremony', 'Pre-Ceremony', 'Reception']);
|
||||
@@ -154,6 +154,47 @@ describe('category ordering (#782)', () => {
|
||||
});
|
||||
});
|
||||
|
||||
describe('event ownership (PR #790 review)', () => {
|
||||
let limitedToken;
|
||||
let foreignEventId;
|
||||
|
||||
beforeAll(async () => {
|
||||
const bcrypt = require('bcrypt');
|
||||
// A non-super_admin role that DOES hold settings.view + settings.edit —
|
||||
// the exact case the review flagged (settings.edit is grantable).
|
||||
const roleRes = await db('roles').insert({ name: 'gallery-mgr', display_name: 'Gallery Mgr' }).returning('id');
|
||||
const roleId = roleRes[0]?.id ?? roleRes[0];
|
||||
const permIds = await db('permissions').whereIn('name', ['settings.view', 'settings.edit']).pluck('id');
|
||||
await db('role_permissions').insert(permIds.map((permission_id) => ({ role_id: roleId, permission_id })));
|
||||
|
||||
const a2 = await db('admin_users').insert({
|
||||
username: 'limited', email: '[email protected]',
|
||||
password_hash: await bcrypt.hash('x', 4), role_id: roleId,
|
||||
must_change_password: false, created_at: new Date(),
|
||||
}).returning('id');
|
||||
limitedToken = mintAdminToken(a2[0]?.id ?? a2[0]);
|
||||
|
||||
// An event owned by a DIFFERENT admin (the seeded super_admin).
|
||||
const owner = (await db('admin_users').where({ username: 'tester' }).first()).id;
|
||||
await db('events').insert({
|
||||
event_type: 'wedding', password_hash: 'x',
|
||||
expires_at: new Date(Date.now() + 9e9).toISOString(),
|
||||
is_active: true, is_archived: false, slug: 'owned-ev', share_link: 'owned-ev',
|
||||
event_name: 'Owned', event_date: '2026-01-01', created_by: owner,
|
||||
});
|
||||
foreignEventId = (await db('events').where({ slug: 'owned-ev' }).first()).id;
|
||||
});
|
||||
|
||||
const limitedAuth = (r) => r.set('Authorization', `Bearer ${limitedToken}`);
|
||||
|
||||
it('blocks a non-owner from reading, reordering or resetting another event', async () => {
|
||||
await limitedAuth(request(app).get(`/api/admin/categories/event/${foreignEventId}`)).expect(403);
|
||||
await limitedAuth(request(app).post('/api/admin/categories/reorder'))
|
||||
.send({ event_id: foreignEventId, orderedIds: [1] }).expect(403);
|
||||
await limitedAuth(request(app).delete(`/api/admin/categories/reorder/${foreignEventId}`)).expect(403);
|
||||
});
|
||||
});
|
||||
|
||||
describe('POST / (create) appends to the end of its scope', () => {
|
||||
it('assigns display_order = max + 1 within the event', async () => {
|
||||
const eventId = await insertEvent('append-ev');
|
||||
|
||||
+1
-1
@@ -1,5 +1,5 @@
|
||||
/**
|
||||
* Migration 158: per-event category ordering (#782).
|
||||
* Migration 159: per-event category ordering (#782).
|
||||
*
|
||||
* Adds a `display_order` integer to `photo_categories` so photographers can
|
||||
* arrange an event's categories in the flow of the day (Pre-Ceremony →
|
||||
+2
-2
@@ -1,7 +1,7 @@
|
||||
/**
|
||||
* Migration 159: per-event category order override (#782).
|
||||
* Migration 160: per-event category order override (#782).
|
||||
*
|
||||
* Builds on migration 158 (photo_categories.display_order = the GLOBAL default
|
||||
* Builds on migration 159 (photo_categories.display_order = the GLOBAL default
|
||||
* order) by adding a per-event OVERRIDE layer. Global categories are shared
|
||||
* across every event, so a single display_order can only express one order for
|
||||
* them. This table lets a single gallery arrange its categories — globals AND
|
||||
@@ -4,6 +4,7 @@ const { db, logActivity } = require('../database/db');
|
||||
const { formatBoolean } = require('../utils/dbCompat');
|
||||
const { adminAuth } = require('../middleware/auth');
|
||||
const { requirePermission } = require('../middleware/permissions');
|
||||
const { requireEventOwnership } = require('../middleware/ownership');
|
||||
const { getEventCategoriesOrdered } = require('../utils/categoryOrder');
|
||||
const logger = require('../utils/logger');
|
||||
const router = express.Router();
|
||||
@@ -26,7 +27,7 @@ router.get('/global', adminAuth, requirePermission('settings.view'), async (req,
|
||||
// Get categories for a specific event (global + event-specific), resolved to
|
||||
// the event's effective order: per-event override, else global default, else
|
||||
// name (#782). Each row carries `override_position` (null when not customised).
|
||||
router.get('/event/:eventId', adminAuth, requirePermission('settings.view'), async (req, res) => {
|
||||
router.get('/event/:eventId', adminAuth, requirePermission('settings.view'), requireEventOwnership, async (req, res) => {
|
||||
try {
|
||||
const categories = await getEventCategoriesOrdered(req.params.eventId);
|
||||
res.json(categories);
|
||||
@@ -282,6 +283,20 @@ router.post('/reorder', adminAuth, requirePermission('settings.edit'), [
|
||||
const eventId = parseInt(req.body.event_id, 10);
|
||||
const orderedIds = req.body.orderedIds.map((id) => parseInt(id, 10));
|
||||
|
||||
// Event ownership (event_id comes from the body, so requireEventOwnership —
|
||||
// which reads req.params — can't be used here). Mirror it: super_admins
|
||||
// bypass; other admins may only reorder events they own (ownerless
|
||||
// legacy/system events allowed).
|
||||
if (req.admin.roleName !== 'super_admin') {
|
||||
const event = await db('events').where('id', eventId).first();
|
||||
if (!event) {
|
||||
return res.status(404).json({ error: 'Event not found' });
|
||||
}
|
||||
if (event.created_by && event.created_by !== req.admin.id) {
|
||||
return res.status(403).json({ error: 'Access denied' });
|
||||
}
|
||||
}
|
||||
|
||||
// Every id must be a category available to this event: a shared global OR
|
||||
// one of the event's own categories. Anything else is out of scope.
|
||||
const available = await db('photo_categories')
|
||||
@@ -317,7 +332,7 @@ router.post('/reorder', adminAuth, requirePermission('settings.edit'), [
|
||||
});
|
||||
|
||||
// Clear an event's override — revert this gallery to the global default order.
|
||||
router.delete('/reorder/:eventId', adminAuth, requirePermission('settings.edit'), async (req, res) => {
|
||||
router.delete('/reorder/:eventId', adminAuth, requirePermission('settings.edit'), requireEventOwnership, async (req, res) => {
|
||||
try {
|
||||
const eventId = parseInt(req.params.eventId, 10);
|
||||
await db('event_category_order').where('event_id', eventId).del();
|
||||
|
||||
@@ -4,7 +4,7 @@
|
||||
* Resolves an event's categories into their effective display order, layering:
|
||||
* 1. per-event override — event_category_order.position, when the event has
|
||||
* been customised;
|
||||
* 2. the global default — photo_categories.display_order (migration 158);
|
||||
* 2. the global default — photo_categories.display_order (migration 159);
|
||||
* 3. name.
|
||||
*
|
||||
* Globals and event-specific categories are ordered together so a custom order
|
||||
|
||||
@@ -177,7 +177,7 @@ export const EventCategoryManager: React.FC<EventCategoryManagerProps> = ({ even
|
||||
<p className="text-xs text-neutral-500 dark:text-neutral-400 italic">
|
||||
{isCustomised
|
||||
? t('categories.orderCustomisedHint', 'This gallery uses a custom order. Reset to follow the global default (Settings → Photo Categories).')
|
||||
: t('categories.orderDefaultHint', 'Drag the arrows to set the order for this gallery. Otherwise it follows the global default (Settings → Photo Categories).')}
|
||||
: t('categories.orderDefaultHint', 'Use the arrows to set the order for this gallery. Otherwise it follows the global default (Settings → Photo Categories).')}
|
||||
</p>
|
||||
|
||||
{/* Add new category form */}
|
||||
@@ -378,7 +378,7 @@ export const EventCategoryManager: React.FC<EventCategoryManagerProps> = ({ even
|
||||
/>
|
||||
</div>
|
||||
{isSelected && (
|
||||
<div className="absolute top-2 right-2 bg-accent-dark/150 text-white rounded-full p-1">
|
||||
<div className="absolute top-2 right-2 bg-accent-dark text-white rounded-full p-1">
|
||||
<Check className="w-4 h-4" />
|
||||
</div>
|
||||
)}
|
||||
|
||||
Reference in New Issue
Block a user