Skip to content

Commit b02f2d0

Browse files
committed
Cache admin status per request and clean up method formatting
- Add optional isAdmin parameter to checkPermission and getPermissionSummary to avoid repeated DB queries per entity in list endpoints - Compute admin status once in handleListScoreFolderContent and pass through - Remove trailing semicolons from method closing braces in Auth.ts - Use SQL column aliases (AS userId, AS authType, AS groupId) instead of destructuring renames in verifyAndRotateRefreshToken - Clarify null/undefined convention: null acceptable at external API boundaries, convert to undefined at first opportunity Signed-off-by: Mike Lischke <mike@lischke-online.de>
1 parent dd8643c commit b02f2d0

3 files changed

Lines changed: 35 additions & 29 deletions

File tree

.github/copilot-instructions.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -135,7 +135,7 @@ Always put a blank line after blocks (`if`/`for`/`while`/`switch`/`case`/anonymo
135135

136136
- Use enums for discriminated union type literals (e.g., `enum SelectionGranularity { ... }` instead of `type X = "a" | "b"`).
137137
- Enum members do not carry string values unless the value is consumed directly as a string (e.g., CSS values).
138-
- Use `undefined` instead of `null` everywhere.
138+
- Use `undefined` instead of `null` everywhere in project-owned code. `null` from external APIs (database, libraries) is acceptable at the boundary — convert to `undefined` at the first opportunity.
139139
- Use `field?: Type` syntax instead of `field: Type | undefined` for optional fields.
140140
- Interface names always start with a capital `I` (e.g., `ISoundStyleMeta`, `IMeasureStep`).
141141
- Inline type casts (`as { ... }`) are acceptable for one-off use. If the same anonymous shape appears more than once, extract it to a named interface.

src/server/Auth.ts

Lines changed: 30 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -292,7 +292,7 @@ export class Auth {
292292
canManageInstruments: false,
293293
canExportMP3: false,
294294
};
295-
};
295+
}
296296

297297
/**
298298
* Checks whether a user has the required access level on an entity.
@@ -309,13 +309,15 @@ export class Auth {
309309
* @param entityType The type of entity ("score", "folder", "feature").
310310
* @param entityId The entity id, or null for features.
311311
* @param requiredLevel The required access level (Read or Write).
312+
* @param isAdmin Optional. If true, skips the admin group DB query.
312313
*
313314
* @returns True if the user has the required access.
314315
*/
315316
public async checkPermission(user: ITokenPayload | undefined, entityType: EntityType,
316-
entityId: number | null, requiredLevel: AccessLevel,): Promise<boolean> {
317+
entityId: number | null, requiredLevel: AccessLevel, isAdmin?: boolean,): Promise<boolean> {
317318
// Admin users always have full access.
318-
if (user && await this.isUserInAdminGroup(user.userId)) {
319+
const admin = isAdmin ?? (user ? await this.isUserInAdminGroup(user.userId) : false);
320+
if (admin) {
319321
return true;
320322
}
321323

@@ -377,7 +379,7 @@ export class Auth {
377379
}
378380

379381
return false;
380-
};
382+
}
381383

382384
/**
383385
* Computes a permission summary for a user on an entity.
@@ -386,12 +388,13 @@ export class Auth {
386388
* @param user The authenticated user, or undefined for anonymous.
387389
* @param entityType The type of entity ("score", "folder").
388390
* @param entityId The entity id.
391+
* @param isAdmin Optional. If true, skips the admin group DB query.
389392
*
390393
* @returns A summary of the user's access.
391394
*/
392395
public async getPermissionSummary(user: ITokenPayload | undefined, entityType: EntityType,
393-
entityId: number): Promise<IPermissionSummary> {
394-
const isAdmin = user ? await this.isUserInAdminGroup(user.userId) : false;
396+
entityId: number, isAdmin?: boolean,): Promise<IPermissionSummary> {
397+
const admin = isAdmin ?? (user ? await this.isUserInAdminGroup(user.userId) : false);
395398

396399
const resolved = await this.resolvePermission(entityType, entityId);
397400

@@ -418,8 +421,8 @@ export class Auth {
418421
userGroupIds.add(worldId);
419422
}
420423

421-
let canRead = isAdmin || isOwner;
422-
let canWrite = isAdmin || isOwner;
424+
let canRead = admin || isOwner;
425+
let canWrite = admin || isOwner;
423426

424427
const isWorld = worldId !== undefined
425428
&& resolved.groupEntries.some((e) => {
@@ -449,7 +452,7 @@ export class Auth {
449452
isWorld,
450453
groupIds,
451454
};
452-
};
455+
}
453456

454457
/**
455458
* Sets the owner for an entity. An explicit NULL means "inherit from parent"
@@ -473,7 +476,7 @@ export class Auth {
473476
await this.adapter.execute(`INSERT INTO permissions (entity_type, entity_id, owner_id) VALUES (?, ?, ?) ` +
474477
`ON DUPLICATE KEY UPDATE owner_id = VALUES(owner_id)`, [entityType, entityId, ownerId],
475478
);
476-
};
479+
}
477480

478481
/**
479482
* Adds a group assignment to an entity. If the group is already assigned,
@@ -491,7 +494,7 @@ export class Auth {
491494
`VALUES (?, ?, ?, ?) ON DUPLICATE KEY UPDATE writable = GREATEST(writable, VALUES(writable))`,
492495
[entityType, entityId, groupId, writable ? 1 : 0],
493496
);
494-
};
497+
}
495498

496499
/**
497500
* Removes a group assignment from an entity.
@@ -506,7 +509,7 @@ export class Auth {
506509
"DELETE FROM entity_groups WHERE entity_type = ? AND entity_id = ? AND group_id = ?",
507510
[entityType, entityId, groupId],
508511
);
509-
};
512+
}
510513

511514
/**
512515
* @returns the explicit group assignments for an entity (not inherited).
@@ -524,7 +527,7 @@ export class Auth {
524527
return rows.map((r) => {
525528
return { groupId: r.group_id, writable: Boolean(r.writable) };
526529
});
527-
};
530+
}
528531

529532
/**
530533
* Returns the explicit owner for an entity (or null if inherited/deleted).
@@ -542,7 +545,7 @@ export class Auth {
542545
);
543546

544547
return rows[0]?.owner_id ?? null;
545-
};
548+
}
546549

547550
/**
548551
* Records a login audit event.
@@ -557,7 +560,7 @@ export class Auth {
557560
await this.adapter.execute(`INSERT INTO login_audit (user_id, event, group_id, ip_address) VALUES (?, ?, ?, ?)`,
558561
[userId, event, groupId ?? null, ipAddress ?? null],
559562
);
560-
};
563+
}
561564

562565
/**
563566
* Checks whether a user is a member of the Admins group.
@@ -572,7 +575,7 @@ export class Auth {
572575
);
573576

574577
return (rows[0]?.cnt ?? 0) > 0;
575-
};
578+
}
576579

577580
/**
578581
* Returns the ID of the World group.
@@ -581,7 +584,7 @@ export class Auth {
581584
*/
582585
public async getWorldGroupId(): Promise<number | undefined> {
583586
return this.firstMatchingGroupId(Auth.worldGroupName);
584-
};
587+
}
585588

586589
/**
587590
* Returns the ID of the Admins group.
@@ -590,7 +593,7 @@ export class Auth {
590593
*/
591594
public async getAdminGroupId(): Promise<number | undefined> {
592595
return this.firstMatchingGroupId(Auth.adminGroupName);
593-
};
596+
}
594597

595598
/**
596599
* Verifies a refresh token against the stored hash and rotates it.
@@ -607,14 +610,15 @@ export class Auth {
607610
const hash = crypto.createHash("sha256").update(rawToken).digest("hex");
608611

609612
const rows = await this.adapter.query<{
610-
id: number; auth_type: string | null; group_id: number | null;
611-
}>("SELECT id, auth_type, group_id FROM users WHERE refresh_token_hash = ?", [hash]);
613+
userId: number; authType: string | null; groupId: number | null;
614+
}>("SELECT id AS userId, auth_type AS authType, group_id AS groupId FROM users WHERE refresh_token_hash = ?",
615+
[hash]);
612616

613617
if (rows.length === 0) {
614618
return undefined;
615619
}
616620

617-
const { id: userId, auth_type: authType, group_id: groupId } = rows[0];
621+
const { userId, authType, groupId } = rows[0];
618622
const newRaw = crypto.randomBytes(32).toString("hex");
619623
const newHash = crypto.createHash("sha256").update(newRaw).digest("hex");
620624

@@ -626,7 +630,7 @@ export class Auth {
626630
authType: authType ?? undefined,
627631
groupId: groupId ?? undefined,
628632
};
629-
};
633+
}
630634

631635
/**
632636
* Resolves the effective owner for an entity by walking up the tree.
@@ -681,7 +685,7 @@ export class Auth {
681685
}
682686

683687
return null;
684-
};
688+
}
685689

686690
/**
687691
* Collects all group assignments for an entity by walking up the tree.
@@ -742,7 +746,7 @@ export class Auth {
742746
}
743747

744748
return collected;
745-
};
749+
}
746750

747751
/**
748752
* Resolves the full effective permission state for an entity by combining
@@ -764,7 +768,7 @@ export class Auth {
764768
return { groupId, writable };
765769
}),
766770
};
767-
};
771+
}
768772

769773
private async getUserGroups(userId: number): Promise<Array<{ group_id: number; }>> {
770774
return this.adapter.query<{ group_id: number; }>(
@@ -778,6 +782,6 @@ export class Auth {
778782
[groupName]);
779783

780784
return rows[0]?.id;
781-
};
785+
}
782786

783787
}

src/server/ScoreRoutes.ts

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -60,9 +60,11 @@ export class ScoreRoutes {
6060
scoreParams,
6161
);
6262

63+
const isAdmin = user ? await this.ctx.auth.isUserInAdminGroup(user.userId) : false;
64+
6365
const readableFolders: Array<Record<string, unknown>> = [];
6466
for (const f of folders) {
65-
const summary = await this.ctx.auth.getPermissionSummary(user, EntityType.Folder, f.id as number);
67+
const summary = await this.ctx.auth.getPermissionSummary(user, EntityType.Folder, f.id as number, isAdmin);
6668

6769
if (summary.canRead) {
6870
readableFolders.push({
@@ -77,7 +79,7 @@ export class ScoreRoutes {
7779

7880
const readableScores: Array<Record<string, unknown>> = [];
7981
for (const s of scores) {
80-
const summary = await this.ctx.auth.getPermissionSummary(user, EntityType.Score, s.id as number);
82+
const summary = await this.ctx.auth.getPermissionSummary(user, EntityType.Score, s.id as number, isAdmin);
8183

8284
if (summary.canRead) {
8385
readableScores.push({

0 commit comments

Comments
 (0)