Skip to content

fix(libraries): prevent adding documents to read-only libraries #845

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
thostetler merged 3 commits into from
Apr 15, 2026
Merged

fix(libraries): prevent adding documents to read-only libraries #845

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
cdd6dd5
fix(libraries): prevent adding documents to read-only libraries
thostetler Apr 13, 2026
9a1ffcf
fix(libraries): restore hidden permission column in add-to-library modal
thostetler Apr 13, 2026
7029529
fix(libraries): address review feedback on row styling and aria labels
thostetler Apr 13, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 8 additions & 4 deletions src/components/Libraries/AddToLibraryModal.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,9 +58,9 @@ export const AddToLibraryModal = ({

const clearSelections = useStore((state) => state.clearSelected);

const { mutate: editDocs, isLoading: isEditingDocs } = useEditLibraryDocuments();
const { mutateAsync: editDocs, isLoading: isEditingDocs } = useEditLibraryDocuments();

const { mutate: addDocsByQuery, isLoading: isAddingDocs } = useAddDocumentsByQuery();
const { mutateAsync: addDocsByQuery, isLoading: isAddingDocs } = useAddDocumentsByQuery();

const toast = useToast();

Expand Down Expand Up @@ -171,6 +171,8 @@ const AddToExistingLibraryPane = ({

const libraries = librariesData?.libraries ?? [];

const readOnlyLibIds = libraries.filter((l) => l.permission === 'read').map((l) => l.id);

const entries = librariesData?.count ?? 0;

const handleSortChange = (sort: ILibraryListTableSort) => {
Expand All @@ -188,11 +190,12 @@ const AddToExistingLibraryPane = ({
};

const handleSelectLibrary = (id: LibraryIdentifier) => {
if (readOnlyLibIds.includes(id)) {
return;
}
if (selectedLibs.includes(id)) {
// deselect
setSelectedLibs((prev) => prev.filter((l) => l !== id));
} else {
// select
setSelectedLibs((prev) => [...prev, id]);
}
};
Expand Down Expand Up @@ -234,6 +237,7 @@ const AddToExistingLibraryPane = ({
hideCols={['public', 'num_users', 'permission', 'date_created']}
selectable
selected={selectedLibs}
disabledIds={readOnlyLibIds}
onChangeSort={handleSortChange}
onChangePageIndex={handlePageIndexChange}
onChangePageSize={handlePageSizeChange}
Expand Down
189 changes: 100 additions & 89 deletions src/components/Libraries/LibraryListTable.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,7 @@ export interface ILibraryListTableProps extends TableProps {
showDescription?: boolean;
selected?: LibraryIdentifier[];
selectable?: boolean;
disabledIds?: LibraryIdentifier[];
onChangeSort: (sort: ILibraryListTableSort) => void;
onChangePageIndex: (index: number) => void;
onChangePageSize: (size: NumPerPageType) => void;
Expand All @@ -130,6 +131,7 @@ export const LibraryListTable = (props: ILibraryListTableProps) => {
showDescription = true,
selected = [],
selectable = false,
disabledIds = [],
onChangeSort,
onChangePageIndex,
onChangePageSize,
Expand Down Expand Up @@ -247,97 +249,106 @@ export const LibraryListTable = (props: ILibraryListTableProps) => {
date_last_modified,
},
index,
) => (
<Tr
key={id}
cursor="pointer"
_hover={{ backgroundColor: colors.highlightBackground, color: colors.highlightForeground }}
onClick={() => onLibrarySelect(id)}
tabIndex={0}
onKeyDown={(e) => {
if (e.key === 'Enter' || e.key === ' ') {
onLibrarySelect(id);
) => {
const isDisabled = disabledIds.includes(id);
return (
<Tr
key={id}
cursor={isDisabled ? 'not-allowed' : 'pointer'}
opacity={isDisabled ? 0.5 : 1}
_hover={
!isDisabled
? { backgroundColor: colors.highlightBackground, color: colors.highlightForeground }
: undefined
}
}}
role="row"
aria-selected={selected.includes(id)}
backgroundColor={selected.includes(id) ? colors.highlightBackground : 'transparent'}
color={selected.includes(id) ? colors.highlightForeground : colors.text}
style={{ backgroundColor: colors.highlightBackground, color: colors.highlightForeground }}
>
{!isMobile && (selectable || showIndex) && (
<Td>
{showIndex && `${pageSize * pageIndex + index + 1} `}
{selectable && (
<Checkbox
isChecked={selected.includes(id)}
pointerEvents="none"
tabIndex={-1}
aria-label={`library ${selected.includes(id)} ? 'selected' ? 'not selected`}
/>
)}
</Td>
)}
{allHiddenCols.indexOf('public') === -1 && (
<Td>
{isPublic ? (
<Tooltip label="Public">
<UnlockIcon color="green.500" aria-label="public" w={3} h={3} />
</Tooltip>
) : (
<Tooltip label="Private">
<LockIcon aria-label="private" w={3} h={3} />
</Tooltip>
)}
</Td>
)}
{allHiddenCols.indexOf('num_users') === -1 && (
<Td>
{num_users === 1 ? (
<Tooltip label="No collaborators">
<IconButton as={UserIcon} aria-label="no collaborators" w={4} h={4} variant="unstyled" />
</Tooltip>
) : (
<Tooltip label={`${num_users} collaborators`}>
<IconButton
as={UserGroupIcon}
aria-label="has collaborators"
color="green.500"
w={4}
h={4}
variant="unstyled"
onClick={() => !isDisabled && onLibrarySelect(id)}
tabIndex={isDisabled ? -1 : 0}
onKeyDown={(e) => {
if (!isDisabled && (e.key === 'Enter' || e.key === ' ')) {
onLibrarySelect(id);
}
}}
role="row"
aria-selected={selected.includes(id)}
aria-disabled={isDisabled}
backgroundColor={selected.includes(id) ? colors.highlightBackground : 'transparent'}
color={selected.includes(id) ? colors.highlightForeground : colors.text}
>
{!isMobile && (selectable || showIndex) && (
<Td>
{showIndex && `${pageSize * pageIndex + index + 1} `}
{selectable && (
<Checkbox
isChecked={selected.includes(id)}
isDisabled={isDisabled}
pointerEvents="none"
tabIndex={-1}
aria-label={selected.includes(id) ? 'library selected' : 'library not selected'}
/>
</Tooltip>
)}
</Td>
)}
{allHiddenCols.indexOf('name') === -1 && (
<Td>
<Text fontWeight="bold">{name}</Text>
{showDescription && <Text>{description}</Text>}
</Td>
)}
{allHiddenCols.indexOf('num_documents') === -1 && <Td>{num_documents}</Td>}
{allHiddenCols.indexOf('owner') === -1 && <Td>{owner}</Td>}
{allHiddenCols.indexOf('permission') === -1 && <Td>{permission}</Td>}
{allHiddenCols.indexOf('date_last_modified') === -1 && (
<Td>
<TimeSince date={date_last_modified} />
</Td>
)}
{showSettings && !isMobile && (
<Td>
<Center>
<Action
onDelete={() => handleDeleteLibrary(id)}
onSetting={() => handleSettings(id)}
disableDelete={permission !== 'owner'}
/>
</Center>
</Td>
)}
</Tr>
),
)}
</Td>
)}
{allHiddenCols.indexOf('public') === -1 && (
<Td>
{isPublic ? (
<Tooltip label="Public">
<UnlockIcon color="green.500" aria-label="public" w={3} h={3} />
</Tooltip>
) : (
<Tooltip label="Private">
<LockIcon aria-label="private" w={3} h={3} />
</Tooltip>
)}
</Td>
)}
{allHiddenCols.indexOf('num_users') === -1 && (
<Td>
{num_users === 1 ? (
<Tooltip label="No collaborators">
<IconButton as={UserIcon} aria-label="no collaborators" w={4} h={4} variant="unstyled" />
</Tooltip>
) : (
<Tooltip label={`${num_users} collaborators`}>
<IconButton
as={UserGroupIcon}
aria-label="has collaborators"
color="green.500"
w={4}
h={4}
variant="unstyled"
/>
</Tooltip>
)}
</Td>
)}
{allHiddenCols.indexOf('name') === -1 && (
<Td>
<Text fontWeight="bold">{name}</Text>
{showDescription && <Text>{description}</Text>}
</Td>
)}
{allHiddenCols.indexOf('num_documents') === -1 && <Td>{num_documents}</Td>}
{allHiddenCols.indexOf('owner') === -1 && <Td>{owner}</Td>}
{allHiddenCols.indexOf('permission') === -1 && <Td>{permission}</Td>}
{allHiddenCols.indexOf('date_last_modified') === -1 && (
<Td>
<TimeSince date={date_last_modified} />
</Td>
)}
{showSettings && !isMobile && (
<Td>
<Center>
<Action
onDelete={() => handleDeleteLibrary(id)}
onSetting={() => handleSettings(id)}
disableDelete={permission !== 'owner'}
/>
</Center>
</Td>
)}
</Tr>
);
},
)}
</Tbody>
</Table>
Expand Down
102 changes: 102 additions & 0 deletions src/components/Libraries/__tests__/AddToLibraryModal.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,102 @@
import { render, waitFor } from '@/test-utils';
import { describe, expect, test, vi } from 'vitest';
import { AddToLibraryModal } from '../AddToLibraryModal';

const mocks = vi.hoisted(() => ({
useRouter: vi.fn(() => ({
query: { q: 'star' },
asPath: '/search?q=star',
push: vi.fn(),
events: { on: vi.fn(), off: vi.fn() },
})),
}));

vi.mock('next/router', () => ({ useRouter: mocks.useRouter }));

describe('AddToLibraryModal', () => {
const defaultProps = {
bibcodes: ['2021ApJ...000A...1X'],
isOpen: true,
onClose: vi.fn(),
};

test('read-only library cannot be selected', async () => {
const { user, findByRole, findByText } = render(<AddToLibraryModal {...defaultProps} />);

// Wait for libraries to load — the table should appear
await findByRole('table');

// Library "003" from mock data has permission "read" — its name is "003"
const readOnlyRow = await findByText('003');
await user.click(readOnlyRow);

// Submit button should still be disabled because nothing was selected
const submitButton = await findByRole('button', { name: /submit/i });
expect(submitButton).toBeDisabled();
});

test('read-only library row is visually marked as disabled', async () => {
const { findByRole, findAllByRole } = render(<AddToLibraryModal {...defaultProps} />);

await findByRole('table');

const rows = await findAllByRole('row');
// Find the row for library "003" (read-only)
const readOnlyRow = rows.find(
(row) => row.textContent?.includes('003') && row.getAttribute('aria-disabled') === 'true',
);
expect(readOnlyRow).toBeDefined();
});

test('writable library can be selected and enables submit', async () => {
const { user, findByRole, findByText } = render(<AddToLibraryModal {...defaultProps} />);

await findByRole('table');

// Library "001" has permission "admin" — should be selectable
const writableRow = await findByText('001');
await user.click(writableRow);

const submitButton = await findByRole('button', { name: /submit/i });
expect(submitButton).not.toBeDisabled();
});

test('does not invoke success callback when adding to a library returns 403', async () => {
// This tests the mutateAsync fix: Promise.all should reject when API returns 403,
// so onClose(true) must NOT be called — previously it was called immediately because
// mutate() returned void (not a Promise), causing Promise.all to resolve right away.
const { server } = await import('@/mocks/server');
const { rest } = await import('msw');
const { ApiTargets } = await import('@/api/models');
const onClose = vi.fn();

server.use(
rest.post(`*${ApiTargets.DOCUMENTS}/:id`, (_req, res, ctx) => {
return res(ctx.status(403), ctx.json({ error: 'Insufficient permissions' }));
}),
);

const { user, findByRole, findByText, queryByText } = render(
<AddToLibraryModal bibcodes={['2021ApJ...000A...1X']} isOpen={true} onClose={onClose} />,
);

await findByRole('table');

// Select writable library "001"
const writableRow = await findByText('001');
await user.click(writableRow);

const submitButton = await findByRole('button', { name: /submit/i });
await user.click(submitButton);

// Wait for the mutation to settle — the button exits its loading state once
// the 403 response is received and the error handler runs
await waitFor(() => expect(submitButton).not.toBeDisabled());

// Success callback (onClose(true)) must NOT have been called
expect(onClose).not.toHaveBeenCalledWith(true);

// Success banner text must NOT appear anywhere
expect(queryByText(/paper\(s\) added/i)).not.toBeInTheDocument();
});
});
5 changes: 5 additions & 0 deletions src/mocks/handlers/libraries.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,6 +168,11 @@ export const librariesHandlers = [
apiHandlerRoute(ApiTargets.DOCUMENTS, '/:id'),
async (req, res, ctx) => {
const id = req.params.id;
const library = libraries.find((l) => l.id === id);

if (library?.permission === 'read') {
return res(ctx.status(403), ctx.json({ error: 'Insufficient permissions' }));
}

if (req.body.action === 'remove') {
// remove docs
Expand Down
Loading