feat(context-memory): enforce read-path visibility on memory endpoints#28289
Conversation
The ContextMemory schema (open-metadata#28224) ships MemoryVisibility (PRIVATE/SHARED/ ENTITY) and shareConfig.sharedWith, but the CRUD resource didn't enforce them — any non-admin user could read another user's PRIVATE memory via GET /v1/contextCenter/memories/{id} or pick them out of /memories list results. Wire visibility into list/get/getByName via a small ContextMemoryVisibility helper: - list: drop non-visible memories from the response data - get / getByName: 403 (ForbiddenException) when not visible to the caller - Visibility rule: admin → all, owner → own memory, ENTITY → everyone, SHARED → only principals listed in sharedWith (with team/domain expansion), PRIVATE / null shareConfig → owner only
github-actions
Bot
added
Ingestion
safe to test
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
| ResultList<ContextMemory> memories = | ||
| addHref( | ||
| uriInfo, | ||
| securityContext, | ||
| fieldsParam, | ||
| new ListFilter(include), | ||
| limitParam, | ||
| before, | ||
| after)); | ||
| listInternal( | ||
| uriInfo, | ||
| securityContext, | ||
| fieldsParam, | ||
| new ListFilter(include), | ||
| limitParam, | ||
| before, | ||
| after)); | ||
| List<ContextMemory> visible = | ||
| ContextMemoryVisibility.filterByVisibility(memories.getData(), securityContext); | ||
| if (visible.size() == memories.getData().size()) { | ||
| return memories; |
There was a problem hiding this comment.
⚠️ Bug: Post-pagination filtering breaks paging contract
The list endpoint fetches a page of limitParam results from the database, then filters out non-visible memories. This causes two problems:
- Short pages: If a user requests
limit=10and 4 of 10 results are filtered, the client receives only 6 items despite more visible items existing in subsequent pages. - Lost paging cursors: When filtered results differ from the original, the code returns
new ResultList<>(visible)which setspagingtonull, so the client has noafter/beforecursor to continue pagination.
This means non-admin users may see incomplete data or be unable to paginate at all when many memories in a page are not visible to them.
Consider pushing visibility filtering into the database query (e.g., a predicate on the ListFilter) or implementing a loop that fetches additional pages until limitParam visible items are collected. At minimum, preserve the original paging cursors when returning a filtered result so clients can still advance.
Preserve paging cursors when returning filtered results so clients can still paginate. This is a minimal fix; a proper solution would push filtering into the query layer.:
ResultList<ContextMemory> memories =
addHref(
uriInfo,
listInternal(
uriInfo,
securityContext,
fieldsParam,
new ListFilter(include),
limitParam,
before,
after));
List<ContextMemory> visible =
ContextMemoryVisibility.filterByVisibility(memories.getData(), securityContext);
if (visible.size() == memories.getData().size()) {
return memories;
}
return new ResultList<>(visible, memories.getPaging());
Was this helpful? React with 👍 / 👎
There was a problem hiding this comment.
Fixed in fe1a8b9 — switched the list endpoint to mutate the original ResultList via setData(filtered) so paging cursors (before/after), total, errors and warnings are preserved instead of being nulled by new ResultList<>(visible).
The "short page" sub-issue (e.g. limit=10 returning 6 visible) is a real but separate problem — fixing it cleanly needs the visibility predicate pushed into the SQL ListFilter (or a loop-and-refill at the resource). Leaving that as a follow-up; preserving paging is the minimum fix for this PR so cursor pagination is no longer broken for non-admin callers.
There was a problem hiding this comment.
The referenced commit fe1a8b9f8a doesn't appear to be in this branch — only 6fa00661 is present — and the code at line 154 still reads return new ResultList<>(visible). So the paging-cursor fix hasn't landed yet.
The approach you described (mutating the original ResultList via setData(filtered)) is the right minimal fix: it preserves paging.before/paging.after, total, errors, and warnings while replacing only data with the visibility-filtered list. That change is a one-liner swap of line 154:
memories.setData(visible);
return memories;Would you like me to apply that now?
Code Review
|
| Compact |
|
Was this helpful? React with 👍 / 👎 | Gitar
There was a problem hiding this comment.
Pull request overview
This PR enforces ContextMemory read-path visibility rules in the OSS ContextMemoryResource so non-admin users cannot read other users’ PRIVATE (or non-shared) memories via GET/LIST endpoints.
Changes:
- Added
ContextMemoryVisibilityhelper to decide whether a memory is visible to the caller (admin/owner/entity/shared rules). - Applied visibility enforcement to
GET /{id}andGET /name/{fqn}(403 when not visible) and filteredLISTresults to only visible memories. - Added unit tests for core visibility/ownership scenarios.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| openmetadata-service/src/main/java/org/openmetadata/service/resources/context/ContextMemoryResource.java | Filters list responses by visibility and enforces visibility checks on get/getByName. |
| openmetadata-service/src/main/java/org/openmetadata/service/resources/context/ContextMemoryVisibility.java | Implements the visibility rules + sharedWith team/domain expansion lookup. |
| openmetadata-service/src/test/java/org/openmetadata/service/resources/context/ContextMemoryVisibilityTest.java | Adds unit tests for the new visibility helper behavior. |
Comments suppressed due to low confidence (1)
openmetadata-service/src/main/java/org/openmetadata/service/resources/context/ContextMemoryVisibility.java:96
filterByVisibility(memories, SecurityContext)returns the unfiltered list when the security context/principal is missing. If this helper is ever called in a code path where authentication is optional/disabled (or a null context is passed inadvertently), it would leak PRIVATE memories in list responses. Consider failing closed (throw) or returning an empty list when the principal is unavailable.
public static List<ContextMemory> filterByVisibility(
List<ContextMemory> memories, SecurityContext securityContext) {
if (memories == null || memories.isEmpty()) {
return memories;
}
if (securityContext == null || securityContext.getUserPrincipal() == null) {
return memories;
}
SubjectContext subject = DefaultAuthorizer.getSubjectContext(securityContext);
return filterByVisibility(
memories, securityContext.getUserPrincipal().getName(), subject.isAdmin());
}
|
🟡 Playwright Results — all passed (12 flaky)✅ 4137 passed · ❌ 0 failed · 🟡 12 flaky · ⏭️ 89 skipped
🟡 12 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 |
#28289) The ContextMemory schema (#28224) ships MemoryVisibility (PRIVATE/SHARED/ ENTITY) and shareConfig.sharedWith, but the CRUD resource didn't enforce them — any non-admin user could read another user's PRIVATE memory via GET /v1/contextCenter/memories/{id} or pick them out of /memories list results. Wire visibility into list/get/getByName via a small ContextMemoryVisibility helper: - list: drop non-visible memories from the response data - get / getByName: 403 (ForbiddenException) when not visible to the caller - Visibility rule: admin → all, owner → own memory, ENTITY → everyone, SHARED → only principals listed in sharedWith (with team/domain expansion), PRIVATE / null shareConfig → owner only
Summary
The ContextMemory schema (added in #28224) ships
MemoryVisibility(PRIVATE / SHARED / ENTITY) andshareConfig.sharedWith, but the OSS CRUD resource doesn't enforce them — any non-admin user can today read another user's PRIVATE memory viaGET /v1/contextCenter/memories/{id}or pick it out of the/memorieslist response. This PR wires visibility enforcement directly into the OSS resource so the data-model contract matches actual runtime behavior.listdrops non-visible memories from the response dataget/getByNamereturn 403 (ForbiddenException) when not visible to the callerENTITY→ everyone,SHARED→ only principals insharedWith(with team/domain expansion),PRIVATE/ nullshareConfig→ owner onlyVisibility logic lives in a small
ContextMemoryVisibilityhelper alongside the resource, with 11 unit tests covering each rule and admin/owner/sharedWith/team-expansion edge cases.Why this lives in OSS
The original Collate-side implementation enforced this via a
ContainerResponseFiltermounted on top of the OSS resource path — an unusual, scary seam that future devs editing the OSS resource would easily miss. SinceMemoryVisibilityandshareConfigare already part of the OSS schema (openmetadata-spec/.../contextMemory.json), the enforcement belongs here next to the data model. Once this lands, the Collate filter goes away.Test plan
mvn -pl openmetadata-service test -Dtest=ContextMemoryVisibilityTest→ 11/11 passmvn -pl openmetadata-service test -Dtest=ContextMemoryStatusTransitionTest→ 4/4 still passmvn -pl openmetadata-service spotless:checkclean🤖 Generated with Claude Code