Fix for permanent authentication failure if initial call to loadAuthenticationKey fails

[foxxyz]

When using authentication, it seems like a condition can occur that is impossible to recover from. It can be reproduced as follows:

1. Comment out the procedure to read with authentication in examples/index.js (lines 70-86, except for line 78)
2. Run the example script with an ACR122u reader: npm run example
3. Quickly tap a card (for example, Mifare 1K), pulling away before it has a chance to fully authenticate (< ~300ms)
4. Every subsequent read should now fail with error when authenticating data

When inspecting the code, this seems originate from Line 423 in Reader.js where the Promise of load AuthenticationKey is being stored in this.pendingLoadAuthenticationKey - presumably to be able to wait for this Promise to resolve at a later time.

However, if this Promise is rejected (for example, a failed auth operation in loadAuthenticationKey), the rejected promise will now be stored in this.pendingLoadAuthenticationKey. Later calls to authenticate will now continue to await on this rejected Promise and never complete.

This pull request removes the tracking of Promises in this.pendingLoadAuthenticationKey so subsequent calls to authenticate will now run the whole authentication key load procedure if the key is not present, no matter the result of the initial call. I believe await should ensure it will always run to completion anyway (except in case of an exception, of course).

I want to make sure I'm not regressing any existing use-cases, so let me know if I'm missing your original intent when using pendingLoadAuthenticationKey or overlooking any situations dictating its need.

[foxxyz]

@pokusew Any changes you'd like to see for this PR to be accepted? Let me know!

[pokusew]

Hi @foxxyz,

Sorry for my late reply. I've been a bit busy recently. Thank you very much for your PR. 🙂 I really appreciate your contribution. 👍

I looked into it and I think you are right 👍 (although I don't have any Mifare Classic card to test it). The issue can really occur in the situation you described.

But I don't think that removing the tracking of Promises in this.pendingLoadAuthenticationKey is the best solution. Let me explain its purpose.

Suppose no keys are loaded yet (initial state) and you have this code, where you want to authenticate two blocks (4 and 5):

const key = 'FFFFFFFFFFFF';
const keyType = KEY_TYPE_A;

await Promise.all([
    reader.authenticate(4, keyType, key), 
    reader.authenticate(5, keyType, key),
]);

The following actions will occur in this order:

1. reader.authenticate for block 4 is called
   Because the key is not loaded and it is not being written now, this.loadAuthenticationKey is called and the returned Promise is saved to this.pendingLoadAuthenticationKey[key]. While the function is waiting for the Promise, code execution continues.

2. reader.authenticate for block 5 is called
   The key is not loaded yet, but it is being written now. We will wait for the Promise stored in this.pendingLoadAuthenticationKey[key].

When there was no pendingLoadAuthenticationKey, this.loadAuthenticationKey would be called again. It would result into another (unnecessary) communication with the driver respectively with the reader. The key would be saved into the reader's store (from the first call) but it would be immediately overwritten due the second call. It would work, but it could be slower due to unnecessary communication.

Of course, it will only appear when there are more than one consecutive reader.authenticate calls with the same key. To be honest, the impact of this behaviour can be minimal or none at all, but I've never tested it (it's not so easy). I rather implemented this feature, to be sure it will work in all cases and on all readers.

But I obviously forgot to deal with the situation you described. I think I even forgot to clear the resolved Promise from this.pendingLoadAuthenticationKey. 😄 Thank you very much for pointing me out to it. 👍

I think that the best solution would be to clear this.pendingLoadAuthenticationKey[key] after it is resolved/rejected.

On line 428 (after try/catch block), the following code can be added to remove the resolved/rejected Promise from the this.pendingLoadAuthenticationKey object (it won't be called until the Promise is either resolved or rejected):

// remove the loadAuthenticationKey Promise from pendingLoadAuthenticationKey
// as it is already resolved or rejected
delete this.pendingLoadAuthenticationKey[key];

That should resolve all the issues.

Is everything clear? What do you think of that? I'll be grateful for any comments on it. Could you please test if this solution works?

I'm looking forward to your reply.

---

PS Don't forget to star ⭐️ my library, if you find it useful. 😃 Thanks.

[foxxyz]

Hello @pokusew!

Sorry for my very late reply to your very detailed explanation. I think I understand the use-case you put forth, and I've added an additional commit that will hopefully address and mitigate these concerns.

In summary, the pendingLoadAuthenticationKey object has been added back in and once again tracks the currently executing authentication process for a given key. If this process is not already pending, a new one is added, otherwise the current process is awaited on.

After the authentication completes, the pending process is cleared using a finally block to ensure its execution even if an exception is thrown in the catch block.

I've tested correct behavior via the example, using both single and multiple authentication blocks, but I would invite you to check it as well.

Let me know if this addresses the outstanding issues, and if not, what I can improve!

[pokusew]

Hi @foxxyz,

Thank you very much for your changes. I really appreciate it.

Unfortunately, I don't have any Mifare Classic cards, so I can't test it now (I have currently just Mifare Ultralight and Mifare DESFire – Yeah, I have to broaden my cards collection 😄).

I reviewed your code and everything looks fine – except the one possible situation that may occur.
Can you please take a look at it? (see my review comments)

Otherwise, I think that it solves the outstanding issues. 👍

Looking forward to your reply.

[pokusew]

Suppose the following situation:

1. key is not in the storage (condition !keyNumber is met)
2. there is already an authentication process for this key (so the condition !this.pendingLoadAuthenticationKey[key] is not met)
3. then try-catch-finally block is executed
4. the keyNumber remains null – that's the problem
   You should assign return value of await this.pendingLoadAuthenticationKey[key] to keyNumber (like in my code here on line 402).
5. when the command is constructed, null results in 0x00 (which is the same as for 0), so it will work for one key, but the error could appear when using two keys – that's probably the reason why you didn't notice when you were testing it

Suggested fix: replace line 421 with the following code:

keyNumber = await this.pendingLoadAuthenticationKey[key];

P.S.: Yeah, in some cases, we know the keyNumber, but because of simplified conditions (much cleaner code), we need to reassign the keyNumber in order to handle the situation described above.

[pokusew]

Using finally is great solution! 👍 Thank you very much. I didn't realize that my previously suggested fix won't work.

[foxxyz]

Thank you @pokusew for the thorough review! The fix you suggested is indeed something I overlooked. Glad you found it!

I have made the change, let me know if you see any other obstacles for merging!

[foxxyz]

@pokusew Is the change sufficient for the PR to be accepted? Please let me know!

[foxxyz]

Thank you for merging and for your great library! Keep up the good work! 👍

[pokusew]

Hi @foxxyz,

You are welcome! I greatly appreciate your contribution. 👍 It really makes a difference!
Thank you very, very much. 🙂

Hope you enjoy using this library! Feel free to create another PR or an issue or to contact me if you encounter any problems or have a feature idea.

P.S.: Check out the contributors field in package.json. 😉

---

BTW Sorry for my very late reply. I was a bit busy over the last few months. 🙂

I reviewed your last commit and accepted ✅ the changes. Moreover, I got a Mifare Classic card, so I was able to test the fix and it seems to work! 🙂

I merged the PR in f7c2ebf and released it in version 0.6.2. 🚀