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
_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
