On Fri, Nov 14, 2014 at 05:27:29PM +0100, Jakub Hrozek wrote:
On Thu, Nov 13, 2014 at 11:58:13AM +0100, Sumit Bose wrote:
Hi,
Coverity run was clean, CI run succeeded and testing against a recent
IPA server went OK. Before I ack, I also want to test against an old
server, but I'm running out of time today.
See some comments inline.
Thank you for the review and testing.
> From 9f807e92b8db786c331842117e1481dbb494a1f7 Mon Sep 17 00:00:00 2001
> From: Sumit Bose <sbose(a)redhat.com>
> Date: Thu, 6 Nov 2014 13:13:27 +0100
> Subject: [PATCH 1/7] ipa: add split_ipa_anchor()
ACK, see one remark inline.
> +errno_t split_ipa_anchor(TALLOC_CTX *mem_ctx, const char *anchor,
> + char **_anchor_domain, char **_ipa_uuid)
> +{
> + const char *sep;
> +
> + if (anchor == NULL) {
> + return EINVAL;
> + }
> + if (strncmp(OVERRIDE_ANCHOR_IPA_PREFIX, anchor,
> + OVERRIDE_ANCHOR_IPA_PREFIX_LEN) != 0) {
> + DEBUG(SSSDBG_CRIT_FAILURE, "No IPA anchor [%s].\n", anchor);
> + return ENOMSG;
This is fine since we're only using the error as an..error..so the caller
will only test ret != EOK, but if we used it to differentiate between
different conditions, we should use a custom error code.
> + }
> +
> + sep = strchr(anchor + OVERRIDE_ANCHOR_IPA_PREFIX_LEN, ':');
> + if (sep == NULL || sep[1] == '\0') {
> + DEBUG(SSSDBG_CRIT_FAILURE, "Broken IPA anchor [%s].\n", anchor);
> + return EINVAL;
> + }
> From ec4cf492945cb8f30fcb2467e9d26836de7ff6d8 Mon Sep 17 00:00:00 2001
> From: Sumit Bose <sbose(a)redhat.com>
> Date: Fri, 7 Nov 2014 13:55:01 +0100
> Subject: [PATCH 2/7] LDAP: add support for lookups by UUID
>
> Related to
https://fedorahosted.org/sssd/ticket/2481
> ---
> src/providers/data_provider.h | 2 ++
> src/providers/ldap/ldap_id.c | 58 +++++++++++++++++++++++++++++++++++++++----
> 2 files changed, 55 insertions(+), 5 deletions(-)
>
> diff --git a/src/providers/data_provider.h b/src/providers/data_provider.h
> index e1cb4be..5df493e 100644
> --- a/src/providers/data_provider.h
> +++ b/src/providers/data_provider.h
> @@ -127,6 +127,7 @@
> #define BE_FILTER_IDNUM 2
> #define BE_FILTER_ENUM 3
> #define BE_FILTER_SECID 4
> +#define BE_FILTER_UUID 5
>
> #define BE_REQ_USER 0x0001
> #define BE_REQ_GROUP 0x0002
> @@ -139,6 +140,7 @@
> #define BE_REQ_HOST 0x0010
> #define BE_REQ_BY_SECID 0x0011
> #define BE_REQ_USER_AND_GROUP 0x0012
> +#define BE_REQ_BY_UUID 0x0013
> #define BE_REQ_TYPE_MASK 0x00FF
> #define BE_REQ_FAST 0x1000
>
> diff --git a/src/providers/ldap/ldap_id.c b/src/providers/ldap/ldap_id.c
> index e8b3a0e..2e58f4e 100644
> --- a/src/providers/ldap/ldap_id.c
> +++ b/src/providers/ldap/ldap_id.c
> @@ -179,6 +179,20 @@ struct tevent_req *users_get_send(TALLOC_CTX *memctx,
> goto done;
> }
> break;
> + case BE_FILTER_UUID:
> + attr_name = ctx->opts->user_map[SDAP_AT_USER_UUID].name;
> + if (attr_name == NULL) {
> + DEBUG(SSSDBG_CRIT_FAILURE,
> + "UUID search not configured for this backend.\n");
> + ret = EINVAL;
I wonder if failing is appropriate? I guess yes, but on the other hand,
calling an unconfigured back end handler (maybe an SSH handler with a
pure LDAP backend) is mostly a noop, right?
But I can see the point, just wanted to double check with you..
currently the UUID search is only used by the IPA provider which expects
that UUIDs are properly configured. If new use-caese come around we
might want to re-consider it.
Otherwise ACK
> From 15ea0a84cc1d1ded27c6f84ac563b7902f203526 Mon Sep 17 00:00:00 2001
> From: Sumit Bose <sbose(a)redhat.com>
> Date: Fri, 7 Nov 2014 21:33:36 +0100
> Subject: [PATCH 3/7] LDAP: always store UUID if available
>
> Related to
https://fedorahosted.org/sssd/ticket/2481
> ---
> src/providers/ldap/sdap_async_groups.c | 20 ++++++++++++++++++++
> src/providers/ldap/sdap_async_users.c | 19 +++++++++++++++++++
> 2 files changed, 39 insertions(+)
>
> diff --git a/src/providers/ldap/sdap_async_groups.c
b/src/providers/ldap/sdap_async_groups.c
> index a82d2aa..dc1b60d 100644
> --- a/src/providers/ldap/sdap_async_groups.c
> +++ b/src/providers/ldap/sdap_async_groups.c
> @@ -511,6 +511,7 @@ static int sdap_save_group(TALLOC_CTX *memctx,
> bool posix_group;
> bool use_id_mapping;
> char *sid_str;
> + const char *uuid;
> struct sss_domain_info *subdomain;
> int32_t ad_group_type;
>
> @@ -547,6 +548,25 @@ static int sdap_save_group(TALLOC_CTX *memctx,
> sid_str = NULL;
> }
>
> + /* Always store UUID if available */
> + ret = sysdb_attrs_get_string(attrs,
> + opts->group_map[SDAP_AT_GROUP_UUID].sys_name,
> + &uuid);
> + if (ret == EOK) {
> + ret = sysdb_attrs_add_string(group_attrs, SYSDB_UUID, uuid);
> + if (ret != EOK) {
> + DEBUG(SSSDBG_MINOR_FAILURE, "Could not add UUID string:
[%s]\n",
> + strerror(ret));
Please use sss_strerror() in new code (also elsewhere in this patch).
done, I replaced the other usages of strerror() in sdap_save_group() and
sdap_save_user() as well.
Otherwise ACK
> From 3adf966d906cf491f7ca852bb36921fc05542dbf Mon Sep 17 00:00:00 2001
> From: Sumit Bose <sbose(a)redhat.com>
> Date: Fri, 7 Nov 2014 15:05:41 +0100
> Subject: [PATCH 4/7] ipa: add get_be_acct_req_for_uuid()
ACK
> From 9047069105444545a2f5dc0be3ef30d4c43689d1 Mon Sep 17 00:00:00 2001
> From: Sumit Bose <sbose(a)redhat.com>
> Date: Fri, 7 Nov 2014 21:34:55 +0100
> Subject: [PATCH 5/7] IPA: make get_object_from_cache() public
ACK
I just noticed one oddity when reading the code. The function mixes if
and if-else statements, I think it would be nice to add another simple
patch that would converge to if-else style.
fix, I commited the change to the 6ht patch because here the call was
touched to add the uuid support.
> From 9286597c2ed35db04403f72caa75bbfb966cae9b Mon Sep 17 00:00:00 2001
> From: Sumit Bose <sbose(a)redhat.com>
> Date: Wed, 5 Nov 2014 15:58:04 +0100
> Subject: [PATCH 6/7] IPA: check overrrides for IPA users as well
>
> Currently overrides were only available for sub-domains, e.g. trusted AD
> domains. With this patch overrides can be used for IPA users as well.
>
> Related to
https://fedorahosted.org/sssd/ticket/2481
> ---
[...]
> +static errno_t ipa_id_get_account_info_get_original_step(struct tevent_req *req,
> + struct be_acct_req *ar)
> +{
> + struct ipa_id_get_account_info_state *state = tevent_req_data(req,
> + struct ipa_id_get_account_info_state);
> + struct tevent_req *subreq;
> + int ret;
ret is unused and triggers a compilation warning.
fixed
> +
> + subreq = sdap_handle_acct_req_send(state, state->be_req, ar,
> + state->ipa_ctx->sdap_id_ctx,
> +
state->ipa_ctx->sdap_id_ctx->opts->sdom,
> + state->ipa_ctx->sdap_id_ctx->conn,
true);
> + if (subreq == NULL) {
> + DEBUG(SSSDBG_OP_FAILURE, "sdap_handle_acct_req_send failed.\n");
> + return ENOMEM;
> + }
> + tevent_req_set_callback(subreq, ipa_id_get_account_info_orig_done, req);
> +
> + return EOK;
> +}
> +
The rest looks good.
> From d3d3d51a1d49936c9ad1a3733388bfd384fd07c4 Mon Sep 17 00:00:00 2001
> From: Sumit Bose <sbose(a)redhat.com>
> Date: Fri, 7 Nov 2014 21:36:12 +0100
> Subject: [PATCH 7/7] Enable views for all domains
ACK
new version attached.
bye,
Sumit