fix: harden entity pagination, multi-owner authz, and cursor charset#29750
Open
mohityadav766 wants to merge 6 commits into
Open
fix: harden entity pagination, multi-owner authz, and cursor charset#29750mohityadav766 wants to merge 6 commits into
mohityadav766 wants to merge 6 commits into
Conversation
- EntityRepository.listAfter/listBefore: guard empty result pages so a held cursor whose remaining rows were concurrently deleted returns an empty page instead of throwing IndexOutOfBounds (HTTP 500). - SubjectContext.isTeamAsset: evaluate ALL owners rather than returning inside the first loop iteration. A multi-owner asset [user, teamB] with a teamB policy no longer wrongly denies members of teamB. - RestUtil.decodeCursor: decode Base64 cursor bytes as UTF-8 to match encodeCursor, fixing pagination-cursor corruption for non-ASCII entity names on JVMs whose default charset is not UTF-8. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
❌ PR checklist incompleteThis PR cannot be merged until the following are addressed on its linked issue:
The fields live on the linked issue in the Shipping project (open the issue → right sidebar → Projects). After you set them, re-run this check (or push a commit) — issue/project changes do not re-trigger it automatically. Maintainers can bypass this check by adding the |
github-actions
Bot
added
backend
safe to test
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+172
to
185
| private boolean isOwnerUnderTeam(EntityReference owner, String parentTeam) { | ||
| boolean result = false; | ||
| if (owner.getType().equals(Entity.USER)) { | ||
| result = getSubjectContext(owner.getName()).isUserUnderTeam(parentTeam); | ||
| } else if (owner.getType().equals(Entity.TEAM)) { | ||
| try { | ||
| Team team = Entity.getEntity(Entity.TEAM, owner.getId(), TEAM_FIELDS, Include.NON_DELETED); | ||
| result = isInTeam(parentTeam, team.getEntityReference()); | ||
| } catch (Exception ex) { | ||
| // Team could not be resolved (e.g. deleted); treat as not under the team. | ||
| } | ||
| } | ||
| return result; | ||
| } |
Contributor
There was a problem hiding this comment.
When a multi-owner asset has an unresolved or deleted user before a valid team owner, this branch lets getSubjectContext(owner.getName()) throw before the loop can check the later owner. matchTeam() then returns an error or denial even though the asset is owned by a matching team, so the new all-owner OR behavior is still skipped for that owner ordering.
Suggested change
| private boolean isOwnerUnderTeam(EntityReference owner, String parentTeam) { | |
| boolean result = false; | |
| if (owner.getType().equals(Entity.USER)) { | |
| result = getSubjectContext(owner.getName()).isUserUnderTeam(parentTeam); | |
| } else if (owner.getType().equals(Entity.TEAM)) { | |
| try { | |
| Team team = Entity.getEntity(Entity.TEAM, owner.getId(), TEAM_FIELDS, Include.NON_DELETED); | |
| result = isInTeam(parentTeam, team.getEntityReference()); | |
| } catch (Exception ex) { | |
| // Team could not be resolved (e.g. deleted); treat as not under the team. | |
| } | |
| } | |
| return result; | |
| } | |
| private boolean isOwnerUnderTeam(EntityReference owner, String parentTeam) { | |
| boolean result = false; | |
| if (owner != null && Entity.USER.equals(owner.getType())) { | |
| try { | |
| result = getSubjectContext(owner.getName()).isUserUnderTeam(parentTeam); | |
| } catch (Exception ex) { | |
| // User could not be resolved (e.g. deleted); treat as not under the team. | |
| } | |
| } else if (owner != null && Entity.TEAM.equals(owner.getType())) { | |
| try { | |
| Team team = Entity.getEntity(Entity.TEAM, owner.getId(), TEAM_FIELDS, Include.NON_DELETED); | |
| result = isInTeam(parentTeam, team.getEntityReference()); | |
| } catch (Exception ex) { | |
| // Team could not be resolved (e.g. deleted); treat as not under the team. | |
| } | |
| } | |
| return result; | |
| } |
Context Used: CLAUDE.md (source)
Contributor
There was a problem hiding this comment.
Pull request overview
This PR applies three backend correctness hardening fixes across pagination, authorization evaluation for multi-owner assets, and cursor charset handling to prevent server errors and access-denial regressions.
Changes:
- Hardened keyset pagination cursors in
EntityRepository.listAfter/listBeforeto avoid cursor computation on empty result pages. - Fixed
SubjectContext.isTeamAssetto evaluate all owners (OR semantics) instead of returning after the first owner. - Made
RestUtil.decodeCursordecode Base64 cursor bytes as UTF-8 to matchencodeCursor.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| openmetadata-service/src/main/java/org/openmetadata/service/util/RestUtil.java | Ensures cursor decoding uses UTF-8 to round-trip non-ASCII cursor values reliably. |
| openmetadata-service/src/main/java/org/openmetadata/service/security/policyevaluator/SubjectContext.java | Corrects team-ownership checks for multi-owner assets by evaluating all owners. |
| openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/EntityRepository.java | Adds guards for empty pagination pages to prevent cursor computation exceptions and HTTP 500s. |
Comment on lines
2612
to
2616
| if (entities.size() | ||
| > limitParam) { // If extra result exists, then previous page exists - return before cursor | ||
| entities.remove(0); | ||
| beforeCursor = getCursorValue(entities.get(0)); | ||
| beforeCursor = getCursorValue(entities.getFirst()); | ||
| } |
Comment on lines
119
to
123
| public static String decodeCursor(String cursor) { | ||
| return cursor == null || cursor.isEmpty() | ||
| ? null | ||
| : new String(Base64.getUrlDecoder().decode(cursor)); | ||
| : new String(Base64.getUrlDecoder().decode(cursor), StandardCharsets.UTF_8); | ||
| } |
Comment on lines
+160
to
+169
| /** Returns true if any of the resource owners is under the team hierarchy of parentTeam */ | ||
| public boolean isTeamAsset(String parentTeam, List<EntityReference> owners) { | ||
| for (EntityReference owner : owners) { | ||
| if (owner.getType().equals(Entity.USER)) { | ||
| SubjectContext subjectContext = getSubjectContext(owner.getName()); | ||
| return subjectContext.isUserUnderTeam(parentTeam); | ||
| } else if (owner.getType().equals(Entity.TEAM)) { | ||
| try { | ||
| Team team = | ||
| Entity.getEntity(Entity.TEAM, owner.getId(), TEAM_FIELDS, Include.NON_DELETED); | ||
| return isInTeam(parentTeam, team.getEntityReference()); | ||
| } catch (Exception ex) { | ||
| // Ignore and return false | ||
| } | ||
| boolean isUnderTeam = false; | ||
| for (EntityReference owner : listOrEmpty(owners)) { | ||
| if (isOwnerUnderTeam(owner, parentTeam)) { | ||
| isUnderTeam = true; | ||
| break; | ||
| } | ||
| } | ||
| return false; | ||
| return isUnderTeam; |
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
Comment on lines
+172
to
185
| private boolean isOwnerUnderTeam(EntityReference owner, String parentTeam) { | ||
| boolean result = false; | ||
| if (owner.getType().equals(Entity.USER)) { | ||
| result = getSubjectContext(owner.getName()).isUserUnderTeam(parentTeam); | ||
| } else if (owner.getType().equals(Entity.TEAM)) { | ||
| try { | ||
| Team team = Entity.getEntity(Entity.TEAM, owner.getId(), TEAM_FIELDS, Include.NON_DELETED); | ||
| result = isInTeam(parentTeam, team.getEntityReference()); | ||
| } catch (Exception ex) { | ||
| // Team could not be resolved (e.g. deleted); treat as not under the team. | ||
| } | ||
| } | ||
| return result; | ||
| } |
There was a problem hiding this comment.
⚠️ Bug: Unresolved user owner aborts multi-owner team check
The intent of this change is to evaluate ALL owners so that a multi-owner asset such as [user, teamB] correctly grants access to teamB members via a matchTeam(teamB) policy. However the USER branch of isOwnerUnderTeam calls getSubjectContext(owner.getName()).isUserUnderTeam(parentTeam) without any try/catch, unlike the TEAM branch which swallows exceptions.
getSubjectContext -> SubjectCache.getUserContext throws (EntityNotFoundException) when the user cannot be resolved — e.g. a deleted/renamed user still referenced as an owner. When the unresolved user is the FIRST owner in the list, that exception propagates out of isOwnerUnderTeam and isTeamAsset entirely, so the later teamB owner is never evaluated. The scan aborts and teamB members are wrongly denied — reintroducing exactly the fail-closed regression this PR set out to fix.
Wrap the USER branch in the same defensive try/catch as the TEAM branch (or handle the exception inside the owner loop) so one unresolvable owner does not short-circuit evaluation of the remaining owners.
Wrap both owner branches so an unresolvable owner does not abort the multi-owner scan.:
private boolean isOwnerUnderTeam(EntityReference owner, String parentTeam) {
boolean result = false;
try {
if (owner.getType().equals(Entity.USER)) {
result = getSubjectContext(owner.getName()).isUserUnderTeam(parentTeam);
} else if (owner.getType().equals(Entity.TEAM)) {
Team team = Entity.getEntity(Entity.TEAM, owner.getId(), TEAM_FIELDS, Include.NON_DELETED);
result = isInTeam(parentTeam, team.getEntityReference());
}
} catch (Exception ex) {
// Owner could not be resolved (e.g. deleted user/team); treat as not under the team.
}
return result;
}
- Apply fix
Check the box to apply the fix or reply for a change | Was this helpful? React with 👍 / 👎
Contributor
🟡 Playwright Results — all passed (30 flaky)✅ 4490 passed · ❌ 0 failed · 🟡 30 flaky · ⏭️ 37 skipped
🟡 30 flaky test(s) (passed on retry)
How to debug locally# Download playwright-test-results-<shard> artifact and unzip
npx playwright show-trace path/to/trace.zip # view trace |
…ead leak - ElasticSearchBulkSink: guard the async bulk() submit like the OpenSearch twin — a synchronous throw (dead transport) previously leaked the concurrency permit and the activeBulkRequests slot, eventually starving the pipeline and hanging close(). On throw we now record the failure and, if no retry is scheduled, decrement + release. - ApplicationContext: ConcurrentHashMap for the apps map — registerApp() runs on every Quartz execution while request/event threads iterate getAllApps()/getApp(), which threw ConcurrentModificationException / torn reads on a plain HashMap. - AppScheduler: retain the error-trigger-reset pool in a field, make its thread a daemon, and shutdownNow() it in shutDown(); it was a non-daemon local that survived shutdown and blocked JVM exit (and leaked on re-initialize). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ption scheduleAtFixedRate cancels a periodic task permanently if it throws (ScheduledExecutorService contract), so an unchecked exception from flushInternal silently disabled periodic flushing — trailing partial buffers then only shipped on an explicit flush/close. Wrap the flushIfNeeded body in try/catch so a failure logs and the task keeps running on the next interval (both ES and OS sinks). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Comment on lines
2612
to
2616
| if (entities.size() | ||
| > limitParam) { // If extra result exists, then previous page exists - return before cursor | ||
| entities.remove(0); | ||
| beforeCursor = getCursorValue(entities.get(0)); | ||
| beforeCursor = getCursorValue(entities.getFirst()); | ||
| } |
…isabled Postgres SQL - AbstractEventConsumer: advance the subscription cursor to the true maximum change_event.offset read (via the offset-returning listWithOffset DAO) instead of by row count. change_event.offset is a non-contiguous AUTO_INCREMENT, so advancing by count re-read the tail across gaps (rolled-back txns / retention deletes) and redelivered those events (duplicate webhook/alert delivery). - ListFilter.getDisabledCondition (Postgres branch): fix malformed SQL — ::boolean (not :boolean), balanced parentheses, and drop the undefined `c.` alias (use json ->> 'disabled'). The branch was dormant (no caller wires a `disabled` param today) but threw a syntax error the moment one did. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…IOException The permit-leak guard caught IOException, which was never imported (and the ES async client does not throw a checked IOException synchronously). Catch Exception — handleBulkFailure takes a Throwable — so a synchronous RuntimeException from the async submit is handled and the permit / activeBulkRequests slot are released. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Code Review
|
| Compact |
|
Was this helpful? React with 👍 / 👎 | Gitar
|
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
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.
Three targeted correctness fixes surfaced during a downstream backend review, each traced end-to-end.
EntityRepository.listAfter/listBefore— guard empty result pages. A client holding a valid cursor whose remaining rows were concurrently deleted hitentities.get(0)/get(size-1)on an empty list →IndexOutOfBoundsException→ HTTP 500. Now returns an empty page with null cursors.SubjectContext.isTeamAsset— evaluate all owners instead of returning inside the first loop iteration. A multi-owner asset[user, teamB](or[teamA, teamB]) with amatchTeam()policy onteamBpreviously checked only the first owner and wrongly denied teamB members access to an asset their team owns. Fail-closed regression, now fixed (OR across owners).RestUtil.decodeCursor— decode Base64 cursor bytes as UTF-8 to matchencodeCursor. On JVMs whose default charset is not UTF-8, pagination cursors for non-ASCII entity names round-tripped incorrectly.Compiles clean (
openmetadata-service).🤖 Generated with Claude Code
Summary by Gitar
lastReadOffsetinAbstractEventConsumerto ensure cursors advance by true database offset, preventing event redelivery.ElasticSearchBulkSinkandOpenSearchBulkSinkto prevent concurrent request leaks and scheduler task cancellation.ListFilterto correctly evaluate thedisabledfield for PostgreSQL boolean casting.This will update automatically on new commits.
Greptile Summary
This PR hardens several backend correctness paths. The main changes are:
Confidence Score: 5/5
This looks safe to merge.
Important Files Changed
Reviews (5): Last reviewed commit: "fix: ES bulk-sink sync-throw guard catch..." | Re-trigger Greptile
Context used (3)