Support Kerberos on Android

net/android/http_auth_negotiate_android_unittest.cc

+// Copyright (c) 2014 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "base/run_loop.h"
+#include "base/test/test_ui_thread_android.h"
+#include "net/android/http_auth_negotiate_android.h"
+#include "net/base/net_errors.h"
+#include "net/http/http_auth_challenge_tokenizer.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace net {
+namespace android {
+
+namespace {
+
+void DummyCallback(base::Closure closure, int /*result*/) {
+  closure.Run();
+}
+
+}  // namespace
+
+TEST(AndroidAuthNegotiateTest, GenerateAuthToken) {
+  HttpAuthNegotiateAndroid auth("org.chromium.test.DummySpnegoAuthenticator");
+  EXPECT_TRUE(auth.Init());
+
+  base::StartTestUiThreadLooper();
+
+  std::string auth_token;

[Ryan Sleevi, 2015/06/16 01:07:46]

API DANGER: This sort of highlights the danger in your std::string* parameter
taking. The issue is that the string has a shorter lifetime than
HttpAuthNegotiateAndroid (line 24), except the API contract you've used for
asycnhronicity is that the std::string must live until the callback is satisfied
or until the HttpAuthNegotiateAndroid is destructed, whatever comes first.

If, in between lines 24 and 29, you had an object that on destruction pumped the
message loop (e.g. for idle tasks), you could end up crashing on line 40 (when
things go out of scope) if the EXPECT_EQ's weren't satisfied.

To be clear, this isn't a problem in the existing code, because you don't, but
it shows how easily it is to write dangerous code using this pattern.

You could reorder such that the string was declared before the
HttpAuthNegotiateAndroid, which would solve the problem, or possibly contemplate
a more explicit lifetime contract than the object itself (in //net, we use the
scoped_ptr<Request> pattern as the 'holder to an unsatisfied asynchronous
request', and which as long as its valid, so too must the pointer arguments to
the asynchronous request be valid)

This implicitly encourages safer programming, since you wouldn't allocate the
request until line 35 (when you're calling things), and by needing to allocate
it at the same time as this string, be forced to think about lifetimes.

[aberent, 2015/06/19 15:06:24]

I agree that the std::string* parameter is horrible and dangerous (and was aware
of this when I wrote the code), but it is built into all the Http authenticators
(see
https://cs.corp.google.com/#clankium/src/net/http/http_auth_handler.h&q=http_...)
and would require going considerably beyond the scope of the current CL to fix.
I have moved this declaration upward however.

+
+  base::RunLoop run_loop;
+  EXPECT_EQ(ERR_IO_PENDING,
+            auth.GenerateAuthToken(
+                nullptr, "Dummy", &auth_token,
+                base::Bind(&DummyCallback, run_loop.QuitClosure())));
+
+  run_loop.Run();
+
+  EXPECT_EQ("Negotiate DummyToken", auth_token);

[Ryan Sleevi, 2015/06/16 01:07:46]

It'd be much better to use a net::TestCompletionCallback() here, which will
handle quitting the loop, but also avoid spinning the loop in the event of
synchronous returns.

For example, if the expectation on line 32 failed, this test wouldn't fail - it
would deadlock to crash. You could make line 32 an ASSERT_EQ (so that the test
failed sooner), but it seems 'cleaner' to just use the
net::TestCompletionCallback.

[aberent, 2015/06/19 15:06:24]

Done.

+}
+
+}  // namespace android
+}  // namespace net