Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Replace EncryptionUtil decryption methods with at_chops #1185

Merged
merged 10 commits into from
Jan 3, 2024

Conversation

murali-shris
Copy link
Member

@murali-shris murali-shris commented Dec 19, 2023

- What I did

  • Replaced EncryptionUtil.decrypt methods in LocalKeyDecryption, SelfKeyDecryption and SharedKeyDecryption with atChops
    - How I did it
  • local_key_decryption.dart , self_key_decryption.dart, shared_key_decryption
    • constructed AESEncryptionAlgo object from symmetric key, IV and pass it to atChops.decrypt
    • Handle and rethrow AtDecryptionException
  • shared_key_decryption.dart
    • refactored code to remove unwanted conditions since there is no relation between getting encryptionSharedKey and encryptionPublic key
  • added unit tests
    - How to verify it
  • unit and functional tests should pass

@murali-shris murali-shris marked this pull request as ready for review December 19, 2023 15:03
throw AtPublicKeyChangeException(
'Public key has changed. Cannot decrypt shared key ${atKey.toString()}',
intent: Intent.fetchEncryptionPublicKey,
exceptionScenario: ExceptionScenario.encryptionFailed);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be ExceptionScenario.decryptionFailed?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes Gary. changed

@@ -15,16 +15,16 @@ import 'package:at_chops/at_chops.dart';
abstract class AbstractAtKeyEncryption implements AtKeyEncryption {
late final AtSignLogger _logger;
late String _sharedKey;
final AtClient _atClient;
final AtClient atClient;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why has the visibility been increased from _atClient to atClient?

Copy link
Member Author

@murali-shris murali-shris Jan 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in LocalKeyDecryption i had call to super.atClient.
Modified the code in LocalKeyDecryption to remove call to super.atClient in latest commit

@@ -7,6 +7,8 @@ import 'package:encrypt/encrypt.dart';
import 'package:crypto/crypto.dart';
import 'package:at_utils/at_logger.dart';

//#TODO Replace calls to methods in this class with at_chops methods and
// move this class to test folder in next major release
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it safe to mark this class and all of the methods as @Deprecated now?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are 4 sub tasks in #1145
this PR is for first sub task. I am currently working on second sub task.
Once all sub tasks are done, we can mark the methods deprecated

Copy link
Contributor

@gkc gkc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See individual comments

@murali-shris murali-shris requested a review from gkc January 3, 2024 08:20
@murali-shris murali-shris merged commit e43bc1a into trunk Jan 3, 2024
8 checks passed
@murali-shris murali-shris deleted the replace_encryption_util_methods branch January 3, 2024 16:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants