Minimize login prompts

chrome/browser/ui/login/login_prompt_browsertest.cc

+// Copyright (c) 2010 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 <set>
+#include <map>
+
+#include "net/base/auth.h"

[cbentzel, 2010/12/17 19:22:56]

Includes need to be alphabetically ordered.

+#include "chrome/browser/ui/browser.h"
+#include "chrome/browser/browser_thread.h"
+#include "chrome/browser/ui/login/login_prompt.h"
+#include "chrome/browser/ui/tab_contents/tab_contents_wrapper.h"
+#include "chrome/test/in_process_browser_test.h"
+#include "chrome/test/ui_test_utils.h"
+#include "chrome/common/notification_service.h"
+
+namespace {
+
+class LoginPromptBrowserTest : public InProcessBrowserTest {
+ public:
+  LoginPromptBrowserTest():

[cbentzel, 2010/12/17 19:22:56]

Nit: the first initializer list argument should be on the same line as the ":". 

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Constructor_In...

+      bad_password_(L"incorrect"), bad_username_(L"nouser") {
+    set_show_window(true);
+
+    auth_map_[L"foo"] = AuthInfo(L"testuser", L"foopassword");
+    auth_map_[L"bar"] = AuthInfo(L"testuser", L"barpassword");
+  }
+
+ protected:
+

[cbentzel, 2010/12/17 19:22:56]

Nit: extra newline

+  struct AuthInfo {
+    std::wstring username_;
+    std::wstring password_;
+
+    AuthInfo() {}
+
+    AuthInfo(const std::wstring username,
+             const std::wstring password):
+        username_(username), password_(password) {}
+  };
+
+  NotificationRegistrar registrar_;
+  std::map<std::wstring, AuthInfo> auth_map_;
+  std::wstring bad_password_;
+  std::wstring bad_username_;
+
+  void SetAuthFor(LoginHandler* handler);

[cbentzel, 2010/12/17 19:22:56]

Nit: SetAuthFor needs to be declared before the member variables.

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Declaration_Order

+};
+
+const char* kMultiRealmTestPage = "files/login/multi_realm.html";
+const char* kSingleRealmTestPage = "files/login/single_realm.html";

[cbentzel, 2010/12/17 19:22:56]

Might make sense to move these right above the TEST_F's below, since that's the
first time they are used.

+
+void LoginPromptBrowserTest::SetAuthFor(LoginHandler* handler) {
+  net::AuthChallengeInfo* challenge = handler->auth_info();
+  std::map<std::wstring, AuthInfo>::iterator i;

[cbentzel, 2010/12/17 19:22:56]

Don't use default constructor for i, assign with
auth_map_.find(challenge->realm) from below.

+
+  ASSERT_TRUE(challenge != NULL);
+  i = auth_map_.find(challenge->realm);
+  EXPECT_NE(auth_map_.end(), i);
+  if (i != auth_map_.end()) {
+    AuthInfo& info = i->second;

[cbentzel, 2010/12/17 19:22:56]

const AuthInfo

+

[cbentzel, 2010/12/17 19:22:56]

Nit: extra newline

+    handler->SetAuth(info.username_, info.password_);
+  }
+}
+
+// Maintains a set of LoginHandlers that are currently active and
+// keeps a count of the notifications that were observed.
+class LoginPromptBrowserTestObserver : public NotificationObserver {
+ public:
+

[cbentzel, 2010/12/17 19:22:56]

Nit: extra new line

+  LoginPromptBrowserTestObserver():

[cbentzel, 2010/12/17 19:22:56]

Nit: first initializer list argument on same line as :

+      n_auth_needed(0), n_auth_supplied(0), n_auth_cancelled(0) {}

[cbentzel, 2010/12/17 19:22:56]

Use auth_needed_count rather than n_auth_needed [etc].

+
+  virtual void Observe(NotificationType type,
+                       const NotificationSource& source,
+                       const NotificationDetails& details);
+
+  void AddSelf(NotificationRegistrar* registrar,

[cbentzel, 2010/12/17 19:22:56]

Register and Unregister may be clearer than AddSelf/RemoveSelf.

+               const NotificationSource& source);
+
+  void RemoveSelf(NotificationRegistrar* registrar,
+                  const NotificationSource& source);
+
+  std::set<LoginHandler*> handlers_;
+
+  int n_auth_needed;
+  int n_auth_supplied;
+  int n_auth_cancelled;
+
+  DISALLOW_COPY_AND_ASSIGN(LoginPromptBrowserTestObserver);
+};
+
+void LoginPromptBrowserTestObserver::Observe(
+    NotificationType type,
+    const NotificationSource& source,
+    const NotificationDetails& details) {
+

[cbentzel, 2010/12/17 19:22:56]

Nit: extra new line

+  LoginNotificationDetails* login_details =

[cbentzel, 2010/12/17 19:22:56]

The Details cast here is typically done after checking the type - since it's
effectively a reinterpret cast. 

I recommend rewriting as 

switch (type) {
  case NotificationType::AUTH_NEEDED:
    // do the cast and other things.
    break;
  case NotificationType::AUTH_SUPPLIED:
  case NotificationType::AUTH_CANCELLED:
    // Do the other stuff
    break;
  default:
    ADD_FAILURE();
    break;
  }
}

+      Details<LoginNotificationDetails>(details).ptr();
+
+  if (type == NotificationType::AUTH_NEEDED) {
+    EXPECT_TRUE(handlers_.find(login_details->handler()) == handlers_.end());
+    handlers_.insert(login_details->handler());
+    n_auth_needed++;
+  } else if (type == NotificationType::AUTH_SUPPLIED ||
+             type == NotificationType::AUTH_CANCELLED) {
+    std::set<LoginHandler *>::iterator i;

[cbentzel, 2010/12/17 19:22:56]

Nit: do the handlers_.find on the same line as i declaration.

+
+    i = handlers_.find(login_details->handler());
+    EXPECT_TRUE(i != handlers_.end());
+    if (i != handlers_.end())
+      handlers_.erase(i);
+
+    if (type == NotificationType::AUTH_SUPPLIED)
+      n_auth_supplied++;
+    else
+      n_auth_cancelled++;
+  }
+}
+
+void LoginPromptBrowserTestObserver::AddSelf(
+    NotificationRegistrar* registrar,
+    const NotificationSource& source) {
+  registrar->Add(this, NotificationType::AUTH_NEEDED, source);
+  registrar->Add(this, NotificationType::AUTH_SUPPLIED, source);
+  registrar->Add(this, NotificationType::AUTH_CANCELLED, source);
+}
+
+void LoginPromptBrowserTestObserver::RemoveSelf(
+    NotificationRegistrar* registrar,
+    const NotificationSource& source) {
+  registrar->Remove(this, NotificationType::AUTH_NEEDED, source);
+  registrar->Remove(this, NotificationType::AUTH_SUPPLIED, source);
+  registrar->Remove(this, NotificationType::AUTH_CANCELLED, source);
+}
+
+template <NotificationType::Type T>
+class WindowedNavigationObserver
+    : public ui_test_utils::WindowedNotificationObserver {
+ public:
+  explicit WindowedNavigationObserver(NavigationController* controller)
+      : ui_test_utils::WindowedNotificationObserver(
+          T, Source<NavigationController>(controller)) {}
+};
+
+typedef WindowedNavigationObserver<NotificationType::LOAD_STOP>
+WindowedLoadStopObserver;
+
+typedef WindowedNavigationObserver<NotificationType::AUTH_NEEDED>
+WindowedAuthNeededObserver;
+
+typedef WindowedNavigationObserver<NotificationType::AUTH_CANCELLED>
+WindowedAuthCancelledObserver;
+
+typedef WindowedNavigationObserver<NotificationType::AUTH_SUPPLIED>
+WindowedAuthSuppliedObserver;
+
+// Test handling of resource that require authentication even though

[cbentzel, 2010/12/17 19:22:56]

Nit: s/resource/resources

+// the page they are included on doesn't.  In this case we should only
+// present the minimal number of prompts necessary for successfully
+// displaying the page.  First we check whether cancelling works as
+// expected.
+IN_PROC_BROWSER_TEST_F(LoginPromptBrowserTest, MultipleRealmCancellation) {
+  ASSERT_TRUE(test_server()->Start());
+  GURL test_page = test_server()->GetURL(kMultiRealmTestPage);
+
+  TabContentsWrapper* contents =
+      browser()->GetSelectedTabContentsWrapper();
+  ASSERT_TRUE(contents != NULL);

[cbentzel, 2010/12/17 19:22:56]

ASSERT_TRUE(contents) is sufficient.

+
+  NavigationController* controller = &contents->controller();
+  LoginPromptBrowserTestObserver observer;
+
+  observer.AddSelf(&registrar_, Source<NavigationController>(controller));
+
+  WindowedLoadStopObserver load_stop_waiter(controller);

[cbentzel, 2010/12/17 19:22:56]

Does the OpenURL trigger LOAD_STOP on the first auth prompt? When does that
notification event get sent?

[asanka (google), 2010/12/17 21:35:35]

LOAD_STOP is received after the page is done loading (after all the login
prompts have been resolved).  We are waiting on LOAD_STOP so that we can have a
clean teardown after the test run.

+
+  {

[cbentzel, 2010/12/17 19:22:56]

It doesn't seem like scoping the WindowedAuthNeededObserver is necessary here
[although it isn't incorrect] - for example, I don't think there will be an
AUTH_NEEDED if before this scope.

I would just keep it in the top level function scope so future readers don't
have to wonder why.

[asanka (google), 2010/12/17 21:35:35]

I expect the behavior to be identical if the WindowedAuthNeededObserver was at
the top level.  But I scoped it here so that the next auth_needed_waiter a few
lines down doesn't shadow it.

+    WindowedAuthNeededObserver auth_needed_waiter(controller);
+    browser()->OpenURL(test_page, GURL(), CURRENT_TAB, PageTransition::TYPED);
+    auth_needed_waiter.Wait();
+  }
+
+  int n_handlers = 0;
+
+  while (observer.n_auth_cancelled < 4) {

[cbentzel, 2010/12/17 19:22:56]

You should make 4 a constant so it's clear what it represent.

+    WindowedAuthNeededObserver auth_needed_waiter(controller);
+
+    while (!observer.handlers_.empty()) {
+      WindowedAuthCancelledObserver auth_cancelled_waiter(controller);
+      LoginHandler * handler = *observer.handlers_.begin();

[cbentzel, 2010/12/17 19:22:56]

Nit: LoginHandler* handler

+
+      ASSERT_TRUE(handler != NULL);

[cbentzel, 2010/12/17 19:22:56]

Nit: ASSERT_TRUE(handler);

+      n_handlers++;
+      handler->CancelAuth();
+      auth_cancelled_waiter.Wait();
+    }
+
+    if (observer.n_auth_cancelled < 4)
+      auth_needed_waiter.Wait();
+  }
+
+  load_stop_waiter.Wait();
+
+  EXPECT_EQ(2, n_handlers);

[cbentzel, 2010/12/17 19:22:56]

Add a constant for 2 and indicate that there are only two realms [essentially,
why this is 2 compared to the 4 for number of auth_needed/auth_cancelled].

+  EXPECT_EQ(4, observer.n_auth_needed);
+  EXPECT_EQ(0, observer.n_auth_supplied);
+  EXPECT_EQ(4, observer.n_auth_cancelled);
+
+  observer.RemoveSelf(&registrar_, Source<NavigationController>(controller));

[cbentzel, 2010/12/17 19:22:56]

Should this be done in the destructor for LoginPromptBrowserTestObserver?

+}
+
+// Similar to the MultipleRealmCancellation test above, but tests
+// whether supplying credentials work as exepcted.
+IN_PROC_BROWSER_TEST_F(LoginPromptBrowserTest, MultipleRealmConfirmation) {
+  ASSERT_TRUE(test_server()->Start());
+  GURL test_page = test_server()->GetURL(kMultiRealmTestPage);
+
+  TabContentsWrapper* contents =
+      browser()->GetSelectedTabContentsWrapper();
+  ASSERT_TRUE(contents != NULL);
+
+  NavigationController* controller = &contents->controller();
+  LoginPromptBrowserTestObserver observer;
+
+  observer.AddSelf(&registrar_, Source<NavigationController>(controller));
+
+  WindowedLoadStopObserver load_stop_waiter(controller);
+  int n_handlers = 0;
+
+  {
+    WindowedAuthNeededObserver auth_needed_waiter(controller);
+
+    browser()->OpenURL(test_page, GURL(), CURRENT_TAB, PageTransition::TYPED);
+    auth_needed_waiter.Wait();
+  }
+
+  while (observer.n_auth_supplied < 4) {
+    WindowedAuthNeededObserver auth_needed_waiter(controller);
+
+    while (!observer.handlers_.empty()) {
+      WindowedAuthSuppliedObserver auth_supplied_waiter(controller);
+      LoginHandler * handler = *observer.handlers_.begin();
+
+      ASSERT_TRUE(handler != NULL);
+      n_handlers++;
+      SetAuthFor(handler);
+      auth_supplied_waiter.Wait();
+    }
+
+    if (observer.n_auth_supplied < 4)
+      auth_needed_waiter.Wait();
+  }
+
+  load_stop_waiter.Wait();
+
+  EXPECT_EQ(2, n_handlers);
+  EXPECT_EQ(4, observer.n_auth_needed);
+  EXPECT_EQ(4, observer.n_auth_supplied);
+  EXPECT_EQ(0, observer.n_auth_cancelled);
+
+  observer.RemoveSelf(&registrar_, Source<NavigationController>(controller));
+}
+
+// Testing for recovery from an incorrect password for the case where
+// there are multiple authenticated resources.
+IN_PROC_BROWSER_TEST_F(LoginPromptBrowserTest, IncorrectConfirmation) {
+  ASSERT_TRUE(test_server()->Start());
+  GURL test_page = test_server()->GetURL(kSingleRealmTestPage);
+
+  TabContentsWrapper* contents =
+      browser()->GetSelectedTabContentsWrapper();
+  ASSERT_TRUE(contents != NULL);
+
+  NavigationController* controller = &contents->controller();
+  LoginPromptBrowserTestObserver observer;
+
+  observer.AddSelf(&registrar_, Source<NavigationController>(controller));
+
+  WindowedLoadStopObserver load_stop_waiter(controller);
+
+  {
+    WindowedAuthNeededObserver auth_needed_waiter(controller);
+    browser()->OpenURL(test_page, GURL(), CURRENT_TAB, PageTransition::TYPED);
+    auth_needed_waiter.Wait();
+  }
+
+  EXPECT_FALSE(observer.handlers_.empty());
+
+  if (!observer.handlers_.empty()) {
+    WindowedAuthNeededObserver auth_needed_waiter(controller);
+    WindowedAuthSuppliedObserver auth_supplied_waiter(controller);
+    LoginHandler * handler = *observer.handlers_.begin();
+
+    ASSERT_TRUE(handler != NULL);
+    handler->SetAuth(bad_username_, bad_password_);
+    auth_supplied_waiter.Wait();
+
+    // The request should be retried after the incorrect password is
+    // supplied.  This should result in a new AUTH_NEEDED notification
+    // for the same realm.
+    auth_needed_waiter.Wait();
+  }
+
+  int n_handlers = 0;
+
+  while (observer.n_auth_supplied < 12) {
+    WindowedAuthNeededObserver auth_needed_waiter(controller);
+
+    while (!observer.handlers_.empty()) {
+      WindowedAuthSuppliedObserver auth_supplied_waiter(controller);
+      LoginHandler * handler = *observer.handlers_.begin();
+
+      ASSERT_TRUE(handler != NULL);
+      n_handlers++;
+      SetAuthFor(handler);
+      auth_supplied_waiter.Wait();
+    }
+
+    if (observer.n_auth_supplied < 12)
+      auth_needed_waiter.Wait();
+  }
+
+  load_stop_waiter.Wait();
+
+  // n_auth_needed and n_auth_supplied are twice the number of
+  // resources since the incorrect password attempt should have
+  // resulted in a retry for the whole page.
+  EXPECT_EQ(1, n_handlers);
+  EXPECT_EQ(12, observer.n_auth_needed);
+  EXPECT_EQ(12, observer.n_auth_supplied);
+  EXPECT_EQ(0, observer.n_auth_cancelled);
+
+  observer.RemoveSelf(&registrar_, Source<NavigationController>(controller));
+}
+}