fix: Commit log special key bug

[murali-shris]

Fixes #2114

- What I did

• fixed a bug in _specialKey method in commit log. Due to this bug, lastCommitSequenceNumber was incorrectly returned when regex is passed.
  - How I did it
• modified a condition to check public encryption key from "public:" to "public:publickey"
• match only reserved shared key (@bob:shared_key@alice, shared_key.bob@alice) in _specialKey
• removed pkam signature and signing private key from _specialKey check
• added unit tests for getEntries and lastSequenceNumber
• fixed an issue in commit log test by replacing commitLog.getBox().add(doesn't update commitLog cache) with commitLog.add
• renamed method names to reflect their purpose
  - How to verify it
• tests should pass
  fix: test persistence at_client_sdk#1414

Dependent at_commons PR:
atsign-foundation/at_libraries#694

[gkc]

Thanks @murali-shris

1. Noticed another bug while I was reviewing that we should also fix via this PR
2. All of this code could really do with some explanation of what's going on and why - especially what is the purpose of _acceptKey?

[gkc]

this contains AtConstants.atEncryptionSharedKey check is also a bug. Why are we treating keys that contain "shared_key" as special?

And if we are treating them as special for good reason, we should specifically only match shared_key\..*@this_atservers_at_sign or @some_other_atsign:shared_key@this_servers_at_sign (precise regexes are in at_commons in Regexes._reservedKeysWithAtsignSuffix) ... currently this code would also match keys like

"@bob:key1.shared_keys.buzz@alice" 

[gkc]

@murali-shris I don't see a good explanation for why we are not treating the two reserved shared_key variants as 'special' which (as I understand it) would result in those keys not being synced?

[gkc]

Oh - ignore that last comment. I understand it now. This ensures that those keys are synced, regardless of whether they match the enrollment namespace or not.

[murali-shris]

Thanks @murali-shris

1. Noticed another bug while I was reviewing that we should also fix via this PR
2. All of this code could really do with some explanation of what's going on and why - especially what is the purpose of _acceptKey?

These are my findings on _acceptKey.

1. Initially the method name was _isRegexMatches which will match the key with regex and if there is match will be included in the result of getChanges(commitId, regex) method. _isRegexMatches method also had special keys which will not match the regex(shared key, pkam signature, signing private key, public encryption key)
2. minor refactoring was done to move the special keys check to _isSpecialKey. _isRegexMatches was changed to _acceptKey. _acceptKey invoked _isRegexMatches and _isSpecialKey
3. After enrollment feature, new check was added to match key name space with enrolled namespace.
   So _accept key does this functionality - matches regex or checks if a key is special key and does namespace check(if enrolledNamespace is passed).
   I will try removing the signing private key and pkam signature from the implementation and test. These keys should remain on the server and need not be synced.

[gkc]

@murali-shris thanks for adding comments explaining the purpose of this code which is to ensure that we are syncing none of the stuff that should not be synced (i.e. anything in namespaces the client does not have access to) and all of the stuff that should be synced (i.e. anything in namespaces the client does have access to, plus other things that all clients should have access to)

As I understand it now, therefore, shouldn't we also, in _isSpecialKey, match everything which (a) is public (b) has no namespace? This would explain why the code had a startsWith 'public:' match, although that was incorrect since it should be 'starts with public:' and 'does not contain a period'

[murali-shris]

@murali-shris thanks for adding comments explaining the purpose of this code which is to ensure that we are syncing none of the stuff that should not be synced (i.e. anything in namespaces the client does not have access to) and all of the stuff that should be synced (i.e. anything in namespaces the client does have access to, plus other things that all clients should have access to)

As I understand it now, therefore, shouldn't we also, in _isSpecialKey, match everything which (a) is public (b) has no namespace? This would explain why the code had a startsWith 'public:' match, although that was incorrect since it should be 'starts with public:' and 'does not contain a period'

Scenario 2) and 4) are the ones you meant. correct ?

1. _acceptKey(public:phone@alice, .*) 
_isNamespaceAuthorised -  true (no namespace passed)
_isRegexMatches - true
_isSpecialKey - false
_acceptKey - true


2. _acceptKey(public:phone@alice, ‘.buzz’) 
_isNamespaceAuthorised -  true (no namespace passed)
_isRegexMatches - false
_isSpecialKey - false // must be true
_acceptKey - false // must be true


3. _acceptKey(public:phone@alice, .*, [buzz, rw]) 

_isNamespaceAuthorised -  true (key doesn’t have namespace)
_isRegexMatches - true
_isSpecialKey - false
_acceptKey - true


4. _acceptKey(public:phone@alice, ‘.buzz’, [buzz, rw]) 
_isNamespaceAuthorised - true (key doesn’t have namespace)
_isRegexMatches - false
_isSpecialKey - false // must be true
_acceptKey - false //must be true 

There are currently no tests to check whether public: keys without namespace are returned. I will add them

[gkc]

@murali-shris yes that's correct. Please can you also come up with better names for these functions - i.e. names which are more reflective of their purpose? Future maintainers will thank you!