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

[Jan Cholasta]

On 10.4.2012 16:00, Petr Viktorin wrote:
> 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.
>

I wholeheartedly agree.

Like you said above, we should either not validate --{set,add,del}attr 
or don't allow them on known attributes.

To be functionally complete, we should also add validated equivalents of 
--{add,del}attr to *-mod commands for all multivalue params (think 
--add-<param> and --del-<param> for each --<param>).

Honza

-- 
Jan Cholasta