[Freeipa-devel] [RANT] --setattr validation is a minefield.

[Petr Viktorin]

I'm aware that we have backwards compatibility requirements so we have 
to stick with unfortunate decisions, but I wanted you to know what I 
think. Please tell me I'm wrong!



It is not clear what --{set,add,del}attr and friends should do. On the 
one hand they should be powerful -- presumably as powerful as 
ldapmodify. On the other hand they should be checked to ensure they 
can't be used to break the system. These requirements are contradictory. 
And in either case we're doing it wrong:
- If they should be all-powerful, we shouldn't validate them.
- If they shouldn't break the system we can just disable them for 
IPA-managed attributes. My understanding is that they were originally 
only added for attributes IPA doesn't know about. People can still use 
ldapmodify to bypass validation if they want.
- If somewhere in between, we need to clearly define what they should 
do, instead of drawing the line ad-hoc based on individual details we 
forgot about, as tickets come from QE.


I would hope people won't use --setattr for IPA-managed attributes. 
Which would however mean we won't get much community testing for all 
this extra code.


Then, there's an unfortunate detail in IPA implementation: attribute 
Params need to be cloned to method objects (Create, Update, etc.) to 
work properly (e.g. get the `attribute` flag set). If they are marked 
no_update, they don't get cloned, so they don't work properly.
Yet --setattr apparently needs to be able to update and validate 
attributes marked no_update (this ties to the confusing requirements on 
--setattr I already mentioned). This leads to doing the same work again, 
slightly differently.



tl;dr: --setattr work on IPA-managed attributes (with validation) is a 
mistake. It adds no functionality, only complexity. We don't want people 
to use it. It will cost us a lot of maintenance work to support.


Thank you for listening. A patch for the newest regression is coming up.

-- 
Petr³