[Freeipa-devel] [PATCH] 0130 -- create missing idranges in trust-fetch-domains

[Sumit Bose]

On Tue, Jan 14, 2014 at 04:03:06PM +0200, Alexander Bokovoy wrote:
> On Tue, 14 Jan 2014, Martin Kosek wrote:
> >On 01/14/2014 01:27 PM, Alexander Bokovoy wrote:
> >>On Tue, 14 Jan 2014, Martin Kosek wrote:
> >>>On 01/14/2014 01:02 PM, Alexander Bokovoy wrote:
> >>>>Hi,
> >>>>
> >>>>attached patch implements missing idranges when new child domains
> >>>>discovered through 'ipa trust-fetch-domains'. This functionality existed
> >>>>in 'ipa trust-add' but was not exposed in the 'ipa trust-fetch-domains'.
> >>>>
> >>>>Additionally, error message is shown in case trust name is incorrect.
> >>>>
> >>>>https://fedorahosted.org/freeipa/ticket/4104
> >>>>https://fedorahosted.org/freeipa/ticket/4111
> >>>>
> >>>
> >>>I did not test the patch, just few observations from reading it:
> >>It is generally wrong to base opinion purely on reading the code (see below
> >>why) :)
> >
> >One does not need to run the code that see the places where it may be rusty :)
> >
> >>
> >>>1) Why are you moving add_range method from trust object to the module? IMO it
> >>>could be left there, it belongs to the object. Plus, the patch won't be that
> >>>big and easier to backport. add_range can be still referred from other commands
> >>>as "self.obj.add_range", no need to move it.
> >>It was in trust_add class, not in the object. I need it in the other
> >>code and without explicit dependency on the object.
> >
> >Ok. Though I would still consider having it rather in the trust object to make
> >it easier accessible from other modules, though our API system.
> I deliberately don't want that. This is internal API for purposes of
> trust code -- do you envision any situation when anyone else might want
> to create idranges programmatically for child domains of the existing
> trust? Note that you are required to know fairly low-level details of
> the AD trusts to call it.
> 
> >
> >>
> >>>2) This part looks *very* suspicious:
> >>>
> >>>-        trust = self.api.Command.trust_show(keys[0], raw=True)['result']
> >>>+        try:
> >>>+            trust = self.api.Command.trust_show(keys[0], raw=True)['result']
> >>>+        except AssertionError:
> >>>
> >>>I do not think that AssertionError should be raised and caught in normal
> >>>operation, it should be an exceptional exception raised when the world falls
> >>>apart IMO. I.e. I would rather see some PublicError or Exception based
> >>>exception to be raised in trust_show in that case...
> >>It *is* raised and should be caught because this particular snippet is
> >>to catch situation when wrong trust domain name is passed. Previously
> >>the code simply generated server-side exception which resulted in
> >>'internal error'.
> >
> >Ok, understood. My point is, trust_show should not raise AssertException just
> >because wrong trust domain is passed. There are better means to express that
> >error - ValidationError or NotFound, based on the situation.
> Yes, this is due to the way trust.get_dn() is built. Do not commit this
> patch yet (though building packages for testing would be good), I'm
> reworking it a bit to move this logic to get_dn() -- otherwise
> LDAPRetrieve() will always issue AssertError.

I guess I already hit this while testing. trust-fetch-domains does not
work for valid forest roots anymore :-)

Btw, can you add the forest root check to trustdomain-find as well?

bye,
Sumit

> 
> -- 
> / Alexander Bokovoy
> 
> _______________________________________________
> Freeipa-devel mailing list
> Freeipa-devel at redhat.com
> https://www.redhat.com/mailman/listinfo/freeipa-devel