Report Enum metrics from CertificateChecker.
The certificate checker was reporting a "user action" whenever an
update check HTTPS connection or HTTPS payload download had an invalid
HTTPS certificate or a valid one that was changed since the last
connection to the same server.
This patch sends an Enum metric for every HTTPS connection to check for
and update or download the payload with one of the three options: an
invalid certificate, a valid one already seen or a valid but different
certificate.
This patch also moves these metrics to the metrics.{h,cc} module, where
all the other metrics are reported, using an observer pattern in the
CertificateChecker, needed to remove the dependency on the metrics
library from the libpayload_consumer.
Bug: 25818567
TEST=FEATURES=test emerge-link update_engine; mma;
Change-Id: Ia1b6eb799e13b439b520ba14549d8973e18bcbfa
diff --git a/common/certificate_checker_unittest.cc b/common/certificate_checker_unittest.cc
index 3dfdadb..15811c0 100644
--- a/common/certificate_checker_unittest.cc
+++ b/common/certificate_checker_unittest.cc
@@ -22,12 +22,10 @@
#include <base/strings/stringprintf.h>
#include <gmock/gmock.h>
#include <gtest/gtest.h>
-#include <metrics/metrics_library_mock.h>
#include "update_engine/common/constants.h"
#include "update_engine/common/mock_certificate_checker.h"
#include "update_engine/common/mock_prefs.h"
-#include "update_engine/fake_system_state.h"
using ::testing::DoAll;
using ::testing::Return;
@@ -38,49 +36,41 @@
namespace chromeos_update_engine {
-class CertificateCheckerTest : public testing::Test {
+class MockCertificateCheckObserver : public CertificateChecker::Observer {
public:
- CertificateCheckerTest() {}
+ MOCK_METHOD2(CertificateChecked,
+ void(ServerToCheck server_to_check,
+ CertificateCheckResult result));
+};
+class CertificateCheckerTest : public testing::Test {
protected:
void SetUp() override {
- depth_ = 0;
- length_ = 4;
- digest_[0] = 0x17;
- digest_[1] = 0x7D;
- digest_[2] = 0x07;
- digest_[3] = 0x5F;
- digest_hex_ = "177D075F";
- diff_digest_hex_ = "1234ABCD";
- cert_key_prefix_ = kPrefsUpdateServerCertificate;
- server_to_check_ = CertificateChecker::kUpdate;
cert_key_ = base::StringPrintf("%s-%d-%d",
cert_key_prefix_.c_str(),
- server_to_check_,
+ static_cast<int>(server_to_check_),
depth_);
- kCertChanged = "Updater.ServerCertificateChanged";
- kCertFailed = "Updater.ServerCertificateFailed";
- CertificateChecker::set_system_state(&fake_system_state_);
- CertificateChecker::set_openssl_wrapper(&openssl_wrapper_);
- prefs_ = fake_system_state_.mock_prefs();
+ cert_checker.SetObserver(&observer_);
}
- void TearDown() override {}
+ void TearDown() override {
+ cert_checker.SetObserver(nullptr);
+ }
- FakeSystemState fake_system_state_;
- MockPrefs* prefs_; // shortcut to fake_system_state_.mock_prefs()
+ MockPrefs prefs_;
MockOpenSSLWrapper openssl_wrapper_;
// Parameters of our mock certificate digest.
- int depth_;
- unsigned int length_;
- uint8_t digest_[4];
- string digest_hex_;
- string diff_digest_hex_;
- string cert_key_prefix_;
- CertificateChecker::ServerToCheck server_to_check_;
+ int depth_{0};
+ unsigned int length_{4};
+ uint8_t digest_[4]{0x17, 0x7D, 0x07, 0x5F};
+ string digest_hex_{"177D075F"};
+ string diff_digest_hex_{"1234ABCD"};
+ string cert_key_prefix_{kPrefsUpdateServerCertificate};
+ ServerToCheck server_to_check_{ServerToCheck::kUpdate};
string cert_key_;
- string kCertChanged;
- string kCertFailed;
+
+ testing::StrictMock<MockCertificateCheckObserver> observer_;
+ CertificateChecker cert_checker{&prefs_, &openssl_wrapper_, server_to_check_};
};
// check certificate change, new
@@ -91,12 +81,12 @@
SetArgumentPointee<2>(length_),
SetArrayArgument<3>(digest_, digest_ + 4),
Return(true)));
- EXPECT_CALL(*prefs_, GetString(cert_key_, _))
- .WillOnce(Return(false));
- EXPECT_CALL(*prefs_, SetString(cert_key_, digest_hex_))
- .WillOnce(Return(true));
- ASSERT_TRUE(CertificateChecker::CheckCertificateChange(
- server_to_check_, 1, nullptr));
+ EXPECT_CALL(prefs_, GetString(cert_key_, _)).WillOnce(Return(false));
+ EXPECT_CALL(prefs_, SetString(cert_key_, digest_hex_)).WillOnce(Return(true));
+ EXPECT_CALL(observer_,
+ CertificateChecked(server_to_check_,
+ CertificateCheckResult::kValid));
+ ASSERT_TRUE(cert_checker.CheckCertificateChange(1, nullptr));
}
// check certificate change, unchanged
@@ -107,13 +97,13 @@
SetArgumentPointee<2>(length_),
SetArrayArgument<3>(digest_, digest_ + 4),
Return(true)));
- EXPECT_CALL(*prefs_, GetString(cert_key_, _))
- .WillOnce(DoAll(
- SetArgumentPointee<1>(digest_hex_),
- Return(true)));
- EXPECT_CALL(*prefs_, SetString(_, _)).Times(0);
- ASSERT_TRUE(CertificateChecker::CheckCertificateChange(
- server_to_check_, 1, nullptr));
+ EXPECT_CALL(prefs_, GetString(cert_key_, _))
+ .WillOnce(DoAll(SetArgumentPointee<1>(digest_hex_), Return(true)));
+ EXPECT_CALL(prefs_, SetString(_, _)).Times(0);
+ EXPECT_CALL(observer_,
+ CertificateChecked(server_to_check_,
+ CertificateCheckResult::kValid));
+ ASSERT_TRUE(cert_checker.CheckCertificateChange(1, nullptr));
}
// check certificate change, changed
@@ -124,61 +114,22 @@
SetArgumentPointee<2>(length_),
SetArrayArgument<3>(digest_, digest_ + 4),
Return(true)));
- EXPECT_CALL(*prefs_, GetString(cert_key_, _))
- .WillOnce(DoAll(
- SetArgumentPointee<1>(diff_digest_hex_),
- Return(true)));
- EXPECT_CALL(*prefs_, SetString(kPrefsCertificateReportToSendUpdate,
- kCertChanged))
- .WillOnce(Return(true));
- EXPECT_CALL(*prefs_, SetString(cert_key_, digest_hex_))
- .WillOnce(Return(true));
- ASSERT_TRUE(CertificateChecker::CheckCertificateChange(
- server_to_check_, 1, nullptr));
+ EXPECT_CALL(prefs_, GetString(cert_key_, _))
+ .WillOnce(DoAll(SetArgumentPointee<1>(diff_digest_hex_), Return(true)));
+ EXPECT_CALL(observer_,
+ CertificateChecked(server_to_check_,
+ CertificateCheckResult::kValidChanged));
+ EXPECT_CALL(prefs_, SetString(cert_key_, digest_hex_)).WillOnce(Return(true));
+ ASSERT_TRUE(cert_checker.CheckCertificateChange(1, nullptr));
}
// check certificate change, failed
TEST_F(CertificateCheckerTest, FailedCertificate) {
- EXPECT_CALL(*prefs_, SetString(kPrefsCertificateReportToSendUpdate,
- kCertFailed))
- .WillOnce(Return(true));
- EXPECT_CALL(*prefs_, GetString(_, _)).Times(0);
+ EXPECT_CALL(observer_, CertificateChecked(server_to_check_,
+ CertificateCheckResult::kFailed));
+ EXPECT_CALL(prefs_, GetString(_, _)).Times(0);
EXPECT_CALL(openssl_wrapper_, GetCertificateDigest(_, _, _, _)).Times(0);
- ASSERT_FALSE(CertificateChecker::CheckCertificateChange(
- server_to_check_, 0, nullptr));
-}
-
-// flush send report
-TEST_F(CertificateCheckerTest, FlushReport) {
- EXPECT_CALL(*prefs_, GetString(kPrefsCertificateReportToSendUpdate, _))
- .WillOnce(DoAll(
- SetArgumentPointee<1>(kCertChanged),
- Return(true)));
- EXPECT_CALL(*prefs_, GetString(kPrefsCertificateReportToSendDownload, _))
- .WillOnce(Return(false));
- EXPECT_CALL(*fake_system_state_.mock_metrics_lib(),
- SendUserActionToUMA(kCertChanged))
- .WillOnce(Return(true));
- EXPECT_CALL(*prefs_, Delete(kPrefsCertificateReportToSendUpdate))
- .WillOnce(Return(true));
- EXPECT_CALL(*prefs_, SetString(kPrefsCertificateReportToSendDownload, _))
- .Times(0);
- CertificateChecker::FlushReport();
-}
-
-// flush nothing to report
-TEST_F(CertificateCheckerTest, FlushNothingToReport) {
- string empty = "";
- EXPECT_CALL(*prefs_, GetString(kPrefsCertificateReportToSendUpdate, _))
- .WillOnce(DoAll(
- SetArgumentPointee<1>(empty),
- Return(true)));
- EXPECT_CALL(*prefs_, GetString(kPrefsCertificateReportToSendDownload, _))
- .WillOnce(Return(false));
- EXPECT_CALL(*fake_system_state_.mock_metrics_lib(),
- SendUserActionToUMA(_)).Times(0);
- EXPECT_CALL(*prefs_, SetString(_, _)).Times(0);
- CertificateChecker::FlushReport();
+ ASSERT_FALSE(cert_checker.CheckCertificateChange(0, nullptr));
}
} // namespace chromeos_update_engine