enhance "hit paywall" error message to mention the probable cause

Reporting errors from Done() is bad for progress reporting and such, so
factoring this out is a good idea and we start with moving the supposed-
to-be clearsigned file isn't clearsigned out first – improving the error
message in the process as we use the same message for a similar case
(NODATA) as this is what I have to look at with the venue wifi at
DebCamp and the old errormessage doesn't really say anything.

6 files changed, 83 insertions, 60 deletions

diff --git a/apt-pkg/acquire-item.cc b/apt-pkg/acquire-item.cc
index 8a566fea0..0a7e0e11f 100644
--- a/apt-pkg/acquire-item.cc
+++ b/apt-pkg/acquire-item.cc
@@ -514,6 +514,23 @@ void pkgAcquire::Item::Start(string const &/*Message*/, unsigned long long const
       FileSize = Size;
 }
 									/*}}}*/
+// Acquire::Item::VerifyDone - check if Item was downloaded OK		/*{{{*/
+/* Note that hash-verification is 'hardcoded' in acquire-worker and has
+ * already passed if this method is called. */
+bool pkgAcquire::Item::VerifyDone(std::string const &Message,
+	 pkgAcquire::MethodConfig const * const /*Cnf*/)
+{
+   std::string const FileName = LookupTag(Message,"Filename");
+   if (FileName.empty() == true)
+   {
+      Status = StatError;
+      ErrorText = "Method gave a blank filename";
+      return false;
+   }
+
+   return true;
+}
+									/*}}}*/
 // Acquire::Item::Done - Item downloaded OK				/*{{{*/
 void pkgAcquire::Item::Done(string const &/*Message*/, HashStringList const &Hashes,
 			    pkgAcquire::MethodConfig const * const /*Cnf*/)
@@ -585,8 +602,8 @@ bool pkgAcquire::Item::RenameOnError(pkgAcquire::Item::RenameOnErrorState const
 	 Status = StatError;
 	 break;
       case NotClearsigned:
-	 errtext = _("Does not start with a cleartext signature");
-	 Status = StatError;
+	 strprintf(errtext, _("Clearsigned file isn't valid, got '%s' (does the network require authentication?)"), "NOSPLIT");
+	 Status = StatAuthError;
 	 break;
       case MaximumSizeExceeded:
 	 // the method is expected to report a good error for this
@@ -783,7 +800,7 @@ bool pkgAcqMetaBase::CheckStopAuthentication(pkgAcquire::Item * const I, const s
       _error->Error(_("GPG error: %s: %s"),
                     Desc.Description.c_str(),
                     LookupTag(Message,"Message").c_str());
-      I->Status = StatError;
+      I->Status = StatAuthError;
       return true;
    } else {
       _error->Warning(_("GPG error: %s: %s"),
@@ -829,14 +846,7 @@ bool pkgAcqMetaBase::CheckDownloadDone(pkgAcqTransactionItem * const I, const st
    // We have just finished downloading a Release file (it is not
    // verified yet)
 
-   string const FileName = LookupTag(Message,"Filename");
-   if (FileName.empty() == true)
-   {
-      I->Status = StatError;
-      I->ErrorText = "Method gave a blank filename";
-      return false;
-   }
-
+   std::string const FileName = LookupTag(Message,"Filename");
    if (FileName != I->DestFile && RealFileExists(I->DestFile) == false)
    {
       I->Local = true;
@@ -1142,6 +1152,16 @@ string pkgAcqMetaClearSig::Custom600Headers() const
    return Header;
 }
 									/*}}}*/
+bool pkgAcqMetaClearSig::VerifyDone(std::string const &Message,
+	 pkgAcquire::MethodConfig const * const Cnf)
+{
+   Item::VerifyDone(Message, Cnf);
+
+   if (FileExists(DestFile) && !StartsWithGPGClearTextSignature(DestFile))
+      return RenameOnError(NotClearsigned);
+
+   return true;
+}
 // pkgAcqMetaClearSig::Done - We got a file				/*{{{*/
 void pkgAcqMetaClearSig::Done(std::string const &Message,
                               HashStringList const &Hashes,
@@ -1149,17 +1169,6 @@ void pkgAcqMetaClearSig::Done(std::string const &Message,
 {
    Item::Done(Message, Hashes, Cnf);
 
-   // if we expect a ClearTextSignature (InRelease), ensure that
-   // this is what we get and if not fail to queue a 
-   // Release/Release.gpg, see #346386
-   if (FileExists(DestFile) && !StartsWithGPGClearTextSignature(DestFile))
-   {
-      pkgAcquire::Item::Failed(Message, Cnf);
-      RenameOnError(NotClearsigned);
-      TransactionManager->AbortTransaction();
-      return;
-   }
-
    if(AuthPass == false)
    {
       if(CheckDownloadDone(this, Message, Hashes) == true)
@@ -1190,6 +1199,16 @@ void pkgAcqMetaClearSig::Failed(string const &Message,pkgAcquire::MethodConfig c
 
    if (AuthPass == false)
    {
+      if (Status == StatAuthError)
+      {
+	 // if we expected a ClearTextSignature (InRelease) and got a file,
+	 // but it wasn't valid we end up here (see VerifyDone).
+	 // As these is usually called by web-portals we do not try Release/Release.gpg
+	 // as this is gonna fail anyway and instead abort our try (LP#346386)
+	 TransactionManager->AbortTransaction();
+	 return;
+      }
+
       // Queue the 'old' InRelease file for removal if we try Release.gpg
       // as otherwise the file will stay around and gives a false-auth
       // impression (CVE-2012-0214)
@@ -2500,7 +2519,7 @@ void pkgAcqIndex::StageDownloadDone(string const &Message, HashStringList const
    Complete = true;
 
    // Handle the unzipd case
-   string FileName = LookupTag(Message,"Alt-Filename");
+   std::string FileName = LookupTag(Message,"Alt-Filename");
    if (FileName.empty() == false)
    {
       Stage = STAGE_DECOMPRESS_AND_VERIFY;
@@ -2511,13 +2530,7 @@ void pkgAcqIndex::StageDownloadDone(string const &Message, HashStringList const
       SetActiveSubprocess("copy");
       return;
    }
-
    FileName = LookupTag(Message,"Filename");
-   if (FileName.empty() == true)
-   {
-      Status = StatError;
-      ErrorText = "Method gave a blank filename";
-   }
 
    // Methods like e.g. "file:" will give us a (compressed) FileName that is
    // not the "DestFile" we set, in this case we uncompress from the local file
@@ -2791,15 +2804,7 @@ void pkgAcqArchive::Done(string const &Message, HashStringList const &Hashes,
    Item::Done(Message, Hashes, Cfg);
 
    // Grab the output filename
-   string FileName = LookupTag(Message,"Filename");
-   if (FileName.empty() == true)
-   {
-      Status = StatError;
-      ErrorText = "Method gave a blank filename";
-      return;
-   }
-
-   // Reference filename
+   std::string const FileName = LookupTag(Message,"Filename");
    if (DestFile !=  FileName && RealFileExists(DestFile) == false)
    {
       StoreFilename = DestFile = FileName;
@@ -3121,14 +3126,7 @@ void pkgAcqFile::Done(string const &Message,HashStringList const &CalcHashes,
 {
    Item::Done(Message,CalcHashes,Cnf);
 
-   string FileName = LookupTag(Message,"Filename");
-   if (FileName.empty() == true)
-   {
-      Status = StatError;
-      ErrorText = "Method gave a blank filename";
-      return;
-   }
-
+   std::string const FileName = LookupTag(Message,"Filename");
    Complete = true;
 
    // The files timestamp matches
diff --git a/apt-pkg/acquire-item.h b/apt-pkg/acquire-item.h
index 2349d386c..d6bcdafcb 100644
--- a/apt-pkg/acquire-item.h
+++ b/apt-pkg/acquire-item.h
@@ -176,6 +176,28 @@ class pkgAcquire::Item : public WeakPointable				/*{{{*/
     */
    virtual void Failed(std::string const &Message,pkgAcquire::MethodConfig const * const Cnf);
 
+   /** \brief Invoked by the acquire worker to check if the successfully
+    * fetched object is also the objected we wanted to have.
+    *
+    *  Note that the object might \e not have been written to
+    *  DestFile; check for the presence of an Alt-Filename entry in
+    *  Message to find the file to which it was really written.
+    *
+    *  This is called before Done is called and can prevent it by returning
+    *  \b false which will result in Failed being called instead.
+    *
+    *  You should prefer to use this method over calling Failed() from Done()
+    *  as this has e.g. the wrong progress reporting.
+    *
+    *  \param Message Data from the acquire method.  Use LookupTag()
+    *  to parse it.
+    *  \param Cnf The method via which the object was fetched.
+    *
+    *  \sa pkgAcqMethod
+    */
+   virtual bool VerifyDone(std::string const &Message,
+	 pkgAcquire::MethodConfig const * const Cnf);
+
    /** \brief Invoked by the acquire worker when the object was
     *  fetched successfully.
     *
@@ -563,6 +585,7 @@ class APT_HIDDEN pkgAcqMetaClearSig : public pkgAcqMetaIndex
 
    virtual void Failed(std::string const &Message,pkgAcquire::MethodConfig const * const Cnf) APT_OVERRIDE;
    virtual std::string Custom600Headers() const APT_OVERRIDE;
+   virtual bool VerifyDone(std::string const &Message, pkgAcquire::MethodConfig const * const Cnf) APT_OVERRIDE;
    virtual void Done(std::string const &Message, HashStringList const &Hashes,
 		     pkgAcquire::MethodConfig const * const Cnf) APT_OVERRIDE;
 
diff --git a/apt-pkg/acquire-worker.cc b/apt-pkg/acquire-worker.cc
index dc03a8870..2c84020fe 100644
--- a/apt-pkg/acquire-worker.cc
+++ b/apt-pkg/acquire-worker.cc
@@ -412,10 +412,13 @@ bool pkgAcquire::Worker::RunMessages()
 		  consideredOkay = true;
 
 	       if (consideredOkay == true)
+		  consideredOkay = Owner->VerifyDone(Message, Config);
+	       else // hashsum mismatch
+		  Owner->Status = pkgAcquire::Item::StatAuthError;
+
+	       if (consideredOkay == true)
 	       {
 		  Owner->Done(Message, ReceivedHashes, Config);
-
-		  // Log that we are done
 		  if (Log != 0)
 		  {
 		     if (isIMSHit)
@@ -426,9 +429,7 @@ bool pkgAcquire::Worker::RunMessages()
 	       }
 	       else
 	       {
-		  Owner->Status = pkgAcquire::Item::StatAuthError;
 		  Owner->Failed(Message,Config);
-
 		  if (Log != 0)
 		     Log->Fail(Owner->GetItemDesc());
 	       }
diff --git a/test/integration/test-apt-update-nofallback b/test/integration/test-apt-update-nofallback
index 2ded73122..5bffab6ee 100755
--- a/test/integration/test-apt-update-nofallback
+++ b/test/integration/test-apt-update-nofallback
@@ -151,9 +151,8 @@ test_subvert_inrelease()
     # replace InRelease with something else
     mv $APTARCHIVE/dists/unstable/Release $APTARCHIVE/dists/unstable/InRelease
 
-    testfailureequal "W: Failed to fetch file:${APTARCHIVE}/dists/unstable/InRelease  Does not start with a cleartext signature
-
-E: Some index files failed to download. They have been ignored, or old ones used instead." aptget update -qq
+    testfailuremsg "W: Failed to fetch file:${APTARCHIVE}/dists/unstable/InRelease  Clearsigned file isn't valid, got 'NOSPLIT' (does the network require authentication?)
+E: Some index files failed to download. They have been ignored, or old ones used instead." aptget update
 
     # ensure we keep the repo
     testfileequal lists.before "$(listcurrentlistsdirectory)"
diff --git a/test/integration/test-ubuntu-bug-346386-apt-get-update-paywall b/test/integration/test-ubuntu-bug-346386-apt-get-update-paywall
index ea516fc12..8f468b376 100755
--- a/test/integration/test-ubuntu-bug-346386-apt-get-update-paywall
+++ b/test/integration/test-ubuntu-bug-346386-apt-get-update-paywall
@@ -36,8 +36,8 @@ ensure_n_canary_strings_in_dir() {
 
 LISTS='rootdir/var/lib/apt/lists'
 rm -rf rootdir/var/lib/apt/lists
-msgtest 'Got expected failure message' 'apt-get update'
-aptget update -qq 2>&1 | grep -q 'W:.*Does not start with a cleartext signature' && msgpass || msgfail
+testfailure aptget update
+testsuccess grep '^W:.*Clearsigned file .*NOSPLIT.*' rootdir/tmp/testfailure.output
 
 ensure_n_canary_strings_in_dir $LISTS 'ni ni ni' 0
 testequal 'lock
@@ -48,8 +48,8 @@ for f in Release Release.gpg main_binary-amd64_Packages main_source_Sources; do
     echo 'peng neee-wom' > $LISTS/localhost:8080_dists_stable_${f}
 done
 
-msgtest 'Got expected failure message in' 'apt-get update'
-aptget update -qq 2>&1 | grep -q 'W:.*Does not start with a cleartext signature' && msgpass || msgfail
+testfailure aptget update
+testsuccess grep '^W:.*Clearsigned file .*NOSPLIT.*' rootdir/tmp/testfailure.output
 
 ensure_n_canary_strings_in_dir $LISTS 'peng neee-wom' 4
 ensure_n_canary_strings_in_dir $LISTS 'ni ni ni' 0
@@ -58,7 +58,8 @@ ensure_n_canary_strings_in_dir $LISTS 'ni ni ni' 0
 echo 'peng neee-wom' > $LISTS/localhost:8080_dists_stable_InRelease
 rm -f $LISTS/localhost:8080_dists_stable_Release $LISTS/localhost:8080_dists_stable_Release.gpg
 msgtest 'excpected failure of' 'apt-get update'
-aptget update -qq 2>&1 | grep -q 'W:.*Does not start with a cleartext signature' && msgpass || msgfail
+testfailure aptget update
+testsuccess grep '^W:.*Clearsigned file .*NOSPLIT.*' rootdir/tmp/testfailure.output
 
 ensure_n_canary_strings_in_dir $LISTS 'peng neee-wom' 3
 ensure_n_canary_strings_in_dir $LISTS 'ni ni ni' 0
diff --git a/test/integration/test-ubuntu-bug-784473-InRelease-one-message-only b/test/integration/test-ubuntu-bug-784473-InRelease-one-message-only
index 754487a90..dbcb49a41 100755
--- a/test/integration/test-ubuntu-bug-784473-InRelease-one-message-only
+++ b/test/integration/test-ubuntu-bug-784473-InRelease-one-message-only
@@ -27,8 +27,9 @@ MD5Sum:
  1b895931853981ad8204d2439821b999     4144 Packages.gz'; echo; cat ${RELEASE}.old;) > ${RELEASE}
 done
 
-msgtest 'The unsigned garbage before signed block is' 'ignored'
-aptget update -qq 2>&1 | grep -q 'W:.*Does not start with a cleartext signature' && msgpass || msgfail
+testfailure aptget update
+testsuccess grep '^W:.*Clearsigned file .*NOSPLIT.*' rootdir/tmp/testfailure.output
+
 
 ROOTDIR="$(readlink -f .)"
 testsuccessequal "Package files: