On 04/10/2012 07:07 PM, Martin Kosek wrote:
On Tue, 2012-04-10 at 17:03 +0200, Jan Cholasta wrote:
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.
This is indeed a mine field and we need to make a look from at the issue
from all sides before accepting a decision.
Yes.
Like you said above, we should either not validate --{set,add,del}attr
or don't allow them on known attributes.
IMHO, validating attributes we manage in the same way for both --setattr
and standard attrs is not that wrong. It is a good precaution, because
if we let an unvalidated value in, it can make even a bigger mess later
in our pre_callbacks or post_callbacks where we assume that at this
point everything is valid.
Then we should validate *exactly* the same way, including not allowing
no_update attributes to be updated.
If somebody wants to modify attributes in an uncontrolled, unvalidated
way, he is free to use ldapmodify or other tool to play with raw LDAP
values. Without our guarantee of course.
That's clear.
But if he chooses to use our --{set,add,del}attr we should at least help
him to not shoot himself to the leg and validate/normalize/encode the
value. I don't know how many users use this API, but removing a support
for all managed attributes seems as a big compatibility break to me.
Well, it was broken (see https://fedorahosted.org/freeipa/ticket/2405,
2407, 2408), so I don't think many people used it.
Anyway, what's the use case? Why would the user want to use --setattr
for validated attributes? Is our regular API lacking something?
--
Petr³
_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
