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

Mon May 6 11:48:19 UTC 2013

Hello,

here is review of Web UI part.

ACK for abbra-102, it's a fix of error (typo), but it won't probably 
have any impact. Because links associated with 'a' elements are 
different than the resulting ones.

NACK for abbra-103:
NACK for abbra-pvoborni... (is included in new abbra-103)

Are CLI and IPA-API parts of old abbra-103 in some different patch? We 
should make proper patches from the API/CLI part and WebUI part. Because 
of this I didn't test following fixes by using installed IPA with 
established trust.

Attaching a diff with fixes for following errors of abbra-103:

1) There are jslint errors (missing semicolons, extra semicolons, 
trailing commas) (run `jslint -conf jsl.conf` in install/ui dir)

Funny thing, I do the exact opposite mistakes when writing python code.

2) Do not use deferred directly as a value, use promise instead:
> value[i][that.attribute] = {
>                 promise: deferred.promise,
>                 temp: sid
>             };

Latter is better because promise can't be changed by consumer component. 
Its resolution is still controlled by deferred.

3) We should not call trust-resolve when there are no sids. It's 
pointless and trust-resolve requires at least one sid (`Str('sids+',`)

4) I see that you copied attribute facet preop as sid preop but omitted 
adding of facet update policy. Is there a reason for it? IMO it's better 
just to copy the whole or don't do it at all and just specified the 
facet as:
   $type: 'attribute',
   $factory: IPA.sid_facet,

This change is in fix2.diff.

Nitpicks:
a) Use `[]` instead of `new Array()`
b) add space before `for`
c) specifying sids: '' in command construction is not needed - it's set 
later

--
Petr

On 05/04/2013 08:04 AM, Alexander Bokovoy wrote:
> On Sat, 04 May 2013, 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.
>
> ... and here is rebase of install/ui/src/freeipa to Web UI refactoring
> branch, to help testing on top of Petr's changes. With this patch SID
> resolving works in new Web UI.
>
> There are probably some changes that could further be removed, I haven't
> looked into greater detail.
>
> Please note that attached patch only covers parts in
> install/ui/src/freeipa, you'd still need to add plugin changes from
> ipalib/plugins/trust.py.
>

-- 
Petr Vobornik
-------------- next part --------------
A non-text attachment was scrubbed...
Name: abbra-103-fix1.diff
Type: text/x-patch
Size: 1940 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20130506/f09fa29b/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: abbra-103-fix2.diff
Type: text/x-patch
Size: 2055 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20130506/f09fa29b/attachment-0001.bin>