Intergration of the client-side phishing detection.

chrome/browser/safe_browsing/client_side_detection_host_unittest.cc

+// Copyright (c) 2011 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/command_line.h"
+#include "base/ref_counted.h"
+#include "base/scoped_ptr.h"
+#include "chrome/browser/browser_thread.h"
+#include "chrome/browser/renderer_host/test/test_render_view_host.h"
+#include "chrome/browser/safe_browsing/client_side_detection_host.h"
+#include "chrome/browser/safe_browsing/client_side_detection_service.h"
+#include "chrome/browser/safe_browsing/safe_browsing_service.h"
+#include "chrome/browser/tab_contents/test_tab_contents.h"
+#include "chrome/common/chrome_switches.h"
+#include "chrome/test/ui_test_utils.h"
+#include "googleurl/src/gurl.h"
+#include "testing/gmock/include/gmock/gmock.h"
+#include "testing/gmock/include/gmock/gmock-spec-builders.h"
+
+using ::testing::_;
+using ::testing::DoAll;
+using ::testing::Mock;
+using ::testing::SaveArg;
+
+namespace safe_browsing {
+
+class MockClientSideDetectionService : public ClientSideDetectionService {
+ public:
+  MockClientSideDetectionService() {}
+  virtual ~MockClientSideDetectionService() {};
+
+  MOCK_METHOD3(SendClientReportPhishingRequest,
+               void(const GURL& phishing_url,

[Brian Ryner, 2011/02/11 01:30:39]

Hm, I usually see these written without the parameter names, since they aren't
needed, e.g.

MOCK_METHOD7(SendClientReportPhishingRequest,
                                void(const GURL&, double,
ClientReportPhishingRequestCallback*));

[noelutz, 2011/02/15 23:00:55]

Done.

+                    double score,
+                    ClientReportPhishingRequestCallback* callback));
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(MockClientSideDetectionService);
+};
+
+class MockSafeBrowsingService : public SafeBrowsingService {
+ public:
+  MockSafeBrowsingService() {}
+  virtual ~MockSafeBrowsingService() {}
+
+  MOCK_METHOD7(DisplayBlockingPage,
+               void(const GURL& url,
+                    const GURL& original_url,
+                    ResourceType::Type resource_type,
+                    UrlCheckResult result,
+                    Client* client,
+                    int render_process_host_id,
+                    int render_view_id));
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(MockSafeBrowsingService);
+};
+
+ACTION_P(QuitMessageLoop, msg_loop) {
+  msg_loop->Quit();
+}
+
+class ClientSideDetectionHostTest : public RenderViewHostTestHarness {
+ public:
+  virtual void SetUp() {
+    RenderViewHostTestHarness::SetUp();
+    ui_thread_.reset(new BrowserThread(BrowserThread::UI, &message_loop_));
+    io_thread_.reset(new BrowserThread(BrowserThread::IO, &message_loop_));

[Brian Ryner, 2011/02/11 01:30:39]

Unless there's a reason not to, you may want to actually spin this up as a real
thread.  That way it will exercise your DCHECKs about events happening on the
correct thread.

[noelutz, 2011/02/15 23:00:55]

Done.  This was much harder than I thought but it improves the test I think.

+
+    // Inject service classes.
+    csd_service_.reset(new MockClientSideDetectionService());
+    sb_service_ = new MockSafeBrowsingService();
+    csd_host_ = contents()->safebrowsing_detection_host();
+    csd_host_->set_client_side_detection_service(csd_service_.get());
+    csd_host_->set_safe_browsing_service(sb_service_.get());

[lzheng, 2011/02/11 19:39:42]

You can get rid of these two set_xx by using
SafeBrowsingService::RegisterFactory() and also create something similar for csd
service. You can use safe_browsing_blocking_page_test.cc as an example. The
basic idea is to create a factory that will use your mock as the implementation.

[noelutz, 2011/02/15 23:00:55]

I don't think that would work.  Another reason I inject the service objects like
this is because there is no resource dispatcher host object in my unit test. 
Even if I register a special safebrowsing service factory sb_service_ will be
NULL in my test. if I don't use these setters.

An alternative would be to create a factory class for the new client-side
detection host class.  TabContents would use the factory to create an instance. 
In the test I could provide a test factory instance that would create the csd
host object with the two mock service instances.  Would you like that better
than the current solution?

[lzheng, 2011/02/16 19:09:32]

Hm, let's stay with what you have for now. It is not too bad. In case we need to
mock out the csd_host someday, I think we could use the factory approach.


On 2011/02/15 23:00:55, noelutz wrote:

+  }
+
+  virtual void TearDown() {
+    io_thread_.reset();
+    ui_thread_.reset();
+    RenderViewHostTestHarness::TearDown();
+  }
+
+  virtual void ExpectNotShowInterstitial() {
+    // Make sure DisplayBlockingPage is not called.
+    EXPECT_CALL(*sb_service_,
+                DisplayBlockingPage(_, _, _, _, _, _, _)).Times(0);
+    MessageLoop::current()->RunAllPending();
+    EXPECT_TRUE(Mock::VerifyAndClear(sb_service_.get()));
+  }
+
+  virtual void ExpectShowInterstital(const GURL& phishing_url) {
+

[Brian Ryner, 2011/02/11 01:30:39]

Extra blank line?

[noelutz, 2011/02/15 23:00:55]

Done.

+    SafeBrowsingService::Client* client;
+    EXPECT_CALL(*sb_service_,
+                DisplayBlockingPage(
+                    phishing_url,
+                    phishing_url,
+                    ResourceType::MAIN_FRAME,
+                    SafeBrowsingService::URL_PHISHING,
+                    _ /* a CsdClient object */,
+                    contents()->GetRenderProcessHost()->id(),
+                    contents()->render_view_host()->routing_id()))
+        .WillOnce(DoAll(SaveArg<4>(&client),
+                        QuitMessageLoop(MessageLoop::current())));
+    MessageLoop::current()->Run();

[Brian Ryner, 2011/02/11 01:30:39]

This is kind of a nit, but you're using different logic for when you expect the
interstitial to be shown vs. not shown.

For example, suppose the interstitial is shown after a task gets posted back to
the message loop and runs.  Your current code will work fine in that case.  But,
if it does two asynchronous operations, ExpectNotShowInterstitial will now
always succeed, because it only runs through the message loop once.

I'm not sure if there's a better solution here, but it seems like calling
MessageLoop::RunAllPending the same number of times for both cases would help
ensure that the test is working correctly.  Thoughts?

[noelutz, 2011/02/15 23:00:55]

Agreed.  It makes more sense to use the same logic in both cases.  Changed.

+    EXPECT_TRUE(Mock::VerifyAndClear(sb_service_.get()));
+    ASSERT_TRUE(client != NULL);
+    client->OnBlockingPageComplete(false);
+  }
+
+ protected:
+  ClientSideDetectionHost* csd_host_;
+  scoped_ptr<MockClientSideDetectionService> csd_service_;
+  scoped_refptr<MockSafeBrowsingService> sb_service_;
+  scoped_ptr<BrowserThread> ui_thread_;
+  scoped_ptr<BrowserThread> io_thread_;
+};
+
+TEST_F(ClientSideDetectionHostTest, OnDetectedPhishingSite) {
+  ClientSideDetectionService::ClientReportPhishingRequestCallback* cb;
+  GURL phishing_url("http://phishingurl.com/");
+
+  // Case 1: client thinks the page is phishing.  The server does not agree.
+  // No interstitial is shown.
+  EXPECT_CALL(*csd_service_,
+              SendClientReportPhishingRequest(phishing_url, 1.0, _))
+      .WillOnce(SaveArg<2>(&cb));
+  csd_host_->OnDetectedPhishingSite(phishing_url, 1.0);
+  EXPECT_TRUE(Mock::VerifyAndClear(csd_service_.get()));
+  ASSERT_TRUE(cb != NULL);
+  cb->Run(phishing_url, false);
+  delete cb;
+  ExpectNotShowInterstitial();
+
+  // Case 2: client thinks the page is phishing and so does the server but
+  // showing the interstitial is disabled => no interstitial is shown.
+  EXPECT_CALL(*csd_service_,
+              SendClientReportPhishingRequest(phishing_url, 1.0, _))
+      .WillOnce(SaveArg<2>(&cb));
+  csd_host_->OnDetectedPhishingSite(phishing_url, 1.0);
+  EXPECT_TRUE(Mock::VerifyAndClear(csd_service_.get()));
+  ASSERT_TRUE(cb != NULL);
+  cb->Run(phishing_url, true);
+  delete cb;
+  ExpectNotShowInterstitial();
+
+  // Case 3: client thinks the page is phishing and so does the server.
+  // We show an interstitial.
+  EXPECT_CALL(*csd_service_,
+              SendClientReportPhishingRequest(phishing_url, 1.0, _))
+      .WillOnce(SaveArg<2>(&cb));
+  csd_host_->OnDetectedPhishingSite(phishing_url, 1.0);
+  CommandLine::ForCurrentProcess()->AppendSwitch(
+      switches::kEnableClientSidePhishingInterstitial);
+  EXPECT_TRUE(Mock::VerifyAndClear(csd_service_.get()));
+  ASSERT_TRUE(cb != NULL);
+  cb->Run(phishing_url, true);
+  delete cb;
+  // Verify that we would show an interstitial in this case.
+  ExpectShowInterstital(phishing_url);
+
+  // Case 4 & 5: client thinks a page is phishing then navigates to
+  // another page which is also considered phishing by the client
+  // before the server responds with a verdict.  After a while the
+  // server responds for both requests with a phishing verdict.  Only
+  // a single interstitial is shown for the second URL.
+  EXPECT_CALL(*csd_service_,
+              SendClientReportPhishingRequest(phishing_url, 1.0, _))
+      .WillOnce(SaveArg<2>(&cb));
+  csd_host_->OnDetectedPhishingSite(phishing_url, 1.0);
+  EXPECT_TRUE(Mock::VerifyAndClear(csd_service_.get()));
+  ASSERT_TRUE(cb != NULL);
+  GURL other_phishing_url("http://other_phishing_url.com/bla");
+  // We navigate away.  The callback cb should be revoked.
+  NavigateAndCommit(other_phishing_url);
+  ClientSideDetectionService::ClientReportPhishingRequestCallback* cb_other;
+  EXPECT_CALL(*csd_service_,
+              SendClientReportPhishingRequest(other_phishing_url, 0.8, _))
+      .WillOnce(SaveArg<2>(&cb_other));
+  csd_host_->OnDetectedPhishingSite(other_phishing_url, 0.8);
+  EXPECT_TRUE(Mock::VerifyAndClear(csd_service_.get()));
+  cb->Run(phishing_url, true);  // Should have no effect.
+  delete cb;
+  cb_other->Run(other_phishing_url, true);  // Should show interstitial.
+  delete cb_other;
+  // Check that only the interstitial for the other URL is shown.
+  ExpectShowInterstital(other_phishing_url);
+}
+
+}  // namespace safe_browsing