[dev] [carhartlewis] lewis/comp-people-fix#2596
Merged
Merged
Conversation
Contributor
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
![vercel[bot]](https://avatars.githubusercontent.com/in/8329?s=60&v=4){kind=small}
Findings were scattered across ~7 entry points (Tasks, People directory,
People Tasks, People Devices, People Chart, Documents/evidence form types,
Questionnaire context, Admin). Consolidate everything into a dedicated
`/overview/findings` route so auditors raise findings in one place and
everyone else sees one list with deep-links back to the linked item.
Highlights
- Schema: drop `FindingScope`; add `FindingSeverity`, `FindingArea`; add
nullable FKs for policy / vendor / risk / member / device alongside
existing task / evidence-submission / evidence-form-type targets.
Migration backfills legacy `scope` rows onto `area = 'people'`.
- API: rewrite FindingsService, audit service, notifier, and controllers
around the unified target validator (exactly one of 8 targets or an
area). Single `GET /v1/findings` supports filtering by status, severity,
area, and any target FK. Admin controller reuses the same DTO.
- Permissions: `finding:create` and `finding:delete` are auditor-only;
owners/admins keep `read` + `update` so they can transition status.
- Frontend
- New `/overview/findings` route with Link-based tabs so the dashboard
no longer loads findings data. Open-count badge on the Findings tab.
- DS-styled `FindingsTab` table with search + status / severity /
framework filters, DS `Empty` state, 360px balanced columns, and
truncated content with a hover tooltip.
- `CreateFindingSheet`: target-type picker (task / policy / vendor /
risk / person / device / document type / area) with live comboboxes
backed by existing endpoints, capitalised labels, and
auto-selection of the org's framework when only one is configured.
- `FindingDetailSheet`: edit content (auditor-only), change status /
severity, add revision note, view activity log, and delete. Content
renders as read-only for non-auditors.
- `use-findings-api` collapsed to `useOrganizationFindings`,
`useFindingHistory`, `useFindingTemplates`, `useFindingActions`.
- Deletions: `tasks/[taskId]/components/findings/` (whole folder),
`PeopleFindingsUnifiedList`, `DocumentFindingsSection`,
`FindingsOverview`, plus call-site cleanup in SingleTask,
CompanyFormPageClient, people/page, and PeoplePageTabs.
- Admin: old `FindingForm` removed; admin `FindingsTab` now reuses the
unified `CreateFindingSheet` via a `createFn` override that targets
`/v1/admin/organizations/:orgId/findings`.
- People perf: wrap `TeamMembers` RSC in `<Suspense>` with a skeleton so
the tabs paint immediately; `DevicesTabContent` renders agent devices
right away and shows a small loading pill while the slower Fleet SWR
resolves. Dev-only 3s delay in `useFleetHosts` for local testing.
- Audit log: drop the "Admin"/"Auditor" prefix from finding descriptions
in `extractFindingDescription` — the actor name already says it.
- DX: `packages/db/scripts/seed-load-test.ts` seeds ~100 members, 25
devices, and a 10×10 org-chart grid for local load testing.
- Misc: auditor view "Export All Evidence" swapped from broken
Popover-over-render-Button to a proper DS `Sheet`; local DS patch
adding `nativeButton={false}` to pagination `ButtonPrimitive`s while
the upstream fix lands.
- Tests: placeholder specs replaced with real coverage for the target
validator, notifier recipient resolution, admin findings tab, and the
unified FindingsTab (empty state + auditor-gated create).
carhartlewis
force-pushed
the
lewis/comp-people-fix
branch
from
c6c628a to
3b5418a
Compare
…b functionality - Modified the deep link format in `finding-notifier.service.ts` to direct users to the Findings page with the finding sheet pre-opened. - Enhanced `FindingsTab.tsx` to support deep linking by auto-opening the specified finding's sheet when the page loads, and removing the query parameter to prevent reopening on refresh. - Integrated `useRouter` and `useSearchParams` from Next.js for improved navigation and state management.
… sharing functionality - Implemented an AlertDialog for confirming the deletion of findings, enhancing user experience by preventing accidental deletions. - Added a "Copy link" button to share the finding's link, allowing users to easily share findings with their team. - Updated the delete button to open the confirmation dialog instead of directly deleting the finding.
`getTaskRecipients` was using `user.role != 'admin'` (better-auth platform admin) in the where clause, which effectively matched every active non-platform-admin member of the org. That meant a task-scoped finding would email every employee/contractor/auditor/custom-role member with a 200-char preview of the finding content plus a deep link — rather than just the people who can actually act on it. This mirrors what the policy/vendor/risk/device branches already do: fetch the target's assignee and union with org owners/admins via `includeAdmins`. Finding content is now only disclosed to stakeholders of that specific task. Pre-existing behaviour (not introduced by the unified-findings PR) but worth fixing here since it's a concrete over-share of audit content. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
Security Review —
|
| # | File | Claim | Verdict |
|---|---|---|---|
| 1 | apps/api/src/findings/finding-notifier.service.ts — getTaskRecipients |
Task-finding notifications fan out to every active non-platform-admin | |
| member of the org (employees, contractors, etc.), each receiving a 200-char content preview + deep link. | Pre-existing (identical `OR: [{ user: { role: | ||
{ not: 'admin' } } }, { role: { contains: 'owner' } }]query existed onmainingetTaskAssigneeAndAdmins). **Fixed in this branch** (commit 2db2f7b`) |
|||
| — now mirrors the policy/vendor/risk/device branches by notifying owners/admins + task assignee only. |
Positive confirmations (verified secure in this PR)
- Org-scoping on mutations.
FindingsService.findById,update,delete,getActivity, and the target resolver usefindFirst({ where: { id, organizationId } })before any mutation. NofindUnique(id)that would allow cross-tenant access. - Target FK validation.
resolveTargetvalidates each supplied target FK (task/policy/vendor/risk/member/device/evidenceSubmission) is in the caller's
org before attaching. Cross-tenant target attachment is prevented. - DTO validation.
ValidationPipe({ whitelist: true, forbidNonWhitelisted: true })+class-validatordecorators onCreateFindingDto/
UpdateFindingDto. Enum filters viaObject.values().includes(...)onstatus,area,severity.ValidateFindingIdPipeenforcesfnd_[a-z0-9]+. - Permission gating. Every customer-facing endpoint on
FindingsControllerhas@UseGuards(HybridAuthGuard, PermissionGuard)+
@RequirePermission('finding', ...). Admin controller retainsPlatformAdminGuard. - Permission tightening.
finding:createandfinding:deleteremoved from owner/admin; only auditor + platform admin can create/delete findings. - Deep-link safety.
buildFindingDeepLinkreturns/{orgId}/overview/findings?open={findingId}. Both params are non-secret, server-generated; no token,
no redirect-back.FindingsTabonly opens the detail sheet if the finding is present in the org-scoped list. - Share-link button. Uses
window.location.origin+ the same scoped route; no secrets or tokens embedded in the URL. - No SQL injection vectors. All queries use Prisma; the migration SQL is static with no dynamic input.
- Cascade deletes.
onDelete: CascadeonFindingFKs to task/evidenceSubmission/policy/vendor/risk/member/device is intentional (dependent findings
are removed when target is gone). Cannot cross tenant boundaries because the target FKs themselves are org-scoped. - Route guards preserved.
apps/app/src/app/(app)/[orgId]/overview/layout.tsxstill callsawait requireRoutePermission('overview', orgId). - Audit log content.
AuditLog.data.previousContent/newContentare written server-side and scoped byorganizationIdon reads; no cross-tenant
exposure. - Notifier unsubscribe honoured.
isUserUnsubscribed(..., 'findingNotifications', organizationId)checked before send.
Recommendation
Approve from a security standpoint.
Contributor
|
🎉 This PR is included in version 3.24.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is an automated pull request to merge lewis/comp-people-fix into dev.
It was created by the [Auto Pull Request] action.