On 07/21/2014 06:29 PM, Michal Židek wrote:
On 07/21/2014 05:54 PM, Michal Židek wrote:
> On 07/18/2014 03:41 PM, Pavel Reichl wrote:
>>
>> Thanks for the quick update, I have some more concerns and questions
>> about the patches.
>>
>> 1st patch:
>>> +int confdb_set_string(struct confdb_ctx *cdb,
>>> + const char *section,
>>> + const char *attribute,
>>> + char *val)
>>> +{
>> [snip]
>>> + lret = ldb_msg_add_string(msg, attribute, val);
>>> + if (lret != LDB_SUCCESS) {
>>> + DEBUG(SSSDBG_MINOR_FAILURE,
>>> + "ldb_msg_add_string failed: [%s]\n",
>>> ldb_strerror(lret));
>>> + ret = EIO;
>>> + goto done;
>>> + }
>>> +
>>> +
>> Two empty lines.
>
> Fixed.
>
>>> + lret = ldb_modify(cdb->ldb, msg);
>>> + if (lret != LDB_SUCCESS) {
>>> + DEBUG(SSSDBG_MINOR_FAILURE,
>>> + "ldb_modify failed: [%s]\n", ldb_strerror(lret));
>>> + ret = EIO;
>>> + goto done;
>>> + }
>>> +
>>> + ret = EOK;
>>> +
>>> +done:
>>> + talloc_free(tmp_ctx);
>>> + if (ret != EOK) {
>>> + DEBUG(SSSDBG_CRIT_FAILURE,
>>> + "Failed to set [%s] from [%s], error [%d] (%s)\n",
>>> + attribute, section, ret, strerror(ret));
>>> + }
>>> + return ret;
>>> +}
>>
>> missing new line here
>
> Fixed.
>
>>
>>> int confdb_get_string(struct confdb_ctx *cdb, TALLOC_CTX *ctx,
>>> const char *section, const char *attribute,
>>> const char *defstr, char **result)
>>> diff --git a/src/confdb/confdb.h b/src/confdb/confdb.h
>>> index ba33ea5..f81c6d4 100644
>>> --- a/src/confdb/confdb.h
>>> +++ b/src/confdb/confdb.h
>>> @@ -216,7 +216,7 @@ struct sss_domain_info {
>>>
>>> bool cache_credentials;
>>> bool legacy_passwords;
>>> - bool case_sensitive;
>>
>> Why do you remove case_sensitive here and add it back in second
>> patch? I
>> think this 'ping-pong' is confusing and needless, could you fix it,
>> please?
>>
>
> Sorry, bad rebasing. Fixed.
>
>>> + bool case_preserve;
>>>
>>> gid_t override_gid;
>>> const char *override_homedir;
>>> @@ -459,6 +459,11 @@ int confdb_set_bool(struct confdb_ctx *cdb,
>>> const char *attribute,
>>> bool val);
>>>
>>
>> Would you consider adding doxygen comment here? All functions with
>> exception of confdb_set_bool() have one.
>> But maybe it's not worth it, what do you think?
>
> I do not think it is necessary in this case, but I added it
> anyway.
Michal you didn't have to do that if you didn't like it. I was just
suggesting the idea. Sorry if you felt like I made you to do that.
>
>>
>>> +int confdb_set_string(struct confdb_ctx *cdb,
>>> + const char *section,
>>> + const char *attribute,
>>> + char *val);
>>> +
>>> /**
>>> * @brief Convenience function to retrieve a single-valued attribute
>>> as a
>>> * null-terminated array of strings
>>> --
>>> 1.9.3
>>
>> Generally, I think there's custom do remove any unused function from
>> code-base when they aren't called. So I think you should remove
>> confdb_set_string(). I guess you will need a new patch for that to keep
>> every commit compilable.
>
> You mean confdb_set_bool? I added new patch to remove
> that function.
>
Yes, thanks.
>>
>> 2nd patch:
>>
>>> @@ -1218,12 +1218,27 @@ static int confdb_get_domain_internal(struct
>>> confdb_ctx *cdb,
>>> }
>>> }
>>>
>>> - ret = get_entry_as_bool(res->msgs[0],
&domain->case_sensitive,
>>> - CONFDB_DOMAIN_CASE_SENSITIVE, true);
>>> - if(ret != EOK) {
>>> - DEBUG(SSSDBG_FATAL_FAILURE,
>>> - "Invalid value for %s\n",
>>> CONFDB_DOMAIN_CASE_SENSITIVE);
>>> - goto done;
>>> + tmp = ldb_msg_find_attr_as_string(res->msgs[0],
>>> + CONFDB_DOMAIN_CASE_SENSITIVE, "true");
>>> + if (tmp != NULL) {
>>> + if (strcasecmp(tmp, "true") == 0) {
>>> + domain->case_sensitive = true;
>>> + domain->case_preserve = true;
>>> + } else if (strcasecmp(tmp, "false") == 0) {
>>> + domain->case_sensitive = false;
>>> + domain->case_preserve = false;
>>> + } else if (strcasecmp(tmp, "preserving") == 0) {
>>> + domain->case_sensitive = false;
>>> + domain->case_preserve = true;
>>> + } else {
>>> + DEBUG(SSSDBG_FATAL_FAILURE,
>>> + "Invalid value for %s\n",
>>> CONFDB_DOMAIN_CASE_SENSITIVE);
>>> + goto done;
>>> + }
>>> + } else {
>>> + /* default */
>>> + domain->case_sensitive = true;
>>> + domain->case_preserve = true;
>>> }
>>> if (domain->case_sensitive == false &&
>>> strcasecmp(domain->provider, "local") == 0) {
>>> diff --git a/src/confdb/confdb.h b/src/confdb/confdb.h
>>> index f81c6d4..8a642b3 100644
>>> --- a/src/confdb/confdb.h
>>> +++ b/src/confdb/confdb.h
>>> @@ -216,6 +216,7 @@ struct sss_domain_info {
>>>
>>> bool cache_credentials;
>>> bool legacy_passwords;
>>> + bool case_sensitive;
>>> bool case_preserve;
>>>
>>> gid_t override_gid;
>>> diff --git a/src/providers/ad/ad_common.c
>>> b/src/providers/ad/ad_common.c
>>> index 67ded36..672a1e1 100644
>>> --- a/src/providers/ad/ad_common.c
>>> +++ b/src/providers/ad/ad_common.c
>>> @@ -263,6 +263,7 @@ ad_get_common_options(TALLOC_CTX *mem_ctx,
>>> char *realm;
>>> char *ad_hostname;
>>> char hostname[HOST_NAME_MAX + 1];
>>> + char *tmp;
>>
>> I personally dislike general variable names, it's my opinion that if
>> you
>> are creating variable for specific use then you should name it
>> accordingly.
>> Of course there are exceptions from this rule, do you see any not to
>> name it 'case_sensitive_str' or something like this?
>
> I wanted to have the same name as in confdb.c, but ok, I will
> change the name in the ad specific code to case_sensitive_opt.
>
>>
>>>
>>> opts = talloc_zero(mem_ctx, struct ad_options);
>>> if (!opts) return ENOMEM;
>>> @@ -333,13 +334,36 @@ ad_get_common_options(TALLOC_CTX *mem_ctx,
>>> }
>>>
>>> /* Active Directory is always case-insensitive */
>>> - dom->case_sensitive = false;
>>> + ret = confdb_get_string(cdb, mem_ctx, conf_path,
>>> + CONFDB_DOMAIN_CASE_SENSITIVE,
"false",
>>> + &tmp);
>>> + if (ret != EOK) {
>>> + DEBUG(SSSDBG_CRIT_FAILURE, "condb_get_string failed.\n");
>>> + goto done;
>>> + }
>>> +
>>> + if (strcasecmp(tmp, "true") == 0) {
>>> + DEBUG(SSSDBG_CRIT_FAILURE,
>>> + "Warning: AD domain can not be set as
>>> case-sensitive.\n");
>>> + dom->case_sensitive = false;
>>> + dom->case_preserve = false;
>>> + } else if (strcasecmp(tmp, "false") == 0) {
>>> + dom->case_sensitive = false;
>>> + dom->case_preserve = false;
>>> + } else if (strcasecmp(tmp, "preserving") == 0) {
>>> + dom->case_sensitive = false;
>>> + dom->case_preserve = true;
>>> + } else {
>>> + DEBUG(SSSDBG_FATAL_FAILURE,
>>> + "Invalid value for %s\n",
>>> CONFDB_DOMAIN_CASE_SENSITIVE);
>>> + goto done;
>>> + }
>>>
>>> /* Set this in the confdb so that the responders pick it
>>> * up when they start up.
>>> */
>>> - ret = confdb_set_bool(cdb, conf_path, "case_sensitive",
>>> - dom->case_sensitive);
>>> + ret = confdb_set_string(cdb, conf_path, "case_sensitive",
>>> + tmp);
>>> if (ret != EOK) {
>>> DEBUG(SSSDBG_CRIT_FAILURE,
>>> "Could not set domain case-sensitive: [%s]\n",
>>> diff --git a/src/providers/ipa/ipa_selinux.c
>>> b/src/providers/ipa/ipa_selinux.c
>>> index 927e545..3cd1cc1 100644
>>> --- a/src/providers/ipa/ipa_selinux.c
>>> +++ b/src/providers/ipa/ipa_selinux.c
>>> @@ -757,7 +757,7 @@ static errno_t write_selinux_login_file(const char
>>> *orig_name,
>>> /* pam_selinux needs the username in the same format getpwnam()
>>> would
>>> * return it
>>> */
>>> - username = sss_get_cased_name(tmp_ctx, orig_name,
>>> dom->case_sensitive);
>>> + username = sss_get_cased_name(tmp_ctx, orig_name,
>>> dom->case_preserve);
>>> if (username == NULL) {
>>> ret = ENOMEM;
>>> goto done;
>>> diff --git a/src/responder/nss/nsssrv_cmd.c
>>> b/src/responder/nss/nsssrv_cmd.c
>>> index a168a3e..c201d3a 100644
>>> --- a/src/responder/nss/nsssrv_cmd.c
>>> +++ b/src/responder/nss/nsssrv_cmd.c
>>> @@ -365,7 +365,7 @@ static int fill_pwent(struct sss_packet *packet,
>>> packet_initialized = true;
>>> }
>>>
>>> - tmpstr = sss_get_cased_name(tmp_ctx, orig_name,
>>> dom->case_sensitive);
>>> + tmpstr = sss_get_cased_name(tmp_ctx, orig_name,
>>> dom->case_preserve);
>>> if (tmpstr == NULL) {
>>> DEBUG(SSSDBG_CRIT_FAILURE,
>>> "sss_get_cased_name failed, skipping\n");
>>> @@ -2492,7 +2492,7 @@ static int fill_grent(struct sss_packet *packet,
>>> }
>>> }
>>>
>>> - tmpstr = sss_get_cased_name(tmp_ctx, orig_name,
>>> dom->case_sensitive);
>>> + tmpstr = sss_get_cased_name(tmp_ctx, orig_name,
>>> dom->case_preserve);
>>> if (tmpstr == NULL) {
>>> DEBUG(SSSDBG_CRIT_FAILURE,
>>> "sss_get_cased_name failed, skipping\n");
>>> --
>>> 1.9.3
>>
>> I don't like very much the code duplication around parsing the option
>> value case_sensitive. Would you consider creating some utility
>> function
>> that would do so and call it from ad_get_common_options() and
>> confdb_get_domain_internal()?
>> Something like:
>>
>> int sss_parse_case_sensitivity_option(const char* str)
>> {
>> if (strcasecmp(str, "true") == 0) {
>> return CASE_INSENSITIVE;
>> } else if (strcasecmp(str, "false") == 0) {
>> return CASE_INSENSITIVE;
>> ....
>>
>>
>
> I do not think this will help. If we added such function
> than the only think that would change would be, that
> the
> 'if (strcasecmp(tmp, "something") == 0)'
> lines would be replaced with
> 'if (ret == SSS_SOMETHING)'.
> Or maybe switch-case might be used, but the
> lines would still have to be there. See that
> I set different values in the AD specific code
> and print additional debug message in one case
> so it is not really duplication and would still
> have to be handled outside of this utility
> function, which is the reason why IMO such
> function would only add code but would not be
> very helpful -- what do you think?).
>
I agree with your concerns about my proposal, however I think you could
reduce number of lines by setting default values for case_sensitive and
case_preserve and then update them just for case where default value is
not the right value.
But generally I don't think this is crucial for the patch. I'm fine if
you don't do any further changes with this part of code. But thanks for
considering.
>>
>> 3rd patch:
>>
>>> @@ -1817,12 +1817,16 @@ fallback_homedir = /home/%u
>>> </varlistentry>
>>>
>>> <varlistentry>
>>> - <term>case_sensitive (boolean)</term>
>>> + <term>case_sensitive (string)</term>
>>> <listitem>
>>> <para>
>>> Treat user and group names as case
>>> sensitive. At
>>> the moment, this option is not
>>> supported in
>>> - the local provider.
>>
+ the local
provider. Possible options are:
>>> + True,
False, Preserving. Preserving is
>>> the
>>> + same as False (case insensitive), but
>>> does
>>> + not lowercase names in the output of
>>> getpwnam
>>> + and getgrnam.
>>> </para>
>>> <para>
>>> Default: True
>> I wonder if it were better to list each option value on its own line.
>> Something like:
>>
>> <para>
>> Treat user and group names as case
>> sensitive. At
>> the moment, this option is not
>> supported in
>> - the local provider. Possible options are:
>> - True, False, Preserving. Preserving is the
>> - same as False (case insensitive), but does
>> + the local provider.
>> + </para>
>> + <para>
>> + Supported values:
>> + </para>
>> + <para>
>> + True: case insensitive
>> + </para>
>> + <para>
>> + True: case sensitive
>> + </para>
>> + <para>
>> + Preserving: case insensitive, but does
>> not lowercase names in the output of
>> getpwnam
>> and g
>>
>> But I think this is a matter of personal taste so I leave it up to you.
>
> Ok, I added a list of possible options.
Thanks, I hope you agree it's
better now.
+ the local provider. Possible options
are:
Could you change it to something like: "the local provider. Possible
option values are:"? Hmm I'm not really sure with wording, I just think
word 'value' should be part of the sentence.
+ <variablelist>
+ <varlistentry>
+
Could you also mention AD specifics for this option?
>
>> Anyway, I think that man page changes must be acked-by native speaker
>> (Stephen),
>>
>> Thanks,
>>
>
The 4th patch had wrong author in description. Sending the
patches again.
Michal
_______________________________________________
sssd-devel mailing list
sssd-devel(a)lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
I finally tested the patches and it seems to me to be working with AD
and LDAP provider, but does not seem to work with IPA provider.
I was not able to create an user in IPA with some upper case so I had to
change it directly in LDAP.
This is what I see when using ipa:
ipa user-find max
--------------
1 user matched
--------------
User login: MaX
First name: xx
Last name: AAA
Home directory: /home/max
Login shell: /bin/sh
Email address: max(a)ipa.work
UID: 1994000038
GID: 1994000038
Account disabled: False
Password: False
Kerberos keys available: False
----------------------------
Number of entries returned 1
----------------------------
This is what I see when using sssd as ipa client:
case_sensitive = preserving:
getent group MaX(a)ipa.work
max@ipa.work:*:1994000038:
case_sensitive = false:
getent group MaX(a)ipa.work
max@ipa.work:*:1994000038:
case_sensitive = true:
does not return anything
Was the patch working with IPA provider to you?
Thanks!