DomainID authorization check moved to doApply

Author: Bronek

src/test/app/Vault_test.cpp

@@ -720,6 +720,7 @@ class Vault_test : public beast::unit_test::suite
                  .id = keylet.key,
                  .amount = asset(50)});
             env(tx, ter{tecNO_AUTH});
+            env.close();
         }
 
         {
@@ -733,6 +734,7 @@ class Vault_test : public beast::unit_test::suite
                  .id = keylet.key,
                  .amount = asset(50)});
             env(tx);
+            env.close();
         }
 
         {

src/xrpld/app/tx/detail/VaultDeposit.cpp

@@ -71,19 +71,27 @@ VaultDeposit::preclaim(PreclaimContext const& ctx)
     if (isFrozen(ctx.view, account, share))
         return tecFROZEN;
 
-    if (vault->getFlags() == tfVaultPrivate && account != vault->at(sfOwner))
+    if ((vault->getFlags() & tfVaultPrivate) && account != vault->at(sfOwner))
     {
+        // The authorization check below is based on DomainID stored in
+        // MPTokenIssuance. Had the vault shares been a regular MPToken, we
+        // would allow authorization granted by the issuer explicitly, but Vault
+        // does not have an MPT issuer (instead it uses pseudo-account, which is
+        // blackholed and cannot create any transactions).
+        //
+        // We also need to do similar check inside doApply(), in order to remove
+        // expired credentials and/or adjust authorization flag on tokens owned
+        // by DomainID (i.e. with lsfMPTDomainCheck flag). This is why we
+        // suppress authorization errors if domainId is set.
+        uint256 domainId = beast::zero;
         auto const err = requireAuth(
-            ctx.view, MPTIssue(vault->at(sfMPTokenIssuanceID)), account);
-        return err;
+            ctx.view,
+            MPTIssue(vault->at(sfMPTokenIssuanceID)),
+            account,
+            &domainId);
 
-        // The above will perform authorization check based on DomainID stored
-        // in MPTokenIssuance. Had this been a regular MPToken, it would also
-        // allow use of authorization granted by the issuer explicitly, but
-        // Vault does not have an MPT issuer (instead it uses pseudo-account).
-        //
-        // If we passed the above check then we also need to do similar check
-        // inside doApply(), in order to check for expired credentials.
+        if (domainId == beast::zero)
+            return err;
     }
 
     return tesSUCCESS;
@@ -120,7 +128,7 @@ VaultDeposit::doApply()
 
     MPTIssue const mptIssue(mptIssuanceID);
     // Note, vault owner is always authorized
-    if (account_ != vault->at(sfOwner) && (vault->getFlags() & tfVaultPrivate))
+    if ((vault->getFlags() & tfVaultPrivate) && account_ != vault->at(sfOwner))
     {
         if (auto const err = enforceMPTokenAuthorization(
                 ctx_.view(), mptIssue, account_, mPriorBalance, j_);

src/xrpld/ledger/View.h

@@ -654,7 +654,8 @@ requireAuth(ReadView const& view, Issue const& issue, AccountID const& account);
 requireAuth(
     ReadView const& view,
     MPTIssue const& mptIssue,
-    AccountID const& account);
+    AccountID const& account,
+    uint256* domainId = nullptr);
 
 /** Check if the account lacks required authorization.
  *

src/xrpld/ledger/detail/View.cpp

@@ -2183,7 +2183,8 @@ TER
 requireAuth(
     ReadView const& view,
     MPTIssue const& mptIssue,
-    AccountID const& account)
+    AccountID const& account,
+    uint256* domainId)
 {
     auto const mptID = keylet::mptIssuance(mptIssue.getMptID());
     auto const sleIssuance = view.read(mptID);
@@ -2220,7 +2221,12 @@ requireAuth(
     if (auto const err =
             credentials::validDomain(view, *maybeDomainID, account);
         !isTesSuccess(err))
+    {
+        if (err != tecINVALID_DOMAIN && domainId != nullptr)
+            (*domainId) = *maybeDomainID;
+
         return err;
+    }
 
     // We are authorized by permissioned domain.
     return tesSUCCESS;
@@ -2237,16 +2243,17 @@ enforceMPTokenAuthorization(
     auto const mptIssuanceID = mptIssue.getMptID();
     auto const sleIssuance = view.read(keylet::mptIssuance(mptIssuanceID));
     if (!sleIssuance)
-        return tefINTERNAL;  // Should have called requireAuth earlier
+        return tefINTERNAL;
 
     XRPL_ASSERT(
-        sleIssuance->getFieldU32(sfFlags) & lsfMPTRequireAuth,
+        sleIssuance->getFlags() & lsfMPTRequireAuth,
         "ripple::verifyAuth : MPTokenIssuance requires authorization");
 
     if (account == sleIssuance->at(sfIssuer))
         return tesSUCCESS;  // Won't create MPToken for the token issuer
 
-    auto sleToken = view.read(keylet::mptoken(mptIssuanceID, account));
+    auto const keylet = keylet::mptoken(mptIssuanceID, account);
+    auto sleToken = view.read(keylet);
     bool const domainOwned =
         (sleToken && (sleToken->getFlags() & lsfMPTDomainCheck));
 
@@ -2282,16 +2289,15 @@ enforceMPTokenAuthorization(
     {
         // We found an MPToken with lsfMPTDomainCheck flag, but the account is
         // no longer authorized.
-        if (sleToken->getFieldU32(sfFlags) & lsfMPTAuthorized)
+        if (sleToken->getFlags() & lsfMPTAuthorized)
         {
             // Must reset lsfMPTAuthorized, no current credentials
-            auto sleMpt = view.peek(keylet::mptoken(mptIssuanceID, account));
+            auto sleMpt = view.peek(keylet);
             XRPL_ASSERT(sleMpt, "ripple::verifyAuth : non-null bad MPToken");
-            std::uint32_t const flags = sleMpt->getFieldU32(sfFlags);
-            sleMpt->setFieldU32(sfFlags, flags & ~lsfMPTAuthorized);
+            sleMpt->clearFlag(lsfMPTAuthorized);
             view.update(sleMpt);
 
-            sleToken = nullptr;  // return tecNO_AUTH at the end of function
+            sleToken = sleMpt;  // without lsfMPTAuthorized
         }
     }
     else if (!authorizedByDomain)
@@ -2310,10 +2316,9 @@ enforceMPTokenAuthorization(
         if ((sleToken->getFlags() & lsfMPTAuthorized) == 0)
         {
             // Must set lsfMPTAuthorized, we found new credentials
-            auto sleMpt = view.peek(keylet::mptoken(mptIssuanceID, account));
+            auto sleMpt = view.peek(keylet);
             XRPL_ASSERT(sleMpt, "ripple::verifyAuth : non-null good MPToken");
-            std::uint32_t const flags = sleMpt->getFieldU32(sfFlags);
-            sleMpt->setFieldU32(sfFlags, flags | lsfMPTAuthorized);
+            sleMpt->setFlag(lsfMPTAuthorized);
             view.update(sleMpt);
 
             sleToken = sleMpt;  // with lsfMPTAuthorized
@@ -2334,11 +2339,9 @@ enforceMPTokenAuthorization(
             !isTesSuccess(err))
             return err;
 
-        auto sleMpt = view.peek(keylet::mptoken(mptIssuanceID, account));
+        auto sleMpt = view.peek(keylet);
         XRPL_ASSERT(sleMpt, "ripple::verifyAuth : non-null new MPToken");
-        std::uint32_t const flags = sleMpt->getFieldU32(sfFlags);
-        sleMpt->setFieldU32(
-            sfFlags, flags | lsfMPTDomainCheck | lsfMPTAuthorized);
+        sleMpt->setFlag(lsfMPTDomainCheck | lsfMPTAuthorized);
         view.update(sleMpt);
 
         sleToken = sleMpt;  // with lsfMPTAuthorized