[Freeipa-devel] [HELP] Regular users should not be able to add OTP tokens with custom name

[Nathaniel McCallum]

On Tue, 2014-10-14 at 10:38 +0200, Jan Cholasta wrote:
> 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.

If we're going to implement temporary workarounds like this in order to
merge this patch, I'd prefer to just wait until 4.2. Without activating
the post read control for all add operations, there is really no benefit
to this patch and added risk.

I propose that for 4.1, we use the attached patch to remove the field
from the UI. Once we have proper ACI/UUID-plugin integration in 389, we
can revisit these patches.

Nathaniel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Remove-token-ID-from-self-service-UI.patch
Type: text/x-patch
Size: 1244 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20141014/1821b730/attachment.bin>