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
From 244834182add8e927171f6e9f1b4966c829b7aa4 Mon Sep 17 00:00:00 2001
From: Nathaniel McCallum <npmccal...@redhat.com>
Date: Tue, 14 Oct 2014 14:30:01 -0400
Subject: [PATCH] Remove token ID from self-service UI
Also, fix labels to properly use i18n strings for token types.
---
install/ui/src/freeipa/otptoken.js | 8 ++------
1 file changed, 2 insertions(+), 6 deletions(-)
diff --git a/install/ui/src/freeipa/otptoken.js b/install/ui/src/freeipa/otptoken.js
index d5afd25e66c58f4a3b5ce7241b3ddd0ca4f00850..2daeed9b6956921850b527e60b00ad124fb5f3d0 100644
--- a/install/ui/src/freeipa/otptoken.js
+++ b/install/ui/src/freeipa/otptoken.js
@@ -289,14 +289,10 @@ return {
name: 'type',
default_value: 'totp',
options: [
- { label: 'TOTP', value: 'totp' },
- { label: 'HOTP', value: 'hotp' }
+ { label: '@i18n:objects.otptoken.type_totp', value: 'totp' },
+ { label: '@i18n:objects.otptoken.type_hotp', value: 'hotp' }
]
},
- {
- name: 'ipatokenuniqueid',
- required: false
- },
'description'
]
}
--
2.1.0
_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
