[webcrypto] Add error messages for failed operations.

content/renderer/webcrypto/webcrypto_impl.cc

Index: content/renderer/webcrypto/webcrypto_impl.cc
diff --git a/content/renderer/webcrypto/webcrypto_impl.cc b/content/renderer/webcrypto/webcrypto_impl.cc
index 5d5a1415bb13cf28d5255cd384496e166831419d..782f475ccf803f4195d20b101574cd49518d1f04 100644
--- a/content/renderer/webcrypto/webcrypto_impl.cc
+++ b/content/renderer/webcrypto/webcrypto_impl.cc
@@ -13,15 +13,27 @@
 #include "base/memory/scoped_ptr.h"
 #include "base/strings/string_piece.h"
 #include "base/values.h"
-#include "content/renderer/webcrypto/webcrypto_util.h"
 #include "third_party/WebKit/public/platform/WebCryptoAlgorithm.h"
 #include "third_party/WebKit/public/platform/WebCryptoAlgorithmParams.h"
 #include "third_party/WebKit/public/platform/WebCryptoKey.h"
+#include "third_party/WebKit/public/platform/WebString.h"
 
 namespace content {
 
+using webcrypto::Status;

[Ryan Sleevi, 2014/01/24 01:21:50]

I thought this was discouraged in Chromium side.

[eroman, 2014/01/24 03:19:21]

The practice is quite common in Chrome code, occurring nearly 13,000 times in
Chromium code:

$ cd src
$ git grep '^using ' | wc -l
12824

I am unaware of any decrees forbidding it.

[Ryan Sleevi, 2014/01/24 20:27:01]

Thread:

https://groups.google.com/a/chromium.org/d/msg/chromium-dev/qSrl_UAqAkQ/mn5OV...

Summary:

https://groups.google.com/a/chromium.org/d/msg/chromium-dev/qSrl_UAqAkQ/SGDZG...

[eroman, 2014/01/24 21:33:10]

Thanks for the link to the thread!

I'll give it a more detailed look after lunch, but it doesn't seem there was any
real consensus on that thread, and the style guide was never amended.

At any rate, I have uploaded a patchset with the prefix for comparison to see if
you prefer it. My take is the extra verbosity of webcrypto:: inside of a
WebCryptoImpl class is pretty weird.

The summary on that thread leaves the decision up to the author's judgement.

+
 namespace {
 
+// Complete |result| with a failure if |status| indicates an error and return
+// true. Otherwise return false.
+bool completeWithError(const Status& status, blink::WebCryptoResult* result) {

[Ryan Sleevi, 2014/01/24 01:21:50]

naming: I find it counter-intuitive that completeWithError takes a negative
result (an error), and will return true if it was an error, because the phrase
"complete with error" suggests "perform this task", not "is this an error"

if (AbortOnErrorStatus(status, &result))
  return;

style: completeWithError -> CompleteWithError

[eroman, 2014/01/24 03:19:21]

How about CompletedWithError() ?

[Ryan Sleevi, 2014/01/24 20:27:01]

I still find that a bit confusing. ShouldReturnWithError() ?

[eroman, 2014/01/25 03:26:08]

Seeing how this was confusing, I removed the function all together, and replaced
it with something a bit more verbose but easier to understand.

+  if (status.IsError()) {
+    result->completeWithError(blink::WebString::fromUTF8(status.ToString()));
+    return true;
+  }
+  return false;
+}
+
 bool IsAlgorithmAsymmetric(const blink::WebCryptoAlgorithm& algorithm) {
   // TODO(padolph): include all other asymmetric algorithms once they are
   // defined, e.g. EC and DH.
@@ -175,9 +187,8 @@ void WebCryptoImpl::encrypt(
     blink::WebCryptoResult result) {
   DCHECK(!algorithm.isNull());
   blink::WebArrayBuffer buffer;
-  if (!EncryptInternal(algorithm, key, data, data_size, &buffer)) {
-    result.completeWithError();
-  } else {
+  Status status = EncryptInternal(algorithm, key, data, data_size, &buffer);
+  if (!completeWithError(status, &result)) {
     result.completeWithBuffer(buffer);
   }
 }
@@ -190,9 +201,8 @@ void WebCryptoImpl::decrypt(
     blink::WebCryptoResult result) {
   DCHECK(!algorithm.isNull());
   blink::WebArrayBuffer buffer;
-  if (!DecryptInternal(algorithm, key, data, data_size, &buffer)) {
-    result.completeWithError();
-  } else {
+  Status status = DecryptInternal(algorithm, key, data, data_size, &buffer);
+  if (!completeWithError(status, &result)) {
     result.completeWithBuffer(buffer);
   }
 }
@@ -204,9 +214,8 @@ void WebCryptoImpl::digest(
     blink::WebCryptoResult result) {
   DCHECK(!algorithm.isNull());
   blink::WebArrayBuffer buffer;
-  if (!DigestInternal(algorithm, data, data_size, &buffer)) {
-    result.completeWithError();
-  } else {
+  Status status = DigestInternal(algorithm, data, data_size, &buffer);
+  if (!completeWithError(status, &result)) {
     result.completeWithBuffer(buffer);
   }
 }
@@ -220,10 +229,9 @@ void WebCryptoImpl::generateKey(
   if (IsAlgorithmAsymmetric(algorithm)) {
     blink::WebCryptoKey public_key = blink::WebCryptoKey::createNull();
     blink::WebCryptoKey private_key = blink::WebCryptoKey::createNull();
-    if (!GenerateKeyPairInternal(
-             algorithm, extractable, usage_mask, &public_key, &private_key)) {
-      result.completeWithError();
-    } else {
+    Status status = GenerateKeyPairInternal(
+        algorithm, extractable, usage_mask, &public_key, &private_key);
+    if (!completeWithError(status, &result)) {
       DCHECK(public_key.handle());
       DCHECK(private_key.handle());
       DCHECK_EQ(algorithm.id(), public_key.algorithm().id());
@@ -236,9 +244,9 @@ void WebCryptoImpl::generateKey(
     }
   } else {
     blink::WebCryptoKey key = blink::WebCryptoKey::createNull();
-    if (!GenerateKeyInternal(algorithm, extractable, usage_mask, &key)) {
-      result.completeWithError();
-    } else {
+    Status status = GenerateKeyInternal(
+        algorithm, extractable, usage_mask, &key);
+    if (!completeWithError(status, &result)) {
       DCHECK(key.handle());
       DCHECK_EQ(algorithm.id(), key.algorithm().id());
       DCHECK_EQ(extractable, key.extractable());
@@ -258,24 +266,24 @@ void WebCryptoImpl::importKey(
     blink::WebCryptoResult result) {
   blink::WebCryptoKey key = blink::WebCryptoKey::createNull();
   if (format == blink::WebCryptoKeyFormatJwk) {
-    if (!ImportKeyJwk(key_data,
-                      key_data_size,
-                      algorithm_or_null,
-                      extractable,
-                      usage_mask,
-                      &key)) {
-      result.completeWithError();
+    Status status = ImportKeyJwk(key_data,
+                                 key_data_size,
+                                 algorithm_or_null,
+                                 extractable,
+                                 usage_mask,
+                                 &key);
+    if (completeWithError(status, &result)) {
       return;
     }
   } else {
-    if (!ImportKeyInternal(format,
-                           key_data,
-                           key_data_size,
-                           algorithm_or_null,
-                           extractable,
-                           usage_mask,
-                           &key)) {
-      result.completeWithError();
+    Status status = ImportKeyInternal(format,
+                                      key_data,
+                                      key_data_size,
+                                      algorithm_or_null,
+                                      extractable,
+                                      usage_mask,
+                                      &key);
+    if (completeWithError(status, &result)) {
       return;
     }
   }
@@ -290,11 +298,10 @@ void WebCryptoImpl::exportKey(
     const blink::WebCryptoKey& key,
     blink::WebCryptoResult result) {
   blink::WebArrayBuffer buffer;
-  if (!ExportKeyInternal(format, key, &buffer)) {
-    result.completeWithError();
-    return;
+  Status status = ExportKeyInternal(format, key, &buffer);
+  if (!completeWithError(status, &result)) {
+    result.completeWithBuffer(buffer);
   }
-  result.completeWithBuffer(buffer);
 }
 
 void WebCryptoImpl::sign(
@@ -305,9 +312,8 @@ void WebCryptoImpl::sign(
     blink::WebCryptoResult result) {
   DCHECK(!algorithm.isNull());
   blink::WebArrayBuffer buffer;
-  if (!SignInternal(algorithm, key, data, data_size, &buffer)) {
-    result.completeWithError();
-  } else {
+  Status status = SignInternal(algorithm, key, data, data_size, &buffer);
+  if (!completeWithError(status, &result)) {
     result.completeWithBuffer(buffer);
   }
 }
@@ -322,20 +328,19 @@ void WebCryptoImpl::verifySignature(
     blink::WebCryptoResult result) {
   DCHECK(!algorithm.isNull());
   bool signature_match = false;
-  if (!VerifySignatureInternal(algorithm,
-                               key,
-                               signature,
-                               signature_size,
-                               data,
-                               data_size,
-                               &signature_match)) {
-    result.completeWithError();
-  } else {
+  Status status = VerifySignatureInternal(algorithm,
+                                          key,
+                                          signature,
+                                          signature_size,
+                                          data,
+                                          data_size,
+                                          &signature_match);
+  if (!completeWithError(status, &result)) {
     result.completeWithBoolean(signature_match);
   }
 }
 
-bool WebCryptoImpl::ImportKeyJwk(
+Status WebCryptoImpl::ImportKeyJwk(
     const unsigned char* key_data,
     unsigned key_data_size,
     const blink::WebCryptoAlgorithm& algorithm_or_null,
@@ -479,7 +484,7 @@ bool WebCryptoImpl::ImportKeyJwk(
   //
 
   if (!key_data_size)
-    return false;
+    return Status::ErrorEmptyKeyData();
   DCHECK(key);
 
   // Parse the incoming JWK JSON.
@@ -489,19 +494,19 @@ bool WebCryptoImpl::ImportKeyJwk(
   // Note, bare pointer dict_value is ok since it points into scoped value.
   base::DictionaryValue* dict_value = NULL;
   if (!value.get() || !value->GetAsDictionary(&dict_value) || !dict_value)
-    return false;
+    return Status::ErrorJwkNotDictionary();
 
   // JWK "kty". Exit early if this required JWK parameter is missing.
   std::string jwk_kty_value;
   if (!dict_value->GetString("kty", &jwk_kty_value))
-    return false;
+    return Status::ErrorJwkMissingKty();
 
   // JWK "extractable" (optional) --> extractable parameter
   {
     bool jwk_extractable_value;
     if (dict_value->GetBoolean("extractable", &jwk_extractable_value)) {
       if (!jwk_extractable_value && extractable)
-        return false;
+        return Status::ErrorJwkExtractableInconsistent();
       extractable = extractable && jwk_extractable_value;
     }
   }
@@ -528,7 +533,7 @@ bool WebCryptoImpl::ImportKeyJwk(
         jwk_alg_factory.Get().CreateAlgorithmFromName(jwk_alg_value);
     if (jwk_algorithm.isNull()) {
       // JWK alg unrecognized
-      return false;  // case 1
+      return Status::ErrorJwkUnrecognizedAlgorithm();  // case 1
     }
     // JWK alg valid
     if (algorithm_or_null.isNull()) {
@@ -537,13 +542,13 @@ bool WebCryptoImpl::ImportKeyJwk(
     } else {
       // input algorithm specified
       if (!WebCryptoAlgorithmsConsistent(jwk_algorithm, algorithm_or_null))
-        return false;  // case 3
+        return Status::ErrorJwkAlgorithmInconsistent();  // case 3
       algorithm = algorithm_or_null;  // case 4
     }
   } else {
     // JWK alg missing
     if (algorithm_or_null.isNull())
-      return false;  // case 5
+      return Status::ErrorJwkAlgorithmMissing();  // case 5
     algorithm = algorithm_or_null;  // case 6
   }
   DCHECK(!algorithm.isNull());
@@ -562,11 +567,11 @@ bool WebCryptoImpl::ImportKeyJwk(
       jwk_usage_mask =
           blink::WebCryptoKeyUsageWrapKey | blink::WebCryptoKeyUsageUnwrapKey;
     } else {
-      return false;
+      return Status::ErrorJwkUnrecognizedUsage();
     }
     if ((jwk_usage_mask & usage_mask) != usage_mask) {
       // A usage_mask must be a subset of jwk_usage_mask.
-      return false;
+      return Status::ErrorJwkUsageInconsistent();
     }
   }
 
@@ -575,7 +580,7 @@ bool WebCryptoImpl::ImportKeyJwk(
 
     std::string jwk_k_value;
     if (!GetDecodedUrl64ValueByKey(*dict_value, "k", &jwk_k_value))
-      return false;
+      return Status::ErrorJwkDecodeK();
 
     // TODO(padolph): Some JWK alg ID's embed information about the key length
     // in the alg ID string. For example "A128" implies the JWK carries 128 bits
@@ -602,14 +607,14 @@ bool WebCryptoImpl::ImportKeyJwk(
     // entry is found.
     // TODO(padolph): Support RSA private key import.
     if (dict_value->HasKey("d"))
-      return false;
+      return Status::ErrorJwkRsaPrivateKeyUnsupported();
 
     std::string jwk_n_value;
     if (!GetDecodedUrl64ValueByKey(*dict_value, "n", &jwk_n_value))
-      return false;
+      return Status::ErrorJwkDecodeN();
     std::string jwk_e_value;
     if (!GetDecodedUrl64ValueByKey(*dict_value, "e", &jwk_e_value))
-      return false;
+      return Status::ErrorJwkDecodeE();
 
     return ImportRsaPublicKeyInternal(
         reinterpret_cast<const uint8*>(jwk_n_value.data()),
@@ -622,10 +627,10 @@ bool WebCryptoImpl::ImportKeyJwk(
         key);
 
   } else {
-    return false;
+    return Status::ErrorJwkUnrecognizedKty();
   }
 
-  return true;
+  return Status::Success();
 }
 
 }  // namespace content