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.