[Freeipa-devel] [PATCH] 0118 add support for subdomains

[Alexander Bokovoy]

On Thu, 03 Oct 2013, Martin Kosek wrote:
>On 10/03/2013 03:10 PM, Alexander Bokovoy wrote:
>> On Wed, 02 Oct 2013, Sumit Bose wrote:
>>>> Please note that I did not test with more than 1 subdomain, since I
>>>> do not have more ADs available.
>>>>
>>>
>>> I have done some testing as well and the patches are working as expected
>>> except the trustdomain-disable issue Tomas mentioned. But I think it
>>> would be sufficient to add a comment to the release notes and fix this
>>> with the next release to not delay this release anymore.
>>>
>>> The patches are also working for trusts which were added with older
>>> releases. So ACK from my side for the functional part.
>> New patchset is attached. I've fixed all outstanding issues and
>> implemented proper SID filtering for subdomains. In addition, I've
>> added MS-PAC cache eviction when we change blacklists from IPA side
>> and forced removal of the domain from SID blacklist if the domain is
>> being removed by trustdomain-del.
>>
>
>1) Minor issue in 0118:
>
>+        if keys[0].lower() == keys[1].lower():
>+            raise errors.ValidationError(name='trustdomain_enable',
>+                error=_("Root domain of the trust is always enabled for the
>existing trust"))
>
>The error message looks weird (double trustdomain_enable)
>
># ipa trustdomain-enable realm domain
>ipa: ERROR: invalid 'trustdomain_enable': Root domain of the trust is always
>enabled for the existing trust
>
>I would rather do something like
>
>+            raise errors.ValidationError(name='domain',
>
>
>2) trustconfig-enable and trustconfig-disable should use standard output like
>other enable/disable methods. See user-enable/user-disable for example. Current
>situation puts all the authoritative information to summary which:
>a) Does not look nice in terminal
># ipa trustdomain-disable very.long.long.long.realm very.long.long.long.domain
>----------------------------------------------------------------------------------------------------------------------------
>Domain very.long.long.long.domain of trust very.long.long.long.realm is not
>allowed to access IPA resources
>----------------------------------------------------------------------------------------------------------------------------
>b) How am I supposed to parse an information about the result if all I get is a
>text in summary? Using standard errors and output values will allow easier
>consumption of the API later (like in Web UI).
>
>I am attaching a patch (0001) how to make it consistent with other
>enable/disable commands. Example:
>
># ipa trustdomain-disable realm domain
>ipa: ERROR: This entry is already disabled
>
># ipa trustdomain-enable realm domain
>-----------------------------
>Enabled trust domain "domain"
>-----------------------------
>
>3) Let's use standard primary key for the trustdomain object. This will let us
>overcome some hacks and also let us use handle_not_found method - patch
>attached (0002).
>
>0002 also changes some ValidationError errors to standard errors, just for
>being consistent with the rest of the API.
>
>Note that in order to make primary_key=True, I had to enhance trustdomain_del
>command to manage multiple domains.
>
>
>I think these API fixes are a must, it would be very hard to amend the API
>later. If these patches are squashed to your 0118, it would be ACK from me to
>the Python side. I will let C parts and a functional test to Sumit's mighty hands.
Thanks. I've merged these changes, along with a API.txt correction. In
my tests these worked fine.

I'll resend 0118 shortly.

-- 
/ Alexander Bokovoy