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: initial commit for at_chops uptake #809

Merged
merged 38 commits into from
Jan 9, 2023
Merged

Conversation

murali-shris
Copy link
Member

@murali-shris murali-shris commented Nov 21, 2022

- What I did

  • uptake at_chops package in at_client
  • upgraded dependencies in pubspec for at_client
    - How I did it
  • created and injected an instance of atChops in persistKeys method of at_client_service.dart
  • added a feature flag useAtChops to at_client_preference.dart. Defaults to false. Dependent clients have to set this flag to true in order to use at_chops. This flag will be defaulted to true once all packages have completed uptake of at_chops
  • Modified implementation of packages/at_client/lib/src/decryption_service/shared_key_decryption.dart and packages/at_client/lib/src/encryption_service/abstract_atkey_encryption.dart to use current implementation and new atChops implementation based on feature flag
  • Deprecated decryptKey method in encryption_util.dart which uses private key
  • Modified unit tests to check results from current and new implementation
    - How to verify it
  • run the unit tests in decryption_service_test.dart

@murali-shris murali-shris marked this pull request as ready for review November 25, 2022 18:00
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.

Looking good! I've made a few comments; FYI I've also requested a change on the at_chops PR which will have impact here

packages/at_client/analysis_options.yaml Show resolved Hide resolved
packages/at_client/lib/src/client/at_client_impl.dart Outdated Show resolved Hide resolved
packages/at_client/lib/src/client/at_client_impl.dart Outdated Show resolved Hide resolved
@gkc
Copy link
Contributor

gkc commented Jan 2, 2023

@murali-shris When do you think it will be ready to merge this to trunk?

@murali-shris
Copy link
Member Author

@murali-shris When do you think it will be ready to merge this to trunk?

Final piece that has to solved is from where to call AtClientImpl._createAtChopsInstance() when atChops instance is not set for an at_client instance. Packages using at_client without at_client_mobile will break because _createAtChopsInstance() needs the encryption and pkam keys set in localSecondary.

Alternate approach is have useAtChops=false by default in at_client_preference and tackle one package at a time by setting useAtChops=true when we uptake at_chops for that particular package.

@murali-shris
Copy link
Member Author

removed at_client_mobile related changes in this PR and moved to a new PR
#863

@murali-shris
Copy link
Member Author

TODOs before merging this PR

@murali-shris
Copy link
Member Author

TODOs before merging this PR

done

@murali-shris murali-shris requested a review from gkc January 9, 2023 07:49
@@ -954,4 +973,27 @@ class AtClientImpl implements AtClient {
_preference = preference;
_namespace = namespace;
}
}

Future<AtChops> _createAtChopsInstance() async {
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't think we need this method any more?

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah.i will remove this.

gkc
gkc previously approved these changes Jan 9, 2023
@murali-shris murali-shris merged commit efa077f into trunk Jan 9, 2023
@murali-shris murali-shris deleted the uptake_atchops branch January 9, 2023 12:17
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.

None yet

3 participants