gerlowskija commented on a change in pull request #427:
URL: https://github.com/apache/solr/pull/427#discussion_r757890304
##########
File path:
solr/core/src/test/org/apache/solr/security/BaseTestRuleBasedAuthorizationPlugin.java
##########
@@ -213,14 +212,14 @@ public void testBasicPermissions() {
"userPrincipal", "tim",
"handler", new ReplicationHandler(),
"collectionRequests", singletonList(new CollectionRequest("mycoll")) )
- , FORBIDDEN);
+ , STATUS_OK); // Replication requires "READ" permission, which Tim has
Review comment:
Thanks for these helpful explanations. Makes reviewing very easy.
##########
File path:
solr/core/src/java/org/apache/solr/security/RuleBasedAuthorizationPluginBase.java
##########
@@ -173,6 +173,7 @@ private boolean
predefinedPermissionAppliesToRequest(Permission predefinedPermis
log.trace("'ALL' perm applies to all requests; perm applies.");
return true; //'ALL' applies to everything!
} else if (! (context.getHandler() instanceof PermissionNameProvider)) {
+ // TODO: Is this code path needed anymore, now that all handlers
implement the interface? context.getHandler returns Object and is not documented
Review comment:
I'm fine leaving this in for the sake of caution.
That said, if we wanted to try removal, I guess we could rely on our tests a
bit here - add temporary `throw new RuntimeException("blah")` here and see if
any tests trigger it in local runs.
But maybe its not worth the hassle.
##########
File path:
solr/core/src/java/org/apache/solr/handler/admin/ZookeeperStatusHandler.java
##########
@@ -329,4 +330,9 @@ protected boolean validateZkRawResponse(List<String>
response, String zkHostPort
}
return true;
}
+
+ @Override
+ public Name getPermissionName(AuthorizationContext request) {
+ return Name.CONFIG_READ_PERM;
Review comment:
ditto, re: tweaking "CONFIG_READ" ref-guide docs to match this wider
definition
##########
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:
[Q] What does 'null' mean as a return value from
`PermissionNameProvider.getPermissionName`?
##########
File path: solr/solr-ref-guide/src/major-changes-in-solr-9.adoc
##########
@@ -80,6 +80,8 @@ All the usages are replaced by
BaseHttpSolrClient.RemoteSolrException and BaseHt
* SOLR-15409: Zookeeper client libraries upgraded to 3.7.0, which may not be
compatible with your existing server installations
+* SOLR-11623: Every request handler in Solr now implements
PermissionNameProvider. Any custom or 3rd party request handler must also do
this
Review comment:
+1. This should be implied, as users shouldn't be dragging any config
across major versions, but many will regardless so it's worth calling out IMO.
##########
File path:
solr/core/src/java/org/apache/solr/handler/admin/SystemInfoHandler.java
##########
@@ -420,7 +421,11 @@ private static String humanReadableUnits(long bytes,
DecimalFormat df) {
}
return list;
}
-
+
+ @Override
+ public Name getPermissionName(AuthorizationContext request) {
+ return Name.CONFIG_READ_PERM;
Review comment:
ditto, re: tweaking "CONFIG_READ" ref-guide docs to match this wider
definition
##########
File path:
solr/core/src/java/org/apache/solr/handler/admin/PropertiesRequestHandler.java
##########
@@ -75,4 +76,9 @@ public String getDescription() {
public Category getCategory() {
return Category.ADMIN;
}
+
+ @Override
+ public Name getPermissionName(AuthorizationContext request) {
+ return Name.CONFIG_READ_PERM;
Review comment:
[-1] The ref-guide describes CONFIG_READ as being scoped mostly to
collection level config. (see snippet from the 'Rule Based Authorization
Plugin' page below
> this permission is allowed to read a collection’s configuration using the
Config API, the Request Parameters API, and other APIs which modify
configoverlay.json
IMO widening this definition is totally OK if we don't want to create a new
system-level-config perm, but if we're going this route the ref-guide docs
should be updated to match.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
