6:23 a.m.
On Wed, Aug 07, 2013 at 01:14:14PM +0200, Jakub Hrozek wrote:
On Tue, Aug 06, 2013 at 03:04:28PM +0200, Pavel Březina wrote:
> On 08/06/2013 01:50 PM, Jakub Hrozek wrote:
> >On Mon, Jul 29, 2013 at 10:56:12AM +0200, Pavel Březina wrote:
> >>On 07/26/2013 11:58 AM, Jakub Hrozek wrote:
> >>>On Fri, Jul 26, 2013 at 11:49:37AM +0200, Pavel Březina wrote:
> >>>>https://fedorahosted.org/sssd/ticket/2026
> >>>>
> >>>>I created a separate ticket to support subdomain users and groups
in
> >>>>allow/deny lists.
> >>>>https://fedorahosted.org/sssd/ticket/2034
> >>>>
> >>>>There is a bug in PAC responder at the moment which makes simple
access
> >>>>provider to always deny access to subdomain users. PAC responder
currently
> >>>>fails to resolve group membership, thus we get error in access
provider when
> >>>>looking up groups and access is denied. Jakub will look into this.
> >>>>
> >>>
> >>>Can you also amend the unit tests for the simple access provider?
> >>
> >>Sure.
> >>Patches are attached.
> >
> >Hi, only some nitpicks now:
> >
> >>@@ -103,66 +169,47 @@ int sssm_simple_access_init(struct be_ctx *bectx,
struct bet_ops **ops,
> >> {
> >> int ret = EINVAL;
> >> struct simple_ctx *ctx;
> >>+ int i;
> >>+ struct {
> >>+ const char *name;
> >>+ const char *option;
> >>+ char **orig_list;
> >>+ char ***ctx_list;
> >>+ } lists[4] = {{"Allow users", CONFDB_SIMPLE_ALLOW_USERS,
NULL, NULL},
> >>+ {"Deny users", CONFDB_SIMPLE_DENY_USERS, NULL,
NULL},
> >>+ {"Allow groups", CONFDB_SIMPLE_ALLOW_GROUPS,
NULL, NULL},
> >>+ {"Deny groups", CONFDB_SIMPLE_DENY_GROUPS,
NULL, NULL}};
> >
> >I don't think you need to hardcode the size here, just lists[] would do.
> >
> >>
> >> ctx = talloc_zero(bectx, struct simple_ctx);
> >> if (ctx == NULL) {
> >>- DEBUG(1, ("talloc_zero failed.\n"));
> >>+ DEBUG(SSSDBG_CRIT_FAILURE, ("talloc_zero failed.\n"));
> >> return ENOMEM;
> >> }
> >>
> >> ctx->domain = bectx->domain;
> >> ctx->be_ctx = bectx;
> >>
> >>- /* Users */
> >>- ret = confdb_get_string_as_list(bectx->cdb, ctx,
bectx->conf_path,
> >>- CONFDB_SIMPLE_ALLOW_USERS,
> >>- &ctx->allow_users);
> >>- if (ret != EOK) {
> >>- if (ret == ENOENT) {
> >>- DEBUG(9, ("Allow user list is empty.\n"));
> >>- ctx->allow_users = NULL;
> >>- } else {
> >>- DEBUG(1, ("confdb_get_string_as_list failed.\n"));
> >>- goto failed;
> >>- }
> >>- }
> >>-
> >>- ret = confdb_get_string_as_list(bectx->cdb, ctx,
bectx->conf_path,
> >>- CONFDB_SIMPLE_DENY_USERS,
> >>- &ctx->deny_users);
> >>- if (ret != EOK) {
> >>- if (ret == ENOENT) {
> >>- DEBUG(9, ("Deny user list is empty.\n"));
> >>- ctx->deny_users = NULL;
> >>- } else {
> >>- DEBUG(1, ("confdb_get_string_as_list failed.\n"));
> >>- goto failed;
> >>- }
> >>- }
> >>+ lists[0].ctx_list = &ctx->allow_users;
> >>+ lists[1].ctx_list = &ctx->deny_users;
> >>+ lists[2].ctx_list = &ctx->allow_groups;
> >>+ lists[3].ctx_list = &ctx->deny_groups;
> >>
> >>- /* Groups */
> >>- ret = confdb_get_string_as_list(bectx->cdb, ctx,
bectx->conf_path,
> >>- CONFDB_SIMPLE_ALLOW_GROUPS,
> >>- &ctx->allow_groups);
> >>- if (ret != EOK) {
> >
> >And then when iterating over the array, you can use sizeof(lists) as
> >it's the same scope. Alternatively, you can use a last "sentinel"
> >element that could be initialized to { NULL, NULL, NULL, NULL } and
> >iterate until .name != NULL.
> >
> >>+ for (i = 0; i < 4; i++) {
> >>+ ret = confdb_get_string_as_list(bectx->cdb, ctx,
bectx->conf_path,
> >>+ lists[i].option,
&lists[i].orig_list);
> >> if (ret == ENOENT) {
> >>- DEBUG(9, ("Allow group list is empty.\n"));
> >>- ctx->allow_groups = NULL;
> >>- } else {
> >>- DEBUG(1, ("confdb_get_string_as_list failed.\n"));
> >>+ DEBUG(SSSDBG_FUNC_DATA, ("%s list is empty.\n",
lists[i].name));
> >>+ *lists[i].ctx_list = NULL;
> >
> >Do you actually need to special case ENOENT here?
> >simple_access_parse_names() handles empty input list just fine. If you
> >want to double check also here, then I think you should continue
> >instead.
> >
> >>+ } else if (ret != EOK) {
> >>+ DEBUG(SSSDBG_CRIT_FAILURE, ("confdb_get_string_as_list
failed.\n"));
> >> goto failed;
> >> }
> >>- }
> >>
> >>- ret = confdb_get_string_as_list(bectx->cdb, ctx,
bectx->conf_path,
> >>- CONFDB_SIMPLE_DENY_GROUPS,
> >>- &ctx->deny_groups);
> >>- if (ret != EOK) {
> >>- if (ret == ENOENT) {
> >>- DEBUG(9, ("Deny user list is empty.\n"));
> >>- ctx->deny_groups = NULL;
> >>- } else {
> >>- DEBUG(1, ("confdb_get_string_as_list failed.\n"));
> >>+ ret = simple_access_parse_names(ctx, bectx, lists[i].orig_list,
> >>+ lists[i].ctx_list);
> >>+ if (ret != EOK) {
> >>+ DEBUG(SSSDBG_CRIT_FAILURE, ("Unable to parse %s list [%d]:
%s\n",
> >>+ lists[i].name, ret,
sss_strerror(ret)));
> >> goto failed;
> >> }
> >> }
> >>--
> >>1.7.11.7
>
> Thanks for the review. New patch is attached.
Thank you, these patches look good to me and work fine (I only tested
against IPA, but I don't think that the backend matters much for these
patches)
Pushed to master and sssd-1-10