Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -135,16 +135,23 @@ public ResultList<ContextMemory> list(
@QueryParam("include")
@DefaultValue("non-deleted")
Include include) {
return addHref(
uriInfo,
listInternal(
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;
Comment on lines +138 to +152
Copy link
Copy Markdown

@gitar-bot gitar-bot Bot May 20, 2026

Choose a reason for hiding this comment

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

⚠️ 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:

  1. Short pages: If a user requests limit=10 and 4 of 10 results are filtered, the client receives only 6 items despite more visible items existing in subsequent pages.
  2. Lost paging cursors: When filtered results differ from the original, the code returns new ResultList<>(visible) which sets paging to null, so the client has no after/before cursor 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 👍 / 👎

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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?

}
return new ResultList<>(visible);
Comment thread
pmbrull marked this conversation as resolved.
}

@GET
Expand Down Expand Up @@ -175,7 +182,9 @@ public ContextMemory get(
@QueryParam("include")
@DefaultValue("non-deleted")
Include include) {
return getInternal(uriInfo, securityContext, id, fieldsParam, include);
ContextMemory memory = getInternal(uriInfo, securityContext, id, fieldsParam, include);
ContextMemoryVisibility.enforceVisibility(memory, securityContext);
return memory;
}

@GET
Expand Down Expand Up @@ -205,7 +214,9 @@ public ContextMemory getByName(
@QueryParam("include")
@DefaultValue("non-deleted")
Include include) {
return getByNameInternal(uriInfo, securityContext, fqn, fieldsParam, include);
ContextMemory memory = getByNameInternal(uriInfo, securityContext, fqn, fieldsParam, include);
ContextMemoryVisibility.enforceVisibility(memory, securityContext);
return memory;
}

@GET
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,157 @@
/*
* Copyright 2024 Collate
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
* http://www.apache.org/licenses/LICENSE-2.0
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

package org.openmetadata.service.resources.context;

import static org.openmetadata.common.utils.CommonUtil.listOrEmpty;

import jakarta.ws.rs.ForbiddenException;
import jakarta.ws.rs.core.SecurityContext;
import java.util.HashSet;
import java.util.List;
import java.util.Set;
import org.openmetadata.schema.entity.context.ContextMemory;
import org.openmetadata.schema.entity.context.MemoryVisibility;
import org.openmetadata.schema.entity.teams.User;
import org.openmetadata.schema.type.EntityReference;
import org.openmetadata.schema.type.Include;
import org.openmetadata.service.Entity;
import org.openmetadata.service.security.DefaultAuthorizer;
import org.openmetadata.service.security.policyevaluator.SubjectContext;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

/**
* Visibility rules for {@link ContextMemory}. Every read on {@code /v1/contextCenter/memories} runs
* through this check so a non-admin user cannot read another user's PRIVATE memory via the public
* API. Visibility is independent of the OSS policy/authorizer model because it is driven by the
* per-memory {@code shareConfig} (visibility + sharedWith) rather than role/policy.
*/
public final class ContextMemoryVisibility {

private static final Logger LOG = LoggerFactory.getLogger(ContextMemoryVisibility.class);

private ContextMemoryVisibility() {}

public static boolean isVisibleToUser(ContextMemory memory, String userName, boolean isAdmin) {
if (isAdmin) {
return true;
}
if (isOwnedBy(memory, userName)) {
return true;
}
if (memory.getShareConfig() == null) {
return false;
}
MemoryVisibility visibility = memory.getShareConfig().getVisibility();
if (visibility == MemoryVisibility.ENTITY) {
return true;
}
if (visibility == MemoryVisibility.SHARED) {
return isInSharedWithList(memory, userName);
}
return false;
}

public static void enforceVisibility(ContextMemory memory, String userName, boolean isAdmin) {
if (!isVisibleToUser(memory, userName, isAdmin)) {
throw new ForbiddenException(getVisibilityDeniedMessage(memory));
}
}

public static void enforceVisibility(ContextMemory memory, SecurityContext securityContext) {
if (memory == null || securityContext == null || securityContext.getUserPrincipal() == null) {
return;
}
SubjectContext subject = DefaultAuthorizer.getSubjectContext(securityContext);
enforceVisibility(memory, securityContext.getUserPrincipal().getName(), subject.isAdmin());
}
Comment thread
pmbrull marked this conversation as resolved.

public static List<ContextMemory> filterByVisibility(
List<ContextMemory> memories, String userName, boolean isAdmin) {
return memories.stream().filter(m -> isVisibleToUser(m, userName, isAdmin)).toList();
}

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());
}

public static boolean isOwnedBy(ContextMemory memory, String userName) {
if (memory.getOwners() == null || memory.getOwners().isEmpty() || userName == null) {
return false;
}
return memory.getOwners().stream()
.anyMatch(o -> userName.equals(o.getName()) || userName.equals(o.getFullyQualifiedName()));
}

private static boolean isInSharedWithList(ContextMemory memory, String userName) {
if (memory.getShareConfig() == null || memory.getShareConfig().getSharedWith() == null) {
return false;
}
Set<String> principalIds = resolvePrincipalIdentifiers(userName);
return memory.getShareConfig().getSharedWith().stream()
.anyMatch(
sp ->
sp.getPrincipal() != null
&& (principalIds.contains(sp.getPrincipal().getName())
|| principalIds.contains(sp.getPrincipal().getFullyQualifiedName())));
}

private static Set<String> resolvePrincipalIdentifiers(String userName) {
Set<String> ids = new HashSet<>();
ids.add(userName);
try {
User user =
Entity.getEntityByName(Entity.USER, userName, "teams,domains", Include.NON_DELETED);
addRefNames(ids, user.getTeams());
addRefNames(ids, user.getDomains());
} catch (Exception e) {
LOG.debug("Could not resolve teams/domains for user '{}'", userName, e);
}
return ids;
}
Comment thread
pmbrull marked this conversation as resolved.

private static void addRefNames(Set<String> ids, List<EntityReference> refs) {
for (EntityReference ref : listOrEmpty(refs)) {
if (ref.getName() != null) {
ids.add(ref.getName());
}
if (ref.getFullyQualifiedName() != null) {
ids.add(ref.getFullyQualifiedName());
}
}
}

private static String getVisibilityDeniedMessage(ContextMemory memory) {
if (memory.getShareConfig() == null) {
return "Not authorized to access this memory.";
}
MemoryVisibility visibility = memory.getShareConfig().getVisibility();
if (visibility == null || visibility == MemoryVisibility.PRIVATE) {
return "Memory with visibility PRIVATE is only accessible to its owner.";
}
if (visibility == MemoryVisibility.SHARED) {
return "Memory with visibility SHARED is only accessible to explicitly shared users.";
}
return "Not authorized to access this memory.";
}
}
Loading
Loading