Skip to content

fix(security): enforce authorization on sub-resource, listing and admin endpoints - #34838

Merged
mohityadav766 merged 13 commits into
mainfrom
fix/java-sast-issues
Oct 16, 2026
Merged

mohityadav766 merged 13 commits into
mainfrom
fix/java-sast-issues

Conversation

@mohityadav766

Copy link
Copy Markdown
Member

Describe your changes:

No linked issue: security hardening, labelled skip-pr-checks.

Several endpoints answered without the per-entity check the entity itself gets, or returned data about entities the caller may not view. This PR puts each of them behind the same authorization the entity's own endpoints apply.

  • Sub-resources authorize against their parent entity: table system profiles (ViewDataProfile), table/schema entity relationships, ingestion pipeline status history, KPI results, data contract validation (/validate, /validate/yaml, /odcs/validate/yaml), audit report cancel, governance workflow trigger and redeploy, AI governance framework fork, intake forms by entity type, lineage edges (both ends), and the root of every lineage graph and export endpoint. The automations workflow trigger is now checked per workflow type: test connection as before, reverse ingestion needs EditAll on its service, query runner is limited to the requesting user, and the Spark engine test needs Create on ingestion pipelines.
  • Asset listings and counts carry the caller's search access policies: domain, data product, glossary term, tag, team and user assets, plus the domain, data product and team asset counts. Domain asset counts under domain isolation cover only the caller's domains. Knowledge page hierarchies and the search field/source-URL lookups carry the same policies.
  • Mixed responses keep only entities the caller may view: activity feeds (all events by actor, user activity, my feed, following, mentions), lineage neighbours, column search and export, folder contents, recognizer feedback, and MCP semantic_search.
  • Votes and follows require ViewBasic on the entity, in all 26 resources that expose them. Usage endpoints build their resource context with the entity id, so entity-conditioned policies apply.
  • Admin or bot only: change events (GET /v1/events), system entity/service counts and status, raw web analytic events (GET .../collect), and search documents by id. Admin only: email template reset, reindex failures (now paged, max 1000) and the recognizer feedback review queue. Non-admins get a generic reason for an unavailable pipeline service.
  • Outbound URLs: ValidationHttpUtil validates every request, so validator fetches that follow a discovery document cannot reach metadata or private addresses. The SAML connectivity probe skips a refused URL as a warning.
  • Tokens: password reset and email verification tokens are no longer logged, and no reset token is stored when SMTP is disabled. Page views are recorded for the caller, whatever user id the payload names.
  • RBACConditionEvaluator no longer shares one SpEL evaluation context across concurrent callers.
  • GET /v1/reports lists through the policy-filtered listing. This also fixes the endpoint failing with HV000151.

Type of change:

  • Bug fix

High-level design:

The fixes reuse three existing mechanisms rather than adding new authorization concepts.

  1. Parent-entity checks. Sub-resource endpoints call authorizer.authorize(...) with the parent's ResourceContext, built by id or name with the right Include. Conditional policies (tags, owners, domains) therefore see the real entity.
  2. Search-side policies. Asset listings, counts, hierarchy and lookup queries AND in the caller's compiled search access policies (RBACConditionEvaluator) through the new package-private ElasticRbacQueries and OpenSearchRbacQueries. These fail closed when the policies compile to nothing, and are skipped for admins or when search access control is off. SubjectContext is threaded through SearchRepository, the search and aggregation managers, and InheritedFieldQuery.
  3. Post-filtering mixed responses. The new ViewPermissionFilter buckets references by type and batch-loads each bucket with exactly the fields authorization reads. It hands the authorizer pre-resolved ResourceContexts, hydrates tags lazily, and treats anything but a clear allow as hidden. LineagePermissionFilter now delegates to it.

Compatibility: existing repository and search-client signatures stay and delegate with a null caller, so downstream callers compile unchanged. SearchClient gets default caller-aware listPageHierarchy* overloads.

Behaviour changes integrators may notice: the admin/bot-only endpoints listed above, the generic pipeline-service reason for non-admins, no reset token without SMTP, and page views bound to the caller. The Health Check page that reads /v1/system/status is already admin-only in the UI.

Not in this PR: pruning denied nodes from the search-based lineage graph endpoints (their root is checked here), and /v1/columns/grid.

Tests:

Use cases covered

  • A user denied ViewBasic on a table, unconditionally or through a tag-conditioned deny policy, gets 403 on its system profiles, entity relationships, lineage edge and lineage graph, and data contract validation. The same applies to KPI results, pipeline status history and intake forms.
  • Cancelling an audit report, triggering or redeploying a governance workflow, and forking a framework need EditAll/Create. Query runner triggers work for the requester only; reverse ingestion works for the service owner only.
  • Asset listings and counts leave out assets the caller's search policies hide. A domain-restricted user sees counts only for their own domains.
  • Activity feeds by actor, user, following and mentions drop events on tables the caller cannot view; admins still see every event.
  • Folder contents hide files not shared with the caller; column search leaves out denied tables; MCP semantic search drops denied hits.
  • Votes and follows need view access; usage reads apply entity-conditioned policies.
  • Each admin-only endpoint answers 403 to a data consumer and stays available to admins, and to bots where listed.
  • Requests to metadata or private address literals are refused before sending; the SAML probe never contacts them.
  • Reset tokens never reach the log; no token is stored when SMTP is off.
  • A page view naming another user is recorded for the caller.
  • A caller whose policies compile while another caller's compilation is in flight keeps its own identity.

Unit tests

  • New: BasicAuthenticatorTokenTest.
  • Updated: IngestionPipelineResourceStatusTest, ValidationHttpUtilTest, SamlValidatorTest, SemanticSearchToolTest, ElasticSearchFieldQueryTest, OpenSearchFieldQueryTest, OpenSearchRBACConditionEvaluatorTest, and signature updates in SearchRepositoryBehaviorTest, ElasticSearchFieldlessQueryTest and OpenSearchFieldlessQueryTest.
  • The evaluator test was confirmed to fail against the previous RBACConditionEvaluator.
  • Coverage on changed classes was not measured; most changed classes are resource endpoints exercised by the ITs below, which are not instrumented.
  • Local run on this branch, rebased on main: every unit test class that references a changed class. That is 207 service classes (3046 tests) and 7 MCP classes (155 tests): 0 failures, 1 skipped.

Backend integration tests

  • New: SubResourceAuthorizationIT (24 tests) and AdminOnlyEndpointsIT (8 tests).
  • Updated: FeedAccessAuthzIT (activity by actor, mentions), WorkflowTriggerPermissionsIT (query runner, reverse ingestion, Spark), IntakeFormResourceIT, FolderIT, DomainIsolationIT, and DenyPolicyPrincipals.clientDeniedWhen for tag-conditioned denies.
  • Local run on this branch (MySQL + Elasticsearch, parallel lane): 33 IT classes, 2758 tests, 0 failures, 1 error, 214 skipped. The classes are the ones above plus ActivityResourceIT, DataContractResourceIT, DataContractPermissionIT, WebAnalyticEventResourceIT, SystemResourceIT, TagRecognizerFeedbackIT, KnowledgePageHierarchyIT, LineageResourceIT, UsageResourceIT, WorkflowDefinitionResourceIT, KpiResourceIT, ColumnBulkUpdateIT, ColumnResourceIT, McpIntegrationIT, McpToolsValidationIT, SearchResourceIT, DomainResourceIT, DataProductResourceIT, TeamResourceIT, UserResourceIT, GlossaryTermResourceIT, TagResourceIT, IngestionPipelineResourceIT, WorkflowResourceIT and DocStoreResourceIT. The one error, WorkflowDefinitionResourceIT.test_NewApprovalRunSupersedesStalePendingTask (an approval task not superseded within 60 s), passed when the class ran alone on the same commit (59 tests, 0 failures). This PR changes no workflow execution code.

Ingestion integration tests

  • Not applicable (no ingestion changes).

Playwright (UI) tests

  • Not applicable (no UI changes).

make playwright_affected maps these backend files to about 190 specs and reports impact-map gaps, so the merge queue runs the full suite.

Not run locally: no UI change, and the planner selects about 190 specs plus impact-map gaps (full suite in the merge queue).

Manual testing performed

No manual UI testing. The ITs run each scenario end to end against a real server, MySQL and Elasticsearch, using a data consumer, deny-policy principals and bots.

I also checked the OpenMetadata and Collate UIs for calls to the endpoints that became admin/bot-only. The only caller is the admin-only Health Check page.

UI screen recording / screenshots:

Not applicable.

Checklist:

  • I have read the CONTRIBUTING document.
  • My PR title is Fixes <issue-number>: <short explanation>. No issue, see above.
  • My PR is linked to a GitHub issue via Fixes #<issue-number> above. No issue, see above.
  • I have commented on my code, particularly in hard-to-understand areas.
  • For JSON Schema changes: not applicable (no schema change).
  • For UI changes: not applicable.
  • I have added tests (unit / integration / Playwright as applicable) and listed them above.
  • I have added a test that covers the exact scenario we are fixing.

🤖 Generated with Claude Code

…in endpoints

Several endpoints answered without the per-entity check the entity itself
gets, or returned data about entities the caller may not view.

- Sub-resource reads and actions now authorize against their parent entity:
  system profiles, entity relationships, pipeline statuses, KPI results,
  data contract validation, audit cancel, workflow trigger and redeploy,
  framework fork, intake forms, lineage edges and lineage graph roots, and
  the automations workflow trigger (checked per workflow type).
- Asset listings and counts (domain, data product, glossary term, tag,
  team, user), knowledge page hierarchies and field/source-URL search
  lookups carry the caller's search access policies.
- Responses that mix entities are filtered to those the caller may view:
  activity feeds, lineage neighbours, column search/export, folder
  contents, recognizer feedback, MCP semantic search. ViewPermissionFilter
  batch-loads entities per type for this and fails closed.
- Votes and follows require view access; usage checks carry the entity id.
- Deployment and installation-wide endpoints are limited to admins (and
  bots where an integration reads them): email template reset, reindex
  failures, change events, system counts and status, raw web analytics,
  the recognizer feedback queue, search documents by id. Non-admins get a
  generic pipeline service status reason.
- ValidationHttpUtil validates every outbound URL, so validator fetches
  that follow a discovery document cannot reach metadata or private
  addresses; the SAML connectivity probe skips refused URLs.
- Password reset and verification tokens are no longer logged, and a reset
  token is not stored when no mail can be sent. Page views are recorded
  for the caller.
- RBACConditionEvaluator no longer shares one evaluation context across
  concurrent callers.
- Reports list through the policy-filtered listing; this also fixes
  GET /v1/reports failing with HV000151.
The mentions feed, added on main after the other activity feeds were filtered, returned change events on any entity a reply mentioned the caller under. It now goes through the same per-entity view filter.
@github-actions

github-actions Bot commented Oct 7, 2026 •

Copy link
Copy Markdown
Contributor

✅ SSO Login Nightly — PASSED

View workflow run · Commit e69a084

Provider Status Pass Fail Flaky Skip Duration
basic ✅ pass 28 0 0 0 6m 59s
ldap ✅ pass 7 0 0 0 0m 55s
keycloak-oidc-confidential ✅ pass 12 0 0 0 1m 49s
keycloak-oidc-public ✅ pass 11 0 0 0 1m 33s
msal-mock ✅ pass 7 0 0 0 0m 37s
auth0-mock ✅ pass 6 0 0 0 0m 55s
okta ⏩ skipped - - - - -
keycloak-azure-saml ✅ pass 23 0 0 0 6m 05s
keycloak-azure-saml-crosssite ⏩ skipped - - - - -

Totals: 94 passed · 0 failed · 0 flaky · 0 skipped · duration 18m 55s

@@ -192,6 +194,22 @@ protected Response deleteFollowerInternal(
.toResponse();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We check ViewBasic when adding a follower, but not when removing one. Since deleteFollower returns a ChangeEvent containing the entity, a user who followed it before losing access can still retrieve its data. Can we apply the same view check here and add coverage for DELETE?

@mohityadav766
mohityadav766 added this pull request to the merge queue Oct 8, 2026
@mohityadav766
mohityadav766 removed this pull request from the merge queue due to a manual request Oct 8, 2026
@github-actions

github-actions Bot commented Oct 8, 2026

Copy link
Copy Markdown
Contributor

🚦 Removed from the merge queue — manual (2026-10-08T02:40:54Z)

The entry left the queue before it was built, so no checks ran against it.

# Conflicts:
#	openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/ColumnRepository.java
#	openmetadata-service/src/main/java/org/openmetadata/service/lineage/LineagePermissionFilter.java
…ollow

Search lineage roots its graph on the indexed entity with the requested
FQN and never uses `type` to find it, so a missing, unknown or wrong type
skipped the root check while the search still ran. The check now looks the
root up the way the search does (SearchRepository.getLineageRoot) and
authorizes ViewBasic on that entity, falling back to the named type only
when the index does not have the root yet.

Removing a follower returns the entity, so it now needs the same ViewBasic
check as adding one.
totalFound, returnedCount and the page messages were computed before
unviewable hits were dropped, and hiddenCount reported how many were
dropped. Hits are now filtered before anything is counted; only the
cursor advances past the dropped ones, resuming right after the last
viewable hit a budget-trimmed page kept.
@github-actions

Copy link
Copy Markdown
Contributor

🚦 Removed from the merge queue — merge_conflict (2026-10-15T04:41:54Z)

The entry left the queue before it was built, so no checks ran against it.

Applies cleanly to the latest main; it conflicted only with an entry ahead in the queue. Re-queue once that entry has merged or left the queue.

@github-actions

Copy link
Copy Markdown
Contributor

🚦 Removed from the merge queue — merge_conflict (2026-10-15T15:32:47Z)

The entry left the queue before it was built, so no checks ran against it.

Conflicts with the latest main — rebase, then re-queue.

# Conflicts:
#	openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/DomainRepository.java
# Conflicts:
#	openmetadata-service/src/main/java/org/openmetadata/service/resources/tags/TagResource.java
@gitar-bot

gitar-bot Bot commented Oct 15, 2026

Copy link
Copy Markdown
Code Review 👍 Approved with suggestions 0 closed / 1 findings

🔴 High risk · Changes authorization across listings, feeds, lineage, and admin endpoints; incorrect policy enforcement could expose restricted catalog data or block legitimate access.

Adds authorization checks across sub-resource, listing, count, feed, lineage, vote/follow and admin endpoints, reusing parent-entity checks, search-side access policies and a new post-filter for mixed responses. Approved with one suggestion: SemanticSearchTool.viewableHits authorizes each hit individually, causing an N+1 of entity loads (up to 50 per call). Batching the hit references through ViewPermissionFilter.viewableIds once would remove it.

💡 Performance: Semantic search authorizes hits one by one, an N+1 of entity loads

📄 openmetadata-mcp/src/main/java/org/openmetadata/mcp/tools/SemanticSearchTool.java:274-276 📄 openmetadata-mcp/src/main/java/org/openmetadata/mcp/tools/SemanticSearchTool.java:329-337 📄 openmetadata-service/src/main/java/org/openmetadata/service/security/ViewPermissionFilter.java:35-39 📄 openmetadata-service/src/main/java/org/openmetadata/service/security/ViewPermissionFilter.java:64-69 📄 openmetadata-service/src/main/java/org/openmetadata/service/security/ViewPermissionFilter.java:149-152

viewableHits calls isViewable for each ranked hit. Each call builds a new ViewPermissionFilter and goes through canView, which gives the authorizer a lazy ResourceContext. So every hit costs its own entity load plus policy evaluation, up to 50 per semantic_search call. ViewPermissionFilter's own javadoc says it batches per type to avoid exactly this N+1, but that only happens in viewableIds. Fix: collect the hits' references (entityType + parentId), call viewableIds once, and filter by the returned id set. Hits with no parseable parentId can still fall back to canView.

🤖 Prompt for agents
Code Review: Adds authorization checks across sub-resource, listing, count, feed, lineage, vote/follow and admin endpoints, reusing parent-entity checks, search-side access policies and a new post-filter for mixed responses. Approved with one suggestion: `SemanticSearchTool.viewableHits` authorizes each hit individually, causing an N+1 of entity loads (up to 50 per call). Batching the hit references through `ViewPermissionFilter.viewableIds` once would remove it.

1. 💡 Performance: Semantic search authorizes hits one by one, an N+1 of entity loads
   Files: openmetadata-mcp/src/main/java/org/openmetadata/mcp/tools/SemanticSearchTool.java:274-276, openmetadata-mcp/src/main/java/org/openmetadata/mcp/tools/SemanticSearchTool.java:329-337, openmetadata-service/src/main/java/org/openmetadata/service/security/ViewPermissionFilter.java:35-39, openmetadata-service/src/main/java/org/openmetadata/service/security/ViewPermissionFilter.java:64-69, openmetadata-service/src/main/java/org/openmetadata/service/security/ViewPermissionFilter.java:149-152

   `viewableHits` calls `isViewable` for each ranked hit. Each call builds a new `ViewPermissionFilter` and goes through `canView`, which gives the authorizer a lazy `ResourceContext`. So every hit costs its own entity load plus policy evaluation, up to 50 per `semantic_search` call. `ViewPermissionFilter`'s own javadoc says it batches per type to avoid exactly this N+1, but that only happens in `viewableIds`. Fix: collect the hits' references (entityType + parentId), call `viewableIds` once, and filter by the returned id set. Hits with no parseable parentId can still fall back to `canView`.

Verification

Rules No rules evaluated

Functional validation Not enabled

Set up

Auto-approval Not enabled

Set up

For more verification details, comment gitar display:verbose.

Options

Display: compact → Showing compact verification results.

Comment with these commands to change the behavior for this request:

Compact
gitar display:verbose         

Was this helpful? React with 👍 / 👎 | Powered by Gitar — free for open source

@sonarqubecloud

Copy link
Copy Markdown

This branch had an error being deployed

1 failed deployment
test — 0e3fe9b4 Deployed Oct 15, 2026 by mohityadav766 via RDF Playwright execution #33179
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backend safe to test Add this label to run secure Github workflows on PRs skip-pr-checks Bypass PR metadata validation check

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants