[PATCHES] Add SID related lookups to the NSS responder - part 2

[Sumit Bose]

Hi,

this is the second series of patches for the SID related lookups. With
these 4 patches, together with the ones send before, the FreeIPA WebUI
can do the SID-to-name lookups as described in
https://fedorahosted.org/freeipa/ticket/3302.

The patches currently only support IPA subdomain user, i.e. user and
groups from trusted domains. Upcoming patches will add support for IPA
user and support for the AD provider as well.

bye,
Sumit

[Jakub Hrozek]

On Thu, May 02, 2013 at 04:07:57PM +0200, Sumit Bose wrote:
>  Hi,
>  
>  this is the second series of patches for the SID related lookups. With
>  these 4 patches, together with the ones send before, the FreeIPA WebUI
>  can do the SID-to-name lookups as described in
>  https://fedorahosted.org/freeipa/ticket/3302.
>  
>  The patches currently only support IPA subdomain user, i.e. user and
>  groups from trusted domains. Upcoming patches will add support for IPA
>  user and support for the AD provider as well.
>  
>  bye,
>  Sumit 
Seems like the patches need rebasing on top of Pavel's recent patchset..

[Sumit Bose]

On Thu, May 02, 2013 at 07:23:11PM +0200, Jakub Hrozek wrote:
>  On Thu, May 02, 2013 at 04:07:57PM +0200, Sumit Bose wrote:
>  > Hi,
>  > 
>  > this is the second series of patches for the SID related lookups. With
>  > these 4 patches, together with the ones send before, the FreeIPA WebUI
>  > can do the SID-to-name lookups as described in
>  > https://fedorahosted.org/freeipa/ticket/3302.
>  > 
>  > The patches currently only support IPA subdomain user, i.e. user and
>  > groups from trusted domains. Upcoming patches will add support for IPA
>  > user and support for the AD provider as well.
>  > 
>  > bye,
>  > Sumit
>  
>  Seems like the patches need rebasing on top of Pavel's recent patchset.. 
rebased on current master.

bye,
Sumit
>  _______________________________________________
>  sssd-devel mailing list
>  sssd-devel(a)lists.fedorahosted.org
>  https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

[Sumit Bose]

On Thu, May 02, 2013 at 08:31:40PM +0200, Sumit Bose wrote:
>  On Thu, May 02, 2013 at 07:23:11PM +0200, Jakub Hrozek wrote:
>  > On Thu, May 02, 2013 at 04:07:57PM +0200, Sumit Bose wrote:
>  > > Hi,
>  > > 
>  > > this is the second series of patches for the SID related lookups. With
>  > > these 4 patches, together with the ones send before, the FreeIPA WebUI
>  > > can do the SID-to-name lookups as described in
>  > > https://fedorahosted.org/freeipa/ticket/3302.
>  > > 
>  > > The patches currently only support IPA subdomain user, i.e. user and
>  > > groups from trusted domains. Upcoming patches will add support for IPA
>  > > user and support for the AD provider as well.
>  > > 
>  > > bye,
>  > > Sumit
>  > 
>  > Seems like the patches need rebasing on top of Pavel's recent patchset..
>  
>  rebased on current master.
>  
>  bye,
>  Sumit 
I forgot to add some hints about how to test all this. You need a
FreeIPA server with trust to an AD domain. Since I found some issues in
the IPA extdom plugin during the development of the new API you need a
very recent build of FreeIPA with contains the patches:
- c152c9e Allow ID-to-SID mappings in the extdom plugin
- 0f43cd6 Do not store SID string in a local buffer
- 631b3cf Do not lookup up the domain too early if only the SID is known
The packages from http://jdennis.fedorapeople.org/ipa-devel/fedora/19
already have those patches.

If all this is in place and sssd with the patches is installed a SID can
be mapped to a name with the help of the python interface provided by
the last patch:

python -c 'import pysss_nss_idmap; print
pysss_nss_idmap.getnamebysid("S-1-5-21-111-222-333-513");'

For other mapping pysss_nss_idmap.getidbysid,
pysss_nss_idmap.getsidbyid and pysss_nss_idmap.getsidbyname can be used.

HTH

bye,
Sumit
>  > _______________________________________________
>  > sssd-devel mailing list
>  > sssd-devel(a)lists.fedorahosted.org
>  > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

[Jakub Hrozek]

On Fri, May 03, 2013 at 09:27:29AM +0200, Sumit Bose wrote:
>  On Thu, May 02, 2013 at 08:31:40PM +0200, Sumit Bose wrote:
>  > On Thu, May 02, 2013 at 07:23:11PM +0200, Jakub Hrozek wrote:
>  > > On Thu, May 02, 2013 at 04:07:57PM +0200, Sumit Bose wrote:
>  > > > Hi,
>  > > > 
>  > > > this is the second series of patches for the SID related lookups. With
>  > > > these 4 patches, together with the ones send before, the FreeIPA WebUI
>  > > > can do the SID-to-name lookups as described in
>  > > > https://fedorahosted.org/freeipa/ticket/3302.
>  > > > 
>  > > > The patches currently only support IPA subdomain user, i.e. user and
>  > > > groups from trusted domains. Upcoming patches will add support for IPA
>  > > > user and support for the AD provider as well.
>  > > > 
>  > > > bye,
>  > > > Sumit
>  > > 
>  > > Seems like the patches need rebasing on top of Pavel's recent patchset..
>  > 
>  > rebased on current master.
>  >  
Patch 0003 still doesn't apply on master for me.. (sorry, I think it's
because I pushed Abhishek's tests after you rebased)

>  > bye,
>  > Sumit
>  
>  I forgot to add some hints about how to test all this. You need a
>  FreeIPA server with trust to an AD domain. Since I found some issues in
>  the IPA extdom plugin during the development of the new API you need a
>  very recent build of FreeIPA with contains the patches:
>  - c152c9e Allow ID-to-SID mappings in the extdom plugin
>  - 0f43cd6 Do not store SID string in a local buffer
>  - 631b3cf Do not lookup up the domain too early if only the SID is known
>  The packages from http://jdennis.fedorapeople.org/ipa-devel/fedora/19
>  already have those patches.
>  
>  If all this is in place and sssd with the patches is installed a SID can
>  be mapped to a name with the help of the python interface provided by
>  the last patch:
>  
>  python -c 'import pysss_nss_idmap; print
> pysss_nss_idmap.getnamebysid("S-1-5-21-111-222-333-513");'
>  
>  For other mapping pysss_nss_idmap.getidbysid,
>  pysss_nss_idmap.getsidbyid and pysss_nss_idmap.getsidbyname can be used. 
Thanks, that's quite helpful.

[Sumit Bose]

On Fri, May 03, 2013 at 12:27:32PM +0200, Jakub Hrozek wrote:
>  On Fri, May 03, 2013 at 09:27:29AM +0200, Sumit Bose wrote:
>  > On Thu, May 02, 2013 at 08:31:40PM +0200, Sumit Bose wrote:
>  > > On Thu, May 02, 2013 at 07:23:11PM +0200, Jakub Hrozek wrote:
>  > > > On Thu, May 02, 2013 at 04:07:57PM +0200, Sumit Bose wrote:
>  > > > > Hi,
>  > > > > 
>  > > > > this is the second series of patches for the SID related lookups.
> With
>  > > > > these 4 patches, together with the ones send before, the FreeIPA
> WebUI
>  > > > > can do the SID-to-name lookups as described in
>  > > > > https://fedorahosted.org/freeipa/ticket/3302.
>  > > > > 
>  > > > > The patches currently only support IPA subdomain user, i.e. user and
>  > > > > groups from trusted domains. Upcoming patches will add support for
> IPA
>  > > > > user and support for the AD provider as well.
>  > > > > 
>  > > > > bye,
>  > > > > Sumit
>  > > > 
>  > > > Seems like the patches need rebasing on top of Pavel's recent
> patchset..
>  > > 
>  > > rebased on current master.
>  > > 
>  
>  Patch 0003 still doesn't apply on master for me.. (sorry, I think it's
>  because I pushed Abhishek's tests after you rebased) 
I realized that there also was a patch in my tree which might have
caused the issue. New version attached.

bye,
Sumit

>  
>  > > bye,
>  > > Sumit
>  > 
>  > I forgot to add some hints about how to test all this. You need a
>  > FreeIPA server with trust to an AD domain. Since I found some issues in
>  > the IPA extdom plugin during the development of the new API you need a
>  > very recent build of FreeIPA with contains the patches:
>  > - c152c9e Allow ID-to-SID mappings in the extdom plugin
>  > - 0f43cd6 Do not store SID string in a local buffer
>  > - 631b3cf Do not lookup up the domain too early if only the SID is known
>  > The packages from http://jdennis.fedorapeople.org/ipa-devel/fedora/19
>  > already have those patches.
>  > 
>  > If all this is in place and sssd with the patches is installed a SID can
>  > be mapped to a name with the help of the python interface provided by
>  > the last patch:
>  > 
>  > python -c 'import pysss_nss_idmap; print
> pysss_nss_idmap.getnamebysid("S-1-5-21-111-222-333-513");'
>  > 
>  > For other mapping pysss_nss_idmap.getidbysid,
>  > pysss_nss_idmap.getsidbyid and pysss_nss_idmap.getsidbyname can be used.
>  
>  Thanks, that's quite helpful.
>  _______________________________________________
>  sssd-devel mailing list
>  sssd-devel(a)lists.fedorahosted.org
>  https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

[Sumit Bose]

On Fri, May 03, 2013 at 01:31:12PM +0200, Sumit Bose wrote:
>  On Fri, May 03, 2013 at 12:27:32PM +0200, Jakub Hrozek wrote:
>  > On Fri, May 03, 2013 at 09:27:29AM +0200, Sumit Bose wrote:
>  > > On Thu, May 02, 2013 at 08:31:40PM +0200, Sumit Bose wrote:
>  > > > On Thu, May 02, 2013 at 07:23:11PM +0200, Jakub Hrozek wrote:
>  > > > > On Thu, May 02, 2013 at 04:07:57PM +0200, Sumit Bose wrote:
>  > > > > > Hi,
>  > > > > > 
>  > > > > > this is the second series of patches for the SID related
> lookups. With
>  > > > > > these 4 patches, together with the ones send before, the FreeIPA
> WebUI
>  > > > > > can do the SID-to-name lookups as described in
>  > > > > > https://fedorahosted.org/freeipa/ticket/3302.
>  > > > > > 
>  > > > > > The patches currently only support IPA subdomain user, i.e. user
> and
>  > > > > > groups from trusted domains. Upcoming patches will add support
> for IPA
>  > > > > > user and support for the AD provider as well.
>  > > > > > 
>  > > > > > bye,
>  > > > > > Sumit
>  > > > > 
>  > > > > Seems like the patches need rebasing on top of Pavel's recent
> patchset..
>  > > > 
>  > > > rebased on current master.
>  > > > 
>  > 
>  > Patch 0003 still doesn't apply on master for me.. (sorry, I think it's
>  > because I pushed Abhishek's tests after you rebased)
>  
>  I realized that there also was a patch in my tree which might have
>  caused the issue. New version attached.
>  
>  bye,
>  Sumit 
Jakub send me some review comments off-list which I added to the second
patch. New versions attached.

I added a 5th patch which adds support for tuples and Unicode to the
python API as inquired by Alexander. I didn't squeezed them because I'm
not sure if Alexander was already able to test them. This way they can
easily be dropped from this release and can be added to the next.

bye,
Sumit

[Jakub Hrozek]

On Fri, May 03, 2013 at 06:39:17PM +0200, Sumit Bose wrote:
>  On Fri, May 03, 2013 at 01:31:12PM +0200, Sumit Bose wrote:
>  > On Fri, May 03, 2013 at 12:27:32PM +0200, Jakub Hrozek wrote:
>  > > On Fri, May 03, 2013 at 09:27:29AM +0200, Sumit Bose wrote:
>  > > > On Thu, May 02, 2013 at 08:31:40PM +0200, Sumit Bose wrote:
>  > > > > On Thu, May 02, 2013 at 07:23:11PM +0200, Jakub Hrozek wrote:
>  > > > > > On Thu, May 02, 2013 at 04:07:57PM +0200, Sumit Bose wrote:
>  > > > > > > Hi,
>  > > > > > > 
>  > > > > > > this is the second series of patches for the SID related
> lookups. With
>  > > > > > > these 4 patches, together with the ones send before, the
> FreeIPA WebUI
>  > > > > > > can do the SID-to-name lookups as described in
>  > > > > > > https://fedorahosted.org/freeipa/ticket/3302.
>  > > > > > > 
>  > > > > > > The patches currently only support IPA subdomain user, i.e.
> user and
>  > > > > > > groups from trusted domains. Upcoming patches will add
> support for IPA
>  > > > > > > user and support for the AD provider as well.
>  > > > > > > 
>  > > > > > > bye,
>  > > > > > > Sumit
>  > > > > > 
>  > > > > > Seems like the patches need rebasing on top of Pavel's
> recent patchset..
>  > > > > 
>  > > > > rebased on current master.
>  > > > > 
>  > > 
>  > > Patch 0003 still doesn't apply on master for me.. (sorry, I think it's
>  > > because I pushed Abhishek's tests after you rebased)
>  > 
>  > I realized that there also was a patch in my tree which might have
>  > caused the issue. New version attached.
>  > 
>  > bye,
>  > Sumit
>  
>  Jakub send me some review comments off-list which I added to the second
>  patch. New versions attached.
>  
>  I added a 5th patch which adds support for tuples and Unicode to the
>  python API as inquired by Alexander. I didn't squeezed them because I'm
>  not sure if Alexander was already able to test them. This way they can
>  easily be dropped from this release and can be added to the next.
>  
>  bye,
>  Sumit 
Patches 1-4: Ack

I haven't reviewed patch #5 yet.

[Jakub Hrozek]

On Fri, May 03, 2013 at 07:59:07PM +0200, Jakub Hrozek wrote:
>  On Fri, May 03, 2013 at 06:39:17PM +0200, Sumit Bose wrote:
>  > On Fri, May 03, 2013 at 01:31:12PM +0200, Sumit Bose wrote:
>  > > On Fri, May 03, 2013 at 12:27:32PM +0200, Jakub Hrozek wrote:
>  > > > On Fri, May 03, 2013 at 09:27:29AM +0200, Sumit Bose wrote:
>  > > > > On Thu, May 02, 2013 at 08:31:40PM +0200, Sumit Bose wrote:
>  > > > > > On Thu, May 02, 2013 at 07:23:11PM +0200, Jakub Hrozek wrote:
>  > > > > > > On Thu, May 02, 2013 at 04:07:57PM +0200, Sumit Bose
> wrote:
>  > > > > > > > Hi,
>  > > > > > > > 
>  > > > > > > > this is the second series of patches for the SID
> related lookups. With
>  > > > > > > > these 4 patches, together with the ones send before,
> the FreeIPA WebUI
>  > > > > > > > can do the SID-to-name lookups as described in
>  > > > > > > > https://fedorahosted.org/freeipa/ticket/3302.
>  > > > > > > > 
>  > > > > > > > The patches currently only support IPA subdomain user,
> i.e. user and
>  > > > > > > > groups from trusted domains. Upcoming patches will add
> support for IPA
>  > > > > > > > user and support for the AD provider as well.
>  > > > > > > > 
>  > > > > > > > bye,
>  > > > > > > > Sumit
>  > > > > > > 
>  > > > > > > Seems like the patches need rebasing on top of Pavel's
> recent patchset..
>  > > > > > 
>  > > > > > rebased on current master.
>  > > > > > 
>  > > > 
>  > > > Patch 0003 still doesn't apply on master for me.. (sorry, I think
> it's
>  > > > because I pushed Abhishek's tests after you rebased)
>  > > 
>  > > I realized that there also was a patch in my tree which might have
>  > > caused the issue. New version attached.
>  > > 
>  > > bye,
>  > > Sumit
>  > 
>  > Jakub send me some review comments off-list which I added to the second
>  > patch. New versions attached.
>  > 
>  > I added a 5th patch which adds support for tuples and Unicode to the
>  > python API as inquired by Alexander. I didn't squeezed them because I'm
>  > not sure if Alexander was already able to test them. This way they can
>  > easily be dropped from this release and can be added to the next.
>  > 
>  > bye,
>  > Sumit
>  
>  Patches 1-4: Ack
>  
>  I haven't reviewed patch #5 yet. 
Patches 1-4 were pushed to master.

[Jakub Hrozek]

On Fri, May 03, 2013 at 06:39:17PM +0200, Sumit Bose wrote:
>  I added a 5th patch which adds support for tuples and Unicode to the
>  python API as inquired by Alexander. I didn't squeezed them because I'm
>  not sure if Alexander was already able to test them. This way they can
>  easily be dropped from this release and can be added to the next.
>  
>  bye,
>  Sumit 
The first four patches were pushed upstream already. See below for some
comments about the last patch.

Sorry at first I thought this patch only handled the list input, so I
split Alexander's comments into a separate ticket. I will also mark the
second one (#1914) as assigned to you and set the patch boolean.

>  From 4ab0e369d28e3febcc14de2e0801b0e577fc823d Mon Sep 17 00:00:00
> 2001
>  From: Sumit Bose <sbose(a)redhat.com&gt;
>  Date: Fri, 3 May 2013 14:05:23 +0200
>  Subject: [PATCH 5/5] Add support for tuples and unicode pysss_nss_idmap.so
>  
>  This patch allows tuples as well as lists as input and adds support for
>  Unicode objects as input and always returns the results as Unicode
>  objects.
>  ---
>   src/python/pysss_nss_idmap.c |   64 ++++++++++++++++++++++++++++++-----------
>   1 files changed, 47 insertions(+), 17 deletions(-)
>  
>  diff --git a/src/python/pysss_nss_idmap.c b/src/python/pysss_nss_idmap.c
>  index dd1fb4c..edd0c00 100644
>  --- a/src/python/pysss_nss_idmap.c
>  +++ b/src/python/pysss_nss_idmap.c
>  @@ -64,6 +64,19 @@ static int add_dict(PyObject *py_result, PyObject *key, PyObject
> *res_type,
>       return ret;
>   }
>    
I think this function would be a nice addition to src/util/sss_python.c 

>  +static char *py_string_or_unicode_as_string(PyObject *inp)
>  +{
>  +    PyObject *py_str = NULL;
>  +
>  +    if (PyUnicode_Check(inp)) {
>  +        py_str = PyUnicode_AsUTF8String(inp);
>  +    } else {
>  +        py_str = inp; 
Now the function relies on "inp" being a string, would it make sense to
call PyString_Check() here in an else-if branch and raise a TypeError in
the else branch? Maybe with:
    PyErr_Format(PyExc_TypeError, "input must be unicode or a string");

>  +    }
>  +
>  +    return PyString_AS_STRING(py_str);
>  +}
>  +
>   static int do_getsidbyname(PyObject *py_result, PyObject *py_name)
>   {
>       int ret;
>  @@ -71,7 +84,7 @@ static int do_getsidbyname(PyObject *py_result, PyObject *py_name)
>       char *sid = NULL;
>       enum sss_id_type id_type;
>   
>  -    name = PyString_AS_STRING(py_name);
>  +    name = py_string_or_unicode_as_string(py_name);
>       if (name == NULL) {
>           return EINVAL;
>       }
>  @@ -79,7 +92,7 @@ static int do_getsidbyname(PyObject *py_result, PyObject *py_name)
>       ret = sss_nss_getsidbyname(name, &sid, &id_type);
>       if (ret == 0) {
>           ret = add_dict(py_result, py_name, PyString_FromString(SSS_SID_KEY),
>  -                       PyString_FromString(sid), PyInt_FromLong(id_type));
>  +                       PyUnicode_FromString(sid), PyInt_FromLong(id_type));
>       }
>       free(sid);
>   
>  @@ -93,7 +106,7 @@ static int do_getnamebysid(PyObject *py_result, PyObject *py_sid)
>       char *name = NULL;
>       enum sss_id_type id_type;
>   
>  -    sid = PyString_AS_STRING(py_sid);
>  +    sid = py_string_or_unicode_as_string(py_sid);
>       if (sid == NULL) {
>           return EINVAL;
>       }
>  @@ -101,7 +114,7 @@ static int do_getnamebysid(PyObject *py_result, PyObject *py_sid)
>       ret = sss_nss_getnamebysid(sid, &name, &id_type);
>       if (ret == 0) {
>           ret = add_dict(py_result, py_sid, PyString_FromString(SSS_NAME_KEY),
>  -                       PyString_FromString(name), PyInt_FromLong(id_type));
>  +                       PyUnicode_FromString(name), PyInt_FromLong(id_type));
>       }
>       free(name);
>   
>  @@ -121,8 +134,8 @@ static int do_getsidbyid(PyObject *py_result, PyObject *py_id)
>           id = PyInt_AS_LONG(py_id);
>       } else if (PyLong_Check(py_id)) {
>           id = PyLong_AsLong(py_id);
>  -    } else if (PyString_Check(py_id)) {
>  -        id_str = PyString_AS_STRING(py_id);
>  +    } else if (PyString_Check(py_id) || PyUnicode_Check(py_id)) { 
If we moved the string check to py_string_or_unicode_as_string() this
check would be obsolete.

>  +        id_str = py_string_or_unicode_as_string(py_id);
>           errno = 0;
>           id = strtol(id_str, &endptr, 10);
>           if (errno != 0 || *endptr != '\0') {
>  @@ -139,7 +152,7 @@ static int do_getsidbyid(PyObject *py_result, PyObject *py_id)
>       ret = sss_nss_getsidbyid((uint32_t) id, &sid, &id_type);
>       if (ret == 0) {
>           ret = add_dict(py_result, py_id, PyString_FromString(SSS_SID_KEY),
>  -                       PyString_FromString(sid), PyInt_FromLong(id_type));
>  +                       PyUnicode_FromString(sid), PyInt_FromLong(id_type));
>       }
>       free(sid);
>   
>  @@ -153,7 +166,7 @@ static int do_getidbysid(PyObject *py_result, PyObject *py_sid)
>       enum sss_id_type id_type;
>       int ret;
>   
>  -    sid = PyString_AS_STRING(py_sid);
>  +    sid = py_string_or_unicode_as_string(py_sid);
>       if (sid == NULL) {
>           return EINVAL;
>       }
>  @@ -190,22 +203,29 @@ static int do_lookup(enum lookup_type type, PyObject *py_result,
>       return ENOSYS;
>   }
>   
>  +typedef Py_ssize_t (*py_lt_size_fn_t)(PyObject *);
>  +typedef PyObject *(*py_lt_get_item_fn_t)(PyObject *, Py_ssize_t); 
I haven't actually tested it but I think you could use
PySequence_GetItem() and PySequence_Size() instead, I suspect they would
work on both lists and tuples.

>   static PyObject *check_args(enum lookup_type type, PyObject *args)
>   {
>       PyObject *obj, *py_value;
>       int ret;
>       Py_ssize_t len, i;
>       PyObject *py_result;
>  +    py_lt_size_fn_t lt_size = NULL;
>  +    py_lt_get_item_fn_t lt_get_item = NULL;
>  +    bool multival = false;
>   
>       if (!PyArg_ParseTuple(args, sss_py_const_p(char, "O"), &obj)) {
>           PyErr_Format(PyExc_ValueError, "Unable to retrieve argument\n");
>           return NULL;
>       }
>   
>  -    if (!(PyList_Check(obj) || PyString_Check(obj) ||
>  +    if (!(PyList_Check(obj) || PyTuple_Check(obj) ||
>  +          PyString_Check(obj) || PyUnicode_Check(obj) ||
>             (type == SIDBYID && (PyInt_Check(obj) || PyLong_Check(obj))))) {
>           PyErr_Format(PyExc_ValueError,
>  -                     "Only string, long or list of them are accepted\n");
>  +                     "Only string, long or list or tuples of them " \
>  +                     "are accepted\n");
>           return NULL;
>       }
>   
>  @@ -218,11 +238,21 @@ static PyObject *check_args(enum lookup_type type, PyObject *args)
>       }
>   
>       if (PyList_Check(obj)) {
>  -        len = PyList_Size(obj);
>  +        lt_size = PyList_Size;
>  +        lt_get_item = PyList_GetItem;
>  +        multival = true;
>  +    } else if (PyTuple_Check(obj)) {
>  +        lt_size = PyTuple_Size;
>  +        lt_get_item = PyTuple_GetItem;
>  +        multival = true;
>  +    }
>  +
>  +    if (multival) {
>  +        len = lt_size(obj);
>           for(i=0; i < len; i++) {
>  -            py_value = PyList_GetItem(obj, i);
>  +            py_value = lt_get_item(obj, i);
>               if ((py_value != NULL) &&
>  -                (PyString_Check(py_value) ||
>  +                (PyString_Check(py_value) || PyUnicode_Check(py_value) ||
>                    (type == SIDBYID &&
>                     (PyInt_Check(py_value) || PyLong_Check(py_value))))) {
>                   ret = do_lookup(type, py_result, py_value);

[Sumit Bose]

On Fri, May 10, 2013 at 05:34:27PM +0200, Jakub Hrozek wrote:
>  On Fri, May 03, 2013 at 06:39:17PM +0200, Sumit Bose wrote:
>  > I added a 5th patch which adds support for tuples and Unicode to the
>  > python API as inquired by Alexander. I didn't squeezed them because I'm
>  > not sure if Alexander was already able to test them. This way they can
>  > easily be dropped from this release and can be added to the next.
>  > 
>  > bye,
>  > Sumit
>  
>  The first four patches were pushed upstream already. See below for some
>  comments about the last patch.
>  
>  Sorry at first I thought this patch only handled the list input, so I
>  split Alexander's comments into a separate ticket. I will also mark the
>  second one (#1914) as assigned to you and set the patch boolean. 
Thank you for the review, I've included all your comments, new version
attached.

bye,
Sumit

[Jakub Hrozek]

On Mon, May 13, 2013 at 11:03:31AM +0200, Sumit Bose wrote:
>  On Fri, May 10, 2013 at 05:34:27PM +0200, Jakub Hrozek wrote:
>  > On Fri, May 03, 2013 at 06:39:17PM +0200, Sumit Bose wrote:
>  > > I added a 5th patch which adds support for tuples and Unicode to the
>  > > python API as inquired by Alexander. I didn't squeezed them because
> I'm
>  > > not sure if Alexander was already able to test them. This way they can
>  > > easily be dropped from this release and can be added to the next.
>  > > 
>  > > bye,
>  > > Sumit
>  > 
>  > The first four patches were pushed upstream already. See below for some
>  > comments about the last patch.
>  > 
>  > Sorry at first I thought this patch only handled the list input, so I
>  > split Alexander's comments into a separate ticket. I will also mark the
>  > second one (#1914) as assigned to you and set the patch boolean.
>  
>  Thank you for the review, I've included all your comments, new version
>  attached.
>  
>  bye,
>  Sumit 
Ack

[Jakub Hrozek]

On Mon, May 13, 2013 at 05:30:03PM +0200, Jakub Hrozek wrote:
>  On Mon, May 13, 2013 at 11:03:31AM +0200, Sumit Bose wrote:
>  > On Fri, May 10, 2013 at 05:34:27PM +0200, Jakub Hrozek wrote:
>  > > On Fri, May 03, 2013 at 06:39:17PM +0200, Sumit Bose wrote:
>  > > > I added a 5th patch which adds support for tuples and Unicode to the
>  > > > python API as inquired by Alexander. I didn't squeezed them because
> I'm
>  > > > not sure if Alexander was already able to test them. This way they can
>  > > > easily be dropped from this release and can be added to the next.
>  > > > 
>  > > > bye,
>  > > > Sumit
>  > > 
>  > > The first four patches were pushed upstream already. See below for some
>  > > comments about the last patch.
>  > > 
>  > > Sorry at first I thought this patch only handled the list input, so I
>  > > split Alexander's comments into a separate ticket. I will also mark the
>  > > second one (#1914) as assigned to you and set the patch boolean.
>  > 
>  > Thank you for the review, I've included all your comments, new version
>  > attached.
>  > 
>  > bye,
>  > Sumit
>  
>  Ack 
I amended the commit message to include both #1905 and #1914 and pushed
the patch to master.

[Alexander Bokovoy]

On Thu, 02 May 2013, Sumit Bose wrote:
> On Thu, May 02, 2013 at 07:23:11PM +0200, Jakub Hrozek wrote:
> > On Thu, May 02, 2013 at 04:07:57PM +0200, Sumit Bose wrote:
> > > Hi,
> > >
> > > this is the second series of patches for the SID related lookups. With
> > > these 4 patches, together with the ones send before, the FreeIPA WebUI
> > > can do the SID-to-name lookups as described in
> > > https://fedorahosted.org/freeipa/ticket/3302.
> > >
> > > The patches currently only support IPA subdomain user, i.e. user and
> > > groups from trusted domains. Upcoming patches will add support for IPA
> > > user and support for the AD provider as well.
> > >
> > > bye,
> > > Sumit
> >
> > Seems like the patches need rebasing on top of Pavel's recent patchset..
>
> rebased on current master. 
Regarding Python interface I have made few suggestions on
IRC which are not
yet present in the proposed patches:

1. Support tuple in addition to list as arguments. IPA Python code
provides tuples to its methods when an argument is declared as list (+ or * suffix),
to allow position-significant arguments.

2. Support also Unicode strings and return them by default. Again, IPA
Python code by default deals with Unicode strings in arguments (so, in
upcoming 'trust-resolve' command '--sids' argument will give tuple of
unicode strings to the Python method).

Additionally, returning unicode strings is important due to the fact
that user/group names are not limited to ASCII. More to that, Windows is
known to localise group and user names to target language, so that for
example, in Russian version a group with RID 498, "enterprise read-only
domain controllers" is actually "Контроллеры домена предприятия — только
чтение".

-- 
/ Alexander Bokovoy

[Jakub Hrozek]

On Fri, May 03, 2013 at 04:46:58PM +0300, Alexander Bokovoy wrote:
>  On Thu, 02 May 2013, Sumit Bose wrote:
>  >On Thu, May 02, 2013 at 07:23:11PM +0200, Jakub Hrozek wrote:
>  >>On Thu, May 02, 2013 at 04:07:57PM +0200, Sumit Bose wrote:
>  >>> Hi,
>  >>>
>  >>> this is the second series of patches for the SID related lookups. With
>  >>> these 4 patches, together with the ones send before, the FreeIPA WebUI
>  >>> can do the SID-to-name lookups as described in
>  >>> https://fedorahosted.org/freeipa/ticket/3302.
>  >>>
>  >>> The patches currently only support IPA subdomain user, i.e. user and
>  >>> groups from trusted domains. Upcoming patches will add support for IPA
>  >>> user and support for the AD provider as well.
>  >>>
>  >>> bye,
>  >>> Sumit
>  >>
>  >>Seems like the patches need rebasing on top of Pavel's recent patchset..
>  >
>  >rebased on current master.
>  Regarding Python interface I have made few suggestions on IRC which are not
>  yet present in the proposed patches:
>  
>  1. Support tuple in addition to list as arguments. IPA Python code
>  provides tuples to its methods when an argument is declared as list (+ or * suffix),
>  to allow position-significant arguments.
>  
>  2. Support also Unicode strings and return them by default. Again, IPA
>  Python code by default deals with Unicode strings in arguments (so, in
>  upcoming 'trust-resolve' command '--sids' argument will give tuple of
>  unicode strings to the Python method).
>  
>  Additionally, returning unicode strings is important due to the fact
>  that user/group names are not limited to ASCII. More to that, Windows is
>  known to localise group and user names to target language, so that for
>  example, in Russian version a group with RID 498, "enterprise read-only
>  domain controllers" is actually "Контроллеры домена предприятия — только
>  чтение". 
I think we can do these changes post-beta1 though, I would like to
include this patchset in the beta1 and according to the conversation we
had yesterday on the SSSD meeting, these patches already are enough to
make the feature testable.

Do you see any problem in filing tickets for the suggestion above? We
could then also assign these enhancements to somebody else than Sumit
and save his valuable time.

[Alexander Bokovoy]

On Fri, 03 May 2013, Jakub Hrozek wrote:
> On Fri, May 03, 2013 at 04:46:58PM +0300, Alexander Bokovoy wrote:
> > On Thu, 02 May 2013, Sumit Bose wrote:
> > >On Thu, May 02, 2013 at 07:23:11PM +0200, Jakub Hrozek wrote:
> > >>On Thu, May 02, 2013 at 04:07:57PM +0200, Sumit Bose wrote:
> > >>> Hi,
> > >>>
> > >>> this is the second series of patches for the SID related lookups. With
> > >>> these 4 patches, together with the ones send before, the FreeIPA WebUI
> > >>> can do the SID-to-name lookups as described in
> > >>> https://fedorahosted.org/freeipa/ticket/3302.
> > >>>
> > >>> The patches currently only support IPA subdomain user, i.e. user and
> > >>> groups from trusted domains. Upcoming patches will add support for IPA
> > >>> user and support for the AD provider as well.
> > >>>
> > >>> bye,
> > >>> Sumit
> > >>
> > >>Seems like the patches need rebasing on top of Pavel's recent patchset..
> > >
> > >rebased on current master.
> > Regarding Python interface I have made few suggestions on IRC which are not
> > yet present in the proposed patches:
> >
> > 1. Support tuple in addition to list as arguments. IPA Python code
> > provides tuples to its methods when an argument is declared as list (+ or * suffix),
> > to allow position-significant arguments.
> >
> > 2. Support also Unicode strings and return them by default. Again, IPA
> > Python code by default deals with Unicode strings in arguments (so, in
> > upcoming 'trust-resolve' command '--sids' argument will give tuple
> of
> > unicode strings to the Python method).
> >
> > Additionally, returning unicode strings is important due to the fact
> > that user/group names are not limited to ASCII. More to that, Windows is
> > known to localise group and user names to target language, so that for
> > example, in Russian version a group with RID 498, "enterprise read-only
> > domain controllers" is actually "Контроллеры домена предприятия — только
> > чтение".
>
> I think we can do these changes post-beta1 though, I would like to
> include this patchset in the beta1 and according to the conversation we
> had yesterday on the SSSD meeting, these patches already are enough to
> make the feature testable. 
True. I'm actually accomodating IPA code to pass
normal string and list
to pysss_nss_idmap right now so there should be no issue. Since I'm
using getnamebysid(), it is guaranteed that input is never beyond ASCII
(it is SID, after all) and for the output we can fix it post FreeIPA
3.2 before Fedora 19 release.


> Do you see any problem in filing tickets for the suggestion above? We
> could then also assign these enhancements to somebody else than Sumit
> and save his valuable time. 
Absolutely no problem. I'll file tickets.

-- 
/ Alexander Bokovoy

[Jakub Hrozek]

On Fri, May 03, 2013 at 05:15:32PM +0300, Alexander Bokovoy wrote:
>  On Fri, 03 May 2013, Jakub Hrozek wrote:
>  >On Fri, May 03, 2013 at 04:46:58PM +0300, Alexander Bokovoy wrote:
>  >>On Thu, 02 May 2013, Sumit Bose wrote:
>  >>>On Thu, May 02, 2013 at 07:23:11PM +0200, Jakub Hrozek wrote:
>  >>>>On Thu, May 02, 2013 at 04:07:57PM +0200, Sumit Bose wrote:
>  >>>>> Hi,
>  >>>>>
>  >>>>> this is the second series of patches for the SID related lookups.
> With
>  >>>>> these 4 patches, together with the ones send before, the FreeIPA
> WebUI
>  >>>>> can do the SID-to-name lookups as described in
>  >>>>> https://fedorahosted.org/freeipa/ticket/3302.
>  >>>>>
>  >>>>> The patches currently only support IPA subdomain user, i.e. user
> and
>  >>>>> groups from trusted domains. Upcoming patches will add support for
> IPA
>  >>>>> user and support for the AD provider as well.
>  >>>>>
>  >>>>> bye,
>  >>>>> Sumit
>  >>>>
>  >>>>Seems like the patches need rebasing on top of Pavel's recent
> patchset..
>  >>>
>  >>>rebased on current master.
>  >>Regarding Python interface I have made few suggestions on IRC which are not
>  >>yet present in the proposed patches:
>  >>
>  >>1. Support tuple in addition to list as arguments. IPA Python code
>  >>provides tuples to its methods when an argument is declared as list (+ or *
> suffix),
>  >>to allow position-significant arguments.
>  >>
>  >>2. Support also Unicode strings and return them by default. Again, IPA
>  >>Python code by default deals with Unicode strings in arguments (so, in
>  >>upcoming 'trust-resolve' command '--sids' argument will give
> tuple of
>  >>unicode strings to the Python method).
>  >>
>  >>Additionally, returning unicode strings is important due to the fact
>  >>that user/group names are not limited to ASCII. More to that, Windows is
>  >>known to localise group and user names to target language, so that for
>  >>example, in Russian version a group with RID 498, "enterprise read-only
>  >>domain controllers" is actually "Контроллеры домена предприятия —
> только
>  >>чтение".
>  >
>  >I think we can do these changes post-beta1 though, I would like to
>  >include this patchset in the beta1 and according to the conversation we
>  >had yesterday on the SSSD meeting, these patches already are enough to
>  >make the feature testable.
>  True. I'm actually accomodating IPA code to pass normal string and list
>  to pysss_nss_idmap right now so there should be no issue. Since I'm
>  using getnamebysid(), it is guaranteed that input is never beyond ASCII
>  (it is SID, after all) and for the output we can fix it post FreeIPA
>  3.2 before Fedora 19 release.
>  
>  
>  >Do you see any problem in filing tickets for the suggestion above? We
>  >could then also assign these enhancements to somebody else than Sumit
>  >and save his valuable time.
>  Absolutely no problem. I'll file tickets. 
Alexander filed:
https://fedorahosted.org/sssd/ticket/1905

I sent some smallish changes to Sumit in a private e-mail (it was easier
to do the change locally than to describe on the list), but the patches
are mostly looking good to me.

IIRC the changes only affected patch #2, patches 1,3 and 4 can be acked.

The code builds, tests pass, distcheck and make rpms also work fine. I
will just run a couple of sanity tests now.