Re: [SSSD] [PATCHES] sss_case = preserving

2014-07-29 Thread Jakub Hrozek 
On Tue, Jul 22, 2014 at 09:34:25AM -0400, Stephen Gallagher wrote:
 -BEGIN PGP SIGNED MESSAGE-
 Hash: SHA1
 
 On 07/22/2014 09:32 AM, Michal Židek wrote:
  On 07/22/2014 02:49 PM, Pavel Reichl wrote:
  
  On 07/22/2014 02:03 PM, Pavel Reichl wrote:
  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: m...@ipa.work UID:
  199438 GID: 199438 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 m...@ipa.work 
  m...@ipa.work:*:199438:
  
  case_sensitive = false: getent group m...@ipa.work 
  m...@ipa.work:*:199438:
  
  case_sensitive = true: does not return anything
  
  Was the patch working with IPA provider to you?
  
  Thanks!
  
  Michal this seems irrelevant now, I can't replicate it anymore.
  It must have been some quirk on my side. Sorry for that.
  
  ACK to all patches except the 3rd (the man page update) which I
  can't ack.
  
  Thanks!
  

Pushed to master:
2b94ab415b30861f42b68725d9231905baf8c3bd
abbf4f494f57c2b0a7ad0ac758db24a1c05df9be
ff22e829fd73fc53027d1e6ca005a9ac334086dd
5328aaeea84268b6d4e26cd33a2b3e8ea89bc349
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

Re: [SSSD] [PATCHES] sss_case = preserving

2014-07-22 Thread Pavel Reichl 


On 07/22/2014 02:03 PM, Pavel Reichl wrote:
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: m...@ipa.work
  UID: 199438
  GID: 199438
  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 m...@ipa.work
m...@ipa.work:*:199438:

case_sensitive = false:
getent group m...@ipa.work
m...@ipa.work:*:199438:

case_sensitive = true:
does not return anything

Was the patch working with IPA provider to you?

Thanks!

Michal this seems irrelevant now, I can't replicate it anymore. It must 
have been some quirk on my side. Sorry for that.


ACK to all patches except the 3rd (the man page update) which I can't ack.

Thanks!



___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

Re: [SSSD] [PATCHES] sss_case = preserving

2014-07-22 Thread Michal Židek 

On 07/22/2014 02:49 PM, Pavel Reichl wrote:


On 07/22/2014 02:03 PM, Pavel Reichl wrote:

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: m...@ipa.work
  UID: 199438
  GID: 199438
  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 m...@ipa.work
m...@ipa.work:*:199438:

case_sensitive = false:
getent group m...@ipa.work
m...@ipa.work:*:199438:

case_sensitive = true:
does not return anything

Was the patch working with IPA provider to you?

Thanks!


Michal this seems irrelevant now, I can't replicate it anymore. It must
have been some quirk on my side. Sorry for that.

ACK to all patches except the 3rd (the man page update) which I can't ack.

Thanks!



Thanks, I updated the man page changes (added the value word
and AD comment). CC-ing Stephen for the man page review (3rd patch).

Michal


From fdad249f822089f872377df512c1d14cd05a00d2 Mon Sep 17 00:00:00 2001
From: Michal Zidek mzi...@redhat.com
Date: Tue, 15 Jul 2014 12:00:36 -0400
Subject: [PATCH 1/4] Add function confdb_set_string.

Part of fix for:
https://fedorahosted.org/sssd/ticket/2367
---
 src/confdb/confdb.c | 71 +
 src/confdb/confdb.h | 22 +
 2 files changed, 93 insertions(+)

diff --git a/src/confdb/confdb.c b/src/confdb/confdb.c
index 15de961..ae7abd7 100644
--- a/src/confdb/confdb.c
+++ b/src/confdb/confdb.c
@@ -369,6 +369,77 @@ done:
 return ret;
 }
 
+int confdb_set_string(struct confdb_ctx *cdb,
+  const char *section,
+  const char *attribute,
+  char *val)
+{
+TALLOC_CTX *tmp_ctx;
+struct ldb_dn *dn;
+char *secdn;
+struct ldb_message *msg;
+int ret, lret;
+
+tmp_ctx = talloc_new(NULL);
+if (!tmp_ctx) {
+return ENOMEM;
+}
+
+ret = parse_section(tmp_ctx, section, secdn, NULL);
+if (ret != EOK) {
+goto done;
+}
+
+dn = ldb_dn_new(tmp_ctx, cdb-ldb, secdn);
+if (!dn) {
+ret = EIO;
+goto done;
+}
+
+msg = ldb_msg_new(tmp_ctx);
+if (!msg) {
+ret = ENOMEM;
+goto done;
+}
+
+msg-dn = dn;
+
+lret = ldb_msg_add_empty(msg, attribute, LDB_FLAG_MOD_REPLACE, NULL);
+if (lret != LDB_SUCCESS) {
+DEBUG(SSSDBG_MINOR_FAILURE,
+  ldb_msg_add_empty failed: [%s]\n, ldb_strerror(lret));
+ret = EIO;
+goto done;
+}
+
+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;
+}
+
+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;
+}
+
 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..7c51518 100644
--- a/src/confdb/confdb.h
+++ b/src/confdb/confdb.h
@@ -460,6 +460,28 @@ int confdb_set_bool(struct confdb_ctx *cdb,
  bool val);
 
 /**
+ * @brief Convenience function to set a single-valued attribute as a string
+ *
+ * @param[in] cdb The connection object to the confdb
+ * @param[in] section The ConfDB section to update. This is constructed from
+ *the format of the sssd.conf file. All sections start
+ *with 'config/'. Subsections are separated by slashes.
+ *e.g. [domain/LDAP] in sssd.conf would translate to
+ *config/domain/LDAP
+ * @param[in] attribute The name of the attribute to update
+ * @param[in] val New value of the attribute.
+ *
+ * @return 0 - Successfully retrieved the entry (or used the default)
+ * @return ENOMEM - There was insufficient memory to complete the operation
+ * @return EINVAL - The section could not be parsed
+ * @return EIO -

Re: [SSSD] [PATCHES] sss_case = preserving

2014-07-22 Thread Stephen Gallagher 
-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1

On 07/22/2014 09:32 AM, Michal Židek wrote:
 On 07/22/2014 02:49 PM, Pavel Reichl wrote:
 
 On 07/22/2014 02:03 PM, Pavel Reichl wrote:
 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: m...@ipa.work UID:
 199438 GID: 199438 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 m...@ipa.work 
 m...@ipa.work:*:199438:
 
 case_sensitive = false: getent group m...@ipa.work 
 m...@ipa.work:*:199438:
 
 case_sensitive = true: does not return anything
 
 Was the patch working with IPA provider to you?
 
 Thanks!
 
 Michal this seems irrelevant now, I can't replicate it anymore.
 It must have been some quirk on my side. Sorry for that.
 
 ACK to all patches except the 3rd (the man page update) which I
 can't ack.
 
 Thanks!
 
 
 Thanks, I updated the man page changes (added the value word and
 AD comment). CC-ing Stephen for the man page review (3rd patch).
 
 Michal
 
 

Ack to the manpage language.
-BEGIN PGP SIGNATURE-
Version: GnuPG v1
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iEYEARECAAYFAlPOaGEACgkQeiVVYja6o6NhvwCeIQ7upLLCa2PdfWha7tfssuSC
hSoAn1blLO+hqsJi4vSiD7NCr692u5jo
=uPQw
-END PGP SIGNATURE-
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

Re: [SSSD] [PATCHES] sss_case = preserving

2014-07-21 Thread Michal Židek 

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.




+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.



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

Re: [SSSD] [PATCHES] sss_case = preserving

2014-07-21 Thread Michal Židek 

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.




+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.



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

Re: [SSSD] [PATCHES] sss_case = preserving

2014-07-18 Thread Pavel Reichl 


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.

+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


 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?



+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?


+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.


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?




 opts = talloc_zero(mem_ctx, struct ad_options);
 if (!opts) return ENOMEM;
@@ -333,13 +334,36 @@ ad_get_common_options(TALLOC_CTX *mem_ctx,
 }

 /*

Re: [SSSD] [PATCHES] sss_case = preserving

2014-07-17 Thread Pavel Reichl 
On Wed, 2014-07-16 at 15:40 +0200, Michal Židek wrote:
 Hi,
 
 patches for ticket
 https://fedorahosted.org/sssd/ticket/2367
 are in attachment.
 
 Michal
 
 ___
 sssd-devel mailing list
 sssd-devel@lists.fedorahosted.org
 https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

Hi Michal,

I haven't tested the patches yet. I just wanted to share some nitpicks
with you:


 From 071b3ca8cd9a194c8cb287f9abca2fe7c58323a2 Mon Sep 17 00:00:00 2001
 From: Michal Zidek mzi...@redhat.com
 Date: Tue, 15 Jul 2014 12:00:36 -0400
 Subject: [PATCH 1/3] Add function confdb_set_string.
 
 ---
  src/confdb/confdb.c | 70 
 +
  src/confdb/confdb.h |  6 +
  2 files changed, 76 insertions(+)
 
 diff --git a/src/confdb/confdb.c b/src/confdb/confdb.c
 index 15de961..79c89b7 100644
 --- a/src/confdb/confdb.c
 +++ b/src/confdb/confdb.c
 @@ -369,6 +369,76 @@ done:
  return ret;
  }
  
 +int confdb_set_string(struct confdb_ctx *cdb,
 +  const char *section,
 +  const char *attribute,
 +  char *val)
 +{
 +TALLOC_CTX *tmp_ctx;
 +struct ldb_dn *dn;
 +char *secdn;
 +struct ldb_message *msg;
 +int ret, lret;
 +
 +tmp_ctx = talloc_new(NULL);
 +if (!tmp_ctx)
 +return ENOMEM;

Although there's not a consensus about the form of checking allocated
pointers ( !tmp_ctx vs. tmp_ctx != NULL ) among SSSD developers, still I
think there is an agreement that 'if' should be followed by block or
condition and action should be one-liner.

Could you change the code to something like?

if (!tmp_ctx) return ENOMEM;

or

if (!tmp_ctx) { 
return ENOMEM;
}

In the second patch you use 2 forms of testing result of strcasecmp

 !strcasecmp(tmp, true)
 strcasecmp(domain-provider, local) == 0
 
Could you please just use one of them? From my POV the second is
preferred. 



___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

Re: [SSSD] [PATCHES] sss_case = preserving

2014-07-17 Thread Michal Židek 

On 07/17/2014 06:12 PM, Pavel Reichl wrote:

On Wed, 2014-07-16 at 15:40 +0200, Michal Židek wrote:

Hi,

patches for ticket
https://fedorahosted.org/sssd/ticket/2367
are in attachment.

Michal

___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel


Hi Michal,

I haven't tested the patches yet. I just wanted to share some nitpicks
with you:



 From 071b3ca8cd9a194c8cb287f9abca2fe7c58323a2 Mon Sep 17 00:00:00 2001
From: Michal Zidek mzi...@redhat.com
Date: Tue, 15 Jul 2014 12:00:36 -0400
Subject: [PATCH 1/3] Add function confdb_set_string.

---
  src/confdb/confdb.c | 70 +
  src/confdb/confdb.h |  6 +
  2 files changed, 76 insertions(+)

diff --git a/src/confdb/confdb.c b/src/confdb/confdb.c
index 15de961..79c89b7 100644
--- a/src/confdb/confdb.c
+++ b/src/confdb/confdb.c
@@ -369,6 +369,76 @@ done:
  return ret;
  }

+int confdb_set_string(struct confdb_ctx *cdb,
+  const char *section,
+  const char *attribute,
+  char *val)
+{
+TALLOC_CTX *tmp_ctx;
+struct ldb_dn *dn;
+char *secdn;
+struct ldb_message *msg;
+int ret, lret;
+
+tmp_ctx = talloc_new(NULL);
+if (!tmp_ctx)
+return ENOMEM;


Although there's not a consensus about the form of checking allocated
pointers ( !tmp_ctx vs. tmp_ctx != NULL ) among SSSD developers, still I
think there is an agreement that 'if' should be followed by block or
condition and action should be one-liner.

Could you change the code to something like?

if (!tmp_ctx) return ENOMEM;

or

if (!tmp_ctx) {
return ENOMEM;
}


Fixed.



In the second patch you use 2 forms of testing result of strcasecmp


!strcasecmp(tmp, true)
strcasecmp(domain-provider, local) == 0


Could you please just use one of them? From my POV the second is
preferred.


Fixed.





___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel



Thank you Pavel!
New version is attached.

Michal
From bb62af3c02a539930352a2d9d0e350d217fc443d Mon Sep 17 00:00:00 2001
From: Michal Zidek mzi...@redhat.com
Date: Tue, 15 Jul 2014 12:00:36 -0400
Subject: [PATCH 1/3] Add function confdb_set_string.

Part of fix for:
https://fedorahosted.org/sssd/ticket/2367
---
 src/confdb/confdb.c | 71 +
 src/confdb/confdb.h |  7 +-
 2 files changed, 77 insertions(+), 1 deletion(-)

diff --git a/src/confdb/confdb.c b/src/confdb/confdb.c
index 15de961..b97e0bf 100644
--- a/src/confdb/confdb.c
+++ b/src/confdb/confdb.c
@@ -369,6 +369,77 @@ done:
 return ret;
 }
 
+int confdb_set_string(struct confdb_ctx *cdb,
+  const char *section,
+  const char *attribute,
+  char *val)
+{
+TALLOC_CTX *tmp_ctx;
+struct ldb_dn *dn;
+char *secdn;
+struct ldb_message *msg;
+int ret, lret;
+
+tmp_ctx = talloc_new(NULL);
+if (!tmp_ctx) {
+return ENOMEM;
+}
+
+ret = parse_section(tmp_ctx, section, secdn, NULL);
+if (ret != EOK) {
+goto done;
+}
+
+dn = ldb_dn_new(tmp_ctx, cdb-ldb, secdn);
+if (!dn) {
+ret = EIO;
+goto done;
+}
+
+msg = ldb_msg_new(tmp_ctx);
+if (!msg) {
+ret = ENOMEM;
+goto done;
+}
+
+msg-dn = dn;
+
+lret = ldb_msg_add_empty(msg, attribute, LDB_FLAG_MOD_REPLACE, NULL);
+if (lret != LDB_SUCCESS) {
+DEBUG(SSSDBG_MINOR_FAILURE,
+  ldb_msg_add_empty failed: [%s]\n, ldb_strerror(lret));
+ret = EIO;
+goto done;
+}
+
+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;
+}
+
+
+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;
+}
 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;
+bool

Re: [SSSD] [PATCHES] sss_case = preserving

2014-07-16 Thread Michal Židek 

On 07/16/2014 03:40 PM, Michal Židek wrote:

Hi,

patches for ticket
https://fedorahosted.org/sssd/ticket/2367
are in attachment.

Michal



I forgot to add reference to the ticket in the
patch description. New patches are attached.

Michal

From 5f7094e93988d6aa475ea33ceb87d380737b4795 Mon Sep 17 00:00:00 2001
From: Michal Zidek mzi...@redhat.com
Date: Tue, 15 Jul 2014 12:00:36 -0400
Subject: [PATCH 1/3] Add function confdb_set_string.

Part of fix for:
https://fedorahosted.org/sssd/ticket/2367
---
 src/confdb/confdb.c | 70 +
 src/confdb/confdb.h |  6 +
 2 files changed, 76 insertions(+)

diff --git a/src/confdb/confdb.c b/src/confdb/confdb.c
index 15de961..79c89b7 100644
--- a/src/confdb/confdb.c
+++ b/src/confdb/confdb.c
@@ -369,6 +369,76 @@ done:
 return ret;
 }
 
+int confdb_set_string(struct confdb_ctx *cdb,
+  const char *section,
+  const char *attribute,
+  char *val)
+{
+TALLOC_CTX *tmp_ctx;
+struct ldb_dn *dn;
+char *secdn;
+struct ldb_message *msg;
+int ret, lret;
+
+tmp_ctx = talloc_new(NULL);
+if (!tmp_ctx)
+return ENOMEM;
+
+ret = parse_section(tmp_ctx, section, secdn, NULL);
+if (ret != EOK) {
+goto done;
+}
+
+dn = ldb_dn_new(tmp_ctx, cdb-ldb, secdn);
+if (!dn) {
+ret = EIO;
+goto done;
+}
+
+msg = ldb_msg_new(tmp_ctx);
+if (!msg) {
+ret = ENOMEM;
+goto done;
+}
+
+msg-dn = dn;
+
+lret = ldb_msg_add_empty(msg, attribute, LDB_FLAG_MOD_REPLACE, NULL);
+if (lret != LDB_SUCCESS) {
+DEBUG(SSSDBG_MINOR_FAILURE,
+  ldb_msg_add_empty failed: [%s]\n, ldb_strerror(lret));
+ret = EIO;
+goto done;
+}
+
+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;
+}
+
+
+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;
+}
 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..8a642b3 100644
--- a/src/confdb/confdb.h
+++ b/src/confdb/confdb.h
@@ -217,6 +217,7 @@ struct sss_domain_info {
 bool cache_credentials;
 bool legacy_passwords;
 bool case_sensitive;
+bool case_preserve;
 
 gid_t override_gid;
 const char *override_homedir;
@@ -459,6 +460,11 @@ int confdb_set_bool(struct confdb_ctx *cdb,
  const char *attribute,
  bool val);
 
+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.7.11.2

From 3ebc6f3755f8f24056ef99634cc0cab79d8b0f74 Mon Sep 17 00:00:00 2001
From: Michal Zidek mzi...@redhat.com
Date: Tue, 15 Jul 2014 12:10:34 -0400
Subject: [PATCH 2/3] case_sensitivity = preserving

If case_sensitivity is set to 'preserving', getXXnam
returns name attribute in the same format as
stored in LDAP.

Fixes:
https://fedorahosted.org/sssd/ticket/2367
---
 src/confdb/confdb.c | 31 +++
 src/providers/ad/ad_common.c| 30 +++---
 src/providers/ipa/ipa_selinux.c |  2 +-
 src/responder/nss/nsssrv_cmd.c  |  4 ++--
 4 files changed, 53 insertions(+), 14 deletions(-)

diff --git a/src/confdb/confdb.c b/src/confdb/confdb.c
index 79c89b7..674a2c4 100644
--- a/src/confdb/confdb.c
+++ b/src/confdb/confdb.c
@@ -1217,15 +1217,30 @@ 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)) {
+domain-case_sensitive = true;
+domain-case_preserve = true;
+} else if