Re-execute auth policies on conflict

include/ccf/endpoint_context.h

@@ -22,11 +22,8 @@ namespace ccf::endpoints
   // Commands are endpoints which do not interact with the kv, even to read
   struct CommandEndpointContext
   {
-    CommandEndpointContext(
-      const std::shared_ptr<ccf::RpcContext>& r,
-      std::unique_ptr<AuthnIdentity>&& c) :
-      rpc_ctx(r),
-      caller(std::move(c))
+    CommandEndpointContext(const std::shared_ptr<ccf::RpcContext>& r) :
+      rpc_ctx(r)
     {}
 
     std::shared_ptr<ccf::RpcContext> rpc_ctx;
@@ -54,11 +51,8 @@ namespace ccf::endpoints
 
   struct EndpointContext : public CommandEndpointContext
   {
-    EndpointContext(
-      const std::shared_ptr<ccf::RpcContext>& r,
-      std::unique_ptr<AuthnIdentity>&& c,
-      kv::Tx& t) :
-      CommandEndpointContext(r, std::move(c)),
+    EndpointContext(const std::shared_ptr<ccf::RpcContext>& r, kv::Tx& t) :
+      CommandEndpointContext(r),
       tx(t)
     {}
 
@@ -70,10 +64,8 @@ namespace ccf::endpoints
   struct ReadOnlyEndpointContext : public CommandEndpointContext
   {
     ReadOnlyEndpointContext(
-      const std::shared_ptr<ccf::RpcContext>& r,
-      std::unique_ptr<AuthnIdentity>&& c,
-      kv::ReadOnlyTx& t) :
-      CommandEndpointContext(r, std::move(c)),
+      const std::shared_ptr<ccf::RpcContext>& r, kv::ReadOnlyTx& t) :
+      CommandEndpointContext(r),
       tx(t)
     {}
 

src/endpoints/endpoint_registry.cpp

@@ -159,8 +159,8 @@ namespace ccf::endpoints
              method,
              verb,
              [f](EndpointContext& ctx) {
-               ReadOnlyEndpointContext ro_ctx(
-                 ctx.rpc_ctx, std::move(ctx.caller), ctx.tx);
+               ReadOnlyEndpointContext ro_ctx(ctx.rpc_ctx, ctx.tx);
+               ro_ctx.caller = std::move(ctx.caller);
                f(ro_ctx);
              },
              ap)

src/http/http_builder.h

@@ -117,6 +117,11 @@ namespace http
       set_path(p);
     }
 
+    void set_method(llhttp_method m)
+    {
+      method = m;
+    }
+
     llhttp_method get_method() const
     {
       return method;

src/kv/apply_changes.h

@@ -69,12 +69,15 @@ namespace kv
     }
 
     bool ok = true;
-    for (auto it = views.begin(); it != views.end(); ++it)
+    if (has_writes)
     {
-      if (!it->second->prepare(track_read_versions))
+      for (auto it = views.begin(); it != views.end(); ++it)
       {
-        ok = false;
-        break;
+        if (!it->second->prepare(track_read_versions))
+        {
+          ok = false;
+          break;
+        }
       }
     }
 

src/kv/test/kv_test.cpp

@@ -2354,6 +2354,78 @@ TEST_CASE("Conflict resolution")
   REQUIRE_THROWS(tx2.commit());
 }
 
+TEST_CASE("Cross-map conflicts")
+{
+  kv::Store kv_store;
+  auto encryptor = std::make_shared<kv::NullTxEncryptor>();
+  kv_store.set_encryptor(encryptor);
+  MapTypes::StringString source("public:source");
+  MapTypes::StringString dest("public:dest");
+
+  {
+    INFO("Set initial state");
+
+    auto tx = kv_store.create_tx();
+    auto source_handle = tx.wo(source);
+    source_handle->put("hello", "world");
+    REQUIRE(tx.commit() == kv::CommitResult::SUCCESS);
+  }
+
+  {
+    INFO("Start an operation copying a value across tables");
+    auto copy_tx = kv_store.create_tx();
+    {
+      auto src_handle = copy_tx.ro(source);
+      auto dst_handle = copy_tx.wo(dest);
+      const auto v = src_handle->get("hello");
+      REQUIRE(v.has_value());
+      dst_handle->put("hello", v.value());
+    }
+
+    INFO(
+      "Before the copy commits, another operation changes the source, and "
+      "commits");
+    {
+      auto interfere_tx = kv_store.create_tx();
+      auto src_handle = interfere_tx.wo(source);
+      src_handle->put("hello", "alice");
+      REQUIRE(interfere_tx.commit() == kv::CommitResult::SUCCESS);
+    }
+
+    INFO("Copying operation should conflict on commit");
+    REQUIRE(copy_tx.commit() == kv::CommitResult::FAIL_CONFLICT);
+  }
+
+  {
+    INFO("Start an operation moving a value across tables");
+    auto move_tx = kv_store.create_tx();
+    {
+      auto src_handle = move_tx.rw(source);
+      auto dst_handle = move_tx.wo(dest);
+      const auto v = src_handle->get("hello");
+      REQUIRE(v.has_value());
+      dst_handle->put("hello", v.value());
+
+      // Unlike copy, this operation destroys the source! That should not change
+      // its conflict set
+      src_handle->remove("hello");
+    }
+
+    INFO(
+      "Before the move commits, another operation changes the source, and "
+      "commits");
+    {
+      auto interfere_tx = kv_store.create_tx();
+      auto src_handle = interfere_tx.wo(source);
+      src_handle->put("hello", "bob");
+      REQUIRE(interfere_tx.commit() == kv::CommitResult::SUCCESS);
+    }
+
+    INFO("Moving operation should conflict on commit");
+    REQUIRE(move_tx.commit() == kv::CommitResult::FAIL_CONFLICT);
+  }
+}
+
 std::string rand_string(size_t i)
 {
   return fmt::format("{}: {}", i, rand());

src/kv/untyped_map.h

@@ -115,10 +115,6 @@ namespace kv::untyped
       bool prepare(bool track_read_versions) override
       {
         auto& roll = map.get_roll();
-        if (change_set.writes.empty())
-        {
-          return true;
-        }
 
         // If the parent map has rolled back since this transaction began, this
         // transaction must fail.

src/node/rpc/frontend.h

@@ -252,31 +252,6 @@ namespace ccf
 
       try
       {
-        std::unique_ptr<AuthnIdentity> identity = nullptr;
-
-        // If any auth policy was required, check that at least one is accepted
-        if (!endpoint->authn_policies.empty())
-        {
-          std::string auth_error_reason;
-          for (const auto& policy : endpoint->authn_policies)
-          {
-            identity = policy->authenticate(tx, ctx, auth_error_reason);
-            if (identity != nullptr)
-            {
-              break;
-            }
-          }
-
-          if (identity == nullptr)
-          {
-            // If none were accepted, let the last set an error
-            endpoint->authn_policies.back()->set_unauthenticated_error(
-              ctx, std::move(auth_error_reason));
-            update_metrics(ctx, endpoint);
-            return ctx->serialise_response();
-          }
-        }
-
         update_history();
 
         const bool is_primary = (consensus == nullptr) ||
@@ -316,7 +291,7 @@ namespace ccf
           }
         }
 
-        auto args = endpoints::EndpointContext(ctx, std::move(identity), tx);
+        auto args = endpoints::EndpointContext(ctx, tx);
 
         size_t attempts = 0;
         constexpr auto max_attempts = 30;
@@ -342,6 +317,37 @@ namespace ccf
               pre_exec(tx, *ctx.get());
             }
 
+            // Check auth policies
+            {
+              std::unique_ptr<AuthnIdentity> identity = nullptr;
+
+              // If any auth policy was required, check that at least one is
+              // accepted
+              if (!endpoint->authn_policies.empty())
+              {
+                std::string auth_error_reason;
+                for (const auto& policy : endpoint->authn_policies)
+                {
+                  identity = policy->authenticate(tx, ctx, auth_error_reason);
+                  if (identity != nullptr)
+                  {
+                    break;
+                  }
+                }
+
+                if (identity == nullptr)
+                {
+                  // If none were accepted, let the last set an error
+                  endpoint->authn_policies.back()->set_unauthenticated_error(
+                    ctx, std::move(auth_error_reason));
+                  update_metrics(ctx, endpoint);
+                  return ctx->serialise_response();
+                }
+              }
+
+              args.caller = std::move(identity);
+            }
+
             endpoints.execute_endpoint(endpoint, args);
 
             if (!ctx->should_apply_writes())