Re: [SSSD] [PATCH] CI: Enforce coverage make check failures

[Lukas Slebodnik]

On (23/06/15 17:01), Lukas Slebodnik wrote:
>On (17/06/15 16:26), Nikolai Kondrashov wrote:
>>Hi everyone,
>>
>>The attached patch enforces make check failures for the CI coverage build, so
>>we can catch more test failures.
>>
>>I think this should also be applied onto sssd-1-12.
>>
>>NOTE: CI won't pass with this patch ATM, as test_ipa_subdom_server needs to be
>>  fixed first.
>>
>>Nick
>
>>From 14485fbe72ab4c5acaea30899b1dc5bd652e4d36 Mon Sep 17 00:00:00 2001
>>From: Nikolai Kondrashov 
>>Date: Wed, 17 Jun 2015 15:13:41 +0300
>>Subject: [PATCH] CI: Enforce coverage make check failures
>>
>>Fail CI coverage build, if make-check stage fails. Previously make-check
>>stage failures were ignored for coverage build for the sake of
>>collecting coverage data in any case. However, catching extra test
>>failures seems more important than getting coverage data in all cases,
>>thus the change.
>>---
>> contrib/ci/run | 5 -
>> 1 file changed, 4 insertions(+), 1 deletion(-)
>>
>>diff --git a/contrib/ci/run b/contrib/ci/run
>>index 5f668ff..e22ed41 100755
>>--- a/contrib/ci/run
>>+++ b/contrib/ci/run
>>@@ -255,6 +255,7 @@ function build_coverage()
>> {
>> declare -r coverage_report_dir="ci-report-coverage"
>> declare test_dir
>>+declare status
>> 
>> export CFLAGS="$COVERAGE_CFLAGS"
>> 
>>@@ -271,8 +272,10 @@ function build_coverage()
>>  --base-directory "$BASE_DIR" \
>>  --output-file ci-base.info
>> # Run tests
>>-stage make-checkmake -j $CPU_NUM check || true
>>+status=$?
>>+stage make-checkmake -j $CPU_NUM check || status=$?
>> mv "$test_dir" ci-test-dir
>>+((status == 0))
>> 
>> stage lcov-post lcov --capture --directory . \
>>  --base-directory "$BASE_DIR" \
>>-- 
>>2.1.4
>>
>Just for record.
>The patch works but need to fix failed test on rhel6.
>
Nobody have tried to fix it for some time.
Therefore I filed a ticket.
https://fedorahosted.org/sssd/ticket/2819

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

Re: [SSSD] [PATCH] intg: Add more LDAP tests

[Lukas Slebodnik]

On (06/10/15 11:27), Jakub Hrozek wrote:
>On Wed, Sep 30, 2015 at 06:15:52PM +0300, Nikolai Kondrashov wrote:
>> Hi everyone,
>> 
>> Here is a patch set fixing some things in integration tests and adding more
>> LDAP tests:
>
>(Not a full review, just adding my ideas and impressions)
>
>I read these patches when I tried to add tests for the failing POSIX
>check. That didn't work out, but at least I have a better idea about the
>integration tests now :-)
>
>Most importantly, the current tests are largely using enumeration. That
>not wrong and we want this codepath to be tested, but it's not the
>default of SSSD, so we want the non-enumeration also.
>
>I also wonder if instead of bool-like parameter that says if we're using
>rfc2307 or rfc2307bis we should have a more generic 'schema' parameter.
>
>Both are in some way here:
>
> https://fedorapeople.org/cgit/jhrozek/public_git/sssd.git/commit/?h=intg_test=568a0090c664d61e0491c2c2e4f8a3b68189a816
>
>Most of the (not well formatted) patch is the schema change, you can
>also see ldap_schema=ad being added. The test_broken_posix_in_ad()
>function also shows a test without enumeration -- I think we want tests
>for adding/removing a user/group/membership with rfc2307(bis) schema
>along these lines as well.
>
>> 
>> * Adding/removing a user/group/membership with rfc2307(bis) schema.
>> * Filtering users/groups with rfc2307(bis) schema.
>> * The effect of override_homedir option.
>> * The effect of fallback_homedir option.
>> * The effect of override_shell option.
>> * The effect of shell_fallback option.
>> * The effect of default_shell option.
>> * The effect of vetoed_shells option.
>> 
>> These are pretty basic, but I think they're good for the start.
>> Suggestions for more tests are welcome :)
>> 
>> NOTE: These still break test_memory_cache.py as seen in the attached log 
>> file.
>>   We need to figure out why and do something with it. Otherwise, the
>>   tests work fine.
>> 
>> Nick
>
>> From 8eb90565998f43fdc6b9ea3564527620c862f7fb Mon Sep 17 00:00:00 2001
>> From: Nikolai Kondrashov 
>> Date: Tue, 29 Sep 2015 20:13:04 +0300
>> Subject: [PATCH 2/7] intg: Remove _rfc2307 from function names
>> 
>> Remove "_rfc2307" from integration test function names for brevity.
>> ---
>>  src/tests/intg/ldap_test.py | 12 +++
>>  src/tests/intg/test_memory_cache.py | 70 
>> ++---
>>  2 files changed, 41 insertions(+), 41 deletions(-)
>> 
>> diff --git a/src/tests/intg/ldap_test.py b/src/tests/intg/ldap_test.py
>> index 0287a28..e359ab4 100644
>> --- a/src/tests/intg/ldap_test.py
>> +++ b/src/tests/intg/ldap_test.py
>> @@ -104,7 +104,7 @@ def create_sssd_fixture(request):
>>  
>>  
>>  @pytest.fixture
>> -def sanity_rfc2307(request, ldap_conn):
>> +def sanity(request, ldap_conn):
>>  ent_list = ldap_ent.List(ldap_conn.ds_inst.base_dn)
>>  ent_list.add_user("user1", 1001, 2001)
>>  ent_list.add_user("user2", 1002, 2002)
>> @@ -149,7 +149,7 @@ def sanity_rfc2307(request, ldap_conn):
>>  
>>  
>>  @pytest.fixture
>> -def simple_rfc2307(request, ldap_conn):
>> +def simple(request, ldap_conn):
>>  ent_list = ldap_ent.List(ldap_conn.ds_inst.base_dn)
>>  ent_list.add_user('usr001', 181818, 181818)
>>  ent_list.add_group("group1", 181818)
>> @@ -180,7 +180,7 @@ def simple_rfc2307(request, ldap_conn):
>>  
>>  
>>  @pytest.fixture
>> -def sanity_rfc2307_bis(request, ldap_conn):
>> +def sanity_bis(request, ldap_conn):
>>  ent_list = ldap_ent.List(ldap_conn.ds_inst.base_dn)
>>  ent_list.add_user("user1", 1001, 2001)
>>  ent_list.add_user("user2", 1002, 2002)
>> @@ -238,14 +238,14 @@ def sanity_rfc2307_bis(request, ldap_conn):
>>  return None
>>  
>>  
>> -def test_regression_ticket2163(ldap_conn, simple_rfc2307):
>> +def test_regression_ticket2163(ldap_conn, simple):
>>  ent.assert_passwd_by_name(
>>  'usr\\001',
>>  dict(name='usr\\001', passwd='*', uid=181818, gid=181818,
>>   gecos='181818', shell='/bin/bash'))
>>  
>>  
>> -def test_sanity_rfc2307(ldap_conn, sanity_rfc2307):
>> +def test_sanity(ldap_conn, sanity):
>>  passwd_pattern = ent.contains_only(
>>  dict(name='user1', passwd='*', uid=1001, gid=2001, gecos='1001', 
>> dir='/home/user1', shell='/bin/bash'),
>>  dict(name='user2', passwd='*', uid=1002, gid=2002, gecos='1002', 
>> dir='/home/user2', shell='/bin/bash'),
>> @@ -272,7 +272,7 @@ def test_sanity_rfc2307(ldap_conn, sanity_rfc2307):
>>  grp.getgrgid(1)
>>  
>>  
>> -def test_sanity_rfc2307_bis(ldap_conn, sanity_rfc2307_bis):
>> +def test_sanity_bis(ldap_conn, sanity_bis):
>>  passwd_pattern = ent.contains_only(
>>  dict(name='user1', passwd='*', uid=1001, gid=2001, gecos='1001', 
>> dir='/home/user1', shell='/bin/bash'),
>>  dict(name='user2', passwd='*', uid=1002, gid=2002, gecos='1002', 
>> dir='/home/user2', 

Re: [SSSD] More upstream CI tests

[Jakub Hrozek]

On Wed, Oct 07, 2015 at 08:58:32AM +0200, Lukas Slebodnik wrote:
> On (31/08/15 12:31), Jakub Hrozek wrote:
> >On Fri, Aug 28, 2015 at 03:16:57PM +0200, Lukas Slebodnik wrote:
> >> On (19/08/15 23:38), Jakub Hrozek wrote:
> >> >Hi,
> >> >
> >> >as we're stabilizing the 1.13 branch and before we plan what we want to
> >> >work on during the 1.14 development, we should use that time to write
> >> >some more tests!
> >> >
> >> >Here are some areas where we could add tests. Please discuss or add your
> >> >ideas, I would like to turn this list into tickets we can start
> >> >implementing:
> >> >
> >> >* Extend the LDAP provider tests with more dynamic test cases.
> >> >- add a user to a group, run sss_cache, assert id user displays the
> >> >  new group and getent group displays the new member
> >> >- conversely with removing users from groups
> There are some tests on list, but still it wuld be good to se a test plan.
> 
> >> >* Background refresh
> >> >- could be built atop the LDAP NSS tests as well. I think we have
> >> >  all the infrastructure in place.
> >> >* Local overrides integration test:
> >> >- this could be relatively easy, just call the overrides tool and
> >> >  request the entry. Could be built atop the existing LDAP tests
> >> >  or even use the local provider.
> >> Firstly, we need to prepare test cases for this feature.
> >> We can inspire in downstream ipa-views test case.
> >> After review of test cases we can split it to several tickets.
> >
> >We already do:
> >https://fedorahosted.org/sssd/ticket/2732
> >
> >It 'just' needs an assignee..
> IMHo this should be a very low priority ticket. It's already tested by
> downstream and they will have automated tests. We should focus to other parts
> which cannot be easily tested (whithout greping log files or touching/checking
>  internals in sysdb)
> https://fedorahosted.org/sssd/ticket/2697

Yes, if Pavel didn't start on #2732 yet, then we can move it down, I
agree PAM responder tests have bigger priority (also in Trac:
major > minor), but..

> 
> BTW if the tests were ready together with feature it would be something
> different. It would not be a duplicated effort.
> 
> If we really want to duplicate effort then it would be better to see/review
> a test plan for local views.

..it's not completely useless though, we will refactor sssd_nss in 1.14,
so we should have tests for views as well.

> Transforming to pytest should be a trivial task.
> Should I file a ticket?

I'm not sure I understand? From the start I thought localviews tests
would be in pytest, running tools and then getent..

> 
> >>
> >> >* Add a KDC
> >> >- until we have a pam_wrapper, this would only be useful to test
> >> >  ldap_child, but adding the KDC instantiation might be worth it
> >> >  nonetheless
> >> >- there is a protorype of KDC instantiation on the list for some
> >> >  time now, since we enabled rootless SSSD
> https://fedorahosted.org/sssd/ticket/2822
> >> >* IFP - could we reuse the existing sbus tests to spawn a custom bus?
> >> Someone need to look into running dbus-daemon in cwrap env.
> >> 
> >> Then we need prepare test cases ...
> >
> >Well, the first testcase could be to run introspection -- that would
> >validate the bus is accessible.
> >
> >It could be the first ticket, if you agree, I can file a ticket and
> >assign it?
> >
> cwrap test for IFP should have the highest priority because it's not tested
> by downstream.

I think we should focus not only on what is untested, but what is untested
and will be changed in the next release. Currently we have too much untested
code :-) I know we will change the NSS responder and the LDAP provider so
we need tests for those (we already have some for NSS).

So if we will change IFP in 1.14, then yes, otherwise I would prefer to
add more LDAP tests and Samba first..

> 
> https://fedorahosted.org/sssd/ticket/2823
> https://fedorahosted.org/sssd/ticket/2824
> https://fedorahosted.org/sssd/ticket/2825
> 
> The first two tickets can be done in parallel.
> 
> >> 
> >> >* SUDO - can we trick sudo into connecting to our test sssd instance?
> >> >
> I thought it could be tested using pam_wrapper
> I filed a generat ticket for authentication + authorisation
> https://fedorahosted.org/sssd/ticket/2821
> 
> It can be split into testing pam provider and sudo provider.
> 
> >> >I think the order of priority is roughly as above. I think the LDAP
> >> >provider is critical enough to be well tested. The refresh and local
> >> >override tests might be nice to have because we would be refactoring the
> >> >NSS responder in 1.14, so we should have it tested.
> >> >
> >> >I'll be happy to hear other opionions, though!
> >> We can try to prepare fake AD in openldap (if possible)
> >> and test basic functionality with ldap provider.
> >
> >I would like to talk to Andreas next week and ask about Samba. I hope
> >this would be much easier and more robust.
> >
> Just for the 

Re: [SSSD] More upstream CI tests

[Jakub Hrozek]

On Wed, Oct 07, 2015 at 10:28:43AM +0200, Pavel Reichl wrote:
> Yes, that's how the test are. I'll send the first version on list today. I 
> just need to resolve some final glitches.

That's nice to hear that the tests will be added, but based on trac
priorities I was expecting the PAM responder tests to arrive first..
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

Re: [SSSD] More upstream CI tests

[Lukas Slebodnik]

On (07/10/15 10:23), Jakub Hrozek wrote:
>On Wed, Oct 07, 2015 at 08:58:32AM +0200, Lukas Slebodnik wrote:
>> On (31/08/15 12:31), Jakub Hrozek wrote:
>> >On Fri, Aug 28, 2015 at 03:16:57PM +0200, Lukas Slebodnik wrote:
>> >> On (19/08/15 23:38), Jakub Hrozek wrote:
>> >> >Hi,
>> >> >
>> >> >as we're stabilizing the 1.13 branch and before we plan what we want to
>> >> >work on during the 1.14 development, we should use that time to write
>> >> >some more tests!
>> >> >
>> >> >Here are some areas where we could add tests. Please discuss or add your
>> >> >ideas, I would like to turn this list into tickets we can start
>> >> >implementing:
>> >> >
>> >> >* Extend the LDAP provider tests with more dynamic test cases.
>> >> >- add a user to a group, run sss_cache, assert id user displays the
>> >> >  new group and getent group displays the new member
>> >> >- conversely with removing users from groups
>> There are some tests on list, but still it wuld be good to se a test plan.
>> 
>> >> >* Background refresh
>> >> >- could be built atop the LDAP NSS tests as well. I think we have
>> >> >  all the infrastructure in place.
>> >> >* Local overrides integration test:
>> >> >- this could be relatively easy, just call the overrides tool and
>> >> >  request the entry. Could be built atop the existing LDAP tests
>> >> >  or even use the local provider.
>> >> Firstly, we need to prepare test cases for this feature.
>> >> We can inspire in downstream ipa-views test case.
>> >> After review of test cases we can split it to several tickets.
>> >
>> >We already do:
>> >https://fedorahosted.org/sssd/ticket/2732
>> >
>> >It 'just' needs an assignee..
>> IMHo this should be a very low priority ticket. It's already tested by
>> downstream and they will have automated tests. We should focus to other parts
>> which cannot be easily tested (whithout greping log files or 
>> touching/checking
>>  internals in sysdb)
>> https://fedorahosted.org/sssd/ticket/2697
>
>Yes, if Pavel didn't start on #2732 yet, then we can move it down, I
>agree PAM responder tests have bigger priority (also in Trac:
>major > minor), but..
>
Both of these ticket are in state new.

>> 
>> BTW if the tests were ready together with feature it would be something
>> different. It would not be a duplicated effort.
>> 
>> If we really want to duplicate effort then it would be better to see/review
>> a test plan for local views.
>
>..it's not completely useless though, we will refactor sssd_nss in 1.14,
>so we should have tests for views as well.
>
But it would be also tested by downstream so we could use their tests.

>> Transforming to pytest should be a trivial task.
>> Should I file a ticket?
>
>I'm not sure I understand? From the start I thought localviews tests
>would be in pytest, running tools and then getent..
>
I was writing about test plan for local views. It should be prepared/reviewed
in advance. So should I file a ticket for preparing test plan?
or do we want use test plan from downstream.

>> 
>> >>
>> >> >* Add a KDC
>> >> >- until we have a pam_wrapper, this would only be useful to test
>> >> >  ldap_child, but adding the KDC instantiation might be worth it
>> >> >  nonetheless
>> >> >- there is a protorype of KDC instantiation on the list for some
>> >> >  time now, since we enabled rootless SSSD
>> https://fedorahosted.org/sssd/ticket/2822
>> >> >* IFP - could we reuse the existing sbus tests to spawn a custom bus?
>> >> Someone need to look into running dbus-daemon in cwrap env.
>> >> 
>> >> Then we need prepare test cases ...
>> >
>> >Well, the first testcase could be to run introspection -- that would
>> >validate the bus is accessible.
>> >
>> >It could be the first ticket, if you agree, I can file a ticket and
>> >assign it?
>> >
>> cwrap test for IFP should have the highest priority because it's not tested
>> by downstream.
>
>I think we should focus not only on what is untested, but what is untested
>and will be changed in the next release. Currently we have too much untested
>code :-) I know we will change the NSS responder and the LDAP provider so
>we need tests for those (we already have some for NSS).
>
We do not have a lot of time for this. So we should not waste it.
There is possibility to use downstream tests. So why should we duplicate
effort. IFP is not tested at all. The tests for IFP could be used as an
example/howto for other projects which would like to use IFP.

>So if we will change IFP in 1.14, then yes, otherwise I would prefer to
>add more LDAP tests and Samba first..
>
If we have a time then we can implement additional tests for LDAP or Samba.
We it's very likely it will take a long time to setup more samba-dc in cwrap.
So +1 for sample test for samba, but not for full tests. But it would be better
to focus on part which are not tested at all or for preparing sample test
for each use case (setup samba4-dc, setup dbus-daemon, sample test for pam ...)

If sample 

Re: [SSSD] More upstream CI tests

[Jakub Hrozek]

On Wed, Oct 07, 2015 at 10:38:44AM +0200, Lukas Slebodnik wrote:
> On (07/10/15 10:23), Jakub Hrozek wrote:
> >On Wed, Oct 07, 2015 at 08:58:32AM +0200, Lukas Slebodnik wrote:
> >> On (31/08/15 12:31), Jakub Hrozek wrote:
> >> >On Fri, Aug 28, 2015 at 03:16:57PM +0200, Lukas Slebodnik wrote:
> >> >> On (19/08/15 23:38), Jakub Hrozek wrote:
> >> >> >Hi,
> >> >> >
> >> >> >as we're stabilizing the 1.13 branch and before we plan what we want to
> >> >> >work on during the 1.14 development, we should use that time to write
> >> >> >some more tests!
> >> >> >
> >> >> >Here are some areas where we could add tests. Please discuss or add 
> >> >> >your
> >> >> >ideas, I would like to turn this list into tickets we can start
> >> >> >implementing:
> >> >> >
> >> >> >* Extend the LDAP provider tests with more dynamic test cases.
> >> >> >- add a user to a group, run sss_cache, assert id user displays the
> >> >> >  new group and getent group displays the new member
> >> >> >- conversely with removing users from groups
> >> There are some tests on list, but still it wuld be good to se a test plan.
> >> 
> >> >> >* Background refresh
> >> >> >- could be built atop the LDAP NSS tests as well. I think we have
> >> >> >  all the infrastructure in place.
> >> >> >* Local overrides integration test:
> >> >> >- this could be relatively easy, just call the overrides tool and
> >> >> >  request the entry. Could be built atop the existing LDAP tests
> >> >> >  or even use the local provider.
> >> >> Firstly, we need to prepare test cases for this feature.
> >> >> We can inspire in downstream ipa-views test case.
> >> >> After review of test cases we can split it to several tickets.
> >> >
> >> >We already do:
> >> >https://fedorahosted.org/sssd/ticket/2732
> >> >
> >> >It 'just' needs an assignee..
> >> IMHo this should be a very low priority ticket. It's already tested by
> >> downstream and they will have automated tests. We should focus to other 
> >> parts
> >> which cannot be easily tested (whithout greping log files or 
> >> touching/checking
> >>  internals in sysdb)
> >> https://fedorahosted.org/sssd/ticket/2697
> >
> >Yes, if Pavel didn't start on #2732 yet, then we can move it down, I
> >agree PAM responder tests have bigger priority (also in Trac:
> >major > minor), but..
> >
> Both of these ticket are in state new.
> 
> >> 
> >> BTW if the tests were ready together with feature it would be something
> >> different. It would not be a duplicated effort.
> >> 
> >> If we really want to duplicate effort then it would be better to see/review
> >> a test plan for local views.
> >
> >..it's not completely useless though, we will refactor sssd_nss in 1.14,
> >so we should have tests for views as well.
> >
> But it would be also tested by downstream so we could use their tests.
> 
> >> Transforming to pytest should be a trivial task.
> >> Should I file a ticket?
> >
> >I'm not sure I understand? From the start I thought localviews tests
> >would be in pytest, running tools and then getent..
> >
> I was writing about test plan for local views. It should be prepared/reviewed
> in advance. So should I file a ticket for preparing test plan?
> or do we want use test plan from downstream.

We can base the tests on downstream.

> 
> >> 
> >> >>
> >> >> >* Add a KDC
> >> >> >- until we have a pam_wrapper, this would only be useful to test
> >> >> >  ldap_child, but adding the KDC instantiation might be worth it
> >> >> >  nonetheless
> >> >> >- there is a protorype of KDC instantiation on the list for some
> >> >> >  time now, since we enabled rootless SSSD
> >> https://fedorahosted.org/sssd/ticket/2822
> >> >> >* IFP - could we reuse the existing sbus tests to spawn a custom bus?
> >> >> Someone need to look into running dbus-daemon in cwrap env.
> >> >> 
> >> >> Then we need prepare test cases ...
> >> >
> >> >Well, the first testcase could be to run introspection -- that would
> >> >validate the bus is accessible.
> >> >
> >> >It could be the first ticket, if you agree, I can file a ticket and
> >> >assign it?
> >> >
> >> cwrap test for IFP should have the highest priority because it's not tested
> >> by downstream.
> >
> >I think we should focus not only on what is untested, but what is untested
> >and will be changed in the next release. Currently we have too much untested
> >code :-) I know we will change the NSS responder and the LDAP provider so
> >we need tests for those (we already have some for NSS).
> >
> We do not have a lot of time for this. So we should not waste it.
> There is possibility to use downstream tests. So why should we duplicate
> effort. IFP is not tested at all. The tests for IFP could be used as an
> example/howto for other projects which would like to use IFP.

Downstream tests are currently still not very easy to run. The point of
the downstream tests is to be able to run "make check" or "make
intgcheck" while working on code and see 

Re: [SSSD] More upstream CI tests

[Pavel Reichl]

On 10/07/2015 10:23 AM, Jakub Hrozek wrote:

On Wed, Oct 07, 2015 at 08:58:32AM +0200, Lukas Slebodnik wrote:

On (31/08/15 12:31), Jakub Hrozek wrote:

On Fri, Aug 28, 2015 at 03:16:57PM +0200, Lukas Slebodnik wrote:

On (19/08/15 23:38), Jakub Hrozek wrote:

Hi,

as we're stabilizing the 1.13 branch and before we plan what we want to
work on during the 1.14 development, we should use that time to write
some more tests!

Here are some areas where we could add tests. Please discuss or add your
ideas, I would like to turn this list into tickets we can start
implementing:

* Extend the LDAP provider tests with more dynamic test cases.
- add a user to a group, run sss_cache, assert id user displays the
  new group and getent group displays the new member
- conversely with removing users from groups

There are some tests on list, but still it wuld be good to se a test plan.


* Background refresh
- could be built atop the LDAP NSS tests as well. I think we have
  all the infrastructure in place.
* Local overrides integration test:
- this could be relatively easy, just call the overrides tool and
  request the entry. Could be built atop the existing LDAP tests
  or even use the local provider.

Firstly, we need to prepare test cases for this feature.
We can inspire in downstream ipa-views test case.
After review of test cases we can split it to several tickets.


We already do:
https://fedorahosted.org/sssd/ticket/2732

It 'just' needs an assignee..

IMHo this should be a very low priority ticket. It's already tested by
downstream and they will have automated tests. We should focus to other parts
which cannot be easily tested (whithout greping log files or touching/checking
  internals in sysdb)
https://fedorahosted.org/sssd/ticket/2697


Yes, if Pavel didn't start on #2732 yet, then we can move it down, I
agree PAM responder tests have bigger priority (also in Trac:
major > minor), but..



BTW if the tests were ready together with feature it would be something
different. It would not be a duplicated effort.

If we really want to duplicate effort then it would be better to see/review
a test plan for local views.


..it's not completely useless though, we will refactor sssd_nss in 1.14,
so we should have tests for views as well.


Transforming to pytest should be a trivial task.
Should I file a ticket?


I'm not sure I understand? From the start I thought localviews tests
would be in pytest, running tools and then getent..


Yes, that's how the test are. I'll send the first version on list today. I just 
need to resolve some final glitches.
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

Re: [SSSD] [PATCH] IPA: Check connection validity explicitly

[Sumit Bose]

On Tue, Oct 06, 2015 at 12:54:41PM +0200, Jakub Hrozek wrote:
> On Mon, Oct 05, 2015 at 04:46:55PM +0200, Jakub Hrozek wrote:
> > On Mon, Oct 05, 2015 at 12:33:48PM +0200, Sumit Bose wrote:
> > > On Mon, Oct 05, 2015 at 12:08:26PM +0200, Jakub Hrozek wrote:
> > > > On Fri, Oct 02, 2015 at 10:32:27AM +0200, Jakub Hrozek wrote:
> > > > > On Thu, Oct 01, 2015 at 01:41:07PM +0200, Jakub Hrozek wrote:
> > > > > > I don't have a good idea how to reproduce except simulate the 
> > > > > > failure in
> > > > > > gdb, sorry... at least I verified that after setting the context to 
> > > > > > NULL:
> > > > > > (gdb) set clist[1] = 0
> > > > > > the request runs to completion and SSSD doesn't crash anymore. So I 
> > > > > > think
> > > > > > we should just add a check..
> > > > > 
> > > > > Turns out this is easier to reproduce and hence more embarassing. Just
> > > > > add POSIX attributes to AD but don't replicate them to GC..
> > > > 
> > > > Any takers for review?
> > > 
> > > (I already took it some time ago in patchworks :-)
> > > 
> > > While the patch fixes the issue and is also a right way to fix the issue
> > > I would recommend to change it a bit to make the reason more clear.
> > > 
> > > As you said to reproduce the issue you need trust to AD configured which
> > > POSIX attributes but not replicate them to the Global Catalog. If you
> > > new start with an empty cache a sequence like
> > > 
> > > getent group group_with_posix_gid@ad.domain
> > > sss_cache -E
> > > getent group group_with_posix_gid@ad.domain
> > > 
> > > will trigger the crash.
> > > 
> > > What happens is that during the first lookup SSSD sees that the GC does
> > > not have the POSIX attributes and disable the GC lookup. As a
> > > consequence ad_gc_conn_list() will return only a single entry for the
> > > second lookup and unconditionally accessing the second entry will fail.
> > > While the fix is correct, I would recommend to add a boolean option to 
> > > ad_gc_conn_list() and set ignore_mark_offline in ad_gc_conn_list().
> > > Otherwise you have to add extra logic to ipa_get_ad_acct_send() to make
> > > sure ignore_mark_offline is set even it ad_gc_conn_list() only returns
> > > one element.
> > 
> > You're right that the logic was scattered all over the place. The
> > attached patches consolidate it into ad_common.c so all callers use the
> > same code. It's a larger change than before, but hopefully it's still
> > digestable and as a bonus we can have tests for different combinations
> > of contexts, GC status and master/sub domains.
> > 
> > The second patch is not really needed, the only reason I included it is
> > that I think it's better to have similar code grouped together in small
> > functions and again, tests.
> > 
> > If you think these are too invasive, we can only apply them to master
> > and do exactly what you proposed for sssd-1-13.
> > 
> > btw I had a bit of trouble with downstream tests, the seem to fail in
> > unrelated way, so I'm sending the patches to the list after some manual
> > testing and will resume the downstream tests tomorrow..
> 
> The attached patches fix some more Sumit's review comments. With Lukas'
> help I was also able to run downstream ad_forest tests and didn't see
> any issues even with the patches.

The patches are looking good. I think they are not too invasive and can
be pushed to sssd-1-13 as well. As expected as I wasn't able to trigger
the crash anymore and CI
(http://sssd-ci.duckdns.org/logs/job/29/39/summary.html) passed as well.

ACK

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

Re: [SSSD] [PATCH] IPA: Check connection validity explicitly

[Jakub Hrozek]

On Wed, Oct 07, 2015 at 12:28:33PM +0200, Sumit Bose wrote:
> ACK
> 
> bye,
> Sumit

Thank you for the review:
master:
309aa83d16b5919f727af04850bcd0799ba0962f
afb21fd06690a0bec288a7970abf74ed2ea7dfdc 
sssd-1-13:
15a4b34ccfcfbcec2c9ba529d0113adf251abc16
f1742784d9b1cffd74f67beeb26375124183428a 
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

Re: [SSSD] [PATCH] GPO: fix memory leak

[Lukas Slebodnik]

On (02/10/15 09:06), Lukas Slebodnik wrote:
>On (07/09/15 18:56), Jakub Hrozek wrote:
>>On Mon, Sep 07, 2015 at 05:36:18PM +0200, Michal Židek wrote:
>>> On 09/03/2015 10:53 AM, Pavel Reichl wrote:
>>> >Hello,
>>> >
>>> >please see simple patch attached.
>>> >
>>> >Thanks!
>>> >
>>> 
>>> ACK.
>>> 
>>> CI passed: http://sssd-ci.duckdns.org/logs/job/25/51/summary.html
>>> 
>>> Michal
>>
>>* master: 5dbdcc2c7210a0e3eb60ad1e85ba33f27d7faeda
>Are there any objection for pushing this patch to 1.12 branch?
>
sssd-1-12:

4a89c2ebbe2a98e790536165486ecf2d14836e69

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

Re: [SSSD] [PATCH] intg: Add more LDAP tests

[Nikolai Kondrashov]

On 10/07/2015 10:51 AM, Lukas Slebodnik wrote:

On (06/10/15 11:27), Jakub Hrozek wrote:

On Wed, Sep 30, 2015 at 06:15:52PM +0300, Nikolai Kondrashov wrote:

Remove "_rfc2307" from integration test function names for brevity.


Please do not remove sanity_rfc2307 from this test.
It was added intentionaly. So it's clear from the name of fixture
which ldap schema is used.

If you want to remove it from ldap_test.py feel free to do it
but please do not chage it in test_memory_cache.py


Sure, no problem. Moreover, based on conversation with Jakub, I think I'll
need to keep them everywhere, one way or another.


BTW: there are introduced new pep8 warning by your patches:

0007-intg-Add-more-LDAP-tests.patch
In attachement :-)

Some of warnings neen't be caught by pep8 --diff (blank lines after function ..


Right, I forgot to run it again. Thank you for checking.


And now back to failing test memory cache.
It works for me if I change the order of tests (mv ldap_test_.py test_xldap.py)
So your test had to introduce an issue in last patch.


Yeah, my tests are obviously interfering with your tests. I hoped you might
suggest the reason, since you know them and SSSD better, but I can dig it
myself.


It also constantly fails for me; so it can be related.
 FAILURES 

__ test_add_remove_user 
__


Now, this is interesting. Is this the only test that fails? If not, which
tests fail? Is the failure pattern constant? Which environment were you
running them in? Was it my unchanged patch set, or was that after you switched
the order?

Thank you.

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

Re: [SSSD] [PATCH] sss_override fixes

[Pavel Reichl]

On 10/07/2015 04:12 PM, Pavel Reichl wrote:

On 10/07/2015 02:46 PM, Pavel Březina wrote:

Hi,
the first two patches are just nitpicks. The last patch fixes a memory 
violation.


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



Patch set fixes failing of wip intg. test 
ldap_local_override_test.py::test_local_override_group.
Code LGTM.
CI passed: http://sssd-ci.duckdns.org/logs/job/29/45/summary.html

ACK to all.

Thanks Pavel for prompt fix.
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel


Oops could you please add ticket number to commit messages?
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

Re: [SSSD] [PATCH] typos in integration tests

[Pavel Reichl]

On 10/07/2015 02:48 PM, Pavel Březina wrote:

No big deal...

0001-intg-fix-typos.patch


 From 923d5626fd3cefaf0620c64aecacfba26b439250 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Pavel=20B=C5=99ezina?=
Date: Wed, 7 Oct 2015 14:16:38 +0200
Subject: [PATCH] intg: fix typos

---
  src/tests/intg/sssd_id.py | 6 +++---
  1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/src/tests/intg/sssd_id.py b/src/tests/intg/sssd_id.py
index 
45f2822b5b33d99b6a7ca0450c774e05ff11..765371215eaa203a5b05cdea64caccb549b90e00
 100644
--- a/src/tests/intg/sssd_id.py
+++ b/src/tests/intg/sssd_id.py
@@ -42,7 +42,7 @@ def call_sssd_initgroups(user, gid):
  @param int gid the additional gid will be also added to the list.

  @return (int, int, List[int]) (err, errno, gids)
-gids shoudl contain user group IDs if err is NssReturnCode.SUCCESS
+gids should contain user group IDs if err is NssReturnCode.SUCCESS
  otherwise errno will contain non-zero vlaue.

could you please also change vlaue to value

  """
  libnss_sss_path = config.PREFIX + "/lib/libnss_sss.so.2"
@@ -85,7 +85,7 @@ def get_user_gids(user):
  @param string user name of user

  @return (int, int, List[int]) (err, errno, gids)
-gids shoudl contain user group IDs if err is NssReturnCode.SUCCESS
+gids should contain user group IDs if err is NssReturnCode.SUCCESS
  otherwise errno will contain non-zero vlaue.

same here

  """
  pwd_user = pwd.getpwnam(user)
@@ -106,7 +106,7 @@ def get_user_groups(user):
  @param string user name of user

  @return (int, int, List[string]) (err, errno, groups)
-roups shoudl contain names of user groups
+groups should contain names of user groups
  if err is NssReturnCode.SUCCESS
  otherwise errno will contain non-zero vlaue.

and here

  """
-- 2.1.0

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

Re: [SSSD] [PATCHSET] intg: sss_override

[Lukas Slebodnik]

On (07/10/15 14:25), Pavel Reichl wrote:
>Hello,
>
>please see first version of patch set. Please consider this to be work in 
>progress.
>
>The test 'test_local_override_group' fails, there seem to be a bug in 
>sss_override. Pavel is working on the patch. He kindly provided me with early 
>version of patch and it fixed the test.
>
>Currently also test for #2790 test_local_overrides_regr_2790() is failing 
>which is probably just bug in the test itself.
>
>Thanks for any comments.
>
Woudl you be so kind and describe which test cases did you decide to cover in
your test?

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

Re: [SSSD] [PATCHSET] intg: sss_override

[Pavel Reichl]

On 10/07/2015 02:55 PM, Lukas Slebodnik wrote:

On (07/10/15 14:25), Pavel Reichl wrote:

Hello,

please see first version of patch set. Please consider this to be work in 
progress.

The test 'test_local_override_group' fails, there seem to be a bug in 
sss_override. Pavel is working on the patch. He kindly provided me with early 
version of patch and it fixed the test.

Currently also test for #2790 test_local_overrides_regr_2790() is failing which 
is probably just bug in the test itself.

Thanks for any comments.


Woudl you be so kind and describe which test cases did you decide to cover in
your test?


Sure,

test test_local_override_user()
1) overrides users with different sets of attributes
2) export user overrides
3) check that user overrides do not stack
4) delete user overrides
5) import of user overrides

test_local_override_user_cached()
* does the same as test test_local_override_user() but users are present in 
sysdb before the test starts
Is this test redundant?

test_local_override_group()
* the very same thing as test_local_override_user() but for groups

The remaining 3 tests have ticket number in their names and are intended as 
regression tests for those tickets.


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


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

Re: [SSSD] [PATCH] intg: Add more LDAP tests

[Nikolai Kondrashov]

On 10/06/2015 12:27 PM, Jakub Hrozek wrote:

On Wed, Sep 30, 2015 at 06:15:52PM +0300, Nikolai Kondrashov wrote:

Hi everyone,

Here is a patch set fixing some things in integration tests and adding more
LDAP tests:


(Not a full review, just adding my ideas and impressions)

I read these patches when I tried to add tests for the failing POSIX
check. That didn't work out, but at least I have a better idea about the
integration tests now :-)


Was the problem with the integration test framework or with something else?
If it's the former, can I offer my help?


Most importantly, the current tests are largely using enumeration. That
not wrong and we want this codepath to be tested, but it's not the
default of SSSD, so we want the non-enumeration also.


Alright, I'll try to use the py.test fixture parameters for this. It's a
cumbersome facility, but perhaps we can use it. If that fails, I'll try
something else.

I worry a bit about the doubled test runtime, though. Can we have some tests
running only with either disabled or enabled enumeration?


I also wonder if instead of bool-like parameter that says if we're using
rfc2307 or rfc2307bis we should have a more generic 'schema' parameter.

Both are in some way here:
 
https://fedorapeople.org/cgit/jhrozek/public_git/sssd.git/commit/?h=intg_test=568a0090c664d61e0491c2c2e4f8a3b68189a816

Most of the (not well formatted) patch is the schema change, you can
also see ldap_schema=ad being added. The test_broken_posix_in_ad()
function also shows a test without enumeration -- I think we want tests
for adding/removing a user/group/membership with rfc2307(bis) schema
along these lines as well.


Riiight, I completely forgot about the other schemas. Sure, we'll need
something more than a boolean. However, how about we define a couple constant
string variables and supply them instead of literal strings to reduce the
likelihood of typos?

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

[SSSD] [PATCH] sss_override fixes

[Pavel Březina]

Hi,
the first two patches are just nitpicks. The last patch fixes a memory 
violation.
From 179d51316bd9c7f44075b87e71524f5b14b99ded Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
Date: Wed, 7 Oct 2015 11:23:00 +0200
Subject: [PATCH 1/4] sss_override: fix comment describing format

---
 src/tools/sss_override.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/tools/sss_override.c b/src/tools/sss_override.c
index 075f024f0cda4820795c4d7f0cb9ff20a03bb271..dc7c31c3844009a0707b06b717bfae1d9b8e38d5 100644
--- a/src/tools/sss_override.c
+++ b/src/tools/sss_override.c
@@ -1454,7 +1454,7 @@ static int override_group_export(struct sss_cmdline *cmdline,
 
 for (i = 0; objs[i].orig_name != NULL; i++) {
 /**
- * Format: orig_name:name:uid:gid:gecos:home:shell
+ * Format: orig_name:name:gid
  */
 struct sss_colondb_write_field table[] = {
 {SSS_COLONDB_STRING, {.str = objs[i].orig_name}},
-- 
2.1.0

From f9ce1b3f63e2f1aed6615697c5c137a9894db539 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
Date: Wed, 7 Oct 2015 12:53:21 +0200
Subject: [PATCH 2/4] sss_override: explicitly set ret = EOK

---
 src/tools/sss_override.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/src/tools/sss_override.c b/src/tools/sss_override.c
index dc7c31c3844009a0707b06b717bfae1d9b8e38d5..16b25b216fba82267876e83f383ea0efad977011 100644
--- a/src/tools/sss_override.c
+++ b/src/tools/sss_override.c
@@ -895,6 +895,8 @@ static errno_t append_name(struct sss_domain_info *domain,
 
 talloc_steal(override, fqname);
 
+ret = EOK;
+
 done:
 talloc_free(tmp_ctx);
 
-- 
2.1.0

From 4e94e6db5ecf0e36096037f154e4990a22f712da Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
Date: Wed, 7 Oct 2015 13:47:18 +0200
Subject: [PATCH 3/4] sss_override: steal msgs string to objs

Since msgs is attached to tmp_ctx then all the strings are freed
with tmp_ctx. Now steal the strings to objs.
---
 src/tools/sss_override.c | 9 +
 1 file changed, 9 insertions(+)

diff --git a/src/tools/sss_override.c b/src/tools/sss_override.c
index 16b25b216fba82267876e83f383ea0efad977011..cf19e8287241445cc375e13b1763d33bb8d10985 100644
--- a/src/tools/sss_override.c
+++ b/src/tools/sss_override.c
@@ -1006,6 +1006,12 @@ list_user_overrides(TALLOC_CTX *mem_ctx,
 objs[i].home = ldb_msg_find_attr_as_string(msgs[i], SYSDB_HOMEDIR, NULL);
 objs[i].shell = ldb_msg_find_attr_as_string(msgs[i], SYSDB_SHELL, NULL);
 objs[i].gecos = ldb_msg_find_attr_as_string(msgs[i], SYSDB_GECOS, NULL);
+
+talloc_steal(objs, objs[i].orig_name);
+talloc_steal(objs, objs[i].name);
+talloc_steal(objs, objs[i].home);
+talloc_steal(objs, objs[i].shell);
+talloc_steal(objs, objs[i].gecos);
 }
 
 talloc_steal(mem_ctx, objs);
@@ -1061,6 +1067,9 @@ list_group_overrides(TALLOC_CTX *mem_ctx,
 
 objs[i].name = ldb_msg_find_attr_as_string(msgs[i], SYSDB_NAME, NULL);
 objs[i].gid = ldb_msg_find_attr_as_uint(msgs[i], SYSDB_GIDNUM, 0);
+
+talloc_steal(objs, objs[i].orig_name);
+talloc_steal(objs, objs[i].name);
 }
 
 talloc_steal(mem_ctx, objs);
-- 
2.1.0

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

Re: [SSSD] [PATCH] man: Minor fixes to filter_groups description

[Jakub Hrozek]

On Tue, Oct 06, 2015 at 11:29:58AM +0200, Jakub Hrozek wrote:
> On Wed, Sep 30, 2015 at 06:48:44PM +0300, Nikolai Kondrashov wrote:
> > Hi everyone,
> > 
> > I noticed one little thing was wrong with the combined
> > filter_users/filter_groups description on the sssd.conf(5) manpage and also
> > wanted to add a note WRT nested groups behavior with filter_groups which 
> > was a
> > bit surprising to me. The trivial patches are attached.
> > 
> > Nick
> 
> ACK this is better and hopefully would avoid the confusion you ran into.

* master:
* 27293426dca1bf9140dc6ed277f7129a44a68a62
* bf4ddcde94fc36b44bc9cbcc5d56e6e35776bfc9
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

[SSSD] [PATCHSET] intg: sss_override

[Pavel Reichl]

Hello,

please see first version of patch set. Please consider this to be work in 
progress.

The test 'test_local_override_group' fails, there seem to be a bug in 
sss_override. Pavel is working on the patch. He kindly provided me with early 
version of patch and it fixed the test.

Currently also test for #2790 test_local_overrides_regr_2790() is failing which 
is probably just bug in the test itself.

Thanks for any comments.

>From f8068960a2d393ca180ac40687620bbb4deb003c Mon Sep 17 00:00:00 2001
From: Pavel Reichl 
Date: Mon, 5 Oct 2015 10:00:41 -0400
Subject: [PATCH 1/5] intg: Support users with arb. gecos, homedir, shell

Add support for setting arbitrary values of user attribute: gecos,
homedir and shell.

Resolves:
https://fedorahosted.org/sssd/ticket/2732
---
 src/tests/intg/ldap_ent.py | 18 +++---
 1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/src/tests/intg/ldap_ent.py b/src/tests/intg/ldap_ent.py
index 30eed9d6426f47401712fd88dee7f11bd747b944..4f1e259cd707356dc6c7e819bbd0fc5195f1f1a9 100644
--- a/src/tests/intg/ldap_ent.py
+++ b/src/tests/intg/ldap_ent.py
@@ -18,13 +18,13 @@
 #
 
 
-def user(base_dn, uid, uidNumber, gidNumber):
+def user(base_dn, uid, uidNumber, gidNumber, gecos, homedir, shell):
 """
 Generate an RFC2307(bis) user add-modlist for passing to ldap.add*
 """
 uidNumber = str(uidNumber)
 gidNumber = str(gidNumber)
-return (
+attrs = (
 "uid=" + uid + ",ou=Users," + base_dn,
 [
 ('objectClass', ['top', 'inetOrgPerson', 'posixAccount']),
@@ -33,11 +33,15 @@ def user(base_dn, uid, uidNumber, gidNumber):
 ('uidNumber',   [uidNumber]),
 ('gidNumber',   [gidNumber]),
 ('userPassword',['Password' + uidNumber]),
-('homeDirectory',   ['/home/' + uid]),
-('loginShell',  ['/bin/bash']),
+('homeDirectory',   [homedir if homedir else '/home/' + uid]),
+('loginShell',  [shell if shell else '/bin/bash']),
 ]
 )
 
+if gecos:
+attrs[1].append(('gecos', [gecos]))
+return attrs
+
 
 def group(base_dn, cn, gidNumber, member_uids=[]):
 """
@@ -85,11 +89,11 @@ class List(list):
 def __init__(self, base_dn):
 self.base_dn = base_dn
 
-def add_user(self, uid, uidNumber, gidNumber,
- base_dn=None):
+def add_user(self, uid, uidNumber, gidNumber, gecos=None,
+ homedir=None, shell=None, base_dn=None):
 """Add an RFC2307(bis) user add-modlist."""
 self.append(user(base_dn or self.base_dn,
- uid, uidNumber, gidNumber))
+ uid, uidNumber, gidNumber, gecos, homedir, shell))
 
 def add_group(self, cn, gidNumber, member_uids=[],
   base_dn=None):
-- 
2.4.3

>From e4028b9a81c498a20de5c12a1f0a788be7608463 Mon Sep 17 00:00:00 2001
From: Pavel Reichl 
Date: Mon, 5 Oct 2015 10:02:05 -0400
Subject: [PATCH 2/5] intg: new test for user local overrides

Introduce a new integration test for local view overrides.

Resolves:
https://fedorahosted.org/sssd/ticket/2732
---
 src/tests/intg/ldap_local_override_test.py | 376 +
 1 file changed, 376 insertions(+)
 create mode 100644 src/tests/intg/ldap_local_override_test.py

diff --git a/src/tests/intg/ldap_local_override_test.py b/src/tests/intg/ldap_local_override_test.py
new file mode 100644
index ..35e47b012fdd271dd50b4c2ce3749d28a7a746d8
--- /dev/null
+++ b/src/tests/intg/ldap_local_override_test.py
@@ -0,0 +1,376 @@
+#
+# LDAP integration test
+#
+# Copyright (c) 2015 Red Hat, Inc.
+# Author: Pavel Reichl  
+#
+# This is free software; you can redistribute it and/or modify it
+# under the terms of the GNU General Public License as published by
+# the Free Software Foundation; version 2 only
+#
+# This program is distributed in the hope that it will be useful, but
+# WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+# General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see .
+#
+import os
+import stat
+import ent
+import grp
+import pwd
+import config
+import signal
+import subprocess
+import time
+import pytest
+import ds_openldap
+import ldap_ent
+import sssd_id
+from util import unindent
+
+LDAP_BASE_DN = "dc=example,dc=com"
+
+
+@pytest.fixture(scope="module")
+def ds_inst(request):
+"""LDAP server instance fixture"""
+ds_inst = ds_openldap.DSOpenLDAP(
+config.PREFIX, 10389, LDAP_BASE_DN,
+"cn=admin", "Secret123")
+try:
+ds_inst.setup()
+except:
+ds_inst.teardown()
+raise
+request.addfinalizer(lambda: ds_inst.teardown())
+

[SSSD] [PATCH] typos in integration tests

[Pavel Březina]

No big deal...
From 923d5626fd3cefaf0620c64aecacfba26b439250 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
Date: Wed, 7 Oct 2015 14:16:38 +0200
Subject: [PATCH] intg: fix typos

---
 src/tests/intg/sssd_id.py | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/src/tests/intg/sssd_id.py b/src/tests/intg/sssd_id.py
index 45f2822b5b33d99b6a7ca0450c774e05ff11..765371215eaa203a5b05cdea64caccb549b90e00 100644
--- a/src/tests/intg/sssd_id.py
+++ b/src/tests/intg/sssd_id.py
@@ -42,7 +42,7 @@ def call_sssd_initgroups(user, gid):
 @param int gid the additional gid will be also added to the list.
 
 @return (int, int, List[int]) (err, errno, gids)
-gids shoudl contain user group IDs if err is NssReturnCode.SUCCESS
+gids should contain user group IDs if err is NssReturnCode.SUCCESS
 otherwise errno will contain non-zero vlaue.
 """
 libnss_sss_path = config.PREFIX + "/lib/libnss_sss.so.2"
@@ -85,7 +85,7 @@ def get_user_gids(user):
 @param string user name of user
 
 @return (int, int, List[int]) (err, errno, gids)
-gids shoudl contain user group IDs if err is NssReturnCode.SUCCESS
+gids should contain user group IDs if err is NssReturnCode.SUCCESS
 otherwise errno will contain non-zero vlaue.
 """
 pwd_user = pwd.getpwnam(user)
@@ -106,7 +106,7 @@ def get_user_groups(user):
 @param string user name of user
 
 @return (int, int, List[string]) (err, errno, groups)
-roups shoudl contain names of user groups
+groups should contain names of user groups
 if err is NssReturnCode.SUCCESS
 otherwise errno will contain non-zero vlaue.
 """
-- 
2.1.0

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

Re: [SSSD] [PATCH] sss_override fixes

[Pavel Reichl]

On 10/07/2015 02:46 PM, Pavel Březina wrote:

Hi,
the first two patches are just nitpicks. The last patch fixes a memory 
violation.


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



Patch set fixes failing of wip intg. test 
ldap_local_override_test.py::test_local_override_group.
Code LGTM.
CI passed: http://sssd-ci.duckdns.org/logs/job/29/45/summary.html

ACK to all.

Thanks Pavel for prompt fix.
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

Re: [SSSD] [PATCH] intg: Add more LDAP tests

[Jakub Hrozek]

On Wed, Oct 07, 2015 at 04:02:32PM +0300, Nikolai Kondrashov wrote:
> On 10/06/2015 12:27 PM, Jakub Hrozek wrote:
> >On Wed, Sep 30, 2015 at 06:15:52PM +0300, Nikolai Kondrashov wrote:
> >>Hi everyone,
> >>
> >>Here is a patch set fixing some things in integration tests and adding more
> >>LDAP tests:
> >
> >(Not a full review, just adding my ideas and impressions)
> >
> >I read these patches when I tried to add tests for the failing POSIX
> >check. That didn't work out, but at least I have a better idea about the
> >integration tests now :-)
> 
> Was the problem with the integration test framework or with something else?
> If it's the former, can I offer my help?

No, it was that while the code that triggered the error was in the
generic LDAP provider, the bug itself manifested only in AD provider..so
we'd need to wrap Samba to really test the bug.

> 
> >Most importantly, the current tests are largely using enumeration. That
> >not wrong and we want this codepath to be tested, but it's not the
> >default of SSSD, so we want the non-enumeration also.
> 
> Alright, I'll try to use the py.test fixture parameters for this. It's a
> cumbersome facility, but perhaps we can use it. If that fails, I'll try
> something else.
> 
> I worry a bit about the doubled test runtime, though. Can we have some tests
> running only with either disabled or enabled enumeration?

Maybe..in general I think non-enumeration are more important so if the
rigorous/essential parameters also apply to which integration tests we
run, then I would prefer non-enumeration tests.

> 
> >I also wonder if instead of bool-like parameter that says if we're using
> >rfc2307 or rfc2307bis we should have a more generic 'schema' parameter.
> >
> >Both are in some way here:
> > 
> > https://fedorapeople.org/cgit/jhrozek/public_git/sssd.git/commit/?h=intg_test=568a0090c664d61e0491c2c2e4f8a3b68189a816
> >
> >Most of the (not well formatted) patch is the schema change, you can
> >also see ldap_schema=ad being added. The test_broken_posix_in_ad()
> >function also shows a test without enumeration -- I think we want tests
> >for adding/removing a user/group/membership with rfc2307(bis) schema
> >along these lines as well.
> 
> Riiight, I completely forgot about the other schemas. Sure, we'll need
> something more than a boolean. However, how about we define a couple constant
> string variables and supply them instead of literal strings to reduce the
> likelihood of typos?

Yes, that's even better. Would you prefer if I sent such patch (atop
your patches) ?
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

Re: [SSSD] Review of umask() in SSSD

[Petr Cech]

On 10/04/2015 09:39 PM, Jakub Hrozek wrote:

Finally, because I'm a lazy reviewer, I would prefer:
 - a patch that converts 0177 to DFL, with a comment around the macro
   definition that this is the default secure umask
 - a patch that converts 0077 to DFL_X, with a comment around DFL_X
   definition that unless executable bit is explicitly needed, DFL
   should be used
 - a patch per change if we need to tighten the existing umasks
   further.


Hi Jakub,

I put more care and expanded review of umask in several patches.

Patch 0005-P11-CHILD-NSS was discussed with Sumit (thanks).

I'd like to ask about any special care at patch 0010-KRB5-CHILD.
I investigated it, but second look will be better.

Regards

Petr
>From 97f8c14b58f29cf3ce341ead29f17204faa60f3d Mon Sep 17 00:00:00 2001
From: Petr Cech 
Date: Mon, 5 Oct 2015 09:38:10 -0400
Subject: [PATCH 01/11] REFACTOR: umask(0177) --> umask(SSS_DFL_UMASK)

There are many calls of umask function with 0177 argument. This patch
add new constant SSS_DFL_UMASK which stands for 0177. So all occurences
of umask(0177) (except responder code) are replaced by constant
SSS_DFL_UMASK.

Resolves:
https://fedorahosted.org/sssd/ticket/2424
---
 src/confdb/confdb.c | 2 +-
 src/util/debug.c| 2 +-
 src/util/server.c   | 5 ++---
 src/util/util.h | 3 +++
 4 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/src/confdb/confdb.c b/src/confdb/confdb.c
index d811f7cbf597db5c5ee5fa658c8864233da8f2e0..0f76a3d140ec832467c8382df088ac0e279207c0 100644
--- a/src/confdb/confdb.c
+++ b/src/confdb/confdb.c
@@ -659,7 +659,7 @@ int confdb_init(TALLOC_CTX *mem_ctx,
 return EIO;
 }
 
-old_umask = umask(0177);
+old_umask = umask(SSS_DFL_UMASK);
 
 ret = ldb_connect(cdb->ldb, confdb_location, 0, NULL);
 umask(old_umask);
diff --git a/src/util/debug.c b/src/util/debug.c
index 69df54386101973548108c3194a1bfd111f046f0..bd13fdecdbd37da8e13ed492c115570657d2588c 100644
--- a/src/util/debug.c
+++ b/src/util/debug.c
@@ -362,7 +362,7 @@ int open_debug_file_ex(const char *filename, FILE **filep, bool want_cloexec)
 
 if (debug_file && !filep) fclose(debug_file);
 
-old_umask = umask(0177);
+old_umask = umask(SSS_DFL_UMASK);
 errno = 0;
 f = fopen(logpath, "a");
 if (f == NULL) {
diff --git a/src/util/server.c b/src/util/server.c
index 7e9b76f74ee5e76d2481eb425eff4811cc2e780e..036dace044c1e2c3efbb2411f39bdfd3f9616db4 100644
--- a/src/util/server.c
+++ b/src/util/server.c
@@ -490,9 +490,8 @@ int server_setup(const char *name, int flags,
 
 setup_signals();
 
-/* we want default permissions on created files to be very strict,
-   so set our umask to 0177 */
-umask(0177);
+/* we want default permissions on created files to be very strict */
+umask(SSS_DFL_UMASK);
 
 if (flags & FLAGS_DAEMON) {
 DEBUG(SSSDBG_IMPORTANT_INFO, "Becoming a daemon.\n");
diff --git a/src/util/util.h b/src/util/util.h
index f9fe1ca7189c6b2cdcb29f143005b20a2d969fee..9658d79fe9a0062b46188f2e7a97aaaebdeff29e 100644
--- a/src/util/util.h
+++ b/src/util/util.h
@@ -64,6 +64,9 @@
 #define SSS_ATTRIBUTE_PRINTF(a1, a2)
 #endif
 
+/** Default secure umask */
+#define SSS_DFL_UMASK 0177
+
 extern const char *debug_prg_name;
 extern int debug_level;
 extern int debug_timestamps;
-- 
2.4.3

>From eab27ab030d0efe44ae25e2313bbee40db5cc9d4 Mon Sep 17 00:00:00 2001
From: Petr Cech 
Date: Mon, 5 Oct 2015 09:51:20 -0400
Subject: [PATCH 02/11] REFACTOR: DFL_RSP_UMASK constant in responder code

There is DFL_RSP_UMASK constant for very secure umask in responder
code. This patch replaces occurances of value 0177 with this constant.

Resolves:
https://fedorahosted.org/sssd/ticket/2424
---
 src/responder/common/responder_common.c | 3 ++-
 src/responder/pam/pamsrv.c  | 3 ++-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/src/responder/common/responder_common.c b/src/responder/common/responder_common.c
index 2097004cb0fc24d8b356f9d924243f948227ef58..baaf0412b4a70537a2523a98ff33d8f34f194b47 100644
--- a/src/responder/common/responder_common.c
+++ b/src/responder/common/responder_common.c
@@ -690,7 +690,8 @@ static int set_unix_socket(struct resp_ctx *rctx)
 if (rctx->priv_sock_name != NULL ) {
 /* create privileged pipe */
 if (rctx->priv_lfd == -1) {
-ret = create_pipe_fd(rctx->priv_sock_name, >priv_lfd, 0177);
+ret = create_pipe_fd(rctx->priv_sock_name, >priv_lfd,
+ DFL_RSP_UMASK);
 if (ret != EOK) {
 goto failed;
 }
diff --git a/src/responder/pam/pamsrv.c b/src/responder/pam/pamsrv.c
index 3fe467c3cfc4c63b9c261065a17a54c20ea4a546..6ac770b7ac80676824cd572444359b96279902f7 100644
--- a/src/responder/pam/pamsrv.c
+++ b/src/responder/pam/pamsrv.c
@@ -396,7 +396,8 @@ int main(int argc, const char *argv[])
 return 2;
 }
 
-ret = 

Re: [SSSD] [PATCH] cache_req: support UPN

[Sumit Bose]

On Wed, Oct 07, 2015 at 04:22:31PM +0200, Sumit Bose wrote:
> On Wed, Oct 07, 2015 at 02:42:18PM +0200, Pavel Březina wrote:
> > On 10/06/2015 03:16 PM, Pavel Březina wrote:
> > >On 10/01/2015 05:06 PM, Sumit Bose wrote:
> > >>On Thu, Sep 17, 2015 at 11:06:30AM +0200, Pavel Březina wrote:
> > >>>On 09/17/2015 10:32 AM, Petr Cech wrote:
> > Hi Pavel!
> > 
> > There is some code between my last end and this continuation. I was
> > read
> > it and did't find anything wrong.
> > >>>
> > >>>Hi Petr,
> > >>>thank you for you review. New patches are attached. All comments from
> > >>>the
> > >>>previous mail should be addressed.
> > >>>
> > >>
> > >>
> > >>[PATCH 2/3] cache_req: add support for UPN
> > >>
> > >>>diff --git a/src/db/sysdb_search.c b/src/db/sysdb_search.c
> > >>>index
> > >>>5f33b225a31e1c1add7f776215d500fb09e127ca..a72a8bac594dc10ff9aa337399f27d9936830a8e
> > >>>100644
> > >>>--- a/src/db/sysdb_search.c
> > >>>+++ b/src/db/sysdb_search.c
> > >>>@@ -295,6 +295,59 @@ static char *enum_filter(TALLOC_CTX *mem_ctx,
> > >>>  return filter;
> > >>>  }
> > >>>
> > >>>+int sysdb_getpwupn(TALLOC_CTX *mem_ctx,
> > >>>+   struct sss_domain_info *domain,
> > >>>+   const char *upn,
> > >>>+   struct ldb_result **_res)
> > >>>+{
> > >>>+TALLOC_CTX *tmp_ctx;
> > >>>+struct ldb_message *msg;
> > >>>+struct ldb_result *res;
> > >>>+static const char *attrs[] = SYSDB_PW_ATTRS;
> > >>>+errno_t ret;
> > >>>+
> > >>>+tmp_ctx = talloc_new(NULL);
> > >>>+if (tmp_ctx == NULL) {
> > >>>+DEBUG(SSSDBG_CRIT_FAILURE, "talloc_new() failed\n");
> > >>>+return ENOMEM;
> > >>>+}
> > >>>+
> > >>>+ret = sysdb_search_user_by_upn(tmp_ctx, domain, upn, attrs, );
> > >>>+if (ret != EOK && ret != ENOENT) {
> > >>>+DEBUG(SSSDBG_OP_FAILURE, "sysdb_search_user_by_upn()
> > >>>failed.\n");
> > >>>+goto done;
> > >>>+}
> > >>
> > >>The call stack here would be
> > >>
> > >>sysdb_getpwupn()->sysdb_search_user_by_upn()->sysdb_search_entry()->ldb_search()
> > >>
> > >>
> > >>and the returned results, starting from ldb_search()
> > >>
> > >>ldb_result -> ldb_message (array) -> ldb_message -> ldb_result
> > >>
> > >>I think it would be easier and safe some allocations if it would be
> > >>
> > >>sysdb_search_user_by_upn()->sysdb_getpwupn()->ldb_search() .
> > >>
> > >>Do you think this change makes sense?
> > >
> > >Hi, thanks for the review. Done.
> > >
> > >Although I created a new function sysdb_search_user_by_upn_res since you
> > >can't pass attrs into sysdb_getpwnupn (unless we are inconsistent with
> > >api).
> > >
> > >>
> > >const char *orig_name;
> > >>
> > >>Instead of adding a new member which again will hold the originally
> > >>provided name I would suggest to refactor the change added by
> > >>e87b2a6e94c1066b3044fe683825ff5b4f8716c2 (cache_req: parse input name if
> > >>needed) by introducing a member called e.g. parsed_name which keeps the
> > >>name returned by sss_parse_inp_recv() and return it in cache_req_recv().
> > >>
> > >>In e87b2a6e94c1066b3044fe683825ff5b4f8716c2 it made sense to overwrite
> > >>orig_name because no additional member was needed. But now I think the
> > >>code would be more clear if orig_name will be kept unmodified.
> > >
> > >Done in separate patch so the changes are more visible. It can be
> > >squashed to the second patch.
> > 
> > Sumit found that test won't pass. New patchset is attached.
> > 
> > 
> 
> Thank you, the tests pass now. I've seen a compiler warning which is
> seen with the first patch I attached.
> 
> During testing I found that UPN lookups for sub-domain users do not
> work even with the original nss responder code. I attached fixes for
> your patches and the nss responder since the sysdb change depends on you
> patches.

and now with patches ...

> 
> bye,
> Sumit
> ___
> sssd-devel mailing list
> sssd-devel@lists.fedorahosted.org
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
From 617c1a39fab16d12f9a346073eaee4a8bee4ac08 Mon Sep 17 00:00:00 2001
From: Sumit Bose 
Date: Wed, 7 Oct 2015 16:11:56 +0200
Subject: [PATCH 1/3] fix ldb_search usage

---
 src/db/sysdb_ops.c | 9 +
 1 file changed, 1 insertion(+), 8 deletions(-)

diff --git a/src/db/sysdb_ops.c b/src/db/sysdb_ops.c
index 
5c086cb0a25b452a38ca717b2459c76fdb3f22ff..fafa0207cd068b22151f805a217e2cd55b357d17
 100644
--- a/src/db/sysdb_ops.c
+++ b/src/db/sysdb_ops.c
@@ -483,7 +483,6 @@ int sysdb_search_user_by_upn_res(TALLOC_CTX *mem_ctx,
 TALLOC_CTX *tmp_ctx;
 struct ldb_result *res;
 struct ldb_dn *base_dn;
-char *filter;
 int ret;
 const char *def_attrs[] = { SYSDB_NAME, SYSDB_UPN, SYSDB_CANONICAL_UPN,
 NULL };
@@ -500,15 +499,9 @@ int sysdb_search_user_by_upn_res(TALLOC_CTX *mem_ctx,
 goto done;
 }
 
-filter = 

Re: [SSSD] [PATCH] cache_req: support UPN

[Sumit Bose]

On Wed, Oct 07, 2015 at 02:42:18PM +0200, Pavel Březina wrote:
> On 10/06/2015 03:16 PM, Pavel Březina wrote:
> >On 10/01/2015 05:06 PM, Sumit Bose wrote:
> >>On Thu, Sep 17, 2015 at 11:06:30AM +0200, Pavel Březina wrote:
> >>>On 09/17/2015 10:32 AM, Petr Cech wrote:
> Hi Pavel!
> 
> There is some code between my last end and this continuation. I was
> read
> it and did't find anything wrong.
> >>>
> >>>Hi Petr,
> >>>thank you for you review. New patches are attached. All comments from
> >>>the
> >>>previous mail should be addressed.
> >>>
> >>
> >>
> >>[PATCH 2/3] cache_req: add support for UPN
> >>
> >>>diff --git a/src/db/sysdb_search.c b/src/db/sysdb_search.c
> >>>index
> >>>5f33b225a31e1c1add7f776215d500fb09e127ca..a72a8bac594dc10ff9aa337399f27d9936830a8e
> >>>100644
> >>>--- a/src/db/sysdb_search.c
> >>>+++ b/src/db/sysdb_search.c
> >>>@@ -295,6 +295,59 @@ static char *enum_filter(TALLOC_CTX *mem_ctx,
> >>>  return filter;
> >>>  }
> >>>
> >>>+int sysdb_getpwupn(TALLOC_CTX *mem_ctx,
> >>>+   struct sss_domain_info *domain,
> >>>+   const char *upn,
> >>>+   struct ldb_result **_res)
> >>>+{
> >>>+TALLOC_CTX *tmp_ctx;
> >>>+struct ldb_message *msg;
> >>>+struct ldb_result *res;
> >>>+static const char *attrs[] = SYSDB_PW_ATTRS;
> >>>+errno_t ret;
> >>>+
> >>>+tmp_ctx = talloc_new(NULL);
> >>>+if (tmp_ctx == NULL) {
> >>>+DEBUG(SSSDBG_CRIT_FAILURE, "talloc_new() failed\n");
> >>>+return ENOMEM;
> >>>+}
> >>>+
> >>>+ret = sysdb_search_user_by_upn(tmp_ctx, domain, upn, attrs, );
> >>>+if (ret != EOK && ret != ENOENT) {
> >>>+DEBUG(SSSDBG_OP_FAILURE, "sysdb_search_user_by_upn()
> >>>failed.\n");
> >>>+goto done;
> >>>+}
> >>
> >>The call stack here would be
> >>
> >>sysdb_getpwupn()->sysdb_search_user_by_upn()->sysdb_search_entry()->ldb_search()
> >>
> >>
> >>and the returned results, starting from ldb_search()
> >>
> >>ldb_result -> ldb_message (array) -> ldb_message -> ldb_result
> >>
> >>I think it would be easier and safe some allocations if it would be
> >>
> >>sysdb_search_user_by_upn()->sysdb_getpwupn()->ldb_search() .
> >>
> >>Do you think this change makes sense?
> >
> >Hi, thanks for the review. Done.
> >
> >Although I created a new function sysdb_search_user_by_upn_res since you
> >can't pass attrs into sysdb_getpwnupn (unless we are inconsistent with
> >api).
> >
> >>
> >const char *orig_name;
> >>
> >>Instead of adding a new member which again will hold the originally
> >>provided name I would suggest to refactor the change added by
> >>e87b2a6e94c1066b3044fe683825ff5b4f8716c2 (cache_req: parse input name if
> >>needed) by introducing a member called e.g. parsed_name which keeps the
> >>name returned by sss_parse_inp_recv() and return it in cache_req_recv().
> >>
> >>In e87b2a6e94c1066b3044fe683825ff5b4f8716c2 it made sense to overwrite
> >>orig_name because no additional member was needed. But now I think the
> >>code would be more clear if orig_name will be kept unmodified.
> >
> >Done in separate patch so the changes are more visible. It can be
> >squashed to the second patch.
> 
> Sumit found that test won't pass. New patchset is attached.
> 
> 

Thank you, the tests pass now. I've seen a compiler warning which is
seen with the first patch I attached.

During testing I found that UPN lookups for sub-domain users do not
work even with the original nss responder code. I attached fixes for
your patches and the nss responder since the sysdb change depends on you
patches.

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

Re: [SSSD] [PATCH v2] intg: Add more LDAP tests

[Nikolai Kondrashov]

On 09/30/2015 06:15 PM, Nikolai Kondrashov wrote:

Hi everyone,

Here is a patch set fixing some things in integration tests and adding more
LDAP tests:

 * Adding/removing a user/group/membership with rfc2307(bis) schema.
 * Filtering users/groups with rfc2307(bis) schema.
 * The effect of override_homedir option.
 * The effect of fallback_homedir option.
 * The effect of override_shell option.
 * The effect of shell_fallback option.
 * The effect of default_shell option.
 * The effect of vetoed_shells option.

These are pretty basic, but I think they're good for the start.
Suggestions for more tests are welcome :)

NOTE: These still break test_memory_cache.py as seen in the attached log file.
   We need to figure out why and do something with it. Otherwise, the
   tests work fine.


Here's another version of the patch set. It's not complete, but takes some
comments into account. Namely:

* Explicitly name the new arguments for ldap_ent.user and
  ldap_ent.List.add_user, instead of using "kwargs". Add "gecos" too. This
  makes the function more suitable for Pavel Reichl's needs.
* Don't remove "_rfc2307" from function names anywhere.
* Use a string "schema" argument with configuration formatting functions
  instead of boolean "bis" argument to support other schemas. Use
  constants to specify the values in invocations.
* Explicitly specify "enum" argument name when invoking configuration
  formatting functions.
* Remove duplicate "blank" fixture function.

I'll continue working on the patch set. Namely adding tests without
enumeration, looking for and fixing the memory cache test failures induced by
the new tests, trying to move commonly used fixtures and other functions to a
module so we don't copy them.

Nick
>From d79fd27165d08efe07b1dafe52898a01ac06a4e3 Mon Sep 17 00:00:00 2001
From: Nikolai Kondrashov 
Date: Tue, 29 Sep 2015 20:00:14 +0300
Subject: [PATCH 1/6] intg: Get base DN from LDAP connection object

Don't use the global LDAP_BASE_DN in integration tests and fixtures, but
instead take it from the LDAP connection object (ldap_conn) passed to
them explicitly. This makes the tests and fixtures a bit more modular.
---
 src/tests/intg/ldap_test.py | 8 
 src/tests/intg/test_memory_cache.py | 2 +-
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/src/tests/intg/ldap_test.py b/src/tests/intg/ldap_test.py
index ea114dc..0287a28 100644
--- a/src/tests/intg/ldap_test.py
+++ b/src/tests/intg/ldap_test.py
@@ -105,7 +105,7 @@ def create_sssd_fixture(request):
 
 @pytest.fixture
 def sanity_rfc2307(request, ldap_conn):
-ent_list = ldap_ent.List(LDAP_BASE_DN)
+ent_list = ldap_ent.List(ldap_conn.ds_inst.base_dn)
 ent_list.add_user("user1", 1001, 2001)
 ent_list.add_user("user2", 1002, 2002)
 ent_list.add_user("user3", 1003, 2003)
@@ -150,7 +150,7 @@ def sanity_rfc2307(request, ldap_conn):
 
 @pytest.fixture
 def simple_rfc2307(request, ldap_conn):
-ent_list = ldap_ent.List(LDAP_BASE_DN)
+ent_list = ldap_ent.List(ldap_conn.ds_inst.base_dn)
 ent_list.add_user('usr001', 181818, 181818)
 ent_list.add_group("group1", 181818)
 
@@ -181,7 +181,7 @@ def simple_rfc2307(request, ldap_conn):
 
 @pytest.fixture
 def sanity_rfc2307_bis(request, ldap_conn):
-ent_list = ldap_ent.List(LDAP_BASE_DN)
+ent_list = ldap_ent.List(ldap_conn.ds_inst.base_dn)
 ent_list.add_user("user1", 1001, 2001)
 ent_list.add_user("user2", 1002, 2002)
 ent_list.add_user("user3", 1003, 2003)
@@ -309,7 +309,7 @@ def test_sanity_rfc2307_bis(ldap_conn, sanity_rfc2307_bis):
 
 @pytest.fixture
 def refresh_after_cleanup_task(request, ldap_conn):
-ent_list = ldap_ent.List(LDAP_BASE_DN)
+ent_list = ldap_ent.List(ldap_conn.ds_inst.base_dn)
 ent_list.add_user("user1", 1001, 2001)
 
 ent_list.add_group_bis("group1", 2001, ["user1"])
diff --git a/src/tests/intg/test_memory_cache.py b/src/tests/intg/test_memory_cache.py
index 1f4ccb0..76d85fd 100644
--- a/src/tests/intg/test_memory_cache.py
+++ b/src/tests/intg/test_memory_cache.py
@@ -109,7 +109,7 @@ def create_sssd_fixture(request):
 
 
 def load_data_to_ldap(request, ldap_conn):
-ent_list = ldap_ent.List(LDAP_BASE_DN)
+ent_list = ldap_ent.List(ldap_conn.ds_inst.base_dn)
 ent_list.add_user("user1", 1001, 2001)
 ent_list.add_user("user2", 1002, 2002)
 ent_list.add_user("user3", 1003, 2003)
-- 
2.5.1

>From fb6f62e692c6acb03e046bb9dccce5b0fedb8265 Mon Sep 17 00:00:00 2001
From: Nikolai Kondrashov 
Date: Mon, 28 Sep 2015 16:12:48 +0300
Subject: [PATCH 2/6] intg: Add support for specifying all user attrs

Support passing all user attributes to ldap_ent.py's user-creation
functions, in integration tests.
---
 src/tests/intg/ldap_ent.py | 45 -
 1 file changed, 36 insertions(+), 9 deletions(-)

diff --git 

Re: [SSSD] More upstream CI tests

[Lukas Slebodnik]

On (31/08/15 12:31), Jakub Hrozek wrote:
>On Fri, Aug 28, 2015 at 03:16:57PM +0200, Lukas Slebodnik wrote:
>> On (19/08/15 23:38), Jakub Hrozek wrote:
>> >Hi,
>> >
>> >as we're stabilizing the 1.13 branch and before we plan what we want to
>> >work on during the 1.14 development, we should use that time to write
>> >some more tests!
>> >
>> >Here are some areas where we could add tests. Please discuss or add your
>> >ideas, I would like to turn this list into tickets we can start
>> >implementing:
>> >
>> >* Extend the LDAP provider tests with more dynamic test cases.
>> >- add a user to a group, run sss_cache, assert id user displays the
>> >  new group and getent group displays the new member
>> >- conversely with removing users from groups
There are some tests on list, but still it wuld be good to se a test plan.

>> >* Background refresh
>> >- could be built atop the LDAP NSS tests as well. I think we have
>> >  all the infrastructure in place.
>> >* Local overrides integration test:
>> >- this could be relatively easy, just call the overrides tool and
>> >  request the entry. Could be built atop the existing LDAP tests
>> >  or even use the local provider.
>> Firstly, we need to prepare test cases for this feature.
>> We can inspire in downstream ipa-views test case.
>> After review of test cases we can split it to several tickets.
>
>We already do:
>https://fedorahosted.org/sssd/ticket/2732
>
>It 'just' needs an assignee..
IMHo this should be a very low priority ticket. It's already tested by
downstream and they will have automated tests. We should focus to other parts
which cannot be easily tested (whithout greping log files or touching/checking
 internals in sysdb)
https://fedorahosted.org/sssd/ticket/2697

BTW if the tests were ready together with feature it would be something
different. It would not be a duplicated effort.

If we really want to duplicate effort then it would be better to see/review
a test plan for local views. Transforming to pytest should be a trivial task.
Should I file a ticket?

>>
>> >* Add a KDC
>> >- until we have a pam_wrapper, this would only be useful to test
>> >  ldap_child, but adding the KDC instantiation might be worth it
>> >  nonetheless
>> >- there is a protorype of KDC instantiation on the list for some
>> >  time now, since we enabled rootless SSSD
https://fedorahosted.org/sssd/ticket/2822
>> >* IFP - could we reuse the existing sbus tests to spawn a custom bus?
>> Someone need to look into running dbus-daemon in cwrap env.
>> 
>> Then we need prepare test cases ...
>
>Well, the first testcase could be to run introspection -- that would
>validate the bus is accessible.
>
>It could be the first ticket, if you agree, I can file a ticket and
>assign it?
>
cwrap test for IFP should have the highest priority because it's not tested
by downstream.

https://fedorahosted.org/sssd/ticket/2823
https://fedorahosted.org/sssd/ticket/2824
https://fedorahosted.org/sssd/ticket/2825

The first two tickets can be done in parallel.

>> 
>> >* SUDO - can we trick sudo into connecting to our test sssd instance?
>> >
I thought it could be tested using pam_wrapper
I filed a generat ticket for authentication + authorisation
https://fedorahosted.org/sssd/ticket/2821

It can be split into testing pam provider and sudo provider.

>> >I think the order of priority is roughly as above. I think the LDAP
>> >provider is critical enough to be well tested. The refresh and local
>> >override tests might be nice to have because we would be refactoring the
>> >NSS responder in 1.14, so we should have it tested.
>> >
>> >I'll be happy to hear other opionions, though!
>> We can try to prepare fake AD in openldap (if possible)
>> and test basic functionality with ldap provider.
>
>I would like to talk to Andreas next week and ask about Samba. I hope
>this would be much easier and more robust.
>
Just for the record, here is a link to samba4-dc tracking ticket.
https://fedorahosted.org/sssd/ticket/2818

But I think we will need to test with SSL or TLS, so here is another ticket
https://fedorahosted.org/sssd/ticket/2820

Did I miss something?

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

Re: [SSSD] [PATCHSET] intg: sss_override

[Nikolai Kondrashov]

On 10/07/2015 03:25 PM, Pavel Reichl wrote:

Hello,

please see first version of patch set. Please consider this to be work in 
progress.

The test 'test_local_override_group' fails, there seem to be a bug in 
sss_override. Pavel is working on the patch. He kindly provided me with early 
version of patch and it fixed the test.

Currently also test for #2790 test_local_overrides_regr_2790() is failing which 
is probably just bug in the test itself.

Thanks for any comments.


Yay, more tests :)! I had a quick look and left some comments below.


0001-intg-Support-users-with-arb.-gecos-homedir-shell.patch


 From f8068960a2d393ca180ac40687620bbb4deb003c Mon Sep 17 00:00:00 2001
From: Pavel Reichl
Date: Mon, 5 Oct 2015 10:00:41 -0400
Subject: [PATCH 1/5] intg: Support users with arb. gecos, homedir, shell

Add support for setting arbitrary values of user attribute: gecos,
homedir and shell.


I have a similar patch in my "intg: Add more LDAP tests" patch set:
https://lists.fedorahosted.org/pipermail/sssd-devel/2015-September/025045.html

We can use either mine (after modifications) or yours.


Resolves:
https://fedorahosted.org/sssd/ticket/2732
---
  src/tests/intg/ldap_ent.py | 18 +++---
  1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/src/tests/intg/ldap_ent.py b/src/tests/intg/ldap_ent.py
index 
30eed9d6426f47401712fd88dee7f11bd747b944..4f1e259cd707356dc6c7e819bbd0fc5195f1f1a9
 100644
--- a/src/tests/intg/ldap_ent.py
+++ b/src/tests/intg/ldap_ent.py
@@ -18,13 +18,13 @@
  #


-def user(base_dn, uid, uidNumber, gidNumber):
+def user(base_dn, uid, uidNumber, gidNumber, gecos, homedir, shell):


I'm going a kwargs approach, but I like yours better. However, could you
please keep the argument names matching actual LDAP attribute names?
I.e. use "homeDirectory" and "loginShell" instead. Could you assign
defaults to these arguments here as well? Sometimes it can be useful to use
this function directly. E.g. when creating just one user, as I do in my patch
set.

Also, will we ever need to specify "cn", or "sn" as well?


  """
  Generate an RFC2307(bis) user add-modlist for passing to ldap.add*
  """
  uidNumber = str(uidNumber)
  gidNumber = str(gidNumber)
-return (
+attrs = (
  "uid=" + uid + ",ou=Users," + base_dn,
  [
  ('objectClass', ['top', 'inetOrgPerson', 'posixAccount']),
@@ -33,11 +33,15 @@ def user(base_dn, uid, uidNumber, gidNumber):
  ('uidNumber',   [uidNumber]),
  ('gidNumber',   [gidNumber]),
  ('userPassword',['Password' + uidNumber]),
-('homeDirectory',   ['/home/' + uid]),
-('loginShell',  ['/bin/bash']),
+('homeDirectory',   [homedir if homedir else '/home/' + uid]),
+('loginShell',  [shell if shell else '/bin/bash']),
  ]
  )

+if gecos:
+attrs[1].append(('gecos', [gecos]))
+return attrs
+


Just checking a string for truth wouldn't be sufficient, because an empty
string is False and we might need to pass it sometimes. So, I think, we need
to test against None here and above.



  def group(base_dn, cn, gidNumber, member_uids=[]):
  """
@@ -85,11 +89,11 @@ class List(list):
  def __init__(self, base_dn):
  self.base_dn = base_dn

-def add_user(self, uid, uidNumber, gidNumber,
- base_dn=None):
+def add_user(self, uid, uidNumber, gidNumber, gecos=None,
+ homedir=None, shell=None, base_dn=None):
  """Add an RFC2307(bis) user add-modlist."""
  self.append(user(base_dn or self.base_dn,
- uid, uidNumber, gidNumber))
+ uid, uidNumber, gidNumber, gecos, homedir, shell))

  def add_group(self, cn, gidNumber, member_uids=[],
base_dn=None):
-- 2.4.3


0002-intg-new-test-for-user-local-overrides.patch


 From e4028b9a81c498a20de5c12a1f0a788be7608463 Mon Sep 17 00:00:00 2001
From: Pavel Reichl
Date: Mon, 5 Oct 2015 10:02:05 -0400
Subject: [PATCH 2/5] intg: new test for user local overrides

Introduce a new integration test for local view overrides.

Resolves:
https://fedorahosted.org/sssd/ticket/2732
---
  src/tests/intg/ldap_local_override_test.py | 376 +
  1 file changed, 376 insertions(+)
  create mode 100644 src/tests/intg/ldap_local_override_test.py

diff --git a/src/tests/intg/ldap_local_override_test.py 
b/src/tests/intg/ldap_local_override_test.py
new file mode 100644
index 
..35e47b012fdd271dd50b4c2ce3749d28a7a746d8
--- /dev/null
+++ b/src/tests/intg/ldap_local_override_test.py
@@ -0,0 +1,376 @@
+#
+# LDAP integration test


Could you please put a more specific description here?


+#
+# Copyright (c) 2015 Red Hat, Inc.
+# Author: Pavel Reichl
+#
+# This is free software; you can redistribute it and/or modify it

Re: [SSSD] More upstream CI tests

[Pavel Reichl]

On 10/07/2015 08:58 AM, Lukas Slebodnik wrote:

On (31/08/15 12:31), Jakub Hrozek wrote:

On Fri, Aug 28, 2015 at 03:16:57PM +0200, Lukas Slebodnik wrote:

On (19/08/15 23:38), Jakub Hrozek wrote:

Hi,

as we're stabilizing the 1.13 branch and before we plan what we want to
work on during the 1.14 development, we should use that time to write
some more tests!

Here are some areas where we could add tests. Please discuss or add your
ideas, I would like to turn this list into tickets we can start
implementing:

* Extend the LDAP provider tests with more dynamic test cases.
- add a user to a group, run sss_cache, assert id user displays the
  new group and getent group displays the new member
- conversely with removing users from groups

There are some tests on list, but still it wuld be good to se a test plan.


* Background refresh
- could be built atop the LDAP NSS tests as well. I think we have
  all the infrastructure in place.
* Local overrides integration test:
- this could be relatively easy, just call the overrides tool and
  request the entry. Could be built atop the existing LDAP tests
  or even use the local provider.

Firstly, we need to prepare test cases for this feature.
We can inspire in downstream ipa-views test case.
After review of test cases we can split it to several tickets.


We already do:
https://fedorahosted.org/sssd/ticket/2732

It 'just' needs an assignee..

IMHo this should be a very low priority ticket. It's already tested by
downstream and they will have automated tests. We should focus to other parts


Well our integration tests already found at least one real nasty bug in 
sss_override so I suppose that it's only good that the priority was higher and 
bug was found by us and not customer.


which cannot be easily tested (whithout greping log files or touching/checking
  internals in sysdb)
https://fedorahosted.org/sssd/ticket/2697

BTW if the tests were ready together with feature it would be something
different. It would not be a duplicated effort.
I'm not sure what you mean by duplicate effort but if you are referring to the fact that intg. tests were not prepared by the author of the feature then I have to disagree. I have found 2 documentation issues which would hardly by found by the author 
as he already know how the feature works. I believe that documentation issues are high priority as one of our major goals is to make SSSD more user friendly.




If we really want to duplicate effort then it would be better to see/review
a test plan for local views. Transforming to pytest should be a trivial task.
Should I file a ticket?




* Add a KDC
- until we have a pam_wrapper, this would only be useful to test
  ldap_child, but adding the KDC instantiation might be worth it
  nonetheless
- there is a protorype of KDC instantiation on the list for some
  time now, since we enabled rootless SSSD

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

* IFP - could we reuse the existing sbus tests to spawn a custom bus?

Someone need to look into running dbus-daemon in cwrap env.

Then we need prepare test cases ...


Well, the first testcase could be to run introspection -- that would
validate the bus is accessible.

It could be the first ticket, if you agree, I can file a ticket and
assign it?


cwrap test for IFP should have the highest priority because it's not tested
by downstream.

https://fedorahosted.org/sssd/ticket/2823
https://fedorahosted.org/sssd/ticket/2824
https://fedorahosted.org/sssd/ticket/2825

The first two tickets can be done in parallel.




* SUDO - can we trick sudo into connecting to our test sssd instance?


I thought it could be tested using pam_wrapper
I filed a generat ticket for authentication + authorisation
https://fedorahosted.org/sssd/ticket/2821

It can be split into testing pam provider and sudo provider.


I think the order of priority is roughly as above. I think the LDAP
provider is critical enough to be well tested. The refresh and local
override tests might be nice to have because we would be refactoring the
NSS responder in 1.14, so we should have it tested.

I'll be happy to hear other opionions, though!

We can try to prepare fake AD in openldap (if possible)
and test basic functionality with ldap provider.


I would like to talk to Andreas next week and ask about Samba. I hope
this would be much easier and more robust.


Just for the record, here is a link to samba4-dc tracking ticket.
https://fedorahosted.org/sssd/ticket/2818

But I think we will need to test with SSL or TLS, so here is another ticket
https://fedorahosted.org/sssd/ticket/2820

Did I miss something?

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


___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org

Re: [SSSD] [PATCHSET] intg: sss_override

[Nikolai Kondrashov]

On 10/07/2015 07:10 PM, Pavel Reichl wrote:

We can use either mine (after modifications) or yours.


I suppose yours are closer to be acked so we should probably use yours.


I'm not so sure, but alright :)


All valid comments, but I will ignore them for now as I suppose this patch
will be superseded by yours.


Alright.


Just checking a string for truth wouldn't be sufficient, because an empty
string is False and we might need to pass it sometimes. So, I think, we need
to test against None here and above.


Thanks, my python skills got a bit rusty I suppose :-)


Mine were never sharp, but I'm generally vary of a language's concept of
truth (seen many strange ones), so I checked :)


I really don't like how we keep copying this stuff around. I'll look into
making a module with these.


OK, should I postpone amending the patch until your patch is in master or
should I do it as part of this patch set?


I think it will be more efficient for you to keep working on your patch as is,
in this part, and if I manage to produce something, then we can resync.


Could you please add descriptions for all functions, especially these two?


Sure. I wasn't sure about those functions. Intention was to verify that
state of overrides is the same after sequence of commands - for example just
before we call 'sss_override user-export' and then after we call
'sss_override user-import'. I wanted to avoid copy approach. But
wasn't sure how to find out a descriptive name. Would something like
check_user_override_state_1() be better name?


I can't really tell, since I haven't dug into what the tests actually do yet.
So far I'm just reacting to somewhat generically-named functions without
descriptions. I'm reserving the functional review until it's easier :)

Thanks for taking my comments into account :)!

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