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.
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..
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).
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.
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.
+
+ 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