From 78995a315bd89ac93b5082939dccf22850f1019b Mon Sep 17 00:00:00 2001 From: Dave Longley Date: Tue, 27 Feb 2024 17:18:39 -0500 Subject: [PATCH] Ensure expired (but in db) doc registration record is updated. --- CHANGELOG.md | 7 ++++ lib/documents.js | 49 ++++++++++++++++++----- lib/tokens/index.js | 3 +- test/mocha/20-tokens.js | 88 +++++++++++++++++++++++++++++++++++++++++ 4 files changed, 136 insertions(+), 11 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 39dcbad..4061d9c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,12 @@ # bedrock-tokenization ChangeLog +## 21.1.1 - 2024-mm-dd + +### Fixed +- Ensure an expired (but still present in the database) document + registration record will be updated when a registration attempt using + a matching document is attempted. + ## 21.1.0 - 2023-10-12 ### Added diff --git a/lib/documents.js b/lib/documents.js index 5fcc02e..ed04911 100644 --- a/lib/documents.js +++ b/lib/documents.js @@ -338,6 +338,11 @@ export async function register({ // at this point the registration is known to be new/not exist, so // throw an error if it was expected to exist + // FIXME: current implementation could trigger a 404 when the update + // modifies no document because of an exactly simultaneous concurrent + // update; this should be handled gracefully externally but could perhaps + // be improved here by checking for `record` above instead of `updated` + // (if that logic is appropriate); this might also simplify logic below if(newRegistration === false) { const details = { httpStatusCode: 404, @@ -348,18 +353,37 @@ export async function register({ 'NotFoundError', details); } - /* Note: At this point, either a document associated with the external ID - and document hash doesn't exist or the same expiration date *happened* to - be set by another concurrent process in the above `refresh` call, resulting - in this process not seeing an update to the existing record there. + /* Note: At this point, we expect that we need to insert a document + registration record because either: + + 1. A document registration record associated with the external ID and + document hash doesn't exist, + + 2. It does exist in the database but has been marked as expired (and the + external caller may or may not have set `newRegistration=true` on this + basis), + + 3. The same expiration date *happened* to be set by another concurrent + process in the above `refresh` call, resulting in this process not seeing + any records being modified in the above update call. We assume the more common case that there is no such document with the given `externalIdHash` and `documentHash` and go ahead and generate an - `internalId` for inserting the document and encrypt it. If incorrect, - an entity with the `internalId` will be inserted (and can be safely ignored - and allowed to expire), but the document registration record will not be - inserted due to a duplicate error (which also means that the encrypted data - won't be used and will be safely discarded). */ + `internalId` for the document and encrypt it. We try to upsert an entity + record with that `internalId` and then try to insert the document + registration record. + + Now, if our assumption is incorrect (and therefore some matching document + registration record exists), the entity record with the given `internalId` + that was inserted can be safely ignored and allowed to expire. The document + registration record will not be inserted due to a duplicate error (which + also means that the encrypted data won't be used and will be safely + discarded). + + If this duplicate error occurs, we need to clear any assumption that a + new registration is being made, clear any `internalId` generated, and loop + to ensure to ensure that the document registration record (if expired) is + updated with a new expiration date. */ // 4. Generate a new random internal ID to associate with `externalId` if // none has been set yet. @@ -402,7 +426,12 @@ export async function register({ throw e; } // document already exists with matching `documentHash` and - // `externalIdHash`; loop to refresh it + // `externalIdHash`; loop to refresh it, but ensure it isn't treated + // as a new registration (this handles the case of an expired + // registration record that wasn't detected by an external caller of + // this API) + newRegistration = undefined; + internalId = undefined; continue; } } diff --git a/lib/tokens/index.js b/lib/tokens/index.js index 107a1e9..c84d45d 100644 --- a/lib/tokens/index.js +++ b/lib/tokens/index.js @@ -151,7 +151,8 @@ export async function registerDocumentAndCreate({ }) ]); - // if `internalId` does not match, then we must try again + // if `internalId` does not match, then we must try again to ensure the + // tokens match the registration record if(registrationResult.registration.internalId.compare(internalId) !== 0) { continue; } diff --git a/test/mocha/20-tokens.js b/test/mocha/20-tokens.js index 6c464c1..3b4e17d 100644 --- a/test/mocha/20-tokens.js +++ b/test/mocha/20-tokens.js @@ -868,6 +868,94 @@ describe('Tokens', function() { tokenResult.should.include.keys(['tokens', 'validUntil']); tokenResult.validUntil.should.be.a('Date'); }); + it('should register expired duplicate and create token concurrently', + async function() { + const dateOfBirth = '2000-05-01'; + const expires = '2021-05-01'; + const identifier = 'T99991234'; + const issuer = 'VA'; + const type = 'DriversLicense'; + const recipients = [ + { + header: { + kid: 'did:key:z6MkpTHR8VNsBxYAAWHut2Geadd9jSwuBV8xRoA' + + 'nwWsdvktH#z6LSbysY2xFMRpGMhb7tFTLMpeuPRaqaWM1yECx2AtzE3KCc', + alg: 'ECDH-ES+A256KW', + } + } + ]; + const tokenCount = 1; + // canonicalize object then hash it then base58 encode it + const externalId = encode(crypto.createHash('sha256') + .update(canonicalize({dateOfBirth, identifier, issuer})) + .digest()); + + let registrationRecord; + { + let tokenResult; + let err; + try { + tokenResult = await tokens.registerDocumentAndCreate({ + registerOptions: { + externalId, + document: {dateOfBirth, expires, identifier, issuer, type}, + recipients, + ttl: 1209600000 + }, + tokenCount + }); + } catch(e) { + err = e; + } + assertNoError(err); + should.exist(tokenResult); + tokenResult.should.include.keys(['tokens', 'validUntil']); + tokenResult.validUntil.should.be.a('Date'); + ({registrationRecord} = tokenResult); + } + + // now mark registration record expired and register again w/ success + const {registration} = registrationRecord; + const collection = database.collections['tokenization-registration']; + const yesterday = new Date(); + yesterday.setDate(yesterday.getDate() - 1); + const updateResult = await collection.updateOne({ + 'registration.externalIdHash': registration.externalIdHash, + 'registration.documentHash': registration.documentHash + }, { + $set: {'registration.expires': yesterday} + }); + updateResult.result.nModified.should.equal(1); + + // now re-registration should update expired registration record + { + let tokenResult; + let err; + try { + tokenResult = await tokens.registerDocumentAndCreate({ + registerOptions: { + externalId, + document: {dateOfBirth, expires, identifier, issuer, type}, + recipients, + ttl: 1209600000 + }, + tokenCount + }); + } catch(e) { + err = e; + } + assertNoError(err); + should.exist(tokenResult); + tokenResult.should.include.keys(['tokens', 'validUntil']); + tokenResult.validUntil.should.be.a('Date'); + tokenResult.registrationRecord.registration.externalIdHash.should + .deep.equal(registrationRecord.registration.externalIdHash); + tokenResult.registrationRecord.registration.documentHash.should + .deep.equal(registrationRecord.registration.documentHash); + tokenResult.registrationRecord.registration.expires.should.not.equal( + yesterday); + } + }); it('should not resolve token from invalidated batch', async function() { // create tokens