at_server icon indicating copy to clipboard operation
at_server copied to clipboard

Review usage of encoding of keys in secondary store and toLowerCase in server

Open murali-shris opened this issue 2 years ago • 6 comments

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

murali-shris avatar Oct 11 '22 10:10 murali-shris

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

murali-shris avatar Oct 14 '22 13:10 murali-shris

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]

murali-shris avatar Oct 18 '22 15:10 murali-shris

outcome from arch call. Maintain the lower case behaviour in server. convert the entire key part to lower case while accepting request from server

murali-shris avatar Nov 07 '22 07:11 murali-shris

@srieteja See also https://github.com/atsign-foundation/at_client_sdk/issues/743

gkc avatar Nov 13 '22 14:11 gkc

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.

srieteja avatar Nov 14 '22 08:11 srieteja

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?

gkc avatar Nov 14 '22 13:11 gkc

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.

srieteja avatar Nov 28 '22 15:11 srieteja

Carrying this issue and https://github.com/atsign-foundation/at_client_sdk/issues/743 to PR51 for smoothening out the implementation and testing.

srieteja avatar Nov 28 '22 15:11 srieteja

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.

srieteja avatar Dec 12 '22 11:12 srieteja