Make the function signature overloads of `SubtleCrypto#exportKey` more flexible #1593

steveluscher · 2023-07-12T18:28:55Z

Problem

This test case fails with the current state of the exportKey overloads (playground).

(['jwk', 'raw', 'pkcs8', 'spki'] as KeyFormat[]).forEach(format => {
    crypto.subtle.exportKey(
      format, // ERROR
      {} as CryptoKey,
    );
});

Change

This PR makes it so that the "jwk" overload works as expected by producing Promise<JsonWebKey> as the return type, and the remaining keys in the KeyFormat union produce Promise<ArrayBuffer> as the return type, and the example code above typechecks correctly (playground).

…e flexible # Problem This test case _fails_ with the current state of the `exportKey` overloads ([playground](https://www.typescriptlang.org/play?#code/NocgVg7g1iA0AEIBOBDCdEAcoGMDOAHBiHtgJYgC6AdAGYD2SAoijgBYAUDSAtigC7wAvAD54AbwBQ8GfBxIAnpn71qeAK4AjfgBsAptT0APTI34BpPQq6M+-BOIC+8FHngBhRcvqWFASgBuaVkAehDZCMiAPQB+SUdAoA)). ```ts ['jwk', 'raw', 'pkcs8', 'spki'].forEach(format => { crypto.subtle.exportKey( format, // ERROR {} as CryptoKey, ); }); ``` # Change This PR makes it so that the `"jwk"` overload works as expected by producing `Promise<JsonWebKey>` as the return type, and the remaining keys in the `KeyFormat` union produce `Promise<ArrayBuffer>` as the return type, _and_ the example code above typechecks correctly.

github-actions · 2023-07-12T18:29:13Z

Thanks for the PR!

This section of the codebase is owned by @saschanaz - if they write a comment saying "LGTM" then it will be merged.

steveluscher · 2023-07-12T18:35:08Z

@microsoft-github-policy-service agree

steveluscher · 2023-07-12T18:45:45Z

Mmm… hold on. I see why this doesn't work for importKey quite the same way. I'll revert that.

saschanaz · 2023-07-12T19:03:46Z

I'm pretty sure this is done to fix the jwk incorrectly returning ArrayBuffer. Can you write some unit tests and check the return type?

steveluscher · 2023-07-12T19:12:17Z

Check out the second playground link; "jwk" still returns Promise<JsonWebKey>!

saschanaz · 2023-07-12T19:18:58Z

I mean: playground

saschanaz · 2023-07-12T19:20:02Z

unittests/files/keyusage.ts

+function assertType<T>(_x: T) {}
+
+const mockKey = {} as CryptoKey;
+assertType<Promise<JsonWebKey>>(crypto.subtle.exportKey('jwk', mockKey));


Please also check what happens if the input is 'jwk' | 'pkcs8'.

100%. I'm on it.

saschanaz · 2023-07-12T19:20:43Z

I think we are blocked by microsoft/TypeScript#14107 which is still open.

steveluscher · 2023-07-12T19:28:04Z

There we go! Updated the playground link in the description.

saschanaz · 2023-07-12T19:32:40Z

Ah, that works better 👍

saschanaz · 2023-07-12T19:40:25Z

unittests/files/keyusage.ts

+function assertType<T>(_x: T) {}
+
+const mockKey = {} as CryptoKey;
+assertType<Promise<ArrayBuffer | JsonWebKey>>(crypto.subtle.exportKey('' as KeyFormat, mockKey));


This is too loose as either Promise<ArrayBuffer> or Promise<JsonWebKey> will match the type. But not sure how to make it strict enough 🤔

Maybe we can test whether it works for both as Promise<ArrayBuffer> and as Promise<JsonWebKey>, like:

crypto.subtle.exportKey('' as KeyFormat, mockKey) as Promise<ArrayBuffer>; // type includes ArrayBuffer crypto.subtle.exportKey('' as KeyFormat, mockKey) as Promise<JsonWebKey>; // type also includes JsonWebKey

This is too loose as either Promise or Promise will match the type

Isn't that exactly the point when format is not known to yield one or the other?

How about this?

assertType<Promise<ArrayBuffer | JsonWebKey>>( crypto.subtle .exportKey("" as KeyFormat, mockKey) .then((ambiguousExportedKeyData) => ambiguousExportedKeyData instanceof ArrayBuffer ? (ambiguousExportedKeyData satisfies ArrayBuffer) : (ambiguousExportedKeyData satisfies JsonWebKey) ) );

Isn't that exactly the point when format is not known to yield one or the other?

Yes but that'll never fail when the return type regresses to either ArrayBuffer or JsonWebKey.

How about this?

Sounds good!

…not known

saschanaz · 2023-07-13T17:20:06Z

This is technically a breaking change but I think it's reasonable.

LGTM

github-actions · 2023-07-13T17:20:21Z

Merging because @saschanaz is a code-owner of all the changes - thanks!

Update test case

129c933

steveluscher added 2 commits July 12, 2023 11:37

Update all test cases

8955c16

Make the same change to importKey

7376efb

steveluscher changed the title ~~Make the function signature overloads of SubtleCrypto#exportKey more flexible~~ Make the function signature overloads of SubtleCrypto#exportKey and SubtleCrypto#importKey more flexible Jul 12, 2023

Revert changes to importKey

7bde1d2

steveluscher changed the title ~~Make the function signature overloads of SubtleCrypto#exportKey and SubtleCrypto#importKey more flexible~~ Make the function signature overloads of SubtleCrypto#exportKeymore flexible Jul 12, 2023

steveluscher changed the title ~~Make the function signature overloads of SubtleCrypto#exportKeymore flexible~~ Make the function signature overloads of SubtleCrypto#exportKey more flexible Jul 12, 2023

Unit tests for exportKey

e9b315d

saschanaz reviewed Jul 12, 2023

View reviewed changes

Make overrides unambiguous with respect to format

4db3d7a

Oops

23fcba7

saschanaz reviewed Jul 12, 2023

View reviewed changes

steveluscher added 3 commits July 13, 2023 16:23

Unambiguously assert the return type of exportKey when format is …

de55f55

…not known

Words

0922075

Put the assertType function back

da41b81

github-actions bot merged commit 13115c8 into microsoft:main Jul 13, 2023

steveluscher deleted the patch-1 branch July 13, 2023 20:49

Make the function signature overloads of SubtleCrypto#exportKey more flexible #1593

Make the function signature overloads of SubtleCrypto#exportKey more flexible #1593

Uh oh!

Conversation

steveluscher commented Jul 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem

Change

Uh oh!

github-actions bot commented Jul 12, 2023

Uh oh!

steveluscher commented Jul 12, 2023

Uh oh!

steveluscher commented Jul 12, 2023

Uh oh!

saschanaz commented Jul 12, 2023

Uh oh!

steveluscher commented Jul 12, 2023

Uh oh!

saschanaz commented Jul 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

saschanaz Jul 12, 2023

Choose a reason for hiding this comment

Uh oh!

steveluscher Jul 12, 2023

Choose a reason for hiding this comment

Uh oh!

saschanaz commented Jul 12, 2023

Uh oh!

steveluscher commented Jul 12, 2023

Uh oh!

saschanaz commented Jul 12, 2023

Uh oh!

saschanaz Jul 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

saschanaz Jul 12, 2023

Choose a reason for hiding this comment

Uh oh!

steveluscher Jul 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

saschanaz Jul 13, 2023

Choose a reason for hiding this comment

Uh oh!

saschanaz commented Jul 13, 2023

Uh oh!

github-actions bot commented Jul 13, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Make the function signature overloads of `SubtleCrypto#exportKey` more flexible #1593

Make the function signature overloads of `SubtleCrypto#exportKey` more flexible #1593

steveluscher commented Jul 12, 2023 •

edited

Loading

saschanaz commented Jul 12, 2023 •

edited

Loading

saschanaz Jul 12, 2023 •

edited

Loading

steveluscher Jul 13, 2023 •

edited

Loading