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
Draft
Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
49 commits
Select commit Hold shift + click to select a range
d6adff0
initial without sep24 claim verification
reecexlm Apr 12, 2024
2b7aa01
resolve conflicts
reecexlm Apr 16, 2024
6b24019
missed resolving 2 conflicts
reecexlm Apr 16, 2024
66b017d
updates to serveopts for multitenancy to support external-id
reecexlm Apr 16, 2024
99bb3c9
fix for tenant domain in unit test
reecexlm Apr 18, 2024
ee87c8b
draft of customer-id/hashed mobile# verification
reecexlm Apr 18, 2024
88245da
adding more tests
reecexlm Apr 19, 2024
b05dc79
Fix indentation and tests.
marcelosalloum Apr 19, 2024
09aeba1
Remove extra spaces.
marcelosalloum Apr 19, 2024
b90ae10
fix db cleanup in tests
reecexlm Apr 20, 2024
94f554d
fix conflicts
reecexlm Apr 20, 2024
75a9f94
test fixes
reecexlm Apr 20, 2024
ab60cac
remove rollback
reecexlm Apr 20, 2024
fe65c5e
fix
reecexlm Apr 20, 2024
a36577a
noop
reecexlm Apr 21, 2024
8148138
noop
reecexlm Apr 21, 2024
21eefee
noop
reecexlm Apr 21, 2024
b7f7e96
noop
reecexlm Apr 21, 2024
4fa13da
noop
reecexlm Apr 21, 2024
4c28c3d
noop
reecexlm Apr 21, 2024
0942120
noop
reecexlm Apr 21, 2024
5f00001
noop
reecexlm Apr 22, 2024
dc61fd8
noop
reecexlm Apr 23, 2024
ad5603f
Update internal/services/send_receiver_wallets_invite_service.go
reecexlm Apr 23, 2024
292015f
noop
reecexlm Apr 23, 2024
573de96
noop
marwen-abid Apr 23, 2024
f83d283
Fix lint issues and other errors.
marwen-abid Apr 23, 2024
20ae751
make `kafka-security-protocol` optional
marwen-abid Apr 23, 2024
fcb9827
noop
reecexlm Apr 24, 2024
eda7452
Merge branch 'sdp-1116-externalid' of https://github.com/stellar/stel…
reecexlm Apr 24, 2024
aab9010
noop
reecexlm Apr 24, 2024
37c3db1
noop
reecexlm Apr 25, 2024
f9ae6b0
do not crash if default tenant is missing
reecexlm Apr 26, 2024
7793716
do not error on missing tenant
reecexlm Apr 27, 2024
d8d0896
providing an informative error message
reecexlm Apr 27, 2024
2c47a3e
noop
reecexlm Apr 27, 2024
acc79af
noop
reecexlm Apr 27, 2024
3be5dc2
Add Postman Sample Collection
reecexlm Apr 27, 2024
6efdfec
noop
reecexlm Apr 30, 2024
b88185d
noop
reecexlm May 1, 2024
19ee8de
noop
reecexlm May 1, 2024
558eae4
debugging
reecexlm May 1, 2024
7d6c88e
debugging
reecexlm May 1, 2024
9495335
put middleware tenant check back
reecexlm May 9, 2024
91844b6
turn debug off
reecexlm May 9, 2024
cbae53c
try moving health to top of handleHTTP
reecexlm May 9, 2024
0a0f0ef
only warn on tenant missing
reecexlm May 9, 2024
a9c87f9
Merge branch 'develop' into sdp-1116-externalid
reecexlm May 31, 2024
3e6ef02
Merge branch 'develop' into sdp-1116-externalid
reecexlm Jul 20, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions cmd/serve.go
marcelosalloum marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
Expand Up @@ -131,6 +131,7 @@ func (s *ServerService) SetupConsumers(ctx context.Context, o SetupConsumersOpti
MaxInvitationSMSResendAttempts: int64(o.ServeOpts.MaxInvitationSMSResendAttempts),
Sep10SigningPrivateKey: o.ServeOpts.Sep10SigningPrivateKey,
CrashTrackerClient: o.ServeOpts.CrashTrackerClient.Clone(),
UseExternalID: o.ServeOpts.UseExternalID,
}),
)
if err != nil {
Expand Down Expand Up @@ -323,6 +324,14 @@ func (c *ServeCommand) Command(serverService ServerServiceInterface, monitorServ
FlagDefault: 3,
Required: true,
},
{
Name: "use-external-id",
Usage: "Enable or disable the use of external ID in wallet deep links",
OptType: types.Bool,
ConfigKey: &serveOpts.UseExternalID, // Ensure ServeOptions has a UseExternalID field of type bool
FlagDefault: false, // Default value set to false. Do Not embed external_id in wallet deep links
Copy link
Collaborator

Choose a reason for hiding this comment

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

👋 Is you want to make this feature available for tests without changing anything in the dpeloyments, you can simply set the default to true:

Suggested change
FlagDefault: false, // Default value set to false. Do Not embed external_id in wallet deep links
FlagDefault: true, // Default value set to true so the feature is enabled in the PR preview. // TODO: default to false if merging it in the actual code.

Required: false,
},
{
Name: "single-tenant-mode",
Usage: "This option enables the Single Tenant Mode feature. In the case where multi-tenancy is not required, this options bypasses the tenant resolution by always resolving to the default tenant configured in the database.",
Expand Down
1 change: 1 addition & 0 deletions internal/data/assets.go
Original file line number Diff line number Diff line change
Expand Up @@ -286,6 +286,7 @@ func (a *AssetModel) GetAssetsPerReceiverWallet(ctx context.Context, receiverWal
r.id AS "receiver_wallet.receiver.id",
r.phone_number AS "receiver_wallet.receiver.phone_number",
r.email AS "receiver_wallet.receiver.email",
r.external_id AS "receiver_wallet.receiver.external_id",
a.id AS "asset.id",
a.code AS "asset.code",
a.issuer AS "asset.issuer",
Expand Down
13 changes: 11 additions & 2 deletions internal/data/assets_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,12 @@ import (
"time"

"github.com/stellar/go/protocols/horizon/base"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"

"github.com/stellar/stellar-disbursement-platform-backend/db"
"github.com/stellar/stellar-disbursement-platform-backend/db/dbtest"
"github.com/stellar/stellar-disbursement-platform-backend/internal/message"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
)

func Test_Asset_IsNative(t *testing.T) {
Expand Down Expand Up @@ -639,6 +640,7 @@ func Test_GetAssetsPerReceiverWallet(t *testing.T) {
ID: receiverX.ID,
Email: receiverX.Email,
PhoneNumber: receiverX.PhoneNumber,
ExternalID: receiverX.ExternalID,
},
ReceiverWalletStats: ReceiverWalletStats{
TotalInvitationSMSResentAttempts: 2,
Expand All @@ -656,6 +658,7 @@ func Test_GetAssetsPerReceiverWallet(t *testing.T) {
ID: receiverX.ID,
Email: receiverX.Email,
PhoneNumber: receiverX.PhoneNumber,
ExternalID: receiverX.ExternalID,
},
InvitationSentAt: &invitationSentAt,
},
Expand All @@ -670,6 +673,7 @@ func Test_GetAssetsPerReceiverWallet(t *testing.T) {
ID: receiverX.ID,
Email: receiverX.Email,
PhoneNumber: receiverX.PhoneNumber,
ExternalID: receiverX.ExternalID,
},
},
WalletID: walletB.ID,
Expand All @@ -683,6 +687,7 @@ func Test_GetAssetsPerReceiverWallet(t *testing.T) {
ID: receiverX.ID,
Email: receiverX.Email,
PhoneNumber: receiverX.PhoneNumber,
ExternalID: receiverX.ExternalID,
},
},
WalletID: walletB.ID,
Expand All @@ -696,6 +701,7 @@ func Test_GetAssetsPerReceiverWallet(t *testing.T) {
ID: receiverY.ID,
Email: receiverY.Email,
PhoneNumber: receiverY.PhoneNumber,
ExternalID: receiverY.ExternalID,
},
},
WalletID: walletA.ID,
Expand All @@ -709,6 +715,7 @@ func Test_GetAssetsPerReceiverWallet(t *testing.T) {
ID: receiverY.ID,
Email: receiverY.Email,
PhoneNumber: receiverY.PhoneNumber,
ExternalID: receiverY.ExternalID,
},
},
WalletID: walletA.ID,
Expand All @@ -722,6 +729,7 @@ func Test_GetAssetsPerReceiverWallet(t *testing.T) {
ID: receiverY.ID,
Email: receiverY.Email,
PhoneNumber: receiverY.PhoneNumber,
ExternalID: receiverY.ExternalID,
},
},
WalletID: walletB.ID,
Expand All @@ -735,6 +743,7 @@ func Test_GetAssetsPerReceiverWallet(t *testing.T) {
ID: receiverY.ID,
Email: receiverY.Email,
PhoneNumber: receiverY.PhoneNumber,
ExternalID: receiverY.ExternalID,
},
},
WalletID: walletB.ID,
Expand Down
25 changes: 25 additions & 0 deletions internal/data/receivers.go
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,9 @@
VerificationValue string `json:"verification"`
VerificationType VerificationField `json:"verification_type"`
ReCAPTCHAToken string `json:"recaptcha_token"`
ExternalID string `json:"external_id"`
CustomerID string `json:"customer_id"` //customer identifier

Check failure on line 35 in internal/data/receivers.go

View workflow job for this annotation

GitHub Actions / check

File is not `gofumpt`-ed (gofumpt)
Copy link
Collaborator

Choose a reason for hiding this comment

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

One of these will eventually be removed once we nail the customer_id vs external_id discussion.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh, I can see that SEP-24 uses customer_id, so we should use the same wording.

Suggested change
ExternalID string `json:"external_id"`
CustomerID string `json:"customer_id"` //customer identifier
// CustomerID is the SEP-24 field that we store in our database's `receivers.external_id`.
CustomerID string `json:"customer_id"` //customer identifier

MobileNumberHash string `json:"mobile_number"` //hashed mobile number
}

type ReceiverStats struct {
Expand Down Expand Up @@ -442,3 +445,25 @@
return nil
})
}

// GetByExternalID retrieves a receiver's phone number based on the external_id.
func (r *ReceiverModel) GetByExternalID(ctx context.Context, sqlExec db.SQLExecuter, externalID string) (*Receiver, error) {
receiver := Receiver{}

query := `
SELECT id, phone_number
FROM receivers
WHERE external_id = $1
LIMIT 1
`

err := sqlExec.GetContext(ctx, &receiver, query, externalID)
if err != nil {
if err == sql.ErrNoRows {
return nil, fmt.Errorf("no receiver found with external_id %s: %w", externalID, err)
}
// Handle other potential errors
return nil, fmt.Errorf("error fetching receiver by external ID: %w", err)
}
return &receiver, nil
}
3 changes: 2 additions & 1 deletion internal/data/receivers_wallet.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,9 @@ import (
"time"

"github.com/lib/pq"

"github.com/stellar/go/network"
"github.com/stellar/go/support/log"

"github.com/stellar/stellar-disbursement-platform-backend/db"
)

Expand Down Expand Up @@ -231,6 +231,7 @@ const getPendingRegistrationReceiverWalletsBaseQuery = `
r.id AS "receiver.id",
r.phone_number AS "receiver.phone_number",
r.email AS "receiver.email",
r.external_id AS "receiver.external_id",
w.id AS "wallet.id",
w.name AS "wallet.name"
FROM
Expand Down
13 changes: 11 additions & 2 deletions internal/data/receivers_wallet_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,12 +7,13 @@ import (
"time"

"github.com/stellar/go/network"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"

"github.com/stellar/stellar-disbursement-platform-backend/db"
"github.com/stellar/stellar-disbursement-platform-backend/db/dbtest"
"github.com/stellar/stellar-disbursement-platform-backend/internal/message"
"github.com/stellar/stellar-disbursement-platform-backend/internal/utils"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
)

func Test_ReceiversWalletModelGetWithReceiverId(t *testing.T) {
Expand Down Expand Up @@ -892,6 +893,7 @@ func Test_ReceiverWallet_GetAllPendingRegistration(t *testing.T) {
ID: receiver.ID,
PhoneNumber: receiver.PhoneNumber,
Email: receiver.Email,
ExternalID: receiver.ExternalID,
},
Wallet: Wallet{
ID: wallet3.ID,
Expand All @@ -904,6 +906,7 @@ func Test_ReceiverWallet_GetAllPendingRegistration(t *testing.T) {
ID: receiver.ID,
PhoneNumber: receiver.PhoneNumber,
Email: receiver.Email,
ExternalID: receiver.ExternalID,
},
Wallet: Wallet{
ID: wallet4.ID,
Expand Down Expand Up @@ -1013,6 +1016,7 @@ func Test_ReceiverWallet_GetAllPendingRegistrationByReceiverWalletIDs(t *testing
ID: receiver.ID,
PhoneNumber: receiver.PhoneNumber,
Email: receiver.Email,
ExternalID: receiver.ExternalID,
},
Wallet: Wallet{
ID: wallet3.ID,
Expand All @@ -1025,6 +1029,7 @@ func Test_ReceiverWallet_GetAllPendingRegistrationByReceiverWalletIDs(t *testing
ID: receiver.ID,
PhoneNumber: receiver.PhoneNumber,
Email: receiver.Email,
ExternalID: receiver.ExternalID,
},
Wallet: Wallet{
ID: wallet4.ID,
Expand Down Expand Up @@ -1096,6 +1101,7 @@ func Test_ReceiverWallet_GetAllPendingRegistrationByReceiverWalletIDs(t *testing
ID: receiver.ID,
PhoneNumber: receiver.PhoneNumber,
Email: receiver.Email,
ExternalID: receiver.ExternalID,
},
Wallet: Wallet{
ID: wallet1.ID,
Expand All @@ -1108,6 +1114,7 @@ func Test_ReceiverWallet_GetAllPendingRegistrationByReceiverWalletIDs(t *testing
ID: receiver.ID,
PhoneNumber: receiver.PhoneNumber,
Email: receiver.Email,
ExternalID: receiver.ExternalID,
},
Wallet: Wallet{
ID: wallet2.ID,
Expand Down Expand Up @@ -1198,6 +1205,7 @@ func Test_ReceiverWallet_GetAllPendingRegistrationByDisbursementID(t *testing.T)
ID: receiver3.ID,
PhoneNumber: receiver3.PhoneNumber,
Email: receiver3.Email,
ExternalID: receiver3.ExternalID,
},
Wallet: Wallet{
ID: wallet.ID,
Expand All @@ -1210,6 +1218,7 @@ func Test_ReceiverWallet_GetAllPendingRegistrationByDisbursementID(t *testing.T)
ID: receiver4.ID,
PhoneNumber: receiver4.PhoneNumber,
Email: receiver4.Email,
ExternalID: receiver4.ExternalID,
},
Wallet: Wallet{
ID: wallet.ID,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ type SendReceiverWalletsSMSInvitationEventHandlerOptions struct {
MaxInvitationSMSResendAttempts int64
Sep10SigningPrivateKey string
CrashTrackerClient crashtracker.CrashTrackerClient
UseExternalID bool
}

type SendReceiverWalletsSMSInvitationEventHandler struct {
Expand All @@ -50,6 +51,7 @@ func NewSendReceiverWalletsSMSInvitationEventHandler(options SendReceiverWallets
options.Sep10SigningPrivateKey,
options.MaxInvitationSMSResendAttempts,
options.CrashTrackerClient,
options.UseExternalID,
)
if err != nil {
log.Fatalf("error instantiating service: %s", err.Error())
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import (
"time"

"github.com/stellar/go/support/log"

"github.com/stellar/stellar-disbursement-platform-backend/internal/crashtracker"
"github.com/stellar/stellar-disbursement-platform-backend/internal/data"
"github.com/stellar/stellar-disbursement-platform-backend/internal/message"
Expand All @@ -22,6 +23,7 @@ type SendReceiverWalletsSMSInvitationJobOptions struct {
MaxInvitationSMSResendAttempts int64
Sep10SigningPrivateKey string
CrashTrackerClient crashtracker.CrashTrackerClient
UseExternalID bool
JobIntervalSeconds int
}

Expand Down Expand Up @@ -62,6 +64,7 @@ func NewSendReceiverWalletsSMSInvitationJob(options SendReceiverWalletsSMSInvita
options.Sep10SigningPrivateKey,
options.MaxInvitationSMSResendAttempts,
options.CrashTrackerClient,
options.UseExternalID,
)
if err != nil {
log.Fatalf("error instantiating service: %s", err.Error())
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,17 +10,16 @@ import (
"time"

"github.com/google/uuid"
"github.com/stellar/stellar-disbursement-platform-backend/stellar-multitenant/pkg/tenant"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"

"github.com/stellar/stellar-disbursement-platform-backend/db"
"github.com/stellar/stellar-disbursement-platform-backend/db/dbtest"

"github.com/stellar/stellar-disbursement-platform-backend/internal/crashtracker"
"github.com/stellar/stellar-disbursement-platform-backend/internal/data"
"github.com/stellar/stellar-disbursement-platform-backend/internal/message"
"github.com/stellar/stellar-disbursement-platform-backend/internal/services"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
"github.com/stellar/stellar-disbursement-platform-backend/stellar-multitenant/pkg/tenant"
)

func Test_NewSendReceiverWalletsSMSInvitationJob(t *testing.T) {
Expand Down Expand Up @@ -143,6 +142,7 @@ func Test_SendReceiverWalletsSMSInvitationJob_Execute(t *testing.T) {
stellarSecretKey,
maxInvitationSMSResendAttempts,
crashTrackerClientMock,
true,
)
require.NoError(t, err)

Expand Down Expand Up @@ -206,6 +206,7 @@ func Test_SendReceiverWalletsSMSInvitationJob_Execute(t *testing.T) {
OrganizationName: "MyCustomAid",
AssetCode: asset1.Code,
AssetIssuer: asset1.Issuer,
ExternalID: receiver1.ExternalID,
}
deepLink1, err := walletDeepLink1.GetSignedRegistrationLink(stellarSecretKey)
require.NoError(t, err)
Expand All @@ -217,6 +218,7 @@ func Test_SendReceiverWalletsSMSInvitationJob_Execute(t *testing.T) {
OrganizationName: "MyCustomAid",
AssetCode: asset2.Code,
AssetIssuer: asset2.Issuer,
ExternalID: receiver2.ExternalID,
}
deepLink2, err := walletDeepLink2.GetSignedRegistrationLink(stellarSecretKey)
require.NoError(t, err)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,25 @@ func (v VerifyReceiverRegistrationHandler) processReceiverVerificationPII(
now := time.Now()
truncatedPhoneNumber := utils.TruncateString(receiver.PhoneNumber, 3)

// STEP 0: if customer-id exists in sep24 claims, use it to look up reciever, hash Phone Number in db, compare against hashed mobile_number in sep24 claims
if receiverRegistrationRequest.CustomerID != "" {
Copy link
Collaborator

Choose a reason for hiding this comment

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

👋 The trigger to validate the customerID should not depend on it being in the request, it should instead depend on the use-external-id CLI configuration.

Otherwise the externalID check will be ignored if the externalID is not provided.

This suggestion shows how to wire this configuration: #256 (comment)

receiverByExternalID, err := v.Models.Receiver.GetByExternalID(ctx, dbTx, receiver.ExternalID)
if err != nil {
log.Ctx(ctx).Errorf("Error retrieving receiver by external ID: %v", err)
return &ErrorInformationNotFound{cause: err}
}
fmt.Println(receiverByExternalID.PhoneNumber)

if data.CompareVerificationValue(receiverRegistrationRequest.MobileNumberHash, receiverByExternalID.PhoneNumber) {
// Compare the hashed phone number with the hashed mobile number from the SEP-24 claims
//if hashedPhoneNumber != receiverRegistrationRequest.MobileNumberHash {
//err := fmt.Errorf("hashed phone number does not match the hashed mobile number from SEP-24 claims")
// return &ErrorInformationNotFound{cause: err}
//}
return nil
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

👋 👋 👋 This logic is incorrect. When you return nil here, you're skipping all the check from the following steps (STEP 1 -> 4).

What you should be doing here is returning an error if the comparison fails, similar to what's in the comment:

Suggested change
if data.CompareVerificationValue(receiverRegistrationRequest.MobileNumberHash, receiverByExternalID.PhoneNumber) {
// Compare the hashed phone number with the hashed mobile number from the SEP-24 claims
//if hashedPhoneNumber != receiverRegistrationRequest.MobileNumberHash {
//err := fmt.Errorf("hashed phone number does not match the hashed mobile number from SEP-24 claims")
// return &ErrorInformationNotFound{cause: err}
//}
return nil
}
if !data.CompareVerificationValue(receiverRegistrationRequest.MobileNumberHash, receiverByExternalID.PhoneNumber) {
err := fmt.Errorf("hashed phone number does not match the hashed mobile number from SEP-24 claims")
return &ErrorInformationNotFound{cause: err}
}

}

// STEP 1: find the receiverVerification entry that matches the pair [receiverID, verificationType]
receiverVerifications, err := v.Models.ReceiverVerification.GetByReceiverIDsAndVerificationField(ctx, dbTx, []string{receiver.ID}, receiverRegistrationRequest.VerificationType)
if err != nil {
Expand Down Expand Up @@ -141,7 +160,7 @@ func (v VerifyReceiverRegistrationHandler) processReceiverVerificationPII(
receiverVerification.FailedAt = &now
receiverVerification.ConfirmedAt = nil

// this update is done using the DBConnectionPool and not dbTx because we don't want to roolback these changes after returning the error
// this update is done using the DBConnectionPool and not dbTx because we don't want to rollback these changes after returning the error
updateErr := v.Models.ReceiverVerification.UpdateReceiverVerification(ctx, *receiverVerification, v.Models.DBConnectionPool)
if updateErr != nil {
err = fmt.Errorf("%s: %w", baseErrMsg, updateErr)
Expand Down
Loading
Loading