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

[Martin Kosek]

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