fix: preserve domain checks for feed delete authorization

[atharvasingh7007]

Summary

• preserve domain context when authorizing feed thread and post deletions
• resolve domains from the linked about entity, with fallback to stored thread domain ids
• only treat hasDomain() as a list operation when there is neither a concrete entity nor any domain context
• add regression coverage for linked-entity domains and stored thread domains

Why

FeedResource.deleteThread() and deletePost() previously built feed policy contexts from author names alone. Those contexts returned no entity and no domains, so domain-scoped policies could be skipped during delete authorization. In practice, thread and post deletes could be authorized without the domain checks that protect the linked asset.

Validation

• mvn -B -f openmetadata-service/pom.xml spotless:check
• mvn -B -f openmetadata-service/pom.xml -Dtest=FeedResourceContextTest -DfailIfNoTests=false test

[github-actions]

Hi there 👋 Thanks for your contribution!

The OpenMetadata team will review the PR shortly! Once it has been labeled as safe to test, the CI workflows
will start executing and we'll be able to make sure everything is working as expected.

Let us know if you need any help!

[github-actions]

Hi there 👋 Thanks for your contribution!

The OpenMetadata team will review the PR shortly! Once it has been labeled as safe to test, the CI workflows
will start executing and we'll be able to make sure everything is working as expected.

Let us know if you need any help!

[gitar-bot]

⚠️ Edge Case: Unhandled exception in resolveOwners prevents domain resolution

The constructor eagerly calls resolveOwners() (line 29) before resolveAboutEntity() and resolveDomains(). If Entity.getEntityReferenceByName() at line 65 throws (e.g., the thread creator was deleted), the entire ThreadResourceContext construction fails and no domain context is resolved.

Previously, owner resolution was lazy — called only when getOwners() was invoked — so a failure there didn't block domain resolution. Now, a deleted user breaks domain-based authorization for the entire thread/post delete flow.

This matters because the PR's goal is to preserve domain checks: if the thread author is deleted but the thread still references a domain-scoped asset, the authorization should still evaluate domain policies.

Suggested fix:

private static List<EntityReference> resolveOwners(Thread thread) {
    if (thread == null || thread.getCreatedBy() == null) {
      return null;
    }
    try {
      List<EntityReference> owners = new ArrayList<>();
      owners.add(
          Entity.getEntityReferenceByName(Entity.USER, thread.getCreatedBy(), Include.NON_DELETED));
      return owners;
    } catch (EntityNotFoundException e) {
      LOG.debug("Thread creator '{}' not found", thread.getCreatedBy());
      return null;
    }
}

_{Was this helpful? React with 👍 / 👎 | Reply gitar fix to apply this suggestion}

[gitar-bot]

Code Review ⚠️ Changes requested 2 resolved / 3 findings

Fixes feed delete authorization by caching resolveAboutEntity and replacing broad exception handling, but the constructor calls resolveOwners() before resolveDomains() is initialized, causing unhandled exceptions during domain resolution.

⚠️ Edge Case: Unhandled exception in resolveOwners prevents domain resolution

📄 openmetadata-service/src/main/java/org/openmetadata/service/security/policyevaluator/ThreadResourceContext.java:27-31 📄 openmetadata-service/src/main/java/org/openmetadata/service/security/policyevaluator/ThreadResourceContext.java:59-66

The constructor eagerly calls resolveOwners() (line 29) before resolveAboutEntity() and resolveDomains(). If Entity.getEntityReferenceByName() at line 65 throws (e.g., the thread creator was deleted), the entire ThreadResourceContext construction fails and no domain context is resolved.

Previously, owner resolution was lazy — called only when getOwners() was invoked — so a failure there didn't block domain resolution. Now, a deleted user breaks domain-based authorization for the entire thread/post delete flow.

This matters because the PR's goal is to preserve domain checks: if the thread author is deleted but the thread still references a domain-scoped asset, the authorization should still evaluate domain policies.

Suggested fix

private static List<EntityReference> resolveOwners(Thread thread) {
    if (thread == null || thread.getCreatedBy() == null) {
      return null;
    }
    try {
      List<EntityReference> owners = new ArrayList<>();
      owners.add(
          Entity.getEntityReferenceByName(Entity.USER, thread.getCreatedBy(), Include.NON_DELETED));
      return owners;
    } catch (EntityNotFoundException e) {
      LOG.debug("Thread creator '{}' not found", thread.getCreatedBy());
      return null;
    }
}

✅ 2 resolved

✅ Performance: resolveAboutEntity called repeatedly without caching

📄 openmetadata-service/src/main/java/org/openmetadata/service/security/policyevaluator/ThreadResourceContext.java:42 📄 openmetadata-service/src/main/java/org/openmetadata/service/security/policyevaluator/ThreadResourceContext.java:47 📄 openmetadata-service/src/main/java/org/openmetadata/service/security/policyevaluator/ThreadResourceContext.java:54-56 📄 openmetadata-service/src/main/java/org/openmetadata/service/security/policyevaluator/ThreadResourceContext.java:68-78
Both getEntity() and getDomains() in ThreadResourceContext (and transitively in PostResourceContext) call the static resolveAboutEntity(thread) which parses the EntityLink and performs a database lookup via Entity.getEntity() every time.

In RuleEvaluator.hasDomain(), a single policy evaluation can invoke getEntity() up to 3 times (lines 111, 144) and getDomains() up to 2 times (lines 112, 119). Since getDomains() → resolveDomains() also calls resolveAboutEntity() internally, this results in up to 5 redundant database queries for the same entity per authorization check.

Since ThreadResourceContext is a record, all fields are final and you can't use a simple lazy field. Consider computing and storing the resolved entity in a private field via a compact constructor, or converting the record to a class with lazy initialization.

✅ Quality: Broad catch(Exception) silently swallows errors

📄 openmetadata-service/src/main/java/org/openmetadata/service/security/policyevaluator/ThreadResourceContext.java:75-76
In resolveAboutEntity() (line 75), catch (Exception ignored) swallows all exceptions including unexpected ones like NullPointerException, IllegalStateException, or database connection failures. This makes debugging authorization failures very difficult since there's no log output.

Consider catching only EntityNotFoundException (or a narrow set of expected exceptions) and at minimum logging other exceptions at DEBUG/WARN level.

🤖 Prompt for agents

Code Review: Fixes feed delete authorization by caching resolveAboutEntity and replacing broad exception handling, but the constructor calls resolveOwners() before resolveDomains() is initialized, causing unhandled exceptions during domain resolution.

1. ⚠️ Edge Case: Unhandled exception in resolveOwners prevents domain resolution
   Files: openmetadata-service/src/main/java/org/openmetadata/service/security/policyevaluator/ThreadResourceContext.java:27-31, openmetadata-service/src/main/java/org/openmetadata/service/security/policyevaluator/ThreadResourceContext.java:59-66

   The constructor eagerly calls `resolveOwners()` (line 29) before `resolveAboutEntity()` and `resolveDomains()`. If `Entity.getEntityReferenceByName()` at line 65 throws (e.g., the thread creator was deleted), the entire `ThreadResourceContext` construction fails and no domain context is resolved.
   
   Previously, owner resolution was lazy — called only when `getOwners()` was invoked — so a failure there didn't block domain resolution. Now, a deleted user breaks domain-based authorization for the entire thread/post delete flow.
   
   This matters because the PR's goal is to preserve domain checks: if the thread author is deleted but the thread still references a domain-scoped asset, the authorization should still evaluate domain policies.

   Suggested fix:
   private static List<EntityReference> resolveOwners(Thread thread) {
       if (thread == null || thread.getCreatedBy() == null) {
         return null;
       }
       try {
         List<EntityReference> owners = new ArrayList<>();
         owners.add(
             Entity.getEntityReferenceByName(Entity.USER, thread.getCreatedBy(), Include.NON_DELETED));
         return owners;
       } catch (EntityNotFoundException e) {
         LOG.debug("Thread creator '{}' not found", thread.getCreatedBy());
         return null;
       }
   }

Options

Display: compact → Showing less information.

Comment with these commands to change:

Compact

gitar display:verbose

_{Was this helpful? React with 👍 / 👎 | Gitar}

[atharvasingh7007]

Addressed the Gitar feedback in the latest commit (034a32b):

• cache the resolved thread/about entity to avoid repeated lookups
• narrow and log unexpected resolution failures
• local validation passed with mvn -B -f openmetadata-service/pom.xml spotless:check and mvn -B -f openmetadata-service/pom.xml -Dtest=FeedResourceContextTest -DfailIfNoTests=false test

Could a maintainer please add the safe to test label so the full CI can run?