You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@jackrabbit.apache.org by "adamcin (via GitHub)" <gi...@apache.org> on 2023/09/01 01:28:47 UTC
[GitHub] [jackrabbit-oak] adamcin opened a new pull request, #1094: OAK-10424 add support for FASTSIZE query option and related JCR privilege
adamcin opened a new pull request, #1094:
URL: https://github.com/apache/jackrabbit-oak/pull/1094
- add SQL2 query option FASTSIZE
- if oak.fastQuerySize is not enabled globally, require privilege `rep:queryOptionFastSize` on repository root path
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [jackrabbit-oak] thomasmueller commented on pull request #1094: OAK-10424 add support for INSECURE RESULT SIZE and INSECURE FACETS query options and rep:insecureQueryOptions privilege
Posted by "thomasmueller (via GitHub)" <gi...@apache.org>.
thomasmueller commented on PR #1094:
URL: https://github.com/apache/jackrabbit-oak/pull/1094#issuecomment-1720784973
I'm not blocking this, but the "finally" block would be nice.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] OAK-10424 add support for INSECURE RESULT SIZE and INSECURE FACETS query options and rep:insecureQueryOptions privilege [jackrabbit-oak]
Posted by "adamcin (via GitHub)" <gi...@apache.org>.
adamcin commented on PR #1094:
URL: https://github.com/apache/jackrabbit-oak/pull/1094#issuecomment-1745396023
@anchela That's unfortunate. I guess the issue is with a new PrivilegeBits constant not being able to reflect the value of `next` in an existing privilege tree that is being upgraded, which probably would mean, for example, that upon upgrade, `rep:insecureQueryOptions` would have the same bit position as `crx:replicate`, and there would either be an error on startup, or there would just be a silent failure to treat them as distinct permissions.
https://github.com/apache/jackrabbit-oak/blob/320df1f83d0580b65aa61e95e81494f973fcdf7d/oak-core/src/main/java/org/apache/jackrabbit/oak/security/privilege/PrivilegeDefinitionWriter.java#L55-L59
Since this approach would likely work if it were defined as a custom privilege, such as `crx:insecureQueryOptions`, I can try reverting the changes under `oak-security-spi` and make the privilege name a configuration option in `QueryEngineSettingsMBean`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [jackrabbit-oak] adamcin commented on a diff in pull request #1094: OAK-10424 add support for INSECURE RESULT SIZE and INSECURE FACETS query options and rep:insecureQueryOptions privilege
Posted by "adamcin (via GitHub)" <gi...@apache.org>.
adamcin commented on code in PR #1094:
URL: https://github.com/apache/jackrabbit-oak/pull/1094#discussion_r1327273737
##########
oak-lucene/src/test/java/org/apache/jackrabbit/oak/jcr/query/ResultSizeTest.java:
##########
@@ -126,5 +151,90 @@ private void doTestResultSize(boolean union, int expected) throws RepositoryExce
System.clearProperty("oak.fastQuerySize");
}
+
+ private void doTestResultSizeOption(boolean aggregateAtQueryTime) throws RepositoryException {
+ createData();
+ int expectedForUnion = 400;
+ int expectedForTwoConditions = aggregateAtQueryTime ? 400 : 200;
+ doTestResultSizeOption(superuser, false, expectedForTwoConditions);
+ doTestResultSizeOption(superuser, true, expectedForUnion);
+ Session readOnlySession = null;
+ try {
+ readOnlySession = getHelper().getReadOnlySession();
+ assertNotNull(readOnlySession);
+ doTestResultSizeOption(readOnlySession, false, -1);
+ doTestResultSizeOption(readOnlySession, true, -1);
+ } finally {
+ if (readOnlySession != null) {
+ readOnlySession.logout();
+ }
+ }
+ }
+
+ private void doTestResultSizeOption(Session session, boolean union, int expected) throws RepositoryException {
+ QueryManager qm = session.getWorkspace().getQueryManager();
+
+ String statement;
+ if (union) {
+ statement = "select a.[jcr:path] from [nt:base] as a where contains(a.[text], 'Hello') UNION select a.[jcr:path] from [nt:base] as a where contains(a.[text], 'World')";
+ } else {
+ statement = "select a.[jcr:path] from [nt:base] as a where contains(a.[text], 'Hello World')";
+ }
+
+ Query q;
+ long result;
+ NodeIterator it;
+ StringBuilder buff;
+
+ // enabled by default now, in LuceneOakRepositoryStub. Disable global
+ System.setProperty("oak.fastQuerySize", "false");
+
+ // fast (insecure) case
+ String fastSizeResult = "";
+ q = qm.createQuery(statement + " option (insecure result size)", Query.JCR_SQL2);
+ if (expected < 0) {
+ // if expected < 0, i.e. insufficient permissions, expect a InvalidQueryException on execute().
+ try {
+ it = q.execute().getNodes();
+ fail("expected an InvalidQueryException caused by a IllegalArgumentException");
+ } catch (InvalidQueryException e) {
+ assertTrue("expected an InvalidQueryException caused by a ParseException",
+ e.getCause() instanceof IllegalArgumentException);
+ }
+ } else {
+ it = q.execute().getNodes();
+ result = it.getSize();
+ assertTrue("size: " + result + " expected around " + expected,
+ result > expected - 50 &&
+ result < expected + 50);
+ buff = new StringBuilder();
+ while (it.hasNext()) {
+ Node n = it.nextNode();
+ buff.append(n.getPath()).append('\n');
+ }
+ fastSizeResult = buff.toString();
+ q = qm.createQuery(statement + " option (insecure result size)", Query.JCR_SQL2);
+ q.setLimit(90);
+ it = q.execute().getNodes();
+ assertEquals(90, it.getSize());
+ }
+
+ // default (secure) case
+ q = qm.createQuery(statement, Query.JCR_SQL2);
+ it = q.execute().getNodes();
+ result = it.getSize();
+ assertEquals(-1, result);
+ buff = new StringBuilder();
+ while (it.hasNext()) {
+ Node n = it.nextNode();
+ buff.append(n.getPath()).append('\n');
+ }
+ String regularResult = buff.toString();
+ if (expected >= 0) {
+ assertEquals(regularResult, fastSizeResult);
+ }
+
+ System.clearProperty("oak.fastQuerySize");
Review Comment:
I used the try-finally approach since ResultSizeTest is still in pre-junit4 style. Also modified the existing test method in the same way.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [jackrabbit-oak] thomasmueller commented on a diff in pull request #1094: OAK-10424 add support for INSECURE RESULT SIZE and INSECURE FACETS query options and rep:insecureQueryOptions privilege
Posted by "thomasmueller (via GitHub)" <gi...@apache.org>.
thomasmueller commented on code in PR #1094:
URL: https://github.com/apache/jackrabbit-oak/pull/1094#discussion_r1326883609
##########
oak-lucene/src/test/java/org/apache/jackrabbit/oak/jcr/query/ResultSizeTest.java:
##########
@@ -126,5 +151,90 @@ private void doTestResultSize(boolean union, int expected) throws RepositoryExce
System.clearProperty("oak.fastQuerySize");
}
+
+ private void doTestResultSizeOption(boolean aggregateAtQueryTime) throws RepositoryException {
+ createData();
+ int expectedForUnion = 400;
+ int expectedForTwoConditions = aggregateAtQueryTime ? 400 : 200;
+ doTestResultSizeOption(superuser, false, expectedForTwoConditions);
+ doTestResultSizeOption(superuser, true, expectedForUnion);
+ Session readOnlySession = null;
+ try {
+ readOnlySession = getHelper().getReadOnlySession();
+ assertNotNull(readOnlySession);
+ doTestResultSizeOption(readOnlySession, false, -1);
+ doTestResultSizeOption(readOnlySession, true, -1);
+ } finally {
+ if (readOnlySession != null) {
+ readOnlySession.logout();
+ }
+ }
+ }
+
+ private void doTestResultSizeOption(Session session, boolean union, int expected) throws RepositoryException {
+ QueryManager qm = session.getWorkspace().getQueryManager();
+
+ String statement;
+ if (union) {
+ statement = "select a.[jcr:path] from [nt:base] as a where contains(a.[text], 'Hello') UNION select a.[jcr:path] from [nt:base] as a where contains(a.[text], 'World')";
+ } else {
+ statement = "select a.[jcr:path] from [nt:base] as a where contains(a.[text], 'Hello World')";
+ }
+
+ Query q;
+ long result;
+ NodeIterator it;
+ StringBuilder buff;
+
+ // enabled by default now, in LuceneOakRepositoryStub. Disable global
+ System.setProperty("oak.fastQuerySize", "false");
+
+ // fast (insecure) case
+ String fastSizeResult = "";
+ q = qm.createQuery(statement + " option (insecure result size)", Query.JCR_SQL2);
+ if (expected < 0) {
+ // if expected < 0, i.e. insufficient permissions, expect a InvalidQueryException on execute().
+ try {
+ it = q.execute().getNodes();
+ fail("expected an InvalidQueryException caused by a IllegalArgumentException");
+ } catch (InvalidQueryException e) {
+ assertTrue("expected an InvalidQueryException caused by a ParseException",
+ e.getCause() instanceof IllegalArgumentException);
+ }
+ } else {
+ it = q.execute().getNodes();
+ result = it.getSize();
+ assertTrue("size: " + result + " expected around " + expected,
+ result > expected - 50 &&
+ result < expected + 50);
+ buff = new StringBuilder();
+ while (it.hasNext()) {
+ Node n = it.nextNode();
+ buff.append(n.getPath()).append('\n');
+ }
+ fastSizeResult = buff.toString();
+ q = qm.createQuery(statement + " option (insecure result size)", Query.JCR_SQL2);
+ q.setLimit(90);
+ it = q.execute().getNodes();
+ assertEquals(90, it.getSize());
+ }
+
+ // default (secure) case
+ q = qm.createQuery(statement, Query.JCR_SQL2);
+ it = q.execute().getNodes();
+ result = it.getSize();
+ assertEquals(-1, result);
+ buff = new StringBuilder();
+ while (it.hasNext()) {
+ Node n = it.nextNode();
+ buff.append(n.getPath()).append('\n');
+ }
+ String regularResult = buff.toString();
+ if (expected >= 0) {
+ assertEquals(regularResult, fastSizeResult);
+ }
+
+ System.clearProperty("oak.fastQuerySize");
Review Comment:
This should be inside a "finally" block, so that if the test fails, other tests are not affected.
##########
oak-doc/src/site/markdown/query/query-engine.md:
##########
@@ -267,6 +269,22 @@ Limitations:
you need to also set the property `refresh` to `true` (Boolean),
so that the change is applied. No indexing is required.
+#### Query Option Insecure Result Size
+
+`@since Oak 1.60.0 (OAK-10424)`
+
+NOTE: The principal executing the query must have been granted the repository privilege `rep:insecureQueryOptions` (see [Privilege Management / Query Execution](../security/privilege/mappingtoprivileges.md#query-execution)).
+
+Enabling this option activates the same Compatibility behavior for NodeIterator.getSize() as described in [Result Size](#result-size), but only for the query being executed.
Review Comment:
Typo(?): lowercase "compatibility"
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [jackrabbit-oak] thomasmueller commented on pull request #1094: OAK-10424 add support for INSECURE RESULT SIZE and INSECURE FACETS query options and rep:insecureQueryOptions privilege
Posted by "thomasmueller (via GitHub)" <gi...@apache.org>.
thomasmueller commented on PR #1094:
URL: https://github.com/apache/jackrabbit-oak/pull/1094#issuecomment-1725148719
We discussed this in the team today, and came to the conclusion that we also need somebody from the security component to review the change, before we can deploy it. I'm sorry!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [jackrabbit-oak] nfsantos commented on a diff in pull request #1094: OAK-10424 add support for INSECURE RESULT SIZE and INSECURE FACETS query options and rep:insecureQueryOptions privilege
Posted by "nfsantos (via GitHub)" <gi...@apache.org>.
nfsantos commented on code in PR #1094:
URL: https://github.com/apache/jackrabbit-oak/pull/1094#discussion_r1329183290
##########
oak-lucene/src/test/java/org/apache/jackrabbit/oak/jcr/query/ResultSizeTest.java:
##########
@@ -126,5 +151,90 @@ private void doTestResultSize(boolean union, int expected) throws RepositoryExce
System.clearProperty("oak.fastQuerySize");
}
+
+ private void doTestResultSizeOption(boolean aggregateAtQueryTime) throws RepositoryException {
+ createData();
+ int expectedForUnion = 400;
+ int expectedForTwoConditions = aggregateAtQueryTime ? 400 : 200;
+ doTestResultSizeOption(superuser, false, expectedForTwoConditions);
+ doTestResultSizeOption(superuser, true, expectedForUnion);
+ Session readOnlySession = null;
+ try {
+ readOnlySession = getHelper().getReadOnlySession();
+ assertNotNull(readOnlySession);
+ doTestResultSizeOption(readOnlySession, false, -1);
+ doTestResultSizeOption(readOnlySession, true, -1);
+ } finally {
+ if (readOnlySession != null) {
+ readOnlySession.logout();
+ }
+ }
+ }
+
+ private void doTestResultSizeOption(Session session, boolean union, int expected) throws RepositoryException {
+ QueryManager qm = session.getWorkspace().getQueryManager();
+
+ String statement;
+ if (union) {
+ statement = "select a.[jcr:path] from [nt:base] as a where contains(a.[text], 'Hello') UNION select a.[jcr:path] from [nt:base] as a where contains(a.[text], 'World')";
+ } else {
+ statement = "select a.[jcr:path] from [nt:base] as a where contains(a.[text], 'Hello World')";
+ }
+
+ Query q;
+ long result;
+ NodeIterator it;
+ StringBuilder buff;
+
+ // enabled by default now, in LuceneOakRepositoryStub. Disable global
+ System.setProperty("oak.fastQuerySize", "false");
+
+ // fast (insecure) case
+ String fastSizeResult = "";
+ q = qm.createQuery(statement + " option (insecure result size)", Query.JCR_SQL2);
+ if (expected < 0) {
+ // if expected < 0, i.e. insufficient permissions, expect a InvalidQueryException on execute().
+ try {
+ it = q.execute().getNodes();
+ fail("expected an InvalidQueryException caused by a IllegalArgumentException");
+ } catch (InvalidQueryException e) {
+ assertTrue("expected an InvalidQueryException caused by a ParseException",
+ e.getCause() instanceof IllegalArgumentException);
+ }
+ } else {
+ it = q.execute().getNodes();
+ result = it.getSize();
+ assertTrue("size: " + result + " expected around " + expected,
+ result > expected - 50 &&
+ result < expected + 50);
+ buff = new StringBuilder();
+ while (it.hasNext()) {
+ Node n = it.nextNode();
+ buff.append(n.getPath()).append('\n');
+ }
+ fastSizeResult = buff.toString();
+ q = qm.createQuery(statement + " option (insecure result size)", Query.JCR_SQL2);
+ q.setLimit(90);
+ it = q.execute().getNodes();
+ assertEquals(90, it.getSize());
+ }
+
+ // default (secure) case
+ q = qm.createQuery(statement, Query.JCR_SQL2);
+ it = q.execute().getNodes();
+ result = it.getSize();
+ assertEquals(-1, result);
+ buff = new StringBuilder();
+ while (it.hasNext()) {
+ Node n = it.nextNode();
+ buff.append(n.getPath()).append('\n');
+ }
+ String regularResult = buff.toString();
+ if (expected >= 0) {
+ assertEquals(regularResult, fastSizeResult);
+ }
+
+ System.clearProperty("oak.fastQuerySize");
Review Comment:
I did not look at the PR in detail, I just made a comment in case you were not familiar with the RestoreSystemProperties rule (I was not until recently and discovering it simplified my tests). So I don't feel ready to either approve or to make other comments about the PR. It's fine to use finally to clear the system properties. I'd say to go ahead, you already have two approvals.
The only suggestion I have after a very quick look is to avoid repeating the string literal "oak.fastQuerySize". Better to have a constant defined somewhere, to avoid copy and paste errors.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] OAK-10424 override oak.fastQuerySize system property with OSGi configuration granting same effect, but limited to list of allowed principals [jackrabbit-oak]
Posted by "adamcin (via GitHub)" <gi...@apache.org>.
adamcin commented on code in PR #1094:
URL: https://github.com/apache/jackrabbit-oak/pull/1094#discussion_r1446363899
##########
oak-jcr/src/main/java/org/apache/jackrabbit/oak/jcr/repository/RepositoryImpl.java:
##########
@@ -564,4 +571,30 @@ public synchronized void cancel() {
}
}
}
+
+ /**
+ * This is a fallback implementation of {@link org.apache.jackrabbit.oak.spi.query.SessionQuerySettingsProvider} that
+ * unifies and replaces the previous handling of {@code fastQueryResultSize} and {@code oak.fastQuerySize} here
+ * and in the {@link org.apache.jackrabbit.oak.jcr.session.SessionContext}.
+ */
+ private static class FastQuerySizeSettingsProvider implements SessionQuerySettingsProvider {
+ private final boolean fastQueryResultSize;
+
+ public FastQuerySizeSettingsProvider(boolean fastQueryResultSize) {
+ this.fastQueryResultSize = fastQueryResultSize;
+ }
+
+ @Override
+ public @NotNull SessionQuerySettings getQuerySettings(@NotNull ContentSession session) {
+ return new SessionQuerySettings() {
+ @Override
+ public boolean useDirectResultCount() {
+ if (System.getProperty("oak.fastQuerySize") != null) {
+ return Boolean.getBoolean("oak.fastQuerySize");
+ }
Review Comment:
@thomasmueller Thanks. I made a change to use an Optional expression for the line.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [jackrabbit-oak] thomasmueller commented on a diff in pull request #1094: OAK-10424 add support for INSECURE RESULT SIZE and INSECURE FACETS query options and rep:insecureQueryOptions privilege
Posted by "thomasmueller (via GitHub)" <gi...@apache.org>.
thomasmueller commented on code in PR #1094:
URL: https://github.com/apache/jackrabbit-oak/pull/1094#discussion_r1317041436
##########
oak-core/src/main/java/org/apache/jackrabbit/oak/query/SQL2Parser.java:
##########
@@ -234,6 +249,28 @@ public Query parse(final String query, final boolean initialise) throws ParseExc
return q;
}
+
+ /**
+ * Checks the InsecureQueryOptionsMode for the values ALLOW or DENY. If allowed, the method returns true,
+ * indicating that insecure options should be returned by the parser. If denied, a ParseException is thrown.
+ * For all other modes, the method returns false, indicating that this option should be ignored.
+ *
+ * @return true if the mode is ALLOW, false otherwise
+ * @throws ParseException if the mode is DENY
+ */
+ private boolean allowsInsecureOptions() throws ParseException {
Review Comment:
I think checking (and throwing the exception) need to happen at execution time, not while parsing.
##########
oak-core/src/main/java/org/apache/jackrabbit/oak/query/QueryOptions.java:
##########
@@ -106,6 +110,14 @@ public QueryOptions(QueryOptions defaultValues) {
t.read(']');
prefetch = list;
}
+ x = map.get("insecureResultSize");
+ if (x != null) {
Review Comment:
I think the following should work:
insecureResultSize = Boolean.parseBoolean(map.get("insecureResultSize"))
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] OAK-10424 override oak.fastQuerySize system property with OSGi configuration granting same effect, but limited to list of allowed principals [jackrabbit-oak]
Posted by "anchela (via GitHub)" <gi...@apache.org>.
anchela commented on PR #1094:
URL: https://github.com/apache/jackrabbit-oak/pull/1094#issuecomment-1885204284
thanks a lot @adamcin for all the energy and effort you put into this!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] OAK-10424 override oak.fastQuerySize system property with OSGi configuration granting same effect, but limited to list of allowed principals [jackrabbit-oak]
Posted by "thomasmueller (via GitHub)" <gi...@apache.org>.
thomasmueller commented on code in PR #1094:
URL: https://github.com/apache/jackrabbit-oak/pull/1094#discussion_r1440653552
##########
oak-jcr/src/main/java/org/apache/jackrabbit/oak/jcr/repository/RepositoryImpl.java:
##########
@@ -564,4 +571,30 @@ public synchronized void cancel() {
}
}
}
+
+ /**
+ * This is a fallback implementation of {@link org.apache.jackrabbit.oak.spi.query.SessionQuerySettingsProvider} that
+ * unifies and replaces the previous handling of {@code fastQueryResultSize} and {@code oak.fastQuerySize} here
+ * and in the {@link org.apache.jackrabbit.oak.jcr.session.SessionContext}.
+ */
+ private static class FastQuerySizeSettingsProvider implements SessionQuerySettingsProvider {
+ private final boolean fastQueryResultSize;
+
+ public FastQuerySizeSettingsProvider(boolean fastQueryResultSize) {
+ this.fastQueryResultSize = fastQueryResultSize;
+ }
+
+ @Override
+ public @NotNull SessionQuerySettings getQuerySettings(@NotNull ContentSession session) {
+ return new SessionQuerySettings() {
+ @Override
+ public boolean useDirectResultCount() {
+ if (System.getProperty("oak.fastQuerySize") != null) {
+ return Boolean.getBoolean("oak.fastQuerySize");
+ }
Review Comment:
I would access the system property only once... Something like this:
```suggestion
String x = System.getProperty("oak.fastQuerySize");
if (x != null) {
return Boolean.parseBoolean(x);
}
```
I know the risk of concurrent change to the system property is tiny... but still.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [jackrabbit-oak] nfsantos commented on a diff in pull request #1094: OAK-10424 add support for INSECURE RESULT SIZE and INSECURE FACETS query options and rep:insecureQueryOptions privilege
Posted by "nfsantos (via GitHub)" <gi...@apache.org>.
nfsantos commented on code in PR #1094:
URL: https://github.com/apache/jackrabbit-oak/pull/1094#discussion_r1326909040
##########
oak-lucene/src/test/java/org/apache/jackrabbit/oak/jcr/query/ResultSizeTest.java:
##########
@@ -126,5 +151,90 @@ private void doTestResultSize(boolean union, int expected) throws RepositoryExce
System.clearProperty("oak.fastQuerySize");
}
+
+ private void doTestResultSizeOption(boolean aggregateAtQueryTime) throws RepositoryException {
+ createData();
+ int expectedForUnion = 400;
+ int expectedForTwoConditions = aggregateAtQueryTime ? 400 : 200;
+ doTestResultSizeOption(superuser, false, expectedForTwoConditions);
+ doTestResultSizeOption(superuser, true, expectedForUnion);
+ Session readOnlySession = null;
+ try {
+ readOnlySession = getHelper().getReadOnlySession();
+ assertNotNull(readOnlySession);
+ doTestResultSizeOption(readOnlySession, false, -1);
+ doTestResultSizeOption(readOnlySession, true, -1);
+ } finally {
+ if (readOnlySession != null) {
+ readOnlySession.logout();
+ }
+ }
+ }
+
+ private void doTestResultSizeOption(Session session, boolean union, int expected) throws RepositoryException {
+ QueryManager qm = session.getWorkspace().getQueryManager();
+
+ String statement;
+ if (union) {
+ statement = "select a.[jcr:path] from [nt:base] as a where contains(a.[text], 'Hello') UNION select a.[jcr:path] from [nt:base] as a where contains(a.[text], 'World')";
+ } else {
+ statement = "select a.[jcr:path] from [nt:base] as a where contains(a.[text], 'Hello World')";
+ }
+
+ Query q;
+ long result;
+ NodeIterator it;
+ StringBuilder buff;
+
+ // enabled by default now, in LuceneOakRepositoryStub. Disable global
+ System.setProperty("oak.fastQuerySize", "false");
+
+ // fast (insecure) case
+ String fastSizeResult = "";
+ q = qm.createQuery(statement + " option (insecure result size)", Query.JCR_SQL2);
+ if (expected < 0) {
+ // if expected < 0, i.e. insufficient permissions, expect a InvalidQueryException on execute().
+ try {
+ it = q.execute().getNodes();
+ fail("expected an InvalidQueryException caused by a IllegalArgumentException");
+ } catch (InvalidQueryException e) {
+ assertTrue("expected an InvalidQueryException caused by a ParseException",
+ e.getCause() instanceof IllegalArgumentException);
+ }
+ } else {
+ it = q.execute().getNodes();
+ result = it.getSize();
+ assertTrue("size: " + result + " expected around " + expected,
+ result > expected - 50 &&
+ result < expected + 50);
+ buff = new StringBuilder();
+ while (it.hasNext()) {
+ Node n = it.nextNode();
+ buff.append(n.getPath()).append('\n');
+ }
+ fastSizeResult = buff.toString();
+ q = qm.createQuery(statement + " option (insecure result size)", Query.JCR_SQL2);
+ q.setLimit(90);
+ it = q.execute().getNodes();
+ assertEquals(90, it.getSize());
+ }
+
+ // default (secure) case
+ q = qm.createQuery(statement, Query.JCR_SQL2);
+ it = q.execute().getNodes();
+ result = it.getSize();
+ assertEquals(-1, result);
+ buff = new StringBuilder();
+ while (it.hasNext()) {
+ Node n = it.nextNode();
+ buff.append(n.getPath()).append('\n');
+ }
+ String regularResult = buff.toString();
+ if (expected >= 0) {
+ assertEquals(regularResult, fastSizeResult);
+ }
+
+ System.clearProperty("oak.fastQuerySize");
Review Comment:
For setting and resetting system properties in tests, have a look at the [RestoreSystemProperties](https://stefanbirkner.github.io/system-rules/apidocs/org/junit/contrib/java/lang/system/RestoreSystemProperties.html) class in JUnit. At the end of a test method, it resets any system property that was set in the method to the original value. It is already a dependency in Oak.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [jackrabbit-oak] nfsantos commented on a diff in pull request #1094: OAK-10424 add support for INSECURE RESULT SIZE and INSECURE FACETS query options and rep:insecureQueryOptions privilege
Posted by "nfsantos (via GitHub)" <gi...@apache.org>.
nfsantos commented on code in PR #1094:
URL: https://github.com/apache/jackrabbit-oak/pull/1094#discussion_r1326909040
##########
oak-lucene/src/test/java/org/apache/jackrabbit/oak/jcr/query/ResultSizeTest.java:
##########
@@ -126,5 +151,90 @@ private void doTestResultSize(boolean union, int expected) throws RepositoryExce
System.clearProperty("oak.fastQuerySize");
}
+
+ private void doTestResultSizeOption(boolean aggregateAtQueryTime) throws RepositoryException {
+ createData();
+ int expectedForUnion = 400;
+ int expectedForTwoConditions = aggregateAtQueryTime ? 400 : 200;
+ doTestResultSizeOption(superuser, false, expectedForTwoConditions);
+ doTestResultSizeOption(superuser, true, expectedForUnion);
+ Session readOnlySession = null;
+ try {
+ readOnlySession = getHelper().getReadOnlySession();
+ assertNotNull(readOnlySession);
+ doTestResultSizeOption(readOnlySession, false, -1);
+ doTestResultSizeOption(readOnlySession, true, -1);
+ } finally {
+ if (readOnlySession != null) {
+ readOnlySession.logout();
+ }
+ }
+ }
+
+ private void doTestResultSizeOption(Session session, boolean union, int expected) throws RepositoryException {
+ QueryManager qm = session.getWorkspace().getQueryManager();
+
+ String statement;
+ if (union) {
+ statement = "select a.[jcr:path] from [nt:base] as a where contains(a.[text], 'Hello') UNION select a.[jcr:path] from [nt:base] as a where contains(a.[text], 'World')";
+ } else {
+ statement = "select a.[jcr:path] from [nt:base] as a where contains(a.[text], 'Hello World')";
+ }
+
+ Query q;
+ long result;
+ NodeIterator it;
+ StringBuilder buff;
+
+ // enabled by default now, in LuceneOakRepositoryStub. Disable global
+ System.setProperty("oak.fastQuerySize", "false");
+
+ // fast (insecure) case
+ String fastSizeResult = "";
+ q = qm.createQuery(statement + " option (insecure result size)", Query.JCR_SQL2);
+ if (expected < 0) {
+ // if expected < 0, i.e. insufficient permissions, expect a InvalidQueryException on execute().
+ try {
+ it = q.execute().getNodes();
+ fail("expected an InvalidQueryException caused by a IllegalArgumentException");
+ } catch (InvalidQueryException e) {
+ assertTrue("expected an InvalidQueryException caused by a ParseException",
+ e.getCause() instanceof IllegalArgumentException);
+ }
+ } else {
+ it = q.execute().getNodes();
+ result = it.getSize();
+ assertTrue("size: " + result + " expected around " + expected,
+ result > expected - 50 &&
+ result < expected + 50);
+ buff = new StringBuilder();
+ while (it.hasNext()) {
+ Node n = it.nextNode();
+ buff.append(n.getPath()).append('\n');
+ }
+ fastSizeResult = buff.toString();
+ q = qm.createQuery(statement + " option (insecure result size)", Query.JCR_SQL2);
+ q.setLimit(90);
+ it = q.execute().getNodes();
+ assertEquals(90, it.getSize());
+ }
+
+ // default (secure) case
+ q = qm.createQuery(statement, Query.JCR_SQL2);
+ it = q.execute().getNodes();
+ result = it.getSize();
+ assertEquals(-1, result);
+ buff = new StringBuilder();
+ while (it.hasNext()) {
+ Node n = it.nextNode();
+ buff.append(n.getPath()).append('\n');
+ }
+ String regularResult = buff.toString();
+ if (expected >= 0) {
+ assertEquals(regularResult, fastSizeResult);
+ }
+
+ System.clearProperty("oak.fastQuerySize");
Review Comment:
For setting and resetting system properties in tests, have a look at the `[RestoreSystemProperties](https://stefanbirkner.github.io/system-rules/apidocs/org/junit/contrib/java/lang/system/RestoreSystemProperties.html)` class in JUnit. At the end of a test method, it resets any system property that was set in the method to the original value. It is already a dependency in Oak.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] OAK-10424 add support for INSECURE RESULT SIZE and INSECURE FACETS query options restricted by configurable JCR privilege [jackrabbit-oak]
Posted by "anchela (via GitHub)" <gi...@apache.org>.
anchela commented on code in PR #1094:
URL: https://github.com/apache/jackrabbit-oak/pull/1094#discussion_r1356877584
##########
oak-api/src/main/java/org/apache/jackrabbit/oak/api/jmx/QueryEngineSettingsMBean.java:
##########
@@ -188,4 +188,21 @@ void setIgnoredClassNamesInCallTrace(
// @Description("Get the Java package / fully qualified class names to ignore when finding the caller of query")
@NotNull
String[] getIgnoredClassNamesInCallTrace();
+
+ /**
+ * Set the name of a JCR privilege that grants a repository permission to use insecure query options.
+ *
+ * @param privilegeName the insecure query options privilege name
+ * @since 1.60
+ */
+ @Description("Set the name of a JCR privilege that grants a repository permission to use insecure query options.")
+ void setInsecureQueryOptionsPrivilegeName(@NotNull String privilegeName);
+
+ /**
+ * Get the configured name of a JCR privilege that grants a repository permission to use insecure query options.
Review Comment:
same as above.
##########
oak-api/src/main/java/org/apache/jackrabbit/oak/api/Result.java:
##########
@@ -74,6 +74,15 @@ public interface Result {
* count is higher than max, it returns Long.MAX_VALUE.
*/
long getSize(SizePrecision precision, long max);
+
+ /**
+ * Indicates whether the query specified the INSECURE RESULT SIZE option.
+ *
+ * @return true if the INSECURE RESULT SIZE query option was set
+ */
+ default boolean isQueryOptionInsecureResultSize() {
Review Comment:
from a security point of view, i find it questionable to add public API to make something insecure by design.
i wonder if there is any, remote possiblity to introduce the feature without altering public API contract that we cannot revert or adjust later on.
##########
oak-api/src/main/java/org/apache/jackrabbit/oak/api/jmx/QueryEngineSettingsMBean.java:
##########
@@ -188,4 +188,21 @@ void setIgnoredClassNamesInCallTrace(
// @Description("Get the Java package / fully qualified class names to ignore when finding the caller of query")
@NotNull
String[] getIgnoredClassNamesInCallTrace();
+
+ /**
+ * Set the name of a JCR privilege that grants a repository permission to use insecure query options.
+ *
+ * @param privilegeName the insecure query options privilege name
+ * @since 1.60
+ */
+ @Description("Set the name of a JCR privilege that grants a repository permission to use insecure query options.")
Review Comment:
the description and the javadoc of this method is not entirely accurate. if a custom privilege is registered (and by doing so relying on application to verify that privilege to be present), we can no longer talk about a repository permission. the repository and it's permission evaluation is agnostic to this custom privilege and will not have the ability to enforce it.... so, i would like the phrasing here to accurately reflect on how the 'insecure-query-option' is being mitigated to some extend
##########
oak-query-spi/src/main/java/org/apache/jackrabbit/oak/spi/query/QueryLimits.java:
##########
@@ -43,4 +43,14 @@ default String getStrictPathRestriction() {
default @NotNull String[] getIgnoredClassNamesInCallTrace() {
return new String[] {};
}
+
+ /**
+ * Retrieve the configured name of a JCR privilege that grants a repository permission to use insecure query
Review Comment:
see comment above about being accurate with respect to custom privilege vs repository enforced permissions.
##########
oak-core/src/main/java/org/apache/jackrabbit/oak/query/QueryImpl.java:
##########
@@ -95,6 +95,9 @@
import org.apache.jackrabbit.oak.spi.query.QueryIndex.OrderEntry;
import org.apache.jackrabbit.oak.spi.query.QueryIndex.OrderEntry.Order;
import org.apache.jackrabbit.oak.spi.query.QueryIndexProvider;
+import org.apache.jackrabbit.oak.spi.query.QueryLimits;
+import org.apache.jackrabbit.oak.spi.security.authorization.permission.PermissionProvider;
Review Comment:
this import is unused -> remove. spotted when looking at the code in my IDE
##########
oak-core/src/main/java/org/apache/jackrabbit/oak/query/QueryImpl.java:
##########
@@ -1421,6 +1425,43 @@ public long getSize(SizePrecision precision, long max) {
return Math.min(limit.orElse(Long.MAX_VALUE), source.getSize(context.getBaseState(), precision, max));
}
+ /**
+ * Check whether the execution context has been granted the privilege represented by
+ * {@link org.apache.jackrabbit.oak.api.jmx.QueryEngineSettingsMBean#getInsecureQueryOptionsPrivilegeName()} as a
+ * repository permission.
+ *
+ * @return true if the context has the insecure query options privilege granted as a repository permission
+ */
+ boolean isAllowedInsecureOptions() {
+ return Optional.ofNullable(context.getSettings().getInsecureQueryOptionsPrivilegeName())
+ .filter(name -> !name.isEmpty())
+ .flatMap(privilegeName -> Optional.ofNullable(context)
+ .map(ExecutionContext::getPermissionProvider)
+ .map(provider -> provider.hasPrivileges(null, privilegeName)))
+ .orElse(false);
+ }
+
+ private boolean hasInsecureQueryOptions() {
+ return this.queryOptions != null && (this.queryOptions.insecureResultSize || this.queryOptions.insecureFacets);
+ }
+
+ @Override
+ public void verifyInsecureOptions() {
+ if (hasInsecureQueryOptions() && !isAllowedInsecureOptions()) {
+ throw new IllegalArgumentException("Insecure query options are not allowed.");
Review Comment:
serious note: this exception may actually bubble up unfiltered like the one recently spotted by a customer and reported by @joerghoh in [OAK-10473](https://issues.apache.org/jira/browse/OAK-10473).
again not a query expert but i would rather avoid that.
##########
oak-core/src/main/java/org/apache/jackrabbit/oak/query/QueryImpl.java:
##########
@@ -1421,6 +1425,43 @@ public long getSize(SizePrecision precision, long max) {
return Math.min(limit.orElse(Long.MAX_VALUE), source.getSize(context.getBaseState(), precision, max));
}
+ /**
+ * Check whether the execution context has been granted the privilege represented by
+ * {@link org.apache.jackrabbit.oak.api.jmx.QueryEngineSettingsMBean#getInsecureQueryOptionsPrivilegeName()} as a
+ * repository permission.
+ *
+ * @return true if the context has the insecure query options privilege granted as a repository permission
+ */
+ boolean isAllowedInsecureOptions() {
+ return Optional.ofNullable(context.getSettings().getInsecureQueryOptionsPrivilegeName())
+ .filter(name -> !name.isEmpty())
+ .flatMap(privilegeName -> Optional.ofNullable(context)
+ .map(ExecutionContext::getPermissionProvider)
+ .map(provider -> provider.hasPrivileges(null, privilegeName)))
Review Comment:
this means that the privilege can only be granted as a repository-level privilege. a given subject (as associated with every jcr session) either has the privilege granted or not for the whole repository.
in other words: this applies to all queries and there is no way to turn it on/off for particular query or query results. right?
that would be crucial information on the public facing configuration options and osgi configurations.
##########
oak-core/src/main/java/org/apache/jackrabbit/oak/query/QueryImpl.java:
##########
@@ -1421,6 +1425,43 @@ public long getSize(SizePrecision precision, long max) {
return Math.min(limit.orElse(Long.MAX_VALUE), source.getSize(context.getBaseState(), precision, max));
}
+ /**
+ * Check whether the execution context has been granted the privilege represented by
+ * {@link org.apache.jackrabbit.oak.api.jmx.QueryEngineSettingsMBean#getInsecureQueryOptionsPrivilegeName()} as a
+ * repository permission.
+ *
+ * @return true if the context has the insecure query options privilege granted as a repository permission
+ */
+ boolean isAllowedInsecureOptions() {
+ return Optional.ofNullable(context.getSettings().getInsecureQueryOptionsPrivilegeName())
+ .filter(name -> !name.isEmpty())
+ .flatMap(privilegeName -> Optional.ofNullable(context)
+ .map(ExecutionContext::getPermissionProvider)
+ .map(provider -> provider.hasPrivileges(null, privilegeName)))
+ .orElse(false);
+ }
+
+ private boolean hasInsecureQueryOptions() {
+ return this.queryOptions != null && (this.queryOptions.insecureResultSize || this.queryOptions.insecureFacets);
+ }
+
+ @Override
+ public void verifyInsecureOptions() {
+ if (hasInsecureQueryOptions() && !isAllowedInsecureOptions()) {
Review Comment:
ouch
that is hard to read
##########
oak-api/src/main/java/org/apache/jackrabbit/oak/api/jmx/QueryEngineSettingsMBean.java:
##########
@@ -188,4 +188,21 @@ void setIgnoredClassNamesInCallTrace(
// @Description("Get the Java package / fully qualified class names to ignore when finding the caller of query")
@NotNull
String[] getIgnoredClassNamesInCallTrace();
+
+ /**
+ * Set the name of a JCR privilege that grants a repository permission to use insecure query options.
+ *
+ * @param privilegeName the insecure query options privilege name
+ * @since 1.60
+ */
+ @Description("Set the name of a JCR privilege that grants a repository permission to use insecure query options.")
+ void setInsecureQueryOptionsPrivilegeName(@NotNull String privilegeName);
+
+ /**
+ * Get the configured name of a JCR privilege that grants a repository permission to use insecure query options.
+ *
+ * @return the insecure query options privilege name
+ * @since 1.60
+ */
+ String getInsecureQueryOptionsPrivilegeName();
Review Comment:
that method name and it's associated 'setter' hurt my security heart. 'insecure' + 'privilegeName' really doesn't got together. i understand your intention but i would strongly recommend to reconsider the naming here.
given that this class is public API and not just an implementation detail, i would like you to put reconsider the naming convention used for this feature. once we have it in the API we cannot get rid of it easily.
##########
oak-core/src/main/java/org/apache/jackrabbit/oak/query/QueryImpl.java:
##########
@@ -95,6 +95,9 @@
import org.apache.jackrabbit.oak.spi.query.QueryIndex.OrderEntry;
import org.apache.jackrabbit.oak.spi.query.QueryIndex.OrderEntry.Order;
import org.apache.jackrabbit.oak.spi.query.QueryIndexProvider;
+import org.apache.jackrabbit.oak.spi.query.QueryLimits;
+import org.apache.jackrabbit.oak.spi.security.authorization.permission.PermissionProvider;
+import org.apache.jackrabbit.oak.spi.security.authorization.permission.Permissions;
Review Comment:
this import is unused -> remove. spotted when looking at the code in my IDE
##########
oak-query-spi/src/main/java/org/apache/jackrabbit/oak/spi/query/QueryLimits.java:
##########
@@ -43,4 +43,14 @@ default String getStrictPathRestriction() {
default @NotNull String[] getIgnoredClassNamesInCallTrace() {
return new String[] {};
}
+
+ /**
+ * Retrieve the configured name of a JCR privilege that grants a repository permission to use insecure query
+ * options.
+ * @return the insecure query options privilege name
+ * @since 1.60
+ */
+ default String getInsecureQueryOptionsPrivilegeName() {
Review Comment:
ok.... the same option now is present in
- QueryEngineSettings
- QueryOptions
- QueryLimits
that looks wrong to me. i am mentioning this here as this particular configuration option doesn't sound like a 'limit' to me.
i am not a query expert but adding the same option to 3 different interfaces in API/Spi range does look like a good design. on top it ends up in the Oak.java class.
##########
oak-core/src/main/java/org/apache/jackrabbit/oak/query/QueryImpl.java:
##########
@@ -95,6 +95,9 @@
import org.apache.jackrabbit.oak.spi.query.QueryIndex.OrderEntry;
import org.apache.jackrabbit.oak.spi.query.QueryIndex.OrderEntry.Order;
import org.apache.jackrabbit.oak.spi.query.QueryIndexProvider;
+import org.apache.jackrabbit.oak.spi.query.QueryLimits;
Review Comment:
this import is unused -> remove. spotted when looking at the code in my IDE
##########
oak-core/src/main/java/org/apache/jackrabbit/oak/query/QueryImpl.java:
##########
@@ -1421,6 +1425,43 @@ public long getSize(SizePrecision precision, long max) {
return Math.min(limit.orElse(Long.MAX_VALUE), source.getSize(context.getBaseState(), precision, max));
}
+ /**
+ * Check whether the execution context has been granted the privilege represented by
+ * {@link org.apache.jackrabbit.oak.api.jmx.QueryEngineSettingsMBean#getInsecureQueryOptionsPrivilegeName()} as a
+ * repository permission.
+ *
+ * @return true if the context has the insecure query options privilege granted as a repository permission
+ */
+ boolean isAllowedInsecureOptions() {
Review Comment:
as far as i can see this method is only used inside QueryImpl itself. is there a reason for not marking it as private?
##########
oak-core/src/main/java/org/apache/jackrabbit/oak/query/QueryImpl.java:
##########
@@ -1421,6 +1425,43 @@ public long getSize(SizePrecision precision, long max) {
return Math.min(limit.orElse(Long.MAX_VALUE), source.getSize(context.getBaseState(), precision, max));
}
+ /**
+ * Check whether the execution context has been granted the privilege represented by
+ * {@link org.apache.jackrabbit.oak.api.jmx.QueryEngineSettingsMBean#getInsecureQueryOptionsPrivilegeName()} as a
+ * repository permission.
+ *
+ * @return true if the context has the insecure query options privilege granted as a repository permission
+ */
+ boolean isAllowedInsecureOptions() {
+ return Optional.ofNullable(context.getSettings().getInsecureQueryOptionsPrivilegeName())
+ .filter(name -> !name.isEmpty())
+ .flatMap(privilegeName -> Optional.ofNullable(context)
+ .map(ExecutionContext::getPermissionProvider)
+ .map(provider -> provider.hasPrivileges(null, privilegeName)))
Review Comment:
i haven't looked at PermissionProvider.hasPrivieges in a while. so my question would be: what happens if an invalid privilege name has been configured? e.g. one that never got registered. will the method return true or false?
i remember from recent work on OAK-9674 that i had to introduce a way to validate privilege names and i suspect it's not validated for internal calls. the main usage for `PermissionProvider.hasPrivileges` is the JCR API calls AccessControlManager.hasPrivileges which takes a privilege object and not a simple string.
what i suspect (but have not verified): the assertion will return true if an unknown privilege name has been configured.
please check that.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] OAK-10424 override oak.fastQuerySize system property with OSGi configuration granting same effect, but limited to list of allowed principals [jackrabbit-oak]
Posted by "thomasmueller (via GitHub)" <gi...@apache.org>.
thomasmueller commented on code in PR #1094:
URL: https://github.com/apache/jackrabbit-oak/pull/1094#discussion_r1447631658
##########
oak-jcr/src/main/java/org/apache/jackrabbit/oak/jcr/repository/RepositoryImpl.java:
##########
@@ -564,4 +571,30 @@ public synchronized void cancel() {
}
}
}
+
+ /**
+ * This is a fallback implementation of {@link org.apache.jackrabbit.oak.spi.query.SessionQuerySettingsProvider} that
+ * unifies and replaces the previous handling of {@code fastQueryResultSize} and {@code oak.fastQuerySize} here
+ * and in the {@link org.apache.jackrabbit.oak.jcr.session.SessionContext}.
+ */
+ private static class FastQuerySizeSettingsProvider implements SessionQuerySettingsProvider {
+ private final boolean fastQueryResultSize;
+
+ public FastQuerySizeSettingsProvider(boolean fastQueryResultSize) {
+ this.fastQueryResultSize = fastQueryResultSize;
+ }
+
+ @Override
+ public @NotNull SessionQuerySettings getQuerySettings(@NotNull ContentSession session) {
+ return new SessionQuerySettings() {
+ @Override
+ public boolean useDirectResultCount() {
+ if (System.getProperty("oak.fastQuerySize") != null) {
+ return Boolean.getBoolean("oak.fastQuerySize");
+ }
Review Comment:
I personally would have preferred the "if" statement (it's shorter) but OK.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] OAK-10424 override oak.fastQuerySize system property with OSGi configuration granting same effect, but limited to list of allowed principals [jackrabbit-oak]
Posted by "anchela (via GitHub)" <gi...@apache.org>.
anchela merged PR #1094:
URL: https://github.com/apache/jackrabbit-oak/pull/1094
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [jackrabbit-oak] adamcin commented on a diff in pull request #1094: OAK-10424 add support for INSECURE RESULT SIZE and INSECURE FACETS query options and rep:insecureQueryOptions privilege
Posted by "adamcin (via GitHub)" <gi...@apache.org>.
adamcin commented on code in PR #1094:
URL: https://github.com/apache/jackrabbit-oak/pull/1094#discussion_r1317755379
##########
oak-core/src/main/java/org/apache/jackrabbit/oak/query/SQL2Parser.java:
##########
@@ -234,6 +249,28 @@ public Query parse(final String query, final boolean initialise) throws ParseExc
return q;
}
+
+ /**
+ * Checks the InsecureQueryOptionsMode for the values ALLOW or DENY. If allowed, the method returns true,
+ * indicating that insecure options should be returned by the parser. If denied, a ParseException is thrown.
+ * For all other modes, the method returns false, indicating that this option should be ignored.
+ *
+ * @return true if the mode is ALLOW, false otherwise
+ * @throws ParseException if the mode is DENY
+ */
+ private boolean allowsInsecureOptions() throws ParseException {
Review Comment:
I moved the evaluation to QueryImpl as a distinct `verifyInsecureOptions()` method and call it within the `QueryEngineImpl.executeQuery()` method on each query after it has been parsed/inited but before it has been prepared.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [jackrabbit-oak] thomasmueller commented on a diff in pull request #1094: OAK-10424 add support for INSECURE RESULTSIZE and INSECURE FACETS query options and rep:insecureQueryOptions privilege
Posted by "thomasmueller (via GitHub)" <gi...@apache.org>.
thomasmueller commented on code in PR #1094:
URL: https://github.com/apache/jackrabbit-oak/pull/1094#discussion_r1315792134
##########
oak-core/src/main/java/org/apache/jackrabbit/oak/query/UnionQueryImpl.java:
##########
@@ -195,7 +195,13 @@ public long getSize(SizePrecision precision, long max) {
long total = QueryImpl.saturatedAdd(a, b);
return Math.min(localLimit, total);
}
-
+
+ @Override
+ public boolean isInsecureResultSize() {
+ // assumes that the same queryOptions are passed to both left and right queries in setQueryOptions()
+ return right.isInsecureResultSize();
Review Comment:
Why not use left.isInsecureResultSize() || right.isInsecureResultSize(): that way the implementation can be changed later.
##########
oak-core/src/main/java/org/apache/jackrabbit/oak/query/xpath/Statement.java:
##########
@@ -382,6 +382,18 @@ private static void appendQueryOptions(StringBuilder buff, QueryOptions queryOpt
SQL2Parser::escapeStringLiteral));
optionValues.add("prefetch (" + list + ")");
}
+ if (queryOptions.hasInsecureOptions()) {
+ List<String> insecureOpts = new ArrayList<>();
+ insecureOpts.add("insecure");
+ if (queryOptions.insecureResultSize) {
+ insecureOpts.add("resultsize");
Review Comment:
I don't like "resultsize" as it's two words concatenated... Options are:
* "resultsize": can be hard to read
* "result_size": looks a bit ugly
* "resultSize": the parser is not case sensitive so it might work
* "result size": this approach is used for "index tag" already
I would prefer "result size" (two words).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [jackrabbit-oak] adamcin commented on a diff in pull request #1094: OAK-10424 add support for INSECURE RESULT SIZE and INSECURE FACETS query options and rep:insecureQueryOptions privilege
Posted by "adamcin (via GitHub)" <gi...@apache.org>.
adamcin commented on code in PR #1094:
URL: https://github.com/apache/jackrabbit-oak/pull/1094#discussion_r1329116561
##########
oak-lucene/src/test/java/org/apache/jackrabbit/oak/jcr/query/ResultSizeTest.java:
##########
@@ -126,5 +151,90 @@ private void doTestResultSize(boolean union, int expected) throws RepositoryExce
System.clearProperty("oak.fastQuerySize");
}
+
+ private void doTestResultSizeOption(boolean aggregateAtQueryTime) throws RepositoryException {
+ createData();
+ int expectedForUnion = 400;
+ int expectedForTwoConditions = aggregateAtQueryTime ? 400 : 200;
+ doTestResultSizeOption(superuser, false, expectedForTwoConditions);
+ doTestResultSizeOption(superuser, true, expectedForUnion);
+ Session readOnlySession = null;
+ try {
+ readOnlySession = getHelper().getReadOnlySession();
+ assertNotNull(readOnlySession);
+ doTestResultSizeOption(readOnlySession, false, -1);
+ doTestResultSizeOption(readOnlySession, true, -1);
+ } finally {
+ if (readOnlySession != null) {
+ readOnlySession.logout();
+ }
+ }
+ }
+
+ private void doTestResultSizeOption(Session session, boolean union, int expected) throws RepositoryException {
+ QueryManager qm = session.getWorkspace().getQueryManager();
+
+ String statement;
+ if (union) {
+ statement = "select a.[jcr:path] from [nt:base] as a where contains(a.[text], 'Hello') UNION select a.[jcr:path] from [nt:base] as a where contains(a.[text], 'World')";
+ } else {
+ statement = "select a.[jcr:path] from [nt:base] as a where contains(a.[text], 'Hello World')";
+ }
+
+ Query q;
+ long result;
+ NodeIterator it;
+ StringBuilder buff;
+
+ // enabled by default now, in LuceneOakRepositoryStub. Disable global
+ System.setProperty("oak.fastQuerySize", "false");
+
+ // fast (insecure) case
+ String fastSizeResult = "";
+ q = qm.createQuery(statement + " option (insecure result size)", Query.JCR_SQL2);
+ if (expected < 0) {
+ // if expected < 0, i.e. insufficient permissions, expect a InvalidQueryException on execute().
+ try {
+ it = q.execute().getNodes();
+ fail("expected an InvalidQueryException caused by a IllegalArgumentException");
+ } catch (InvalidQueryException e) {
+ assertTrue("expected an InvalidQueryException caused by a ParseException",
+ e.getCause() instanceof IllegalArgumentException);
+ }
+ } else {
+ it = q.execute().getNodes();
+ result = it.getSize();
+ assertTrue("size: " + result + " expected around " + expected,
+ result > expected - 50 &&
+ result < expected + 50);
+ buff = new StringBuilder();
+ while (it.hasNext()) {
+ Node n = it.nextNode();
+ buff.append(n.getPath()).append('\n');
+ }
+ fastSizeResult = buff.toString();
+ q = qm.createQuery(statement + " option (insecure result size)", Query.JCR_SQL2);
+ q.setLimit(90);
+ it = q.execute().getNodes();
+ assertEquals(90, it.getSize());
+ }
+
+ // default (secure) case
+ q = qm.createQuery(statement, Query.JCR_SQL2);
+ it = q.execute().getNodes();
+ result = it.getSize();
+ assertEquals(-1, result);
+ buff = new StringBuilder();
+ while (it.hasNext()) {
+ Node n = it.nextNode();
+ buff.append(n.getPath()).append('\n');
+ }
+ String regularResult = buff.toString();
+ if (expected >= 0) {
+ assertEquals(regularResult, fastSizeResult);
+ }
+
+ System.clearProperty("oak.fastQuerySize");
Review Comment:
@nfsantos are you able to approve?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [jackrabbit-oak] adamcin commented on pull request #1094: OAK-10424 add support for INSECURE RESULT SIZE and INSECURE FACETS query options and rep:insecureQueryOptions privilege
Posted by "adamcin (via GitHub)" <gi...@apache.org>.
adamcin commented on PR #1094:
URL: https://github.com/apache/jackrabbit-oak/pull/1094#issuecomment-1724298220
@thomasmueller I don't have commit rights, so if you are willing to merge it, please do.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org