[SSSD] [PATCH] NSS: replace with space with specified character in names.
Pavel Reichl
preichl at redhat.com
Fri Jul 25 08:51:27 UTC 2014
Just a few more questions.
On 07/25/2014 08:40 AM, Pavel Reichl wrote:
> Hello Lukas,
>
> just some nitpicks. Please see inline. Thanks!
>
> On 07/24/2014 11:28 AM, Lukas Slebodnik wrote:
>> ehlo,
>>
>> This patch add possibility to replace whitespace in user and group names with
>> a specified string. With string "-", sssd will return the same result as
>> winbind enabled option "winbind normalize names"
>>
>> Resolves:
>> https://fedorahosted.org/sssd/ticket/1854
>>
>> patch is attached.
>>
>> LS
>>
>>
>> _______________________________________________
>> sssd-devel mailing list
>> sssd-devel at lists.fedorahosted.org
>> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
>> ---
>> src/confdb/confdb.h | 1 +
>> src/config/SSSDConfig/__init__.py.in | 1 +
>> src/config/etc/sssd.api.conf | 1 +
>> src/man/sssd.conf.5.xml | 18 ++++
>> src/responder/nss/nsssrv.c | 5 +
>> src/responder/nss/nsssrv.h | 1 +
>> src/responder/nss/nsssrv_cmd.c | 192 ++++++++++++++++++++++++++++++++++-
>> 7 files changed, 218 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/confdb/confdb.h b/src/confdb/confdb.h
>> index ba33ea5d7123c6e799f13529b60c5938ad20e411..acb139adf6bc1ca2919906fe577333970ccdbfdd 100644
>> --- a/src/confdb/confdb.h
>> +++ b/src/confdb/confdb.h
>> @@ -99,6 +99,7 @@
>> #define CONFDB_MEMCACHE_TIMEOUT "memcache_timeout"
>> #define CONFDB_NSS_HOMEDIR_SUBSTRING "homedir_substring"
>> #define CONFDB_DEFAULT_HOMEDIR_SUBSTRING "/home"
>> +#define CONFDB_NSS_WHITESPACES_REPLACEMENT_STRING "whitespaces_replacement_string"
>>
>> /* PAM */
>> #define CONFDB_PAM_CONF_ENTRY "config/pam"
>> diff --git a/src/config/SSSDConfig/__init__.py.in b/src/config/SSSDConfig/__init__.py.in
>> index 439378ff86d07311f67d51ed775e2d973cd93869..910cdb4b5f0a8cca6ea3328b99722022e2f7ff24 100644
>> --- a/src/config/SSSDConfig/__init__.py.in
>> +++ b/src/config/SSSDConfig/__init__.py.in
>> @@ -73,6 +73,7 @@ option_strings = {
>> 'shell_fallback' : _('If a shell stored in central directory is allowed but not available, use this fallback'),
>> 'default_shell': _('Shell to use if the provider does not list one'),
>> 'memcache_timeout': _('How long will be in-memory cache records valid'),
>> + 'whitespaces_replacement_string': _('All white spaces will be replaced in group name with this string'),
>>
>> # [pam]
>> 'offline_credentials_expiration' : _('How long to allow cached logins between online logins (days)'),
>> diff --git a/src/config/etc/sssd.api.conf b/src/config/etc/sssd.api.conf
>> index 5e5a9284e9ad80d822fe1db4269353aeb7925682..2b4841cec3d5a52002a618a2f38d48a8948ef261 100644
>> --- a/src/config/etc/sssd.api.conf
>> +++ b/src/config/etc/sssd.api.conf
>> @@ -44,6 +44,7 @@ shell_fallback = str, None, false
>> default_shell = str, None, false
>> get_domains_timeout = int, None, false
>> memcache_timeout = int, None, false
>> +whitespaces_replacement_string = str, None, false
>>
>> [pam]
>> # Authentication service
>> diff --git a/src/man/sssd.conf.5.xml b/src/man/sssd.conf.5.xml
>> index 27d22f44ef4449ef9cad026757ff54dd5083672b..6f1c42b0aa3fb22e144fadc19880787e88fb860c 100644
>> --- a/src/man/sssd.conf.5.xml
>> +++ b/src/man/sssd.conf.5.xml
>> @@ -645,6 +645,24 @@ fallback_homedir =/home/%u
>> </para>
>> </listitem>
>> </varlistentry>
>> + <varlistentry>
>> + <term>whitespaces_replacement_string (string)</term>
>> + <listitem>
>> + <para>
>> + This parameter controls whether sssd will replace
>> + whitespace in user and group names with a given
>> + string e.g. (_). For example, whether the name
>> + "john doe" should be replaced with
>> + the string "john_doe". Frequently Unix
>> + shell scripts will have difficulty with usernames
>> + contains whitespace due to the default field
>> + separator in the shell.
>> + </para>
>> + <para>
>> + Default: not set (whitespaces will not be replaced)
>> + </para>
>> + </listitem>
>> + </varlistentry>
>> </variablelist>
>> </refsect2>
>> <refsect2 id='PAM'>
>> diff --git a/src/responder/nss/nsssrv.c b/src/responder/nss/nsssrv.c
>> index 84a6b7fedd096a7d4159b7ac6670820c1d8fd941..422dc6a05906eb2c2fbe5c32b3d0646d69b26891 100644
>> --- a/src/responder/nss/nsssrv.c
>> +++ b/src/responder/nss/nsssrv.c
>> @@ -298,6 +298,11 @@ static int nss_get_config(struct nss_ctx *nctx,
>> &nctx->homedir_substr);
>> if (ret != EOK) goto done;
>>
>> + ret = confdb_get_string(cdb, nctx, CONFDB_NSS_CONF_ENTRY,
>> + CONFDB_NSS_WHITESPACES_REPLACEMENT_STRING, NULL,
>> + &nctx->wsp_replacement_str);
>> + if (ret != EOK) goto done;
>> +
>> ret = 0;
>> done:
>> return ret;
>> diff --git a/src/responder/nss/nsssrv.h b/src/responder/nss/nsssrv.h
>> index a5b946b7e4a38d7d8b35ec5df1b6644d01896470..2e20b886ebbf2bc94ff7278b60b2f015f3eae74d 100644
>> --- a/src/responder/nss/nsssrv.h
>> +++ b/src/responder/nss/nsssrv.h
>> @@ -69,6 +69,7 @@ struct nss_ctx {
>> char **etc_shells;
>> char *shell_fallback;
>> char *default_shell;
>> + char *wsp_replacement_str;
>>
>> struct sss_mc_ctx *pwd_mc_ctx;
>> struct sss_mc_ctx *grp_mc_ctx;
>> diff --git a/src/responder/nss/nsssrv_cmd.c b/src/responder/nss/nsssrv_cmd.c
>> index a168a3e5d6116a1f6ca2c358707650c0e6c9e514..ec272ea06a92730e93ca811f5bc5e8e32380e22f 100644
>> --- a/src/responder/nss/nsssrv_cmd.c
>> +++ b/src/responder/nss/nsssrv_cmd.c
>> @@ -32,6 +32,138 @@
>> #include "sss_client/idmap/sss_nss_idmap.h"
>> #include <time.h>
>>
>> +static const char *replace_whitespaces(TALLOC_CTX *mem_ctx,
>> + const char *orig_name,
>> + const char *replace_string){
> missing space before {
>
>> + char *new_name;
>> + const char *ptr;
>> + size_t replace_string_len;
>> + TALLOC_CTX *tmp_ctx;
>> +
>> + if (replace_string == NULL || replace_string[0] == '\0') {
>> + return orig_name;
>> + }
>> +
>> + replace_string_len = strlen(replace_string);
>> + /* faster implementations without multiple allocations */
>> + if (replace_string_len == 1) {
>> + char *p;
>> + new_name = talloc_strdup(mem_ctx, orig_name);
>> + if (new_name == NULL) {
>> + return NULL;
>> + }
>> +
>> + for (p = new_name; *p != '\0'; ++p) {
>> + if (isspace(*p)){
> missing space
>> + *p = replace_string[0];
>> + }
>> + }
>> +
>> + return new_name;
>> + }
>> +
>> + tmp_ctx = talloc_new(mem_ctx);
Is there any difference in creation of tmp_ctx on NULL and on mem_ctx
int this case? I guess as long as you steal result from tmp_ctx and then
free tmp_ctx it doesn't really matter, right? I guess it's just more
usual to allocate on NULL in utility function.
>> + if (tmp_ctx == NULL) {
>> + return NULL;
>> + }
>> +
>> + new_name = talloc_strdup(tmp_ctx, "");
>> + if (new_name == NULL) {
>> + goto done;
>> + }
>> +
>> + ptr = orig_name;
>> + while (*ptr != '\0') {
>> + if (isspace(*ptr)){
> missing space
>> + new_name = talloc_asprintf_append(new_name, "%s", replace_string);
>> + } else{
>> + new_name = talloc_asprintf_append(new_name, "%c", *ptr);
>> + }
>> + if (new_name == NULL) {
>> + goto done;;
>> + }
>> +
>> + ++ptr;
>> + }
>> +
>> + new_name = talloc_steal(mem_ctx, new_name);
>
> please consider adding new line
>> +done:
>> + talloc_free(tmp_ctx);
>> + return new_name;
>> +}
>> +
>> +static char *reverse_replace_withespaces(TALLOC_CTX *mem_ctx,
>> + char *orig_name,
>> + const char *replace_string){
> missing space
>> + TALLOC_CTX *tmp_ctx;
>> + char *substr;
>> + char *new_name;
>> + const char *ptr = orig_name;
>> + size_t replace_string_len;
>> +
>> + if (replace_string == NULL || replace_string[0] == '\0') {
>> + return orig_name;
>> + }
>> +
>> + replace_string_len = strlen(replace_string);
>> + /* faster implementations without multiple allocations */
>> + if (replace_string_len == 1) {
>> + char *p;
>> + new_name = talloc_strdup(mem_ctx, orig_name);
>> + if (new_name == NULL) {
>> + return NULL;
>> + }
>> +
>> + for (p = new_name; *p != '\0'; ++p) {
>> + if (*p == replace_string[0] ){
>> + *p = ' ';
>> + }
>> + }
>> +
>> + return new_name;
>> + }
>> +
>> + tmp_ctx = talloc_new(mem_ctx);
>> + if (tmp_ctx == NULL) {
>> + return NULL;
>> + }
>> +
>> + new_name = talloc_strdup(tmp_ctx, "");
>> + if (new_name == NULL) {
>> + goto done;
>> + }
>> +
>> + do {
>> + substr = strstr(ptr, replace_string);
>> + if (substr != NULL) {
>> + new_name = talloc_asprintf_append(new_name, "%.*s",
>> + (int)(substr - ptr), ptr);
>> + if (new_name == NULL) {
>> + goto done;
>> + }
>> + ptr += substr - ptr;
>> + ptr += replace_string_len;
>> +
>> + /* append space */
>> + new_name = talloc_asprintf_append(new_name, " ");
You were interested in saving allocations - could adding space be part
of prev talloc_asprintf_append?
>> + if (new_name == NULL) {
>> + goto done;
>> + }
>> + } else {
>> + new_name = talloc_asprintf_append(new_name, "%s", ptr);
>> + if (new_name == NULL) {
>> + goto done;
>> + }
>> + break;
Why the break here? do-while loop would end anyway, right?
>> + }
>> + } while (substr != NULL);
>> +
>> + new_name = talloc_steal(mem_ctx, new_name);
> You might consider adding new line here. If you don't want then don't.
>> +done:
>> + talloc_free(tmp_ctx);
>> + return new_name;
>> +}
>> +
>> static int nss_cmd_send_error(struct nss_cmd_ctx *cmdctx, int err)
>> {
>> return sss_cmd_send_error(cmdctx->cctx, err);
>> @@ -371,6 +503,15 @@ static int fill_pwent(struct sss_packet *packet,
>> "sss_get_cased_name failed, skipping\n");
>> continue;
>> }
>> +
>> + tmpstr = replace_whitespaces(tmp_ctx, tmpstr,
>> + nctx->wsp_replacement_str);
>> + if (tmpstr == NULL) {
>> + DEBUG(SSSDBG_CRIT_FAILURE,
>> + "replace_whitespaces failed, skipping\n");
>> + continue;
>> + }
>> +
>> to_sized_string(&name, tmpstr);
>>
>> tmpstr = ldb_msg_find_attr_as_string(msg, SYSDB_GECOS, NULL);
>> @@ -743,6 +884,14 @@ static int nss_cmd_getpwnam_search(struct nss_dom_ctx *dctx)
>> name = sss_get_cased_name(cmdctx, cmdctx->name, dom->case_sensitive);
>> if (!name) return ENOMEM;
>>
>> + name = reverse_replace_withespaces(dctx, name,
>> + nctx->wsp_replacement_str);
>> + if (name == NULL) {
>> + DEBUG(SSSDBG_CRIT_FAILURE,
>> + "reverse_replace_withespaces failed, skipping\n");
>> + continue;
>> + }
>> +
>> /* verify this user has not yet been negatively cached,
>> * or has been permanently filtered */
>> ret = sss_ncache_check_user(nctx->ncache, nctx->neg_timeout,
>> @@ -2316,7 +2465,7 @@ static int fill_members(struct sss_packet *packet,
>> int memnum = *_memnum;
>> size_t rzero= *_rzero;
>> size_t rsize = *_rsize;
>> - char *tmpstr;
>> + const char *tmpstr;
>> struct sized_string name;
>> TALLOC_CTX *tmp_ctx = NULL;
>>
>> @@ -2344,6 +2493,14 @@ static int fill_members(struct sss_packet *packet,
>> continue;
>> }
>>
>> + tmpstr = replace_whitespaces(tmp_ctx, tmpstr,
>> + nctx->wsp_replacement_str);
>> + if (tmpstr == NULL) {
>> + DEBUG(SSSDBG_CRIT_FAILURE,
>> + "replace_whitespaces failed, skipping\n");
>> + continue;
>> + }
>> +
>> if (nctx->filter_users_in_groups) {
>> ret = sss_ncache_check_user(nctx->ncache,
>> nctx->neg_timeout,
>> @@ -2498,6 +2655,15 @@ static int fill_grent(struct sss_packet *packet,
>> "sss_get_cased_name failed, skipping\n");
>> continue;
>> }
>> +
>> + tmpstr = replace_whitespaces(tmp_ctx, tmpstr,
>> + nctx->wsp_replacement_str);
>> + if (tmpstr == NULL) {
>> + DEBUG(SSSDBG_CRIT_FAILURE,
>> + "replace_whitespaces failed, skipping\n");
>> + continue;
>> + }
>> +
>> to_sized_string(&name, tmpstr);
>>
>> /* fill in gid and name and set pointer for number of members */
>> @@ -2692,6 +2858,14 @@ static int nss_cmd_getgrnam_search(struct nss_dom_ctx *dctx)
>> name = sss_get_cased_name(dctx, cmdctx->name, dom->case_sensitive);
>> if (!name) return ENOMEM;
>>
>> + name = reverse_replace_withespaces(dctx, name,
>> + nctx->wsp_replacement_str);
>> + if (name == NULL) {
>> + DEBUG(SSSDBG_CRIT_FAILURE,
>> + "reverse_replace_withespaces failed, skipping\n");
>> + continue;
>> + }
>> +
>> /* verify this group has not yet been negatively cached,
>> * or has been permanently filtered */
> If you are changing nearby code can you add a space before *?
>> ret = sss_ncache_check_group(nctx->ncache, nctx->neg_timeout,
>> @@ -3715,6 +3889,14 @@ static int nss_cmd_initgroups_search(struct nss_dom_ctx *dctx)
>> name = sss_get_cased_name(dctx, cmdctx->name, dom->case_sensitive);
>> if (!name) return ENOMEM;
>>
>> + name = reverse_replace_withespaces(dctx, name,
>> + nctx->wsp_replacement_str);
>> + if (name == NULL) {
>> + DEBUG(SSSDBG_CRIT_FAILURE,
>> + "reverse_replace_withespaces failed, skipping\n");
>> + continue;
>> + }
>> +
>> /* verify this user has not yet been negatively cached,
>> * or has been permanently filtered */
> same as above please
>> ret = sss_ncache_check_user(nctx->ncache, nctx->neg_timeout,
>> @@ -3874,6 +4056,14 @@ static errno_t nss_cmd_getsidby_search(struct nss_dom_ctx *dctx)
>> goto done;
>> }
>>
>> + name = reverse_replace_withespaces(dctx, name,
>> + nctx->wsp_replacement_str);
>> + if (name == NULL) {
>> + DEBUG(SSSDBG_CRIT_FAILURE,
>> + "reverse_replace_withespaces failed, skipping\n");
>> + continue;
>> + }
>> +
>> /* For subdomains a fully qualified name is needed for
>> * sysdb_search_user_by_name and sysdb_search_group_by_name. */
>> if (IS_SUBDOMAIN(dom)) {
>
>
>
>
> _______________________________________________
> sssd-devel mailing list
> sssd-devel at lists.fedorahosted.org
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20140725/59dec706/attachment.html>
More information about the sssd-devel
mailing list