From d531ed0a4b2bf169f0c8765db71fbcd9b9321d86 Mon Sep 17 00:00:00 2001 From: Oleksandr <115580134+oleks-rip@users.noreply.github.com> Date: Wed, 25 Sep 2024 10:35:02 -0400 Subject: [PATCH] RXI-1295 specification fixes: 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 --- include/xrpl/protocol/ErrorCodes.h | 5 +- include/xrpl/protocol/SField.h | 1 + src/libxrpl/protocol/ErrorCodes.cpp | 3 +- src/libxrpl/protocol/LedgerFormats.cpp | 2 +- src/libxrpl/protocol/SField.cpp | 1 + src/test/app/AccountDelete_test.cpp | 5 +- src/test/app/Credentials_test.cpp | 125 +++++++++++++++++-- src/test/app/DepositAuth_test.cpp | 88 +++++++++++-- src/test/app/Escrow_test.cpp | 6 - src/test/app/PayChan_test.cpp | 11 +- src/test/rpc/DepositAuthorized_test.cpp | 15 +-- src/xrpld/app/tx/detail/Credentials.cpp | 86 +++++++------ src/xrpld/app/tx/detail/DeleteAccount.cpp | 20 +-- src/xrpld/app/tx/detail/DepositPreauth.cpp | 10 +- src/xrpld/app/tx/detail/Escrow.cpp | 22 ++-- src/xrpld/app/tx/detail/PayChan.cpp | 5 +- src/xrpld/app/tx/detail/Payment.cpp | 24 ++-- src/xrpld/rpc/handlers/DepositAuthorized.cpp | 46 ++++--- 18 files changed, 325 insertions(+), 150 deletions(-) diff --git a/include/xrpl/protocol/ErrorCodes.h b/include/xrpl/protocol/ErrorCodes.h index d8ec3052b7b..39cfa9369cd 100644 --- a/include/xrpl/protocol/ErrorCodes.h +++ b/include/xrpl/protocol/ErrorCodes.h @@ -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. diff --git a/include/xrpl/protocol/SField.h b/include/xrpl/protocol/SField.h index f33334ceca8..d4bd6125e8a 100644 --- a/include/xrpl/protocol/SField.h +++ b/include/xrpl/protocol/SField.h @@ -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; diff --git a/src/libxrpl/protocol/ErrorCodes.cpp b/src/libxrpl/protocol/ErrorCodes.cpp index 4c934f4fd53..c157d9b296c 100644 --- a/src/libxrpl/protocol/ErrorCodes.cpp +++ b/src/libxrpl/protocol/ErrorCodes.cpp @@ -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 diff --git a/src/libxrpl/protocol/LedgerFormats.cpp b/src/libxrpl/protocol/LedgerFormats.cpp index 4ebab2f531c..81df6fb709c 100644 --- a/src/libxrpl/protocol/LedgerFormats.cpp +++ b/src/libxrpl/protocol/LedgerFormats.cpp @@ -373,7 +373,7 @@ LedgerFormats::LedgerFormats() {sfCredentialType, soeREQUIRED}, {sfExpiration, soeOPTIONAL}, {sfURI, soeOPTIONAL}, - {sfOwnerNode, soeREQUIRED}, + {sfSubjectNode, soeREQUIRED}, {sfIssuerNode, soeREQUIRED}, {sfPreviousTxnID, soeREQUIRED}, {sfPreviousTxnLgrSeq, soeREQUIRED}, diff --git a/src/libxrpl/protocol/SField.cpp b/src/libxrpl/protocol/SField.cpp index 76d8cfe0907..70b1513bdf1 100644 --- a/src/libxrpl/protocol/SField.cpp +++ b/src/libxrpl/protocol/SField.cpp @@ -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); diff --git a/src/test/app/AccountDelete_test.cpp b/src/test/app/AccountDelete_test.cpp index bca775327a2..bfeb76ac865 100644 --- a/src/test/app/AccountDelete_test.cpp +++ b/src/test/app/AccountDelete_test.cpp @@ -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(); { diff --git a/src/test/app/Credentials_test.cpp b/src/test/app/Credentials_test.cpp index 97bd97f4f08..8910dfb24d9 100644 --- a/src/test/app/Credentials_test.cpp +++ b/src/test/app/Credentials_test.cpp @@ -81,10 +81,11 @@ 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 @@ -92,7 +93,6 @@ struct Credentials_test : public beast::unit_test::suite credentials::uri(uri)); env.close(); { - BEAST_EXPECT(ownerCnt(env, iss) == 1); auto const sleCred = env.le(kCred); BEAST_EXPECT(static_cast(sleCred)); if (!sleCred) @@ -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)); @@ -124,7 +121,6 @@ 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(sleCred)); if (!sleCred) @@ -132,7 +128,7 @@ struct Credentials_test : public beast::unit_test::suite 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)); @@ -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(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)); + } + } } { diff --git a/src/test/app/DepositAuth_test.cpp b/src/test/app/DepositAuth_test.cpp index 09c4ca07a5c..08725108be6 100644 --- a/src/test/app/DepositAuth_test.cpp +++ b/src/test/app/DepositAuth_test.cpp @@ -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(); } { @@ -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."); @@ -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, {}}}); @@ -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 diff --git a/src/test/app/Escrow_test.cpp b/src/test/app/Escrow_test.cpp index b37fc64e1ae..2b6132567eb 100644 --- a/src/test/app/Escrow_test.cpp +++ b/src/test/app/Escrow_test.cpp @@ -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}), diff --git a/src/test/app/PayChan_test.cpp b/src/test/app/PayChan_test.cpp index d8e06f27761..2007fbf40af 100644 --- a/src/test/app/PayChan_test.cpp +++ b/src/test/app/PayChan_test.cpp @@ -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; @@ -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}), @@ -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)); diff --git a/src/test/rpc/DepositAuthorized_test.cpp b/src/test/rpc/DepositAuthorized_test.cpp index 9d1e230a155..f294d7dac93 100644 --- a/src/test/rpc/DepositAuthorized_test.cpp +++ b/src/test/rpc/DepositAuthorized_test.cpp @@ -374,9 +374,10 @@ class DepositAuthorized_test : public beast::unit_test::suite auto jv = env.rpc("json", "deposit_authorized", args.toStyledString()); + auto const& result{jv[jss::result]}; - BEAST_EXPECT(result[jss::status] == jss::success); - BEAST_EXPECT(result[jss::deposit_authorized] == false); + BEAST_EXPECT(result[jss::status] == jss::error); + BEAST_EXPECT(result[jss::error_code] == rpcBAD_CREDENTIALS); } { @@ -389,8 +390,8 @@ class DepositAuthorized_test : public beast::unit_test::suite depositAuthArgs(alice, becky, "validated", {credIdx}) .toStyledString()); auto const& result{jv[jss::result]}; - BEAST_EXPECT(result[jss::status] == jss::success); - BEAST_EXPECT(result[jss::deposit_authorized] == false); + BEAST_EXPECT(result[jss::status] == jss::error); + BEAST_EXPECT(result[jss::error_code] == rpcBAD_CREDENTIALS); } // alice accept credentials @@ -418,7 +419,7 @@ class DepositAuthorized_test : public beast::unit_test::suite .toStyledString()); auto const& result{jv[jss::result]}; BEAST_EXPECT(result[jss::status] == jss::success); - BEAST_EXPECT(result[jss::deposit_authorized] == false); + BEAST_EXPECT(result[jss::deposit_authorized] == true); } { @@ -470,8 +471,8 @@ class DepositAuthorized_test : public beast::unit_test::suite .toStyledString()); auto const& result{jv[jss::result]}; - BEAST_EXPECT(result[jss::status] == jss::success); - BEAST_EXPECT(result[jss::deposit_authorized] == false); + BEAST_EXPECT(result[jss::status] == jss::error); + BEAST_EXPECT(result[jss::error_code] == rpcBAD_CREDENTIALS); } } } diff --git a/src/xrpld/app/tx/detail/Credentials.cpp b/src/xrpld/app/tx/detail/Credentials.cpp index 138500a7df2..50db35b8e14 100644 --- a/src/xrpld/app/tx/detail/Credentials.cpp +++ b/src/xrpld/app/tx/detail/Credentials.cpp @@ -172,11 +172,11 @@ CredentialCreate::preclaim(PreclaimContext const& ctx) TER CredentialCreate::doApply() { - auto const sleOwner = view().peek(keylet::account(account_)); + auto const sleIssuer = view().peek(keylet::account(account_)); { STAmount const reserve{view().fees().accountReserve( - sleOwner->getFieldU32(sfOwnerCount) + 1)}; + sleIssuer->getFieldU32(sfOwnerCount) + 1)}; if (mPriorBalance < reserve) return tecINSUFFICIENT_RESERVE; } @@ -210,22 +210,37 @@ CredentialCreate::doApply() if (ctx_.tx.isFieldPresent(sfURI)) sleCred->setFieldVL(sfURI, ctx_.tx.getFieldVL(sfURI)); - view().insert(sleCred); - auto const page = view().dirInsert( - keylet::ownerDir(account_), kCred, describeOwnerDir(account_)); + if (subject == issuer) + sleCred->setFieldU32(sfFlags, lsfAccepted); - JLOG(j_.trace()) << "Adding Credential to owner directory " - << to_string(kCred.key) << ": " - << (page ? "success" : "failure"); + { + auto const page = view().dirInsert( + keylet::ownerDir(issuer), kCred, describeOwnerDir(issuer)); + JLOG(j_.trace()) << "Adding Credential to owner directory " + << to_string(kCred.key) << ": " + << (page ? "success" : "failure"); + if (!page) + return tecDIR_FULL; + sleCred->setFieldU64(sfIssuerNode, *page); - if (!page) - return tecDIR_FULL; + adjustOwnerCount(view(), sleIssuer, 1, j_); + view().update(sleIssuer); + } - sleCred->setFieldU64(sfOwnerNode, *page); - sleCred->setFieldU64(sfIssuerNode, *page); + if (subject != issuer) + { + auto const page = view().dirInsert( + keylet::ownerDir(subject), kCred, describeOwnerDir(subject)); + JLOG(j_.trace()) << "Adding Credential to owner directory " + << to_string(kCred.key) << ": " + << (page ? "success" : "failure"); + if (!page) + return tecDIR_FULL; + sleCred->setFieldU64(sfSubjectNode, *page); + view().update(view().peek(keylet::account(subject))); + } - adjustOwnerCount(view(), sleOwner, 1, j_); - view().update(sleOwner); + view().insert(sleCred); return tesSUCCESS; } @@ -288,10 +303,11 @@ CredentialDelete::deleteSLE( if (!sle) return tecNO_ENTRY; - auto delSLE = [&view, &sle, j]( - AccountID const& owner, SField const& node) -> TER { - auto const sleOwner = view.peek(keylet::account(owner)); - if (!sleOwner) + auto delSLE = + [&view, &sle, j]( + AccountID const& account, SField const& node, bool isOwner) -> TER { + auto const sleAccount = view.peek(keylet::account(account)); + if (!sleAccount) { JLOG(j.fatal()) << "Internal error: can't retrieve Owner account."; return tecINTERNAL; @@ -299,25 +315,32 @@ CredentialDelete::deleteSLE( // Remove object from owner directory std::uint64_t const page = sle->getFieldU64(node); - if (!view.dirRemove(keylet::ownerDir(owner), page, sle->key(), false)) + if (!view.dirRemove(keylet::ownerDir(account), page, sle->key(), false)) { JLOG(j.fatal()) << "Unable to delete Credential from owner."; return tefBAD_LEDGER; } - adjustOwnerCount(view, sleOwner, -1, j); - view.update(sleOwner); + if (isOwner) + { + adjustOwnerCount(view, sleAccount, -1, j); + view.update(sleAccount); + } return tesSUCCESS; }; - auto err = delSLE(sle->getAccountID(sfIssuer), sfIssuerNode); + auto const issuer = sle->getAccountID(sfIssuer); + auto const subject = sle->getAccountID(sfSubject); + bool const accepted = sle->getFlags() & lsfAccepted; + + auto err = delSLE(issuer, sfIssuerNode, !accepted || (subject == issuer)); if (!isTesSuccess(err)) return err; - if (sle->getFlags() & lsfAccepted) + if (subject != issuer) { - err = delSLE(sle->getAccountID(sfSubject), sfOwnerNode); + err = delSLE(subject, sfSubjectNode, accepted); if (!isTesSuccess(err)) return err; } @@ -434,21 +457,12 @@ CredentialAccept::doApply() return tecEXPIRED; } - { - // Share ownership between issuer and subject - auto const page = view().dirInsert( - keylet::ownerDir(subject), kCred, describeOwnerDir(subject)); - JLOG(j_.trace()) << "Moving Credential to subject directory " - << to_string(kCred.key) << ": " - << (page ? "success" : "failure"); - if (!page) - return tecDIR_FULL; - sleCred->setFieldU64(sfOwnerNode, *page); - } - sleCred->setFieldU32(sfFlags, lsfAccepted); view().update(sleCred); + adjustOwnerCount(view(), sleIss, -1, j_); + view().update(sleIss); + adjustOwnerCount(view(), sleSubj, 1, j_); view().update(sleSubj); diff --git a/src/xrpld/app/tx/detail/DeleteAccount.cpp b/src/xrpld/app/tx/detail/DeleteAccount.cpp index c11683325cd..364f7df3448 100644 --- a/src/xrpld/app/tx/detail/DeleteAccount.cpp +++ b/src/xrpld/app/tx/detail/DeleteAccount.cpp @@ -241,9 +241,6 @@ DeleteAccount::preclaim(PreclaimContext const& ctx) // Check whether the destination account requires deposit authorization. if (ctx.tx.isFieldPresent(sfCredentialIDs)) { - if (!(sleDst->getFlags() & lsfDepositAuth)) - return tecNO_PERMISSION; - STArray authCreds; for (auto const& h : ctx.tx.getFieldV256(sfCredentialIDs)) { @@ -268,14 +265,19 @@ DeleteAccount::preclaim(PreclaimContext const& ctx) return tecBAD_CREDENTIALS; } - auto credential = STObject::makeInnerObject(sfCredential); - credential.setAccountID(sfIssuer, sleCred->getAccountID(sfIssuer)); - credential.setFieldVL( - sfCredentialType, sleCred->getFieldVL(sfCredentialType)); - authCreds.push_back(std::move(credential)); + if (sleDst->getFlags() & lsfDepositAuth) + { + auto credential = STObject::makeInnerObject(sfCredential); + credential.setAccountID( + sfIssuer, sleCred->getAccountID(sfIssuer)); + credential.setFieldVL( + sfCredentialType, sleCred->getFieldVL(sfCredentialType)); + authCreds.push_back(std::move(credential)); + } } - if (!ctx.view.exists(keylet::depositPreauth(dst, authCreds))) + if ((sleDst->getFlags() & lsfDepositAuth) && + !ctx.view.exists(keylet::depositPreauth(dst, authCreds))) { JLOG(ctx.j.trace()) << "DepositPreauth doesn't exist"; return tecNO_PERMISSION; diff --git a/src/xrpld/app/tx/detail/DepositPreauth.cpp b/src/xrpld/app/tx/detail/DepositPreauth.cpp index 91e3cba3dd2..0b0fbccd14f 100644 --- a/src/xrpld/app/tx/detail/DepositPreauth.cpp +++ b/src/xrpld/app/tx/detail/DepositPreauth.cpp @@ -138,14 +138,6 @@ DepositPreauth::preflight(PreflightContext const& ctx) return temINVALID_ACCOUNT_ID; } - // An account may not preauthorize itself. - if (target == account) - { - JLOG(j.trace()) << "Malformed transaction: Attempting to " - "DepositPreauth self."; - return temCANNOT_PREAUTH_SELF; - } - auto const ct = o[sfCredentialType]; if (ct.empty() || (ct.size() > maxCredentialTypeLength)) { @@ -287,7 +279,7 @@ DepositPreauth::doApply() STArray& arr = *static_cast(pArr); arr.reserve(authCred.size()); - // eleminate duplicates + // eliminate duplicates std::unordered_set hashes; for (auto const& o : authCred) { diff --git a/src/xrpld/app/tx/detail/Escrow.cpp b/src/xrpld/app/tx/detail/Escrow.cpp index 480e5b9195c..04cb210c985 100644 --- a/src/xrpld/app/tx/detail/Escrow.cpp +++ b/src/xrpld/app/tx/detail/Escrow.cpp @@ -401,12 +401,9 @@ EscrowFinish::preclaim(PreclaimContext const& ctx) if (!sleDst) return tecNO_DST; + AccountID const src = ctx.tx[sfAccount]; if (ctx.tx.isFieldPresent(sfCredentialIDs)) { - AccountID const src = ctx.tx[sfAccount]; - if (!(sleDst->getFlags() & lsfDepositAuth) || (src == dst)) - return tecNO_PERMISSION; - STArray authCreds; for (auto const& h : ctx.tx.getFieldV256(sfCredentialIDs)) { @@ -431,14 +428,19 @@ EscrowFinish::preclaim(PreclaimContext const& ctx) return tecBAD_CREDENTIALS; } - auto credential = STObject::makeInnerObject(sfCredential); - credential.setAccountID(sfIssuer, sleCred->getAccountID(sfIssuer)); - credential.setFieldVL( - sfCredentialType, sleCred->getFieldVL(sfCredentialType)); - authCreds.push_back(std::move(credential)); + if ((sleDst->getFlags() & lsfDepositAuth) && (src != dst)) + { + auto credential = STObject::makeInnerObject(sfCredential); + credential.setAccountID( + sfIssuer, sleCred->getAccountID(sfIssuer)); + credential.setFieldVL( + sfCredentialType, sleCred->getFieldVL(sfCredentialType)); + authCreds.push_back(std::move(credential)); + } } - if (!ctx.view.exists(keylet::depositPreauth(dst, authCreds))) + if (((sleDst->getFlags() & lsfDepositAuth) && (src != dst)) && + !ctx.view.exists(keylet::depositPreauth(dst, authCreds))) { JLOG(ctx.j.trace()) << "DepositPreauth doesn't exist"; return tecNO_PERMISSION; diff --git a/src/xrpld/app/tx/detail/PayChan.cpp b/src/xrpld/app/tx/detail/PayChan.cpp index 18697b5c3b5..9ba8244a3e7 100644 --- a/src/xrpld/app/tx/detail/PayChan.cpp +++ b/src/xrpld/app/tx/detail/PayChan.cpp @@ -495,8 +495,6 @@ PayChanClaim::preclaim(PreclaimContext const& ctx) return tecNO_DST; AccountID const src = ctx.tx[sfAccount]; - if (!(sleDst->getFlags() & lsfDepositAuth) || (src == dst)) - return tecNO_PERMISSION; STArray authCreds; for (auto const& h : ctx.tx.getFieldV256(sfCredentialIDs)) @@ -529,7 +527,8 @@ PayChanClaim::preclaim(PreclaimContext const& ctx) authCreds.push_back(std::move(credential)); } - if (!ctx.view.exists(keylet::depositPreauth(dst, authCreds))) + if (((sleDst->getFlags() & lsfDepositAuth) && (src != dst)) && + !ctx.view.exists(keylet::depositPreauth(dst, authCreds))) { JLOG(ctx.j.trace()) << "DepositPreauth doesn't exist"; return tecNO_PERMISSION; diff --git a/src/xrpld/app/tx/detail/Payment.cpp b/src/xrpld/app/tx/detail/Payment.cpp index 25ec8482f29..66c4ab327f1 100644 --- a/src/xrpld/app/tx/detail/Payment.cpp +++ b/src/xrpld/app/tx/detail/Payment.cpp @@ -313,12 +313,9 @@ Payment::preclaim(PreclaimContext const& ctx) { // Don't check for minimal balance with credentials provided. // The amendement rules have already been checked in - // preflight() + // preflight(). auto const src = ctx.tx[sfAccount]; - if (!(sleDst && sleDst->getFlags() & lsfDepositAuth) || - (src == uDstAccountID)) - return tecNO_PERMISSION; STArray authCreds; for (auto const& h : ctx.tx.getFieldV256(sfCredentialIDs)) @@ -344,14 +341,21 @@ Payment::preclaim(PreclaimContext const& ctx) return tecBAD_CREDENTIALS; } - auto credential = STObject::makeInnerObject(sfCredential); - credential.setAccountID(sfIssuer, sleCred->getAccountID(sfIssuer)); - credential.setFieldVL( - sfCredentialType, sleCred->getFieldVL(sfCredentialType)); - authCreds.push_back(std::move(credential)); + if ((sleDst && (sleDst->getFlags() & lsfDepositAuth)) && + (src != uDstAccountID)) + { + auto credential = STObject::makeInnerObject(sfCredential); + credential.setAccountID( + sfIssuer, sleCred->getAccountID(sfIssuer)); + credential.setFieldVL( + sfCredentialType, sleCred->getFieldVL(sfCredentialType)); + authCreds.push_back(std::move(credential)); + } } - if (!ctx.view.exists(keylet::depositPreauth(uDstAccountID, authCreds))) + if (((sleDst && (sleDst->getFlags() & lsfDepositAuth)) && + (src != uDstAccountID)) && + !ctx.view.exists(keylet::depositPreauth(uDstAccountID, authCreds))) { JLOG(ctx.j.trace()) << "DepositPreauth doesn't exist"; return tecNO_PERMISSION; diff --git a/src/xrpld/rpc/handlers/DepositAuthorized.cpp b/src/xrpld/rpc/handlers/DepositAuthorized.cpp index 140e357eee3..89d38df76b2 100644 --- a/src/xrpld/rpc/handlers/DepositAuthorized.cpp +++ b/src/xrpld/rpc/handlers/DepositAuthorized.cpp @@ -93,9 +93,8 @@ doDepositAuthorized(RPC::JsonContext& context) bool const reqAuth = sleDest->getFlags() & lsfDepositAuth; bool const credentialsPresent = params.isMember(jss::credentials); - bool invalidCredentials = false; STArray authCreds; - if (credentialsPresent && reqAuth) + if (credentialsPresent) { auto const& creds(params[jss::credentials]); if (!creds.isArray() || !creds) @@ -128,28 +127,30 @@ doDepositAuthorized(RPC::JsonContext& context) std::shared_ptr sleCred = ledger->read(keylet::credential(credH)); - if (!sleCred || (sleCred->getType() != ltCREDENTIAL)) + if (!sleCred || (sleCred->getType() != ltCREDENTIAL) || + !(sleCred->getFlags() & lsfAccepted)) { - invalidCredentials = true; - break; + RPC::inject_error(rpcBAD_CREDENTIALS, result); + return result; } - AccountID const subj = sleCred->getAccountID(sfSubject); AccountID const iss = sleCred->getAccountID(sfIssuer); Blob const credType = sleCred->getFieldVL(sfCredentialType); - if ((subj != srcID) || !(sleCred->getFlags() & lsfAccepted) || - Credentials::checkExpired( + if (Credentials::checkExpired( sleCred, context.app.timeKeeper().now())) { - invalidCredentials = true; - break; + RPC::inject_error(rpcBAD_CREDENTIALS, result); + return result; } - auto credential = STObject::makeInnerObject(sfCredential); - credential.setAccountID(sfIssuer, iss); - credential.setFieldVL(sfCredentialType, credType); - authCreds.push_back(std::move(credential)); + if (reqAuth && (srcAcct != dstAcct)) + { + auto credential = STObject::makeInnerObject(sfCredential); + credential.setAccountID(sfIssuer, iss); + credential.setFieldVL(sfCredentialType, credType); + authCreds.push_back(std::move(credential)); + } } } @@ -157,21 +158,16 @@ doDepositAuthorized(RPC::JsonContext& context) // not set, then the deposit should be fine. bool depositAuthorized = true; - if (credentialsPresent && !reqAuth) - depositAuthorized = false; - else if ((srcAcct != dstAcct) && reqAuth) - { - if (credentialsPresent) - depositAuthorized = !invalidCredentials && - ledger->exists(keylet::depositPreauth(dstAcct, authCreds)); - else - depositAuthorized = - ledger->exists(keylet::depositPreauth(dstAcct, srcAcct)); - } + if (reqAuth && (srcAcct != dstAcct)) + depositAuthorized = credentialsPresent + ? ledger->exists(keylet::depositPreauth(dstAcct, authCreds)) + : ledger->exists(keylet::depositPreauth(dstAcct, srcAcct)); result[jss::source_account] = params[jss::source_account].asString(); result[jss::destination_account] = params[jss::destination_account].asString(); + if (credentialsPresent) + result[jss::credentials] = params[jss::credentials]; result[jss::deposit_authorized] = depositAuthorized; return result;