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

fix: Update the error message and add skip commit for enrollment key deletion #2117

Merged
merged 7 commits into from
Oct 10, 2024

Conversation

sitaram-kalluri
Copy link
Member

@sitaram-kalluri sitaram-kalluri commented Oct 9, 2024

- What I did

  • In enroll_verb_handler.dart, modify the error message to specify that enrollment keys can only be deleted for enrollments that are denied or revoked.
  • Add "remove" method to the enrollment_manager.dart class which will delete the enrollment key and set "skipCommit" to true to prevent the entry getting logged into the commit log and to prevent enrollment data getting sync'ed between the client and server, therefore skip it.
  • Since client does not need to the apkamPublicKey to be sync'ed, set skipCommit to true when storing them into the keystore and update the tests accordingly.
  • Use the "remove" method from enrollment_manager.dart in enroll_verb_handler.dart to delete the enrollment key.
  • Update the unit test accordingly to match the error message.

- How to verify it

  • Added the following unit tests to verify the commit log state during various enrollment operations:
    • A test to verify commit log state during create approve revoke and delete an enrollment request
    • A test to verify commit log state during create deny and delete an enrollment request

@gkc
Copy link
Contributor

gkc commented Oct 9, 2024

  1. Is there a commit skip when enrollments are added?

  2. In the PR description you say enrollment info "is synced", but I think you mean "is not synced"

@sitaram-kalluri
Copy link
Member Author

sitaram-kalluri commented Oct 9, 2024

  1. Is there a commit skip when enrollments are added?

  2. In the PR description you say enrollment info "is synced", but I think you mean "is not synced"

@gkc : Yes, when we add the enrollments we have a skipCommit set to true. Attaching the code snippet below from the "EnrollmentManager.dart" . As for point # 2, Correct, I meant that there should be no syncing between the client and server

Future<void> put(String enrollmentId, AtData atData) async {
    String enrollmentKey = buildEnrollmentKey(enrollmentId);
    await _keyStore.put(enrollmentKey, atData, skipCommit: true);
  }

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.

Thanks @sitaram-kalluri

Please add unit test(s) which check commit log state after the various enrollment operations

@sitaram-kalluri
Copy link
Member Author

Thanks @sitaram-kalluri

Please add unit test(s) which check commit log state after the various enrollment operations

Sure, @gkc

@sitaram-kalluri sitaram-kalluri requested a review from gkc October 9, 2024 11:42
packages/at_secondary_server/test/enroll_verb_test.dart Outdated Show resolved Hide resolved
packages/at_secondary_server/test/enroll_verb_test.dart Outdated Show resolved Hide resolved
// public:appName.deviceName.pkam.__pkams.__public_keys@atSign key. Therefore,
// commit log has an entry.
expect(
itr.current.key.contains('pkam.__pkams.__public_keys$alice'), true);
Copy link
Contributor

Choose a reason for hiding this comment

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

please do a more precise check including the appName and deviceName being used

packages/at_secondary_server/test/enroll_verb_test.dart Outdated Show resolved Hide resolved
packages/at_secondary_server/test/enroll_verb_test.dart Outdated Show resolved Hide resolved
}
// Ensure there are no other keys in the commit log.
expect(itr.moveNext(), false);
expect(itr!.moveNext(), false);
Copy link
Contributor

Choose a reason for hiding this comment

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

The EnrollVerbHandler is not requesting to skip the commit when storing this key. So in theory this test is required - but I believe the commit log is not currently being updated because of this #2115

So - we should probably explicitly change the EnrollVerbHandler so that it skips the commit log when storing this public key also, since these are not things that need to be synced to clients and, indeed, currently are not synced - but would start syncing if #2115 were merged

Copy link
Contributor

Choose a reason for hiding this comment

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

Specifically add skipCommit: true to the calls to keyStore.put which are being made on lines 257 and 362 in enroll_verb_handler.dart

Copy link
Member Author

Choose a reason for hiding this comment

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

  • Set skipCommit to true and modified unit and functional tests accordingly.
  • Also did a round of manual testing with at_onboarding_cli to approve the enrollment request and use the new apkam keys authenticate the connection.

@sitaram-kalluri sitaram-kalluri requested a review from gkc October 10, 2024 06:22
@@ -121,7 +121,7 @@ void main() {
expect(
int.parse(
jsonDecode(statsResponse.replaceAll('data:', ''))[0]['value']),
lastCommitIdBeforeUpdate + 4);
lastCommitIdBeforeUpdate + 3);
Copy link
Member Author

Choose a reason for hiding this comment

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

Since akamPublicKeys are not logged into the commit log, reduced value to 3.

@gkc gkc merged commit dc628d0 into trunk Oct 10, 2024
26 checks passed
@gkc gkc deleted the update_enrollment_error_messages branch October 10, 2024 07:38
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