Fixing the pk identities claim command to augment DIs
#278
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Is there a reason why message ProviderMessage {
string provider_id = 1;
string message = 2;
} |
|
Not sure. Who wrote this?
On 29 October 2021 10:27:06 am AEDT, emmacasolin ***@***.***> wrote:
Is there a reason why `ProviderMessage` uses the parameter `message` rather than `identityId`? Because it seems like everywhere that uses it assumes it's an identity id:
```proto
message ProviderMessage {
string provider_id = 1;
string message = 2;
}
```
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
#278 (comment)
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.
|
Looks like it just hasn't been changed since it was first added months ago. I'll change it to |
|
Since await sigchain.addClaim(claimData);
const claimId = await sigchain.getLatestClaimId();
const claimEncoded = await sigchain.getClaim(claimId!);
const claimDecoded = claimsUtils.decodeClaim(claimEncoded);Although this assumes that the most recent claim on the sigchain is the one we just created (i.e. no other claims will be generated by other processes in between |
|
I was running into issues with importing the sigchain into rpc identities where I was getting an undefined error after trying to run a function from the sigchain domain. To get around this I've created a function called The augmentation process now seems to work in a simple test in |
|
@tegefaulkes is changing the proto schema in #279, so this may need to be updated on that. |
|
I'm having some issues with the identities bin tests where the await polykeyAgent.sigchain.clearDB();
await testProvider.delToken(testToken.identityId);
await polykeyAgent.nodes.clearDB();
await polykeyAgent.gestalts.clearDB();
await polykeyAgent.identities.delToken(testToken.providerId, testToken.identityId);However none of these have solved the problem. Not sure what else it could possibly be... |
|
When prototyping, don't use the tests. Create it from scratch using a
`test.ts`. Then you can port it over when everything works/makes sense.
…On 11/1/21 3:40 PM, emmacasolin wrote:
I'm having some issues with the identities bin tests where the
|identities claim| test is interfering with the |identities discover|
(by node) test later on, causing it to fail with |Error: 2 UNKNOWN:
Unknown Error|. I'm assuming it's from some sort of state that isn't
getting cleaned up, since skipping the claims test causes everything
else to pass, however I'm having trouble working out what it is. At
the moment I'm clearing the sigchain, claimed identities on the
provider, registered providers, the node graph, and the gestalt graph:
await polykeyAgent.sigchain.clearDB();
await testProvider.delToken(testToken.identityId);
await polykeyAgent.nodes.clearDB();
await polykeyAgent.gestalts.clearDB();
await polykeyAgent.identities.delToken(testToken.providerId, testToken.identityId);
However none of these have solved the problem. Not sure what else it
could possibly be...
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#278 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAE4OHLJN4XZCDCYGR4DXDDUJYK4LANCNFSM5G6AB65A>.
|
|
It's useful to know exactly what the error is. You'll want to find the RPC function throwing the error and add a |
CMCDragonkai
force-pushed
the
identities-augment
branch
from
af14c8a
to
0b76870
Compare
emmacasolin
force-pushed
the
identities-augment
branch
from
0b76870
to
50df7f3
Compare
|
Rebased |
CMCDragonkai
force-pushed
the
identities-augment
branch
from
00fd0cf
to
0b76870
Compare
My bad in only seeing this now. It should be fine to actually make |
|
From our sprint meeting yesterday, I believe there'll also be some testing fixes to be done after the CLI MR on Gitlab is merged and we rebase on those changes. |
|
The errors for |
|
We shouldn't be throwing generic errors anywhere, it should always be attached to the error hierarchy somewhere. So the 2 choices are client service errors or domain specific errors. If the identities domain is returning undefined, then the exception should in client service. We will later review the entire system of how to do this. There's general exceptions for something not existing in HTTP like Or we can propagate "undefined", which means one has to have a representation of This has to be done in the #249 where we review how the API used in a larger scope. |
|
@emmacasolin can you provide an update on the status of this PR when you're back in? It will also need to be rebased on master before merging. |
Will need to fix up the command to match the new cli style + double check tests but other than that this is done. Will probably be finished off today. |
emmacasolin
force-pushed
the
identities-augment
branch
from
0b76870
to
0346fc5
Compare
|
This has been rebased on master |
emmacasolin
force-pushed
the
identities-augment
branch
2 times, most recently
from
d84a164
to
53b73d5
Compare
|
There's a lot of uncleared state in the identities bin tests, primarily once an identity is authenticated in any test it remains authenticated in future tests. So far I haven't been able to find a way to clear this between tests without destroying and recreating the entire |
|
Still having an issue with a successful identity claim causing an error in "Should start discovery by node" in |
|
Not sure if you've pushed your version, but that test seems to be passing on its own on my end: Seems like there's some interference between tests though when ran together: |
|
I haven’t pushed up my most recent changes but the “Should fail for unauthenticated identities” test is now passing on my end (there was interference from the test above it where the authentication wasn’t being undone - this is now being cleared in the afterEach block). I haven’t been able to fix the discovery failure yet but it’s the first claims test which is creating the interference causing it.
…________________________________
From: Josh ***@***.***>
Sent: Friday, December 3, 2021 9:39:51 AM
To: MatrixAI/js-polykey ***@***.***>
Cc: emmacasolin ***@***.***>; Mention ***@***.***>
Subject: Re: [MatrixAI/js-polykey] Fixing the `pk identities claim` command to augment DIs (PR #278)
Not sure if you've pushed your version, but that test seems to be passing on its own on my end:
PASS tests/bin/identities.test.ts (21.953 s)
CLI Identities
commandAllowGestalts
○ skipped Should allow permissions on node.
○ skipped Should allow permissions on Identity.
○ skipped Should fail on invalid inputs.
commandDisallowGestalts
○ skipped Should disallow permissions on Node.
○ skipped Should disallow permissions on Identity.
○ skipped Should fail on invalid inputs.
commandPermissionsGestalts
○ skipped Should get permissions on Node.
○ skipped Should get permissions on Identity.
commandTrustGestalts
○ skipped Should set trust on Node.
○ skipped Should set trust on Identity.
○ skipped Should fail on invalid inputs.
commandUntrustGestalts
○ skipped Should unset trust on Node.
○ skipped Should unset trust on Identity.
○ skipped Should fail on invalid inputs.
commandClaimIdentity
○ skipped Should claim an identity.
○ skipped Should fail for unauthenticated identities.
commandAuthenticateProvider
○ skipped Should authenticate an identity with a provider.
commandGetGestalts
○ skipped Should list gestalt by Node
○ skipped Should list gestalt by Identity
commandListGestalts
○ skipped Should list gestalts with permissions.
commandSearchIdentities
○ skipped Should find a connected identity.
commandDiscoverGestalts
✓ Should start discovery by Node (1136 ms)
○ skipped Should start discovery by Identity
Test Suites: 1 passed, 1 total
Tests: 22 skipped, 1 passed, 23 total
Snapshots: 0 total
Time: 22.013 s, estimated 27 s
Ran all test suites matching /tests\/bin\/identities.test.ts/i.
GLOBAL TEARDOWN
Seems like there's some interference between tests though when ran together:
FAIL tests/bin/identities.test.ts (28.247 s)
CLI Identities
commandAllowGestalts
✓ Should allow permissions on node. (228 ms)
✓ Should allow permissions on Identity. (164 ms)
✓ Should fail on invalid inputs. (135 ms)
commandDisallowGestalts
✓ Should disallow permissions on Node. (73 ms)
✓ Should disallow permissions on Identity. (72 ms)
✓ Should fail on invalid inputs. (114 ms)
commandPermissionsGestalts
✓ Should get permissions on Node. (72 ms)
✓ Should get permissions on Identity. (66 ms)
commandTrustGestalts
✓ Should set trust on Node. (64 ms)
✓ Should set trust on Identity. (77 ms)
✓ Should fail on invalid inputs. (85 ms)
commandUntrustGestalts
✓ Should unset trust on Node. (70 ms)
✓ Should unset trust on Identity. (60 ms)
✓ Should fail on invalid inputs. (85 ms)
commandClaimIdentity
✓ Should claim an identity. (76 ms)
✕ Should fail for unauthenticated identities. (78 ms)
commandAuthenticateProvider
✓ Should authenticate an identity with a provider. (52 ms)
commandGetGestalts
✓ Should list gestalt by Node (74 ms)
✓ Should list gestalt by Identity (67 ms)
commandListGestalts
✓ Should list gestalts with permissions. (198 ms)
commandSearchIdentities
✓ Should find a connected identity. (55 ms)
commandDiscoverGestalts
✕ Should start discovery by Node (1290 ms)
✓ Should start discovery by Identity (969 ms)
● CLI Identities › commandClaimIdentity › Should fail for unauthenticated identities.
expect(received).toBeFalsy()
Received: true
575 | ];
576 | const result = await testUtils.pkStdio(commands, {}, dataDir);
577 | expect(result.exitCode === 0).toBeFalsy(); // Fails..
| ^
578 | });
579 | });
580 | describe('commandAuthenticateProvider', () => {
at Object.<anonymous> (tests/bin/identities.test.ts:577:37)
● CLI Identities › commandDiscoverGestalts › Should start discovery by Node
expect(received).toBe(expected) // Object.is equality
Expected: 0
Received: 1
754 | ];
755 | const result = await testUtils.pkStdio(commands, {}, dataDir);
756 | expect(result.exitCode).toBe(0);
| ^
757 |
758 | // We expect to find a gestalt now.
759 | const gestalt = await polykeyAgent.gestaltGraph.getGestalts();
at Object.<anonymous> (tests/bin/identities.test.ts:756:31)
Test Suites: 1 failed, 1 total
Tests: 2 failed, 21 passed, 23 total
Snapshots: 0 total
Time: 28.313 s
Ran all test suites matching /tests\/bin\/identities.test.ts/i.
GLOBAL TEARDOWN
npm ERR! Test failed. See above for more details.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub<https://apac01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2FMatrixAI%2Fjs-polykey%2Fpull%2F278%23issuecomment-985059516&data=04%7C01%7C%7C1470e9c1151c46cbc01e08d9b5e4a7ac%7C84df9e7fe9f640afb435aaaaaaaaaaaa%7C1%7C0%7C637740815942888729%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=nQdhF1FCqgDovTkeDWBFm4iCGkD26gdPsTd2vNzxuyc%3D&reserved=0>, or unsubscribe<https://apac01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FAUUKME3PG6JCJLIJVM7TJD3UO7YTPANCNFSM5G6AB65A&data=04%7C01%7C%7C1470e9c1151c46cbc01e08d9b5e4a7ac%7C84df9e7fe9f640afb435aaaaaaaaaaaa%7C1%7C0%7C637740815942898689%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=3yr7oLI0g6zPlw6mk9z0zA9RJ%2BuQjV4TLl9QGuM3p1E%3D&reserved=0>.
Triage notifications on the go with GitHub Mobile for iOS<https://apac01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fapps.apple.com%2Fapp%2Fapple-store%2Fid1477376905%3Fct%3Dnotification-email%26mt%3D8%26pt%3D524675&data=04%7C01%7C%7C1470e9c1151c46cbc01e08d9b5e4a7ac%7C84df9e7fe9f640afb435aaaaaaaaaaaa%7C1%7C0%7C637740815942908647%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=Ob2Y293xFlCIALl3D0UmQC7cHmd0p2x09ukv4ERShHE%3D&reserved=0> or Android<https://apac01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fplay.google.com%2Fstore%2Fapps%2Fdetails%3Fid%3Dcom.github.android%26referrer%3Dutm_campaign%253Dnotification-email%2526utm_medium%253Demail%2526utm_source%253Dgithub&data=04%7C01%7C%7C1470e9c1151c46cbc01e08d9b5e4a7ac%7C84df9e7fe9f640afb435aaaaaaaaaaaa%7C1%7C0%7C637740815942908647%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=3m5gxNvMswoNm6L4hM2J4kcBpCRcijwZ%2FA%2FlWt764Ig%3D&reserved=0>.
|
|
I've found the source of the error, however I don't know enough about the nodes domain to work out why it's occurring or how to fix it. What's happening is that when the node graph is being set up in the discover by node test (using |
Files intended to be changed in this PR:
Sigchain returns newly created claim
"Reset" option added to Test Provider to clear changes between tests
Replacing indentities exceptions with domain/client specific exceptions
Tests
|
emmacasolin
force-pushed
the
identities-augment
branch
from
fe15814
to
37d560a
Compare
3 tasks
|
The map used in protobuf is not a ES6 map. It's google-protobuf library's own map. The documentation is here: To convert it to a POJO: // this is the closest way to do it, the X is whatever the field name is
Object.fromEntries(message.getXMap().entries()); |
|
The provider authentication is working now. Naming of the protobuf messages came from https://datatracker.ietf.org/doc/html/rfc6749. However there is one problem. We have not dealt with stream cancellation from the client side. If the client side cancels the stream, the server does not listen for this event. Therefore subsequent write calls just hang forever. And server does not have deadlines, so it's just hanging forever. For example in the At the end where it tries to: await genWritable.next(authProcess);
await genWritable.next(null);The first call to write may never return and finish if the client has already cancelled the stream that This is because in our grpc utilities, we have not been listening for any kind of cancellation event. But more over we don't even have a server-side deadline system. There should 3 potential events that we will need to address in a later PR targetting #249 and #243 and #297:
|
|
@joshuakarp You need to update the Unsuccessful cases in |
|
After verifying all tests, lint, squash, and prepare to merge. |
|
Running tests:
|
joshuakarp
force-pushed
the
identities-augment
branch
from
c89c541
to
da50df6
Compare
|
Squashed and ready for merge. |
Fix naming of `identityId` field of `Provider` proto message Sigchain `addClaim()` method returns generated claim "Reset" functionality for Test Provider Replacing identities rpc exceptions with domain+client exceptions Refactoring identitiesAuthenticate process to correctly use a stream and expected data
joshuakarp
force-pushed
the
identities-augment
branch
from
5688a3c
to
16b8af2
Compare
14 tasks
Merged
15 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Currently the
pk-identities claimcommand does not perform a DI augmentation.src/client/rpcIdentities.tsandsrc/bin/identities/claim.tswill need to be rewritten to address this.Issues Fixed
pk identities claimcommand should perform augmentation #267Tasks
ClaimLinkIdentityobjectsigchain.addClaim()to add the claimpublishClaim()method on the providerpk identities claimcommand to augment DIs #278 (comment)Final checklist