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

[Martin Kosek]

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.

Martin
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Make-trustdomain-enable-and-trustdomain-disable-outp.patch
Type: text/x-patch
Size: 5356 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20131003/36218ca4/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-Use-primary_key-instead-of-custom-args.patch
Type: text/x-patch
Size: 7242 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20131003/36218ca4/attachment-0001.bin>