[ANCNHOR-626] Update ClientsConfig with SigningKeys and Domains #1355

JiahuiWho · 2024-05-08T07:49:54Z

Description

Deprecate signing_key and domain, replace with signing_keys and domains
Remove signing_key field for noncustodial client (usage only found in validation)
Replace the use of signing_key and domain with signing_keys and domains in related function and test cases

Testing

./gradlew test

Documentation

Will update in https://developers.stellar.org/network/anchor-platform/admin-guide/sep10

core/src/main/java/org/stellar/anchor/client/ClientFinder.java

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

platform/src/main/java/org/stellar/anchor/platform/config/PropertyClientsConfig.java

platform/src/test/kotlin/org/stellar/anchor/platform/config/PropertyClientsConfigTest.kt

core/src/main/java/org/stellar/anchor/client/ClientFinder.java

platform/src/main/java/org/stellar/anchor/platform/event/EventProcessorManager.java

lijamie98 · 2024-05-10T01:24:41Z

Can we add tests that has multiple signing_keys and multiple domains?

JiahuiWho · 2024-05-10T02:44:06Z

Can we add tests that has multiple signing_keys and multiple domains?

Yes I have changed PropertyClientsConfigTest.kt to use signingKeys and domains for testing, in this way we can test with multiple values

I will add tests for getClientConfigBySigningKey and getClientConfigByDomain to validate it

Ifropc · 2024-05-09T22:00:49Z

core/src/main/java/org/stellar/anchor/client/ClientFinder.java

@@ -42,7 +43,7 @@ public String getClientName(String clientDomain, String account)
      throw new SepNotAuthorizedException("Client not found");
    }

-    if (clientDomain != null && !isDomainAllowed(client.getDomain())) {
+    if (clientDomain != null && !isClientDomainsAllowed(client)) {


This check looks like a legacy.
We define allowed clients in the clientAllowList (which allows all clients by default), and based on allowed clients we get allowed domains (using configured clients).
So, this will always be matching the check below, because allowed domains are gathered from allowed client. So it will always be true if client name is allowed and false otherwise.

You are right, but only if we know the client is in clientAllowList for sure. I saw the only call site is the Sep10Service, where the account and domain were extracted from request and passed down here, but does it guarantee the client find is allowed?

If this is the case we should also either move or rename the function indicating this is for Sep10 use only, ClientFinder is just too general and the validation should be there for other cases.

If it's not in clientAllowList, wouldn't isClientNameAllowed also be false?

I see. This check is redundant, we only need to check the name.

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

platform/src/main/java/org/stellar/anchor/platform/config/PropertyClientsConfig.java

platform/src/main/java/org/stellar/anchor/platform/config/PropertySep10Config.java

Ifropc

LGTM!
If you could, do you mind adding tests to check backward compatibility? (i.e. checking that when domain is defined, domains is automatically set to set of one domain and same for custodial key)
Just to ensure backward compatibility is covered by the test

Ifropc · 2024-05-13T20:54:32Z

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

@@ -12,8 +12,10 @@ public interface ClientsConfig {
  class ClientConfig {
    String name;
    ClientType type;
-    String signingKey;
-    String domain;
+    @Deprecated String signingKey; // ANCHOR-696


lijamie98

LGTM

JiahuiWho · 2024-05-13T22:01:05Z

LGTM! If you could, do you mind adding tests to check backward compatibility? (i.e. checking that when domain is defined, domains is automatically set to set of one domain and same for custodial key) Just to ensure backward compatibility is covered by the test

Yeah test for postConstruct() is added to PropertyClientsConfigTest

Ifropc · 2024-05-14T03:33:08Z

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

@@ -596,9 +591,9 @@ String getClientName(String account) throws BadRequestException {
    if (sep10Config.isClientAttributionRequired() && client == null) {
      throw new BadRequestException("Client not found");
    }
-    if (client != null && !sep10Config.getAllowedClientDomains().contains(client.getDomain())) {
+    System.out.println(Arrays.toString(sep10Config.getAllowedClientNames().toArray()));


sout should be removed @JiahuiWho

### Description [[ANCNHOR-626] Update ClientsConfig with SigningKeys and Domains](#1355) introduced change to ClientsConfig, merge develop to bring feature/v3 up to date ### Context Merge `develop` into `feature/v3` ### Testing - `./gradlew test` --------- Co-authored-by: Philip Liu <[email protected]> Co-authored-by: Jamie Li <[email protected]> Co-authored-by: Kanwalpreet Dhindsa <[email protected]> Co-authored-by: Gleb <[email protected]>

JiahuiWho added 2 commits May 8, 2024 15:40

update clients config

dab99af

remove clients config prefix

524120e

JiahuiWho changed the title ~~[ANCNHOR-626] Update ClientsConfig~~ [ANCNHOR-626] Update ClientsConfig with SigningKeys and Domains May 9, 2024

JiahuiWho marked this pull request as ready for review May 9, 2024 19:59

philipliu reviewed May 9, 2024

View reviewed changes

lijamie98 reviewed May 10, 2024

View reviewed changes

core/src/main/java/org/stellar/anchor/client/ClientFinder.java Outdated Show resolved Hide resolved

platform/src/main/java/org/stellar/anchor/platform/event/EventProcessorManager.java Outdated Show resolved Hide resolved

address comments

1435c0f

Ifropc reviewed May 10, 2024

View reviewed changes

JiahuiWho added 2 commits May 13, 2024 10:26

add more validation rule

e9ef4ef

remove redundant domain check

330f1a0

philipliu approved these changes May 13, 2024

View reviewed changes

Ifropc reviewed May 13, 2024

View reviewed changes

Ifropc approved these changes May 13, 2024

View reviewed changes

lijamie98 approved these changes May 13, 2024

View reviewed changes

JiahuiWho and others added 2 commits May 13, 2024 18:36

Merge branch 'develop' into anchor-626-update-clients-config

060ca01

fix tests

fde1806

JiahuiWho merged commit f46d75a into stellar:develop May 13, 2024
8 checks passed

JiahuiWho deleted the anchor-626-update-clients-config branch May 13, 2024 23:35

Ifropc reviewed May 14, 2024

View reviewed changes

This was referenced Jun 4, 2024

[CHORE] Merge develop into feature/v3 #1378

Closed

[CHORE] Merge develop into feature/v3 #1379

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ANCNHOR-626] Update ClientsConfig with SigningKeys and Domains #1355

[ANCNHOR-626] Update ClientsConfig with SigningKeys and Domains #1355

JiahuiWho commented May 8, 2024 •

edited

Loading

lijamie98 commented May 10, 2024

JiahuiWho commented May 10, 2024

Ifropc May 9, 2024

JiahuiWho May 13, 2024

Ifropc May 13, 2024

JiahuiWho May 13, 2024 •

edited

Loading

Ifropc left a comment

Ifropc May 13, 2024

lijamie98 left a comment

JiahuiWho commented May 13, 2024

Ifropc May 14, 2024

[ANCNHOR-626] Update ClientsConfig with SigningKeys and Domains #1355

[ANCNHOR-626] Update ClientsConfig with SigningKeys and Domains #1355

Conversation

JiahuiWho commented May 8, 2024 • edited Loading

Description

Testing

Documentation

lijamie98 commented May 10, 2024

JiahuiWho commented May 10, 2024

Ifropc May 9, 2024

Choose a reason for hiding this comment

JiahuiWho May 13, 2024

Choose a reason for hiding this comment

Ifropc May 13, 2024

Choose a reason for hiding this comment

JiahuiWho May 13, 2024 • edited Loading

Choose a reason for hiding this comment

Ifropc left a comment

Choose a reason for hiding this comment

Ifropc May 13, 2024

Choose a reason for hiding this comment

lijamie98 left a comment

Choose a reason for hiding this comment

JiahuiWho commented May 13, 2024

Ifropc May 14, 2024

Choose a reason for hiding this comment

JiahuiWho commented May 8, 2024 •

edited

Loading

JiahuiWho May 13, 2024 •

edited

Loading