[Freeipa-devel] [PATCH] Resolve SIDs in Web UI

[Martin Kosek]

On 05/06/2013 07:03 PM, Sumit Bose wrote:
> On Mon, May 06, 2013 at 05:55:35PM +0200, Martin Kosek wrote:
>> On 05/06/2013 01:28 PM, Martin Kosek wrote:
>>> On 05/04/2013 07:13 AM, Alexander Bokovoy wrote:
>>>> On Fri, 03 May 2013, Sumit Bose wrote:
>>>>> On Fri, May 03, 2013 at 09:46:47PM +0300, Alexander Bokovoy wrote:
>>>>>> Hi!
>>>>>>
>>>>>> Attached are patches to allow resolving SIDs in Web UI in external
>>>>>> membership panel for groups. Please see more detailed description in the
>>>>>> main patch.
>>>>>>
>>>>>> I haven't rebased it yet on top of Petr's Web UI rework, hopefully it
>>>>>> should be simple.
>>>>>>
>>>>>> https://fedorahosted.org/freeipa/ticket/3302
>>>>>>
>>>>>> Since framework doesn't allow to hide commands from CLI, underlying
>>>>>> command is usable from CLI too:
>>>>>> # ipa trust-resolve
>>>>>> --sids=S-1-5-21-3502988750-125904550-3683905862-{500,512,498}
>>>>>>   Name: enterprise read-only domain controllers at ad.lan
>>>>>>   SID: S-1-5-21-3502988750-125904550-3683905862-498
>>>>>>
>>>>>>   Name: administrator at ad.lan
>>>>>>   SID: S-1-5-21-3502988750-125904550-3683905862-500
>>>>>>
>>>>>>   Name: domain admins at ad.lan
>>>>>>   SID: S-1-5-21-3502988750-125904550-3683905862-512
>>>>>>
>>>>>> --
>>>>>> / Alexander Bokovoy
>>>>>> +        try:
>>>>>> +            sids = map(lambda x: str(x), options['sids'])
>>>>>> +            xlate = pysss_nss_idmap.getnamebysid(sids)
>>>>>
>>>>> The latest version, which is already committed to sssd, return a dict.
>>>>> The output of ipa trust-resolve now look like:
>>>>>
>>>>> [root at ipa18-devel ~]# ipa trust-resolve
>>>>> --sids=S-1-5-21-3090815309-2627318493-3395719201-{498,500,513}
>>>>>   Name: {'type': 3, 'name': u'administrator at ad18.ipa18.devel'}
>>>>>   SID: S-1-5-21-3090815309-2627318493-3395719201-500
>>>>>
>>>>>   Name: {'type': 2, 'name': u'enterprise read-only domain
>>>>> controllers at ad18.ipa18.devel'}
>>>>>   SID: S-1-5-21-3090815309-2627318493-3395719201-498
>>>>>
>>>>>   Name: {'type': 2, 'name': u'domain users at ad18.ipa18.devel'}
>>>>>   SID: S-1-5-21-3090815309-2627318493-3395719201-513
>>>>>
>>>>>> +            for sid in xlate:
>>>>>> +           entry = dict()
>>>>>> +               entry['sid'] = [unicode(sid)]
>>>>>> +               entry['name'] = [unicode(xlate[sid])]
>>>>>
>>>>> I think you need  entry['name'] =
>>>>> [unicode(xlate[sid][pysss_nss_idmap.NAME_KEY])]
>>>>> here.
>>>> Fixed, thanks!
>>>> I also added type conversion to a text (user, group, both). The type is not
>>>> shown by default
>>>> in CLI but is available through --all option. We might consider using it
>>>> in Web UI for visual hint about the name nature.
>>>>
>>>>> I tried with firefox, but the SIDs of the external members are not
>>>>> resolved. Do I have to clean any firefox cache?
>>>> No, you do not. When picking up changes from my development VM, I
>>>> omitted one chunk in group.js where sid_facet was actually taken in use.
>>>> Without that one nothing is used.
>>>>
>>>> Updated patch 0103 is attached, tested against sssd in ipa-devel repo
>>>> which already includes your patches.
>>>>
>>>
>>> Thanks for the patch! Still, I have few comments:
>>>
>>> 1) Exception should be raised instead of returning empty result:
>>>
>>> +        if not _nss_idmap_installed:
>>> +            return dict(result=result)
>>>
>>> Otherwise people will be confused what's wrong.
>>>
>>> 2) Why do we hide error raised in SID processing code?
>>>
>>> ...
>>> +        except ValueError, e:
>>> +            pass
>>> ...
>>>
>>> I think that the try-catch should be as localized possible, ideally in the FOR
>>> loop. If processing of the second SID out of 10 fails, just one SID would be
>>> return, with no additional error. People will be confused what's wrong:
>>>
>>> # ipa trust-resolve --sids S-1-5-21-3035198329-144811719-1378114514-500
>>> #
>>>
>>> This does not really tell me what's wrong.
>>>
>>> Could we rather return all requested SIDs either with a proper result or with a
>>> respective error? This is how I would image the translation to look like:
>>>
>>> ...
>>> try:
>>>      sids = map(lambda x: str(x), options['sids'])
>>>      xlate = pysss_nss_idmap.getnamebysid(sids)
>>> except SomeError, e:
>>>      raise SomeException(e)
>>>
>>> for sid in xlate:
>>>      entry = dict()
>>>      entry['sid'] = ...
>>>      try:
>>>          name = ...
>>>          type = ...
>>>          entry['name'], entry['type'] = name, type
>>>      except SomeError, e:
>>>          entry['failedtranslation'] = unicode(e)
>>>      results.append(entry)
>>> ...
>>>
>>>
>>> I filed ticket for SSSD part of the issue:
>>> https://fedorahosted.org/sssd/ticket/1911
>>>
>>> 3) Tab/Space indentation mix:
>>>
>>> +            for sid in xlate:
>>> +	       entry = dict()
>>> +               entry['sid'] = [unicode(sid)]
>>>
>>>
>>> 4) Unneeded import:
>>>   from ipalib import api, Str, StrEnum, Password, DefaultFrom, _, ngettext, Object
>>> +from types import NoneType
>>>   from ipalib.parameters import Enu
>>>
>>>
>>> Martin
>>>
>>
>> As Alexander is not here ATM, sending updated patch based on current master
>> branch (with Web UI refactoring) which also includes few squashes:
>> - fix for my point 3)
>> - fix for my point 4)
>> - squashed Petr Vobornik's Web UI cleanups
>>
>> I tested it and it worked fine. As for the points 1) and 2) I will file a
>> ticket, these are not critical.
>>
>> Martin
>
> Patch is working as expected. So ACK from my side for the functional
> part.
>
> bye,
> Sumit
>

Pushed to master.
Martin