digitalbazaar / bedrock-account Goto Github PK

This kind of insert could avoid using transactions (and therefore be quicker) but presently does not. The insert would need to be looped -- and if a duplicate error were detected during such an insert, the duplicate record would have to be checked for pending transactions (which would have to be processed followed by looping).

Some transaction while loops retry indefinitely as concurrent issues / transaction aborts should always eventually win / fail in some competing process. We should ensure this is true or add some hard limits and throw exceptions if they are exhausted.

The error will never get thrown here because the conditional should be based on validationResult.valid.

• Also update findOne projection calls

This comes from stress testing, but the easiest way to completely destroy a site is to find an endpoint with unlimited results. Right now get uses findOne and most of the other methods in this project have a limit on how many results are returned except getAll. This leads to the following scenario:

let's say a Red Cross Nurse is assigned to 1,000 patients which in our system would
identify nurse by email then create 1,000 accounts with the same email. If someone were to request that nurse's account we would return 1,000 accounts in one query. This is a non-malicious use case. A malicious use case could use jmeter to create over 1 million accounts with the same email resulting in easy DDOS attacks on bedrock-account-http. Additionally speaking even in non malicious cases getAll allows for queries that could return all meta active, created before a unix time stamp etc. basically getAll has the ability to return a lot of data in one query.

so I think we need to implement cursor based pagination such as what this guy recommends:

https://www.codementor.io/arpitbhayani/fast-and-efficient-pagination-in-mongodb-9095flbqr

the query to getAll would use

db.collection.find().limit(10) if no cursor is provided and $gte if a cursor is present.

let me know because most getAll use cases I think would benefit from pagination.

Remove the any unique indexes in the accounts collection that prohibit sharding and enforce the constraints via separation collections similar to what was done with bedrock-edv-storage.

Presently, the internal record collection helper function (_update) is called in record transaction processing code. Instead, we should create a cleaner API in the helper that distinguishes between calling update to modify the record's data / meta (and sequence) and just modifying its transaction tracking state.

A number of comments around calling _update can then be removed.

This might be intentional, but I might have found a bug:

  if(record.meta.sequence !== sequence) {
    return new BedrockError(
      'Could not update Account. Record sequence does not match.',
      'InvalidStateError', {
        httpStatusCode: 409,
        public: true,
        actual: sequence,
        expected: record.meta.sequence
      });
  }

  const errors = jsonpatch.validate(patch, record.account);
  if(errors) {
    throw new BedrockError(
      'The given JSON patch is invalid.', 'ValidationError', {
        httpStatusCode: 400,
        public: true,
        patch,
        errors
      });
  }

in the meta sequence we return a new BedRockError object
in the other case we throw.
the inconsistently means that when the passed in sequence does not match the
sequence from the database it returns an error object.

So which behavior do we want returning an object or throwing an error?

• TODO: deprecate auto-retrieving capabilities, require devs to call
  Commit: (a360337) Add TODO.
  File: lib/index.js:714
  Dave Longley commented 2 years ago

• FIXME: call update meta on brIdentity or update sequence here
  Commit: (ae1eebd) Ensure sequence is updated.
  File: lib/index.js:625
  Dave Longley commented 2 years ago

• TODO: deprecate use of id here?
  Commit: (b1f032e) Add TODO.
  File: lib/index.js:550
  Dave Longley commented 2 years ago

• FIXME remove options.fields from all libraries that call on bedrock-account
  Commit: (183d870) Allow fields to be undefined.
  File: lib/index.js:298
  Andrew L Jones commented 13 days ago

• TODO: move permission check to after query to allow users with
  Commit: (16e72ca) Add core API.
  File: lib/index.js:288
  Dave Longley commented 2 years ago

Remove manageIdentity API and bedrock-identity dependency, that module is deprecated.

It's challenging to robustly test the transaction system used in this module to enforce uniqueness constraints. Ideally, every possible state that transactions (including overlapping / concurrent transactions) could be in would be tested. A strategy needs to be found that could allow for this kind of testing of the system.

2021-10-28T01:34:54.780Z - info: [bedrock-account] attempting to insert an account workerPid=98968, workerId=724aec46c029610c, details={
  "account": {
    "id": "b34e5f2c-b879-11e9-9f4f-b7e5472c15d2",
    "email": "[email protected]",
    "controllerKeySeed": "ohne2Aitohshiquohmah"
  }
}

I don't believe account details like controllerKeySeed should be exposed in log files. There may be other instances of this happening. All logging in this library should be audited.

One email should have only one account.