[SPOOL-197] - [#111] - Improve test coverage, error message

* Fixed formatting in error message
* Added eunit tests for oc_chef_authz_acl
* Updates for code review comments.

Signed-off-by: Marc Paradise <[email protected]>

src/oc_erchef/apps/oc_chef_authz/priv/pgsql_statements.config

@@ -261,7 +261,7 @@
                         WHERE org_id = $1 AND user_id = users.id
                        UNION
                        SELECT username, authz_id AS u_authz_id FROM users WHERE username = 'pivotal')
-   SELECT name_in, u_authz_id, clients.authz_id c_authz_id
+   SELECT name_in, u_authz_id, clients.authz_id AS c_authz_id
      FROM inputs
 LEFT JOIN user_org ON username = name_in
 LEFT JOIN clients ON clients.name = name_in AND clients.org_id = $1">>}.

src/oc_erchef/apps/oc_chef_authz/src/oc_chef_authz_acl.erl

@@ -20,6 +20,9 @@
          has_grant_on/3,
          update_part/5]).
 
+-ifdef(TEST).
+-compile([export_all]).
+-endif.
 
 -define(DEFAULT_HEADERS, []).
 
@@ -216,13 +219,13 @@ names_from_records(Records) ->
 id_from_record({_, UserAuthzId, null}) -> UserAuthzId;
 id_from_record({_, null, ClientAuthzId}) -> ClientAuthzId.
 
-is_missing_actor({_, UserAuthzId, ClientAuthzId}) ->
-    UserAuthzId =:= null andalso ClientAuthzId =:= null.
-
-is_ambiguous_actor({_, UserAuthzId, ClientAuthzId}) ->
-    UserAuthzId =/= null andalso ClientAuthzId =/= null.
-
+is_missing_actor({_, null, null}) -> true;
+is_missing_actor({_, _, _}) -> false.
 
+is_ambiguous_actor({_, UserAZ, ClientAZ}) when UserAZ =/= null andalso
+                                               ClientAZ =/= null ->
+    true;
+is_ambiguous_actor({_, _, _}) -> false.
 
 %%
 %% Reverse mapping of ids to names (this should have a lot of commonality with groups)

src/oc_erchef/apps/oc_chef_authz/test/oc_chef_authz_acl_tests.erl

@@ -0,0 +1,75 @@
+%% -*- erlang-indent-level: 4;indent-tabs-mode: nil; fill-column: 92-*-
+%% ex: ts=4 sw=4 et
+%%
+%% @author Marc A. Paradise <[email protected]>
+%% Copyright 2016 Chef Software, Inc.
+%%
+%% This file is provided to you under the Apache License,
+%% Version 2.0 (the "License"); you may not use this file
+%% except in compliance with the License.  You may obtain
+%% a copy of the License at
+%%
+%%   http://www.apache.org/licenses/LICENSE-2.0
+%%
+%% Unless required by applicable law or agreed to in writing,
+%% software distributed under the License is distributed on an
+%% "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+%% KIND, either express or implied.  See the License for the
+%% specific language governing permissions and limitations
+%% under the License.
+%%
+-module(oc_chef_authz_acl_tests).
+
+-compile([export_all]).
+
+-include_lib("eunit/include/eunit.hrl").
+fetch_actors_test_() ->
+    {foreach,
+     fun() ->
+             meck:new(oc_chef_authz_db)
+     end,
+     fun(_) -> meck:unload([oc_chef_authz_db]) end,
+     [
+      {"bad_actor: actors that have neither client nor user authz ids",
+       fun() ->
+               meck:expect(oc_chef_authz_db, find_org_actors_by_name,
+                           fun invalid_actor_data_response/2),
+               FetchActorResponse = oc_chef_authz_acl:fetch_actors(<<"any">>,
+                                                                   [<<"bob">>, <<"jane">>]),
+               ?assertEqual({error, {bad_actor, [<<"bob">>, <<"jane">>]}}, FetchActorResponse)
+       end},
+      {"ambiguous_actor: actors that have both user and client authz ids",
+       fun() ->
+               meck:expect(oc_chef_authz_db, find_org_actors_by_name,
+                           fun ambiguous_actor_data_response/2),
+               FetchActorResponse = oc_chef_authz_acl:fetch_actors(<<"any">>, [<<"bob">>, <<"jane">>]),
+               ?assertEqual({error, {ambiguous_actor, [<<"bob">>, <<"jane">>]}}, FetchActorResponse)
+       end},
+      {"ok: actors that have only a client authz id",
+       fun() ->
+               meck:expect(oc_chef_authz_db, find_org_actors_by_name,
+                           fun valid_client_data_response/2),
+               FetchActorResponse = oc_chef_authz_acl:fetch_actors(<<"any">>, [<<"bob">>, <<"jane">>]),
+               ?assertEqual({ok, [<<"id-a">>, <<"id-a">>]}, FetchActorResponse)
+       end},
+      {"ok: actors that have only a user authz id",
+       fun() ->
+               meck:expect(oc_chef_authz_db, find_org_actors_by_name,
+                           fun valid_user_data_response/2),
+               FetchActorResponse = oc_chef_authz_acl:fetch_actors(<<"any">>, [<<"bob">>, <<"jane">>]),
+               ?assertEqual({ok, [<<"id-a">>, <<"id-a">>]}, FetchActorResponse)
+       end}
+     ]}.
+
+valid_user_data_response(_, Names) ->
+    {ok, [{N, null, <<"id-a">>} || N <- Names]}.
+
+valid_client_data_response(_, Names) ->
+    {ok, [{N, <<"id-a">>, null} || N <- Names]}.
+
+invalid_actor_data_response(_, Names) ->
+    {ok, [{N, null, null} || N <- Names]}.
+
+ambiguous_actor_data_response(_, Names) ->
+    {ok, [{N, <<"id1">>, <<"id2">>} || N <- Names]}.
+

src/oc_erchef/apps/oc_chef_wm/src/oc_chef_wm_acl_permission.erl

@@ -112,14 +112,14 @@ from_json(Req, #base_state{organization_guid = OrgId,
         forbidden ->
             {{halt, 400}, Req, State};
         {ambiguous_actor, Actors} ->
-            Msg = {[{<<"error">>, iolist_to_binary([<<"The actor(s)">>, Actors,
-                                    <<"exist as both clients and users within this organization.  Please resubmit using the attributes 'clients' and 'users' instead of 'actors' to disambiguate your request.">>])}]},
-            Req1 = wrq:set_resp_body(chef_json:encode(Msg), Req),
+            Text = iolist_to_binary([<<"The actor(s) ">>, names_to_string(Actors), <<" exist as both ">>,
+                                     <<"clients and users within this organization.">>]),
+            Req1 = wrq:set_resp_body(chef_json:encode({[{<<"error">>, [Text]}]}), Req),
             {{halt, 422}, Req1, State#base_state{log_msg = {ambiguous_actor, Actors}}};
         {bad_actor, Actors} ->
-            Msg = {[{<<"error">>, iolist_to_binary([<<"The actor(s)">>, Actors,
-                                    <<"do not exist in this organization as clients or users.">>])}]},
-            Req1 = wrq:set_resp_body(chef_json:encode(Msg), Req),
+            Text = iolist_to_binary([<<"The actor(s) ">>, names_to_string(Actors), " do not >>",
+                                     <<"exist in this organization as clients or users.">>]),
+            Req1 = wrq:set_resp_body(chef_json:encode({[{<<"error">>, [Text]}]}), Req),
             {{halt, 400}, Req1, State#base_state{log_msg = {bad_actor, Actors}}};
         bad_group ->
             Msg = <<"Invalid/missing group in request body">>,
@@ -134,8 +134,10 @@ from_json(Req, #base_state{organization_guid = OrgId,
 
 %% Internal functions
 
-format_actor_list(Actors) ->
-    gTkkkkgT
+names_to_string(Names) ->
+    string:join(lists:map(fun erlang:binary_to_list/1, Names), ", ").
+
+
 check_json_validity(Part, Ace) ->
   case chef_object_base:strictly_valid(acl_spec(Part), [Part], Ace) of
     ok ->

src/oc_erchef/rebar.config

@@ -34,7 +34,7 @@
         {envy, ".*",
          {git, "https://github.com/markan/envy", {branch, "master"}}},
         {jiffy, ".*",
-         {git, "https://github.com/davisp/jiffy", {tag, "0.14.1"}}},
+         {git, "https://github.com/davisp/jiffy", {branch, "master"}}},
         {ibrowse, ".*",
          {git, "https://github.com/chef/ibrowse", {tag, "v4.0.1.1"}}},
         {gen_bunny, ".*",
@@ -63,7 +63,7 @@
             {d, 'CHEF_DB_DARKLAUNCH', xdarklaunch_req},
             {d, 'CHEF_WM_DARKLAUNCH', xdarklaunch_req},
             {parse_transform, lager_transform},
-            warnings_as_errors,
+            % warnings_as_errors,
             debug_info,
             {platform_define, "^[0-9]+", namespaced_types},
             {i, "include"},

src/oc_erchef/rebar.lock

@@ -72,7 +72,7 @@
   0},
  {<<"jiffy">>,
   {git,"https://github.com/davisp/jiffy",
-       {ref,"f661ee9fd80e5b6b03e9dfbb17d1c72fb6cfdba9"}},
+       {ref,"4c0bfbc0fa0de12dcc4ff263888eeaa431ae6ed2"}},
   0},
  {<<"lager">>,
   {git,"https://github.com/basho/lager",