at_server
at_server copied to clipboard
Review usage of encoding of keys in secondary store and toLowerCase in server
Describe the bug
a) hive_keystore.dart put/create and delete methods use Utf7 encoding. Encoding is inconsistent while adding entry to metadataCache. See https://github.com/atsign-foundation/at_client_sdk/issues/742#issuecomment-1274406790 Review the usage of at keys encoding through out persistence and server code base.
b) secondary_util.dart convert command converts command part of at protocol command to lowercase. If an atsign/command has mixed case, analyse the impact
Steps to reproduce
n/a
Expected behavior
- encoding and lower case conversion should be consistent and well documented
encoding of keys.. https://github.com/atsign-foundation/at_server/pull/966
- keys in hive store will be encoded(existing behavior)
- keys in metadata cache of hive keystore will be stored without encoding. (previous impl had utf7 encoding)
- in notification keystore we are encoding/decoding in getKeys and getExpiredKeys. we can remove this and do a testing
- in commit log we are passing encoded key to commit method..we are decoding before adding to commit log keystore. Document this behaviour
FINER|2022-10-18 15:04:48.417665|AtSecondaryServer|inside _executeVerbCallBack: Update:Public:Email@alice🛠 [email protected]
FINER|2022-10-18 15:04:48.417686|AtSecondaryServer|after conversion : update:public:Email@alice🛠 [email protected]
hive keystore: public:email@alice+2D3e4A-
@alice🛠@scan
public:email@alice🛠
@lookup:email@alice🛠
data:[email protected]
outcome from arch call. Maintain the lower case behaviour in server. convert the entire key part to lower case while accepting request from server
@srieteja See also https://github.com/atsign-foundation/at_client_sdk/issues/743
Expected behaviour enforced from the verb handler side of things as part of the attached PR. This is causing issues with the CRAM verb and PKAM verb as they should not be converted to lowercase. Carrying over to PR50 to discuss and address Cram/Pkam related issues and https://github.com/atsign-foundation/at_client_sdk/issues/743.
Thinking out loud here: Maybe we should narrow the scope here specifically to just ensuring that keys are always lower-cased rather than the entirety of a command. In addition to happening before persisting to keyStore, this can and should also be done in AtClient.put() and get() as well as the various types of notify. Maybe it's sufficient for at_server to continue to just do the lower-casing when writing to the keystore?
After considering the last comment and a couple of other approaches, we are currently introducing a toLowerCase in the AtKey class which will be triggered in the fromString() and toString() methods in AtKey. Additionally, this will also be triggered from methods in AtClient where the conversion to lowercase will be logged to address https://github.com/atsign-foundation/at_client_sdk/issues/743.
Carrying this issue and https://github.com/atsign-foundation/at_client_sdk/issues/743 to PR51 for smoothening out the implementation and testing.
Carrying this issue and https://github.com/atsign-foundation/at_client_sdk/issues/743 to PR52 as I could not work on this in PR51.