Ling0925
/
buster
mirror of https://github.com/buster-so/buster.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

Fix access control linting

This commit is contained in:
Nate Kelley 2025-07-07 10:14:13 -06:00
parent 5d532a2957
commit c432f7d1f5
No known key found for this signature in database
GPG Key ID: FD90372AB8D98B4F
5 changed files with 118 additions and 100 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

15
packages/access-controls/src/chats.ts
View File

@ -27,20 +27,20 @@ export const canUserAccessChat = async ({
}): Promise<boolean> => {
// Validate inputs
const input = CanUserAccessChatSchema.parse({ userId, chatId });
const db = getDb();
// Run all permission checks concurrently for optimal performance
const [directPermission, collectionPermission, chatInfo, userOrgs] = await Promise.all([
// Check 1: Direct user permission on chat
checkDirectChatPermission(db, input.userId, input.chatId),
// Check 2: User permission through collections
checkCollectionChatPermission(db, input.userId, input.chatId),
// Check 3: Get chat info (creator & organization)
getChatInfo(db, input.chatId),
// Check 4: Get user's organizations and roles
getUserOrganizations(db, input.userId),
]);
@ -157,12 +157,7 @@ async function getUserOrganizations(
role: usersToOrganizations.role,
})
.from(usersToOrganizations)
.where(
and(
eq(usersToOrganizations.userId, userId),
isNull(usersToOrganizations.deletedAt)
)
);
.where(and(eq(usersToOrganizations.userId, userId), isNull(usersToOrganizations.deletedAt)));
return result;
}

61
packages/access-controls/tests/integration/chats.int.test.ts
View File

@ -1,22 +1,22 @@
import { afterAll, beforeAll, describe, expect, test } from 'vitest';
import { randomUUID } from 'node:crypto';
import {
and,
assetPermissions,
chats,
collectionsToAssets,
getDb,
usersToOrganizations,
users,
organizations,
collections,
collectionsToAssets,
eq,
and,
getDb,
organizations,
users,
usersToOrganizations,
} from '@buster/database';
import { afterAll, beforeAll, describe, expect, test } from 'vitest';
import { canUserAccessChat } from '../../src/chats';
describe('canUserAccessChat Integration Tests', () => {
const db = getDb();
// Test data IDs
const testOrgId = randomUUID();
const systemUserId = randomUUID(); // System user for created_by/updated_by fields
@ -193,39 +193,42 @@ describe('canUserAccessChat Integration Tests', () => {
afterAll(async () => {
// Clean up test data in reverse order of creation
// Delete collections_to_assets
await db.delete(collectionsToAssets).where(eq(collectionsToAssets.collectionId, testCollectionId));
await db
.delete(collectionsToAssets)
.where(eq(collectionsToAssets.collectionId, testCollectionId));
// Delete asset_permissions
await db.delete(assetPermissions).where(
and(
eq(assetPermissions.assetId, testChatId),
eq(assetPermissions.identityId, testUserId)
)
);
await db.delete(assetPermissions).where(
and(
eq(assetPermissions.assetId, testCollectionId),
eq(assetPermissions.identityId, testUserId)
)
);
await db
.delete(assetPermissions)
.where(
and(eq(assetPermissions.assetId, testChatId), eq(assetPermissions.identityId, testUserId))
);
await db
.delete(assetPermissions)
.where(
and(
eq(assetPermissions.assetId, testCollectionId),
eq(assetPermissions.identityId, testUserId)
)
);
// Delete collections
await db.delete(collections).where(eq(collections.id, testCollectionId));
// Delete chats
await db.delete(chats).where(eq(chats.organizationId, testOrgId));
// Delete users_to_organizations
await db.delete(usersToOrganizations).where(eq(usersToOrganizations.organizationId, testOrgId));
// Delete users
await db.delete(users).where(eq(users.id, testUserId));
await db.delete(users).where(eq(users.id, testAdminUserId));
await db.delete(users).where(eq(users.id, testUserNoAccessId));
await db.delete(users).where(eq(users.id, systemUserId));
// Delete organization
await db.delete(organizations).where(eq(organizations.id, testOrgId));
});
@ -335,4 +338,4 @@ describe('canUserAccessChat Integration Tests', () => {
// Concurrent execution should be significantly faster
expect(duration).toBeLessThan(100); // Generous threshold for CI environments
});
});
});

10
packages/access-controls/tests/integration/chats.simple.int.test.ts
View File

@ -4,7 +4,7 @@ import { canUserAccessChat } from '../../src/chats';
describe('canUserAccessChat Simple Integration Tests', () => {
// These tests will use existing test data in the database
// They demonstrate the function works with real database queries
test('should return false for non-existent chat', async () => {
const result = await canUserAccessChat({
userId: '00000000-0000-0000-0000-000000000001',
@ -32,15 +32,15 @@ describe('canUserAccessChat Simple Integration Tests', () => {
test('should execute all queries concurrently', async () => {
const startTime = Date.now();
// Even with non-existent IDs, all 4 queries should run concurrently
await canUserAccessChat({
userId: '00000000-0000-0000-0000-000000000001',
chatId: '00000000-0000-0000-0000-000000000002',
});
const duration = Date.now() - startTime;
// If queries were sequential, they would take much longer
// With concurrent execution, should be under 200ms even in CI
expect(duration).toBeLessThan(200);
@ -56,4 +56,4 @@ describe('canUserAccessChat Simple Integration Tests', () => {
expect(result).toBe(true);
});
});
});

18
packages/access-controls/tests/unit/chats-cached.test.ts
View File

@ -1,12 +1,12 @@
import { beforeEach, describe, expect, it, vi } from 'vitest';
import {
canUserAccessChatCached,
getCacheStats,
resetCacheStats,
clearCache,
getCacheStats,
invalidateAccess,
invalidateUserAccess,
invalidateChatAccess,
invalidateUserAccess,
resetCacheStats,
} from '../../src/chats-cached';
// Mock the original canUserAccessChat function
@ -21,7 +21,7 @@ describe('canUserAccessChatCached', () => {
vi.clearAllMocks();
clearCache();
resetCacheStats();
// Get the mocked function
const chatsModule = await import('../../src/chats');
mockCanUserAccessChat = vi.mocked(chatsModule.canUserAccessChat);
@ -73,10 +73,10 @@ describe('canUserAccessChatCached', () => {
// Setup different responses
mockCanUserAccessChat
.mockResolvedValueOnce(true) // user1:chat1
.mockResolvedValueOnce(false) // user1:chat2
.mockResolvedValueOnce(false) // user2:chat1
.mockResolvedValueOnce(true); // user2:chat2
.mockResolvedValueOnce(true) // user1:chat1
.mockResolvedValueOnce(false) // user1:chat2
.mockResolvedValueOnce(false) // user2:chat1
.mockResolvedValueOnce(true); // user2:chat2
// Make calls
expect(await canUserAccessChatCached({ userId: user1, chatId: chat1 })).toBe(true);
@ -248,4 +248,4 @@ describe('canUserAccessChatCached', () => {
expect(result).toBe(true);
expect(mockCanUserAccessChat).toHaveBeenCalledTimes(2); // Called again, error wasn't cached
});
});
});

114
packages/access-controls/tests/unit/chats.test.ts
View File

@ -40,7 +40,7 @@ describe('canUserAccessChat', () => {
beforeEach(async () => {
vi.clearAllMocks();
// Get the mocked module
const dbModule = await import('@buster/database');
getDb = vi.mocked(dbModule.getDb);
@ -57,9 +57,9 @@ describe('canUserAccessChat', () => {
mockLimit.mockResolvedValue([]);
mockWhere.mockReturnValue({ limit: mockLimit });
mockInnerJoin.mockReturnValue({ where: mockWhere });
mockFrom.mockReturnValue({
mockFrom.mockReturnValue({
where: mockWhere,
innerJoin: mockInnerJoin
innerJoin: mockInnerJoin,
});
mockSelect.mockReturnValue({ from: mockFrom });
mockSelectDistinct.mockReturnValue({ from: mockFrom });
@ -79,7 +79,7 @@ describe('canUserAccessChat', () => {
it('should return false if chat does not exist', async () => {
// All queries return empty arrays
mockDb._mockLimit.mockResolvedValue([]);
const result = await canUserAccessChat({
userId: '123e4567-e89b-12d3-a456-426614174000',
chatId: '223e4567-e89b-12d3-a456-426614174000',
@ -96,12 +96,15 @@ describe('canUserAccessChat', () => {
if (callCount === 1) {
// Direct permission check - has permission
return Promise.resolve([{ id: 'chat-id' }]);
} else if (callCount === 3) {
}
if (callCount === 3) {
// Chat info
return Promise.resolve([{
createdBy: 'other-user',
organizationId: 'org-id',
}]);
return Promise.resolve([
{
createdBy: 'other-user',
organizationId: 'org-id',
},
]);
}
return Promise.resolve([]);
});
@ -129,12 +132,15 @@ describe('canUserAccessChat', () => {
if (callCount === 2) {
// Collection permission check - has permission
return Promise.resolve([{ collectionId: 'collection-id' }]);
} else if (callCount === 3) {
}
if (callCount === 3) {
// Chat info
return Promise.resolve([{
createdBy: 'other-user',
organizationId: 'org-id',
}]);
return Promise.resolve([
{
createdBy: 'other-user',
organizationId: 'org-id',
},
]);
}
return Promise.resolve([]);
});
@ -157,16 +163,18 @@ describe('canUserAccessChat', () => {
it('should return true if user is the creator', async () => {
const userId = '123e4567-e89b-12d3-a456-426614174000';
let callCount = 0;
mockDb._mockLimit.mockImplementation(() => {
callCount++;
if (callCount === 3) {
// Chat info - user is creator
return Promise.resolve([{
createdBy: userId,
organizationId: 'org-id',
}]);
return Promise.resolve([
{
createdBy: userId,
organizationId: 'org-id',
},
]);
}
return Promise.resolve([]);
});
@ -189,16 +197,18 @@ describe('canUserAccessChat', () => {
it('should return true if user is workspace_admin', async () => {
const orgId = 'org-123';
let callCount = 0;
mockDb._mockLimit.mockImplementation(() => {
callCount++;
if (callCount === 3) {
// Chat info
return Promise.resolve([{
createdBy: 'other-user',
organizationId: orgId,
}]);
return Promise.resolve([
{
createdBy: 'other-user',
organizationId: orgId,
},
]);
}
return Promise.resolve([]);
});
@ -206,10 +216,12 @@ describe('canUserAccessChat', () => {
mockDb._mockWhere.mockImplementation(() => {
// For user organizations query (doesn't use limit)
if (mockDb._mockWhere.mock.calls.length === 4) {
return Promise.resolve([{
organizationId: orgId,
role: 'workspace_admin',
}]);
return Promise.resolve([
{
organizationId: orgId,
role: 'workspace_admin',
},
]);
}
return { limit: mockDb._mockLimit };
});
@ -224,16 +236,18 @@ describe('canUserAccessChat', () => {
it('should return true if user is data_admin', async () => {
const orgId = 'org-123';
let callCount = 0;
mockDb._mockLimit.mockImplementation(() => {
callCount++;
if (callCount === 3) {
// Chat info
return Promise.resolve([{
createdBy: 'other-user',
organizationId: orgId,
}]);
return Promise.resolve([
{
createdBy: 'other-user',
organizationId: orgId,
},
]);
}
return Promise.resolve([]);
});
@ -241,10 +255,12 @@ describe('canUserAccessChat', () => {
mockDb._mockWhere.mockImplementation(() => {
// For user organizations query (doesn't use limit)
if (mockDb._mockWhere.mock.calls.length === 4) {
return Promise.resolve([{
organizationId: orgId,
role: 'data_admin',
}]);
return Promise.resolve([
{
organizationId: orgId,
role: 'data_admin',
},
]);
}
return { limit: mockDb._mockLimit };
});
@ -259,16 +275,18 @@ describe('canUserAccessChat', () => {
it('should return false if user has no access', async () => {
const orgId = 'org-123';
let callCount = 0;
mockDb._mockLimit.mockImplementation(() => {
callCount++;
if (callCount === 3) {
// Chat info exists
return Promise.resolve([{
createdBy: 'other-user',
organizationId: orgId,
}]);
return Promise.resolve([
{
createdBy: 'other-user',
organizationId: orgId,
},
]);
}
return Promise.resolve([]);
});
@ -276,10 +294,12 @@ describe('canUserAccessChat', () => {
mockDb._mockWhere.mockImplementation(() => {
// For user organizations query (doesn't use limit)
if (mockDb._mockWhere.mock.calls.length === 4) {
return Promise.resolve([{
organizationId: orgId,
role: 'viewer', // Not an admin role
}]);
return Promise.resolve([
{
organizationId: orgId,
role: 'viewer', // Not an admin role
},
]);
}
return { limit: mockDb._mockLimit };
});
@ -307,4 +327,4 @@ describe('canUserAccessChat', () => {
})
).rejects.toThrow();
});
});
});