You are viewing a plain text version of this content. The canonical link for it is here.

Posted to dev@zookeeper.apache.org by Eugene Koontz <ek...@hiro-tan.org> on 2011/09/19 22:08:50 UTC

Review Request: ZOOKEEPER-1185 : Send AuthFailed event to client if SASL authentication fails

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/1959/
-----------------------------------------------------------

Review request for zookeeper.

Summary
-------

There are 3 places where ClientCnxn should queue a AuthFailed event if client fails to authenticate. Without sending this event, clients may be stuck watching for a SaslAuthenticated event that will never come (since the client failed to authenticate).

This patch fixes SaslAuthFailTest.testBadSaslAuthNotifiesWatch() to test for the AuthFailed event : previously, the test was incorrectly not testing for this event.

It also removes the testBadSaslAuthNotifiesWatch() method from the SaslAuthTest class : this method belongs in SaslAuthFailTest, not SaslAuthTest. The former tests unsuccessful SASL authentication; the latter, successful SASL authentication.

Diffs
-----

src/java/main/org/apache/zookeeper/ClientCnxn.java 723efa1
src/java/main/org/apache/zookeeper/client/ZooKeeperSaslClient.java 43382c8
src/java/test/org/apache/zookeeper/test/SaslAuthFailTest.java 8de7c2a
src/java/test/org/apache/zookeeper/test/SaslAuthTest.java fd20346

Diff: https://reviews.apache.org/r/1959/diff

Testing
-------

All unit tests pass. Also tested with an HBase cluster with an hbase shell running as an unauthenticated Zookeeper client. As expected, hbase shell could not access cluster, but, as expected, did not hang.

Thanks,

Eugene

Re: Review Request: ZOOKEEPER-1185 : Send AuthFailed event to client if SASL authentication fails

Posted by Eugene Koontz <ek...@hiro-tan.org>.

(Updated 2011-09-22 18:33:42.805934)

Review request for zookeeper.

Changes
-------

add link to JIRA.

Summary
-------

This patch fixes SaslAuthFailTest.testBadSaslAuthNotifiesWatch() to test for the AuthFailed event : previously, the test was incorrectly not testing for this event.

This addresses bug ZOOKEEPER-1185.
https://issues.apache.org/jira/browse/ZOOKEEPER-1185

Diffs
-----

src/java/main/org/apache/zookeeper/ClientCnxn.java db15348
src/java/main/org/apache/zookeeper/client/ZooKeeperSaslClient.java 43382c8
src/java/test/org/apache/zookeeper/test/SaslAuthFailTest.java 8de7c2a
src/java/test/org/apache/zookeeper/test/SaslAuthTest.java fd20346

Diff: https://reviews.apache.org/r/1959/diff

Testing
-------

Thanks,

Eugene

Re: Review Request: ZOOKEEPER-1185 : Send AuthFailed event to client if SASL authentication fails

Posted by Eugene Koontz <ek...@hiro-tan.org>.

(Updated 2011-09-20 23:59:16.329913)

Review request for zookeeper.

Changes
-------

Add ZooKeeperSaslClient.hasFailed() method (thanks to comment by Thomas Koch).

Summary
-------

This patch fixes SaslAuthFailTest.testBadSaslAuthNotifiesWatch() to test for the AuthFailed event : previously, the test was incorrectly not testing for this event.

Diffs (updated)
-----

Diff: https://reviews.apache.org/r/1959/diff

Testing
-------

Thanks,

Eugene

Re: Review Request: ZOOKEEPER-1185 : Send AuthFailed event to client if SASL authentication fails

Posted by Eugene Koontz <ek...@hiro-tan.org>.


> On 2011-09-19 20:31:15, Thomas Koch wrote:
> > src/java/main/org/apache/zookeeper/ClientCnxn.java, line 563
> > <https://reviews.apache.org/r/1959/diff/1/?file=41755#file41755line563>
> >
> >     Could this condition be simplified? For example instead of 
> >     clientCnxn.zooKeeperSaslClient.getSaslState() == ZooKeeperSaslClient.SaslState.FAILED
> >     
> >     one could call clientCnxn.zooKeeperSaslClient.hasFailed()

Thanks Thomas; uploaded new patch with this change.


- Eugene


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/1959/#review1967
-----------------------------------------------------------


On 2011-09-20 23:59:16, Eugene Koontz wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/1959/
> -----------------------------------------------------------
> 
> (Updated 2011-09-20 23:59:16)
> 
> 
> Review request for zookeeper.
> 
> 
> Summary
> -------
> 
> There are 3 places where ClientCnxn should queue a AuthFailed event if client fails to authenticate. Without sending this event, clients may be stuck watching for a SaslAuthenticated event that will never come (since the client failed to authenticate).
> 
> This patch fixes SaslAuthFailTest.testBadSaslAuthNotifiesWatch() to test for the AuthFailed event : previously, the test was incorrectly not testing for this event.
> 
> It also removes the testBadSaslAuthNotifiesWatch() method from the SaslAuthTest class : this method belongs in SaslAuthFailTest, not SaslAuthTest. The former tests unsuccessful SASL authentication; the latter, successful SASL authentication. 
> 
> 
> Diffs
> -----
> 
>   src/java/main/org/apache/zookeeper/ClientCnxn.java db15348 
>   src/java/main/org/apache/zookeeper/client/ZooKeeperSaslClient.java 43382c8 
>   src/java/test/org/apache/zookeeper/test/SaslAuthFailTest.java 8de7c2a 
>   src/java/test/org/apache/zookeeper/test/SaslAuthTest.java fd20346 
> 
> Diff: https://reviews.apache.org/r/1959/diff
> 
> 
> Testing
> -------
> 
> All unit tests pass. Also tested with an HBase cluster with an hbase shell running as an unauthenticated Zookeeper client. As expected, hbase shell could not access cluster, but, as expected, did not hang.
> 
> 
> Thanks,
> 
> Eugene
> 
>

Re: Review Request: ZOOKEEPER-1185 : Send AuthFailed event to client if SASL authentication fails

Posted by Thomas Koch <th...@koch.ro>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/1959/#review1967
-----------------------------------------------------------



src/java/main/org/apache/zookeeper/ClientCnxn.java
<https://reviews.apache.org/r/1959/#comment4446>

    Could this condition be simplified? For example instead of 
    clientCnxn.zooKeeperSaslClient.getSaslState() == ZooKeeperSaslClient.SaslState.FAILED
    
    one could call clientCnxn.zooKeeperSaslClient.hasFailed()


- Thomas


On 2011-09-19 20:08:50, Eugene Koontz wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/1959/
> -----------------------------------------------------------
> 
> (Updated 2011-09-19 20:08:50)
> 
> 
> Review request for zookeeper.
> 
> 
> Summary
> -------
> 
> There are 3 places where ClientCnxn should queue a AuthFailed event if client fails to authenticate. Without sending this event, clients may be stuck watching for a SaslAuthenticated event that will never come (since the client failed to authenticate).
> 
> This patch fixes SaslAuthFailTest.testBadSaslAuthNotifiesWatch() to test for the AuthFailed event : previously, the test was incorrectly not testing for this event.
> 
> It also removes the testBadSaslAuthNotifiesWatch() method from the SaslAuthTest class : this method belongs in SaslAuthFailTest, not SaslAuthTest. The former tests unsuccessful SASL authentication; the latter, successful SASL authentication. 
> 
> 
> Diffs
> -----
> 
>   src/java/main/org/apache/zookeeper/ClientCnxn.java 723efa1 
>   src/java/main/org/apache/zookeeper/client/ZooKeeperSaslClient.java 43382c8 
>   src/java/test/org/apache/zookeeper/test/SaslAuthFailTest.java 8de7c2a 
>   src/java/test/org/apache/zookeeper/test/SaslAuthTest.java fd20346 
> 
> Diff: https://reviews.apache.org/r/1959/diff
> 
> 
> Testing
> -------
> 
> All unit tests pass. Also tested with an HBase cluster with an hbase shell running as an unauthenticated Zookeeper client. As expected, hbase shell could not access cluster, but, as expected, did not hang.
> 
> 
> Thanks,
> 
> Eugene
> 
>