On Wed, 2012-11-07 at 13:13 +0100, Sumit Bose wrote:
> From 9bfb75134ede4d6fe335e2492416e974f239008d Mon Sep 17 00:00:00 2001
> From: Sumit Bose <sb...@redhat.com>
> Date: Wed, 7 Nov 2012 11:53:13 +0100
> Subject: [PATCH 3/5] Use sysdb_initgroups() instead of
> sysdb_get_user_by_name()
>
> To be able to efficiently store group memberships we need to know the
> current memberships of a user. Instead of just looking up the user in
> the cache sysdb_initgroups() is used which returns the user entry
> together with all groups the user is a member of.
> ---
> src/responder/pac/pacsrv_cmd.c | 44
> +++++++++++++++++++++++++++++++++------
> 1 files changed, 37 insertions(+), 7 deletions(-)
>
> diff --git a/src/responder/pac/pacsrv_cmd.c
> b/src/responder/pac/pacsrv_cmd.c
> index 7777983..051f697 100644
> --- a/src/responder/pac/pacsrv_cmd.c
> +++ b/src/responder/pac/pacsrv_cmd.c
> @@ -60,11 +60,15 @@ struct pac_req_ctx {
>
> size_t gid_count;
> gid_t *gids;
> +
> + size_t current_gid_count;
> + gid_t *current_gids;
> };
>
> static errno_t pac_add_user_next(struct pac_req_ctx *pr_ctx);
> static void pac_get_domains_done(struct tevent_req *req);
> -static errno_t save_pac_user(struct pac_req_ctx *pr_ctx);
> +static errno_t save_pac_user(TALLOC_CTX *mem_ctx, struct pac_req_ctx
> *pr_ctx,
> + size_t *_current_gid_count, gid_t
> **_current_gids);
> static void pac_get_group_done(struct tevent_req *subreq);
> static errno_t pac_save_memberships_next(struct tevent_req *req);
> static errno_t pac_store_membership(struct pac_req_ctx *pr_ctx,
> @@ -188,7 +192,8 @@ static errno_t pac_add_user_next(struct
> pac_req_ctx *pr_ctx)
> struct dom_sid *my_dom_sid;
> struct local_mapping_ranges *my_range_map;
>
> - ret = save_pac_user(pr_ctx);
> + ret = save_pac_user(pr_ctx, pr_ctx, &pr_ctx->current_gid_count,
> + &pr_ctx->current_gids);
> if (ret != EOK) {
> DEBUG(SSSDBG_OP_FAILURE, ("save_pac_user failed.\n"));
> goto done;
> @@ -223,15 +228,18 @@ done:
> return ret;
> }
>
> -static errno_t save_pac_user(struct pac_req_ctx *pr_ctx)
> +static errno_t save_pac_user(TALLOC_CTX *mem_ctx, struct pac_req_ctx
> *pr_ctx,
> + size_t *_current_gid_count, gid_t
> **_current_gids)
> {
> struct sysdb_ctx *sysdb;
> int ret;
> - const char *attrs[] = {SYSDB_NAME, SYSDB_UIDNUM, SYSDB_GIDNUM,
> NULL};
> - struct ldb_message *msg;
> struct passwd *pwd = NULL;
> TALLOC_CTX *tmp_ctx = NULL;
> struct sysdb_attrs *user_attrs = NULL;
> + struct ldb_result *res = NULL;
> + gid_t *current_gids = NULL;
> + size_t current_gid_count = 0;
> + size_t c;
>
> sysdb = pr_ctx->dom->sysdb;
> if (sysdb == NULL) {
> @@ -247,9 +255,29 @@ static errno_t save_pac_user(struct pac_req_ctx
> *pr_ctx)
> goto done;
> }
>
> - ret = sysdb_search_user_by_name(tmp_ctx, sysdb,
> pr_ctx->user_name, attrs,
> - &msg);
> + ret = sysdb_initgroups(tmp_ctx, sysdb, pr_ctx->user_name, &res);
> if (ret == EOK) {
> + /* First result is the user entry then the groups follow */
> + if (res->count > 1) {
> + current_gid_count = res->count - 1;
> + current_gids = talloc_array(mem_ctx, gid_t,
> current_gid_count);
> + if (current_gids == NULL) {
> + DEBUG(SSSDBG_OP_FAILURE, ("talloc_array failed.\n"));
> + ret = ENOMEM;
> + goto done;
> + }
> +
> + for (c = 0; c < current_gid_count; c++) {
> + current_gids[c] =
> ldb_msg_find_attr_as_uint64(res->msgs[c + 1],
> +
> SYSDB_GIDNUM, 0);
> + if (current_gids[c] == 0) {
> + DEBUG(SSSDBG_OP_FAILURE, ("Missing GID.\n"));
> + ret = EINVAL;
> + goto done;
> + }
> + }
> + }
> +
> /* TODO: check id uid and gid are equal. */
> } else if (ret == ENOENT) {
> ret = get_pwd_from_pac(tmp_ctx, pr_ctx->pac_ctx, pr_ctx->dom,
> @@ -274,6 +302,8 @@ static errno_t save_pac_user(struct pac_req_ctx
> *pr_ctx)
> goto done;
> }
>
> + *_current_gid_count = current_gid_count;
> + *_current_gids = current_gids;
> ret = EOK;
>
> done:
> --
> 1.7.7.6
Here it seems like you are overloading save_pac_user() with also
returning the current group memberships, but I do not see any real
advantage in doing it as part of pac_sve_user, there is no optimization
involved.
I think it would be more readable if you created a pac_user_get_gids()
that does the initgroups call and just called it before/after
save_pac_user()
Also this patch seem to make save_pac_user() skip storing the user if
sysdb_initgroups() is successful. This means that if the PAC has new
information (like different fullname) that information is simply
discarded and not updated.
I thik keeping these 2 separate makes it more readable and avoids this
bug.
Simo.
--
Simo Sorce * Red Hat, Inc * New York
_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
