Common Name Mismatch Handler For WWW Subdomain Mismatch case

chrome/browser/ssl/common_name_mismatch_handler.h

+// Copyright (c) 2015 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.
+
+#ifndef CHROME_BROWSER_SSL_COMMON_NAME_MISMATCH_HANDLER_H_
+#define CHROME_BROWSER_SSL_COMMON_NAME_MISMATCH_HANDLER_H_
+
+#include "base/basictypes.h"
+#include "base/callback.h"
+#include "base/memory/ref_counted.h"
+#include "base/memory/scoped_ptr.h"
+#include "base/threading/non_thread_safe.h"
+#include "base/time/time.h"
+#include "net/url_request/url_fetcher.h"

[Ryan Sleevi, 2015/08/14 00:40:16]

Can forward declare

[Bhanu Dev, 2015/08/15 00:18:53]

Done.

+#include "net/url_request/url_fetcher_delegate.h"
+#include "net/url_request/url_request_context_getter.h"
+#include "url/gurl.h"
+
+// This class contains methods to get a suggested URL when certificate
+// validation fails due to |ERR_CERT_COMMON_NAME_INVALID| and has methods to
+// perform a network request to check the validity of new URL.

[Ryan Sleevi, 2015/08/14 00:40:16]

As a class comment, this feels worded wrong. Perhaps the following:

// This class handles errors due to common name mismatches
(|ERR_CERT_COMMON_NAME_INVALID|)
// and helps remediate them by suggesting alternative URLs that may be what
// the user intended to load.

[Bhanu Dev, 2015/08/15 00:18:53]

Done.

+class CommonNameMismatchHandler : public net::URLFetcherDelegate,
+                                  public base::NonThreadSafe {
+ public:
+  enum SuggestedUrlCheckResult {
+    // The request succeeds with good response code i.e. URL exists and its
+    // certificate is valid.
+    SUGGESTED_URL_AVAILABLE,
+    // Suggested URL is either not available or has a bad certificate.
+    SUGGESTED_URL_NOT_AVAILABLE
+  };
+
+  enum TestingState {
+    NOT_TESTING,
+    // Disables the actual request to the |suggested_url|.
+    IGNORE_REQUESTS_FOR_TESTING
+  };
+
+  struct Results {
+    Results() : result(SUGGESTED_URL_NOT_AVAILABLE) {}
+
+    SuggestedUrlCheckResult result;
+    GURL suggested_url;
+  };

[Ryan Sleevi, 2015/08/14 00:40:16]

The use of a struct helper for this seems unnecessary; why not just return the
two arguments?

[Bhanu Dev, 2015/08/15 00:18:53]

Done.

+
+  typedef base::Callback<void(const Results& results)> CheckUrlCallback;
+
+  CommonNameMismatchHandler(
+      const GURL request_url,

[Ryan Sleevi, 2015/08/14 00:40:16]

const GURL&

[Bhanu Dev, 2015/08/15 00:18:53]

Done.

+      const scoped_refptr<net::URLRequestContextGetter>& request_context);
+  ~CommonNameMismatchHandler() override;
+
+  // Triggers a check to validate suggested URL. After completion, runs the
+  // |callback|.

[Ryan Sleevi, 2015/08/14 00:40:16]

This comment could use beefing up.

If I'm only reading this header file, and trying to understand what the class
does, what is a check to validate a URL? What might I reasonably expect?

At the same time, you want to be careful to avoid documenting exactly what will
happen (that is, your header comment shouldn't be a prose form of the code,
since code will change but prose lives on), but attempt to explain the narrative
of what are the preconditions/postconditions and what might happen / how might
this be used

[Bhanu Dev, 2015/08/15 00:18:53]

Done.

+  void CheckSuggestedUrl(const GURL& url, const CheckUrlCallback& callback);
+
+  // Returns true if any other DNS names in the certificate's subject
+  // alternative names list closely match the original request hostname.
+  // If such a domain name exists, writes a suggestion for a new URL
+  // to try into |suggested_url|.

[Ryan Sleevi, 2015/08/14 00:40:17]

comment-wise, "writes a suggestion" feels weird in the broader context.

// Determines if, for |request_url| serving a certificate that is valid for
// the domain names |dns_names|, there is a name that the certificate is
// valid for that closely matches the original name in |request_url|. If
// so, returns true, and sets |*suggested_url| to a URL that is unlikely
// to cause an ERR_CERT_COMMON_NAME_INVALID error.

[Bhanu Dev, 2015/08/15 00:18:53]

Done. Thanks.

+  static bool GetSuggestedUrl(const GURL& request_url,
+                              const std::vector<std::string>& dns_names,
+                              GURL* suggested_url);
+
+  // Used in tests, to disable the request to |suggested_url|.
+  static void set_state_for_testing(TestingState testing_state) {

[Ryan Sleevi, 2015/08/14 00:40:16]

Static methods are generally a strong anti-pattern, especially for tests (as
implicitly it makes test non-hermetic unless the state is reset)

Why can't this be exposed as an instance method with an instance member?

[Bhanu Dev, 2015/08/15 00:18:53]

I was considering that this is good enough as long as we are setting and
resetting the states correctly. Also, was trying to follow existing peices of
code like
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/cap...

+    testing_state_ = testing_state;
+  }
+  static TestingState get_state_for_testing() { return testing_state_; }

[Ryan Sleevi, 2015/08/14 00:40:16]

Why is it needed to access the testing state? We rarely (never?) return this.

[Bhanu Dev, 2015/08/15 00:18:53]

Done.

+
+ private:
+  // net::URLFetcherDelegate:
+  void OnURLFetchComplete(const net::URLFetcher* source) override;
+
+  // Takes a |net::URLFetcher| that has finished trying to retrieve the
+  // suggested URL, and fills a |Results| struct based on its result.
+  void GetSuggestedUrlCheckResult(const net::URLFetcher* url_fetcher,
+                                  Results* results) const;

[Ryan Sleevi, 2015/08/14 00:40:16]

This doesn't seem like it needs to be a private method - that is, it could be
fully localized to the .cc in an unnamed namespace.

[Bhanu Dev, 2015/08/15 00:18:53]

Done.

+
+  // Returns true if the check is currently running.
+  bool CheckingSuggestedUrl() const;

[Ryan Sleevi, 2015/08/14 00:40:16]

s/CheckingSuggestedUrl/IsCheckingSuggestedUrl/

[Bhanu Dev, 2015/08/15 00:18:53]

Done.

+
+  static TestingState testing_state_;
+
+  const GURL request_url_;
+
+  scoped_refptr<net::URLRequestContextGetter> request_context_;
+
+  CheckUrlCallback check_url_callback_;
+
+  scoped_ptr<net::URLFetcher> url_fetcher_;

[Ryan Sleevi, 2015/08/14 00:40:16]

There seems to be a lot of unnecessary vertical whitespace here. You can delete
the line breaks on 84, 86, 88, and 90.

[Bhanu Dev, 2015/08/15 00:18:53]

Done.

+
+  DISALLOW_COPY_AND_ASSIGN(CommonNameMismatchHandler);
+};
+
+#endif  // CHROME_BROWSER_SSL_COMMON_NAME_MISMATCH_HANDLER_H_