DomainID authorization check moved to doApply

Author: Bronek

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,7 +2243,7 @@ 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,