feat: negotiation and auto accept credentials #336

berendsliedrecht · 2021-06-29T14:34:45Z

This allows for negotiation between the holder and the issuer via the negotiateOffer and negotiateProposal functions on agent.credentials.

It also allows for auto accept of credentials. the autoAcceptCredentials can be set on the agent itself or on a specific credentialRecord where the credentialRecord has priority.

The autoAcceptCredential has three states: always, contentApproved and never where never is the default.

always: This means that any credential, no matter from who is automatically accepted (even if the attributes or credentialDefinitionId changed).
contentApproved: This means that when a credential is received it has to be accepted once and the attributes and credentialDefinitionId did not change. If something did change it will not automate the flow.
never: This simply means that no credentials are automatically accepted.

Signed-off-by: blu3beri <berend@animo.id>

Signed-off-by: Berend Sliedrecht <berend@animo.id>

Signed-off-by: Berend Sliedrecht <berend@animo.id> added check assert instead of try catch Signed-off-by: Berend Sliedrecht <berend@animo.id> Moved the else statement Signed-off-by: Berend Sliedrecht <berend@animo.id>

Signed-off-by: Berend Sliedrecht <berend@animo.id>

codecov-commenter · 2021-06-29T14:43:40Z

Codecov Report

Merging #336 (19a9ed0) into main (d35ede5) will increase coverage by 0.32%.
The diff coverage is 90.25%.

@@            Coverage Diff             @@
##             main     #336      +/-   ##
==========================================
+ Coverage   88.01%   88.34%   +0.32%     
==========================================
  Files         237      239       +2     
  Lines        4624     4796     +172     
  Branches      571      615      +44     
==========================================
+ Hits         4070     4237     +167     
- Misses        554      559       +5

Impacted Files	Coverage Δ
src/modules/proofs/services/ProofService.ts	`86.74% <ø> (ø)`
src/types.ts	`100.00% <ø> (ø)`
...s/credentials/handlers/ProposeCredentialHandler.ts	`76.00% <70.00%> (-24.00%)`	⬇️
...les/credentials/handlers/IssueCredentialHandler.ts	`89.47% <85.71%> (-10.53%)`	⬇️
...les/credentials/handlers/OfferCredentialHandler.ts	`89.47% <85.71%> (-10.53%)`	⬇️
...s/credentials/handlers/RequestCredentialHandler.ts	`89.47% <85.71%> (-10.53%)`	⬇️
...dules/credentials/CredentialResponseCoordinator.ts	`93.42% <93.42%> (ø)`
src/modules/credentials/CredentialsModule.ts	`90.74% <93.93%> (+3.08%)`	⬆️
src/agent/AgentConfig.ts	`94.91% <100.00%> (+0.27%)`	⬆️
...rc/modules/credentials/CredentialAutoAcceptType.ts	`100.00% <100.00%> (ø)`
... and 8 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d35ede5...19a9ed0. Read the comment docs.

TimoGlastra

This is really great @blu3beri! Please see my suggestions.

One thing I didn't mention in the comments yet, is that . We said that CredentialRecord.credentialAttributes would be the source of truth on our interpretation of what we expect to issue or get issued. Currently e.g. processCredential overwrites this value. This should only happen if we agreed to this proposal. Could you go over all methods in the module/service/handlers and make sure credentialAttributes is always what we see as the source of truth? I'm open to other approaches.

src/modules/credentials/CredentialUtils.ts

src/modules/credentials/CredentialsModule.ts

src/modules/credentials/handlers/IssueCredentialHandler.ts

src/types.ts

src/__tests__/credentials-autoAccept.test.ts

berendsliedrecht · 2021-07-01T13:39:11Z

This is really great @blu3beri! Please see my suggestions.

One thing I didn't mention in the comments yet, is that . We said that CredentialRecord.credentialAttributes would be the source of truth on our interpretation of what we expect to issue or get issued. Currently e.g. processCredential overwrites this value. This should only happen if we agreed to this proposal. Could you go over all methods in the module/service/handlers and make sure credentialAttributes is always what we see as the source of truth? I'm open to other approaches.

I understand what you mean, but processCredential does not overwrite the credentialAttributes. Am I missing something or did you mean another function?

TimoGlastra · 2021-07-01T13:44:09Z

I see. Forget that method. I should look before writing...

But the point stands: only overwrite the credentialAttributes if that's our interpretation of the credential data (so not in processCredential, but do in createAck. not in processOffer, but do in createRequest)

not sure if we're already doing this.

Signed-off-by: Berend Sliedrecht <berend@animo.id>

src/modules/credentials/handlers/AutoResponseHandler.ts

Signed-off-by: Berend Sliedrecht <berend@animo.id>

src/__tests__/helpers.ts

src/modules/credentials/CredentialResponseCoordinator.ts

src/types.ts

TimoGlastra

Few minor nits

TimoGlastra

Few minor nits

Signed-off-by: Berend Sliedrecht <berend@animo.id>

TimoGlastra · 2021-07-12T12:53:55Z

@jakubkoci could you take a look?

jakubkoci · 2021-07-12T15:32:19Z

@TimoGlastra Yes, I'll do it, first thing tomorrow morning :)

TimoGlastra · 2021-07-12T15:32:21Z

@blu3beri can you resolve the conflicts?

TimoGlastra · 2021-07-12T15:32:39Z

@TimoGlastra Yes, I'll do it, first thing tomorrow morning :)

Awesome! :)

…redential Signed-off-by: Berend Sliedrecht <berend@animo.id>

berendsliedrecht · 2021-07-13T08:31:03Z

Tests are currenlty failing due to merging. Will resolve asap.

jakubkoci

I'm not sure if I understand ContentApproved correctly but otherwise, this seems like a nice feature.

I was struggling a bit with understanding the validation part inside CredentialResponseCoordinator :)

src/__tests__/credentials-autoAccept.test.ts

src/modules/credentials/CredentialAutoAcceptType.ts

src/modules/credentials/CredentialResponseCoordinator.ts

src/modules/credentials/handlers/OfferCredentialHandler.ts

src/modules/credentials/handlers/RequestCredentialHandler.ts

src/modules/credentials/CredentialResponseCoordinator.ts

jakubkoci · 2021-07-13T08:31:40Z

src/modules/credentials/CredentialResponseCoordinator.ts

+    )
+
+    if (autoAccept === AutoAcceptCredential.Always) {
+      return true


It's interesting that we don't validate the credential in this case. Does it mean that ContentApproved means the content was validated?

What's the use case when you want to accept the different values than they're in credentialAttributes? Are these credentialAttributes updated when that case happen?

I guess I don't fully understand the ContentApproved. 😕 I thought its like that the user approved the offer. I think common use case when the user doesn't want to approve credential offer automatically but after approving the offer she doesn't care about the rest of the exchange (assuming it's validated by the framework)

Yeah, so the contentApproved was to check two things:

The user has accepted the credential once (offer or proposal)

The content of the credential did not change (if it changed the automation will be cancelled)

I am completely open to better naming or comprehensive description at the enum.

Yeah, I see. That's clear to me now, also not able to think of better naming 😄

src/modules/credentials/CredentialResponseCoordinator.ts

jakubkoci · 2021-07-13T09:28:18Z

Ah, sorry, I missed this "it has to be accepted once" in the explanation of ContentApproved. But does it mean accepting a credential offer?

Signed-off-by: Berend Sliedrecht <berend@animo.id>

berendsliedrecht · 2021-07-14T11:35:40Z

Ah, sorry, I missed this "it has to be accepted once" in the explanation of ContentApproved. But does it mean accepting a credential offer?

It means that once in the whole issuance flow the credential has been accepted. Whether it is a propose or offer does not matter. Also with contentApproved the attribute values and credentialDefinitionId are not allowed to change.

Signed-off-by: Berend Sliedrecht <berend@animo.id>

src/modules/credentials/CredentialResponseCoordinator.ts

Signed-off-by: Berend Sliedrecht <berend@animo.id>

jakubkoci · 2021-07-15T09:33:27Z

Ah, sorry, I missed this "it has to be accepted once" in the explanation of ContentApproved. But does it mean accepting a credential offer?

It means that once in the whole issuance flow the credential has been accepted. Whether it is a propose or offer does not matter. Also with contentApproved the attribute values and credentialDefinitionId are not allowed to change.

“Whether it is a propose or offer does not matter”

But we need to distinguish between the roles of issuer and holder, right?. As I understand it:

an issuer can accept the proposal, and then the rest of the issuance is automated on the issuer side
a holder can accept the offer, and then the rest of the issuance is automated on the holder side

Is that correct?

I’m still unsure if it’s good that there is no validation when a developer handles the process manually. Is it a valid use case when credential definition ID is changed during the process?

Maybe we can discuss it in the AFJ call. It's not definitely a blocker for merging.

jakubkoci

I still have some questions, but they're rather about explaining the behavior. That can be updated in documentation or comments later if needed.

berendsliedrecht · 2021-07-15T10:05:16Z

But we need to distinguish between the roles of issuer and holder, right?. As I understand it:
* an issuer can accept the proposal, and then the rest of the issuance is automated on the issuer side

* a holder can accept the offer, and then the rest of the issuance is automated on the holder side
Is that correct?

Yes, you only automate the flow for your agent. When negotiation starts, contentApproved sees that and will cancel the automation. This credential changed detection might be nice to expose to the public API so a framework consumer could easily detect changes and alert a user.

Is it a valid use case when credential definition ID is changed during the process?

I think so yeah, this could happen with negotiation.

I do think that the documentation is a bit lacking and unclear.

berendsliedrecht added 11 commits June 23, 2021 13:19

Auto accept credential implemented for 'always'

ce46e2e

Signed-off-by: blu3beri <berend@animo.id>

Always accept now works on agentconfig aswell

dbd2713

Signed-off-by: Berend Sliedrecht <berend@animo.id>

Attributes not changed works.

bec392b

Signed-off-by: Berend Sliedrecht <berend@animo.id>

Issue credential handler now checks for changes

8ae9fcd

Signed-off-by: Berend Sliedrecht <berend@animo.id>

Added checkValuesMatch as a wrapper to assertValuesMatch

c916870

Signed-off-by: Berend Sliedrecht <berend@animo.id> added check assert instead of try catch Signed-off-by: Berend Sliedrecht <berend@animo.id> Moved the else statement Signed-off-by: Berend Sliedrecht <berend@animo.id>

Added singleAccept and improved testing

4d71aa5

Signed-off-by: Berend Sliedrecht <berend@animo.id>

Merge branch 'main' into feature/auto-accept-credential

2a4149c

Signed-off-by: Berend Sliedrecht <berend@animo.id>

Removed attribute not changed state

9636336

Signed-off-by: Berend Sliedrecht <berend@animo.id>

refactored testing

9c8800d

Signed-off-by: Berend Sliedrecht <berend@animo.id>

Refactored credential handlers

5d88a0f

Signed-off-by: Berend Sliedrecht <berend@animo.id>

Added credentialDefinitionId to contentApproved

c728ea0

Signed-off-by: Berend Sliedrecht <berend@animo.id>

berendsliedrecht requested a review from a team as a code owner June 29, 2021 14:34

TimoGlastra requested changes Jun 30, 2021

View reviewed changes

berendsliedrecht added 2 commits July 2, 2021 13:51

Resolved feedback

a36c41d

Signed-off-by: Berend Sliedrecht <berend@animo.id>

Split the AutoResponseHandler into each handler

37777da

Signed-off-by: Berend Sliedrecht <berend@animo.id>

TimoGlastra reviewed Jul 3, 2021

View reviewed changes

src/modules/credentials/handlers/AutoResponseHandler.ts Outdated Show resolved Hide resolved

berendsliedrecht added 3 commits July 5, 2021 14:00

Added auto Accept credential to the issueCredential helper

06ac5a0

Signed-off-by: Berend Sliedrecht <berend@animo.id>

added dependency injection and renamed class

8a0901e

Signed-off-by: Berend Sliedrecht <berend@animo.id>

renamed to CredentialResponseCoordinator

8311a38

Signed-off-by: Berend Sliedrecht <berend@animo.id>

berendsliedrecht mentioned this pull request Jul 6, 2021

feat: auto accept proofs #367

Merged

TimoGlastra reviewed Jul 6, 2021

View reviewed changes

src/__tests__/helpers.ts Outdated Show resolved Hide resolved

TimoGlastra reviewed Jul 6, 2021

View reviewed changes

src/modules/credentials/CredentialResponseCoordinator.ts Outdated Show resolved Hide resolved

TimoGlastra reviewed Jul 6, 2021

View reviewed changes

src/types.ts Outdated Show resolved Hide resolved

TimoGlastra approved these changes Jul 6, 2021

View reviewed changes

berendsliedrecht added 2 commits July 7, 2021 11:04

fix: resolved feedback

0960bcb

Signed-off-by: Berend Sliedrecht <berend@animo.id>

fix: changed typing agentConfig

d345ddb

Signed-off-by: Berend Sliedrecht <berend@animo.id>

TimoGlastra requested a review from jakubkoci July 12, 2021 12:53

Merge remote-tracking branch 'origin/main' into feature/auto-accept-c…

3c741e2

…redential Signed-off-by: Berend Sliedrecht <berend@animo.id>

jakubkoci requested changes Jul 13, 2021

View reviewed changes

berendsliedrecht added 3 commits July 14, 2021 10:53

fix: resolved feedback

c988e5b

Signed-off-by: Berend Sliedrecht <berend@animo.id>

Merged and resolved async issues

d56c60f

Signed-off-by: Berend Sliedrecht <berend@animo.id>

fixed proof testing

a33e32a

Signed-off-by: Berend Sliedrecht <berend@animo.id>

berendsliedrecht added 2 commits July 14, 2021 14:34

test: increased testTimeout due to heavy e2e tests

fcff94f

Signed-off-by: Berend Sliedrecht <berend@animo.id>

test: fixed e2e test

41d9880

Signed-off-by: Berend Sliedrecht <berend@animo.id>

JamesKEbert reviewed Jul 14, 2021

View reviewed changes

src/modules/credentials/CredentialResponseCoordinator.ts Outdated Show resolved Hide resolved

fix: spelling

19a9ed0

Signed-off-by: Berend Sliedrecht <berend@animo.id>

jakubkoci approved these changes Jul 15, 2021

View reviewed changes

berendsliedrecht merged commit 55e8697 into openwallet-foundation:main Jul 15, 2021

berendsliedrecht deleted the feature/auto-accept-credential branch August 31, 2022 07:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: negotiation and auto accept credentials #336

feat: negotiation and auto accept credentials #336

berendsliedrecht commented Jun 29, 2021

codecov-commenter commented Jun 29, 2021 •

edited

Loading

TimoGlastra left a comment

berendsliedrecht commented Jul 1, 2021

TimoGlastra commented Jul 1, 2021 •

edited

Loading

TimoGlastra left a comment

TimoGlastra left a comment

TimoGlastra commented Jul 12, 2021

jakubkoci commented Jul 12, 2021

TimoGlastra commented Jul 12, 2021

TimoGlastra commented Jul 12, 2021

berendsliedrecht commented Jul 13, 2021

jakubkoci left a comment

jakubkoci Jul 13, 2021

berendsliedrecht Jul 13, 2021

jakubkoci Jul 15, 2021

jakubkoci commented Jul 13, 2021

berendsliedrecht commented Jul 14, 2021

jakubkoci commented Jul 15, 2021

jakubkoci left a comment

berendsliedrecht commented Jul 15, 2021

feat: negotiation and auto accept credentials #336

feat: negotiation and auto accept credentials #336

Conversation

berendsliedrecht commented Jun 29, 2021

codecov-commenter commented Jun 29, 2021 • edited Loading

Codecov Report

TimoGlastra left a comment

Choose a reason for hiding this comment

berendsliedrecht commented Jul 1, 2021

TimoGlastra commented Jul 1, 2021 • edited Loading

TimoGlastra left a comment

Choose a reason for hiding this comment

TimoGlastra left a comment

Choose a reason for hiding this comment

TimoGlastra commented Jul 12, 2021

jakubkoci commented Jul 12, 2021

TimoGlastra commented Jul 12, 2021

TimoGlastra commented Jul 12, 2021

berendsliedrecht commented Jul 13, 2021

jakubkoci left a comment

Choose a reason for hiding this comment

jakubkoci Jul 13, 2021

Choose a reason for hiding this comment

berendsliedrecht Jul 13, 2021

Choose a reason for hiding this comment

jakubkoci Jul 15, 2021

Choose a reason for hiding this comment

jakubkoci commented Jul 13, 2021

berendsliedrecht commented Jul 14, 2021

jakubkoci commented Jul 15, 2021

jakubkoci left a comment

Choose a reason for hiding this comment

berendsliedrecht commented Jul 15, 2021

codecov-commenter commented Jun 29, 2021 •

edited

Loading

TimoGlastra commented Jul 1, 2021 •

edited

Loading