diff --git a/CHANGELOG.md b/CHANGELOG.md index 1828035d85..8991e7f74c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,6 +18,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), - Fix audit log writing errors for rollover-enabled alias indices ([#5878](https://github.com/opensearch-project/security/pull/5878) - Fix the issue of unprocessed X-Request-Id ([#5954](https://github.com/opensearch-project/security/pull/5954)) +- Improve DLS error message to identify undefined user attributes when query substitution fails ([#5975](https://github.com/opensearch-project/security/pull/5975)) ### Refactoring ### Maintenance diff --git a/src/integrationTest/java/org/opensearch/security/privileges/dlsfls/DocumentPrivilegesTest.java b/src/integrationTest/java/org/opensearch/security/privileges/dlsfls/DocumentPrivilegesTest.java index 7182b22ed6..87a8ca60d8 100644 --- a/src/integrationTest/java/org/opensearch/security/privileges/dlsfls/DocumentPrivilegesTest.java +++ b/src/integrationTest/java/org/opensearch/security/privileges/dlsfls/DocumentPrivilegesTest.java @@ -64,6 +64,7 @@ import org.opensearch.test.framework.TestSecurityConfig; import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.containsString; import static org.opensearch.security.util.MockIndexMetadataBuilder.dataStreams; import static org.opensearch.security.util.MockIndexMetadataBuilder.indices; import static org.junit.Assert.assertEquals; @@ -1175,6 +1176,33 @@ public void invalidTemplatedQuery() throws Exception { .evaluate(MockPrivilegeEvaluationContextBuilder.ctx().get()); } + @Test + public void invalidTemplatedQuery_errorMessageIdentifiesUndefinedAttribute() throws Exception { + try { + DocumentPrivileges.DlsQuery.create("{\"terms\":{\"arr\":[${attr.jwt.array}]}}", xContentRegistry) + .evaluate(MockPrivilegeEvaluationContextBuilder.ctx().attr("attr.jwt.other", "x").get()); + fail("Expected PrivilegesEvaluationException"); + } catch (PrivilegesEvaluationException e) { + assertThat(e.getMessage(), containsString("attr.jwt.array")); + assertThat(e.getMessage(), containsString("attr.jwt.other")); + } + } + + @Test + public void invalidTemplatedQuery_errorMessageTruncatesWhenMoreThanTenAvailableAttributes() throws Exception { + MockPrivilegeEvaluationContextBuilder ctx = MockPrivilegeEvaluationContextBuilder.ctx(); + for (int i = 1; i <= 12; i++) { + ctx.attr("attr.jwt.attr" + i, "v" + i); + } + try { + DocumentPrivileges.DlsQuery.create("{\"term\":{\"dept\":\"${attr.jwt.missing}\"}}", xContentRegistry).evaluate(ctx.get()); + fail("Expected PrivilegesEvaluationException"); + } catch (PrivilegesEvaluationException e) { + assertThat(e.getMessage(), containsString("attr.jwt.missing")); + assertThat(e.getMessage(), containsString("... and 2 more")); + } + } + @Test public void equals() throws Exception { DocumentPrivileges.DlsQuery query1a = DocumentPrivileges.DlsQuery.create( diff --git a/src/main/java/org/opensearch/security/privileges/UserAttributes.java b/src/main/java/org/opensearch/security/privileges/UserAttributes.java index f1a8acf148..14655ea8f5 100644 --- a/src/main/java/org/opensearch/security/privileges/UserAttributes.java +++ b/src/main/java/org/opensearch/security/privileges/UserAttributes.java @@ -10,8 +10,13 @@ */ package org.opensearch.security.privileges; +import java.util.ArrayList; +import java.util.Collections; +import java.util.List; import java.util.Map; import java.util.Set; +import java.util.regex.Matcher; +import java.util.regex.Pattern; import com.google.common.base.Joiner; import com.google.common.collect.Iterables; @@ -25,10 +30,25 @@ * This code was moved over from ConfigModelV7. */ public class UserAttributes { + private static final Pattern UNRESOLVED_ATTRIBUTE_PATTERN = Pattern.compile("\\$\\{([^}]+)\\}"); + public static boolean needsAttributeSubstitution(String patternString) { return patternString.contains("${"); } + /** + * Returns the names of all unresolved ${...} attribute references remaining + * in {@code s} after substitution has been performed. + */ + public static List findUnresolvedAttributes(String s) { + List result = new ArrayList<>(); + Matcher matcher = UNRESOLVED_ATTRIBUTE_PATTERN.matcher(s); + while (matcher.find()) { + result.add(matcher.group(1)); + } + return Collections.unmodifiableList(result); + } + public static String replaceProperties(String orig, PrivilegesEvaluationContext context) { User user = context.getUser(); diff --git a/src/main/java/org/opensearch/security/privileges/dlsfls/DocumentPrivileges.java b/src/main/java/org/opensearch/security/privileges/dlsfls/DocumentPrivileges.java index bb4563517e..37548f3c8a 100644 --- a/src/main/java/org/opensearch/security/privileges/dlsfls/DocumentPrivileges.java +++ b/src/main/java/org/opensearch/security/privileges/dlsfls/DocumentPrivileges.java @@ -16,6 +16,8 @@ import java.util.List; import java.util.Map; import java.util.Objects; +import java.util.Set; +import java.util.stream.Collectors; import org.apache.logging.log4j.util.Strings; @@ -46,6 +48,8 @@ * Instances of this class are managed by DlsFlsProcessedConfig. */ public class DocumentPrivileges extends AbstractRuleBasedPrivileges { + private static final int MAX_ATTRIBUTES_IN_ERROR_MESSAGE = 10; + private final NamedXContentRegistry xContentRegistry; public DocumentPrivileges( @@ -176,8 +180,16 @@ static class Dynamic extends DlsQuery { RenderedDlsQuery evaluate(PrivilegesEvaluationContext context) throws PrivilegesEvaluationException { String effectiveQueryString = UserAttributes.replaceProperties(this.queryString, context); if (UserAttributes.needsAttributeSubstitution(effectiveQueryString)) { + List unresolved = UserAttributes.findUnresolvedAttributes(effectiveQueryString); + Set available = context.getUser().getCustomAttributesMap().keySet(); + String availableStr = available.size() > MAX_ATTRIBUTES_IN_ERROR_MESSAGE + ? available.stream().limit(MAX_ATTRIBUTES_IN_ERROR_MESSAGE).collect(Collectors.joining(", ")) + + " ... and " + + (available.size() - MAX_ATTRIBUTES_IN_ERROR_MESSAGE) + + " more" + : String.join(", ", available); throw new PrivilegesEvaluationException( - "Invalid DLS query: " + effectiveQueryString, + "DLS query references undefined user attributes: " + unresolved + ". Available attributes are: " + availableStr, new OpenSearchSecurityException("User attribute substitution failed") ); } diff --git a/src/test/java/org/opensearch/security/privileges/UserAttributesUnitTest.java b/src/test/java/org/opensearch/security/privileges/UserAttributesUnitTest.java index 05da8d5419..665fcc3c3f 100644 --- a/src/test/java/org/opensearch/security/privileges/UserAttributesUnitTest.java +++ b/src/test/java/org/opensearch/security/privileges/UserAttributesUnitTest.java @@ -25,9 +25,11 @@ public class UserAttributesUnitTest { @Test public void testNeedsAttributeSubstitution() { - assertTrue(UserAttributes.needsAttributeSubstitution("{\"foo\": \"${user.name}}\"")); + assertTrue(UserAttributes.needsAttributeSubstitution(""" + {"foo": "${user.name}}"}""")); assertTrue(UserAttributes.needsAttributeSubstitution("${attr1.proxy.foo}")); - assertFalse(UserAttributes.needsAttributeSubstitution("{\"foo\": \"bar\"}")); + assertFalse(UserAttributes.needsAttributeSubstitution(""" + {"foo": "bar"}""")); } @Test @@ -71,4 +73,34 @@ public void testReplaceProperties() { """; assertEquals(expectedString, UserAttributes.replaceProperties(stringWithPlaceholders, ctx)); } + + @Test + public void testFindUnresolvedAttributes_noTokens() { + assertTrue(UserAttributes.findUnresolvedAttributes(""" + {"term": {"dept": "value"}}""").isEmpty()); + } + + @Test + public void testFindUnresolvedAttributes_singleToken() { + assertEquals(List.of("attr.jwt.array"), UserAttributes.findUnresolvedAttributes(""" + {"terms": {"arr": [${attr.jwt.array}]}}""")); + } + + @Test + public void testFindUnresolvedAttributes_multipleTokens() { + assertEquals(List.of("attr.jwt.dept", "attr.proxy.region"), UserAttributes.findUnresolvedAttributes(""" + { + "bool": { + "must": [ + {"term": {"dept": "${attr.jwt.dept}"}}, + {"term": {"region": "${attr.proxy.region}"}} + ] + } + }""")); + } + + @Test(expected = UnsupportedOperationException.class) + public void testFindUnresolvedAttributes_returnsUnmodifiableList() { + UserAttributes.findUnresolvedAttributes("${attr.foo}").add("extra"); + } }