> On Dec. 8, 2016, 3:55 p.m., Aihua Xu wrote:
> > common/src/java/org/apache/hadoop/hive/conf/HiveConf.java, line 2426
> > <https://reviews.apache.org/r/53204/diff/3/?file=1579539#file1579539line2426>
> >
> > Just curious why we don't just put the constant string
> > "hive.server2.authentication.ldap.userMembershipKey" here like most of
> > other entries?
>
> Illya Yalovyy wrote:
> Because it uses in several places. In particular in documentation.
> Putting a string in documentation is not maintainable, because later someone
> can change the string and forget to update in in all places. Documentation
> would become stale. In such a big project in will be a problem. JavaDoc has
> means to prevent that from happening by using string constants in
> documentation sections.
I got what you mean. But you can define as
HIVE_SERVER2_PLAIN_LDAP_USERMEMBERSHIP_KEY("the string", null) and then refer
the string as
HiveConf.ConfVars.HIVE_SERVER2_PLAIN_LDAP_USERMEMBERSHIP_KEY.varname. Is that
what you want?
> On Dec. 8, 2016, 3:55 p.m., Aihua Xu wrote:
> > service/src/java/org/apache/hive/service/auth/ldap/GroupFilterFactory.java,
> > line 90
> > <https://reviews.apache.org/r/53204/diff/3/?file=1579541#file1579541line90>
> >
> > This seems to be a useful info that will help in diagnostics. Wondering
> > why changes from info to debug level?
>
> Illya Yalovyy wrote:
> I totally agree, but Naveen doesn't want to expose group names in logs.
> It is a questionable concern, but moving it to DEBUG may be a good compromise.
I see. That makes sense. Then maybe we can have both, so sensitive group only
printed during debug level and we will still see authetication succuss message
during info level? How do you think?
LOG.debug("GroupMembershipKeyFilter passes: user '{}' is a member of '{}'
group",...
LOG.info("Authentication succeeded based on group membership");
> On Dec. 8, 2016, 3:55 p.m., Aihua Xu wrote:
> > service/src/java/org/apache/hive/service/auth/ldap/GroupFilterFactory.java,
> > line 115
> > <https://reviews.apache.org/r/53204/diff/3/?file=1579541#file1579541line115>
> >
> > This should be info level which will be consistent with
> > GroupMembershipKeyFilter class.
>
> Illya Yalovyy wrote:
> Ok. I'll generate 2 log entries then: 1. INFO without group information;
> 2. DEBUG with full information.
>
> Does it make sense?
>
> See Naveen's comments for more details.
>
> Illya Yalovyy wrote:
> Actually this is a bit different. I'll change it to INFO.
Now I'm convinced for having 2 entries, 1. INFO without group information; 2.
DEBUG with full information.
What do you mean it's different?
> On Dec. 8, 2016, 3:55 p.m., Aihua Xu wrote:
> > service/src/java/org/apache/hive/service/auth/ldap/GroupFilterFactory.java,
> > line 124
> > <https://reviews.apache.org/r/53204/diff/3/?file=1579541#file1579541line124>
> >
> > Seems 'warn' is not necessary since that could be expected in the for
> > loop, right?
>
> Illya Yalovyy wrote:
> It means we have a group in configuration that doesn't exist... Would you
> recommend log it at DEBUG level?
I see. So we configure several groups in the configuration and it's possible
that we configure some incorrectly which are not in LDAP. The normal logic is
to except no NamingException, right? Actually based on that, "warn" is correct
level. Sorry, I was misunderstanding the logic.
for (String groupId : groupFilter) {
try {
String groupDn = ldap.findGroupDn(groupId);
groupDns.add(groupDn);
} catch (NamingException e) {
LOG.debug("Cannot find DN for group " + groupId, e);
}
}
> On Dec. 8, 2016, 3:55 p.m., Aihua Xu wrote:
> > service/src/java/org/apache/hive/service/auth/ldap/GroupFilterFactory.java,
> > line 132
> > <https://reviews.apache.org/r/53204/diff/3/?file=1579541#file1579541line132>
> >
> > Since we are throwing the exception, I guess such debug may be
> > redundant. We should display such exception in the caller somewhere.
>
> Illya Yalovyy wrote:
> Exception message has a different (less descriptive) message. Please see
> Naveen's comments for more details.
Got it.
> On Dec. 8, 2016, 3:55 p.m., Aihua Xu wrote:
> > service/src/java/org/apache/hive/service/auth/ldap/GroupFilterFactory.java,
> > line 139
> > <https://reviews.apache.org/r/53204/diff/3/?file=1579541#file1579541line139>
> >
> > Seems this could be a info level message.
>
> Illya Yalovyy wrote:
> Same here.
Same as previous comment. Maybe print info level without sensitive info and
debug with?
> On Dec. 8, 2016, 3:55 p.m., Aihua Xu wrote:
> > service/src/java/org/apache/hive/service/auth/ldap/GroupFilterFactory.java,
> > line 145
> > <https://reviews.apache.org/r/53204/diff/3/?file=1579541#file1579541line145>
> >
> > You may need to change message since it's expected that the user is not
> > in some groups. Probably change to "Cannot match user ... and group ..."
> > since "Failed to" seems to be an error.
>
> Illya Yalovyy wrote:
> I will update the message.
>
> Illya Yalovyy wrote:
> Usually it should just return true or false. If it fails with exception
> then something is wrong. That was reflected in the message. I noticed that
> I'm hiding the exception, which is a very bad practice. Will fix it as well.
> May be even WARN log message with exception details is required here. What
> you think? Again it should not happen usually, if it does - something wrong.
Seems I misunderstood here again. yeah. You are right. WARN with details looks
good to me.
> On Dec. 8, 2016, 3:55 p.m., Aihua Xu wrote:
> > service/src/test/org/apache/hive/service/auth/TestLdapAuthenticationProviderImpl.java,
> > line 265
> > <https://reviews.apache.org/r/53204/diff/3/?file=1579546#file1579546line265>
> >
> > You may need to add some tests for the default configuraiton which is
> > null for HIVE_SERVER2_PLAIN_LDAP_USERMEMBERSHIP_KEY.
>
> Illya Yalovyy wrote:
> If HIVE_SERVER2_PLAIN_LDAP_USERMEMBERSHIP_KEY is NULL this filter will
> not be used. I think we have enough test for this case. Did I get you
> correctly? Could you please provide more details about the test case you have
> in mind?
>
> Illya Yalovyy wrote:
> I think this use case is tested in
> #testAuthenticateWhenGroupFilterPasses(). Probably I should rename other
> tests to make it clear.
Looks good. :)
- Aihua
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53204/#review158532
-----------------------------------------------------------
On Dec. 9, 2016, 1:03 a.m., Illya Yalovyy wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53204/
> -----------------------------------------------------------
>
> (Updated Dec. 9, 2016, 1:03 a.m.)
>
>
> Review request for hive, Aihua Xu, Ashutosh Chauhan, Chaoyu Tang, and Szehon
> Ho.
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> HIVE-15076 Improve scalability of LDAP authentication provider group filter
>
> https://issues.apache.org/jira/browse/HIVE-15076
>
>
> Diffs
> -----
>
> common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 5ea9751
> service/src/java/org/apache/hive/service/auth/ldap/DirSearch.java 33b6088
> service/src/java/org/apache/hive/service/auth/ldap/GroupFilterFactory.java
> 152c4b2
> service/src/java/org/apache/hive/service/auth/ldap/LdapSearch.java 65076ea
> service/src/java/org/apache/hive/service/auth/ldap/Query.java b8bf938
> service/src/java/org/apache/hive/service/auth/ldap/QueryFactory.java
> e9172d3
>
> service/src/test/org/apache/hive/service/auth/TestLdapAtnProviderWithMiniDS.java
> cd62935
>
> service/src/test/org/apache/hive/service/auth/TestLdapAuthenticationProviderImpl.java
> 4fad755
>
> service/src/test/org/apache/hive/service/auth/ldap/LdapAuthenticationTestCase.java
> acde8c1
> service/src/test/org/apache/hive/service/auth/ldap/TestGroupFilter.java
> 0cc2ead
> service/src/test/org/apache/hive/service/auth/ldap/TestLdapSearch.java
> 499b624
> service/src/test/org/apache/hive/service/auth/ldap/TestQueryFactory.java
> 3054e33
> service/src/test/resources/ldap/ad.example.com.ldif PRE-CREATION
> service/src/test/resources/ldap/microsoft.schema.ldif PRE-CREATION
>
> Diff: https://reviews.apache.org/r/53204/diff/
>
>
> Testing
> -------
>
> Build succeeded.
>
> Test results:
>
> Tests run: 149, Failures: 0, Errors: 0, Skipped: 0
>
> [INFO]
> ------------------------------------------------------------------------
> [INFO] BUILD SUCCESS
> [INFO]
> ------------------------------------------------------------------------
> [INFO] Total time: 03:14 min
> [INFO] Finished at: 2016-10-26T13:53:15-07:00
> [INFO] Final Memory: 36M/1091M
> [INFO]
> ------------------------------------------------------------------------
>
>
> Thanks,
>
> Illya Yalovyy
>
>
