[sssd PR#246][opened] filter_users and filter_groups stop working properly in v 1.15

[fidencio]

URL: https://github.com/SSSD/sssd/pull/246
Author: fidencio
 Title: #246: filter_users and filter_groups stop working properly in v 1.15
Action: opened

PR body:
"""
This patchset fix the issue reported on https://pagure.io/SSSD/sssd/issue/3362.

@pbrezina suggested to do the changes in a new cache_req module, but I'm really not sure whether we want to have NSS specific code (like nss_get_pwent() and nss_get_grent() calls) there.

For now I'm leaving this as it was before the nss/cache_req refactoring.
"""

To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/246/head:pr246
git checkout pr246

[sssd PR#246][comment] filter_users and filter_groups stop working properly in v 1.15

[pbrezina]

URL: https://github.com/SSSD/sssd/pull/246
Title: #246: filter_users and filter_groups stop working properly in v 1.15

pbrezina commented:
"""
Hi, we should solve this on `cache_req` level so we get the same resultt in `nss` and `ifp` (and others) responders. What I had in mind was to create another plugin function that will be called in the and of `cache_req` process. I.e. something like this:

```c
/**
 * Filter the result through negative cache. 
 *
 * This is useful for plugins that don't use name as an input token but can be affected by filter_users and filter_groups options. 
 *
 * @return EOK    If the object is not found.
 * @return EEXIST If the object is found in negative cache.
 * @return Other errno code in case of an error.
 */
typedef errno_t
(*cache_req_ncache_filter_fn)(struct sss_nc_ctx *ncache,
                             struct sss_domain_info *domain,
                             struct ldb_message *msg);
```
In the end, we should iterate over the result and pass each ldb_message to the function (if defined) and remove it from the result if it is present in the negative cache.
"""

See the full comment at https://github.com/SSSD/sssd/pull/246#issuecomment-296985950

[sssd PR#246][comment] filter_users and filter_groups stop working properly in v 1.15

[pbrezina]

URL: https://github.com/SSSD/sssd/pull/246
Title: #246: filter_users and filter_groups stop working properly in v 1.15

pbrezina commented:
"""
This is really from top of my mind, there may be another, easier way to do it. But we should do it on cache_req level.
"""

See the full comment at https://github.com/SSSD/sssd/pull/246#issuecomment-296986260

[sssd PR#246][comment] filter_users and filter_groups stop working properly in v 1.15

[fidencio]

URL: https://github.com/SSSD/sssd/pull/246
Title: #246: filter_users and filter_groups stop working properly in v 1.15

fidencio commented:
"""
On Tue, Apr 25, 2017 at 12:16 PM, Pavel Březina 
wrote:
> Hi, we should solve this on cache_req level so we get the same resultt in
> nss and ifp (and others) responders. What I had in mind was to create
> another plugin function that will be called in the and of cache_req
> process. I.e. something like this:
>
> /** * Filter the result through negative cache.  * * This is useful for plugins that don't use name as an input token but can be affected by filter_users and filter_groups options.  * * @return EOK    If the object is not found. * @return EEXIST If the object is found in negative cache. * @return Other errno code in case of an error. */typedef errno_t
> (*cache_req_ncache_filter_fn)(struct sss_nc_ctx *ncache,
>                              struct sss_domain_info *domain,
>                              struct ldb_message *msg);
>
> In the end, we should iterate over the result and pass each ldb_message to
> the function (if defined) and remove it from the result if it is present in
> the negative cache.

I had the same thought Yesterday but for doing this I'd have to expose some
internals from NSS responder in order to actually have the name and group
relative to the searched id ... as it is, IMO, too intrusive at this point.

As it's a regression, I really would prefer to keep the patch fixing the
regression and introduce the new cache_req module later on (without being
worried on breaking something else right now).

Anyways, it's up to you. Would you prefer to have these changes done *now*?
> —
> You are receiving this because you authored the thread.
> Reply to this email directly, view it on GitHub
> https://github.com/SSSD/sssd/pull/246#issuecomment-296985950, or mute
> the thread
> https://github.com/notifications/unsubscribe-auth/AAG4epUZ7M2riOZzAKK1x_WutGz-jVNqks5rzcfkgaJpZM4NGkQK
> .

"""

See the full comment at https://github.com/SSSD/sssd/pull/246#issuecomment-296994965

[sssd PR#246][comment] filter_users and filter_groups stop working properly in v 1.15

[lslebodn]

URL: https://github.com/SSSD/sssd/pull/246
Title: #246: filter_users and filter_groups stop working properly in v 1.15

lslebodn commented:
"""
On (25/04/17 03:59), fidencio wrote:
> On Tue, Apr 25, 2017 at 12:16 PM, Pavel Březina 
> wrote:
> > Hi, we should solve this on cache_req level so we get the same resultt in
> > nss and ifp (and others) responders. What I had in mind was to create
> > another plugin function that will be called in the and of cache_req
> > process. I.e. something like this:
> >
> > /** * Filter the result through negative cache.  * * This is useful for plugins that don't use name as an input token but can be affected by filter_users and filter_groups options.  * * @return EOK    If the object is not found. * @return EEXIST If the object is found in negative cache. * @return Other errno code in case of an error. */typedef errno_t
> > (*cache_req_ncache_filter_fn)(struct sss_nc_ctx *ncache,
> >                              struct sss_domain_info *domain,
> >                              struct ldb_message *msg);
> >
> > In the end, we should iterate over the result and pass each ldb_message to
> > the function (if defined) and remove it from the result if it is present in
> > the negative cache.
>
> I had the same thought Yesterday but for doing this I'd have to expose some
> internals from NSS responder in order to actually have the name and group
> relative to the searched id ... as it is, IMO, too intrusive at this point.
>
> As it's a regression, I really would prefer to keep the patch fixing the
> regression and introduce the new cache_req module later on (without being
> worried on breaking something else right now).

-1.

There is no reason to use temporary solution.

LS

"""

See the full comment at https://github.com/SSSD/sssd/pull/246#issuecomment-296999474

[sssd PR#246][synchronized] filter_users and filter_groups stop working properly in v 1.15

[fidencio]

URL: https://github.com/SSSD/sssd/pull/246
Author: fidencio
 Title: #246: filter_users and filter_groups stop working properly in v 1.15
Action: synchronized

To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/246/head:pr246
git checkout pr246

[sssd PR#246][comment] filter_users and filter_groups stop working properly in v 1.15

[fidencio]

URL: https://github.com/SSSD/sssd/pull/246
Title: #246: filter_users and filter_groups stop working properly in v 1.15

fidencio commented:
"""
New patch set pushed and in the end the changes were not intrusive at all.

@pbrezina, I've partially taken your idea (with some changes).
Please, let me know if you're okay of the way the new plugin function has been introduced.
"""

See the full comment at https://github.com/SSSD/sssd/pull/246#issuecomment-297186170

[sssd PR#246][synchronized] filter_users and filter_groups stop working properly in v 1.15

[fidencio]

URL: https://github.com/SSSD/sssd/pull/246
Author: fidencio
 Title: #246: filter_users and filter_groups stop working properly in v 1.15
Action: synchronized

To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/246/head:pr246
git checkout pr246

[sssd PR#246][comment] filter_users and filter_groups stop working properly in v 1.15

[pbrezina]

URL: https://github.com/SSSD/sssd/pull/246
Title: #246: filter_users and filter_groups stop working properly in v 1.15

pbrezina commented:
"""
* **NSS: Use fqnames when performing a ncache check**
This should be done in the ncache module, not in the callers.

* **RESPONDER: Make nss_get_name_from_msg() part of responder_utils**
Nikolai already did the same for his `tlog` integration and he need this function also in providers. Can you cherry-pick this commit instead so Nikolai doesn't have to solve conflicts?
https://github.com/SSSD/sssd/pull/136/commits/7465d487ef52fd1cc704e2e67c62d4...

* **CACHE_REQ: Add a new cache_req_ncache_filter_fn() plugin function**
```c
 /**
+ * Filter the result through the negative cache.
+ *
+ * This is useful for plugins which don't use name as an input
+ * takes but can be affected by filter_users and filter_groups
     ^ token
+ * options.
+ */
+typedef errno_t
+(*cache_req_ncache_filter_fn)(struct sss_nc_ctx *ncache,
+                              struct sss_domain_info *domain,
+                              char *name);

* **CACHE_REQ: Make use of cache_req_ncache_filter_fn()**
```c
static errno_t
cache_req_search_get_name_from_msg(TALLOC_CTX *mem_ctx,
                                   struct ldb_message *msg,
                                   struct sss_domain_info *domain,
                                   bool override_space,
                                   char **_name)
{
    TALLOC_CTX *tmp_ctx;
    const char *name;
    char *output_name;
    char *fqname;
    errno_t ret;

tmp_ctx = talloc_new(NULL);
    if (tmp_ctx == NULL) {
        return ENOMEM;
    }

name = sss_get_name_from_msg(domain, msg);

^^^ name can be NULL and we don't want to fail with EINVAL in this case

output_name = sss_output_name(tmp_ctx, name, domain->case_preserve,
                                  override_space);
    if (output_name == NULL) {
        DEBUG(SSSDBG_CRIT_FAILURE, "sss_output_name() failed\n");
        ret = ENOMEM;
        goto done;
    }

fqname = sss_create_internal_fqname(tmp_ctx, output_name, domain->name);
    if (fqname == NULL) {
        DEBUG(SSSDBG_CRIT_FAILURE, "sss_create_internal_fqname() failed\n");
        ret = ENOMEM;
        goto done;
    }

*_name = talloc_steal(mem_ctx, fqname);
    ret = EOK;

done:
    talloc_free(tmp_ctx);
    return ret;
}
```

Besides these small things, the approach you've taken is good. I would just like you to do more thing -- move code that deals with creating the new result into separate function(s) into `cache_req_result.c`  and enable this also for enumeration so you can remove the check (and the first patch) from `nss` responder.
"""

See the full comment at https://github.com/SSSD/sssd/pull/246#issuecomment-297319252

[sssd PR#246][comment] filter_users and filter_groups stop working properly in v 1.15

[pbrezina]

URL: https://github.com/SSSD/sssd/pull/246
Title: #246: filter_users and filter_groups stop working properly in v 1.15

pbrezina commented:
"""
* **NSS: Use fqnames when performing a ncache check**
This should be done in the ncache module, not in the callers.

* **RESPONDER: Make nss_get_name_from_msg() part of responder_utils**
Nikolai already did the same for his `tlog` integration and he need this function also in providers. Can you cherry-pick this commit instead so Nikolai doesn't have to solve conflicts?
https://github.com/SSSD/sssd/pull/136/commits/7465d487ef52fd1cc704e2e67c62d4...

* **CACHE_REQ: Add a new cache_req_ncache_filter_fn() plugin function**
```c
 /**
+ * Filter the result through the negative cache.
+ *
+ * This is useful for plugins which don't use name as an input
+ * takes but can be affected by filter_users and filter_groups
     ^ token
+ * options.
+ */
+typedef errno_t
+(*cache_req_ncache_filter_fn)(struct sss_nc_ctx *ncache,
+                              struct sss_domain_info *domain,
+                              char *name);

* **CACHE_REQ: Make use of cache_req_ncache_filter_fn()**
```c
static errno_t
cache_req_search_get_name_from_msg(TALLOC_CTX *mem_ctx,
                                   struct ldb_message *msg,
                                   struct sss_domain_info *domain,
                                   bool override_space,
                                   char **_name)
{
    TALLOC_CTX *tmp_ctx;
    const char *name;
    char *output_name;
    char *fqname;
    errno_t ret;

tmp_ctx = talloc_new(NULL);
    if (tmp_ctx == NULL) {
        return ENOMEM;
    }

name = sss_get_name_from_msg(domain, msg);

^^^ name can be NULL and we don't want to fail with EINVAL in this case

output_name = sss_output_name(tmp_ctx, name, domain->case_preserve,
                                  override_space);
    if (output_name == NULL) {
        DEBUG(SSSDBG_CRIT_FAILURE, "sss_output_name() failed\n");
        ret = ENOMEM;
        goto done;
    }

fqname = sss_create_internal_fqname(tmp_ctx, output_name, domain->name);
    if (fqname == NULL) {
        DEBUG(SSSDBG_CRIT_FAILURE, "sss_create_internal_fqname() failed\n");
        ret = ENOMEM;
        goto done;
    }

*_name = talloc_steal(mem_ctx, fqname);
    ret = EOK;

done:
    talloc_free(tmp_ctx);
    return ret;
}
```

Besides these small things, the approach you've taken is good. I would just like you to do more thing -- move code that deals with creating the new result into separate function(s) into `cache_req_result.c`  and enable this also for enumeration so you can remove the check (and the first patch) from `nss` responder.
"""

See the full comment at https://github.com/SSSD/sssd/pull/246#issuecomment-297319252

[sssd PR#246][comment] filter_users and filter_groups stop working properly in v 1.15

[fidencio]

URL: https://github.com/SSSD/sssd/pull/246
Title: #246: filter_users and filter_groups stop working properly in v 1.15

fidencio commented:
"""
On Wed, Apr 26, 2017 at 11:34 AM, Pavel Březina 
wrote:
> *NSS: Use fqnames when performing a ncache check*
>    This should be done in the ncache module, not in the callers.

Okay.
> *RESPONDER: Make nss_get_name_from_msg() part of responder_utils*
>    Nikolai already did the same for his tlog integration and he need this
>    function also in providers. Can you cherry-pick this commit instead so
>    Nikolai doesn't have to solve conflicts?
>    7465d48
>    https://github.com/SSSD/sssd/pull/136/commits/7465d487ef52fd1cc704e2e67c62d427143f0af1

Okay, but I really would prefer to use have the function names as
sss_get_name_from_msg instead of sss_nss_get_name_from_msg.
I'll sync with Nikolai about that.
> *CACHE_REQ: Add a new cache_req_ncache_filter_fn() plugin function*
>
> /**+ * Filter the result through the negative cache.+ *+ * This is useful for plugins which don't use name as an input+ * takes but can be affected by filter_users and filter_groups     ^ token+ * options.+ */
> +typedef errno_t
> +(*cache_req_ncache_filter_fn)(struct sss_nc_ctx *ncache,
>                          struct sss_domain_info *domain,
>                          char *name);
>
> **CACHE_REQ: Make use of cache_req_ncache_filter_fn()**
> static errno_t
> cache_req_search_get_name_from_msg(TALLOC_CTX *mem_ctx,
>                                    struct ldb_message *msg,
>                                    struct sss_domain_info *domain,
>                                    bool override_space,
>                                    char **_name)
> {
>     TALLOC_CTX *tmp_ctx;
>     const char *name;
>     char *output_name;
>     char *fqname;
>     errno_t ret;
>
>     tmp_ctx = talloc_new(NULL);
>     if (tmp_ctx == NULL) {
>         return ENOMEM;
>     }
>
>     name = sss_get_name_from_msg(domain, msg);
>
> ^^^ name can be NULL and we don't want to fail with EINVAL in this case

Sure!
> output_name = sss_output_name(tmp_ctx, name, domain->case_preserve,                                  override_space);    if (output_name == NULL) {        DEBUG(SSSDBG_CRIT_FAILURE, "sss_output_name() failed\n");        ret = ENOMEM;        goto done;    }    fqname = sss_create_internal_fqname(tmp_ctx, output_name, domain->name);    if (fqname == NULL) {        DEBUG(SSSDBG_CRIT_FAILURE, "sss_create_internal_fqname() failed\n");        ret = ENOMEM;        goto done;    }    *_name = talloc_steal(mem_ctx, fqname);    ret = EOK;done:    talloc_free(tmp_ctx);    return ret;}
>
> Besides these small things, the approach you've taken is good. I would
> just like you to do more thing -- move code that deals with creating the
> new result into separate function(s) into cache_req_result.c and enable
> this also for enumeration so you can remove the check (and the first patch)
> from nss responder.

Okay, super!

Thanks for the review!
> —
> You are receiving this because you authored the thread.
> Reply to this email directly, view it on GitHub
> https://github.com/SSSD/sssd/pull/246#issuecomment-297319252, or mute
> the thread
> https://github.com/notifications/unsubscribe-auth/AAG4eoKAi8oQnPiyuUdS6y_WWXWU8bihks5rzw-vgaJpZM4NGkQK
> .

"""

See the full comment at https://github.com/SSSD/sssd/pull/246#issuecomment-297336143

[sssd PR#246][+Changes requested] filter_users and filter_groups stop working properly in v 1.15

[fidencio]

URL: https://github.com/SSSD/sssd/pull/246
Title: #246: filter_users and filter_groups stop working properly in v 1.15

Label: +Changes requested

[sssd PR#246][comment] filter_users and filter_groups stop working properly in v 1.15

[fidencio]

URL: https://github.com/SSSD/sssd/pull/246
Title: #246: filter_users and filter_groups stop working properly in v 1.15

fidencio commented:
"""
On Wed, Apr 26, 2017 at 11:34 AM, Pavel Březina 
wrote:
> *NSS: Use fqnames when performing a ncache check*
>    This should be done in the ncache module, not in the callers.

Well, I'm not exactly sure about this.
Currently we the situation we have in the code is that we call ncache check
using both fully qualified names and non fully qualified names ... so
patching it in ncache module itself may end up causing more issues.

Any input here is appreciated :-)
> *RESPONDER: Make nss_get_name_from_msg() part of responder_utils*
>    Nikolai already did the same for his tlog integration and he need this
>    function also in providers. Can you cherry-pick this commit instead so
>    Nikolai doesn't have to solve conflicts?
>    7465d48
>    https://github.com/SSSD/sssd/pull/136/commits/7465d487ef52fd1cc704e2e67c62d427143f0af1
>
> *CACHE_REQ: Add a new cache_req_ncache_filter_fn() plugin function*
>
> /**+ * Filter the result through the negative cache.+ *+ * This is useful for plugins which don't use name as an input+ * takes but can be affected by filter_users and filter_groups     ^ token+ * options.+ */
> +typedef errno_t
> +(*cache_req_ncache_filter_fn)(struct sss_nc_ctx *ncache,
>                          struct sss_domain_info *domain,
>                          char *name);
>
> **CACHE_REQ: Make use of cache_req_ncache_filter_fn()**
> static errno_t
> cache_req_search_get_name_from_msg(TALLOC_CTX *mem_ctx,
>                                    struct ldb_message *msg,
>                                    struct sss_domain_info *domain,
>                                    bool override_space,
>                                    char **_name)
> {
>     TALLOC_CTX *tmp_ctx;
>     const char *name;
>     char *output_name;
>     char *fqname;
>     errno_t ret;
>
>     tmp_ctx = talloc_new(NULL);
>     if (tmp_ctx == NULL) {
>         return ENOMEM;
>     }
>
>     name = sss_get_name_from_msg(domain, msg);
>
> ^^^ name can be NULL and we don't want to fail with EINVAL in this case    output_name = sss_output_name(tmp_ctx, name, domain->case_preserve,                                  override_space);    if (output_name == NULL) {        DEBUG(SSSDBG_CRIT_FAILURE, "sss_output_name() failed\n");        ret = ENOMEM;        goto done;    }    fqname = sss_create_internal_fqname(tmp_ctx, output_name, domain->name);    if (fqname == NULL) {        DEBUG(SSSDBG_CRIT_FAILURE, "sss_create_internal_fqname() failed\n");        ret = ENOMEM;        goto done;    }    *_name = talloc_steal(mem_ctx, fqname);    ret = EOK;done:    talloc_free(tmp_ctx);    return ret;}
>
> Besides these small things, the approach you've taken is good. I would
> just like you to do more thing -- move code that deals with creating the
> new result into separate function(s) into cache_req_result.c and enable
> this also for enumeration so you can remove the check (and the first patch)
> from nss responder.
>
> —
> You are receiving this because you authored the thread.
> Reply to this email directly, view it on GitHub
> <https://github.com/SSSD/sssd/pull/246#issuecomment-297319252>, or mute
> the thread
> <https://github.com/notifications/unsubscribe-auth/AAG4eoKAi8oQnPiyuUdS6y_WWXWU8bihks5rzw-vgaJpZM4NGkQK>
> .

"""

See the full comment at https://github.com/SSSD/sssd/pull/246#issuecomment-297468578

[sssd PR#246][comment] filter_users and filter_groups stop working properly in v 1.15

[fidencio]

URL: https://github.com/SSSD/sssd/pull/246
Title: #246: filter_users and filter_groups stop working properly in v 1.15

fidencio commented:
"""
On Wed, Apr 26, 2017 at 6:34 PM, Fabiano Fidêncio 
wrote:
> On Wed, Apr 26, 2017 at 11:34 AM, Pavel Březina 
> wrote:
> > *NSS: Use fqnames when performing a ncache check*
> >    This should be done in the ncache module, not in the callers.
>
> Well, I'm not exactly sure about this.
> Currently we the situation we have in the code is that we call ncache
> check using both fully qualified names and non fully qualified names ... so
> patching it in ncache module itself may end up causing more issues.
>
> Any input here is appreciated :-)

Nevermind, got it :-)
> > *RESPONDER: Make nss_get_name_from_msg() part of responder_utils*
> >    Nikolai already did the same for his tlog integration and he need
> >    this function also in providers. Can you cherry-pick this commit instead so
> >    Nikolai doesn't have to solve conflicts?
> >    7465d48
> >    https://github.com/SSSD/sssd/pull/136/commits/7465d487ef52fd1cc704e2e67c62d427143f0af1
> >
> > *CACHE_REQ: Add a new cache_req_ncache_filter_fn() plugin function*
> >
> > /**+ * Filter the result through the negative cache.+ *+ * This is useful for plugins which don't use name as an input+ * takes but can be affected by filter_users and filter_groups     ^ token+ * options.+ */
> > +typedef errno_t
> > +(*cache_req_ncache_filter_fn)(struct sss_nc_ctx *ncache,
> >                          struct sss_domain_info *domain,
> >                          char *name);
> >
> > **CACHE_REQ: Make use of cache_req_ncache_filter_fn()**
> > static errno_t
> > cache_req_search_get_name_from_msg(TALLOC_CTX *mem_ctx,
> >                                    struct ldb_message *msg,
> >                                    struct sss_domain_info *domain,
> >                                    bool override_space,
> >                                    char **_name)
> > {
> >     TALLOC_CTX *tmp_ctx;
> >     const char *name;
> >     char *output_name;
> >     char *fqname;
> >     errno_t ret;
> >
> >     tmp_ctx = talloc_new(NULL);
> >     if (tmp_ctx == NULL) {
> >         return ENOMEM;
> >     }
> >
> >     name = sss_get_name_from_msg(domain, msg);
> >
> > ^^^ name can be NULL and we don't want to fail with EINVAL in this case

@pbrezina ...

What do we want to do in this case? It would fail with EINVAL later on when
filling the grent/pwent anyways, no?
> output_name = sss_output_name(tmp_ctx, name, domain->case_preserve,                                  override_space);    if (output_name == NULL) {        DEBUG(SSSDBG_CRIT_FAILURE, "sss_output_name() failed\n");        ret = ENOMEM;        goto done;    }    fqname = sss_create_internal_fqname(tmp_ctx, output_name, domain->name);    if (fqname == NULL) {        DEBUG(SSSDBG_CRIT_FAILURE, "sss_create_internal_fqname() failed\n");        ret = ENOMEM;        goto done;    }    *_name = talloc_steal(mem_ctx, fqname);    ret = EOK;done:    talloc_free(tmp_ctx);    return ret;}
> > Besides these small things, the approach you've taken is good. I would
> > just like you to do more thing -- move code that deals with creating the
> > new result into separate function(s) into cache_req_result.c and enable
> > this also for enumeration so you can remove the check (and the first patch)
> > from nss responder.

And last comment ... the introduced functions are not dealing with
cache_req_result at all. What they do is changing the ldb_result and moving
this part of the code to the cache_req_result.c does seem a little bit
weird to me.

—
> > You are receiving this because you authored the thread.
> > Reply to this email directly, view it on GitHub
> > https://github.com/SSSD/sssd/pull/246#issuecomment-297319252, or mute
> > the thread
> > https://github.com/notifications/unsubscribe-auth/AAG4eoKAi8oQnPiyuUdS6y_WWXWU8bihks5rzw-vgaJpZM4NGkQK
> > .

Best Regards,

"""

See the full comment at https://github.com/SSSD/sssd/pull/246#issuecomment-297515332

[sssd PR#246][comment] filter_users and filter_groups stop working properly in v 1.15

[pbrezina]

URL: https://github.com/SSSD/sssd/pull/246
Title: #246: filter_users and filter_groups stop working properly in v 1.15

pbrezina commented:
"""
> What do we want to do in this case? It would fail with EINVAL later on when filling the grent/pwent anyways, no?

That comment should said `ENOMEM`. If `name` is `NULL`, `sss_output_name` will return `NULL` and we fail with `ENOMEM`.
> And last comment ... the introduced functions are not dealing with cache_req_result at all. What they do is changing the ldb_result and moving this part of the code to the cache_req_result.c does seem a little bit weird to me.

But it still deals in some sort of result that is used but cache_req` so I think it is ok. It deserves to be placed in separate function at least, doesn't matter if static or in `cache_req_result.c`.

"""

See the full comment at https://github.com/SSSD/sssd/pull/246#issuecomment-297654116

[sssd PR#246][comment] filter_users and filter_groups stop working properly in v 1.15

[fidencio]

URL: https://github.com/SSSD/sssd/pull/246
Title: #246: filter_users and filter_groups stop working properly in v 1.15

fidencio commented:
"""
On Thu, Apr 27, 2017 at 10:53 AM, Pavel Březina 
wrote:
> What do we want to do in this case? It would fail with EINVAL later on
> when filling the grent/pwent anyways, no?
>
> That comment should said ENOMEM. If name is NULL, sss_output_name will
> return NULL and we fail with ENOMEM.

But there's no memory involved at all on sss_get_name_from_msg().
I'll follow you suggestion and update the PR anyways.
> And last comment ... the introduced functions are not dealing with
> cache_req_result at all. What they do is changing the ldb_result and moving
> this part of the code to the cache_req_result.c does seem a little bit
> weird to me.
>
> But it still deals in some sort of result that is used but cache_reqso I
> think it is ok. It deserves to be placed in separate function at least,
> doesn't matter if static or incache_req_result.c`.

Okay, I'm going for a new static function cache_req_search() then.
> —
> You are receiving this because you authored the thread.
> Reply to this email directly, view it on GitHub
> https://github.com/SSSD/sssd/pull/246#issuecomment-297654116, or mute
> the thread
> https://github.com/notifications/unsubscribe-auth/AAG4emnmFcpvUfLeSxW9MXXs76I3ZC1Zks5r0FeFgaJpZM4NGkQK
> .

Thanks for the review!

"""

See the full comment at https://github.com/SSSD/sssd/pull/246#issuecomment-297655912

[sssd PR#246][comment] filter_users and filter_groups stop working properly in v 1.15

[pbrezina]

URL: https://github.com/SSSD/sssd/pull/246
Title: #246: filter_users and filter_groups stop working properly in v 1.15

pbrezina commented:
"""
> But there's no memory involved at all on sss_get_name_from_msg(). I'll follow you suggestion and update the PR anyways.

Yes, that means if `SYSDB_NAME` attribute exists, it is returned. What if the attribute does not exist? It should, but we should handle case were an error happened.
"""

See the full comment at https://github.com/SSSD/sssd/pull/246#issuecomment-297663449

[sssd PR#246][comment] filter_users and filter_groups stop working properly in v 1.15

[pbrezina]

URL: https://github.com/SSSD/sssd/pull/246
Title: #246: filter_users and filter_groups stop working properly in v 1.15

pbrezina commented:
"""
> But there's no memory involved at all on sss_get_name_from_msg(). I'll follow you suggestion and update the PR anyways.

Yes, that means if `SYSDB_NAME` attribute exists, it is returned. What if the attribute does not exist? It should, but we should handle case were an error happened.
"""

See the full comment at https://github.com/SSSD/sssd/pull/246#issuecomment-297663449

[sssd PR#246][synchronized] filter_users and filter_groups stop working properly in v 1.15

[fidencio]

URL: https://github.com/SSSD/sssd/pull/246
Author: fidencio
 Title: #246: filter_users and filter_groups stop working properly in v 1.15
Action: synchronized

To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/246/head:pr246
git checkout pr246

[sssd PR#246][-Changes requested] filter_users and filter_groups stop working properly in v 1.15

[fidencio]

URL: https://github.com/SSSD/sssd/pull/246
Title: #246: filter_users and filter_groups stop working properly in v 1.15

Label: -Changes requested

[sssd PR#246][comment] filter_users and filter_groups stop working properly in v 1.15

[fidencio]

URL: https://github.com/SSSD/sssd/pull/246
Title: #246: filter_users and filter_groups stop working properly in v 1.15

fidencio commented:
"""
patchset updated.
"""

See the full comment at https://github.com/SSSD/sssd/pull/246#issuecomment-297677084

[sssd PR#246][comment] filter_users and filter_groups stop working properly in v 1.15

[fidencio]

URL: https://github.com/SSSD/sssd/pull/246
Title: #246: filter_users and filter_groups stop working properly in v 1.15

fidencio commented:
"""
On Thu, Apr 27, 2017 at 1:26 PM, Pavel Březina 
wrote:
> *@pbrezina* requested changes on this pull request.
>
> In src/responder/common/cache_req/cache_req_search.c
> https://github.com/SSSD/sssd/pull/246#discussion_r113672158:
> > if (cr->plugin->ncache_filter_fn == NULL) {
>    CACHE_REQ_DEBUG(SSSDBG_TRACE_FUNC, cr,
>                    "This request type does not support filtering negative cache\n");
>    *_result = result;
>    return EOK;
> }
> tmp_ctx = talloc_new(NULL);
> if (tmp_ctx == NULL) {
>    return ENOMEM;
> }
> CACHE_REQ_DEBUG(SSSDBG_TRACE_FUNC, cr,
>                "Filtering out results from negative cache\n");
> filtered_result = talloc_zero(mem_ctx, struct ldb_result);
> Unused at this moment.
>
> In src/responder/common/cache_req/cache_req_search.c
> https://github.com/SSSD/sssd/pull/246#discussion_r113672504:
> >
> tmp_ctx = talloc_new(NULL);
> if (tmp_ctx == NULL) {
>    return ENOMEM;
> }
> CACHE_REQ_DEBUG(SSSDBG_TRACE_FUNC, cr,
>                "Filtering out results from negative cache\n");
> filtered_result = talloc_zero(mem_ctx, struct ldb_result);
> if (filtered_result == NULL) {
>    ret = ENOMEM;
>    goto done;
> }
> msgs = talloc_zero(tmp_ctx, struct ldb_message *);
>
> Because this function will filter out only users and groups from the
> configuration file, there won't be many objects filtered out. But there
> will be many allocations with enumeration. It would be better to allocate
> enough space to hold all the messages at once to safe some allocations. I.e.
>
> msgs = talloc_zero_array(tmp_ctx, struct ldb_message *, result->count)
> And remove the reallocs.
>
> In src/responder/common/cache_req/cache_req_search.c
> https://github.com/SSSD/sssd/pull/246#discussion_r113672787:
> > TALLOC_CTX *tmp_ctx;
> const char *name;
> char *cased_name;
> errno_t ret;
> tmp_ctx = talloc_new(NULL);
> if (tmp_ctx == NULL) {
>    return ENOMEM;
> }
> name = sss_get_name_from_msg(domain, msg);
> if (name == NULL) {
>    DEBUG(SSSDBG_CRIT_FAILURE,
>          "sss_get_name_from_msg() returned NULL, which should never "
>          "happen in this scenario!\n");
>    ret = ENOMEM;
>
> ERR_INTERNAL since it is corrupted cache.
>
> In src/responder/common/cache_req/cache_req_search.c
> https://github.com/SSSD/sssd/pull/246#discussion_r113672851:
> >
> tmp_ctx = talloc_new(NULL);
> if (tmp_ctx == NULL) {
>    return ENOMEM;
> }
> name = sss_get_name_from_msg(domain, msg);
> if (name == NULL) {
>    DEBUG(SSSDBG_CRIT_FAILURE,
>          "sss_get_name_from_msg() returned NULL, which should never "
>          "happen in this scenario!\n");
>    ret = ENOMEM;
>    goto done;
> }
> cased_name = sss_get_cased_name(tmp_ctx, name, domain->case_preserve);
>
> You used sss_output_name in previous patch set. Why did you change it?

I realized that getting sss_output_name() doesn't make much sense here.
We're just interested in the fully-qualified name (which we already have
here).

So, the whole part of sss_output_name() + sss_create_internal_fqname() is
unnecessary here.
Do you agree?
> —
> You are receiving this because you authored the thread.
> Reply to this email directly, view it on GitHub
> https://github.com/SSSD/sssd/pull/246#pullrequestreview-35079762, or mute
> the thread
> https://github.com/notifications/unsubscribe-auth/AAG4eujB3Aq3lA4kxmVS4g54yQXC4s8jks5r0HtegaJpZM4NGkQK
> .

"""

See the full comment at https://github.com/SSSD/sssd/pull/246#issuecomment-297687915

[sssd PR#246][synchronized] filter_users and filter_groups stop working properly in v 1.15

[fidencio]

URL: https://github.com/SSSD/sssd/pull/246
Author: fidencio
 Title: #246: filter_users and filter_groups stop working properly in v 1.15
Action: synchronized

To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/246/head:pr246
git checkout pr246

[sssd PR#246][synchronized] filter_users and filter_groups stop working properly in v 1.15

[fidencio]

URL: https://github.com/SSSD/sssd/pull/246
Author: fidencio
 Title: #246: filter_users and filter_groups stop working properly in v 1.15
Action: synchronized

To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/246/head:pr246
git checkout pr246

[sssd PR#246][comment] filter_users and filter_groups stop working properly in v 1.15

[fidencio]

URL: https://github.com/SSSD/sssd/pull/246
Title: #246: filter_users and filter_groups stop working properly in v 1.15

fidencio commented:
"""
On Thu, Apr 27, 2017 at 1:29 PM, Fabiano Fidêncio 
wrote:
> On Thu, Apr 27, 2017 at 1:26 PM, Pavel Březina 
> wrote:
> > *@pbrezina* requested changes on this pull request.
> >
> > In src/responder/common/cache_req/cache_req_search.c
> > https://github.com/SSSD/sssd/pull/246#discussion_r113672158:
> > > if (cr->plugin->ncache_filter_fn == NULL) {
> >    CACHE_REQ_DEBUG(SSSDBG_TRACE_FUNC, cr,
> >                    "This request type does not support filtering negative cache\n");
> >    *_result = result;
> >    return EOK;
> > }
> > tmp_ctx = talloc_new(NULL);
> > if (tmp_ctx == NULL) {
> >    return ENOMEM;
> > }
> > CACHE_REQ_DEBUG(SSSDBG_TRACE_FUNC, cr,
> >                "Filtering out results from negative cache\n");
> > filtered_result = talloc_zero(mem_ctx, struct ldb_result);
> > Unused at this moment.
> >
> > In src/responder/common/cache_req/cache_req_search.c
> > https://github.com/SSSD/sssd/pull/246#discussion_r113672504:
> > >
> > tmp_ctx = talloc_new(NULL);
> > if (tmp_ctx == NULL) {
> >    return ENOMEM;
> > }
> > CACHE_REQ_DEBUG(SSSDBG_TRACE_FUNC, cr,
> >                "Filtering out results from negative cache\n");
> > filtered_result = talloc_zero(mem_ctx, struct ldb_result);
> > if (filtered_result == NULL) {
> >    ret = ENOMEM;
> >    goto done;
> > }
> > msgs = talloc_zero(tmp_ctx, struct ldb_message *);
> >
> > Because this function will filter out only users and groups from the
> > configuration file, there won't be many objects filtered out. But there
> > will be many allocations with enumeration. It would be better to allocate
> > enough space to hold all the messages at once to safe some allocations. I.e.
> >
> > msgs = talloc_zero_array(tmp_ctx, struct ldb_message *, result->count)
> > And remove the reallocs.
> >
> > In src/responder/common/cache_req/cache_req_search.c
> > https://github.com/SSSD/sssd/pull/246#discussion_r113672787:
> > > TALLOC_CTX *tmp_ctx;
> > const char *name;
> > char *cased_name;
> > errno_t ret;
> > tmp_ctx = talloc_new(NULL);
> > if (tmp_ctx == NULL) {
> >    return ENOMEM;
> > }
> > name = sss_get_name_from_msg(domain, msg);
> > if (name == NULL) {
> >    DEBUG(SSSDBG_CRIT_FAILURE,
> >          "sss_get_name_from_msg() returned NULL, which should never "
> >          "happen in this scenario!\n");
> >    ret = ENOMEM;
> >
> > ERR_INTERNAL since it is corrupted cache.
> >
> > In src/responder/common/cache_req/cache_req_search.c
> > https://github.com/SSSD/sssd/pull/246#discussion_r113672851:
> > >
> > tmp_ctx = talloc_new(NULL);
> > if (tmp_ctx == NULL) {
> >    return ENOMEM;
> > }
> > name = sss_get_name_from_msg(domain, msg);
> > if (name == NULL) {
> >    DEBUG(SSSDBG_CRIT_FAILURE,
> >          "sss_get_name_from_msg() returned NULL, which should never "
> >          "happen in this scenario!\n");
> >    ret = ENOMEM;
> >    goto done;
> > }
> > cased_name = sss_get_cased_name(tmp_ctx, name, domain->case_preserve);
> >
> > You used sss_output_name in previous patch set. Why did you change it?
>
> I realized that getting sss_output_name() doesn't make much sense here.
> We're just interested in the fully-qualified name (which we already have
> here).
>
> So, the whole part of sss_output_name() + sss_create_internal_fqname() is
> unnecessary here.
> Do you agree?

I have updated the PR with the other issues you have noticed.

One thing that is not exactly clear to me (and I may need your guidance) is
... just getting the name from the DB is not enough to the the
fully-qualified name that we added to the negative cache? Do I really have
to get the cased name and replace the space?
> > —
> > You are receiving this because you authored the thread.
> > Reply to this email directly, view it on GitHub
> > https://github.com/SSSD/sssd/pull/246#pullrequestreview-35079762, or mute
> > the thread
> > https://github.com/notifications/unsubscribe-auth/AAG4eujB3Aq3lA4kxmVS4g54yQXC4s8jks5r0HtegaJpZM4NGkQK
> > .

"""

See the full comment at https://github.com/SSSD/sssd/pull/246#issuecomment-297689611

[sssd PR#246][comment] filter_users and filter_groups stop working properly in v 1.15

[pbrezina]

URL: https://github.com/SSSD/sssd/pull/246
Title: #246: filter_users and filter_groups stop working properly in v 1.15

pbrezina commented:
"""
> One thing that is not exactly clear to me (and I may need your guidance) is ... just getting the name from the DB is not enough to the the fully-qualified name that we added to the negative cache? Do I really have to get the cased name and replace the space?

I'm not sure from the top of my head. Please, see what's stored in the negative cache, when you use overriden names, names with replaced space and qualified names in filter_users and filter_groups.

From a quick look into the code, I can see that we parse qualified names into shortname and domain. We also store the entries in internal fqname format. We don't use reverse_replace_space here. An overriden name is stored into ncache. I think you are fine with this code. But please test it. See: `sss_ncache_prepopulate`.
"""

See the full comment at https://github.com/SSSD/sssd/pull/246#issuecomment-297692941

[sssd PR#246][comment] filter_users and filter_groups stop working properly in v 1.15

[fidencio]

URL: https://github.com/SSSD/sssd/pull/246
Title: #246: filter_users and filter_groups stop working properly in v 1.15

fidencio commented:
"""
On Thu, Apr 27, 2017 at 1:57 PM, Pavel Březina 
wrote:
> One thing that is not exactly clear to me (and I may need your guidance)
> is ... just getting the name from the DB is not enough to the the
> fully-qualified name that we added to the negative cache? Do I really have
> to get the cased name and replace the space?
>
> I'm not sure from the top of my head. Please, see what's stored in the
> negative cache, when you use overriden names, names with replaced space and
> qualified names in filter_users and filter_groups.

I didn't test it manually but we do have tests covering those cases (and
yes, they keep passing with these changes).

I'll also do some manual tests just to be more than sure.
> From a quick look into the code, I can see that we parse qualified names
> into shortname and domain. We also store the entries in internal fqname
> format. We don't use reverse_replace_space here. An overriden name is
> stored into ncache. I think you are fine with this code. But please test
> it. See: sss_ncache_prepopulate.

Thanks!
> —
> You are receiving this because you authored the thread.
> Reply to this email directly, view it on GitHub
> https://github.com/SSSD/sssd/pull/246#issuecomment-297692941, or mute
> the thread
> https://github.com/notifications/unsubscribe-auth/AAG4esz8oi3xheMLKfQTr2YqYF6nMQpaks5r0IKOgaJpZM4NGkQK
> .

"""

See the full comment at https://github.com/SSSD/sssd/pull/246#issuecomment-297694425

[sssd PR#246][comment] filter_users and filter_groups stop working properly in v 1.15

[jhrozek]

URL: https://github.com/SSSD/sssd/pull/246
Title: #246: filter_users and filter_groups stop working properly in v 1.15

jhrozek commented:
"""
On Thu, Apr 27, 2017 at 04:57:02AM -0700, Pavel Březina wrote:
> > One thing that is not exactly clear to me (and I may need your guidance) is ... just getting the name from the DB is not enough to the the fully-qualified name that we added to the negative cache? Do I really have to get the cased name and replace the space?
>
> I'm not sure from the top of my head. Please, see what's stored in the negative cache, when you use overriden names, names with replaced space and qualified names in filter_users and filter_groups.
>
> From a quick look into the code, I can see that we parse qualified names into shortname and domain. We also store the entries in internal fqname format. We don't use reverse_replace_space here. An overriden name is stored into ncache. I think you are fine with this code. But please test it. See: `sss_ncache_prepopulate`.

(I didn't look into the code at all. That said..)

I don't actually see a reason to use anything else than the internal
qualified name for negative cache. Definitely not the output name..the
only reason to use the input name might be to check the negative cache
very fast, before the name is parsed, but that seems like a wrong
optimization..

"""

See the full comment at https://github.com/SSSD/sssd/pull/246#issuecomment-297751927

[sssd PR#246][synchronized] filter_users and filter_groups stop working properly in v 1.15

[fidencio]

URL: https://github.com/SSSD/sssd/pull/246
Author: fidencio
 Title: #246: filter_users and filter_groups stop working properly in v 1.15
Action: synchronized

To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/246/head:pr246
git checkout pr246

[sssd PR#246][comment] filter_users and filter_groups stop working properly in v 1.15

[fidencio]

URL: https://github.com/SSSD/sssd/pull/246
Title: #246: filter_users and filter_groups stop working properly in v 1.15

fidencio commented:
"""
Okay, patchset update and using directly the name we get from the message (which is the internal fqname).

It does work for the bug report and it doesn't break any tests that we already have.

IMO, if @pbrezina agrees, the patches are good to go.
"""

See the full comment at https://github.com/SSSD/sssd/pull/246#issuecomment-297915894

[sssd PR#246][synchronized] filter_users and filter_groups stop working properly in v 1.15

[fidencio]

URL: https://github.com/SSSD/sssd/pull/246
Author: fidencio
 Title: #246: filter_users and filter_groups stop working properly in v 1.15
Action: synchronized

To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/246/head:pr246
git checkout pr246

[sssd PR#246][comment] filter_users and filter_groups stop working properly in v 1.15

[fidencio]

URL: https://github.com/SSSD/sssd/pull/246
Title: #246: filter_users and filter_groups stop working properly in v 1.15

fidencio commented:
"""
And I started a CI build ...
"""

See the full comment at https://github.com/SSSD/sssd/pull/246#issuecomment-297916257

[sssd PR#246][comment] filter_users and filter_groups stop working properly in v 1.15

[pbrezina]

URL: https://github.com/SSSD/sssd/pull/246
Title: #246: filter_users and filter_groups stop working properly in v 1.15

pbrezina commented:
"""
* **CACHE_REQ: Add a new cache_req_ncache_filter_fn() plugin function**
```c
 /**
+ * Filter the result through the negative cache.
+ *
+ * This is useful for plugins which don't use name as an input
+ * takes but can be affected by filter_users and filter_groups
    ^ token
+ * options.
+ */
+typedef errno_t
+(*cache_req_ncache_filter_fn)(struct sss_nc_ctx *ncache,
+                              struct sss_domain_info *domain,
+                              const char *name);
+
+/**
```
* **CACHE_REQ: Make use of cache_req_ncache_filter_fn()**
Since the original result may be theoretically quite big, we should free it in `cache_req_search_done` after we obtain the filtered result and use `talloc_steal` in `cache_req_search_ncache_filter` to steal the messages on new context. i.e.

```c
       msgs[msg_count] = talloc_steal(msgs, result->msgs[i]);
       msg_count++
```

And also use `tmp_ctx` for filtered result and then steal it (for clarity):

```c
+    filtered_result = cache_req_create_ldb_result_from_msg_list(mem_ctx, msgs,
+                                                                msg_count);

^ tmp_ctx here

+    if (filtered_result == NULL) {
+        ret = ENOMEM;
+        goto done;
+    }
+
+    *_result = filtered_result;

^ *_result = talloc_steal(mem_ctx, filtered_result)
```

Besides those nitpicks, the patches are good to go.
"""

See the full comment at https://github.com/SSSD/sssd/pull/246#issuecomment-298595083

[sssd PR#246][synchronized] filter_users and filter_groups stop working properly in v 1.15

[fidencio]

URL: https://github.com/SSSD/sssd/pull/246
Author: fidencio
 Title: #246: filter_users and filter_groups stop working properly in v 1.15
Action: synchronized

To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/246/head:pr246
git checkout pr246

[sssd PR#246][comment] filter_users and filter_groups stop working properly in v 1.15

[fidencio]

URL: https://github.com/SSSD/sssd/pull/246
Title: #246: filter_users and filter_groups stop working properly in v 1.15

fidencio commented:
"""
@pbrezina: patch set updated.

I'm firing our internal CI with the latest changes.
"""

See the full comment at https://github.com/SSSD/sssd/pull/246#issuecomment-298620971

[sssd PR#246][comment] filter_users and filter_groups stop working properly in v 1.15

[pbrezina]

URL: https://github.com/SSSD/sssd/pull/246
Title: #246: filter_users and filter_groups stop working properly in v 1.15

pbrezina commented:
"""
```c
static errno_t cache_req_search_ncache_filter(TALLOC_CTX *mem_ctx,
                                              struct cache_req *cr,
                                              struct ldb_result *result,
                                              struct ldb_result **_result)
{
    ...

if (cr->plugin->ncache_filter_fn == NULL) {
        CACHE_REQ_DEBUG(SSSDBG_TRACE_FUNC, cr,
                        "This request type does not support filtering negative cache\n");
        ^^^ This request type does not support filtering result by negative cache

filtered_result = cache_req_create_ldb_result_from_msg_list(tmp_ctx,
                                                                    result->msgs,
                                                                    result->count);
        if (filtered_result == NULL) {
            ret = ENOMEM;
            goto done;
        }

goto immediately;

^^^ shorter version:
        *_result = talloc_steal(mem_ctx, result);

ret = EOK;
        goto done;

}

CACHE_REQ_DEBUG(SSSDBG_TRACE_FUNC, cr,
                    "Filtering out results from negative cache\n");
    ^^^ Filtering out results by negative cache

msgs = talloc_zero_array(tmp_ctx, struct ldb_message *, result->count);
    msg_count = 0;

for (size_t i = 0; i < result->count; i++) {
        name = sss_get_name_from_msg(cr->domain, result->msgs[i]);
        if (name == NULL) {
            CACHE_REQ_DEBUG(SSSDBG_CRIT_FAILURE, cr,
                  "sss_get_name_from_msg() returned NULL, which should never "
                  "happen in this scenario!\n");
            ret = ERR_INTERNAL;
            goto done;
        }

ret = cr->plugin->ncache_filter_fn(cr->ncache, cr->domain, name);
        if (ret == EEXIST) {
            CACHE_REQ_DEBUG(SSSDBG_TRACE_FUNC, cr,
                            "[%s] filtered out! (negative cache)\n",
                            name);
            continue;
        } else if (ret != EOK && ret != ENOENT) {
            CACHE_REQ_DEBUG(SSSDBG_CRIT_FAILURE, cr,
                            "Unable to check negative cache [%d]: %s\n",
                            ret, sss_strerror(ret));
            goto done;
        }

CACHE_REQ_DEBUG(SSSDBG_TRACE_FUNC, cr,
                        "[%s] is not present in negative cache\n",
                        name);

^^^ Please, remove this debug message.
            We will know which users are removed. We know which
            remain. This would be insanely long for enumeration.

msgs[msg_count] = talloc_steal(msgs, result->msgs[i]);
        msg_count++;
    }

if (msg_count == 0) {
        ret = ENOENT;
        goto done;
    }

filtered_result = cache_req_create_ldb_result_from_msg_list(tmp_ctx, msgs,
                                                                msg_count);
    if (filtered_result == NULL) {
        ret = ENOMEM;
        goto done;
    }

---   immediately:
    *_result = talloc_steal(mem_ctx, filtered_result);
    ret = EOK;

done:
    talloc_free(tmp_ctx);
    return ret;
}
```

"""

See the full comment at https://github.com/SSSD/sssd/pull/246#issuecomment-299438086

[sssd PR#246][comment] filter_users and filter_groups stop working properly in v 1.15

[pbrezina]

URL: https://github.com/SSSD/sssd/pull/246
Title: #246: filter_users and filter_groups stop working properly in v 1.15

pbrezina commented:
"""
```c
static errno_t cache_req_search_ncache_filter(TALLOC_CTX *mem_ctx,
                                              struct cache_req *cr,
                                              struct ldb_result *result,
                                              struct ldb_result **_result)
{
    ...

if (cr->plugin->ncache_filter_fn == NULL) {
        CACHE_REQ_DEBUG(SSSDBG_TRACE_FUNC, cr,
                        "This request type does not support filtering negative cache\n");
        ^^^ This request type does not support filtering result by negative cache

filtered_result = cache_req_create_ldb_result_from_msg_list(tmp_ctx,
                                                                    result->msgs,
                                                                    result->count);
        if (filtered_result == NULL) {
            ret = ENOMEM;
            goto done;
        }

goto immediately;

^^^ shorter version:
        *_result = talloc_steal(mem_ctx, result);

ret = EOK;
        goto done;

}

CACHE_REQ_DEBUG(SSSDBG_TRACE_FUNC, cr,
                    "Filtering out results from negative cache\n");
    ^^^ Filtering out results by negative cache

msgs = talloc_zero_array(tmp_ctx, struct ldb_message *, result->count);
    msg_count = 0;

for (size_t i = 0; i < result->count; i++) {
        name = sss_get_name_from_msg(cr->domain, result->msgs[i]);
        if (name == NULL) {
            CACHE_REQ_DEBUG(SSSDBG_CRIT_FAILURE, cr,
                  "sss_get_name_from_msg() returned NULL, which should never "
                  "happen in this scenario!\n");
            ret = ERR_INTERNAL;
            goto done;
        }

ret = cr->plugin->ncache_filter_fn(cr->ncache, cr->domain, name);
        if (ret == EEXIST) {
            CACHE_REQ_DEBUG(SSSDBG_TRACE_FUNC, cr,
                            "[%s] filtered out! (negative cache)\n",
                            name);
            continue;
        } else if (ret != EOK && ret != ENOENT) {
            CACHE_REQ_DEBUG(SSSDBG_CRIT_FAILURE, cr,
                            "Unable to check negative cache [%d]: %s\n",
                            ret, sss_strerror(ret));
            goto done;
        }

CACHE_REQ_DEBUG(SSSDBG_TRACE_FUNC, cr,
                        "[%s] is not present in negative cache\n",
                        name);

^^^ Please, remove this debug message.
            We will know which users are removed. We know which
            remain. This would be insanely long for enumeration.

msgs[msg_count] = talloc_steal(msgs, result->msgs[i]);
        msg_count++;
    }

if (msg_count == 0) {
        ret = ENOENT;
        goto done;
    }

filtered_result = cache_req_create_ldb_result_from_msg_list(tmp_ctx, msgs,
                                                                msg_count);
    if (filtered_result == NULL) {
        ret = ENOMEM;
        goto done;
    }

---   immediately:
    *_result = talloc_steal(mem_ctx, filtered_result);
    ret = EOK;

done:
    talloc_free(tmp_ctx);
    return ret;
}
```

"""

See the full comment at https://github.com/SSSD/sssd/pull/246#issuecomment-299438086

[sssd PR#246][+Changes requested] filter_users and filter_groups stop working properly in v 1.15

[lslebodn]

URL: https://github.com/SSSD/sssd/pull/246
Title: #246: filter_users and filter_groups stop working properly in v 1.15

Label: +Changes requested

[sssd PR#246][synchronized] filter_users and filter_groups stop working properly in v 1.15

[fidencio]

URL: https://github.com/SSSD/sssd/pull/246
Author: fidencio
 Title: #246: filter_users and filter_groups stop working properly in v 1.15
Action: synchronized

To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/246/head:pr246
git checkout pr246

[sssd PR#246][-Changes requested] filter_users and filter_groups stop working properly in v 1.15

[fidencio]

URL: https://github.com/SSSD/sssd/pull/246
Title: #246: filter_users and filter_groups stop working properly in v 1.15

Label: -Changes requested

[sssd PR#246][comment] filter_users and filter_groups stop working properly in v 1.15

[fidencio]

URL: https://github.com/SSSD/sssd/pull/246
Title: #246: filter_users and filter_groups stop working properly in v 1.15

fidencio commented:
"""
Patchset updated!
"""

See the full comment at https://github.com/SSSD/sssd/pull/246#issuecomment-299454749

[sssd PR#246][comment] filter_users and filter_groups stop working properly in v 1.15

[pbrezina]

URL: https://github.com/SSSD/sssd/pull/246
Title: #246: filter_users and filter_groups stop working properly in v 1.15

pbrezina commented:
"""
Ack. Thank you for your patience. 
"""

See the full comment at https://github.com/SSSD/sssd/pull/246#issuecomment-300441160

[sssd PR#246][+Accepted] filter_users and filter_groups stop working properly in v 1.15

[jhrozek]

URL: https://github.com/SSSD/sssd/pull/246
Title: #246: filter_users and filter_groups stop working properly in v 1.15

Label: +Accepted

[sssd PR#246][comment] filter_users and filter_groups stop working properly in v 1.15

[jhrozek]

URL: https://github.com/SSSD/sssd/pull/246
Title: #246: filter_users and filter_groups stop working properly in v 1.15

jhrozek commented:
"""
* master:
 * e1052a50b9cf42963e0a805a43e295742609693 8
 * 4ef0b19a5e8a327443d027e57487c8a1e4f654ce
 * 180e0b282be6aeb047c4b24b46e0b56afba1fdc8
 * f24ee5cca4cd43e7edf26fec453fbd99392bbe4b
 * a012a71f21bf1a4687e58085f19c18cc5b2bbadd
"""

See the full comment at https://github.com/SSSD/sssd/pull/246#issuecomment-300477386

[sssd PR#246][closed] filter_users and filter_groups stop working properly in v 1.15

[jhrozek]

URL: https://github.com/SSSD/sssd/pull/246
Author: fidencio
 Title: #246: filter_users and filter_groups stop working properly in v 1.15
Action: closed

To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/246/head:pr246
git checkout pr246

[sssd PR#246][+Pushed] filter_users and filter_groups stop working properly in v 1.15

[jhrozek]

URL: https://github.com/SSSD/sssd/pull/246
Title: #246: filter_users and filter_groups stop working properly in v 1.15

Label: +Pushed