Dne 14.10.2014 v 10:23 Petr Viktorin napsal(a):
On 10/14/2014 08:51 AM, Jan Cholasta wrote:
Dne 14.10.2014 v 08:37 Martin Kosek napsal(a):
On 10/13/2014 07:23 PM, Nathaniel McCallum wrote:
On Mon, 2014-10-13 at 12:39 +0200, Martin Kosek wrote:
Also, few comments to your current patch set (though the patches
themselves
will probably not land in 4.1):
Patch 0001:
- while it may work (after DS fixes), it adds PostRead for all our
commands,
not just otptoken-add. I would rather have some attribute like
"read_dn_from_postread" on LDAPObject which defaults to False to
make sure
there is no regression or performance hit with other LDAP calls.
As Honza says later in the mail, this should be an argument for
add_entry (or alternatively an additional add_entry_with_postread
method). Storing settings in data objects leads to trouble – when you
get such an object from somewhere, you never know what an operation on
it will do.
I would prefer add_entry argument rather than a new method, as that's
how find_entries does controls.
In the new code, we actually get a performance gain as the manual post
read is eliminated if the post read control is successful. Only one
issue can arise, which is when the post read control evaluates ACIs
in a
different context than a normal manual read. This problem is well known
and is trivial to fix (s/USERDN/SELFDN/).
That's my point - with such a big change, we can hit many unforeseen
issues and
we are close to deadline. I would rather like to use the PostRead
control only
in otptoken_add command. CCing Petr and Honza to chime in.
I agree it should be opt-in for now. Add a boolean argument to add_entry
as a switch.
+1
Also, I think the add_entry should not return anything; rather the
PostRead result should be merged into the added entry object. Honza,
what do you think?
It should, good point.
In the future it might be useful for add_entry to return a list of
controls, the `ctrls` in this patch.
--
Jan Cholasta
_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
