From 7f6e5e48f97b5ab2ed05788bb8725bce0099efbf Mon Sep 17 00:00:00 2001 From: Peter Fern Date: Wed, 26 Aug 2026 18:33:54 +0200 Subject: [PATCH] idp: Implement initgroups sync Other backends treat initgroups results as authoritative rather than additive. This change brings the idp backend into line with this behaviour so that group members are correctly removed from the cache when they are removed from the directory. --- src/providers/idp/idp_id.c | 14 +- src/providers/idp/idp_id_eval.c | 319 ++++++++++++++++++++++++++--- src/providers/idp/idp_private.h | 16 +- src/tests/system/tests/test_idp.py | 84 ++++++++ 4 files changed, 401 insertions(+), 32 deletions(-) diff --git a/src/providers/idp/idp_id.c b/src/providers/idp/idp_id.c index 8ad85fb9217..6daeb7dc5e4 100644 --- a/src/providers/idp/idp_id.c +++ b/src/providers/idp/idp_id.c @@ -303,10 +303,12 @@ static void idp_type_get_done(struct tevent_req *subreq) DEBUG(SSSDBG_TRACE_ALL, "[%zd][%.*s]\n", buflen, (int) buflen, buf); switch (state->lookup_type) { case IDP_LOOKUP_USER: - ret = eval_user_buf(state->idp_id_ctx, NULL, state->filter_value, state->noexist_delete, buf, buflen); + ret = eval_user_buf(state->idp_id_ctx, state->filter_value, + state->noexist_delete, buf, buflen); break; case IDP_LOOKUP_GROUP: - ret = eval_group_buf(state->idp_id_ctx, NULL, state->filter_value, state->noexist_delete, buf, buflen); + ret = eval_group_buf(state->idp_id_ctx, state->filter_value, + state->noexist_delete, buf, buflen); if (ret == EOK && !state->no_members) { DEBUG(SSSDBG_TRACE_ALL, "Looking up group members.\n"); @@ -321,12 +323,12 @@ static void idp_type_get_done(struct tevent_req *subreq) } break; case IDP_LOOKUP_GROUP_MEMBERS: - ret = eval_user_buf(state->idp_id_ctx, state->filter_value, state->filter_value, - false, buf, buflen); + ret = eval_group_members_buf(state->idp_id_ctx, state->filter_value, + buf, buflen); break; case IDP_LOOKUP_USER_GROUPS: - ret = eval_group_buf(state->idp_id_ctx, state->filter_value, state->filter_value, - false, buf, buflen); + ret = eval_user_groups_buf(state->idp_id_ctx, state->filter_value, + buf, buflen); break; default: DEBUG(SSSDBG_OP_FAILURE, "Unsupported lookup type [%d].\n", diff --git a/src/providers/idp/idp_id_eval.c b/src/providers/idp/idp_id_eval.c index 808eed460ec..a22e7e49aa8 100644 --- a/src/providers/idp/idp_id_eval.c +++ b/src/providers/idp/idp_id_eval.c @@ -110,12 +110,17 @@ static errno_t store_json_user(struct idp_id_ctx *idp_id_ctx, json_t *user, if (group_name != NULL) { ret = sysdb_add_group_member(dom, group_name, fqdn, SYSDB_MEMBER_USER, false); - if (ret != EOK) { - DEBUG(SSSDBG_OP_FAILURE, - "Failed to store user [%s] as member of group [%s].\n", - fqdn, group_name); - goto done; - } + if (ret == EEXIST) { + DEBUG(SSSDBG_FUNC_DATA, + "Group [%s] already has member [%s]. Skipping.\n", + group_name, fqdn); + ret = EOK; + } else if (ret != EOK) { + DEBUG(SSSDBG_OP_FAILURE, + "Failed to store user [%s] as member of group [%s].\n", + fqdn, group_name); + goto done; + } } done: @@ -217,7 +222,12 @@ static errno_t store_json_group(struct idp_id_ctx *idp_id_ctx, json_t *group, ret = sysdb_add_group_member(dom, fqdn, user_name, SYSDB_MEMBER_USER, false); - if (ret != EOK) { + if (ret == EEXIST) { + DEBUG(SSSDBG_FUNC_DATA, + "Group [%s] already has member [%s]. Skipping.\n", + fqdn, user_name); + ret = EOK; + } else if (ret != EOK) { DEBUG(SSSDBG_OP_FAILURE, "Failed to store user [%s] as member of group [%s].\n", user_name, fqdn); @@ -243,32 +253,26 @@ typedef errno_t (store_func_t)(struct idp_id_ctx *idp_id_ctx, json_t *obj, typedef errno_t (del_func_t)(struct idp_id_ctx *idp_id_ctx, const char *name); -static errno_t eval_obj_buf(struct idp_id_ctx *idp_id_ctx, - const char *type, store_func_t *store_func, - del_func_t *del_func, const char *name, - const char *del_obj_name, bool noexist_delete, - const uint8_t *buf, ssize_t buflen) +static errno_t parse_obj_buf(const char *type, + const uint8_t *buf, ssize_t buflen, + json_t **_data) { - errno_t ret; - json_t *data = NULL; json_error_t json_error; + json_t *data = NULL; char *tmp = NULL; - size_t index; - json_t *obj; data = json_loadb((const char *) buf, buflen, 0, &json_error); if (data == NULL) { DEBUG(SSSDBG_OP_FAILURE, "Failed to parse %s data on line [%d]: [%s].\n", type, json_error.line, json_error.text); - ret = EINVAL; - goto done; + return EINVAL; } if (!json_is_array(data)) { DEBUG(SSSDBG_OP_FAILURE, "Array of %ss expected.\n", type); - ret = EINVAL; - goto done; + json_decref(data); + return EINVAL; } if (DEBUG_IS_SET(SSSDBG_TRACE_ALL)) { @@ -281,6 +285,27 @@ static errno_t eval_obj_buf(struct idp_id_ctx *idp_id_ctx, } } + *_data = data; + return EOK; +} + +static errno_t eval_obj_buf(struct idp_id_ctx *idp_id_ctx, + const char *type, store_func_t *store_func, + del_func_t *del_func, const char *name, + const char *del_obj_name, bool noexist_delete, + const uint8_t *buf, ssize_t buflen) +{ + errno_t ret; + json_t *data = NULL; + char *tmp = NULL; + size_t index; + json_t *obj; + + ret = parse_obj_buf(type, buf, buflen, &data); + if (ret != EOK) { + goto done; + } + if (json_array_size(data) == 0 && noexist_delete) { ret = del_func(idp_id_ctx, del_obj_name); if (ret == ENOENT) { @@ -309,22 +334,268 @@ static errno_t eval_obj_buf(struct idp_id_ctx *idp_id_ctx, return ret; } +static errno_t replace_group_members(struct idp_id_ctx *idp_id_ctx, + const char *group_name, + json_t *data) +{ + struct sss_domain_info *dom = idp_id_ctx->be_ctx->domain; + struct sysdb_attrs *attrs = NULL; + json_t *obj; + json_t *user_name; + char *fqdn = NULL; + char *user_dn = NULL; + size_t index; + errno_t ret; + + attrs = sysdb_new_attrs(idp_id_ctx); + if (attrs == NULL) { + ret = ENOMEM; + goto done; + } + + /* An empty replacement removes all cached members. */ + ret = sysdb_attrs_add_empty(attrs, SYSDB_MEMBER); + if (ret != EOK) { + goto done; + } + + json_array_foreach(data, index, obj) { + user_name = json_object_get(obj, "posixUsername"); + if (!json_is_string(user_name)) { + ret = EINVAL; + goto done; + } + + fqdn = sss_create_internal_fqname(attrs, + json_string_value(user_name), + dom->name); + if (fqdn == NULL) { + ret = ENOMEM; + goto done; + } + + user_dn = sysdb_user_strdn(attrs, dom->name, fqdn); + if (user_dn == NULL) { + ret = ENOMEM; + goto done; + } + + ret = sysdb_attrs_add_string_safe(attrs, SYSDB_MEMBER, user_dn); + if (ret != EOK) { + goto done; + } + } + + ret = sysdb_set_group_attr(dom, group_name, attrs, SYSDB_MOD_REP); + +done: + talloc_free(attrs); + return ret; +} + +static errno_t update_user_groups(struct idp_id_ctx *idp_id_ctx, + const char *user_name, + json_t *data) +{ + struct sss_domain_info *dom = idp_id_ctx->be_ctx->domain; + TALLOC_CTX *tmp_ctx = NULL; + json_t *obj; + json_t *group_name; + char **idp_groups = NULL; + char **sysdb_groups = NULL; + char **add_groups = NULL; + char **del_groups = NULL; + size_t index; + size_t count; + errno_t ret; + + tmp_ctx = talloc_new(NULL); + if (tmp_ctx == NULL) { + return ENOMEM; + } + + count = json_array_size(data); + idp_groups = talloc_zero_array(tmp_ctx, char *, count + 1); + if (idp_groups == NULL) { + ret = ENOMEM; + goto done; + } + + json_array_foreach(data, index, obj) { + group_name = json_object_get(obj, "posixGroupname"); + if (!json_is_string(group_name)) { + ret = EINVAL; + goto done; + } + + idp_groups[index] = sss_create_internal_fqname( + idp_groups, + json_string_value(group_name), + dom->name); + if (idp_groups[index] == NULL) { + ret = ENOMEM; + goto done; + } + } + + ret = sysdb_get_direct_parents(tmp_ctx, dom, dom, SYSDB_MEMBER_USER, + user_name, &sysdb_groups); + if (ret != EOK) { + goto done; + } + + ret = diff_string_lists(tmp_ctx, idp_groups, sysdb_groups, + &add_groups, &del_groups, NULL); + if (ret != EOK) { + goto done; + } + + ret = sysdb_update_members(dom, user_name, SYSDB_MEMBER_USER, + (const char *const *)add_groups, + (const char *const *)del_groups); + +done: + talloc_free(tmp_ctx); + return ret; +} + +static errno_t store_obj_array(struct idp_id_ctx *idp_id_ctx, + const char *type, + store_func_t *store_func, + const char *name, + json_t *data) +{ + json_t *obj; + char *tmp = NULL; + size_t index; + errno_t ret; + + json_array_foreach(data, index, obj) { + ret = store_func(idp_id_ctx, obj, name); + if (ret != EOK) { + tmp = json_dumps(obj, 0); + DEBUG(SSSDBG_OP_FAILURE, "Failed to store JSON %s [%s].\n", type, + tmp); + free(tmp); + return ret; + } + } + + return EOK; +} + +errno_t eval_group_members_buf(struct idp_id_ctx *idp_id_ctx, + const char *group_name, + const uint8_t *buf, ssize_t buflen) +{ + struct sysdb_ctx *sysdb = idp_id_ctx->be_ctx->domain->sysdb; + json_t *data = NULL; + errno_t ret; + errno_t sret; + bool in_transaction = false; + + ret = parse_obj_buf("user", buf, buflen, &data); + if (ret != EOK) { + goto done; + } + + ret = sysdb_transaction_start(sysdb); + if (ret != EOK) { + goto done; + } + in_transaction = true; + + ret = store_obj_array(idp_id_ctx, "user", store_json_user, + group_name, data); + if (ret != EOK) { + goto done; + } + + ret = replace_group_members(idp_id_ctx, group_name, data); + if (ret != EOK) { + goto done; + } + + ret = sysdb_transaction_commit(sysdb); + if (ret == EOK) { + in_transaction = false; + } + +done: + if (in_transaction) { + sret = sysdb_transaction_cancel(sysdb); + if (sret != EOK) { + DEBUG(SSSDBG_CRIT_FAILURE, + "Failed to cancel group membership transaction.\n"); + } + } + json_decref(data); + return ret; +} + +errno_t eval_user_groups_buf(struct idp_id_ctx *idp_id_ctx, + const char *user_name, + const uint8_t *buf, ssize_t buflen) +{ + struct sysdb_ctx *sysdb = idp_id_ctx->be_ctx->domain->sysdb; + json_t *data = NULL; + errno_t ret; + errno_t sret; + bool in_transaction = false; + + ret = parse_obj_buf("group", buf, buflen, &data); + if (ret != EOK) { + goto done; + } + + ret = sysdb_transaction_start(sysdb); + if (ret != EOK) { + goto done; + } + in_transaction = true; + + ret = store_obj_array(idp_id_ctx, "group", store_json_group, + user_name, data); + if (ret != EOK) { + goto done; + } + + ret = update_user_groups(idp_id_ctx, user_name, data); + if (ret != EOK) { + goto done; + } + + ret = sysdb_transaction_commit(sysdb); + if (ret == EOK) { + in_transaction = false; + } + +done: + if (in_transaction) { + sret = sysdb_transaction_cancel(sysdb); + if (sret != EOK) { + DEBUG(SSSDBG_CRIT_FAILURE, + "Failed to cancel user membership transaction.\n"); + } + } + json_decref(data); + return ret; +} + errno_t eval_user_buf(struct idp_id_ctx *idp_id_ctx, - const char *group_name, const char *del_name, bool noexist_delete, uint8_t *buf, ssize_t buflen) { return eval_obj_buf(idp_id_ctx, "user", store_json_user, del_user, - group_name, del_name, noexist_delete, buf, buflen); + NULL, del_name, noexist_delete, buf, buflen); } errno_t eval_group_buf(struct idp_id_ctx *idp_id_ctx, - const char *user_name, const char *del_name, bool noexist_delete, uint8_t *buf, ssize_t buflen) { return eval_obj_buf(idp_id_ctx, "group", store_json_group, del_group, - user_name, del_name, noexist_delete, buf, buflen); + NULL, del_name, noexist_delete, buf, buflen); } diff --git a/src/providers/idp/idp_private.h b/src/providers/idp/idp_private.h index 9b14f85a4e5..fe1eed53bd4 100644 --- a/src/providers/idp/idp_private.h +++ b/src/providers/idp/idp_private.h @@ -33,20 +33,32 @@ * the cache if all required information is available. */ errno_t eval_user_buf(struct idp_id_ctx *idp_id_ctx, - const char *group_name, const char *del_name, bool noexist_delete, uint8_t *buf, ssize_t buflen); +/** @brief Evaluate JSON encoded group member data, store the returned users + * and replace the cached membership of the group. + */ +errno_t eval_group_members_buf(struct idp_id_ctx *idp_id_ctx, + const char *group_name, + const uint8_t *buf, ssize_t buflen); + /** @brief Evaluate JSON encoded group data and store a POSIX group object in * the cache if all required information is available. */ errno_t eval_group_buf(struct idp_id_ctx *idp_id_ctx, - const char *user_name, const char *del_name, bool noexist_delete, uint8_t *buf, ssize_t buflen); +/** @brief Evaluate JSON encoded initgroups data, store the returned groups + * and reconcile the cached group memberships of the user. + */ +errno_t eval_user_groups_buf(struct idp_id_ctx *idp_id_ctx, + const char *user_name, + const uint8_t *buf, ssize_t buflen); + /** Internal data used to identify ongoing OAUTH 2.0 Device Authorization * requests. */ diff --git a/src/tests/system/tests/test_idp.py b/src/tests/system/tests/test_idp.py index 0b74e5e82ea..d217fb16ed6 100644 --- a/src/tests/system/tests/test_idp.py +++ b/src/tests/system/tests/test_idp.py @@ -129,6 +129,90 @@ def test_idp__group_members(client: Client, keycloak: Keycloak, use_fully_qualif assert out.stdout.endswith(f":user1{domain}") +@pytest.mark.parametrize("use_fully_qualified_names", ["true", "false"]) +@pytest.mark.topology(KnownTopology.Keycloak) +@pytest.mark.builtwith(client="idp-provider") +def test_idp__group_member_removal(client: Client, keycloak: Keycloak, use_fully_qualified_names: str): + """ + :title: Refreshing a group removes revoked members + :setup: + 1. Create two users + 2. Create a group with both users as members + :steps: + 1. Look up the group to populate the cache + 2. Check the initial group membership + 3. Remove one user, expire the group cache, and look up the group again + 4. Check the refreshed group membership + :expectedresults: + 1. The group is returned + 2. Both users are returned as members + 3. The group is returned after the cache refresh + 4. Only the remaining user is returned as a member + :customerscenario: True + """ + + user1 = keycloak.user("user1").add(password="Secret123") + user2 = keycloak.user("user2").add(password="Secret123") + group = keycloak.group("group1").add().add_members([user1, user2]) + + client.sssd.dom("test")["use_fully_qualified_names"] = use_fully_qualified_names + domain = f"@{client.sssd.default_domain}" if use_fully_qualified_names == "true" else "" + + client.sssd.start(check_config=False) + + result = client.tools.getent.group(group.name + domain) + assert result is not None + assert set(result.members) == {user1.name + domain, user2.name + domain} + + group.remove_member(user2) + client.sssctl.cache_expire(users=False, groups=True) + + result = client.tools.getent.group(group.name + domain) + assert result is not None + assert result.members == [user1.name + domain] + + +@pytest.mark.parametrize("use_fully_qualified_names", ["true", "false"]) +@pytest.mark.topology(KnownTopology.Keycloak) +@pytest.mark.builtwith(client="idp-provider") +def test_idp__user_group_removal(client: Client, keycloak: Keycloak, use_fully_qualified_names: str): + """ + :title: Refreshing initgroups removes revoked group membership + :setup: + 1. Create a user in a group + :steps: + 1. Look up the user's groups to populate the cache + 2. Check the initial group membership + 3. Remove the user, expire the user cache, and look up the user again + 4. Check the refreshed group membership + :expectedresults: + 1. The user is returned + 2. The added group is returned + 3. The user is returned after the cache refresh + 4. The removed group is no longer returned + :customerscenario: True + """ + + user = keycloak.user("user1").add(password="Secret123") + group = keycloak.group("group1").add().add_member(user) + + client.sssd.dom("test")["use_fully_qualified_names"] = use_fully_qualified_names + domain = f"@{client.sssd.default_domain}" if use_fully_qualified_names == "true" else "" + + client.sssd.start(check_config=False) + + result = client.tools.id(user.name + domain) + assert result is not None + assert result.memberof(group.name + domain) + + group.remove_member(user) + client.sssctl.cache_expire(users=True, groups=False) + + result = client.tools.id(user.name + domain) + assert result is not None + assert not result.memberof(group.name + domain) + + @pytest.mark.parametrize("use_fully_qualified_names", ["true", "false"]) @pytest.mark.topology(KnownTopology.Keycloak) @pytest.mark.builtwith(client="idp-provider")