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

CORE-12411 - Add declination reason #4696

Merged
merged 10 commits into from
Sep 27, 2023

Conversation

dimosr
Copy link
Contributor

@dimosr dimosr commented Sep 25, 2023

Changes

This change adjusts the MGM code, so that it communicates the reason for declined requests back to the user and the appropriate handler, so that this information is stored in the DB and surfaced to the user. In some cases, we don't want to reveal potentially sensitive information to a member (especially in cases where the request hasn't gone fully through authentication yet). In these cases, a general message is communicated instructing the user to communicate with the network operator for more details.

Depends on: corda/corda-api#1261

Note: api version will be switched back to beta+ before merging. It's switched to alpha temporarily to test the integration.

Testing

Tested manually one of these cases to ensure everything works end-to-end, by adjusting one of our e2e tests to register a notary with a vnode name that is the same as the notary service name.

Registration request (as viewed from the user side) - Before:

{
  "registrationId": "a0c5e535-d121-4554-b211-53891041c0a1",
  "registrationSent": "2023-09-25T08:41:03.873Z",
  "registrationUpdated": "2023-09-25T08:41:04.488Z",
  "registrationStatus": "DECLINED",
  "memberInfoSubmitted": {
    "data": {
      "registrationProtocolVersion": "1",
      "corda.session.keys.0.id": "958863771798",
      "corda.session.keys.0.signature.spec": "SHA256withECDSA",
      "corda.ledger.keys.0.id": "AE42C9D61F05",
      "corda.ledger.keys.0.signature.spec": "SHA256withECDSA",
      "corda.endpoints.0.connectionURL": "https://corda-p2p-gateway-worker.dimosraptis-cluster-b:8080",
      "corda.endpoints.0.protocolVersion": "1",
      "corda.roles.0": "notary",
      "corda.notary.service.name": "O=NotaryService, L=London, C=GB",
      "corda.notary.service.flow.protocol.name": "com.r3.corda.notary.plugin.nonvalidating",
      "corda.notary.service.flow.protocol.version.0": "1",
      "corda.notary.keys.0.id": "1309B3FAF53C",
      "corda.notary.keys.0.signature.spec": "SHA256withECDSA"
    }
  },
  "reason": null,
  "serial": 0
}

Registration request (as viewed from the user side) - After:

{
  "registrationId": "794ec5f2-f9dc-4fd2-9da0-53b4397c5fcf",
  "registrationSent": "2023-09-25T14:41:59.909Z",
  "registrationUpdated": "2023-09-25T14:42:15.219Z",
  "registrationStatus": "DECLINED",
  "memberInfoSubmitted": {
    "data": {
      "registrationProtocolVersion": "1",
      "corda.session.keys.0.id": "D9CF07C5CD29",
      "corda.session.keys.0.signature.spec": "SHA256withECDSA",
      "corda.ledger.keys.0.id": "5FE47FE47C07",
      "corda.ledger.keys.0.signature.spec": "SHA256withECDSA",
      "corda.endpoints.0.connectionURL": "https://localhost:8080",
      "corda.endpoints.0.protocolVersion": "1",
      "corda.roles.0": "notary",
      "corda.notary.service.name": "O=NotaryService, L=London, C=GB",
      "corda.notary.service.flow.protocol.name": "com.r3.corda.notary.plugin.nonvalidating",
      "corda.notary.service.flow.protocol.version.0": "1",
      "corda.notary.keys.0.id": "14BA31F11AEF",
      "corda.notary.keys.0.signature.spec": "SHA256withECDSA"
    }
  },
  "reason": "The virtual node `O=NotaryService, L=London, C=GB` and the notary service `O=NotaryService, L=London, C=GB` name cannot be the same.",
  "serial": 0
}

I also run the following tests:

@dimosr dimosr self-assigned this Sep 25, 2023
@corda-jenkins-ci02
Copy link
Contributor

corda-jenkins-ci02 bot commented Sep 25, 2023

Jenkins build for PR 4696 build 8

Build Successful:
Jar artifact version produced by this PR: 5.1.0.0-alpha-1695741367784
Helm chart version produced by this PR: 5.1.0-alpha.1695741367784
Helm chart pushed to: oci://corda-os-docker-dev.software.r3.com/helm-charts/pr-4696/corda

@dimosr dimosr marked this pull request as ready for review September 26, 2023 08:30
@dimosr dimosr requested review from a team as code owners September 26, 2023 08:30
yift-r3
yift-r3 previously approved these changes Sep 26, 2023
Copy link
Contributor

@yift-r3 yift-r3 left a comment

Choose a reason for hiding this comment

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

LGTM (but we need to merge the API change first).

yift-r3
yift-r3 previously approved these changes Sep 26, 2023
Copy link
Contributor

@yift-r3 yift-r3 left a comment

Choose a reason for hiding this comment

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

LGTM

charlieR3
charlieR3 previously approved these changes Sep 26, 2023
@dimosr dimosr dismissed stale reviews from charlieR3 and yift-r3 via 09e9a71 September 26, 2023 15:15
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.

3 participants