Add tests for Import Key Exception #135

HamdaanAliQuatil · 2024-06-16T08:44:15Z

Created optional param for importKeyException.
Added new Test Data with the importKeyException param.
Test passes if:
1. Import Key Exception is not null in Test Data.
2. And curve for the key in Test Data is not the same as the curve specified during import.
Test for deriveBits and subsequent tests should not proceed if the importKeyException is found.

jonasfj · 2024-07-09T11:03:39Z

lib/src/testing/utils/testrunner.dart

+  // Parameters for import key exception (optional, if there is an exception)
+  final Map<String, dynamic>? importKeyException;


Let's make this simpler.

Suggested change

// Parameters for import key exception (optional, if there is an exception)

final Map<String, dynamic>? importKeyException;

// If not `null`, then importing private or public key MUST throw

// an exception that contains this string.

final String? importKeyException;

Instead of making a test case that contains different strings for pkcs8Exception and jwkException, we just make two test cases. One that tests the exception when importing a PKCS8 key and on that tests it when importing a JWK key.

Notice that when specifying a test case, you don't have to specify both privatePkcs8KeyData and privateJsonWebKeyData, it's enough to specify one of them. So if we want to test different error messages (from different formats), we really should just make two test cases.

That way, we also avoid having an absolute explosion of properties on the test case objects.

We also need to update _validateTestCase

Updated with one string importKeyException. Made necessary changes to _validateTestCase

jonasfj · 2024-07-09T11:05:14Z

lib/src/testing/utils/testrunner.dart

@@ -746,6 +765,23 @@ void _runTests<PrivateKey, PublicKey>(
      publicKey = pair.publicKey;
      privateKey = pair.privateKey;
    });
+  } else if (c.importKeyException != null) {
+    test('pkcs8 import exception', () async {
+      try {


formatting of code is weird here. Please use dart format you can enable it in vscode with the Dart extension. I think it's even on by default.

Tried dart format, but nothing seemed to change on this line. A few other parts were formatted though.

jonasfj · 2024-07-09T11:07:48Z

lib/src/testing/utils/testrunner.dart

+    } catch (e) {
+      check(e.toString().contains(c.jwkImportKeyException!));
+    }
+    });


If importKeyException is not null, then we can test that importing public and private keys fail, but all the other tests we can't do, right?

So maybe it's fine to just do a return; here? That way we don't have to add && c.pkcs8ImportKeyException == null to all the other test cases.

We should also when doing this, make sure that in _validateTestCase we validate that the test case doesn't have values that won't be used.

In particular we need to validate that:

if generateKeyParams is not null, then importKeyException must be null!

if importKeyException is not null, then

All the other properties that won't be used must be null.

This includes plaintext, signature, ciphertext, derivedBits, derivedLength and more.
(If the tests don't use the values, they MUST not be specified, otherwise the test case will be hard to read and understand)

Probably also includes signVerifyParams, etc. since again, we can't use them for anything, so they shouldn't be specified.

At-least one of:

publicRawKeyData, publicSpkiKeyData, publicJsonWebKeyData, privateRawKeyData, privatePkcs8KeyData, or privateJsonWebKeyData must be specified.

Note: that we probably don't need all of them to be specified.

Note: it's probably okay if there is no private key or no public key, just so long as there is at-least ONE of these properties provided. Normally, when importKeyException == null, this is ofcourse not allowed.

Added a return; in this section. Removed recurring && c.pkcs8ImportKeyException == null
Updated _validateTestCase with the above checks

jonasfj · 2024-07-24T12:06:33Z

lib/src/testing/utils/testrunner.dart

-  check((c.deriveParams != null) == (r._deriveBits != null));
+
+  if (c.deriveParams != null) {
+    check((c.deriveParams != null) == (r._deriveBits != null));
+
+    if (r._deriveBits != null) {
+      check(c.derivedLength != null);
+    }
+  }


Is this unrelated to this change, if so let's make it a different PR

jonasfj · 2024-07-24T12:17:14Z

lib/src/testing/utils/testrunner.dart

+    if (c.privatePkcs8KeyData != null) {
+      test('pkcs8 import exception', () async {
+        try {
+          await r._importPrivatePkcs8Key!(
+              c.privatePkcs8KeyData!, {'curve': 'p-521'});
+          check(false, 'Expected an exception for P-512 import');
+        } catch (e) {
+          check(e.toString().contains(c.importKeyException!));
+        }
+      });
+    }
+    if (c.privateJsonWebKeyData != null) {
+      test('jwk import exception', () async {
+        try {
+          await r._importPrivateJsonWebKey!(
+              c.privateJsonWebKeyData!, {'curve': 'p-521'});
+          check(false, 'Expected an exception for P-512 import');
+        } catch (e) {
+          check(e.toString().contains(c.importKeyException!));
+        }
+      });
+    }


We need a test case for each kind of key possible:

final List<int>? privateRawKeyData; final List<int>? privatePkcs8KeyData; final Map<String, dynamic>? privateJsonWebKeyData; final List<int>? publicRawKeyData; final List<int>? publicSpkiKeyData; final Map<String, dynamic>? publicJsonWebKeyData;

Also I'd suggest naming the test cases like:

if (c.publicRawKeyData != null) { test('importPublicRawKey() throws', () async { ... }); } if (c.publicJsonWebKeyData != null) { test('importPublicJsonWebKey() throws', () async { ... }); } ... // This test case is testing failure to import a key // further tests will not be conducted, since they don't // make sense after failing to import a key. return;

jonasfj · 2024-07-24T12:22:12Z

lib/src/testing/webcrypto/ecdh.dart

+    "privatePkcs8KeyData":
+        "MIGHAgEAMBMGByqGSM49AgEGCCqGSM49AwEHBG0wawIBAQQg3aTiZ7odKAODYk4BpZlzulBCB/BptmxjtvrzyXI71UyhRANCAATl0GVa8O1sXXf2NV5qGJ/9/Vq8PVWCZuezADa1F0Vr2TaB8BseZIW+rhmEmLC2FfCdxj9NmLp00SilRTm40Hxm",
+    "publicRawKeyData":
+        "BHiIXxrwhM92v4ueDrj3x1JJY4uS+II/IJPjqMvaKj/QfoOllnEkrnaOW1owBYRBMnP0pPouPkqbVfPACMUsfKs=",
+    "publicSpkiKeyData":
+        "MFkwEwYHKoZIzj0CAQYIKoZIzj0DAQcDQgAEeIhfGvCEz3a/i54OuPfHUklji5L4gj8gk+Ooy9oqP9B+g6WWcSSudo5bWjAFhEEyc/Sk+i4+SptV88AIxSx8qw==",


We should change the _validateTestCase further.

Right now a test case is require to have one of:

import key parameters,

if symmetric has private key (and not public key)

if not symmetric has both:

a private key, and,

a public key.

We should want that if importKeyException then it is not necessary to have both private and public key.

Because, in some cases you may want to check an exception that can only be produced when trying to import a private key (or trying to import a public key).

Example: trying to import a public key will never complain about d being missing from the JWT.

jonasfj · 2024-07-24T12:24:35Z

lib/src/testing/webcrypto/ecdh.dart

@@ -111,6 +111,39 @@ final _testData = [
    "importKeyParams": {"curve": "p-256"},
    "deriveParams": {}
  },
+  {
+    "name": "generated on boringssl/linux (pkcs8 import key exception) at 2020-01-22T23:24:34",


I think we can do a better "name" here.

For exceptions, we shouldn't really care where the test case was generated.
Because it's NOT valid, you probably did something to it to make it invalid! 🤣

So better make the test case name say: "import incorrect curve" or something like that.

jonasfj · 2024-07-24T12:25:45Z

lib/src/testing/webcrypto/ecdh.dart

+        "MFkwEwYHKoZIzj0CAQYIKoZIzj0DAQcDQgAEeIhfGvCEz3a/i54OuPfHUklji5L4gj8gk+Ooy9oqP9B+g6WWcSSudo5bWjAFhEEyc/Sk+i4+SptV88AIxSx8qw==",
+    "publicJsonWebKeyData": {
+      "kty": "EC",
+      "crv": "P-256",


This curve doesn't look wrong?

test: add tests for importKeyException

9a57b44

jonasfj reviewed Jul 9, 2024

View reviewed changes

test: add validation for importKeyException

440bcde

HamdaanAliQuatil requested a review from jonasfj July 15, 2024 01:26

jonasfj reviewed Jul 24, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add tests for Import Key Exception #135

Add tests for Import Key Exception #135

HamdaanAliQuatil commented Jun 16, 2024

jonasfj Jul 9, 2024

jonasfj Jul 9, 2024

HamdaanAliQuatil Jul 15, 2024

jonasfj Jul 9, 2024

HamdaanAliQuatil Jul 15, 2024

jonasfj Jul 9, 2024

jonasfj Jul 9, 2024

HamdaanAliQuatil Jul 15, 2024

jonasfj Jul 24, 2024

jonasfj Jul 24, 2024

jonasfj Jul 24, 2024

jonasfj Jul 24, 2024

jonasfj Jul 24, 2024

		// Parameters for import key exception (optional, if there is an exception)
		final Map<String, dynamic>? importKeyException;

-  // Parameters for import key exception (optional, if there is an exception)
-  final Map<String, dynamic>? importKeyException;
+  // If not `null`, then importing private or public key MUST throw
+  // an exception that contains this string.
+  final String? importKeyException;

Add tests for Import Key Exception #135

Are you sure you want to change the base?

Add tests for Import Key Exception #135

Conversation

HamdaanAliQuatil commented Jun 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment