> On Tue, Feb 21, 2012 at 01:34:08PM +0100, Jan Zelený wrote: > > > On Thu, Feb 09, 2012 at 06:24:33PM +0100, Jan Zelený wrote: > > > > > On Tue, Feb 07, 2012 at 04:35:21PM +0100, Jan Zelený wrote: > > > > > > If all triplets of a netgroup are removed from LDAP server > > > > > > record, this change won't be projected to the sysdb and all > > > > > > triplets will remain there. The same situation will happen when > > > > > > removing all netgroup members. > > > > > > > > > > > > This patch fixes these bugs and provides the possibility to fix > > > > > > similar issues elsewhere. > > > > > > > > > > > > https://fedorahosted.org/sssd/ticket/1136 > > > > > > > > > > > > Thanks > > > > > > Jan > > > > > > > > > > I think that situations like this was the reason we use the > > > > > list_missing_attrs() function and then remove the "extra" > > > > > attributes during save. I see that list_missing_attrs() is used > > > > > only when saving users and services, not netgroups. Have you > > > > > reproduced the bug with users or groups? (I see the patch also > > > > > touches sdap_save_user()) > > > > > > > > > > Using list_missing_attrs() and extending sysdb_add_netgroup() > > > > > (maybe with a sysdb_store_user() so the code is similar to users) > > > > > seems like the proper fix to me. > > > > > > > > I looked at the code and I believe the best solution is actually to > > > > switch the original approach you are referring to to this new one I'm > > > > proposing. > > > > > > > > The entity (user/group/netgroup) in sysdb will always have only those > > > > attributes set by sdap_attrs_add_*. Therefore it is IMO better to > > > > delete it directly when preparing the replace operation instead of > > > > searching for these attributes afterwards. This would also mean that > > > > the flag I'm setting is redundant and the operation can be performed > > > > every time. Is there any case which I'm missing and which would make > > > > my approach unusable? > > > > > > You'd have to also take care of options that are passed to sysdb_save_* > > > directly not set into the attrs array with sdap_attrs_add - that's > > > attributes like loginShell or gecos. > > > > > > So instead of calling list_missing_attrs() on one place just before the > > > sysdb_save_* function, you'd have to check all the attributes manually > > > and that's quite error-prone. > > > > In the end I've applied the approach you suggested. I'm sending the patch > > together with one other patch which proposes a simplification of the > > list_missing_attrs() function. > > > > Thanks > > Jan > > nack, breaks unit tests: > CC src/providers/krb5/krb5_utils_tests-krb5_utils.o > src/tests/sysdb-tests.c: In function ‘test_odd_characters’: > src/tests/sysdb-tests.c:2788:30: warning: passing argument 5 of > ‘sysdb_add_netgroup’ makes pointer from integer without a cast [enabled by > default] ./src/db/sysdb.h:531:5: note: expected ‘char **’ but argument is > of type ‘int’ src/tests/sysdb-tests.c:2788:30: error: too few arguments to > function ‘sysdb_add_netgroup’ ./src/db/sysdb.h:531:5: note: declared here
Patches attached Jan
From ac47dcbb0dfcc27e8d2328810f1c1d4f9896fdd4 Mon Sep 17 00:00:00 2001 From: Jan Zeleny <jzel...@redhat.com> Date: Tue, 21 Feb 2012 07:07:30 -0500 Subject: [PATCH 1/2] Delete missing attributes from netgroups to be stored https://fedorahosted.org/sssd/ticket/1136 --- src/db/sysdb.h | 2 ++ src/db/sysdb_ops.c | 14 ++++++++++++++ src/providers/ipa/ipa_netgroups.c | 2 +- src/providers/ldap/sdap_async_netgroups.c | 27 ++++++++++++++++++++++++++- src/providers/proxy/proxy_netgroup.c | 2 +- src/tests/sysdb-tests.c | 2 +- 6 files changed, 45 insertions(+), 4 deletions(-) diff --git a/src/db/sysdb.h b/src/db/sysdb.h index e9a89606b37e02560be8c01c813043c29d1e9e64..9e4b8c39adca13f4d5c736cfec71e90671d67ef4 100644 --- a/src/db/sysdb.h +++ b/src/db/sysdb.h @@ -532,6 +532,7 @@ int sysdb_add_netgroup(struct sysdb_ctx *sysdb, const char *name, const char *description, struct sysdb_attrs *attrs, + char **missing, int cache_timeout, time_t now); @@ -563,6 +564,7 @@ int sysdb_store_group(struct sysdb_ctx *sysdb, enum sysdb_member_type { SYSDB_MEMBER_USER, SYSDB_MEMBER_GROUP, + SYSDB_MEMBER_NETGROUP, SYSDB_MEMBER_SERVICE, SYSDB_MEMBER_AUTOFSENTRY }; diff --git a/src/db/sysdb_ops.c b/src/db/sysdb_ops.c index 41070843b1ce9d65bdef95cbc03dd2c2f0cfa398..96fba98d6e2d0023be90c67274e30bddf7fcfb27 100644 --- a/src/db/sysdb_ops.c +++ b/src/db/sysdb_ops.c @@ -1378,6 +1378,7 @@ int sysdb_add_netgroup(struct sysdb_ctx *sysdb, const char *name, const char *description, struct sysdb_attrs *attrs, + char **missing, int cache_timeout, time_t now) { @@ -1422,6 +1423,15 @@ int sysdb_add_netgroup(struct sysdb_ctx *sysdb, ret = sysdb_set_netgroup_attr(sysdb, name, attrs, SYSDB_MOD_REP); + if (missing) { + ret = sysdb_remove_attrs(sysdb, name, + SYSDB_MEMBER_NETGROUP, + missing); + if (ret != EOK) { + DEBUG(4, ("Could not remove missing attributes\n")); + } + } + done: if (ret == EOK) { ret = ldb_transaction_commit(sysdb->ldb); @@ -2956,6 +2966,10 @@ errno_t sysdb_remove_attrs(struct sysdb_ctx *sysdb, msg->dn = sysdb_group_dn(sysdb, msg, sysdb->domain->name, name); break; + case SYSDB_MEMBER_NETGROUP: + msg->dn = sysdb_netgroup_dn(sysdb, msg, sysdb->domain->name, name); + break; + case SYSDB_MEMBER_SERVICE: msg->dn = sysdb_svc_dn(sysdb, msg, sysdb->domain->name, name); break; diff --git a/src/providers/ipa/ipa_netgroups.c b/src/providers/ipa/ipa_netgroups.c index 647818fa7b31d630cb427acfbc661cbc1daf75f5..2d6e624df4ba83b7871df76ec231148ea506b12a 100644 --- a/src/providers/ipa/ipa_netgroups.c +++ b/src/providers/ipa/ipa_netgroups.c @@ -171,7 +171,7 @@ static errno_t ipa_save_netgroup(TALLOC_CTX *mem_ctx, DEBUG(6, ("Storing info for netgroup %s\n", name)); - ret = sysdb_add_netgroup(ctx, name, NULL, netgroup_attrs, + ret = sysdb_add_netgroup(ctx, name, NULL, netgroup_attrs, NULL, dom->netgroup_timeout, 0); if (ret) goto fail; diff --git a/src/providers/ldap/sdap_async_netgroups.c b/src/providers/ldap/sdap_async_netgroups.c index 37aa2f112d88834162135c29b8294af90898f922..c03b9bb52082dbdde2cad7f4242300976e3c034e 100644 --- a/src/providers/ldap/sdap_async_netgroups.c +++ b/src/providers/ldap/sdap_async_netgroups.c @@ -49,6 +49,8 @@ static errno_t sdap_save_netgroup(TALLOC_CTX *memctx, const char *name = NULL; int ret; char *timestamp = NULL; + const char **ldap_attrs = NULL; + char **missing = NULL; ret = sysdb_attrs_get_el(attrs, opts->netgroup_map[SDAP_AT_NETGROUP_NAME].sys_name, @@ -127,7 +129,30 @@ static errno_t sdap_save_netgroup(TALLOC_CTX *memctx, goto fail; } - ret = sysdb_add_netgroup(ctx, name, NULL, netgroup_attrs, + ret = build_attrs_from_map(attrs, opts->netgroup_map, SDAP_OPTS_NETGROUP, + &ldap_attrs); + if (ret != EOK) { + DEBUG(SSSDBG_CRIT_FAILURE, ("Failed to retrieve attributes from map\n")); + goto fail; + } + + /* Make sure that any attributes we requested from LDAP that we + * did not receive are also removed from the sysdb + */ + ret = list_missing_attrs(attrs, opts->netgroup_map, SDAP_OPTS_NETGROUP, + ldap_attrs, attrs, &missing); + if (ret != EOK) { + DEBUG(SSSDBG_CRIT_FAILURE, ("Failed to list missing attributes\n")); + goto fail; + } + + /* Remove missing attributes */ + if (missing && !missing[0]) { + /* Nothing to remove */ + talloc_zfree(missing); + } + + ret = sysdb_add_netgroup(ctx, name, NULL, netgroup_attrs, missing, dom->netgroup_timeout, now); if (ret) goto fail; diff --git a/src/providers/proxy/proxy_netgroup.c b/src/providers/proxy/proxy_netgroup.c index 47a425b4673f2ec59c067385101b5ee3666ca0dd..797f8c6b88ec4c07440f134fb2a1071b1c5c9976 100644 --- a/src/providers/proxy/proxy_netgroup.c +++ b/src/providers/proxy/proxy_netgroup.c @@ -87,7 +87,7 @@ static errno_t save_netgroup(struct sysdb_ctx *sysdb, } } - ret = sysdb_add_netgroup(sysdb, name, NULL, attrs, cache_timeout, 0); + ret = sysdb_add_netgroup(sysdb, name, NULL, attrs, NULL, cache_timeout, 0); if (ret != EOK) { DEBUG(SSSDBG_OP_FAILURE, ("sysdb_add_netgroup failed.\n")); return ret; diff --git a/src/tests/sysdb-tests.c b/src/tests/sysdb-tests.c index 886491a77198a88fd0375f0d00195bb63ef47d43..b3aaa2664776afa65bb9d597df2e04f2bec16023 100644 --- a/src/tests/sysdb-tests.c +++ b/src/tests/sysdb-tests.c @@ -2785,7 +2785,7 @@ START_TEST(test_odd_characters) /* Add */ ret = sysdb_add_netgroup(test_ctx->sysdb, odd_netgroupname, "No description", - NULL, 30, 0); + NULL, NULL, 30, 0); fail_unless(ret == EOK, "sysdb_add_netgroup error [%d][%s]", ret, strerror(ret)); -- 1.7.6.4
From 486f1ced68adbcef469103db341405973b740882 Mon Sep 17 00:00:00 2001
From: Jan Zeleny <jzel...@redhat.com>
Date: Tue, 21 Feb 2012 07:18:12 -0500
Subject: [PATCH 2/2] Modifications to simplify list_missing_attrs
---
src/providers/ldap/ldap_common.c | 9 +++++++--
src/providers/ldap/ldap_common.h | 1 -
src/providers/ldap/sdap_async_groups.c | 2 +-
src/providers/ldap/sdap_async_initgroups.c | 2 +-
src/providers/ldap/sdap_async_netgroups.c | 10 +---------
src/providers/ldap/sdap_async_private.h | 2 --
src/providers/ldap/sdap_async_services.c | 9 ++-------
src/providers/ldap/sdap_async_users.c | 7 ++-----
8 files changed, 14 insertions(+), 28 deletions(-)
diff --git a/src/providers/ldap/ldap_common.c b/src/providers/ldap/ldap_common.c
index 4f78313bcd172b5caff98bbc1a4189f01300856d..f8eae7c701c1a2b1406dcd99732901bba5f79aa5 100644
--- a/src/providers/ldap/ldap_common.c
+++ b/src/providers/ldap/ldap_common.c
@@ -1498,7 +1498,6 @@ errno_t get_sysdb_attr_name(TALLOC_CTX *mem_ctx,
errno_t list_missing_attrs(TALLOC_CTX *mem_ctx,
struct sdap_attr_map *map,
size_t map_size,
- const char **expected_attrs,
struct sysdb_attrs *recvd_attrs,
char ***missing_attrs)
{
@@ -1506,10 +1505,11 @@ errno_t list_missing_attrs(TALLOC_CTX *mem_ctx,
size_t attr_count = 0;
size_t i, j, k;
char **missing = NULL;
+ const char **expected_attrs;
char *sysdb_name;
TALLOC_CTX *tmp_ctx;
- if (!expected_attrs || !recvd_attrs || !missing_attrs) {
+ if (!recvd_attrs || !missing_attrs) {
return EINVAL;
}
@@ -1518,6 +1518,11 @@ errno_t list_missing_attrs(TALLOC_CTX *mem_ctx,
return ENOMEM;
}
+ ret = build_attrs_from_map(tmp_ctx, map, map_size, &expected_attrs);
+ if (ret != EOK) {
+ goto done;
+ }
+
/* Count the expected attrs */
while(expected_attrs[attr_count]) attr_count++;
diff --git a/src/providers/ldap/ldap_common.h b/src/providers/ldap/ldap_common.h
index c912576347b3c95d27ebc613b95947a7a1fd364a..ea0c21174ee797883878cddd4826a342d91c096c 100644
--- a/src/providers/ldap/ldap_common.h
+++ b/src/providers/ldap/ldap_common.h
@@ -186,7 +186,6 @@ errno_t get_sysdb_attr_name(TALLOC_CTX *mem_ctx,
errno_t list_missing_attrs(TALLOC_CTX *mem_ctx,
struct sdap_attr_map *map,
size_t map_size,
- const char **expected_attrs,
struct sysdb_attrs *recvd_attrs,
char ***missing_attrs);
diff --git a/src/providers/ldap/sdap_async_groups.c b/src/providers/ldap/sdap_async_groups.c
index aefe3538587a89816ea0750a0b49ef0e863d7965..634de6ab4c6388dbaa4f523c9c7dc565aedc2d65 100644
--- a/src/providers/ldap/sdap_async_groups.c
+++ b/src/providers/ldap/sdap_async_groups.c
@@ -1121,7 +1121,7 @@ next:
}
if (state->check_count == 0) {
- ret = sdap_save_users(state, state->sysdb, state->attrs,
+ ret = sdap_save_users(state, state->sysdb,
state->dom, state->opts,
state->new_members, state->count, NULL);
if (ret) {
diff --git a/src/providers/ldap/sdap_async_initgroups.c b/src/providers/ldap/sdap_async_initgroups.c
index 5e0e184ad3d8435dbf7507edbd58d2568c421d39..2e5b0393a2dfe8cefdd81f363d18f80edc911ab3 100644
--- a/src/providers/ldap/sdap_async_initgroups.c
+++ b/src/providers/ldap/sdap_async_initgroups.c
@@ -2541,7 +2541,7 @@ static void sdap_get_initgr_user(struct tevent_req *subreq)
ret = sdap_save_user(state, state->sysdb,
state->opts, state->dom,
- state->orig_user, state->user_attrs,
+ state->orig_user,
true, NULL, 0);
if (ret) {
sysdb_transaction_cancel(state->sysdb);
diff --git a/src/providers/ldap/sdap_async_netgroups.c b/src/providers/ldap/sdap_async_netgroups.c
index c03b9bb52082dbdde2cad7f4242300976e3c034e..26572551b8d35401d39e97b57415aa4679e17a9c 100644
--- a/src/providers/ldap/sdap_async_netgroups.c
+++ b/src/providers/ldap/sdap_async_netgroups.c
@@ -49,7 +49,6 @@ static errno_t sdap_save_netgroup(TALLOC_CTX *memctx,
const char *name = NULL;
int ret;
char *timestamp = NULL;
- const char **ldap_attrs = NULL;
char **missing = NULL;
ret = sysdb_attrs_get_el(attrs,
@@ -129,18 +128,11 @@ static errno_t sdap_save_netgroup(TALLOC_CTX *memctx,
goto fail;
}
- ret = build_attrs_from_map(attrs, opts->netgroup_map, SDAP_OPTS_NETGROUP,
- &ldap_attrs);
- if (ret != EOK) {
- DEBUG(SSSDBG_CRIT_FAILURE, ("Failed to retrieve attributes from map\n"));
- goto fail;
- }
-
/* Make sure that any attributes we requested from LDAP that we
* did not receive are also removed from the sysdb
*/
ret = list_missing_attrs(attrs, opts->netgroup_map, SDAP_OPTS_NETGROUP,
- ldap_attrs, attrs, &missing);
+ attrs, &missing);
if (ret != EOK) {
DEBUG(SSSDBG_CRIT_FAILURE, ("Failed to list missing attributes\n"));
goto fail;
diff --git a/src/providers/ldap/sdap_async_private.h b/src/providers/ldap/sdap_async_private.h
index 4192a225efaa94f00b420836c6276f63529e2718..f6ed680051d6a7fe8badfdf006e23947547b855a 100644
--- a/src/providers/ldap/sdap_async_private.h
+++ b/src/providers/ldap/sdap_async_private.h
@@ -94,14 +94,12 @@ int sdap_save_user(TALLOC_CTX *memctx,
struct sdap_options *opts,
struct sss_domain_info *dom,
struct sysdb_attrs *attrs,
- const char **ldap_attrs,
bool is_initgr,
char **_usn_value,
time_t now);
int sdap_save_users(TALLOC_CTX *memctx,
struct sysdb_ctx *sysdb,
- const char **attrs,
struct sss_domain_info *dom,
struct sdap_options *opts,
struct sysdb_attrs **users,
diff --git a/src/providers/ldap/sdap_async_services.c b/src/providers/ldap/sdap_async_services.c
index 5bc044630f1bdeded945d1878f02aa2acc55c3dd..5da09e6619963654595dd4259832fd1e9fd394f6 100644
--- a/src/providers/ldap/sdap_async_services.c
+++ b/src/providers/ldap/sdap_async_services.c
@@ -53,7 +53,6 @@ sdap_get_services_process(struct tevent_req *subreq);
static errno_t
sdap_save_services(TALLOC_CTX *memctx,
struct sysdb_ctx *sysdb,
- const char **attrs,
struct sss_domain_info *dom,
struct sdap_options *opts,
struct sysdb_attrs **services,
@@ -65,7 +64,6 @@ sdap_save_service(TALLOC_CTX *mem_ctx,
struct sdap_options *opts,
struct sss_domain_info *dom,
struct sysdb_attrs *attrs,
- const char **ldap_attrs,
char **_usn_value,
time_t now);
@@ -230,7 +228,6 @@ sdap_get_services_process(struct tevent_req *subreq)
}
ret = sdap_save_services(state, state->sysdb,
- state->attrs,
state->dom, state->opts,
state->services, state->count,
&state->higher_usn);
@@ -250,7 +247,6 @@ sdap_get_services_process(struct tevent_req *subreq)
static errno_t
sdap_save_services(TALLOC_CTX *mem_ctx,
struct sysdb_ctx *sysdb,
- const char **attrs,
struct sss_domain_info *dom,
struct sdap_options *opts,
struct sysdb_attrs **services,
@@ -285,7 +281,7 @@ sdap_save_services(TALLOC_CTX *mem_ctx,
usn_value = NULL;
ret = sdap_save_service(tmp_ctx, sysdb, opts, dom,
- services[i], attrs,
+ services[i],
&usn_value, now);
/* Do not fail completely on errors.
@@ -343,7 +339,6 @@ sdap_save_service(TALLOC_CTX *mem_ctx,
struct sdap_options *opts,
struct sss_domain_info *dom,
struct sysdb_attrs *attrs,
- const char **ldap_attrs,
char **_usn_value,
time_t now)
{
@@ -450,7 +445,7 @@ sdap_save_service(TALLOC_CTX *mem_ctx,
* that have been removed from LDAP
*/
ret = list_missing_attrs(svc_attrs, opts->service_map, SDAP_OPTS_SERVICES,
- ldap_attrs, attrs, &missing);
+ attrs, &missing);
if (ret != EOK) {
DEBUG(SSSDBG_MINOR_FAILURE,
("Failed to identify removed attributes: [%s]\n",
diff --git a/src/providers/ldap/sdap_async_users.c b/src/providers/ldap/sdap_async_users.c
index a8595ac8919c673df99aa91bf606bb3287cd97e2..74169c8b1ce6ae110f24c9fef29ac35ffee5c3ef 100644
--- a/src/providers/ldap/sdap_async_users.c
+++ b/src/providers/ldap/sdap_async_users.c
@@ -34,7 +34,6 @@ int sdap_save_user(TALLOC_CTX *memctx,
struct sdap_options *opts,
struct sss_domain_info *dom,
struct sysdb_attrs *attrs,
- const char **ldap_attrs,
bool is_initgr,
char **_usn_value,
time_t now)
@@ -256,7 +255,7 @@ int sdap_save_user(TALLOC_CTX *memctx,
* did not receive are also removed from the sysdb
*/
ret = list_missing_attrs(user_attrs, opts->user_map, SDAP_OPTS_USER,
- ldap_attrs, attrs, &missing);
+ attrs, &missing);
if (ret != EOK) {
goto fail;
}
@@ -293,7 +292,6 @@ fail:
int sdap_save_users(TALLOC_CTX *memctx,
struct sysdb_ctx *sysdb,
- const char **attrs,
struct sss_domain_info *dom,
struct sdap_options *opts,
struct sysdb_attrs **users,
@@ -327,7 +325,7 @@ int sdap_save_users(TALLOC_CTX *memctx,
usn_value = NULL;
ret = sdap_save_user(tmpctx, sysdb, opts, dom,
- users[i], attrs, false,
+ users[i], false,
&usn_value, now);
/* Do not fail completely on errors.
@@ -557,7 +555,6 @@ static void sdap_get_users_process(struct tevent_req *subreq)
}
ret = sdap_save_users(state, state->sysdb,
- state->attrs,
state->dom, state->opts,
state->users, state->count,
&state->higher_usn);
--
1.7.6.4
signature.asc
Description: This is a digitally signed message part.
_______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/sssd-devel
