[SPOOL-197] [#111] clients can be added to ACL even if user exist

This addresses three long-standing issues:

1. when a user exists in the Chef Server, it was not possible to add
   a client to an object's ACL if that client had the same name. The
   logic that retrieved actors based on names obeyed org constraints for
   clients, but looked at the global users list without consideration
   for whether or not the users were members of the organization.  This
   has been corrected, so that the presence of a user anywhere in the
   system can no longer block a same-named client from being added to an
   object's ACL.
2. when an actor being added does not exist or is not in the
   organization, the request would fail with a 400 'missing/invalid actor'
   message. It would not give any indication of which actor(s) caused a
   problem. This has been corrected, and the error message now includes the
   list of actor(s) that could not be added.
3. when an actor being added exists as both an org client and a user,
   the same "400 missing/invalid actor" message would be sent.
   Occurrences of this will be reduced now that we restrict the search
   to users in the org, but can still occur if an org-user shares a name
   with a client.   We have changed this to reply with "422 (Unprocessable Entity)".
   The error message explains that the actor name(s) are ambiguous and
   provides the list of names.

   Further updates will provide a means to

The next round of updates will expand the acl API to accept and provide
`clients` and `users` attributes.  Using those attributes instead of
`actors` when updating ACLs will give a workaround for the scenario described above (3)
and will be the preferred method for updating ACLs via the API.

RELEASE_NOTES.md

@@ -4,9 +4,19 @@ This document contains an overview of significant customer-facing changes
 in the release. For a detailed list of changed components, refer to
 [CHANGELOG.md](CHANGELOG.md).
 
-This document contains release notes for the current release and all patches.
+This document contains release notes for the current major release and all patches.
 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.
+
+
+
 ## 12.8.0 (2016-07-06)
 
 ### Chef Server

oc-chef-pedant/lib/pedant/platform.rb

@@ -190,6 +190,10 @@ def cleanup
       delete_org_from_config
     end
 
+    def delete_node(name, org = self.test_org)
+      delete(api_url("/nodes/#{name}", org), self.superuser)
+    end
+
     def create_client(name, org = self.test_org)
       clientname = name.to_s
       payload = { "name" => clientname }
@@ -594,4 +598,5 @@ def gen_rsa_key(name)
       key
     end
   end
+
 end

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

@@ -4,6 +4,13 @@
 describe "ACL API", :acl do
   include Pedant::ACL
 
+    # Generate random string identifier prefixed with current pid
+    # TODO - we have about 6 flavors of this. Let's clean them up
+    # and move them into Platform.
+    def rand_id
+      "#{Process.pid}_#{rand(10**7...10**8).to_s}"
+    end
+
   # (temporarily?) deprecating /users/*/_acl endpoint due to its broken state and lack of usefulness
   skip "/users/<name>/_acl endpoint" do
     let(:username) { platform.admin_user.name }
@@ -43,7 +50,6 @@
       end
     end
 
-
     %w(create read update delete grant).each do |permission|
       context "/users/<user>/_acl/#{permission} endpoint" do
         if (permission == "read")
@@ -724,8 +730,54 @@
     end # context /organizations/_acl/<permission> endpoint
   end # loop (each) over permissions
 
-  context "/<type>/<name>/_acl endpoint" do
+  # Special case that doesn't fit into the generic behaviors above - specifically
+  # when a client & a user of the same name exist, updates to an acl specifying
+  # the common name as the actor should fail with a 422, because we have no
+  # way of knowing if the caller wanted to add the client or the user to the ACL.
+  context "when a client exists with the same name as a user" do
+    let(:admin_requestor){admin_user}
+    let(:requestor){admin_requestor}
+    let(:shared_name) { "pedant-acl-#{rand_id}" }
+    let(:request_url) { api_url("/clients/#{shared_name}/_acl/read") }
+    let(:acl_request_body) {
+      { read: { actors: ['pivotal', shared_name],
+                         groups: ['admins'] } }
+    }
+
+    before :each do
+      @user = platform.create_user(shared_name, associate: false)
+      @client = platform.create_client(shared_name, platform.test_org)
+    end
 
+    after :each do
+      platform.delete_user(@user)
+      platform.delete_client(@client)
+    end
+
+    context "and the user is a member of the organization" do
+      before :each do
+        platform.associate_user_with_org(platform.test_org.name, @user)
+      end
+
+      it "updates of the object ACL results in a 422 due to ambiguous request" do
+        put(request_url, platform.admin_user, payload: acl_request_body)
+          .should look_like(status: 422)
+      end
+    end
+
+    context "and the user is not a member of the organization" do
+      # TODO - once we support separating clients and users in the ACL GET API
+      # response (up next), we can verify that the correct actor was added too.
+      it "there is no ambiguity and the object ACL update succeeds" do
+        put(request_url, platform.admin_user, payload: acl_request_body)
+          .should look_like(status: 200)
+      end
+    end
+
+
+  end
+
+  context "/<type>/<name>/_acl endpoint" do
     # TODO: Sanity check: users don't seem to have any ACLs, or at least, nothing is
     # accessible from external API as far as I can tell:
     # - [jkeiser] Users have ACLs, but they are at /users/NAME/_acl

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

@@ -193,9 +193,11 @@
  {find_group_name_in_authz_ids, <<"SELECT name, authz_id FROM groups WHERE authz_id = ANY($1)">>}.
  {find_group_authz_id_in_names, <<"SELECT authz_id FROM groups WHERE org_id = $1 AND name = ANY($2)">>}.
 
+
   %% cookbook artifact versions
 {insert_cookbook_artifact_version,
   <<"SELECT * FROM insert_cookbook_artifact_version($1, $2, $3, $4, $5, $6, $7, $8, $9)">>}.
+
 {find_cookbook_artifact_version_by_org_name_identifier,
   <<"SELECT cav.id, cav.identifier, cav.metadata, cav.serialized_object, cav.created_at, cav.created_by,"
            "ca.org_id, ca.name, ca.authz_id, ARRAY_AGG(cavc.checksum) AS checksums "
@@ -244,3 +246,23 @@
       "WHERE org_id = $1 "
         "AND checksum = ANY($2) "
    "GROUP BY checksum">>}.
+
+% For each name provided as $1 (array), this will return a row
+% containing the name (name_in), the client authz id (c_authz_id) and the
+% user authz_id of user in the org.
+% If u_authz_id is null, the actor does not exist as a user *in the org*
+% If c_authz_id is null, the actor does not exist as a client in the org.
+% THere is a special case for pivotal, because pivotal is not a member
+% of any org, but is commonly present in ACLs.
+{find_org_actors_by_name,
+  <<"WITH inputs   AS (SELECT unnest($2::text[]) AS name_in),
+          user_org AS (SELECT username, authz_id AS u_authz_id
+                         FROM org_user_associations, users
+                        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
+     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

@@ -182,32 +182,48 @@ has_grant_on(ObjectType, ObjectId, ActorId) ->
 convert_group_names_to_ids(GroupNames, OrgId) ->
     oc_chef_group:find_group_authz_ids(GroupNames, OrgId, fun chef_sql:select_rows/1).
 
-convert_actor_names_to_ids(Names, OrgId) ->
-    ClientIds = oc_chef_group:find_client_authz_ids(Names, OrgId,
-                                                    fun chef_sql:select_rows/1),
-    UserIds = oc_chef_group:find_user_authz_ids(Names, fun chef_sql:select_rows/1),
-    ClientIds ++ UserIds.
-
 names_to_ids(Ace, OrgId) ->
     ActorNames = ej:get({<<"actors">>}, Ace),
     GroupNames = ej:get({<<"groups">>}, Ace),
-    ActorIds = convert_actor_names_to_ids(ActorNames, OrgId),
-    % Check to make sure everything got converted; if something is missing,
-    % there was an invalid actor or group name in the request body
-    case length(ActorNames) == length(ActorIds) of
-        false ->
-            throw(bad_actor);
-        _ ->
+    case fetch_actors(OrgId, ActorNames) of
+        {ok, ActorIds} ->
             GroupIds = convert_group_names_to_ids(GroupNames, OrgId),
             case length(GroupNames) == length(GroupIds) of
                 false ->
                     throw(bad_group);
                 _ ->
                     Ace1 = ej:set({<<"actors">>}, Ace, ActorIds),
                     ej:set({<<"groups">>}, Ace1, GroupIds)
-            end
+            end;
+        {error, Reason} ->
+            throw(Reason)
+    end.
+
+fetch_actors(OrgId, ActorNames) ->
+    {ok, Actors} = oc_chef_authz_db:find_org_actors_by_name(OrgId, ActorNames),
+    {Missing, Remaining} = lists:partition(fun is_missing_actor/1, Actors),
+    {Ambiguous, Valid} = lists:partition(fun is_ambiguous_actor/1, Remaining),
+    case {Valid, Missing, Ambiguous} of
+        {Ids, [], []}      -> {ok, [id_from_record(R) || R <- Ids]};
+        {_, Missing, []}   -> {error, {bad_actor, names_from_records(Missing)}};
+        {_, [], Ambiguous} -> {error, {ambiguous_actor, names_from_records(Ambiguous)}};
+        {_, _, Ambiguous}  -> {error, {ambiguous_actor, names_from_records(Ambiguous)}}
     end.
 
+names_from_records(Records) ->
+    [ Name || {Name, _, _} <- 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.
+
+
+
 %%
 %% Reverse mapping of ids to names (this should have a lot of commonality with groups)
 %%

src/oc_erchef/apps/oc_chef_authz/src/oc_chef_authz_db.erl

@@ -36,7 +36,8 @@
          fetch_group/3,
          make_context/3,
          statements/1,
-         make_org_prefixed_group_name/2
+         make_org_prefixed_group_name/2,
+         find_org_actors_by_name/2
         ]).
 
 -ifdef(TEST).
@@ -182,3 +183,18 @@ fetch_group(#oc_chef_authz_context{reqid = ReqId, server_api_version = ApiVersio
         {error, _} = Error ->
             Error
     end.
+
+find_org_actors_by_name(OrgId, ActorNames) ->
+    case sqerl:select(find_org_actors_by_name, [OrgId, ActorNames]) of
+        {ok, L} when is_list(L) ->
+            % WIP - change this to a record.
+            R = [{proplists:get_value(<<"name_in">>, Row),
+                  proplists:get_value(<<"u_authz_id">>, Row),
+                  proplists:get_value(<<"c_authz_id">>, Row)} || Row <- L],
+            {ok, R};
+        {ok, none} ->
+            {ok, []};
+        {error, Error} ->
+            {error, Error}
+    end.
+

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

@@ -111,15 +111,16 @@ from_json(Req, #base_state{organization_guid = OrgId,
     case update_from_json(AclState, Part, OrgId) of
         forbidden ->
             {{halt, 400}, Req, State};
-        bad_actor ->
-            % We don't actually know which one, so can't use a more specific
-            % message without checking every single object (which we don't yet
-            % do efficiently); we only get back a list of Ids that has a
-            % different length than the list of names we started with.
-            Msg = <<"Invalid/missing actor in request body">>,
-            Msg1 = {[{<<"error">>, [Msg]}]},
-            Req1 = wrq:set_resp_body(chef_json:encode(Msg1), Req),
-            {{halt, 400}, Req1, State#base_state{log_msg = bad_actor}};
+        {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),
+            {{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),
+            {{halt, 400}, Req1, State#base_state{log_msg = {bad_actor, Actors}}};
         bad_group ->
             Msg = <<"Invalid/missing group in request body">>,
             Msg1 = {[{<<"error">>, [Msg]}]},
@@ -133,6 +134,8 @@ from_json(Req, #base_state{organization_guid = OrgId,
 
 %% Internal functions
 
+format_actor_list(Actors) ->
+    gTkkkkgT
 check_json_validity(Part, Ace) ->
   case chef_object_base:strictly_valid(acl_spec(Part), [Part], Ace) of
     ok ->
@@ -156,10 +159,12 @@ update_from_json(#acl_state{type = Type, authz_id = AuthzId, acl_data = Data},
     try
         oc_chef_authz_acl:update_part(Part, Data, Type, AuthzId, OrgId)
     catch
+        throw:{ambiguous_actor, Actors} ->
+            {ambiguous_actor, Actors};
         throw:forbidden ->
             forbidden;
-        throw:bad_actor ->
-            bad_actor;
+        throw:{bad_actor, Actors} ->
+            {bad_actor, Actors};
         throw:bad_group ->
             bad_group
     end.