janhoy commented on a change in pull request #427:
URL: https://github.com/apache/solr/pull/427#discussion_r753722612
##########
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:
The user "tim" has the `dev` and `admin` roles, and thus the permission
`read`, `collection-admin-edit` and `schema-edit`.
Before this PR, `/replication` was not covered by any particular permission,
so replication request was denied, since the user did not either have the `all`
permission. After the PR, access is allowed since he has `read`.
##########
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
checkRules(Map.of("resource", ReplicationHandler.PATH,
"httpMethod", "POST",
"userPrincipal", "cio",
"handler", new ReplicationHandler(),
"collectionRequests", singletonList(new CollectionRequest("mycoll")) )
- , STATUS_OK);
+ , FORBIDDEN); // User cio has role 'su' which does not have 'read'
permission
Review comment:
The user "cio" has only one role `su` with the `all` permission.
Before this PR, he was allowed access since `all` was the governing
permission for the request. After the PR, the governing permission becomes
`read` which is defined earlier in the permission list, and since cio lacks
that permission, it does not matter that he has the `all` permission, since
evaluation stops at the first governing permission.
This is a bit confusing, but it is by design and documented. However, this
change may cause people's existing `security.json` configs to behave
differently. Typically relying on some `all` permission at the end of the chain
connected to some role is no longer catching all these Request Handlers that
are now covered by a specific permission, so people may need to adjust role and
permission mappings to adjust.
Perhaps we should spell that out in the change-note.
##########
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:
Perhaps here and/or in upgrade-notes we should say that users may need
to adjust their roles and permission mappings to accommodate these changes.
##########
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 though of removing this code, but did not dare to, as I'm not sure if
`getHandler()` may perhaps still return a class not implementing the
interface...
--
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]
