[ANCHOR-380] Add client_id to `getRate()` in `Sep31Service` #1068

lijamie98 · 2023-08-28T23:41:35Z

Description

Fix the bug where client_id was not passed to getRate() when the customer of the wallet exists.

Context

Bug fix

Testing

./gradlew test

stellar-jenkins · 2023-08-28T23:46:52Z

Reference Server Preview is available here:
https://anchor-ref-pr1068.previews.kube001.services.stellar-ops.com/
SEP Server Preview is available here:
https://anchor-sep-pr1068.previews.kube001.services.stellar-ops.com/

stellar-jenkins · 2023-08-28T23:49:19Z

Something went wrong with PR preview build please check

JakeUrban · 2023-08-28T23:55:50Z

core/src/main/java/org/stellar/anchor/sep38/Sep38Service.java

+  private String getClientIdFromToken(Sep10Jwt token) throws AnchorException {
+    if (token == null || token.getAccount() == null) return null;
+    // Get clientId if the client exists
+    try {
+      GetCustomerResponse customer =
+          customerIntegration.getCustomer(
+              GetCustomerRequest.builder().account(token.getAccount()).build());
+      return customer.getId();
+    } catch (NotFoundException nfex) {
+      // skip only if customer not found.
+      debugF("Unable to get customer {} from customer integration server.", token.getAccount());
+    }
+    return null;
+  }


I think there is a misunderstanding of what clientId is. Its not the ID of the customer requesting the quote.

From the documentation:

Client IDs will be the Stellar public key the public key used to authenticate via SEP-10. Anchors must ensure that > the public key specified in the request matches a public key known to belong to the sending anchor

This is only true for SEP-31 though. In SEP-24 & SEP-6, the client ID is either the Stellar public key of the wallet OR the wallet's domain.

@Ifropc wants to replace usage of Stellar public keys with domains, even for custodial applications. For 1.x, we probably need to continue using Stellar public keys, but for 2.x we could use home domains if we require the clients.domain field for each client.

To clarify, not domains, but client name that's defined in the config (client.name)

From the spec

An identifier for the client application making the request

So I believe using client.name is appropriate here, as initially the idea of introducing client.name was to use it as application identification

Oh, yeah, I was wondering about this and forgot to check the documentation.

I think the name client_id is very confusing though. Should we change to client_account? or client_stellar_account?

I am not familiar with the context of the client.name. Can we add a Jira ticket for discussion?

I have changed the client_id to account.

stellar-jenkins · 2023-08-29T00:45:30Z

Reference Server Preview is available here:
https://anchor-ref-pr1068.previews.kube001.services.stellar-ops.com/
SEP Server Preview is available here:
https://anchor-sep-pr1068.previews.kube001.services.stellar-ops.com/

stellar-jenkins · 2023-08-29T06:27:54Z

Reference Server Preview is available here:
https://anchor-ref-pr1068.previews.kube001.services.stellar-ops.com/
SEP Server Preview is available here:
https://anchor-sep-pr1068.previews.kube001.services.stellar-ops.com/

platform/src/main/java/org/stellar/anchor/platform/component/sep/SepBeans.java

JakeUrban · 2023-08-29T15:53:51Z

core/src/main/java/org/stellar/anchor/sep38/Sep38Service.java

-            .expireAfter(request.getExpireAfter())
-            .clientId(account)
-            .build();
-    GetRateResponse.Rate rate = this.rateIntegration.getRate(getRateRequest).getRate();
+            .expireAfter(request.getExpireAfter());
+
+    // Sets the clientId if the customer exists
+    String clientId = getClientIdFromToken(token);
+    if (clientId != null) {
+      getRateRequestBuilder.clientId(clientId);
+    }
+    GetRateRequest getRateRequest = getRateRequestBuilder.build();


Firm quotes will always be authenticated, so we don't need to call getClientIdFromToken(). If the token is null, we should throw an error, rather than handle it gracefully.

The check of the authentication with the token is always done at the controller. We don't need to worry about them in the service layer. It makes simpler not to worry about authentication in the service layer.

Great, so if the controller makes sure that the token is present, we don't need to check it clientId is null and can just do

getRateRequestBuilder.clientId(token.getAccount())

JakeUrban · 2023-08-29T15:56:29Z

core/src/main/java/org/stellar/anchor/sep38/Sep38Service.java

+  private String getClientIdFromToken(Sep10Jwt token) {
+    if (token == null) return null;
+    return token.getAccount();
+  }


This works until we add support for using SEP-38 prices & quotes for SEP-24 & SEP-6 transactions. If we want to replace the usage of public keys and domains with client names, we can't merge this because it would be a breaking change to change clientId from a public key to names defined in the config. cc @Ifropc

stellar-jenkins · 2023-08-29T16:47:10Z

Reference Server Preview is available here:
https://anchor-ref-pr1068.previews.kube001.services.stellar-ops.com/
SEP Server Preview is available here:
https://anchor-sep-pr1068.previews.kube001.services.stellar-ops.com/

stellar-jenkins · 2023-08-29T16:56:53Z

Reference Server Preview is available here:
https://anchor-ref-pr1068.previews.kube001.services.stellar-ops.com/
SEP Server Preview is available here:
https://anchor-sep-pr1068.previews.kube001.services.stellar-ops.com/

stellar-jenkins · 2023-09-05T18:01:51Z

Reference Server Preview is available here:
https://anchor-ref-pr1068.previews.kube001.services.stellar-ops.com/
SEP Server Preview is available here:
https://anchor-sep-pr1068.previews.kube001.services.stellar-ops.com/

…ssing-client-id

core/src/main/java/org/stellar/anchor/sep31/Sep31Service.java

core/src/main/java/org/stellar/anchor/sep38/Sep38Service.java

core/src/test/kotlin/org/stellar/anchor/sep10/Sep10ServiceTest.kt

core/src/main/java/org/stellar/anchor/sep31/Sep31Service.java

core/src/test/kotlin/org/stellar/anchor/sep31/Sep31ServiceTest.kt

…ssing-client-id

lijamie98

LGTM

lijamie98 · 2023-09-18T20:29:23Z

core/src/main/java/org/stellar/anchor/sep31/Sep31Service.java

+    ClientConfig client = clientsConfig.getClientConfigBySigningKey(account);
+    if (sep10Config.isClientAttributionRequired() && client == null) {
+      throw new BadRequestException("Client not found");
+    }
+    if (client != null &&
+        !sep10Config.getAllowedClientDomains().contains(client.getDomain())) {
+      client = null;
+    }
+    return client == null ? null : client.getName();
+  }
+


Suggested change

ClientConfig client = clientsConfig.getClientConfigBySigningKey(account);

if (sep10Config.isClientAttributionRequired() && client == null) {

throw new BadRequestException("Client not found");

}

if (client != null &&

!sep10Config.getAllowedClientDomains().contains(client.getDomain())) {

client = null;

}

return client == null ? null : client.getName();

}

if (sep10Config.isClientAttributionRequired()) {

if (client == null) {

throw new BadRequestException("Client not found");

}

} else {

// client attribution not required

if (client == null) {

return null;

}

}

return client.getName();

Is this more clear?

I think this is still missing the logic that, even if we find the client, we should still return null if it's not in getAllowedClientDomains()

Ifropc · 2023-09-18T20:33:09Z

core/src/main/java/org/stellar/anchor/config/Sep10Config.java

   *
   * @return the list of allowed client domains.
   */
-  List<String> getClientAttributionAllowList();
+  List<String> getAllowedClientDomains();


Isn't it a breaking change?

This should works just fine. Only the function name changed, the name of the config field staysclientAllowList,

How did you achieve that?

Can you elaborate more on the question? We didn't touch the field clientAllowList in Sep10Config..

The getClientAttributionAllowList returns a list of allowed client domains (which is the intersection of sep10config.clientAllowList and clientsConfig.clients), since we are introducing a new function to return the name of same group of clients, it's better to make the naming

Sorry didn't see SEP10 config is an interface

JakeUrban · 2023-09-18T21:18:02Z

Please don't force-push if avoidable @JiahuiWho

### Description Clean up dead code. All uses has been removed in #1068 ### Context Better engineering

lijamie98 requested review from JakeUrban and JiahuiWho August 28, 2023 23:50

JakeUrban reviewed Aug 28, 2023

View reviewed changes

lijamie98 self-assigned this Aug 29, 2023

lijamie98 added 4 commits August 28, 2023 23:21

Add CLIENT_ID tests

aade8cd

Make sep10 jwt optional for getPrice

ea9fa79

empty

74df9f7

Use token account for client_id

e26aaa1

lijamie98 force-pushed the feature/anchor-380-missing-client-id branch from 827b9de to e26aaa1 Compare August 29, 2023 06:25

JakeUrban requested changes Aug 29, 2023

View reviewed changes

revert client_id to token's account

5a5f295

revert client_id to token's account #2

ca14b8a

Merge remote-tracking branch 'sdf/develop' into HEAD

9a7f42d

lijamie98 and others added 7 commits September 8, 2023 08:47

Merge remote-tracking branch 'sdf/develop' into feature/anchor-380-mi…

138b3e3

…ssing-client-id

remove unused isKnownCustodialAccountRequired from sep10

f4c5cf7

Merge remote-tracking branch 'sdf/develop' into feature/anchor-380-mi…

c8eb230

…ssing-client-id

Add clientId to Sep31Service

0594b16

add unit test for sep31 getClientName()

f2137a9

fix Sep31DepositInfoGeneratorTest

1c51429

Fix the stellar validation tests error

a214b5b

philipliu reviewed Sep 13, 2023

View reviewed changes

lijamie98 changed the title ~~[ANCHOR-380] Add client_id to getRate() when possible~~ [ANCHOR-380] Add client_id to getRate() in Sep31Service Sep 13, 2023

wip

316a9a8

JiahuiWho added 2 commits September 14, 2023 12:04

wip

879d262

empty

af52a59

JiahuiWho force-pushed the feature/anchor-380-missing-client-id branch from 1ec1bed to af52a59 Compare September 14, 2023 20:15

lijamie98 force-pushed the feature/anchor-380-missing-client-id branch from af52a59 to 879d262 Compare September 14, 2023 20:43

JiahuiWho added 2 commits September 18, 2023 13:42

empty

4bcd37b

Merge remote-tracking branch 'sdf/develop' into feature/anchor-380-mi…

b56df33

…ssing-client-id

lijamie98 commented Sep 18, 2023

View reviewed changes

JiahuiWho force-pushed the feature/anchor-380-missing-client-id branch from 343c9c1 to b56df33 Compare September 18, 2023 19:05

lijamie98 assigned JiahuiWho and unassigned lijamie98 Sep 18, 2023

getClientName() should return null if client is not in clientAllowList

f1b2578

lijamie98 commented Sep 18, 2023

View reviewed changes

Ifropc reviewed Sep 18, 2023

View reviewed changes

JiahuiWho force-pushed the feature/anchor-380-missing-client-id branch from 5542c6c to f1b2578 Compare September 18, 2023 20:39

allow client_id to be null in GetFeeRequest

9555c0f

JiahuiWho approved these changes Sep 18, 2023

View reviewed changes

JakeUrban approved these changes Sep 18, 2023

View reviewed changes

Ifropc approved these changes Sep 18, 2023

View reviewed changes

JiahuiWho merged commit b53c3f3 into stellar:develop Sep 18, 2023
5 checks passed

JiahuiWho mentioned this pull request Feb 26, 2024

[Dead Code] Clean up isKnownCustodialAccountRequired #1273

Merged

JiahuiWho added a commit that referenced this pull request Feb 27, 2024

[Dead Code] Clean up isKnownCustodialAccountRequired (#1273)

ccc7840

### Description Clean up dead code. All uses has been removed in #1068 ### Context Better engineering

JiahuiWho deleted the feature/anchor-380-missing-client-id branch July 31, 2024 19:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ANCHOR-380] Add client_id to `getRate()` in `Sep31Service` #1068

[ANCHOR-380] Add client_id to `getRate()` in `Sep31Service` #1068

lijamie98 commented Aug 28, 2023

stellar-jenkins commented Aug 28, 2023

stellar-jenkins commented Aug 28, 2023

JakeUrban Aug 28, 2023

JakeUrban Aug 28, 2023

Ifropc Aug 29, 2023 •

edited

Loading

Ifropc Aug 29, 2023

lijamie98 Aug 29, 2023

lijamie98 Aug 29, 2023

lijamie98 Aug 29, 2023

stellar-jenkins commented Aug 29, 2023

stellar-jenkins commented Aug 29, 2023

JakeUrban Aug 29, 2023

lijamie98 Aug 29, 2023

JakeUrban Aug 29, 2023

JakeUrban Aug 29, 2023

stellar-jenkins commented Aug 29, 2023

stellar-jenkins commented Aug 29, 2023

stellar-jenkins commented Sep 5, 2023

lijamie98 left a comment

lijamie98 Sep 18, 2023

JiahuiWho Sep 18, 2023

Ifropc Sep 18, 2023

JiahuiWho Sep 18, 2023

Ifropc Sep 18, 2023

JiahuiWho Sep 18, 2023

Ifropc Sep 18, 2023

JakeUrban commented Sep 18, 2023

[ANCHOR-380] Add client_id to getRate() in Sep31Service #1068

[ANCHOR-380] Add client_id to getRate() in Sep31Service #1068

Conversation

lijamie98 commented Aug 28, 2023

Description

Context

Testing

stellar-jenkins commented Aug 28, 2023

stellar-jenkins commented Aug 28, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ifropc Aug 29, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stellar-jenkins commented Aug 29, 2023

stellar-jenkins commented Aug 29, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stellar-jenkins commented Aug 29, 2023

stellar-jenkins commented Aug 29, 2023

stellar-jenkins commented Sep 5, 2023

lijamie98 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JakeUrban commented Sep 18, 2023

[ANCHOR-380] Add client_id to `getRate()` in `Sep31Service` #1068

[ANCHOR-380] Add client_id to `getRate()` in `Sep31Service` #1068

Ifropc Aug 29, 2023 •

edited

Loading