MB-35937: Don't re-check an already authorized command

1) A command passed authorization and is executed, e.g. a sync-write ADD
2) The command returns "would block" (and has set engine-specific)
3) some time passes and ns_server disconnects
4) The engine calls notifyIOComplete
5) The command resumes and is authorized, this time because ns_server is
   down authorization fails and the command returns "no access".
6) ns_server resumes
7) A new sync-write ADD passes authorization and is executed, it
   observes that the engine-specific is set and "short-cuts" the actual
   ADD, it returns success.

In this scenario we have now returned success for the ADD at step 7, yet
the key has not been stored.

To address this issue, update the Cookie object so it can track when
authorization was successful, allowing the resumption of the command
to skip authorization and complete within the engine.

Change-Id: I8e077786b8aadfead849d4f72b8c93450c8dd437
Reviewed-on: http://review.couchbase.org/114815
Tested-by: Build Bot <[email protected]>
Reviewed-by: Trond Norbye <[email protected]>

Author: jimwwalker

daemon/cookie.cc

@@ -522,6 +522,7 @@ void Cookie::initialize(cb::const_byte_buffer header, bool tracing_enabled) {
     tracer.begin(cb::tracing::TraceCode::REQUEST, start);
     ewouldblock = false;
     openTracingContext.clear();
+    authorized = false;
 }
 
 void Cookie::reset() {
@@ -536,6 +537,7 @@ void Cookie::reset() {
     tracer.clear();
     ewouldblock = false;
     openTracingContext.clear();
+    authorized = false;
 }
 
 void Cookie::setOpenTracingContext(cb::const_byte_buffer context) {

daemon/cookie.h

@@ -520,6 +520,26 @@ class Cookie {
         validated = value;
     }
 
+    /**
+     * @return true is setAuthorized has been called
+     */
+    bool isAuthorized() const {
+        return authorized;
+    }
+
+    /**
+     * This method should be used when the command has successfully passed
+     * authorization check(s).
+     *
+     * This exists to assist with correct "would block" command processing,
+     * this method should be used to tag that the authorization process has
+     * returned "success| and the command shouldn't get a second authorization
+     * test when unblocked and re-executed.
+     */
+    void setAuthorized() {
+        authorized = true;
+    }
+
 protected:
     bool enableTracing = false;
     cb::tracing::Tracer tracer;
@@ -608,4 +628,7 @@ class Cookie {
     /// might have multiple cookies in flight and needs to be able to
     /// lock them independently
     uint8_t refcount = 0;
+
+    /// see isAuthorized/setAuthorized
+    bool authorized = false;
 };

daemon/mcbp_executors.cc

@@ -833,7 +833,11 @@ void execute_client_request_packet(Cookie& cookie,
     static McbpPrivilegeChains privilegeChains;
 
     const auto opcode = request.getClientOpcode();
-    const auto res = privilegeChains.invoke(opcode, cookie);
+    auto res = cb::rbac::PrivilegeAccess::Ok;
+    if (!cookie.isAuthorized()) {
+        res = privilegeChains.invoke(opcode, cookie);
+    }
+
     switch (res) {
     case cb::rbac::PrivilegeAccess::Fail:
         LOG_WARNING("{} {}: no access to command {}",
@@ -849,6 +853,7 @@ void execute_client_request_packet(Cookie& cookie,
         }
         return;
     case cb::rbac::PrivilegeAccess::Ok:
+        cookie.setAuthorized();
         handlers[std::underlying_type<cb::mcbp::ClientOpcode>::type(opcode)](
                 cookie);
         return;