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

[Sumit Bose]

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