Certificate Transparency: Fetching consistency proofs.

components/certificate_transparency/log_proof_fetcher.h

Index: components/certificate_transparency/log_proof_fetcher.h
diff --git a/components/certificate_transparency/log_proof_fetcher.h b/components/certificate_transparency/log_proof_fetcher.h
index ba952e365d6f77177184a9c70648e2b9f6ef6b09..220bef6617bf757350daec603fc29a5827718ef1 100644
--- a/components/certificate_transparency/log_proof_fetcher.h
+++ b/components/certificate_transparency/log_proof_fetcher.h
@@ -7,12 +7,14 @@
 
 #include <map>
 #include <string>
+#include <vector>
 
 #include "base/callback.h"
 #include "base/macros.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/memory/weak_ptr.h"
 #include "net/url_request/url_request.h"
+#include "url/gurl.h"

[Ryan Sleevi, 2015/12/08 01:13:52]

You don't need this;

Alternatively, delete line 33, but pretty sure you don't need this :)

[Eran Messeri, 2015/12/14 13:01:42]

Done.

 
 namespace base {
 class Value;
@@ -36,7 +38,7 @@ namespace certificate_transparency {
 // Transparency logs using the URLRequestContext provided during the instance
 // construction.
 // Must outlive the provided URLRequestContext.
-class LogProofFetcher : public net::URLRequest::Delegate {
+class LogProofFetcher {
  public:
   static const size_t kMaxLogResponseSizeInBytes = 600;
 
@@ -53,8 +55,16 @@ class LogProofFetcher : public net::URLRequest::Delegate {
   using FetchFailedCallback = base::Callback<
       void(const std::string& log_id, int net_error, int http_response_code)>;
 
+  // Callback for successful retrieval of consistency proofs between two
+  // STHs. Called with the log_id of the log the consistency belongs to (as
+  // supplied by the caller to FetchConsistencyProof) and the vector of
+  // proof nodes.
+  using ConsistencyProofFetchedCallback =
+      base::Callback<void(const std::string& log_id,
+                          const std::vector<std::string>& consistency_proof)>;
+
   explicit LogProofFetcher(net::URLRequestContext* request_context);
-  ~LogProofFetcher() override;
+  ~LogProofFetcher();
 
   // Fetch the latest Signed Tree Head from the log identified by |log_id|
   // from |base_log_url|. The |log_id| will be passed into the callbacks to
@@ -73,44 +83,50 @@ class LogProofFetcher : public net::URLRequest::Delegate {
       const SignedTreeHeadFetchedCallback& fetched_callback,
       const FetchFailedCallback& failed_callback);
 
-  // net::URLRequest::Delegate
-  void OnResponseStarted(net::URLRequest* request) override;
-  void OnReadCompleted(net::URLRequest* request, int bytes_read) override;
+  // Fetch a consistency proof between the Merkle trees identified by
+  // |old_tree_size| and |new_tree_size| of the log identified by |log_id|
+  // from |base_log_url|.
+  //
+  // See the documentation of FetchSignedTreeHead regarding request destruction
+  // and multiple requests to the same log.
+  void FetchConsistencyProof(
+      const GURL& base_log_url,
+      const std::string& log_id,
+      uint64_t old_tree_size,
+      uint64_t new_tree_size,
+      const ConsistencyProofFetchedCallback& fetched_callback,
+      const FetchFailedCallback& failed_callback);
 
  private:
-  struct FetchState;
-  // Handles the final result of a URLRequest::Read call on |request|.
-  // Returns true if another read should be started, false if the read
-  // failed completely or we have to wait for OnResponseStarted to
-  // be called.
-  bool HandleReadResult(net::URLRequest* request,
-                        FetchState* params,
-                        int bytes_read);
-
-  // Calls URLRequest::Read on |request| repeatedly, until HandleReadResult
-  // indicates it should no longer be called. Usually this would be when there
-  // is pending IO that requires waiting for OnResponseStarted to be called.
-  void StartNextRead(net::URLRequest* request, FetchState* params);
-
-  // Performs post-report cleanup.
-  void RequestComplete(net::URLRequest* request);
-  // Deletes the request and associated FetchState from the internal map.
-  void CleanupRequest(net::URLRequest* request);
-  // Invokes the failure callback with the supplied arguments, then cleans up
-  // the request.
-  void InvokeFailureCallback(net::URLRequest* request,
-                             int net_error,
-                             int http_response_code);
-
-  // Callbacks for parsing the STH's JSON by the SafeJsonParser
-  void OnSTHJsonParseSuccess(net::URLRequest* request,
-                             scoped_ptr<base::Value> parsed_json);
-  void OnSTHJsonParseError(net::URLRequest* request, const std::string& error);
+  class LogResponseHandler;
+  class GetSTHLogResponseHandler;
+  class GetConsistencyProofLogResponseHandler;
+  class LogFetcher;
+
+  // Creates and stores a LogFetcher, which takes care of fetching
+  // and assembling the CT log's response to a single request.
+  // The created LogFetcher, as well as |log_handler| will be stored
+  // in the map for later cleanup.
+  void StartFetch(const GURL& request_url, LogResponseHandler* log_handler);
+
+  // Callbacks for handling the fetch result. Invoked by the LogFetcher.
+  void OnFetchCompleted(LogFetcher* fetcher);
+  void OnFetchFailed(LogFetcher* fetcher,
+                     int net_error,
+                     int http_response_code);
+
+  // Callbacks for parsing the response JSON by the SafeJsonParser
+  void OnJsonParseSuccess(LogFetcher* fetcher,
+                          scoped_ptr<base::Value> parsed_json);
+  void OnJsonParseError(LogFetcher* fetcher, const std::string& error);
+
+  // Deletes, and removes, the |fetcher| and associated LogResponseHandler
+  // from the |inflight_fetches_| map.
+  void CleanupRequest(LogFetcher* fetcher);
 
   net::URLRequestContext* const request_context_;
 
-  // Owns the contained requests, as well as FetchState.
-  std::map<net::URLRequest*, FetchState*> inflight_requests_;
+  std::map<LogFetcher*, LogResponseHandler*> inflight_fetches_;

[mmenke, 2015/12/08 17:10:12]

I still think you should have the handler own the fetcher.  Having this class
deal with transferring data from one to the other seems weird.

It also requires 2 or 3 extra methods, and using a map rather than a vector
(Maybe it's just my own bias, but a map that owns the objects pointed to be the
keys seems to have a bit of code smell).

[Eran Messeri, 2015/12/14 13:01:42]

Done - now the LogProofFetcher has a set of LogResponseHandlers and only gets a
callback once everything is done, to dispose of the LogResponseHandler.

 
   base::WeakPtrFactory<LogProofFetcher> weak_factory_;