Cleanup AuthenticatorMethod usage.

remoting/protocol/negotiating_authenticator_unittest.cc

Index: remoting/protocol/negotiating_authenticator_unittest.cc
diff --git a/remoting/protocol/negotiating_authenticator_unittest.cc b/remoting/protocol/negotiating_authenticator_unittest.cc
index 4461d1dae5aeb28d1b0b0b9637c0a71164df6eab..62a44898ed6128a81b7000a8f62cfac364550d92 100644
--- a/remoting/protocol/negotiating_authenticator_unittest.cc
+++ b/remoting/protocol/negotiating_authenticator_unittest.cc
@@ -6,6 +6,7 @@
 #include "base/macros.h"
 #include "net/base/net_errors.h"
 #include "remoting/base/rsa_key_pair.h"
+#include "remoting/protocol/auth_util.h"
 #include "remoting/protocol/authenticator_test_base.h"
 #include "remoting/protocol/channel_authenticator.h"
 #include "remoting/protocol/connection_tester.h"
@@ -54,32 +55,34 @@ class NegotiatingAuthenticatorTest : public AuthenticatorTestBase {
                           const std::string& client_interactive_pin,
                           const std::string& host_secret,
                           bool it2me,
-                          bool client_hmac_only) {
+                          bool client_no_it2me) {
     if (it2me) {
       host_ = NegotiatingHostAuthenticator::CreateForIt2Me(
           host_cert_, key_pair_, host_secret);
     } else {
-      std::string host_secret_hash = ApplySharedSecretHashFunction(
-          HashFunction::HMAC_SHA256, kTestHostId, host_secret);
+      std::string host_secret_hash =
+          GetSharedSecretHash(kTestHostId, host_secret);
       host_ = NegotiatingHostAuthenticator::CreateWithPin(
           host_cert_, key_pair_, host_secret_hash, pairing_registry_);
     }
 
-    std::vector<AuthenticationMethod> methods;
-    methods.push_back(AuthenticationMethod::SPAKE2_PAIR);
-    methods.push_back(AuthenticationMethod::SPAKE2_SHARED_SECRET_HMAC);
-    if (!client_hmac_only) {
-      methods.push_back(AuthenticationMethod::SPAKE2_SHARED_SECRET_PLAIN);
-    }
     bool pairing_expected = pairing_registry_.get() != nullptr;
     FetchSecretCallback fetch_secret_callback =
         base::Bind(&NegotiatingAuthenticatorTest::FetchSecret,
                    client_interactive_pin,
                    pairing_expected);
     client_as_negotiating_authenticator_ = new NegotiatingClientAuthenticator(
-        client_id, client_paired_secret,
-        kTestHostId, fetch_secret_callback,
-        nullptr, methods);
+        client_id, client_paired_secret, kTestHostId, fetch_secret_callback,
+        nullptr);
+    client_as_negotiating_authenticator_->methods_.clear();
+    client_as_negotiating_authenticator_->methods_.push_back(
+        NegotiatingAuthenticatorBase::Method::SPAKE2_PAIR);
+    client_as_negotiating_authenticator_->methods_.push_back(
+        NegotiatingAuthenticatorBase::Method::SPAKE2_SHARED_SECRET_HMAC);
+    if (!client_no_it2me) {
+      client_as_negotiating_authenticator_->methods_.push_back(
+          NegotiatingAuthenticatorBase::Method::SPAKE2_SHARED_SECRET_PLAIN);
+    }

[Jamie, 2016/03/08 02:17:38]

What's the purpose of this test? It seems like you're forcing the authenticator
to operate with a set of Methods that can't be used in production code (because
there's no public method to set them).

[Sergey Ulanov, 2016/03/08 18:32:40]

Yes. We need to test the authenticator in the case when the host and client are
not compatible  (see NegotiatingAuthenticatorTest.IncompatibleMethods) to make
sure correct error is returned. It's not possible to do that using public
interface.

[Jamie, 2016/03/08 22:42:50]

Would it make more sense to override them only for the IncompatibleMethods test,
with a comment explaining why it's needed in that case? I'd prefer that the
majority of tests use the code the way it's used in production to minimize
duplication and code-rot.

[Sergey Ulanov, 2016/03/09 03:45:00]

Done.

     client_.reset(client_as_negotiating_authenticator_);
   }
 
@@ -113,7 +116,7 @@ class NegotiatingAuthenticatorTest : public AuthenticatorTestBase {
     }
   }
 
-  void VerifyAccepted(const AuthenticationMethod& expected_method) {
+  void VerifyAccepted(NegotiatingAuthenticatorBase::Method expected_method) {
     ASSERT_NO_FATAL_FAILURE(RunAuthExchange());
 
     ASSERT_EQ(Authenticator::ACCEPTED, host_->state());
@@ -132,9 +135,8 @@ class NegotiatingAuthenticatorTest : public AuthenticatorTestBase {
     tester.Start();
     message_loop_.Run();
     tester.CheckResults();
-    EXPECT_EQ(
-        expected_method,
-        client_as_negotiating_authenticator_->current_method_for_testing());
+    EXPECT_EQ(expected_method,
+              client_as_negotiating_authenticator_->current_method_);
   }
 
   // Use a bare pointer because the storage is managed by the base class.
@@ -149,13 +151,15 @@ class NegotiatingAuthenticatorTest : public AuthenticatorTestBase {
 TEST_F(NegotiatingAuthenticatorTest, SuccessfulAuthMe2MePin) {
   ASSERT_NO_FATAL_FAILURE(InitAuthenticators(kNoClientId, kNoPairedSecret,
                                              kTestPin, kTestPin, false, false));
-  VerifyAccepted(AuthenticationMethod::SPAKE2_SHARED_SECRET_HMAC);
+  VerifyAccepted(
+      NegotiatingAuthenticatorBase::Method::SPAKE2_SHARED_SECRET_HMAC);
 }
 
 TEST_F(NegotiatingAuthenticatorTest, SuccessfulAuthIt2me) {
   ASSERT_NO_FATAL_FAILURE(InitAuthenticators(kNoClientId, kNoPairedSecret,
                                              kTestPin, kTestPin, true, false));
-  VerifyAccepted(AuthenticationMethod::SPAKE2_SHARED_SECRET_PLAIN);
+  VerifyAccepted(
+      NegotiatingAuthenticatorBase::Method::SPAKE2_SHARED_SECRET_PLAIN);
 }
 
 TEST_F(NegotiatingAuthenticatorTest, InvalidMe2MePin) {
@@ -186,7 +190,8 @@ TEST_F(NegotiatingAuthenticatorTest, PairingNotSupported) {
   ASSERT_NO_FATAL_FAILURE(InitAuthenticators(kTestClientId, kTestPairedSecret,
                                              kTestPin, kTestPin, false, false));
   ASSERT_NO_FATAL_FAILURE(RunAuthExchange());
-  VerifyAccepted(AuthenticationMethod::SPAKE2_SHARED_SECRET_HMAC);
+  VerifyAccepted(
+      NegotiatingAuthenticatorBase::Method::SPAKE2_SHARED_SECRET_HMAC);
 }
 
 TEST_F(NegotiatingAuthenticatorTest, PairingSupportedButNotPaired) {
@@ -194,7 +199,7 @@ TEST_F(NegotiatingAuthenticatorTest, PairingSupportedButNotPaired) {
   ASSERT_NO_FATAL_FAILURE(InitAuthenticators(kNoClientId, kNoPairedSecret,
                                              kTestPin, kTestPin, false, false));
   ASSERT_NO_FATAL_FAILURE(RunAuthExchange());
-  VerifyAccepted(AuthenticationMethod::SPAKE2_PAIR);
+  VerifyAccepted(NegotiatingAuthenticatorBase::Method::SPAKE2_PAIR);
 }
 
 TEST_F(NegotiatingAuthenticatorTest, PairingRevokedPinOkay) {
@@ -202,7 +207,7 @@ TEST_F(NegotiatingAuthenticatorTest, PairingRevokedPinOkay) {
   ASSERT_NO_FATAL_FAILURE(InitAuthenticators(kTestClientId, kTestPairedSecret,
                                              kTestPin, kTestPin, false, false));
   ASSERT_NO_FATAL_FAILURE(RunAuthExchange());
-  VerifyAccepted(AuthenticationMethod::SPAKE2_PAIR);
+  VerifyAccepted(NegotiatingAuthenticatorBase::Method::SPAKE2_PAIR);
 }
 
 TEST_F(NegotiatingAuthenticatorTest, PairingRevokedPinBad) {
@@ -218,7 +223,7 @@ TEST_F(NegotiatingAuthenticatorTest, PairingSucceeded) {
   ASSERT_NO_FATAL_FAILURE(InitAuthenticators(
       kTestClientId, kTestPairedSecret, kTestPinBad, kTestPin, false, false));
   ASSERT_NO_FATAL_FAILURE(RunAuthExchange());
-  VerifyAccepted(AuthenticationMethod::SPAKE2_PAIR);
+  VerifyAccepted(NegotiatingAuthenticatorBase::Method::SPAKE2_PAIR);
 }
 
 TEST_F(NegotiatingAuthenticatorTest,
@@ -227,7 +232,7 @@ TEST_F(NegotiatingAuthenticatorTest,
   ASSERT_NO_FATAL_FAILURE(InitAuthenticators(
       kTestClientId, kTestPairedSecretBad, kTestPin, kTestPin, false, false));
   ASSERT_NO_FATAL_FAILURE(RunAuthExchange());
-  VerifyAccepted(AuthenticationMethod::SPAKE2_PAIR);
+  VerifyAccepted(NegotiatingAuthenticatorBase::Method::SPAKE2_PAIR);
 }
 
 TEST_F(NegotiatingAuthenticatorTest, PairingFailedInvalidSecretAndPin) {