Skip to content

fix(analytics): authorize person ids on metric-results - #2082

Merged
aleksdotbar merged 5 commits into
mainfrom
fix/2035-metric-results-authz-gate
Jul 31, 2026
Merged

fix(analytics): authorize person ids on metric-results#2082
aleksdotbar merged 5 commits into
mainfrom
fix/2035-metric-results-authz-gate

Conversation

@aleksdotbar

Copy link
Copy Markdown
Contributor

Problem

POST /v1/metric-results returned any person's metrics to any authenticated caller in the tenant. The handler read the SecurityContext for its tenant id only; nothing checked that the requested entity.ids were the caller or within their reporting scope.

Fix

Identity answers the authorization question, analytics enforces it.

  • POST /v1/visible-persons (identity-resolution): takes emails, returns the subset the caller may see. The existing visible-set predicate — caller, active grants, whole tenant on a wildcard grant, org_chart descendants — materialised once and joined against the resolved candidates, instead of one traversal per person. A wildcard grant short-circuits the traversal entirely.
  • The gate (analytics): requested person ids are filtered through that endpoint; any id outside the answer refuses the whole request with 403. No partial responses — a filtered 200 is indistinguishable from absent data.

No semantic change to who may see whom. The admin role still confers no visibility, and an email is visible when any person it resolves to is, matching POST /v1/profiles.

Failing to reach identity is a server error, never a denial, so an authorization backend that is down or unconfigured cannot read as "you may not see this".

Emails resolve by input position: persons.value_id compares case- and accent-insensitively, so the stored spelling cannot be mapped back to the request reliably. A candidate whose stored email merely compares equal — a different person under a loose collation — is dropped, since consumers key metric rows by the email bytes.

Verification

Live-MariaDB cases assert the predicate: a caller with no reports still sees themselves, a manager sees a transitive descendant and not an unrelated person, an explicit grant reaches outside the reporting line, a wildcard grant covers the tenant, and an admin role holder sees no one extra. Gate cases cover admit, refuse, whole-request refusal, service-subject bypass, unconfigured identity, and an identity that predates the endpoint (404/405 → 5xx, not 403). Route-table cases drive the real operation table.

Known gaps, not addressed here

  • POST /v1/metrics/{id}/query and POST /v1/metrics/queries interpolate client-supplied person_id / org_unit_id filter values with no visibility check — the same exposure class, reachable without touching this endpoint. Now documented as planned rather than implemented.
  • identityResolution.deploy defaults to false while identity_url is set unconditionally, so a default chart install refuses person metric requests. Needs a chart change and a release note.
  • migrations_and_bootstrap_are_idempotent_against_live_mariadb fails on a pristine migrated database, independent of this branch — no migration or SQL file is touched here.

ADR-0015 records the non-admin read, relaxing ADR-0012 for one self-scoped query.

Closes #2035

Batch form of the existing visible-set predicate, so a caller can be
authorized against many people in one query instead of one traversal
each. Semantics are unchanged: the admin role confers no visibility,
and an email is visible when any person it resolves to is.

Emails resolve by input position because value_id matches case- and
accent-insensitively, so the stored spelling cannot be mapped back to
the request reliably.

Signed-off-by: Aleksandr Barkhatov <pm@aleks.bar>
Any authenticated caller could read any person's metrics by changing the
requested id. Requested ids are now filtered through identity's
visible-persons check, and the whole request is refused when any one of
them is not visible.

Failing to reach identity is a server error, never a denial, so an
authorization backend that is down or unconfigured cannot read as "you
may not see this".

Closes #2035

Signed-off-by: Aleksandr Barkhatov <pm@aleks.bar>
ADR-0015 relaxes ADR-0012's admin-only rule for one read: answering a
caller's own visible set needs no admin, since the caller comes from the
gateway JWT and the response is a subset of the ids they supplied.

Corrects the analytics design, which described org-unit AccessScope
validation as implemented. It is not — the OData filter paths still
accept client-supplied person and org-unit ids unchecked, and that gap
is now marked as planned rather than current.

Signed-off-by: Aleksandr Barkhatov <pm@aleks.bar>
@aleksdotbar
aleksdotbar requested a review from a team as a code owner July 30, 2026 21:20
@coderabbitai

coderabbitai Bot commented Jul 30, 2026

Copy link
Copy Markdown

Warning

Review limit reached

@aleksdotbar, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 34 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: da5323e8-0254-4291-baf8-e614c22c4c7a

📥 Commits

Reviewing files that changed from the base of the PR and between b908f1c and 47001af.

📒 Files selected for processing (24)
  • docs/components/backend/analytics/DESIGN.md
  • docs/components/backend/identity-resolution/identity/README.md
  • docs/components/backend/identity-resolution/identity/specs/ADR/0015-self-scoped-visibility-read-without-admin.md
  • docs/components/backend/identity-resolution/identity/specs/DESIGN.md
  • docs/components/backend/identity-resolution/openapi.json
  • src/backend/services/analytics/src/api/handlers.rs
  • src/backend/services/analytics/src/api/http_live_tests.rs
  • src/backend/services/analytics/src/api/metric_results.rs
  • src/backend/services/analytics/src/api/mod.rs
  • src/backend/services/analytics/src/domain/metric_results/validation.rs
  • src/backend/services/analytics/src/domain/mod.rs
  • src/backend/services/analytics/src/domain/person_visibility.rs
  • src/backend/services/analytics/src/gear.rs
  • src/backend/services/analytics/src/infra/identity/mod.rs
  • src/backend/services/identity-resolution/src/api/mod.rs
  • src/backend/services/identity-resolution/src/api/visible_persons.rs
  • src/backend/services/identity-resolution/src/infra/db/mod.rs
  • src/backend/services/identity-resolution/src/infra/db/persons_repo.rs
  • src/backend/services/identity-resolution/src/infra/db/subchart_repo.rs
  • src/backend/services/identity-resolution/src/infra/db/visible_set_live_tests.rs
  • src/ingestion/tests/e2e/api/test_metric_results.py
  • src/ingestion/tests/e2e/identity/test_visible_persons.py
  • src/ingestion/tests/e2e/lib/api_coverage.py
  • src/ingestion/tests/e2e/lib/identity_stub.py

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actions

Copy link
Copy Markdown
Contributor

⚠️ Regenerate the connectors-ddl snapshot

This PR changes src/ingestion/**. If your change affects any
bronze / silver / gold schema, regenerate the committed DDL snapshot
and include it in this PR.

Prerequisites (details: src/ingestion/scripts/bootstrap-db/README.md):

  • docker + a fresh throwaway ClickHouse 25.7.5 (README "Local ClickHouse for testing")
  • .env from .env.bootstrap.example pointing at it; use the host LAN IP,
    reachable from both the host and connector containers
    (host.docker.internal does not resolve on the macOS host itself)
  • python3.12 or python3.11 on PATH (pinned dbt venv)
  • HubSpot + Salesforce credentials in .env — their discover calls the
    live APIs; without them, apply ../connectors-ddl/{hubspot,salesforce}.sql
    (relative to bootstrap-db/) to seed their bronze, then run the dbt step
cd src/ingestion/scripts/bootstrap-db
set -a; source pins.env; source .env; set +a
./bootstrap-db.sh connectors-config.yaml   # fresh ClickHouse 25.7.5
./dump-ddl.sh                              # writes scripts/connectors-ddl/*.sql

Commit the resulting scripts/connectors-ddl/*.sql diff. If nothing
changed, no snapshot update is needed. (Regeneration is manual for now.)

Comment thread src/backend/services/analytics/src/infra/identity/mod.rs
Comment thread src/backend/services/identity-resolution/src/api/visible_persons.rs
@cyberantonz

Copy link
Copy Markdown
Contributor

The queries is heavy, can we add caching for the requests?

@aleksdotbar

Copy link
Copy Markdown
Contributor Author

The queries is heavy, can we add caching for the requests?

I dont think we need it here. At least not in this PR. We can measure and see how it behaves and then decide.

An explicit COLLATE alone fails when the connection negotiates a charset
other than utf8mb4 ("COLLATION 'utf8mb4_unicode_ci' is not valid for
CHARACTER SET 'utf8mb3'"). CONVERT first, so matching follows the
column's own charset and collation whatever the session carries.

Signed-off-by: Aleksandr Barkhatov <pm@aleks.bar>
…ts-authz-gate

Signed-off-by: Aleksandr Barkhatov <pm@aleks.bar>

# Conflicts:
#	src/backend/services/analytics/src/api/metric_results.rs
#	src/backend/services/analytics/src/domain/metric_results/validation.rs
@aleksdotbar
aleksdotbar enabled auto-merge July 31, 2026 08:04
@aleksdotbar
aleksdotbar added this pull request to the merge queue Jul 31, 2026
Merged via the queue into main with commit e97d7fd Jul 31, 2026
50 checks passed
@aleksdotbar
aleksdotbar deleted the fix/2035-metric-results-authz-gate branch July 31, 2026 08:47
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.

Any logged-in user can view any employee's personal dashboard by changing the email in the URL

2 participants