Fix invalid operator invalidating web session
So far, enforcing authentication for query operators which require it
was done by matching an error message. Problem with this approach is,
if the query operator is invalid (as, for example, "has:starss") this
invalidates the web session for the logged in user.
Create a new exception QueryRequiresAuthException to explicitly handle
the case where an operator requires authentication and throw this when
needed instead of the generic QueryParseException.
Bug: issue 6657
Change-Id: I93096ec3b0918f8238cf2eeef96cf2cf42410e72
diff --git a/gerrit-antlr/BUILD b/gerrit-antlr/BUILD
index f4ce4c7..cb05469 100644
--- a/gerrit-antlr/BUILD
+++ b/gerrit-antlr/BUILD
@@ -2,7 +2,10 @@
java_library(
name = "query_exception",
- srcs = ["src/main/java/com/google/gerrit/server/query/QueryParseException.java"],
+ srcs = [
+ "src/main/java/com/google/gerrit/server/query/QueryParseException.java",
+ "src/main/java/com/google/gerrit/server/query/QueryRequiresAuthException.java",
+ ],
visibility = ["//visibility:public"],
)
diff --git a/gerrit-antlr/src/main/java/com/google/gerrit/server/query/QueryRequiresAuthException.java b/gerrit-antlr/src/main/java/com/google/gerrit/server/query/QueryRequiresAuthException.java
new file mode 100644
index 0000000..a41e54f
--- /dev/null
+++ b/gerrit-antlr/src/main/java/com/google/gerrit/server/query/QueryRequiresAuthException.java
@@ -0,0 +1,32 @@
+// Copyright (C) 2017 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.gerrit.server.query;
+
+/**
+ * Exception thrown when a search query is invalid.
+ *
+ * <p><b>NOTE:</b> the message is visible to end users.
+ */
+public class QueryRequiresAuthException extends QueryParseException {
+ private static final long serialVersionUID = 1L;
+
+ public QueryRequiresAuthException(String message) {
+ super(message);
+ }
+
+ public QueryRequiresAuthException(String msg, Throwable why) {
+ super(msg, why);
+ }
+}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java
index 5c3e8c9..52009ad 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java
@@ -71,6 +71,7 @@
import com.google.gerrit.server.query.Predicate;
import com.google.gerrit.server.query.QueryBuilder;
import com.google.gerrit.server.query.QueryParseException;
+import com.google.gerrit.server.query.QueryRequiresAuthException;
import com.google.gwtorm.server.OrmException;
import com.google.inject.Inject;
import com.google.inject.Provider;
@@ -388,23 +389,23 @@
return asUser(userFactory.create(otherId));
}
- IdentifiedUser getIdentifiedUser() throws QueryParseException {
+ IdentifiedUser getIdentifiedUser() throws QueryRequiresAuthException {
try {
CurrentUser u = getUser();
if (u.isIdentifiedUser()) {
return u.asIdentifiedUser();
}
- throw new QueryParseException(NotSignedInException.MESSAGE);
+ throw new QueryRequiresAuthException(NotSignedInException.MESSAGE);
} catch (ProvisionException e) {
- throw new QueryParseException(NotSignedInException.MESSAGE, e);
+ throw new QueryRequiresAuthException(NotSignedInException.MESSAGE, e);
}
}
- CurrentUser getUser() throws QueryParseException {
+ CurrentUser getUser() throws QueryRequiresAuthException {
try {
return self.get();
} catch (ProvisionException e) {
- throw new QueryParseException(NotSignedInException.MESSAGE, e);
+ throw new QueryRequiresAuthException(NotSignedInException.MESSAGE, e);
}
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/QueryChanges.java b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/QueryChanges.java
index f6ace3b..370bd21 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/QueryChanges.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/QueryChanges.java
@@ -29,6 +29,7 @@
import com.google.gerrit.server.change.ChangeJson;
import com.google.gerrit.server.index.change.ChangeField;
import com.google.gerrit.server.query.QueryParseException;
+import com.google.gerrit.server.query.QueryRequiresAuthException;
import com.google.gerrit.server.query.QueryResult;
import com.google.gwtorm.server.OrmException;
import com.google.inject.Inject;
@@ -36,8 +37,6 @@
import java.util.Collections;
import java.util.EnumSet;
import java.util.List;
-import java.util.regex.Matcher;
-import java.util.regex.Pattern;
import org.kohsuke.args4j.Option;
public class QueryChanges implements RestReadView<TopLevelResource> {
@@ -110,15 +109,9 @@
List<List<ChangeInfo>> out;
try {
out = query();
+ } catch (QueryRequiresAuthException e) {
+ throw new AuthException("Must be signed-in to use this operator");
} catch (QueryParseException e) {
- // This is a hack to detect an operator that requires authentication.
- Pattern p =
- Pattern.compile("^Error in operator (.*:self|is:watched|is:owner|is:reviewer|has:.*)$");
- Matcher m = p.matcher(e.getMessage());
- if (m.matches()) {
- String op = m.group(1);
- throw new AuthException("Must be signed-in to use " + op);
- }
throw new BadRequestException(e.getMessage(), e);
}
return out.size() == 1 ? out.get(0) : out;
