date:20151113

Re: [SSSD] [PATCH] test_sysdb_subdomains: Do use assignment in assertions

2015-11-13 Thread Lukas Slebodnik

On (13/11/15 11:12), Jakub Hrozek wrote:
>On Fri, Nov 13, 2015 at 08:51:17AM +0100, Lukas Slebodnik wrote:
>> ehlo,
>> 
>> I found these iisues due to different size of integer and pointer
>> on el6.
>> 
>> /home/build/sssd/src/tests/cmocka/test_sysdb_subdomains.c:248: error: cast 
>> from
>> pointer to integer of different size
>> 
>> Simple patch is attached.
>> 
>> LS
>
>Code-wise ACK, waiting for CI results. But I think the commit message is
>incorrect, I would like to reword it to "Don't use.." before pushing.
  ^^^
  of course.
I'm sorry for the mistake

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

[SSSD] 1.13.2 release notes

2015-11-13 Thread Jakub Hrozek

Hi,

I prepared release notes page for the 1.13.2 release:
https://fedorahosted.org/sssd/wiki/Releases/Notes-1.13.2

Comments or edits are welcome!
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

Re: [SSSD] [PATCH] TEST: recent_valid filter testing

2015-11-13 Thread Jakub Hrozek

On Fri, Nov 13, 2015 at 12:00:36PM +0100, Lukas Slebodnik wrote:
> On (13/11/15 11:32), Jakub Hrozek wrote:
> >On Fri, Nov 13, 2015 at 10:52:08AM +0100, Petr Cech wrote:
> >> On 11/13/2015 10:30 AM, Petr Cech wrote:
> >> >On 11/13/2015 10:27 AM, Petr Cech wrote:
> >> >>
> >> >>Patches are rebased now. I hope it will be ok now.
> >> >>
> >> >>Petr
> >> >Sorry, now my local CI tests failed... I will rebase it again.
> >> 
> >> Well, now it is right. Local CI tests passed. There has been patch:
> >> 
> >>   "TESTS: Fix warnings -Wshadow":
> >>   commit df9e9a1f9b7dc255eb62c390163c25917b08f5a2
> >>   Refs: sssd-1_13_1-137-gdf9e9a1
> >>   Author: Lukas Slebodnik 
> >>   AuthorDate: Mon Nov 9 10:59:55 2015 +0100
> >>   Commit: Jakub Hrozek 
> >>   CommitDate: Tue Nov 10 15:34:41 2015 +0100
> >> 
> >> There is change
> >> # - time_t time)
> >> # + time_t transaction_time)
> >> in static void prepare_user().
> >> My patches were in conflict with it.
> >> 
> >> Regards
> >> 
> >> Petr
> >
> >> From 3ce6073dda27fd7a4626f5cbac1c765274ca5fe0 Mon Sep 17 00:00:00 2001
> >> From: Petr Cech 
> >> Date: Fri, 2 Oct 2015 07:34:08 -0400
> >> Subject: [PATCH 1/8] TEST: Add test_user_by_recent_filter_valid
> >> 
> >> Test users_by_filter_valid() was removed in past. We will add two new
> >> tests instead of it. Logic of those tests is connected to RECENT
> >> filter. It returns only records which have been wrote or updated after
> >> filter was created (or another given time).
> >> 
> >> users_by_filter_valid() --> user_by_recent_filter_valid()
> >> users_by_recent_filter_valid()
> >> 
> >> The first of new tests, user_by_recent_filter_valid(), counts with two
> >> users. One is stored before filter request creation and the second user
> >> is stored after filter request creation. So filter returns only one
> >> user.
> >> 
> >> The second of new tests, users_by_recent_filter_valid(), counts with
> >> three users. One is stored before filter request creation and two users
> >> are stored after filter request creation. So filter returns two users.
> >> 
> >> This patch adds user_by_recent_filter_valid().
> >> 
> >> Resolves:
> >> https://fedorahosted.org/sssd/ticket/2730
> >> ---
> >>  src/tests/cmocka/test_responder_cache_req.c | 50 
> >> +
> >>  1 file changed, 50 insertions(+)
> >> 
> >> diff --git a/src/tests/cmocka/test_responder_cache_req.c 
> >> b/src/tests/cmocka/test_responder_cache_req.c
> >> index 
> >> 85d986bd7d159dc238bce4bc770272d18288f2dd..14a40ae6e56b2f6d0b18608bac09bc4680245153
> >>  100644
> >> --- a/src/tests/cmocka/test_responder_cache_req.c
> >> +++ b/src/tests/cmocka/test_responder_cache_req.c
> >> @@ -1239,6 +1239,53 @@ static void 
> >> cache_req_user_by_filter_test_done(struct tevent_req *req)
> >>  ctx->tctx->done = true;
> >>  }
> >>  
> >> +void test_user_by_recent_filter_valid(void **state)
> >> +{
> >> +struct cache_req_test_ctx *test_ctx = NULL;
> >> +TALLOC_CTX *req_mem_ctx = NULL;
> >> +struct tevent_req *req = NULL;
> >> +const char *ldbname = NULL;
> >> +errno_t ret;
> >> +
> >> +test_ctx = talloc_get_type_abort(*state, struct cache_req_test_ctx);
> >> +test_ctx->create_user = true;
> >> +
> >> +ret = sysdb_store_user(test_ctx->tctx->dom, TEST_USER_NAME2,
> >> +   "pwd", 1001, 1001, NULL, NULL, NULL,
> >> +   "cn="TEST_USER_NAME2",dc=test",
> >> +   NULL, NULL, 1000, time(NULL)-1);
> >> +assert_int_equal(ret, EOK);
> >> +
> >> +req_mem_ctx = talloc_new(test_ctx->tctx);
> >> +check_leaks_push(req_mem_ctx);
> >
> >I think the last question is whether we want to use this new context or
> >just call check_leaks_push(test_ctx) recursively. I don't really mind
> >too much, both would work for me.
> >
> >Unless someone opposes, I would push the patch as-is.
> >
> I have a different question. (i haven't read patches yet)
> But I can see that check_leaks_push is called after sysdb_store_user.
> 
> I would like to know why.
> because we shout try to check leaks "caused" in this function.

Wouldn't these leaks be caught by leaks checks that are pushed in
setup() and popped in teardown() ?
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

Re: [SSSD] [PATCH] TEST: recent_valid filter testing

2015-11-13 Thread Petr Cech


On 11/13/2015 11:32 AM, Jakub Hrozek wrote:

On Fri, Nov 13, 2015 at 10:52:08AM +0100, Petr Cech wrote:

>On 11/13/2015 10:30 AM, Petr Cech wrote:

> >On 11/13/2015 10:27 AM, Petr Cech wrote:

> >>
> >>Patches are rebased now. I hope it will be ok now.
> >>
> >>Petr

> >Sorry, now my local CI tests failed... I will rebase it again.

>
>Well, now it is right. Local CI tests passed. There has been patch:
>
>   "TESTS: Fix warnings -Wshadow":
>   commit df9e9a1f9b7dc255eb62c390163c25917b08f5a2
>   Refs: sssd-1_13_1-137-gdf9e9a1
>   Author: Lukas Slebodnik
>   AuthorDate: Mon Nov 9 10:59:55 2015 +0100
>   Commit: Jakub Hrozek
>   CommitDate: Tue Nov 10 15:34:41 2015 +0100
>
>There is change
># - time_t time)
># + time_t transaction_time)
>in static void prepare_user().
>My patches were in conflict with it.
>
>Regards
>
>Petr
> From 3ce6073dda27fd7a4626f5cbac1c765274ca5fe0 Mon Sep 17 00:00:00 2001
>From: Petr Cech
>Date: Fri, 2 Oct 2015 07:34:08 -0400
>Subject: [PATCH 1/8] TEST: Add test_user_by_recent_filter_valid
>
>Test users_by_filter_valid() was removed in past. We will add two new
>tests instead of it. Logic of those tests is connected to RECENT
>filter. It returns only records which have been wrote or updated after
>filter was created (or another given time).
>
>users_by_filter_valid() --> user_by_recent_filter_valid()
> users_by_recent_filter_valid()
>
>The first of new tests, user_by_recent_filter_valid(), counts with two
>users. One is stored before filter request creation and the second user
>is stored after filter request creation. So filter returns only one
>user.
>
>The second of new tests, users_by_recent_filter_valid(), counts with
>three users. One is stored before filter request creation and two users
>are stored after filter request creation. So filter returns two users.
>
>This patch adds user_by_recent_filter_valid().
>
>Resolves:
>https://fedorahosted.org/sssd/ticket/2730
>---
>  src/tests/cmocka/test_responder_cache_req.c | 50 
+
>  1 file changed, 50 insertions(+)
>
>diff --git a/src/tests/cmocka/test_responder_cache_req.c 
b/src/tests/cmocka/test_responder_cache_req.c
>index 
85d986bd7d159dc238bce4bc770272d18288f2dd..14a40ae6e56b2f6d0b18608bac09bc4680245153 
100644
>--- a/src/tests/cmocka/test_responder_cache_req.c
>+++ b/src/tests/cmocka/test_responder_cache_req.c
>@@ -1239,6 +1239,53 @@ static void cache_req_user_by_filter_test_done(struct 
tevent_req *req)
>  ctx->tctx->done = true;
>  }
>
>+void test_user_by_recent_filter_valid(void **state)
>+{
>+struct cache_req_test_ctx *test_ctx = NULL;
>+TALLOC_CTX *req_mem_ctx = NULL;
>+struct tevent_req *req = NULL;
>+const char *ldbname = NULL;
>+errno_t ret;
>+
>+test_ctx = talloc_get_type_abort(*state, struct cache_req_test_ctx);
>+test_ctx->create_user = true;
>+
>+ret = sysdb_store_user(test_ctx->tctx->dom, TEST_USER_NAME2,
>+   "pwd", 1001, 1001, NULL, NULL, NULL,
>+   "cn="TEST_USER_NAME2",dc=test",
>+   NULL, NULL, 1000, time(NULL)-1);
>+assert_int_equal(ret, EOK);
>+
>+req_mem_ctx = talloc_new(test_ctx->tctx);
>+check_leaks_push(req_mem_ctx);

I think the last question is whether we want to use this new context or
just call check_leaks_push(test_ctx) recursively. I don't really mind
too much, both would work for me.

Unless someone opposes, I would push the patch as-is.

OK.




>+
>+/* Filters always go to DP */
>+will_return(__wrap_sss_dp_get_account_send, test_ctx);
>+mock_account_recv_simple();
>+
>+/* User TEST_USER is created with a DP callback. */
>+req = cache_req_user_by_filter_send(req_mem_ctx, test_ctx->tctx->ev,
>+test_ctx->rctx,
>+test_ctx->tctx->dom->name,
>+"test*");
>+assert_non_null(req);
> From df9717ca932f95f55b528024829758dd9b2f2f56 Mon Sep 17 00:00:00 2001
>From: Petr Cech
>Date: Wed, 4 Nov 2015 06:50:33 -0500
>Subject: [PATCH 2/8] TEST: Refactor of test_responder_cache_req.c
>
>This patch only defines constant TEST_USER_FILTER. So code will be more


TEST_USER_PREFIX is defined.

Fixed.


The code is fine.



> From ae448cc95f9ab9fbca3aba5107bb964caf8250ec Mon Sep 17 00:00:00 2001
>From: Petr Cech
>Date: Tue, 27 Oct 2015 03:53:18 -0400
>Subject: [PATCH 3/8] TEST: Refactor of test_responder_cache_req.c
>
>We need little more in background of responder_cache_req tests. There
>will be tests which will use three test users. This patch add support
>for it.

ACK

Thanks.




> From 943d828ec283284269f954b3044292fb491cf5fa Mon Sep 17 00:00:00 2001
>From: Petr

Re: [SSSD] [PATCH] TEST: recent_valid filter testing

2015-11-13 Thread Petr Cech

On 11/13/2015 12:27 PM, Jakub Hrozek wrote:

+req_mem_ctx = talloc_new(test_ctx->tctx);
> >>+check_leaks_push(req_mem_ctx);

> >
> >I think the last question is whether we want to use this new context or
> >just call check_leaks_push(test_ctx) recursively. I don't really mind
> >too much, both would work for me.
> >
> >Unless someone opposes, I would push the patch as-is.
> >

>I have a different question. (i haven't read patches yet)
>But I can see that check_leaks_push is called after sysdb_store_user.
>
>I would like to know why.
>because we shout try to check leaks "caused" in this function.

Wouldn't these leaks be caught by leaks checks that are pushed in
setup() and popped in teardown() ?

I found out that we use only this expression in test code:

req_mem_ctx = talloc_new(global_talloc_context);
check_leaks_push(req_mem_ctx);

So it is possible that I added this check in vain.
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

Re: [SSSD] [PATCH] SSSD: Add a new command diag_cmd

2015-11-13 Thread Petr Cech

On 11/13/2015 08:20 AM, Petr Cech wrote:

>Hi Jakub,
>
>it works due to your reproducer. It is really need to have
>setenforce == 1

You meant setenforce 0, right?

Hi Jakub,

yes, of course, I meant setenforce 1. It was mistake.

   --^-- 1 --> 0
I did little mistake again.
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

Re: [SSSD] [PATCH] TEST: recent_valid filter testing

2015-11-13 Thread Jakub Hrozek

On Fri, Nov 13, 2015 at 08:32:55AM +0100, Petr Cech wrote:
> bump

Hi, patch 003 doesn't apply cleanly for me, can you rebase?
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

Re: [SSSD] [PATCH] TEST: recent_valid filter testing

2015-11-13 Thread Petr Cech


On 11/13/2015 10:09 AM, Jakub Hrozek wrote:

Hi, patch 003 doesn't apply cleanly for me, can you rebase?


Patches are rebased now. I hope it will be ok now.

Petr
>From 3e43417db9b66bdb44d60b5f186156c5ac26ad4b Mon Sep 17 00:00:00 2001
From: Petr Cech 
Date: Fri, 2 Oct 2015 07:34:08 -0400
Subject: [PATCH 1/8] TEST: Add test_user_by_recent_filter_valid

Test users_by_filter_valid() was removed in past. We will add two new
tests instead of it. Logic of those tests is connected to RECENT
filter. It returns only records which have been wrote or updated after
filter was created (or another given time).

users_by_filter_valid() --> user_by_recent_filter_valid()
users_by_recent_filter_valid()

The first of new tests, user_by_recent_filter_valid(), counts with two
users. One is stored before filter request creation and the second user
is stored after filter request creation. So filter returns only one
user.

The second of new tests, users_by_recent_filter_valid(), counts with
three users. One is stored before filter request creation and two users
are stored after filter request creation. So filter returns two users.

This patch adds user_by_recent_filter_valid().

Resolves:
https://fedorahosted.org/sssd/ticket/2730
---
 src/tests/cmocka/test_responder_cache_req.c | 50 +
 1 file changed, 50 insertions(+)

diff --git a/src/tests/cmocka/test_responder_cache_req.c b/src/tests/cmocka/test_responder_cache_req.c
index 85d986bd7d159dc238bce4bc770272d18288f2dd..14a40ae6e56b2f6d0b18608bac09bc4680245153 100644
--- a/src/tests/cmocka/test_responder_cache_req.c
+++ b/src/tests/cmocka/test_responder_cache_req.c
@@ -1239,6 +1239,53 @@ static void cache_req_user_by_filter_test_done(struct tevent_req *req)
 ctx->tctx->done = true;
 }
 
+void test_user_by_recent_filter_valid(void **state)
+{
+struct cache_req_test_ctx *test_ctx = NULL;
+TALLOC_CTX *req_mem_ctx = NULL;
+struct tevent_req *req = NULL;
+const char *ldbname = NULL;
+errno_t ret;
+
+test_ctx = talloc_get_type_abort(*state, struct cache_req_test_ctx);
+test_ctx->create_user = true;
+
+ret = sysdb_store_user(test_ctx->tctx->dom, TEST_USER_NAME2,
+   "pwd", 1001, 1001, NULL, NULL, NULL,
+   "cn="TEST_USER_NAME2",dc=test",
+   NULL, NULL, 1000, time(NULL)-1);
+assert_int_equal(ret, EOK);
+
+req_mem_ctx = talloc_new(test_ctx->tctx);
+check_leaks_push(req_mem_ctx);
+
+/* Filters always go to DP */
+will_return(__wrap_sss_dp_get_account_send, test_ctx);
+mock_account_recv_simple();
+
+/* User TEST_USER is created with a DP callback. */
+req = cache_req_user_by_filter_send(req_mem_ctx, test_ctx->tctx->ev,
+test_ctx->rctx,
+test_ctx->tctx->dom->name,
+"test*");
+assert_non_null(req);
+
+tevent_req_set_callback(req, cache_req_user_by_filter_test_done, test_ctx);
+
+ret = test_ev_loop(test_ctx->tctx);
+assert_int_equal(ret, ERR_OK);
+assert_true(check_leaks_pop(req_mem_ctx));
+
+assert_non_null(test_ctx->result);
+assert_int_equal(test_ctx->result->count, 1);
+
+ldbname = ldb_msg_find_attr_as_string(test_ctx->result->msgs[0],
+  SYSDB_NAME, NULL);
+assert_non_null(ldbname);
+assert_string_equal(ldbname, TEST_USER_NAME);
+}
+
+
 void test_users_by_filter_filter_old(void **state)
 {
 struct cache_req_test_ctx *test_ctx = NULL;
@@ -1476,11 +1523,14 @@ int main(int argc, const char *argv[])
 new_multi_domain_test(group_by_id_multiple_domains_found),
 new_multi_domain_test(group_by_id_multiple_domains_notfound),
 
+new_single_domain_test(user_by_recent_filter_valid),
+
 new_single_domain_test(users_by_filter_filter_old),
 new_single_domain_test(users_by_filter_notfound),
 new_multi_domain_test(users_by_filter_multiple_domains_notfound),
 new_single_domain_test(groups_by_filter_notfound),
 new_multi_domain_test(groups_by_filter_multiple_domains_notfound),
+
 };
 
 /* Set debug level to invalid value so we can deside if -d 0 was used. */
-- 
2.4.3

>From 94d583476335324c4f4b62e547a74241582f807f Mon Sep 17 00:00:00 2001
From: Petr Cech 
Date: Wed, 4 Nov 2015 06:50:33 -0500
Subject: [PATCH 2/8] TEST: Refactor of test_responder_cache_req.c

This patch only defines constant TEST_USER_FILTER. So code will be more
redeable.

Resolves:
https://fedorahosted.org/sssd/ticket/2730
---
 src/tests/cmocka/test_responder_cache_req.c | 6 --
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/src/tests/cmocka/test_responder_cache_req.c b/src/tests/cmocka/test_responder_cache_req.c
index 14a40ae6e56b2f6d0b18608bac09bc4680245153..5ff6c95681d899e2ae93bd7964b492e52a2d223a 100644
---

Re: [SSSD] [PATCH] TEST: recent_valid filter testing

2015-11-13 Thread Petr Cech


On 11/13/2015 10:27 AM, Petr Cech wrote:


Patches are rebased now. I hope it will be ok now.

Petr

Sorry, now my local CI tests failed... I will rebase it again.
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

Re: [SSSD] [PATCH] SSSD: Add a new command diag_cmd

2015-11-13 Thread Jakub Hrozek

On Fri, Nov 13, 2015 at 08:20:53AM +0100, Petr Cech wrote:
> I reviewed the code. All remarks were addressed.
> CI tests passed:
> http://sssd-ci.duckdns.org/logs/job/33/10/summary.html
> 
> => ACK
> 
> Regards
> 
> Petr

I changed the wording of the commit message a bit (we added an option,
not a command) and pushed the patch to master:
89530c830ded58c6140cdb34c9de07bf77bb5bc0
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

Re: [SSSD] [PATCH] Guard against invalid DP messages

2015-11-13 Thread Sumit Bose

On Fri, Nov 13, 2015 at 02:56:53PM +0100, Jakub Hrozek wrote:
> On Fri, Nov 13, 2015 at 02:27:05PM +0100, Sumit Bose wrote:
> > > +static int sbus_request_valist_check(va_list va, int first_arg_type)
> > > +{
> > > +int ret = EOK;
> > > +#ifdef HAVE_DBUSBASICVALUE
> > > +int type;
> > > +va_list va_check;
> > > +const DBusBasicValue *value;
> > > +bool ok;
> > > +
> > > +va_copy(va_check, va);
> > > +
> > > +type = first_arg_type;
> > > +while (type != DBUS_TYPE_INVALID) {
> > > +value = va_arg(va_check, const DBusBasicValue*);
> > > +
> > > +if (type == DBUS_TYPE_STRING) {
> > > + ok = sss_utf8_check((const uint8_t *) value->str,
> > > +  strlen(value->str));
> > > + if (!ok) {
> > > +   DEBUG(SSSDBG_MINOR_FAILURE,
> > > + "Back end message [%s] contains invalid 
> > > non-UTF8 " \
> > > + "characters");
> > 
> > value->str is missing, additionally sbus_request_return_and_finish() is
> > used by the responders as well so 'Back end message' should be changed
> > to 'S-Bus message' or similar.
> > 
> > bye,
> > Sumit
> 
> Oops, sorry..

ACK.

CI is still running, but I don't expect any issue, so I needed feel free
to push them without waiting for the result.

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

[SSSD] [PATCH] CONTRIB: Add a gdb pretty-printer for ldb and sysdb_attrs

2015-11-13 Thread Jakub Hrozek

Hi,

I meant to send the pretty-printers for some time already..
>From 2927820959e1ffdb83c8f196a9195673bc927e44 Mon Sep 17 00:00:00 2001
From: Jakub Hrozek 
Date: Fri, 13 Nov 2015 16:44:45 +0100
Subject: [PATCH] CONTRIB: Add a gdb pretty-printer for ldb and sysdb_attrs

Printing ldb structures and sysdb_attrs can be a pain. This patch adds a
gdb pretty-printer to help

SSSD and LDB debugging plugins

Activate them by putting:
  source /path/to/this/file.py
to your .gdbinit file

To bypass the pretty printer and print the raw values, use the "/r" option:
  print /r foobar
---
 contrib/gdb/sssd_gdb_plugin.py | 196 +
 1 file changed, 196 insertions(+)
 create mode 100644 contrib/gdb/sssd_gdb_plugin.py

diff --git a/contrib/gdb/sssd_gdb_plugin.py b/contrib/gdb/sssd_gdb_plugin.py
new file mode 100644
index 
..730c7e894297012d897aafff97a3d8f8a3b7d042
--- /dev/null
+++ b/contrib/gdb/sssd_gdb_plugin.py
@@ -0,0 +1,196 @@
+# SSSD and LDB debugging plugins
+#
+# Activate them by putting:
+#   source /path/to/this/file.py
+# to your .gdbinit file
+#
+# To bypass the pretty printer and print the raw values, use the "/r" option:
+#   print /r foobar
+import gdb
+
+
+def gdb_printer_decorator(fn):
+gdb.pretty_printers.append(fn)
+return fn
+
+
+def indent_string(s, indent):
+return '\n'.join(["%s%s" % ("\t" * indent, part) for part in 
s.split('\n')])
+
+
+class StringPrinter(object):
+"Shared code between different string-printing classes"
+def __init__(self, val):
+self.val = val
+
+def to_string(self):
+return self.as_string()
+
+
+class LdbDnPrinter(StringPrinter):
+" print an ldb dn "
+
+def as_string(self, indent=0):
+ret = "{ <%s>\tlinearized:%s }" % (self.val.type, 
self.val['linearized'])
+return indent_string(ret, indent)
+
+
+class LdbValPrinter(StringPrinter):
+" print a ldb value"
+
+def as_string(self, indent=0):
+ret = "data = %(data)s, length = %(length)s" % self.val
+return indent_string("{ <%s>\t%s }" % (self.val.type, ret), indent)
+
+
+class LdbMessageElementPrinter(StringPrinter):
+" print a ldb message element "
+
+def as_string(self, indent=0):
+ret = "flags = %(flags)s, name = %(name)s, num_values = 
%(num_values)s" % self.val
+try:
+nvals = int(self.val['num_values'])
+except ValueError:
+return "num_values is not numeric?"
+
+for i in range(nvals):
+ldbval = LdbValPrinter(self.val['values'][i])
+ret += "\n%s" % (ldbval.as_string(indent+1))
+
+return indent_string("{ <%s>\t%s }" % (self.val.type, ret), indent)
+
+
+class LdbMessagePrinter(StringPrinter):
+" print a ldb message "
+
+def as_string(self, indent=0):
+try:
+nels = int(self.val['num_elements'])
+except ValueError:
+return "num_elements is not numeric?"
+
+dn = LdbDnPrinter(self.val['dn'])
+ret = "num_elements:\t%s\ndn:\t%s\nelements:\t" % (nels, 
dn.as_string(indent))
+
+for i in range(nels):
+el = LdbMessageElementPrinter(self.val['elements'][i])
+ret += "\n%s" % (el.as_string(indent+1))
+
+return indent_string("{ <%s>\n%s }" % (self.val.type, ret), indent)
+
+
+class LdbResultPrinter(StringPrinter):
+" print a ldb message element "
+
+def as_string(self, indent=0):
+ret = "count = %(count)s, extended = %(extended)s, controls = 
%(controls)s, refs = %(refs)s" % self.val
+try:
+count = int(self.val['count'])
+except ValueError:
+ret += 'Count is not numeric value?'
+return ret
+
+for i in range(count):
+msg = LdbMessagePrinter(self.val['msgs'][i])
+ret += "\n%s" % (msg.as_string(indent+1))
+
+return indent_string("{ <%s>\t%s }" % (self.val.type, ret), indent)
+
+
+class SysdbAttrsPrinter(StringPrinter):
+" print a struct sysdb attrs "
+
+def as_string(self, indent=0):
+ret = "num = %(num)s" % self.val
+
+try:
+num = int(self.val['num'])
+except ValueError:
+ret += 'num is not numeric value?'
+return ret
+
+for i in range(num):
+el = LdbMessageElementPrinter(self.val['a'][i])
+ret += "\n%s" % (el.as_string(indent+1))
+
+return indent_string("{ <%s>\t%s }" % (self.val.type, ret), indent)
+
+
+# ---
+# --- register pretty printers ---
+# ---
+@gdb_printer_decorator
+def ldb_val_element_printer(val):
+if str(val.type) == 'struct ldb_dn':
+return LdbDnPrinter(val)
+return None
+
+
+@gdb_printer_decorator
+def ldb_val_element_printer(val):
+if str(val.type) == 'struct ldb_val':
+return LdbValPrinter(val)
+return None
+
+
+@gdb_printer_decorator
+def ldb_message_element_printer(val):
+if str(val.type) == 'struct

Re: [SSSD] [PATCHSET] intg: sss_override

2015-11-13 Thread Pavel Reichl




On 11/13/2015 05:44 PM, Nikolai Kondrashov wrote:

On 11/13/2015 06:19 PM, Pavel Reichl wrote:

Thanks Nick, updated patch attached.


Thanks, Pavel, this looks really neat now! I have only two comments.


+def test_simple_user_override(ldap_conn, env_simple_user_override):
+"""Test entries are overriden"""
+
+


Shouldn't there be an actual test here?

Sorry :-)




+# Regression test for bug #2802
+# sss_override segfaults when accidentally adding --help flag to some commands
+
+
+def test_regr_2802_override(request, ldap_conn):
+prepare_sssd(request, ldap_conn)


I had no idea this could actually work, I mean getting "request" in a test.
Can you give a link to documentation where this is explained?


I don't have any, I was in a hurry and I did a mistake.



Otherwise, could you please move the setup to a proper fixture? Tests are not
supposed to have setup/teardown inside, AFAIK, unless this is really official.


+
+subprocess.check_call(["sss_override", "user-del", "--help"])


Sure.


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


Updated patch attached.
>From b25ec9152a36b161e8558d4b81bde1d5c603acb2 Mon Sep 17 00:00:00 2001
From: Pavel Reichl 
Date: Mon, 5 Oct 2015 10:02:05 -0400
Subject: [PATCH] intg: Add test for user and group local overrides

Introduce a new integration test for local view overrides.

Regression tests for: #2790, #2757 and #2802.

Resolves:
https://fedorahosted.org/sssd/ticket/2732
---
 src/tests/intg/Makefile.am |   1 +
 src/tests/intg/ldap_local_override_test.py | 935 +
 2 files changed, 936 insertions(+)
 create mode 100644 src/tests/intg/ldap_local_override_test.py

diff --git a/src/tests/intg/Makefile.am b/src/tests/intg/Makefile.am
index 12a4fc279e790b4432800a9ea0d7130afe16ec5e..7394997319142d581237ab8a37270bfd7bc974ca 100644
--- a/src/tests/intg/Makefile.am
+++ b/src/tests/intg/Makefile.am
@@ -6,6 +6,7 @@ dist_noinst_DATA = \
 ent.py \
 ent_test.py \
 ldap_ent.py \
+ldap_local_override_test.py \
 ldap_test.py \
 test_local_domain.py \
 util.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 ..b55746700cca77afa3779acd8f06a6c7ccd2b294
--- /dev/null
+++ b/src/tests/intg/ldap_local_override_test.py
@@ -0,0 +1,935 @@
+#
+# integration test for sss_override tool
+#
+# 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
+
+
+@pytest.fixture(scope="module")
+def ds_inst(request):
+"""LDAP server instance fixture"""
+ds_inst = ds_openldap.DSOpenLDAP(
+config.PREFIX, 10389, 'dc=example,dc=com',
+"cn=admin", "Secret123")
+try:
+ds_inst.setup()
+except:
+ds_inst.teardown()
+raise
+request.addfinalizer(lambda: ds_inst.teardown())
+return ds_inst
+
+
+@pytest.fixture(scope="module")
+def ldap_conn(request, ds_inst):
+"""LDAP server connection fixture"""
+ldap_conn = ds_inst.bind()
+ldap_conn.ds_inst = ds_inst
+request.addfinalizer(lambda: ldap_conn.unbind_s())
+return ldap_conn
+
+
+def create_ldap_fixture(request, ldap_conn, ent_list):
+"""Add LDAP entries and add teardown for removing them"""
+for entry in ent_list:
+ldap_conn.add_s(entry[0], entry[1])
+
+def teardown():
+for entry in ent_list:
+ldap_conn.delete_s(entry[0])
+request.addfinalizer(teardown)
+
+
+def create_conf_fixture(request, contents):
+"""Generate sssd.conf and add teardown for removing it"""
+conf = open(config.CONF_PATH, "w")
+conf.write(contents)
+conf.close()
+os.chmod(config.CONF_PATH, stat.S_IRUSR | stat.S_IWUSR)
+request.addfinalizer(lambda: os.unlink(config.CONF_PATH))
+
+
+def stop_sssd():
+pid_file = open(config.PIDFILE_PATH, "r")
+pid = int(pid_file.read())
+os.kill(pid, signal.SIGTERM)
+while True:
+try:
+os.kill(pid, signal.SIGCONT)
+except:
+

Re: [SSSD] [PATCHSET] intg: sss_override

2015-11-13 Thread Pavel Reichl


Thanks Nick, updated patch attached.
>From ef503789ba6eb8618efef600e063e8031afcb0bc Mon Sep 17 00:00:00 2001
From: Pavel Reichl 
Date: Mon, 5 Oct 2015 10:02:05 -0400
Subject: [PATCH] intg: Add test for user and group local overrides

Introduce a new integration test for local view overrides.

Regression tests for: #2790, #2757 and #2802.

Resolves:
https://fedorahosted.org/sssd/ticket/2732
---
 src/tests/intg/Makefile.am |   1 +
 src/tests/intg/ldap_local_override_test.py | 928 +
 2 files changed, 929 insertions(+)
 create mode 100644 src/tests/intg/ldap_local_override_test.py

diff --git a/src/tests/intg/Makefile.am b/src/tests/intg/Makefile.am
index 12a4fc279e790b4432800a9ea0d7130afe16ec5e..7394997319142d581237ab8a37270bfd7bc974ca 100644
--- a/src/tests/intg/Makefile.am
+++ b/src/tests/intg/Makefile.am
@@ -6,6 +6,7 @@ dist_noinst_DATA = \
 ent.py \
 ent_test.py \
 ldap_ent.py \
+ldap_local_override_test.py \
 ldap_test.py \
 test_local_domain.py \
 util.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 ..7d0229c6a484e0abb9dfcd06e3ad61fb97db6f87
--- /dev/null
+++ b/src/tests/intg/ldap_local_override_test.py
@@ -0,0 +1,928 @@
+#
+# integration test for sss_override tool
+#
+# 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
+
+
+@pytest.fixture(scope="module")
+def ds_inst(request):
+"""LDAP server instance fixture"""
+ds_inst = ds_openldap.DSOpenLDAP(
+config.PREFIX, 10389, 'dc=example,dc=com',
+"cn=admin", "Secret123")
+try:
+ds_inst.setup()
+except:
+ds_inst.teardown()
+raise
+request.addfinalizer(lambda: ds_inst.teardown())
+return ds_inst
+
+
+@pytest.fixture(scope="module")
+def ldap_conn(request, ds_inst):
+"""LDAP server connection fixture"""
+ldap_conn = ds_inst.bind()
+ldap_conn.ds_inst = ds_inst
+request.addfinalizer(lambda: ldap_conn.unbind_s())
+return ldap_conn
+
+
+def create_ldap_fixture(request, ldap_conn, ent_list):
+"""Add LDAP entries and add teardown for removing them"""
+for entry in ent_list:
+ldap_conn.add_s(entry[0], entry[1])
+
+def teardown():
+for entry in ent_list:
+ldap_conn.delete_s(entry[0])
+request.addfinalizer(teardown)
+
+
+def create_conf_fixture(request, contents):
+"""Generate sssd.conf and add teardown for removing it"""
+conf = open(config.CONF_PATH, "w")
+conf.write(contents)
+conf.close()
+os.chmod(config.CONF_PATH, stat.S_IRUSR | stat.S_IWUSR)
+request.addfinalizer(lambda: os.unlink(config.CONF_PATH))
+
+
+def stop_sssd():
+pid_file = open(config.PIDFILE_PATH, "r")
+pid = int(pid_file.read())
+os.kill(pid, signal.SIGTERM)
+while True:
+try:
+os.kill(pid, signal.SIGCONT)
+except:
+break
+time.sleep(1)
+
+
+def start_sssd():
+"""Start sssd"""
+if subprocess.call(["sssd", "-D", "-f"]) != 0:
+raise Exception("sssd start failed")
+
+
+def restart_sssd():
+stop_sssd()
+start_sssd()
+
+
+def create_sssd_fixture(request):
+"""Start sssd and add teardown for stopping it and removing state"""
+if subprocess.call(["sssd", "-D", "-f"]) != 0:
+raise Exception("sssd start failed")
+
+def teardown():
+try:
+stop_sssd()
+except:
+pass
+subprocess.call(["sss_cache", "-E"])
+for path in os.listdir(config.DB_PATH):
+os.unlink(config.DB_PATH + "/" + path)
+for path in os.listdir(config.MCACHE_PATH):
+os.unlink(config.MCACHE_PATH + "/" + path)
+request.addfinalizer(teardown)
+
+
+OVERRIDE_FILENAME = "export_file"
+
+
+def prepare_sssd(request, ldap_conn, use_fully_qualified_names=False):
+"""Prepare SSSD with defaults"""
+conf = unindent("""\
+[sssd]
+domains = LDAP
+services= nss
+
+[nss]
+memcache_timeout = 1
+
+[domain/LDAP]
+

Re: [SSSD] [PATCHSET] intg: sss_override

2015-11-13 Thread Nikolai Kondrashov


On 11/13/2015 06:19 PM, Pavel Reichl wrote:

Thanks Nick, updated patch attached.


Thanks, Pavel, this looks really neat now! I have only two comments.


+def test_simple_user_override(ldap_conn, env_simple_user_override):
+"""Test entries are overriden"""
+
+


Shouldn't there be an actual test here?


+# Regression test for bug #2802
+# sss_override segfaults when accidentally adding --help flag to some commands
+
+
+def test_regr_2802_override(request, ldap_conn):
+prepare_sssd(request, ldap_conn)


I had no idea this could actually work, I mean getting "request" in a test.
Can you give a link to documentation where this is explained?

Otherwise, could you please move the setup to a proper fixture? Tests are not
supposed to have setup/teardown inside, AFAIK, unless this is really official.


+
+subprocess.check_call(["sss_override", "user-del", "--help"])


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

Re: [SSSD] [PATCHSET] intg: sss_override

2015-11-13 Thread Nikolai Kondrashov


On 11/13/2015 07:06 PM, Pavel Reichl wrote:

On 11/13/2015 05:44 PM, Nikolai Kondrashov wrote:

On 11/13/2015 06:19 PM, Pavel Reichl wrote:

Thanks Nick, updated patch attached.


Thanks, Pavel, this looks really neat now! I have only two comments.


+def test_simple_user_override(ldap_conn, env_simple_user_override):
+"""Test entries are overriden"""
+
+


Shouldn't there be an actual test here?

Sorry :-)


No problem :)




+# Regression test for bug #2802
+# sss_override segfaults when accidentally adding --help flag to some commands
+
+
+def test_regr_2802_override(request, ldap_conn):
+prepare_sssd(request, ldap_conn)


I had no idea this could actually work, I mean getting "request" in a test.
Can you give a link to documentation where this is explained?


I don't have any, I was in a hurry and I did a mistake.


Ah, fine.


Otherwise, could you please move the setup to a proper fixture? Tests are not
supposed to have setup/teardown inside, AFAIK, unless this is really official.


+
+subprocess.check_call(["sss_override", "user-del", "--help"])


Sure.


Thanks a lot!

Looks fine to me now. Tests passed on my machine both all together, and each
separately.  Great job!

ACK

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

Re: [SSSD] [PATCH] test_sysdb_subdomains: Do use assignment in assertions

2015-11-13 Thread Jakub Hrozek

On Fri, Nov 13, 2015 at 08:51:17AM +0100, Lukas Slebodnik wrote:
> ehlo,
> 
> I found these iisues due to different size of integer and pointer
> on el6.
> 
> /home/build/sssd/src/tests/cmocka/test_sysdb_subdomains.c:248: error: cast 
> from
> pointer to integer of different size
> 
> Simple patch is attached.
> 
> LS

Code-wise ACK, waiting for CI results. But I think the commit message is
incorrect, I would like to reword it to "Don't use.." before pushing.
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

Re: [SSSD] [PATCH] TEST: recent_valid filter testing

2015-11-13 Thread Lukas Slebodnik

On (13/11/15 11:32), Jakub Hrozek wrote:
>On Fri, Nov 13, 2015 at 10:52:08AM +0100, Petr Cech wrote:
>> On 11/13/2015 10:30 AM, Petr Cech wrote:
>> >On 11/13/2015 10:27 AM, Petr Cech wrote:
>> >>
>> >>Patches are rebased now. I hope it will be ok now.
>> >>
>> >>Petr
>> >Sorry, now my local CI tests failed... I will rebase it again.
>> 
>> Well, now it is right. Local CI tests passed. There has been patch:
>> 
>>   "TESTS: Fix warnings -Wshadow":
>>   commit df9e9a1f9b7dc255eb62c390163c25917b08f5a2
>>   Refs: sssd-1_13_1-137-gdf9e9a1
>>   Author: Lukas Slebodnik 
>>   AuthorDate: Mon Nov 9 10:59:55 2015 +0100
>>   Commit: Jakub Hrozek 
>>   CommitDate: Tue Nov 10 15:34:41 2015 +0100
>> 
>> There is change
>> # - time_t time)
>> # + time_t transaction_time)
>> in static void prepare_user().
>> My patches were in conflict with it.
>> 
>> Regards
>> 
>> Petr
>
>> From 3ce6073dda27fd7a4626f5cbac1c765274ca5fe0 Mon Sep 17 00:00:00 2001
>> From: Petr Cech 
>> Date: Fri, 2 Oct 2015 07:34:08 -0400
>> Subject: [PATCH 1/8] TEST: Add test_user_by_recent_filter_valid
>> 
>> Test users_by_filter_valid() was removed in past. We will add two new
>> tests instead of it. Logic of those tests is connected to RECENT
>> filter. It returns only records which have been wrote or updated after
>> filter was created (or another given time).
>> 
>> users_by_filter_valid() --> user_by_recent_filter_valid()
>> users_by_recent_filter_valid()
>> 
>> The first of new tests, user_by_recent_filter_valid(), counts with two
>> users. One is stored before filter request creation and the second user
>> is stored after filter request creation. So filter returns only one
>> user.
>> 
>> The second of new tests, users_by_recent_filter_valid(), counts with
>> three users. One is stored before filter request creation and two users
>> are stored after filter request creation. So filter returns two users.
>> 
>> This patch adds user_by_recent_filter_valid().
>> 
>> Resolves:
>> https://fedorahosted.org/sssd/ticket/2730
>> ---
>>  src/tests/cmocka/test_responder_cache_req.c | 50 
>> +
>>  1 file changed, 50 insertions(+)
>> 
>> diff --git a/src/tests/cmocka/test_responder_cache_req.c 
>> b/src/tests/cmocka/test_responder_cache_req.c
>> index 
>> 85d986bd7d159dc238bce4bc770272d18288f2dd..14a40ae6e56b2f6d0b18608bac09bc4680245153
>>  100644
>> --- a/src/tests/cmocka/test_responder_cache_req.c
>> +++ b/src/tests/cmocka/test_responder_cache_req.c
>> @@ -1239,6 +1239,53 @@ static void cache_req_user_by_filter_test_done(struct 
>> tevent_req *req)
>>  ctx->tctx->done = true;
>>  }
>>  
>> +void test_user_by_recent_filter_valid(void **state)
>> +{
>> +struct cache_req_test_ctx *test_ctx = NULL;
>> +TALLOC_CTX *req_mem_ctx = NULL;
>> +struct tevent_req *req = NULL;
>> +const char *ldbname = NULL;
>> +errno_t ret;
>> +
>> +test_ctx = talloc_get_type_abort(*state, struct cache_req_test_ctx);
>> +test_ctx->create_user = true;
>> +
>> +ret = sysdb_store_user(test_ctx->tctx->dom, TEST_USER_NAME2,
>> +   "pwd", 1001, 1001, NULL, NULL, NULL,
>> +   "cn="TEST_USER_NAME2",dc=test",
>> +   NULL, NULL, 1000, time(NULL)-1);
>> +assert_int_equal(ret, EOK);
>> +
>> +req_mem_ctx = talloc_new(test_ctx->tctx);
>> +check_leaks_push(req_mem_ctx);
>
>I think the last question is whether we want to use this new context or
>just call check_leaks_push(test_ctx) recursively. I don't really mind
>too much, both would work for me.
>
>Unless someone opposes, I would push the patch as-is.
>
I have a different question. (i haven't read patches yet)
But I can see that check_leaks_push is called after sysdb_store_user.

I would like to know why.
because we shout try to check leaks "caused" in this function.

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

Re: [SSSD] [PATCH] TEST: recent_valid filter testing

2015-11-13 Thread Jakub Hrozek

On Fri, Nov 13, 2015 at 10:52:08AM +0100, Petr Cech wrote:
> On 11/13/2015 10:30 AM, Petr Cech wrote:
> >On 11/13/2015 10:27 AM, Petr Cech wrote:
> >>
> >>Patches are rebased now. I hope it will be ok now.
> >>
> >>Petr
> >Sorry, now my local CI tests failed... I will rebase it again.
> 
> Well, now it is right. Local CI tests passed. There has been patch:
> 
>   "TESTS: Fix warnings -Wshadow":
>   commit df9e9a1f9b7dc255eb62c390163c25917b08f5a2
>   Refs: sssd-1_13_1-137-gdf9e9a1
>   Author: Lukas Slebodnik 
>   AuthorDate: Mon Nov 9 10:59:55 2015 +0100
>   Commit: Jakub Hrozek 
>   CommitDate: Tue Nov 10 15:34:41 2015 +0100
> 
> There is change
> # - time_t time)
> # + time_t transaction_time)
> in static void prepare_user().
> My patches were in conflict with it.
> 
> Regards
> 
> Petr

> From 3ce6073dda27fd7a4626f5cbac1c765274ca5fe0 Mon Sep 17 00:00:00 2001
> From: Petr Cech 
> Date: Fri, 2 Oct 2015 07:34:08 -0400
> Subject: [PATCH 1/8] TEST: Add test_user_by_recent_filter_valid
> 
> Test users_by_filter_valid() was removed in past. We will add two new
> tests instead of it. Logic of those tests is connected to RECENT
> filter. It returns only records which have been wrote or updated after
> filter was created (or another given time).
> 
> users_by_filter_valid() --> user_by_recent_filter_valid()
> users_by_recent_filter_valid()
> 
> The first of new tests, user_by_recent_filter_valid(), counts with two
> users. One is stored before filter request creation and the second user
> is stored after filter request creation. So filter returns only one
> user.
> 
> The second of new tests, users_by_recent_filter_valid(), counts with
> three users. One is stored before filter request creation and two users
> are stored after filter request creation. So filter returns two users.
> 
> This patch adds user_by_recent_filter_valid().
> 
> Resolves:
> https://fedorahosted.org/sssd/ticket/2730
> ---
>  src/tests/cmocka/test_responder_cache_req.c | 50 
> +
>  1 file changed, 50 insertions(+)
> 
> diff --git a/src/tests/cmocka/test_responder_cache_req.c 
> b/src/tests/cmocka/test_responder_cache_req.c
> index 
> 85d986bd7d159dc238bce4bc770272d18288f2dd..14a40ae6e56b2f6d0b18608bac09bc4680245153
>  100644
> --- a/src/tests/cmocka/test_responder_cache_req.c
> +++ b/src/tests/cmocka/test_responder_cache_req.c
> @@ -1239,6 +1239,53 @@ static void cache_req_user_by_filter_test_done(struct 
> tevent_req *req)
>  ctx->tctx->done = true;
>  }
>  
> +void test_user_by_recent_filter_valid(void **state)
> +{
> +struct cache_req_test_ctx *test_ctx = NULL;
> +TALLOC_CTX *req_mem_ctx = NULL;
> +struct tevent_req *req = NULL;
> +const char *ldbname = NULL;
> +errno_t ret;
> +
> +test_ctx = talloc_get_type_abort(*state, struct cache_req_test_ctx);
> +test_ctx->create_user = true;
> +
> +ret = sysdb_store_user(test_ctx->tctx->dom, TEST_USER_NAME2,
> +   "pwd", 1001, 1001, NULL, NULL, NULL,
> +   "cn="TEST_USER_NAME2",dc=test",
> +   NULL, NULL, 1000, time(NULL)-1);
> +assert_int_equal(ret, EOK);
> +
> +req_mem_ctx = talloc_new(test_ctx->tctx);
> +check_leaks_push(req_mem_ctx);

I think the last question is whether we want to use this new context or
just call check_leaks_push(test_ctx) recursively. I don't really mind
too much, both would work for me.

Unless someone opposes, I would push the patch as-is.

> +
> +/* Filters always go to DP */
> +will_return(__wrap_sss_dp_get_account_send, test_ctx);
> +mock_account_recv_simple();
> +
> +/* User TEST_USER is created with a DP callback. */
> +req = cache_req_user_by_filter_send(req_mem_ctx, test_ctx->tctx->ev,
> +test_ctx->rctx,
> +test_ctx->tctx->dom->name,
> +"test*");
> +assert_non_null(req);

> From df9717ca932f95f55b528024829758dd9b2f2f56 Mon Sep 17 00:00:00 2001
> From: Petr Cech 
> Date: Wed, 4 Nov 2015 06:50:33 -0500
> Subject: [PATCH 2/8] TEST: Refactor of test_responder_cache_req.c
> 
> This patch only defines constant TEST_USER_FILTER. So code will be more
   
   TEST_USER_PREFIX is defined.

The code is fine.


> From ae448cc95f9ab9fbca3aba5107bb964caf8250ec Mon Sep 17 00:00:00 2001
> From: Petr Cech 
> Date: Tue, 27 Oct 2015 03:53:18 -0400
> Subject: [PATCH 3/8] TEST: Refactor of test_responder_cache_req.c
> 
> We need little more in background of responder_cache_req tests. There
> will be tests which will use three test users. This patch add support
> for it.

ACK

> From 943d828ec283284269f954b3044292fb491cf5fa Mon Sep 17 00:00:00 2001

Re: [SSSD] [PATCH] TEST: recent_valid filter testing

2015-11-13 Thread Jakub Hrozek

On Fri, Nov 13, 2015 at 12:27:03PM +0100, Jakub Hrozek wrote:
> > I have a different question. (i haven't read patches yet)
> > But I can see that check_leaks_push is called after sysdb_store_user.
> > 
> > I would like to know why.
> > because we shout try to check leaks "caused" in this function.
> 
> Wouldn't these leaks be caught by leaks checks that are pushed in
> setup() and popped in teardown() ?

Ah, but we don't use leak checks in setup() and teardown(), sorry!

Then it's something we definitely need to fix -- I think a leak in
requests (like cached_req) is only possible if we use a long-lived
(responder or event) context by mistake.

So I think we should file another ticket and add leak checks to setup and
teardown in this setup. It's not related to Petr's tests, but I think it
should be done before we switch to the cache_req API in NSS to make sure
we don't regress.
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

Re: [SSSD] [PATCH] TEST: recent_valid filter testing

2015-11-13 Thread Jakub Hrozek

On Fri, Nov 13, 2015 at 12:45:00PM +0100, Petr Cech wrote:
> >Otherwise ACK.
> Thanks.
> 
> >
> >CI is still running..
> 
> There is new patch set attached.

These patches look OK to me code-wise, waiting for CI results..
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

Re: [SSSD] [PATCH] Guard against invalid DP messages

2015-11-13 Thread Lukas Slebodnik

On (13/11/15 13:40), Sumit Bose wrote:
>On Wed, Nov 11, 2015 at 12:00:31PM +0100, Jakub Hrozek wrote:
>> On Mon, Nov 09, 2015 at 03:14:31PM +0100, Jakub Hrozek wrote:
>> > On Mon, Nov 09, 2015 at 12:44:17PM +0100, Sumit Bose wrote:
>> > > On Fri, Nov 06, 2015 at 09:32:23PM +0100, Jakub Hrozek wrote:
>> > > > On Fri, Nov 06, 2015 at 09:13:07PM +0100, Jakub Hrozek wrote:
>> > > > > On Fri, Nov 06, 2015 at 04:59:58PM +0100, Sumit Bose wrote:
>> > > > > > > @@ -658,11 +678,7 @@ static void get_subdomains_callback(struct 
>> > > > > > > be_req *req,
>> > > > > > >   */
>> > > > > > >  err_maj = dp_err_type;
>> > > > > > >  err_min = errnum;
>> > > > > > > -if (errstr) {
>> > > > > > > -err_msg = errstr;
>> > > > > > > -} else {
>> > > > > > > -err_msg = dp_err_to_string(dp_err_type);
>> > > > > > > -}
>> > > > > > > +err_msg = safe_be_req_err_msg(errstr, dp_err_type);
>> > > > > > 
>> > > > > > Wouldn't it be even more safe to do this check in
>> > > > > > sbus_request_return_and_finish() and not rely on the callers?
>> > > > > 
>> > > > > Something like the attached patch? I think we should check in
>> > > > > sbus_request_return_and_finish() in addition to checking in the 
>> > > > > callers,
>> > > > > not instead, because the string in the callback is just a diagnostics
>> > > > > message and we can recover by using some fallback.
>> > > > > 
>> > > > > If we get all the way to sbus_request_return_and_finish(), then 
>> > > > > failing
>> > > > > gracefully is better then abort() that libdbus calls, but it's even 
>> > > > > better
>> > > > > to finish the request with wrong error message then fail completely.
>> > > > > 
>> > > > > (I also still dislike that libdbus aborts...)
>> > > > 
>> > > > Also, self-nack. dbus_validate_utf8() is not available on RHEL-6:
>> > > > 
>> > > > http://sssd-ci.duckdns.org/logs/job/32/17/rhel6/ci-build-debug/ci-make-tests.log
>> > > > 
>> > > > I guess we can limit these checks to new OSs or use sss_utf8_check() on
>> > > > older platforms..
>> > > 
>> > > Why not always use sss_utf8_check()? Is there an advantage using
>> > > dbus_validate_utf8()?
>> > 
>> > I don't know how dbus_validate_utf8() works internally, so I tried to
>> > stick to the dbus-provided version to check dbus types.
>> > 
>> > But if both do the same thing (If they don't it would be a bug, sure),
>> > we can stick to sss_utf8_check()
>> > 
>> > (Another advantage might be that we could get away with linking against
>> > libdbus only)
>> 
>> Hi,
>> 
>> attached are patches that only use sss_utf8_check() and work on RHEL-6
>> also.
>
>The patches look good and fixing the issue as expected. CI passes as
>well http://sssd-ci.duckdns.org/logs/job/33/16/summary.html (the RHEL6
>failure is unrelated). ACK
>
>I just have a minor comment
>
>>  
>> +static const char *safe_be_req_err_msg(const char *msg_in,
>> +   int dp_err_type)
>> +{
>> +bool ok;
>> +
>> +if (msg_in == NULL) {
>> +/* No custom error, just use default */
>> +return dp_err_to_string(dp_err_type);
>> +}
>> +
>> +ok = sss_utf8_check((const uint8_t *) msg_in,
>> +strlen(msg_in));
>> +if (!ok) {
>> +DEBUG(SSSDBG_MINOR_FAILURE,
>> +  "Back end message is invalid, using default\n");
>
>Maybe something like
>
>DEBUG(SSSDBG_MINOR_FAILURE,
>  "Back end message [%s] contains invalid non-UTF8 character, " \
^^
Sumit, FYI. The
   concatenation by preporcessor will
   happen even without backslash at
   then end of line.

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

Re: [SSSD] [PATCH] Guard against invalid DP messages

2015-11-13 Thread Jakub Hrozek

On Fri, Nov 13, 2015 at 02:27:05PM +0100, Sumit Bose wrote:
> > +static int sbus_request_valist_check(va_list va, int first_arg_type)
> > +{
> > +int ret = EOK;
> > +#ifdef HAVE_DBUSBASICVALUE
> > +int type;
> > +va_list va_check;
> > +const DBusBasicValue *value;
> > +bool ok;
> > +
> > +va_copy(va_check, va);
> > +
> > +type = first_arg_type;
> > +while (type != DBUS_TYPE_INVALID) {
> > +value = va_arg(va_check, const DBusBasicValue*);
> > +
> > +if (type == DBUS_TYPE_STRING) {
> > + ok = sss_utf8_check((const uint8_t *) value->str,
> > +  strlen(value->str));
> > + if (!ok) {
> > +   DEBUG(SSSDBG_MINOR_FAILURE,
> > + "Back end message [%s] contains invalid non-UTF8 
> > " \
> > + "characters");
> 
> value->str is missing, additionally sbus_request_return_and_finish() is
> used by the responders as well so 'Back end message' should be changed
> to 'S-Bus message' or similar.
> 
> bye,
> Sumit

Oops, sorry..
>From 9f50257515cdb0e839fa31130dbfa5c5454225d7 Mon Sep 17 00:00:00 2001
From: Jakub Hrozek 
Date: Fri, 6 Nov 2015 12:33:59 +0100
Subject: [PATCH 1/3] DP: Drop dp_pam_err_to_string

Resolves:
https://fedorahosted.org/sssd/ticket/2861

All back end requests were using pam_strerror() to print additional info
about why request failed. Since pam_strerror() returns localized message
and we don't know the locale beforehand, this message failed to be
transferred through D-Bus, resulting in a crash.
---
 src/providers/data_provider_be.c | 75 
 1 file changed, 23 insertions(+), 52 deletions(-)

diff --git a/src/providers/data_provider_be.c b/src/providers/data_provider_be.c
index 
104016ca19c093dcbb4b0c1011f06e4bb4e2a49c..5f299a7209d3d1f1e83c625ae9db6d731a5cf5ab
 100644
--- a/src/providers/data_provider_be.c
+++ b/src/providers/data_provider_be.c
@@ -115,6 +115,23 @@ struct bet_queue_item {
 
 };
 
+static const char *dp_err_to_string(int dp_err_type)
+{
+switch (dp_err_type) {
+case DP_ERR_OK:
+return "Success";
+case DP_ERR_OFFLINE:
+return "Provider is Offline";
+case DP_ERR_TIMEOUT:
+return "Request timed out";
+case DP_ERR_FATAL:
+default:
+return "Internal Error";
+}
+
+return "Unknown Error";
+}
+
 #define REQ_PHASE_ACCESS 0
 #define REQ_PHASE_SELINUX 1
 
@@ -616,37 +633,6 @@ static void be_reset_offline(struct be_ctx *ctx)
 be_run_online_cb(ctx);
 }
 
-static char *dp_pam_err_to_string(TALLOC_CTX *memctx, int dp_err_type, int 
errnum)
-{
-switch (dp_err_type) {
-case DP_ERR_OK:
-return talloc_asprintf(memctx, "Success (%s)",
-   pam_strerror(NULL, errnum));
-break;
-
-case DP_ERR_OFFLINE:
-return talloc_asprintf(memctx,
-   "Provider is Offline (%s)",
-   pam_strerror(NULL, errnum));
-break;
-
-case DP_ERR_TIMEOUT:
-return talloc_asprintf(memctx,
-   "Request timed out (%s)",
-   pam_strerror(NULL, errnum));
-break;
-
-case DP_ERR_FATAL:
-default:
-return talloc_asprintf(memctx,
-   "Internal Error (%s)",
-   pam_strerror(NULL, errnum));
-break;
-}
-
-return NULL;
-}
-
 static void get_subdomains_callback(struct be_req *req,
 int dp_err_type,
 int errnum,
@@ -659,7 +645,7 @@ static void get_subdomains_callback(struct be_req *req,
 
 DEBUG(SSSDBG_TRACE_FUNC, "Backend returned: (%d, %d, %s) [%s]\n",
   dp_err_type, errnum, errstr?errstr:"",
-  dp_pam_err_to_string(req, dp_err_type, errnum));
+  dp_err_to_string(dp_err_type));
 
 be_queue_next_request(req, BET_SUBDOMAINS);
 
@@ -675,12 +661,7 @@ static void get_subdomains_callback(struct be_req *req,
 if (errstr) {
 err_msg = errstr;
 } else {
-err_msg = dp_pam_err_to_string(req, dp_err_type, errnum);
-}
-if (!err_msg) {
-DEBUG(SSSDBG_CRIT_FAILURE,
-  "Failed to set err_msg, Out of memory?\n");
-err_msg = "OOM";
+err_msg = dp_err_to_string(dp_err_type);
 }
 
 sbus_request_return_and_finish(dbus_req,
@@ -819,12 +800,7 @@ static void acctinfo_callback(struct be_req *req,
 if (errstr) {
 err_msg = errstr;
 } else {
-err_msg = dp_pam_err_to_string(req, dp_err_type, errnum);
-}
-if (!err_msg) {
-DEBUG(SSSDBG_CRIT_FAILURE,
-  "Failed to set err_msg, Out of memory?\n");
-err_msg = "OOM";
+err_msg = dp_err_to_string(dp_err_type);
 }

Re: [SSSD] [PATCH] Guard against invalid DP messages

2015-11-13 Thread Sumit Bose

On Wed, Nov 11, 2015 at 12:00:31PM +0100, Jakub Hrozek wrote:
> On Mon, Nov 09, 2015 at 03:14:31PM +0100, Jakub Hrozek wrote:
> > On Mon, Nov 09, 2015 at 12:44:17PM +0100, Sumit Bose wrote:
> > > On Fri, Nov 06, 2015 at 09:32:23PM +0100, Jakub Hrozek wrote:
> > > > On Fri, Nov 06, 2015 at 09:13:07PM +0100, Jakub Hrozek wrote:
> > > > > On Fri, Nov 06, 2015 at 04:59:58PM +0100, Sumit Bose wrote:
> > > > > > > @@ -658,11 +678,7 @@ static void get_subdomains_callback(struct 
> > > > > > > be_req *req,
> > > > > > >   */
> > > > > > >  err_maj = dp_err_type;
> > > > > > >  err_min = errnum;
> > > > > > > -if (errstr) {
> > > > > > > -err_msg = errstr;
> > > > > > > -} else {
> > > > > > > -err_msg = dp_err_to_string(dp_err_type);
> > > > > > > -}
> > > > > > > +err_msg = safe_be_req_err_msg(errstr, dp_err_type);
> > > > > > 
> > > > > > Wouldn't it be even more safe to do this check in
> > > > > > sbus_request_return_and_finish() and not rely on the callers?
> > > > > 
> > > > > Something like the attached patch? I think we should check in
> > > > > sbus_request_return_and_finish() in addition to checking in the 
> > > > > callers,
> > > > > not instead, because the string in the callback is just a diagnostics
> > > > > message and we can recover by using some fallback.
> > > > > 
> > > > > If we get all the way to sbus_request_return_and_finish(), then 
> > > > > failing
> > > > > gracefully is better then abort() that libdbus calls, but it's even 
> > > > > better
> > > > > to finish the request with wrong error message then fail completely.
> > > > > 
> > > > > (I also still dislike that libdbus aborts...)
> > > > 
> > > > Also, self-nack. dbus_validate_utf8() is not available on RHEL-6:
> > > > 
> > > > http://sssd-ci.duckdns.org/logs/job/32/17/rhel6/ci-build-debug/ci-make-tests.log
> > > > 
> > > > I guess we can limit these checks to new OSs or use sss_utf8_check() on
> > > > older platforms..
> > > 
> > > Why not always use sss_utf8_check()? Is there an advantage using
> > > dbus_validate_utf8()?
> > 
> > I don't know how dbus_validate_utf8() works internally, so I tried to
> > stick to the dbus-provided version to check dbus types.
> > 
> > But if both do the same thing (If they don't it would be a bug, sure),
> > we can stick to sss_utf8_check()
> > 
> > (Another advantage might be that we could get away with linking against
> > libdbus only)
> 
> Hi,
> 
> attached are patches that only use sss_utf8_check() and work on RHEL-6
> also.

The patches look good and fixing the issue as expected. CI passes as
well http://sssd-ci.duckdns.org/logs/job/33/16/summary.html (the RHEL6
failure is unrelated). ACK

I just have a minor comment

>  
> +static const char *safe_be_req_err_msg(const char *msg_in,
> +   int dp_err_type)
> +{
> +bool ok;
> +
> +if (msg_in == NULL) {
> +/* No custom error, just use default */
> +return dp_err_to_string(dp_err_type);
> +}
> +
> +ok = sss_utf8_check((const uint8_t *) msg_in,
> +strlen(msg_in));
> +if (!ok) {
> +DEBUG(SSSDBG_MINOR_FAILURE,
> +  "Back end message is invalid, using default\n");

Maybe something like

DEBUG(SSSDBG_MINOR_FAILURE,
  "Back end message [%s] contains invalid non-UTF8 character, " \
  "using default\n", msg_in);

would be more specific and would help to better identify the source of the
message as well? Similar for sbus_request_valist_check().

But to fix the issue at hand and not to delay the release this can of
course be added later as well.

bye,
Sumit

> +return dp_err_to_string(dp_err_type);
> +}
> +
> +return msg_in;
> +}
> +
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

Re: [SSSD] [PATCH] Guard against invalid DP messages

2015-11-13 Thread Jakub Hrozek

On Fri, Nov 13, 2015 at 01:40:16PM +0100, Sumit Bose wrote:
> > Hi,
> > 
> > attached are patches that only use sss_utf8_check() and work on RHEL-6
> > also.
> 
> The patches look good and fixing the issue as expected. CI passes as
> well http://sssd-ci.duckdns.org/logs/job/33/16/summary.html (the RHEL6
> failure is unrelated). ACK
> 
> I just have a minor comment
> 
> >  
> > +static const char *safe_be_req_err_msg(const char *msg_in,
> > +   int dp_err_type)
> > +{
> > +bool ok;
> > +
> > +if (msg_in == NULL) {
> > +/* No custom error, just use default */
> > +return dp_err_to_string(dp_err_type);
> > +}
> > +
> > +ok = sss_utf8_check((const uint8_t *) msg_in,
> > +strlen(msg_in));
> > +if (!ok) {
> > +DEBUG(SSSDBG_MINOR_FAILURE,
> > +  "Back end message is invalid, using default\n");
> 
> Maybe something like
> 
> DEBUG(SSSDBG_MINOR_FAILURE,
>   "Back end message [%s] contains invalid non-UTF8 character, " \
>   "using default\n", msg_in);
> 
> would be more specific and would help to better identify the source of the
> message as well? Similar for sbus_request_valist_check().
> 
> But to fix the issue at hand and not to delay the release this can of
> course be added later as well.

No, I think it's important to have good debug messages. I used the one
you suggested, there are no other changes in the patches.
>From 9f50257515cdb0e839fa31130dbfa5c5454225d7 Mon Sep 17 00:00:00 2001
From: Jakub Hrozek 
Date: Fri, 6 Nov 2015 12:33:59 +0100
Subject: [PATCH 1/3] DP: Drop dp_pam_err_to_string

Resolves:
https://fedorahosted.org/sssd/ticket/2861

All back end requests were using pam_strerror() to print additional info
about why request failed. Since pam_strerror() returns localized message
and we don't know the locale beforehand, this message failed to be
transferred through D-Bus, resulting in a crash.
---
 src/providers/data_provider_be.c | 75 
 1 file changed, 23 insertions(+), 52 deletions(-)

diff --git a/src/providers/data_provider_be.c b/src/providers/data_provider_be.c
index 
104016ca19c093dcbb4b0c1011f06e4bb4e2a49c..5f299a7209d3d1f1e83c625ae9db6d731a5cf5ab
 100644
--- a/src/providers/data_provider_be.c
+++ b/src/providers/data_provider_be.c
@@ -115,6 +115,23 @@ struct bet_queue_item {
 
 };
 
+static const char *dp_err_to_string(int dp_err_type)
+{
+switch (dp_err_type) {
+case DP_ERR_OK:
+return "Success";
+case DP_ERR_OFFLINE:
+return "Provider is Offline";
+case DP_ERR_TIMEOUT:
+return "Request timed out";
+case DP_ERR_FATAL:
+default:
+return "Internal Error";
+}
+
+return "Unknown Error";
+}
+
 #define REQ_PHASE_ACCESS 0
 #define REQ_PHASE_SELINUX 1
 
@@ -616,37 +633,6 @@ static void be_reset_offline(struct be_ctx *ctx)
 be_run_online_cb(ctx);
 }
 
-static char *dp_pam_err_to_string(TALLOC_CTX *memctx, int dp_err_type, int 
errnum)
-{
-switch (dp_err_type) {
-case DP_ERR_OK:
-return talloc_asprintf(memctx, "Success (%s)",
-   pam_strerror(NULL, errnum));
-break;
-
-case DP_ERR_OFFLINE:
-return talloc_asprintf(memctx,
-   "Provider is Offline (%s)",
-   pam_strerror(NULL, errnum));
-break;
-
-case DP_ERR_TIMEOUT:
-return talloc_asprintf(memctx,
-   "Request timed out (%s)",
-   pam_strerror(NULL, errnum));
-break;
-
-case DP_ERR_FATAL:
-default:
-return talloc_asprintf(memctx,
-   "Internal Error (%s)",
-   pam_strerror(NULL, errnum));
-break;
-}
-
-return NULL;
-}
-
 static void get_subdomains_callback(struct be_req *req,
 int dp_err_type,
 int errnum,
@@ -659,7 +645,7 @@ static void get_subdomains_callback(struct be_req *req,
 
 DEBUG(SSSDBG_TRACE_FUNC, "Backend returned: (%d, %d, %s) [%s]\n",
   dp_err_type, errnum, errstr?errstr:"",
-  dp_pam_err_to_string(req, dp_err_type, errnum));
+  dp_err_to_string(dp_err_type));
 
 be_queue_next_request(req, BET_SUBDOMAINS);
 
@@ -675,12 +661,7 @@ static void get_subdomains_callback(struct be_req *req,
 if (errstr) {
 err_msg = errstr;
 } else {
-err_msg = dp_pam_err_to_string(req, dp_err_type, errnum);
-}
-if (!err_msg) {
-DEBUG(SSSDBG_CRIT_FAILURE,
-  "Failed to set err_msg, Out of memory?\n");
-err_msg = "OOM";
+err_msg = dp_err_to_string(dp_err_type);
 }
 
 sbus_request_return_and_finish(dbus_req,
@@ -819,12 +800,7 @@ static void

Re: [SSSD] [PATCHSET] intg: sss_override

2015-11-13 Thread Nikolai Kondrashov


Hi Pavel,

On 11/13/2015 02:22 PM, Pavel Reichl wrote:

Hello Nick, thanks for the valid comments I hope I addressed them all.

I used the dictionary hint, but I didn't use it everywhere - mostly just
where the same dict would be reused in the same function.


Sure, thank you.


On 11/03/2015 04:40 PM, Nikolai Kondrashov wrote:



+
+# We can create override with name root.
+ent.assert_passwd_by_name(
+'user2',
+dict(name='root', passwd='*', uid=10020, gid=20020,
+ gecos='Overriden User 2',
+ dir='/home/ov/user2',
+ shell='/bin/ov_user2_shell'))
+
+ent.assert_passwd_by_uid(0, dict(name="root"))


Could you please add a comment to this line explaining what it tests?


Well, I can't :-). I just hit this case and I don't think this is a security
bug, but I decided to highlight it and at least canonize this behavior -
whenever implementation would change test would need to be changed too.

I can remove this part of the test - no trouble at all.


Um, do you mean you have no idea what it does :)? I'm sure you had some intent
of putting it there, couldn't you describe it in a short comment?

If I understood you right, you can just write that this line is for tracking
the particular behavior doesn't change and describe what specific behavior it
is testing - that would already be great.



Could you make each set item start on a new line, so it's easier to read?
Something like this:

 assert set(out.splitlines()) == \
set(['user1@LDAP:ov_user1:10010:20010:Overriden User 1:' \
 '/home/ov/user1:/bin/ov_user1_shell',
 'user2@LDAP:ov_user2:10020:20020:Overriden User 2:' \
 '/home/ov/user2:/bin/ov_user2_shell'])


Sorry I could not use this format as pep8 didn't like it. I had to remove
the backslash and then '2nd' part of the string could not be intended (pep8
didn't like too).


Ah, alright. I like how you rewrote it anyway, thanks :)

I have some more comments below, which are mostly regarding sorting the
multiple successive assertions so they're in some order. It would be great if
they kept to a single idea of order throughout the whole file, but if there
was just some order in all of them that would also be good.

This helps locate specific assertions in the list, check if all of them are
there, or if some are extra or wrong.

Otherwise there are just two other little problems I noticed and that's it.


+@pytest.fixture
+def env_two_users_and_group_overriden(request, ldap_conn,
+  env_two_users_and_group):
+
+# Override
+subprocess.check_call(["sss_override", "user-add", "user1",
+   "-u", "10010",
+   "-g", "20010",
+   "-n", "ov_user1",
+   "-c", "Overriden User 1",
+   "-h", "/home/ov/user1",
+   "-s", "/bin/ov_user1_shell"])
+
+subprocess.check_call(["sss_override", "user-add", "user2@LDAP",
+   "-u", "10020",
+   "-g", "20020",
+   "-n", "ov_user2",
+   "-c", "Overriden User 2",
+   "-h", "/home/ov/user2",
+   "-s", "/bin/ov_user2_shell"])
+
+# Restart SSSD so the override might take effect
+restart_sssd()
+
+# Assert entries are overriden
+user1 = dict(name='ov_user1', passwd='*', uid=10010, gid=20010,
+ gecos='Overriden User 1',
+ dir='/home/ov/user1',
+ shell='/bin/ov_user1_shell')
+
+user2 = dict(name='ov_user2', passwd='*', uid=10020, gid=20020,
+ gecos='Overriden User 2',
+ dir='/home/ov/user2',
+ shell='/bin/ov_user2_shell')
+
+ent.assert_passwd_by_name('user1', user1)
+
+ent.assert_passwd_by_name('ov_user1', user1)
+
+ent.assert_passwd_by_name('user1@LDAP', user1)
+
+ent.assert_passwd_by_name('ov_user1@LDAP', user1)
+
+ent.assert_passwd_by_name('ov_user2@LDAP', user2)
+
+ent.assert_passwd_by_name('user2@LDAP', user2)
+
+ent.assert_passwd_by_name('ov_user2', user2)
+
+ent.assert_passwd_by_name('user2', user2)
+


Do you need the empty lines between these assertions? I would rather remove
them and save some vertical space. Also, could you please sort these in some
order (not very important which), so we have something like this:

ent.assert_passwd_by_name('user1', user1)
ent.assert_passwd_by_name('user1@LDAP', user1)
ent.assert_passwd_by_name('ov_user1', user1)
ent.assert_passwd_by_name('ov_user1@LDAP', user1)
ent.assert_passwd_by_name('user2', user2)
ent.assert_passwd_by_name('user2@LDAP', user2)
ent.assert_passwd_by_name('ov_user2', user2)
ent.assert_passwd_by_name('ov_user2@LDAP', user2)

This would make it easier to locate specific assertions and check if

Re: [SSSD] [PATCHSET] intg: sss_override

2015-11-13 Thread Pavel Reichl


Hello Nick, thanks for the valid comments I hope I addressed them all.

I used the dictionary hint, but I didn't use it everywhere - mostly just where 
the same dict would be reused in the same function.


On 11/03/2015 04:40 PM, Nikolai Kondrashov wrote:



+
+# We can create override with name root.
+ent.assert_passwd_by_name(
+'user2',
+dict(name='root', passwd='*', uid=10020, gid=20020,
+ gecos='Overriden User 2',
+ dir='/home/ov/user2',
+ shell='/bin/ov_user2_shell'))
+
+ent.assert_passwd_by_uid(0, dict(name="root"))


Could you please add a comment to this line explaining what it tests?


Well, I can't :-). I just hit this case and I don't think this is a security 
bug, but I decided to highlight it and at least canonize this behavior - 
whenever implementation would change test would need to be changed too.

I can remove this part of the test - no trouble at all.



Could you make each set item start on a new line, so it's easier to read?
Something like this:

 assert set(out.splitlines()) == \
set(['user1@LDAP:ov_user1:10010:20010:Overriden User 1:' \
 '/home/ov/user1:/bin/ov_user1_shell',
 'user2@LDAP:ov_user2:10020:20020:Overriden User 2:' \
 '/home/ov/user2:/bin/ov_user2_shell'])


Sorry I could not use this format as pep8 didn't like it. I had to remove the 
backslash and then '2nd' part of the string could not be intended (pep8 didn't 
like too).
>From c3feaf3ae81f988f18c019752844398ba8f09e7c Mon Sep 17 00:00:00 2001
From: Pavel Reichl 
Date: Mon, 5 Oct 2015 10:02:05 -0400
Subject: [PATCH] intg: Add test for user and group local overrides

Introduce a new integration test for local view overrides.

Regression tests for: #2790, #2757 and #2802.

Resolves:
https://fedorahosted.org/sssd/ticket/2732
---
 src/tests/intg/Makefile.am |1 +
 src/tests/intg/ldap_local_override_test.py | 1032 
 2 files changed, 1033 insertions(+)
 create mode 100644 src/tests/intg/ldap_local_override_test.py

diff --git a/src/tests/intg/Makefile.am b/src/tests/intg/Makefile.am
index 12a4fc279e790b4432800a9ea0d7130afe16ec5e..7394997319142d581237ab8a37270bfd7bc974ca 100644
--- a/src/tests/intg/Makefile.am
+++ b/src/tests/intg/Makefile.am
@@ -6,6 +6,7 @@ dist_noinst_DATA = \
 ent.py \
 ent_test.py \
 ldap_ent.py \
+ldap_local_override_test.py \
 ldap_test.py \
 test_local_domain.py \
 util.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 ..ea05144ee86937c49bf6a9bac9572fd2f5af71f0
--- /dev/null
+++ b/src/tests/intg/ldap_local_override_test.py
@@ -0,0 +1,1032 @@
+#
+# integration test for sss_override tool
+#
+# 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
+
+
+@pytest.fixture(scope="module")
+def ds_inst(request):
+"""LDAP server instance fixture"""
+ds_inst = ds_openldap.DSOpenLDAP(
+config.PREFIX, 10389, 'dc=example,dc=com',
+"cn=admin", "Secret123")
+try:
+ds_inst.setup()
+except:
+ds_inst.teardown()
+raise
+request.addfinalizer(lambda: ds_inst.teardown())
+return ds_inst
+
+
+@pytest.fixture(scope="module")
+def ldap_conn(request, ds_inst):
+"""LDAP server connection fixture"""
+ldap_conn = ds_inst.bind()
+ldap_conn.ds_inst = ds_inst
+request.addfinalizer(lambda: ldap_conn.unbind_s())
+return ldap_conn
+
+
+def create_ldap_fixture(request, ldap_conn, ent_list):
+"""Add LDAP entries and add teardown for removing them"""
+for entry in ent_list:
+ldap_conn.add_s(entry[0], entry[1])
+
+def teardown():
+for entry in ent_list:
+ldap_conn.delete_s(entry[0])
+request.addfinalizer(teardown)
+
+
+def create_conf_fixture(request, contents):
+"""Generate sssd.conf and add teardown for removing it"""
+conf = open(config.CONF_PATH, "w")
+conf.write(contents)
+conf.close()
+os.chmod(config.CONF_PATH, stat.S_IRUSR |

Re: [SSSD] [PATCH] Guard against invalid DP messages

2015-11-13 Thread Sumit Bose

On Fri, Nov 13, 2015 at 02:08:59PM +0100, Jakub Hrozek wrote:
> On Fri, Nov 13, 2015 at 01:40:16PM +0100, Sumit Bose wrote:
> > > Hi,
> > > 
> > > attached are patches that only use sss_utf8_check() and work on RHEL-6
> > > also.
> > 
> > The patches look good and fixing the issue as expected. CI passes as
> > well http://sssd-ci.duckdns.org/logs/job/33/16/summary.html (the RHEL6
> > failure is unrelated). ACK
> > 
> > I just have a minor comment
> > 
> > >  
> > > +static const char *safe_be_req_err_msg(const char *msg_in,
> > > +   int dp_err_type)
> > > +{
> > > +bool ok;
> > > +
> > > +if (msg_in == NULL) {
> > > +/* No custom error, just use default */
> > > +return dp_err_to_string(dp_err_type);
> > > +}
> > > +
> > > +ok = sss_utf8_check((const uint8_t *) msg_in,
> > > +strlen(msg_in));
> > > +if (!ok) {
> > > +DEBUG(SSSDBG_MINOR_FAILURE,
> > > +  "Back end message is invalid, using default\n");
> > 
> > Maybe something like
> > 
> > DEBUG(SSSDBG_MINOR_FAILURE,
> >   "Back end message [%s] contains invalid non-UTF8 character, " 
> > \
> >   "using default\n", msg_in);
> > 
> > would be more specific and would help to better identify the source of the
> > message as well? Similar for sbus_request_valist_check().
> > 
> > But to fix the issue at hand and not to delay the release this can of
> > course be added later as well.
> 
> No, I think it's important to have good debug messages. I used the one
> you suggested, there are no other changes in the patches.


...


>  
> +static int sbus_request_valist_check(va_list va, int first_arg_type)
> +{
> +int ret = EOK;
> +#ifdef HAVE_DBUSBASICVALUE
> +int type;
> +va_list va_check;
> +const DBusBasicValue *value;
> +bool ok;
> +
> +va_copy(va_check, va);
> +
> +type = first_arg_type;
> +while (type != DBUS_TYPE_INVALID) {
> +value = va_arg(va_check, const DBusBasicValue*);
> +
> +if (type == DBUS_TYPE_STRING) {
> + ok = sss_utf8_check((const uint8_t *) value->str,
> +  strlen(value->str));
> + if (!ok) {
> +   DEBUG(SSSDBG_MINOR_FAILURE,
> + "Back end message [%s] contains invalid non-UTF8 " \
> + "characters");

value->str is missing, additionally sbus_request_return_and_finish() is
used by the responders as well so 'Back end message' should be changed
to 'S-Bus message' or similar.

bye,
Sumit

> + ret = EINVAL;
> + break;
> + }
> +}
> +type = va_arg(va_check, int);
> +}
> +
> +va_end(va_check);
> +#endif /* HAVE_DBUSBASICVALUE */
> +return ret;
> +}
> +
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

Re: [SSSD] [PATCH] test_sysdb_subdomains: Do use assignment in assertions

[SSSD] 1.13.2 release notes

Re: [SSSD] [PATCH] TEST: recent_valid filter testing

Re: [SSSD] [PATCH] TEST: recent_valid filter testing

Re: [SSSD] [PATCH] TEST: recent_valid filter testing

Re: [SSSD] [PATCH] SSSD: Add a new command diag_cmd

Re: [SSSD] [PATCH] TEST: recent_valid filter testing

Re: [SSSD] [PATCH] TEST: recent_valid filter testing

Re: [SSSD] [PATCH] TEST: recent_valid filter testing

Re: [SSSD] [PATCH] SSSD: Add a new command diag_cmd

Re: [SSSD] [PATCH] Guard against invalid DP messages

[SSSD] [PATCH] CONTRIB: Add a gdb pretty-printer for ldb and sysdb_attrs

Re: [SSSD] [PATCHSET] intg: sss_override

Re: [SSSD] [PATCHSET] intg: sss_override

Re: [SSSD] [PATCHSET] intg: sss_override

Re: [SSSD] [PATCHSET] intg: sss_override

Re: [SSSD] [PATCH] test_sysdb_subdomains: Do use assignment in assertions

Re: [SSSD] [PATCH] TEST: recent_valid filter testing

Re: [SSSD] [PATCH] TEST: recent_valid filter testing

Re: [SSSD] [PATCH] TEST: recent_valid filter testing

Re: [SSSD] [PATCH] TEST: recent_valid filter testing

Re: [SSSD] [PATCH] Guard against invalid DP messages

Re: [SSSD] [PATCH] Guard against invalid DP messages

Re: [SSSD] [PATCH] Guard against invalid DP messages

Re: [SSSD] [PATCH] Guard against invalid DP messages

Re: [SSSD] [PATCHSET] intg: sss_override

Re: [SSSD] [PATCHSET] intg: sss_override

Re: [SSSD] [PATCH] Guard against invalid DP messages

28 matches

Site Navigation

Mail list logo

Footer information