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