Skip to content
Open
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 @@ -3,6 +3,7 @@
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertFalse;
import static org.junit.jupiter.api.Assertions.assertNotNull;
import static org.junit.jupiter.api.Assertions.assertThrows;
import static org.junit.jupiter.api.Assertions.assertTrue;

import java.util.List;
Expand Down Expand Up @@ -36,6 +37,7 @@
import org.openmetadata.schema.type.MetadataOperation;
import org.openmetadata.schema.type.TagLabel;
import org.openmetadata.sdk.client.OpenMetadataClient;
import org.openmetadata.sdk.exceptions.ForbiddenException;
import org.openmetadata.sdk.fluent.Tables;
import org.openmetadata.sdk.models.ListParams;
import org.openmetadata.sdk.models.ListResponse;
Expand Down Expand Up @@ -334,4 +336,124 @@ void test_matchAnyCertification_nullEntityInResourceContext(TestNamespace ns) {
adminClient.policies().delete(policy.getId());
}
}

/**
* A tag-based DENY rule must be enforced regardless of the {@code fields} query parameter. The
* authorization entity load previously reused the caller-supplied projection, so omitting {@code
* fields=tags} left the policy engine with no tags and matchAnyTag evaluated to false, silently
* allowing the request.
*/
@Test
void test_tagDenyPolicy_enforcedWhenFieldsParamOmitted(TestNamespace ns) {
OpenMetadataClient adminClient = SdkClients.adminClient();
String p = ns.shortPrefix();

Rule denyTaggedRule =
new Rule()
.withName("DenyPiiSensitive")
.withResources(List.of("All"))
.withOperations(List.of(MetadataOperation.VIEW_ALL))
.withEffect(Rule.Effect.DENY)
.withCondition("matchAnyTag('" + PII_SENSITIVE_TAG + "')");

CreatePolicy createPolicy = new CreatePolicy();
createPolicy.setName(p + "_denyTagPol");
createPolicy.setRules(List.of(denyTaggedRule));

Policy policy = adminClient.policies().create(createPolicy);
try {
CreateRole createRole = new CreateRole();
createRole.setName(p + "_denyTagRole");
createRole.setPolicies(List.of(policy.getFullyQualifiedName()));
Role role = adminClient.roles().create(createRole);
try {
CreateTeam createTeam = new CreateTeam();
createTeam.setName(p + "_denyTagTeam");
createTeam.setTeamType(CreateTeam.TeamType.GROUP);
createTeam.setDefaultRoles(List.of(role.getId()));
Team team = adminClient.teams().create(createTeam);
try {
String userEmail = p + "_denytaguser@test.openmetadata.org";
CreateUser createUser = new CreateUser();
createUser.setName(p + "_denytaguser");
createUser.setEmail(userEmail);
createUser.setTeams(List.of(team.getId()));
User testUser = adminClient.users().create(createUser);
try {
DatabaseService dbService = DatabaseServiceTestFactory.createPostgres(ns);
try {
DatabaseSchema schema = DatabaseSchemaTestFactory.createSimple(ns, dbService);
Column column = new Column().withName("id").withDataType(ColumnDataType.INT);

TagLabel piiTag = new TagLabel();
piiTag.setTagFQN(PII_SENSITIVE_TAG);
piiTag.setSource(TagLabel.TagSource.CLASSIFICATION);
piiTag.setLabelType(TagLabel.LabelType.MANUAL);
piiTag.setState(TagLabel.State.CONFIRMED);

Table taggedTable =
Tables.create()
.name(p + "_tagged")
.inSchema(schema.getFullyQualifiedName())
.withColumns(List.of(column))
.withTags(List.of(piiTag))
.execute();
Table untaggedTable =
Tables.create()
.name(p + "_untagged")
.inSchema(schema.getFullyQualifiedName())
.withColumns(List.of(column))
.execute();

OpenMetadataClient testUserClient =
SdkClients.createClient(userEmail, userEmail, new String[] {});
String taggedId = taggedTable.getId().toString();
String taggedFqn = taggedTable.getFullyQualifiedName();

assertThrows(
ForbiddenException.class,
() -> testUserClient.tables().get(taggedId),
"GET by id without fields must be denied for a tag matched by a DENY rule");

assertThrows(
ForbiddenException.class,
() -> testUserClient.tables().getByName(taggedFqn),
"GET by name without fields must be denied for a tag matched by a DENY rule");

assertThrows(
ForbiddenException.class,
() -> testUserClient.tables().get(taggedId, "tags"),
"GET with fields=tags must remain denied");

assertThrows(
ForbiddenException.class,
() -> testUserClient.tables().get(taggedId, "owners"),
"A projection without tags must not bypass the DENY rule");

Table visible = testUserClient.tables().get(untaggedTable.getId().toString());
assertNotNull(visible, "An untagged table must remain viewable");
assertEquals(
untaggedTable.getId(),
visible.getId(),
"The DENY rule must not over-block entities without the tag");
} finally {
adminClient
.databaseServices()
.delete(
dbService.getId().toString(),
Map.of("recursive", "true", "hardDelete", "true"));
}
} finally {
adminClient.users().delete(testUser.getId());
}
} finally {
adminClient.teams().delete(team.getId());
}
} finally {
adminClient.roles().delete(role.getId());
}
} finally {
adminClient.policies().delete(policy.getId());
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -330,7 +330,7 @@ public T getInternal(
fields,
relationIncludes,
operationContext,
getResourceContextById(id, relationIncludes, fields));
getResourceContextById(id, relationIncludes));
}

public T getInternal(
Expand Down Expand Up @@ -452,7 +452,7 @@ public T getByNameInternal(
fields,
relationIncludes,
operationContext,
getResourceContextByName(name, relationIncludes, fields));
getResourceContextByName(name, relationIncludes));
}

public T getByNameInternal(
Expand Down Expand Up @@ -1200,10 +1200,9 @@ protected ResourceContext<T> getResourceContextById(UUID id, Include include) {
return new ResourceContext<>(entityType, id, null, include);
}

protected ResourceContext<T> getResourceContextById(
UUID id, RelationIncludes relationIncludes, Fields fields) {
protected ResourceContext<T> getResourceContextById(UUID id, RelationIncludes relationIncludes) {
Include include = relationIncludes == null ? Include.ALL : relationIncludes.getDefaultInclude();
return new ResourceContext<>(entityType, id, null, include, fields, relationIncludes);
return new ResourceContext<>(entityType, id, null, include, relationIncludes);
}

protected ResourceContext<T> getResourceContextByName(String name) {
Expand All @@ -1220,9 +1219,9 @@ protected ResourceContext<T> getResourceContextByName(String name, Include inclu
}

protected ResourceContext<T> getResourceContextByName(
String name, RelationIncludes relationIncludes, Fields fields) {
String name, RelationIncludes relationIncludes) {
Include include = relationIncludes == null ? Include.ALL : relationIncludes.getDefaultInclude();
return new ResourceContext<>(entityType, null, name, include, fields, relationIncludes);
return new ResourceContext<>(entityType, null, name, include, relationIncludes);
}

protected static final MetadataOperation[] VIEW_ALL_OPERATIONS = {MetadataOperation.VIEW_ALL};
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,6 @@ public class ResourceContext<T extends EntityInterface> implements ResourceConte
private T entity; // Will be lazily initialized
private ResourceContextInterface.Operation operation = ResourceContextInterface.Operation.NONE;
private Include include;
private Fields requestedFields;
private RelationIncludes relationIncludes;

public ResourceContext(@NonNull String resource) {
Expand Down Expand Up @@ -69,13 +68,11 @@ public ResourceContext(
UUID id,
String name,
Include include,
Fields requestedFields,
RelationIncludes relationIncludes) {
this.resource = resource;
this.id = id;
this.name = name;
this.include = include;
this.requestedFields = requestedFields;
this.relationIncludes = relationIncludes;
this.entityRepository = (EntityRepository<T>) Entity.getEntityRepository(resource);
}
Expand Down Expand Up @@ -195,9 +192,10 @@ private EntityInterface resolveEntity() {
fieldList = entityRepository.getPatchFields();
} else if (operation == ResourceContextInterface.Operation.PUT) {
fieldList = entityRepository.getPutFields();
} else if (requestedFields != null) {
fieldList = requestedFields;
} else {
// Authorization must never depend on the caller-supplied `fields` projection: a policy
// attribute that is not loaded reads as absent, so a conditional Deny rule silently fails
// open (e.g. matchAnyTag returns false when tags were not requested).
if (entityRepository.isSupportsOwners()) {
fields = EntityUtil.addField(fields, Entity.FIELD_OWNERS);
}
Expand Down Expand Up @@ -245,9 +243,6 @@ private Include resolveInclude() {
}

private boolean useRepositoryCache() {
if (requestedFields != null) {
return false;
}
return operation != ResourceContextInterface.Operation.PATCH
&& operation != ResourceContextInterface.Operation.PUT;
}
Expand Down
Loading