Skip to content

fix: make clients shared across all users#478

Open
devin-ai-integration[bot] wants to merge 2 commits intomainfrom
devin/1776235955-fix-shared-clients
Open

fix: make clients shared across all users#478
devin-ai-integration[bot] wants to merge 2 commits intomainfrom
devin/1776235955-fix-shared-clients

Conversation

@devin-ai-integration
Copy link
Copy Markdown
Contributor

@devin-ai-integration devin-ai-integration bot commented Apr 15, 2026

Summary

Clients were incorrectly scoped per-user — every query on the clients table filtered by WHERE user_email = ?, so clients created by one user were invisible to others. This PR removes the user_email filter from all client lookups, updates, and deletes across three route files (clients.js, workEntries.js, reports.js), making clients a shared resource. Work entries remain scoped to individual users via user_email.

Files changed:

  • backend/src/routes/clients.js — Remove user_email filter from all GET/PUT/DELETE queries. POST still stores user_email on insert.
  • backend/src/routes/workEntries.js — Client existence checks no longer filter by user_email. Work entry CRUD remains user-scoped. Stale ownership comment updated.
  • backend/src/routes/reports.js — Client lookups no longer filter by user_email. Work entry queries in reports remain user-scoped.
  • Test files updated to match new query parameters and error messages.

Review & Testing Checklist for Human

  • DELETE /api/clients/ is now completely unscoped — it runs DELETE FROM clients with no WHERE clause, meaning any authenticated user can delete every client in the system. Verify this is the desired behavior; consider whether this endpoint should be removed, require admin authorization, or at minimum retain some scoping.
  • Any user can update or delete any client — PUT /:id and DELETE /:id no longer verify ownership. Confirm there's no need for an ownership or role-based check on mutations.
  • Error messages changed"Client not found or does not belong to user" is now "Client not found" in workEntries.js. Verify the frontend does not depend on the exact error string for display or logic.
  • Reports endpoint exposes all clients — a user can now request a report for any client (though they'll only see their own work entries). Verify this is acceptable.

Suggested test plan: Log in as User A, create a client. Log out, log in as User B — verify the client is visible. Create a work entry against that client as User B. Log back in as User A — verify the client is visible but User B's work entry is not. Also test the delete-all endpoint and confirm the blast radius is understood.

Notes

  • The user_email column and foreign key on the clients table schema are unchanged — user_email is still written on INSERT and could serve as a created_by audit field, but is no longer used for access control.
  • The stale comment at workEntries.js:173 has been updated from "verify it belongs to user" to "verify the client exists".
  • All 161 backend unit tests pass.

Link to Devin session: https://partner-workshops.devinenterprise.com/sessions/6b698c37524a4d05bd47986b6725ba79


Open with Devin

Root cause: clients table queries filtered by user_email, scoping
clients per-user. Clients created by one user were invisible to others.

Changes:
- Remove user_email filter from all client SELECT/UPDATE/DELETE queries
- Remove user_email filter from client existence checks in work entries
- Remove user_email filter from client lookups in reports
- Keep user_email on INSERT for audit trail (created_by tracking)
- Work entries remain user-scoped (user_email filter preserved)
- Update tests to reflect shared client behavior
@devin-ai-integration
Copy link
Copy Markdown
Contributor Author

🤖 Devin AI Engineer

I'll be helping with this pull request! Here's what you should know:

✅ I will automatically:

  • Address comments on this PR. Add '(aside)' to your comment to have me ignore it.
  • Look at CI failures and help fix them

Note: I can only respond to comments from users who have write access to this repository.

⚙️ Control Options:

  • Disable automatic comment and CI monitoring

Copy link
Copy Markdown
Contributor Author

@devin-ai-integration devin-ai-integration bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Devin Review found 1 potential issue.

View 5 additional findings in Devin Review.

Open in Devin Review

Comment on lines +194 to +195
'DELETE FROM clients',
[],
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 DELETE /api/clients now deletes ALL clients globally instead of only the authenticated user's clients

The DELETE / route in clients.js changed from DELETE FROM clients WHERE user_email = ? to DELETE FROM clients with no filter. Any authenticated user hitting this endpoint (which is invoked via deleteAllClients() in frontend/src/api/client.ts:85 and exposed with a button in frontend/src/pages/ClientsPage.tsx:155-158) will now delete every client in the entire database across all users. Because work_entries has FOREIGN KEY (client_id) REFERENCES clients (id) ON DELETE CASCADE (backend/src/database/init.js:64), this also cascade-deletes all work entries for all users. Previously this was safely scoped to WHERE user_email = ?, deleting only the current user's clients.

Prompt for agents
The DELETE / route in backend/src/routes/clients.js now executes DELETE FROM clients with no WHERE clause, meaning any authenticated user can wipe every client (and cascade-delete every work entry) for all users in the system. If the intent of the PR is to make clients shared/visible to all users, this bulk-delete endpoint needs to be reconsidered. Options include: (1) Restrict this endpoint to only delete clients owned by the current user (restore the WHERE user_email = ? filter), since client creation still stores user_email via the INSERT. (2) Add an admin-only authorization check before allowing a global delete. (3) Remove this endpoint entirely if global delete is never the intended behavior. The frontend calls this via apiClient.deleteAllClients() in frontend/src/api/client.ts:84-87 and it is triggered by handleDeleteAll in frontend/src/pages/ClientsPage.tsx:155-158.
Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

0 participants