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

SDP-1116 Support External_ID DRAFT PR (See TODO). reece #256

Draft
wants to merge 49 commits into
base: develop
Choose a base branch
from

Conversation

reecexlm
Copy link
Collaborator

What

This change will be deployed in a branch with a PR Preview pipeline to support Vibrant Assist in testing integration of supporting external_id in SDP registration deep link and sep24 post interactive deposit for partner integration.

The receiver’s invitation DeepLink should include the field external_id, containing the ID of that receiver_wallet

This code will not be merged for now. We should decide if we want to adopt this external-id field or not.

TBH, it could remove the need for an OTP step on our end, but we should discuss if we want that.

We need to validate the decision with @nando Vieira before concluding if this will be part of the official implementation, or if an optional field.

added some additional unit tests and verified all existing tests are passing.

Why

To support reconciliation of vibrant client (mobile phone) with SDP Host Customer ID by using an external_id field in the deeplink sent from the sdp host (SDP registration deep link). then this will be passed within SEP24 claims for reconciliation.

Known limitations / TODO

TBD - not yet part of this PR: When validating the wallet-registration, verify if the SEP24 incoming info contains the field: customer_id=external_id:hash(phone_number) Use the external_id to find the user in our database Hash the user’s phone number and compare it with the incoming hashed phone number It’s ok to send an informative response, so Vibrant can understand this well.

PR Structure

  • [x ] This PR has a reasonably narrow scope
  • [x ] This PR title and description are clear enough for anyone to review it.
  • This PR does not mix refactoring changes with feature changes (split into two PRs otherwise).

Thoroughness

  • This PR adds tests for the new functionality or fixes.
  • This PR contains the link to the Jira ticket it addresses.

Configs and Secrets

there is a new configuration option useExternalID (true/false) to make this optional. This has not yet been updated to helm charts yet until we determine if we plan to include this support in our standard release.

Release

not for release at this time. only for testing with Vibrant.

Deployment

Will be deployed only in a pr-preview pipeline at this time.

Copy link
Collaborator

@marcelosalloum marcelosalloum left a comment

Choose a reason for hiding this comment

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

This is looking pretty good, congrats!

I've shared some thoughts below on:

  • some connection points that are missing (to make things easier for you)
  • my opinion on where to do the final verification when validating the user in the SEP24 request.

Also, a nitpicky thing: indentation is looking a bit weird in some places. It looks like the old problem of spaces vs tabs indentation.

internal/data/assets_test.go Outdated Show resolved Hide resolved
Copy link
Collaborator

Choose a reason for hiding this comment

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

You will need to propagate the UseExternalID config to the handler responsible for /wallet-registration/verification, injecting it in:

r.With(sep24HeaderTokenAuthenticationMiddleware).Post("/verification", httphandler.VerifyReceiverRegistrationHandler{
AnchorPlatformAPIService: o.AnchorPlatformAPIService,
Models: o.Models,
ReCAPTCHAValidator: reCAPTCHAValidator,
NetworkPassphrase: o.NetworkPassphrase,
EventProducer: o.EventProducer,
}.VerifyReceiverRegistration)

cmd/serve.go Show resolved Hide resolved
@reecexlm reecexlm had a problem deploying to Receiver Registration - E2E Integration Tests April 16, 2024 18:55 — with GitHub Actions Failure
@reecexlm reecexlm had a problem deploying to Anchor Integration Tests April 16, 2024 18:55 — with GitHub Actions Failure
@stellar-jenkins
Copy link

Something went wrong with PR preview build please check

@reecexlm reecexlm had a problem deploying to Anchor Integration Tests April 16, 2024 19:05 — with GitHub Actions Failure
@reecexlm reecexlm had a problem deploying to Receiver Registration - E2E Integration Tests April 16, 2024 19:05 — with GitHub Actions Failure
@stellar-jenkins
Copy link

Something went wrong with PR preview build please check

@reecexlm reecexlm temporarily deployed to Receiver Registration - E2E Integration Tests April 16, 2024 19:50 — with GitHub Actions Inactive
@reecexlm reecexlm temporarily deployed to Anchor Integration Tests April 16, 2024 19:50 — with GitHub Actions Inactive
@reecexlm reecexlm temporarily deployed to Anchor Integration Tests April 18, 2024 18:46 — with GitHub Actions Inactive
@reecexlm reecexlm temporarily deployed to Receiver Registration - E2E Integration Tests April 18, 2024 18:46 — with GitHub Actions Inactive
@reecexlm reecexlm temporarily deployed to Anchor Integration Tests April 18, 2024 23:17 — with GitHub Actions Inactive
@reecexlm reecexlm temporarily deployed to Receiver Registration - E2E Integration Tests April 18, 2024 23:17 — with GitHub Actions Inactive
@marcelosalloum marcelosalloum temporarily deployed to Receiver Registration - E2E Integration Tests April 19, 2024 14:48 — with GitHub Actions Inactive
@marcelosalloum marcelosalloum temporarily deployed to Anchor Integration Tests April 19, 2024 14:48 — with GitHub Actions Inactive
@reecexlm reecexlm temporarily deployed to Anchor Integration Tests May 1, 2024 20:00 — with GitHub Actions Inactive
@reecexlm reecexlm temporarily deployed to Receiver Registration - E2E Integration Tests May 1, 2024 20:00 — with GitHub Actions Inactive
@reecexlm reecexlm had a problem deploying to Anchor Integration Tests May 9, 2024 20:41 — with GitHub Actions Failure
@reecexlm reecexlm temporarily deployed to Receiver Registration - E2E Integration Tests May 9, 2024 20:41 — with GitHub Actions Inactive
// Create a client to skip certificate verification (for testing)
client := &http.Client{
Transport: &http.Transport{
TLSClientConfig: &tls.Config{InsecureSkipVerify: true},

Check failure

Code scanning / CodeQL

Disabled TLS certificate check High

InsecureSkipVerify should not be used in production code.
@reecexlm reecexlm had a problem deploying to Anchor Integration Tests May 9, 2024 20:51 — with GitHub Actions Failure
@reecexlm reecexlm temporarily deployed to Receiver Registration - E2E Integration Tests May 9, 2024 20:51 — with GitHub Actions Inactive
@reecexlm reecexlm had a problem deploying to Receiver Registration - E2E Integration Tests May 9, 2024 20:57 — with GitHub Actions Failure
@reecexlm reecexlm had a problem deploying to Anchor Integration Tests May 9, 2024 20:57 — with GitHub Actions Failure
@reecexlm reecexlm temporarily deployed to Receiver Registration - E2E Integration Tests May 9, 2024 23:01 — with GitHub Actions Inactive
@reecexlm reecexlm had a problem deploying to Anchor Integration Tests May 9, 2024 23:01 — with GitHub Actions Failure
@reecexlm reecexlm had a problem deploying to Anchor Integration Tests May 31, 2024 05:18 — with GitHub Actions Failure
@reecexlm reecexlm had a problem deploying to Receiver Registration - E2E Integration Tests May 31, 2024 05:18 — with GitHub Actions Failure
@stellar-jenkins
Copy link

Something went wrong with PR preview build please check

@reecexlm reecexlm had a problem deploying to Receiver Registration - E2E Integration Tests (Circle) July 20, 2024 20:33 — with GitHub Actions Failure
@reecexlm reecexlm had a problem deploying to Receiver Registration - E2E Integration Tests (Stellar) July 20, 2024 20:33 — with GitHub Actions Failure
@reecexlm reecexlm had a problem deploying to Anchor Integration Tests July 20, 2024 20:33 — with GitHub Actions Failure
@stellar-jenkins
Copy link

Something went wrong with PR preview build please check

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.

4 participants