From 13da1a51567dce9dca1db285a1143bfa675b9c50 Mon Sep 17 00:00:00 2001 From: Marc Paradise Date: Wed, 17 Aug 2016 21:33:16 -0400 Subject: [PATCH] [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 | 12 +++- oc-chef-pedant/lib/pedant/platform.rb | 1 + .../spec/api/account/account_acl_spec.rb | 56 ++++++++++++++++++- .../priv/pgsql_statements.config | 22 ++++++++ .../oc_chef_authz/src/oc_chef_authz_acl.erl | 44 ++++++++++----- .../oc_chef_authz/src/oc_chef_authz_db.erl | 18 +++++- .../src/oc_chef_wm_acl_permission.erl | 27 +++++---- 7 files changed, 151 insertions(+), 29 deletions(-) diff --git a/RELEASE_NOTES.md b/RELEASE_NOTES.md index b993ed23566..dc4f0183e1f 100644 --- a/RELEASE_NOTES.md +++ b/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 diff --git a/oc-chef-pedant/lib/pedant/platform.rb b/oc-chef-pedant/lib/pedant/platform.rb index af1b15f224f..a69c7dabe59 100755 --- a/oc-chef-pedant/lib/pedant/platform.rb +++ b/oc-chef-pedant/lib/pedant/platform.rb @@ -594,4 +594,5 @@ def gen_rsa_key(name) key end end + end diff --git a/oc-chef-pedant/spec/api/account/account_acl_spec.rb b/oc-chef-pedant/spec/api/account/account_acl_spec.rb index 87e16510c83..e5ef13249e8 100644 --- a/oc-chef-pedant/spec/api/account/account_acl_spec.rb +++ b/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//_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//_acl/#{permission} endpoint" do if (permission == "read") @@ -724,8 +730,54 @@ end # context /organizations/_acl/ endpoint end # loop (each) over permissions - context "///_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 "///_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 diff --git a/src/oc_erchef/apps/oc_chef_authz/priv/pgsql_statements.config b/src/oc_erchef/apps/oc_chef_authz/priv/pgsql_statements.config index ccccd955889..d8b006ecfcb 100644 --- a/src/oc_erchef/apps/oc_chef_authz/priv/pgsql_statements.config +++ b/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">>}. + diff --git a/src/oc_erchef/apps/oc_chef_authz/src/oc_chef_authz_acl.erl b/src/oc_erchef/apps/oc_chef_authz/src/oc_chef_authz_acl.erl index dac07c4b53c..be62f8ebc7a 100644 --- a/src/oc_erchef/apps/oc_chef_authz/src/oc_chef_authz_acl.erl +++ b/src/oc_erchef/apps/oc_chef_authz/src/oc_chef_authz_acl.erl @@ -182,22 +182,11 @@ 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 -> @@ -205,9 +194,36 @@ names_to_ids(Ace, OrgId) -> _ -> 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) %% diff --git a/src/oc_erchef/apps/oc_chef_authz/src/oc_chef_authz_db.erl b/src/oc_erchef/apps/oc_chef_authz/src/oc_chef_authz_db.erl index 63250d51d38..cd1b615c0d9 100644 --- a/src/oc_erchef/apps/oc_chef_authz/src/oc_chef_authz_db.erl +++ b/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. + diff --git a/src/oc_erchef/apps/oc_chef_wm/src/oc_chef_wm_acl_permission.erl b/src/oc_erchef/apps/oc_chef_wm/src/oc_chef_wm_acl_permission.erl index e1e05237682..c97feeb91b6 100644 --- a/src/oc_erchef/apps/oc_chef_wm/src/oc_chef_wm_acl_permission.erl +++ b/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.