perf: Skip Role Route Capability Probe for Own and Default Roles (#14073)

This commit is contained in:
Danny Avila 2026-07-02 10:45:41 -04:00 committed by GitHub
parent f1ea4159af
commit b6c0bc7c0d
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
2 changed files with 60 additions and 9 deletions

View file

@ -58,6 +58,7 @@ describe('GET /api/roles/:roleName — isOwnRole authorization', () => {
expect(res.status).toBe(200);
expect(res.body.name).toBe('STAFF');
expect(mockGetRoleByName).toHaveBeenCalledWith('STAFF', '-_id -__v');
expect(mockHasCapability).not.toHaveBeenCalled();
});
it('returns 403 when a custom role user requests a different custom role', async () => {
@ -85,6 +86,17 @@ describe('GET /api/roles/:roleName — isOwnRole authorization', () => {
const res = await request(app).get(`/api/roles/${SystemRoles.USER}`);
expect(res.status).toBe(200);
expect(mockHasCapability).not.toHaveBeenCalled();
});
it('allows a custom role user to fetch a non-admin default role without a capability probe', async () => {
mockGetRoleByName.mockResolvedValue(userRole);
const app = createApp({ id: 'u1', role: 'STAFF' });
const res = await request(app).get(`/api/roles/${SystemRoles.USER}`);
expect(res.status).toBe(200);
expect(mockHasCapability).not.toHaveBeenCalled();
});
it('returns 403 when USER requests the ADMIN role', async () => {
@ -103,6 +115,7 @@ describe('GET /api/roles/:roleName — isOwnRole authorization', () => {
const res = await request(app).get(`/api/roles/${SystemRoles.ADMIN}`);
expect(res.status).toBe(200);
expect(mockHasCapability).not.toHaveBeenCalled();
});
it('allows any user with READ_ROLES capability to fetch any role', async () => {
@ -116,6 +129,21 @@ describe('GET /api/roles/:roleName — isOwnRole authorization', () => {
expect(res.body.name).toBe('STAFF');
});
it('probes READ_ROLES with a coerced CapabilityUser for non-own-role lookups', async () => {
mockHasCapability.mockResolvedValue(true);
mockGetRoleByName.mockResolvedValue(staffRole);
const app = createApp({ id: 'u1', role: SystemRoles.USER });
const res = await request(app).get('/api/roles/STAFF');
expect(res.status).toBe(200);
expect(mockHasCapability).toHaveBeenCalledTimes(1);
expect(mockHasCapability).toHaveBeenCalledWith(
{ id: 'u1', role: SystemRoles.USER, tenantId: undefined, idOnTheSource: null },
expect.any(String),
);
});
it('returns 404 when the requested role does not exist', async () => {
mockGetRoleByName.mockResolvedValue(null);
const app = createApp({ id: 'u1', role: 'GHOST' });
@ -143,7 +171,7 @@ describe('GET /api/roles/:roleName — isOwnRole authorization', () => {
expect(mockGetRoleByName).not.toHaveBeenCalled();
});
it('treats hasCapability failure as no capability (does not 500)', async () => {
it('serves own role without probing even when hasCapability would fail', async () => {
mockHasCapability.mockRejectedValue(new Error('capability check failed'));
const app = createApp({ id: 'u1', role: 'STAFF' });
mockGetRoleByName.mockResolvedValue(staffRole);
@ -151,5 +179,16 @@ describe('GET /api/roles/:roleName — isOwnRole authorization', () => {
const res = await request(app).get('/api/roles/STAFF');
expect(res.status).toBe(200);
expect(mockHasCapability).not.toHaveBeenCalled();
});
it('treats hasCapability failure as no capability for other roles (403, not 500)', async () => {
mockHasCapability.mockRejectedValue(new Error('capability check failed'));
const app = createApp({ id: 'u1', role: 'STAFF' });
const res = await request(app).get('/api/roles/MANAGER');
expect(res.status).toBe(403);
expect(mockGetRoleByName).not.toHaveBeenCalled();
});
});

View file

@ -117,16 +117,28 @@ router.get('/:roleName', async (req, res) => {
const { roleName } = req.params;
try {
let hasReadRoles = false;
try {
hasReadRoles = await hasCapability(req.user, SystemCapabilities.READ_ROLES);
} catch (err) {
logger.warn(`[GET /roles/:roleName] capability check failed: ${err.message}`);
}
const isOwnRole = req.user?.role === roleName;
const isDefaultRole = Object.hasOwn(roleDefaults, roleName);
if (!hasReadRoles && !isOwnRole && (roleName === SystemRoles.ADMIN || !isDefaultRole)) {
return res.status(403).send({ message: 'Unauthorized' });
/** READ_ROLES only gates reading other roles; own role and non-admin default roles skip the probe */
const requiresReadRoles = !isOwnRole && (roleName === SystemRoles.ADMIN || !isDefaultRole);
if (requiresReadRoles) {
let hasReadRoles = false;
try {
hasReadRoles = await hasCapability(
{
id: req.user?.id ?? req.user?._id?.toString() ?? '',
role: req.user?.role ?? '',
tenantId: req.user?.tenantId,
idOnTheSource: req.user?.idOnTheSource ?? null,
},
SystemCapabilities.READ_ROLES,
);
} catch (err) {
logger.warn(`[GET /roles/:roleName] capability check failed: ${err.message}`);
}
if (!hasReadRoles) {
return res.status(403).send({ message: 'Unauthorized' });
}
}
const role = await getRoleByName(roleName, '-_id -__v');