Repository navigation
feat(investigations): Accept typed creation actors - #129095
Conversation
Accept validated user and service-account creators in agentic creation, using existing creator references and preserving metric deduplication. Keep production callers human-only until saved-identity dispatch is ready.
github-actions
Bot
added
the
Scope: Backend
…ation-creation-actors # Conflicts: # src/sentry/investigations/services/orchestration.py Co-Authored-By: Tony Xiao <txiao@sentry.io>
| created_by_service_account_id=( | ||
| creator.id if creator.type == ActorType.SERVICE_ACCOUNT else None | ||
| ), |
There was a problem hiding this comment.
h: For a service-account creator created_by_id is None, and dispatch_investigation_orchestration_create (tasks/seer/investigation.py:386-388) only sets viewer_context["user_id"] from created_by_id. So the run goes to Seer with just organization_id: no actor, and the account's scope_list never limits what Seer does. create_investigation_orchestration_run then calls collect_user_org_context(None, ...), and _monitoring_providers reads created_by_id as well.
The description says production callers stay human-only until saved-identity dispatch is ready, but nothing here enforces that. As soon as a caller passes a service account, _schedule_orchestration_create queues the run on commit. Could we reject service-account creators (or skip scheduling for them) until dispatch can carry the account's identity, or plumb the service account into the viewer context in this PR?
There was a problem hiding this comment.
Addressed in 3b170fc. Creator, account-access, and project validation run first. Service-account creation is then rejected immediately before scheduling dispatch, inside the creation transaction, so the investigation, project links, and orchestration run roll back and no task is queued. Saved-identity dispatch remains deferred.
| if creator.type == ActorType.USER: | ||
| return |
There was a problem hiding this comment.
m: Users return here after only the type/ID check, while service accounts get the org, scope, feature and project checks below. Today the endpoint (permission class plus organization_project_ids) and the Slack operator's own membership and scope checks cover users. That splits the authorization boundary by actor type, though. A future internal caller that passes ActorId(USER, id) for a stale identity or a non-member gets an investigation attributed to that user, and Seer is then called with that user_id. Should users get the equivalent checks here (org membership, has_investigations_access(organization, actor=user), project access), so callers can rely on this function for both actor types?
There was a problem hiding this comment.
Addressed in 3b170fc. Human creators now resolve through the user service and from_user, require active organization membership and an investigation scope, and pass has_investigations_access(organization, actor=user). Both actor types must select a subset of active projects in the organization before creation or metric reuse. Open membership is required, so authorized members can access all active organization projects. Request-specific credential restrictions remain the authenticated caller's responsibility.
| accessible_project_ids = set( | ||
| Project.objects.filter( | ||
| organization=organization, status=ObjectStatus.ACTIVE, id__in=project_ids | ||
| ).values_list("id", flat=True) | ||
| ) | ||
| if not set(project_ids).issubset(accessible_project_ids): | ||
| raise InvestigationValidationError({"projectIds": "One or more projects are inaccessible."}) |
There was a problem hiding this comment.
l: This ACTIVE-status check only runs for service accounts, and it repeats the project query that _create_project_links runs inside the transaction. _create_project_links filters by organization but not by status, so callers that don't pre-filter (unlike the endpoint's organization_project_ids) can still link a pending-deletion project for a user creator. Adding status=ObjectStatus.ACTIVE to the query in _create_project_links would cover every actor with one query, and this block could go.
There was a problem hiding this comment.
Addressed in 3b170fc. Both paths now use the shared _validate_project_ids helper, which filters by organization and ACTIVE status and checks the requested IDs are a subset. _create_project_links invokes it inside the write transaction. Creation validation also invokes it before metric lookup, because a deduplication hit returns without calling _create_project_links.
| investigation = Investigation.objects.create( | ||
| organization=organization, | ||
| created_by_id=user_id, | ||
| created_by_id=creator.id if creator.type == ActorType.USER else None, |
There was a problem hiding this comment.
m: With a service-account creator this column is None, but the readers still only look at created_by_id: the serializer's createdBy (endpoints/serializers/investigation.py:183), _monitoring_providers (seer_client.py:110), and both _resolve_seer_run_mirror(user_id=...) calls in orchestration_events.py (1313, 1396). So a service-account investigation shows "createdBy": null, gets no monitoring-provider connections, and its SeerRun mirror has no user. Should those readers go through investigation.creator_actor so the attribution isn't lost?
There was a problem hiding this comment.
Addressed the investigation response and monitoring readers in bdac0bd. The response now includes nullable creator: {type, id} from creator_actor, while createdBy retains its existing human-user-ID contract.
Monitoring connections now follow the saved actor for the current dispatch: the creator initially, and requested_by_actor for commands. Humans use their own personal connections plus organization connections; service-account/system actors use organization connections. A null user ID already permits organization connections. Lookup failures now stop the request and raise a retryable error rather than omit the field and let Seer retain a previous actor's credentials.
SeerRun.user_id remains unchanged: it references User and participates in human ownership/history behavior. The authoritative service-account creator reference remains on Investigation; broader generic-run attribution is deferred.
| ({"creator": ActorId(ActorType.USER, True)}, InvestigationValidationError), | ||
| ({"creator": foreign_creator}, PermissionDenied), | ||
| ({"creator": unprivileged_creator}, PermissionDenied), | ||
| ({"project_ids": [foreign_project.id]}, InvestigationValidationError), |
There was a problem hiding this comment.
m: This case passes without the new project check: _create_project_links already raises InvestigationValidationError for another org's project, inside the transaction that gets rolled back. What the new check adds is the status=ObjectStatus.ACTIVE filter, and nothing covers it. Nothing covers a has_investigations_access denial either (feature off, or allow_joinleave=False). Deleting lines 168-174 of orchestration.py, or the has_investigations_access(organization) clause, would leave these tests green. Could you add a pending-deletion project case and a feature-off / closed-membership case?
There was a problem hiding this comment.
Addressed in 3b170fc. Replaced the redundant foreign-project case with pending-deletion project cases for both actor types, and added feature-off / closed-membership coverage for both. These checks exercise the metric reuse path, where the temporary pre-dispatch service-account rejection cannot mask missing authorization validation. The updated investigation and Seer RPC regression run passed 845 tests and 36 subtests; prek, scoped mypy, and private API schema validation also passed.
Expose typed creator attribution while preserving the legacy user field. Select monitoring connections per dispatch actor and retry failed lookups before sending requests to Seer.
![sentry-junior[bot]](https://avatars.githubusercontent.com/in/2960336?s=60&v=4){kind=small}
There was a problem hiding this comment.
Looks good to me. Callers can now rely on _validate_creation_actor for both actor types: user creators go through membership, scope, and feature checks, and both actor types get the same active-project check, including the metric dedup path. The service-account rejection inside the creation transaction rolls back before anything gets scheduled. Turning the monitoring-provider lookup failure into a retryable SeerApiError fits the existing dispatch retry and mark-failed handling, so a stale actor's connections can't stick around silently.
One thing to be aware of, no change needed: system-attributed commands now send org-only connections. That includes record_action_outcome and rerun_after_connection when mark_investigation_telemetry_connected runs without a user_id. So that auto-connect rerun replaces the creator's personal connections on the Seer run. test_nonhuman_commands_replace_personal_connections locks this in, so I'm reading it as intentional.
Accept validated user and service-account creators in agentic creation, using existing creator references and preserving metric deduplication.
Keep production callers human-only until saved-identity dispatch is ready.