[erchef] Fix ACL updates when actor names includes '.' #1275
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Unlike users and groups, client names can contain the `.` character. In fact, this character this likely since the client name is often based on the node name which is based on the FQDN of the node. In #1159, we added the ability to add name-spaced actors and groups to ACLs. However, that changed validated the actor names against the more restrictive user/group regular expression. This change uses the standard NAME_REGEX for validation which includes `.`. It does mean that some invalid user and group names will pass validation; however, presumably such users and groups will fail to be found in the database since they can't be created. Fixes #1274 Signed-off-by: Steven Danna <[email protected]>
Signed-off-by: Steven Danna <[email protected]>
marcparadise
approved these changes
May 16, 2017
| end | ||
|
|
||
| it "returns 200", :acl do | ||
| put("#{request_url}/#{permission}", platform.admin_user, :payload => update_body).should look_like({:status => 200}) |
There was a problem hiding this comment.
Let's also verify through a get that the name isn't munged in any way,
| @@ -58,7 +58,8 @@ | |||
| %% http://wiki.chef.io/display/chef/Roles#Roles-name. Judging | |||
| %% from the cookbook names and recipes in the opscode/cookbooks | |||
| %% repository, this regular expression applies to them as well. | |||
| -define(NAME_REGEX, "[.[:alnum:]_-]+"). | |||
| -define(NAME_CHAR_CLASS, "[.[:alnum:]_-]"). | |||
| -define(NAME_REGEX, ?NAME_CHAR_CLASS ++ "+"). | |||
There was a problem hiding this comment.
It's minor, but I don't know if the precompile will resolve this - the append might be done inline on each call. Same for the others. May not be significant enough to matter.
There was a problem hiding this comment.
Hm, I wonder if we avoid the ++ and just use the feature where you can put two string literals next to each other: "foo" "bar" if that happens at compile time.
There was a problem hiding this comment.
I'm pretty confident that's the case.
There was a problem hiding this comment.
Confirmed it does:
-module(test).
-export([foo/0]).
foo() ->
"bar" " " "baz".
> erl
Erlang/OTP 18 [erts-7.3.1] [source] [64-bit] [smp:8:8] [async-threads:10] [hipe] [kernel-poll:false]
Eshell V7.3.1 (abort with ^G)
1> compile:file("test.erl", S).
* 1: variable 'S' is unbound
2> compile:file("test.erl", 'S').
{ok,test}
3>
User switch command
--> q
> head -15 test.S
{module, test}. %% version = 0
{exports, [{foo,0},{module_info,0},{module_info,1}]}.
{attributes, []}.
{labels, 7}.
{function, foo, 0, 2}.
{label,1}.
{line,[{location,"test.erl",3}]}.
{func_info,{atom,test},{atom,foo},0}.
{label,2}.
{move,{literal,"bar baz"},{x,0}}.
Signed-off-by: Steven Danna <[email protected]>
Signed-off-by: Steven Danna <[email protected]>
You may ask "why not remove pivotal from the list" and the answer is that I'm being paranoid about platform.admin_user being something other than pivotal. Signed-off-by: Steven Danna <[email protected]>
markan
reviewed
May 16, 2017
| @@ -58,7 +58,8 @@ | |||
| %% http://wiki.chef.io/display/chef/Roles#Roles-name. Judging | |||
| %% from the cookbook names and recipes in the opscode/cookbooks | |||
| %% repository, this regular expression applies to them as well. | |||
| -define(NAME_REGEX, "[.[:alnum:]_-]+"). | |||
| -define(NAME_CHAR_CLASS, "[.[:alnum:]_-]"). | |||
| -define(NAME_REGEX, ?NAME_CHAR_CLASS ++ "+"). | |||
There was a problem hiding this comment.
I'm pretty confident that's the case.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Unlike users and groups, client names can contain the
.character.In fact, this character this likely since the client name is often
based on the node name which is based on the FQDN of the node.
In #1159, we added the ability to add name-spaced
actors and groups to ACLs. However, that changed validated the actor
names against the more restrictive user/group regular expression.
This change uses the standard NAME_REGEX for validation which includes
.. It does mean that some invalid user and group names will passvalidation; however, presumably such users and groups will fail to be
found in the database since they can't be created.
Fixes #1274
Signed-off-by: Steven Danna [email protected]