[webcrypto] Set the error type for failures.

content/child/webcrypto/shared_crypto_unittest.cc

Index: content/child/webcrypto/shared_crypto_unittest.cc
diff --git a/content/child/webcrypto/shared_crypto_unittest.cc b/content/child/webcrypto/shared_crypto_unittest.cc
index 815c9c2050776c12eabaa076da3e1865b6ebd706..908e5e2995193cec3c3547c3ee96a729e9faf469 100644
--- a/content/child/webcrypto/shared_crypto_unittest.cc
+++ b/content/child/webcrypto/shared_crypto_unittest.cc
@@ -17,6 +17,7 @@
 #include "base/path_service.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/string_util.h"
+#include "base/strings/stringprintf.h"
 #include "content/child/webcrypto/crypto_data.h"
 #include "content/child/webcrypto/status.h"
 #include "content/child/webcrypto/webcrypto_util.h"
@@ -40,9 +41,9 @@
 // Helper macros to verify the value of a Status.
 #define EXPECT_STATUS_ERROR(code) EXPECT_FALSE((code).IsSuccess())
 #define EXPECT_STATUS(expected, code) \
-  EXPECT_EQ(expected.ToString(), (code).ToString())
+  EXPECT_EQ(StatusToString(expected), StatusToString(code))
 #define ASSERT_STATUS(expected, code) \
-  ASSERT_EQ(expected.ToString(), (code).ToString())
+  ASSERT_EQ(StatusToString(expected), StatusToString(code))
 #define EXPECT_STATUS_SUCCESS(code) EXPECT_STATUS(Status::Success(), code)
 #define ASSERT_STATUS_SUCCESS(code) ASSERT_STATUS(Status::Success(), code)
 
@@ -52,6 +53,14 @@ namespace webcrypto {
 
 namespace {
 
+std::string StatusToString(const Status& status) {
+  if (status.IsSuccess())
+    return "Success";
+  return base::StringPrintf("error_type=%d: %s, error_details=",
+                            status.error_type(),
+                            status.error_details().c_str());
+}

[Ryan Sleevi, 2014/04/24 01:17:01]

This isn't really idiomatic GTest to do this.

The GTest friendly way -

bool StatusAreEqual(const Status& a, const Status& b) {
  if (a.IsSuccess() != b.IsSuccess())
    return false;
  if (a.IsSuccess())
    return true;
  return a.error_type() == b.error_type() && a.error_details ==
b.error_details();
}

void PrintTo(const Status& status, ::std::ostream* os) {
  if (status.IsSuccess())
    *os << "Success";
  else
    *os << "Error type: " << status.error_type() << " Error details: " <<
status.error_details();
}


And then
EXPECT_PRED_FORMAT2(StatusAreEqual, Status::DataError(), status);


If you decide not to use the GTest/GMock friendly comparators, then you've at
least got your format string wrong. Presumably you meant to write
"error_type=%d, error_details=%s" - otherwise, the error_details= seems
superfluous

[eroman, 2014/04/24 02:34:37]

I addressed this by changing the code to using EXPECT_EQ(), ASSERT_EQ() directly
on Status objects. And defining an operator==, operator!=, and PrintTo() for it
in the test file.

+
 // TODO(eroman): For Linux builds using system NSS, AES-GCM support is a
 // runtime dependency. Test it by trying to import a key.
 // TODO(padolph): Consider caching the result of the import key test.
@@ -479,7 +488,7 @@ Status AesGcmEncrypt(const blink::WebCryptoKey& key,
 
   if (output.byteLength() * 8 < tag_length_bits) {
     EXPECT_TRUE(false);
-    return Status::Error();
+    return Status::OperationError();
   }
 
   // The encryption result is cipher text with authentication tag appended.
@@ -682,17 +691,49 @@ TEST_F(SharedCryptoTest, CheckAesGcm) {
   }
 }
 
-TEST_F(SharedCryptoTest, StatusToString) {
-  EXPECT_EQ("Success", Status::Success().ToString());
-  EXPECT_EQ("", Status::Error().ToString());
-  EXPECT_EQ("The requested operation is unsupported",
-            Status::ErrorUnsupported().ToString());
+TEST_F(SharedCryptoTest, Status) {

[Ryan Sleevi, 2014/04/24 01:17:01]

It's not clear what you're trying to test by this test - what the success or
failure conditions are, or why it's necessary.

Please add a comment to this test to indicate what you're testing, and then
comments to each of these sections where you're doing status = "" to indicate
what you're testing.

The way you have it structured now, it seems you're just testing data identical
to what you have hardcoded, and that makes no sense.

[eroman, 2014/04/24 02:34:37]

The relevant distinction is to make sure that errors with no details, but a
different error type, do not compare as equal.

In other words, Status::OperationError() != Status::DataError() !=
Status::Success().

The intent of these tests is to verify that the comparison of errors works. If
it doesn't, then all of the assertions on the error earlier in this test file
mean very little.

Added a comment. Also now that it is testing the equality operator things are
clearer.

+  Status status = Status::Success();
+
+  EXPECT_EQ(false, status.IsError());
+  EXPECT_EQ("", status.error_details());
+
+  status = Status::OperationError();
+  EXPECT_EQ(true, status.IsError());
+  EXPECT_EQ("", status.error_details());
+  EXPECT_EQ(blink::WebCryptoErrorTypeOperation, status.error_type());
+
+  status = Status::DataError();
+  EXPECT_EQ(true, status.IsError());
+  EXPECT_EQ("", status.error_details());
+  EXPECT_EQ(blink::WebCryptoErrorTypeData, Status::DataError().error_type());
+
+  // Even though the error message is the same, these should not be considered
+  // the same by the tests.

[Ryan Sleevi, 2014/04/24 01:17:01]

This doesn't make any sense. Is "error message" the output of StatusToString? If
so, you're testing exactly the opposite of what you say. If not, it's ambiguous.

[eroman, 2014/04/24 02:34:37]

error message, versus error type.

+  EXPECT_NE(StatusToString(Status::DataError()),
+            StatusToString(Status::OperationError()));
+
+  status = Status::ErrorUnsupported();
+  EXPECT_EQ(true, status.IsError());
+  EXPECT_EQ("The requested operation is unsupported", status.error_details());
+  EXPECT_EQ(blink::WebCryptoErrorTypeNotSupported, status.error_type());
+
+  status = Status::ErrorJwkPropertyMissing("kty");
+  EXPECT_EQ(true, status.IsError());
   EXPECT_EQ("The required JWK property \"kty\" was missing",
-            Status::ErrorJwkPropertyMissing("kty").ToString());
+            status.error_details());
+  EXPECT_EQ(blink::WebCryptoErrorTypeData, Status::DataError().error_type());
+
+  status = Status::ErrorJwkPropertyWrongType("kty", "string");
+  EXPECT_EQ(true, status.IsError());
   EXPECT_EQ("The JWK property \"kty\" must be a string",
-            Status::ErrorJwkPropertyWrongType("kty", "string").ToString());
+            status.error_details());
+  EXPECT_EQ(blink::WebCryptoErrorTypeData, Status::DataError().error_type());
+
+  status = Status::ErrorJwkBase64Decode("n");
+  EXPECT_EQ(true, status.IsError());
   EXPECT_EQ("The JWK property \"n\" could not be base64 decoded",
-            Status::ErrorJwkBase64Decode("n").ToString());
+            status.error_details());
+  EXPECT_EQ(blink::WebCryptoErrorTypeData, Status::DataError().error_type());
 }
 
 TEST_F(SharedCryptoTest, DigestSampleSets) {
@@ -902,7 +943,7 @@ TEST_F(SharedCryptoTest, AesCbcFailures) {
     std::vector<uint8> iv(16);
 
     blink::WebCryptoKey key = blink::WebCryptoKey::createNull();
-    EXPECT_STATUS(Status::Error(),
+    EXPECT_STATUS(Status::ErrorImportAesKeyLength(),
                   ImportKey(blink::WebCryptoKeyFormatRaw,
                             CryptoData(key_raw),
                             CreateAesCbcAlgorithm(iv),
@@ -972,7 +1013,7 @@ TEST_F(SharedCryptoTest, MAYBE(AesCbcSampleSets)) {
     // up testing decryption over empty cipher text.
     if (test_cipher_text.size() >= kAesCbcBlockSize) {
       EXPECT_STATUS(
-          Status::Error(),
+          Status::OperationError(),
           Decrypt(CreateAesCbcAlgorithm(test_iv),
                   key,
                   CryptoData(&test_cipher_text[0],
@@ -984,7 +1025,7 @@ TEST_F(SharedCryptoTest, MAYBE(AesCbcSampleSets)) {
     // a few bytes off the cipher text.
     if (test_cipher_text.size() > 3) {
       EXPECT_STATUS(
-          Status::Error(),
+          Status::OperationError(),
           Decrypt(CreateAesCbcAlgorithm(test_iv),
                   key,
                   CryptoData(&test_cipher_text[0], test_cipher_text.size() - 3),
@@ -1822,7 +1863,7 @@ TEST_F(SharedCryptoTest, MAYBE(ImportExportSpki)) {
 
   // Failing case: Bad DER encoding.
   EXPECT_STATUS(
-      Status::Error(),
+      Status::DataError(),
       ImportKey(blink::WebCryptoKeyFormatSpki,
                 CryptoData(HexStringToBytes("618333c4cb")),
                 CreateAlgorithm(blink::WebCryptoAlgorithmIdRsaEsPkcs1v1_5),
@@ -1831,7 +1872,7 @@ TEST_F(SharedCryptoTest, MAYBE(ImportExportSpki)) {
                 &key));
 
   // Failing case: Import RSA key but provide an inconsistent input algorithm.
-  EXPECT_STATUS(Status::Error(),
+  EXPECT_STATUS(Status::DataError(),
                 ImportKey(blink::WebCryptoKeyFormatSpki,
                           CryptoData(HexStringToBytes(kPublicKeySpkiDerHex)),
                           CreateAlgorithm(blink::WebCryptoAlgorithmIdAesCbc),
@@ -1905,7 +1946,7 @@ TEST_F(SharedCryptoTest, MAYBE(ImportExportPkcs8)) {
 
   // Failing case: Bad DER encoding.
   EXPECT_STATUS(
-      Status::Error(),
+      Status::DataError(),
       ImportKey(blink::WebCryptoKeyFormatPkcs8,
                 CryptoData(HexStringToBytes("618333c4cb")),
                 CreateAlgorithm(blink::WebCryptoAlgorithmIdRsaSsaPkcs1v1_5),
@@ -1914,7 +1955,7 @@ TEST_F(SharedCryptoTest, MAYBE(ImportExportPkcs8)) {
                 &key));
 
   // Failing case: Import RSA key but provide an inconsistent input algorithm.
-  EXPECT_STATUS(Status::Error(),
+  EXPECT_STATUS(Status::DataError(),
                 ImportKey(blink::WebCryptoKeyFormatPkcs8,
                           CryptoData(HexStringToBytes(kPrivateKeyPkcs8DerHex)),
                           CreateAlgorithm(blink::WebCryptoAlgorithmIdAesCbc),
@@ -2229,7 +2270,7 @@ TEST_F(SharedCryptoTest, MAYBE(RsaEsFailures)) {
           algorithm, private_key, CryptoData(message_hex), &encrypted_data));
 
   // Fail encrypt with empty message.
-  EXPECT_STATUS(Status::Error(),
+  EXPECT_STATUS(Status::ErrorDataTooSmall(),
                 Encrypt(algorithm,
                         public_key,
                         CryptoData(std::vector<uint8>()),
@@ -2263,7 +2304,7 @@ TEST_F(SharedCryptoTest, MAYBE(RsaEsFailures)) {
       static_cast<uint8*>(encrypted_data.data()) + encrypted_data.byteLength());
   corrupted_data[corrupted_data.size() / 2] ^= 0x01;
   EXPECT_STATUS(
-      Status::Error(),
+      Status::OperationError(),
       Decrypt(
           algorithm, private_key, CryptoData(corrupted_data), &decrypted_data));
 
@@ -2507,7 +2548,7 @@ TEST_F(SharedCryptoTest, MAYBE(AesKwKeyImport)) {
   ExpectArrayBufferMatchesHex(key_raw_hex_in, key_raw_out);
 
   // Fail import of 0 length key
-  EXPECT_STATUS(Status::Error(),
+  EXPECT_STATUS(Status::ErrorImportAesKeyLength(),
                 ImportKey(blink::WebCryptoKeyFormatRaw,
                           CryptoData(HexStringToBytes("")),
                           algorithm,
@@ -2517,7 +2558,7 @@ TEST_F(SharedCryptoTest, MAYBE(AesKwKeyImport)) {
 
   // Fail import of 124-bit KEK
   key_raw_hex_in = "3e4566a2bdaa10cb68134fa66c15ddb";
-  EXPECT_STATUS(Status::Error(),
+  EXPECT_STATUS(Status::ErrorImportAesKeyLength(),
                 ImportKey(blink::WebCryptoKeyFormatRaw,
                           CryptoData(HexStringToBytes(key_raw_hex_in)),
                           algorithm,
@@ -2527,7 +2568,7 @@ TEST_F(SharedCryptoTest, MAYBE(AesKwKeyImport)) {
 
   // Fail import of 200-bit KEK
   key_raw_hex_in = "0a1d88608a5ad9fec64f1ada269ebab4baa2feeb8d95638c0e";
-  EXPECT_STATUS(Status::Error(),
+  EXPECT_STATUS(Status::ErrorImportAesKeyLength(),
                 ImportKey(blink::WebCryptoKeyFormatRaw,
                           CryptoData(HexStringToBytes(key_raw_hex_in)),
                           algorithm,
@@ -2538,7 +2579,7 @@ TEST_F(SharedCryptoTest, MAYBE(AesKwKeyImport)) {
   // Fail import of 260-bit KEK
   key_raw_hex_in =
       "72d4e475ff34215416c9ad9c8281247a4d730c5f275ac23f376e73e3bce8d7d5a";
-  EXPECT_STATUS(Status::Error(),
+  EXPECT_STATUS(Status::ErrorImportAesKeyLength(),
                 ImportKey(blink::WebCryptoKeyFormatRaw,
                           CryptoData(HexStringToBytes(key_raw_hex_in)),
                           algorithm,
@@ -2732,7 +2773,7 @@ TEST_F(SharedCryptoTest, MAYBE(AesKwRawSymkeyUnwrapCorruptData)) {
   // AES-KW's built-in integrity check.
   blink::WebCryptoKey unwrapped_key = blink::WebCryptoKey::createNull();
   EXPECT_STATUS(
-      Status::Error(),
+      Status::OperationError(),
       UnwrapKey(blink::WebCryptoKeyFormatRaw,
                 CryptoData(Corrupted(test_ciphertext)),
                 wrapping_key,
@@ -2867,7 +2908,7 @@ TEST_F(SharedCryptoTest, MAYBE(AesGcmSampleSets)) {
     EXPECT_TRUE(ArrayBufferMatches(test_plain_text, plain_text));
 
     // Decryption should fail if any of the inputs are tampered with.
-    EXPECT_STATUS(Status::Error(),
+    EXPECT_STATUS(Status::OperationError(),
                   AesGcmDecrypt(key,
                                 Corrupted(test_iv),
                                 test_additional_data,
@@ -2875,7 +2916,7 @@ TEST_F(SharedCryptoTest, MAYBE(AesGcmSampleSets)) {
                                 test_cipher_text,
                                 test_authentication_tag,
                                 &plain_text));
-    EXPECT_STATUS(Status::Error(),
+    EXPECT_STATUS(Status::OperationError(),
                   AesGcmDecrypt(key,
                                 test_iv,
                                 Corrupted(test_additional_data),
@@ -2883,7 +2924,7 @@ TEST_F(SharedCryptoTest, MAYBE(AesGcmSampleSets)) {
                                 test_cipher_text,
                                 test_authentication_tag,
                                 &plain_text));
-    EXPECT_STATUS(Status::Error(),
+    EXPECT_STATUS(Status::OperationError(),
                   AesGcmDecrypt(key,
                                 test_iv,
                                 test_additional_data,
@@ -2891,7 +2932,7 @@ TEST_F(SharedCryptoTest, MAYBE(AesGcmSampleSets)) {
                                 Corrupted(test_cipher_text),
                                 test_authentication_tag,
                                 &plain_text));
-    EXPECT_STATUS(Status::Error(),
+    EXPECT_STATUS(Status::OperationError(),
                   AesGcmDecrypt(key,
                                 test_iv,
                                 test_additional_data,
@@ -3253,7 +3294,7 @@ TEST_F(SharedCryptoTest, MAYBE(RsaEsJwkSymkeyWrapUnwrapErrors)) {
   // Treat the ciphertext as a wrapped key and try to unwrap it. Ensure a
   // generic error is received.
   blink::WebCryptoKey unwrapped_key = blink::WebCryptoKey::createNull();
-  EXPECT_STATUS(Status::Error(),
+  EXPECT_STATUS(Status::OperationError(),
                 UnwrapKey(blink::WebCryptoKeyFormatJwk,
                           CryptoData(ciphertext),
                           private_wrapping_key,