[Freeipa-devel] [PATCH 0074] Make token window sizes configurable

[Martin Kosek]

On 11/07/2014 04:44 PM, Petr Vobornik wrote:
> On 7.11.2014 08:58, Martin Kosek wrote:
>> On 11/04/2014 05:17 PM, Nathaniel McCallum wrote:
>>> On Wed, 2014-10-29 at 09:34 -0400, Nathaniel McCallum wrote:
>>>> On Wed, 2014-10-29 at 12:21 +0100, Petr Viktorin wrote:
>>>>> On 10/29/2014 10:37 AM, Martin Kosek wrote:
>>>>>> On 10/28/2014 09:59 PM, Nathaniel McCallum wrote:
>>>>>>> On Thu, 2014-10-23 at 18:07 -0400, Nathaniel McCallum wrote:
>>>>>>>> This patch gives the administrator variables to control the size of
>>>>>>>> the authentication and synchronization windows for OTP tokens.
>>>>>>>>
>>>>>>>> https://fedorahosted.org/freeipa/ticket/4511
>>>>>>>>
>>>>>>>> NOTE: There is one known issue with this patch which I don't know
>>>>>>>> how to
>>>>>>>> solve. This patch changes the schema in
>>>>>>>> install/share/60ipaconfig.ldif.
>>>>>>>> On an upgrade, all of the new attributeTypes appear correctly.
>>>>>>>> However,
>>>>>>>> the modifications to the pre-existing objectClass do not show up
>>>>>>>> on the
>>>>>>>> server. What am I doing wrong?
>>>>>>>>
>>>>>>>> After modifying ipaGuiConfig manually, everything in this patch
>>>>>>>> works
>>>>>>>> just fine.
>>>>>>>
>>>>>>> This new version takes into account the new (proper) OIDs and
>>>>>>> attribute
>>>>>>> names.
>>>>>>
>>>>>> Thanks Nathaniel!
>>>>>>
>>>>>>> The above known issue still remains.
>>>>>>
>>>>>> Petr3, any idea what could have gone wrong? ObjectClass MAY list
>>>>>> extension
>>>>>> should work just fine, AFAIK.
>>>>>
>>>>> You added a blank line to the LDIF file. This is an entry separator, so
>>>>> the objectClasses after the blank line don't belong to cn=schema, so
>>>>> they aren't considered in the update.
>>>>> Without the blank line it works fine.
>>>>
>>>> Thanks for the catch!
>>>>
>>>> Here is a version without the blank line.
>>>
>>> I forgot to remove the old steps defines. This patch performs this
>>> cleanup.
>>
>> I am now wondering, is the global config object really the nest place to
>> add these OTP specific settings?
>>
>> I would prefer not to overload the object and instead:
>> - create new ipaOTPConfig objectclass
>> - add it to cn=otp,$SUFFIX
>> - create otpconfig-mod and otpconfig-show commands to follow an example
>> of dnsconfig-* and trustconfig-* commands
>>
>> IMO, this would allow more flexibility for the OTP settings and would
>> also scale better for the future updates.
> 
> +1
> 
> I will comment the patch as if ^^ would not exist because it will still be
> needed in the new plugin.
> 
> Because of ^^ I did not test, just read.
> 
> 1. Got:
> install/ui/src/freeipa/serverconfig.js(135): lint warning: extra comma is not
> recommended in array initializers
> 
> Please run:
>   jsl -nofilelisting -nosummary -nologo -conf jsl.conf
> in install/ui directory
> 
> The goal is no have no warnings and errors.
> 
> 2. new attrs should be added to 'System: Read Global Configuration' managed
> permission

+1. Though if we go with OTP config, it should be called

System: Read OTP Configuration

Martin