-
Notifications
You must be signed in to change notification settings - Fork 12
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: add enrollment details to public hidden key #2020
base: trunk
Are you sure you want to change the base?
Conversation
Have added @cconstab and @XavierChanth as reviewers as I'd like this change to have more eyes on it before we merge it |
String newEnrollmentId, | ||
EnrollDataStoreValue enrollmentDataStoreValue, | ||
String currentAtSign) async { | ||
var key = 'public:$enrollmentPublicHiddenKey'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wait does this actually work as intended? A public key with _
is also hidden? I was under the impression that this was only for self-keys, but it makes sense that it works for all key types.
If so, the changes I proposed in atsign-foundation/at_protocol#166 are not quite right.
No action items for this PR intended out of this comment, but rather clarification for the changes I am making to the protocol spec.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As discussed let's park this while we revisit the problem we're trying to solve and come up with the desired user experience first. As it stands, this would leak information which a user might not at all want
@@ -847,6 +867,14 @@ void main() { | |||
response, approveEnrollVerbParams, inboundConnection); | |||
expect(jsonDecode(response.data!)['enrollmentId'], enrollmentId); | |||
expect(jsonDecode(response.data!)['status'], 'approved'); | |||
// verify enrollmentId is present in hidden public key before enroll:revoke |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be made into its own unit test rather than added to an existing one
@@ -856,6 +884,12 @@ void main() { | |||
response, enrollVerbParams, inboundConnection); | |||
expect(jsonDecode(response.data!)['enrollmentId'], enrollmentId); | |||
expect(jsonDecode(response.data!)['status'], 'revoked'); | |||
// verify enrollmentId is removed from hidden public key after enroll:revoke | |||
enrollmentAtData = await secondaryKeyStore |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be made into its own unit test rather than added to an existing one
@@ -744,6 +755,15 @@ void main() { | |||
enrollmentData = await secondaryKeyStore.get(enrollmentKey); | |||
expect(enrollmentData!.metaData!.expiresAt, null); | |||
expect(enrollmentData.metaData!.ttl, 0); | |||
var enrollmentAtData = await secondaryKeyStore | |||
.get('public:$enrollmentPublicHiddenKey$alice'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be made into its own unit test rather than added to an existing one
- What I did
- How I did it
- How to verify it