Reland "Use crypo RNG for auto ID generation to reduce conflicts (#2764)" #3012

wu-hui · 2020-05-04T21:10:21Z

Manually verified on IE11 and web-workers.

…)"

google-oss-bot · 2020-05-04T21:18:34Z

Binary Size Report

Affected SDKs

@firebase/firestore

Type Base (df44d12) Head (66cc9ee) Diff

browser 247 kB 248 kB +655 B (+0.3%)

esm2017 193 kB 194 kB +352 B (+0.2%)

main 488 kB 489 kB +987 B (+0.2%)

module 245 kB 246 kB +633 B (+0.3%)
@firebase/firestore/memory

Type Base (df44d12) Head (66cc9ee) Diff

browser 188 kB 189 kB +216 B (+0.1%)

esm2017 148 kB 148 kB +210 B (+0.1%)

main 365 kB 365 kB +496 B (+0.1%)

module 187 kB 187 kB +216 B (+0.1%)

`firebase`

Type	Base (`df44d12`)	Head (`66cc9ee`)	Diff
firebase-firestore.js	286 kB	287 kB	+632 B (+0.2%)
firebase-firestore.memory.js	229 kB	229 kB	+217 B (+0.1%)
firebase.js	820 kB	820 kB	+634 B (+0.1%)

Test Logs

Base (df44d12e): https://github.com/firebase/firebase-js-sdk/actions/runs/95441389
Head (66cc9eee): https://github.com/firebase/firebase-js-sdk/actions/runs/98347194

schmidt-sebastian · 2020-05-04T21:58:07Z

packages/firestore/src/platform_browser/browser_platform.ts

+
+    // Polyfill for IE and WebWorker
+    // eslint-disable-next-line no-restricted-globals
+    const crypto =


We might also consider adding support for this shim on ReactNative: https://stackoverflow.com/questions/45301900/howto-patch-shim-crypto-getrandomvalues-for-react-native

While the fallback will make sure these environments don't break, we can possibly extend our support for crypto.

Edit: I missed that you added self && self.crypto. I blame it on the inline Linter warning which you can suppress by moving your no-any line two down :)

So we don't need it, right?

I am ok with not adding support for ReactNative until the ReactNative Firebase wrapper bundles "crypto", but note that without getting "crypto" from globals, we will always fall back to Math.random() for RN.

Yep, I am filing a ticket to get crypto added for react native firestore.

react-native-firebase participant here, just cross-linking invertase/react-native-firebase#3618

Just thought I'd quickly summerize; React Native Firebase doesn't use the JS SDK, we're a thin JS wrapper around the native Android & iOS SDKs so this change has no direct affect on us.

However our JS wrapper does use an identical random id generator - so I think we may need to port this change across in some form - we'll pick this up separately, thanks!

dconeybe

A couple of drive-by comments.

dconeybe · 2020-05-05T04:43:56Z

packages/firestore/src/platform/platform.ts

@@ -43,6 +43,12 @@ export interface Platform {
  /** Converts a binary string to a Base64 encoded string. */
  btoa(raw: string): string;

+  /**
+   * Generates `nBytes` of random bytes. If `nBytes` is negative, an empty array
+   * will be returned.


What is the rationale for accepting negative numbers? This seems like it may be hiding bugs in the calling code.

Changed it to validate input, and reject anything <=0.

dconeybe · 2020-05-05T04:45:35Z

packages/firestore/src/platform_browser/browser_platform.ts

+        (this.window.crypto || (this.window as any)['msCrypto'])) ||
+      (self && self.crypto);
+    const v = new Uint8Array(nBytes);
+    if (!!crypto) {


Is this double exclamation mark intentional? If not, you may want to add a unit test that would fail if Math.random() is used unintentionally.

This can probably just be if(crypto) since an if statement just needs a "truthy" value and not a boolean.

schmidt-sebastian · 2020-05-05T05:08:44Z

packages/firestore/src/platform_browser/browser_platform.ts

+    const crypto =
+      (this.window &&
+        (this.window.crypto || (this.window as any)['msCrypto'])) ||
+      (self && self.crypto);


According to https://developer.mozilla.org/en-US/docs/Web/API/Window/self, you can just write:

const crypto = typeof self !=== 'undefined' && (self.crypto || (self as any)['msCrypto']);

schmidt-sebastian · 2020-05-05T05:11:06Z

packages/firestore/src/platform_browser/browser_platform.ts

+        (this.window.crypto || (this.window as any)['msCrypto'])) ||
+      (self && self.crypto);
+    const v = new Uint8Array(nBytes);
+    if (!!crypto) {


This can probably just be if(crypto) since an if statement just needs a "truthy" value and not a boolean.

schmidt-sebastian · 2020-05-05T05:16:33Z

packages/firestore/src/util/misc.ts

-      autoId += chars.charAt(Math.floor(Math.random() * chars.length));
+    while (autoId.length < 20) {
+      const bytes = PlatformSupport.getPlatform().randomBytes(40);
+      for (const b of Array.from(bytes)) {


Nit: You can save one copy here if you do a for(let i = 0; i < bytes.length; ++i) loop.

schmidt-sebastian · 2020-05-05T19:16:12Z

packages/firestore/src/platform_browser/browser_platform.ts

@@ -72,4 +74,22 @@ export class BrowserPlatform implements Platform {
  btoa(raw: string): string {
    return btoa(raw);
  }
+
+  randomBytes(nBytes: number): Uint8Array {
+    validatePositiveNumber('randomBytes', 1, nBytes);


This should be a debugAssert since we are not validating a public API call.

You should also allow calls with nBytes: 0 (unless you have to write special code for this).

schmidt-sebastian · 2020-05-05T19:17:34Z

packages/firestore/src/platform_browser/browser_platform.ts

+
+    // Polyfill for IE and WebWorker
+    // eslint-disable-next-line no-restricted-globals
+    const crypto =


I am ok with not adding support for ReactNative until the ReactNative Firebase wrapper bundles "crypto", but note that without getting "crypto" from globals, we will always fall back to Math.random() for RN.

schmidt-sebastian · 2020-05-05T20:26:07Z

packages/firestore/src/platform_node/node_platform.ts

@@ -75,4 +77,10 @@ export class NodePlatform implements Platform {
  btoa(raw: string): string {
    return new Buffer(raw, 'binary').toString('base64');
  }
+
+  randomBytes(nBytes: number): Uint8Array {
+    validatePositiveNumber('randomBytes', 1, nBytes);


DebugAssert here as well.

Also, make sure that 0 is accepted as a valid value for nBytes.

dconeybe · 2020-05-06T14:32:52Z

packages/firestore/src/platform/platform.ts

+  /**
+   * Generates `nBytes` of random bytes.
+   *
+   * If `nBytes <= 0` , an error will be thrown.


nit: nBytes==0 is valid; please update this comment to state that it will only reject values that are strictly less than zero.

dconeybe · 2020-05-06T14:36:19Z

packages/firestore/src/platform_browser/browser_platform.ts

@@ -72,4 +74,23 @@ export class BrowserPlatform implements Platform {
  btoa(raw: string): string {
    return btoa(raw);
  }
+
+  randomBytes(nBytes: number): Uint8Array {
+    debugAssert(nBytes >= 0, 'Number of random bytes must not be negative');


Consider including the invalid value in the failure message; this could assist in debugging should the assert ever get triggered and all we have to use for debugging is the stack track and error message.

e.g.

debugAssert(nBytes >= 0, `invalid nBytes: ${nBytes}`);

dconeybe · 2020-05-06T14:37:51Z

packages/firestore/src/platform_browser/browser_platform.ts

+  randomBytes(nBytes: number): Uint8Array {
+    debugAssert(nBytes >= 0, 'Number of random bytes must not be negative');
+
+    // Polyfill for IE and WebWorker


Can you improve this comment to explain how it affects the reader? Is the polyfill being done by the code block that follows? Or is it instructing the reader to perform some polyfilling? Consider re-writing this comment as a sentence that would be either clearly informative or clearly actionable by the reader.

dconeybe · 2020-05-06T14:45:33Z

packages/firestore/src/platform_browser/browser_platform.ts

+    const crypto =
+      // eslint-disable-next-line @typescript-eslint/no-explicit-any
+      typeof self !== 'undefined' && (self.crypto || (self as any)['msCrypto']);
+    const v = new Uint8Array(nBytes);


nit: Consider a more meaningful name than v (e.g. generatedBytes).

dconeybe · 2020-05-06T14:50:43Z

packages/firestore/src/util/misc.ts

+      for (let i = 0; i < bytes.length; ++i) {
+        // Length of `chars` is 62. We only take bytes between 0 and 62*4-1
+        // (both inclusive). The value is then evenly mapped to indices of `char`
+        // via a modulo operation.


I'm assuming this logic is used to ensure an even distribution between all of the characters in chars. Please update the comment to explain why this logic exists because it is non-obvious.

dconeybe · 2020-05-06T14:53:31Z

packages/firestore/src/util/misc.ts

@@ -28,8 +29,17 @@ export class AutoId {
    const chars =
      'ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789';
    let autoId = '';
-    for (let i = 0; i < 20; i++) {
-      autoId += chars.charAt(Math.floor(Math.random() * chars.length));
+    while (autoId.length < 20) {


Consider re-writing this loop as while (true) and then breaking out of the loop with a break statement immediately after appending the 20th character to autoId. This would avoid hardcoding 20 in two different places.

Rewrote the section to reduce hardcoding numbers.

dconeybe · 2020-05-06T14:56:14Z

packages/firestore/src/util/misc.ts

+        // Length of `chars` is 62. We only take bytes between 0 and 62*4-1
+        // (both inclusive). The value is then evenly mapped to indices of `char`
+        // via a modulo operation.
+        const maxValue = 62 * 4 - 1;


Consider moving the calculation of maxValue to immediately follow the declaration of chars. IMO, having it re-calculated on each loop iteration detracts from readability because it incorrectly implies that its value may change between iterations.

dconeybe · 2020-05-06T14:58:06Z

packages/firestore/src/util/misc.ts

+        // Length of `chars` is 62. We only take bytes between 0 and 62*4-1
+        // (both inclusive). The value is then evenly mapped to indices of `char`
+        // via a modulo operation.
+        const maxValue = 62 * 4 - 1;


Hardcoding 62 in three different places (including in the comment) is a bit unnerving. It would be preferable to use chars.length, for robustness. How about something like this:

const chars = '...'; const maxValue = chars.length * 4 - 1; debugAssert(maxValue < 256);

Then the hardcoded value of 62 in the modulo operation below can be safely replaced with chars.length.

dconeybe · 2020-05-06T14:59:21Z

packages/firestore/src/platform_node/node_platform.ts

@@ -75,4 +77,10 @@ export class NodePlatform implements Platform {
  btoa(raw: string): string {
    return new Buffer(raw, 'binary').toString('base64');
  }
+
+  randomBytes(nBytes: number): Uint8Array {
+    validatePositiveNumber('randomBytes', 1, nBytes);


Also, make sure that 0 is accepted as a valid value for nBytes.

schmidt-sebastian

LGTM++

dconeybe

Looks great! Thanks for refactoring the logic in newId(); it is now far more readable IMO.

wu-hui added 3 commits May 4, 2020 11:29

Reland "Use crypo RNG for auto ID generation to reduce conflicts (#2872…

867417c

…)"

Reland "Use crypo RNG for auto ID generation to reduce conflicts (#2872…

a2b89c2

…)"

Fix merge error

02592ec

wu-hui changed the title ~~Reland SRNG~~ Reland "Use crypo RNG for auto ID generation to reduce conflicts (#2764)" May 4, 2020

wu-hui requested a review from schmidt-sebastian May 4, 2020 21:14

wu-hui assigned schmidt-sebastian May 4, 2020

wu-hui added the api: firestore label May 4, 2020

schmidt-sebastian reviewed May 4, 2020

View reviewed changes

schmidt-sebastian assigned wu-hui and unassigned schmidt-sebastian May 4, 2020

dconeybe reviewed May 5, 2020

View reviewed changes

schmidt-sebastian reviewed May 5, 2020

View reviewed changes

Address comments

17b6bc5

wu-hui assigned schmidt-sebastian and dconeybe and unassigned wu-hui May 5, 2020

schmidt-sebastian approved these changes May 5, 2020

View reviewed changes

schmidt-sebastian assigned wu-hui and unassigned schmidt-sebastian May 5, 2020

Suppress lint

ea4967a

schmidt-sebastian reviewed May 5, 2020

View reviewed changes

dconeybe requested changes May 6, 2020

View reviewed changes

dconeybe removed their assignment May 6, 2020

wu-hui mentioned this pull request May 6, 2020

Make window.crypto available in reactive native for Firestore invertase/react-native-firebase#3618

Closed

Address comments

434e650

schmidt-sebastian approved these changes May 7, 2020

View reviewed changes

wu-hui assigned dconeybe and unassigned wu-hui May 7, 2020

dconeybe approved these changes May 7, 2020

View reviewed changes

dconeybe assigned wu-hui and unassigned dconeybe and wu-hui May 7, 2020

wu-hui merged commit 631a0ec into master May 7, 2020

firebase locked and limited conversation to collaborators Jun 7, 2020

Reland "Use crypo RNG for auto ID generation to reduce conflicts (#2764)" #3012

Reland "Use crypo RNG for auto ID generation to reduce conflicts (#2764)" #3012

Conversation

wu-hui commented May 4, 2020 • edited Loading

google-oss-bot commented May 4, 2020 • edited Loading

Binary Size Report

Affected SDKs

@firebase/firestore

@firebase/firestore/memory

firebase

Test Logs

schmidt-sebastian May 4, 2020 • edited Loading

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

dconeybe left a 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

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

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

schmidt-sebastian left a comment

Choose a reason for hiding this comment

dconeybe left a comment

Choose a reason for hiding this comment

wu-hui commented May 4, 2020 •

edited

Loading

google-oss-bot commented May 4, 2020 •

edited

Loading

`@firebase/firestore`

`@firebase/firestore/memory`

`firebase`

schmidt-sebastian May 4, 2020 •

edited

Loading