Merge pull request #917 from chef/SPOOL-340/granular-object-acl-get

Add a 'detail=granular' query option to GET ACL endpoint

.travis.yml

@@ -75,7 +75,7 @@ matrix:
       before_script: cd oc-chef-pedant
       script: bundle exec rake chef_zero_spec
       env:
-        - "GEMFILE_MOD=\"gem 'rake'; gem 'chef-zero', github: 'chef/chef-zero'\""
+        - "GEMFILE_MOD=\"gem 'rake'; gem 'chef-zero', github: 'chef/chef-zero', branch: 'master' \""
       # Remove things only used by erlang
       install:
       after_failure:
@@ -93,7 +93,7 @@ matrix:
       before_script: cd oc-chef-pedant
       script: bundle exec rake chef_zero_spec
       env:
-        - "GEMFILE_MOD=\"gem 'rake'; gem 'chef-zero', github: 'chef/chef-zero'\""
+        - "GEMFILE_MOD=\"gem 'rake'; gem 'chef-zero', github: 'chef/chef-zero', branch: 'master' \""
         - CHEF_FS=1
       # Remove things only used by erlang
       install:

RELEASE_NOTES.md

@@ -10,21 +10,29 @@ For prior releases, see [PRIOR\_RELEASE\_NOTES.md](PRIOR_RELEASE_NOTES.md).
 ## 12.8.1
 
 ### Chef Server
-* The object ACL API now requires that any users being added to an
-  object's ACL exist in the same organization as the object itself.
-  Existing users that are not organization members and have already been added
-  to an ACL will not be affected, and can still be seen in the GET
+
+#### API Changes
+
+* The object ACL update API (`/organizations/ORG/OBJECT_TYPE/OBJECT_NAME/_acl/PERM`)
+  now requires that any users being added to an object's ACL exist in the same
+  organization as the object itself.  Existing users that are not organization members
+  and have already been added to an ACL will not be affected, and can still be seen in the GET
   response for this API.
 * When a client and a user of the same name exist in the organization,
   the HTTP response code `422` is returned and the error text will
   indicate which client/user was ambiguous.
 * The object ACL API now accepts `clients` and `users`
   fields which will allow you to disambiguate in this situation by separating
-  clients from users in the request. When using this option, the `actors` field must be set to `[]` in the request.
-  The GET API response for object ACL has not been changed.
+  clients from users in the request. When using this option, the `actors` field
+  must be set to `[]` in the request.
 * 400 responses in object ACL updates will now include the specific
   missing or incorrect entities where appropriate.
-
+* The GET API response for object ACL has not been changed by default,
+  but if the query argument `?detail=granular` is provided, the json
+  body will contain `actors` which will be an empty array; and
+  `clients:[...]`, `users:[...]`.  This body is compatible
+  with the PUT API changes, and can be used in that request without
+  modification.
 
 ## 12.8.0 (2016-07-06)
 

oc-chef-pedant/spec/api/account/account_acl_spec.rb

@@ -817,6 +817,8 @@ def rand_id
         # the defaults of defaults, which are overridden below for the different
         # types:
         let(:actors) { ["pivotal", platform.admin_user.name].uniq }
+        let(:users) { ["pivotal", platform.admin_user.name].uniq }
+        let(:clients) { [] }
         let(:groups) { ["admins"] }
         let(:read_groups) { groups }
         let(:update_groups) { groups }
@@ -831,6 +833,14 @@ def rand_id
             "grant" => {"actors" => actors, "groups" => grant_groups}
           }}
 
+        let(:granular_acl_body) {{
+          "create" => {"actors" => [], "users" => users, "clients" => clients, "groups" => groups},
+          "read" => {"actors" => [], "users" => users, "clients" => clients, "groups" => read_groups},
+          "update" => {"actors" => [], "users" => users, "clients" => clients, "groups" => update_groups},
+          "delete" => {"actors" => [], "users" => users, "clients" => clients, "groups" => delete_groups},
+          "grant" => {"actors" => [], "users" => users, "clients" => clients, "groups" => grant_groups},
+        }}
+
         # Mainly this is for the different creation bodies, but also for the
         # different default ACLs for each type, etc.  We love consistency!
         case type
@@ -841,6 +851,9 @@ def rand_id
             # actors list
             ["pivotal", new_object, setup_user.name].uniq
           }
+          let(:clients) { [ new_object ] }
+          let(:users) { ["pivotal", setup_user.name].uniq }
+
           let(:read_groups) { ["users", "admins"] }
           let(:delete_groups) { ["users", "admins"] }
         when "groups"
@@ -854,6 +867,7 @@ def rand_id
               "containerpath" => "/"
             }}
           let(:actors) { [platform.admin_user.name] }
+          let(:users) {  [platform.admin_user.name] }
           let(:groups) { [] }
           let(:grant_groups) { [] }
         when "data"
@@ -968,9 +982,16 @@ def rand_id
             it "can get object ACL" do
               get(request_url, platform.admin_user).should look_like({
                   :status => 200,
-                  :body => acl_body
+                  :body_exact => acl_body
                 })
             end
+            it "can get a granular object ACL" do
+              get("#{request_url}?detail=granular", platform.admin_user).should look_like({
+                  :status => 200,
+                  :body_exact => granular_acl_body
+                })
+
+            end
           end
 
           context "default normal user" do

omnibus/config/projects/chef-server.rb

@@ -24,7 +24,7 @@
 replace         "private-chef"
 conflict        "private-chef"
 install_dir     "/opt/opscode"
-build_version   "12.8.0"
+build_version   "12.8.1"
 build_iteration 1
 
 override :rabbitmq, version: "3.3.4"

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

@@ -30,6 +30,7 @@
          fetch_id/4,
          fetch_cookbook_id/3,
          fetch/2,
+         fetch/3,
          has_grant_on/3,
          validate_actors_clients_users/2,
          update_part/5,
@@ -206,12 +207,15 @@ fetch_cookbook_id(DbContext, Name, OrgId) ->
 
 -spec fetch(chef_type(), id()) -> list() | {error, term()}.
 fetch(Type, AuthzId) ->
+    fetch(Type, AuthzId, undefined).
+
+fetch(Type, AuthzId, Granular) ->
     Path = acl_path(Type, AuthzId),
     SuperuserId = envy:get(oc_chef_authz, authz_superuser_id, binary),
     Result = oc_chef_authz_http:request(Path, get, ?DEFAULT_HEADERS, [], SuperuserId),
     case Result of
         {ok, Record} ->
-            ids_to_names(Record);
+            convert_all_ids_to_names(Record, Granular);
         {error, forbidden} ->
             forbidden;
         Other ->
@@ -329,27 +333,37 @@ convert_actor_ids_to_names(AuthzIds) ->
         oc_chef_group:find_clients_names(AuthzIds, fun chef_sql:select_rows/1),
     {UserNames, DefunctActorAuthzIds} =
         oc_chef_group:find_users_names(RemainingAuthzIds, fun chef_sql:select_rows/1),
-    {ClientNames ++ UserNames, DefunctActorAuthzIds}.
+    {ClientNames, UserNames, DefunctActorAuthzIds}.
+
+
+convert_all_ids_to_names(Record, Granular) ->
+    convert_ids_to_names_in_part([<<"create">>, <<"read">>, <<"update">>,
+                                  <<"delete">>, <<"grant">>],
+                                 Record, Granular).
 
--spec process_part(binary(), ejson_term()) -> ejson_term().
-process_part(Part, Record) ->
+-spec convert_ids_to_names_in_part(list(binary()), ejson_term(), granular|undefined) -> ejson_term().
+convert_ids_to_names_in_part([], Record, _Granular) ->
+    Record;
+convert_ids_to_names_in_part([Part | Rest], Record, Granular) ->
     Members = ej:get({Part}, Record),
     ActorIds = ej:get({<<"actors">>}, Members),
     GroupIds = ej:get({<<"groups">>}, Members),
-    {ActorNames, DefunctActorAuthzIds} = convert_actor_ids_to_names(ActorIds),
+    {ClientNames, UserNames, DefunctActorAuthzIds} = convert_actor_ids_to_names(ActorIds),
     {GroupNames, DefunctGroupAuthzIds} = convert_group_ids_to_names(GroupIds),
-    % We do this for groups, probably good to do it here too
     oc_chef_authz_cleanup:add_authz_ids(DefunctActorAuthzIds, DefunctGroupAuthzIds),
-    Members1 = ej:set({<<"actors">>}, Members, ActorNames),
+    Members1 = part_with_actors(Members, ClientNames, UserNames, Granular),
     Members2 = ej:set({<<"groups">>}, Members1, GroupNames),
-    ej:set({Part}, Record, Members2).
-
-ids_to_names(Record) ->
-    Record1 = process_part(<<"create">>, Record),
-    Record2 = process_part(<<"read">>, Record1),
-    Record3 = process_part(<<"update">>, Record2),
-    Record4 = process_part(<<"delete">>, Record3),
-    process_part(<<"grant">>, Record4).
+    NewRecord = ej:set({Part}, Record, Members2),
+    convert_ids_to_names_in_part(Rest, NewRecord, Granular).
+
+
+part_with_actors(PartRecord, Clients, Users, granular) ->
+    PartRecord0 = ej:set({<<"users">>}, PartRecord, Users),
+    PartRecord1 = ej:set({<<"clients">>}, PartRecord0, Clients),
+    ej:set({<<"actors">>}, PartRecord1, []);
+part_with_actors(PartRecord, Clients, Users, _) ->
+    ej:set({<<"actors">>}, PartRecord, Clients ++ Users).
+
 
 
 % Path helper functions

src/oc_erchef/apps/oc_chef_authz/src/oc_chef_group.erl

@@ -6,7 +6,6 @@
 -module(oc_chef_group).
 
 -include("oc_chef_types.hrl").
--include_lib("mixer/include/mixer.hrl").
 
 -behaviour(chef_object).
 
@@ -380,6 +379,8 @@ find_authz_id_in_names(QueryName, Args, CallbackFun) ->
             Other
     end.
 
+query_and_diff_authz_ids(_QueryName, [], _) ->
+    {[], []};
 query_and_diff_authz_ids(QueryName, AuthzIds, CallbackFun) ->
     case CallbackFun({QueryName, [AuthzIds]}) of
         not_found ->

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

@@ -1,4 +1,4 @@
-%% -*- erlang-indent-level: 4;indent-tabs-mode: nil; fill-column: 92-*-
+%% -*- erlang-indent-level: 4; indent-tabs-mode: nil; fill-column: 92-*-
 %% ex: ts=4 sw=4 et
 %%
 %% @author Marc A. Paradise <[email protected]>
@@ -109,8 +109,21 @@ validate_actors_clients_users_test_() ->
       }
      ].
 
+part_with_actors_test_() ->
+    Subject = fun oc_chef_authz_acl:part_with_actors/4,
+    [
+     {"when 'granular' is set, response includes clients, users, empty actors",
+      ?_assertEqual({[{<<"users">>, [<<"u1">>]},
+                      {<<"clients">>, [<<"c1">>]},
+                      {<<"actors">>, []}]},
+                    Subject({[]}, [<<"c1">>], [<<"u1">>], granular))},
+     {"when 'granular' is not set, response is only 'actors'",
+      ?_assertEqual({[{<<"actors">>, [<<"c1">>, <<"u1">>]}]},
+                    Subject({[]}, [<<"c1">>], [<<"u1">>], undefined))}
+    ].
+
 %%
-%% Helpers for inputs and outputs.
+%% Helpers for test inputs and outputs.
 %%
 
 valid_user_data_response(_, Names) ->

src/oc_erchef/apps/oc_chef_wm/src/chef_wm_util.erl

@@ -163,7 +163,9 @@ not_found_message(policy_group, Name) ->
 
 %% "Cannot load data bag item not_really_there for data bag sack"
 
--spec error_message_envelope(binary() | ejson_term()) -> ejson_term().
+-spec error_message_envelope(binary() | list() | ejson_term()) -> ejson_term().
+error_message_envelope(Message) when is_list(Message) ->
+    error_message_envelope(iolist_to_binary(Message));
 error_message_envelope(Message) when is_binary(Message) orelse
                                      is_tuple(Message) ->
     %% Tuple guard is really intended for grabbing EJson-encoded json objects, but we don't

src/oc_erchef/apps/oc_chef_wm/src/oc_chef_wm_acl.erl

@@ -80,10 +80,14 @@ validate_request('GET', Req, #base_state{chef_db_context = DbContext,
 auth_info(Req, State) ->
     check_acl_auth(Req, State).
 
-to_json(Req, #base_state{resource_state = AclState} = State) ->
-    case fetch(AclState) of
+to_json(Req, #base_state{resource_state = #acl_state{type = Type, authz_id = AuthzId}} = State) ->
+    Granular = case wrq:get_qs_value("detail", Req) of
+                   "granular" -> granular;
+                   _ -> undefined
+               end,
+    case oc_chef_authz_acl:fetch(Type, AuthzId, Granular) of
         forbidden ->
-            {{halt, 403}, Req, State#base_state{log_msg = acl_not_found}};
+            {{halt, 403}, Req, State#base_state{log_msg = requestor_access_failed}};
         Ejson ->
             Json = chef_json:encode(Ejson),
             {Json, Req, State}
@@ -129,8 +133,5 @@ check_acl_auth(Req, #base_state{requestor_id = RequestorId,
             end
     end.
 
-fetch(#acl_state{type = Type, authz_id = AuthzId}) ->
-    oc_chef_authz_acl:fetch(Type,AuthzId).
-
 malformed_request_message(Any, _Req, _State) ->
     error({unexpected_malformed_request_message, Any}).

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

@@ -112,19 +112,24 @@ from_json(Req, #base_state{organization_guid = OrgId,
         forbidden ->
             {{halt, 400}, Req, State};
         {ambiguous_actor, Actors} ->
-            Text = iolist_to_binary([<<"The actor(s) ">>, chef_wm_malformed:bin_str_join(Actors, ", "), <<" exist as both ">>,
-                                     <<"clients and users within this organization.">>]),
-            Req1 = wrq:set_resp_body(chef_json:encode({[{<<"error">>, [Text]}]}), Req),
+            Message = [<<"The following actor(s) exist as both clients and users within this organization: ">>,
+                       chef_wm_malformed:bin_str_join(Actors, ", "),
+                       <<". To perform this update, supply separate 'clients' and 'users' ">>,
+                       <<"fields in your request, and use an empty array for the value of 'actors'.">>],
+            Body = chef_wm_util:error_message_envelope(Message),
+            Req1 = wrq:set_resp_body(chef_json:encode(Body), Req),
             {{halt, 422}, Req1, State#base_state{log_msg = {ambiguous_actor, Actors}}};
         {invalid, Type, Names} ->
-            Text = iolist_to_binary([<<"The ">>, atom_to_list(Type), <<"(s) ">>, chef_wm_malformed:bin_str_join(Names, ", "),
-                                     <<" do not exist in this organization.">>]),
-            Req1 = wrq:set_resp_body(chef_json:encode({[{<<"error">>, [Text]}]}), Req),
+            Body = chef_wm_util:error_message_envelope([<<"The ">>, atom_to_list(Type),
+                                                        <<"(s) ">>, chef_wm_malformed:bin_str_join(Names, ", "),
+                                                        <<" do not exist in this organization.">>]),
+            Req1 = wrq:set_resp_body(chef_json:encode(Body), Req),
             {{halt, 400}, Req1, State#base_state{log_msg = {invalid_object_in_ace, Names}}};
         {bad_actor, Actors} ->
-            Text = iolist_to_binary([<<"The actor(s) ">>, chef_wm_malformed:bin_str_join(Actors, ", "), " do not >>",
-                                     <<"exist in this organization as clients or users.">>]),
-            Req1 = wrq:set_resp_body(chef_json:encode({[{<<"error">>, [Text]}]}), Req),
+            Body = chef_wm_util:error_message_envelope([<<"The actor(s) ">>,
+                                                        chef_wm_malformed:bin_str_join(Actors, ", "),
+                                                        <<" do not exist in this organization as clients or users.">>]),
+            Req1 = wrq:set_resp_body(chef_json:encode(Body), Req),
             {{halt, 400}, Req1, State#base_state{log_msg = {bad_actor, Actors}}};
         _Other ->
             % So we return 200 instead of 204, for backwards compatibility:

src/oc_erchef/include/oc_chef_wm.hrl

@@ -363,7 +363,9 @@
 -record(acl_state, {
           type,
           authz_id,
-          acl_data
+          acl_data,
+          granular :: 'granular' | undefined
+
          }).
 
 -record(association_state, {

src/oc_erchef/rebar.config

@@ -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"},