crypto(randomInt): put back optional min argument

nodejs · olalonde · Aug 2, 2020 · Aug 6, 2020 · Aug 6, 2020 · Aug 6, 2020
commit b220b3cc98e71869e47f8dd4ec92c70487f0e3d0
diff --git a/doc/api/crypto.md b/doc/api/crypto.md
@@ -2800,19 +2800,22 @@ threadpool request. To minimize threadpool task length variation, partition
 large `randomFill` requests when doing so as part of fulfilling a client
 request.
 
-### `crypto.randomInt(max[, callback])`
+### `crypto.randomInt([min, ]max[, callback])`
 <!-- YAML
 added: REPLACEME
 -->
 
-* `max` {integer} End of random range `[0, max)` (`max` is exclusive).
-  `max` must be less than `2^48`.
+* `min` {integer} Start of random range (inclusive). **Default**: `0`.
+* `max` {integer} End of random range (exclusive).
 * `callback` {Function} `function(err, n) {}`.
 
-Return a random integer `n` such that `0 <= n < max`.  This
+Return a random integer `n` such that `min <= n < max`.  This
 implementation avoids [modulo
 bias](https://en.wikipedia.org/wiki/Fisher%E2%80%93Yates_shuffle#Modulo_bias).
 
+The range (`max - min`) must be less than `2^48`. `min` and `max` must
+be safe integers.
+
 If the `callback` function is not provided, the random integer is
 generated synchronously.
 
@@ -2831,10 +2834,9 @@ console.log(`Random number chosen from (0, 1, 2): ${n}`);
 ```
 
 ```js
-crypto.randomInt(6, (err, n) => {
-  if (err) throw err;
-  console.log(`The dice rolled: ${n + 1}`);
-});
+// With `min` argument
+const n = crypto.randomInt(1, 7);
+console.log(`The dice rolled: ${n}`);
 ```
 
 ### `crypto.scrypt(password, salt, keylen[, options], callback)`

diff --git a/lib/internal/crypto/random.js b/lib/internal/crypto/random.js
@@ -126,10 +126,10 @@ const RAND_MAX = 0xFFFF_FFFF_FFFF;
 
 // Generates an integer in [min, max) range where min is inclusive and max is
 // exclusive.
-function randomIntRange(min, max, cb) {
+function randomInt(min, max, cb) {
   // Detect optional min syntax
-  // randomIntRange(max)
-  // randomIntRange(max, cb)
+  // randomInt(max)
+  // randomInt(max, cb)
   const minNotSpecified = typeof max === 'undefined' ||
     typeof max === 'function';
 
@@ -196,14 +196,6 @@ function randomIntRange(min, max, cb) {
   }
 }
 
-// Introduce optional "min" argument eventually?
-function randomInt(max, cb) {
-  if (typeof cb !== 'undefined' && typeof cb !== 'function') {
-    throw new ERR_INVALID_CALLBACK(cb);
-  }
-  return randomIntRange(max, cb);
-}
-
 function handleError(ex, buf) {
   if (ex) throw ex;
   return buf;

diff --git a/test/parallel/test-crypto-random.js b/test/parallel/test-crypto-random.js
@@ -352,18 +352,99 @@ assert.throws(
   assert.ok(randomInts.includes(2));
   assert.ok(!randomInts.includes(3));
 }
+{
+  // Positive range
+  const randomInts = [];
+  for (let i = 0; i < 100; i++) {
+    crypto.randomInt(1, 3, common.mustCall((err, n) => {
+      assert.ifError(err);
+      assert.ok(n >= 1);
+      assert.ok(n < 3);
+      randomInts.push(n);
+      if (randomInts.length === 100) {
+        assert.ok(!randomInts.includes(0));
+        assert.ok(randomInts.includes(1));
+        assert.ok(randomInts.includes(2));
+        assert.ok(!randomInts.includes(3));
+      }
+    }));
+  }
+}
+{
+  // Negative range
+  const randomInts = [];
+  for (let i = 0; i < 100; i++) {
+    crypto.randomInt(-10, -8, common.mustCall((err, n) => {
+      assert.ifError(err);
+      assert.ok(n >= -10);
+      assert.ok(n < -8);
+      randomInts.push(n);
+      if (randomInts.length === 100) {
+        assert.ok(!randomInts.includes(-11));
+        assert.ok(randomInts.includes(-10));
+        assert.ok(randomInts.includes(-9));
+        assert.ok(!randomInts.includes(-8));
+      }
+    }));
+  }
+}
 {
 
   ['10', true, NaN, null, {}, []].forEach((i) => {
-    assert.throws(() => crypto.randomInt(i, common.mustNotCall()), {
+    const invalidMinError = {
+      code: 'ERR_INVALID_ARG_TYPE',
+      name: 'TypeError',
+      message: 'The "min" argument must be safe integer.' +
+               `${common.invalidArgTypeHelper(i)}`,
+    };
+    const invalidMaxError = {
       code: 'ERR_INVALID_ARG_TYPE',
       name: 'TypeError',
       message: 'The "max" argument must be safe integer.' +
                `${common.invalidArgTypeHelper(i)}`,
-    });
+    };
+
+    assert.throws(
+      () => crypto.randomInt(i, 100),
+      invalidMinError
+    );
+    assert.throws(
+      () => crypto.randomInt(i, 100, common.mustNotCall()),
+      invalidMinError
+    );
+    assert.throws(
+      () => crypto.randomInt(i),
+      invalidMaxError
+    );
+    assert.throws(
+      () => crypto.randomInt(i, common.mustNotCall()),
+      invalidMaxError
+    );
+    assert.throws(
+      () => crypto.randomInt(0, i, common.mustNotCall()),
+      invalidMaxError
+    );
+    assert.throws(
+      () => crypto.randomInt(0, i),
+      invalidMaxError
+    );
   });
 
   const maxInt = Number.MAX_SAFE_INTEGER;
+  const minInt = Number.MIN_SAFE_INTEGER;
+
+  crypto.randomInt(minInt, minInt + 5, common.mustCall());
+  crypto.randomInt(maxInt - 5, maxInt, common.mustCall());
+
+  assert.throws(
+    () => crypto.randomInt(minInt - 1, minInt + 5, common.mustNotCall()),
+    {
+      code: 'ERR_INVALID_ARG_TYPE',
+      name: 'TypeError',
+      message: 'The "min" argument must be safe integer.' +
+      `${common.invalidArgTypeHelper(minInt - 1)}`,
+    }
+  );
 
   assert.throws(
     () => crypto.randomInt(maxInt + 1, common.mustNotCall()),
@@ -376,6 +457,7 @@ assert.throws(
   );
 
   crypto.randomInt(0, common.mustCall());
+  crypto.randomInt(0, 0, common.mustCall());
   assert.throws(() => crypto.randomInt(-1, common.mustNotCall()), {
     code: 'ERR_OUT_OF_RANGE',
     name: 'RangeError',
@@ -384,6 +466,18 @@ assert.throws(
 
   const MAX_RANGE = 0xFFFF_FFFF_FFFF;
   crypto.randomInt(MAX_RANGE, common.mustCall());
+  crypto.randomInt(1, MAX_RANGE + 1, common.mustCall());
+  assert.throws(
+    () => crypto.randomInt(1, MAX_RANGE + 2, common.mustNotCall()),
+    {
+      code: 'ERR_OUT_OF_RANGE',
+      name: 'RangeError',
+      message: 'The value of "max - min" is out of range. ' +
+               `It must be <= ${MAX_RANGE}. ` +
+               'Received 281_474_976_710_656'
+    }
+  );
+
   assert.throws(() => crypto.randomInt(MAX_RANGE + 1, common.mustNotCall()), {
     code: 'ERR_OUT_OF_RANGE',
     name: 'RangeError',
@@ -392,14 +486,12 @@ assert.throws(
              'Received 281_474_976_710_656'
   });
 
-  [1, true, NaN, null, {}, []].forEach((i) => {
-    assert.throws(
-      () => crypto.randomInt(1, i),
-      {
-        code: 'ERR_INVALID_CALLBACK',
-        name: 'TypeError',
-        message: `Callback must be a function. Received ${inspect(i)}`
-      }
-    );
+  [true, NaN, null, {}, [], 10].forEach((i) => {
+    const cbError = {
+      code: 'ERR_INVALID_CALLBACK',
+      name: 'TypeError',
+      message: `Callback must be a function. Received ${inspect(i)}`
+    };
+    assert.throws(() => crypto.randomInt(0, 1, i), cbError);
   });
 }