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