From 9331760d9e5d566ed0b43337c764977483293531 Mon Sep 17 00:00:00 2001
From: kjnilsson <knilsson@pivotal.io>
Date: Thu, 30 Jun 2016 11:04:21 +0100
Subject: [PATCH] Treat noSuchObject responses as 'false` during 'or' and 'and'
 clause evaluations

---
 .../src/rabbit_auth_backend_ldap.erl          | 10 +++--
 .../test/system_SUITE.erl                     | 40 +++++++++++++++++--
 2 files changed, 43 insertions(+), 7 deletions(-)

diff --git a/deps/rabbitmq_auth_backend_ldap/src/rabbit_auth_backend_ldap.erl b/deps/rabbitmq_auth_backend_ldap/src/rabbit_auth_backend_ldap.erl
index 26c5a09f96..7915caf902 100644
--- a/deps/rabbitmq_auth_backend_ldap/src/rabbit_auth_backend_ldap.erl
+++ b/deps/rabbitmq_auth_backend_ldap/src/rabbit_auth_backend_ldap.erl
@@ -175,15 +175,17 @@ evaluate0({'not', SubQuery}, Args, User, LDAP) ->
     not R;
 
 evaluate0({'and', Queries}, Args, User, LDAP) when is_list(Queries) ->
-    R = lists:foldl(fun (Q,  true)  -> evaluate(Q, Args, User, LDAP);
-                        (_Q, false) -> false
+    R = lists:foldl(fun (Q,  true)    -> evaluate(Q, Args, User, LDAP);
+                        % Treat any non-true result as false
+                        (_Q, _Result) -> false
                     end, true, Queries),
     ?L1("'and' result: ~s", [R]),
     R;
 
 evaluate0({'or', Queries}, Args, User, LDAP) when is_list(Queries) ->
-    R = lists:foldl(fun (_Q, true)  -> true;
-                        (Q,  false) -> evaluate(Q, Args, User, LDAP)
+    R = lists:foldl(fun (_Q, true)    -> true;
+                        % Treat any non-true result as false
+                        (Q,  _Result) -> evaluate(Q, Args, User, LDAP)
                     end, false, Queries),
     ?L1("'or' result: ~s", [R]),
     R;
diff --git a/deps/rabbitmq_auth_backend_ldap/test/system_SUITE.erl b/deps/rabbitmq_auth_backend_ldap/test/system_SUITE.erl
index 25bdd5d794..58f8ce18bb 100644
--- a/deps/rabbitmq_auth_backend_ldap/test/system_SUITE.erl
+++ b/deps/rabbitmq_auth_backend_ldap/test/system_SUITE.erl
@@ -103,7 +103,9 @@ groups() ->
                                 internal_followed_ldap_and_internal,
                                 tag_attribution_ldap_only,
                                 tag_attribution_ldap_and_internal,
-                                tag_attribution_internal_followed_by_ldap_and_internal
+                                tag_attribution_internal_followed_by_ldap_and_internal,
+                                invalid_or_clause_ldap_only,
+                                invalid_and_clause_ldap_only
                                ]}
     ].
 
@@ -218,7 +220,7 @@ ldap_only(Config) ->
 
 ldap_and_internal(Config) ->
     ok = rabbit_ct_broker_helpers:rpc(Config, 0,
-           application, set_env, [rabbit, auth_backends, 
+           application, set_env, [rabbit, auth_backends,
                                   [{rabbit_auth_backend_ldap, rabbit_auth_backend_internal}]]),
     login(Config),
     permission_match(Config),
@@ -227,7 +229,7 @@ ldap_and_internal(Config) ->
 
 internal_followed_ldap_and_internal(Config) ->
     ok = rabbit_ct_broker_helpers:rpc(Config, 0,
-           application, set_env, [rabbit, auth_backends, 
+           application, set_env, [rabbit, auth_backends,
                                   [rabbit_auth_backend_internal, {rabbit_auth_backend_ldap, rabbit_auth_backend_internal}]]),
     login(Config),
     permission_match(Config),
@@ -257,6 +259,23 @@ tag_attribution_internal_followed_by_ldap_and_internal(Config) ->
     tag_check(Config, <<"Edward">>, <<"password">>,
                [monitor, normal] ++ internal_authorization_tags()).
 
+invalid_or_clause_ldap_only(Config) ->
+    set_env(Config, vhost_access_query_or_in_group()),
+    ok = rabbit_ct_broker_helpers:rpc(Config, 0,
+           application, set_env, [rabbit, auth_backends, [rabbit_auth_backend_ldap]]),
+    B = #amqp_params_network{port = rabbit_ct_broker_helpers:get_node_config(Config, 0, tcp_port_amqp)},
+    {ok, C} = amqp_connection:start(B?ALICE),
+    ok = amqp_connection:close(C).
+
+invalid_and_clause_ldap_only(Config) ->
+    set_env(Config, vhost_access_query_and_in_group()),
+    ok = rabbit_ct_broker_helpers:rpc(Config, 0,
+           application, set_env, [rabbit, auth_backends, [rabbit_auth_backend_ldap]]),
+    B = #amqp_params_network{port = rabbit_ct_broker_helpers:get_node_config(Config, 0, tcp_port_amqp)},
+    % NB: if the query crashes the ldap plugin it returns {error, access_refused}
+    % This may not be a reliable return value assertion
+    {error, not_allowed} = amqp_connection:start(B?ALICE).
+
 %%--------------------------------------------------------------------
 
 login(Config) ->
@@ -370,6 +389,21 @@ posix_vhost_access_multiattr_env() ->
                 {attribute, "${user_dn}","memberOf"}}
               ]}}].
 
+vhost_access_query_or_in_group() ->
+    [{vhost_access_query,
+      {'or', [
+            {in_group, "cn=bananas,ou=groups,dc=rabbitmq,dc=com"},
+            {in_group, "cn=apples,ou=groups,dc=rabbitmq,dc=com"},
+            {in_group, "cn=wheel,ou=groups,dc=rabbitmq,dc=com"}
+             ]}}].
+
+vhost_access_query_and_in_group() ->
+    [{vhost_access_query,
+      {'and', [
+            {in_group, "cn=bananas,ou=groups,dc=rabbitmq,dc=com"},
+            {in_group, "cn=wheel,ou=groups,dc=rabbitmq,dc=com"}
+             ]}}].
+
 vhost_access_query_nested_groups_env() ->
     [{vhost_access_query, {in_group_nested, "cn=admins,ou=groups,dc=rabbitmq,dc=com"}}].