[PATCH] sudo: use "higher value wins" when ordering rules

[Pavel Březina]

https://fedorahosted.org/sssd/ticket/2682

I think this option should stay undocumented since we want the users to 
use the correct sorting logic.

[Jakub Hrozek]

On Thu, Jul 30, 2015 at 01:05:56PM +0200, Pavel Březina wrote:
> https://fedorahosted.org/sssd/ticket/2682
>
> I think this option should stay undocumented since we want the users to use
> the correct sorting logic.

I agree.
> From db18a64109d9e49fa8bcdad14f412c6e7159137d Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> Date: Wed, 29 Jul 2015 14:51:30 +0200
> Subject: [PATCH] sudo: use "higher value wins" when ordering rules
>
> This commit changes the default ordering logic (lower value wins) to
> a correct one that is used by native ldap support. It also adds a new
> option sudo_inverse_order to switch to the original SSSD (incorrect)
> behaviour if needed.

Did you already build a test RPM for the RHEL customer who reported the
bug? If not, please do so and let them confirm the fix.

One comment in the code:
> @@ -680,7 +684,7 @@ static errno_t sudosrv_get_sudorules_query_cache(TALLOC_CTX *mem_ctx,
>          goto done;
>      }
> ret = sort_sudo_rules(rules, count);
> ret = sort_sudo_rules(rules, count, inverse_order == false);

This is unreadable to me, because == false passes true to the function.
I would prefer:
    inverse_order ? false : true
>  if (ret != EOK) {
>      DEBUG(SSSDBG_OP_FAILURE,
>            "Could not sort rules by sudoOrder\n");

[Lukas Slebodnik]

On (04/08/15 18:54), Jakub Hrozek wrote:
> On Thu, Jul 30, 2015 at 01:05:56PM +0200, Pavel Březina wrote:
> > https://fedorahosted.org/sssd/ticket/2682
> >
> > I think this option should stay undocumented since we want the users to use
> > the correct sorting logic.
>
> I agree.
> > From db18a64109d9e49fa8bcdad14f412c6e7159137d Mon Sep 17 00:00:00 2001
> > From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> > Date: Wed, 29 Jul 2015 14:51:30 +0200
> > Subject: [PATCH] sudo: use "higher value wins" when ordering rules
> >
> > This commit changes the default ordering logic (lower value wins) to
> > a correct one that is used by native ldap support. It also adds a new
> > option sudo_inverse_order to switch to the original SSSD (incorrect)
> > behaviour if needed.
>
> Did you already build a test RPM for the RHEL customer who reported the
> bug? If not, please do so and let them confirm the fix.
>
> One comment in the code:
> > @@ -680,7 +684,7 @@ static errno_t sudosrv_get_sudorules_query_cache(TALLOC_CTX *mem_ctx,
> >          goto done;
> >      }
> > ret = sort_sudo_rules(rules, count);
> > ret = sort_sudo_rules(rules, count, inverse_order == false);
>
> This is unreadable to me, because == false passes true to the function.
> I would prefer:
>    inverse_order ? false : true

Neither proposed solution nor original version improve readability.

The prototype of sort_sudo_rules is:
static errno_t
sort_sudo_rules(struct sysdb_attrs **rules, size_t count, bool higher_wins);

The most readable version is:

const higher_wins = !inverse_order
    ret = sort_sudo_rules(rules, count, higher_wins);

or to change loginc in function sort_sudo_rules.

LS

[Pavel Březina]

On 08/05/2015 08:00 AM, Lukas Slebodnik wrote:
> On (04/08/15 18:54), Jakub Hrozek wrote:
> > On Thu, Jul 30, 2015 at 01:05:56PM +0200, Pavel Březina wrote:
> > > https://fedorahosted.org/sssd/ticket/2682
> > >
> > > I think this option should stay undocumented since we want the users to use
> > > the correct sorting logic.
> >
> > I agree.
> > > From db18a64109d9e49fa8bcdad14f412c6e7159137d Mon Sep 17 00:00:00 2001
> > > From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> > > Date: Wed, 29 Jul 2015 14:51:30 +0200
> > > Subject: [PATCH] sudo: use "higher value wins" when ordering rules
> > >
> > > This commit changes the default ordering logic (lower value wins) to
> > > a correct one that is used by native ldap support. It also adds a new
> > > option sudo_inverse_order to switch to the original SSSD (incorrect)
> > > behaviour if needed.
> >
> > Did you already build a test RPM for the RHEL customer who reported the
> > bug? If not, please do so and let them confirm the fix.
> >
> > One comment in the code:
> > > @@ -680,7 +684,7 @@ static errno_t sudosrv_get_sudorules_query_cache(TALLOC_CTX *mem_ctx,
> > >           goto done;
> > >       }
> > > ret = sort_sudo_rules(rules, count);
> > > ret = sort_sudo_rules(rules, count, inverse_order == false);
> >
> > This is unreadable to me, because == false passes true to the function.
> > I would prefer:
> >     inverse_order ? false : true
>
> Neither proposed solution nor original version improve readability.
>
> The prototype of sort_sudo_rules is:
> static errno_t
> sort_sudo_rules(struct sysdb_attrs **rules, size_t count, bool higher_wins);
>
> The most readable version is:
>  const higher_wins = !inverse_order
>  ret = sort_sudo_rules(rules, count, higher_wins);
>
> or to change loginc in function sort_sudo_rules.
>
> LS

I changed the logic... new patch is attached.

[Jakub Hrozek]

On Thu, Aug 06, 2015 at 12:18:54PM +0200, Pavel Březina wrote:
> On 08/05/2015 08:00 AM, Lukas Slebodnik wrote:
> > On (04/08/15 18:54), Jakub Hrozek wrote:
> > > On Thu, Jul 30, 2015 at 01:05:56PM +0200, Pavel Březina wrote:
> > > > https://fedorahosted.org/sssd/ticket/2682
> > > >
> > > > I think this option should stay undocumented since we want the users to use
> > > > the correct sorting logic.
> > >
> > > I agree.
> > > > From db18a64109d9e49fa8bcdad14f412c6e7159137d Mon Sep 17 00:00:00 2001
> > > > From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> > > > Date: Wed, 29 Jul 2015 14:51:30 +0200
> > > > Subject: [PATCH] sudo: use "higher value wins" when ordering rules
> > > >
> > > > This commit changes the default ordering logic (lower value wins) to
> > > > a correct one that is used by native ldap support. It also adds a new
> > > > option sudo_inverse_order to switch to the original SSSD (incorrect)
> > > > behaviour if needed.
> > >
> > > Did you already build a test RPM for the RHEL customer who reported the
> > > bug? If not, please do so and let them confirm the fix.
> > >
> > > One comment in the code:
> > > > @@ -680,7 +684,7 @@ static errno_t sudosrv_get_sudorules_query_cache(TALLOC_CTX *mem_ctx,
> > > >          goto done;
> > > >      }
> > > > ret = sort_sudo_rules(rules, count);
> > > > ret = sort_sudo_rules(rules, count, inverse_order == false);
> > >
> > > This is unreadable to me, because == false passes true to the function.
> > > I would prefer:
> > >    inverse_order ? false : true
> >
> > Neither proposed solution nor original version improve readability.
> >
> > The prototype of sort_sudo_rules is:
> > static errno_t
> > sort_sudo_rules(struct sysdb_attrs **rules, size_t count, bool higher_wins);
> >
> > The most readable version is:
> > const higher_wins = !inverse_order
> > ret = sort_sudo_rules(rules, count, higher_wins);
> >
> > or to change loginc in function sort_sudo_rules.
> >
> > LS
>
> I changed the logic... new patch is attached.

ACK

I'll just wait for CI results before pushing.

[Jakub Hrozek]

On Thu, Aug 13, 2015 at 05:17:32PM +0200, Jakub Hrozek wrote:
> ACK
>
> I'll just wait for CI results before pushing.

* master: 52e3ee5c5ff2c5a4341041826a803ad42d2b2de7

[Jakub Hrozek]

On Fri, Aug 14, 2015 at 11:00:25PM +0200, Jakub Hrozek wrote:
> On Thu, Aug 13, 2015 at 05:17:32PM +0200, Jakub Hrozek wrote:
> > ACK
> >
> > I'll just wait for CI results before pushing.
> master: 52e3ee5c5ff2c5a4341041826a803ad42d2b2de7

Attached is a backport to the sssd-1-11 branch. Please sanity-check
before I push..

[Lukas Slebodnik]

On (15/02/16 18:20), Jakub Hrozek wrote:
> On Fri, Aug 14, 2015 at 11:00:25PM +0200, Jakub Hrozek wrote:
> > On Thu, Aug 13, 2015 at 05:17:32PM +0200, Jakub Hrozek wrote:
> > > ACK
> > >
> > > I'll just wait for CI results before pushing.
> > master: 52e3ee5c5ff2c5a4341041826a803ad42d2b2de7
>
> Attached is a backport to the sssd-1-11 branch. Please sanity-check
> before I push..
> From 7ce09a4d04b7e18b729b0c064ba51f06e4df981d Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> Date: Wed, 29 Jul 2015 14:51:30 +0200
> Subject: [PATCH] sudo: use "higher value wins" when ordering rules
>
> This commit changes the default ordering logic (lower value wins) to
> a correct one that is used by native ldap support. It also adds a new
> option sudo_inverse_order to switch to the original SSSD (incorrect)
> behaviour if needed.
>
> Resolves:
> https://fedorahosted.org/sssd/ticket/2682
>
> Reviewed-by: Jakub Hrozek 
> (cherry picked from commit 52e3ee5c5ff2c5a4341041826a803ad42d2b2de7)

It compiles without issues and there is not difference in added lines
with the original patch.

LGTM

LS

[Lukas Slebodnik]

On (15/02/16 18:27), Lukas Slebodnik wrote:
> On (15/02/16 18:20), Jakub Hrozek wrote:
> > On Fri, Aug 14, 2015 at 11:00:25PM +0200, Jakub Hrozek wrote:
> > > On Thu, Aug 13, 2015 at 05:17:32PM +0200, Jakub Hrozek wrote:
> > > > ACK
> > > >
> > > > I'll just wait for CI results before pushing.
> > > master: 52e3ee5c5ff2c5a4341041826a803ad42d2b2de7
> >
> > Attached is a backport to the sssd-1-11 branch. Please sanity-check
> > before I push..
> > From 7ce09a4d04b7e18b729b0c064ba51f06e4df981d Mon Sep 17 00:00:00 2001
> > From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> > Date: Wed, 29 Jul 2015 14:51:30 +0200
> > Subject: [PATCH] sudo: use "higher value wins" when ordering rules
> >
> > This commit changes the default ordering logic (lower value wins) to
> > a correct one that is used by native ldap support. It also adds a new
> > option sudo_inverse_order to switch to the original SSSD (incorrect)
> > behaviour if needed.
> >
> > Resolves:
> > https://fedorahosted.org/sssd/ticket/2682
> >
> > Reviewed-by: Jakub Hrozek 
> > (cherry picked from commit 52e3ee5c5ff2c5a4341041826a803ad42d2b2de7)
>
> It compiles without issues and there is not difference in added lines
> with the original patch.
>
> LGTM

sssd-1-11:
* caa1686123b158c79349edd6916ca927f4522a88

LS