XWiki Platform: Any user with editing rights can access password properties through Database List Properties

@@ -26,18 +26,29 @@
 
 import javax.inject.Inject;
 import javax.inject.Named;
+import javax.inject.Provider;
 import javax.inject.Singleton;
 
+import org.apache.commons.lang3.StringUtils;
 import org.xwiki.component.annotation.Component;
+import org.xwiki.mail.GeneralMailConfiguration;
+import org.xwiki.model.reference.DocumentReference;
+import org.xwiki.model.reference.DocumentReferenceResolver;
+import org.xwiki.model.reference.WikiReference;
 import org.xwiki.query.Query;
 import org.xwiki.query.QueryBuilder;
 import org.xwiki.query.QueryException;
 import org.xwiki.query.QueryFilter;
 import org.xwiki.query.QueryManager;
-import org.xwiki.text.StringUtils;
 
+import com.xpn.xwiki.XWikiContext;
+import com.xpn.xwiki.XWikiException;
+import com.xpn.xwiki.objects.PropertyInterface;
+import com.xpn.xwiki.objects.classes.BaseClass;
 import com.xpn.xwiki.objects.classes.DBListClass;
 import com.xpn.xwiki.objects.classes.DBTreeListClass;
+import com.xpn.xwiki.objects.classes.EmailClass;
+import com.xpn.xwiki.objects.classes.PasswordClass;
 
 /**
  * Builds a query from the meta data of a Database List property.
@@ -69,6 +80,8 @@ private static final class DBListQuerySpec
         private boolean hasValueField;
 
         private boolean hasParentField;
+
+        private BaseClass xClass;
     }
 
     private static final String DOC_PREFIX = "doc.";
@@ -88,6 +101,15 @@ private static final class DBListQuerySpec
     @Named("viewableAllowedDBListPropertyValue")
     private QueryFilter viewableValueFilter;
 
+    @Inject
+    private Provider<XWikiContext> contextProvider;
+
+    @Inject
+    private GeneralMailConfiguration mailConfiguration;
+
+    @Inject
+    private DocumentReferenceResolver<String> documentReferenceResolver;
+
     /**
      * {@inheritDoc} The query is constructed according to the following rules:
      * <ul>
@@ -148,22 +170,58 @@ private Query build(DBListQuerySpec spec) throws QueryException
         return query;
     }
 
-    private void addFieldToQuery(String fieldName, String fieldAlias, boolean hasClassName, List<String> selectClause,
-        List<String> fromClause, List<String> whereClause, Map<String, Object> parameters)
+    private void addFieldToQuery(String fieldName, String fieldAlias, DBListQuerySpec spec, List<String> selectClause,
+        List<String> fromClause, List<String> whereClause, Map<String, Object> parameters) throws QueryException
     {
         if (fieldName.startsWith(DOC_PREFIX) || fieldName.startsWith(OBJ_PREFIX)) {
+            checkSimpleFieldName(fieldName);
             selectClause.add(fieldName);
-        } else if (!hasClassName) {
+        } else if (!spec.hasClassName) {
+            checkSimpleFieldName(fieldName);
             selectClause.add(DOC_PREFIX + fieldName);
         } else {
-            selectClause.add(fieldAlias + ".value");
-            fromClause.add("StringProperty as " + fieldAlias);
-            whereClause.add(String.format("obj.id = %1$s.id.id and %1$s.id.name = :%1$s", fieldAlias));
-            parameters.put(fieldAlias, fieldName);
+            try {
+                // Only load the XClass when really needed.
+                if (spec.xClass == null) {
+                    // Resolve the reference relative to the specified wiki.
+                    DocumentReference classReference = this.documentReferenceResolver.resolve(spec.className,
+                        new WikiReference(spec.wiki));
+                    XWikiContext xWikiContext = this.contextProvider.get();
+                    spec.xClass = xWikiContext.getWiki().getXClass(classReference, xWikiContext);
+                }
+
+                PropertyInterface propertyInterface = spec.xClass.get(fieldName);
+                if (propertyInterface instanceof PasswordClass) {
+                    throw new QueryException("Queries for password field [%s] on class [%s] aren't allowed"
+                        .formatted(fieldName, spec.className), null);
+                } else if (propertyInterface instanceof EmailClass && this.mailConfiguration.shouldObfuscate()) {
+                    throw new QueryException(("Queries for email property [%s] on class [%s] aren't allowed as email "
+                        + "obfuscation is enabled.").formatted(fieldName, spec.className), null);
+                }
+                selectClause.add(fieldAlias + ".value");
+                fromClause.add("StringProperty as " + fieldAlias);
+                whereClause.add(String.format("obj.id = %1$s.id.id and %1$s.id.name = :%1$s", fieldAlias));
+                parameters.put(fieldAlias, fieldName);
+            } catch (XWikiException e) {
+                throw new QueryException("Failed to get the XClass definition", null, e);
+            }
+        }
+    }
+
+    private void checkSimpleFieldName(String fieldName) throws QueryException
+    {
+        // Ensure that the passed string is really a (single) database field and nothing else.
+        if (!StringUtils.isAlphanumeric(
+            // Remove whitespace at both ends and a document or object prefix. We don't care about having both
+            // prefixes after each other, this is not about catching all invalid names.
+            StringUtils.removeStart(StringUtils.removeStart(fieldName, StringUtils.strip(DOC_PREFIX)), OBJ_PREFIX)))
+        {
+            throw new QueryException("Invalid field name [%s]".formatted(fieldName), null);
         }
     }
 
     private String getStatementWhenIdOrValueFieldsAreSpecified(DBListQuerySpec spec, Map<String, Object> parameters)
+        throws QueryException
     {
         // Make sure we always have an id field. Ignore the value field if it duplicates the id field and there's no
         // parent field specified.
@@ -184,6 +242,7 @@ private String getStatementWhenIdOrValueFieldsAreSpecified(DBListQuerySpec spec,
     }
 
     private String getStatementWhenIdFieldIsSpecified(DBListQuerySpec spec, Map<String, Object> parameters)
+        throws QueryException
     {
         List<String> selectClause = new ArrayList<>();
         List<String> fromClause = new ArrayList<>();
@@ -207,16 +266,16 @@ private String getStatementWhenIdFieldIsSpecified(DBListQuerySpec spec, Map<Stri
             }
         }
 
-        addFieldToQuery(spec.idField, "idProp", spec.hasClassName, selectClause, fromClause, whereClause, parameters);
+        addFieldToQuery(spec.idField, "idProp", spec, selectClause, fromClause, whereClause, parameters);
 
         if (spec.hasValueField) {
-            addFieldToQuery(spec.valueField, "valueProp", spec.hasClassName, selectClause, fromClause, whereClause,
+            addFieldToQuery(spec.valueField, "valueProp", spec, selectClause, fromClause, whereClause,
                 parameters);
 
             // We cannot include the parent field if there's no value field because we would confuse it with the value
             // field (the second column in the result set is reserved for the value).
             if (spec.hasParentField) {
-                addFieldToQuery(spec.parentField, "parentProp", spec.hasClassName, selectClause, fromClause,
+                addFieldToQuery(spec.parentField, "parentProp", spec, selectClause, fromClause,
                     whereClause, parameters);
             }
         }

@@ -19,21 +19,32 @@
  */
 package com.xpn.xwiki.internal.objects.classes;
 
-import org.junit.Before;
-import org.junit.Rule;
-import org.junit.Test;
+import javax.inject.Named;
+
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.ValueSource;
+import org.xwiki.mail.GeneralMailConfiguration;
 import org.xwiki.model.reference.DocumentReference;
 import org.xwiki.query.Query;
-import org.xwiki.query.QueryBuilder;
+import org.xwiki.query.QueryException;
 import org.xwiki.query.QueryFilter;
 import org.xwiki.query.QueryManager;
-import org.xwiki.test.mockito.MockitoComponentMockingRule;
+import org.xwiki.test.junit5.mockito.InjectMockComponents;
+import org.xwiki.test.junit5.mockito.MockComponent;
 
 import com.xpn.xwiki.doc.XWikiDocument;
 import com.xpn.xwiki.objects.classes.DBListClass;
 import com.xpn.xwiki.objects.classes.DBTreeListClass;
-
-import static org.junit.Assert.assertSame;
+import com.xpn.xwiki.test.MockitoOldcore;
+import com.xpn.xwiki.test.junit5.mockito.InjectMockitoOldcore;
+import com.xpn.xwiki.test.junit5.mockito.OldcoreTest;
+import com.xpn.xwiki.test.reference.ReferenceComponentList;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertSame;
+import static org.junit.jupiter.api.Assertions.assertThrows;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.verify;
 import static org.mockito.Mockito.when;
@@ -44,24 +55,32 @@
  * @version $Id$
  * @since 9.8RC1
  */
-public class ImplicitlyAllowedValuesDBListQueryBuilderTest
+@OldcoreTest
+@ReferenceComponentList
+@SuppressWarnings("checkstyle:MultipleStringLiterals")
+class ImplicitlyAllowedValuesDBListQueryBuilderTest
 {
-    @Rule
-    public MockitoComponentMockingRule<QueryBuilder<DBListClass>> mocker =
-        new MockitoComponentMockingRule<QueryBuilder<DBListClass>>(ImplicitlyAllowedValuesDBListQueryBuilder.class);
+    @InjectMockComponents
+    private ImplicitlyAllowedValuesDBListQueryBuilder implicitlyAllowedValuesDBListQueryBuilder;
 
+    @MockComponent
     private QueryManager queryManager;
 
+    @MockComponent
+    @Named("viewableAllowedDBListPropertyValue")
     private QueryFilter viewableValueFilter;
 
+    @MockComponent
+    private GeneralMailConfiguration mailConfiguration;
+
+    @InjectMockitoOldcore
+    private MockitoOldcore oldcore;
+
     private DBListClass dbListClass = new DBListClass();
 
-    @Before
-    public void configure() throws Exception
+    @BeforeEach
+    void configure()
     {
-        this.queryManager = this.mocker.getInstance(QueryManager.class);
-        this.viewableValueFilter = this.mocker.getInstance(QueryFilter.class, "viewableAllowedDBListPropertyValue");
-
         XWikiDocument ownerDocument = mock(XWikiDocument.class);
         when(ownerDocument.getDocumentReference()).thenReturn(new DocumentReference("tests", "Some", "Page"));
         this.dbListClass.setOwnerDocument(ownerDocument);
@@ -72,12 +91,12 @@ private Query assertQuery(String statement) throws Exception
         Query query = mock(Query.class);
         when(this.queryManager.createQuery(statement, Query.HQL)).thenReturn(query);
 
-        assertSame(query, this.mocker.getComponentUnderTest().build(this.dbListClass));
+        assertSame(query, this.implicitlyAllowedValuesDBListQueryBuilder.build(this.dbListClass));
         return query;
     }
 
     @Test
-    public void buildDefaultQuery() throws Exception
+    void buildDefaultQuery() throws Exception
     {
         Query query = assertQuery("select doc.name from XWikiDocument doc where 1 = 0");
 
@@ -86,7 +105,7 @@ public void buildDefaultQuery() throws Exception
     }
 
     @Test
-    public void buildWithClassName() throws Exception
+    void buildWithClassName() throws Exception
     {
         this.dbListClass.setClassname("Blog.CategoryClass");
 
@@ -98,7 +117,7 @@ public void buildWithClassName() throws Exception
     }
 
     @Test
-    public void buildWithId() throws Exception
+    void buildWithId() throws Exception
     {
         this.dbListClass.setIdField("doc.name");
         assertQuery("select distinct doc.fullName as unfilterable0, doc.name from XWikiDocument as doc");
@@ -112,7 +131,7 @@ public void buildWithId() throws Exception
     }
 
     @Test
-    public void buildWithValue() throws Exception
+    void buildWithValue() throws Exception
     {
         this.dbListClass.setValueField("doc.name");
         assertQuery("select distinct doc.fullName as unfilterable0, doc.name from XWikiDocument as doc");
@@ -126,7 +145,7 @@ public void buildWithValue() throws Exception
     }
 
     @Test
-    public void buildWithIdAndClassName() throws Exception
+    void buildWithIdAndClassName() throws Exception
     {
         this.dbListClass.setClassname("XWiki.XWikiUsers");
         this.dbListClass.setIdField("doc.name");
@@ -150,7 +169,7 @@ public void buildWithIdAndClassName() throws Exception
     }
 
     @Test
-    public void buildWithIdAndValue() throws Exception
+    void buildWithIdAndValue() throws Exception
     {
         this.dbListClass.setIdField("doc.name");
         this.dbListClass.setValueField("doc.name");
@@ -200,7 +219,7 @@ public void buildWithIdAndValue() throws Exception
     }
 
     @Test
-    public void buildWithIdValueAndClassName() throws Exception
+    void buildWithIdValueAndClassName() throws Exception
     {
         this.dbListClass.setClassname("XWiki.TagClass");
         this.dbListClass.setIdField("doc.name");
@@ -275,7 +294,7 @@ public void buildWithIdValueAndClassName() throws Exception
     }
 
     @Test
-    public void buildWithParent() throws Exception
+    void buildWithParent() throws Exception
     {
         DBTreeListClass dbTreeListClass = new DBTreeListClass();
         dbTreeListClass.setOwnerDocument(this.dbListClass.getOwnerDocument());
@@ -292,7 +311,7 @@ public void buildWithParent() throws Exception
         this.dbListClass.setValueField("title");
         assertQuery("select distinct doc.fullName as unfilterable0, doc.title, doc.title, doc.parent"
             + " from XWikiDocument as doc");
-        
+
         this.dbListClass.setIdField("title");
         assertQuery("select distinct doc.fullName as unfilterable0, doc.title, doc.title, doc.parent"
             + " from XWikiDocument as doc");
@@ -309,4 +328,71 @@ public void buildWithParent() throws Exception
             + " obj.id = valueProp.id.id and valueProp.id.name = :valueProp and obj.id = parentProp.id.id and"
             + " parentProp.id.name = :parentProp");
     }
+
+    @ParameterizedTest
+    @ValueSource(strings = { "doc.invalid, other", "foo, bar", "obj.a, b" })
+    void buildWithInvalidId(String field)
+    {
+        this.dbListClass.setIdField(field);
+        QueryException queryException = assertThrows(QueryException.class, () -> assertQuery(null));
+        assertEquals("Invalid field name [%s]".formatted(field), queryException.getMessage());
+        this.dbListClass.setIdField("");
+
+        this.dbListClass.setValueField(field);
+        queryException = assertThrows(QueryException.class, () -> assertQuery(null));
+        assertEquals("Invalid field name [%s]".formatted(field), queryException.getMessage());
+    }
+
+    @Test
+    void buildWithPasswordField() throws Exception
+    {
+        DocumentReference classReference = new DocumentReference("tests", "Space", "XClass");
+        XWikiDocument classDocument =
+            this.oldcore.getSpyXWiki().getDocument(classReference, this.oldcore.getXWikiContext());
+        String fieldName = "passwordField";
+        classDocument.getXClass().addPasswordField(fieldName, "My Password", 10);
+        this.oldcore.getSpyXWiki().saveDocument(classDocument, "Add password field", this.oldcore.getXWikiContext());
+
+        this.dbListClass.setIdField(fieldName);
+        this.dbListClass.setClassname("Space.XClass");
+        QueryException queryException = assertThrows(QueryException.class, () -> assertQuery(null));
+
+        assertEquals("Queries for password field [passwordField] on class [Space.XClass] aren't allowed",
+            queryException.getMessage());
+    }
+
+    @ParameterizedTest
+    @ValueSource(booleans = { true, false })
+    void buildWithEmailField(boolean obfuscate) throws Exception
+    {
+        when(this.mailConfiguration.shouldObfuscate()).thenReturn(obfuscate);
+
+        DocumentReference classReference = new DocumentReference("tests", "Space", "XClass");
+        XWikiDocument classDocument =
+            this.oldcore.getSpyXWiki().getDocument(classReference, this.oldcore.getXWikiContext());
+        String fieldName = "emailField";
+        classDocument.getXClass().addEmailField(fieldName, "My Email", 10);
+        this.oldcore.getSpyXWiki().saveDocument(classDocument, "Add email field", this.oldcore.getXWikiContext());
+
+        this.dbListClass.setIdField(fieldName);
+        this.dbListClass.setClassname("Space.XClass");
+
+        if (obfuscate) {
+            QueryException queryException = assertThrows(QueryException.class, () -> assertQuery(null));
+            assertEquals(
+                "Queries for email property [emailField] on class [Space.XClass] aren't allowed as email"
+                    + " obfuscation is enabled.",
+                queryException.getMessage());
+        } else {
+            Query query =
+                assertQuery(
+                    "select distinct doc.fullName as unfilterable0, idProp.value from XWikiDocument as doc, "
+                        + "BaseObject as obj, StringProperty as idProp where doc.fullName = obj.name and "
+                        + "obj.className = :className and doc.fullName <> :templateName and obj.id = idProp.id.id "
+                        + "and idProp.id.name = :idProp");
+            verify(query).bindValue("className", "Space.XClass");
+            verify(query).bindValue("templateName", "Space.XTemplate");
+            verify(query).bindValue("idProp", fieldName);
+        }
+    }
 }