From 8d2bd136288868ebdaebf03efdf9a5b4b5a4fd37 Mon Sep 17 00:00:00 2001
From: Rob Hoes <rob.hoes@citrix.com>
Date: Thu, 17 Oct 2024 15:24:14 +0000
Subject: [PATCH] xapi_http: unify cases in add_handler

The main difference between the BufIO and FdIO cases was that the former
calls `assert_credentials_ok` with the `callback` in its `~fn`
parameter, while the latter executed the `callback` directly after the
credentials check.

The function `assert_credentials_ok` either calls `fn` or raises an
exception. Well, nearly... It did not actually call `fn` in the unix
socket case, where checks are bypassed. This looks unintended and this
patch corrects it. This only affects the following handlers in xapi,
which use BufIO and require RBAC checks: post_remote_db_access,
post_remote_db_access_v2, get_wlb_report, get_wlb_diagnostics,
get_audit_log. I guess those were simply never used on the unix socket.

The other thing that happens when using `~fn` is that the
function `Rbac_audit.allowed_post_fn_ok` is called after `~fn`. This
writes an "ALLOWED OK" line to the audit log. I don't see a reason not
to do the same in all cases.

The outcome is that now both cases of `add_handler` do the same and only
the channel types are different. In the following commit the two handler
types are joining into a single one, which is now easier.

Signed-off-by: Rob Hoes <rob.hoes@citrix.com>
---
 ocaml/xapi/xapi_http.ml | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

diff --git a/ocaml/xapi/xapi_http.ml b/ocaml/xapi/xapi_http.ml
index 65de926376..93a6e23525 100644
--- a/ocaml/xapi/xapi_http.ml
+++ b/ocaml/xapi/xapi_http.ml
@@ -137,7 +137,7 @@ let assert_credentials_ok realm ?(http_action = realm) ?(fn = Rbac.nofn)
       )
   in
   if Context.is_unix_socket ic then
-    ()
+    fn ()
   (* Connections from unix-domain socket implies you're root on the box, ergo everything is OK *)
   else
     match
@@ -367,7 +367,7 @@ let add_handler (name, handler) =
                 try
                   if check_rbac then (
                     try
-                      (* rbac checks *)
+                      (* session and rbac checks *)
                       assert_credentials_ok name req
                         ~fn:(fun () -> callback req ic context)
                         (Buf_io.fd_of ic)
@@ -395,9 +395,18 @@ let add_handler (name, handler) =
             Debug.with_thread_associated ?client name
               (fun () ->
                 try
-                  if check_rbac then assert_credentials_ok name req ic ;
-                  (* session and rbac checks *)
-                  callback req ic context
+                  if check_rbac then (
+                    try
+                      (* session and rbac checks *)
+                      assert_credentials_ok name req
+                        ~fn:(fun () -> callback req ic context)
+                        ic
+                    with e ->
+                      debug "Leaving RBAC-handler in xapi_http after: %s"
+                        (ExnHelper.string_of_exn e) ;
+                      raise e
+                  ) else (* no rbac checks *)
+                    callback req ic context
                 with Api_errors.Server_error (name, params) as e ->
                   error "Unhandled Api_errors.Server_error(%s, [ %s ])" name
                     (String.concat "; " params) ;