Enhance API to handle global groups in local contexts #1159
Conversation
markan
force-pushed
the
ma/disambiguate_globals
branch
3 times, most recently
from
6bf4148 to
8a2d8d4
Compare
|
@chef/chef-server-maintainers I'm still working on dialyzer typespecs (coming shortly), but other than that this should be ready to roll. |
|
@markan Thanks for the heads up. Right now this is looking like it will probably land in release+1 since we've already got a ton in this release to test. |
markan
force-pushed
the
ma/disambiguate_globals
branch
from
911cb66 to
044357a
Compare
|
@ssd No worries ( @marcparadise has kept me up to date on your status ). I suspect this will go through a few review cycles before it's done anyways. This should unblock #886 & #935, so we'll probably want to bring this in early in the release cycle so we have a chance to tackle those, as well as maybe review whether #163 is still useful and relevant. |
markan
force-pushed
the
ma/disambiguate_globals
branch
3 times, most recently
from
99d0cf8 to
399769e
Compare
|
Im rebasing and pushing since we have merged a couple of commits from this PR. |
afiune
force-pushed
the
ma/disambiguate_globals
branch
5 times, most recently
from
9d86684 to
ef9396f
Compare
|
|
||
| %% Map org names to org ids | ||
| {NamesWithOrgIds, Errors2, _} = | ||
| lists:foldl(fun(Name, Acc) -> lookup_org_id(Name, Acc, MapperContext) end, |
There was a problem hiding this comment.
I might be missing something but why do we pass the MapperContext to this lookup fun if it doesn't use it?
| %% be robust to that. | ||
| case org_id_to_name(AnotherOrgId) of | ||
| not_found -> | ||
| {Expanded, Context}; |
There was a problem hiding this comment.
@markan I think here is where we are dropping the errors when an organization doesn't exist.
There was a problem hiding this comment.
I think per @markan's comment re deleted orgs, we should keep this, but perhaps we should log it so we can start getting a sense of how often this happens?
afiune
force-pushed
the
ma/disambiguate_globals
branch
2 times, most recently
from
0544697 to
1b7fc74
Compare
| -spec render_names_in_context(binary(),[{binary(), [binary()]}], #context{}) -> [binary()]. | ||
| render_names_in_context(OrgId, ScopedNames, Context) -> | ||
| GroupedScopedNames = group_by_key(ScopedNames), | ||
| {Expanded, _Cache} = lists:foldl(fun(E, A) -> render_names_in_context_f(OrgId, E, A) end, |
There was a problem hiding this comment.
We do not use the Context in this fun. This is the only place we consume render_names_in_context_f and we drop it in _Cache. The fun itself is not using it so maybe we should remove it?
afiune
force-pushed
the
ma/disambiguate_globals
branch
4 times, most recently
from
8170db3 to
5517156
Compare
| filter_by_error_type/2, | ||
| authz_id_to_names/3]). | ||
|
|
||
| -export([make_scoped_names/2, |
There was a problem hiding this comment.
This is a pretty big API for a brand new module. Do we need to export all of these functions? If so, is there anything we can do to tighten up the API a bit?
| %% If no match, then we have bad name | ||
| process_match(nomatch, Name, _Context, _ScopedOk) -> | ||
| {ill_formed_name, Name}; | ||
| %% If we only onl match the name, then it is an unscoped name, and we use the org context |
There was a problem hiding this comment.
Small typo in comment: only onl
| %% be robust to that. | ||
| case org_id_to_name(AnotherOrgId) of | ||
| not_found -> | ||
| {Expanded, Context}; |
There was a problem hiding this comment.
I think per @markan's comment re deleted orgs, we should keep this, but perhaps we should log it so we can start getting a sense of how often this happens?
| %% Design note: we drop missing orgs silently. Org deletion leaks many objects and we must | ||
| %% be robust to that. | ||
| case org_id_to_name(AnotherOrgId) of | ||
| not_found -> |
There was a problem hiding this comment.
an error_logger:warning_msg()** would be fine?
There was a problem hiding this comment.
Let's use lager instead of error_logger
| %% | ||
| -spec org_id_to_name(binary()) -> not_found | binary(). | ||
| org_id_to_name(OrgId) -> | ||
| %% TODO maybe rework this; it bypasses a bunch of our statistics gathering code. |
There was a problem hiding this comment.
I think we should probably wrap this in a stats hero timer.
| %% Sometimes the list of authz ids is empty; shortcut that and save a DB call. | ||
| {[], []}; | ||
| query_and_diff_authz_ids(QueryName, AuthzIds, CallbackFun) -> | ||
| case CallbackFun({QueryName, [AuthzIds]}) of |
There was a problem hiding this comment.
Probably a stats_hero timer around this as well.
|
@chef/maintainers We are ready to review this change! cc/ @markan @stevendanna |
afiune
changed the title
Signed-off-by: Mark Anderson <[email protected]>
The server_admins group introduces an issue with ACLS and groups; we have a global group in the context of a organization scoped object. Not only do we have the potential of conflicts with a org scoped group with the same name, we also lack a mechanisim to look up groups that aren't in the org we're looking at. This PR adds new syntax '::' to indicate scoping. ::GROUPNAME without a prefix indicates a global (across multiple orgs) entity, while ORGNAME::GROUPNAME refers to a group in an another org. This should unblock the work around server admins, including improved org creation. This will need to be implemented in chef_zero to maintain feature parity. Signed-off-by: Mark Anderson <[email protected]>
Signed-off-by: Mark Anderson <[email protected]>
Signed-off-by: Mark Anderson <[email protected]>
Signed-off-by: Mark Anderson <[email protected]>
Signed-off-by: Mark Anderson <[email protected]>
chef-zero behaves a little different with `CHEF_FS` enabled. this commit is just fixing the test failures. Signed-off-by: Salim Afiune <[email protected]>
Also quick style fixes Signed-off-by: Salim Afiune <[email protected]>
If a user tries to add an ACL that has a scoped group pointing to and organization that doesn't exist, we would need to throw an error and not allow such action. Signed-off-by: Salim Afiune <[email protected]>
This commit re structure the oc_chef_authz_scoped_name module to separate the fn's we need to export because other modules leverage them, and the internal functions of the module. Signed-off-by: Salim Afiune <[email protected]>
In order to send a proper message we need to extract the full_name from the Scope Records. If the provided Names are not Scope Records, just use their names. Signed-off-by: Salim Afiune <[email protected]>
Even though this peace of code needs to be robust enough, we also would like to be transparent and log a warning message saying that there is something pointing to an organization that doesn't exist. Signed-off-by: Salim Afiune <[email protected]>
Signed-off-by: Salim Afiune <[email protected]>
Signed-off-by: Salim Afiune <[email protected]>
We still 400 for most requests because of another latent bug that exists on master as well. We'll tackle that bug in a follow-up commit. Signed-off-by: Steven Danna <[email protected]>
stevendanna
force-pushed
the
ma/disambiguate_globals
branch
from
5156b87 to
2cbcb5b
Compare
Signed-off-by: Steven Danna <[email protected]>
|
👍 LGTM |
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]>
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]>
Support nonlocal group names
The server_admins group introduces an issue with ACLS and groups; we
have a global group in the context of a organization scoped
object. Not only do we have the potential of conflicts with a org
scoped group with the same name, we also lack a mechanisim to look up
groups that aren't in the org we're looking at.
This PR adds new syntax '::' to indicate scoping. ::GROUPNAME without
a prefix indicates a global (across multiple orgs) entity, while
ORGNAME::GROUPNAME refers to a group in an another org.
This should unblock the work around server admins, including improved
org creation.
There remain a few rough spots around error handling, and a few
opportunities to streamline and simplify the logic around 'lowering'
qualified names to authz_ids, but this should be enough to kickstart
the discussion.
This will need to be implemented in chef_zero to maintain feature parity.
Ticket: POOL-570