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

[thierry bordaz]

On 11/07/2014 05:40 PM, Nathaniel McCallum wrote:
> On Fri, 2014-11-07 at 15:02 +0100, thierry bordaz 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.
>>>
>>>
>>>
>>>
>>> _______________________________________________
>>> Freeipa-devel mailing list
>>> Freeipa-devel at redhat.com
>>> https://www.redhat.com/mailman/listinfo/freeipa-devel
>> Hello Nathaniel,
>>
>>          Few comments on the review:
>>                * in authcfg
>>                        * in string_to_types, I would prefer you set the
>>                          last element of 'map' to { NULL, 0 }.
> Why? What I have is perfectly legal ISO C and is exactly the equivalent
> of your code. In a structure initializer, undefined structure elements
> are zero'd.
>
>>                        * in entry_to_window, you may declare the
>>                          'defaults' array as 'static const'
> Fixed.
>
>>                        * Would use define for
>>                          "ipaUserAuthType","ipaHOTPAuthWindow",
>>                          "ipaTOTPAuthWindow",
>>                          "ipaHOTPSyncWindow","ipaTOTPSyncWindow" that
>>                          are present multiple times
> Fixed.
>
>>                        * suffix_to_config: cfg is set (and returned)
>>                          calling entry_to_config(entry). Now the
>>                          entry_to_config returns a structure on the
>>                          stack so it is not valid to access outside of
>>                          the entry_to_config
> Nope. We are not passing by reference but by copy. This is perfectly
> valid C.
>
>>                        *  authcfg_fini free the configs. config->cfg
>>                          should have been allocated and must be freed
>>                          (be care that configs->cfg may contains
>>                          DEFAULTS)
> Nope. The config->cfg structure is allocated and freed when config is.
> This is assignment by copy not by reference.
>
>>                        * authcfg_get_auth_types:322 should it return
>>                          'gbl' or AUTHCFG_AUTH_TYPE_PASSWORD
> If both the global and per-user auth type is unset, the default is
> AUTHCFG_AUTH_TYPE_PASSWORD. We special case this here so that we don't
> have to special case it everywhere else in the code. The code is correct
> as stands.
>
>>                        * authcfg_get_auth_window/authcfg_get_sync_window returns a window structure that is on the stack. It is not valid outside of those functions
> Nope. Structure return by copy is perfectly legal ISO C.
Hi Nathaniel,

    You are right, I am not use to structure assignment and all these
    assignments are valid.
    Sorry for the noise. The patch is fine for me.

    thanks
    theirry

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20141107/41bc3f80/attachment.htm>