Skip to content

Commit

Permalink
RXI-1295 specification fixes:
Browse files Browse the repository at this point in the history 
Added 'credentials' array to 'deposit_authorized' response
'deposit_authorized' return errors on bad or expired credentials
'Credential' ledger object live in both 'Subject' and 'Issuer' directories
Correct 'CredentialIDs' will not fail payment transaction if 'lsfDepositAuth' flag not set on destination
  • Loading branch information
@oleks-rip
oleks-rip committed Sep 25, 2024
1 parent b15ec33 commit d531ed0
Show file tree
Hide file tree
Showing 18 changed files with 325 additions and 150 deletions.
5 changes: 4 additions & 1 deletion include/xrpl/protocol/ErrorCodes.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -148,7 +148,10 @@ enum error_code_i {
// Oracle
rpcORACLE_MALFORMED = 94,

rpcLAST = rpcORACLE_MALFORMED // rpcLAST should always equal the last code.
// deposit_authorized + credentials
rpcBAD_CREDENTIALS = 95,

rpcLAST = rpcBAD_CREDENTIALS // rpcLAST should always equal the last code.
};

/** Codes returned in the `warnings` array of certain RPC commands.
Expand Down
1 change: 1 addition & 0 deletions include/xrpl/protocol/SField.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -468,6 +468,7 @@ extern SF_UINT64 const sfXChainAccountCreateCount;
extern SF_UINT64 const sfXChainAccountClaimCount;
extern SF_UINT64 const sfAssetPrice;
extern SF_UINT64 const sfIssuerNode;
extern SF_UINT64 const sfSubjectNode;

// 128-bit
extern SF_UINT128 const sfEmailHash;
Expand Down
3 changes: 2 additions & 1 deletion src/libxrpl/protocol/ErrorCodes.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,8 @@ constexpr static ErrorInfo unorderedErrorInfos[]{
{rpcTOO_BUSY, "tooBusy", "The server is too busy to help you now.", 503},
{rpcTXN_NOT_FOUND, "txnNotFound", "Transaction not found.", 404},
{rpcUNKNOWN_COMMAND, "unknownCmd", "Unknown method.", 405},
{rpcORACLE_MALFORMED, "oracleMalformed", "Oracle request is malformed.", 400}};
{rpcORACLE_MALFORMED, "oracleMalformed", "Oracle request is malformed.", 400},
{rpcBAD_CREDENTIALS, "badCredentials", "Credentials do not exist, are not accepted, or have expired.", 400}};
// clang-format on

// Sort and validate unorderedErrorInfos at compile time. Should be
Expand Down
2 changes: 1 addition & 1 deletion src/libxrpl/protocol/LedgerFormats.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -373,7 +373,7 @@ LedgerFormats::LedgerFormats()
{sfCredentialType, soeREQUIRED},
{sfExpiration, soeOPTIONAL},
{sfURI, soeOPTIONAL},
{sfOwnerNode, soeREQUIRED},
{sfSubjectNode, soeREQUIRED},
{sfIssuerNode, soeREQUIRED},
{sfPreviousTxnID, soeREQUIRED},
{sfPreviousTxnLgrSeq, soeREQUIRED},
Expand Down
1 change: 1 addition & 0 deletions src/libxrpl/protocol/SField.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -194,6 +194,7 @@ CONSTRUCT_TYPED_SFIELD(sfXChainAccountCreateCount, "XChainAccountCreateCount", U
CONSTRUCT_TYPED_SFIELD(sfXChainAccountClaimCount, "XChainAccountClaimCount", UINT64, 22);
CONSTRUCT_TYPED_SFIELD(sfAssetPrice, "AssetPrice", UINT64, 23);
CONSTRUCT_TYPED_SFIELD(sfIssuerNode, "IssuerNode", UINT64, 24);
CONSTRUCT_TYPED_SFIELD(sfSubjectNode, "SubjectNode", UINT64, 25);

// 128-bit
CONSTRUCT_TYPED_SFIELD(sfEmailHash, "EmailHash", UINT128, 1);
Expand Down
5 changes: 2 additions & 3 deletions src/test/app/AccountDelete_test.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -945,12 +945,11 @@ class AccountDelete_test : public beast::unit_test::suite

auto const acctDelFee{drops(env.current()->fees().increment)};

// becky use credentials but lsfDepositAuth not set and can't
// delete account
// becky use credentials but they aren't accepted
env(acctdelete(becky, alice),
credentials::IDs({credIdx}),
fee(acctDelFee),
ter(tecNO_PERMISSION));
ter(tecBAD_CREDENTIALS));
env.close();

{
Expand Down
125 changes: 118 additions & 7 deletions src/test/app/Credentials_test.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -81,18 +81,18 @@ struct Credentials_test : public beast::unit_test::suite

Account const iss{"issuer"};
Account const subj{"subject"};
Account const other{"other"};

auto const kCred = credKL(subj, iss, credType);

env.fund(XRP(5000), subj, iss);
env.fund(XRP(5000), subj, iss, other);
env.close();

// Test Create credentials
env(credentials::createIssuer(subj, iss, credType),
credentials::uri(uri));
env.close();
{
BEAST_EXPECT(ownerCnt(env, iss) == 1);
auto const sleCred = env.le(kCred);
BEAST_EXPECT(static_cast<bool>(sleCred));
if (!sleCred)
Expand All @@ -101,9 +101,6 @@ struct Credentials_test : public beast::unit_test::suite
BEAST_EXPECT(sleCred->getAccountID(sfSubject) == subj.id());
BEAST_EXPECT(sleCred->getAccountID(sfIssuer) == iss.id());
BEAST_EXPECT(!(sleCred->getFieldU32(sfFlags) & lsfAccepted));
BEAST_EXPECT(
sleCred->getFieldU64(sfIssuerNode) ==
sleCred->getFieldU64(sfOwnerNode));
BEAST_EXPECT(ownerCnt(env, iss) == 1);
BEAST_EXPECT(!ownerCnt(env, subj));
BEAST_EXPECT(checkVL(sleCred, sfCredentialType, credType));
Expand All @@ -124,15 +121,14 @@ struct Credentials_test : public beast::unit_test::suite
{
// check switching owner of the credentials from isser to
// subject
BEAST_EXPECT(ownerCnt(env, subj) == 1);
auto const sleCred = env.le(kCred);
BEAST_EXPECT(static_cast<bool>(sleCred));
if (!sleCred)
return;

BEAST_EXPECT(sleCred->getAccountID(sfSubject) == subj.id());
BEAST_EXPECT(sleCred->getAccountID(sfIssuer) == iss.id());
BEAST_EXPECT(ownerCnt(env, iss) == 1);
BEAST_EXPECT(!ownerCnt(env, iss));
BEAST_EXPECT(ownerCnt(env, subj) == 1);
BEAST_EXPECT(checkVL(sleCred, sfCredentialType, credType));
BEAST_EXPECT(checkVL(sleCred, sfURI, uri));
Expand All @@ -153,6 +149,121 @@ struct Credentials_test : public beast::unit_test::suite
jle.isObject() && jle.isMember(jss::result) &&
jle[jss::result].isMember(jss::error));
}

{
testcase("Credentials for themself.");

auto const kCred = credKL(iss, iss, credType);

env(credentials::createIssuer(iss, iss, credType),
credentials::uri(uri));
env.close();
{
auto const sleCred = env.le(kCred);
BEAST_EXPECT(static_cast<bool>(sleCred));
if (!sleCred)
return;

BEAST_EXPECT(sleCred->getAccountID(sfSubject) == iss.id());
BEAST_EXPECT(sleCred->getAccountID(sfIssuer) == iss.id());
BEAST_EXPECT((sleCred->getFieldU32(sfFlags) & lsfAccepted));
BEAST_EXPECT(
sleCred->getFieldU64(sfIssuerNode) ==
sleCred->getFieldU64(sfSubjectNode));
BEAST_EXPECT(ownerCnt(env, iss) == 1);
BEAST_EXPECT(checkVL(sleCred, sfCredentialType, credType));
BEAST_EXPECT(checkVL(sleCred, sfURI, uri));
auto const jle = credentials::ledgerEntryCredential(
env, iss, iss, credType);
BEAST_EXPECT(
jle.isObject() && jle.isMember(jss::result) &&
!jle[jss::result].isMember(jss::error) &&
jle[jss::result].isMember(jss::node) &&
jle[jss::result][jss::node].isMember(
"LedgerEntryType") &&
jle[jss::result][jss::node]["LedgerEntryType"] ==
jss::Credential);
}

env(credentials::del(iss, iss, iss, credType));
env.close();
{
BEAST_EXPECT(!env.le(kCred));
BEAST_EXPECT(!ownerCnt(env, iss));

// check no credential exists anymore
auto const jle = credentials::ledgerEntryCredential(
env, iss, iss, credType);
BEAST_EXPECT(
jle.isObject() && jle.isMember(jss::result) &&
jle[jss::result].isMember(jss::error));
}
}

{
testcase("Delete issuer");

env(credentials::createIssuer(subj, iss, credType));
env.close();

// delete issuer
{
int const delta = env.seq(iss) + 255;
for (int i = 0; i < delta; ++i)
env.close();
auto const acctDelFee{
drops(env.current()->fees().increment)};
env(acctdelete(iss, other), fee(acctDelFee));
env.close();
}

// check credentials deleted too
{
BEAST_EXPECT(!env.le(kCred));
BEAST_EXPECT(!ownerCnt(env, subj));

// check no credential exists anymore
auto const jle = credentials::ledgerEntryCredential(
env, subj, iss, credType);
BEAST_EXPECT(
jle.isObject() && jle.isMember(jss::result) &&
jle[jss::result].isMember(jss::error));
}

// restore issuer
env.fund(XRP(5000), iss);
env.close();

testcase("Delete subject");
env(credentials::createIssuer(subj, iss, credType));
env.close();
env(credentials::accept(subj, iss, credType));
env.close();

// delete subject
{
int const delta = env.seq(subj) + 255;
for (int i = 0; i < delta; ++i)
env.close();
auto const acctDelFee{
drops(env.current()->fees().increment)};
env(acctdelete(subj, other), fee(acctDelFee));
env.close();
}

// check credentials deleted too
{
BEAST_EXPECT(!env.le(kCred));
BEAST_EXPECT(!ownerCnt(env, iss));

// check no credential exists anymore
auto const jle = credentials::ledgerEntryCredential(
env, subj, iss, credType);
BEAST_EXPECT(
jle.isObject() && jle.isMember(jss::result) &&
jle[jss::result].isMember(jss::error));
}
}
}

{
Expand Down
88 changes: 76 additions & 12 deletions src/test/app/DepositAuth_test.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -877,11 +877,11 @@ struct DepositPreauth_test : public beast::unit_test::suite
env.close();

{
// Don't use credentials for yourself
// Don't use invalids credentials
env(finish(bob, alice, seq1),
credentials::IDs({credIdx}),
fee(1500),
ter(tecNO_PERMISSION));
ter(tecBAD_CREDENTIALS));
env.close();
}
{
Expand All @@ -903,6 +903,77 @@ struct DepositPreauth_test : public beast::unit_test::suite
}
}

{
Env env(*this);
Account const iss{"issuer"};
Account const alice{"alice"};
Account const bob{"bob"};
Account const john("john");

env.fund(XRP(5000), iss, alice, bob, john);
env.close();

{
testcase("Escrow with credentials without depositPreauth");
using namespace std::chrono;

const char credType2[] = "fghijk";

env(credentials::createIssuer(bob, iss, credType2));
env.close();
env(credentials::accept(bob, iss, credType2));
env.close();

env(credentials::createIssuer(john, iss, credType));
env.close();
env(credentials::accept(john, iss, credType));
env.close();

// Get the index of the credentials
auto const credIdxBob =
credentials::ledgerEntryCredential(
env, bob, iss, credType2)[jss::result][jss::index]
.asString();
auto const credIdxJohn =
credentials::ledgerEntryCredential(
env, john, iss, credType)[jss::result][jss::index]
.asString();

{
auto const seq1 = env.seq(alice);
env(escrow(alice, bob, XRP(1000)),
finish_time(env.now() + 1s));
env.close();

// Use any valid credentials without depositPreauth
// requirements
env(finish(john, alice, seq1),
credentials::IDs({credIdxJohn}),
fee(1500));
env.close();
}

{
auto const seq1 = env.seq(alice);
env(escrow(alice, bob, XRP(1000)),
finish_time(env.now() + 1s));
env.close();

// Bob require preauthorization
env(fset(bob, asfDepositAuth), fee(drops(10)));
env.close();
env(deposit::authCredentials(bob, {{iss, credType}}));
env.close();

// Use any valid credentials without if src == dst
env(finish(bob, alice, seq1),
credentials::IDs({credIdxBob}),
fee(1500));
env.close();
}
}
}

{
testcase("Creating / deleting with credentials.");

Expand Down Expand Up @@ -956,12 +1027,6 @@ struct DepositPreauth_test : public beast::unit_test::suite
env(jv, ter(temINVALID_ACCOUNT_ID));
}

{
// try preauth itself
auto jv = deposit::authCredentials(bob, {{bob, credType}});
env(jv, ter(temCANNOT_PREAUTH_SELF));
}

{
// empty credential type
auto jv = deposit::authCredentials(bob, {{iss, {}}});
Expand Down Expand Up @@ -1069,10 +1134,9 @@ struct DepositPreauth_test : public beast::unit_test::suite
jCred[jss::result][jss::index].asString();

{
// Fail as destination didn't enable preauthorization
env(pay(alice, bob, XRP(100)),
credentials::IDs({credIdx}),
ter(tecNO_PERMISSION));
// Success as destination didn't enable preauthorization so
// valid credentials will not fail
env(pay(alice, bob, XRP(100)), credentials::IDs({credIdx}));
}

// Bob require preauthorization
Expand Down
6 changes: 0 additions & 6 deletions src/test/app/Escrow_test.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1567,12 +1567,6 @@ struct Escrow_test : public beast::unit_test::suite
env(fset(bob, asfDepositAuth), fee(drops(10)));
env.close();

// Fail, src == dst and use credentials
env(finish(bob, alice, seq),
credentials::IDs({credIdx}),
fee(1500),
ter(tecNO_PERMISSION));

// Fail, credentials doesn’t belong to
env(finish(dillon, alice, seq),
credentials::IDs({credIdx}),
Expand Down
11 changes: 1 addition & 10 deletions src/test/app/PayChan_test.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -867,11 +867,6 @@ struct PayChan_test : public beast::unit_test::suite

auto const delta = XRP(500).value();

// Fail, bob's lsfDepositAuth flag is NOT set.
env(claim(alice, chan, delta, delta),
credentials::IDs({credBadIdx}),
ter(tecNO_PERMISSION));

{ // create credentials
auto jv = credentials::createIssuer(alice, carol, credType);
uint32_t const t = env.now().time_since_epoch().count() + 100;
Expand All @@ -889,11 +884,6 @@ struct PayChan_test : public beast::unit_test::suite
env(fset(bob, asfDepositAuth));
env.close();

// Fail, src == dst doesn't need credentials
env(claim(bob, chan, delta, delta),
credentials::IDs({credIdx}),
ter(tecNO_PERMISSION));

// Fail, credentials doesn’t belong to
env(claim(dillon, chan, delta, delta),
credentials::IDs({credIdx}),
Expand All @@ -908,6 +898,7 @@ struct PayChan_test : public beast::unit_test::suite
env(credentials::accept(alice, carol, credType));
env.close();

// Fail, no depositPreauth object
env(claim(alice, chan, delta, delta),
credentials::IDs({credIdx}),
ter(tecNO_PERMISSION));
Expand Down
Loading

0 comments on commit d531ed0

Please sign in to comment.