----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/56179/#review163860 -----------------------------------------------------------
Ship it! Overall, this patch looks fine to me. In general, I'd prefer to keep from having a copy of a class like this in a separate project, and would prefer to have the Ambari code just subclass/extend the Solr plugin being reused here. That being said, since the related methods are all private in the Solr auth plugin, that may not be feasible. Are we sure that the licensing for this plugin makes it ok to add it to a separate project? It looks like an Apache license, but it makes sense to verify this during the review. Thanks. ambari-logsearch/ambari-infra-solr-plugin/src/main/java/org.apache.ambari.infra.security/InfraRuleBasedAuthorizationPlugin.java (line 63) <https://reviews.apache.org/r/56179/#comment235342> Would there be any cases in the future where we might want to make this a pluggable strategy? This current implementation is fine, but I was just wondering if it would be worth the effort to abstract out this operation into an interface, and have the plugin class only handle that. If Solr doesn't provide a way to configure the creation of this plugin instance, then perhaps this wouldn't be helpful anyway. I'm not opening this as an issue, rather I'm just trying to see if this can be more flexible. If there's no benefit to abstracting out the operation, then this code should stay as-is. - Robert Nettleton On Feb. 1, 2017, 5:16 p.m., Oliver Szabo wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/56179/ > ----------------------------------------------------------- > > (Updated Feb. 1, 2017, 5:16 p.m.) > > > Review request for Ambari, Miklos Gergely, Robert Nettleton, and Sebastian > Toader. > > > Bugs: AMBARI-19822 > https://issues.apache.org/jira/browse/AMBARI-19822 > > > Repository: ambari > > > Description > ------- > > Problem: > If an ambari cluster is secured and kerberos authentication is used for Solr, > we need (default) authorizations as well to make sure only the specific > service users (ranger, atlas, logsearch) can access their collections (and > solr user as well) > > Solution: > Although RuleBasedAuthorizationPlugin seems to be a good solution here, to > map default users to default permissions, unfortunately, permissions and > roles using principal name for mapping (not username) from the authentication > tokens. Also Solr name rules applied on the username and not on the > principal, therefore we need the fully qualified hostname as well in the > role-permission mapping. In order to avoid that issue, I added an own plugin > (org.apache.ambari.infra.security.InfraRuleBasedAuthorizationPlugin), to map > users with <name>@<DOMAIN> format. > > to problem is in here in RuleBasedAuthorizationPlugin.java: > https://github.com/apache/lucene-solr/blob/releases/lucene-solr/5.5.2/solr/core/src/java/org/apache/solr/security/RuleBasedAuthorizationPlugin.java#L153 > > notice that InfraRuleBasedAuthorizationPlugin is only a copy of that file > (InfraUserRolesLookupStrategy class which I added and included in the new > plugin class) > > > Diffs > ----- > > ambari-logsearch/ambari-infra-solr-plugin/pom.xml PRE-CREATION > > ambari-logsearch/ambari-infra-solr-plugin/src/main/java/org.apache.ambari.infra.security/InfraRuleBasedAuthorizationPlugin.java > PRE-CREATION > > ambari-logsearch/ambari-infra-solr-plugin/src/main/java/org.apache.ambari.infra.security/InfraUserRolesLookupStrategy.java > PRE-CREATION > > ambari-logsearch/ambari-infra-solr-plugin/src/test/java/org/apache/ambari/infra/security/InfraRuleBasedAuthorizationPluginTest.java > PRE-CREATION > > ambari-logsearch/ambari-infra-solr-plugin/src/test/java/org/apache/ambari/infra/security/InfraUserRolesLookupStrategyTest.java > PRE-CREATION > ambari-logsearch/ambari-logsearch-assembly/pom.xml c486050 > ambari-logsearch/pom.xml 7aeb4a7 > > ambari-server/src/main/resources/common-services/AMBARI_INFRA/0.1.0/package/scripts/params.py > 526baea > > ambari-server/src/main/resources/common-services/AMBARI_INFRA/0.1.0/properties/infra-solr-security.json.j2 > d8aea24 > > Diff: https://reviews.apache.org/r/56179/diff/ > > > Testing > ------- > > unit tests done, behavior validated with unit tests. other tests (FT) with > using ranger and atlas are in progress... > > > Thanks, > > Oliver Szabo > >
