You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@solr.apache.org by GitBox <gi...@apache.org> on 2021/10/26 14:44:15 UTC
[GitHub] [solr] janhoy opened a new pull request #372: SOLR-11623 Every request handler in Solr implement PermissionNameProvider
janhoy opened a new pull request #372:
URL: https://github.com/apache/solr/pull/372
https://issues.apache.org/jira/browse/SOLR-11623
This is a back-compat breaking change that likely will require lots of 3rd party request handlers out there to explicitly declare their permission requirement in order to be used with Solr 9. But I think we cannot let this be optional anymore.
I added one permission `HEALTH_PERM` for the `/node/health` and `/<coll>/ping` endpoints. That way you can easily open those to the world or connect them to a "health" role that can be given to e.g. solr-operator user. I thought of using the `METRICS_READ_PERM` but that reveals more data.
I struggle a bit with what perm to assign to our `/admin/info/system` handler, since it outputs some metrics, some config, all commandline args, some (insensitive) security info etc. I chose `CONFIG_READ_PERM` but this also means that all Admin UI users will need this permission to do almost anything, since Dashboard calls this. The same struggle goes for the `<coll>/analysis/field`, `<coll>/admin/thread` and several others. They reveal some config and sometimes some content. Please review whether things look sane.
The `ZookeeperInfoHandler` is special since it allows read of everything in Zookeeper. I tried to give it `CONFIG_READ_PERM`, unless you request `path=/security.json&detail=true`, then it will require `SECURITY_READ_PERM`.
Do we have other endpoints in the system that reveal Zookeeper content, where security.json might leak?
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] dsmiley commented on a change in pull request #372: SOLR-11623 Every request handler in Solr implement PermissionNameProvider
Posted by GitBox <gi...@apache.org>.
dsmiley commented on a change in pull request #372:
URL: https://github.com/apache/solr/pull/372#discussion_r737735230
##########
File path: solr/core/src/java/org/apache/solr/handler/admin/SegmentsInfoRequestHandler.java
##########
@@ -434,4 +435,9 @@ public String getDescription() {
public Category getCategory() {
return Category.ADMIN;
}
+
+ @Override
+ public Name getPermissionName(AuthorizationContext request) {
+ return Name.CONFIG_READ_PERM;
Review comment:
Is it really "config" (configuration)? I think it's metrics.
##########
File path: solr/core/src/java/org/apache/solr/handler/DumpRequestHandler.java
##########
@@ -126,4 +127,9 @@ public void init(NamedList<?> args) {
if (nl!=null) subpaths = nl.getAll("subpath");
}
}
+
+ @Override
+ public Name getPermissionName(AuthorizationContext request) {
+ return Name.CONFIG_READ_PERM;
Review comment:
Only with "remote streaming" would this be bad but that's disabled by default and should perhaps only be thought of as a local dev/POC scenario only.
##########
File path: solr/core/src/java/org/apache/solr/handler/admin/LoggingHandler.java
##########
@@ -168,4 +169,8 @@ public Category getCategory() {
return Category.ADMIN;
}
+ @Override
+ public Name getPermissionName(AuthorizationContext request) {
+ return Name.CONFIG_READ_PERM;
Review comment:
Well it can be WRITE. Can we vary this based on the HTTP verb? Yet sadly we permit mutations on GET...
##########
File path: solr/core/src/java/org/apache/solr/handler/DumpRequestHandler.java
##########
@@ -126,4 +127,9 @@ public void init(NamedList<?> args) {
if (nl!=null) subpaths = nl.getAll("subpath");
}
}
+
+ @Override
+ public Name getPermissionName(AuthorizationContext request) {
+ return Name.CONFIG_READ_PERM;
Review comment:
It just dumps the input back to output; right? Shouldn't the completely open?
##########
File path: solr/core/src/java/org/apache/solr/handler/admin/ThreadDumpHandler.java
##########
@@ -172,4 +173,9 @@ public String getDescription() {
public Category getCategory() {
return Category.ADMIN;
}
+
+ @Override
+ public Name getPermissionName(AuthorizationContext request) {
+ return Name.CONFIG_READ_PERM;
Review comment:
Is it really "config" (configuration)? I think it's metrics.
##########
File path: solr/core/src/java/org/apache/solr/handler/RequestHandlerBase.java
##########
@@ -51,9 +52,11 @@
import static org.apache.solr.core.RequestParams.USEPARAM;
/**
- *
+ * Base class for all request handlers.
+ * Since v9.0 all sub classes must implement {@link PermissionNameProvider} to decide on required security permissions
Review comment:
I don't think we should add a statement like that. APIs evolve over time and I'm dubious we need a chronology of that evolution in the javadocs.
##########
File path: solr/core/src/java/org/apache/solr/handler/admin/PluginInfoHandler.java
##########
@@ -82,4 +83,9 @@ public String getDescription() {
public Category getCategory() {
return Category.ADMIN;
}
+
+ @Override
+ public Name getPermissionName(AuthorizationContext request) {
+ return Name.METRICS_READ_PERM;
Review comment:
Why? This isn't obvious to me but may be right.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] janhoy commented on a change in pull request #372: SOLR-11623 Every request handler in Solr implement PermissionNameProvider
Posted by GitBox <gi...@apache.org>.
janhoy commented on a change in pull request #372:
URL: https://github.com/apache/solr/pull/372#discussion_r742980079
##########
File path: solr/core/src/java/org/apache/solr/handler/DocumentAnalysisRequestHandler.java
##########
@@ -345,4 +346,9 @@ private ContentStream extractSingleContentStream(SolrQueryRequest req) {
}
return stream;
}
+
+ @Override
+ public Name getPermissionName(AuthorizationContext request) {
+ return Name.READ_PERM;
Review comment:
Agree, I switched DocumentAnalysis to ALL
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] janhoy commented on a change in pull request #372: SOLR-11623 Every request handler in Solr implement PermissionNameProvider
Posted by GitBox <gi...@apache.org>.
janhoy commented on a change in pull request #372:
URL: https://github.com/apache/solr/pull/372#discussion_r742983203
##########
File path: solr/core/src/java/org/apache/solr/security/PermissionNameProvider.java
##########
@@ -48,6 +48,7 @@
SECURITY_EDIT_PERM("security-edit", null),
SECURITY_READ_PERM("security-read", null),
METRICS_READ_PERM("metrics-read", null),
+ HEALTH_PERM("health", unmodifiableSet(new HashSet<>(asList("*", null)))),
Review comment:
I'll defer Java11 cleanup to a separate issue.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] janhoy commented on a change in pull request #372: SOLR-11623 Every request handler in Solr implement PermissionNameProvider
Posted by GitBox <gi...@apache.org>.
janhoy commented on a change in pull request #372:
URL: https://github.com/apache/solr/pull/372#discussion_r742990786
##########
File path: solr/core/src/java/org/apache/solr/handler/DumpRequestHandler.java
##########
@@ -127,4 +135,19 @@ public void init(NamedList<?> args) {
if (nl!=null) subpaths = nl.getAll("subpath");
}
}
+
+ @Override
+ public Name getPermissionName(AuthorizationContext request) {
+ if (solrCore != null && solrCore.getSolrConfig().isEnableRemoteStreams()) {
+ log.warn("Dump request handler requires config-read permission when remote streams are enabled");
+ return Name.CONFIG_READ_PERM;
Review comment:
This shuold increase security a bit, in case remote streaming is enabled.. Although, if someone has permission to switch on remote streaming, they would likely also have the config-edit and config-read permissions...
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] janhoy commented on pull request #372: SOLR-11623 Every request handler in Solr implement PermissionNameProvider
Posted by GitBox <gi...@apache.org>.
janhoy commented on pull request #372:
URL: https://github.com/apache/solr/pull/372#issuecomment-966260473
Added 'health' permission to refguide docs. Going to merge this now. It's a 9.0 only change, and we can open futher PRs to refine and adjust if needed.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] janhoy commented on a change in pull request #372: SOLR-11623 Every request handler in Solr implement PermissionNameProvider
Posted by GitBox <gi...@apache.org>.
janhoy commented on a change in pull request #372:
URL: https://github.com/apache/solr/pull/372#discussion_r742757625
##########
File path: solr/core/src/java/org/apache/solr/handler/admin/ZookeeperReadAPI.java
##########
@@ -58,41 +59,44 @@
public class ZookeeperReadAPI {
private final CoreContainer coreContainer;
+ private SolrParams rawWtParams;
public ZookeeperReadAPI(CoreContainer coreContainer) {
this.coreContainer = coreContainer;
+ Map<String, String> map = new HashMap<>(1);
+ map.put(WT, "raw");
+ map.put(OMIT_HEADER, "true");
+ rawWtParams = new MapSolrParams(map);
}
+
+ /**
+ * Request contents of a znode, except security.json
+ */
@EndPoint(path = "/cluster/zk/data/*",
method = SolrRequest.METHOD.GET,
permission = ZK_READ_PERM)
public void readNode(SolrQueryRequest req, SolrQueryResponse rsp) {
String path = req.getPathTemplateValues().get("*");
if (path == null || path.isEmpty()) path = "/";
- byte[] d = null;
- try {
- d = coreContainer.getZkController().getZkClient().getData(path, null, null, false);
- } catch (KeeperException.NoNodeException e) {
- throw new SolrException(SolrException.ErrorCode.NOT_FOUND, "No such node: " + path);
- } catch (Exception e) {
- throw new SolrException(SolrException.ErrorCode.SERVER_ERROR, "Unexpected error", e);
- }
- if (d == null || d.length == 0) {
- rsp.add(path, null);
- return;
- }
-
- Map<String, String> map = new HashMap<>(1);
- map.put(WT, "raw");
- map.put(OMIT_HEADER, "true");
- req.setParams(SolrParams.wrapDefaults(new MapSolrParams(map), req.getParams()));
-
- String mime = BinaryResponseParser.BINARY_CONTENT_TYPE;
+ req.setParams(SolrParams.wrapDefaults(rawWtParams, req.getParams()));
+ readNodeAndAddToResponse(path, rsp);
+ }
- if (d[0] == '{') mime = CommonParams.JSON_MIME;
- if (d[0] == '<' || d[1] == '?') mime = XMLResponseParser.XML_CONTENT_TYPE;
- rsp.add(CONTENT, new ContentStreamBase.ByteArrayStream(d, null, mime));
+ /**
+ * Request contents of the security.json node
+ */
+ @EndPoint(path = "/cluster/zk/data/security.json",
+ method = SolrRequest.METHOD.GET,
+ permission = SECURITY_READ_PERM)
+ public void readSecurityJsonNode(SolrQueryRequest req, SolrQueryResponse rsp) {
Review comment:
@noblepaul This actually worked. Solr chooses the most specific `@EndPoint` first, thus when I request `/security.json`, this method gets called, with a different permission. The other end point is the catch-all for all other paths.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] janhoy commented on a change in pull request #372: SOLR-11623 Every request handler in Solr implement PermissionNameProvider
Posted by GitBox <gi...@apache.org>.
janhoy commented on a change in pull request #372:
URL: https://github.com/apache/solr/pull/372#discussion_r743000618
##########
File path: solr/core/src/java/org/apache/solr/handler/admin/LukeRequestHandler.java
##########
@@ -105,6 +106,11 @@
static final int HIST_ARRAY_SIZE = 33;
+ @Override
+ public Name getPermissionName(AuthorizationContext request) {
+ return Name.CONFIG_READ_PERM;
Review comment:
Done.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] janhoy commented on a change in pull request #372: SOLR-11623 Every request handler in Solr implement PermissionNameProvider
Posted by GitBox <gi...@apache.org>.
janhoy commented on a change in pull request #372:
URL: https://github.com/apache/solr/pull/372#discussion_r746157330
##########
File path: solr/core/src/java/org/apache/solr/handler/RequestHandlerBase.java
##########
@@ -51,9 +52,10 @@
import static org.apache.solr.core.RequestParams.USEPARAM;
/**
- *
+ * Base class for all request handlers.
*/
-public abstract class RequestHandlerBase implements SolrRequestHandler, SolrInfoBean, NestedRequestHandler, ApiSupport {
+public abstract class RequestHandlerBase implements
Review comment:
> So does the Schema screen requires "read" to show the screen? That's an interesting screen... it can expose data in terms of field values in term statistics but otherwise it's mostly a "config-read" tool. I don't recall how this screen maps to handlers.
I just checked what end points are queried when clicking around in the UI. Turns out that the collection-read and core-read are always queried to fill the collection/core dropdowns. It should of course as a SPA remember the values and not re-query every time, but I did not dive in to try to fix that.
The Schema screen uses Luke, and Luke requires the READ permission. I think it fetches all field names etc from luke as well, so the screen would be pretty useless without access to Luke.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] janhoy commented on a change in pull request #372: SOLR-11623 Every request handler in Solr implement PermissionNameProvider
Posted by GitBox <gi...@apache.org>.
janhoy commented on a change in pull request #372:
URL: https://github.com/apache/solr/pull/372#discussion_r746154948
##########
File path: solr/core/src/java/org/apache/solr/handler/RequestHandlerBase.java
##########
@@ -51,9 +52,10 @@
import static org.apache.solr.core.RequestParams.USEPARAM;
/**
- *
+ * Base class for all request handlers.
*/
-public abstract class RequestHandlerBase implements SolrRequestHandler, SolrInfoBean, NestedRequestHandler, ApiSupport {
+public abstract class RequestHandlerBase implements
Review comment:
Forcing each new request handler to make a permission decision ensures there is a concious decision and not just some default.
What does it even mean for an endpoint to declare the ALL permission?
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] dsmiley commented on a change in pull request #372: SOLR-11623 Every request handler in Solr implement PermissionNameProvider
Posted by GitBox <gi...@apache.org>.
dsmiley commented on a change in pull request #372:
URL: https://github.com/apache/solr/pull/372#discussion_r743855022
##########
File path: solr/core/src/java/org/apache/solr/handler/PingRequestHandler.java
##########
@@ -133,6 +134,12 @@
private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
public static final String HEALTHCHECK_FILE_PARAM = "healthcheckFile";
+
+ @Override
+ public Name getPermissionName(AuthorizationContext request) {
+ return Name.HEALTH_PERM;
Review comment:
good catch!
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] dsmiley commented on a change in pull request #372: SOLR-11623 Every request handler in Solr implement PermissionNameProvider
Posted by GitBox <gi...@apache.org>.
dsmiley commented on a change in pull request #372:
URL: https://github.com/apache/solr/pull/372#discussion_r738347507
##########
File path: solr/core/src/java/org/apache/solr/security/PermissionNameProvider.java
##########
@@ -48,6 +48,7 @@
SECURITY_EDIT_PERM("security-edit", null),
SECURITY_READ_PERM("security-read", null),
METRICS_READ_PERM("metrics-read", null),
+ HEALTH_PERM("health", unmodifiableSet(new HashSet<>(asList("*", null)))),
Review comment:
Please use Java 11 stuff now, like Set.of("*", null)
##########
File path: solr/core/src/java/org/apache/solr/handler/DocumentAnalysisRequestHandler.java
##########
@@ -345,4 +346,9 @@ private ContentStream extractSingleContentStream(SolrQueryRequest req) {
}
return stream;
}
+
+ @Override
+ public Name getPermissionName(AuthorizationContext request) {
+ return Name.READ_PERM;
Review comment:
I looked and I don't see this handler using the index, it only analyzes the input and returns it. Text Analysis as a Service. ALL perm seems appropriate.
##########
File path: solr/core/src/java/org/apache/solr/handler/admin/LukeRequestHandler.java
##########
@@ -105,6 +106,11 @@
static final int HIST_ARRAY_SIZE = 33;
+ @Override
+ public Name getPermissionName(AuthorizationContext request) {
+ return Name.CONFIG_READ_PERM;
Review comment:
It exposes data so should be READ_PERM; right? Perhaps you are thinking CONFIG_READ_PERM implies READ_PERM but I don't think so (not where I work; customer data is restricted to us engineers even).
##########
File path: solr/core/src/java/org/apache/solr/handler/DumpRequestHandler.java
##########
@@ -126,4 +127,9 @@ public void init(NamedList<?> args) {
if (nl!=null) subpaths = nl.getAll("subpath");
}
}
+
+ @Override
+ public Name getPermissionName(AuthorizationContext request) {
+ return Name.CONFIG_READ_PERM;
Review comment:
Given remote streaming is off by default and we warn about enabling it, lets assume it's not used and just return ALL.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] janhoy commented on pull request #372: SOLR-11623 Every request handler in Solr implement PermissionNameProvider
Posted by GitBox <gi...@apache.org>.
janhoy commented on pull request #372:
URL: https://github.com/apache/solr/pull/372#issuecomment-962163811
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] janhoy commented on a change in pull request #372: SOLR-11623 Every request handler in Solr implement PermissionNameProvider
Posted by GitBox <gi...@apache.org>.
janhoy commented on a change in pull request #372:
URL: https://github.com/apache/solr/pull/372#discussion_r742993915
##########
File path: solr/core/src/java/org/apache/solr/handler/admin/InfoHandler.java
##########
@@ -157,4 +158,17 @@ public SolrRequestHandler getSubHandler(String subPath) {
public Boolean registerV2() {
return Boolean.TRUE;
}
+
+ @Override
+ public Name getPermissionName(AuthorizationContext request) {
+ // Delegate permission to the actual handler
+ String path = request.getResource();
+ String lastPath = path.substring(path.lastIndexOf("/") +1 );
+ RequestHandlerBase handler = handlers.get(lastPath.toLowerCase(Locale.ROOT));
+ if (handler != null) {
+ return handler.getPermissionName(request);
+ } else {
+ return null;
Review comment:
Not entirely sure what to do in case the path is unknown, e.g `/admin/info/foo`. Shoudl we return `null`, `ALL` or throw an exception? @noblepaul is this correct use of 'null'? Since the handler does not exist, the handleRequest() call will likely return an error anyway, so it does not really matter what we do here.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] janhoy merged pull request #372: SOLR-11623 Every request handler in Solr implement PermissionNameProvider
Posted by GitBox <gi...@apache.org>.
janhoy merged pull request #372:
URL: https://github.com/apache/solr/pull/372
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] janhoy commented on a change in pull request #372: SOLR-11623 Every request handler in Solr implement PermissionNameProvider
Posted by GitBox <gi...@apache.org>.
janhoy commented on a change in pull request #372:
URL: https://github.com/apache/solr/pull/372#discussion_r742982104
##########
File path: solr/core/src/java/org/apache/solr/handler/admin/LukeRequestHandler.java
##########
@@ -105,6 +106,11 @@
static final int HIST_ARRAY_SIZE = 33;
+ @Override
+ public Name getPermissionName(AuthorizationContext request) {
+ return Name.CONFIG_READ_PERM;
Review comment:
It also exposes schema which is configuration. But I guess data is more sensitive, so better choose READ for this. Then users can setup custom permissions that require diferent roles to read data from different collections, and this will obey.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] janhoy commented on a change in pull request #372: SOLR-11623 Every request handler in Solr implement PermissionNameProvider
Posted by GitBox <gi...@apache.org>.
janhoy commented on a change in pull request #372:
URL: https://github.com/apache/solr/pull/372#discussion_r742993915
##########
File path: solr/core/src/java/org/apache/solr/handler/admin/InfoHandler.java
##########
@@ -157,4 +158,17 @@ public SolrRequestHandler getSubHandler(String subPath) {
public Boolean registerV2() {
return Boolean.TRUE;
}
+
+ @Override
+ public Name getPermissionName(AuthorizationContext request) {
+ // Delegate permission to the actual handler
+ String path = request.getResource();
+ String lastPath = path.substring(path.lastIndexOf("/") +1 );
+ RequestHandlerBase handler = handlers.get(lastPath.toLowerCase(Locale.ROOT));
+ if (handler != null) {
+ return handler.getPermissionName(request);
+ } else {
+ return null;
Review comment:
Not entirely sure what to do in case the path is unknown, e.g `/admin/info/foo`. Shoudl we return `null`, `ALL` or throw an exception?
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] janhoy commented on a change in pull request #372: SOLR-11623 Every request handler in Solr implement PermissionNameProvider
Posted by GitBox <gi...@apache.org>.
janhoy commented on a change in pull request #372:
URL: https://github.com/apache/solr/pull/372#discussion_r742975679
##########
File path: solr/core/src/java/org/apache/solr/handler/admin/SegmentsInfoRequestHandler.java
##########
@@ -434,4 +435,9 @@ public String getDescription() {
public Category getCategory() {
return Category.ADMIN;
}
+
+ @Override
+ public Name getPermissionName(AuthorizationContext request) {
+ return Name.CONFIG_READ_PERM;
Review comment:
I had to change the predefined perm `METRICS_READ_PERM` to `'*' or null` so that it will work both for node-level handler and for collection-level handler such as <coll>/admin/segments. This authz system is quite trappy!
So now the segments handler is guarded by metrics-read. I also tested the ping handler, and it is guarded by the new 'health' permission.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] janhoy commented on a change in pull request #372: SOLR-11623 Every request handler in Solr implement PermissionNameProvider
Posted by GitBox <gi...@apache.org>.
janhoy commented on a change in pull request #372:
URL: https://github.com/apache/solr/pull/372#discussion_r743000618
##########
File path: solr/core/src/java/org/apache/solr/handler/admin/LukeRequestHandler.java
##########
@@ -105,6 +106,11 @@
static final int HIST_ARRAY_SIZE = 33;
+ @Override
+ public Name getPermissionName(AuthorizationContext request) {
+ return Name.CONFIG_READ_PERM;
Review comment:
Done.
##########
File path: solr/core/src/java/org/apache/solr/handler/PingRequestHandler.java
##########
@@ -133,6 +134,12 @@
private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
public static final String HEALTHCHECK_FILE_PARAM = "healthcheckFile";
+
+ @Override
+ public Name getPermissionName(AuthorizationContext request) {
+ return Name.HEALTH_PERM;
Review comment:
From the javadoc of PingRequestHandler... This is a write operation, so perhaps `action=enable` and `action=disable` should be guarded by `config-edit`? Else, it can be used as an attack to disconnect all solr nodes from a Load Balancer :)
##########
File path: solr/core/src/java/org/apache/solr/handler/PingRequestHandler.java
##########
@@ -133,6 +134,12 @@
private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
public static final String HEALTHCHECK_FILE_PARAM = "healthcheckFile";
+
+ @Override
+ public Name getPermissionName(AuthorizationContext request) {
+ return Name.HEALTH_PERM;
Review comment:
I aded a test for "action" as proposed, and just tested locally that it works.
##########
File path: solr/core/src/java/org/apache/solr/handler/PingRequestHandler.java
##########
@@ -133,6 +134,12 @@
private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
public static final String HEALTHCHECK_FILE_PARAM = "healthcheckFile";
+
+ @Override
+ public Name getPermissionName(AuthorizationContext request) {
+ return Name.HEALTH_PERM;
Review comment:
From the javadoc of PingRequestHandler... This is a write operation, so perhaps `action=enable` and `action=disable` should be guarded by `config-edit`? Else, it can be used as an attack to disconnect all solr nodes from a Load Balancer :)
##########
File path: solr/core/src/java/org/apache/solr/handler/PingRequestHandler.java
##########
@@ -133,6 +134,12 @@
private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
public static final String HEALTHCHECK_FILE_PARAM = "healthcheckFile";
+
+ @Override
+ public Name getPermissionName(AuthorizationContext request) {
+ return Name.HEALTH_PERM;
Review comment:
I aded a test for "action" as proposed, and just tested locally that it works.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] janhoy commented on a change in pull request #372: SOLR-11623 Every request handler in Solr implement PermissionNameProvider
Posted by GitBox <gi...@apache.org>.
janhoy commented on a change in pull request #372:
URL: https://github.com/apache/solr/pull/372#discussion_r737870861
##########
File path: solr/core/src/java/org/apache/solr/handler/admin/ZookeeperInfoHandler.java
##########
@@ -103,6 +104,18 @@ public Category getCategory() {
return Category.ADMIN;
}
+ @Override
+ public Name getPermissionName(AuthorizationContext request) {
+ var params = request.getParams();
+ var path = params.get("path", "");
+ var detail = params.get("detail", "false");
+ if ("/security.json".equalsIgnoreCase(path) && "true".equalsIgnoreCase(detail)) {
+ return Name.SECURITY_READ_PERM;
+ } else {
+ return Name.CONFIG_READ_PERM;
Review comment:
Hmm, this handler reads all of ZK, which mostly contains configuration. But there is also a `ZK_READ_PERM` available, so if we have to pick only one, perhaps that should be used instead. here.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] dsmiley commented on a change in pull request #372: SOLR-11623 Every request handler in Solr implement PermissionNameProvider
Posted by GitBox <gi...@apache.org>.
dsmiley commented on a change in pull request #372:
URL: https://github.com/apache/solr/pull/372#discussion_r737860732
##########
File path: solr/core/src/java/org/apache/solr/handler/admin/SegmentsInfoRequestHandler.java
##########
@@ -434,4 +435,9 @@ public String getDescription() {
public Category getCategory() {
return Category.ADMIN;
}
+
+ @Override
+ public Name getPermissionName(AuthorizationContext request) {
+ return Name.CONFIG_READ_PERM;
Review comment:
I confess I haven't really worked with Solr authorization, but my intuition is that some of our permissions should imply having others. If you can read configuration, why withhold metrics? Yet the converse is not true. Config can be more sensitive.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] janhoy commented on pull request #372: SOLR-11623 Every request handler in Solr implement PermissionNameProvider
Posted by GitBox <gi...@apache.org>.
janhoy commented on pull request #372:
URL: https://github.com/apache/solr/pull/372#issuecomment-962915819
Here is a matrix of what permissions each Admin screen now requires. If something seems odd, shout out.
https://docs.google.com/spreadsheets/d/1s2xokDxw9IkXr7ZA5n06RPDj6EwvpbsZ7zUeKpvRC3Q/edit?usp=sharing
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] janhoy commented on a change in pull request #372: SOLR-11623 Every request handler in Solr implement PermissionNameProvider
Posted by GitBox <gi...@apache.org>.
janhoy commented on a change in pull request #372:
URL: https://github.com/apache/solr/pull/372#discussion_r743840541
##########
File path: solr/core/src/java/org/apache/solr/handler/PingRequestHandler.java
##########
@@ -133,6 +134,12 @@
private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
public static final String HEALTHCHECK_FILE_PARAM = "healthcheckFile";
+
+ @Override
+ public Name getPermissionName(AuthorizationContext request) {
+ return Name.HEALTH_PERM;
Review comment:
From the javadoc of PingRequestHandler... This is a write operation, so perhaps `action=enable` and `action=disable` should be guarded by `config-edit`? Else, it can be used as an attack to disconnect all solr nodes from a Load Balancer :)
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] janhoy commented on pull request #372: SOLR-11623 Every request handler in Solr implement PermissionNameProvider
Posted by GitBox <gi...@apache.org>.
janhoy commented on pull request #372:
URL: https://github.com/apache/solr/pull/372#issuecomment-964007288
I plan to merge this on Thursday.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] janhoy commented on a change in pull request #372: SOLR-11623 Every request handler in Solr implement PermissionNameProvider
Posted by GitBox <gi...@apache.org>.
janhoy commented on a change in pull request #372:
URL: https://github.com/apache/solr/pull/372#discussion_r748080017
##########
File path: solr/core/src/java/org/apache/solr/handler/RequestHandlerBase.java
##########
@@ -51,9 +52,10 @@
import static org.apache.solr.core.RequestParams.USEPARAM;
/**
- *
+ * Base class for all request handlers.
*/
-public abstract class RequestHandlerBase implements SolrRequestHandler, SolrInfoBean, NestedRequestHandler, ApiSupport {
+public abstract class RequestHandlerBase implements
Review comment:
@madrob How does the `ALL` permission work? If we made it the default, how would that be different from a handler not implementing the `PermissionNameProvider` interface at all? And how is that again different from `null`?
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] janhoy commented on pull request #372: SOLR-11623 Every request handler in Solr implement PermissionNameProvider
Posted by GitBox <gi...@apache.org>.
janhoy commented on pull request #372:
URL: https://github.com/apache/solr/pull/372#issuecomment-953256267
I discovered that the `ZookeeperReadAPI` clas exposes ZK on `/cluster/zk/data`, and it uses `ZK_READ_PERM` in the `@EndPoint` annotation. What if someone requests `/cluster/zk/data/security.json`, then they should really have `SECURITY_READ_PERM`. But I think with the `@EndPoint` way of assigning permissions, there is no way of assigning more fine-grained permissions based on path. @noblepaul is it possible to lock down only the `/security.json` path by declaring another method with `@EndPoint(path = "/cluster/zk/data/security.json"...` and permission `SECURITY_READ_PERM`? Will it take precedence over the `path = "/cluster/zk/data/*"` one?
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] janhoy commented on a change in pull request #372: SOLR-11623 Every request handler in Solr implement PermissionNameProvider
Posted by GitBox <gi...@apache.org>.
janhoy commented on a change in pull request #372:
URL: https://github.com/apache/solr/pull/372#discussion_r737795303
##########
File path: solr/core/src/java/org/apache/solr/handler/DumpRequestHandler.java
##########
@@ -126,4 +127,9 @@ public void init(NamedList<?> args) {
if (nl!=null) subpaths = nl.getAll("subpath");
}
}
+
+ @Override
+ public Name getPermissionName(AuthorizationContext request) {
+ return Name.CONFIG_READ_PERM;
Review comment:
Reminds me of https://issues.apache.org/jira/browse/SOLR-14853. What if we made it a global option, then we could change the permission needed based on whether remote streaming is enabled or not? But I'm open to the idea of changing it to `ALL`.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] janhoy commented on a change in pull request #372: SOLR-11623 Every request handler in Solr implement PermissionNameProvider
Posted by GitBox <gi...@apache.org>.
janhoy commented on a change in pull request #372:
URL: https://github.com/apache/solr/pull/372#discussion_r742765070
##########
File path: solr/core/src/java/org/apache/solr/handler/DumpRequestHandler.java
##########
@@ -126,4 +127,9 @@ public void init(NamedList<?> args) {
if (nl!=null) subpaths = nl.getAll("subpath");
}
}
+
+ @Override
+ public Name getPermissionName(AuthorizationContext request) {
+ return Name.CONFIG_READ_PERM;
Review comment:
I committed a solution where it is wide open, unless remote streaming is enabled for the core, and then we require CONFIG_READ_PERM.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] dsmiley commented on a change in pull request #372: SOLR-11623 Every request handler in Solr implement PermissionNameProvider
Posted by GitBox <gi...@apache.org>.
dsmiley commented on a change in pull request #372:
URL: https://github.com/apache/solr/pull/372#discussion_r743855022
##########
File path: solr/core/src/java/org/apache/solr/handler/PingRequestHandler.java
##########
@@ -133,6 +134,12 @@
private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
public static final String HEALTHCHECK_FILE_PARAM = "healthcheckFile";
+
+ @Override
+ public Name getPermissionName(AuthorizationContext request) {
+ return Name.HEALTH_PERM;
Review comment:
good catch!
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] madrob commented on a change in pull request #372: SOLR-11623 Every request handler in Solr implement PermissionNameProvider
Posted by GitBox <gi...@apache.org>.
madrob commented on a change in pull request #372:
URL: https://github.com/apache/solr/pull/372#discussion_r746072094
##########
File path: solr/core/src/java/org/apache/solr/handler/RequestHandlerBase.java
##########
@@ -51,9 +52,10 @@
import static org.apache.solr.core.RequestParams.USEPARAM;
/**
- *
+ * Base class for all request handlers.
*/
-public abstract class RequestHandlerBase implements SolrRequestHandler, SolrInfoBean, NestedRequestHandler, ApiSupport {
+public abstract class RequestHandlerBase implements
Review comment:
would we want to have a default implementation returning ALL?
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] janhoy commented on a change in pull request #372: SOLR-11623 Every request handler in Solr implement PermissionNameProvider
Posted by GitBox <gi...@apache.org>.
janhoy commented on a change in pull request #372:
URL: https://github.com/apache/solr/pull/372#discussion_r737866257
##########
File path: solr/core/src/java/org/apache/solr/handler/admin/SegmentsInfoRequestHandler.java
##########
@@ -434,4 +435,9 @@ public String getDescription() {
public Category getCategory() {
return Category.ADMIN;
}
+
+ @Override
+ public Name getPermissionName(AuthorizationContext request) {
+ return Name.CONFIG_READ_PERM;
Review comment:
This was also commented in a ML thread recently. Any WRITE/EDIT permission should imply READ. And perhaps other implicit hierarchies as well.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] janhoy commented on a change in pull request #372: SOLR-11623 Every request handler in Solr implement PermissionNameProvider
Posted by GitBox <gi...@apache.org>.
janhoy commented on a change in pull request #372:
URL: https://github.com/apache/solr/pull/372#discussion_r737870861
##########
File path: solr/core/src/java/org/apache/solr/handler/admin/ZookeeperInfoHandler.java
##########
@@ -103,6 +104,18 @@ public Category getCategory() {
return Category.ADMIN;
}
+ @Override
+ public Name getPermissionName(AuthorizationContext request) {
+ var params = request.getParams();
+ var path = params.get("path", "");
+ var detail = params.get("detail", "false");
+ if ("/security.json".equalsIgnoreCase(path) && "true".equalsIgnoreCase(detail)) {
+ return Name.SECURITY_READ_PERM;
+ } else {
+ return Name.CONFIG_READ_PERM;
Review comment:
Hmm, this handler reads all of ZK, which mostly contains configuration. But there is also a `ZK_READ_PERM` available, so if we have to pick only one... I'll swtich to `ZK_READ_PERM`
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] dsmiley commented on pull request #372: SOLR-11623 Every request handler in Solr implement PermissionNameProvider
Posted by GitBox <gi...@apache.org>.
dsmiley commented on pull request #372:
URL: https://github.com/apache/solr/pull/372#issuecomment-964542435
There spreadsheet confuses me a bit. There there are two columns that have a blue for the same row, are both needed or just one to show the screen?
So does the Schema screen requires "read" to show the screen? That's an interesting screen... it can expose data in terms of field values in term statistics but otherwise it's mostly a "config-read" tool. I don't recall how this screen maps to handlers.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] janhoy commented on a change in pull request #372: SOLR-11623 Every request handler in Solr implement PermissionNameProvider
Posted by GitBox <gi...@apache.org>.
janhoy commented on a change in pull request #372:
URL: https://github.com/apache/solr/pull/372#discussion_r737864469
##########
File path: solr/core/src/java/org/apache/solr/handler/admin/ThreadDumpHandler.java
##########
@@ -172,4 +173,9 @@ public String getDescription() {
public Category getCategory() {
return Category.ADMIN;
}
+
+ @Override
+ public Name getPermissionName(AuthorizationContext request) {
+ return Name.CONFIG_READ_PERM;
Review comment:
Hmm, you list all threads, and for each thread there are some metrics like cpuTime, STATE etc. So guess, apart from a user in AdminUI, some metrics applications like PromExporter may have interest in this endpoint.. I'll switch it to METRICS.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] janhoy commented on a change in pull request #372: SOLR-11623 Every request handler in Solr implement PermissionNameProvider
Posted by GitBox <gi...@apache.org>.
janhoy commented on a change in pull request #372:
URL: https://github.com/apache/solr/pull/372#discussion_r742992355
##########
File path: solr/core/src/java/org/apache/solr/handler/PingRequestHandler.java
##########
@@ -133,6 +134,12 @@
private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
public static final String HEALTHCHECK_FILE_PARAM = "healthcheckFile";
+
+ @Override
+ public Name getPermissionName(AuthorizationContext request) {
+ return Name.HEALTH_PERM;
Review comment:
Thinking about this - should we rather use `metrics-read` here instead of inventing a new permission? Health status is one type of metric.. Only reason to distinguish is if you simply want to allow an external Load Balancer or something to know whether Solr is alive, but don't want to leak all metrics...
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] epugh commented on pull request #372: SOLR-11623 Every request handler in Solr implement PermissionNameProvider
Posted by GitBox <gi...@apache.org>.
epugh commented on pull request #372:
URL: https://github.com/apache/solr/pull/372#issuecomment-963146439
I put two comments into your spreadsheet... I wonder if we should link to that spreadsheet from the Ref Guide? It's a great visual to map out what permissions do what!
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] janhoy commented on a change in pull request #372: SOLR-11623 Every request handler in Solr implement PermissionNameProvider
Posted by GitBox <gi...@apache.org>.
janhoy commented on a change in pull request #372:
URL: https://github.com/apache/solr/pull/372#discussion_r742819554
##########
File path: solr/core/src/java/org/apache/solr/handler/admin/LoggingHandler.java
##########
@@ -168,4 +169,8 @@ public Category getCategory() {
return Category.ADMIN;
}
+ @Override
+ public Name getPermissionName(AuthorizationContext request) {
+ return Name.CONFIG_READ_PERM;
Review comment:
Actually, I found a bug here while testing. Turns out that `InfoHandler` is hack and contains an internal map of its sub handlers like LoggingHandler.
I had to add custom code to `getPermissionName()` and delgate the call to the actual sub handler. Now it seems to work.
E.g. in admin UI you cannot change log-level unless you have config-edit permission. But there is no clear error-message in UI (yet), only the standard red box on top. Granular HTTP 403 errors in UI can come later. Perhpas you have something planned, @thelabdude ?
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] dsmiley commented on a change in pull request #372: SOLR-11623 Every request handler in Solr implement PermissionNameProvider
Posted by GitBox <gi...@apache.org>.
dsmiley commented on a change in pull request #372:
URL: https://github.com/apache/solr/pull/372#discussion_r743855022
##########
File path: solr/core/src/java/org/apache/solr/handler/PingRequestHandler.java
##########
@@ -133,6 +134,12 @@
private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
public static final String HEALTHCHECK_FILE_PARAM = "healthcheckFile";
+
+ @Override
+ public Name getPermissionName(AuthorizationContext request) {
+ return Name.HEALTH_PERM;
Review comment:
good catch!
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] janhoy commented on a change in pull request #372: SOLR-11623 Every request handler in Solr implement PermissionNameProvider
Posted by GitBox <gi...@apache.org>.
janhoy commented on a change in pull request #372:
URL: https://github.com/apache/solr/pull/372#discussion_r737800224
##########
File path: solr/core/src/java/org/apache/solr/handler/admin/LoggingHandler.java
##########
@@ -168,4 +169,8 @@ public Category getCategory() {
return Category.ADMIN;
}
+ @Override
+ public Name getPermissionName(AuthorizationContext request) {
+ return Name.CONFIG_READ_PERM;
Review comment:
If the `set` param is present, then it is write. We can check that. See latest commit, have not yet tested if it works.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] janhoy commented on a change in pull request #372: SOLR-11623 Every request handler in Solr implement PermissionNameProvider
Posted by GitBox <gi...@apache.org>.
janhoy commented on a change in pull request #372:
URL: https://github.com/apache/solr/pull/372#discussion_r737857540
##########
File path: solr/core/src/java/org/apache/solr/handler/admin/SegmentsInfoRequestHandler.java
##########
@@ -434,4 +435,9 @@ public String getDescription() {
public Category getCategory() {
return Category.ADMIN;
}
+
+ @Override
+ public Name getPermissionName(AuthorizationContext request) {
+ return Name.CONFIG_READ_PERM;
Review comment:
I think I agree that this is metrics. But if you have search (READ) access and CONFIG_READ permission so you can use the Admin UI, should you then be denied viewing the segment page if you lack METRICS permission? I guess people will learn this and assign a role with necessary permissions. I'll switch this to `METRICS_READ_PERM`.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] janhoy commented on a change in pull request #372: SOLR-11623 Every request handler in Solr implement PermissionNameProvider
Posted by GitBox <gi...@apache.org>.
janhoy commented on a change in pull request #372:
URL: https://github.com/apache/solr/pull/372#discussion_r737863235
##########
File path: solr/core/src/java/org/apache/solr/handler/admin/PluginInfoHandler.java
##########
@@ -82,4 +83,9 @@ public String getDescription() {
public Category getCategory() {
return Category.ADMIN;
}
+
+ @Override
+ public Name getPermissionName(AuthorizationContext request) {
+ return Name.METRICS_READ_PERM;
Review comment:
The `/solr/<coll>/admin/plugins` endpoint lists all other implicit endpoints (plugins) available, and by adding `stats=true` you get all kids of stats for the MBeans. So this smells metrics to me.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] janhoy commented on a change in pull request #372: SOLR-11623 Every request handler in Solr implement PermissionNameProvider
Posted by GitBox <gi...@apache.org>.
janhoy commented on a change in pull request #372:
URL: https://github.com/apache/solr/pull/372#discussion_r737795928
##########
File path: solr/core/src/java/org/apache/solr/handler/RequestHandlerBase.java
##########
@@ -51,9 +52,11 @@
import static org.apache.solr.core.RequestParams.USEPARAM;
/**
- *
+ * Base class for all request handlers.
+ * Since v9.0 all sub classes must implement {@link PermissionNameProvider} to decide on required security permissions
Review comment:
Sure, can remove.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org