fix: save enrollment details to local keystore #599

murali-shris · 2024-06-18T07:00:28Z

Closes #556
Enroll details are needed in local storage for at_client to perform enrollment authorization checks for local secondary verbs. While we started implementing authorization checks in at_client, we wanted to use enroll:fetch instead of local key to get enrollment details. But enroll:fetch approach will not work for 1) offline at_client 2) every verb operation will result in remote call. So we decided to store a local key for enrollment details

- What I did

Store enrollment details on LocalSecondary after approval and successful auth using a new enrollment
Handle edge cases, when null SelfEncryptionKey or EncryptionPrivateKey is received from server
Changes to OnboardingService.close() :
- call AtLookup.close() only if AtLookup.isConnectionAvailable is true (to NOT trigger the null exception)
- Also close atClient.notificationService as this might keep a monitor connection running in the background in some cases
Changed OnboardingService._readAtKeysFile () to visibleForTesting
introduced a setter for enrollmentBase to allow injection of mocks (only visibleForTesting)
Formatted method docs
Some refactoring to support mocking

- How to verify it

unit tests and functional tests added to assert the changes

- Description for the changelog

fix: save enrollment details to local keystore

…o save_enrollment_details_new

…ss is null

…am_functional_tests # Conflicts: # tests/at_onboarding_cli_functional_tests/pubspec.yaml

srieteja · 2024-07-17T15:54:35Z

waiting for #606 to be merged into this branch.

test: add couple of functional tests related to APKAM

gkc · 2024-07-19T08:59:47Z

waiting for #606 to be merged into this branch.

#606 has been merged to trunk

gkc · 2024-07-19T09:01:01Z

waiting for #606 to be merged into this branch.

#606 has been merged to trunk

@srieteja I'll resolve the merge conflicts and merge trunk into this branch

…ils_new # Conflicts: # packages/at_onboarding_cli/lib/src/onboard/at_onboarding_service_impl.dart

srieteja · 2024-07-19T09:06:28Z

@gkc once the conflicts are resolved, functional tests will fail as a result of changes. Working on updating them

gkc · 2024-07-19T09:09:18Z

@gkc once the conflicts are resolved, functional tests will fail as a result of changes. Working on updating them

I've fixed all the ripple effects and pushed to the branch

gkc

The PR description explains what has been done, but not why. Please add an explanation of the problem that is being addressed here (or link to a ticket which has that explanation).
This PR handles writing to the keystore ... is the intent that there will be code elsewhere which reads this information from the keystore?
The information is currently written to '${enrollmentResponse.enrollmentId}.new.enrollments.__manage${atClient!.getCurrentAtSign()}'

Given that this is purely for local client-side use, we should use a local key
And we should not be using the __manage namespace

murali-shris · 2024-07-19T10:02:34Z

The PR description explains what has been done, but not why. Please add an explanation of the problem that is being addressed here (or link to a ticket which has that explanation).

This PR handles writing to the keystore ... is the intent that there will be code elsewhere which reads this information from the keystore?

The information is currently written to '${enrollmentResponse.enrollmentId}.new.enrollments.__manage${atClient!.getCurrentAtSign()}'

Given that this is purely for local client-side use, we should use a local key

And we should not be using the __manage namespace

added to the PR description
the local key which stores the enrollment details will be read from LocalSecondary --> isEnrollmentAuthorizedForOperation(..) in at_client
agreed. Will make the change

…sing onboarding_cli

…o save_enrollment_details_new

gkc · 2024-07-25T13:02:47Z

tests/at_onboarding_cli_functional_tests/test/enrollment_test.dart


      // The put operation is expected to fail as the new enrollment only has
      // access to wavi namespace
-      expect(() => client?.put(buzzKey, 'value'), throwsA(predicate((e) {
+      expect(() async => await client?.put(buzzKey, 'value'),


it's cleaner to do await expectLater(...)

gkc

Please un-skip the test(s) in test/at_onboarding_cli_test.dart

gkc

LGTM

fix: save enrollment details to local keystore

85cfe61

murali-shris marked this pull request as ready for review June 18, 2024 07:01

murali-shris marked this pull request as draft June 18, 2024 07:02

srieteja added 2 commits June 24, 2024 16:23

feat: improve atKeys filNaming + handle null encryption keys from server

b85774d

test: add unit test

d6dc6be

srieteja marked this pull request as ready for review June 24, 2024 11:10

srieteja added 5 commits June 25, 2024 16:45

test: skip problematic functional test

1fdf715

Merge remote-tracking branch 'origin/save_enrollment_details_new' int…

2a3d1b9

…o save_enrollment_details_new

test: use if-else instead of switch + other minor changes

745dcf0

test: fix failing functional test

c9e55c6

fix: at_auth_impl: only create new atChops if atChops instance in cla…

0cf5df5

…ss is null

srieteja requested a review from sitaram-kalluri June 26, 2024 07:20

sitaram-kalluri previously approved these changes Jul 1, 2024

View reviewed changes

srieteja added 15 commits July 5, 2024 14:59

feat: introduce option to not exit onboardingCLI when close() is called.

6cbc2e4

test: add functional test

85f21c0

test: more change to EnrollmentOperations

c80b7d1

feat: changes in onboarding_cli to improve testability

8c9ce28

test: make EnrollmentOperations stateless

90bcc76

feat: introduce option to not exit onboardingCLI when close() is called.

3a7ac2c

test: add functional test

9cdb6db

test: more change to EnrollmentOperations

fc8e5e5

feat: changes in onboarding_cli to improve testability

ff72043

test: make EnrollmentOperations stateless

08f5154

Merge remote-tracking branch 'origin/apkam_functional_tests' into apk…

bbb74a3

…am_functional_tests # Conflicts: # tests/at_onboarding_cli_functional_tests/pubspec.yaml

test: fix failing functional test

8b68baf

build[deps]: introduce at_commons into functional test pubspec

aaf1c0f

test: add functional test

9f6db8c

chore: minor refactoring

e5d85f7

srieteja requested a review from sitaram-kalluri July 17, 2024 15:53

gkc mentioned this pull request Jul 19, 2024

fix: add enrollment details to local keystore #559

Closed

Merge pull request #606 from atsign-foundation/apkam_functional_tests

c83999a

test: add couple of functional tests related to APKAM

gkc dismissed sitaram-kalluri’s stale review via c83999a July 19, 2024 08:59

Merge remote-tracking branch 'origin/trunk' into save_enrollment_deta…

e45c614

…ils_new # Conflicts: # packages/at_onboarding_cli/lib/src/onboard/at_onboarding_service_impl.dart

refactor: back out changes to signature of AtOnboardingService.close

a70b0c6

gkc requested changes Jul 19, 2024

View reviewed changes

srieteja and others added 2 commits July 19, 2024 17:16

test: unskip a test, other minor changes

8d5444e

fix: rename __manage key to local key for storing enrollment details

850f9b4

murali-shris mentioned this pull request Jul 23, 2024

fix: rename key from __manage to local key for storing enrollment details atsign-foundation/at_client_sdk#1360

Merged

srieteja and others added 9 commits July 24, 2024 15:51

feat: close atClient -notification service and sync service while clo…

d93781c

…sing onboarding_cli

test: fix failing functional test

0b9cf24

Merge remote-tracking branch 'origin/save_enrollment_details_new' int…

5efe459

…o save_enrollment_details_new

fix: Update to ports numbers to accomodate the new atsigns

531d028

test: separate tests and test utils

c95cef4

Merge remote-tracking branch 'origin/save_enrollment_details_new' int…

67cd660

…o save_enrollment_details_new

revert: changes reg atkeys file naming

79f1941

fix: improve at_onb_cli close logic + other minor changes

7ed2725

docs: update pubspec and changelog

4f4314d

srieteja requested a review from gkc July 25, 2024 12:42

gkc reviewed Jul 25, 2024

View reviewed changes

gkc requested changes Jul 25, 2024

View reviewed changes

test: use expectLater() and unskip a test

1ce8780

gkc approved these changes Jul 25, 2024

View reviewed changes

srieteja merged commit 922195b into trunk Jul 25, 2024
11 checks passed

srieteja deleted the save_enrollment_details_new branch July 25, 2024 16:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: save enrollment details to local keystore #599

fix: save enrollment details to local keystore #599

murali-shris commented Jun 18, 2024 •

edited by srieteja

Loading

srieteja commented Jul 17, 2024

gkc commented Jul 19, 2024 •

edited

Loading

gkc commented Jul 19, 2024

srieteja commented Jul 19, 2024

gkc commented Jul 19, 2024

gkc left a comment

murali-shris commented Jul 19, 2024 •

edited

Loading

gkc Jul 25, 2024

gkc left a comment

gkc left a comment

fix: save enrollment details to local keystore #599

fix: save enrollment details to local keystore #599

Conversation

murali-shris commented Jun 18, 2024 • edited by srieteja Loading

srieteja commented Jul 17, 2024

gkc commented Jul 19, 2024 • edited Loading

gkc commented Jul 19, 2024

srieteja commented Jul 19, 2024

gkc commented Jul 19, 2024

gkc left a comment

Choose a reason for hiding this comment

murali-shris commented Jul 19, 2024 • edited Loading

gkc Jul 25, 2024

Choose a reason for hiding this comment

gkc left a comment

Choose a reason for hiding this comment

gkc left a comment

Choose a reason for hiding this comment

murali-shris commented Jun 18, 2024 •

edited by srieteja

Loading

gkc commented Jul 19, 2024 •

edited

Loading

murali-shris commented Jul 19, 2024 •

edited

Loading