Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

store ACEs by client/user #240

Merged
merged 1 commit into from
Sep 1, 2016
Merged

store ACEs by client/user #240

merged 1 commit into from
Sep 1, 2016

Conversation

marcparadise
Copy link
Member

Chef Server has the ability to return users and clients separately
within an GET ACL request when detail=granular. To support that,
we need to store them separately and determine if we want to present
actors or users and clients at the time of the request.

This change makes a reasonable best effort at capturing the creator
type (user v client) correctly and uses that for determining its
assignment in acls.

Chef Server has the ability to return users and clients separately
within an GET ACL request when `detail=granular`.  To support that,
we need to store them separately and determine if we want to present
`actors` or `users` and `clients` at the time of the request.

This change makes a reasonable best effort at capturing the creator
type (user v client) correctly and uses that for determining its
assignment in acls.
@tduffield
Copy link
Contributor

Looks sane enough to me.

@tduffield
Copy link
Contributor

Tapping @jkeiser for a review :) Maybe @randomcamel or @danielsdeleo as well?

@danielsdeleo
Copy link
Contributor

LGTM

@@ -22,6 +22,17 @@ def get(request)
end
acls = FFI_Yajl::Parser.parse(get_data(request, acl_path), :create_additions => false)
acls = ChefData::DataNormalizer.normalize_acls(acls)
if request.query_params["detail"] == "granular"
acls.each do |perm, ace|
acls[perm]["actors"] = []
Copy link

@ksubrama ksubrama Sep 1, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Out of curiosity - does it make sense to leave all the actors in here even if granular is requested? I don't know who uses this field.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is mirroring the behavior that we added to the Chef Server, where if you request granular actors will be empty but users and clients will not.

This prevents someone from using a modified version of the GET response to PUT an acl update, and getting an error because Chef Server won't except a PUT body with both actors and users/clients.

It's a bit awkward, but is the path least likely to break existing tooling outside of what we maintain - the most common method of modifying ACLs is to request the ACLs, modify a single ACE, and PUT it back to the server.

@ksubrama
Copy link

ksubrama commented Sep 1, 2016

👍 but I'm a n00b.

@marcparadise marcparadise merged commit b79ded7 into master Sep 1, 2016
@marcparadise marcparadise deleted the mp/SPOOL-340 branch September 1, 2016 20:36
@thommay thommay added Type: Enhancement Adds new functionality. and removed enhancement labels Jan 24, 2017
@chef chef locked and limited conversation to collaborators Nov 16, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Type: Enhancement Adds new functionality.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants