From 3715d7e2e49f24d1aa7222f62df7cfb27ad2b5b2 Mon Sep 17 00:00:00 2001 From: Bronek Kozicki Date: Tue, 11 Mar 2025 22:58:02 +0000 Subject: [PATCH] Fix bugs related to perm. domain checks, add unit test --- src/test/app/Credentials_test.cpp | 28 ++--- src/test/app/Vault_test.cpp | 142 ++++++++++++++++++++++ src/test/jtx/credentials.h | 10 ++ src/xrpld/app/misc/CredentialHelpers.cpp | 14 ++- src/xrpld/app/tx/detail/VaultDeposit.cpp | 5 +- src/xrpld/app/tx/detail/VaultSet.cpp | 11 +- src/xrpld/app/tx/detail/VaultWithdraw.cpp | 2 +- src/xrpld/ledger/detail/View.cpp | 65 ++++++---- 8 files changed, 225 insertions(+), 52 deletions(-) diff --git a/src/test/app/Credentials_test.cpp b/src/test/app/Credentials_test.cpp index 481850562f..a8d5fbfb2c 100644 --- a/src/test/app/Credentials_test.cpp +++ b/src/test/app/Credentials_test.cpp @@ -42,16 +42,6 @@ checkVL( return strHex(expected) == strHex(sle->getFieldVL(field)); } -static inline Keylet -credentialKeylet( - test::jtx::Account const& subject, - test::jtx::Account const& issuer, - std::string_view credType) -{ - return keylet::credential( - subject.id(), issuer.id(), Slice(credType.data(), credType.size())); -} - struct Credentials_test : public beast::unit_test::suite { void @@ -71,7 +61,7 @@ struct Credentials_test : public beast::unit_test::suite { testcase("Create for subject."); - auto const credKey = credentialKeylet(subject, issuer, credType); + auto const credKey = credentials::keylet(subject, issuer, credType); env.fund(XRP(5000), subject, issuer, other); env.close(); @@ -149,7 +139,7 @@ struct Credentials_test : public beast::unit_test::suite { testcase("Create for themself."); - auto const credKey = credentialKeylet(issuer, issuer, credType); + auto const credKey = credentials::keylet(issuer, issuer, credType); env(credentials::create(issuer, issuer, credType), credentials::uri(uri)); @@ -223,7 +213,7 @@ struct Credentials_test : public beast::unit_test::suite { testcase("Delete issuer before accept"); - auto const credKey = credentialKeylet(subject, issuer, credType); + auto const credKey = credentials::keylet(subject, issuer, credType); env(credentials::create(subject, issuer, credType)); env.close(); @@ -259,7 +249,7 @@ struct Credentials_test : public beast::unit_test::suite { testcase("Delete issuer after accept"); - auto const credKey = credentialKeylet(subject, issuer, credType); + auto const credKey = credentials::keylet(subject, issuer, credType); env(credentials::create(subject, issuer, credType)); env.close(); env(credentials::accept(subject, issuer, credType)); @@ -297,7 +287,7 @@ struct Credentials_test : public beast::unit_test::suite { testcase("Delete subject before accept"); - auto const credKey = credentialKeylet(subject, issuer, credType); + auto const credKey = credentials::keylet(subject, issuer, credType); env(credentials::create(subject, issuer, credType)); env.close(); @@ -333,7 +323,7 @@ struct Credentials_test : public beast::unit_test::suite { testcase("Delete subject after accept"); - auto const credKey = credentialKeylet(subject, issuer, credType); + auto const credKey = credentials::keylet(subject, issuer, credType); env(credentials::create(subject, issuer, credType)); env.close(); env(credentials::accept(subject, issuer, credType)); @@ -371,7 +361,7 @@ struct Credentials_test : public beast::unit_test::suite { testcase("Delete by other"); - auto const credKey = credentialKeylet(subject, issuer, credType); + auto const credKey = credentials::keylet(subject, issuer, credType); auto jv = credentials::create(subject, issuer, credType); uint32_t const t = env.current() ->info() @@ -416,7 +406,7 @@ struct Credentials_test : public beast::unit_test::suite env.close(); { auto const credKey = - credentialKeylet(subject, issuer, credType); + credentials::keylet(subject, issuer, credType); BEAST_EXPECT(!env.le(credKey)); BEAST_EXPECT(!ownerCount(env, subject)); BEAST_EXPECT(!ownerCount(env, issuer)); @@ -438,7 +428,7 @@ struct Credentials_test : public beast::unit_test::suite env.close(); { auto const credKey = - credentialKeylet(subject, issuer, credType); + credentials::keylet(subject, issuer, credType); BEAST_EXPECT(!env.le(credKey)); BEAST_EXPECT(!ownerCount(env, subject)); BEAST_EXPECT(!ownerCount(env, issuer)); diff --git a/src/test/app/Vault_test.cpp b/src/test/app/Vault_test.cpp index 98f245cc02..260837565e 100644 --- a/src/test/app/Vault_test.cpp +++ b/src/test/app/Vault_test.cpp @@ -19,8 +19,10 @@ #include #include +#include #include #include +#include #include #include #include @@ -605,6 +607,145 @@ class Vault_test : public beast::unit_test::suite }); } + void + testWithDomainCheck() + { + testcase("private vault"); + + Env env{*this}; + Account issuer{"issuer"}; + Account owner{"owner"}; + Account depositor{"depositor"}; + Account pdOwner{"pdOwner"}; + Account credIssuer1{"credIssuer1"}; + Account credIssuer2{"credIssuer2"}; + std::string const credType = "credential"; + auto vault = env.vault(); + env.fund( + XRP(1000), + issuer, + owner, + depositor, + pdOwner, + credIssuer1, + credIssuer2); + env.close(); + env(fset(issuer, asfAllowTrustLineClawback)); + env.close(); + env.require(flags(issuer, asfAllowTrustLineClawback)); + + PrettyAsset asset{xrpIssue(), 1'000'000}; + auto [tx, keylet] = vault.create( + {.owner = owner, .asset = asset, .flags = tfVaultPrivate}); + env(tx); + env.close(); + BEAST_EXPECT(env.le(keylet)); + + { + testcase("private vault owner can deposit"); + auto tx = vault.deposit( + {.depositor = owner, .id = keylet.key, .amount = asset(50)}); + env(tx); + } + + { + testcase("private vault depositor not authorized yet"); + auto tx = vault.deposit( + {.depositor = depositor, + .id = keylet.key, + .amount = asset(50)}); + env(tx, ter{tecNO_AUTH}); + } + + { + testcase("private vault set domainId"); + + pdomain::Credentials const credentials{ + {.issuer = credIssuer1, .credType = credType}, + {.issuer = credIssuer2, .credType = credType}}; + + env(pdomain::setTx(pdOwner, credentials)); + auto const domainId = [&]() { + auto tx = env.tx()->getJson(JsonOptions::none); + return pdomain::getNewDomain(env.meta()); + }(); + + auto tx = vault.set({.owner = owner, .id = keylet.key}); + tx[sfDomainID] = to_string(domainId); + env(tx); + env.close(); + } + + { + testcase("private vault depositor still not authorized"); + auto tx = vault.deposit( + {.depositor = depositor, + .id = keylet.key, + .amount = asset(50)}); + env(tx, ter{tecNO_AUTH}); + env.close(); + } + + auto const credKeylet = + credentials::keylet(depositor, credIssuer1, credType); + { + testcase("private vault depositor now authorized"); + env(credentials::create(depositor, credIssuer1, credType)); + env(credentials::accept(depositor, credIssuer1, credType)); + env.close(); + auto credSle = env.le(credKeylet); + BEAST_EXPECT(credSle != nullptr); + + auto tx = vault.deposit( + {.depositor = depositor, + .id = keylet.key, + .amount = asset(50)}); + env(tx); + env.close(); + } + + { + testcase("private vault depositor lost authorization"); + env(credentials::deleteCred( + credIssuer1, depositor, credIssuer1, credType)); + env.close(); + auto credSle = env.le(credKeylet); + BEAST_EXPECT(credSle == nullptr); + + auto tx = vault.deposit( + {.depositor = depositor, + .id = keylet.key, + .amount = asset(50)}); + env(tx, ter{tecNO_AUTH}); + } + + { + testcase("private vault depositor new authorization"); + env(credentials::create(depositor, credIssuer2, credType)); + env(credentials::accept(depositor, credIssuer2, credType)); + env.close(); + + auto tx = vault.deposit( + {.depositor = depositor, + .id = keylet.key, + .amount = asset(50)}); + env(tx); + } + + { + testcase("private vault no authorization needed to withdraw"); + env(credentials::deleteCred( + depositor, depositor, credIssuer2, credType)); + env.close(); + + auto tx = vault.withdraw( + {.depositor = depositor, + .id = keylet.key, + .amount = asset(100)}); + env(tx); + } + } + public: void run() override @@ -614,6 +755,7 @@ public: testCreateFailIOU(); testCreateFailMPT(); testWithMPT(); + testWithDomainCheck(); } }; diff --git a/src/test/jtx/credentials.h b/src/test/jtx/credentials.h index 2f5c63dccb..b0ac387ab3 100644 --- a/src/test/jtx/credentials.h +++ b/src/test/jtx/credentials.h @@ -29,6 +29,16 @@ namespace jtx { namespace credentials { +inline Keylet +keylet( + test::jtx::Account const& subject, + test::jtx::Account const& issuer, + std::string_view credType) +{ + return keylet::credential( + subject.id(), issuer.id(), Slice(credType.data(), credType.size())); +} + // Sets the optional URI. class uri { diff --git a/src/xrpld/app/misc/CredentialHelpers.cpp b/src/xrpld/app/misc/CredentialHelpers.cpp index 62d4720480..0e43ae602f 100644 --- a/src/xrpld/app/misc/CredentialHelpers.cpp +++ b/src/xrpld/app/misc/CredentialHelpers.cpp @@ -205,9 +205,10 @@ validDomain(ReadView const& view, uint256 domainID, AccountID const& subject) return tefINTERNAL; auto const issuer = h.getAccountID(sfIssuer); - auto const type = makeSlice(h.getFieldVL(sfCredentialType)); - auto const sleCredential = - view.read(keylet::credential(subject, issuer, type)); + auto const type = h.getFieldVL(sfCredentialType); + auto const keyletCredential = + keylet::credential(subject, issuer, makeSlice(type)); + auto const sleCredential = view.read(keyletCredential); // We cannot delete expired credentials, that would require ApplyView& // However we can check if credentials are expired. Expected transaction @@ -228,7 +229,7 @@ validDomain(ReadView const& view, uint256 domainID, AccountID const& subject) } } - return foundExpired ? tecEXPIRED : tecNO_PERMISSION; + return foundExpired ? tecEXPIRED : tecNO_AUTH; } TER @@ -338,8 +339,9 @@ verifyValidDomain( return tefINTERNAL; auto const issuer = h.getAccountID(sfIssuer); - auto const type = makeSlice(h.getFieldVL(sfCredentialType)); - auto const keyletCredential = keylet::credential(account, issuer, type); + auto const type = h.getFieldVL(sfCredentialType); + auto const keyletCredential = + keylet::credential(account, issuer, makeSlice(type)); if (view.exists(keyletCredential)) credentials.push_back(keyletCredential.key); } diff --git a/src/xrpld/app/tx/detail/VaultDeposit.cpp b/src/xrpld/app/tx/detail/VaultDeposit.cpp index cf7b41f436..ddf04901b0 100644 --- a/src/xrpld/app/tx/detail/VaultDeposit.cpp +++ b/src/xrpld/app/tx/detail/VaultDeposit.cpp @@ -119,7 +119,8 @@ VaultDeposit::doApply() auto const& vaultAccount = vault->at(sfAccount); MPTIssue const mptIssue(mptIssuanceID); - if (vault->getFlags() == tfVaultPrivate) + // Note, vault owner is always authorized + if (account_ != vault->at(sfOwner) && (vault->getFlags() & tfVaultPrivate)) { if (auto const err = enforceMPTokenAuthorization( ctx_.view(), mptIssue, account_, mPriorBalance, j_); @@ -130,7 +131,7 @@ VaultDeposit::doApply() { // No authorization needed, but must ensure there is MPToken auto sleMpt = view().read(keylet::mptoken(mptIssuanceID, account_)); - if (!sleMpt && account_ != vaultAccount) + if (!sleMpt) { if (auto const err = MPTokenAuthorize::authorize( view(), diff --git a/src/xrpld/app/tx/detail/VaultSet.cpp b/src/xrpld/app/tx/detail/VaultSet.cpp index 2f9b9cf645..768d4ebca3 100644 --- a/src/xrpld/app/tx/detail/VaultSet.cpp +++ b/src/xrpld/app/tx/detail/VaultSet.cpp @@ -95,6 +95,11 @@ VaultSet::doApply() if (!vault) return tecOBJECT_NOT_FOUND; + auto const mptIssuanceID = (*vault)[sfMPTokenIssuanceID]; + auto const sleIssuance = view().peek(keylet::mptIssuance(mptIssuanceID)); + if (!sleIssuance) + return tefINTERNAL; + // Update mutable flags and fields if given. if (tx.isFieldPresent(sfData)) vault->at(sfData) = tx[sfData]; @@ -108,8 +113,10 @@ VaultSet::doApply() { // In VaultSet::preclaim we enforce that tfVaultPrivate must have been // set in the vault. We currently do not support making such a vault - // public (i.e. removal of tfVaultPrivate flag) - vault->setFieldH256(sfDomainID, tx.getFieldH256(sfDomainID)); + // public (i.e. removal of tfVaultPrivate flag). The sfDomainID flag + // must be set in the MPTokenIssuance object and can be freely updated. + sleIssuance->setFieldH256(sfDomainID, tx.getFieldH256(sfDomainID)); + view().update(sleIssuance); } view().update(vault); diff --git a/src/xrpld/app/tx/detail/VaultWithdraw.cpp b/src/xrpld/app/tx/detail/VaultWithdraw.cpp index c0b133acfd..3eb32b22fb 100644 --- a/src/xrpld/app/tx/detail/VaultWithdraw.cpp +++ b/src/xrpld/app/tx/detail/VaultWithdraw.cpp @@ -116,7 +116,7 @@ VaultWithdraw::doApply() account_, share, FreezeHandling::fhZERO_IF_FROZEN, - AuthHandling::ahZERO_IF_UNAUTHORIZED, + AuthHandling::ahIGNORE_AUTH, j_) < shares) { return tecINSUFFICIENT_FUNDS; diff --git a/src/xrpld/ledger/detail/View.cpp b/src/xrpld/ledger/detail/View.cpp index 6bb20343e7..203191a4a3 100644 --- a/src/xrpld/ledger/detail/View.cpp +++ b/src/xrpld/ledger/detail/View.cpp @@ -2216,7 +2216,7 @@ requireAuth( return tesSUCCESS; } - // err = tefINTERNAL | tecINVALID_DOMAIN | tecNO_PERMISSION | tecEXPIRED + // err = tefINTERNAL | tecINVALID_DOMAIN | tecNO_AUTH | tecEXPIRED if (auto const err = credentials::validDomain(view, *maybeDomainID, account); !isTesSuccess(err)) @@ -2239,6 +2239,10 @@ enforceMPTokenAuthorization( if (!sleIssuance) return tefINTERNAL; // Should have called requireAuth earlier + XRPL_ASSERT( + sleIssuance->getFieldU32(sfFlags) & lsfMPTRequireAuth, + "ripple::verifyAuth : MPTokenIssuance requires authorization"); + if (account == sleIssuance->at(sfIssuer)) return tesSUCCESS; // Won't create MPToken for the token issuer @@ -2250,8 +2254,15 @@ enforceMPTokenAuthorization( if (domainOwned || sleToken == nullptr) { // We check DomainID if: + // // 1. Token not found or // 2. Token found and has lsfMPTDomainCheck flag + // + // In case 1. we check authorization in order to create the token + // (below), but only if the account is authorized by the domain. In + // case 2. we re-check authorization in case the credentials are + // expired, in which case the token needs to be unauthorized (below) + auto const maybeDomainID = sleIssuance->at(~sfDomainID); authorizedByDomain = maybeDomainID.has_value() && verifyValidDomain(view, account, *maybeDomainID, j) == tesSUCCESS; @@ -2260,33 +2271,47 @@ enforceMPTokenAuthorization( if (!authorizedByDomain && sleToken == nullptr) { // Intentionally empty. This could be either of: - // 1. DomainID not set in MPTokenIssuance or - // 2. account has no matching and accepted credentials or - // 3. only expired credentials (removed in verifyValidDomain) - // Either way, move to check at the end of this function + // + // 1. Field sfDomainID not set in MPTokenIssuance or + // 2. Account has no matching and accepted credentials or + // 3. Account has all expired credentials (removed in verifyValidDomain) + // + // Either way, will return tecNO_AUTH at the end of this function } else if (!authorizedByDomain && domainOwned) { - auto sleMpt = view.peek(keylet::mptoken(mptIssuanceID, account)); - XRPL_ASSERT(sleMpt, "ripple::verifyAuth : non-null old MPToken"); - std::uint32_t const flags = sleMpt->getFieldU32(sfFlags); - // Remove lsfMPTAuthorized flag - sleMpt->setFieldU32(sfFlags, flags & ~lsfMPTAuthorized); - view.update(sleMpt); + // We found an MPToken with lsfMPTDomainCheck flag, but the account is + // no longer authorized. + if (sleToken->getFieldU32(sfFlags) & lsfMPTAuthorized) + { + // Must reset lsfMPTAuthorized, no current credentials + auto sleMpt = view.peek(keylet::mptoken(mptIssuanceID, account)); + XRPL_ASSERT(sleMpt, "ripple::verifyAuth : non-null bad MPToken"); + std::uint32_t const flags = sleMpt->getFieldU32(sfFlags); + sleMpt->setFieldU32(sfFlags, flags & ~lsfMPTAuthorized); + view.update(sleMpt); - sleToken = nullptr; + sleToken = nullptr; // return tecNO_AUTH at the end of function + } } - else if (authorizedByDomain && sleToken) + else if (!authorizedByDomain) { XRPL_ASSERT( - sleToken->getFlags() & lsfMPTDomainCheck, - "ripple::verifyAuth : MPToken owned by domain"); + sleToken != nullptr && !domainOwned, + "ripple::verifyAuth : MPToken not owned by domain"); + // MPToken was created by other means, we will check its authorization + // at the end of this function. No need to do anything here. + } + else if (authorizedByDomain && sleToken != nullptr) + { + XRPL_ASSERT( + domainOwned, "ripple::verifyAuth : MPToken owned by domain"); if ((sleToken->getFlags() & lsfMPTAuthorized) == 0) { // Must set lsfMPTAuthorized, we found new credentials auto sleMpt = view.peek(keylet::mptoken(mptIssuanceID, account)); - XRPL_ASSERT(sleMpt, "ripple::verifyAuth : non-null new MPToken"); + XRPL_ASSERT(sleMpt, "ripple::verifyAuth : non-null good MPToken"); std::uint32_t const flags = sleMpt->getFieldU32(sfFlags); sleMpt->setFieldU32(sfFlags, flags | lsfMPTAuthorized); view.update(sleMpt); @@ -2310,7 +2335,7 @@ enforceMPTokenAuthorization( return err; auto sleMpt = view.peek(keylet::mptoken(mptIssuanceID, account)); - XRPL_ASSERT(sleMpt, "ripple::verifyAuth : found new MPToken"); + XRPL_ASSERT(sleMpt, "ripple::verifyAuth : non-null new MPToken"); std::uint32_t const flags = sleMpt->getFieldU32(sfFlags); sleMpt->setFieldU32( sfFlags, flags | lsfMPTDomainCheck | lsfMPTAuthorized); @@ -2319,11 +2344,7 @@ enforceMPTokenAuthorization( sleToken = sleMpt; // with lsfMPTAuthorized } - if (!sleToken) - return tecNO_AUTH; - - if (sleIssuance->getFieldU32(sfFlags) & lsfMPTRequireAuth && - !(sleToken->getFlags() & lsfMPTAuthorized)) + if (sleToken == nullptr || (sleToken->getFlags() & lsfMPTAuthorized) == 0) return tecNO_AUTH; return tesSUCCESS;