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.

Sign up for GitHub

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

Jump to bottom

[PAYMTS-1728] Add account updater look up service #442

Open
wants to merge 12 commits into
base: master
Choose a base branch
from
Open

[PAYMTS-1728] Add account updater look up service #442

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
50f16cb
Add account updater service
odovzhenko Jan 17, 2024
d2493a1
Rename stuff
fangpenlin Jan 23, 2024
29f6534
Add client id and client secret
fangpenlin Jan 23, 2024
af3eeb8
Add missing params
fangpenlin Jan 23, 2024
cff9baf
Rename stuff
fangpenlin Jan 23, 2024
a45e4db
Add tests
fangpenlin Jan 23, 2024
696e272
Make tests work
fangpenlin Jan 23, 2024
b3b973f
Fix broken tests
fangpenlin Jan 23, 2024
e8d0c86
Rename number parameter to PAN
fangpenlin Jan 23, 2024
f39a810
Add error msg
fangpenlin Jan 23, 2024
3593ae6
Move module under vgs pkg
fangpenlin Jan 23, 2024
00fd708
Add missing header checker
fangpenlin Jan 23, 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
2 changes: 2 additions & 0 deletions larky/src/main/java/com/verygood/security/larky/ModuleSupplier.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableSet;

import com.verygood.security.larky.modules.AccountUpdaterModule;
fangpenlin marked this conversation as resolved.
Show resolved Hide resolved
import com.verygood.security.larky.modules.BinasciiModule;
import com.verygood.security.larky.modules.C99MathModule;
import com.verygood.security.larky.modules.NetworkTokenModule;
Expand Down Expand Up @@ -98,6 +99,7 @@ public class ModuleSupplier {
public static final ImmutableSet<StarlarkValue> VGS_MODULES = ImmutableSet.of(
VaultModule.INSTANCE,
NetworkTokenModule.INSTANCE,
AccountUpdaterModule.INSTANCE,
CerebroModule.INSTANCE,
ChaseModule.INSTANCE,
JKSModule.INSTANCE
Expand Down
123 changes: 123 additions & 0 deletions larky/src/main/java/com/verygood/security/larky/modules/AccountUpdaterModule.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,123 @@
package com.verygood.security.larky.modules;

import com.google.common.collect.ImmutableList;
import com.verygood.security.larky.modules.vgs.aus.LarkyAccountUpdater;
import com.verygood.security.larky.modules.vgs.aus.MockAccountUpdaterService;
import com.verygood.security.larky.modules.vgs.aus.NoopAccountUpdaterService;
import com.verygood.security.larky.modules.vgs.aus.spi.AccountUpdaterService;
import java.util.List;
import java.util.ServiceLoader;
import net.starlark.java.annot.Param;
import net.starlark.java.annot.ParamType;
import net.starlark.java.annot.StarlarkBuiltin;
import net.starlark.java.annot.StarlarkMethod;
import net.starlark.java.eval.*;

@StarlarkBuiltin(
name = "native_au",
category = "BUILTIN",
doc = "Overridable Account Updater API in Larky")
public class AccountUpdaterModule implements LarkyAccountUpdater {
public static final AccountUpdaterModule INSTANCE = new AccountUpdaterModule();

public static final String ENABLE_MOCK_PROPERTY =
"larky.modules.vgs.nts.enableMockAccountUpdater";

private final AccountUpdaterService AccountUpdaterService;

public AccountUpdaterModule() {
final ServiceLoader<AccountUpdaterService> loader =
ServiceLoader.load(AccountUpdaterService.class);
final List<AccountUpdaterService> AccountUpdaterProviders =
ImmutableList.copyOf(loader.iterator());

if (Boolean.getBoolean(ENABLE_MOCK_PROPERTY)) {
AccountUpdaterService = new MockAccountUpdaterService();
} else if (AccountUpdaterProviders.isEmpty()) {
AccountUpdaterService = new NoopAccountUpdaterService();
} else {
if (AccountUpdaterProviders.size() != 1) {
throw new IllegalArgumentException(
String.format(
"AccountUpdaterModule expecting only 1 network token provider of type AccountUpdaterService, found %d",
AccountUpdaterProviders.size()));
}
AccountUpdaterService = AccountUpdaterProviders.get(0);
}
}

@StarlarkMethod(
name = "lookup_card",
Copy link
Collaborator

Choose a reason for hiding this comment

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

should this change to lookup PAN instead of lookup card to match the documentation?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think lookup card makes more sense as the updated info could be just expiring month or year as well, not just PAN. So more accurately, we are looking up a whole card's update info.

doc = "Lookup account updates for a given PAN.",
useStarlarkThread = true,
parameters = {
@Param(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are all of these parameters required? If we pass None, this code will throw an error. Is this the desired behavior?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yeah, those are meant to be required, so passing None will result in error, that's desired behavior

name = "number",
named = true,
doc = "Card PAN. Used to look up the corresponding card information to be returned",
allowedTypes = {@ParamType(type = String.class)}),
@Param(
name = "exp_month",
named = true,
doc =
"Card expiration month. Used to pass to the network for retrieving the corresponding card information",
allowedTypes = {@ParamType(type = StarlarkInt.class)}),
@Param(
name = "exp_year",
named = true,
doc =
"Card expiration year. Used to pass to the network for retrieving the corresponding card information",
allowedTypes = {@ParamType(type = StarlarkInt.class)}),
@Param(
name = "name",
named = true,
doc =
"Card owner name. Used to pass to the network for retrieving the corresponding card information",
allowedTypes = {@ParamType(type = String.class)}),
@Param(
Copy link
Collaborator

Choose a reason for hiding this comment

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

For me, this immediately stood out as 'hmm.' as the credentials are passed in via method call. Is this what we are intending?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, that's what we are intending.

This is for our needs in payments team to make it possible for our customer to get the latest card number from account updater service (provided by visa or mastercard). And to achieve that, we need to make a call to public API of calm. I envisioned this could be used like this

if use_account_updater(input.headers):
    card = aus.lookup_card(
        pan=body.cardNumber,
        exp_month=body.expMon,
        exp_year=body.expYear,
        name=body.name,
        client_id="<CALM SERVICE ACCOUNT CLIENT ID>",
        client_secret="<CALM SERVICE ACCOUNT CLIENT SECRET>",
    )
    if card is not None and card.number != input.cardNumber or ...:
        input.cardNumber = card.number
        # update card
# proceed to PSP with or without new card info

As you can see the challenge here is to authenticate with calm API server. The easiest way right now is to hardwire the service account credentials in the larky script. I know this may not be the best way in the world to authenticate to our own API service in VGS, but considering we need this feature in short time, it's the best way. If there's a concern for hardwiring the credentials in the larky script, we can actually ask the merchant to put their service account credentials in the vault and use reveal function to get the actual credential secret values. Like this

card = aus.lookup_card(
     # ...
    client_id=vault.reveal("<CALM SERVICE ACCOUNT CLIENT ID>"),
    client_secret=vault.reveal("<CALM SERVICE ACCOUNT CLIENT SECRET>"),
)

And if we don't want the possibility of merchant putting their service account credentials in larky script and force them to use vault, we can even reveal the secret in module and reject the request if the provided values are not alias.

In a perfect world, if we have enough time to build this feature and some major changes to vault is allowed, I was thinking about providing something like OIDC auth token like the ones from Kubernetes pod for authenticating with Calm server. Since the larky script environment is kind of already a trusted environment, I don't see why we need to authenticate again. So requests coming from larky can be trusted as long as there's a policy says that we trust calm API requests coming from our own vault proxy. Then the API call can be simplified as just

card = aus.lookup_card(
    pan=body.cardNumber,
    exp_month=body.expMon,
    exp_year=body.expYear,
    name=body.name,
)

The account updater module will create a OIDC auth token for making the request and calm can authenticate and accept that automatically based on a pre-defined policy.

Regardless, that's too much effort for now, as we want this feature to be done ASAP, provides if keeping calm's service account credentials in vault (or even hardwiring in larky script) is acceptable, I think this is the best approach for now.

Copy link

Choose a reason for hiding this comment

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

To me the ability to use account updater should be done through a module. I don't feel that we should be using credentials like this for service to service calls. Here is my thinking.

Create a Vault Preference called "Account Updater" that is default false.
If that preference is false - horizon, simply does not allow the call to the service be completed.
If that preference is true - it goes through

The preference can only be set by someone/something with permissions to do so. ( this is something that the new Vault Management spec intends to support)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

so it sounds like we only want to enable this feature for customers who really need it, is my understanding correct?

is this vault preference feature something we can use right now, or is this a new feature and not yet implemented? I did a quick search in the code base and cannot find something looks like vault preference. If it's already there, can you please point that to me where's it and if there's an example of it?

Copy link

Choose a reason for hiding this comment

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

Yes, it's there... @sharifelgamal or @Iapetus999 can point you in the right direction. We made it so that preferences for the vault are available in horizon. Adding a new preference - 🤷🏼 not sure about. but I am going to hope it can be quick to add one🤞🏼 .

Copy link
Contributor

Choose a reason for hiding this comment

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

note that we never actually added the ability to read preferences from horizon, it would involve the proxy (or operation pipeline as it were) passing in the preference as a header into larky

name = "client_id",
named = true,
doc = "Client ID of service account to access calm API server",
allowedTypes = {@ParamType(type = String.class)}),
@Param(
name = "client_secret",
named = true,
doc = "Client secret of service account to access calm API server",
allowedTypes = {@ParamType(type = String.class)}),
})
@Override
public Object lookupCard(
String number,
StarlarkInt expireMonth,
StarlarkInt expireYear,
String name,
String clientId,
String clientSecret,
StarlarkThread thread)
throws EvalException {
if (number.trim().isEmpty()) {
throw Starlark.errorf("card number argument cannot be blank");
}
final AccountUpdaterService.Card card;
try {
card =
AccountUpdaterService.lookupCard(
number,
expireMonth.toInt("invalid int range"),
fangpenlin marked this conversation as resolved.
Show resolved Hide resolved
expireYear.toInt("invalid int range"),
fangpenlin marked this conversation as resolved.
Show resolved Hide resolved
name,
clientId,
clientSecret);
} catch (UnsupportedOperationException exception) {
throw Starlark.errorf("aus.lookup_card operation must be overridden");
}
if (card == null) {
return Starlark.NONE;
}
return Dict.<String, Object>builder()
.put("number", card.getNumber())
.put("exp_month", StarlarkInt.of(card.getExpireMonth()))
.put("exp_year", StarlarkInt.of(card.getExpireYear()))
.build(thread.mutability());
}
}
31 changes: 31 additions & 0 deletions larky/src/main/java/com/verygood/security/larky/modules/vgs/aus/LarkyAccountUpdater.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
package com.verygood.security.larky.modules.vgs.aus;

import net.starlark.java.eval.Dict;
import net.starlark.java.eval.EvalException;
import net.starlark.java.eval.StarlarkInt;
import net.starlark.java.eval.StarlarkThread;
import net.starlark.java.eval.StarlarkValue;

public interface LarkyAccountUpdater extends StarlarkValue {
/**
* Get updated info for the provided card
*
* @param number card's number
* @param expireMonth card's expiration month
* @param expireYear card's expiration year
* @param name the name on the card
* @param clientId client id of service account to access calm API
* @param clientSecret client secret of service account to access calm API
* @param thread Starlark thread object
* @return a dict contains the network token values
*/
Object lookupCard(
String number,
fangpenlin marked this conversation as resolved.
Show resolved Hide resolved
StarlarkInt expireMonth,
StarlarkInt expireYear,
fangpenlin marked this conversation as resolved.
Show resolved Hide resolved
String name,
String clientId,
String clientSecret,
Copy link
Contributor

Choose a reason for hiding this comment

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

secret? 🤔

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

please see my comment at #442 (comment) explaining the rationale

StarlarkThread thread)
throws EvalException;
}
39 changes: 39 additions & 0 deletions .../src/main/java/com/verygood/security/larky/modules/vgs/aus/MockAccountUpdaterService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
package com.verygood.security.larky.modules.vgs.aus;

import com.google.common.collect.ImmutableMap;
import com.verygood.security.larky.modules.vgs.aus.spi.AccountUpdaterService;
import java.util.Map;

public class MockAccountUpdaterService implements AccountUpdaterService {

private static final Map<String, Card> CARDS =
ImmutableMap.of(
"4111111111111111",
Card.builder()
.number("4111111111111111")
.expireMonth(10)
.expireYear(27)
.name("John Doe")
.build(),
"4242424242424242",
Card.builder()
.number("4242424242424243")
.expireMonth(12)
.expireYear(27)
.name("John Doe")
.build());

@Override
public Card lookupCard(
String number,
Integer expireMonth,
Integer expireYear,
String name,
String clientId,
String clientSecret) {
if (!CARDS.containsKey(number)) {
return null;
}
return CARDS.get(number);
}
}
16 changes: 16 additions & 0 deletions .../src/main/java/com/verygood/security/larky/modules/vgs/aus/NoopAccountUpdaterService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
package com.verygood.security.larky.modules.vgs.aus;

import com.verygood.security.larky.modules.vgs.aus.spi.AccountUpdaterService;

public class NoopAccountUpdaterService implements AccountUpdaterService {
@Override
public Card lookupCard(
String number,
Integer expireMonth,
Integer expireYear,
String name,
String clientId,
String clientSecret) {
throw new UnsupportedOperationException("Not implemented");
}
}
34 changes: 34 additions & 0 deletions .../src/main/java/com/verygood/security/larky/modules/vgs/aus/spi/AccountUpdaterService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
package com.verygood.security.larky.modules.vgs.aus.spi;

import lombok.Builder;
import lombok.Data;

public interface AccountUpdaterService {
/**
* Get updated info for the provided card
*
* @param number card's number
* @param expireMonth card's expiration month
* @param expireYear card's expiration year
* @param name the name on the card
* @param clientId client id of service account to access calm API
* @param clientSecret client secret of service account to access calm API
* @return the updated card
*/
Card lookupCard(
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure of the point of this method since it returns what it was passed in

Copy link
Contributor

Choose a reason for hiding this comment

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

Or does it return a different card? 🤔

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

A card could be expired, exp date changed, or stolen, so the number changed. It's still the same card, but a different number or exp date. So this service and this method is for looking up the given card number and see if there's updates for the card. The customer would like to use the new card number is there's one in case of the original one is expired.

String number,
Integer expireMonth,
Integer expireYear,
String name,
String clientId,
Copy link
Contributor

Choose a reason for hiding this comment

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

wonder why these need to be passed in and why they are env vars or something

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

these are per merchant/vault credentials for calm's service account

String clientSecret);

@Data
@Builder
class Card {
Copy link
Contributor

Choose a reason for hiding this comment

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

consider making record instead of class

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I tried to put record syntax here, but my IDE is showing red font. It seems like record is supported only in Java 14, are we using Java 14 in this larky project?

Copy link
Contributor

Choose a reason for hiding this comment

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

You should be using JDK 17 here

private final String number;
private final Integer expireMonth;
private final Integer expireYear;
private final String name;
}
}
16 changes: 16 additions & 0 deletions larky/src/main/resources/vgs/aus.star
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
load("@vgs//native_au", _au="native_au")
load("@stdlib//larky", larky="larky")

def lookup_card(pan, exp_month, exp_year, name, client_id, client_secret):
return _au.lookup_card(
number=pan,
exp_month=exp_month,
exp_year=exp_year,
name=name,
client_id=client_id,
client_secret=client_secret,
)

aus = larky.struct(
lookup_card=lookup_card,
)
1 change: 1 addition & 0 deletions ...s/META-INF/services/com.verygood.security.larky.modules.vgs.aus.spi.AccountUpdaterService
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
com.verygood.security.larky.modules.vgs.aus.MockAccountUpdaterService
55 changes: 55 additions & 0 deletions larky/src/test/resources/vgs_tests/aus/test_default_aus.star
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
load("@vendor//asserts", "asserts")
load("@stdlib//unittest", "unittest")
load("@vgs//aus", "aus")


def _test_lookup_card_with_exp_updates():
card = aus.lookup_card(
pan="4111111111111111",
exp_year=25,
exp_month=11,
name="Jane Doe",
client_id="AC5RA6rLA-calm-CaWsf",
client_secret="4624b52c-8906-4890-8a35-345168d7c8d7",
)
asserts.assert_that(card["number"]).is_equal_to("4111111111111111")
asserts.assert_that(card["exp_year"]).is_equal_to(27)
asserts.assert_that(card["exp_month"]).is_equal_to(10)

def _test_lookup_card_with_number_updates():
card = aus.lookup_card(
pan="4242424242424242",
exp_year=25,
exp_month=11,
name="Jane Doe",
client_id="AC5RA6rLA-calm-CaWsf",
client_secret="4624b52c-8906-4890-8a35-345168d7c8d7",
)
asserts.assert_that(card["number"]).is_equal_to("4242424242424243")
asserts.assert_that(card["exp_year"]).is_equal_to(27)
asserts.assert_that(card["exp_month"]).is_equal_to(12)


def _test_lookup_card_with_not_existing_card():
card = aus.lookup_card(
pan="999999999999",
exp_year=25,
exp_month=11,
name="Jane Doe",
client_id="AC5RA6rLA-calm-CaWsf",
client_secret="4624b52c-8906-4890-8a35-345168d7c8d7",
)
asserts.assert_that(card).is_none()



def _suite():
_suite = unittest.TestSuite()
_suite.addTest(unittest.FunctionTestCase(_test_lookup_card_with_exp_updates))
_suite.addTest(unittest.FunctionTestCase(_test_lookup_card_with_number_updates))
_suite.addTest(unittest.FunctionTestCase(_test_lookup_card_with_not_existing_card))
return _suite


_runner = unittest.TextTestRunner()
_runner.run(_suite())
28 changes: 28 additions & 0 deletions larky/src/test/resources/vgs_tests/aus/test_noop_aus.star
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
"""Unit tests for AccountUpdateModule.java using NoopVault API"""

load("@vendor//asserts", "asserts")
load("@stdlib//unittest", "unittest")
load("@vgs//aus", "aus")


def _test_lookup_card():
asserts.assert_fails(
lambda: aus.lookup_card(
pan="4242424242424242",
exp_year=25,
exp_month=11,
name="Jane Doe",
client_id="AC5RA6rLA-calm-CaWsf",
client_secret="4624b52c-8906-4890-8a35-345168d7c8d7",
),
"aus.lookup_card operation must be overridden")


def _suite():
_suite = unittest.TestSuite()
_suite.addTest(unittest.FunctionTestCase(_test_lookup_card))
return _suite


_runner = unittest.TextTestRunner()
_runner.run(_suite())