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

[Martin Kosek]

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-abbra-0103-2-Resolve-SIDs-in-Web-UI.patch
Type: text/x-patch
Size: 9928 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20130506/700a9227/attachment.bin>