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

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

Merged
merged 7 commits into from
May 13, 2024
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
package org.stellar.anchor.client;

import java.util.HashSet;
import javax.annotation.Nullable;
import lombok.NonNull;
import lombok.RequiredArgsConstructor;
Expand Down Expand Up @@ -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)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@JiahuiWho JiahuiWho May 13, 2024

Choose a reason for hiding this comment

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

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

throw new SepNotAuthorizedException("Client domain not allowed");
}
if (!isClientNameAllowed(client.getName())) {
Expand All @@ -64,6 +65,11 @@ private ClientConfig getClient(String clientDomain, String account) {
return clientByDomain != null ? clientByDomain : clientByAccount;
}

private boolean isClientDomainsAllowed(ClientConfig client) {
JiahuiWho marked this conversation as resolved.
Show resolved Hide resolved
return new HashSet<>(sep10Config.getAllowedClientDomains()).containsAll(client.getDomains())
|| sep10Config.getAllowedClientDomains().isEmpty();
}

private boolean isDomainAllowed(String domain) {
JiahuiWho marked this conversation as resolved.
Show resolved Hide resolved
return sep10Config.getAllowedClientDomains().contains(domain)
|| sep10Config.getAllowedClientDomains().isEmpty();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,10 @@ public interface ClientsConfig {
class ClientConfig {
String name;
ClientType type;
String signingKey;
String domain;
@Deprecated String signingKey;
JiahuiWho marked this conversation as resolved.
Show resolved Hide resolved
Set<String> signingKeys;
@Deprecated String domain;
Set<String> domains;
String callbackUrl;
boolean allowAnyDestination = false;
Set<String> destinationAccounts;
Expand Down
16 changes: 8 additions & 8 deletions core/src/main/java/org/stellar/anchor/sep31/Sep31Service.java
Original file line number Diff line number Diff line change
Expand Up @@ -24,12 +24,7 @@
import io.micrometer.core.instrument.Counter;
import java.math.BigDecimal;
import java.time.Instant;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.Objects;
import java.util.Optional;
import java.util.UUID;
import java.util.*;
import java.util.stream.Collectors;
import javax.transaction.Transactional;
import lombok.Data;
Expand Down Expand Up @@ -596,8 +591,13 @@ 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())) {
client = null;

if (client != null && client.getDomains() != null) {
JiahuiWho marked this conversation as resolved.
Show resolved Hide resolved
boolean hasDomainIntersection =
new HashSet<>(sep10Config.getAllowedClientDomains()).containsAll(client.getDomains());
if (!hasDomainIntersection) {
client = null;
}
}
return client == null ? null : client.getName();
}
Expand Down
29 changes: 21 additions & 8 deletions core/src/test/kotlin/org/stellar/anchor/sep31/Sep31ServiceTest.kt
Original file line number Diff line number Diff line change
Expand Up @@ -275,8 +275,10 @@ class Sep31ServiceTest {
ClientsConfig.ClientConfig(
"lobstr",
ClientsConfig.ClientType.NONCUSTODIAL,
"GBLGJA4TUN5XOGTV6WO2BWYUI2OZR5GYQ5PDPCRMQ5XEPJOYWB2X4CJO",
"lobstr.co",
null,
setOf("GBLGJA4TUN5XOGTV6WO2BWYUI2OZR5GYQ5PDPCRMQ5XEPJOYWB2X4CJO"),
null,
setOf("lobstr.co"),
"https://callback.lobstr.co/api/v2/anchor/callback",
false,
null
Expand All @@ -287,8 +289,18 @@ class Sep31ServiceTest {
return Stream.of(
Arguments.of(listOf<String>(), false, null, false),
Arguments.of(listOf<String>(), true, null, true),
Arguments.of(listOf(lobstrClientConfig.domain), false, lobstrClientConfig.name, false),
Arguments.of(listOf(lobstrClientConfig.domain), true, lobstrClientConfig.name, true),
Arguments.of(
listOf(lobstrClientConfig.domains.first()),
false,
lobstrClientConfig.name,
false
),
Arguments.of(
listOf(lobstrClientConfig.domains.first()),
true,
lobstrClientConfig.name,
true
),
)
}
}
Expand Down Expand Up @@ -789,7 +801,7 @@ class Sep31ServiceTest {
every { sep10Config.allowedClientDomains } returns listOf("vibrant.stellar.org")
every { clientsConfig.getClientConfigBySigningKey(any()) } returns
ClientsConfig.ClientConfig().apply {
domain = "vibrant.stellar.org"
domains = setOf("vibrant.stellar.org")
name = "vibrant"
}

Expand Down Expand Up @@ -1095,11 +1107,12 @@ class Sep31ServiceTest {
) {
every { sep10Config.allowedClientDomains } returns allowedClientDomains
every { sep10Config.isClientAttributionRequired } returns isClientAttributionRequired
every { clientsConfig.getClientConfigBySigningKey(lobstrClientConfig.signingKey) } returns
lobstrClientConfig
every {
clientsConfig.getClientConfigBySigningKey(lobstrClientConfig.signingKeys.first())
} returns lobstrClientConfig

// client name should be returned for valid input
val clientName = sep31Service.getClientName(lobstrClientConfig.signingKey)
val clientName = sep31Service.getClientName(lobstrClientConfig.signingKeys.first())
assertEquals(expectedClientName, clientName)

// exception maybe thrown for invalid input
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,10 @@ class ClientFinderTest {
ClientsConfig.ClientConfig(
"name",
ClientsConfig.ClientType.CUSTODIAL,
"signing-key",
"domain",
null,
setOf("signing-key"),
null,
setOf("domain"),
"http://localhost:8000",
false,
emptySet()
Expand All @@ -41,7 +43,7 @@ class ClientFinderTest {
fun setup() {
MockKAnnotations.init(this, relaxUnitFun = true)
every { sep10Config.isClientAttributionRequired } returns true
every { sep10Config.allowedClientDomains } returns listOf(clientConfig.domain)
every { sep10Config.allowedClientDomains } returns clientConfig.domains.toList()
every { sep10Config.allowedClientNames } returns listOf(clientConfig.name)
every {
clientsConfig.getClientConfigByDomain(ExchangeAmountsCalculatorTest.token.clientDomain)
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
package org.stellar.anchor.platform.config;

import static io.jsonwebtoken.lang.Collections.setOf;
import static org.stellar.anchor.util.Log.debugF;
import static org.stellar.anchor.util.StringHelper.isEmpty;

Expand All @@ -9,13 +10,12 @@
import java.net.URL;
import java.util.List;
import java.util.Map;
import javax.annotation.PostConstruct;
import lombok.Data;
import org.jetbrains.annotations.NotNull;
import org.springframework.validation.Errors;
import org.springframework.validation.Validator;
import org.stellar.anchor.api.exception.SepException;
import org.stellar.anchor.config.ClientsConfig;
import org.stellar.anchor.sep10.Sep10Helper;

@Data
public class PropertyClientsConfig implements ClientsConfig, Validator {
Expand All @@ -24,14 +24,32 @@ public class PropertyClientsConfig implements ClientsConfig, Validator {
Map<String, String> domainToClientNameMap = null;
Map<String, String> signingKeyToClientNameMap = null;

@PostConstruct
public void postConstruct() {
clients.forEach(
JiahuiWho marked this conversation as resolved.
Show resolved Hide resolved
clientConfig -> {
if (!isEmpty(clientConfig.getSigningKey())
&& (clientConfig.getSigningKeys() == null
|| clientConfig.getSigningKeys().isEmpty())) {
clientConfig.setSigningKeys(setOf(clientConfig.getSigningKey()));
}
if (!isEmpty(clientConfig.getDomain())
&& (clientConfig.getDomains() == null || clientConfig.getDomains().isEmpty())) {
clientConfig.setDomains(setOf(clientConfig.getDomain()));
}
});
}

@Override
public ClientsConfig.ClientConfig getClientConfigBySigningKey(String signingKey) {
if (signingKeyToClientNameMap == null) {
signingKeyToClientNameMap = Maps.newHashMap();
clients.forEach(
clientConfig -> {
if (clientConfig.getSigningKey() != null) {
signingKeyToClientNameMap.put(clientConfig.getSigningKey(), clientConfig.getName());
if (clientConfig.getSigningKeys() != null && !clientConfig.getSigningKeys().isEmpty()) {
JiahuiWho marked this conversation as resolved.
Show resolved Hide resolved
for (String key : clientConfig.getSigningKeys()) {
signingKeyToClientNameMap.put(key, clientConfig.getName());
}
}
});
}
Expand All @@ -44,8 +62,10 @@ public ClientConfig getClientConfigByDomain(String domain) {
domainToClientNameMap = Maps.newHashMap();
clients.forEach(
clientConfig -> {
if (clientConfig.getDomain() != null) {
domainToClientNameMap.put(clientConfig.getDomain(), clientConfig.getName());
if (clientConfig.getDomains() != null && !clientConfig.getDomains().isEmpty()) {
JiahuiWho marked this conversation as resolved.
Show resolved Hide resolved
for (String domainName : clientConfig.getDomains()) {
domainToClientNameMap.put(domainName, clientConfig.getName());
}
}
});
}
Expand Down Expand Up @@ -84,9 +104,11 @@ private void validateClient(ClientConfig clientConfig, Errors errors) {
}

public void validateCustodialClient(ClientConfig clientConfig, Errors errors) {
if (isEmpty(clientConfig.getSigningKey())) {
if (isEmpty(clientConfig.getSigningKey())
JiahuiWho marked this conversation as resolved.
Show resolved Hide resolved
&& (clientConfig.getSigningKeys() == null || clientConfig.getSigningKeys().isEmpty())) {
errors.reject(
"empty-client-signing-key", "The client.signingKey cannot be empty and must be defined");
"empty-client-signing-keys",
"The client.signingKeys cannot be empty and must be defined");
}
if (!isEmpty(clientConfig.getCallbackUrl())) {
try {
Expand All @@ -98,24 +120,10 @@ public void validateCustodialClient(ClientConfig clientConfig, Errors errors) {
}

public void validateNonCustodialClient(ClientConfig clientConfig, Errors errors) {
if (isEmpty(clientConfig.getDomain())) {
errors.reject("empty-client-domain", "The client.domain cannot be empty and must be defined");
}

if (!isEmpty(clientConfig.getSigningKey())) {
try {
String clientSigningKey =
Sep10Helper.fetchSigningKeyFromClientDomain(clientConfig.getDomain(), false);
if (!clientConfig.getSigningKey().equals(clientSigningKey)) {
errors.reject(
"client-signing-key-does-not-match",
"The client.signingKey does not matched any valid registered keys");
}
} catch (SepException e) {
errors.reject(
"client-signing-key-toml-read-failure",
"SIGNING_KEY not present in 'client_domain' TOML or TOML file does not exist");
}
if (isEmpty(clientConfig.getDomain())
&& (clientConfig.getDomains() == null || clientConfig.getDomains().isEmpty())) {
errors.reject(
JiahuiWho marked this conversation as resolved.
Show resolved Hide resolved
"empty-client-domains", "The client.domains cannot be empty and must be defined");
}

if (!isEmpty(clientConfig.getCallbackUrl())) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -45,9 +45,8 @@ public PropertySep10Config(
this.secretConfig = secretConfig;
this.knownCustodialAccountList =
clientsConfig.getClients().stream()
.filter(
cfg -> cfg.getType() == CUSTODIAL && StringHelper.isNotEmpty(cfg.getSigningKey()))
.map(ClientConfig::getSigningKey)
.filter(cfg -> cfg.getType() == CUSTODIAL && !cfg.getSigningKeys().isEmpty())
.flatMap(cfg -> cfg.getSigningKeys().stream())
.collect(Collectors.toList());
}

Expand Down Expand Up @@ -170,7 +169,7 @@ void validateClientAttribution(Errors errors) {
if (clientAttributionRequired) {
List<String> nonCustodialClientNames =
clientsConfig.clients.stream()
.filter(cfg -> cfg.getType() == NONCUSTODIAL && isNotEmpty(cfg.getDomain()))
.filter(cfg -> cfg.getType() == NONCUSTODIAL)
.map(ClientConfig::getName)
.collect(Collectors.toList());

Expand Down Expand Up @@ -230,18 +229,18 @@ public List<String> getAllowedClientDomains() {
// if clientAllowList is not defined, all client domains from the clients section are allowed.
if (clientAllowList == null || clientAllowList.isEmpty()) {
return clientsConfig.clients.stream()
.map(ClientConfig::getDomain)
.filter(StringHelper::isNotEmpty)
.filter(cfg -> cfg.getDomains() != null && !cfg.getDomains().isEmpty())
.flatMap(cfg -> cfg.getDomains().stream())
.collect(Collectors.toList());
}

// If clientAllowList is defined, only the clients in the allow list are allowed.
return clientAllowList.stream()
.map(
.flatMap(
JiahuiWho marked this conversation as resolved.
Show resolved Hide resolved
domain ->
(clientsConfig.getClientConfigByName(domain) == null)
? null
: clientsConfig.getClientConfigByName(domain).getDomain())
: clientsConfig.getClientConfigByName(domain).getDomains().stream())
.filter(StringHelper::isNotEmpty)
.collect(Collectors.toList());
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -93,11 +93,13 @@ public void start() {
switch (clientConfig.getType()) {
case CUSTODIAL:
processorName =
CLIENT_STATUS_CALLBACK_EVENT_PROCESSOR_NAME_PREFIX + clientConfig.getSigningKey();
CLIENT_STATUS_CALLBACK_EVENT_PROCESSOR_NAME_PREFIX
+ clientConfig.getSigningKeys().stream().findFirst();
JiahuiWho marked this conversation as resolved.
Show resolved Hide resolved
break;
case NONCUSTODIAL:
processorName =
CLIENT_STATUS_CALLBACK_EVENT_PROCESSOR_NAME_PREFIX + clientConfig.getDomain();
CLIENT_STATUS_CALLBACK_EVENT_PROCESSOR_NAME_PREFIX
+ clientConfig.getDomains().stream().findFirst();
break;
default:
errorF("Unknown client type: {}", clientConfig.getType());
Expand Down
Loading
Loading